From 8a53b5a4517de44c09869d8786e3fd7b5f88fd0c Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 7 Jun 2020 07:49:30 -0700 Subject: [PATCH] When landing changes in an empty repository, merge cleanly in Git Summary: Fixes T12876. Ref T13546. When you make the first change in a new Git repository, "arc land" currently can not merge it because there's nothing to merge into. Support merging into the empty state formally, reachable by using "--into-empty" (which should be uncommon) or "arc land" in an empty repository. Test Plan: - Used "arc land --into-empty --hold ..." to generate merges against the empty state under "squash" and "merge" strategies in Git. - Got sensible result commits with appropriate parents and content. Maniphest Tasks: T13546, T12876 Differential Revision: https://secure.phabricator.com/D21324 --- src/__phutil_library_map__.php | 4 + src/land/engine/ArcanistGitLandEngine.php | 98 ++++++---- src/land/engine/ArcanistLandEngine.php | 6 +- src/repository/api/ArcanistGitAPI.php | 18 ++ src/repository/raw/ArcanistGitRawCommit.php | 183 ++++++++++++++++++ .../ArcanistGitRawCommitTestCase.php | 91 +++++++++ 6 files changed, 359 insertions(+), 41 deletions(-) create mode 100644 src/repository/raw/ArcanistGitRawCommit.php create mode 100644 src/repository/raw/__tests__/ArcanistGitRawCommitTestCase.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 31e4bd10..260ff380 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -220,6 +220,8 @@ phutil_register_library_map(array( 'ArcanistGitCommitSymbolCommitHardpointQuery' => 'ref/commit/ArcanistGitCommitSymbolCommitHardpointQuery.php', 'ArcanistGitLandEngine' => 'land/engine/ArcanistGitLandEngine.php', 'ArcanistGitLocalState' => 'repository/state/ArcanistGitLocalState.php', + 'ArcanistGitRawCommit' => 'repository/raw/ArcanistGitRawCommit.php', + 'ArcanistGitRawCommitTestCase' => 'repository/raw/__tests__/ArcanistGitRawCommitTestCase.php', 'ArcanistGitUpstreamPath' => 'repository/api/ArcanistGitUpstreamPath.php', 'ArcanistGitWorkingCopy' => 'workingcopy/ArcanistGitWorkingCopy.php', 'ArcanistGitWorkingCopyRevisionHardpointQuery' => 'query/ArcanistGitWorkingCopyRevisionHardpointQuery.php', @@ -1239,6 +1241,8 @@ phutil_register_library_map(array( 'ArcanistGitCommitSymbolCommitHardpointQuery' => 'ArcanistWorkflowGitHardpointQuery', 'ArcanistGitLandEngine' => 'ArcanistLandEngine', 'ArcanistGitLocalState' => 'ArcanistRepositoryLocalState', + 'ArcanistGitRawCommit' => 'Phobject', + 'ArcanistGitRawCommitTestCase' => 'PhutilTestCase', 'ArcanistGitUpstreamPath' => 'Phobject', 'ArcanistGitWorkingCopy' => 'ArcanistWorkingCopy', 'ArcanistGitWorkingCopyRevisionHardpointQuery' => 'ArcanistWorkflowGitHardpointQuery', diff --git a/src/land/engine/ArcanistGitLandEngine.php b/src/land/engine/ArcanistGitLandEngine.php index 21915ad4..2e6a8587 100644 --- a/src/land/engine/ArcanistGitLandEngine.php +++ b/src/land/engine/ArcanistGitLandEngine.php @@ -226,20 +226,18 @@ final class ArcanistGitLandEngine return $this->getLandTargetLocalCommit($target); } - private function updateWorkingCopy($into_commit) { - $api = $this->getRepositoryAPI(); - if ($into_commit === null) { - throw new Exception('TODO: Author a new empty state.'); - } else { - $api->execxLocal('checkout %s --', $into_commit); - } - } - protected function executeMerge(ArcanistLandCommitSet $set, $into_commit) { $api = $this->getRepositoryAPI(); $log = $this->getLogEngine(); - $this->updateWorkingCopy($into_commit); + $is_empty = ($into_commit === null); + + if ($is_empty) { + $empty_commit = ArcanistGitRawCommit::newEmptyCommit(); + $into_commit = $api->writeRawCommit($empty_commit); + } + + $api->execxLocal('checkout %s --', $into_commit); $commits = $set->getCommits(); $max_commit = last($commits); @@ -251,7 +249,8 @@ final class ArcanistGitLandEngine // as changes. list($changes) = $api->execxLocal( - 'diff --no-ext-diff HEAD..%s --', + 'diff --no-ext-diff %s..%s --', + $into_commit, $source_commit); $changes = trim($changes); if (!strlen($changes)) { @@ -274,20 +273,30 @@ final class ArcanistGitLandEngine $this->getDisplayHash($source_commit), $max_commit->getDisplaySummary())); + $argv = array(); + $argv[] = '--no-stat'; + $argv[] = '--no-commit'; + + // When we're merging into the empty state, Git refuses to perform the + // merge until we tell it explicitly that we're doing something unusual. + if ($is_empty) { + $argv[] = '--allow-unrelated-histories'; + } + + if ($this->isSquashStrategy()) { + // NOTE: We're explicitly specifying "--ff" to override the presence + // of "merge.ff" options in user configuration. + $argv[] = '--ff'; + $argv[] = '--squash'; + } else { + $argv[] = '--no-ff'; + } + + $argv[] = '--'; + $argv[] = $source_commit; + try { - - if ($this->isSquashStrategy()) { - // NOTE: We're explicitly specifying "--ff" to override the presence - // of "merge.ff" options in user configuration. - - $api->execxLocal( - 'merge --no-stat --no-commit --ff --squash -- %s', - $source_commit); - } else { - $api->execxLocal( - 'merge --no-stat --no-commit --no-ff -- %s', - $source_commit); - } + $api->execxLocal('merge %Ls', $argv); } catch (CommandException $ex) { // TODO: If we previously succeeded with at least one merge, we could @@ -340,14 +349,23 @@ final class ArcanistGitLandEngine list($stdout) = $api->execxLocal('rev-parse --verify %s', 'HEAD'); $new_cursor = trim($stdout); - if ($into_commit === null) { + if ($is_empty) { + // See T12876. If we're landing into the empty state, we just did a fake + // merge on top of an empty commit. We're now on a commit with all of the + // right details except that it has an extra empty commit as a parent. + + // Create a new commit which is the same as the current HEAD, except that + // it doesn't have the extra parent. + + $raw_commit = $api->readRawCommit($new_cursor); if ($this->isSquashStrategy()) { - throw new Exception( - pht('TODO: Rewrite HEAD to have no parents.')); + $raw_commit->setParents(array()); } else { - throw new Exception( - pht('TODO: Rewrite HEAD to have only source as a parent.')); + $raw_commit->setParents(array($source_commit)); } + $new_cursor = $api->writeRawCommit($raw_commit); + + $api->execxLocal('checkout %s --', $new_cursor); } return $new_cursor; @@ -720,9 +738,14 @@ final class ArcanistGitLandEngine ); } - private function didHoldChanges() { + protected function didHoldChanges( + ArcanistRepositoryLocalState $state) { $log = $this->getLogEngine(); + // TODO: This probably needs updates. + + // TODO: We should refuse "--hold" if we stash. + if ($this->getIsGitPerforce()) { $this->writeInfo( pht('HOLD'), @@ -738,16 +761,15 @@ final class ArcanistGitLandEngine pht( 'Holding change locally, it has not been pushed.')); - $push_command = csprintf( - '$ git push -- %R %R:%R', - $this->getTargetRemote(), - $this->mergedRef, - $this->getTargetOnto()); + $push_command = 'TODO: ...'; + // csprintf( + // '$ git push -- %R %R:%R', + // $this->getOntoRemote(), + // $this->mergedRef, + // $this->getOnto()); } - $restore_command = csprintf( - '$ git checkout %R --', - $this->localRef); + $restore_command = 'TODO: ...'; echo tsprintf( "\n%s\n\n". diff --git a/src/land/engine/ArcanistLandEngine.php b/src/land/engine/ArcanistLandEngine.php index fa613dec..ec2107a1 100644 --- a/src/land/engine/ArcanistLandEngine.php +++ b/src/land/engine/ArcanistLandEngine.php @@ -1266,8 +1266,8 @@ abstract class ArcanistLandEngine extends Phobject { } if ($is_hold) { - $this->didHoldChanges(); - $this->discardLocalState(); + $this->didHoldChanges($local_state); + $local_state->discardLocalState(); } else { $this->reconcileLocalState($into_commit, $local_state); } @@ -1275,6 +1275,7 @@ abstract class ArcanistLandEngine extends Phobject { // TODO: Restore this. // $this->getWorkflow()->askForRepositoryUpdate(); + // TODO: This is misleading under "--hold". $log->writeSuccess( pht('DONE'), pht('Landed changes.')); @@ -1287,7 +1288,6 @@ abstract class ArcanistLandEngine extends Phobject { } } - protected function validateArguments() { $log = $this->getLogEngine(); diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php index 2a07f9dd..3610dda0 100644 --- a/src/repository/api/ArcanistGitAPI.php +++ b/src/repository/api/ArcanistGitAPI.php @@ -1753,4 +1753,22 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { ->setRepositoryAPI($this); } + public function readRawCommit($hash) { + list($stdout) = $this->execxLocal( + 'cat-file commit -- %s', + $hash); + + return ArcanistGitRawCommit::newFromRawBlob($stdout); + } + + public function writeRawCommit(ArcanistGitRawCommit $commit) { + $blob = $commit->getRawBlob(); + + $future = $this->execFutureLocal('hash-object -t commit --stdin -w'); + $future->write($blob); + list($stdout) = $future->resolvex(); + + return trim($stdout); + } + } diff --git a/src/repository/raw/ArcanistGitRawCommit.php b/src/repository/raw/ArcanistGitRawCommit.php new file mode 100644 index 00000000..7fa01de1 --- /dev/null +++ b/src/repository/raw/ArcanistGitRawCommit.php @@ -0,0 +1,183 @@ +setTreeHash(self::GIT_EMPTY_TREE_HASH); + return $raw; + } + + public static function newFromRawBlob($blob) { + $lines = phutil_split_lines($blob); + + $seen = array(); + $raw = new self(); + + $pattern = '(^(\w+) ([^\n]+)\n?\z)'; + foreach ($lines as $key => $line) { + unset($lines[$key]); + + $is_divider = ($line === "\n"); + if ($is_divider) { + break; + } + + $matches = null; + $ok = preg_match($pattern, $line, $matches); + if (!$ok) { + throw new Exception( + pht( + 'Expected to match pattern "%s" against line "%s" in raw commit '. + 'blob: %s', + $pattern, + $line, + $blob)); + } + + $label = $matches[1]; + $value = $matches[2]; + + // Detect unexpected repeated lines. + + if (isset($seen[$label])) { + switch ($label) { + case 'parent': + break; + default: + throw new Exception( + pht( + 'Encountered two "%s" lines ("%s", "%s") while parsing raw '. + 'commit blob, expected at most one: %s', + $label, + $seen[$label], + $line, + $blob)); + } + } else { + $seen[$label] = $line; + } + + switch ($label) { + case 'tree': + $raw->setTreeHash($value); + break; + case 'parent': + $raw->addParent($value); + break; + case 'author': + $raw->setRawAuthor($value); + break; + case 'committer': + $raw->setRawCommitter($value); + break; + default: + throw new Exception( + pht( + 'Unknown attribute label "%s" in line "%s" while parsing raw '. + 'commit blob: %s', + $label, + $line, + $blob)); + } + } + + $message = implode('', $lines); + $raw->setMessage($message); + + return $raw; + } + + public function getRawBlob() { + $out = array(); + + $tree = $this->getTreeHash(); + if ($tree !== null) { + $out[] = sprintf("tree %s\n", $tree); + } + + $parents = $this->getParents(); + foreach ($parents as $parent) { + $out[] = sprintf("parent %s\n", $parent); + } + + $raw_author = $this->getRawAuthor(); + if ($raw_author !== null) { + $out[] = sprintf("author %s\n", $raw_author); + } + + $raw_committer = $this->getRawCommitter(); + if ($raw_committer !== null) { + $out[] = sprintf("committer %s\n", $raw_committer); + } + + $out[] = "\n"; + + $message = $this->getMessage(); + if ($message !== null) { + $out[] = $message; + } + + return implode('', $out); + } + + public function setTreeHash($tree_hash) { + $this->treeHash = $tree_hash; + return $this; + } + + public function getTreeHash() { + return $this->treeHash; + } + + public function setRawAuthor($raw_author) { + $this->rawAuthor = $raw_author; + return $this; + } + + public function getRawAuthor() { + return $this->rawAuthor; + } + + public function setRawCommitter($raw_committer) { + $this->rawCommitter = $raw_committer; + return $this; + } + + public function getRawCommitter() { + return $this->rawCommitter; + } + + public function setParents(array $parents) { + $this->parents = $parents; + return $this; + } + + public function getParents() { + return $this->parents; + } + + public function addParent($hash) { + $this->parents[] = $hash; + return $this; + } + + public function setMessage($message) { + $this->message = $message; + return $this; + } + + public function getMessage() { + return $this->message; + } + +} diff --git a/src/repository/raw/__tests__/ArcanistGitRawCommitTestCase.php b/src/repository/raw/__tests__/ArcanistGitRawCommitTestCase.php new file mode 100644 index 00000000..8b04ba7e --- /dev/null +++ b/src/repository/raw/__tests__/ArcanistGitRawCommitTestCase.php @@ -0,0 +1,91 @@ + 'empty', + 'blob' => array( + 'tree fcfd0454eac6a28c729aa6bf7d38a5f1efc5cc5d', + '', + '', + ), + 'tree' => 'fcfd0454eac6a28c729aa6bf7d38a5f1efc5cc5d', + ), + array( + 'name' => 'parents', + 'blob' => array( + 'tree 63ece8fd5a8283f1da2c14735d059669a09ba628', + 'parent 4aebaaf60895c3f3dd32a8cadff00db2c8f74899', + 'parent 0da1a2e17d921dc27ce9afa76b123cb4c8b73b17', + 'author alice', + 'committer alice', + '', + 'Quack quack quack.', + '', + ), + 'tree' => '63ece8fd5a8283f1da2c14735d059669a09ba628', + 'parents' => array( + '4aebaaf60895c3f3dd32a8cadff00db2c8f74899', + '0da1a2e17d921dc27ce9afa76b123cb4c8b73b17', + ), + 'author' => 'alice', + 'committer' => 'alice', + 'message' => "Quack quack quack.\n", + ), + ); + + foreach ($cases as $case) { + $name = $case['name']; + $blob = $case['blob']; + + if (is_array($blob)) { + $blob = implode("\n", $blob); + } + + $raw = ArcanistGitRawCommit::newFromRawBlob($blob); + $out = $raw->getRawBlob(); + + $this->assertEqual( + $blob, + $out, + pht( + 'Expected read + write to produce the original raw Git commit '. + 'blob in case "%s".', + $name)); + + $tree = idx($case, 'tree'); + $this->assertEqual( + $tree, + $raw->getTreeHash(), + pht('Tree hashes in case "%s".', $name)); + + $parents = idx($case, 'parents', array()); + $this->assertEqual( + $parents, + $raw->getParents(), + pht('Parents in case "%s".', $name)); + + $author = idx($case, 'author'); + $this->assertEqual( + $author, + $raw->getRawAuthor(), + pht('Authors in case "%s".', $name)); + + $committer = idx($case, 'committer'); + $this->assertEqual( + $committer, + $raw->getRawCommitter(), + pht('Committer in case "%s".', $name)); + + $message = idx($case, 'message', ''); + $this->assertEqual( + $message, + $raw->getMessage(), + pht('Message in case "%s".', $name)); + } + } + +}