Improve "--hold", save/restore state, bookmark creation, and some warnings for "arc land" in Mercurial

Summary:
Ref T13546. Ref T9948.

  - Make "--hold" show the same set of commands to manually push that the normal workflow would use.
  - Make save/restore state work.
  - Make bookmark creation prompt for confirmation.
  - Improve / provide some additional warnings and help text.

Test Plan: Ran various increasingly complex "arc land" workflows, e.g. "arc land --hold --onto fauxmark1 --onto fauxmark2 --into default . --revision 118 --trace"

Maniphest Tasks: T13546, T9948

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

src/land/engine/ArcanistMercurialLandEngine.php

@@ -89,7 +89,7 @@ final class ArcanistMercurialLandEngine
 
       $markers = mgroup($markers, 'getName');
 
-      foreach ($unresolved as $key =>  $symbol) {
+      foreach ($unresolved as $key => $symbol) {
         $raw_symbol = $symbol->getSymbol();
 
         $named_markers = idx($markers, $raw_symbol);
@@ -98,12 +98,25 @@ final class ArcanistMercurialLandEngine
         }
 
         if (count($named_markers) > 1) {
-          throw new PhutilArgumentUsageException(
+          echo tsprintf(
+            "\n%!\n%W\n\n",
+            pht('AMBIGUOUS SYMBOL'),
             pht(
               'Symbol "%s" is ambiguous: it matches multiple markers '.
               '(of type "%s"). Use an unambiguous identifier.',
               $raw_symbol,
               $marker_type));
+
+          foreach ($named_markers as $named_marker) {
+            echo tsprintf('%s', $named_marker->newDisplayRef());
+          }
+
+          echo tsprintf("\n");
+
+          throw new PhutilArgumentUsageException(
+            pht(
+              'Symbol "%s" is ambiguous.',
+              $symbol));
         }
 
         $marker = head($named_markers);
@@ -300,16 +313,58 @@ final class ArcanistMercurialLandEngine
 
       $branch_count = count($branches);
       if ($branch_count > 1) {
+        echo tsprintf(
+          "\n%!\n%W\n\n%W\n\n%W\n\n",
+          pht('MULTIPLE "ONTO" BRANCHES'),
+          pht(
+            'You have selected multiple branches to push changes onto. '.
+            'Pushing to multiple branches is not supported by "arc land" '.
+            'in Mercurial: Mercurial commits may only belong to one '.
+            'branch, so this operation can not be executed atomically.'),
+          pht(
+            'You may land one branches and any number of bookmarks in a '.
+            'single operation.'),
+          pht('These branches were selected:'));
+
+        foreach ($branches as $branch) {
+          echo tsprintf('%s', $branch->newDisplayRef());
+        }
+
+        echo tsprintf("\n");
+
         throw new PhutilArgumentUsageException(
           pht(
-            'TODO: You can not push onto multiple branches in Mercurial.'));
+            'Landing onto multiple branches at once is not supported in '.
+            'Mercurial.'));
       } else if ($branch_count) {
         $this->ontoBranchMarker = head($branches);
       }
     }
 
     if ($new_markers) {
-      // TODO: If we're creating bookmarks, ask the user to confirm.
+      echo tsprintf(
+        "\n%!\n%W\n\n",
+        pht('CREATE %s BOOKMARK(S)', phutil_count($new_markers)),
+        pht(
+          'These %s symbol(s) do not exist in the remote. They will be created '.
+          'as new bookmarks:',
+          phutil_count($new_markers)));
+
+
+      foreach ($new_markers as $new_marker) {
+        echo tsprintf('%s', $new_marker->newDisplayRef());
+      }
+
+      echo tsprintf("\n");
+
+      $query = pht(
+        'Create %s new remote bookmark(s)?',
+        phutil_count($new_markers));
+
+      $this->getWorkflow()
+        ->getPrompt('arc.land.create')
+        ->setQuery($query)
+        ->execute();
     }
 
     $this->ontoMarkers = $onto_markers;
