Further correct and disambigutate ref selectors passed to Git on the CLI

Summary:
Ref T13589. In D21510, not every ref selector got touched, and this isn't a valid construction in Git:

```
$ git ls-tree ... -- ''
```

Thus:

  - Disambiguate more (all?) ref selectors.
  - Correct the construction of "git ls-tree" when there is no path.
  - Clean some stuff up: make the construction of some flags and arguments more explicit, get rid of a needless "%C", prefer "%Ls" over acrobatics, etc.

Test Plan: Browsed/updated a local Git repository. (This change is somewhat difficult to test exhaustively, as evidenced by the "ls-tree" issue in D21510.)

Maniphest Tasks: T13589

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

src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php

@@ -35,22 +35,26 @@ final class DiffusionBrowseQueryConduitAPIMethod
   protected function getGitResult(ConduitAPIRequest $request) {
     $drequest = $this->getDiffusionRequest();
     $repository = $drequest->getRepository();
+
     $path = $request->getValue('path');
+    if (!strlen($path)) {
+      $path = null;
+    }
+
     $commit = $request->getValue('commit');
     $offset = (int)$request->getValue('offset');
     $limit = (int)$request->getValue('limit');
     $result = $this->getEmptyResultSet();
 
-    if ($path == '') {
+    if ($path === null) {
       // Fast path to improve the performance of the repository view; we know
       // the root is always a tree at any commit and always exists.
       $stdout = 'tree';
     } else {
       try {
         list($stdout) = $repository->execxLocalCommand(
-          'cat-file -t -- %s:%s',
+          'cat-file -t -- %s',
-          $commit,
+          sprintf('%s:%s', $commit, $path));
-          $path);
       } catch (CommandException $e) {
         // The "cat-file" command may fail if the path legitimately does not
         // exist, but it may also fail if the path is a submodule. This can
@@ -121,14 +125,20 @@ final class DiffusionBrowseQueryConduitAPIMethod
       return $result;
     }
 
-    list($stdout) = $repository->execxLocalCommand(
+    if ($path === null) {
-      'ls-tree -z -l %s -- %s',
+      list($stdout) = $repository->execxLocalCommand(
-      gitsprintf('%s', $commit),
+        'ls-tree -z -l %s --',
-      $path);
+        gitsprintf('%s', $commit));
+    } else {
+      list($stdout) = $repository->execxLocalCommand(
+        'ls-tree -z -l %s -- %s',
+        gitsprintf('%s', $commit),
+        $path);
+    }
 
     $submodules = array();
 
-    if (strlen($path)) {
+    if ($path !== null) {
       $prefix = rtrim($path, '/').'/';
     } else {
       $prefix = '';
@@ -226,8 +236,8 @@ final class DiffusionBrowseQueryConduitAPIMethod
           $dict[$key] = $value;
         }
 
-        foreach ($submodules as $path) {
+        foreach ($submodules as $submodule_path) {
-          $full_path = $path->getFullPath();
+          $full_path = $submodule_path->getFullPath();
           $key = 'submodule.'.$full_path.'.url';
           if (isset($dict[$key])) {
             $path->setExternalURI($dict[$key]);

src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php

@@ -45,7 +45,12 @@ final class DiffusionHistoryQueryConduitAPIMethod
     $repository = $drequest->getRepository();
     $commit_hash = $request->getValue('commit');
     $against_hash = $request->getValue('against');
+
     $path = $request->getValue('path');
+    if (!strlen($path)) {
+      $path = null;
+    }
+
     $offset = $request->getValue('offset');
     $limit = $request->getValue('limit');
 
@@ -55,18 +60,27 @@ final class DiffusionHistoryQueryConduitAPIMethod
       $commit_range = $commit_hash;
     }
 
+    $argv = array();
+
+    $argv[] = '--skip';
+    $argv[] = $offset;
+
+    $argv[] = '--max-count';
+    $argv[] = $limit;
+
+    $argv[] = '--format=%H:%P';
+
+    $argv[] = gitsprintf('%s', $commit_range);
+
+    $argv[] = '--';
+
+    if ($path !== null) {
+      $argv[] = $path;
+    }
+
     list($stdout) = $repository->execxLocalCommand(
-      'log '.
+      'log %Ls',
-        '--skip=%d '.
+      $argv);
-        '-n %d '.
-        '--pretty=format:%s '.
-        '%s -- %C',
-      $offset,
-      $limit,
-      '%H:%P',
-      gitsprintf('%s', $commit_range),
-      // Git omits merge commits if the path is provided, even if it is empty.
-      (strlen($path) ? csprintf('%s', $path) : ''));
 
     $lines = explode("\n", trim($stdout));
     $lines = array_filter($lines);

src/applications/diffusion/conduit/DiffusionInternalGitRawDiffQueryConduitAPIMethod.php

@@ -43,9 +43,9 @@ final class DiffusionInternalGitRawDiffQueryConduitAPIMethod
     // it requires the commit to have a parent that we can diff against. The
     // first commit doesn't, so "commit^" is not a valid ref.
     list($parents) = $repository->execxLocalCommand(
-      'log -n1 --format=%s %s',
+      'log -n1 %s %s --',
-      '%P',
+      '--format=%P',
-      $commit);
+      gitsprintf('%s', $commit));
     $use_log = !strlen(trim($parents));
 
     // First, get a fast raw diff without "--find-copies-harder". This flag
@@ -96,18 +96,18 @@ final class DiffusionInternalGitRawDiffQueryConduitAPIMethod
       // NOTE: "--pretty=format: " is to disable diff output, we only want the
       // part we get from "--raw".
       $future = $repository->getLocalCommandFuture(
-        'log %Ls --pretty=format: %s',
+        'log %Ls --pretty=format: %s --',
         $flags,
-        $commit);
+        gitsprintf('%s', $commit));
     } else {
       // Otherwise, we can use "diff", which will give us output for merges.
       // We diff against the first parent, as this is generally the expectation
       // and results in sensible behavior.
       $future = $repository->getLocalCommandFuture(
-        'diff %Ls %s^1 %s',
+        'diff %Ls %s %s --',
         $flags,
-        $commit,
+        gitsprintf('%s^1', $commit),
-        $commit);
+        gitsprintf('%s', $commit));
     }
 
     // Don't spend more than 30 seconds generating the slower output.

