An assortment of fixes and updates to using arc-land with mercurial

Summary:
Refs T13546

**Behavior Changes**
1. Currently, after landing the `--onto` bookmark will not be advanced to the newly pushed commit(s)
  - This updates so that after pushing commits upstream, onto-marker bookmarks are pulled back from the server to retrieve their updated state
2. Currently, after landing the working directory will typically be the old `--onto` commit
  - This updates the behavior in the following ways
    1. If the starting working state is a commit that is part of a revision being landed, the post-land state will be the newly published commit for that revision
    2. If the starting working state is a commit for a tip revision being landed and there is only a single `--onto` target, the post-land state will be the newly published commit and the `--onto` bookmark will be activated, if applicable. If there are multiple `--onto` targets defined (uncommon) then the resulting working state will be the same behavior as before, as the desired behavior for this case is ambiguous.
    3. If the starting working state is a commit that is not part of any revision being landed, then the post-land state will be that same commit, activating the same previous bookmark if applicable.

**Bugs Fixed**
1. When landing a diff that includes multiple commits, where the non-tip commit adds a file and later commit modifies that file, the land process would fail during rebasing.
2. When landing, the display of what commits are going to be landed only includes the commit hash but should include part of the commit message for easy identification.
3. If errors occur during a rebase while landing, the resulting state is an in-progress rebase. Users will typically not be able to resolve this in-progress rebase and possibly confuse them further as they have to first abort the rebase before trying to clean up. These rebases will now be aborted by arcanist before exiting.
4. If using evolve, landing a non-tip revision would leave behind orphaned commits.

Test Plan:
Bookmark test
- I created a diff with bookmark `test`, as a branch from before the `master` commit
- I updated working state to activate `test`
- I landed that diff
- I verified that the resulting working state was on the newly published `master` commit

Non-bookmark test
- I created a diff with bookmark `test`, as a branch from before the `master` commit
- I updated working state to be the new commit, but not activate `test` even though it points to that same commit
- I landed that diff
- I verified that the resulting working state was on the newly published `master` commit

Not-working state test
- I created a diff with bookmark `test`, as a branch from before the `master` commit
- I updated working state to an older commit not related to the one being landed
- I landed that diff
- I verified the ending resulting working state was on the older commit I was on prior to landing

Multiple commits test
- Using `test` bookmark I created a commit that added a new file, made a diff, made another new commit on top of it which modified that file, then updated the diff
- I updated my working state to the first commit for the diff (non-head)
- I ran `arc land test` to land the diff
- I verified that the resulting working state was on the newly published `master` commit

Landing while on `master`
- I created a diff with a bookmark `test`, as a branch from before the `master` commit
- I updated working state to the `master` bookmark
- I landed that diff
- I verified that the resulting working state was on the newly published `master` commit

Landing with no local `master` bookmark
- I created a diff consisting of two commits with bookmark `test`, as a branch from before the `master` commit
- I left working state on `test` and deleted my local `master` bookmark
- I landed that diff
- I verified that the resulting working state was on the newly published `master` commit and the `master` bookmark was pulled and active

Cascading diffs while on an earlier diff changeset
- I setup two diffs, each consisting of two commits, one depending on the other
- I put the working state onto a commit from the earlier diff
- I landed the later diff
- I verified that the resulting working state was on the newly published commit associated with the earlier diff, which is not the new `master` commit so there was no active bookmark

Cascading diffs while on the later diff changeset
- I setup two diffs, each consisting of two commits, one depending on the other
- I put the working state into a commit from the later diff
- I landed the later diff
- I verified that the resulting working state was on the newly published commit for the later diff which was `master` and `master` was active

Landing a non-tip revision, with evolve
- I setup three diffs, each consisting of two commits, creating a dependency chain
- I verified I had the evolve extension enabled via `hg debugextensions`
- I put the working state on a commit in the first revision
- I landed the second revision
- I verified that the resulting working state was clean, that revisions 1 and 2 were properly landed and published, and the resulting working state was on the published commit for the first revision.

Landing a non-tip revision, without evolve
- I setup three diffs, each consisting of two commits, creating a dependency chain
- I verified I did not have the evolve extension enabled via `hg debugextensions`
- I put the working state on a commit in the first revision
- I landed the second revision
- I verified that the resulting working state was clean, that revisions 1 and 2 were properly landed and published, and the resulting working state was on the published commit for the first revision.

Landing a non-tip revision, while the non-landing revision is active
- I setup three diffs, each consisting of two commits, creating a dependency chain
- I put the working state on the latest tip revision commit and its bookmark
- I landed the second revision
- I verified that revisions 1 and 2 were properly landed and published, and the resulting working state was still on the latest tip revision commit and its bookmark was active.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T13546

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

src/land/engine/ArcanistLandEngine.php

