[PATCH v1] Remove no-op revisions

Index: cvs2hg-example.options
===================================================================
--- cvs2hg-example.options	(revision 5065)
+++ cvs2hg-example.options	(working copy)
@@ -439,6 +439,11 @@
 # output repository:
 ctx.retain_conflicting_attic_files = False
 
+# To exclude "no-op" revisions, where someone has used "cvs commit -f"
+# to force a new revision of a file to be committed even though the
+# file has not actually changed, change this option to True:
+ctx.remove_noop_revisions = False
+
 # CVS uses unix login names as author names whereas "hg fastimport"
 # format requires author names to be of the form "foo <bar>".  The
 # default is to set the author to "cvsauthor <cvsauthor>".
Index: test-data/empty-commits-cvsrepos/alpha,v
===================================================================
--- test-data/empty-commits-cvsrepos/alpha,v	(revision 0)
+++ test-data/empty-commits-cvsrepos/alpha,v	(revision 0)
@@ -0,0 +1,181 @@
+head	1.9;
+access;
+symbols
+	alpha_r_1_1_4_1_based_branch:1.1.4.1.0.2
+	alpha_r_1_1_4_1_tag:1.1.4.1
+	alpha_r_1_1_based_branch_with_noop_commit:1.1.0.4
+	alpha_r_1_9_based_branch:1.9.0.2
+	alpha_r_1_9_tag:1.9
+	alpha_r_1_8_based_branch:1.8.0.2
+	alpha_r_1_8_tag:1.8
+	alpha_r_1_7_based_branch:1.7.0.2
+	alpha_r_1_7_tag:1.7
+	alpha_r_1_6_based_branch:1.6.0.2
+	alpha_r_1_6_tag:1.6
+	alpha_r_1_5_based_branch:1.5.0.2
+	alpha_r_1_5_tag:1.5
+	alpha_r_1_4_based_branch:1.4.0.2
+	alpha_r_1_4_tag:1.4
+	alpha_r_1_3_based_branch:1.3.0.2
+	alpha_r_1_3_tag:1.3
+	alpha_r_1_2_based_branch:1.2.0.2
+	alpha_r_1_2_tag:1.2
+	alpha_r_1_1_based_branch:1.1.0.2
+	alpha_r_1_1_tag:1.1;
+locks; strict;
+comment	@# @;
+
+
+1.9
+date	2010.02.11.14.29.17;	author fosterj;	state Exp;
+branches;
+next	1.8;
+commitid	VpaVVAfXfwofSVmu;
+
+1.8
+date	2010.02.11.14.28.33;	author fosterj;	state Exp;
+branches;
+next	1.7;
+commitid	ZuzmP7RseeLZRVmu;
+
+1.7
+date	2010.02.11.14.28.04;	author fosterj;	state Exp;
+branches;
+next	1.6;
+commitid	cnxlVi1WOHbPRVmu;
+
+1.6
+date	2010.02.11.14.27.28;	author fosterj;	state Exp;
+branches;
+next	1.5;
+commitid	JNSyUDjAYKACRVmu;
+
+1.5
+date	2010.02.11.14.27.24;	author fosterj;	state Exp;
+branches;
+next	1.4;
+commitid	3hA1HALQEV8BRVmu;
+
+1.4
+date	2010.02.11.14.27.18;	author fosterj;	state Exp;
+branches;
+next	1.3;
+commitid	HYxr2CkVgqXyRVmu;
+
+1.3
+date	2010.02.11.14.27.05;	author fosterj;	state Exp;
+branches;
+next	1.2;
+commitid	wTsGZ5aXkLzuRVmu;
+
+1.2
+date	2010.02.11.14.26.22;	author fosterj;	state Exp;
+branches;
+next	1.1;
+commitid	xBnhGWX7qS9fRVmu;
+
+1.1
+date	2010.02.11.14.25.13;	author fosterj;	state Exp;
+branches
+	1.1.4.1;
+next	;
+commitid	m9dhvGOzYTHQQVmu;
+
+1.1.4.1
+date	2010.02.11.14.33.02;	author fosterj;	state Exp;
+branches;
+next	;
+commitid	23TPGn42NUSwTVmu;
+
+
+desc
+@@
+
+
+1.9
+log
+@NO-OP commit of alpha 1.9
+@
+text
+@Modified contents of alpha on trunk (second change)
+@
+
+
+1.8
+log
+@REAL commit of alpha 1.8
+@
+text
+@@
+
+
+1.7
+log
+@NO-OP commit of alpha 1.7
+@
+text
+@d1 1
+a1 1
+Modified contents of alpha on trunk (first change)
+@
+
+
+1.6
+log
+@NO-OP commit of alpha 1.6
+@
+text
+@@
+
+
+1.5
+log
+@NO-OP commit of alpha 1.5
+@
+text
+@@
+
+
+1.4
+log
+@NO-OP commit of alpha 1.4
+@
+text
+@@
+
+
+1.3
+log
+@REAL commit of alpha 1.3
+@
+text
+@@
+
+
+1.2
+log
+@NO-OP commit of alpha 1.2
+@
+text
+@d1 1
+a1 1
+Initial contents of alpha
+@
+
+
+1.1
+log
+@REAL commit of alpha 1.1
+@
+text
+@@
+
+
+1.1.4.1
+log
+@NO-OP commit of alpha 1.1.4.1
+@
+text
+@@
+
+
Index: cvs2svn-example.options
===================================================================
--- cvs2svn-example.options	(revision 5065)
+++ cvs2svn-example.options	(working copy)
@@ -547,6 +547,11 @@
 # the file will be left in an SVN subdirectory called "Attic":
 ctx.retain_conflicting_attic_files = False
 
