Provide "gitsprintf(...)" and disambiguate Git ref selectors

Summary: Ref T13589. See that task for discussion.

Test Plan:
  - Created this diff, ran most commands in isolation.
  - This change is difficult to test extensively.

Maniphest Tasks: T13589

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

src/__phutil_library_map__.php

@@ -762,6 +762,7 @@ phutil_register_library_map(array(
     'PhutilGitHubResponse' => 'future/github/PhutilGitHubResponse.php',
     'PhutilGitURI' => 'parser/PhutilGitURI.php',
     'PhutilGitURITestCase' => 'parser/__tests__/PhutilGitURITestCase.php',
+    'PhutilGitsprintfTestCase' => 'xsprintf/__tests__/PhutilGitsprintfTestCase.php',
     'PhutilHTMLParser' => 'parser/html/PhutilHTMLParser.php',
     'PhutilHTMLParserTestCase' => 'parser/html/__tests__/PhutilHTMLParserTestCase.php',
     'PhutilHTTPEngineExtension' => 'future/http/PhutilHTTPEngineExtension.php',
@@ -927,6 +928,7 @@ phutil_register_library_map(array(
     'csprintf' => 'xsprintf/csprintf.php',
     'exec_manual' => 'future/exec/execx.php',
     'execx' => 'future/exec/execx.php',
+    'gitsprintf' => 'xsprintf/gitsprintf.php',
     'head' => 'utils/utils.php',
     'head_key' => 'utils/utils.php',
     'hgsprintf' => 'xsprintf/hgsprintf.php',
@@ -1043,6 +1045,7 @@ phutil_register_library_map(array(
     'xsprintf' => 'xsprintf/xsprintf.php',
     'xsprintf_callback_example' => 'xsprintf/xsprintf.php',
     'xsprintf_command' => 'xsprintf/csprintf.php',
+    'xsprintf_git' => 'xsprintf/gitsprintf.php',
     'xsprintf_javascript' => 'xsprintf/jsprintf.php',
     'xsprintf_ldap' => 'xsprintf/ldapsprintf.php',
     'xsprintf_mercurial' => 'xsprintf/hgsprintf.php',
@@ -1825,6 +1828,7 @@ phutil_register_library_map(array(
     'PhutilGitHubResponse' => 'Phobject',
     'PhutilGitURI' => 'Phobject',
     'PhutilGitURITestCase' => 'PhutilTestCase',
+    'PhutilGitsprintfTestCase' => 'PhutilTestCase',
     'PhutilHTMLParser' => 'Phobject',
     'PhutilHTMLParserTestCase' => 'PhutilTestCase',
     'PhutilHTTPEngineExtension' => 'Phobject',

src/land/engine/ArcanistGitLandEngine.php

@@ -274,9 +274,11 @@ final class ArcanistGitLandEngine
     // as changes.
 
     list($changes) = $api->execxLocal(
-      'diff --no-ext-diff %s..%s --',
+      'diff --no-ext-diff %s --',
-      $into_commit,
+      gitsprintf(
-      $max_hash);
+        '%s..%s',
+        $into_commit,
+        $max_hash));
     $changes = trim($changes);
     if (!strlen($changes)) {
 
@@ -762,7 +764,7 @@ final class ArcanistGitLandEngine
     $api = $this->getRepositoryAPI();
 
     list($stdout) = $api->execxLocal(
-      'merge-base %s %s',
+      'merge-base -- %s %s',
       $branch,
       $commit);
     $merge_base = trim($stdout);
@@ -781,7 +783,7 @@ final class ArcanistGitLandEngine
     list($info) = $api->execxLocal(
       'log -n1 --format=%s %s --',
       '%aD%n%an%n%ae',
-      $commit);
+      gitsprintf('%s', $commit));
 
     $info = trim($info);
     list($date, $author, $email) = explode("\n", $info, 3);
@@ -1452,19 +1454,19 @@ final class ArcanistGitLandEngine
     $commit_map = array();
     foreach ($symbols as $symbol) {
       $symbol_commit = $symbol->getCommit();
-      $format = '%H%x00%P%x00%s%x00';
+      $format = '--format=%H%x00%P%x00%s%x00';
 
       if ($into_commit === null) {
         list($commits) = $api->execxLocal(
-          'log %s --format=%s',
+          'log %s %s --',
-          $symbol_commit,
+          $format,
-          $format);
+          gitsprintf('%s', $symbol_commit));
       } else {
         list($commits) = $api->execxLocal(
-          'log %s --not %s --format=%s',
+          'log %s %s --not %s --',
-          $symbol_commit,
+          $format,
-          $into_commit,
+          gitsprintf('%s', $symbol_commit),
-          $format);
+          gitsprintf('%s', $into_commit));
       }
 
       $commits = phutil_split_lines($commits, false);

src/query/ArcanistGitCommitMessageHardpointQuery.php

@@ -27,7 +27,7 @@ final class ArcanistGitCommitMessageHardpointQuery
       $futures[$hash] = $api->execFutureLocal(
         'log -n1 --format=%s %s --',
         '%s%n%n%b',
-        $hash);
+        gitsprintf('%s', $hash));
     }
 
     yield $this->yieldFutures($futures);

src/repository/api/ArcanistGitAPI.php

@@ -88,7 +88,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
    */
   private function isDescendant($child, $parent) {
     list($common_ancestor) = $this->execxLocal(
-      'merge-base %s %s',
+      'merge-base -- %s %s',
       $child,
       $parent);
     $common_ancestor = trim($common_ancestor);
@@ -214,7 +214,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
       }
 
       list($err, $merge_base) = $this->execManualLocal(
-        'merge-base %s %s',
+        'merge-base -- %s %s',
         $symbolic_commit,
         $this->getHeadCommit());
       if ($err) {
@@ -381,7 +381,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
     }
 
     list($merge_base) = $this->execxLocal(
-      'merge-base %s HEAD',
+      'merge-base -- %s HEAD',
       $default_relative);
 
     return trim($merge_base);
@@ -569,22 +569,24 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
     } else if ($relative == self::GIT_MAGIC_ROOT_COMMIT) {
       // First commit.
       list($stdout) = $this->execxLocal(
-        'log --format=medium HEAD');
+        'log --format=medium HEAD --');
     } else {
       // 2..N commits.
       list($stdout) = $this->execxLocal(
-        'log --first-parent --format=medium %s..%s',
+        'log --first-parent --format=medium %s --',
-        $this->getBaseCommit(),
+        gitsprintf(
-        $this->getHeadCommit());
+          '%s..%s',
+          $this->getBaseCommit(),
+          $this->getHeadCommit()));
     }
     return $stdout;
   }
 
   public function getGitHistoryLog() {
     list($stdout) = $this->execxLocal(
-      'log --format=medium -n%d %s',
+      'log --format=medium -n%d %s --',
       self::SEARCH_LENGTH_FOR_PARENT_REVISIONS,
-      $this->getBaseCommit());
+      gitsprintf('%s', $this->getBaseCommit()));
     return $stdout;
   }
 
