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 @@ -6,6 +6,8 @@ private $ontoBranchMarker; private $ontoMarkers; + private $rebasedActiveCommit; + protected function getDefaultSymbols() { $api = $this->getRepositoryAPI(); $log = $this->getLogEngine(); @@ -698,8 +700,11 @@ $commit_map = array(); foreach ($symbols as $symbol) { $symbol_commit = $symbol->getCommit(); - $template = '{node}-{parents}-'; + $template = '{node}-{parents % \'{node} \'}-{desc|firstline}\\n'; + // The returned array of commits is expected to be ordered by newest + // to oldest commits. Use 'reverse()' in the template so the output + // is ordered with newest commit as the first line. if ($into_commit === null) { list($commits) = $api->execxLocal( 'log --rev %s --template %s --', @@ -789,8 +794,11 @@ $commits = $set->getCommits(); - $min_commit = last($commits)->getHash(); - $max_commit = head($commits)->getHash(); + // confirmCommits() reverses the order of the commits as they're ordered + // above in selectCommits(), now the head of the list is the oldest and + // last is the newest. + $newest_commit = last($commits)->getHash(); + $oldest_commit = head($commits)->getHash(); $revision_ref = $set->getRevisionRef(); $commit_message = $revision_ref->getCommitMessage(); @@ -812,7 +820,7 @@ $argv[] = hgsprintf('%s', $into_commit); $argv[] = '--rev'; - $argv[] = hgsprintf('%s..%s', $min_commit, $max_commit); + $argv[] = hgsprintf('%s..%s', $oldest_commit, $newest_commit); $argv[] = '--logfile'; $argv[] = '-'; @@ -825,8 +833,9 @@ $future->resolvex(); } catch (CommandException $ex) { - // TODO - // $api->execManualLocal('rebase --abort'); + // Aborting the rebase should restore the same state prior to running the + // rebase command. + $api->execManualLocal('rebase --abort'); throw $ex; } @@ -855,13 +864,21 @@ list($stdout) = $api->execxLocal('log --rev tip --template %s', '{node}'); $new_cursor = trim($stdout); + // If any of the commits that were rebased was the active commit before the + // workflow started, track the new commit so it can be used as the working + // directory after the land has succeeded. + if (isset($obsolete_map[$this->getLocalState()->getLocalCommit()])) { + $this->rebasedActiveCommit = $new_cursor; + } + return $new_cursor; } protected function pushChange($into_commit) { $api = $this->getRepositoryAPI(); - list($head, $body, $tail) = $this->newPushCommands($into_commit); + list($head, $body, $tail_pass, $tail_fail) = $this->newPushCommands( + $into_commit); foreach ($head as $command) { $api->execxLocal('%Ls', $command); @@ -876,10 +893,20 @@ 'Push failed! Fix the error and run "arc land" again.')); } } - } finally { - foreach ($tail as $command) { + + foreach ($tail_pass as $command) { + $api->execxLocal('%Ls', $command); + } + } catch (Exception $ex) { + foreach ($tail_fail as $command) { $api->execxLocal('%Ls', $command); } + throw $ex; + } catch (Throwable $ex) { + foreach ($tail_fail as $command) { + $api->execxLocal('%Ls', $command); + } + throw $ex; } } @@ -888,7 +915,8 @@ $head_commands = array(); $body_commands = array(); - $tail_commands = array(); + $tail_pass_commands = array(); + $tail_fail_commands = array(); $bookmarks = array(); foreach ($this->ontoMarkers as $onto_marker) { @@ -902,7 +930,8 @@ // to the merge commit. (There doesn't seem to be any way to specify // "push commit X as bookmark Y" in Mercurial.) - $restore = array(); + $delete_bookmarks = array(); + $restore_bookmarks = array(); if ($bookmarks) { $markers = $api->newMarkerRefQuery() ->withNames(mpull($bookmarks, 'getName')) @@ -917,6 +946,7 @@ $new_position = $into_commit; if ($old_position === $new_position) { + $delete_bookmarks[] = $bookmark_name; continue; } @@ -934,7 +964,11 @@ hgsprintf('%s', $new_position), $bookmark_name); - $restore[$bookmark_name] = $old_position; + if ($old_position === null) { + $delete_bookmarks[] = $bookmark_name; + } else { + $restore_bookmarks[$bookmark_name] = $old_position; + } } } @@ -961,30 +995,68 @@ $body_commands[] = $argv; - // Finally, restore the bookmarks. + // Finally, clean up the bookmarks. + + // If the onto-marker didn't exist locally when landing it will be pulled + // down and created above. To restore the original state these bookmarks + // are tracked so we can delete them when done, regardless of whether the + // land succeeded. + // NOTE: This doesn't end up having working because any calls to + // newMarkerRefQuery() prior to this function will have pulled down + // remote bookmarks. To properly support this the state of only the + // local bookmarks needs to be captured prior to any other marker ref + // queries. In practice pulling down the remote markers which were + // landed onto and leaving them here is probably fine. + if ($delete_bookmarks) { + // Delete all bookmarks in a single command rather than individually. + $tail = array( + 'bookmark', + '--delete', + '--', + ); + foreach ($delete_bookmarks as $bookmark_name) { + $tail[] = $bookmark_name; + } - foreach ($restore as $bookmark_name => $old_position) { - $tail = array(); - $tail[] = 'bookmark'; + $tail_pass_commands[] = $tail; + $tail_fail_commands[] = $tail; + } - if ($old_position === null) { - $tail[] = '--delete'; - } else { - $tail[] = '--force'; - $tail[] = '--rev'; - $tail[] = hgsprintf('%s', $api->getDisplayHash($old_position)); + if ($restore_bookmarks) { + // Instead of restoring the previous state, assume landing onto bookmarks + // also updates those bookmarks in the remote. After pushing, pull the + // latest state of these bookmarks. Mercurial allows pulling multiple + // bookmarks in a single pull command which will be faster than pulling + // them from a remote individually. + $tail = array( + 'pull', + ); + + foreach ($restore_bookmarks as $bookmark_name => $old_position) { + $tail[] = '--bookmark'; + $tail[] = $bookmark_name; + + // In the failure case restore the state of the bookmark. Mercurial + // does not provide a way to move multiple bookmarks in a single + // command however these commands do not involve the remote. + $tail_fail_commands[] = array( + 'bookmark', + '--force', + '--rev', + hgsprintf('%s', $api->getDisplayHash($old_position)), + ); } - $tail[] = '--'; - $tail[] = $bookmark_name; - - $tail_commands[] = $tail; + if ($tail) { + $tail_pass_commands[] = $tail; + } } return array( $head_commands, $body_commands, - $tail_commands, + $tail_pass_commands, + $tail_fail_commands, ); } @@ -1020,7 +1092,9 @@ $child_hash, $new_commit); } catch (CommandException $ex) { - // TODO: Recover state. + // Aborting the rebase should restore the same state prior to running + // the rebase command. + $api->execManualLocal('rebase --abort'); throw $ex; } } @@ -1040,6 +1114,8 @@ $revs = array(); $obsolete_map = array(); + $using_evolve = $api->getMercurialFeature('evolve'); + // We've rebased all descendants already, so we can safely delete all // of these commits. @@ -1047,10 +1123,17 @@ foreach ($sets as $set) { $commits = $set->getCommits(); - $min_commit = head($commits)->getHash(); - $max_commit = last($commits)->getHash(); + $oldest_commit = head($commits)->getHash(); + $newest_commit = last($commits)->getHash(); - $revs[] = hgsprintf('%s::%s', $min_commit, $max_commit); + if ($using_evolve) { + // If non-head series of commits are rebased while the evolve extension + // is in use, the rebase leaves behind the entire series of descendants + // in which case the entire chain needs removed, not just a section. + $revs[] = hgsprintf('%s::', $oldest_commit); + } else { + $revs[] = hgsprintf('%s::%s', $oldest_commit, $newest_commit); + } foreach ($commits as $commit) { $obsolete_map[$commit->getHash()] = true; @@ -1071,10 +1154,7 @@ // bookmarks which point at these now-obsoleted commits. $bookmark_refs = $api->newMarkerRefQuery() - ->withMarkerTypes( - array( - ArcanistMarkerRef::TYPE_BOOKMARK, - )) + ->withMarkerTypes(array(ArcanistMarkerRef::TYPE_BOOKMARK)) ->execute(); foreach ($bookmark_refs as $bookmark_ref) { $bookmark_hash = $bookmark_ref->getCommitHash(); @@ -1093,7 +1173,7 @@ $bookmark_name); } - if ($api->getMercurialFeature('evolve')) { + if ($using_evolve) { $api->execxLocal( 'prune --rev %s', $rev_set); @@ -1108,7 +1188,54 @@ $into_commit, ArcanistRepositoryLocalState $state) { - // TODO: For now, just leave users wherever they ended up. + $api = $this->getRepositoryAPI(); + + // If the starting working state was not part of land process just update + // to that original working state. + if (!$this->rebasedActiveCommit) { + $update_marker = $this->getLocalState()->getLocalCommit(); + if ($this->getLocalState()->getLocalBookmark() !== null) { + $update_marker = $this->getLocalState()->getLocalBookmark(); + } + + $api->execxLocal( + 'update -- %s', + $update_marker); + + $state->discardLocalState(); + return; + } + + // If the working state was landed into multiple destinations then the + // resulting working state is ambiguous. + if (count($this->ontoMarkers) != 1) { + $state->discardLocalState(); + return; + } + + // Get the current state of bookmarks + $bookmark_refs = $api->newMarkerRefQuery() + ->withMarkerTypes(array(ArcanistMarkerRef::TYPE_BOOKMARK)) + ->execute(); + + $update_marker = $this->rebasedActiveCommit; + + // Find any bookmarks which exist on the commit which is the result of the + // starting working directory's rebase. If any of those bookmarks are also + // the destination marker then we use that bookmark as the update in order + // for it to become active. + $onto_marker = $this->ontoMarkers[0]->getName(); + foreach ($bookmark_refs as $bookmark_ref) { + if ($bookmark_ref->getCommitHash() == $this->rebasedActiveCommit && + $bookmark_ref->getName() == $onto_marker) { + $update_marker = $onto_marker; + break; + } + } + + $api->execxLocal( + 'update -- %s', + $update_marker); $state->discardLocalState(); } @@ -1120,8 +1247,9 @@ $message = pht( 'Holding changes locally, they have not been pushed.'); - list($head, $body, $tail) = $this->newPushCommands($into_commit); - $commands = array_merge($head, $body, $tail); + list($head, $body, $tail_pass, $tail_fail) = $this->newPushCommands( + $into_commit); + $commands = array_merge($head, $body, $tail_pass); echo tsprintf( "\n%!\n%s\n\n", diff --git a/src/query/ArcanistMercurialWorkingCopyRevisionHardpointQuery.php b/src/query/ArcanistMercurialWorkingCopyRevisionHardpointQuery.php --- a/src/query/ArcanistMercurialWorkingCopyRevisionHardpointQuery.php +++ b/src/query/ArcanistMercurialWorkingCopyRevisionHardpointQuery.php @@ -22,6 +22,14 @@ // TODO: This has a lot in common with the Git query in the same role. + // TODO: In Mercurial if commits are rebased or changed locally they are + // given brand new commit hashes, resulting in this query not being + // able to identify revisions associated with the commit. This can + // happen after landing a non-tip revision in a chain of dependent + // revisions. This can be confusing as `arc which` will still be able + // to identify the right revision by using the `base` ruleset. Maybe + // this could re-use the same `base` ruleset logic as a fallback? + $hashes = array(); $map = array(); foreach ($refs as $ref_key => $ref) { 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 @@ -7,6 +7,14 @@ private $localBranch; private $localBookmark; + public function getLocalCommit() { + return $this->localCommit; + } + + public function getLocalBookmark() { + return $this->localBookmark; + } + protected function executeSaveLocalState() { $api = $this->getRepositoryAPI(); $log = $this->getWorkflow()->getLogEngine();