From b2e715fc5a9ccd36048ae469850f293b0780403a Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 12 Jan 2021 10:33:47 -0800 Subject: [PATCH] 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 | 4 ++ src/land/engine/ArcanistGitLandEngine.php | 28 ++++---- ...ArcanistGitCommitMessageHardpointQuery.php | 2 +- src/repository/api/ArcanistGitAPI.php | 48 +++++++------- .../query/ArcanistGitCommitGraphQuery.php | 2 +- .../__tests__/PhutilGitsprintfTestCase.php | 40 ++++++++++++ src/xsprintf/gitsprintf.php | 64 +++++++++++++++++++ 7 files changed, 150 insertions(+), 38 deletions(-) create mode 100644 src/xsprintf/__tests__/PhutilGitsprintfTestCase.php create mode 100644 src/xsprintf/gitsprintf.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 23a9df4b..9f9914ec 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/land/engine/ArcanistGitLandEngine.php b/src/land/engine/ArcanistGitLandEngine.php index 76cce096..47161b27 100644 --- a/src/land/engine/ArcanistGitLandEngine.php +++ b/src/land/engine/ArcanistGitLandEngine.php @@ -274,9 +274,11 @@ final class ArcanistGitLandEngine // as changes. list($changes) = $api->execxLocal( - 'diff --no-ext-diff %s..%s --', - $into_commit, - $max_hash); + 'diff --no-ext-diff %s --', + gitsprintf( + '%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', - $symbol_commit, - $format); + 'log %s %s --', + $format, + gitsprintf('%s', $symbol_commit)); } else { list($commits) = $api->execxLocal( - 'log %s --not %s --format=%s', - $symbol_commit, - $into_commit, - $format); + 'log %s %s --not %s --', + $format, + gitsprintf('%s', $symbol_commit), + gitsprintf('%s', $into_commit)); } $commits = phutil_split_lines($commits, false); diff --git a/src/query/ArcanistGitCommitMessageHardpointQuery.php b/src/query/ArcanistGitCommitMessageHardpointQuery.php index fd5b1659..ba2a1b73 100644 --- a/src/query/ArcanistGitCommitMessageHardpointQuery.php +++ b/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); diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php index 2ce9e41b..6c6d2ac4 100644 --- a/src/repository/api/ArcanistGitAPI.php +++ b/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', - $this->getBaseCommit(), - $this->getHeadCommit()); + 'log --first-parent --format=medium %s --', + gitsprintf( + '%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', - $since_commit); + 'diff --raw %s --', + 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', - '%s', - $commit); + 'log -n 1 %s %s --', + '--format=%s', + 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( diff --git a/src/repository/graph/query/ArcanistGitCommitGraphQuery.php b/src/repository/graph/query/ArcanistGitCommitGraphQuery.php index 53587e45..32a3a08e 100644 --- a/src/repository/graph/query/ArcanistGitCommitGraphQuery.php +++ b/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); diff --git a/src/xsprintf/__tests__/PhutilGitsprintfTestCase.php b/src/xsprintf/__tests__/PhutilGitsprintfTestCase.php new file mode 100644 index 00000000..493b887b --- /dev/null +++ b/src/xsprintf/__tests__/PhutilGitsprintfTestCase.php @@ -0,0 +1,40 @@ + '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)); + } + } + +} diff --git a/src/xsprintf/gitsprintf.php b/src/xsprintf/gitsprintf.php new file mode 100644 index 00000000..73a85ecd --- /dev/null +++ b/src/xsprintf/gitsprintf.php @@ -0,0 +1,64 @@ + -- ... + + // 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; +}