@matheusbsbs / matheus-bagliano / commits / 3a1f91cfd9

rev-parse: handle --end-of-options

We taught rev-list a new way to separate options from revisions in 19e8789b23 (revision: allow --end-of-options to end option parsing, 2019-08-06), but rev-parse uses its own parser. It should know about --end-of-options not only for consistency, but because it may be presented with similarly ambiguous cases. E.g., if a caller does: git rev-parse "$rev" -- "$path" to parse an untrusted input, then it will get confused if $rev contains an option-like string like "--local-env-vars". Or even "--not-real", which we'd keep as an option to pass along to rev-list. Or even more importantly: git rev-parse --verify "$rev" can be confused by options, even though its purpose is safely parsing untrusted input. On the plus side, it will always fail the --verify part, as it will not have parsed a revision, so the caller will generally "fail closed" rather than continue to use the untrusted string. But it will still trigger whatever option was in "$rev"; this should be mostly harmless, since rev-parse options are all read-only, but I didn't carefully audit all paths. This patch lets callers write: git rev-parse --end-of-options "$rev" -- "$path" and: git rev-parse --verify --end-of-options "$rev" which will both treat "$rev" always as a revision parameter. The latter is a bit clunky. It would be nicer if we had defined "--verify" to require that its next argument be the revision. But we have not historically done so, and: git rev-parse --verify -q "$rev" does currently work. I added a test here to confirm that we didn't break that. A few implementation notes: - We don't document --end-of-options explicitly in commands, but rather in gitcli(7). So I didn't give it its own section in git-rev-parse(1). But I did call it out specifically in the --verify section, and include it in the examples, which should show best practices. - We don't have to re-indent the main option-parsing block, because we can combine our "did we see end of options" check with "does it start with a dash". The exception is the pre-setup options, which need their own block. - We do however have to pull the "--" parsing out of the "does it start with dash" block, because we want to parse it even if we've seen --end-of-options. - We'll leave "--end-of-options" in the output. This is probably not technically necessary, as a careful caller will do: git rev-parse --end-of-options $revs -- $paths and anything in $revs will be resolved to an object id. However, it does help a slightly less careful caller like: git rev-parse --end-of-options $revs_or_paths where a path "--foo" will remain in the output as long as it also exists on disk. In that case, it's helpful to retain --end-of-options to get passed along to rev-list, s it would otherwise see just "--foo". Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Nov 10, 2020 at 16:40 UTC 3a1f91cfd9633a4f59e0534fa5ba076e031a78ed

4 files changed +68 -25

Documentation/git-rev-parse.txt

+6 -2

@@ -109,6 +109,10 @@ names an existing object that is a commit-ish (i.e. a commit, or an

109 annotated tag that points at a commit). To make sure that `$VAR`

110 names an existing object of any type, `git rev-parse "$VAR^{object}"`

111 can be used.

112 ++

113 +Note that if you are verifying a name from an untrusted source, it is

114 +wise to use `--end-of-options` so that the name argument is not mistaken

115 +for another option.

116

117 -q::

118 --quiet::

@@ -446,7 +450,7 @@ $ git rev-parse --verify HEAD

450 * Print the commit object name from the revision in the $REV shell variable:

451 +

452 ------------

449 -$ git rev-parse --verify $REV^{commit}

453 +$ git rev-parse --verify --end-of-options $REV^{commit}

454 ------------

455 +

456 This will error out if $REV is empty or not a valid revision.

@@ -454,7 +458,7 @@ This will error out if $REV is empty or not a valid revision.

458 * Similar to above:

459 +

460 ------------

457 -$ git rev-parse --default master --verify $REV

461 +$ git rev-parse --default master --verify --end-of-options $REV

462 ------------

463 +

464 but if $REV is empty, the commit object name from master will be printed.

builtin/rev-parse.c

+33 -23

@@ -595,6 +595,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)

595 struct object_context unused;

596 struct strbuf buf = STRBUF_INIT;

597 const int hexsz = the_hash_algo->hexsz;

598 + int seen_end_of_options = 0;

599

600 if (argc > 1 && !strcmp("--parseopt", argv[1]))

601 return cmd_parseopt(argc - 1, argv + 1, prefix);

@@ -628,21 +629,23 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)

629 continue;

630 }

