fbamend: add the ability for amends to automatically restack if there won't be conflicts

Summary:
This uses the --noconflict flag added in the previous diff to make auto-restacking, as long as there aren't conflicts or a mergedriver run, trivial.

It creates the `amend.autorestack` flag with four options:

- `never`: never attempt to restack.
- `only-trivial`: restack only if the manifest is unchanged (the old behavior). We can remove this after the rollout.
- `no-conflict`: restack if there won't be merge conflicts.
- `always`: always attempt to restack.

Reviewed By: DurhamG

Differential Revision: D8721726

fbshipit-source-id: 620c08f6082033272dde35cb27b199615f16a787

hgext/fbamend/__init__.py

@@ -36,10 +36,11 @@ To automatically update the commit date, enable the following config option::
     [fbamend]
     date = implicitupdate
 
-To stop fbamend from automatically rebasing stacked changes::
+Commits are restacked automatically on amend, if doing so doesn't create
+conflicts. To never automatically restack::
 
-    [commands]
+    [amend]
-    amend.autorebase = false
+    autorestack = none
 
 Note that if --date is specified on the command line, it takes precedence.
 
@@ -111,6 +112,30 @@ amendopts = [
     ("", "to", "", _("amend to a specific commit in the current stack")),
 ]
 
+# Never restack commits on amend.
+RESTACK_NEVER = "never"
+
+# Restack commits on amend only if they chage manifest, and don't change the
+# commit manifest.
+RESTACK_ONLY_TRIVIAL = "only-trivial"
+
+# Restack commits on amend only if doing so won't create merge conflicts.
+RESTACK_NO_CONFLICT = "no-conflict"
+
+# Always attempt to restack commits on amend, even if doing so will leave the
+# user in a conflicted state.
+RESTACK_ALWAYS = "always"
+
+# Possible restack values for `amend.autorestack`.
+RESTACK_VALUES = [
+    RESTACK_NEVER,
+    RESTACK_ONLY_TRIVIAL,
+    RESTACK_NO_CONFLICT,
+    RESTACK_ALWAYS,
+]
+
+RESTACK_DEFAULT = RESTACK_ONLY_TRIVIAL
+
 
 @hint("strip-hide")
 def hinthide():
@@ -328,24 +353,61 @@ def amend(ui, repo, *pats, **opts):
             ui.status(_("nothing changed\n"))
             return 1
 
-        if haschildren and rebase is None and not _histediting(repo):
+        conf = ui.config("amend", "autorestack", RESTACK_DEFAULT)
-            # If the user has chosen the default behaviour for the
+        noconflict = None
-            # rebase, then see if we can apply any heuristics. This
-            # will not performed if a histedit is in flight.
 
-            newcommit = repo[node]
+        # RESTACK_NO_CONFLICT requires IMM.
-            # If the rebase did not change the manifest and the
+        if conf == RESTACK_NO_CONFLICT and not ui.config(
-            # working copy is clean, force the children to be
+            "rebase", "experimental.inmemory", False
-            # restacked.
+        ):
-            if (
+            conf = RESTACK_DEFAULT
-                old.manifestnode() == newcommit.manifestnode()
+
-                and not repo[None].dirty()
+        # If they explicitly disabled the old behavior, disable the new behavior
-            ):
+        # too, for now.
-                if ui.configbool("commands", "amend.autorebase"):
+        # internal config: commands.amend.autorebase
-                    hintutil.trigger("amend-autorebase")
+        if ui.configbool("commands", "amend.autorebase") is False:
-                    rebase = True
+            # In the future we'll add a nag message here.
-                else:
+            conf = RESTACK_NEVER
+
+        if conf not in RESTACK_VALUES:
+            ui.warn(
+                _('invalid amend.autorestack config of "%s"; falling back to %s\n')
+                % (conf, RESTACK_DEFAULT)
+            )
+            conf = RESTACK_DEFAULT
+
+        if haschildren and rebase is None and not _histediting(repo):
+            if conf == RESTACK_ALWAYS:
+                rebase = True
+            elif conf == RESTACK_NO_CONFLICT:
+                if repo[None].dirty():
+                    # For now, only restack if the WC is clean (t31742174).
+                    ui.status(_("not restacking because working copy is dirty\n"))
                     rebase = False
