From 43bee4562c72a59fcbea054427a9d239c4c96a13 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 13 Oct 2015 15:46:30 -0700 Subject: [PATCH] If the stars align, make "Land Revision" kind of work Summary: Ref T182. If 35 other things are configured completely correctly, make it remotely possible that this button may do something approximating the thing that the user wanted. This primarily fleshes out the idea that "operations" (like landing, merging or cherry-picking) can have some beahavior, and when we run an operation we do whatever that behavior is instead of just running `git show`. Broadly, this isn't too terrible because Drydock seems like it actually works properly for the most part (???!?!). Test Plan: {F876431} Reviewers: chad Reviewed By: chad Maniphest Tasks: T182 Differential Revision: https://secure.phabricator.com/D14270 --- ...ifferentialRevisionOperationController.php | 10 ++- .../differential/storage/DifferentialDiff.php | 13 +-- .../DrydockLandRepositoryOperation.php | 90 +++++++++++++++++++ .../DrydockRepositoryOperationType.php | 15 ++++ .../query/DrydockRepositoryOperationQuery.php | 15 +++- .../storage/DrydockRepositoryOperation.php | 16 ++++ ...DrydockRepositoryOperationUpdateWorker.php | 10 ++- 7 files changed, 159 insertions(+), 10 deletions(-) diff --git a/src/applications/differential/controller/DifferentialRevisionOperationController.php b/src/applications/differential/controller/DifferentialRevisionOperationController.php index 49d28787f3..6ec5b8edec 100644 --- a/src/applications/differential/controller/DifferentialRevisionOperationController.php +++ b/src/applications/differential/controller/DifferentialRevisionOperationController.php @@ -10,6 +10,7 @@ final class DifferentialRevisionOperationController $revision = id(new DifferentialRevisionQuery()) ->withIDs(array($id)) ->setViewer($viewer) + ->needActiveDiffs(true) ->executeOne(); if (!$revision) { return new Aphront404Response(); @@ -58,13 +59,20 @@ final class DifferentialRevisionOperationController } if ($request->isFormPost()) { + // NOTE: The operation is locked to the current active diff, so if the + // revision is updated before the operation applies nothing sneaky + // occurs. + + $diff = $revision->getActiveDiff(); + $op = new DrydockLandRepositoryOperation(); $operation = DrydockRepositoryOperation::initializeNewOperation($op) ->setAuthorPHID($viewer->getPHID()) ->setObjectPHID($revision->getPHID()) ->setRepositoryPHID($repository->getPHID()) - ->setRepositoryTarget('branch:master'); + ->setRepositoryTarget('branch:master') + ->setProperty('differential.diffPHID', $diff->getPHID()); $operation->save(); $operation->scheduleUpdate(); diff --git a/src/applications/differential/storage/DifferentialDiff.php b/src/applications/differential/storage/DifferentialDiff.php index 7b3a31635a..959d7f24c1 100644 --- a/src/applications/differential/storage/DifferentialDiff.php +++ b/src/applications/differential/storage/DifferentialDiff.php @@ -447,12 +447,8 @@ final class DifferentialDiff $results['repository.vcs'] = $repo->getVersionControlSystem(); $results['repository.uri'] = $repo->getPublicCloneURI(); - // TODO: We're just hoping to get lucky. Instead, `arc` should store - // where it sent changes and we should only provide staging details - // if we reasonably believe they are accurate. - $staging_ref = 'refs/tags/phabricator/diff/'.$this->getID(); $results['repository.staging.uri'] = $repo->getStagingURI(); - $results['repository.staging.ref'] = $staging_ref; + $results['repository.staging.ref'] = $this->getStagingRef(); } } @@ -480,6 +476,13 @@ final class DifferentialDiff ); } + public function getStagingRef() { + // TODO: We're just hoping to get lucky. Instead, `arc` should store + // where it sent changes and we should only provide staging details + // if we reasonably believe they are accurate. + return 'refs/tags/phabricator/diff/'.$this->getID(); + } + /* -( PhabricatorApplicationTransactionInterface )------------------------- */ diff --git a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php index d61b8e7be2..a4549e072c 100644 --- a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php +++ b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php @@ -5,4 +5,94 @@ final class DrydockLandRepositoryOperation const OPCONST = 'land'; + public function applyOperation( + DrydockRepositoryOperation $operation, + DrydockInterface $interface) { + $viewer = $this->getViewer(); + $repository = $operation->getRepository(); + + $cmd = array(); + $arg = array(); + + $object = $operation->getObject(); + 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(); + } else { + throw new Exception( + pht( + 'Invalid or unknown object ("%s") for land operation, expected '. + 'Differential Revision.', + $operation->getObjectPHID())); + } + + $target = $operation->getRepositoryTarget(); + list($type, $name) = explode(':', $target, 2); + switch ($type) { + case 'branch': + $push_dst = 'refs/heads/'.$name; + $merge_dst = 'refs/remotes/origin/'.$name; + break; + default: + throw new Exception( + pht( + 'Unknown repository operation target type "%s" (in target "%s").', + $type, + $target)); + } + + $cmd[] = 'git checkout %s'; + $arg[] = $merge_dst; + + $cmd[] = 'git merge --no-stat --squash --ff-only -- %s'; + $arg[] = $merge_src; + + $cmd[] = 'git -c user.name=%s -c user.email=%s commit --author %s -m %s'; + $arg[] = 'autocommitter'; + $arg[] = 'autocommitter@example.com'; + $arg[] = 'autoauthor '; + $arg[] = pht('(Automerge!)'); + + $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); + } + } diff --git a/src/applications/drydock/operation/DrydockRepositoryOperationType.php b/src/applications/drydock/operation/DrydockRepositoryOperationType.php index bbccc0a9bf..3ec8f8ba9e 100644 --- a/src/applications/drydock/operation/DrydockRepositoryOperationType.php +++ b/src/applications/drydock/operation/DrydockRepositoryOperationType.php @@ -2,6 +2,21 @@ abstract class DrydockRepositoryOperationType extends Phobject { + private $viewer; + + abstract public function applyOperation( + DrydockRepositoryOperation $operation, + DrydockInterface $interface); + + final public function setViewer(PhabricatorUser $viewer) { + $this->viewer = $viewer; + return $this; + } + + final public function getViewer() { + return $this->viewer; + } + final public function getOperationConstant() { return $this->getPhobjectClassConstant('OPCONST', 32); } diff --git a/src/applications/drydock/query/DrydockRepositoryOperationQuery.php b/src/applications/drydock/query/DrydockRepositoryOperationQuery.php index 409a01a07e..b72e654603 100644 --- a/src/applications/drydock/query/DrydockRepositoryOperationQuery.php +++ b/src/applications/drydock/query/DrydockRepositoryOperationQuery.php @@ -42,6 +42,19 @@ final class DrydockRepositoryOperationQuery extends DrydockQuery { } protected function willFilterPage(array $operations) { + $implementations = DrydockRepositoryOperationType::getAllOperationTypes(); + + foreach ($operations as $key => $operation) { + $impl = idx($implementations, $operation->getOperationType()); + if (!$impl) { + $this->didRejectResult($operation); + unset($operations[$key]); + continue; + } + $impl = clone $impl; + $operation->attachImplementation($impl); + } + $repository_phids = mpull($operations, 'getRepositoryPHID'); if ($repository_phids) { $repositories = id(new PhabricatorRepositoryQuery()) @@ -75,7 +88,7 @@ final class DrydockRepositoryOperationQuery extends DrydockQuery { ->setParentQuery($this) ->withPHIDs($object_phids) ->execute(); - $objects = mpull($objects, 'getPHID'); + $objects = mpull($objects, null, 'getPHID'); } else { $objects = array(); } diff --git a/src/applications/drydock/storage/DrydockRepositoryOperation.php b/src/applications/drydock/storage/DrydockRepositoryOperation.php index d5befa37b2..aed6b147fa 100644 --- a/src/applications/drydock/storage/DrydockRepositoryOperation.php +++ b/src/applications/drydock/storage/DrydockRepositoryOperation.php @@ -23,6 +23,7 @@ final class DrydockRepositoryOperation extends DrydockDAO private $repository = self::ATTACHABLE; private $object = self::ATTACHABLE; + private $implementation = self::ATTACHABLE; public static function initializeNewOperation( DrydockRepositoryOperationType $op) { @@ -77,6 +78,15 @@ final class DrydockRepositoryOperation extends DrydockDAO return $this->assertAttached($this->object); } + public function attachImplementation(DrydockRepositoryOperationType $impl) { + $this->implementation = $impl; + return $this; + } + + public function getImplementation() { + return $this->implementation; + } + public function getProperty($key, $default = null) { return idx($this->properties, $key, $default); } @@ -120,6 +130,12 @@ final class DrydockRepositoryOperation extends DrydockDAO )); } + public function applyOperation(DrydockInterface $interface) { + return $this->getImplementation()->applyOperation( + $this, + $interface); + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php b/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php index d1d1faccd8..556119f6b3 100644 --- a/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php +++ b/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php @@ -25,6 +25,8 @@ final class DrydockRepositoryOperationUpdateWorker private function handleUpdate(DrydockRepositoryOperation $operation) { + $viewer = $this->getViewer(); + $operation_state = $operation->getOperationState(); switch ($operation_state) { @@ -59,9 +61,11 @@ final class DrydockRepositoryOperationUpdateWorker // No matter what happens here, destroy the lease away once we're done. $lease->releaseOnDestruction(true); - // TODO: Some day, do useful things instead of running `git show`. - list($stdout) = $interface->execx('git show'); - phlog($stdout); + $operation->getImplementation() + ->setViewer($viewer); + + $operation->applyOperation($interface); + } catch (PhabricatorWorkerYieldException $ex) { throw $ex; } catch (Exception $ex) {