From 9c394937961758d77f911de2a5da58335b055108 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 26 Oct 2015 12:40:16 -0700 Subject: [PATCH] Make WorkingCopyBlueprint responsible for performing merges Summary: Ref T182. Currently, the "RepositoryLand" operation is responsible for performing merges when landing a revision. However, we'd like to be able to perform these merges in a larger set of cases in the future. For example: - After Releeph is revamped, when someone says "I want to merge bug fix X into stable branch Y", it would probably be nice to make that a Buildable and let tests run against it without requring that it actually be pushed anywhere. - Same deal if we want a merge-from-Diffusion or cherry-pick-from-Diffusion operation. - Similar deal if we want a "random web UI edits from Diffusion". Move the merging part into WorkingCopy so more applications can share/use it in the future. A big chunk of this is me making stuff up for now (the ol' undocumented dictionary full of arbitrary magic keys), but I anticipate formalizing it as we move along. Test Plan: Pushed rGITTEST0d58eef3ce0fa5a10732d2efefc56aec126bc219 up from my local install via "Land Revision". Reviewers: chad Reviewed By: chad Maniphest Tasks: T182 Differential Revision: https://secure.phabricator.com/D14337 --- ...dockWorkingCopyBlueprintImplementation.php | 37 +++++- .../command/DrydockCommandInterface.php | 28 +++- .../DrydockLandRepositoryOperation.php | 121 ++++++++++-------- .../DrydockRepositoryOperationType.php | 4 + .../storage/DrydockRepositoryOperation.php | 5 + ...DrydockRepositoryOperationUpdateWorker.php | 8 +- 6 files changed, 138 insertions(+), 65 deletions(-) diff --git a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php index f4b33adb8b..0eb735439a 100644 --- a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php @@ -237,8 +237,7 @@ final class DrydockWorkingCopyBlueprintImplementation $cmd = array(); $arg = array(); - $cmd[] = 'cd %s'; - $arg[] = "{$root}/repo/{$directory}/"; + $interface->pushWorkingDirectory("{$root}/repo/{$directory}/"); $cmd[] = 'git clean -d --force'; $cmd[] = 'git fetch'; @@ -285,6 +284,15 @@ final class DrydockWorkingCopyBlueprintImplementation if (idx($spec, 'default')) { $default = $directory; } + + $merges = idx($spec, 'merges'); + if ($merges) { + foreach ($merges as $merge) { + $this->applyMerge($interface, $merge); + } + } + + $interface->popWorkingDirectory(); } if ($default === null) { @@ -333,7 +341,7 @@ final class DrydockWorkingCopyBlueprintImplementation $command_interface = $host_lease->getInterface($type); $path = $lease->getAttribute('workingcopy.default'); - $command_interface->setWorkingDirectory($path); + $command_interface->pushWorkingDirectory($path); return $command_interface; } @@ -407,5 +415,28 @@ final class DrydockWorkingCopyBlueprintImplementation return true; } + private function applyMerge( + DrydockCommandInterface $interface, + array $merge) { + + $src_uri = $merge['src.uri']; + $src_ref = $merge['src.ref']; + + $interface->execx( + 'git fetch --no-tags -- %s +%s:%s', + $src_uri, + $src_ref, + $src_ref); + + try { + $interface->execx( + 'git merge --no-stat --squash --ff-only -- %s', + $src_ref); + } catch (CommandException $ex) { + // TODO: Specifically note this as a merge conflict. + throw $ex; + } + } + } diff --git a/src/applications/drydock/interface/command/DrydockCommandInterface.php b/src/applications/drydock/interface/command/DrydockCommandInterface.php index 8034e0f732..f6b8b132da 100644 --- a/src/applications/drydock/interface/command/DrydockCommandInterface.php +++ b/src/applications/drydock/interface/command/DrydockCommandInterface.php @@ -4,15 +4,27 @@ abstract class DrydockCommandInterface extends DrydockInterface { const INTERFACE_TYPE = 'command'; - private $workingDirectory; + private $workingDirectoryStack = array(); - public function setWorkingDirectory($working_directory) { - $this->workingDirectory = $working_directory; + public function pushWorkingDirectory($working_directory) { + $this->workingDirectoryStack[] = $working_directory; return $this; } - public function getWorkingDirectory() { - return $this->workingDirectory; + public function popWorkingDirectory() { + if (!$this->workingDirectoryStack) { + throw new Exception( + pht( + 'Unable to pop working directory, directory stack is empty.')); + } + return array_pop($this->workingDirectoryStack); + } + + public function peekWorkingDirectory() { + if ($this->workingDirectoryStack) { + return last($this->workingDirectoryStack); + } + return null; } final public function getInterfaceType() { @@ -38,12 +50,14 @@ abstract class DrydockCommandInterface extends DrydockInterface { abstract public function getExecFuture($command); protected function applyWorkingDirectoryToArgv(array $argv) { - if ($this->getWorkingDirectory() !== null) { + $directory = $this->peekWorkingDirectory(); + + if ($directory !== null) { $cmd = $argv[0]; $cmd = "(cd %s && {$cmd})"; $argv = array_merge( array($cmd), - array($this->getWorkingDirectory()), + array($directory), array_slice($argv, 1)); } diff --git a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php index df888f3f6f..00b5c71181 100644 --- a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php +++ b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php @@ -35,6 +35,28 @@ final class DrydockLandRepositoryOperation } } + public function getWorkingCopyMerges(DrydockRepositoryOperation $operation) { + $repository = $operation->getRepository(); + $merges = array(); + + $object = $operation->getObject(); + if ($object instanceof DifferentialRevision) { + $diff = $this->loadDiff($operation); + $merges[] = array( + 'src.uri' => $repository->getStagingURI(), + 'src.ref' => $diff->getStagingRef(), + ); + } else { + throw new Exception( + pht( + 'Invalid or unknown object ("%s") for land operation, expected '. + 'Differential Revision.', + $operation->getObjectPHID())); + } + + return $merges; + } + public function applyOperation( DrydockRepositoryOperation $operation, DrydockInterface $interface) { @@ -48,36 +70,7 @@ final class DrydockLandRepositoryOperation if ($object instanceof DifferentialRevision) { $revision = $object; - $diff_phid = $operation->getProperty('differential.diffPHID'); - - $diff = id(new DifferentialDiffQuery()) - ->setViewer($viewer) - ->withPHIDs(array($diff_phid)) - ->executeOne(); - if (!$diff) { - throw new Exception( - pht( - 'Unable to load diff "%s".', - $diff_phid)); - } - - $diff_revid = $diff->getRevisionID(); - $revision_id = $revision->getID(); - if ($diff_revid != $revision_id) { - throw new Exception( - pht( - 'Diff ("%s") has wrong revision ID ("%s", expected "%s").', - $diff_phid, - $diff_revid, - $revision_id)); - } - - $cmd[] = 'git fetch --no-tags -- %s +%s:%s'; - $arg[] = $repository->getStagingURI(); - $arg[] = $diff->getStagingRef(); - $arg[] = $diff->getStagingRef(); - - $merge_src = $diff->getStagingRef(); + $diff = $this->loadDiff($operation); $dict = $diff->getDiffAuthorshipDict(); $author_name = idx($dict, 'authorName'); @@ -104,7 +97,6 @@ final class DrydockLandRepositoryOperation switch ($type) { case 'branch': $push_dst = 'refs/heads/'.$name; - $merge_dst = 'refs/remotes/origin/'.$name; break; default: throw new Exception( @@ -116,30 +108,24 @@ final class DrydockLandRepositoryOperation $committer_info = $this->getCommitterInfo($operation); - $cmd[] = 'git checkout %s'; - $arg[] = $merge_dst; + // NOTE: We're doing this commit with "-F -" so we don't run into trouble + // with enormous commit messages which might otherwise exceed the maximum + // size of a command. - $cmd[] = 'git merge --no-stat --squash --ff-only -- %s'; - $arg[] = $merge_src; + $future = $interface->getExecFuture( + 'git -c user.name=%s -c user.email=%s commit --author %s -F - --', + $committer_info['name'], + $committer_info['email'], + "{$author_name} <{$author_email}>"); - $cmd[] = 'git -c user.name=%s -c user.email=%s commit --author %s -m %s'; + $future + ->write($commit_message) + ->resolvex(); - $arg[] = $committer_info['name']; - $arg[] = $committer_info['email']; - - $arg[] = "{$author_name} <{$author_email}>"; - $arg[] = $commit_message; - - $cmd[] = 'git push origin -- %s:%s'; - $arg[] = 'HEAD'; - $arg[] = $push_dst; - - $cmd = implode(' && ', $cmd); - $argv = array_merge(array($cmd), $arg); - - $result = call_user_func_array( - array($interface, 'execx'), - $argv); + $interface->execx( + 'git push origin -- %s:%s', + 'HEAD', + $push_dst); } private function getCommitterInfo(DrydockRepositoryOperation $operation) { @@ -172,4 +158,35 @@ final class DrydockLandRepositoryOperation ); } + private function loadDiff(DrydockRepositoryOperation $operation) { + $viewer = $this->getViewer(); + $revision = $operation->getObject(); + + $diff_phid = $operation->getProperty('differential.diffPHID'); + + $diff = id(new DifferentialDiffQuery()) + ->setViewer($viewer) + ->withPHIDs(array($diff_phid)) + ->executeOne(); + if (!$diff) { + throw new Exception( + pht( + 'Unable to load diff "%s".', + $diff_phid)); + } + + $diff_revid = $diff->getRevisionID(); + $revision_id = $revision->getID(); + if ($diff_revid != $revision_id) { + throw new Exception( + pht( + 'Diff ("%s") has wrong revision ID ("%s", expected "%s").', + $diff_phid, + $diff_revid, + $revision_id)); + } + + return $diff; + } + } diff --git a/src/applications/drydock/operation/DrydockRepositoryOperationType.php b/src/applications/drydock/operation/DrydockRepositoryOperationType.php index 1ae6d6e4f8..7ec3689aff 100644 --- a/src/applications/drydock/operation/DrydockRepositoryOperationType.php +++ b/src/applications/drydock/operation/DrydockRepositoryOperationType.php @@ -16,6 +16,10 @@ abstract class DrydockRepositoryOperationType extends Phobject { DrydockRepositoryOperation $operation, PhabricatorUser $viewer); + public function getWorkingCopyMerges(DrydockRepositoryOperation $operation) { + return array(); + } + final public function setViewer(PhabricatorUser $viewer) { $this->viewer = $viewer; return $this; diff --git a/src/applications/drydock/storage/DrydockRepositoryOperation.php b/src/applications/drydock/storage/DrydockRepositoryOperation.php index 2ee09f3227..6ee86bf5bb 100644 --- a/src/applications/drydock/storage/DrydockRepositoryOperation.php +++ b/src/applications/drydock/storage/DrydockRepositoryOperation.php @@ -158,6 +158,11 @@ final class DrydockRepositoryOperation extends DrydockDAO return false; } + public function getWorkingCopyMerges() { + return $this->getImplementation()->getWorkingCopyMerges( + $this); + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php b/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php index 556119f6b3..e0361d3eed 100644 --- a/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php +++ b/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php @@ -53,6 +53,9 @@ final class DrydockRepositoryOperationUpdateWorker // waiting for a lease we're holding. try { + $operation->getImplementation() + ->setViewer($viewer); + $lease = $this->loadWorkingCopyLease($operation); $interface = $lease->getInterface( @@ -61,9 +64,6 @@ final class DrydockRepositoryOperationUpdateWorker // No matter what happens here, destroy the lease away once we're done. $lease->releaseOnDestruction(true); - $operation->getImplementation() - ->setViewer($viewer); - $operation->applyOperation($interface); } catch (PhabricatorWorkerYieldException $ex) { @@ -166,6 +166,8 @@ final class DrydockRepositoryOperationUpdateWorker $target)); } + $spec['merges'] = $operation->getWorkingCopyMerges(); + $map = array(); $map[$repository->getCloneName()] = array( 'phid' => $repository->getPHID(),