@@ -1407,6 +1407,21 @@ abstract class ArcanistLandEngine
     ArcanistLandCommitSet $set,
     $into_commit);
   abstract protected function pushChange($into_commit);
+
+  /**
+   * Update all local refs that depend on refs selected-and-modified during the
+   * land. E.g. with branches named change1 -> change2 -> change3 and using
+   * `arc land change2`, in the general case the local change3 should be
+   * rebased onto the landed version of change2 so that it no longer has
+   * out-of-date ancestors.
+   *
+   * When multiple revisions are landed at once this will be called in a loop
+   * for each set, in order of max to min, where max is the latest descendant
+   * and min is the earliest ancestor. This is done so that non-landing commits
+   * that are descendants of the latest revision will only be rebased once.
+   *
+   * @param ArcanistLandCommitSet  The current commit set to cascade.
+   */
   abstract protected function cascadeState(
     ArcanistLandCommitSet $set,
     $into_commit);
@@ -1415,12 +1430,35 @@ abstract class ArcanistLandEngine
     return ($this->getStrategy() === 'squash');
   }
 
+  /**
+   * Prunes the given sets of commits. This should be called after the sets
+   * have been merged.
+   *
+   * @param array  The list of ArcanistLandCommitSet to prune, in order of
+   *   min to max commit set, where min is the earliest ancestor and max
+   *   is the latest descendant.
+   */
   abstract protected function pruneBranches(array $sets);
 
+  /**
+   * Restore the local repository to an expected state after landing. This
+   * should only be called after all changes have been merged, pruned, and
+   * pushed.
+   *
+   * @param string  The commit hash that was landed into.
+   * @param ArcanistRepositoryLocalState  The local state that was captured
+   *   at the beginning of the land process. This may include stashed changes.
+   */
   abstract protected function reconcileLocalState(
     $into_commit,
     ArcanistRepositoryLocalState $state);
 
+  /**
+   * Display information to the user about how to proceed since the land
+   * process was not fully completed. The merged branch has not been pushed.
+   *
+   * @param string  The commit hash that was landed into.
+   */
   abstract protected function didHoldChanges($into_commit);
 
   private function selectMergeStrategy() {

src/land/engine/ArcanistMercurialLandEngine.php

@@ -6,6 +6,8 @@ final class ArcanistMercurialLandEngine
   private $ontoBranchMarker;
   private $ontoMarkers;
 
+  private $rebasedActiveCommit;
+
   protected function getDefaultSymbols() {
     $api = $this->getRepositoryAPI();
     $log = $this->getLogEngine();
@@ -698,8 +700,14 @@ final class ArcanistMercurialLandEngine
     $commit_map = array();
     foreach ($symbols as $symbol) {
       $symbol_commit = $symbol->getCommit();
-      $template = '{node}-{parents}-';
+      $template = '{node}-{parents % \'{node} \'}-{desc|firstline}\\n';
 
+      // The returned array of commits is expected to be ordered by max to min
+      // where the max commit has no descendants in the range and the min
+      // commit has no ancestors in the range. Use 'reverse()' in the template
+      // so the output is ordered with the max commit as the first line. The
+      // max/min terms are used in a topological sense as chronological terms
+      // for commits may be misleading or incorrect in some situations.
       if ($into_commit === null) {
         list($commits) = $api->execxLocal(
           'log --rev %s --template %s --',
@@ -789,8 +797,14 @@ final class ArcanistMercurialLandEngine
 
     $commits = $set->getCommits();
 
-    $min_commit = last($commits)->getHash();
+    // confirmCommits() reverses the order of the commits as they're ordered
-    $max_commit = head($commits)->getHash();
+    // above in selectCommits(). Now the head of the list is the min commit and
+    // the last is the max commit, where within the range the max commit has no
+    // 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();
 
     $revision_ref = $set->getRevisionRef();
     $commit_message = $revision_ref->getCommitMessage();
@@ -825,8 +839,9 @@ final class ArcanistMercurialLandEngine
       $future->resolvex();
 
     } catch (CommandException $ex) {
-      // TODO
+      // Aborting the rebase should restore the same state prior to running the
-      // $api->execManualLocal('rebase --abort');
+      // rebase command.
+      $api->execManualLocal('rebase --abort');
       throw $ex;
     }
 
@@ -855,13 +870,21 @@ final class ArcanistMercurialLandEngine
     list($stdout) = $api->execxLocal('log --rev tip --template %s', '{node}');
     $new_cursor = trim($stdout);
 
+    // If any of the commits that were rebased was the active commit before the
+    // workflow started, track the new commit so it can be used as the working
+    // directory after the land has succeeded.
+    if (isset($obsolete_map[$this->getLocalState()->getLocalCommit()])) {
+      $this->rebasedActiveCommit = $new_cursor;
+    }
+
     return $new_cursor;
   }
 
   protected function pushChange($into_commit) {
     $api = $this->getRepositoryAPI();
 
-    list($head, $body, $tail) = $this->newPushCommands($into_commit);
+    list($head, $body, $tail_pass, $tail_fail) = $this->newPushCommands(
+      $into_commit);
 
     foreach ($head as $command) {
       $api->execxLocal('%Ls', $command);
@@ -876,10 +899,20 @@ final class ArcanistMercurialLandEngine
               'Push failed! Fix the error and run "arc land" again.'));
         }
       }
-    } finally {
+
-      foreach ($tail as $command) {
+      foreach ($tail_pass as $command) {
         $api->execxLocal('%Ls', $command);
       }
+    } catch (Exception $ex) {
+      foreach ($tail_fail as $command) {
+        $api->execxLocal('%Ls', $command);
+      }
+      throw $ex;
+    } catch (Throwable $ex) {
+      foreach ($tail_fail as $command) {
+        $api->execxLocal('%Ls', $command);
+      }
+      throw $ex;
     }
   }
 
