diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php index 3f24d984..df441210 100644 --- a/src/repository/api/ArcanistGitAPI.php +++ b/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 parent commit SHA. - * @return bool + * @param Child commit SHA. + * @param Parent commit SHA. + * @return bool True if the child is a descendant of the parent. */ private function isDescendant($child, $parent) { - list($common_ancestor) = - $this->execxLocal('merge-base %s %s', $child, $parent); + list($common_ancestor) = $this->execxLocal( + '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 - // user expectations even though it is not actually the diff range. + // not reachable from the base commit; this is consistent with user + // 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. - $base_commit = $this->getBaseCommit(); - $head_commit = $this->getHeadCommit(); - if ($this->isDescendant($head_commit, $base_commit) === false) { - throw new ArcanistUsageException( - "base commit ${base_commit} is not a child of head commit ". - "${head_commit}"); + + if ($this->symbolicHeadCommit !== null) { + $base_commit = $this->getBaseCommit(); + $resolved_base = $this->resolveCommit($base_commit); + + $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( + pht( + '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', - $this->getHeadCommit(), $this->getBaseCommit()); + $against = csprintf( + '%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."); } - $this->setBaseCommitExplanation( - "it is the merge-base of '{$symbolic_commit}' and ". - "{$this->symbolicHeadCommit}, as you explicitly specified."); + if ($this->symbolicHeadCommit === null) { + $this->setBaseCommitExplanation( + "it is the merge-base of the explicitly specified base commit ". + "'{$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->resolveCommit($this->symbolicHeadCommit); + $this->resolvedHeadCommit = $this->resolveCommit( + coalesce($this->symbolicHeadCommit, 'HEAD')); } return $this->resolvedHeadCommit; diff --git a/src/workflow/ArcanistDiffWorkflow.php b/src/workflow/ArcanistDiffWorkflow.php index fead0459..699ffad1 100644 --- a/src/workflow/ArcanistDiffWorkflow.php +++ b/src/workflow/ArcanistDiffWorkflow.php @@ -397,15 +397,19 @@ EOTEXT '*' => 'paths', 'head' => array( 'param' => 'commit', - 'help' => "specify the head commit.\n". - "This disables many Arcanist/Phabricator features which depend on ". - "having access to the working copy.", + 'help' => pht( + 'Specify the end of the commit range. This disables many '. + '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) { - if (!$range_supported) { - throw new ArcanistUsageException('Ranged are not supported'); - } - + if ($head_commit !== null) { $repo->setHeadCommit($head_commit); } - if ($range_supported) { + if ($repo->supportsCommitRanges()) { $repo->getBaseCommit(); } diff --git a/src/workflow/ArcanistWhichWorkflow.php b/src/workflow/ArcanistWhichWorkflow.php index 1160d4e8..9f8a62a8 100644 --- a/src/workflow/ArcanistWhichWorkflow.php +++ b/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(<< 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}"; + $head_commit = $this->getArgument('head'); + 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 ". - "Differential, because {$explanation}\n\n". + "{$will_be_sent}\n\n". "You can see the exact changes that will be sent by running ". "this command:\n\n". " $ {$command}\n\n".