From ccd37afab860b12f036731e6a4d8ab55f06ec2fd Mon Sep 17 00:00:00 2001 From: vrana Date: Mon, 21 May 2012 17:55:35 -0700 Subject: [PATCH] Attach commit diff to its revision Summary: This attaches commit diff to its associated revision as any other diff. The consequence is that the revision page now shows the actual commit instead of the last diff. It may be disturbing but it is desired. Another consequence is that lint and unit results are displayed as skipped after the revision was committed. I want to fix it somehow. My next plan is to automatically diff against the last normal diff and include the link to this diff in commit e-mail if non-empty. Test Plan: reparse.php Diff against last normal diff. Reviewers: epriestley, jungejason Reviewed By: jungejason CC: aran, Koolvin, jungejason Maniphest Tasks: T201 Differential Revision: https://secure.phabricator.com/D2530 --- .../DifferentialRevisionViewController.php | 17 ++++-- .../DifferentialRevisionUpdateHistoryView.php | 3 +- ...torRepositoryCommitMessageParserWorker.php | 54 ++++++++++++++++--- .../commitmessageparser/base/__init__.php | 7 +++ 4 files changed, 68 insertions(+), 13 deletions(-) diff --git a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php index 1b3d3b96f6..92ba4b7c65 100644 --- a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php @@ -58,6 +58,15 @@ final class DifferentialRevisionViewController extends DifferentialController { } } + $target_manual = $target; + if (!$target_id) { + foreach ($diffs as $diff) { + if ($diff->getCreationMethod() != 'commit') { + $target_manual = $diff; + } + } + } + $diffs = mpull($diffs, null, 'getID'); if (empty($diffs[$diff_vs])) { $diff_vs = null; @@ -65,7 +74,7 @@ final class DifferentialRevisionViewController extends DifferentialController { list($aux_fields, $props) = $this->loadAuxiliaryFieldsAndProperties( $revision, - $target, + $target_manual, array( 'local:commits', 'arc:unit', @@ -205,18 +214,17 @@ final class DifferentialRevisionViewController extends DifferentialController { newv($custom_renderer_class, array()); $actions = array_merge( $actions, - $custom_renderer->generateActionLinks($revision, $target)); + $custom_renderer->generateActionLinks($revision, $target_manual)); } $whitespace = $request->getStr( 'whitespace', DifferentialChangesetParser::WHITESPACE_IGNORE_ALL); - $arc_project = $target->loadArcanistProject(); + $arc_project = $target_manual->loadArcanistProject(); if ($arc_project) { $symbol_indexes = $this->buildSymbolIndexes( - $target, $arc_project, $visible_changesets); $repository = $arc_project->loadRepository(); @@ -704,7 +712,6 @@ final class DifferentialRevisionViewController extends DifferentialController { } private function buildSymbolIndexes( - DifferentialDiff $target, PhabricatorRepositoryArcanistProject $arc_project, array $visible_changesets) { assert_instances_of($visible_changesets, 'DifferentialChangeset'); diff --git a/src/applications/differential/view/revisionupdatehistory/DifferentialRevisionUpdateHistoryView.php b/src/applications/differential/view/revisionupdatehistory/DifferentialRevisionUpdateHistoryView.php index 6a10980929..d3c242b4d5 100644 --- a/src/applications/differential/view/revisionupdatehistory/DifferentialRevisionUpdateHistoryView.php +++ b/src/applications/differential/view/revisionupdatehistory/DifferentialRevisionUpdateHistoryView.php @@ -24,7 +24,8 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView { private $selectedWhitespace; private $user; - public function setDiffs($diffs) { + public function setDiffs(array $diffs) { + assert_instances_of($diffs, 'DifferentialDiff'); $this->diffs = $diffs; return $this; } diff --git a/src/applications/repository/worker/commitmessageparser/base/PhabricatorRepositoryCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/base/PhabricatorRepositoryCommitMessageParserWorker.php index e8dc74330f..6436a7260f 100644 --- a/src/applications/repository/worker/commitmessageparser/base/PhabricatorRepositoryCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/base/PhabricatorRepositoryCommitMessageParserWorker.php @@ -92,6 +92,14 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker DifferentialRevision::TABLE_COMMIT, $revision->getID(), $commit->getPHID()); + $commit_is_new = $conn_w->getAffectedRows(); + + $message = null; + $committer = $data->getCommitDetail('authorPHID'); + if (!$committer) { + $committer = $revision->getAuthorPHID(); + $message = 'Closed by '.$data->getAuthorName().'.'; + } $status_closed = ArcanistDifferentialRevisionStatus::CLOSED; $should_close = ($revision->getStatus() != $status_closed) && @@ -99,13 +107,6 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker if ($should_close) { $revision->setDateCommitted($commit->getEpoch()); - - $message = null; - $committer = $data->getCommitDetail('authorPHID'); - if (!$committer) { - $committer = $revision->getAuthorPHID(); - $message = 'Closed by '.$data->getAuthorName().'.'; - } $editor = new DifferentialCommentEditor( $revision, $committer, @@ -113,10 +114,49 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker $editor->setIsDaemonWorkflow(true); $editor->setMessage($message)->save(); } + + if ($commit_is_new) { + $this->attachToRevision($revision, $committer); + } } } } + private function attachToRevision( + DifferentialRevision $revision, + $committer) { + + $drequest = DiffusionRequest::newFromDictionary(array( + 'repository' => $this->repository, + 'commit' => $this->commit->getCommitIdentifier(), + )); + + $raw_diff = DiffusionRawDiffQuery::newFromDiffusionRequest($drequest) + ->loadRawDiff(); + + $changes = id(new ArcanistDiffParser())->parseDiff($raw_diff); + $diff = DifferentialDiff::newFromRawChanges($changes) + ->setRevisionID($revision->getID()) + ->setAuthorPHID($committer) + ->setCreationMethod('commit') + ->setSourceControlSystem($this->repository->getVersionControlSystem()) + ->setLintStatus(DifferentialLintStatus::LINT_SKIP) + ->setUnitStatus(DifferentialUnitStatus::UNIT_SKIP) + ->setDateCreated($this->commit->getEpoch()) + ->setDescription( + 'Commit r'. + $this->repository->getCallsign(). + $this->commit->getCommitIdentifier()); + + $parents = DiffusionCommitParentsQuery::newFromDiffusionRequest($drequest) + ->loadParents(); + if ($parents) { + $diff->setSourceControlBaseRevision(head_key($parents)); + } + + $diff->save(); + } + /** * When querying for revisions by hash, more than one revision may be found. * This function identifies the "best" revision from such a set. Typically, diff --git a/src/applications/repository/worker/commitmessageparser/base/__init__.php b/src/applications/repository/worker/commitmessageparser/base/__init__.php index 2822576917..aa3e46001a 100644 --- a/src/applications/repository/worker/commitmessageparser/base/__init__.php +++ b/src/applications/repository/worker/commitmessageparser/base/__init__.php @@ -7,11 +7,18 @@ phutil_require_module('arcanist', 'differential/constants/revisionstatus'); +phutil_require_module('arcanist', 'parser/diff'); phutil_require_module('phabricator', 'applications/differential/constants/action'); +phutil_require_module('phabricator', 'applications/differential/constants/lintstatus'); +phutil_require_module('phabricator', 'applications/differential/constants/unitstatus'); phutil_require_module('phabricator', 'applications/differential/editor/comment'); phutil_require_module('phabricator', 'applications/differential/query/revision'); +phutil_require_module('phabricator', 'applications/differential/storage/diff'); phutil_require_module('phabricator', 'applications/differential/storage/revision'); +phutil_require_module('phabricator', 'applications/diffusion/query/parents/base'); +phutil_require_module('phabricator', 'applications/diffusion/query/rawdiff/base'); +phutil_require_module('phabricator', 'applications/diffusion/request/base'); phutil_require_module('phabricator', 'applications/repository/storage/commitdata'); phutil_require_module('phabricator', 'applications/repository/worker/base'); phutil_require_module('phabricator', 'storage/queryfx');