From 413ca12fda6b97cd64a01b7b974dc8343f6d4c11 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 8 Jan 2016 04:59:31 -0800 Subject: [PATCH] Move commit attachment to a separate CLI command Summary: Ref T9319. See D14967. As before, this is making a deeply-buried, complex operation easier to test by providing a CLI command. This adds `bin/differential attach-commit rXnnnn Dnnnn` to pretend that `rXnnnn` was just committed and matched `Dnnnn`. Test Plan: - Ran `bin/differential attach-commit X Y` for several different values, saw updates in the UI. - Faked the message parser to make sure stuff still worked there. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9319 Differential Revision: https://secure.phabricator.com/D14968 --- src/__phutil_library_map__.php | 2 + .../DifferentialDiffExtractionEngine.php | 170 ++++++++++++++++++ ...icatorDifferentialAttachCommitWorkflow.php | 90 ++++++++++ ...torRepositoryCommitMessageParserWorker.php | 154 ++-------------- 4 files changed, 273 insertions(+), 143 deletions(-) create mode 100644 src/applications/differential/management/PhabricatorDifferentialAttachCommitWorkflow.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 9f2d9c0c21..9639eef11e 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2143,6 +2143,7 @@ phutil_register_library_map(array( 'PhabricatorDiffPreferencesSettingsPanel' => 'applications/settings/panel/PhabricatorDiffPreferencesSettingsPanel.php', 'PhabricatorDifferenceEngine' => 'infrastructure/diff/PhabricatorDifferenceEngine.php', 'PhabricatorDifferentialApplication' => 'applications/differential/application/PhabricatorDifferentialApplication.php', + 'PhabricatorDifferentialAttachCommitWorkflow' => 'applications/differential/management/PhabricatorDifferentialAttachCommitWorkflow.php', 'PhabricatorDifferentialConfigOptions' => 'applications/differential/config/PhabricatorDifferentialConfigOptions.php', 'PhabricatorDifferentialExtractWorkflow' => 'applications/differential/management/PhabricatorDifferentialExtractWorkflow.php', 'PhabricatorDifferentialManagementWorkflow' => 'applications/differential/management/PhabricatorDifferentialManagementWorkflow.php', @@ -6379,6 +6380,7 @@ phutil_register_library_map(array( 'PhabricatorDiffPreferencesSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorDifferenceEngine' => 'Phobject', 'PhabricatorDifferentialApplication' => 'PhabricatorApplication', + 'PhabricatorDifferentialAttachCommitWorkflow' => 'PhabricatorDifferentialManagementWorkflow', 'PhabricatorDifferentialConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorDifferentialExtractWorkflow' => 'PhabricatorDifferentialManagementWorkflow', 'PhabricatorDifferentialManagementWorkflow' => 'PhabricatorManagementWorkflow', diff --git a/src/applications/differential/engine/DifferentialDiffExtractionEngine.php b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php index a77923c417..b2747e8ac8 100644 --- a/src/applications/differential/engine/DifferentialDiffExtractionEngine.php +++ b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php @@ -87,4 +87,174 @@ final class DifferentialDiffExtractionEngine extends Phobject { return $diff->save(); } + public function isDiffChangedBeforeCommit( + PhabricatorRepositoryCommit $commit, + DifferentialDiff $old, + DifferentialDiff $new) { + + $viewer = $this->getViewer(); + $repository = $commit->getRepository(); + $identifier = $commit->getCommitIdentifier(); + + $vs_changesets = array(); + foreach ($old->getChangesets() as $changeset) { + $path = $changeset->getAbsoluteRepositoryPath($repository, $old); + $path = ltrim($path, '/'); + $vs_changesets[$path] = $changeset; + } + + $changesets = array(); + foreach ($new->getChangesets() as $changeset) { + $path = $changeset->getAbsoluteRepositoryPath($repository, $new); + $path = ltrim($path, '/'); + $changesets[$path] = $changeset; + } + + if (array_fill_keys(array_keys($changesets), true) != + array_fill_keys(array_keys($vs_changesets), true)) { + return true; + } + + $file_phids = array(); + foreach ($vs_changesets as $changeset) { + $metadata = $changeset->getMetadata(); + $file_phid = idx($metadata, 'new:binary-phid'); + if ($file_phid) { + $file_phids[$file_phid] = $file_phid; + } + } + + $files = array(); + if ($file_phids) { + $files = id(new PhabricatorFileQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs($file_phids) + ->execute(); + $files = mpull($files, null, 'getPHID'); + } + + foreach ($changesets as $path => $changeset) { + $vs_changeset = $vs_changesets[$path]; + + $file_phid = idx($vs_changeset->getMetadata(), 'new:binary-phid'); + if ($file_phid) { + if (!isset($files[$file_phid])) { + return true; + } + + $drequest = DiffusionRequest::newFromDictionary(array( + 'user' => $viewer, + 'repository' => $repository, + 'commit' => $identifier, + 'path' => $path, + )); + + $corpus = DiffusionFileContentQuery::newFromDiffusionRequest($drequest) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->loadFileContent() + ->getCorpus(); + + if ($files[$file_phid]->loadFileData() != $corpus) { + return true; + } + } else { + $context = implode("\n", $changeset->makeChangesWithContext()); + $vs_context = implode("\n", $vs_changeset->makeChangesWithContext()); + + // We couldn't just compare $context and $vs_context because following + // diffs will be considered different: + // + // -(empty line) + // -echo 'test'; + // (empty line) + // + // (empty line) + // -echo "test"; + // -(empty line) + + $hunk = id(new DifferentialModernHunk())->setChanges($context); + $vs_hunk = id(new DifferentialModernHunk())->setChanges($vs_context); + if ($hunk->makeOldFile() != $vs_hunk->makeOldFile() || + $hunk->makeNewFile() != $vs_hunk->makeNewFile()) { + return true; + } + } + } + + return false; + } + + public function updateRevisionWithCommit( + DifferentialRevision $revision, + PhabricatorRepositoryCommit $commit, + array $more_xactions, + PhabricatorContentSource $content_source) { + + $viewer = $this->getViewer(); + $result_data = array(); + + $new_diff = $this->newDiffFromCommit($commit); + + $old_diff = $revision->getActiveDiff(); + $changed_uri = null; + if ($old_diff) { + $old_diff = id(new DifferentialDiffQuery()) + ->setViewer($viewer) + ->withIDs(array($old_diff->getID())) + ->needChangesets(true) + ->executeOne(); + if ($old_diff) { + $has_changed = $this->isDiffChangedBeforeCommit( + $commit, + $old_diff, + $new_diff); + if ($has_changed) { + $result_data['vsDiff'] = $old_diff->getID(); + + $revision_monogram = $revision->getMonogram(); + $old_id = $old_diff->getID(); + $new_id = $new_diff->getID(); + + $changed_uri = "/{$revision_monogram}?vs={$old_id}&id={$new_id}#toc"; + $changed_uri = PhabricatorEnv::getProductionURI($changed_uri); + } + } + } + + $xactions = array(); + + $xactions[] = id(new DifferentialTransaction()) + ->setTransactionType(DifferentialTransaction::TYPE_UPDATE) + ->setIgnoreOnNoEffect(true) + ->setNewValue($new_diff->getPHID()) + ->setMetadataValue('isCommitUpdate', true); + + foreach ($more_xactions as $more_xaction) { + $xactions[] = $more_xaction; + } + + $editor = id(new DifferentialTransactionEditor()) + ->setActor($viewer) + ->setContinueOnMissingFields(true) + ->setContentSource($content_source) + ->setChangedPriorToCommitURI($changed_uri) + ->setIsCloseByCommit(true); + + $author_phid = $this->getAuthorPHID(); + if ($author_phid !== null) { + $editor->setActingAsPHID($author_phid); + } + + try { + $editor->applyTransactions($revision, $xactions); + } catch (PhabricatorApplicationTransactionNoEffectException $ex) { + // NOTE: We've marked transactions other than the CLOSE transaction + // as ignored when they don't have an effect, so this means that we + // lost a race to close the revision. That's perfectly fine, we can + // just continue normally. + } + + return $result_data; + } + } diff --git a/src/applications/differential/management/PhabricatorDifferentialAttachCommitWorkflow.php b/src/applications/differential/management/PhabricatorDifferentialAttachCommitWorkflow.php new file mode 100644 index 0000000000..5150dba315 --- /dev/null +++ b/src/applications/differential/management/PhabricatorDifferentialAttachCommitWorkflow.php @@ -0,0 +1,90 @@ +setName('attach-commit') + ->setExamples('**attach-commit** __commit__ __revision__') + ->setSynopsis(pht('Forcefully attach a commit to a revision.')) + ->setArguments( + array( + array( + 'name' => 'argv', + 'wildcard' => true, + 'help' => pht('Commit, and a revision to attach it to.'), + ), + )); + } + + public function execute(PhutilArgumentParser $args) { + $viewer = $this->getViewer(); + + $argv = $args->getArg('argv'); + if (count($argv) !== 2) { + throw new PhutilArgumentUsageException( + pht('Specify a commit and a revision to attach it to.')); + } + + $commit_name = head($argv); + $revision_name = last($argv); + + $commit = id(new DiffusionCommitQuery()) + ->setViewer($viewer) + ->withIdentifiers(array($commit_name)) + ->executeOne(); + if (!$commit) { + throw new PhutilArgumentUsageException( + pht('Commit "%s" does not exist.', $commit_name)); + } + + $revision = id(new PhabricatorObjectQuery()) + ->setViewer($viewer) + ->withNames(array($revision_name)) + ->executeOne(); + + if (!$revision) { + throw new PhutilArgumentUsageException( + pht('Revision "%s" does not exist.', $revision_name)); + } + + if (!($revision instanceof DifferentialRevision)) { + throw new PhutilArgumentUsageException( + pht('Object "%s" must be a Differential revision.', $revision_name)); + } + + // Reload the revision to get the active diff. + $revision = id(new DifferentialRevisionQuery()) + ->setViewer($viewer) + ->withIDs(array($revision->getID())) + ->needActiveDiffs(true) + ->executeOne(); + + $differential_phid = id(new PhabricatorDifferentialApplication()) + ->getPHID(); + + $extraction_engine = id(new DifferentialDiffExtractionEngine()) + ->setViewer($viewer) + ->setAuthorPHID($differential_phid); + + $content_source = PhabricatorContentSource::newForSource( + PhabricatorContentSource::SOURCE_CONSOLE, + array()); + + $extraction_engine->updateRevisionWithCommit( + $revision, + $commit, + array(), + $content_source); + + echo tsprintf( + "%s\n", + pht( + 'Attached "%s" to "%s".', + $commit->getMonogram(), + $revision->getMonogram())); + } + + +} diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php index 95ac2e47af..17a2f3a3f2 100644 --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php @@ -216,50 +216,24 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker $low_level_query->getRevisionMatchData()); } - $diff = id(new DifferentialDiffExtractionEngine()) + $extraction_engine = id(new DifferentialDiffExtractionEngine()) ->setViewer($actor) - ->setAuthorPHID($acting_as_phid) - ->newDiffFromCommit($commit); - - $vs_diff = $this->loadChangedByCommit($revision, $diff); - $changed_uri = null; - if ($vs_diff) { - $data->setCommitDetail('vsDiff', $vs_diff->getID()); - - $changed_uri = PhabricatorEnv::getProductionURI( - '/D'.$revision->getID(). - '?vs='.$vs_diff->getID(). - '&id='.$diff->getID(). - '#toc'); - } - - $xactions = array(); - $xactions[] = id(new DifferentialTransaction()) - ->setTransactionType(DifferentialTransaction::TYPE_UPDATE) - ->setIgnoreOnNoEffect(true) - ->setNewValue($diff->getPHID()) - ->setMetadataValue('isCommitUpdate', true); - $xactions[] = $commit_close_xaction; + ->setAuthorPHID($acting_as_phid); $content_source = PhabricatorContentSource::newForSource( PhabricatorContentSource::SOURCE_DAEMON, array()); - $editor = id(new DifferentialTransactionEditor()) - ->setActor($actor) - ->setActingAsPHID($acting_as_phid) - ->setContinueOnMissingFields(true) - ->setContentSource($content_source) - ->setChangedPriorToCommitURI($changed_uri) - ->setIsCloseByCommit(true); + $update_data = $extraction_engine->updateRevisionWithCommit( + $revision, + $commit, + array( + $commit_close_xaction, + ), + $content_source); - try { - $editor->applyTransactions($revision, $xactions); - } catch (PhabricatorApplicationTransactionNoEffectException $ex) { - // NOTE: We've marked transactions other than the CLOSE transaction - // as ignored when they don't have an effect, so this means that we - // lost a race to close the revision. That's perfectly fine, we can - // just continue normally. + foreach ($update_data as $key => $value) { + $data->setCommitDetail($key, $value); } } } @@ -280,112 +254,6 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker PhabricatorRepositoryCommit::IMPORTED_MESSAGE); } - - private function loadChangedByCommit( - DifferentialRevision $revision, - DifferentialDiff $diff) { - - $repository = $this->repository; - - $vs_diff = id(new DifferentialDiffQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withRevisionIDs(array($revision->getID())) - ->needChangesets(true) - ->setLimit(1) - ->executeOne(); - if (!$vs_diff) { - return null; - } - - if ($vs_diff->getCreationMethod() == 'commit') { - return null; - } - - $vs_changesets = array(); - foreach ($vs_diff->getChangesets() as $changeset) { - $path = $changeset->getAbsoluteRepositoryPath($repository, $vs_diff); - $path = ltrim($path, '/'); - $vs_changesets[$path] = $changeset; - } - - $changesets = array(); - foreach ($diff->getChangesets() as $changeset) { - $path = $changeset->getAbsoluteRepositoryPath($repository, $diff); - $path = ltrim($path, '/'); - $changesets[$path] = $changeset; - } - - if (array_fill_keys(array_keys($changesets), true) != - array_fill_keys(array_keys($vs_changesets), true)) { - return $vs_diff; - } - - $file_phids = array(); - foreach ($vs_changesets as $changeset) { - $metadata = $changeset->getMetadata(); - $file_phid = idx($metadata, 'new:binary-phid'); - if ($file_phid) { - $file_phids[$file_phid] = $file_phid; - } - } - - $files = array(); - if ($file_phids) { - $files = id(new PhabricatorFileQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withPHIDs($file_phids) - ->execute(); - $files = mpull($files, null, 'getPHID'); - } - - foreach ($changesets as $path => $changeset) { - $vs_changeset = $vs_changesets[$path]; - - $file_phid = idx($vs_changeset->getMetadata(), 'new:binary-phid'); - if ($file_phid) { - if (!isset($files[$file_phid])) { - return $vs_diff; - } - $drequest = DiffusionRequest::newFromDictionary(array( - 'user' => PhabricatorUser::getOmnipotentUser(), - 'repository' => $this->repository, - 'commit' => $this->commit->getCommitIdentifier(), - 'path' => $path, - )); - $corpus = DiffusionFileContentQuery::newFromDiffusionRequest($drequest) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->loadFileContent() - ->getCorpus(); - if ($files[$file_phid]->loadFileData() != $corpus) { - return $vs_diff; - } - } else { - $context = implode("\n", $changeset->makeChangesWithContext()); - $vs_context = implode("\n", $vs_changeset->makeChangesWithContext()); - - // We couldn't just compare $context and $vs_context because following - // diffs will be considered different: - // - // -(empty line) - // -echo 'test'; - // (empty line) - // - // (empty line) - // -echo "test"; - // -(empty line) - - $hunk = id(new DifferentialModernHunk())->setChanges($context); - $vs_hunk = id(new DifferentialModernHunk())->setChanges($vs_context); - if ($hunk->makeOldFile() != $vs_hunk->makeOldFile() || - $hunk->makeNewFile() != $vs_hunk->makeNewFile()) { - return $vs_diff; - } - } - } - - return null; - } - private function resolveUserPHID( PhabricatorRepositoryCommit $commit, $user_name) {