Update "arc diff" to amend non-head commits with Mercurial

Summary:
After `arc diff` creates a revision in Phabricator it amends the commit to include a link to the revision in the commit message. For Mercurial this is done with `hg commit --amend --logfile` however this will fail when trying to create a diff for a non-head commit.

This updates `ArcanistMercurialAPI::amendCommit()` to allow amending a non-head commit in two ways, depending on whether `evolve` is in use:

No evolve:
1. Rebasing the current commit onto the current commit's parent, using the new commit message
2. Rebasing all children + descendants of the current commit onto the new resulting commit
3. Stripping the original commit

With evolve:
1. Amend the commit with `hg amend --logfile`
2. Run `hg evolve` to tidy up all commits

Test Plan:
I created 6 commits in a row placing a bookmark at commits 2 `bookmark1`, 4 `bookmark2`, and 6 `bookmark3`, and ensured I had `arc:bookmark` in my base ruleset.

No evolve, non-head changeset:
1. I verified I did not have `evolve` enabled by running `hg debugextensions` and did not see `evolve` in the listed active extensions.
2. I updated to `bookmark1` and modified a file to leave a dirty working state.
3. I ran `arc diff` and when prompted to amend my changes I said "yes", and verified a phab revision was created properly.
4. I checked the status of my repository and verified it was still linear and the bookmarks pointed to the proper commits.
5. I ran `hg log -r bookmark1 --template {desc}` to view the full commit message and verified it contained both `Summary: ...` and `Differential Revision: https://...`.
6. I ran `hg diff -c bookmark1` and verified the changes for that commit included the changes I made in step 2.

No evolve, head changeset:
1. I updated to `bookmark3` which is the head commit and modified a file to leave a dirty working state.
2. I ran `arc diff` and when prompted to amend my changes I said "yes", and verified a phab revision was created properly.
3. I checked the status of my repository and verified it was still linear and all the bookmarks pointed to the proper commits.
4. I ran `hg log -r bookmark3 --template {desc}` to view the full commit message and verified it contained both `Summary: ...` and `Differential Revision: https://...`.
5. I ran `hg diff -c bookmar3` and verified the changes for that commit included the changes I made in step 1.

With evolve:
1. I enabled `evolve` and verified it was enabled by running `hg debugextensions` and saw `evolve` in the listed active extensions.
2. I updated to `bookmark2` and modified a file to leave a dirty working state.
3. I ran `arc diff` and when prompted to amend my changes I said "yes", and verified a phab revision was created properly.
4. I checked the status of my repository and verified it was still linear and all the bookmarks pointed to the proper commits.
5. I ran `hg log -r bookmark2 --template {desc}` to view the full commit message and verfieid it contained both `Summary: ...` and `Differential Revision: https://...`.
6. I ran `hg diff -c bookmark2` and verified the changes for that commit included the changes I made in step 2.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D21686

src/land/engine/ArcanistMercurialLandEngine.php

@@ -803,8 +803,8 @@ final class ArcanistMercurialLandEngine
     // descendants and the min commit has no ancestors. The min/max terms are
     // used in a topological sense as chronological terms for commits can be
     // misleading or incorrect in certain situations.
-    $max_commit = last($commits)->getHash();
     $min_commit = head($commits)->getHash();
+    $max_commit = last($commits)->getHash();
 
     $revision_ref = $set->getRevisionRef();
     $commit_message = $revision_ref->getCommitMessage();

src/repository/api/ArcanistMercurialAPI.php

