diff --git a/src/land/engine/ArcanistMercurialLandEngine.php b/src/land/engine/ArcanistMercurialLandEngine.php --- a/src/land/engine/ArcanistMercurialLandEngine.php +++ b/src/land/engine/ArcanistMercurialLandEngine.php @@ -803,8 +803,8 @@ // descendants and the min commit has no ancestors. The min/max terms are // used in a topological sense as chronological terms for commits can be // misleading or incorrect in certain situations. - $max_commit = last($commits)->getHash(); $min_commit = head($commits)->getHash(); + $max_commit = last($commits)->getHash(); $revision_ref = $set->getRevisionRef(); $commit_message = $revision_ref->getCommitMessage(); diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php --- a/src/repository/api/ArcanistGitAPI.php +++ b/src/repository/api/ArcanistGitAPI.php @@ -826,7 +826,7 @@ return $this; } - public function amendCommit($message = null) { + public function amendCommit($workflow, $message = null) { if ($message === null) { $this->execxLocal('commit --amend --allow-empty -C HEAD'); } else { diff --git a/src/repository/api/ArcanistMercurialAPI.php b/src/repository/api/ArcanistMercurialAPI.php --- a/src/repository/api/ArcanistMercurialAPI.php +++ b/src/repository/api/ArcanistMercurialAPI.php @@ -658,35 +658,175 @@ public function doCommit($message) { $tmp_file = new TempFile(); Filesystem::writeFile($tmp_file, $message); - $this->execxLocal('commit -l %s', $tmp_file); + $this->execxLocal('commit --logfile %s', $tmp_file); $this->reloadWorkingCopy(); } - public function amendCommit($message = null) { + public function amendCommit($workflow, $message = null) { + $path_statuses = $this->buildUncommittedStatus(); + if ($message === null) { + if (empty($path_statuses)) { + // If there are no changes to the working directory and the message is + // not being changed then there's nothing to amend. + return; + } + $message = $this->getCommitMessage('.'); } $tmp_file = new TempFile(); Filesystem::writeFile($tmp_file, $message); + if ($this->getMercurialFeature('evolve')) { + $this->execxLocal('amend --logfile %s --', $tmp_file); + try { + $this->execxLocal('evolve --all --'); + } catch (CommandException $ex) { + $this->execxLocal('evolve --abort --'); + throw $ex; + } + $this->reloadWorkingCopy(); + return; + } + + // Get the child nodes of the current changeset. + list($children) = $this->execxLocal( + 'log --template %s --rev %s --', + '{node} ', + 'children(.)'); + $child_nodes = array_filter(explode(' ', $children)); + + // For a head commit we can simply use `commit --amend` for both new commit + // message and amending changes from the working directory. + if (empty($child_nodes)) { + try { + $this->execxLocal('commit --amend --logfile %s --', $tmp_file); + } catch (CommandException $ex) { + if (preg_match('/nothing changed/', $ex->getStdout())) { + // NOTE: Mercurial considers it an error to make a no-op amend. + // Although we generally defer to the underlying VCS to dictate + // behavior, this one seems a little goofy, and we use amend as part + // of various workflows under the assumption that no-op amends are + // fine. If this amend failed because it's a no-op, just continue. + } else { + throw $ex; + } + } + } else { + $this->amendNonHeadCommit( + $workflow, $path_statuses, $child_nodes, $tmp_file); + } + + $this->reloadWorkingCopy(); + } + + /** + * Amends a non-head commit with a new message and file changes. This + * strategy is for Mercurial repositories without the evolve extension. + * + * 1. Stash away any uncommitted changes, which at this point should only be + * changes applied from 'arc lint', or during the initial prompt from + * 'arc diff' whether changes should be amended. + * 2. Rebase the current changeset onto the same parent but with the new + * commit message, keeping the original commit. + * 3. Unstash the changes from step 1 onto the new commit. + * 4. Amend the unstashed changes to the new commit. + * 5. For each branch from the original commit, rebase onto the new commit, + * removing the original branch. Note that there is potential for this to + * cause a conflict but this is something the user has to address. + * 6. Strip the original commit. + * + * @param workflow The workflow this amend is being made for. + * @param array A map of uncommitted file changes. This hould be the + * result of calling `buildUncommittedStatus()`. + * @param array The list of child changesets off the original commit. + * @param file The file containing the new commit message. + */ + private function amendNonHeadCommit( + $workflow, $path_statuses, $child_nodes, $tmp_file) { + + $hg_analyzer = PhutilBinaryAnalyzer::getForBinary('hg'); + if ($hg_analyzer->isMercurialTemplatePnodeAvailable()) { + $template = '{p1.node} {node}'; + } else { + $template = '{p1node} {node}'; + } + + list($current_and_parent) = $this->execxLocal( + 'log --template %s --rev . --', + $template); + $nodes = explode(' ', $current_and_parent); + $parent = $nodes[0]; + $current = $nodes[1]; + + // If there are file changes which will be amended we first need to stash + // them away to get a clean working directory so we can rebase to make the + // new commit. + $stash_name = null; + $local_state = $this->newLocalState() + ->setWorkflow($workflow); + if (!empty($path_statuses)) { + $stash_name = $local_state->saveStash(); + } + try { + // Mercurial doesn't allow rebasing with a new commit message unless + // the `--collapse` flag is also specified... It shouldn't matter + // since we're only rebasing a single commit here. $this->execxLocal( - 'commit --amend -l %s', + 'rebase --dest %s --rev . --keep --collapse --logfile %s --', + $parent, $tmp_file); } catch (CommandException $ex) { - if (preg_match('/nothing changed/', $ex->getStdout())) { - // NOTE: Mercurial considers it an error to make a no-op amend. Although - // we generally defer to the underlying VCS to dictate behavior, this - // one seems a little goofy, and we use amend as part of various - // workflows under the assumption that no-op amends are fine. If this - // amend failed because it's a no-op, just continue. - } else { - throw $ex; + $this->execxLocal('rebase --abort --'); + + // Unstash changes after aborting the rebase and before throwing error. + if ($stash_name !== null) { + $local_state->restoreStash($stash_name); } + + throw $ex; } - $this->reloadWorkingCopy(); + list($new_commit) = $this->execxLocal( + 'log --rev tip --template %s --', + '{node}'); + + // Restore any stashed changes and amend the new commit + if ($stash_name !== null) { + $this->execxLocal('update --rev %s --', $new_commit); + $local_state->restoreStash($stash_name); + + // Pass in the commit message again, even though the commit has the right + // commit message already due to the prior rebase. If we don't specify a + // commit message here then the user's editor will suddenly appear and + // ruin everything we've worked so hard to set up here. + $this->execxLocal('commit --amend --logfile %s --', $tmp_file); + + // Amending the commit will result in a new hash + list($new_commit) = $this->execxLocal( + 'log --rev tip --template %s --', + '{node}'); + } + + try { + foreach ($child_nodes as $child) { + // descendants(rev) will also include rev itself which is why this + // can't use a single rebase of descendants($current). + $revset = hgsprintf('descendants(%s)', $child); + $this->execxLocal( + 'rebase --dest %s --rev %s --', + $new_commit, + $revset); + } + } catch (CommandException $ex) { + $this->execxLocal('rebase --abort --'); + throw $ex; + } + + $this->execxLocal('--config extensions.strip= strip --rev %s --', + $current); } public function getCommitSummary($commit) { diff --git a/src/repository/api/ArcanistRepositoryAPI.php b/src/repository/api/ArcanistRepositoryAPI.php --- a/src/repository/api/ArcanistRepositoryAPI.php +++ b/src/repository/api/ArcanistRepositoryAPI.php @@ -366,7 +366,7 @@ throw new ArcanistCapabilityNotSupportedException($this); } - public function amendCommit($message = null) { + public function amendCommit($workflow, $message = null) { throw new ArcanistCapabilityNotSupportedException($this); } diff --git a/src/repository/state/ArcanistGitLocalState.php b/src/repository/state/ArcanistGitLocalState.php --- a/src/repository/state/ArcanistGitLocalState.php +++ b/src/repository/state/ArcanistGitLocalState.php @@ -120,7 +120,7 @@ ); } - protected function saveStash() { + public function saveStash() { $api = $this->getRepositoryAPI(); // NOTE: We'd prefer to "git stash create" here, because using "push" @@ -140,7 +140,7 @@ return true; } - protected function restoreStash($stash_ref) { + public function restoreStash($stash_ref) { $api = $this->getRepositoryAPI(); $log = $this->getWorkflow()->getLogEngine(); diff --git a/src/repository/state/ArcanistMercurialLocalState.php b/src/repository/state/ArcanistMercurialLocalState.php --- a/src/repository/state/ArcanistMercurialLocalState.php +++ b/src/repository/state/ArcanistMercurialLocalState.php @@ -152,7 +152,7 @@ return $commands; } - protected function saveStash() { + public function saveStash() { $api = $this->getRepositoryAPI(); $log = $this->getWorkflow()->getLogEngine(); @@ -171,7 +171,7 @@ return $stash_ref; } - protected function restoreStash($stash_ref) { + public function restoreStash($stash_ref) { $api = $this->getRepositoryAPI(); $log = $this->getWorkflow()->getLogEngine(); diff --git a/src/repository/state/ArcanistRepositoryLocalState.php b/src/repository/state/ArcanistRepositoryLocalState.php --- a/src/repository/state/ArcanistRepositoryLocalState.php +++ b/src/repository/state/ArcanistRepositoryLocalState.php @@ -192,11 +192,28 @@ return false; } - protected function saveStash() { + /** + * Stash uncommitted changes temporarily. Use {@method:restoreStash()} to + * bring these changes back. + * + * Note that on Git repositories the stash acts as a stack, so saving the + * stash must match appropriately to restoring the stash. + * + * @return wild On Git this returns true, on Mercurial this returns a name + * (string) which references the stash that was made. This name + * should later be passed to {@method:restoreStash()}. + */ + public function saveStash() { throw new PhutilMethodNotImplementedException(); } - protected function restoreStash($ref) { + /** + * Restores changes that were previously stashed by {@method:saveStash()}. + * + * @param wild On Git this parameter is unused, on Mercurial this should be + * the name (string) returned from {@method:saveStash()}. + */ + public function restoreStash($ref) { throw new PhutilMethodNotImplementedException(); } diff --git a/src/workflow/ArcanistAmendWorkflow.php b/src/workflow/ArcanistAmendWorkflow.php --- a/src/workflow/ArcanistAmendWorkflow.php +++ b/src/workflow/ArcanistAmendWorkflow.php @@ -125,7 +125,7 @@ "%B\n", $message); } else { - $repository_api->amendCommit($message); + $repository_api->amendCommit($this, $message); } return 0; diff --git a/src/workflow/ArcanistDiffWorkflow.php b/src/workflow/ArcanistDiffWorkflow.php --- a/src/workflow/ArcanistDiffWorkflow.php +++ b/src/workflow/ArcanistDiffWorkflow.php @@ -542,7 +542,7 @@ $repository_api = $this->getRepositoryAPI(); if ($repository_api->supportsAmend()) { echo pht('Updating commit message...')."\n"; - $repository_api->amendCommit($revised_message); + $repository_api->amendCommit($this, $revised_message); } else { echo pht( 'Commit message was not amended. Amending commit message is '. @@ -1502,7 +1502,7 @@ if ($should_amend) { $wrote = (rtrim($old_message) != rtrim($template)); if ($wrote) { - $repository_api->amendCommit($template); + $repository_api->amendCommit($this, $template); $where = pht('commit message'); } } else { diff --git a/src/workflow/ArcanistLintWorkflow.php b/src/workflow/ArcanistLintWorkflow.php --- a/src/workflow/ArcanistLintWorkflow.php +++ b/src/workflow/ArcanistLintWorkflow.php @@ -344,7 +344,7 @@ $repository_api->execxLocal('add -u'); } - $repository_api->amendCommit(); + $repository_api->amendCommit($this); } else { throw new ArcanistUsageException( pht( diff --git a/src/workflow/ArcanistWorkflow.php b/src/workflow/ArcanistWorkflow.php --- a/src/workflow/ArcanistWorkflow.php +++ b/src/workflow/ArcanistWorkflow.php @@ -1217,7 +1217,7 @@ if ($should_commit) { if ($this->getShouldAmend()) { $commit = head($api->getLocalCommitInformation()); - $api->amendCommit($commit['message']); + $api->amendCommit($this, $commit['message']); } else if ($api->supportsLocalCommits()) { $template = sprintf( "\n\n# %s\n#\n# %s\n#\n",