Show other open revisions affecting the same files in Differential

Summary:
I think this feature is probably good, but Differential is also really starting to get a lot of stuff which is not the diff in it. Not sure how best to deal with that.

The mixed table styles are also pretty ugly.

So I guess this is more feedback / proof-of-concept, I think I want to try to improve it somehow before I land it.

Test Plan: Looked at some diffs, some had an awkward, ugly list of diffs affecting the same files.

Reviewers: bill, aran, btrahan

Reviewed By: aran

CC: aran, epriestley

Maniphest Tasks: T829

Differential Revision: https://secure.phabricator.com/D2027

src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php

@@ -269,6 +269,16 @@ final class DifferentialRevisionViewController extends DifferentialController {
     $local_view->setUser($user);
     $local_view->setLocalCommits(idx($props, 'local:commits'));
 
+    $other_revisions = $this->loadOtherRevisions(
+      $changesets,
+      $target,
+      $repository);
+
+    $other_view = null;
+    if ($other_revisions) {
+      $other_view = $this->renderOtherRevisions($other_revisions);
+    }
+
     $toc_view = new DifferentialDiffTableOfContentsView();
     $toc_view->setChangesets($changesets);
     $toc_view->setVisibleChangesets($visible_changesets);
@@ -320,6 +330,7 @@ final class DifferentialRevisionViewController extends DifferentialController {
         $warning.
         $local_view->render().
         $toc_view->render().
+        $other_view.
         $changeset_view->render());
     if ($comment_form) {
       $page_pane->appendChild($comment_form->render());
@@ -708,5 +719,65 @@ final class DifferentialRevisionViewController extends DifferentialController {
     return $symbol_indexes;
   }
 
+  private function loadOtherRevisions($changesets, $target, $repository) {
+    if (!$repository) {
+      return array();
+    }
+
+    $paths = array();
+    foreach ($changesets as $changeset) {
+      $paths[] = $changeset->getAbsoluteRepositoryPath(
+        $target,
+        $repository);
+    }
+
+    if (!$paths) {
+      return array();
+    }
+
+    $path_map = id(new DiffusionPathIDQuery($paths))->loadPathIDs();
+
+    if (!$path_map) {
+      return array();
+    }
+
+    $query = id(new DifferentialRevisionQuery())
+      ->withStatus(DifferentialRevisionQuery::STATUS_OPEN)
+      ->setOrder(DifferentialRevisionQuery::ORDER_PATH_MODIFIED)
+      ->setLimit(10)
+      ->needRelationships(true);
+
+    foreach ($path_map as $path => $path_id) {
+      $query->withPath($repository->getID(), $path_id);
+    }
+
+    $results = $query->execute();
+
+    // Strip out *this* revision.
+    foreach ($results as $key => $result) {
+      if ($result->getID() == $this->revisionID) {
+        unset($results[$key]);
+      }
+    }
+
+    return $results;
+  }
+
+  private function renderOtherRevisions(array $revisions) {
+    $view = id(new DifferentialRevisionListView())
+      ->setRevisions($revisions)
+      ->setFields(DifferentialRevisionListView::getDefaultFields())
+      ->setUser($this->getRequest()->getUser());
+
+    $phids = $view->getRequiredHandlePHIDs();
+    $handles = id(new PhabricatorObjectHandleData($phids))->loadHandles();
+    $view->setHandles($handles);
+
+    return
+      '<div class="differential-panel">'.
+        '<h1>Open Revisions Affecting These Files</h1>'.
+        $view->render().
+      '</div>';
+  }
 
 }

src/applications/differential/controller/revisionview/__init__.php

@@ -13,6 +13,7 @@ phutil_require_module('phabricator', 'applications/differential/constants/action
 phutil_require_module('phabricator', 'applications/differential/controller/base');
 phutil_require_module('phabricator', 'applications/differential/field/selector/base');
 phutil_require_module('phabricator', 'applications/differential/parser/changeset');
+phutil_require_module('phabricator', 'applications/differential/query/revision');
 phutil_require_module('phabricator', 'applications/differential/storage/auxiliaryfield');
 phutil_require_module('phabricator', 'applications/differential/storage/changeset');
 phutil_require_module('phabricator', 'applications/differential/storage/comment');
@@ -26,7 +27,9 @@ phutil_require_module('phabricator', 'applications/differential/view/localcommit
 phutil_require_module('phabricator', 'applications/differential/view/primarypane');
 phutil_require_module('phabricator', 'applications/differential/view/revisioncommentlist');
 phutil_require_module('phabricator', 'applications/differential/view/revisiondetail');
+phutil_require_module('phabricator', 'applications/differential/view/revisionlist');
 phutil_require_module('phabricator', 'applications/differential/view/revisionupdatehistory');
+phutil_require_module('phabricator', 'applications/diffusion/query/pathid/base');
 phutil_require_module('phabricator', 'applications/draft/storage/draft');
 phutil_require_module('phabricator', 'applications/flag/constants/color');
 phutil_require_module('phabricator', 'applications/flag/query/flag');

src/applications/differential/view/difftableofcontents/DifferentialDiffTableOfContentsView.php

@@ -278,4 +278,5 @@ final class DifferentialDiffTableOfContentsView extends AphrontView {
       ),
       phutil_escape_html($display_file));
   }
+
 }