@@ -721,7 +723,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
       array(
         'diff %C --raw %s --',
         $diff_options,
-        $diff_base,
+        gitsprintf('%s', $diff_base),
       ));
 
     $untracked_future = $this->buildLocalFuture(
@@ -782,7 +784,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
     list($stdout, $stderr) = $this->execxLocal(
       'diff %C --raw %s HEAD --',
       $this->getDiffBaseOptions(),
-      $this->getBaseCommit());
+      gitsprintf('%s', $this->getBaseCommit()));
 
     return $this->parseGitRawDiff($stdout);
   }
@@ -935,15 +937,15 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
 
   public function getChangedFiles($since_commit) {
     list($stdout) = $this->execxLocal(
-      'diff --raw %s',
+      'diff --raw %s --',
-      $since_commit);
+      gitsprintf('%s', $since_commit));
     return $this->parseGitRawDiff($stdout);
   }
 
   public function getBlame($path) {
     list($stdout) = $this->execxLocal(
       'blame --porcelain -w -M %s -- %s',
-      $this->getBaseCommit(),
+      gitsprintf('%s', $this->getBaseCommit()),
       $path);
 
     // the --porcelain format prints at least one header line per source line,
@@ -1030,7 +1032,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
 
     list($stdout) = $this->execxLocal(
       'ls-tree %s -- %s',
-      $revision,
+      gitsprintf('%s', $revision),
       $path);
 
     $info = $this->parseGitTree($stdout);
@@ -1046,7 +1048,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
     }
 
     list($stdout) = $this->execxLocal(
-      'cat-file blob %s',
+      'cat-file blob -- %s',
        $info[$path]['ref']);
     return $stdout;
   }
@@ -1171,7 +1173,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
     list($message) = $this->execxLocal(
       'log -n1 --format=%C %s --',
       '%s%n%n%b',
-      $commit);
+      gitsprintf('%s', $commit));
     return $message;
   }
 
@@ -1249,9 +1251,9 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
     }
 
     list($summary) = $this->execxLocal(
-      'log -n 1 --format=%C %s',
+      'log -n 1 %s %s --',
-      '%s',
+      '--format=%s',
-      $commit);
+      gitsprintf('%s', $commit));
 
     return trim($summary);
   }
