Tweak error and status messages for commit ranges

Summary: Improve/clarify some error messages a bit, hopefully.

Test Plan: Ran `arc which`, `arc diff`, etc., with various explicit, implicit, and `--head` flags. Read error messages, didn't catch anything too awkward.

Reviewers: talshiri

Reviewed By: talshiri

Subscribers: epriestley

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

src/repository/api/ArcanistGitAPI.php

@@ -16,7 +16,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
    */
   const GIT_MAGIC_ROOT_COMMIT = '4b825dc642cb6eb9a060e54bf8d69288fbee4904';
 
-  private $symbolicHeadCommit = 'HEAD';
+  private $symbolicHeadCommit;
   private $resolvedHeadCommit;
 
   public static function newHookAPI($root) {
@@ -81,16 +81,18 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
   /**
    * Tests if a child commit is descendant of a parent commit.
    * If child and parent are the same, it returns false.
-   * @param child commit SHA.
+   * @param Child commit SHA.
-   * @param parent commit SHA.
+   * @param Parent commit SHA.
-   * @return bool
+   * @return bool True if the child is a descendant of the parent.
    */
   private function isDescendant($child, $parent) {
-    list($common_ancestor) =
+    list($common_ancestor) = $this->execxLocal(
-      $this->execxLocal('merge-base %s %s', $child, $parent);
+      'merge-base %s %s',
+      $child,
+      $parent);
     $common_ancestor = trim($common_ancestor);
 
-    return $common_ancestor == $parent && $common_ancestor != $child;
+    return ($common_ancestor == $parent) && ($common_ancestor != $child);
   }
 
   public function getLocalCommitInformation() {
@@ -105,8 +107,8 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
     } else {
 
       // 2..N commits. We include commits reachable from HEAD which are
-      // not reachable from the relative commit; this is consistent with
+      // not reachable from the base commit; this is consistent with user
-      // user expectations even though it is not actually the diff range.
+      // expectations even though it is not actually the diff range.
       // Particularly:
       //
       //    |
@@ -124,16 +126,38 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
       // this as being the commits X and Y. If we log "B..Y", we only show
       // Y. With "Y --not B", we show X and Y.
 
+
+      if ($this->symbolicHeadCommit !== null) {
         $base_commit = $this->getBaseCommit();
-      $head_commit = $this->getHeadCommit();
+        $resolved_base = $this->resolveCommit($base_commit);
-      if ($this->isDescendant($head_commit, $base_commit) === false) {
+
+        $head_commit = $this->symbolicHeadCommit;
+        $resolved_head = $this->getHeadCommit();
+
+        if (!$this->isDescendant($resolved_head, $resolved_base)) {
+          // NOTE: Since the base commit will have been resolved as the
+          // merge-base of the specified base and the specified HEAD, we can't
+          // easily tell exactly what's wrong with the range.
+
+          // For example, `arc diff HEAD --head HEAD^^^` is invalid because it
+          // is reversed, but resolving the commit "HEAD" will compute its
+          // merge-base with "HEAD^^^", which is "HEAD^^^", so the range will
+          // appear empty.
+
           throw new ArcanistUsageException(
-          "base commit ${base_commit} is not a child of head commit ".
+            pht(
-          "${head_commit}");
+              'The specified commit range is empty, backward or invalid: the '.
+              'base (%s) is not an ancestor of the head (%s). You can not '.
+              'diff an empty or reversed commit range.',
+              $base_commit,
+              $head_commit));
+        }
       }
 
-      $against = csprintf('%s --not %s',
+      $against = csprintf(
-        $this->getHeadCommit(), $this->getBaseCommit());
+        '%s --not %s',
+        $this->getHeadCommit(),
+        $this->getBaseCommit());
     }
 
     // NOTE: Windows escaping of "%" symbols apparently is inherently broken;
@@ -197,9 +221,17 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
           "this repository.");
       }
 
+      if ($this->symbolicHeadCommit === null) {
         $this->setBaseCommitExplanation(
-        "it is the merge-base of '{$symbolic_commit}' and ".
+          "it is the merge-base of the explicitly specified base commit ".
-        "{$this->symbolicHeadCommit}, as you explicitly specified.");
+          "'{$symbolic_commit}' and HEAD.");
+      } else {
+        $this->setBaseCommitExplanation(
+          "it is the merge-base of the explicitly specified base commit ".
+          "'{$symbolic_commit}' and the explicitly specified head ".
+          "commit '{$this->symbolicHeadCommit}'.");
+      }
+
       return trim($merge_base);
     }
 
