Make arc land for hg more robust to failures

Summary:
Adds error handling for several kinds of failure in arc land for
mercurial.  Previously it would often leave the repo in a confusing state
if something failed.

- Aborts the land if pull brings in a diverged 'onto' branch.
- Aborts the rebase if there is a conflict. This leaves the repo exactly as
it was before, so the user is not left with a half finished rebase.
- Don't delete the original non-squashed branch until the push succeeds.
- If the push fails, strip the temporary squashed commit. This leaves the
'onto' branch back on the latest commit from the server, and leaves the users
original nonsquashed branch around.
- Always leave the user back on their original branch after an error.

Test Plan:
Ran arc land:
- with pull causing a diverged 'onto' bookmark
- with the 'onto' bookmark already diverged
- with the rebase causing conflicts
- with a push that failed due to a commit hook
- with a successful land
- with a successful collapse and land

In all failure cases the repo was left exactly as it was before arc land,
except for the push-failed case, where the only change was that the branch
was correctly on top of the destination branch due to a successful rebase.

Used bookmark name "foo bar-gah" to test that crazy bookmark names still work.

Reviewers: epriestley, dschleimer, sid0

Reviewed By: epriestley

CC: nh, wez, bos, aran, Korvin

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

src/workflow/ArcanistLandWorkflow.php

@@ -447,28 +447,17 @@ EOTEXT
         "Updating **%s**...\n",
         $this->onto);
 
-      if ($repository_api->supportsPhases()) {
-        list($out) = $repository_api->execxLocal(
-          'log -r %s --template {phase}', $this->onto);
-        if ($out != 'public') {
-          $local_ahead_of_remote = true;
-        }
-      } else {
-        // execManual instead of execx because outgoing returns
-        // code 1 when there is nothing outgoing
-        list($err, $out) = $repository_api->execManualLocal(
-          'outgoing -r %s',
-          $this->onto);
-
-        // $err === 0 means something is outgoing
-        if ($err === 0) {
-          $local_ahead_of_remote = true;
-        }
-      }
-
-      if (!$local_ahead_of_remote) {
       try {
-          $repository_api->execxLocal('pull');
+        list($out, $err) = $repository_api->execxLocal('pull');
+
+        $divergedbookmark = $this->onto.'@'.$repository_api->getBranchName();
+        if (strpos($err, $divergedbookmark) !== false) {
+          throw new ArcanistUsageException(phutil_console_format(
+            "Local bookmark **{$this->onto}** has diverged from the ".
+            "server's **{$this->onto}** (now labeled ".
+            "**{$divergedbookmark}**). Please resolve this divergence and ".
+            "run 'arc land' again."));
+        }
       } catch (CommandException $ex) {
         $err = $ex->getError();
         $stdout = $ex->getStdOut();
@@ -491,12 +480,29 @@ EOTEXT
 
         // TODO: Remove this once we're far enough in the future that
         // deployment of 2.1 is exceedingly rare?
-          if ($err == 1 && preg_match('/no changes found/', $stdout)) {
+        if ($err != 1 || !preg_match('/no changes found/', $stdout)) {
-            return;
-          } else {
           throw $ex;
         }
       }
+
+      // Pull succeeded.  Now make sure master is not on an outgoing change
+      if ($repository_api->supportsPhases()) {
+        list($out) = $repository_api->execxLocal(
+          'log -r %s --template {phase}', $this->onto);
+        if ($out != 'public') {
+          $local_ahead_of_remote = true;
+        }
+      } else {
+        // execManual instead of execx because outgoing returns
+        // code 1 when there is nothing outgoing
+        list($err, $out) = $repository_api->execManualLocal(
+          'outgoing -r %s',
+          $this->onto);
+
+        // $err === 0 means something is outgoing
+        if ($err === 0) {
+          $local_ahead_of_remote = true;
+        }
       }
     }
 
@@ -512,6 +518,11 @@ EOTEXT
   private function rebase() {
     $repository_api = $this->getRepositoryAPI();
 
+    echo phutil_console_format(
+        "Rebasing **%s** onto **%s**\n",
+        $this->branch,
+        $this->onto);
+
     chdir($repository_api->getPath());
     if ($this->isGit) {
       if ($this->shouldUpdateWithRebase) {
@@ -550,12 +561,15 @@ EOTEXT
           'rebase -d %s --keepbranches',
           $this->onto);
         if ($err) {
+          echo phutil_console_format("Aborting rebase\n");
+          $repository_api->execManualLocal(
+            'rebase --abort');
+          $this->restoreBranch();
           throw new ArcanistUsageException(
-            "'hg rebase {$this->onto}' failed. ".
+            "'hg rebase {$this->onto}' failed and the rebase was aborted. ".
-            "You can abort with 'hg rebase --abort', ".
+            "This is most likely due to conflicts. Manually rebase ".
-            "or resolve conflicts and use 'hg rebase ".
+            "{$this->branch} onto {$this->onto}, resolve the conflicts, ".
-            "--continue' to continue forward. After resolving the rebase, ".
+            "then run 'arc land' again.");
-            "run 'arc land' again.");
         }
       }
     }