@@ -658,37 +658,111 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI {
   public function doCommit($message) {
     $tmp_file = new TempFile();
     Filesystem::writeFile($tmp_file, $message);
-    $this->execxLocal('commit -l %s', $tmp_file);
+    $this->execxLocal('commit --logfile %s', $tmp_file);
     $this->reloadWorkingCopy();
   }
 
   public function amendCommit($message = null) {
+    $path_statuses = $this->buildUncommittedStatus();
+
     if ($message === null) {
+      if (empty($path_statuses)) {
+        // If there are no changes to the working directory and the message is
+        // not being changed then there's nothing to amend. Notably Mercurial
+        // will return an error code if trying to amend a commit with no change
+        // to the commit metadata or file changes.
+        return;
+      }
+
       $message = $this->getCommitMessage('.');
     }
 
     $tmp_file = new TempFile();
     Filesystem::writeFile($tmp_file, $message);
 
-    try {
+    if ($this->getMercurialFeature('evolve')) {
-      $this->execxLocal(
+      $this->execxLocal('amend --logfile %s --', $tmp_file);
-        'commit --amend -l %s',
+      try {
-        $tmp_file);
+        $this->execxLocal('evolve --all --');
-    } catch (CommandException $ex) {
+      } catch (CommandException $ex) {
-      if (preg_match('/nothing changed/', $ex->getStdout())) {
+        $this->execxLocal('evolve --abort --');
-        // NOTE: Mercurial considers it an error to make a no-op amend. Although
-        // we generally defer to the underlying VCS to dictate behavior, this
-        // one seems a little goofy, and we use amend as part of various
-        // workflows under the assumption that no-op amends are fine. If this
-        // amend failed because it's a no-op, just continue.
-      } else {
         throw $ex;
       }
+      $this->reloadWorkingCopy();
+      return;
+    }
+
+    // Get the child nodes of the current changeset.
+    list($children) = $this->execxLocal(
+      'log --template %s --rev %s --',
+      '{node} ',
+      'children(.)');
+    $child_nodes = array_filter(explode(' ', $children));
+
+    // For a head commit we can simply use `commit --amend` for both new commit
+    // message and amending changes from the working directory.
+    if (empty($child_nodes)) {
+        $this->execxLocal('commit --amend --logfile %s --', $tmp_file);
+    } else {
+      $this->amendNonHeadCommit($child_nodes, $tmp_file);
     }
 
     $this->reloadWorkingCopy();
   }
 
+  /**
+   * Amends a non-head commit with a new message and file changes. This
+   * strategy is for Mercurial repositories without the evolve extension.
+   *
+   * 1. Run 'arc-amend' which uses Mercurial internals to amend the current
+   *    commit with updated message/file-changes. It results in a new commit
+   *    from the right parent
+   * 2. For each branch from the original commit, rebase onto the new commit,
+   *    removing the original branch. Note that there is potential for this to
+   *    cause a conflict but this is something the user has to address.
+   * 3. Strip the original commit.
+   *
+   * @param array     The list of child changesets off the original commit.
+   * @param file      The file containing the new commit message.
+   */
+  private function amendNonHeadCommit($child_nodes, $tmp_file) {
+    list($current) = $this->execxLocal(
+      'log --template %s --rev . --',
+      '{node}');
+
+    $argv = array();
+    foreach ($this->getMercurialExtensionArguments() as $arg) {
+      $argv[] = $arg;
+    }
+    $argv[] = 'arc-amend';
+    $argv[] = '--logfile';
+    $argv[] = $tmp_file;
+    $this->execxLocal('%Ls', $argv);
+
+    list($new_commit) = $this->execxLocal(
+      'log --rev tip --template %s --',
+      '{node}');
+
+    try {
+      $rebase_args = array(
+        '--dest',
+        $new_commit,
+      );
+      foreach ($child_nodes as $child) {
+        $rebase_args[] = '--source';
+        $rebase_args[] = $child;
+      }
+
+      $this->execxLocal('rebase %Ls --', $rebase_args);
+    } catch (CommandException $ex) {
+      $this->execxLocal('rebase --abort --');
+      throw $ex;
+    }
+
+    $this->execxLocal('--config extensions.strip= strip --rev %s --',
+      $current);
+  }
+
   public function getCommitSummary($commit) {
     if ($commit == 'null') {
       return pht('(The Empty Void)');

src/repository/state/ArcanistRepositoryLocalState.php

@@ -192,10 +192,28 @@ abstract class ArcanistRepositoryLocalState
     return false;
   }
 
+  /**
+   * Stash uncommitted changes temporarily. Use {@method:restoreStash()} to
+   * bring these changes back.
+   *
+   * Note that saving and restoring changes may not behave as expected if used
+   * in a non-stack manner, i.e. proper use involves only restoring stashes in
+   * the reverse order they were saved.
+   *
+   * @return wild  A reference object that refers to the changes which were
+   *               saved. When restoring changes this should be passed to
+   *               {@method:restoreStash()}.
+   */
   protected function saveStash() {
     throw new PhutilMethodNotImplementedException();
   }
 
+  /**
+   * Restores changes that were previously stashed by {@method:saveStash()}.
+   *
+   * @param wild  A reference object referring to which previously stashed
+   *              changes to restore, from invoking {@method:saveStash()}.
+   */
   protected function restoreStash($ref) {
     throw new PhutilMethodNotImplementedException();
   }

support/hg/arc-hg.py

@@ -21,18 +21,118 @@ from mercurial import (
   hg,
   i18n,
   node,
-  registrar,
 )
 
 _ = i18n._
 cmdtable = {}
-command = registrar.command(cmdtable)
+
+# Older veresions of Mercurial (~4.7) moved the command function and the
+# remoteopts object to different modules. Using try/except here to attempt
+# allowing this module to load properly, despite whether individual commands
+# will work properly on older versions of Mercurial or not.
+# https://phab.mercurial-scm.org/rHG46ba2cdda476ac53a8a8f50e4d9435d88267db60
+# https://phab.mercurial-scm.org/rHG04baab18d60a5c833ab3190506147e01b3c6d12c
+try:
+  from mercurial import registrar
+  command = registrar.command(cmdtable)
+except:
+  command = cmdutil.command(cmdtable)
+
+try:
+  if "remoteopts" in cmdutil:
+    remoteopts = cmdutil.remoteopts
+except:
+  from mercurial import commands
+  remoteopts = commands.remoteopts
+
+@command(
+  b'arc-amend',
+  [
+    (b'l',
+      b'logfile',
+      b'',
+      _(b'read commit message from file'),
+      _(b'FILE')),
+    (b'm',
+      b'message',
+      b'',
+      _(b'use text as commit message'),
+      _(b'TEXT')),
+    (b'u',
+      b'user',
+      b'',
+      _(b'record the specified user as committer'),
+      _(b'USER')),
+    (b'd',
+      b'date',
+      b'',
+      _(b'record the specified date as commit date'),
+      _(b'DATE')),
+    (b'A',
+      b'addremove',
+      False,
+      _(b'mark new/missing files as added/removed before committing')),
+    (b'n',
+      b'note',
+      b'',
+      _(b'store a note on amend'),
+      _(b'TEXT')),
+  ],
+  _(b'[OPTION]'))
+def amend(ui, repo, source=None, **opts):
+  """amend
+
+  Uses Mercurial internal API to amend changes to a non-head commit.
+
+  (This is an Arcanist extension to Mercurial.)
+
+  Returns 0 if amending succeeds, 1 otherwise.
+  """
+
+  # The option keys seem to come in as 'str' type but the cmdutil.amend() code
+  # expects them as binary. To account for both Python 2 and Python 3
+  # compatibility, insert the value under both 'str' and binary type.
+  newopts = {}
+  for key in opts:
+    val = opts.get(key)
+    newopts[key] = val
+    if isinstance(key, str):
+      newkey = key.encode('UTF-8')
+      newopts[newkey] = val
+
+  orig = repo[b'.']
+  extra = {}
+  pats = []
+  cmdutil.amend(ui, repo, orig, extra, pats, newopts)
+
+  """
+  # This will allow running amend on older versions of Mercurial, ~3.5, however
+  # the behavior on those versions will squash child commits of the working
+  # directory into the amended commit which is undesired.
+  try:
+    cmdutil.amend(ui, repo, orig, extra, pats, newopts)
+  except:
+    def commitfunc(ui, repo, message, match, opts):
+      return repo.commit(
+        message,
+        opts.get('user') or orig.user(),
+        opts.get('date') or orig.date(),
+        match,
+        extra=extra)
+    cmdutil.amend(ui, repo, commitfunc, orig, extra, pats, newopts)
+  """
+
+  return 0
 
 @command(
   b'arc-ls-markers',
-  [(b'', b'output', b'',
+  [
-    _(b'file to output refs to'), _(b'FILE')),
+    (b'',
-  ] + cmdutil.remoteopts,
+      b'output',
+      b'',
+    _(b'file to output refs to'),
+    _(b'FILE')),
+  ] + remoteopts,
   _(b'[--output FILENAME] [SOURCE]'))
 def lsmarkers(ui, repo, source=None, **opts):
   """list markers