+                else:
+                    # internal config: amend.autorestackmsg
+                    msg = ui.config(
+                        "amend",
+                        "autorestackmsg",
+                        _("restacking children automatically (unless they conflict)"),
+                    )
+                    if msg:
+                        ui.status("%s\n" % msg)
+                    rebase = True
+                    noconflict = True
+            elif conf == RESTACK_ONLY_TRIVIAL:
+                newcommit = repo[node]
+                # If the rebase did not change the manifest and the
+                # working copy is clean, force the children to be
+                # restacked.
+                rebase = (
+                    old.manifestnode() == newcommit.manifestnode()
+                    and not repo[None].dirty()
+                )
+                if rebase:
+                    hintutil.trigger("amend-autorebase")
+            else:
+                rebase = False
 
         if haschildren and not rebase and not _histediting(repo):
             hintutil.trigger("amend-restack", old.node())
@@ -360,12 +422,15 @@ def amend(ui, repo, *pats, **opts):
         tr.close()
 
         if rebase and haschildren:
-            fixupamend(ui, repo)
+            noconflictmsg = _(
+                "restacking would create conflicts (%s in %s), so you must run it manually\n(run `hg restack` manually to restack this commit's children)"
+            )
+            fixupamend(ui, repo, noconflict=noconflict, noconflictmsg=noconflictmsg)
     finally:
         lockmod.release(wlock, lock, tr)
 
 
-def fixupamend(ui, repo):
+def fixupamend(ui, repo, noconflict=None, noconflictmsg=None):
     """rebases any children found on the preamend changset and strips the
     preamend changset
     """
@@ -378,7 +443,9 @@ def fixupamend(ui, repo):
         current = repo["."]
 
         # Use obsolescence information to fix up the amend.
-        common.restackonce(ui, repo, current.rev())
+        common.restackonce(
+            ui, repo, current.rev(), noconflict=noconflict, noconflictmsg=noconflictmsg
+        )
     finally:
         lockmod.release(wlock, lock, tr)
 

hgext/fbamend/common.py

@@ -29,7 +29,15 @@ def getchildrelationships(repo, revs):
     return children
 
 
