diff --git a/src/land/engine/ArcanistLandEngine.php b/src/land/engine/ArcanistLandEngine.php --- a/src/land/engine/ArcanistLandEngine.php +++ b/src/land/engine/ArcanistLandEngine.php @@ -1257,53 +1257,55 @@ $should_push = $is_last; } - if ($should_push) { - try { - $this->pushChange($into_commit); - $this->setHasUnpushedChanges(false); - } catch (ArcanistLandPushFailureException $ex) { - - // TODO: If the push fails, fetch and retry if the remote ref - // has moved ahead of us. - - if ($this->getIntoLocal()) { - $can_retry = false; - } else if ($this->getIntoEmpty()) { - $can_retry = false; - } else if ($this->getIntoRemote() !== $this->getOntoRemote()) { - $can_retry = false; - } else { - $can_retry = false; - } - - if ($can_retry) { - // New commit state here - $into_commit = '..'; - continue; - } - - throw new PhutilArgumentUsageException( - $ex->getMessage()); + if ($should_push === false) { + break; + } + + try { + $this->pushChange($into_commit); + $this->setHasUnpushedChanges(false); + } catch (ArcanistLandPushFailureException $ex) { + + // TODO: If the push fails, fetch and retry if the remote ref + // has moved ahead of us. + + if ($this->getIntoLocal()) { + $can_retry = false; + } else if ($this->getIntoEmpty()) { + $can_retry = false; + } else if ($this->getIntoRemote() !== $this->getOntoRemote()) { + $can_retry = false; + } else { + $can_retry = false; + } + + if ($can_retry) { + // New commit state here + $into_commit = '..'; + continue; } - if ($need_cascade) { + throw new PhutilArgumentUsageException( + $ex->getMessage()); + } - // NOTE: We cascade each set we've pushed, but we're going to - // cascade them from most recent to least recent. This way, - // branches which descend from more recent changes only cascade - // once, directly in to the correct state. + if ($need_cascade) { - $need_cascade = array_reverse($need_cascade); - foreach ($need_cascade as $cascade_set) { - $this->cascadeState($set, $into_commit); - } - $need_cascade = array(); - } + // NOTE: We cascade each set we've pushed, but we're going to + // cascade them from most recent to least recent. This way, + // branches which descend from more recent changes only cascade + // once, directly in to the correct state. - if (!$is_keep) { - $this->pruneBranches($need_prune); - $need_prune = array(); + $need_cascade = array_reverse($need_cascade); + foreach ($need_cascade as $cascade_set) { + $this->cascadeState($cascade_set, $into_commit); } + $need_cascade = array(); + } + + if (!$is_keep) { + $this->pruneBranches($need_prune); + $need_prune = array(); } break; 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,7 +6,9 @@ private $ontoBranchMarker; private $ontoMarkers; - private $rebasedActiveCommit; + // During merge this populates the original commits being landed mapped to + // the new rebased/squashed commit. + private $rebasedCommitMap = array(); protected function getDefaultSymbols() { $api = $this->getRepositoryAPI(); @@ -771,8 +773,12 @@ protected function executeMerge(ArcanistLandCommitSet $set, $into_commit) { $api = $this->getRepositoryAPI(); + $log = $this->getLogEngine(); if ($this->getStrategy() !== 'squash') { + $log->writeError( + pht('TODO'), + pht('Merge strategy is not yet supported on Mercurial repositories.')); throw new Exception(pht('TODO: Support merge strategies')); } @@ -783,10 +789,7 @@ // we continue. $bookmark_refs = $api->newMarkerRefQuery() - ->withMarkerTypes( - array( - ArcanistMarkerRef::TYPE_BOOKMARK, - )) + ->withMarkerTypes(array(ArcanistMarkerRef::TYPE_BOOKMARK)) ->execute(); // TODO: Add a Mercurial version check requiring 2.1.1 or newer. @@ -842,6 +845,14 @@ $future->resolvex(); } catch (CommandException $ex) { + $log->writeError( + pht('REBASE CONFLICT'), + pht( + 'Commits for %s do not rebase cleanly onto %s. Manually rebase and '. + 'resolve conflicts before landing again.', + $revision_ref->getRefDisplayName(), + $api->getDisplayHash($into_commit))); + // Aborting the rebase should restore the same state prior to running the // rebase command. $api->execManualLocalWithExtension( @@ -850,19 +861,21 @@ throw $ex; } - // Find all the bookmarks which pointed at commits we just rebased, and - // put them back the way they were before rebasing moved them. We aren't - // deleting the old commits yet and don't want to move the bookmarks. + list($stdout) = $api->execxLocal('log --rev tip --template %s', '{node}'); + $new_cursor = trim($stdout); - $obsolete_map = array(); foreach ($set->getCommits() as $commit) { - $obsolete_map[$commit->getHash()] = true; + $this->rebasedCommitMap[$commit->getHash()] = $new_cursor; } + // Find all the bookmarks which pointed at commits we just rebased, and + // put them back the way they were before rebasing moved them. We aren't + // deleting the old commits yet and don't want to move the bookmarks. + foreach ($bookmark_refs as $bookmark_ref) { $bookmark_hash = $bookmark_ref->getCommitHash(); - if (!isset($obsolete_map[$bookmark_hash])) { + if (!isset($this->rebasedCommitMap[$bookmark_hash])) { continue; } @@ -872,16 +885,6 @@ $bookmark_ref->getName()); } - 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; } @@ -1053,6 +1056,12 @@ $old_commit = last($set->getCommits())->getHash(); $new_commit = $into_commit; + // Rebase the child commits onto the commit which $old_commit was rebased + // into. The $into_commit will be the last set's final rebased commit but + // this set may be an intermediate set that was landed. + if (isset($this->rebasedCommitMap[$old_commit])) { + $new_commit = $this->rebasedCommitMap[$old_commit]; + } list($output) = $api->execxLocal( 'log --rev %s --template %s', @@ -1065,8 +1074,11 @@ continue; } - // TODO: If the only heads which are descendants of this child will - // be deleted, we can skip this rebase? + // If this child was rebased as part of landing then it shouldn't be + // rebased now. Doing so will result in rebase conflicts. + if (isset($this->rebasedCommitMap[$child_hash])) { + continue; + } try { $api->execxLocalWithExtension( @@ -1075,6 +1087,14 @@ $child_hash, $new_commit); } catch (CommandException $ex) { + $log->writeError( + pht('REBASE CONFLICT'), + pht( + 'Failed to rebase %s cleanly onto %s. Manually rebase and '. + 'resolve conflicts.', + $api->getDisplayHash($child_hash), + $api->getDisplayHash($new_commit))); + // Aborting the rebase should restore the same state prior to running // the rebase command. $api->execManualLocalWithExtension( @@ -1181,10 +1201,11 @@ $api = $this->getRepositoryAPI(); - // If the starting working state was not part of land process just update - // to that original working state. - if ($this->rebasedActiveCommit === null) { - $update_marker = $this->getLocalState()->getLocalCommit(); + // If the previoius working directory was not part of land process just + // update to that original working directory. + $prev_local_commit = $this->getLocalState()->getLocalCommit(); + if (!isset($this->rebasedCommitMap[$prev_local_commit])) { + $update_marker = $prev_local_commit; if ($this->getLocalState()->getLocalBookmark() !== null) { $update_marker = $this->getLocalState()->getLocalBookmark(); } @@ -1209,7 +1230,9 @@ ->withMarkerTypes(array(ArcanistMarkerRef::TYPE_BOOKMARK)) ->execute(); - $update_marker = $this->rebasedActiveCommit; + // Resolve the previous working directory to the new rebased/squashed + // commit, to make that the new working directory. + $update_marker = $this->rebasedCommitMap[$prev_local_commit]; // 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 @@ -1217,7 +1240,7 @@ // for it to become active. $onto_marker = $this->ontoMarkers[0]->getName(); foreach ($bookmark_refs as $bookmark_ref) { - if ($bookmark_ref->getCommitHash() == $this->rebasedActiveCommit && + if ($bookmark_ref->getCommitHash() == $update_marker && $bookmark_ref->getName() == $onto_marker) { $update_marker = $onto_marker; break;