@@ -761,6 +816,30 @@ final class ArcanistMercurialLandEngine
   protected function pushChange($into_commit) {
     $api = $this->getRepositoryAPI();
 
+    list($head, $body, $tail) = $this->newPushCommands($into_commit);
+
+    foreach ($head as $command) {
+      $api->execxLocal('%Ls', $command);
+    }
+
+    try {
+      foreach ($body as $command) {
+        $this->newPasthru('%Ls', $command);
+      }
+    } finally {
+      foreach ($tail as $command) {
+        $api->execxLocal('%Ls', $command);
+      }
+    }
+  }
+
+  private function newPushCommands($into_commit) {
+    $api = $this->getRepositoryAPI();
+
+    $head_commands = array();
+    $body_commands = array();
+    $tail_commands = array();
+
     $bookmarks = array();
     foreach ($this->ontoMarkers as $onto_marker) {
       if (!$onto_marker->isBookmark()) {
@@ -776,8 +855,8 @@ final class ArcanistMercurialLandEngine
     $restore = array();
     if ($bookmarks) {
       $markers = $api->newMarkerRefQuery()
-        ->withNames(array(mpull($bookmarks, 'getName')))
+        ->withNames(mpull($bookmarks, 'getName'))
-        ->withTypes(array(ArcanistMarkerRef::TYPE_BOOKMARK))
+        ->withMarkerTypes(array(ArcanistMarkerRef::TYPE_BOOKMARK))
         ->execute();
       $markers = mpull($markers, 'getCommitHash', 'getName');
 
@@ -791,6 +870,15 @@ final class ArcanistMercurialLandEngine
           continue;
         }
 
+        $head_commands[] = array(
+          'bookmark',
+          '--force',
+          '--rev',
+          hgsprintf('%s', $this->getDisplayHash($new_position)),
+          '--',
+          $bookmark_name,
+        );
+
         $api->execxLocal(
           'bookmark --force --rev %s -- %s',
           hgsprintf('%s', $new_position),
@@ -800,7 +888,7 @@ final class ArcanistMercurialLandEngine
       }
     }
 
-    // Now, do the actual push.
+    // Now, prepare the actual push.
 
     $argv = array();
     $argv[] = 'push';
@@ -821,23 +909,33 @@ final class ArcanistMercurialLandEngine
     $argv[] = '--';
     $argv[] = $this->getOntoRemote();
 
-    try {
+    $body_commands[] = $argv;
-      $this->newPassthru('%Ls', $argv);
+
-    } finally {
+    // Finally, restore the bookmarks.
-      foreach ($restore as $bookmark_name => $old_position) {
+
-        if ($old_position === null) {
+    foreach ($restore as $bookmark_name => $old_position) {
-          $api->execxLocal(
+      $tail = array();
-            'bookmark --delete -- %s',
+      $tail[] = 'bookmark';
-            $bookmark_name);
+
-        } else {
+      if ($old_position === null) {
-          $api->execxLocal(
+        $tail[] = '--delete';
-            'bookmark --force --rev %s -- %s',
+      } else {
-            hgsprintf('%s', $old_position),
+        $tail[] = '--force';
-            $bookmark_name);
+        $tail[] = '--rev';
-        }
+        $tail[] = hgsprintf('%s', $this->getDisplayHash($old_position));
       }
+
+      $tail[] = '--';
+      $tail[] = $bookmark_name;
+
+      $tail_commands[] = $tail;
     }
 
+    return array(
+      $head_commands,
+      $body_commands,
+      $tail_commands,
+    );
   }
 
   protected function cascadeState(ArcanistLandCommitSet $set, $into_commit) {
@@ -940,12 +1038,8 @@ final class ArcanistMercurialLandEngine
     $message = pht(
       'Holding changes locally, they have not been pushed.');
 
-    // TODO: This is only vaguely correct.
+    list($head, $body, $tail) = $this->newPushCommands($into_commit);
-
+    $commands = array_merge($head, $body, $tail);
-    $push_command = csprintf(
-      '$ hg push --rev %s -- %s',
-      hgsprintf('%s', $this->getDisplayHash($into_commit)),
-      $this->getOntoRemote());
 
     echo tsprintf(
       "\n%!\n%s\n\n",
@@ -953,9 +1047,15 @@ final class ArcanistMercurialLandEngine
       $message);
 
     echo tsprintf(
-      "%s\n\n    **%s**\n\n",
+      "%s\n\n",
-      pht('To push changes manually, run this command:'),
+      pht('To push changes manually, run these %s command(s):',
-      $push_command);
+        phutil_count($commands)));
+
+    foreach ($commands as $command) {
+      echo tsprintf('%>', csprintf('hg %Ls', $command));
+    }
+
+    echo tsprintf("\n");
 
     $restore_commands = $local_state->getRestoreCommandsForDisplay();
     if ($restore_commands) {
@@ -967,7 +1067,7 @@ final class ArcanistMercurialLandEngine
           phutil_count($restore_commands)));
 
       foreach ($restore_commands as $restore_command) {
-        echo tsprintf("    **%s**\n", $restore_command);
+        echo tsprintf('%>', $restore_command);
       }
 
       echo tsprintf("\n");
@@ -980,12 +1080,4 @@ final class ArcanistMercurialLandEngine
         'in the same state as before.'));
   }
 
