Add support for querying by commit hashes to DifferentialRevisionQuery class and

corresponding ConduitAPI

Summary: reasonable title...  also made this new functionality used by the
repository worker for parsing diffs

Test Plan:
- looked at the conduit console and queried for various types of hashes,
including hashes with no match.  got correct results.
- identified a reasonable diff from a local git repo.  set the revision status
to 2 (ACCEPTED) in the database.  augmented the worker parser code to var_dump
and die after finding revision id.   ran scripts/repository/reparse.php
--message rX and verified my var_dumps.  removed var_dumps and die and ran
reparse.php again with same paramters.  verified revision looked good in
diffusion and there were no errors.
- repeated the above reparse.php jonx for a mercurial repo.  note svn isn't in
this hash game so that test was particularly exciting no-op'dness i did not
bother with

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, btrahan, epriestley

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

src/applications/conduit/method/differential/query/ConduitAPI_differential_query_Method.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * 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.
@@ -26,6 +26,9 @@ class ConduitAPI_differential_query_Method extends ConduitAPIMethod {
   }
 
   public function defineParamTypes() {
+    $hash_types = DifferentialRevisionHash::getTypes();
+    $hash_types = implode(', ', $hash_types);
+
     $status_types = array(
       DifferentialRevisionQuery::STATUS_ANY,
       DifferentialRevisionQuery::STATUS_OPEN,
@@ -45,6 +48,8 @@ class ConduitAPI_differential_query_Method extends ConduitAPIMethod {
       // TODO: Implement this, it needs to accept a repository ID in addition
       // to a path so the signature needs to be a little more complicated.
       // 'paths'          => 'optional list<pair<...>>',
+      'commitHashes'      => 'optional list<pair<enum<'.
+                             $hash_types.'>, string>>',
       'status'            => 'optional enum<'.$status_types.'>',
       'order'             => 'optional enum<'.$order_types.'>',
       'limit'             => 'optional uint',
@@ -62,6 +67,7 @@ class ConduitAPI_differential_query_Method extends ConduitAPIMethod {
 
   public function defineErrorTypes() {
     return array(
+      'ERR-INVALID-PARAMETER' => 'Missing or malformed parameter.',
     );
   }
 
@@ -71,6 +77,7 @@ class ConduitAPI_differential_query_Method extends ConduitAPIMethod {
     $reviewers          = $request->getValue('reviewers');
     $status             = $request->getValue('status');
     $order              = $request->getValue('order');
+    $commit_hashes      = $request->getValue('commitHashes');
     $limit              = $request->getValue('limit');
     $offset             = $request->getValue('offset');
     $ids                = $request->getValue('ids');
@@ -99,6 +106,19 @@ class ConduitAPI_differential_query_Method extends ConduitAPIMethod {
       }
     }
  */
+    if ($commit_hashes) {
+      $hash_types = DifferentialRevisionHash::getTypes();
+      foreach ($commit_hashes as $info) {
+        list($type, $hash) = $info;
+        if (empty($type) ||
+            !in_array($type, $hash_types) ||
+            empty($hash)) {
+              throw new ConduitException('ERR-INVALID-PARAMETER');
+        }
+      }
+      $query->withCommitHashes($commit_hashes);
+    }
+
     if ($status) {
       $query->withStatus($status);
     }
@@ -163,5 +183,4 @@ class ConduitAPI_differential_query_Method extends ConduitAPIMethod {
 
     return $results;
   }
-
 }

src/applications/conduit/method/differential/query/__init__.php

@@ -7,6 +7,8 @@
 
 
 phutil_require_module('phabricator', 'applications/conduit/method/base');
+phutil_require_module('phabricator', 'applications/conduit/protocol/exception');
+phutil_require_module('phabricator', 'applications/differential/constants/revisionhash');
 phutil_require_module('phabricator', 'applications/differential/constants/revisionstatus');
 phutil_require_module('phabricator', 'applications/differential/query/revision');
 phutil_require_module('phabricator', 'infrastructure/env');

src/applications/differential/constants/revisionhash/DifferentialRevisionHash.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * 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.
@@ -24,4 +24,12 @@ final class DifferentialRevisionHash {
   const HASH_GIT_TREE           = 'gttr';
   const HASH_MERCURIAL_COMMIT   = 'hgcm';
 
+  public static function getTypes() {
+    return array(
+      DifferentialRevisionHash::HASH_GIT_COMMIT,
+      DifferentialRevisionHash::HASH_GIT_TREE,
+      DifferentialRevisionHash::HASH_MERCURIAL_COMMIT,
+    );
+  }
+
 }

src/applications/differential/query/revision/DifferentialRevisionQuery.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * 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.
@@ -42,6 +42,7 @@ final class DifferentialRevisionQuery {
   private $ccs = array();
   private $reviewers = array();
   private $revIDs = array();
+  private $commitHashes = array();
   private $phids = array();
   private $subscribers = array();
   private $responsibles = array();
@@ -89,7 +90,9 @@ final class DifferentialRevisionQuery {
   }
 
   /**
-   * Filter results to revisions authored by one of the given PHIDs.
+   * Filter results to revisions authored by one of the given PHIDs. Calling
+   * this function will clear anything set by previous calls to
+   * @{method:withAuthors}.
    *
    * @param array List of PHIDs of authors
    * @return this
@@ -113,7 +116,6 @@ final class DifferentialRevisionQuery {
     return $this;
   }
 
-
   /**
    * Filter results to revisions that have one of the provided PHIDs as
    * reviewers. Calling this function will clear anything set by previous calls
@@ -128,6 +130,20 @@ final class DifferentialRevisionQuery {
     return $this;
   }
 
+  /**
+   * Filter results to revisions that have one of the provided commit hashes.
+   * Calling this function will clear anything set by previous calls to
+   * @{method:withCommitHashes}.
+   *
+   * @param array List of pairs <Class DifferentialRevisionHash::HASH_$type
+   *              constant, hash>
+   * @return this
+   * @task config
+   */
+  public function withCommitHashes(array $commit_hashes) {
+    $this->commitHashes = $commit_hashes;
+    return $this;
+  }
 
   /**
    * Filter results to revisions with a given status. Provide a class constant,
@@ -351,6 +367,7 @@ final class DifferentialRevisionQuery {
         !$this->ccs &&
         !$this->authors &&
         !$this->revIDs &&
+        !$this->commitHashes &&
         !$this->phids) {
       return true;
     }
@@ -440,6 +457,13 @@ final class DifferentialRevisionQuery {
         $path_table->getTableName());
     }
 
+    if ($this->commitHashes) {
+      $joins[] = qsprintf(
+        $conn_r,
+        'JOIN %T hash_rel ON hash_rel.revisionID = r.id',
+        DifferentialRevisionHash::TABLE_NAME);
+    }
+
     if ($this->ccs) {
       $joins[] = qsprintf(
         $conn_r,
@@ -527,6 +551,20 @@ final class DifferentialRevisionQuery {
         $this->revIDs);
     }
 
+    if ($this->commitHashes) {
+      $hash_clauses = array();
+      foreach ($this->commitHashes as $info) {
+        list($type, $hash) = $info;
+        $hash_clauses[] = qsprintf(
+          $conn_r,
+          '(hash_rel.type = %s AND hash_rel.hash = %s)',
+          $type,
+          $hash);
+      }
+      $hash_clauses = '('.implode(' OR ', $hash_clauses).')';
+      $where[] = $hash_clauses;
+    }
+
     if ($this->phids) {
       $where[] = qsprintf(
         $conn_r,

src/applications/differential/query/revision/__init__.php

@@ -6,6 +6,7 @@
 
 
 
+phutil_require_module('phabricator', 'applications/differential/constants/revisionhash');
 phutil_require_module('phabricator', 'applications/differential/constants/revisionstatus');
 phutil_require_module('phabricator', 'applications/differential/storage/affectedpath');
 phutil_require_module('phabricator', 'applications/differential/storage/diff');

src/applications/repository/worker/commitmessageparser/base/PhabricatorRepositoryCommitMessageParserWorker.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * 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.
@@ -65,22 +65,14 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
         $this->repository,
         $this->commit);
       if ($hashes) {
-        $sql = array();
-        foreach ($hashes as $info) {
-          list($type, $hash) = $info;
-          $sql[] = qsprintf(
-            $conn_w,
-            '(type = %s AND hash = %s)',
-            $type,
-            $hash);
-        }
-        $revision = queryfx_one(
-          $conn_w,
-          'SELECT revisionID FROM %T WHERE %Q LIMIT 1',
-          DifferentialRevisionHash::TABLE_NAME,
-          implode(' OR ', $sql));
-        if ($revision) {
-          $revision_id = $revision['revisionID'];
+
+        $query = new DifferentialRevisionQuery();
+        $query->withCommitHashes($hashes);
+        $revisions = $query->execute();
+
+        if (!empty($revisions)) {
+          $revision = $this->identifyBestRevision($revisions);
+          $revision_id = $revision->getID();
         }
       }
     }
@@ -112,4 +104,60 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
     }
   }
 
+  /**
+   * When querying for revisions by hash, more than one revision may be found.
+   * This function identifies the "best" revision from such a set.  Typically,
+   * there is only one revision found.   Otherwise, we try to pick an accepted
+   * revision first, followed by an open revision, and otherwise we go with a
+   * committed or abandoned revision as a last resort.
+   */
+  private function identifyBestRevision(array $revisions) {
+    // get the simplest, common case out of the way
+    if (count($revisions) == 1) {
+      return reset($revisions);
+    }
+
+    $first_choice = array();
+    $second_choice = array();
+    $third_choice = array();
+    foreach ($revisions as $revision) {
+      switch ($revision->getStatus()) {
+        // "Accepted" revisions -- ostensibly what we're looking for!
+        case DifferentialRevisionStatus::ACCEPTED:
+          $first_choice[] = $revision;
+          break;
+        // "Open" revisions
+        case DifferentialRevisionStatus::NEEDS_REVIEW:
+        case DifferentialRevisionStatus::NEEDS_REVISION:
+          $second_choice[] = $revision;
+          break;
+        // default is a wtf? here
+        default:
+        case DifferentialRevisionStatus::ABANDONED:
+        case DifferentialRevisionStatus::COMMITTED:
+          $third_choice[] = $revision;
+          break;
+      }
+    }
+
+    // go down the ladder like a bro at last call
+    if (!empty($first_choice)) {
+      return $this->identifyMostRecentRevision($first_choice);
+    }
+    if (!empty($second_choice)) {
+      return $this->identifyMostRecentRevision($second_choice);
+    }
+    if (!empty($third_choice)) {
+      return $this->identifyMostRecentRevision($third_choice);
+    }
+  }
+
+  /**
+   * Given a set of revisions, returns the revision with the latest
+   * updated time.   This is ostensibly the most recent revision.
+   */
+  private function identifyMostRecentRevision(array $revisions) {
+    $revisions = msort($revisions, 'getDateModified');
+    return end($revisions);
+  }
 }

src/applications/repository/worker/commitmessageparser/base/__init__.php

@@ -7,13 +7,12 @@
 
 
 phutil_require_module('phabricator', 'applications/differential/constants/action');
-phutil_require_module('phabricator', 'applications/differential/constants/revisionhash');
 phutil_require_module('phabricator', 'applications/differential/constants/revisionstatus');
 phutil_require_module('phabricator', 'applications/differential/editor/comment');
+phutil_require_module('phabricator', 'applications/differential/query/revision');
 phutil_require_module('phabricator', 'applications/differential/storage/revision');
 phutil_require_module('phabricator', 'applications/repository/storage/commitdata');
 phutil_require_module('phabricator', 'applications/repository/worker/base');
-phutil_require_module('phabricator', 'storage/qsprintf');
 phutil_require_module('phabricator', 'storage/queryfx');
 
 phutil_require_module('phutil', 'symbols');