-def restackonce(ui, repo, rev, rebaseopts=None, childrenonly=False):
+def restackonce(
+    ui,
+    repo,
+    rev,
+    rebaseopts=None,
+    childrenonly=False,
+    noconflict=None,
+    noconflictmsg=None,
+):
     """Rebase all descendants of precursors of rev onto rev, thereby
        stabilzing any non-obsolete descendants of those precursors.
        Takes in an optional dict of options for the rebase command.
@@ -53,6 +61,7 @@ def restackonce(ui, repo, rev, rebaseopts=None, childrenonly=False):
         rebaseopts = {}
     rebaseopts["rev"] = descendants
     rebaseopts["dest"] = rev
+    rebaseopts["noconflict"] = noconflict
 
     # We need to ensure that the 'operation' field in the obsmarker metadata
     # is always set to 'rebase', regardless of the current command so that
@@ -69,6 +78,9 @@ def restackonce(ui, repo, rev, rebaseopts=None, childrenonly=False):
             (tweakdefaults.globaldata, tweakdefaults.createmarkersoperation)
         ] = "rebase"
 
+    if noconflictmsg:
+        overrides[("rebase", "noconflictmsg")] = noconflictmsg
+
     # Perform rebase.
     with repo.ui.configoverride(overrides, "restack"):
         rebase.rebase(ui, repo, **rebaseopts)

hgext/rebase.py

@@ -544,9 +544,13 @@ class rebaseruntime(object):
                         kindstr = _("artifact rebuild required")
 
                     if self.opts.get("noconflict"):
-                        raise error.AbortMergeToolError(
+                        # internal config: rebase.noconflictmsg
-                            "%s (in %s) and --noconflict passed" % (kindstr, pathstr)
+                        msg = ui.config(
+                            "rebase",
+                            "noconflictmsg",
+                            _("%s (in %s) and --noconflict passed; exiting"),
                         )
+                        raise error.AbortMergeToolError(msg % (kindstr, pathstr))
                     elif cmdutil.uncommittedchanges(repo):
                         raise error.UncommitedChangesAbort(
                             _(
@@ -1001,7 +1005,7 @@ def rebase(ui, repo, templ=None, **opts):
             with ui.configoverride(overrides):
                 return _origrebase(ui, repo, rbsrt, **opts)
         except error.AbortMergeToolError as e:
-            ui.status(_("%s; exiting.\n") % e)
+            ui.status(_("%s\n") % e)
             clearstatus(repo)
             mergemod.mergestate.clean(repo)
             if repo.currenttransaction():

tests/test-fb-hgext-fbamend-restack-auto.t

@@ -1,5 +1,7 @@
   $ . helpers-usechg.sh
   $ enable fbamend inhibit rebase
+  $ setconfig rebase.experimental.inmemory=True
+  $ setconfig rebase.singletransaction=True
   $ setconfig experimental.evolution.allowdivergence=True
   $ setconfig experimental.evolution="createmarkers, allowunstable"
   $ mkcommit() {
@@ -11,8 +13,42 @@
   >   hg log --graph -T "{rev} {desc|firstline}" | sed \$d
   > }
 
-Test auto-restack heuristics - no changes to manifest and clean working directory
+Test invalid value for amend.autorestack
   $ newrepo
+  $ setconfig amend.autorestack=test
+  $ hg debugdrawdag<<'EOS'
+  > C              # C/file = 1\n2\n3\n4\n
+  > |              # B/file = 1\n2\n
+  > B
+  > |
+  > A
+  > EOS
+  $ hg update B -q
+  $ hg amend -m "new message"
+  invalid amend.autorestack config of "test"; falling back to only-trivial
+  rebasing 2:ca039b450ae0 "C" (C)
+  hint[amend-autorebase]: descendants have been auto-rebased because no merge conflict could have happened - use --no-rebase or set commands.amend.autorebase=False to disable auto rebase
+  hint[hint-ack]: use 'hg hint --ack amend-autorebase' to silence these hints
+
+If they disabled amend.autorestack, disable the new behavior (for now, during rollout)
+  $ newrepo
+  $ setconfig commands.amend.autorebase=False
+  $ setconfig amend.autorestack=always
+  $ hg debugdrawdag<<'EOS'
+  > C              # C/file = 1\n2\n3\n4\n
+  > |              # B/file = 1\n2\n
+  > B
+  > |
+  > A
+  > EOS
+  $ hg update B -q
+  $ hg amend -m "new message"
+  hint[amend-restack]: descendants of fe14e2b67b65 are left behind - use 'hg restack' to rebase them
+  hint[hint-ack]: use 'hg hint --ack amend-restack' to silence these hints
+
+amend.autorestack=only-trivial, and simple changes (expect restack)
+  $ newrepo
+  $ setconfig amend.autorestack=only-trivial
   $ hg debugdrawdag<<'EOS'
   > C
   > |
@@ -34,8 +70,9 @@ Test auto-restack heuristics - no changes to manifest and clean working director
   hint[amend-autorebase]: descendants have been auto-rebased because no merge conflict could have happened - use --no-rebase or set commands.amend.autorebase=False to disable auto rebase
   hint[hint-ack]: use 'hg hint --ack amend-autorebase' to silence these hints
 
-Test commands.amend.autorebase=False flag - no changes to manifest and clean working directory
+amend.autorestack=never
   $ newrepo
+  $ setconfig amend.autorestack=never
   $ hg debugdrawdag<<'EOS'
   > C
   > |
@@ -44,18 +81,19 @@ Test commands.amend.autorebase=False flag - no changes to manifest and clean wor
   > A
   > EOS
   $ hg update B -q
-  $ hg amend --config commands.amend.autorebase=False -m 'Unchanged manifest for B'
+  $ hg amend -m 'Unchanged manifest for B'
   hint[amend-restack]: descendants of 112478962961 are left behind - use 'hg restack' to rebase them
   hint[hint-ack]: use 'hg hint --ack amend-restack' to silence these hints
   $ hg prev
   0 files updated, 0 files merged, 1 files removed, 0 files unresolved
   [426bad] A
-  $ hg amend --config commands.amend.autorebase=False -m 'Unchanged manifest for A'
+  $ hg amend -m 'Unchanged manifest for A'
   hint[amend-restack]: descendants of 426bada5c675 are left behind - use 'hg restack' to rebase them
   hint[hint-ack]: use 'hg hint --ack amend-restack' to silence these hints
 
-Test auto-restack heuristics - manifest changes
+amend.autorestack=only-trivial, and manifest changes (expect no restack)
   $ newrepo
+  $ setconfig amend.autorestack=only-trivial
   $ hg debugdrawdag<<'EOS'
   > C
   > |
@@ -69,8 +107,9 @@ Test auto-restack heuristics - manifest changes
   hint[amend-restack]: descendants of 112478962961 are left behind - use 'hg restack' to rebase them
   hint[hint-ack]: use 'hg hint --ack amend-restack' to silence these hints
 
-Test auto-restack heuristics - no committed changes to manifest but dirty working directory
+amend.autorestack=only-trivial, and dirty working copy (expect no restack)
   $ newrepo
+  $ setconfig amend.autorestack=only-trivial
   $ hg debugdrawdag<<'EOS'
   > C
   > |
@@ -84,8 +123,9 @@ Test auto-restack heuristics - no committed changes to manifest but dirty workin
   hint[amend-restack]: descendants of 112478962961 are left behind - use 'hg restack' to rebase them
   hint[hint-ack]: use 'hg hint --ack amend-restack' to silence these hints
 
-Test auto-restack heuristics - no changes to manifest but no children
+amend.autorestack=only-trivial, and no manifest changes, but no children (expect no restack)
   $ newrepo
+  $ setconfig amend.autorestack=only-trivial
   $ hg debugdrawdag<<'EOS'
   > B
   > |
@@ -93,3 +133,195 @@ Test auto-restack heuristics - no changes to manifest but no children
   > EOS
   $ hg update B -q
   $ hg amend -m 'Unchanged manifest for B'
+
+amend.autorestack=no-conflict, and mergeable changes (expect restack)
+  $ newrepo
+  $ setconfig amend.autorestack=no-conflict
+  $ setconfig amend.autorestackmsg="custom autorestack message"
+  $ hg debugdrawdag<<'EOS'
+  > C              # C/file = 1\n2\n3\n4\n
+  > |              # B/file = 1\n2\n
+  > B
+  > |
+  > A
+  > EOS
+  $ hg update B -q
+  $ $TESTDIR/seq.py 0 2 > file
+  $ hg amend
+  custom autorestack message
+  rebasing 2:ca039b450ae0 "C" (C)
+  merging file
+  $ showgraph
+  o  4 C
+  |
+  @  3 B
+  |
+  | x  2 C
+  | |
+  | x  1 B
+  |/
+  o  0 A
+  $ cat file
+  0
+  1
+  2
+
+amend.autorestack=no-conflict, and mergeable changes, but dirty WC (expect no restack)
+  $ newrepo
+  $ setconfig amend.autorestack=no-conflict
+  $ hg debugdrawdag<<'EOS'
+  > C              # C/file = 1\n2\n3\n4\n
+  > |              # B/file = 1\n2\n
+  > B              # A/other = i don't matter
+  > |
+  > A
+  > EOS
+  $ hg update B -q
+  $ echo "new content" > other
+  $ $TESTDIR/seq.py 0 2 > file
+  $ cat <<EOS | hg amend -i --config ui.interactive=1
+  > y
+  > y
+  > n
+  > EOS
+  diff --git a/file b/file
+  1 hunks, 1 lines changed
+  examine changes to 'file'? [Ynesfdaq?] y
+  
+  @@ -1,2 +1,3 @@
+  +0
+   1
+   2
+  record change 1/2 to 'file'? [Ynesfdaq?] y
+  
+  diff --git a/other b/other
+  1 hunks, 1 lines changed
+  examine changes to 'other'? [Ynesfdaq?] n
+  
+  not restacking because working copy is dirty
+  hint[amend-restack]: descendants of bf943f2ff2de are left behind - use 'hg restack' to rebase them
+  hint[hint-ack]: use 'hg hint --ack amend-restack' to silence these hints
+
+
+
+
+amend.autorestack=no-conflict, and conflicting changes (expect cancelled restack)
+  $ newrepo
+  $ setconfig amend.autorestack=no-conflict
+  $ hg debugdrawdag<<'EOS'
+  > D
+  > |
+  > C              # D/file = 1\n2\n3\n4\n
+  > |              # B/file = 1\n2\n
+  > B
+  > |
+  > A
+  > EOS
+  $ hg update B -q
+  $ echo 'unmergeable!' > file
+  $ hg amend
+  restacking children automatically (unless they conflict)
+  rebasing 2:b6c0d35dc9e9 "C" (C)
+  rebasing 3:02cc3cc1d010 "D" (D)
+  merging file
+  transaction abort!
+  rollback completed
+  restacking would create conflicts (hit merge conflicts in file), so you must run it manually
+  (run `hg restack` manually to restack this commit's children)
+  $ showgraph
+  @  4 B
+  |
+  | o  3 D
+  | |
+  | o  2 C
+  | |
+  | x  1 B
+  |/
+  o  0 A
+  $ cat file
+  unmergeable!
+
+amend.autorestack=always, and conflicting changes (expect restack)
+  $ newrepo
+  $ setconfig amend.autorestack=always
+  $ hg debugdrawdag<<'EOS'
+  > D
+  > |
+  > C              # D/file = 1\n2\n3\n4\n
+  > |              # B/file = 1\n2\n
+  > B
+  > |
+  > A
+  > EOS
+  $ hg update B -q
+  $ echo 'unmergeable!' > file
+  $ hg amend
+  rebasing 2:b6c0d35dc9e9 "C" (C)
+  rebasing 3:02cc3cc1d010 "D" (D)
+  merging file
+  hit merge conflicts (in file); switching to on-disk merge
+  rebasing 3:02cc3cc1d010 "D" (D)
+  merging file
+  warning: conflicts while merging file! (edit, then use 'hg resolve --mark')
+  unresolved conflicts (see hg resolve, then hg rebase --continue)
+  [1]
+  $ hg rebase --abort
+  saved backup bundle to $TESTTMP/repo11/.hg/strip-backup/7655d36150ed-96938de3-backup.hg
+  rebase aborted
+  $ cat file
+  unmergeable!
+  $ showgraph
+  @  4 B
+  |
+  | o  3 D
+  | |
+  | o  2 C
+  | |
+  | x  1 B
+  |/
+  o  0 A
+
+Test rebasing children with obsolete children themselves needing a restack.
+  $ newrepo
+  $ setconfig amend.autorestack=no-conflict
+  $ hg debugdrawdag<<'EOS'
+  > D
+  > |
+  > C C2  # amend: C -> C2
+  > |/
+  > B
+  > |
+  > A     # <-- then amend this
+  > |
+  > Z
+  > EOS
+  $ hg update A -q
+  $ echo "new value" > A
+  $ hg amend
+  restacking children automatically (unless they conflict)
+  rebasing 2:917a077edb8d "B" (B)
+  rebasing 4:ff9eba5e2480 "C2" (C2)
+  rebasing 5:01f26f1a10b2 "D" (D)
+
+NOTE(phillco): This currently gives the wrong result; D should either be atop C
+(obsolete) or C2. `amend --rebase` yields the same bug.
+  $ showgraph
+  o  9 D
+  |
+  | o  8 C2
+  |/
+  o  7 B
+  |
+  @  6 A
+  |
+  | x  5 D
+  | |
+  | | x  4 C2
+  | | |
+  | x |  3 C
+  | |/
+  | x  2 B
+  | |
+  | x  1 A
+  |/
+  o  0 Z

tests/test-rebase-inmemory-noconflict.t

@@ -28,7 +28,7 @@ Confirm it fails when rebasing a change that conflicts:
   rebasing in-memory!
   rebasing 3:955ac081fc7c "g" (tip)
   merging c
-  hit merge conflicts (in c) and --noconflict passed; exiting.
+  hit merge conflicts (in c) and --noconflict passed; exiting
   $ hg st
   M b
   $ cat b