[1/3] rev-parse: don't accept options after dashdash

Commit Message

Jeff King Nov. 10, 2020, 9:37 p.m. UTC

Because of the order in which we check options in rev-parse, there are a
few options we accept even after a "--". This is wrong, because the
whole point of "--" is to say "everything after here is a path". Let's
move the "did we see a dashdash" check (it's called "as_is" in the code)
to the top of the parsing loop.

Note there is one subtlety here. The options are ordered so that some
are checked before we even see if we're in a repository (they continue
the loop, and if we get past a certain point, then we do the repository
setup). By moving the as_is check higher, it's also in that "before
setup" section, even though it might look at the repository via
verify_filename(). However, this works out: we'd never set as_is until
we parse "--", and we don't parse that until after doing the setup.

An alternative here to avoid the subtlety is to put the as_is check at
the top of the post-setup options. But then every pre-setup option would
have to remember to check "if (!as_is && !strcmp(...))". So while this
is a bit magical, it's harder for future code to get wrong.

Signed-off-by: Jeff King <[e-mail]>
---
I guess it could also shove them all into an:

  if (!as_is) {
	if (!strcmp("--local-env-vars"))
  }

conditional. I do end up having to do that later for end-of-options
anyway.

 builtin/rev-parse.c            | 11 ++++++-----
 t/t1506-rev-parse-diagnosis.sh |  9 +++++++++
 2 files changed, 15 insertions(+), 5 deletions(-)

Patch

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index ed200c8af1..293428fa0d 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -622,6 +622,12 @@  int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 
+		if (as_is) {
+			if (show_file(arg, output_prefix) && as_is < 2)
+				verify_filename(prefix, arg, 0);
+			continue;
+		}
+
 		if (!strcmp(arg, "--local-env-vars")) {
 			int i;
 			for (i = 0; local_repo_env[i]; i++)
@@ -655,11 +661,6 @@  int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 			i++;
 			continue;
 		}
-		if (as_is) {
-			if (show_file(arg, output_prefix) && as_is < 2)
-				verify_filename(prefix, arg, 0);
-			continue;
-		}
 		if (!strcmp(arg,"-n")) {
 			if (++i >= argc)
 				die("-n requires an argument");
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index 3e657e693b..2ed5d50059 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -254,4 +254,13 @@  test_expect_success 'escaped char does not trigger wildcard rule' '
 	test_must_fail git rev-parse "foo\\*bar"
 '
 
+test_expect_success 'arg after dashdash not interpreted as option' '
+	cat >expect <<-\EOF &&
+	--
+	--local-env-vars
+	EOF
+	git rev-parse -- --local-env-vars >actual &&
+	test_cmp expect actual
+'
+
 test_done