From 0dd947cceddc35ff01e307b1416991536c189309 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 8 Jan 2016 04:35:17 -0800 Subject: [PATCH] Move diff extraction from commits to a separate test with a CLI command Summary: Ref T9319. When we discover a commit, we sometimes update the corresponding revision with a "this is the actual committed change" diff and send out a link to the changes between review and commit. This is currently very difficult to test, because it only happens the first time and you have to either go set up a bunch of objects or add a bunch of special casing to the parser to hit the workflow. I'm making some changes to how it pulls file content. To make those changes easier to test, first start extracting this stuff so the code can be run with `bin/differential extract ...` instead of needing to do a bunch of more complicated setup steps. Test Plan: - Ran `bin/differential extract ...` to extract diffs from commits. - Forced my way through the daemon workflow by faking out a bunch of flags, got a clean extract + attach + update. After this patch, this should rarely be necessary. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9319 Differential Revision: https://secure.phabricator.com/D14967 --- bin/differential | 1 + scripts/setup/manage_differential.php | 21 +++++ src/__phutil_library_map__.php | 6 ++ .../DifferentialDiffExtractionEngine.php | 90 +++++++++++++++++++ ...PhabricatorDifferentialExtractWorkflow.php | 63 +++++++++++++ ...bricatorDifferentialManagementWorkflow.php | 4 + .../differential/storage/DifferentialDiff.php | 6 ++ ...torRepositoryCommitMessageParserWorker.php | 64 +------------ 8 files changed, 195 insertions(+), 60 deletions(-) create mode 120000 bin/differential create mode 100755 scripts/setup/manage_differential.php create mode 100644 src/applications/differential/engine/DifferentialDiffExtractionEngine.php create mode 100644 src/applications/differential/management/PhabricatorDifferentialExtractWorkflow.php create mode 100644 src/applications/differential/management/PhabricatorDifferentialManagementWorkflow.php diff --git a/bin/differential b/bin/differential new file mode 120000 index 0000000000..5a50360da3 --- /dev/null +++ b/bin/differential @@ -0,0 +1 @@ +../scripts/setup/manage_differential.php \ No newline at end of file diff --git a/scripts/setup/manage_differential.php b/scripts/setup/manage_differential.php new file mode 100755 index 0000000000..30e11d27a9 --- /dev/null +++ b/scripts/setup/manage_differential.php @@ -0,0 +1,21 @@ +#!/usr/bin/env php +setTagline(pht('manage hunks')); +$args->setSynopsis(<<parseStandardArguments(); + +$workflows = id(new PhutilClassMapQuery()) + ->setAncestorClass('PhabricatorDifferentialManagementWorkflow') + ->execute(); +$workflows[] = new PhutilHelpArgumentWorkflow(); +$args->parseWorkflows($workflows); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 0f3ecb4d42..9f2d9c0c21 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -386,6 +386,7 @@ phutil_register_library_map(array( 'DifferentialDiffContentRemovedHeraldField' => 'applications/differential/herald/DifferentialDiffContentRemovedHeraldField.php', 'DifferentialDiffCreateController' => 'applications/differential/controller/DifferentialDiffCreateController.php', 'DifferentialDiffEditor' => 'applications/differential/editor/DifferentialDiffEditor.php', + 'DifferentialDiffExtractionEngine' => 'applications/differential/engine/DifferentialDiffExtractionEngine.php', 'DifferentialDiffHeraldField' => 'applications/differential/herald/DifferentialDiffHeraldField.php', 'DifferentialDiffHeraldFieldGroup' => 'applications/differential/herald/DifferentialDiffHeraldFieldGroup.php', 'DifferentialDiffInlineCommentQuery' => 'applications/differential/query/DifferentialDiffInlineCommentQuery.php', @@ -2143,6 +2144,8 @@ phutil_register_library_map(array( 'PhabricatorDifferenceEngine' => 'infrastructure/diff/PhabricatorDifferenceEngine.php', 'PhabricatorDifferentialApplication' => 'applications/differential/application/PhabricatorDifferentialApplication.php', 'PhabricatorDifferentialConfigOptions' => 'applications/differential/config/PhabricatorDifferentialConfigOptions.php', + 'PhabricatorDifferentialExtractWorkflow' => 'applications/differential/management/PhabricatorDifferentialExtractWorkflow.php', + 'PhabricatorDifferentialManagementWorkflow' => 'applications/differential/management/PhabricatorDifferentialManagementWorkflow.php', 'PhabricatorDifferentialRevisionTestDataGenerator' => 'applications/differential/lipsum/PhabricatorDifferentialRevisionTestDataGenerator.php', 'PhabricatorDiffusionApplication' => 'applications/diffusion/application/PhabricatorDiffusionApplication.php', 'PhabricatorDiffusionConfigOptions' => 'applications/diffusion/config/PhabricatorDiffusionConfigOptions.php', @@ -4334,6 +4337,7 @@ phutil_register_library_map(array( 'DifferentialDiffContentRemovedHeraldField' => 'DifferentialDiffHeraldField', 'DifferentialDiffCreateController' => 'DifferentialController', 'DifferentialDiffEditor' => 'PhabricatorApplicationTransactionEditor', + 'DifferentialDiffExtractionEngine' => 'Phobject', 'DifferentialDiffHeraldField' => 'HeraldField', 'DifferentialDiffHeraldFieldGroup' => 'HeraldFieldGroup', 'DifferentialDiffInlineCommentQuery' => 'PhabricatorDiffInlineCommentQuery', @@ -6376,6 +6380,8 @@ phutil_register_library_map(array( 'PhabricatorDifferenceEngine' => 'Phobject', 'PhabricatorDifferentialApplication' => 'PhabricatorApplication', 'PhabricatorDifferentialConfigOptions' => 'PhabricatorApplicationConfigOptions', + 'PhabricatorDifferentialExtractWorkflow' => 'PhabricatorDifferentialManagementWorkflow', + 'PhabricatorDifferentialManagementWorkflow' => 'PhabricatorManagementWorkflow', 'PhabricatorDifferentialRevisionTestDataGenerator' => 'PhabricatorTestDataGenerator', 'PhabricatorDiffusionApplication' => 'PhabricatorApplication', 'PhabricatorDiffusionConfigOptions' => 'PhabricatorApplicationConfigOptions', diff --git a/src/applications/differential/engine/DifferentialDiffExtractionEngine.php b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php new file mode 100644 index 0000000000..a77923c417 --- /dev/null +++ b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php @@ -0,0 +1,90 @@ +viewer = $viewer; + return $this; + } + + public function getViewer() { + return $this->viewer; + } + + public function setAuthorPHID($author_phid) { + $this->authorPHID = $author_phid; + return $this; + } + + public function getAuthorPHID() { + return $this->authorPHID; + } + + public function newDiffFromCommit(PhabricatorRepositoryCommit $commit) { + $viewer = $this->getViewer(); + + $repository = $commit->getRepository(); + $identifier = $commit->getCommitIdentifier(); + $monogram = $commit->getMonogram(); + + $drequest = DiffusionRequest::newFromDictionary( + array( + 'user' => $viewer, + 'repository' => $repository, + )); + + $raw_diff = DiffusionQuery::callConduitWithDiffusionRequest( + $viewer, + $drequest, + 'diffusion.rawdiffquery', + array( + 'commit' => $identifier, + )); + + // TODO: Support adds, deletes and moves under SVN. + if (strlen($raw_diff)) { + $changes = id(new ArcanistDiffParser())->parseDiff($raw_diff); + } else { + // This is an empty diff, maybe made with `git commit --allow-empty`. + // NOTE: These diffs have the same tree hash as their ancestors, so + // they may attach to revisions in an unexpected way. Just let this + // happen for now, although it might make sense to special case it + // eventually. + $changes = array(); + } + + $diff = DifferentialDiff::newFromRawChanges($viewer, $changes) + ->setRepositoryPHID($repository->getPHID()) + ->setCreationMethod('commit') + ->setSourceControlSystem($repository->getVersionControlSystem()) + ->setLintStatus(DifferentialLintStatus::LINT_AUTO_SKIP) + ->setUnitStatus(DifferentialUnitStatus::UNIT_AUTO_SKIP) + ->setDateCreated($commit->getEpoch()) + ->setDescription($monogram); + + $author_phid = $this->getAuthorPHID(); + if ($author_phid !== null) { + $diff->setAuthorPHID($author_phid); + } + + $parents = DiffusionQuery::callConduitWithDiffusionRequest( + $viewer, + $drequest, + 'diffusion.commitparentsquery', + array( + 'commit' => $identifier, + )); + + if ($parents) { + $diff->setSourceControlBaseRevision(head($parents)); + } + + // TODO: Attach binary files. + + return $diff->save(); + } + +} diff --git a/src/applications/differential/management/PhabricatorDifferentialExtractWorkflow.php b/src/applications/differential/management/PhabricatorDifferentialExtractWorkflow.php new file mode 100644 index 0000000000..3afe232c42 --- /dev/null +++ b/src/applications/differential/management/PhabricatorDifferentialExtractWorkflow.php @@ -0,0 +1,63 @@ +setName('extract') + ->setExamples('**extract** __commit__') + ->setSynopsis(pht('Extract a diff from a commit.')) + ->setArguments( + array( + array( + 'name' => 'extract', + 'wildcard' => true, + 'help' => pht('Commit to extract.'), + ), + )); + } + + public function execute(PhutilArgumentParser $args) { + $viewer = $this->getViewer(); + + $extract = $args->getArg('extract'); + + if (!$extract) { + throw new PhutilArgumentUsageException( + pht('Specify a commit to extract the diff from.')); + } + + if (count($extract) > 1) { + throw new PhutilArgumentUsageException( + pht('Specify exactly one commit to extract.')); + } + + $extract = head($extract); + + $commit = id(new DiffusionCommitQuery()) + ->setViewer($viewer) + ->withIdentifiers(array($extract)) + ->executeOne(); + + if (!$commit) { + throw new PhutilArgumentUsageException( + pht( + 'Commit "%s" is not valid.', + $extract)); + } + + $diff = id(new DifferentialDiffExtractionEngine()) + ->setViewer($viewer) + ->newDiffFromCommit($commit); + + $uri = PhabricatorEnv::getProductionURI($diff->getURI()); + + echo tsprintf( + "%s\n\n %s\n", + pht('Extracted diff from "%s":', $extract), + $uri); + } + + +} diff --git a/src/applications/differential/management/PhabricatorDifferentialManagementWorkflow.php b/src/applications/differential/management/PhabricatorDifferentialManagementWorkflow.php new file mode 100644 index 0000000000..1a369e57ce --- /dev/null +++ b/src/applications/differential/management/PhabricatorDifferentialManagementWorkflow.php @@ -0,0 +1,4 @@ +getID(); + return "/differential/diff/{$id}/"; + } + + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php index 0bc9957aef..95ac2e47af 100644 --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php @@ -216,7 +216,10 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker $low_level_query->getRevisionMatchData()); } - $diff = $this->generateFinalDiff($revision, $acting_as_phid); + $diff = id(new DifferentialDiffExtractionEngine()) + ->setViewer($actor) + ->setAuthorPHID($acting_as_phid) + ->newDiffFromCommit($commit); $vs_diff = $this->loadChangedByCommit($revision, $diff); $changed_uri = null; @@ -277,65 +280,6 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker PhabricatorRepositoryCommit::IMPORTED_MESSAGE); } - private function generateFinalDiff( - DifferentialRevision $revision, - $actor_phid) { - - $viewer = PhabricatorUser::getOmnipotentUser(); - - $drequest = DiffusionRequest::newFromDictionary(array( - 'user' => $viewer, - 'repository' => $this->repository, - )); - - $raw_diff = DiffusionQuery::callConduitWithDiffusionRequest( - $viewer, - $drequest, - 'diffusion.rawdiffquery', - array( - 'commit' => $this->commit->getCommitIdentifier(), - )); - - // TODO: Support adds, deletes and moves under SVN. - if (strlen($raw_diff)) { - $changes = id(new ArcanistDiffParser())->parseDiff($raw_diff); - } else { - // This is an empty diff, maybe made with `git commit --allow-empty`. - // NOTE: These diffs have the same tree hash as their ancestors, so - // they may attach to revisions in an unexpected way. Just let this - // happen for now, although it might make sense to special case it - // eventually. - $changes = array(); - } - - $diff = DifferentialDiff::newFromRawChanges($viewer, $changes) - ->setRepositoryPHID($this->repository->getPHID()) - ->setAuthorPHID($actor_phid) - ->setCreationMethod('commit') - ->setSourceControlSystem($this->repository->getVersionControlSystem()) - ->setLintStatus(DifferentialLintStatus::LINT_AUTO_SKIP) - ->setUnitStatus(DifferentialUnitStatus::UNIT_AUTO_SKIP) - ->setDateCreated($this->commit->getEpoch()) - ->setDescription( - pht( - 'Commit %s', - $this->commit->getMonogram())); - - $parents = DiffusionQuery::callConduitWithDiffusionRequest( - $viewer, - $drequest, - 'diffusion.commitparentsquery', - array( - 'commit' => $this->commit->getCommitIdentifier(), - )); - if ($parents) { - $diff->setSourceControlBaseRevision(head($parents)); - } - - // TODO: Attach binary files. - - return $diff->save(); - } private function loadChangedByCommit( DifferentialRevision $revision,