src/applications/diffusion/conduit/DiffusionLastModifiedQueryConduitAPIMethod.php

@@ -33,7 +33,8 @@ final class DiffusionLastModifiedQueryConduitAPIMethod
         continue;
       }
       list($hash) = $repository->execxLocalCommand(
-        'log -n1 --format=%%H %s -- %s',
+        'log -n1 %s %s -- %s',
+        '--format=%H',
         gitsprintf('%s', $commit),
         $path);
       $results[$path] = trim($hash);

src/applications/diffusion/conduit/DiffusionMergedCommitsQueryConduitAPIMethod.php

@@ -35,8 +35,8 @@ final class DiffusionMergedCommitsQueryConduitAPIMethod
     $limit = $this->getLimit($request);
 
     list($parents) = $repository->execxLocalCommand(
-      'log -n 1 --format=%s %s --',
+      'log -n 1 %s %s --',
-      '%P',
+      '--format=%P',
       gitsprintf('%s', $commit));
 
     $parents = preg_split('/\s+/', trim($parents));
@@ -50,10 +50,10 @@ final class DiffusionMergedCommitsQueryConduitAPIMethod
 
     $first_parent = head($parents);
     list($logs) = $repository->execxLocalCommand(
-      'log -n %d --format=%s %s %s --',
+      'log -n %d %s %s %s --',
       // NOTE: "+ 1" accounts for the merge commit itself.
       $limit + 1,
-      '%H',
+      '--format=%H',
       gitsprintf('%s', $commit),
       gitsprintf('%s', '^'.$first_parent));
 

src/applications/diffusion/conduit/DiffusionRefsQueryConduitAPIMethod.php

@@ -28,9 +28,9 @@ final class DiffusionRefsQueryConduitAPIMethod
     $commit = $request->getValue('commit');
 
     list($stdout) = $repository->execxLocalCommand(
-      'log --format=%s -n 1 %s --',
+      'log -n 1 %s %s --',
-      '%d',
+      '--format=%d',
-      $commit);
+      gitsprintf('%s', $commit));
 
     // %d, gives a weird output format
     // similar to (remote/one, remote/two, remote/three)

src/applications/diffusion/engine/DiffusionCommitHookEngine.php

@@ -608,9 +608,9 @@ final class DiffusionCommitHookEngine extends Phobject {
       // repository. Particularly, this will cover the cases of a new branch, a
       // completely moved tag, etc.
       $futures[$key] = $this->getRepository()->getLocalCommandFuture(
-        'log --format=%s %s --not --all',
+        'log %s %s --not --all --',
-        '%H',
+        '--format=%H',
-        $ref_update->getRefNew());
+        gitsprintf('%s', $ref_update->getRefNew()));
     }
 
     $content_updates = array();

src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php

@@ -10,9 +10,8 @@ final class DiffusionGitFileContentQuery extends DiffusionFileContentQuery {
     $commit = $drequest->getCommit();
 
     return $repository->getLocalCommandFuture(
-      'cat-file blob -- %s:%s',
+      'cat-file blob -- %s',
-      $commit,
+      sprintf('%s:%s', $commit, $path));
-      $path);
   }
 
 }

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

