Improve final messages under "arc land --hold"

Summary: Ref T13546. Update some of the "arc land --hold" behavior to be more functional/consistent with the updated workflow.

Test Plan: Ran "arc land --hold" under various conditions, got sensible forward/restore instructions.

Maniphest Tasks: T13546

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

src/land/engine/ArcanistGitLandEngine.php

@@ -435,18 +435,10 @@ final class ArcanistGitLandEngine
       pht('PUSHING'),
       pht('Pushing changes to "%s".', $this->getOntoRemote()));
 
-    $refspecs = array();
-    foreach ($this->getOntoRefs() as $onto_ref) {
-      $refspecs[] = sprintf(
-        '%s:%s',
-        $into_commit,
-        $onto_ref);
-    }
-
     $err = $api->execPassthru(
       'push -- %s %Ls',
       $this->getOntoRemote(),
-      $refspecs);
+      $this->newOntoRefArguments($into_commit));
 
     if ($err) {
       throw new ArcanistUsageException(
@@ -738,57 +730,57 @@ final class ArcanistGitLandEngine
     );
   }
 
-  protected function didHoldChanges(
-    ArcanistRepositoryLocalState $state) {
+  protected function didHoldChanges($into_commit) {
     $log = $this->getLogEngine();
-
-    // TODO: This probably needs updates.
-
-    // TODO: We should refuse "--hold" if we stash.
+    $local_state = $this->getLocalState();
 
     if ($this->getIsGitPerforce()) {
-      $this->writeInfo(
-        pht('HOLD'),
-        pht(
-          'Holding change locally, it has not been submitted.'));
+      $message = pht(
+        'Holding changes locally, they have not been submitted.');
 
       $push_command = csprintf(
-        '$ git p4 submit -M --commit %R --',
-        $this->mergedRef);
+        'git p4 submit -M --commit %s --',
+        $into_commit);
     } else {
-      $log->writeStatus(
-        pht('HOLD'),
-        pht(
-          'Holding change locally, it has not been pushed.'));
+      $message = pht(
+        'Holding changes locally, they have not been pushed.');
 
-      $push_command = 'TODO: ...';
-      // csprintf(
-      //   '$ git push -- %R %R:%R',
-      //   $this->getOntoRemote(),
-      //   $this->mergedRef,
-      //   $this->getOnto());
+      $push_command = csprintf(
+        'git push -- %s %Ls',
+        $this->getOntoRemote(),
+        $this->newOntoRefArguments($into_commit));
     }
 
-    $restore_command = 'TODO: ...';
+    echo tsprintf(
+      "\n%!\n%s\n\n",
+      pht('HOLD CHANGES'),
+      $message);
+
+    echo tsprintf(
+      "%s\n\n%>\n",
+      pht('To push changes manually, run this command:'),
+      $push_command);
+
+    $restore_commands = $local_state->getRestoreCommandsForDisplay();
+    if ($restore_commands) {
+      echo tsprintf(
+        "%s\n\n",
+        pht(
+          'To go back to how things were before you ran "arc land", run '.
+          'these %s command(s):',
+          phutil_count($restore_commands)));
+
+      foreach ($restore_commands as $restore_command) {
+        echo tsprintf('%>', $restore_command);
+      }
+
+      echo tsprintf("\n");
+    }
 
     echo tsprintf(
-      "\n%s\n\n".
-      "%s\n\n".
-      "    **%s**\n\n".
-      "%s\n\n".
-      "    **%s**\n\n".
       "%s\n",
       pht(
-        'This local working copy now contains the merged changes in a '.
-        'detached state.'),
-      pht('You can push the changes manually with this command:'),
-      $push_command,
-      pht(
-        'You can go back to how things were before you ran "arc land" with '.
-        'this command:'),
-      $restore_command,
-      pht(
-        'Local branches have not been changed, and are still in exactly the '.
+        'Local branches have not been changed, and are still in the '.
         'same state as before.'));
   }
 
@@ -1407,7 +1399,7 @@ final class ArcanistGitLandEngine
     $log = $this->getLogEngine();
 
     $branch = $api->getBranchName();
-    if ($branch === null) {
+    if ($branch !== null) {
       $log->writeStatus(
         pht('SOURCE'),
         pht(
@@ -1425,7 +1417,20 @@ final class ArcanistGitLandEngine
         'Landing the current HEAD, "%s".',
         $commit->getCommitHash()));
 
-    return array($branch);
+    return array($commit->getCommitHash());
+  }
+
+  private function newOntoRefArguments($into_commit) {
+    $refspecs = array();
+
+    foreach ($this->getOntoRefs() as $onto_ref) {
+      $refspecs[] = sprintf(
+        '%s:%s',
+        $this->getDisplayHash($into_commit),
+        $onto_ref);
+    }
+
+    return $refspecs;
   }
 
 }

src/land/engine/ArcanistLandEngine.php

