make repo callsigns optional

Summary:
Ref T4245 Make repo callsigns optional
This is far from done and still very ugly. I'm just submitting it to check if i'm solving this in the right places.
Right now there's three places with duplicate code and building the identifierMap in the CommitQuery is very ugly.
If we only want to support this in the user frontend then i could hack it into the Markup rule itself and not touch the CommitQuery. Even uglier but more limited in scope...

Generally this approach will need a lot of "check this first and then try the other" in a few places.
I could move the Repository queries into a specialised PhabricatorRepositoryQuery method (withCallsignOrID) but i'm not sure about that.

Test Plan:
 - phid.lookup works with R1 and rTEST (which is the same repo)
 - R1 and rTEST euqally work in remarkup (tested in comments).
 - Reviewed the following syntax also all works:
rTEST
rTESTd773137a7cb9
rTEST:d773137a7cb9
R1
R1:d773137a7cb9
d773137a7cb9
{rTEST}
{rTESTd773137a7cb9}
{rTEST:d773137a7cb9}
{R1}
{R1:d773137a7cb9}
{d773137a7cb9}

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Maniphest Tasks: T4245

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

src/__phutil_library_map__.php

@@ -552,6 +552,7 @@ phutil_register_library_map(array(
     'DiffusionRefNotFoundException' => 'applications/diffusion/exception/DiffusionRefNotFoundException.php',
     'DiffusionRefsQueryConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionRefsQueryConduitAPIMethod.php',
     'DiffusionRenameHistoryQuery' => 'applications/diffusion/query/DiffusionRenameHistoryQuery.php',
+    'DiffusionRepositoryByIDRemarkupRule' => 'applications/diffusion/remarkup/DiffusionRepositoryByIDRemarkupRule.php',
     'DiffusionRepositoryController' => 'applications/diffusion/controller/DiffusionRepositoryController.php',
     'DiffusionRepositoryCreateController' => 'applications/diffusion/controller/DiffusionRepositoryCreateController.php',
     'DiffusionRepositoryDatasource' => 'applications/diffusion/typeahead/DiffusionRepositoryDatasource.php',
@@ -3595,6 +3596,7 @@ phutil_register_library_map(array(
     'DiffusionReadmeView' => 'DiffusionView',
     'DiffusionRefNotFoundException' => 'Exception',
     'DiffusionRefsQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod',
+    'DiffusionRepositoryByIDRemarkupRule' => 'PhabricatorObjectRemarkupRule',
     'DiffusionRepositoryController' => 'DiffusionController',
     'DiffusionRepositoryCreateController' => 'DiffusionRepositoryEditController',
     'DiffusionRepositoryDatasource' => 'PhabricatorTypeaheadDatasource',

src/applications/diffusion/application/PhabricatorDiffusionApplication.php

@@ -40,8 +40,9 @@ final class PhabricatorDiffusionApplication extends PhabricatorApplication {
 
   public function getRemarkupRules() {
     return array(
-      new DiffusionRepositoryRemarkupRule(),
       new DiffusionCommitRemarkupRule(),
+      new DiffusionRepositoryRemarkupRule(),
+      new DiffusionRepositoryByIDRemarkupRule(),
     );
   }
 

src/applications/diffusion/query/DiffusionCommitQuery.php

@@ -183,6 +183,9 @@ final class DiffusionCommitQuery
       ->withIDs($repository_ids)
       ->execute();
 
+    $min_qualified = PhabricatorRepository::MINIMUM_QUALIFIED_HASH;
+    $result = array();
+
     foreach ($commits as $key => $commit) {
       $repo = idx($repos, $commit->getRepositoryID());
       if ($repo) {
@@ -190,36 +193,40 @@ final class DiffusionCommitQuery
       } else {
         unset($commits[$key]);
       }
-    }
 
-    if ($this->identifiers !== null) {
+      // Build the identifierMap
-      $ids = array_fuse($this->identifiers);
+      if ($this->identifiers !== null) {
-      $min_qualified = PhabricatorRepository::MINIMUM_QUALIFIED_HASH;
+        $ids = array_fuse($this->identifiers);
-
+        $prefixes = array(
-      $result = array();
+          'r'.$commit->getRepository()->getCallsign(),
-      foreach ($commits as $commit) {
+          'r'.$commit->getRepository()->getCallsign().':',
-        $prefix = 'r'.$commit->getRepository()->getCallsign();
+          'R'.$commit->getRepository()->getID().':',
+          '', // No prefix is valid too and will only match the commitIdentifier
+        );
         $suffix = $commit->getCommitIdentifier();
 
         if ($commit->getRepository()->isSVN()) {
-          if (isset($ids[$prefix.$suffix])) {
+          foreach ($prefixes as $prefix) {
-            $result[$prefix.$suffix][] = $commit;
+            if (isset($ids[$prefix.$suffix])) {
+              $result[$prefix.$suffix][] = $commit;
+            }
           }
         } else {
           // This awkward construction is so we can link the commits up in O(N)
           // time instead of O(N^2).
           for ($ii = $min_qualified; $ii <= strlen($suffix); $ii++) {
             $part = substr($suffix, 0, $ii);
-            if (isset($ids[$prefix.$part])) {
+            foreach ($prefixes as $prefix) {
-              $result[$prefix.$part][] = $commit;
+              if (isset($ids[$prefix.$part])) {
-            }
+                $result[$prefix.$part][] = $commit;
-            if (isset($ids[$part])) {
+              }
-              $result[$part][] = $commit;
             }
           }
         }
       }
+    }
 
+    if ($result) {
       foreach ($result as $identifier => $matching_commits) {
         if (count($matching_commits) == 1) {
           $result[$identifier] = head($matching_commits);
@@ -229,7 +236,6 @@ final class DiffusionCommitQuery
           unset($result[$identifier]);
         }
       }
-
       $this->identifierMap += $result;
     }
 
@@ -327,9 +333,10 @@ final class DiffusionCommitQuery
       $bare = array();
       foreach ($this->identifiers as $identifier) {
         $matches = null;
-        preg_match('/^(?:r([A-Z]+))?(.*)$/', $identifier, $matches);
+        preg_match('/^(?:[rR]([A-Z]+:?|[0-9]+:))?(.*)$/',
-        $repo = nonempty($matches[1], null);
+          $identifier, $matches);
-        $identifier = nonempty($matches[2], null);
+        $repo = nonempty(rtrim($matches[1], ':'), null);
+        $commit_identifier = nonempty($matches[2], null);
 
         if ($repo === null) {
           if ($this->defaultRepository) {
@@ -338,14 +345,14 @@ final class DiffusionCommitQuery
         }
 
         if ($repo === null) {
-          if (strlen($identifier) < $min_unqualified) {
+          if (strlen($commit_identifier) < $min_unqualified) {
             continue;
           }
-          $bare[] = $identifier;
+          $bare[] = $commit_identifier;
         } else {
           $refs[] = array(
             'callsign' => $repo,
-            'identifier' => $identifier,
+            'identifier' => $commit_identifier,
           );
         }
       }
@@ -362,14 +369,17 @@ final class DiffusionCommitQuery
 
       if ($refs) {
         $callsigns = ipull($refs, 'callsign');
+
         $repos = id(new PhabricatorRepositoryQuery())
           ->setViewer($this->getViewer())
-          ->withCallsigns($callsigns)
+          ->withIdentifiers($callsigns);
-          ->execute();
+        $repos->execute();
-        $repos = mpull($repos, null, 'getCallsign');
+
+        $repos = $repos->getIdentifierMap();
 
         foreach ($refs as $key => $ref) {
           $repo = idx($repos, $ref['callsign']);
+
           if (!$repo) {
             continue;
           }

src/applications/diffusion/remarkup/DiffusionCommitRemarkupRule.php

@@ -22,7 +22,6 @@ final class DiffusionCommitRemarkupRule extends PhabricatorObjectRemarkupRule {
       ->withIdentifiers($ids);
 
     $query->execute();
-
     return $query->getIdentifierMap();
   }
 

src/applications/diffusion/remarkup/DiffusionRepositoryByIDRemarkupRule.php

@@ -0,0 +1,29 @@
+<?php
+
+final class DiffusionRepositoryByIDRemarkupRule
+  extends PhabricatorObjectRemarkupRule {
+
+  protected function getObjectNamePrefix() {
+    return 'R';
+  }
+
+  protected function getObjectIDPattern() {
+    return '[0-9]+';
+  }
+
+  public function getPriority() {
+    return 460.0;
+  }
+
+  protected function loadObjects(array $ids) {
+    $viewer = $this->getEngine()->getConfig('viewer');
+
+    $repos = id(new PhabricatorRepositoryQuery())
+      ->setViewer($viewer)
+      ->withIdentifiers($ids);
+
+    $repos->execute();
+    return $repos->getIdentifierMap();
+  }
+
+}

src/applications/diffusion/remarkup/DiffusionRepositoryRemarkupRule.php

@@ -11,15 +11,19 @@ final class DiffusionRepositoryRemarkupRule
     return '[A-Z]+';
   }
 
+  public function getPriority() {
+    return 460.0;
+  }
+
   protected function loadObjects(array $ids) {
     $viewer = $this->getEngine()->getConfig('viewer');
 
-    $repositories = id(new PhabricatorRepositoryQuery())
+    $repos = id(new PhabricatorRepositoryQuery())
       ->setViewer($viewer)
-      ->withCallsigns($ids)
+      ->withIdentifiers($ids);
-      ->execute();
 
-    return mpull($repositories, null, 'getCallsign');
+    $repos->execute();
+    return $repos->getIdentifierMap();
   }
 
 }

src/applications/diffusion/remarkup/__tests__/DiffusionCommitRemarkupRuleTestCase.php

@@ -48,6 +48,79 @@ final class DiffusionCommitRemarkupRuleTestCase extends PhabricatorTestCase {
           ),
         ),
       ),
+      '{rP:1234 key=value}' => array(
+        'embed' => array(
+          array(
+            'offset' => 1,
+            'id' => 'rP:1234',
+            'tail' => ' key=value',
+          ),
+        ),
+        'ref' => array(
+          array(
+            'offset' => 1,
+            'id' => 'rP:1234',
+          ),
+        ),
+      ),
+      '{R123:1234 key=value}' => array(
+        'embed' => array(
+          array(
+            'offset' => 1,
+            'id' => 'R123:1234',
+            'tail' => ' key=value',
+          ),
+        ),
+        'ref' => array(
+          array(
+            'offset' => 1,
+            'id' => 'R123:1234',
+          ),
+        ),
+      ),
+      '{rP:12f3f6d3a9ef9c7731051815846810cb3c4cd248}' => array(
+        'embed' => array(
+          array(
+            'offset' => 1,
+            'id' => 'rP:12f3f6d3a9ef9c7731051815846810cb3c4cd248',
+          ),
+        ),
+        'ref' => array(
+          array(
+            'offset' => 1,
+            'id' => 'rP:12f3f6d3a9ef9c7731051815846810cb3c4cd248',
+          ),
+        ),
+      ),
+      '{R123:12f3f6d3a9ef9c7731051815846810cb3c4cd248}' => array(
+        'embed' => array(
+          array(
+            'offset' => 1,
+            'id' => 'R123:12f3f6d3a9ef9c7731051815846810cb3c4cd248',
+          ),
+        ),
+        'ref' => array(
+          array(
+            'offset' => 1,
+            'id' => 'R123:12f3f6d3a9ef9c7731051815846810cb3c4cd248',
+          ),
+        ),
+      ),
+      '{R123:12f3f6d3a9ef9c7731051815846810cb3c4cd248, key=value}' => array(
+        'embed' => array(
+          array(
+            'offset' => 1,
+            'id' => 'R123:12f3f6d3a9ef9c7731051815846810cb3c4cd248',
+            'tail' => ', key=value',
+          ),
+        ),
+        'ref' => array(
+          array(
+            'offset' => 1,
+            'id' => 'R123:12f3f6d3a9ef9c7731051815846810cb3c4cd248',
+          ),
+        ),
+      ),
     );
 
     foreach ($cases as $input => $expect) {

src/applications/repository/phid/PhabricatorRepositoryCommitPHIDType.php

@@ -54,9 +54,9 @@ final class PhabricatorRepositoryCommitPHIDType extends PhabricatorPHIDType {
     $min_qualified   = PhabricatorRepository::MINIMUM_QUALIFIED_HASH;
 
     return
-      'r[A-Z]+[1-9]\d*'.
+      '(?:r[A-Z]+:?|R[0-9]+:)[1-9]\d*'.
       '|'.
-      'r[A-Z]+[a-f0-9]{'.$min_qualified.',40}'.
+      '(?:r[A-Z]+:?|R[0-9]+:)[a-f0-9]{'.$min_qualified.',40}'.
       '|'.
       '[a-f0-9]{'.$min_unqualified.',40}';
   }

src/applications/repository/phid/PhabricatorRepositoryRepositoryPHIDType.php

@@ -44,33 +44,35 @@ final class PhabricatorRepositoryRepositoryPHIDType
   }
 
   public function canLoadNamedObject($name) {
-    return preg_match('/^r[A-Z]+$/', $name);
+    return preg_match('/^r[A-Z]+|R[0-9]+$/', $name);
   }
 
   public function loadNamedObjects(
     PhabricatorObjectQuery $query,
     array $names) {
 
+    $results = array();
     $id_map = array();
-    foreach ($names as $name) {
+    foreach ($names as $key => $name) {
       $id = substr($name, 1);
       $id_map[$id][] = $name;
+      $names[$key] = substr($name, 1);
     }
 
-    $objects = id(new PhabricatorRepositoryQuery())
+    $query = id(new PhabricatorRepositoryQuery())
       ->setViewer($query->getViewer())
-      ->withCallsigns(array_keys($id_map))
+      ->withIdentifiers($names);
-      ->execute();
 
-    $results = array();
+    if ($query->execute()) {
-    foreach ($objects as $object) {
+      $objects = $query->getIdentifierMap();
-      $callsign = $object->getCallsign();
+      foreach ($objects as $key => $object) {
-      foreach (idx($id_map, $callsign, array()) as $name) {
+        foreach (idx($id_map, $key, array()) as $name)
-        $results[$name] = $object;
+          $results[$name] = $object;
       }
+      return $results;
+    } else {
+      return array();
     }
-
-    return $results;
   }
 
 }

src/applications/repository/query/PhabricatorRepositoryQuery.php

@@ -12,6 +12,12 @@ final class PhabricatorRepositoryQuery
   private $remoteURIs;
   private $anyProjectPHIDs;
 
+  private $numericIdentifiers;
+  private $callsignIdentifiers;
+  private $phidIdentifiers;
+
+  private $identifierMap;
+
   const STATUS_OPEN = 'status-open';
   const STATUS_CLOSED = 'status-closed';
   const STATUS_ALL = 'status-all';
@@ -47,6 +53,27 @@ final class PhabricatorRepositoryQuery
     return $this;
   }
 
+  public function withIdentifiers(array $identifiers) {
+    $ids = array(); $callsigns = array(); $phids = array();
+    foreach ($identifiers as $identifier) {
+      if (ctype_digit($identifier)) {
+        $ids[$identifier] = $identifier;
+      } else {
+        $repository_type = PhabricatorRepositoryRepositoryPHIDType::TYPECONST;
+        if (phid_get_type($identifier) === $repository_type) {
+          $phids[$identifier] = $identifier;
+        } else {
+          $callsigns[$identifier] = $identifier;
+        }
+      }
+    }
+
+    $this->numericIdentifiers = $ids;
+    $this->callsignIdentifiers = $callsigns;
+    $this->phidIdentifiers = $phids;
+    return $this;
+  }
+
   public function withStatus($status) {
     $this->status = $status;
     return $this;
@@ -102,10 +129,22 @@ final class PhabricatorRepositoryQuery
     return $this;
   }
 
+  public function getIdentifierMap() {
+    if ($this->identifierMap === null) {
+      throw new Exception(
+        'You must execute() the query before accessing the identifier map.');
+    }
+    return $this->identifierMap;
+  }
+
   protected function loadPage() {
     $table = new PhabricatorRepository();
     $conn_r = $table->establishConnection('r');
 
+    if ($this->identifierMap === null) {
+      $this->identifierMap = array();
+    }
+
     $data = queryfx_all(
       $conn_r,
       'SELECT * FROM %T r %Q %Q %Q %Q',
@@ -202,6 +241,35 @@ final class PhabricatorRepositoryQuery
       }
     }
 
+    // Build the identifierMap
+    if ($this->numericIdentifiers) {
+      foreach ($this->numericIdentifiers as $id) {
+        if (isset($repositories[$id])) {
+          $this->identifierMap[$id] = $repositories[$id];
+        }
+      }
+    }
+
+    if ($this->callsignIdentifiers) {
+      $repository_callsigns = mpull($repositories, null, 'getCallsign');
+
+      foreach ($this->callsignIdentifiers as $callsign) {
+        if (isset($repository_callsigns[$callsign])) {
+          $this->identifierMap[$callsign] = $repository_callsigns[$callsign];
+        }
+      }
+    }
+
+    if ($this->phidIdentifiers) {
+      $repository_phids = mpull($repositories, null, 'getPHID');
+
+      foreach ($this->phidIdentifiers as $phid) {
+        if (isset($repository_phids[$phid])) {
+          $this->identifierMap[$phid] = $repository_phids[$phid];
+        }
+      }
+    }
+
     return $repositories;
   }
 
@@ -387,6 +455,35 @@ final class PhabricatorRepositoryQuery
         $this->callsigns);
     }
 
+    if ($this->numericIdentifiers ||
+      $this->callsignIdentifiers ||
+      $this->phidIdentifiers) {
+      $identifier_clause = array();
+
+      if ($this->numericIdentifiers) {
+        $identifier_clause[] = qsprintf(
+          $conn_r,
+          'r.id IN (%Ld)',
+          $this->numericIdentifiers);
+      }
+
+      if ($this->callsignIdentifiers) {
+        $identifier_clause[] = qsprintf(
+          $conn_r,
+          'r.callsign IN (%Ls)',
+          $this->callsignIdentifiers);
+      }
+
+      if ($this->phidIdentifiers) {
+        $identifier_clause[] = qsprintf(
+          $conn_r,
+          'r.phid IN (%Ls)',
+          $this->phidIdentifiers);
+      }
+
+      $where = array('('.implode(' OR ', $identifier_clause).')');
+    }
+
     if ($this->types) {
       $where[] = qsprintf(
         $conn_r,
@@ -420,7 +517,6 @@ final class PhabricatorRepositoryQuery
     return $this->formatWhereClause($where);
   }
 
-
   public function getQueryApplicationClass() {
     return 'PhabricatorDiffusionApplication';
   }