+# To exclude "no-op" revisions, where someone has used "cvs commit -f"
+# to force a new revision of a file to be committed even though the
+# file has not actually changed, change this option to True:
+ctx.remove_noop_revisions = False
+
 # Now use stanzas like the following to define CVS projects that
 # should be converted.  The arguments are:
 #
Index: cvs2svn_lib/run_options.py
===================================================================
--- cvs2svn_lib/run_options.py	(revision 5065)
+++ cvs2svn_lib/run_options.py	(working copy)
@@ -318,6 +318,18 @@
             'Convert only trunk commits, not tags nor branches.'
             ),
         ))
+    group.add_option(ContextOption(
+        '--remove-noop-revisions',
+        action='store_true',
+        help=(
+            'if "cvs commit -f" has been used to commit a revision '
+            'to a file where there are no changes, ignore it.'
+            ),
+        man_help=(
+            'if "cvs commit -f" has been used to commit a revision '
+            'to a file where there are no changes, ignore it.'
+            ),
+        ))
     group.add_option(ManOption(
         '--encoding', type='string',
         action='callback', callback=self.callback_encoding,
Index: cvs2svn_lib/collect_data.py
===================================================================
--- cvs2svn_lib/collect_data.py	(revision 5065)
+++ cvs2svn_lib/collect_data.py	(working copy)
@@ -1120,14 +1120,23 @@
     # Remove initial branch deletes that are not needed:
     cvs_file_items.remove_initial_branch_deletes(self.metadata_db)
 
+    cvs_file_items.check_link_consistency()
+
+    # Remove (most) "no-op" revisions, where "cvs commit -f" has been used
+    # to check in a change that does nothing.
+    # NOTE: This has to be done after imported_remove_1_1(), else
+    # it will remove the 1.1.1.1 revision.
+    if Ctx().remove_noop_revisions:
+      cvs_file_items.remove_noop_revisions()
+      cvs_file_items.check_link_consistency()
+
     # If this is a --trunk-only conversion, discard all branches and
     # tags, then draft any non-trunk default branch revisions to
     # trunk:
     if Ctx().trunk_only:
       cvs_file_items.exclude_non_trunk()
+      cvs_file_items.check_link_consistency()
 
-    cvs_file_items.check_link_consistency()
-
     self.add_cvs_file_items(cvs_file_items)
     self.symbol_stats.register(cvs_file_items)
 
Index: cvs2svn_lib/context.py
===================================================================
--- cvs2svn_lib/context.py	(revision 5065)
+++ cvs2svn_lib/context.py	(working copy)
@@ -47,6 +47,7 @@
     self.revision_reader = None
     self.svnadmin_executable = config.SVNADMIN_EXECUTABLE
     self.trunk_only = False
+    self.remove_noop_revisions = False
     self.include_empty_directories = False
     self.prune = True
     self.cvs_author_decoder = CVSTextDecoder(['ascii'])
Index: cvs2svn_lib/cvs_file_items.py
===================================================================
--- cvs2svn_lib/cvs_file_items.py	(revision 5065)
+++ cvs2svn_lib/cvs_file_items.py	(working copy)
@@ -21,6 +21,7 @@
 
 from cvs2svn_lib.common import InternalError
 from cvs2svn_lib.common import FatalError
+from cvs2svn_lib.common import is_trunk_revision
 from cvs2svn_lib.context import Ctx
 from cvs2svn_lib.log import Log
 from cvs2svn_lib.symbol import Trunk
@@ -29,6 +30,7 @@
 from cvs2svn_lib.symbol import ExcludedSymbol
 from cvs2svn_lib.cvs_item import CVSRevision
 from cvs2svn_lib.cvs_item import CVSRevisionModification
+from cvs2svn_lib.cvs_item import CVSRevisionChange
 from cvs2svn_lib.cvs_item import CVSRevisionAbsent
 from cvs2svn_lib.cvs_item import CVSRevisionNoop
 from cvs2svn_lib.cvs_item import CVSSymbol
@@ -593,6 +595,139 @@
         cvs_rev2 = self[id]
         cvs_rev2.prev_id = cvs_rev.id
 
+  def remove_noop_revisions(self):
+    """If possible, remove any revisions which do not change anything.
+
+    These can be created by committing an unchanged file with "cvs commit -f".
+
+    We do not detect and remove in the case where a vendor branch exists
+    (with commits) and then rev 1.2 is created via a "cvs commit -f" with
+    no changes.  This is because CVS always records the diff between rev
+    1.1 (which is always identical to rev 1.1.1.1) and rev 1.2, not between
+    the last vendor branch rev (e.g. rev 1.1.1.3) and rev 1.2.
+
+    NOTE: This should be called after imported_remove_1_1(), else it will
+    remove the 1.1.1.1 revision.
+    """
+
+    # 2-step "find then delete" so we don't change the dictionary whilst
+    # iterating over it.
+    revision_ids_to_delete = []
+    for cvs_rev in self._cvs_items.itervalues():
+      if isinstance(cvs_rev, CVSRevision) and not cvs_rev.deltatext_exists:
+        if is_trunk_revision(cvs_rev.rev):
+          # on trunk, the diffs are backward so deltatext_exists==False means
+          # that the _next_ rev is empty.  So check if the next rev exists
+          # and is a change.  In that case, we delete the _next_ rev.
+          if cvs_rev.next_id is not None:
+            empty_rev = self._cvs_items[cvs_rev.next_id]
+            if (isinstance(empty_rev, CVSRevisionChange)
+                and is_trunk_revision(empty_rev.rev)):
+              revision_ids_to_delete.append(cvs_rev.next_id)
+        else:
+          # on a branch, look for an empty change rev.
+          # (A CVSRevisionChange means that this isn't an add or delete, so
+          # the deltatext should actually be the delta.  Empty delta means
+          # no-op commit).
+          if isinstance(cvs_rev, CVSRevisionChange):
+            assert cvs_rev.prev_id is not None
+            parent_rev = self._cvs_items[cvs_rev.prev_id]
+            assert isinstance(parent_rev, CVSRevision)
+            revision_ids_to_delete.append(cvs_rev.id)
+
+    for rev_id in revision_ids_to_delete:
+      assert rev_id in self._cvs_items
+      empty_rev = self._cvs_items[rev_id]
+      assert isinstance(empty_rev, CVSRevisionChange)
+      Log().debug('Removing empty revision %s' % (empty_rev,))
+      print 'Removing empty revision %s' % (empty_rev,)
+
+      # Figure out the parent revision, which is where tags and branches
+      # will be moved to.
+      assert empty_rev.id not in self.root_ids
+      assert empty_rev.prev_id is not None
+      assert empty_rev.prev_id in self._cvs_items
+      parent_rev = self._cvs_items[empty_rev.prev_id]
+      assert parent_rev is not empty_rev
+
+      Log().debug('Removing empty revision %s' % (empty_rev,))
+
+      if empty_rev.ntdbr:
+        # Nothing special to do here; the parent of a NTDBR rev is always a
+        # NTDBR rev itself.
+        assert parent_rev.ntdbr
+
+      # Move any tags
+      for id in empty_rev.tag_ids:
+        cvs_tag = self._cvs_items[id]
+        assert cvs_tag.source_lod == empty_rev.lod
+        assert cvs_tag.source_id == empty_rev.id
+        cvs_tag.source_lod = parent_rev.lod
+        cvs_tag.source_id = parent_rev.id
+      parent_rev.tag_ids.extend(empty_rev.tag_ids)
+      empty_rev.tag_ids = []
+
+      # Move any branches
+      for id in empty_rev.branch_ids:
+        cvs_branch = self._cvs_items[id]
+        assert cvs_branch.source_lod == empty_rev.lod
+        assert cvs_branch.source_id == empty_rev.id
+        cvs_branch.source_lod = parent_rev.lod
+        cvs_branch.source_id = parent_rev.id
+      # TODO: Should these go to start or end of parent_rev.branch_ids?
+      parent_rev.branch_ids[0:0] = empty_rev.branch_ids
+      empty_rev.branch_ids = []
+
+      for id in empty_rev.branch_commit_ids:
+        assert self._cvs_items[id].prev_id == empty_rev.id
+        self._cvs_items[id].prev_id = parent_rev.id
+      # TODO: Should these go to start or end of parent_rev.branch_commit_ids?
+      parent_rev.branch_commit_ids[0:0] = empty_rev.branch_commit_ids
+      empty_rev.branch_commit_ids = []
+
+      # The first revision on a branch has a special flag.  So if the
+      # empty rev has that flag, move it to the next revision on the
+      # same branch.  It the empty revision is the only commit on the
+      # branch, then the flag will be dropped.
+      if empty_rev.first_on_branch_id is not None:
+        if empty_rev.next_id is not None:
+          self._cvs_items[empty_rev.next_id].first_on_branch_id = \
+                  empty_rev.first_on_branch_id
+
+      # Fixup next/previous links
+      if empty_rev.next_id is not None:
+        next_rev = self._cvs_items[empty_rev.next_id]
+        assert next_rev.prev_id == empty_rev.id
+        next_rev.prev_id = empty_rev.prev_id
+
+      if parent_rev.next_id == empty_rev.id:
+        parent_rev.next_id = empty_rev.next_id
+      else:
+        # Parent is on a different branch.  So we have to adjust the 1st
+        # commit on branch - it's not empty_rev any more.
+        branch = None
+        for possible_branch_id in parent_rev.branch_ids:
+          possible_branch = self._cvs_items[possible_branch_id]
+          if possible_branch.next_id == empty_rev.id:
+            assert branch is None
+            branch = possible_branch
+        assert branch is not None
+        assert branch.next_id == empty_rev.id
+        branch.next_id = empty_rev.next_id
+
+        index = None
+        for n in range(len(parent_rev.branch_commit_ids)):
+          if parent_rev.branch_commit_ids[n] == empty_rev.id:
+            assert index is None
+            index = n
+        assert index is not None
+        if empty_rev.next_id is None:
+          del parent_rev.branch_commit_ids[index]
+        else:
+          parent_rev.branch_commit_ids[index] = empty_rev.next_id
+
+      del self._cvs_items[empty_rev.id]
+
   def _is_unneeded_initial_trunk_delete(self, cvs_item, metadata_db):
     if not isinstance(cvs_item, CVSRevisionNoop):
       # This rule can only be applied to dead revisions.
Index: run-tests.py
===================================================================
--- run-tests.py	(revision 5065)
+++ run-tests.py	(working copy)
@@ -3825,7 +3825,88 @@
     ('/%(branches)s/BRANCH (from /%(trunk)s:8)', 'A'),
     ))
 