src/applications/diffusion/request/git/DiffusionGitRequest.php

@@ -26,64 +26,18 @@ final class DiffusionGitRequest extends DiffusionRequest {
   }
 
   protected function didInitialize() {
-    if ($this->repository) {
+    if (!$this->commit) {
-      $repository = $this->repository;
+      return;
-
-      // TODO: This is not terribly efficient and does not produce terribly
-      // good error messages, but it seems better to put error handling code
-      // here than to try to do it in every query.
-
-      $branch = $this->getBranch();
-
-      // TODO: Here, particularly, we should give the user a specific error
-      // message to indicate whether they've typed in some bogus branch and/or
-      // followed a bad link, or misconfigured the default branch in the
-      // Repository tool.
-      list($this->stableCommitName) = $repository->execxLocalCommand(
-        'rev-parse --verify %s/%s',
-        DiffusionBranchInformation::DEFAULT_GIT_REMOTE,
-        $branch);
-
-      if ($this->commit) {
-        list($commit) = $repository->execxLocalCommand(
-          'rev-parse --verify %s',
-          $this->commit);
-
-        // Beyond verifying them, expand commit short forms to full 40-character
-        // hashes.
-        $this->commit = trim($commit);
-
-        // If we have a commit, overwrite the branch commit with the more
-        // specific commit.
-        $this->stableCommitName = $this->commit;
-
-/*
-
-  TODO: Unclear if this is actually a good idea or not; it breaks commit views
-  at the very least.
-
-        list($contains) = $repository->execxLocalCommand(
-          'branch --contains %s',
-          $this->commit);
-        $contains = array_filter(explode("\n", $contains));
-        $found = false;
-        foreach ($contains as $containing_branch) {
-          $containing_branch = trim($containing_branch, "* \n");
-          if ($containing_branch == $branch) {
-            $found = true;
-            break;
-          }
-        }
-        if (!$found) {
-          throw new Exception(
-            "Commit does not exist on this branch!");
-        }
-*/
-
-      }
     }
 
+    // Expand commit short forms to full 40-character hashes. This does not
+    // verify them, --verify exits with return code 0 for anything that
+    // looks like a valid hash.
 
+    list($commit) = $this->getRepository()->execxLocalCommand(
+      'rev-parse --verify %s',
+      $this->commit);
+    $this->commit = trim($commit);
   }
 
   public function getBranch() {
@@ -105,6 +59,18 @@ final class DiffusionGitRequest extends DiffusionRequest {
   }
 
   public function getStableCommitName() {
+    if (!$this->stableCommitName) {
+      if ($this->commit) {
+        $this->stableCommitName = $this->commit;
+      } else {
+        $branch = $this->getBranch();
+        list($stdout) = $this->getRepository()->execxLocalCommand(
+          'rev-parse --verify %s/%s',
+          DiffusionBranchInformation::DEFAULT_GIT_REMOTE,
+          $branch);
+        $this->stableCommitName = trim($stdout);
+      }
+    }
     return substr($this->stableCommitName, 0, 16);
   }