@@ -331,8 +363,8 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
 
   public function getHeadCommit() {
     if ($this->resolvedHeadCommit === null) {
-      $this->resolvedHeadCommit =
+      $this->resolvedHeadCommit = $this->resolveCommit(
-        $this->resolveCommit($this->symbolicHeadCommit);
+        coalesce($this->symbolicHeadCommit, 'HEAD'));
     }
 
     return $this->resolvedHeadCommit;

src/workflow/ArcanistDiffWorkflow.php

@@ -397,15 +397,19 @@ EOTEXT
       '*' => 'paths',
       'head' => array(
         'param' => 'commit',
-        'help' => "specify the head commit.\n".
+        'help' => pht(
-          "This disables many Arcanist/Phabricator features which depend on ".
+          'Specify the end of the commit range. This disables many '.
-          "having access to the working copy.",
+          'Arcanist/Phabricator features which depend on having access to '.
+          'the working copy.'),
         'supports' => array('git'),
+        'nosupport' => array(
+          'svn' => pht('Subversion does not support commit ranges.'),
+          'hg' => pht('Mercurial does not support --head yet.'),
+        ),
         'conflicts' => array(
           'lintall'   => '--head suppresses lint.',
           'advice'    => '--head suppresses lint.',
         ),
-
       )
     );
 
@@ -447,16 +451,11 @@ EOTEXT
 
       $repo = $this->getRepositoryAPI();
       $head_commit = $this->getArgument('head');
-      $range_supported = $repo->supportsCommitRanges();
+      if ($head_commit !== null) {
-      if ($head_commit) {
-        if (!$range_supported) {
-          throw new ArcanistUsageException('Ranged are not supported');
-        }
-
         $repo->setHeadCommit($head_commit);
       }
 
-      if ($range_supported) {
+      if ($repo->supportsCommitRanges()) {
         $repo->getBaseCommit();
       }
 

src/workflow/ArcanistWhichWorkflow.php

@@ -2,8 +2,6 @@
 
 /**
  * Show which revision or revisions are in the working copy.
- *
- * @group workflow
  */
 final class ArcanistWhichWorkflow extends ArcanistBaseWorkflow {
 
@@ -13,8 +11,8 @@ final class ArcanistWhichWorkflow extends ArcanistBaseWorkflow {
 
   public function getCommandSynopses() {
     return phutil_console_format(<<<EOTEXT
-      **which** (svn)
+      **which** [options] (svn)
-      **which** [commit] (hg, git)
+      **which** [options] [__commit__] (hg, git)
 EOTEXT
       );
   }
@@ -63,7 +61,12 @@ EOTEXT
       ),
       'head' => array(
         'param' => 'commit',
-        'help' => 'specify the head commit.'
+        'help' => pht('Specify the end of the commit range to select.'),
+        'nosupport' => array(
+          'svn' => pht('Subversion does not support commit ranges.'),
+          'hg' => pht('Mercurial does not support --head yet.'),
+        ),
+        'supports' => array('git'),
       ),
       '*' => 'commit',
     );
@@ -89,13 +92,9 @@ EOTEXT
 
     $supports_ranges = $repository_api->supportsCommitRanges();
 
-    if ($this->getArgument('head')) {
-      if ($supports_ranges === false) {
-        throw new Exception('--head is not supported in this VCS');
-      }
-
     $head_commit = $this->getArgument('head');
-      $arg .= " --head {$head_commit}";
+    if ($head_commit !== null) {
+      $arg .= csprintf(' --head %R', $head_commit);
       $repository_api->setHeadCommit($head_commit);
     }
 
@@ -128,22 +127,36 @@ EOTEXT
 
       if ($repository_api instanceof ArcanistGitAPI) {
         $head = $this->getArgument('head', 'HEAD');
-        $command = "git diff {$relative}..{$head}";
+        $command = csprintf('git diff %R', "{$relative}..{$head}");
       } else if ($repository_api instanceof ArcanistMercurialAPI) {
-        $command = "hg diff --rev {$relative}";
+        $command = csprintf(
+          'hg diff --rev %R',
+          hgsprintf('%s', $relative));
       } else {
         throw new Exception('Unknown VCS!');
       }
 
       echo phutil_console_wrap(
         phutil_console_format(
-          "**RELATIVE COMMIT**\n".
+          "**COMMIT RANGE**\n".
           "If you run 'arc diff{$arg}', changes between the commit:\n\n"));
 
       echo  "    {$relative}  {$relative_summary}\n\n";
+
+      if ($head_commit === null) {
+        $will_be_sent = pht(
+          '...and the current working copy state will be sent to '.
+          'Differential, because %s',
+          $explanation);
+      } else {
+        $will_be_sent = pht(
+          '...and "%s" will be sent to Differential, because %s',
+          $head_commit,
+          $explanation);
+      }
+
       echo phutil_console_wrap(
-        "...and the current working copy state will be sent to ".
+        "{$will_be_sent}\n\n".
-        "Differential, because {$explanation}\n\n".
         "You can see the exact changes that will be sent by running ".
         "this command:\n\n".
         "    $ {$command}\n\n".