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

@@ -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',

src/applications/differential/application/PhabricatorDifferentialApplication.php

@@ -67,6 +67,8 @@ EOTEXT
           => 'DifferentialRevisionEditController',
         'revision/land/(?:(?P<id>[1-9]\d*))/(?P<strategy>[^/]+)/'
           => 'DifferentialRevisionLandController',
+        'revision/closedetails/(?P<phid>[^/]+)/'
+          => 'DifferentialRevisionCloseDetailsController',
         'comment/' => array(
           'preview/(?P<id>[1-9]\d*)/' => 'DifferentialCommentPreviewController',
           'save/(?P<id>[1-9]\d*)/' => 'DifferentialCommentSaveController',

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,
+      ),
     );
   }
 

src/applications/differential/controller/DifferentialRevisionCloseDetailsController.php

@@ -0,0 +1,113 @@
+<?php
+
+final class DifferentialRevisionCloseDetailsController
+  extends DifferentialController {
+
+  private $phid;
+
+  public function willProcessRequest(array $data) {
+    $this->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);
+
+  }
+}

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();

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;
+          }
+        }
       }
     }
 

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)
+        ->withCommitRef($ref);
-        ->execute();
+      $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());
+
+          if ($low_level_query) {
             $commit_close_xaction->setMetadataValue(
-            'commitHashes',
+              'revisionMatchData',
-            $hashes);
+              $low_level_query->getRevisionMatchData());
+            $data->setCommitDetail(
+              'revisionMatchData',
+              $low_level_query->getRevisionMatchData());
+          }
 
           $diff = $this->generateFinalDiff($revision, $acting_as_phid);