From 1af6f215733f2da804c79dc9fa730866d7557314 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Mon, 13 Oct 2014 16:55:26 -0700 Subject: [PATCH] Increase clarity when closing a revision in response to a commit Summary: I am not sure how valuable this is *as is* - I think it needs different explanations for what happened in mercurial or subversion? I do not know what those explanations are. Made an error in D10485 - the $hashes that were saved is an array of objects, so it ends up turning into garbage via the wonders of serialization and de-serialization. Fix that by explicitly saving the tree hash. I would like to make this work for the other VCS types we support, add the "undo / nope" button and call it fixed. Ref T3686. Test Plan: clicked "explan why" and saw why Reviewers: epriestley Reviewed By: epriestley Subscribers: epriestley, Korvin Maniphest Tasks: T5693, T3686 Differential Revision: https://secure.phabricator.com/D10489 --- src/__phutil_library_map__.php | 2 + .../PhabricatorDifferentialApplication.php | 2 + ...tialParseCommitMessageConduitAPIMethod.php | 11 ++ ...erentialRevisionCloseDetailsController.php | 113 ++++++++++++++++++ .../storage/DifferentialTransaction.php | 16 +++ .../DiffusionLowLevelCommitFieldsQuery.php | 50 +++++++- ...torRepositoryCommitMessageParserWorker.php | 19 ++- 7 files changed, 206 insertions(+), 7 deletions(-) create mode 100644 src/applications/differential/controller/DifferentialRevisionCloseDetailsController.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 0cbd40806c..e86d8c2f95 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -330,6 +330,7 @@ phutil_register_library_map(array( 'DifferentialReviewersField' => 'applications/differential/customfield/DifferentialReviewersField.php', 'DifferentialReviewersView' => 'applications/differential/view/DifferentialReviewersView.php', 'DifferentialRevision' => 'applications/differential/storage/DifferentialRevision.php', + 'DifferentialRevisionCloseDetailsController' => 'applications/differential/controller/DifferentialRevisionCloseDetailsController.php', 'DifferentialRevisionControlSystem' => 'applications/differential/constants/DifferentialRevisionControlSystem.php', 'DifferentialRevisionDetailView' => 'applications/differential/view/DifferentialRevisionDetailView.php', 'DifferentialRevisionEditController' => 'applications/differential/controller/DifferentialRevisionEditController.php', @@ -3223,6 +3224,7 @@ phutil_register_library_map(array( 'PhabricatorDestructibleInterface', 'PhabricatorProjectInterface', ), + 'DifferentialRevisionCloseDetailsController' => 'DifferentialController', 'DifferentialRevisionDetailView' => 'AphrontView', 'DifferentialRevisionEditController' => 'DifferentialController', 'DifferentialRevisionHasTaskEdgeType' => 'PhabricatorEdgeType', diff --git a/src/applications/differential/application/PhabricatorDifferentialApplication.php b/src/applications/differential/application/PhabricatorDifferentialApplication.php index 1b7495a13c..43cb463e5c 100644 --- a/src/applications/differential/application/PhabricatorDifferentialApplication.php +++ b/src/applications/differential/application/PhabricatorDifferentialApplication.php @@ -67,6 +67,8 @@ EOTEXT => 'DifferentialRevisionEditController', 'revision/land/(?:(?P[1-9]\d*))/(?P[^/]+)/' => 'DifferentialRevisionLandController', + 'revision/closedetails/(?P[^/]+)/' + => 'DifferentialRevisionCloseDetailsController', 'comment/' => array( 'preview/(?P[1-9]\d*)/' => 'DifferentialCommentPreviewController', 'save/(?P[1-9]\d*)/' => 'DifferentialCommentSaveController', diff --git a/src/applications/differential/conduit/DifferentialParseCommitMessageConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialParseCommitMessageConduitAPIMethod.php index e116a939b4..388fa364e0 100644 --- a/src/applications/differential/conduit/DifferentialParseCommitMessageConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialParseCommitMessageConduitAPIMethod.php @@ -81,9 +81,20 @@ final class DifferentialParseCommitMessageConduitAPIMethod } } + // grab some extra information about the Differential Revision: field... + $revision_id_field = new DifferentialRevisionIDField(); + $revision_id_value = idx( + $corpus_map, + $revision_id_field->getFieldKeyForConduit()); + $revision_id_valid_domain = PhabricatorEnv::getProductionURI(''); + return array( 'errors' => $this->errors, 'fields' => $values, + 'revisionIDFieldInfo' => array( + 'value' => $revision_id_value, + 'validDomain' => $revision_id_valid_domain, + ), ); } diff --git a/src/applications/differential/controller/DifferentialRevisionCloseDetailsController.php b/src/applications/differential/controller/DifferentialRevisionCloseDetailsController.php new file mode 100644 index 0000000000..8b02ff2382 --- /dev/null +++ b/src/applications/differential/controller/DifferentialRevisionCloseDetailsController.php @@ -0,0 +1,113 @@ +phid = idx($data, 'phid'); + } + + public function processRequest() { + $request = $this->getRequest(); + + $viewer = $request->getUser(); + $xaction_phid = $this->phid; + + $xaction = id(new PhabricatorObjectQuery()) + ->withPHIDs(array($xaction_phid)) + ->setViewer($viewer) + ->executeOne(); + if (!$xaction) { + return new Aphront404Response(); + } + + $obj_phid = $xaction->getObjectPHID(); + $obj_handle = id(new PhabricatorHandleQuery()) + ->setViewer($viewer) + ->withPHIDs(array($obj_phid)) + ->executeOne(); + + $body = $this->getRevisionMatchExplanation( + $xaction->getMetadataValue('revisionMatchData'), + $obj_handle); + + $dialog = id(new AphrontDialogView()) + ->setUser($viewer) + ->setTitle(pht('Commit Close Explanation')) + ->appendParagraph($body) + ->addCancelButton($obj_handle->getURI()); + + return id(new AphrontDialogResponse())->setDialog($dialog); + } + + private function getRevisionMatchExplanation( + $revision_match_data, + PhabricatorObjectHandle $obj_handle) { + + if (!$revision_match_data) { + return pht( + 'This commit was made before this feature was built and thus this '. + 'information is unavailable.'); + } + + $body_why = array(); + if ($revision_match_data['usedURI']) { + return pht( + 'We found a "Differential Revision" field with value "%s" in the '. + 'commit message, and the domain on the URI matches this install, so '. + 'we linked this commit to %s.', + $revision_match_data['foundURI'], + phutil_tag( + 'a', + array( + 'href' => $obj_handle->getURI(),), + $obj_handle->getName())); + } else if ($revision_match_data['foundURI']) { + $body_why[] = pht( + 'We found a "Differential Revision" field with value "%s" in the '. + 'commit message, but the domain on this URI did not match the '. + 'configured domain for this install, "%s", so we ignored it under '. + 'the assumption that it refers to some third-party revision.', + $revision_match_data['foundURI'], + $revision_match_data['validDomain']); + } else { + $body_why[] = pht( + 'We didn\'t find a "Differential Revision" field in the commit '. + 'message.'); + } + + switch ($revision_match_data['matchHashType']) { + case ArcanistDifferentialRevisionHash::HASH_GIT_TREE: + $hash_info = true; + $hash_type = 'tree'; + break; + case ArcanistDifferentialRevisionHash::HASH_GIT_COMMIT: + case ArcanistDifferentialRevisionHash::HASH_MERCURIAL_COMMIT: + $hash_info = true; + $hash_type = 'commit'; + break; + default: + $hash_info = false; + break; + } + if ($hash_info) { + $diff_link = phutil_tag( + 'a', + array( + 'href' => $obj_handle->getURI(),), + $obj_handle->getName()); + $body_why = pht( + 'This commit and the active diff of %s had the same %s hash '. + '(%s) so we linked this commit to %s.', + $diff_link, + $hash_type, + $revision_match_data['matchHashValue'], + $diff_link); + } + + return phutil_implode_html("\n", $body_why); + + } +} diff --git a/src/applications/differential/storage/DifferentialTransaction.php b/src/applications/differential/storage/DifferentialTransaction.php index 4cc8c4e803..b6df7cf4f5 100644 --- a/src/applications/differential/storage/DifferentialTransaction.php +++ b/src/applications/differential/storage/DifferentialTransaction.php @@ -306,6 +306,22 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction { return parent::getTitle(); } + public function renderExtraInformationLink() { + if ($this->getMetadataValue('revisionMatchData')) { + $details_href = + '/differential/revision/closedetails/'.$this->getPHID().'/'; + $details_link = javelin_tag( + 'a', + array( + 'href' => $details_href, + 'sigil' => 'workflow', + ), + pht('Explain Why')); + return $details_link; + } + return parent::renderExtraInformationLink(); + } + public function getTitleForFeed(PhabricatorFeedStory $story) { $author_phid = $this->getAuthorPHID(); $object_phid = $this->getObjectPHID(); diff --git a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitFieldsQuery.php b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitFieldsQuery.php index 64d541f151..64ad12e096 100644 --- a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitFieldsQuery.php +++ b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitFieldsQuery.php @@ -4,12 +4,28 @@ final class DiffusionLowLevelCommitFieldsQuery extends DiffusionLowLevelQuery { private $ref; + private $revisionMatchData = array( + 'usedURI' => null, + 'foundURI' => null, + 'validDomain' => null, + 'matchHashType' => null, + 'matchHashValue' => null, + ); public function withCommitRef(DiffusionCommitRef $ref) { $this->ref = $ref; return $this; } + public function getRevisionMatchData() { + return $this->revisionMatchData; + } + + private function setRevisionMatchData($key, $value) { + $this->revisionMatchData[$key] = $value; + return $this; + } + public function executeQuery() { $ref = $this->ref; $message = $ref->getMessage(); @@ -25,10 +41,21 @@ final class DiffusionLowLevelCommitFieldsQuery ->execute(); $fields = $result['fields']; + $revision_id = idx($fields, 'revisionID'); + if ($revision_id) { + $this->setRevisionMatchData('usedURI', true); + } else { + $this->setRevisionMatchData('usedURI', false); + } + $revision_id_info = $result['revisionIDFieldInfo']; + $this->setRevisionMatchData('foundURI', $revision_id_info['value']); + $this->setRevisionMatchData( + 'validDomain', + $revision_id_info['validDomain']); + // If there is no "Differential Revision:" field in the message, try to // identify the revision by doing a hash lookup. - $revision_id = idx($fields, 'revisionID'); if (!$revision_id && $hashes) { $hash_list = array(); foreach ($hashes as $hash) { @@ -36,12 +63,33 @@ final class DiffusionLowLevelCommitFieldsQuery } $revisions = id(new DifferentialRevisionQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->needHashes(true) ->withCommitHashes($hash_list) ->execute(); if (!empty($revisions)) { $revision = $this->pickBestRevision($revisions); $fields['revisionID'] = $revision->getID(); + $revision_hashes = $revision->getHashes(); + $revision_hashes = mpull($revision_hashes, 'getHashType'); + // sort the hashes in the order the mighty + // @{class:ArcanstDifferentialRevisionHash} does; probably unnecessary + // but should future proof things nicely. + $revision_hashes = array_select_keys( + $revision_hashes, + ArcanistDifferentialRevisionHash::getTypes()); + foreach ($hashes as $hash) { + $revision_hash = $revision_hashes[$hash->getHashType()]; + if ($revision_hash->getHashValue() == $hash->getHashValue()) { + $this->setRevisionMatchData( + 'matchHashType', + $hash->getHashType()); + $this->setRevisionMatchData( + 'matchHashValue', + $hash->getHashValue()); + break; + } + } } } diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php index ef25a2ef34..b9d2a39125 100644 --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php @@ -53,11 +53,12 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker $differential_app = 'PhabricatorDifferentialApplication'; $revision_id = null; + $low_level_query = null; if (PhabricatorApplication::isClassInstalled($differential_app)) { - $field_values = id(new DiffusionLowLevelCommitFieldsQuery()) + $low_level_query = id(new DiffusionLowLevelCommitFieldsQuery()) ->setRepository($repository) - ->withCommitRef($ref) - ->execute(); + ->withCommitRef($ref); + $field_values = $low_level_query->execute(); $revision_id = idx($field_values, 'revisionID'); if (!empty($field_values['reviewedByPHIDs'])) { @@ -160,9 +161,15 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker $commit_close_xaction->setMetadataValue( 'authorName', $data->getAuthorName()); - $commit_close_xaction->setMetadataValue( - 'commitHashes', - $hashes); + + if ($low_level_query) { + $commit_close_xaction->setMetadataValue( + 'revisionMatchData', + $low_level_query->getRevisionMatchData()); + $data->setCommitDetail( + 'revisionMatchData', + $low_level_query->getRevisionMatchData()); + } $diff = $this->generateFinalDiff($revision, $acting_as_phid);