From 31a54d9b4a31e33fcbcdfe84ae10be42e9a250bb Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 19 Mar 2012 19:17:10 -0700 Subject: [PATCH] When a mercurial working copy has uncommitted changes, prompt to include them in "arc diff" Summary: See task. Allow mercurial users to diff with uncommitted changes. - By default, commit range is merge-base of `hg outgoing` to `.` (dirstate). - You can get JUST dirstate with `arc diff tip` or similar. - This ended up being a giant mess various other changes to deal with empty `hg outgoing` and empty dirstate. Test Plan: Diffed with uncommitted changes, got sensible prompts and results. Reviewers: Makinde, btrahan Reviewed By: btrahan CC: aran, epriestley Maniphest Tasks: T998 Differential Revision: https://secure.phabricator.com/D1954 --- src/__phutil_library_map__.php | 3 + .../ArcanistUncommittedChangesException.php | 21 +++++ .../usage/uncommittedchanges/__init__.php | 12 +++ .../api/mercurial/ArcanistMercurialAPI.php | 77 +++++++++++++++---- src/repository/api/mercurial/__init__.php | 1 - src/workflow/base/ArcanistBaseWorkflow.php | 6 +- src/workflow/base/__init__.php | 1 + src/workflow/diff/ArcanistDiffWorkflow.php | 38 ++++++++- 8 files changed, 137 insertions(+), 22 deletions(-) create mode 100644 src/exception/usage/uncommittedchanges/ArcanistUncommittedChangesException.php create mode 100644 src/exception/usage/uncommittedchanges/__init__.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index b695ff7e..5d37794b 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -54,6 +54,7 @@ phutil_register_library_map(array( 'ArcanistLicenseLinter' => 'lint/linter/license', 'ArcanistLintEngine' => 'lint/engine/base', 'ArcanistLintJSONRenderer' => 'lint/renderer', + 'ArcanistLintLikeCompilerRenderer' => 'lint/renderer', 'ArcanistLintMessage' => 'lint/message', 'ArcanistLintPatcher' => 'lint/patcher', 'ArcanistLintRenderer' => 'lint/renderer', @@ -91,6 +92,7 @@ phutil_register_library_map(array( 'ArcanistSvnHookPreCommitWorkflow' => 'workflow/svn-hook-pre-commit', 'ArcanistTextLinter' => 'lint/linter/text', 'ArcanistTextLinterTestCase' => 'lint/linter/text/__tests__', + 'ArcanistUncommittedChangesException' => 'exception/usage/uncommittedchanges', 'ArcanistUnitTestResult' => 'unit/result', 'ArcanistUnitWorkflow' => 'workflow/unit', 'ArcanistUploadWorkflow' => 'workflow/upload', @@ -168,6 +170,7 @@ phutil_register_library_map(array( 'ArcanistSvnHookPreCommitWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistTextLinter' => 'ArcanistLinter', 'ArcanistTextLinterTestCase' => 'ArcanistLinterTestCase', + 'ArcanistUncommittedChangesException' => 'ArcanistUsageException', 'ArcanistUnitWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistUploadWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistUserAbortException' => 'ArcanistUsageException', diff --git a/src/exception/usage/uncommittedchanges/ArcanistUncommittedChangesException.php b/src/exception/usage/uncommittedchanges/ArcanistUncommittedChangesException.php new file mode 100644 index 00000000..433751be --- /dev/null +++ b/src/exception/usage/uncommittedchanges/ArcanistUncommittedChangesException.php @@ -0,0 +1,21 @@ +execxLocal( - 'log -l 1 --template %s -r %s', - '{node}\\n', - $this->getRelativeCommit()); - return rtrim($stdout, "\n"); + return $this->getCanonicalRevisionName($this->getRelativeCommit()); } public function getCanonicalRevisionName($string) { - throw new ArcanistCapabilityNotSupportedException($this); + list($stdout) = $this->execxLocal( + 'log -l 1 --template %s -r %s --', + '{node}', + $this->getRelativeCommit()); + + return $stdout; } public function getSourceControlPath() { @@ -75,28 +77,40 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { } public function setRelativeCommit($commit) { - if (!$this->hasLocalCommit($commit)) { + try { + $commit = $this->getCanonicalRevisionName($commit); + } catch (Exception $ex) { throw new ArcanistUsageException( "Commit '{$commit}' is not a valid Mercurial commit identifier."); } + $this->relativeCommit = $commit; return $this; } public function getRelativeCommit() { if (empty($this->relativeCommit)) { - list($stdout) = $this->execxLocal( + list($err, $stdout) = $this->execManualLocal( 'outgoing --branch `hg branch` --style default'); - $logs = ArcanistMercurialParser::parseMercurialLog($stdout); - if (!count($logs)) { - throw new ArcanistUsageException("You have no outgoing changes!"); + + if (!$err) { + $logs = ArcanistMercurialParser::parseMercurialLog($stdout); + } else { + // Mercurial (in some versions?) raises an error when there's nothing + // outgoing. + $logs = array(); + } + + if (!$logs) { + // In Mercurial, we support operations against uncommitted changes. + return $this->getWorkingCopyRevision(); } $outgoing_revs = ipull($logs, 'rev'); // This is essentially an implementation of a theoretical `hg merge-base` // command. - $against = 'tip'; + $against = $this->getWorkingCopyRevision(); while (true) { // NOTE: The "^" and "~" syntaxes were only added in hg 1.9, which is // new as of July 2011, so do this in a compatible way. Also, "hg log" @@ -128,7 +142,7 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { } } - $this->relativeCommit = $against; + $this->setRelativeCommit($against); } return $this->relativeCommit; } @@ -205,6 +219,12 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { // just take the entire diff and parse it to figure out what's changed. $diff = $this->getFullMercurialDiff(); + + if (!$diff) { + $this->status = array(); + return $this->status; + } + $parser = new ArcanistDiffParser(); $changes = $parser->parseDiff($diff); @@ -273,7 +293,7 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { 'diff %C --rev %s --rev %s -- %s', $options, $this->getRelativeCommit(), - $this->getWorkingCopyRevision(), + $this->getDiffToRevision(), $path); return $stdout; @@ -286,7 +306,7 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { 'diff %C --rev %s --rev %s --', $options, $this->getRelativeCommit(), - $this->getWorkingCopyRevision()); + $this->getDiffToRevision()); return $stdout; } @@ -341,8 +361,12 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { } public function hasLocalCommit($commit) { - list($err) = $this->execManualLocal('id -ir %s', $commit); - return !$err; + try { + $this->getCanonicalRevisionName($commit); + return true; + } catch (Exception $ex) { + return false; + } } public function parseRelativeLocalCommit(array $argv) { @@ -434,4 +458,23 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { $this->execxLocal('up'); } + public function setIncludeDirectoryStateInDiffs($include) { + $this->includeDirectoryStateInDiffs = $include; + return $this; + } + + private function getDiffToRevision() { + + // Clear status cache since it's now bogus. + $this->status = null; + + if ($this->includeDirectoryStateInDiffs) { + // This is a magic Mercurial revision name which means "current + // directory state"; see lookup() in localrepo.py. + return '.'; + } else { + return $this->getWorkingCopyRevision(); + } + } + } diff --git a/src/repository/api/mercurial/__init__.php b/src/repository/api/mercurial/__init__.php index 5b0e0ef3..d4aa19c1 100644 --- a/src/repository/api/mercurial/__init__.php +++ b/src/repository/api/mercurial/__init__.php @@ -11,7 +11,6 @@ phutil_require_module('arcanist', 'parser/diff'); phutil_require_module('arcanist', 'parser/diff/changetype'); phutil_require_module('arcanist', 'repository/api/base'); phutil_require_module('arcanist', 'repository/parser/mercurial'); -phutil_require_module('arcanist', 'workflow/exception/notsupported'); phutil_require_module('phutil', 'future/exec'); phutil_require_module('phutil', 'utils'); diff --git a/src/workflow/base/ArcanistBaseWorkflow.php b/src/workflow/base/ArcanistBaseWorkflow.php index f5f9cecc..74ce899e 100644 --- a/src/workflow/base/ArcanistBaseWorkflow.php +++ b/src/workflow/base/ArcanistBaseWorkflow.php @@ -687,9 +687,9 @@ abstract class ArcanistBaseWorkflow { $uncommitted = $api->getUncommittedChanges(); if ($uncommitted) { - throw new ArcanistUsageException( - "You have uncommitted changes in this branch. Commit (or revert) them ". - "before proceeding.\n\n". + throw new ArcanistUncommittedChangesException( + "You have uncommitted changes in this working copy. Commit (or ". + "revert) them before proceeding.\n\n". $working_copy_desc. " Uncommitted changes in working copy\n". " ".implode("\n ", $uncommitted)."\n"); diff --git a/src/workflow/base/__init__.php b/src/workflow/base/__init__.php index f409b1b0..9a534dfe 100644 --- a/src/workflow/base/__init__.php +++ b/src/workflow/base/__init__.php @@ -7,6 +7,7 @@ phutil_require_module('arcanist', 'exception/usage'); +phutil_require_module('arcanist', 'exception/usage/uncommittedchanges'); phutil_require_module('arcanist', 'exception/usage/userabort'); phutil_require_module('arcanist', 'parser/bundle'); phutil_require_module('arcanist', 'parser/diff'); diff --git a/src/workflow/diff/ArcanistDiffWorkflow.php b/src/workflow/diff/ArcanistDiffWorkflow.php index 930dbc63..4041aef8 100644 --- a/src/workflow/diff/ArcanistDiffWorkflow.php +++ b/src/workflow/diff/ArcanistDiffWorkflow.php @@ -290,6 +290,12 @@ EOTEXT 'no-amend' => array( 'help' => 'Never amend commits in the working copy.', ), + 'uncommitted' => array( + 'help' => 'Include uncommitted changes without prompting.', + 'supports' => array( + 'hg', + ), + ), '*' => 'paths', ); } @@ -501,7 +507,33 @@ EOTEXT } if ($this->requiresWorkingCopy()) { - $this->requireCleanWorkingCopy(); + try { + $this->requireCleanWorkingCopy(); + } catch (ArcanistUncommittedChangesException $ex) { + if ($repository_api instanceof ArcanistMercurialAPI) { + + // Some Mercurial users prefer to use it like SVN, where they don't + // commit changes before sending them for review. This would be a + // pretty bad workflow in Git, but Mercurial users are significantly + // more expert at change management. + + $use_dirty_changes = false; + if ($this->getArgument('uncommitted')) { + // OK. + } else { + $ok = phutil_console_confirm( + "You have uncommitted changes in your working copy. You can ". + "include them in the diff, or abort and deal with them. (Use ". + "'--uncommitted' to include them and skip this prompt.) ". + "Do you want to include uncommitted changes in the diff?"); + if (!$ok) { + throw $ex; + } + } + + $repository_api->setIncludeDirectoryStateInDiffs(true); + } + } } } @@ -693,6 +725,10 @@ EOTEXT $changes = $parser->parseDiff($diff); } else if ($repository_api instanceof ArcanistMercurialAPI) { $diff = $repository_api->getFullMercurialDiff(); + if (!strlen($diff)) { + throw new ArcanistUsageException( + "No changes found. (Did you specify the wrong commit range?)"); + } $changes = $parser->parseDiff($diff); } else { throw new Exception("Repository API is not supported.");