@@ -1268,7 +1270,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
         $matches = null;
         if (preg_match('/^merge-base\((.+)\)$/', $name, $matches)) {
           list($err, $merge_base) = $this->execManualLocal(
-            'merge-base %s HEAD',
+            'merge-base -- %s HEAD',
             $matches[1]);
           if (!$err) {
             $this->setBaseCommitExplanation(
@@ -1282,7 +1284,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
           }
         } else if (preg_match('/^branch-unique\((.+)\)$/', $name, $matches)) {
           list($err, $merge_base) = $this->execManualLocal(
-            'merge-base %s HEAD',
+            'merge-base -- %s HEAD',
             $matches[1]);
           if ($err) {
             return null;
@@ -1400,7 +1402,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
             if (!$err) {
               $upstream = rtrim($upstream);
               list($upstream_merge_base) = $this->execxLocal(
-                'merge-base %s HEAD',
+                'merge-base -- %s HEAD',
                 $upstream);
               $upstream_merge_base = rtrim($upstream_merge_base);
               $this->setBaseCommitExplanation(

src/repository/graph/query/ArcanistGitCommitGraphQuery.php

@@ -84,7 +84,7 @@ final class ArcanistGitCommitGraphQuery
       $format = implode('%x02', $fields).'%x01';
 
       $future = $api->newFuture(
-        'log --format=%s %Ls --stdin',
+        'log --format=%s %Ls --stdin --',
         $format,
         $flags);
       $future->write($ref_blob);

src/xsprintf/__tests__/PhutilGitsprintfTestCase.php

@@ -0,0 +1,40 @@
+<?php
+
+final class PhutilGitsprintfTestCase extends PhutilTestCase {
+
+  public function testHgsprintf() {
+    $selectors = array(
+      'HEAD' => 'HEAD',
+      'master' => 'master',
+      'a..b' => 'a..b',
+      'feature^' => 'feature^',
+      '--flag' => false,
+    );
+
+    foreach ($selectors as $input => $expect) {
+      $caught = null;
+
+      try {
+        $output = gitsprintf('%s', $input);
+      } catch (Exception $ex) {
+        $caught = $ex;
+      } catch (Throwable $ex) {
+        $caught = $ex;
+      }
+
+      if ($caught !== null) {
+        $actual = false;
+      } else {
+        $actual = $output;
+      }
+
+      $this->assertEqual(
+        $expect,
+        $actual,
+        pht(
+          'Result for input "%s".',
+          $input));
+    }
+  }
+
+}

src/xsprintf/gitsprintf.php

@@ -0,0 +1,64 @@
+<?php
+
+/**
+ * Format a Git ref selector. This formatting is important when executing
+ * commands like "git log" which can not unambiguously parse all values as
+ * ref selectors.
+ *
+ * Supports the following conversions:
+ *
+ *  %s Ref Selector
+ *    Escapes a Git ref selector. In particular, this will reject ref selectors
+ *    which Git may interpret as flags.
+ *
+ *  %R Raw String
+ *    Passes text through unescaped.
+ */
+function gitsprintf($pattern /* , ... */) {
+  $args = func_get_args();
+  return xsprintf('xsprintf_git', null, $args);
+}
+
+/**
+ * @{function:xsprintf} callback for Git encoding.
+ */
+function xsprintf_git($userdata, &$pattern, &$pos, &$value, &$length) {
+  $type = $pattern[$pos];
+
+  switch ($type) {
+    case 's':
+
+      // See T13589. Some Git commands accept both a ref selector and a list of
+      // paths. For example:
+
+      //   $ git log <ref> -- <path> <path> ...
+
+      // These commands disambiguate ref selectors from paths using "--", but
+      // have no mechanism for disambiguating ref selectors from flags.
+
+      // Thus, there appears to be no way (in the general case) to safely
+      // invoke these commands with an arbitrary ref selector string: ref
+      // selector strings like "--flag" may be interpreted as flags, not as
+      // ref selectors.
+
+      // To resolve this, we reject any ref selector which begins with "-".
+      // These selectors are never valid anyway, so there is no loss of overall
+      // correctness. It would be more desirable to pass them to Git in a way
+      // that guarantees Git inteprets the string as a ref selector, but it
+      // appears that no mechanism exists to allow this.
+
+      if (preg_match('(^-)', $value)) {
+        throw new Exception(
+          pht(
+            'Git ref selector "%s" is not a valid selector and can not be '.
+            'passed to the Git CLI safely in the general case.',
+            $value));
+      }
+      break;
+    case 'R':
+      $type = 's';
+      break;
+  }
+
+  $pattern[$pos] = $type;
+}