From 3308da5f8fbe9de8c9c2a25646a5704eca323851 Mon Sep 17 00:00:00 2001 From: Michael Krasnow Date: Mon, 26 Oct 2015 09:48:31 -0700 Subject: [PATCH 1/7] Fix T9635 by initializing the property Summary: Fix T9635 by properly initializing a property Test Plan: run arc unit Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: epriestley Maniphest Tasks: T9635 Differential Revision: https://secure.phabricator.com/D14340 --- src/unit/engine/ArcanistUnitTestEngine.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/unit/engine/ArcanistUnitTestEngine.php b/src/unit/engine/ArcanistUnitTestEngine.php index ad66bb90..e105f216 100644 --- a/src/unit/engine/ArcanistUnitTestEngine.php +++ b/src/unit/engine/ArcanistUnitTestEngine.php @@ -6,7 +6,7 @@ abstract class ArcanistUnitTestEngine extends Phobject { private $workingCopy; - private $paths; + private $paths = array(); private $arguments = array(); private $enableAsyncTests; private $enableCoverage; From a03c6079bb71d4f8d4cd4c8c661642f753349760 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 27 Oct 2015 17:49:01 -0700 Subject: [PATCH 2/7] Make "arc land" great again Summary: Ref T3855. Fixes T9537. Fixes T8620. Fixes T4333. This declares bankruptcy and replaces the entire `arc land` workflow under Git. These are the notable changes: - (T3855) You can now land from a branch to itself. - (T3855) We now try to restore the original state very aggressively after any failure, instead of dumping you into the middle of a mess. - (T9537) You can now land without a local branch. - ([not actually] T9543) We'll now ignore the local branch if it just happens to be named the same thing as the remote branch but doesn't actually track it. - (T8620) You can now land from a detached HEAD. - (T4333) We now preserve the author and author date of whatever you land. This may need some followup work. In particular: - The signal handler (that tries to put you in a better place if you ^C in the middle of things) causes ^C to work awkwardly in prompts. This might not be worth it. - Errors/instructions on push/merge issues might need work. - I dropped support for `--delete-remote` and `--update-with-blah-blah` because I think these flags aren't worth their complexity. - I've simplified the update/merge algorithm a bit. It may need some complexity added back in. - I probably missed a few things because this covers like 200 unique, creative workflows. - Users might need more guidance on the workflows that drop them in the middle of nowhere if they manage to reach them more often than I think. Test Plan: - Used `arc land` to land like at least 15,000 different kinds of changes. - Landed normally. - Landed from a branch onto itself. - Landed from a detached head. - Landed nothing. - Landed with no local branch. - Landed onto made-up branches. - Landed with bad targets. - ^C'd things in the middle. Reviewers: chad Reviewed By: chad Subscribers: tycho.tatitscheff Maniphest Tasks: T3855, T4333, T8620, T9537, T9543 Differential Revision: https://secure.phabricator.com/D14356 --- src/__phutil_library_map__.php | 4 + .../ArcanistUSEnglishTranslation.php | 5 + src/land/ArcanistGitLandEngine.php | 387 ++++++++++++++++++ src/land/ArcanistLandEngine.php | 161 ++++++++ src/workflow/ArcanistLandWorkflow.php | 82 +++- src/workflow/ArcanistWorkflow.php | 6 +- 6 files changed, 632 insertions(+), 13 deletions(-) create mode 100644 src/land/ArcanistGitLandEngine.php create mode 100644 src/land/ArcanistLandEngine.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 96d7da91..3d6429fb 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -112,6 +112,7 @@ phutil_register_library_map(array( 'ArcanistGeneratedLinterTestCase' => 'lint/linter/__tests__/ArcanistGeneratedLinterTestCase.php', 'ArcanistGetConfigWorkflow' => 'workflow/ArcanistGetConfigWorkflow.php', 'ArcanistGitAPI' => 'repository/api/ArcanistGitAPI.php', + 'ArcanistGitLandEngine' => 'land/ArcanistGitLandEngine.php', 'ArcanistGlobalVariableXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistGlobalVariableXHPASTLinterRule.php', 'ArcanistGoLintLinter' => 'lint/linter/ArcanistGoLintLinter.php', 'ArcanistGoLintLinterTestCase' => 'lint/linter/__tests__/ArcanistGoLintLinterTestCase.php', @@ -144,6 +145,7 @@ phutil_register_library_map(array( 'ArcanistJscsLinterTestCase' => 'lint/linter/__tests__/ArcanistJscsLinterTestCase.php', 'ArcanistKeywordCasingXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistKeywordCasingXHPASTLinterRule.php', 'ArcanistLambdaFuncFunctionXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistLambdaFuncFunctionXHPASTLinterRule.php', + 'ArcanistLandEngine' => 'land/ArcanistLandEngine.php', 'ArcanistLandWorkflow' => 'workflow/ArcanistLandWorkflow.php', 'ArcanistLanguageConstructParenthesesXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistLanguageConstructParenthesesXHPASTLinterRule.php', 'ArcanistLesscLinter' => 'lint/linter/ArcanistLesscLinter.php', @@ -398,6 +400,7 @@ phutil_register_library_map(array( 'ArcanistGeneratedLinterTestCase' => 'ArcanistLinterTestCase', 'ArcanistGetConfigWorkflow' => 'ArcanistWorkflow', 'ArcanistGitAPI' => 'ArcanistRepositoryAPI', + 'ArcanistGitLandEngine' => 'ArcanistLandEngine', 'ArcanistGlobalVariableXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistGoLintLinter' => 'ArcanistExternalLinter', 'ArcanistGoLintLinterTestCase' => 'ArcanistExternalLinterTestCase', @@ -430,6 +433,7 @@ phutil_register_library_map(array( 'ArcanistJscsLinterTestCase' => 'ArcanistExternalLinterTestCase', 'ArcanistKeywordCasingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistLambdaFuncFunctionXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', + 'ArcanistLandEngine' => 'Phobject', 'ArcanistLandWorkflow' => 'ArcanistWorkflow', 'ArcanistLanguageConstructParenthesesXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistLesscLinter' => 'ArcanistExternalLinter', diff --git a/src/internationalization/ArcanistUSEnglishTranslation.php b/src/internationalization/ArcanistUSEnglishTranslation.php index 04140399..e83e099e 100644 --- a/src/internationalization/ArcanistUSEnglishTranslation.php +++ b/src/internationalization/ArcanistUSEnglishTranslation.php @@ -78,6 +78,11 @@ final class ArcanistUSEnglishTranslation extends PhutilTranslation { 'Ignore the changes to this submodule and continue?', 'Ignore the changes to these submodules and continue?', ), + + 'These %s commit(s) will be landed:' => array( + 'This commit will be landed:', + 'These commits will be landed:', + ), ); } diff --git a/src/land/ArcanistGitLandEngine.php b/src/land/ArcanistGitLandEngine.php new file mode 100644 index 00000000..9fc08292 --- /dev/null +++ b/src/land/ArcanistGitLandEngine.php @@ -0,0 +1,387 @@ +verifySourceAndTargetExist(); + $this->fetchTarget(); + + $this->printLandingCommits(); + + if ($this->getShouldPreview()) { + $this->writeInfo( + pht('PREVIEW'), + pht('Completed preview of operation.')); + return; + } + + $this->saveLocalState(); + + try { + $this->identifyRevision(); + $this->updateWorkingCopy(); + + if ($this->getShouldHold()) { + $this->writeInfo( + pht('HOLD'), + pht('Holding change locally, it has not been pushed.')); + } else { + $this->pushChange(); + $this->reconcileLocalState(); + + if ($this->getShouldKeep()) { + echo tsprintf( + "%s\n", + pht('Keeping local branch.')); + } else { + $this->destroyLocalBranch(); + } + + $this->writeOkay( + pht('DONE'), + pht('Landed changes.')); + } + + $this->restoreWhenDestroyed = false; + } catch (Exception $ex) { + $this->restoreLocalState(); + throw $ex; + } + } + + public function __destruct() { + if ($this->restoreWhenDestroyed) { + $this->writeWARN( + pht('INTERRUPTED!'), + pht('Restoring working copy to its original state.')); + + $this->restoreLocalState(); + } + } + + protected function getLandingCommits() { + $api = $this->getRepositoryAPI(); + + list($out) = $api->execxLocal( + 'log --oneline %s..%s --', + $this->getTargetFullRef(), + $this->sourceCommit); + + $out = trim($out); + + if (!strlen($out)) { + return array(); + } else { + return phutil_split_lines($out, false); + } + } + + private function identifyRevision() { + $api = $this->getRepositoryAPI(); + $api->execxLocal('checkout %s --', $this->getSourceRef()); + call_user_func($this->getBuildMessageCallback(), $this); + } + + private function verifySourceAndTargetExist() { + $api = $this->getRepositoryAPI(); + + list($err) = $api->execManualLocal( + 'rev-parse --verify %s', + $this->getTargetFullRef()); + + if ($err) { + throw new Exception( + pht( + 'Branch "%s" does not exist in remote "%s".', + $this->getTargetOnto(), + $this->getTargetRemote())); + } + + list($err, $stdout) = $api->execManualLocal( + 'rev-parse --verify %s', + $this->getSourceRef()); + + if ($err) { + throw new Exception( + pht( + 'Branch "%s" does not exist in the local working copy.', + $this->getSourceRef())); + } + + $this->sourceCommit = trim($stdout); + } + + private function fetchTarget() { + $api = $this->getRepositoryAPI(); + + $ref = $this->getTargetFullRef(); + + $this->writeInfo( + pht('FETCH'), + pht('Fetching %s...', $ref)); + + $api->execxLocal( + 'fetch -- %s %s', + $this->getTargetRemote(), + $this->getTargetOnto()); + } + + private function updateWorkingCopy() { + $api = $this->getRepositoryAPI(); + $source = $this->sourceCommit; + + $api->execxLocal( + 'checkout %s --', + $this->getTargetFullRef()); + + list($original_author, $original_date) = $this->getAuthorAndDate($source); + + try { + if ($this->getShouldSquash()) { + $api->execxLocal( + 'merge --no-stat --no-commit --squash -- %s', + $source); + } else { + $api->execxLocal( + 'merge --no-stat --no-commit --no-ff -- %s', + $source); + } + } catch (Exception $ex) { + $api->execManualLocal('merge --abort'); + + // TODO: Maybe throw a better or more helpful exception here? + + throw $ex; + } + + $api->execxLocal( + 'commit --author %s --date %s -F %s --', + $original_author, + $original_date, + $this->getCommitMessageFile()); + + list($stdout) = $api->execxLocal( + 'rev-parse --verify %s', + 'HEAD'); + $this->mergedRef = trim($stdout); + } + + private function pushChange() { + $api = $this->getRepositoryAPI(); + + $this->writeInfo( + pht('PUSHING'), + pht('Pushing changes to "%s".', $this->getTargetFullRef())); + + list($err) = $api->execPassthru( + 'push -- %s %s:%s', + $this->getTargetRemote(), + $this->mergedRef, + $this->getTargetOnto()); + + if ($err) { + throw new ArcanistUsageException( + pht( + 'Push failed! Fix the error and run "%s" again.', + 'arc land')); + } + } + + private function reconcileLocalState() { + $api = $this->getRepositoryAPI(); + + // Try to put the user into the best final state we can. This is very + // complicated because users are incredibly creative and their local + // branches may have the same names as branches in the remote but no + // relationship to them. + + if ($this->localRef != $this->getSourceRef()) { + // The user ran `arc land X` but was on a different branch, so just put + // them back wherever they were before. + echo tsprintf( + "%s\n", + pht('Switching back to "%s".', $this->localRef)); + $this->restoreLocalState(); + return; + } + + list($err) = $api->execManualLocal( + 'rev-parse --verify %s', + $this->getTargetOnto()); + if ($err) { + echo tsprintf( + "%s\n", + pht( + 'Local branch "%s" does not exist, staying on detached HEAD.', + $this->getTargetOnto())); + return; + } + + list($err, $upstream) = $api->execManualLocal( + 'rev-parse --verify --symbolic-full-name %s', + $this->getTargetOnto().'@{upstream}'); + if ($err) { + echo tsprintf( + "%s\n", + pht( + 'Local branch "%s" has no upstream, staying on detached HEAD.', + $this->getTargetOnto())); + return; + } + + $upstream = trim($upstream); + $expect_upstream = 'refs/remotes/'.$this->getTargetFullRef(); + if ($upstream != $expect_upstream) { + echo tsprintf( + "%s\n", + pht( + 'Local branch "%s" tracks remote "%s" (not target remote "%s"), '. + 'staying on detached HEAD.', + $this->getTargetOnto(), + $upstream, + $expect_upstream)); + return; + } + + list($stdout) = $api->execxLocal( + 'log %s..%s --', + $this->mergedRef, + $this->getTargetOnto()); + $stdout = trim($stdout); + + if (!strlen($stdout)) { + echo tsprintf( + "%s\n", + pht( + 'Local "%s" tracks target remote "%s", checking out and '. + 'pulling changes.', + $this->getTargetOnto(), + $this->getTargetFullRef())); + + $api->execxLocal('checkout %s --', $this->getTargetOnto()); + $api->execxLocal('pull --'); + $api->execxLocal('submodule update --init --recursive'); + + return; + } + + if ($this->getTargetOnto() !== $this->getSourceRef()) { + echo tsprintf( + "%s\n", + pht( + 'Local "%s" is ahead of remote "%s". Checking out but '. + 'not pulling changes.', + $this->getTargetOnto(), + $this->getTargetFullRef())); + + $api->execxLocal('checkout %s --', $this->getTargetOnto()); + $api->execxLocal('submodule update --init --recursive'); + + return; + } + + // In this case, the user did something like land a branch onto itself, + // and the branch is tracking the correct remote. We're going to discard + // the local state and reset it to the state we just pushed. + + echo tsprintf( + "%s\n", + pht( + 'Local "%s" landed into remote "%s", resetting local branch to '. + 'remote state.', + $this->getTargetOnto(), + $this->getTargetFullRef())); + + $api->execxLocal('checkout %s --', $this->getTargetOnto()); + $api->execxLocal('reset --hard %s --', $this->getTargetFullRef()); + $api->execxLocal('submodule update --init --recursive'); + } + + private function destroyLocalBranch() { + $api = $this->getRepositoryAPI(); + + if ($this->localRef == $this->getSourceRef()) { + // If we landed a branch onto itself, don't destroy it. + return; + } + + $recovery_command = csprintf( + 'git checkout -b %R %R', + $this->getSourceRef(), + $this->sourceCommit); + + echo tsprintf( + "%s\n", + pht('Cleaning up branch "%s"...', $this->getSourceRef())); + + echo tsprintf( + "%s\n", + pht('(Use `%s` if you want it back.)', $recovery_command)); + + $api->execxLocal('branch -D -- %s', $this->getSourceRef()); + } + + /** + * Save the local working copy state so we can restore it later. + */ + private function saveLocalState() { + $api = $this->getRepositoryAPI(); + + $this->localCommit = $api->getWorkingCopyRevision(); + + list($ref) = $api->execxLocal('rev-parse --abbrev-ref HEAD'); + $ref = trim($ref); + if ($ref === 'HEAD') { + $ref = $this->localCommit; + } + + $this->localRef = $ref; + + $this->restoreWhenDestroyed = true; + } + + /** + * Restore the working copy to the state it was in before we started + * performing writes. + */ + private function restoreLocalState() { + $api = $this->getRepositoryAPI(); + + $api->execxLocal('checkout %s --', $this->localRef); + $api->execxLocal('reset --hard %s --', $this->localCommit); + $api->execxLocal('submodule update --init --recursive'); + + $this->restoreWhenDestroyed = false; + } + + private function getTargetFullRef() { + return $this->getTargetRemote().'/'.$this->getTargetOnto(); + } + + private function getAuthorAndDate($commit) { + $api = $this->getRepositoryAPI(); + + // TODO: This is working around Windows escaping problems, see T8298. + + list($info) = $api->execxLocal( + 'log -n1 --format=%C %s --', + '%aD%n%an%n%ae', + $commit); + + $info = trim($info); + list($date, $author, $email) = explode("\n", $info, 3); + + return array( + "$author <{$email}>", + $date, + ); + } + +} diff --git a/src/land/ArcanistLandEngine.php b/src/land/ArcanistLandEngine.php new file mode 100644 index 00000000..679a629a --- /dev/null +++ b/src/land/ArcanistLandEngine.php @@ -0,0 +1,161 @@ +workflow = $workflow; + return $this; + } + + final public function getWorkflow() { + return $this->workflow; + } + + final public function setRepositoryAPI( + ArcanistRepositoryAPI $repository_api) { + $this->repositoryAPI = $repository_api; + return $this; + } + + final public function getRepositoryAPI() { + return $this->repositoryAPI; + } + + final public function setShouldHold($should_hold) { + $this->shouldHold = $should_hold; + return $this; + } + + final public function getShouldHold() { + return $this->shouldHold; + } + + final public function setShouldKeep($should_keep) { + $this->shouldKeep = $should_keep; + return $this; + } + + final public function getShouldKeep() { + return $this->shouldKeep; + } + + final public function setShouldSquash($should_squash) { + $this->shouldSquash = $should_squash; + return $this; + } + + final public function getShouldSquash() { + return $this->shouldSquash; + } + + final public function setShouldPreview($should_preview) { + $this->shouldPreview = $should_preview; + return $this; + } + + final public function getShouldPreview() { + return $this->shouldPreview; + } + + final public function setTargetRemote($target_remote) { + $this->targetRemote = $target_remote; + return $this; + } + + final public function getTargetRemote() { + return $this->targetRemote; + } + + final public function setTargetOnto($target_onto) { + $this->targetOnto = $target_onto; + return $this; + } + + final public function getTargetOnto() { + return $this->targetOnto; + } + + final public function setSourceRef($source_ref) { + $this->sourceRef = $source_ref; + return $this; + } + + final public function getSourceRef() { + return $this->sourceRef; + } + + final public function setBuildMessageCallback($build_message_callback) { + $this->buildMessageCallback = $build_message_callback; + return $this; + } + + final public function getBuildMessageCallback() { + return $this->buildMessageCallback; + } + + final public function setCommitMessageFile($commit_message_file) { + $this->commitMessageFile = $commit_message_file; + return $this; + } + + final public function getCommitMessageFile() { + return $this->commitMessageFile; + } + + abstract public function execute(); + + abstract protected function getLandingCommits(); + + protected function printLandingCommits() { + $logs = $this->getLandingCommits(); + + if (!$logs) { + throw new ArcanistUsageException( + pht( + 'There are no commits on "%s" which are not already present on '. + 'the target.', + $this->getSourceRef())); + } + + $list = id(new PhutilConsoleList()) + ->setWrap(false) + ->addItems($logs); + + id(new PhutilConsoleBlock()) + ->addParagraph( + pht( + 'These %s commit(s) will be landed:', + new PhutilNumber(count($logs)))) + ->addList($list) + ->draw(); + } + + protected function writeWarn($title, $message) { + return $this->getWorkflow()->writeWarn($title, $message); + } + + protected function writeInfo($title, $message) { + return $this->getWorkflow()->writeInfo($title, $message); + } + + protected function writeOkay($title, $message) { + return $this->getWorkflow()->writeOkay($title, $message); + } + + +} diff --git a/src/workflow/ArcanistLandWorkflow.php b/src/workflow/ArcanistLandWorkflow.php index 8f4356ee..77db9602 100644 --- a/src/workflow/ArcanistLandWorkflow.php +++ b/src/workflow/ArcanistLandWorkflow.php @@ -196,6 +196,53 @@ EOTEXT public function run() { $this->readArguments(); + + $engine = null; + if ($this->isGit && !$this->isGitSvn) { + $engine = new ArcanistGitLandEngine(); + } + + if ($engine) { + $obsolete = array( + 'delete-remote', + 'update-with-merge', + 'update-with-rebase', + ); + + foreach ($obsolete as $flag) { + if ($this->getArgument($flag)) { + throw new ArcanistUsageException( + pht( + 'Flag "%s" is no longer supported under Git.', + '--'.$flag)); + } + } + + $this->requireCleanWorkingCopy(); + + $should_hold = $this->getArgument('hold'); + + $engine + ->setWorkflow($this) + ->setRepositoryAPI($this->getRepositoryAPI()) + ->setSourceRef($this->branch) + ->setTargetRemote($this->remote) + ->setTargetOnto($this->onto) + ->setShouldHold($should_hold) + ->setShouldKeep($this->keepBranch) + ->setShouldSquash($this->useSquash) + ->setShouldPreview($this->preview) + ->setBuildMessageCallback(array($this, 'buildEngineMessage')); + + $engine->execute(); + + if (!$should_hold) { + $this->didPush(); + } + + return 0; + } + $this->validate(); try { @@ -1085,16 +1132,7 @@ EOTEXT $cmd)); } - $this->askForRepositoryUpdate(); - - $mark_workflow = $this->buildChildWorkflow( - 'close-revision', - array( - '--finalize', - '--quiet', - $this->revision['id'], - )); - $mark_workflow->run(); + $this->didPush(); echo "\n"; } @@ -1193,6 +1231,11 @@ EOTEXT $repository_api = $this->getRepositoryAPI(); if ($this->isGit) { $branch = $repository_api->getBranchName(); + + // If we don't have a branch name, just use whatever's at HEAD. + if (!strlen($branch) && !$this->isGitSvn) { + $branch = $repository_api->getWorkingCopyRevision(); + } } else if ($this->isHg) { $branch = $repository_api->getActiveBookmark(); if (!$branch) { @@ -1317,4 +1360,23 @@ EOTEXT } } + public function buildEngineMessage(ArcanistLandEngine $engine) { + // TODO: This is oh-so-gross. + $this->findRevision(); + $engine->setCommitMessageFile($this->messageFile); + } + + public function didPush() { + $this->askForRepositoryUpdate(); + + $mark_workflow = $this->buildChildWorkflow( + 'close-revision', + array( + '--finalize', + '--quiet', + $this->revision['id'], + )); + $mark_workflow->run(); + } + } diff --git a/src/workflow/ArcanistWorkflow.php b/src/workflow/ArcanistWorkflow.php index b1f828ce..e222914c 100644 --- a/src/workflow/ArcanistWorkflow.php +++ b/src/workflow/ArcanistWorkflow.php @@ -1361,7 +1361,7 @@ abstract class ArcanistWorkflow extends Phobject { fwrite(STDERR, $msg); } - final protected function writeInfo($title, $message) { + final public function writeInfo($title, $message) { $this->writeStatusMessage( phutil_console_format( "** %s ** %s\n", @@ -1369,7 +1369,7 @@ abstract class ArcanistWorkflow extends Phobject { $message)); } - final protected function writeWarn($title, $message) { + final public function writeWarn($title, $message) { $this->writeStatusMessage( phutil_console_format( "** %s ** %s\n", @@ -1377,7 +1377,7 @@ abstract class ArcanistWorkflow extends Phobject { $message)); } - final protected function writeOkay($title, $message) { + final public function writeOkay($title, $message) { $this->writeStatusMessage( phutil_console_format( "** %s ** %s\n", From aa5c023fe8fa4b7a770f1e570870b96cf01a70bb Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 28 Oct 2015 08:28:46 -0700 Subject: [PATCH 3/7] Make rules for guessing onto/remote more powerful and more explicit in `arc land` Summary: Fixes T9543. Fixes T9658. Ref T3855. Major functional change is that you can have a sequence of branches like: origin/master -> notmaster -> feature1 ...where they track each other, but you named your local master something else. Currently, we resolve only one level of upstreams, so we try to land onto "notmaster" in this case, which is wrong. Instead, keep resolving upstreams until we either hit a cycle, don't have another upstream to look at, or find someting in a remote. In this case we'll eventually find "origin/master" and select "origin" as the remote and "master" as the target. Other minor changes: - Make this selection process explicit. - Make the help 3000x longer. Also fix a bug where we could incorrectly try to tell Differential to update awith `--preview`. Test Plan: - Landed from a tag. - Landed from a tracking branch. - Landed from an nth-degree tracking branch. - Tried to land from a local branch with a cycle in upstreams. - Landed with --remote and --onto. - Read `arc help land`. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9658, T3855, T9543 Differential Revision: https://secure.phabricator.com/D14357 --- src/land/ArcanistGitLandEngine.php | 2 + src/repository/api/ArcanistGitAPI.php | 67 ++++++++ src/workflow/ArcanistLandWorkflow.php | 225 ++++++++++++++++++++++++-- 3 files changed, 278 insertions(+), 16 deletions(-) diff --git a/src/land/ArcanistGitLandEngine.php b/src/land/ArcanistGitLandEngine.php index 9fc08292..1a55e14a 100644 --- a/src/land/ArcanistGitLandEngine.php +++ b/src/land/ArcanistGitLandEngine.php @@ -167,6 +167,8 @@ final class ArcanistGitLandEngine $original_date, $this->getCommitMessageFile()); + $this->getWorkflow()->didCommitMerge(); + list($stdout) = $api->execxLocal( 'rev-parse --verify %s', 'HEAD'); diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php index 84ac017f..74f97bc8 100644 --- a/src/repository/api/ArcanistGitAPI.php +++ b/src/repository/api/ArcanistGitAPI.php @@ -1333,4 +1333,71 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { $this->resolvedHeadCommit = null; } + /** + * Follow the chain of tracking branches upstream until we reach a remote + * or cycle locally. + * + * @param string Ref to start from. + * @return list Path to an upstream. + */ + public function getPathToUpstream($start) { + $cursor = $start; + $path = array(); + while (true) { + list($err, $upstream) = $this->execManualLocal( + 'rev-parse --symbolic-full-name %s@{upstream}', + $cursor); + + if ($err) { + // We ended up somewhere with no tracking branch, so we're done. + break; + } + + $upstream = trim($upstream); + + if (preg_match('(^refs/heads/)', $upstream)) { + $upstream = preg_replace('(^refs/heads/)', '', $upstream); + + $is_cycle = isset($path[$upstream]); + + $path[$cursor] = array( + 'type' => 'local', + 'name' => $upstream, + 'cycle' => $is_cycle, + ); + + if ($is_cycle) { + // We ran into a local cycle, so we're done. + break; + } + + // We found another local branch, so follow that one upriver. + $cursor = $upstream; + continue; + } + + if (preg_match('(^refs/remotes/)', $upstream)) { + $upstream = preg_replace('(^refs/remotes/)', '', $upstream); + list($remote, $branch) = explode('/', $upstream, 2); + + $path[$cursor] = array( + 'type' => 'remote', + 'name' => $branch, + 'remote' => $remote, + ); + + // We found a remote, so we're done. + break; + } + + throw new Exception( + pht( + 'Got unrecognized upstream format ("%s") from Git, expected '. + '"refs/heads/..." or "refs/remotes/...".', + $upstream)); + } + + return $path; + } + } diff --git a/src/workflow/ArcanistLandWorkflow.php b/src/workflow/ArcanistLandWorkflow.php index 77db9602..38874331 100644 --- a/src/workflow/ArcanistLandWorkflow.php +++ b/src/workflow/ArcanistLandWorkflow.php @@ -38,7 +38,7 @@ final class ArcanistLandWorkflow extends ArcanistWorkflow { public function getCommandSynopses() { return phutil_console_format(<<readEngineArguments(); + $obsolete = array( 'delete-remote', 'update-with-merge', @@ -236,7 +291,7 @@ EOTEXT $engine->execute(); - if (!$should_hold) { + if (!$should_hold && !$this->preview) { $this->didPush(); } @@ -303,6 +358,137 @@ EOTEXT return null; } + private function readEngineArguments() { + // NOTE: This is hard-coded for Git right now. + // TODO: Clean this up and move it into LandEngines. + + $onto = $this->getEngineOnto(); + $remote = $this->getEngineRemote(); + + // This just overwrites work we did earlier, but it has to be up in this + // class for now because other parts of the workflow still depend on it. + $this->onto = $onto; + $this->remote = $remote; + $this->ontoRemoteBranch = $this->remote.'/'.$onto; + } + + private function getEngineOnto() { + $onto = $this->getArgument('onto'); + if ($onto !== null) { + $this->writeInfo( + pht('TARGET'), + pht( + 'Landing onto "%s", selected by the --onto flag.', + $onto)); + return $onto; + } + + $api = $this->getRepositoryAPI(); + $path = $api->getPathToUpstream($this->branch); + + if ($path) { + $last = last($path); + if (isset($last['cycle'])) { + $this->writeWarn( + pht('LOCAL CYCLE'), + pht( + 'Local branch tracks an upstream, but following it leads to a '. + 'local cycle; ignoring branch upstream.')); + + echo tsprintf( + "\n %s\n\n", + $this->formatUpstreamPathCycle($path)); + + } else { + if ($last['type'] == 'remote') { + $onto = $last['name']; + $this->writeInfo( + pht('TARGET'), + pht( + 'Landing onto "%s", selected by following tracking branches '. + 'upstream to the closest remote.', + $onto)); + return $onto; + } else { + $this->writeInfo( + pht('NO PATH TO UPSTREAM'), + pht( + 'Local branch tracks an upstream, but there is no path '. + 'to a remote; ignoring branch upstream.')); + } + } + } + + $config_key = 'arc.land.onto.default'; + $onto = $this->getConfigFromAnySource($config_key); + if ($onto !== null) { + $this->writeInfo( + pht('TARGET'), + pht( + 'Landing onto "%s", selected by "%s" configuration.', + $onto, + $config_key)); + return $onto; + } + + $onto = 'master'; + $this->writeInfo( + pht('TARGET'), + pht( + 'Landing onto "%s", the default target under git.', + $onto)); + return $onto; + } + + private function getEngineRemote() { + $remote = $this->getArgument('remote'); + if ($remote !== null) { + $this->writeInfo( + pht('REMOTE'), + pht( + 'Using remote "%s", selected by the --remote flag.', + $remote)); + return $remote; + } + + $api = $this->getRepositoryAPI(); + $path = $api->getPathToUpstream($this->branch); + + if ($path) { + $last = last($path); + if ($last['type'] == 'remote') { + $remote = $last['remote']; + $this->writeInfo( + pht('REMOTE'), + pht( + 'Using remote "%s", selected by following tracking branches '. + 'upstream to the closest remote.', + $remote)); + return $remote; + } + } + + $remote = 'origin'; + $this->writeInfo( + pht('REMOTE'), + pht( + 'Using remote "%s", the default remote under git.', + $remote)); + return $remote; + } + + private function formatUpstreamPathCycle(array $cycle) { + $parts = array(); + foreach ($cycle as $key => $value) { + $parts[] = $key; + } + $parts[] = idx(last($cycle), 'name'); + $parts[] = pht('...'); + + return implode(' -> ', $parts); + } + + private function readArguments() { $repository_api = $this->getRepositoryAPI(); $this->isGit = $repository_api instanceof ArcanistGitAPI; @@ -320,9 +506,12 @@ EOTEXT $branch = $this->getArgument('branch'); if (empty($branch)) { $branch = $this->getBranchOrBookmark(); - if ($branch) { $this->branchType = $this->getBranchType($branch); + + // TODO: This message is misleading when landing a detached head or + // a tag in Git. + echo pht("Landing current %s '%s'.", $this->branchType, $branch), "\n"; $branch = array($branch); } @@ -1079,9 +1268,7 @@ EOTEXT // We dispatch this event so we can run checks on the merged revision, // right before it gets pushed out. It's easier to do this in arc land // than to try to hook into git/hg. - $this->dispatchEvent( - ArcanistEventType::TYPE_LAND_WILLPUSHREVISION, - array()); + $this->didCommitMerge(); } catch (Exception $ex) { $this->executeCleanupAfterFailedPush(); throw $ex; @@ -1366,6 +1553,12 @@ EOTEXT $engine->setCommitMessageFile($this->messageFile); } + public function didCommitMerge() { + $this->dispatchEvent( + ArcanistEventType::TYPE_LAND_WILLPUSHREVISION, + array()); + } + public function didPush() { $this->askForRepositoryUpdate(); From 411a4f4a3917f6916e7e4d1811ae1c8129af6a7a Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 28 Oct 2015 10:03:51 -0700 Subject: [PATCH 4/7] Fix a check when deciding to destroy the local branch after "arc land" Summary: Fixes T9660. The behavior for this check wasn't quite right -- we want to check the "source ref" (what we're landing) against the "target onto" (the branch we're landing it onto). Test Plan: - Landed from `master` (tracking origin/master). No delete. - Landed from `feature1` (tracking local/master). Delete. - Landed from `feature2` (tracking origin/master). Delete. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9660 Differential Revision: https://secure.phabricator.com/D14358 --- src/land/ArcanistGitLandEngine.php | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/land/ArcanistGitLandEngine.php b/src/land/ArcanistGitLandEngine.php index 1a55e14a..945a5f39 100644 --- a/src/land/ArcanistGitLandEngine.php +++ b/src/land/ArcanistGitLandEngine.php @@ -36,6 +36,9 @@ final class ArcanistGitLandEngine $this->pushChange(); $this->reconcileLocalState(); + $api = $this->getRepositoryAPI(); + $api->execxLocal('submodule update --init --recursive'); + if ($this->getShouldKeep()) { echo tsprintf( "%s\n", @@ -269,7 +272,6 @@ final class ArcanistGitLandEngine $api->execxLocal('checkout %s --', $this->getTargetOnto()); $api->execxLocal('pull --'); - $api->execxLocal('submodule update --init --recursive'); return; } @@ -284,7 +286,6 @@ final class ArcanistGitLandEngine $this->getTargetFullRef())); $api->execxLocal('checkout %s --', $this->getTargetOnto()); - $api->execxLocal('submodule update --init --recursive'); return; } @@ -303,17 +304,20 @@ final class ArcanistGitLandEngine $api->execxLocal('checkout %s --', $this->getTargetOnto()); $api->execxLocal('reset --hard %s --', $this->getTargetFullRef()); - $api->execxLocal('submodule update --init --recursive'); } private function destroyLocalBranch() { $api = $this->getRepositoryAPI(); - if ($this->localRef == $this->getSourceRef()) { - // If we landed a branch onto itself, don't destroy it. + if ($this->getSourceRef() == $this->getTargetOnto()) { + // If we landed a branch into a branch with the same name, so don't + // destroy it. This prevents us from cleaning up "master" if you're + // landing master into itself. return; } + // TODO: Maybe this should also recover the proper upstream? + $recovery_command = csprintf( 'git checkout -b %R %R', $this->getSourceRef(), From 2a2fd6e338b72a3b1b654a8a149657c73fdf599b Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 28 Oct 2015 11:30:57 -0700 Subject: [PATCH 5/7] Pull git upstream-path logic into a separate class Summary: Ref T9661. I need to reuse this to fix the complex workflow described in T9661 where we need to follow multiple paths to the upstream and cascade updates across them. Pull the logic into a separate class to make this easier and less copy/pastey. This shouldn't change any behavior. Test Plan: Ran `arc land --preview` from detached head, remote-tracking branch, non-tracking branch, local-tracking branch. Selection of target/remote seemed correct in all cases. Reviewers: chad Reviewed By: chad Subscribers: edibiase Maniphest Tasks: T9661 Differential Revision: https://secure.phabricator.com/D14360 --- src/__phutil_library_map__.php | 2 + src/repository/api/ArcanistGitAPI.php | 28 ++++--- .../api/ArcanistGitUpstreamPath.php | 82 +++++++++++++++++++ src/workflow/ArcanistLandWorkflow.php | 44 ++++------ 4 files changed, 115 insertions(+), 41 deletions(-) create mode 100644 src/repository/api/ArcanistGitUpstreamPath.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 3d6429fb..ec6a78b5 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -113,6 +113,7 @@ phutil_register_library_map(array( 'ArcanistGetConfigWorkflow' => 'workflow/ArcanistGetConfigWorkflow.php', 'ArcanistGitAPI' => 'repository/api/ArcanistGitAPI.php', 'ArcanistGitLandEngine' => 'land/ArcanistGitLandEngine.php', + 'ArcanistGitUpstreamPath' => 'repository/api/ArcanistGitUpstreamPath.php', 'ArcanistGlobalVariableXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistGlobalVariableXHPASTLinterRule.php', 'ArcanistGoLintLinter' => 'lint/linter/ArcanistGoLintLinter.php', 'ArcanistGoLintLinterTestCase' => 'lint/linter/__tests__/ArcanistGoLintLinterTestCase.php', @@ -401,6 +402,7 @@ phutil_register_library_map(array( 'ArcanistGetConfigWorkflow' => 'ArcanistWorkflow', 'ArcanistGitAPI' => 'ArcanistRepositoryAPI', 'ArcanistGitLandEngine' => 'ArcanistLandEngine', + 'ArcanistGitUpstreamPath' => 'Phobject', 'ArcanistGlobalVariableXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistGoLintLinter' => 'ArcanistExternalLinter', 'ArcanistGoLintLinterTestCase' => 'ArcanistExternalLinterTestCase', diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php index 74f97bc8..d93b8083 100644 --- a/src/repository/api/ArcanistGitAPI.php +++ b/src/repository/api/ArcanistGitAPI.php @@ -1342,7 +1342,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { */ public function getPathToUpstream($start) { $cursor = $start; - $path = array(); + $path = new ArcanistGitUpstreamPath(); while (true) { list($err, $upstream) = $this->execManualLocal( 'rev-parse --symbolic-full-name %s@{upstream}', @@ -1358,13 +1358,15 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { if (preg_match('(^refs/heads/)', $upstream)) { $upstream = preg_replace('(^refs/heads/)', '', $upstream); - $is_cycle = isset($path[$upstream]); + $is_cycle = $path->getUpstream($upstream); - $path[$cursor] = array( - 'type' => 'local', - 'name' => $upstream, - 'cycle' => $is_cycle, - ); + $path->addUpstream( + $cursor, + array( + 'type' => ArcanistGitUpstreamPath::TYPE_LOCAL, + 'name' => $upstream, + 'cycle' => $is_cycle, + )); if ($is_cycle) { // We ran into a local cycle, so we're done. @@ -1380,11 +1382,13 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { $upstream = preg_replace('(^refs/remotes/)', '', $upstream); list($remote, $branch) = explode('/', $upstream, 2); - $path[$cursor] = array( - 'type' => 'remote', - 'name' => $branch, - 'remote' => $remote, - ); + $path->addUpstream( + $cursor, + array( + 'type' => ArcanistGitUpstreamPath::TYPE_REMOTE, + 'name' => $branch, + 'remote' => $remote, + )); // We found a remote, so we're done. break; diff --git a/src/repository/api/ArcanistGitUpstreamPath.php b/src/repository/api/ArcanistGitUpstreamPath.php new file mode 100644 index 00000000..1d3feed0 --- /dev/null +++ b/src/repository/api/ArcanistGitUpstreamPath.php @@ -0,0 +1,82 @@ +path[$key] = $spec; + return $this; + } + + public function getUpstream($key) { + return idx($this->path, $key); + } + + public function getLength() { + return count($this->path); + } + + /** + * Test if this path eventually connects to a remote. + * + * @return bool True if the path connects to a remote. + */ + public function isConnectedToRemote() { + $last = last($this->path); + + if (!$last) { + return false; + } + + return ($last['type'] == self::TYPE_REMOTE); + } + + + public function getRemoteBranchName() { + if (!$this->isConnectedToRemote()) { + return null; + } + + return idx(last($this->path), 'name'); + } + + public function getRemoteRemoteName() { + if (!$this->isConnectedToRemote()) { + return null; + } + + return idx(last($this->path), 'remote'); + } + + + /** + * If this path contains a cycle, return a description of it. + * + * @return list|null Cycle, if the path contains one. + */ + public function getCycle() { + $last = last($this->path); + if (!$last) { + return null; + } + + if (empty($last['cycle'])) { + return null; + } + + $parts = array(); + foreach ($this->path as $key => $item) { + $parts[] = $key; + } + $parts[] = $item['name']; + $parts[] = pht('...'); + + return $parts; + } + +} diff --git a/src/workflow/ArcanistLandWorkflow.php b/src/workflow/ArcanistLandWorkflow.php index 38874331..4525eeb4 100644 --- a/src/workflow/ArcanistLandWorkflow.php +++ b/src/workflow/ArcanistLandWorkflow.php @@ -386,9 +386,9 @@ EOTEXT $api = $this->getRepositoryAPI(); $path = $api->getPathToUpstream($this->branch); - if ($path) { - $last = last($path); - if (isset($last['cycle'])) { + if ($path->getLength()) { + $cycle = $path->getCycle(); + if ($cycle) { $this->writeWarn( pht('LOCAL CYCLE'), pht( @@ -397,11 +397,11 @@ EOTEXT echo tsprintf( "\n %s\n\n", - $this->formatUpstreamPathCycle($path)); + implode(' -> ', $cycle)); } else { - if ($last['type'] == 'remote') { - $onto = $last['name']; + if ($path->isConnectedToRemote()) { + $onto = $path->getRemoteBranchName(); $this->writeInfo( pht('TARGET'), pht( @@ -454,18 +454,15 @@ EOTEXT $api = $this->getRepositoryAPI(); $path = $api->getPathToUpstream($this->branch); - if ($path) { - $last = last($path); - if ($last['type'] == 'remote') { - $remote = $last['remote']; - $this->writeInfo( - pht('REMOTE'), - pht( - 'Using remote "%s", selected by following tracking branches '. - 'upstream to the closest remote.', - $remote)); - return $remote; - } + $remote = $path->getRemoteRemoteName(); + if ($remote !== null) { + $this->writeInfo( + pht('REMOTE'), + pht( + 'Using remote "%s", selected by following tracking branches '. + 'upstream to the closest remote.', + $remote)); + return $remote; } $remote = 'origin'; @@ -477,17 +474,6 @@ EOTEXT return $remote; } - private function formatUpstreamPathCycle(array $cycle) { - $parts = array(); - foreach ($cycle as $key => $value) { - $parts[] = $key; - } - $parts[] = idx(last($cycle), 'name'); - $parts[] = pht('...'); - - return implode(' -> ', $parts); - } - private function readArguments() { $repository_api = $this->getRepositoryAPI(); From c844669326edd0a6ef5b6169a1d7ccc84daf5d3d Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 28 Oct 2015 12:35:13 -0700 Subject: [PATCH 6/7] After pushing at the end of "arc land", cascade the origin through all local tracking branches Summary: Fixes T9661. Users can construct arbitrarily long chains from the remote, like: (remote) origin/master -> (local) cascade-a -> (local) cascade-b -> (local) cascade-c -> (local) cascade-d When a user lands "cascade-d" onto "origin/master", we should pull A, B and C if they aren't ahead of the remote. If a user lands "cascade-d" onto itself, we should pull A, B, and C if they aren't ahead of the remote, then reset D to the remote. We also find this chain if the last component of it is connected by the local branch having the same name as the remote branch (typical for "master") instead of an actual connection through tracking brnaches. Test Plan: See comment below. Reviewers: chad Reviewed By: chad Subscribers: edibiase Maniphest Tasks: T9661 Differential Revision: https://secure.phabricator.com/D14361 --- src/land/ArcanistGitLandEngine.php | 252 ++++++++++++++---- .../api/ArcanistGitUpstreamPath.php | 8 + 2 files changed, 202 insertions(+), 58 deletions(-) diff --git a/src/land/ArcanistGitLandEngine.php b/src/land/ArcanistGitLandEngine.php index 945a5f39..f2b5d632 100644 --- a/src/land/ArcanistGitLandEngine.php +++ b/src/land/ArcanistGitLandEngine.php @@ -158,10 +158,25 @@ final class ArcanistGitLandEngine } } catch (Exception $ex) { $api->execManualLocal('merge --abort'); + $api->execManualLocal('reset --hard HEAD --'); - // TODO: Maybe throw a better or more helpful exception here? + throw new Exception( + pht( + 'Local "%s" does not merge cleanly into "%s". Merge or rebase '. + 'local changes so they can merge cleanly.', + $this->getSourceRef(), + $this->getTargetFullRef())); + } - throw $ex; + list($changes) = $api->execxLocal('diff HEAD --'); + $changes = trim($changes); + if (!strlen($changes)) { + throw new Exception( + pht( + 'Merging local "%s" into "%s" produces an empty diff. '. + 'This usually means these changes have already landed.', + $this->getSourceRef(), + $this->getTargetFullRef())); } $api->execxLocal( @@ -210,100 +225,221 @@ final class ArcanistGitLandEngine if ($this->localRef != $this->getSourceRef()) { // The user ran `arc land X` but was on a different branch, so just put // them back wherever they were before. - echo tsprintf( - "%s\n", + $this->writeInfo( + pht('RESTORE'), pht('Switching back to "%s".', $this->localRef)); $this->restoreLocalState(); return; } - list($err) = $api->execManualLocal( - 'rev-parse --verify %s', - $this->getTargetOnto()); - if ($err) { - echo tsprintf( - "%s\n", + // We're going to try to find a path to the upstream target branch. We + // try in two different ways: + // + // - follow the source branch directly along tracking branches until + // we reach the upstream; or + // - follow a local branch with the same name as the target branch until + // we reach the upstream. + + // First, get the path from whatever we landed to wherever it goes. + $local_branch = $this->getSourceRef(); + + $path = $api->getPathToUpstream($local_branch); + if ($path->getLength()) { + // We may want to discard the thing we landed from the path, if we're + // going to delete it. In this case, we don't want to update it or worry + // if it's dirty. + if ($this->getSourceRef() == $this->getTargetOnto()) { + // In this case, we've done something like land "master" onto itself, + // so we do want to update the actual branch. We're going to use the + // entire path. + } else { + // Otherwise, we're going to delete the branch at the end of the + // workflow, so throw it away the most-local branch that isn't long + // for this world. + $path->removeUpstream($local_branch); + + if (!$path->getLength()) { + $this->writeInfo( + pht('UPDATE'), + pht( + 'Local branch "%s" directly tracks remote, staying on '. + 'detached HEAD.', + $local_branch)); + return; + } + + $local_branch = head($path->getLocalBranches()); + } + } else { + // The source branch has no upstream, so look for a local branch with + // the same name as the target branch. This corresponds to the common + // case where you have "master" and checkout local branches from it + // with "git checkout -b feature", then land onto "master". + + $local_branch = $this->getTargetOnto(); + + list($err) = $api->execManualLocal( + 'rev-parse --verify %s', + $local_branch); + if ($err) { + $this->writeInfo( + pht('UPDATE'), + pht( + 'Local branch "%s" does not exist, staying on detached HEAD.', + $local_branch)); + return; + } + + $path = $api->getPathToUpstream($local_branch); + } + + if ($path->getCycle()) { + $this->writeWarn( + pht('LOCAL CYCLE'), pht( - 'Local branch "%s" does not exist, staying on detached HEAD.', - $this->getTargetOnto())); + 'Local branch "%s" tracks an upstream but following it leads to '. + 'a local cycle, staying on detached HEAD.', + $local_branch)); return; } - list($err, $upstream) = $api->execManualLocal( - 'rev-parse --verify --symbolic-full-name %s', - $this->getTargetOnto().'@{upstream}'); - if ($err) { - echo tsprintf( - "%s\n", + if (!$path->isConnectedToRemote()) { + $this->writeInfo( + pht('UPDATE'), pht( - 'Local branch "%s" has no upstream, staying on detached HEAD.', - $this->getTargetOnto())); + 'Local branch "%s" is not connected to a remote, staying on '. + 'detached HEAD.', + $local_branch)); return; } - $upstream = trim($upstream); - $expect_upstream = 'refs/remotes/'.$this->getTargetFullRef(); - if ($upstream != $expect_upstream) { - echo tsprintf( - "%s\n", + $remote_remote = $path->getRemoteRemoteName(); + $remote_branch = $path->getRemoteBranchName(); + + $remote_actual = $remote_remote.'/'.$remote_branch; + $remote_expect = $this->getTargetFullRef(); + if ($remote_actual != $remote_expect) { + $this->writeInfo( + pht('UPDATE'), pht( - 'Local branch "%s" tracks remote "%s" (not target remote "%s"), '. - 'staying on detached HEAD.', - $this->getTargetOnto(), - $upstream, - $expect_upstream)); + 'Local branch "%s" is connected to a remote ("%s") other than '. + 'the target remote ("%s"), staying on detached HEAD.', + $local_branch, + $remote_actual, + $remote_expect)); return; } - list($stdout) = $api->execxLocal( - 'log %s..%s --', - $this->mergedRef, - $this->getTargetOnto()); - $stdout = trim($stdout); + // If we get this far, we have a sequence of branches which ultimately + // connect to the remote. We're going to try to update them all in reverse + // order, from most-upstream to most-local. - if (!strlen($stdout)) { - echo tsprintf( - "%s\n", + $cascade_branches = $path->getLocalBranches(); + $cascade_branches = array_reverse($cascade_branches); + + // First, check if any of them are ahead of the remote. + + $ahead_of_remote = array(); + foreach ($cascade_branches as $cascade_branch) { + list($stdout) = $api->execxLocal( + 'log %s..%s --', + $this->mergedRef, + $cascade_branch); + $stdout = trim($stdout); + + if (strlen($stdout)) { + $ahead_of_remote[$cascade_branch] = $cascade_branch; + } + } + + // We're going to handle the last branch (the thing we ultimately intend + // to check out) differently. It's OK if it's ahead of the remote, as long + // as we just landed it. + + $local_ahead = isset($ahead_of_remote[$local_branch]); + unset($ahead_of_remote[$local_branch]); + $land_self = ($this->getTargetOnto() === $this->getSourceRef()); + + // We aren't going to pull anything if anything upstream from us is ahead + // of the remote, or the local is ahead of the remote and we didn't land + // it onto itself. + $skip_pull = ($ahead_of_remote || ($local_ahead && !$land_self)); + + if ($skip_pull) { + $this->writeInfo( + pht('UPDATE'), + pht( + 'Local "%s" is ahead of remote "%s". Checking out "%s" but '. + 'not pulling changes.', + nonempty(head($ahead_of_remote), $local_branch), + $this->getTargetFullRef(), + $local_branch)); + + $this->writeInfo( + pht('CHECKOUT'), + pht( + 'Checking out "%s".', + $local_branch)); + + $api->execxLocal('checkout %s --', $local_branch); + + return; + } + + // If nothing upstream from our nearest branch is ahead of the remote, + // pull it all. + + $cascade_targets = array(); + if (!$ahead_of_remote) { + foreach ($cascade_branches as $cascade_branch) { + if ($local_ahead && ($local_branch == $cascade_branch)) { + continue; + } + $cascade_targets[] = $cascade_branch; + } + } + + if ($cascade_targets) { + $this->writeInfo( + pht('UPDATE'), pht( 'Local "%s" tracks target remote "%s", checking out and '. 'pulling changes.', - $this->getTargetOnto(), + $local_branch, $this->getTargetFullRef())); - $api->execxLocal('checkout %s --', $this->getTargetOnto()); - $api->execxLocal('pull --'); + foreach ($cascade_targets as $cascade_branch) { + $this->writeInfo( + pht('PULL'), + pht( + 'Checking out and pulling "%s".', + $cascade_branch)); - return; - } + $api->execxLocal('checkout %s --', $cascade_branch); + $api->execxLocal('pull --'); + } - if ($this->getTargetOnto() !== $this->getSourceRef()) { - echo tsprintf( - "%s\n", - pht( - 'Local "%s" is ahead of remote "%s". Checking out but '. - 'not pulling changes.', - $this->getTargetOnto(), - $this->getTargetFullRef())); - - $api->execxLocal('checkout %s --', $this->getTargetOnto()); - - return; + if (!$local_ahead) { + return; + } } // In this case, the user did something like land a branch onto itself, // and the branch is tracking the correct remote. We're going to discard // the local state and reset it to the state we just pushed. - echo tsprintf( - "%s\n", + $this->writeInfo( + pht('RESET'), pht( 'Local "%s" landed into remote "%s", resetting local branch to '. 'remote state.', $this->getTargetOnto(), $this->getTargetFullRef())); - $api->execxLocal('checkout %s --', $this->getTargetOnto()); + $api->execxLocal('checkout %s --', $local_branch); $api->execxLocal('reset --hard %s --', $this->getTargetFullRef()); + + return; } private function destroyLocalBranch() { diff --git a/src/repository/api/ArcanistGitUpstreamPath.php b/src/repository/api/ArcanistGitUpstreamPath.php index 1d3feed0..ad487106 100644 --- a/src/repository/api/ArcanistGitUpstreamPath.php +++ b/src/repository/api/ArcanistGitUpstreamPath.php @@ -13,6 +13,11 @@ final class ArcanistGitUpstreamPath extends Phobject { return $this; } + public function removeUpstream($key) { + unset($this->path[$key]); + return $this; + } + public function getUpstream($key) { return idx($this->path, $key); } @@ -36,6 +41,9 @@ final class ArcanistGitUpstreamPath extends Phobject { return ($last['type'] == self::TYPE_REMOTE); } + public function getLocalBranches() { + return array_keys($this->path); + } public function getRemoteBranchName() { if (!$this->isConnectedToRemote()) { From baf5eb8a8795f8c2e11e041357b2be4d04d69490 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 28 Oct 2015 16:46:39 -0700 Subject: [PATCH 7/7] Explicitly provide "--ff" when performing squash merges in "arc land" under Git Summary: Ref T3855. See discussion leading up to T3855#142304. Test Plan: See T3855#142304. Reviewers: chad Reviewed By: chad Maniphest Tasks: T3855 Differential Revision: https://secure.phabricator.com/D14365 --- src/land/ArcanistGitLandEngine.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/land/ArcanistGitLandEngine.php b/src/land/ArcanistGitLandEngine.php index f2b5d632..95b33aa1 100644 --- a/src/land/ArcanistGitLandEngine.php +++ b/src/land/ArcanistGitLandEngine.php @@ -148,8 +148,11 @@ final class ArcanistGitLandEngine try { if ($this->getShouldSquash()) { + // NOTE: We're explicitly specifying "--ff" to override the presence + // of "merge.ff" options in user configuration. + $api->execxLocal( - 'merge --no-stat --no-commit --squash -- %s', + 'merge --no-stat --no-commit --ff --squash -- %s', $source); } else { $api->execxLocal( @@ -200,7 +203,7 @@ final class ArcanistGitLandEngine pht('PUSHING'), pht('Pushing changes to "%s".', $this->getTargetFullRef())); - list($err) = $api->execPassthru( + $err = $api->execPassthru( 'push -- %s %s:%s', $this->getTargetRemote(), $this->mergedRef,