From 5b6d6c4fb313718a767e34e09910a0d31ba0be1a Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 14 Apr 2019 10:46:20 -0700 Subject: [PATCH] Use repository identities, not denormalized strings, to identify authors for "Revision closed by commit X" stories Summary: Ref T12164. Ref T13276. Currently, the parsing pipeline copies the author and committer names and PHIDs into the transcaction record as metadata. They are then rendered directly from the metadata. This makes planned changes to the parsing pipeline (to prevent races when multiple commits matching a single revision are pushed simultaneously) more difficult, and generally won't work with repository identities. Instead, load the commit and use its identities. Test Plan: Loaded a revision, saw the same story rendering for a "Closed by commit" story. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13276, T12164 Differential Revision: https://secure.phabricator.com/D20418 --- .../DifferentialRevisionCloseTransaction.php | 52 ++++++++----------- ...rRepositoryManagementUnpublishWorkflow.php | 16 +++--- ...torRepositoryCommitMessageParserWorker.php | 15 +----- 3 files changed, 30 insertions(+), 53 deletions(-) diff --git a/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php b/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php index f20ba7a011..857bcc870b 100644 --- a/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php @@ -83,49 +83,41 @@ final class DifferentialRevisionCloseTransaction } public function getTitle() { - if (!$this->getMetadataValue('isCommitClose')) { + $commit_phid = $this->getMetadataValue('commitPHID'); + if ($commit_phid) { + $commit = id(new DiffusionCommitQuery()) + ->setViewer($this->getViewer()) + ->withPHIDs(array($commit_phid)) + ->needIdentities(true) + ->executeOne(); + } else { + $commit = null; + } + + if (!$commit) { return pht( '%s closed this revision.', $this->renderAuthor()); } - $commit_phid = $this->getMetadataValue('commitPHID'); - $committer_phid = $this->getMetadataValue('committerPHID'); - $author_phid = $this->getMetadataValue('authorPHID'); + $author_phid = $commit->getAuthorDisplayPHID(); + $committer_phid = $commit->getCommitterDisplayPHID(); - if ($committer_phid) { - $committer_name = $this->renderHandle($committer_phid); - } else { - $committer_name = $this->getMetadataValue('committerName'); - } - - if ($author_phid) { - $author_name = $this->renderHandle($author_phid); - } else { - $author_name = $this->getMetadatavalue('authorName'); - } - - $same_phid = - strlen($committer_phid) && - strlen($author_phid) && - ($committer_phid == $author_phid); - - $same_name = - !strlen($committer_phid) && - !strlen($author_phid) && - ($committer_name == $author_name); - - if ($same_name || $same_phid) { + if (!$author_phid) { + return pht( + 'Closed by commit %s.', + $this->renderHandle($commit_phid)); + } else if (!$committer_phid || ($committer_phid === $author_phid)) { return pht( 'Closed by commit %s (authored by %s).', $this->renderHandle($commit_phid), - $author_name); + $this->renderHandle($author_phid)); } else { return pht( 'Closed by commit %s (authored by %s, committed by %s).', $this->renderHandle($commit_phid), - $author_name, - $committer_name); + $this->renderHandle($author_phid), + $this->renderHandle($committer_phid)); } } diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementUnpublishWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementUnpublishWorkflow.php index 5a6afb8c6e..418030864d 100644 --- a/src/applications/repository/management/PhabricatorRepositoryManagementUnpublishWorkflow.php +++ b/src/applications/repository/management/PhabricatorRepositoryManagementUnpublishWorkflow.php @@ -241,15 +241,13 @@ final class PhabricatorRepositoryManagementUnpublishWorkflow if ($xactions) { foreach ($xactions as $xaction) { $metadata = $xaction->getMetadata(); - if (idx($metadata, 'isCommitClose')) { - if (idx($metadata, 'commitPHID') === $src->getPHID()) { - echo tsprintf( - "%s\n", - pht( - 'MANUAL Revision "%s" was likely closed improperly by "%s".', - $dst->getMonogram(), - $src->getMonogram())); - } + if (idx($metadata, 'commitPHID') === $src->getPHID()) { + echo tsprintf( + "%s\n", + pht( + 'MANUAL Revision "%s" was likely closed improperly by "%s".', + $dst->getMonogram(), + $src->getMonogram())); } } } diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php index b0583725a9..e6c9f499b1 100644 --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php @@ -244,24 +244,11 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker $commit_close_xaction = id(new DifferentialTransaction()) ->setTransactionType($type_close) - ->setNewValue(true) - ->setMetadataValue('isCommitClose', true); + ->setNewValue(true); $commit_close_xaction->setMetadataValue( 'commitPHID', $commit->getPHID()); - $commit_close_xaction->setMetadataValue( - 'committerPHID', - $committer_phid); - $commit_close_xaction->setMetadataValue( - 'committerName', - $data->getCommitDetail('committer')); - $commit_close_xaction->setMetadataValue( - 'authorPHID', - $author_phid); - $commit_close_xaction->setMetadataValue( - 'authorName', - $data->getAuthorName()); if ($low_level_query) { $commit_close_xaction->setMetadataValue(