@@ -31,6 +31,8 @@ abstract class ArcanistLandEngine extends Phobject {
   private $intoEmpty;
   private $intoLocal;
 
+  private $localState;
+
   final public function setViewer($viewer) {
     $this->viewer = $viewer;
     return $this;
@@ -258,6 +260,15 @@ abstract class ArcanistLandEngine extends Phobject {
     return $this->intoArgument;
   }
 
+  private function setLocalState(ArcanistRepositoryLocalState $local_state) {
+    $this->localState = $local_state;
+    return $this;
+  }
+
+  final protected function getLocalState() {
+    return $this->localState;
+  }
+
   final protected function getOntoFromConfiguration() {
     $config_key = $this->getOntoConfigurationKey();
     return $this->getWorkflow()->getConfig($config_key);
@@ -1232,6 +1243,8 @@ abstract class ArcanistLandEngine extends Phobject {
       ->setWorkflow($workflow)
       ->saveLocalState();
 
+    $this->setLocalState($local_state);
+
     $seen_into = array();
     try {
       $last_key = last_key($sets);
@@ -1309,19 +1322,18 @@ abstract class ArcanistLandEngine extends Phobject {
       }
 
       if ($is_hold) {
-        $this->didHoldChanges($local_state);
+        $this->didHoldChanges($into_commit);
         $local_state->discardLocalState();
       } else {
+        // TODO: Restore this.
+        // $this->getWorkflow()->askForRepositoryUpdate();
+
         $this->reconcileLocalState($into_commit, $local_state);
+
+        $log->writeSuccess(
+          pht('DONE'),
+          pht('Landed changes.'));
       }
-
-      // TODO: Restore this.
-      // $this->getWorkflow()->askForRepositoryUpdate();
-
-      // TODO: This is misleading under "--hold".
-      $log->writeSuccess(
-        pht('DONE'),
-        pht('Landed changes.'));
     } catch (Exception $ex) {
       $local_state->restoreLocalState();
       throw $ex;
@@ -1413,6 +1425,8 @@ abstract class ArcanistLandEngine extends Phobject {
     $into_commit,
     ArcanistRepositoryLocalState $state);
 
+  abstract protected function didHoldChanges($into_commit);
+
   private function selectMergeStrategy() {
     $log = $this->getLogEngine();
 

src/land/engine/ArcanistMercurialLandEngine.php

@@ -503,7 +503,7 @@ final class ArcanistMercurialLandEngine
 
     $api->execxLocal(
       'push --rev %s -- %s',
-      $into_commit,
+      hgsprintf('%s', $into_commit),
       $this->getOntoRemote());
   }
 
@@ -600,4 +600,52 @@ final class ArcanistMercurialLandEngine
     $state->discardLocalState();
   }
 
+  protected function didHoldChanges($into_commit) {
+    $log = $this->getLogEngine();
+    $local_state = $this->getLocalState();
+
+    $message = pht(
+      'Holding changes locally, they have not been pushed.');
+
+    $push_command = csprintf(
+      '$ hg push -- %s %Ls',
+
+      // TODO: When a parameter contains only "safe" characters, we could
+      // relax the behavior of hgsprintf().
+
+      hgsprintf('%s', $this->getDisplayHash($into_commit)),
+      $this->newOntoRefArguments($into_commit));
+
+    echo tsprintf(
+      "\n%!\n%s\n\n",
+      pht('HOLD CHANGES'),
+      $message);
+
+    echo tsprintf(
+      "%s\n\n    **%s**\n\n",
+      pht('To push changes manually, run this command:'),
+      $push_command);
+
+    $restore_commands = $local_state->getRestoreCommandsForDisplay();
+    if ($restore_commands) {
+      echo tsprintf(
+        "%s\n\n",
+        pht(
+          'To go back to how things were before you ran "arc land", run '.
+          'these %s command(s):',
+          phutil_count($restore_commands)));
+
+      foreach ($restore_commands as $restore_command) {
+        echo tsprintf("    **%s**\n", $restore_command);
+      }
+
+      echo tsprintf("\n");
+    }
+
+    echo tsprintf(
+      "%s\n".
+      pht(
+        'Local branches and bookmarks have not been changed, and are still '.
+        'in the same state as before.'));
+  }
 }

src/repository/state/ArcanistGitLocalState.php

@@ -84,6 +84,29 @@ final class ArcanistGitLocalState
     return;
   }
 
+  protected function newRestoreCommandsForDisplay() {
+    $ref = $this->localRef;
+    $commit = $this->localCommit;
+
+    $commands = array();
+
+    if ($ref !== null) {
+      $commands[] = csprintf(
+        'git checkout -B %s %s --',
+        $ref,
+        $this->getDisplayHash($commit));
+    } else {
+      $commands[] = csprintf(
+        'git checkout %s --',
+        $this->getDisplayHash($commit));
+    }
+
+    // NOTE: We run "submodule update" in the real restore workflow, but
+    // assume users can reasonably figure that out on their own.
+
+    return $commands;
+  }
+
   protected function canStashChanges() {
     return true;
   }

src/repository/state/ArcanistMercurialLocalState.php

@@ -45,6 +45,11 @@ final class ArcanistMercurialLocalState
     return array();
   }
 
+  protected function newRestoreCommandsForDisplay() {
+    // TODO: Provide this.
+    return array();
+  }
+
   protected function saveStash() {
     return null;
   }

src/repository/state/ArcanistRepositoryLocalState.php

@@ -189,6 +189,10 @@ abstract class ArcanistRepositoryLocalState
     $this->discardLocalState();
   }
 
+  final public function getRestoreCommandsForDisplay() {
+    return $this->newRestoreCommandsForDisplay();
+  }
+
   protected function canStashChanges() {
     return false;
   }
@@ -208,6 +212,7 @@ abstract class ArcanistRepositoryLocalState
   abstract protected function executeSaveLocalState();
   abstract protected function executeRestoreLocalState();
   abstract protected function executeDiscardLocalState();
+  abstract protected function newRestoreCommandsForDisplay();
 
   protected function getIgnoreHints() {
     return array();

src/xsprintf/tsprintf.php

@@ -53,6 +53,11 @@ function xsprintf_terminal($userdata, &$pattern, &$pos, &$value, &$length) {
       $value = PhutilTerminalString::escapeStringValue($value, false);
       $type = 's';
       break;
+    case '>':
+      $value = tsprintf("    **$ %s**\n", $value);
+      $value = PhutilTerminalString::escapeStringValue($value, false);
+      $type = 's';
+      break;
     case 'd':
       $type = 'd';
       break;