@@ -888,7 +921,8 @@ final class ArcanistMercurialLandEngine
 
     $head_commands = array();
     $body_commands = array();
-    $tail_commands = array();
+    $tail_pass_commands = array();
+    $tail_fail_commands = array();
 
     $bookmarks = array();
     foreach ($this->ontoMarkers as $onto_marker) {
@@ -902,7 +936,7 @@ final class ArcanistMercurialLandEngine
     // to the merge commit. (There doesn't seem to be any way to specify
     // "push commit X as bookmark Y" in Mercurial.)
 
-    $restore = array();
+    $restore_bookmarks = array();
     if ($bookmarks) {
       $markers = $api->newMarkerRefQuery()
         ->withNames(mpull($bookmarks, 'getName'))
@@ -934,7 +968,9 @@ final class ArcanistMercurialLandEngine
           hgsprintf('%s', $new_position),
           $bookmark_name);
 
-        $restore[$bookmark_name] = $old_position;
+        if ($old_position !== null) {
+          $restore_bookmarks[$bookmark_name] = $old_position;
+        }
       }
     }
 
@@ -963,28 +999,41 @@ final class ArcanistMercurialLandEngine
 
     // Finally, restore the bookmarks.
 
-    foreach ($restore as $bookmark_name => $old_position) {
+    if ($restore_bookmarks) {
-      $tail = array();
+      // Instead of restoring the previous state, assume landing onto bookmarks
-      $tail[] = 'bookmark';
+      // also updates those bookmarks in the remote. After pushing, pull the
+      // latest state of these bookmarks. Mercurial allows pulling multiple
+      // bookmarks in a single pull command which will be faster than pulling
+      // them from a remote individually.
+      $tail = array(
+        'pull',
+      );
 
-      if ($old_position === null) {
+      foreach ($restore_bookmarks as $bookmark_name => $old_position) {
-        $tail[] = '--delete';
+        $tail[] = '--bookmark';
-      } else {
-        $tail[] = '--force';
-        $tail[] = '--rev';
-        $tail[] = hgsprintf('%s', $api->getDisplayHash($old_position));
-      }
-
-      $tail[] = '--';
         $tail[] = $bookmark_name;
 
-      $tail_commands[] = $tail;
+        // In the failure case restore the state of the bookmark. Mercurial
+        // does not provide a way to move multiple bookmarks in a single
+        // command however these commands do not involve the remote.
+        $tail_fail_commands[] = array(
+          'bookmark',
+          '--force',
+          '--rev',
+          hgsprintf('%s', $api->getDisplayHash($old_position)),
+        );
+      }
+
+      if ($tail) {
+        $tail_pass_commands[] = $tail;
+      }
     }
 
     return array(
       $head_commands,
       $body_commands,
-      $tail_commands,
+      $tail_pass_commands,
+      $tail_fail_commands,
     );
   }
 
@@ -1020,7 +1069,9 @@ final class ArcanistMercurialLandEngine
           $child_hash,
           $new_commit);
       } catch (CommandException $ex) {
-        // TODO: Recover state.
+        // Aborting the rebase should restore the same state prior to running
+        // the rebase command.
+        $api->execManualLocal('rebase --abort');
         throw $ex;
       }
     }
@@ -1040,6 +1091,8 @@ final class ArcanistMercurialLandEngine
     $revs = array();
     $obsolete_map = array();
 
+    $using_evolve = $api->getMercurialFeature('evolve');
+
     // We've rebased all descendants already, so we can safely delete all
     // of these commits.
 
