Improve ref resolution for Git branches and tags

Summary:
Fixes T7982.

  - When resolving branches, make sure they get type `'branch'`.
  - Correctly resolve refs when a repository has a branch and tag with the same name.

Test Plan: Disabled ref cache and resolved refs in a Git repository with a 'master' tag and a 'master' branch. Saw refs resolve accurately.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7982

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

src/applications/diffusion/query/lowlevel/DiffusionLowLevelGitRefQuery.php

@@ -3,8 +3,6 @@
 /**
  * Execute and parse a low-level Git ref query using `git for-each-ref`. This
  * is useful for returning a list of tags or branches.
- *
- *
  */
 final class DiffusionLowLevelGitRefQuery extends DiffusionLowLevelQuery {
 
@@ -24,66 +22,85 @@ final class DiffusionLowLevelGitRefQuery extends DiffusionLowLevelQuery {
   protected function executeQuery() {
     $repository = $this->getRepository();
 
-    if ($this->isTag && $this->isOriginBranch) {
-      throw new Exception('Specify tags or origin branches, not both!');
-    } else if ($this->isTag) {
-      $prefix = 'refs/tags/';
-    } else if ($this->isOriginBranch) {
+    $prefixes = array();
+
+    $any = ($this->isTag || $this->isOriginBranch);
+    if (!$any) {
+      throw new Exception(pht('Specify types of refs to query.'));
+    }
+
+    if ($this->isOriginBranch) {
       if ($repository->isWorkingCopyBare()) {
         $prefix = 'refs/heads/';
       } else {
         $remote = DiffusionGitBranch::DEFAULT_GIT_REMOTE;
         $prefix = 'refs/remotes/'.$remote.'/';
       }
-    } else {
-      throw new Exception('Specify tags or origin branches!');
+      $prefixes[] = $prefix;
+    }
+
+    if ($this->isTag) {
+      $prefixes[] = 'refs/tags/';
     }
 
     $order = '-creatordate';
 
-    list($stdout) = $repository->execxLocalCommand(
-      'for-each-ref --sort=%s --format=%s %s',
-      $order,
-      $this->getFormatString(),
-      $prefix);
-
-    $stdout = rtrim($stdout);
-    if (!strlen($stdout)) {
-      return array();
+    $futures = array();
+    foreach ($prefixes as $prefix) {
+      $futures[$prefix] = $repository->getLocalCommandFuture(
+        'for-each-ref --sort=%s --format=%s %s',
+        $order,
+        $this->getFormatString(),
+        $prefix);
     }
 
-    // NOTE: Although git supports --count, we can't apply any offset or limit
-    // logic until the very end because we may encounter a HEAD which we want
-    // to discard.
+    // Resolve all the futures first. We want to iterate over them in prefix
+    // order, not resolution order.
+    foreach (new FutureIterator($futures) as $prefix => $future) {
+      $future->resolvex();
+    }
 
-    $lines = explode("\n", $stdout);
     $results = array();
-    foreach ($lines as $line) {
-      $fields = $this->extractFields($line);
+    foreach ($futures as $prefix => $future) {
+      list($stdout) = $future->resolvex();
 
-      $creator = $fields['creator'];
-      $matches = null;
-      if (preg_match('/^(.*) ([0-9]+) ([0-9+-]+)$/', $creator, $matches)) {
-        $fields['author'] = $matches[1];
-        $fields['epoch'] = (int)$matches[2];
-      } else {
-        $fields['author'] = null;
-        $fields['epoch'] = null;
-      }
-
-      $commit = nonempty($fields['*objectname'], $fields['objectname']);
-
-      $short = substr($fields['refname'], strlen($prefix));
-      if ($short == 'HEAD') {
+      $stdout = rtrim($stdout);
+      if (!strlen($stdout)) {
         continue;
       }
 
-      $ref = id(new DiffusionRepositoryRef())
-        ->setShortName($short)
-        ->setCommitIdentifier($commit)
-        ->setRawFields($fields);
+      // NOTE: Although git supports --count, we can't apply any offset or
+      // limit logic until the very end because we may encounter a HEAD which
+      // we want to discard.
 
-      $results[] = $ref;
+      $lines = explode("\n", $stdout);
+      foreach ($lines as $line) {
+        $fields = $this->extractFields($line);
+
+        $creator = $fields['creator'];
+        $matches = null;
+        if (preg_match('/^(.*) ([0-9]+) ([0-9+-]+)$/', $creator, $matches)) {
+          $fields['author'] = $matches[1];
+          $fields['epoch'] = (int)$matches[2];
+        } else {
+          $fields['author'] = null;
+          $fields['epoch'] = null;
+        }
+
+        $commit = nonempty($fields['*objectname'], $fields['objectname']);
+
+        $short = substr($fields['refname'], strlen($prefix));
+        if ($short == 'HEAD') {
+          continue;
+        }
+
+        $ref = id(new DiffusionRepositoryRef())
+          ->setShortName($short)
+          ->setCommitIdentifier($commit)
+          ->setRawFields($fields);
+
+        $results[] = $ref;
+      }
     }
 
     return $results;

src/applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php

@@ -55,21 +55,70 @@ final class DiffusionLowLevelResolveRefsQuery
   private function resolveGitRefs() {
     $repository = $this->getRepository();
 
-    // TODO: When refs are ambiguous (for example, tags and branches with
-    // the same name) this will only resolve one of them.
+    $unresolved = array_fuse($this->refs);
+    $results = array();
+
+    // First, resolve branches and tags.
+    $ref_map = id(new DiffusionLowLevelGitRefQuery())
+      ->setRepository($repository)
+      ->withIsTag(true)
+      ->withIsOriginBranch(true)
+      ->execute();
+    $ref_map = mgroup($ref_map, 'getShortName');
+
+    $tag_prefix = 'refs/tags/';
+    foreach ($unresolved as $ref) {
+      if (empty($ref_map[$ref])) {
+        continue;
+      }
+
+      foreach ($ref_map[$ref] as $result) {
+        $fields = $result->getRawFields();
+        $objectname = idx($fields, 'refname');
+        if (!strncmp($objectname, $tag_prefix, strlen($tag_prefix))) {
+          $type = 'tag';
+        } else {
+          $type = 'branch';
+        }
+
+        $info = array(
+          'type' => $type,
+          'identifier' => $result->getCommitIdentifier(),
+        );
+
+        if ($type == 'tag') {
+          $alternate = idx($fields, 'objectname');
+          if ($alternate) {
+            $info['alternate'] = $alternate;
+          }
+        }
+
+        $results[$ref][] = $info;
+      }
+
+      unset($unresolved[$ref]);
+    }
+
+    // If we resolved everything, we're done.
+    if (!$unresolved) {
+      return $results;
+    }
+
+    // Try to resolve anything else. This stuff either doesn't exist or is
+    // some ref like "HEAD^^^".
     $future = $repository->getLocalCommandFuture('cat-file --batch-check');
-    $future->write(implode("\n", $this->refs));
+    $future->write(implode("\n", $unresolved));
     list($stdout) = $future->resolvex();
 
     $lines = explode("\n", rtrim($stdout, "\n"));
-    if (count($lines) !== count($this->refs)) {
+    if (count($lines) !== count($unresolved)) {
       throw new Exception('Unexpected line count from `git cat-file`!');
     }
 
     $hits = array();
     $tags = array();
 
-    $lines = array_combine($this->refs, $lines);
+    $lines = array_combine($unresolved, $lines);
     foreach ($lines as $ref => $line) {
       $parts = explode(' ', $line);
       if (count($parts) < 2) {

src/applications/diffusion/request/DiffusionGitRequest.php

@@ -20,13 +20,4 @@ final class DiffusionGitRequest extends DiffusionRequest {
     throw new Exception('Unable to determine branch!');
   }
 
-  protected function getResolvableBranchName($branch) {
-    if ($this->repository->isWorkingCopyBare()) {
-      return $branch;
-    } else {
-      $remote = DiffusionGitBranch::DEFAULT_GIT_REMOTE;
-      return $remote.'/'.$branch;
-    }
-  }
-
 }

src/applications/diffusion/request/DiffusionRequest.php

@@ -723,7 +723,7 @@ abstract class DiffusionRequest {
       $ref = $this->symbolicCommit;
     } else {
       if ($this->supportsBranches()) {
-        $ref = $this->getResolvableBranchName($this->getBranch());
+        $ref = $this->getBranch();
         $types = array(
           PhabricatorRepositoryRefCursor::TYPE_BRANCH,
         );
@@ -795,10 +795,6 @@ abstract class DiffusionRequest {
     return $match;
   }
 
-  protected function getResolvableBranchName($branch) {
-    return $branch;
-  }
-
   private function resolveRefs(array $refs, array $types) {
     // First, try to resolve refs from fast cache sources.
     $cached_query = id(new DiffusionCachedResolveRefsQuery())