@@ -55,12 +55,15 @@ final class DiffusionLowLevelCommitQuery
       $split_body = true;
     }
 
-    // Even though we pass --encoding here, git doesn't always succeed, so
+    $argv = array();
-    // we try a little harder, since git *does* tell us what the actual encoding
+
-    // is correctly (unless it doesn't; encoding is sometimes empty).
+    $argv[] = '-n';
-    list($info) = $repository->execxLocalCommand(
+    $argv[] = '1';
-      'log -n 1 --encoding=%s --format=%s %s --',
+
-      'UTF-8',
+    $argv[] = '--encoding=UTF-8';
+
+    $argv[] = sprintf(
+      '--format=%s',
       implode(
         '%x00',
         array(
@@ -78,8 +81,15 @@ final class DiffusionLowLevelCommitQuery
           // so include an explicit terminator: this makes sure the exact
           // body text is surrounded by "\0" characters.
           '~',
-        )),
+        )));
-      $this->identifier);
+
+    // Even though we pass --encoding here, git doesn't always succeed, so
+    // we try a little harder, since git *does* tell us what the actual encoding
+    // is correctly (unless it doesn't; encoding is sometimes empty).
+    list($info) = $repository->execxLocalCommand(
+      'log -n 1 %Ls %s --',
+      $argv,
+      gitsprintf('%s', $this->identifier));
 
     $parts = explode("\0", $info);
     $encoding = array_shift($parts);

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

@@ -33,7 +33,7 @@ final class DiffusionLowLevelFilesizeQuery
 
     $paths_future = $repository->getLocalCommandFuture(
       'diff-tree -z -r --no-commit-id %s --',
-      $identifier);
+      gitsprintf('%s', $identifier));
 
     // With "-z" we get "<fields>\0<filename>\0" for each line. Process the
     // delimited text as "<fields>, <filename>" pairs.

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

@@ -37,8 +37,8 @@ final class DiffusionLowLevelParentsQuery
     $repository = $this->getRepository();
 
     list($stdout) = $repository->execxLocalCommand(
-      'log -n 1 --format=%s %s --',
+      'log -n 1 %s %s --',
-      '%P',
+      '--format=%P',
       gitsprintf('%s', $this->identifier));
 
     return preg_split('/\s+/', trim($stdout));

src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php

@@ -23,8 +23,8 @@ final class DiffusionGitRawDiffQuery extends DiffusionRawDiffQuery {
       // Check if this is the root commit by seeing if it has parents, since
       // `git diff X^ X` does not work if "X" is the initial commit.
       list($parents) = $repository->execxLocalCommand(
-        'log -n 1 --format=%s %s --',
+        'log -n 1 %s --',
-        '%P',
+        '--format=%P',
         gitsprintf('%s', $commit));
 
       if (strlen(trim($parents))) {

src/applications/repository/daemon/PhabricatorGitGraphStream.php

@@ -19,13 +19,13 @@ final class PhabricatorGitGraphStream
 
     if ($start_commit !== null) {
       $future = $repository->getLocalCommandFuture(
-        'log --format=%s %s --',
+        'log %s %s --',
-        '%H%x01%P%x01%ct',
+        '--format=%H%x01%P%x01%ct',
-        $start_commit);
+        gitsprintf('%s', $start_commit));
     } else {
       $future = $repository->getLocalCommandFuture(
-        'log --format=%s --all --',
+        'log %s --all --',
-        '%H%x01%P%x01%ct');
+        '--format=%H%x01%P%x01%ct');
     }
 
     $this->iterator = new LinesOfALargeExecFuture($future);

src/applications/repository/engine/PhabricatorRepositoryRefEngine.php

@@ -483,17 +483,17 @@ final class PhabricatorRepositoryRefEngine
           $ref_list = implode("\n", $ref_list)."\n";
 
           $future = $this->getRepository()->getLocalCommandFuture(
-            'log --format=%s --stdin',
+            'log %s --stdin --',
-            '%H');
+            '--format=%H');
 
           list($stdout) = $future
             ->write($ref_list)
             ->resolvex();
         } else {
           list($stdout) = $this->getRepository()->execxLocalCommand(
-            'log --format=%s %s',
+            'log %s %s --',
-            '%H',
+            '--format=%H',
-            $new_head);
+            gitsprintf('%s', $new_head));
         }
 
         $stdout = trim($stdout);

src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php

@@ -4,6 +4,8 @@ final class PhabricatorWorkingCopyDiscoveryTestCase
   extends PhabricatorWorkingCopyTestCase {
 
   public function testSubversionCommitDiscovery() {
+    $this->requireBinaryForTest('svn');
+
     $refs = $this->discoverRefs('ST');
     $this->assertEqual(
       array(