+@Cvs2SvnTestFunction
+def remove_noop_revisions():
+  "test --remove-noop-revisions"
 
+  conv = ensure_conversion(
+      'empty-commits', args=['--remove-noop-revisions'],
+      )
+
+  TAG_PREFIX = "This commit was manufactured by cvs2svn to create tag '"
+  TAG_SUFFIX = "'."
+  BRANCH_PREFIX = "This commit was manufactured by cvs2svn to create branch\n'"
+  BRANCH_SUFFIX = "'."
+  INITIAL_MSG = 'Standard project directories initialized by cvs2svn.'
+
+  # Get the set of commit messages, and split it into tags, branches, and
+  # other.
+  messages = set(one_log.msg.strip(' \t\r\n')
+                 for one_log in conv.logs.itervalues())
+  if INITIAL_MSG not in messages:
+    raise Failure()
+  messages.remove(INITIAL_MSG)
+  tag_messages = frozenset(msg for msg in messages
+            if msg.startswith(TAG_PREFIX) and msg.endswith(TAG_SUFFIX))
+  messages -= tag_messages
+  tags = set(msg[len(TAG_PREFIX):-len(TAG_SUFFIX)] for msg in tag_messages)
+  branch_messages = frozenset(msg for msg in messages
+            if msg.startswith(BRANCH_PREFIX) and msg.endswith(BRANCH_SUFFIX))
+  messages -= branch_messages
+  branches = set(msg[len(BRANCH_PREFIX):-len(BRANCH_SUFFIX)]
+                 for msg in branch_messages)
+
+  # For the remaining messages, split them into no-op commits and real
+  # commits.  The test CVS repository indicates this with keywords in
+  # the log messages.
+  noop_messages = frozenset(msg for msg in messages if 'NO-OP' in msg)
+  messages -= noop_messages
+  real_messages = frozenset(msg for msg in messages if 'REAL' in msg)
+  messages -= real_messages
+
+  # All the log messages should have been classified.
+  if messages:
+      raise Failure('Unexpected log message')
+
+  # Check only the 3 real commits were kept, all no-op commits were
+  # thrown away, and all 11 branches and 10 tags were kept.
+  EXPECT_REAL_MESSAGES = frozenset([
+      'REAL commit of alpha 1.1',
+      'REAL commit of alpha 1.3',
+      'REAL commit of alpha 1.8'])
+  EXPECT_TAGS = frozenset([
+      'alpha_r_1_1_4_1_tag',
+      'alpha_r_1_1_tag',
+      'alpha_r_1_2_tag',
+      'alpha_r_1_3_tag',
+      'alpha_r_1_4_tag',
+      'alpha_r_1_5_tag',
+      'alpha_r_1_6_tag',
+      'alpha_r_1_7_tag',
+      'alpha_r_1_8_tag',
+      'alpha_r_1_9_tag'])
+  EXPECT_BRANCHES = frozenset([
+      'alpha_r_1_1_4_1_based_branch',
+      'alpha_r_1_1_based_branch_with_noop_commit',
+      'alpha_r_1_1_based_branch',
+      'alpha_r_1_2_based_branch',
+      'alpha_r_1_3_based_branch',
+      'alpha_r_1_4_based_branch',
+      'alpha_r_1_5_based_branch',
+      'alpha_r_1_6_based_branch',
+      'alpha_r_1_7_based_branch',
+      'alpha_r_1_8_based_branch',
+      'alpha_r_1_9_based_branch'])
+
+  if real_messages != EXPECT_REAL_MESSAGES:
+      raise Failure('Real commits were missing or added')
+  if noop_messages:
+      raise Failure('Some no-op commits were not removed')
+  if tags != EXPECT_TAGS:
+      raise Failure('Missing or added tags')
+  if branches != EXPECT_BRANCHES:
+      raise Failure('Missing or added branches')
+
 ########################################################################
 # Run the tests
 