631

631 - if (!strcmp(arg, "--local-env-vars")) {

632 - int i;

633 - for (i = 0; local_repo_env[i]; i++)

634 - printf("%s\n", local_repo_env[i]);

635 - continue;

636 - }

637 - if (!strcmp(arg, "--resolve-git-dir")) {

638 - const char *gitdir = argv[++i];

639 - if (!gitdir)

640 - die("--resolve-git-dir requires an argument");

641 - gitdir = resolve_gitdir(gitdir);

642 - if (!gitdir)

643 - die("not a gitdir '%s'", argv[i]);

644 - puts(gitdir);

645 - continue;

632 + if (!seen_end_of_options) {

633 + if (!strcmp(arg, "--local-env-vars")) {

634 + int i;

635 + for (i = 0; local_repo_env[i]; i++)

636 + printf("%s\n", local_repo_env[i]);

637 + continue;

638 + }

639 + if (!strcmp(arg, "--resolve-git-dir")) {

640 + const char *gitdir = argv[++i];

641 + if (!gitdir)

642 + die("--resolve-git-dir requires an argument");

643 + gitdir = resolve_gitdir(gitdir);

644 + if (!gitdir)

645 + die("not a gitdir '%s'", argv[i]);

646 + puts(gitdir);

647 + continue;

648 + }

649 }

650

651 /* The rest of the options require a git repository. */

@@ -652,14 +655,15 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)

655 did_repo_setup = 1;

656 }

657

655 - if (*arg == '-') {

656 - if (!strcmp(arg, "--")) {

657 - as_is = 2;

658 - /* Pass on the "--" if we show anything but files.. */

659 - if (filter & (DO_FLAGS | DO_REVS))

660 - show_file(arg, 0);

661 - continue;

662 - }

658 + if (!strcmp(arg, "--")) {

659 + as_is = 2;

660 + /* Pass on the "--" if we show anything but files.. */

661 + if (filter & (DO_FLAGS | DO_REVS))

662 + show_file(arg, 0);

663 + continue;

664 + }

665 +

666 + if (!seen_end_of_options && *arg == '-') {

667 if (!strcmp(arg, "--git-path")) {

668 if (!argv[i + 1])

669 die("--git-path requires an argument");

@@ -937,6 +941,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)

941 puts(the_hash_algo->name);

942 continue;

943 }

944 + if (!strcmp(arg, "--end-of-options")) {

945 + seen_end_of_options = 1;

946 + if (filter & (DO_FLAGS | DO_REVS))

947 + show_file(arg, 0);

948 + continue;

949 + }

950 if (show_flag(arg) && verify)

951 die_no_single_rev(quiet);

952 continue;

t/t1503-rev-parse-verify.sh

+13

@@ -144,4 +144,17 @@ test_expect_success SYMLINKS 'ref resolution not confused by broken symlinks' '

144 test_must_fail git rev-parse --verify broken

145 '

146

147 +test_expect_success 'options can appear after --verify' '

148 + git rev-parse --verify HEAD >expect &&

149 + git rev-parse --verify -q HEAD >actual &&

150 + test_cmp expect actual

151 +'

152 +

153 +test_expect_success 'verify respects --end-of-options' '

154 + git update-ref refs/heads/-tricky HEAD &&

155 + git rev-parse --verify HEAD >expect &&

156 + git rev-parse --verify --end-of-options -tricky >actual &&

157 + test_cmp expect actual

158 +'

159 +

160 test_done

t/t1506-rev-parse-diagnosis.sh

+16

@@ -263,4 +263,20 @@ test_expect_success 'arg after dashdash not interpreted as option' '

263 test_cmp expect actual

264 '

265

266 +test_expect_success 'arg after end-of-options not interpreted as option' '

267 + test_must_fail git rev-parse --end-of-options --not-real -- 2>err &&

268 + test_i18ngrep bad.revision.*--not-real err

269 +'

270 +

271 +test_expect_success 'end-of-options still allows --' '

272 + cat >expect <<-EOF &&

273 + --end-of-options

274 + $(git rev-parse --verify HEAD)

275 + --

276 + path

277 + EOF

278 + git rev-parse --end-of-options HEAD -- path >actual &&

279 + test_cmp expect actual

280 +'

281 +

282 test_done