-
-  private function newRemoteMarkers($remote) {
-    // See T9948. If the user specified "--into X" or "--onto X", we don't know
-    // if it's a branch, a bookmark, or a symbol which doesn't exist yet.
-
-
-  }
-
 }

src/repository/state/ArcanistGitLocalState.php

@@ -47,7 +47,6 @@ final class ArcanistGitLocalState
 
   protected function executeRestoreLocalState() {
     $api = $this->getRepositoryAPI();
-
     $log = $this->getWorkflow()->getLogEngine();
 
     $ref = $this->localRef;
@@ -163,8 +162,4 @@ final class ArcanistGitLocalState
     return substr($stash_ref, 0, 12);
   }
 
-  private function getDisplayHash($hash) {
-    return substr($hash, 0, 12);
-  }
-
 }

src/repository/state/ArcanistMercurialLocalState.php

@@ -4,37 +4,45 @@ final class ArcanistMercurialLocalState
   extends ArcanistRepositoryLocalState {
 
   private $localCommit;
-  private $localRef;
+  private $localBranch;
-
-  public function getLocalRef() {
-    return $this->localRef;
-  }
-
-  public function getLocalPath() {
-    return $this->localPath;
-  }
 
   protected function executeSaveLocalState() {
     $api = $this->getRepositoryAPI();
+    $log = $this->getWorkflow()->getLogEngine();
 
-    // TODO: We need to save the position of "." and the current active
+    // TODO: Both of these can be pulled from "hg arc-ls-markers" more
-    // branch, which may be any symbol at all. Both of these can be pulled
+    // efficiently.
-    // from "hg arc-ls-markers".
 
+    $this->localCommit = $api->getCanonicalRevisionName('.');
+
+    list($branch) = $api->execxLocal('branch');
+    $this->localBranch = trim($branch);
+
+    $log->writeTrace(
+      pht('SAVE STATE'),
+      pht(
+        'Saving local state (at "%s" on branch "%s").',
+        $this->getDisplayHash($this->localCommit),
+        $this->localBranch));
   }
 
   protected function executeRestoreLocalState() {
     $api = $this->getRepositoryAPI();
+    $log = $this->getWorkflow()->getLogEngine();
 
-    // TODO: In Mercurial, we may want to discard commits we've created.
+    $log->writeStatus(
-    // $repository_api->execxLocal(
+      pht('LOAD STATE'),
-    //   '--config extensions.mq= strip %s',
+      pht(
-    //   $this->onto);
+        'Restoring local state (at "%s" on branch "%s").',
+        $this->getDisplayHash($this->localCommit),
+        $this->localBranch));
 
+    $api->execxLocal('update -- %s', $this->localCommit);
+    $api->execxLocal('branch --force -- %s', $this->localBranch);
   }
 
   protected function executeDiscardLocalState() {
-    // TODO: Fix this.
+    return;
   }
 
   protected function canStashChanges() {
@@ -51,8 +59,17 @@ final class ArcanistMercurialLocalState
   }
 
   protected function newRestoreCommandsForDisplay() {
-    // TODO: Provide this.
+    $commands = array();
-    return array();
+
+    $commands[] = csprintf(
+      'hg update -- %s',
+      $this->getDisplayHash($this->localCommit));
+
+    $commands[] = csprintf(
+      'hg branch --force -- %s',
+      $this->localBranch);
+
+    return $commands;
   }
 
   protected function saveStash() {

src/repository/state/ArcanistRepositoryLocalState.php

@@ -256,4 +256,8 @@ abstract class ArcanistRepositoryLocalState
     echo tsprintf("\n");
   }
 
+  final protected function getDisplayHash($hash) {
+    return substr($hash, 0, 12);
+  }
+
 }

src/workflow/ArcanistLandWorkflow.php

@@ -280,6 +280,11 @@ EOTEXT
         ->setDescription(
           pht(
             'Confirms that revisions with ongoing builds should land.')),
+      $this->newPrompt('arc.land.create')
+        ->setDescription(
+          pht(
+            'Confirms that new branches or bookmarks should be created '.
+            'in the remote.')),
     );
   }