@@ -1047,10 +1100,22 @@ final class ArcanistMercurialLandEngine
     foreach ($sets as $set) {
       $commits = $set->getCommits();
 
+      // In the commit set the min commit should be the commit with no
+      // ancestors and the max commit should be the commit with no descendants.
+      // The min/max terms are used in a toplogical sense as chronological
+      // terms for commits may be misleading or incorrect in some situations.
       $min_commit = head($commits)->getHash();
       $max_commit = last($commits)->getHash();
 
+      if ($using_evolve) {
+        // If non-head series of commits are rebased while the evolve extension
+        // is in use, the rebase leaves behind the entire series of descendants
+        // in which case the entire chain needs removed, not just a section.
+        // Otherwise this results in the prune leaving behind orphaned commits.
+        $revs[] = hgsprintf('%s::', $min_commit);
+      } else {
         $revs[] = hgsprintf('%s::%s', $min_commit, $max_commit);
+      }
 
       foreach ($commits as $commit) {
         $obsolete_map[$commit->getHash()] = true;
@@ -1071,10 +1136,7 @@ final class ArcanistMercurialLandEngine
     // bookmarks which point at these now-obsoleted commits.
 
     $bookmark_refs = $api->newMarkerRefQuery()
-      ->withMarkerTypes(
+      ->withMarkerTypes(array(ArcanistMarkerRef::TYPE_BOOKMARK))
-        array(
-          ArcanistMarkerRef::TYPE_BOOKMARK,
-        ))
       ->execute();
     foreach ($bookmark_refs as $bookmark_ref) {
       $bookmark_hash = $bookmark_ref->getCommitHash();
@@ -1093,7 +1155,7 @@ final class ArcanistMercurialLandEngine
         $bookmark_name);
     }
 
-    if ($api->getMercurialFeature('evolve')) {
+    if ($using_evolve) {
       $api->execxLocal(
         'prune --rev %s',
         $rev_set);
@@ -1108,7 +1170,54 @@ final class ArcanistMercurialLandEngine
     $into_commit,
     ArcanistRepositoryLocalState $state) {
 
-    // TODO: For now, just leave users wherever they ended up.
+    $api = $this->getRepositoryAPI();
+
+    // If the starting working state was not part of land process just update
+    // to that original working state.
+    if ($this->rebasedActiveCommit === null) {
+      $update_marker = $this->getLocalState()->getLocalCommit();
+      if ($this->getLocalState()->getLocalBookmark() !== null) {
+        $update_marker = $this->getLocalState()->getLocalBookmark();
+      }
+
+      $api->execxLocal(
+        'update -- %s',
+        $update_marker);
+
+      $state->discardLocalState();
+      return;
+    }
+
+    // If the working state was landed into multiple destinations then the
+    // resulting working state is ambiguous.
+    if (count($this->ontoMarkers) != 1) {
+      $state->discardLocalState();
+      return;
+    }
+
+    // Get the current state of bookmarks
+    $bookmark_refs = $api->newMarkerRefQuery()
+      ->withMarkerTypes(array(ArcanistMarkerRef::TYPE_BOOKMARK))
+      ->execute();
+
+    $update_marker = $this->rebasedActiveCommit;
+
+    // Find any bookmarks which exist on the commit which is the result of the
+    // starting working directory's rebase. If any of those bookmarks are also
+    // the destination marker then we use that bookmark as the update in order
+    // for it to become active.
+    $onto_marker = $this->ontoMarkers[0]->getName();
+    foreach ($bookmark_refs as $bookmark_ref) {
+      if ($bookmark_ref->getCommitHash() == $this->rebasedActiveCommit &&
+        $bookmark_ref->getName() == $onto_marker) {
+          $update_marker = $onto_marker;
+          break;
+      }
+    }
+
+    $api->execxLocal(
+      'update -- %s',
+      $update_marker);
 
     $state->discardLocalState();
   }
@@ -1120,8 +1229,9 @@ final class ArcanistMercurialLandEngine
     $message = pht(
       'Holding changes locally, they have not been pushed.');
 
-    list($head, $body, $tail) = $this->newPushCommands($into_commit);
+    list($head, $body, $tail_pass, $tail_fail) = $this->newPushCommands(
-    $commands = array_merge($head, $body, $tail);
+      $into_commit);
+    $commands = array_merge($head, $body, $tail_pass);
 
     echo tsprintf(
       "\n%!\n%s\n\n",

src/repository/state/ArcanistMercurialLocalState.php

@@ -7,6 +7,14 @@ final class ArcanistMercurialLocalState
   private $localBranch;
   private $localBookmark;
 
+  public function getLocalCommit() {
+    return $this->localCommit;
+  }
+
+  public function getLocalBookmark() {
+    return $this->localBookmark;
+  }
+
   protected function executeSaveLocalState() {
     $api = $this->getRepositoryAPI();
     $log = $this->getWorkflow()->getLogEngine();