@@ -4030,6 +4111,7 @@
     include_empty_directories,
 # 170:
     include_empty_directories_no_prune,
+    remove_noop_revisions,
     ]
 
 if __name__ == '__main__':
Index: cvs2git-example.options
===================================================================
--- cvs2git-example.options	(revision 5065)
+++ cvs2git-example.options	(working copy)
@@ -471,6 +471,11 @@
 # output repository:
 ctx.retain_conflicting_attic_files = False
 
+# To exclude "no-op" revisions, where someone has used "cvs commit -f"
+# to force a new revision of a file to be committed even though the
+# file has not actually changed, change this option to True:
+ctx.remove_noop_revisions = False
+
 # CVS uses unix login names as author names whereas git requires
 # author names to be of the form "foo <bar>".  The default is to set
 # the git author to "cvsauthor <cvsauthor>".  author_transforms can be
Index: cvs2bzr-example.options
===================================================================
--- cvs2bzr-example.options	(revision 5065)
+++ cvs2bzr-example.options	(working copy)
@@ -443,6 +443,11 @@
 # output repository:
 ctx.retain_conflicting_attic_files = False
 
+# To exclude "no-op" revisions, where someone has used "cvs commit -f"
+# to force a new revision of a file to be committed even though the
+# file has not actually changed, change this option to True:
+ctx.remove_noop_revisions = False
+
 # CVS uses unix login names as author names whereas Bazaar requires
 # author names to be of the form "foo <bar>".  The default is to set
 # the Bazaar author to "cvsauthor <cvsauthor>".  author_transforms can be