D5245: fix: add extra field to fixed revisions to avoid creating obsolescence cycles

[hooper (Danny Hooper)]

hooper created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  The extra field prevents sequential invocations of fix from producing the same
  hash twice. Previously, this could cause problems because it would create an
  obsolescence cycle instead of the expected new successor.
  
  This change also adds an explicit check for whether a new revision should be
  committed. Until now, the code relied on memctx.commit() to quietly do nothing
  if the node already exists. Because of the new extra field, this no longer
  covers the case where we don't want to replace an unchanged node.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5245

AFFECTED FILES
  hgext/fix.py
  tests/test-fix.t

CHANGE DETAILS

diff --git a/tests/test-fix.t b/tests/test-fix.t
--- a/tests/test-fix.t
+++ b/tests/test-fix.t
@@ -1195,3 +1195,37 @@
   8
 
   $ cd ..
+
+It's possible for repeated applications of a fixer tool to create cycles in the
+generated content of a file. For example, two users with different versions of
+a code formatter might fight over the formatting when they run hg fix. In the
+absence of other changes, this means we could produce commits with the same
+hash in subsequent runs of hg fix. This is a problem unless we support
+obsolescence cycles well. We avoid this by adding an extra field to the
+successor which forces it to have a new hash. That's why this test creates
+three revisions instead of two.
+
+  $ hg init cyclictool
+  $ cd cyclictool
+
+  $ cat >> .hg/hgrc <<EOF
+  > [fix]
+  > swapletters:command = tr ab ba
+  > swapletters:pattern = foo
+  > EOF
+
+  $ echo ab > foo
+  $ hg commit -Aqm foo
+
+  $ hg fix -r 0
+  $ hg fix -r 1
+
+  $ hg cat -r 0 foo --hidden
+  ab
+  $ hg cat -r 1 foo --hidden
+  ba
+  $ hg cat -r 2 foo
+  ab
+
+  $ cd ..
+
diff --git a/hgext/fix.py b/hgext/fix.py
--- a/hgext/fix.py
+++ b/hgext/fix.py
@@ -586,6 +586,17 @@
     newp1node = replacements.get(p1ctx.node(), p1ctx.node())
     newp2node = replacements.get(p2ctx.node(), p2ctx.node())
 
+    # We don't want to create a revision that has no changes from the original,
+    # but we should if the original revision's parent has been replaced.
+    # Otherwise, we would produce an orphan that needs no actual human
+    # intervention to evolve. We can't rely on commit() to avoid creating the
+    # un-needed revision because the extra field added below produces a new hash
+    # regardless of file content changes.
+    if (not filedata and
+        p1ctx.node() not in replacements and
+        p2ctx.node() not in replacements):
+        return
+
     def filectxfn(repo, memctx, path):
         if path not in ctx:
             return None
@@ -602,15 +613,18 @@
             isexec=fctx.isexec(),
             copied=copied)
 
+    extra = ctx.extra().copy()
+    extra['fix_source'] = ctx.hex()
+
     memctx = context.memctx(
         repo,
         parents=(newp1node, newp2node),
         text=ctx.description(),
         files=set(ctx.files()) | set(filedata.keys()),
         filectxfn=filectxfn,
         user=ctx.user(),
         date=ctx.date(),
-        extra=ctx.extra(),
+        extra=extra,
         branch=ctx.branch(),
         editor=None)
     sucnode = memctx.commit()



To: hooper, #hg-reviewers
Cc: mercurial-devel