Show parent commits in Diffusion

Summary: Show parent commit information to make it easier to understand merges.

Test Plan: Looked at commits in SVN, hg, git.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T961

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

src/__phutil_library_map__.php

@@ -290,6 +290,7 @@ phutil_register_library_map(array(
     'DiffusionCommentView' => 'applications/diffusion/view/comment',
     'DiffusionCommitChangeTableView' => 'applications/diffusion/view/commitchangetable',
     'DiffusionCommitController' => 'applications/diffusion/controller/commit',
+    'DiffusionCommitParentsQuery' => 'applications/diffusion/query/parents/base',
     'DiffusionContainsQuery' => 'applications/diffusion/query/contains/base',
     'DiffusionController' => 'applications/diffusion/controller/base',
     'DiffusionDiffController' => 'applications/diffusion/controller/diff',
@@ -301,6 +302,7 @@ phutil_register_library_map(array(
     'DiffusionGitBranchQuery' => 'applications/diffusion/query/branch/git',
     'DiffusionGitBranchQueryTestCase' => 'applications/diffusion/query/branch/git/__tests__',
     'DiffusionGitBrowseQuery' => 'applications/diffusion/query/browse/git',
+    'DiffusionGitCommitParentsQuery' => 'applications/diffusion/query/parents/git',
     'DiffusionGitContainsQuery' => 'applications/diffusion/query/contains/git',
     'DiffusionGitDiffQuery' => 'applications/diffusion/query/diff/git',
     'DiffusionGitFileContentQuery' => 'applications/diffusion/query/filecontent/git',
@@ -317,6 +319,7 @@ phutil_register_library_map(array(
     'DiffusionLastModifiedQuery' => 'applications/diffusion/query/lastmodified/base',
     'DiffusionMercurialBranchQuery' => 'applications/diffusion/query/branch/mercurial',
     'DiffusionMercurialBrowseQuery' => 'applications/diffusion/query/browse/mercurial',
+    'DiffusionMercurialCommitParentsQuery' => 'applications/diffusion/query/parents/mercurial',
     'DiffusionMercurialContainsQuery' => 'applications/diffusion/query/contains/mercurial',
     'DiffusionMercurialDiffQuery' => 'applications/diffusion/query/diff/mercurial',
     'DiffusionMercurialFileContentQuery' => 'applications/diffusion/query/filecontent/mercurial',
@@ -337,6 +340,7 @@ phutil_register_library_map(array(
     'DiffusionRepositoryPath' => 'applications/diffusion/data/repositorypath',
     'DiffusionRequest' => 'applications/diffusion/request/base',
     'DiffusionSvnBrowseQuery' => 'applications/diffusion/query/browse/svn',
+    'DiffusionSvnCommitParentsQuery' => 'applications/diffusion/query/parents/svn',
     'DiffusionSvnContainsQuery' => 'applications/diffusion/query/contains/svn',
     'DiffusionSvnDiffQuery' => 'applications/diffusion/query/diff/svn',
     'DiffusionSvnFileContentQuery' => 'applications/diffusion/query/filecontent/svn',
@@ -1157,6 +1161,7 @@ phutil_register_library_map(array(
     'DiffusionCommentView' => 'AphrontView',
     'DiffusionCommitChangeTableView' => 'DiffusionView',
     'DiffusionCommitController' => 'DiffusionController',
+    'DiffusionCommitParentsQuery' => 'DiffusionQuery',
     'DiffusionContainsQuery' => 'DiffusionQuery',
     'DiffusionController' => 'PhabricatorController',
     'DiffusionDiffController' => 'DiffusionController',
@@ -1165,6 +1170,7 @@ phutil_register_library_map(array(
     'DiffusionGitBranchQuery' => 'DiffusionBranchQuery',
     'DiffusionGitBranchQueryTestCase' => 'PhabricatorTestCase',
     'DiffusionGitBrowseQuery' => 'DiffusionBrowseQuery',
+    'DiffusionGitCommitParentsQuery' => 'DiffusionCommitParentsQuery',
     'DiffusionGitContainsQuery' => 'DiffusionContainsQuery',
     'DiffusionGitDiffQuery' => 'DiffusionDiffQuery',
     'DiffusionGitFileContentQuery' => 'DiffusionFileContentQuery',
@@ -1180,6 +1186,7 @@ phutil_register_library_map(array(
     'DiffusionLastModifiedController' => 'DiffusionController',
     'DiffusionMercurialBranchQuery' => 'DiffusionBranchQuery',
     'DiffusionMercurialBrowseQuery' => 'DiffusionBrowseQuery',
+    'DiffusionMercurialCommitParentsQuery' => 'DiffusionCommitParentsQuery',
     'DiffusionMercurialContainsQuery' => 'DiffusionContainsQuery',
     'DiffusionMercurialDiffQuery' => 'DiffusionDiffQuery',
     'DiffusionMercurialFileContentQuery' => 'DiffusionFileContentQuery',
@@ -1193,6 +1200,7 @@ phutil_register_library_map(array(
     'DiffusionPathValidateController' => 'DiffusionController',
     'DiffusionRepositoryController' => 'DiffusionController',
     'DiffusionSvnBrowseQuery' => 'DiffusionBrowseQuery',
+    'DiffusionSvnCommitParentsQuery' => 'DiffusionCommitParentsQuery',
     'DiffusionSvnContainsQuery' => 'DiffusionContainsQuery',
     'DiffusionSvnDiffQuery' => 'DiffusionDiffQuery',
     'DiffusionSvnFileContentQuery' => 'DiffusionFileContentQuery',

src/applications/diffusion/controller/commit/DiffusionCommitController.php

@@ -74,22 +74,29 @@ final class DiffusionCommitController extends DiffusionController {
       require_celerity_resource('diffusion-commit-view-css');
       require_celerity_resource('phabricator-remarkup-css');
 
-      $property_table = $this->renderPropertyTable($commit, $commit_data);
+      $parent_query = DiffusionCommitParentsQuery::newFromDiffusionRequest(
+        $drequest);
+      $parents = $parent_query->loadParents();
+
+      $property_table = $this->renderPropertyTable(
+        $commit,
+        $commit_data,
+        $parents);
+
+      $detail_panel->setHeader('Revision Detail');
+      $detail_panel->addButton(
+        '<div class="diffusion-commit-dateline">'.
+          'r'.$callsign.$commit->getCommitIdentifier().
+          ' &middot; '.
+          phabricator_datetime($commit->getEpoch(), $user).
+        '</div>');
 
       $detail_panel->appendChild(
-        '<div class="diffusion-commit-view">'.
-          '<div class="diffusion-commit-dateline">'.
-            'r'.$callsign.$commit->getCommitIdentifier().
-            ' &middot; '.
-            phabricator_datetime($commit->getEpoch(), $user).
-          '</div>'.
-          '<h1>Revision Detail</h1>'.
-          '<div class="diffusion-commit-details">'.
-            $property_table.
-            '<hr />'.
-            '<div class="diffusion-commit-message phabricator-remarkup">'.
-              $engine->markupText($commit_data->getCommitMessage()).
-            '</div>'.
+        '<div class="diffusion-commit-details">'.
+          $property_table.
+          '<hr />'.
+          '<div class="diffusion-commit-message phabricator-remarkup">'.
+            $engine->markupText($commit_data->getCommitMessage()).
           '</div>'.
         '</div>');
 
@@ -268,7 +275,8 @@ final class DiffusionCommitController extends DiffusionController {
 
   private function renderPropertyTable(
     PhabricatorRepositoryCommit $commit,
-    PhabricatorRepositoryCommitData $data) {
+    PhabricatorRepositoryCommitData $data,
+    array $parents) {
 
     $phids = array();
     if ($data->getCommitDetail('authorPHID')) {
@@ -280,6 +288,11 @@ final class DiffusionCommitController extends DiffusionController {
     if ($data->getCommitDetail('differential.revisionPHID')) {
       $phids[] = $data->getCommitDetail('differential.revisionPHID');
     }
+    if ($parents) {
+      foreach ($parents as $parent) {
+        $phids[] = $parent->getPHID();
+      }
+    }
 
     $handles = array();
     if ($phids) {
@@ -314,6 +327,14 @@ final class DiffusionCommitController extends DiffusionController {
         $commit->getAuditStatus());
     }
 
+    if ($parents) {
+      $parent_links = array();
+      foreach ($parents as $parent) {
+        $parent_links[] = $handles[$parent->getPHID()]->renderLink();
+      }
+      $props['Parents'] = implode(' &middot; ', $parent_links);
+    }
+
     $request = $this->getDiffusionRequest();
 
     $contains = DiffusionContainsQuery::newFromDiffusionRequest($request);

src/applications/diffusion/controller/commit/__init__.php

@@ -20,6 +20,7 @@ phutil_require_module('phabricator', 'applications/diffusion/controller/base');
 phutil_require_module('phabricator', 'applications/diffusion/data/pathchange');
 phutil_require_module('phabricator', 'applications/diffusion/query/contains/base');
 phutil_require_module('phabricator', 'applications/diffusion/query/mergedcommits/base');
+phutil_require_module('phabricator', 'applications/diffusion/query/parents/base');
 phutil_require_module('phabricator', 'applications/diffusion/query/path');
 phutil_require_module('phabricator', 'applications/diffusion/query/pathchange/base');
 phutil_require_module('phabricator', 'applications/diffusion/query/pathid/base');

src/applications/diffusion/query/base/DiffusionQuery.php

@@ -58,26 +58,27 @@ abstract class DiffusionQuery {
 /* -(  Query Utilities  )---------------------------------------------------- */
 
 
-  final protected function loadHistoryForCommitIdentifiers(array $identifiers) {
+  final protected function loadCommitsByIdentifiers(array $identifiers) {
     if (!$identifiers) {
       return array();
     }
 
     $commits = array();
     $commit_data = array();
-    $path_changes = array();
 
     $drequest = $this->getRequest();
     $repository = $drequest->getRepository();
 
-    $path = $drequest->getPath();
-
     $commits = id(new PhabricatorRepositoryCommit())->loadAllWhere(
       'repositoryID = %d AND commitIdentifier IN (%Ls)',
         $repository->getID(),
       $identifiers);
     $commits = mpull($commits, null, 'getCommitIdentifier');
 
+    // Reorder the commits in identifier order so we preserve nth-parent
+    // relationships when the identifiers are the parents of a merge commit.
+    $commits = array_select_keys($commits, $identifiers);
+
     if (!$commits) {
       return array();
     }
@@ -87,6 +88,27 @@ abstract class DiffusionQuery {
       mpull($commits, 'getID'));
     $commit_data = mpull($commit_data, null, 'getCommitID');
 
+    foreach ($commits as $commit) {
+      if (idx($commit_data, $commit->getID())) {
+        $commit->attachCommitData($commit_data[$commit->getID()]);
+      }
+    }
+
+    return $commits;
+  }
+
+  final protected function loadHistoryForCommitIdentifiers(array $identifiers) {
+    if (!$identifiers) {
+      return array();
+    }
+
+    $drequest = $this->getRequest();
+    $repository = $drequest->getRepository();
+    $commits = self::loadCommitsByIdentifiers($identifiers);
+
+
+    $path = $drequest->getPath();
+
     $conn_r = $repository->establishConnection('r');
 
     $path_normal = DiffusionPathIDQuery::normalizePath($path);
@@ -113,9 +135,10 @@ abstract class DiffusionQuery {
       $commit = idx($commits, $identifier);
       if ($commit) {
         $item->setCommit($commit);
-        $data = idx($commit_data, $commit->getID());
-        if ($data) {
-          $item->setCommitData($data);
+        try {
+          $item->setCommitData($commit->getCommitData());
+        } catch (Exception $ex) {
+          // Ignore, commit just doesn't have data.
         }
         $change = idx($path_changes, $commit->getID());
         if ($change) {

src/applications/diffusion/query/parents/base/DiffusionCommitParentsQuery.php

@@ -0,0 +1,30 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+abstract class DiffusionCommitParentsQuery extends DiffusionQuery {
+
+  final public static function newFromDiffusionRequest(
+    DiffusionRequest $request) {
+    return self::newQueryObject(__CLASS__, $request);
+  }
+
+  final public function loadParents() {
+    return $this->executeQuery();
+  }
+
+}

src/applications/diffusion/query/parents/base/__init__.php

@@ -0,0 +1,12 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/diffusion/query/base');
+
+
+phutil_require_source('DiffusionCommitParentsQuery.php');

src/applications/diffusion/query/parents/git/DiffusionGitCommitParentsQuery.php

@@ -0,0 +1,35 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class DiffusionGitCommitParentsQuery
+  extends DiffusionCommitParentsQuery {
+
+  protected function executeQuery() {
+    $drequest = $this->getRequest();
+    $repository = $drequest->getRepository();
+
+    list($stdout) = $repository->execxLocalCommand(
+      'log -n 1 --format=%s %s',
+      '%P',
+      $drequest->getStableCommitName());
+
+    $hashes = preg_split('/\s+/', trim($stdout));
+
+    return self::loadCommitsByIdentifiers($hashes);
+  }
+}

src/applications/diffusion/query/parents/git/__init__.php

@@ -0,0 +1,12 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/diffusion/query/parents/base');
+
+
+phutil_require_source('DiffusionGitCommitParentsQuery.php');

src/applications/diffusion/query/parents/mercurial/DiffusionMercurialCommitParentsQuery.php

@@ -0,0 +1,46 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class DiffusionMercurialCommitParentsQuery
+  extends DiffusionCommitParentsQuery {
+
+  protected function executeQuery() {
+    $drequest = $this->getRequest();
+    $repository = $drequest->getRepository();
+
+    list($stdout) = $repository->execxLocalCommand(
+      'log --debug --limit 1 --template={parents} --rev %s',
+      $drequest->getStableCommitName());
+
+    $hashes = preg_split('/\s+/', trim($stdout));
+    foreach ($hashes as $key => $value) {
+      // Mercurial parents look like "23:ad9f769d6f786fad9f76d9a" -- we want
+      // to strip out the local rev part.
+      list($local, $global) = explode(':', $value);
+      $hashes[$key] = $global;
+
+      // With --debug we get 40-character hashes but also get the "000000..."
+      // hash for missing parents; ignore it.
+      if (preg_match('/^0+$/', $global)) {
+        unset($hashes[$key]);
+      }
+    }
+
+    return self::loadCommitsByIdentifiers($hashes);
+  }
+}

src/applications/diffusion/query/parents/mercurial/__init__.php

@@ -0,0 +1,12 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/diffusion/query/parents/base');
+
+
+phutil_require_source('DiffusionMercurialCommitParentsQuery.php');

src/applications/diffusion/query/parents/svn/DiffusionSvnCommitParentsQuery.php

@@ -0,0 +1,38 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class DiffusionSvnCommitParentsQuery
+  extends DiffusionCommitParentsQuery {
+
+  protected function executeQuery() {
+    $drequest = $this->getRequest();
+    $repository = $drequest->getRepository();
+
+    // TODO: With merge properties in recent versions of SVN, can we do
+    // a better job of this?
+
+    $n = $drequest->getStableCommitName();
+    if ($n > 1) {
+      $ids = array($n - 1);
+    } else {
+      $ids = array();
+    }
+
+    return self::loadCommitsByIdentifiers($ids);
+  }
+}

src/applications/diffusion/query/parents/svn/__init__.php

@@ -0,0 +1,12 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/diffusion/query/parents/base');
+
+
+phutil_require_source('DiffusionSvnCommitParentsQuery.php');