@@ -608,12 +622,21 @@ EOTEXT
 
       // Collapse just the landing branch onto master.
       // Leave its children on the original branch.
-      $repository_api->execxLocal(
+      $err = $repository_api->execPassthru(
         'rebase --collapse --keep --logfile %s -r %s -d %s',
         $this->messageFile,
         $branch_range,
         $this->onto);
 
+      if ($err) {
+        $repository_api->execManualLocal(
+          'rebase --abort');
+        $this->restoreBranch();
+        throw new ArcanistUsageException(
+          "Squashing the commits under {$this->branch} failed. ".
+          "Manually squash your commits and run 'arc land' again.");
+      }
+
       if ($repository_api->isBookmark($this->branch)) {
         // a bug in mercurial means bookmarks end up on the revision prior
         // to the collapse when using --collapse with --keep,
@@ -646,19 +669,6 @@ EOTEXT
         }
       }
 
-      // delete the old branch if necessary
-      if (!$this->keepBranch) {
-        $repository_api->execxLocal(
-          '--config extensions.mq= strip -r %s',
-          $branch_root);
-
-        if ($repository_api->isBookmark($this->branch)) {
-          $repository_api->execxLocal(
-            'bookmark -d %s',
-            $this->branch);
-        }
-      }
-
       // All the rebases may have moved us to another branch
       // so we move back.
       $repository_api->execxLocal('checkout %s', $this->onto);
@@ -853,10 +863,15 @@ EOTEXT
   }
 
   private function executeCleanupAfterFailedPush() {
-    if ($this->isGit) {
     $repository_api = $this->getRepositoryAPI();
+    if ($this->isGit) {
       $repository_api->execxLocal('reset --hard HEAD^');
       $this->restoreBranch();
+    } else if ($this->isHg) {
+      $repository_api->execxLocal(
+        '--config extensions.mq= strip %s',
+        $this->onto);
+      $this->restoreBranch();
     }
   }
 
@@ -877,8 +892,28 @@ EOTEXT
       $repository_api->execxLocal(
         'branch -D %s',
         $this->branch);
+    } else if ($this->isHg) {
+      $common_ancestor = $repository_api->getCanonicalRevisionName(
+        hgsprintf("ancestor(%s,%s)",
+          $this->onto,
+          $this->branch));
+
+      $branch_root = $repository_api->getCanonicalRevisionName(
+        hgsprintf("first((%s::%s)-%s)",
+          $common_ancestor,
+          $this->branch,
+          $common_ancestor));
+
+      $repository_api->execxLocal(
+        '--config extensions.mq= strip -r %s',
+        $branch_root);
+
+      if ($repository_api->isBookmark($this->branch)) {
+        $repository_api->execxLocal(
+          'bookmark -d %s',
+          $this->branch);
+      }
     }
-    // hg branches/bookmarks were closed earlier
 
     if ($this->getArgument('delete-remote')) {
       if ($this->isGit) {