From 141856738152d02beac5d4270a310a6007597282 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 6 Dec 2013 17:05:48 -0500 Subject: [PATCH 1/2] rev-parse: correctly diagnose revision errors before "--" Rev-parse understands that a "--" may separate revisions and filenames, and that anything after the "--" is taken as-is. However, it does not understand that anything before the token must be a revision (which is the usual rule implemented by the setup_revisions parser). Since rev-parse prefers revisions to files when parsing before the "--", we end up with the correct result (if such an argument is a revision, we parse it as one, and if it is not, it is an error either way). However, we misdiagnose the errors: $ git rev-parse foobar -- >/dev/null fatal: ambiguous argument 'foobar': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git [...] -- [...]' $ >foobar $ git rev-parse foobar -- >/dev/null fatal: bad flag '--' used after filename In both cases, we should know that the real error is that "foobar" is meant to be a revision, but could not be resolved. Signed-off-by: Jeff King Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- builtin/rev-parse.c | 10 ++++++++++ t/t1506-rev-parse-diagnosis.sh | 24 ++++++++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c index c76b89dc5b..8be641e077 100644 --- a/builtin/rev-parse.c +++ b/builtin/rev-parse.c @@ -476,6 +476,7 @@ N_("git rev-parse --parseopt [options] -- [...]\n" int cmd_rev_parse(int argc, const char **argv, const char *prefix) { int i, as_is = 0, verify = 0, quiet = 0, revs_count = 0, type = 0; + int has_dashdash = 0; int output_prefix = 0; unsigned char sha1[20]; const char *name = NULL; @@ -489,6 +490,13 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix) if (argc > 1 && !strcmp("-h", argv[1])) usage(builtin_rev_parse_usage); + for (i = 1; i < argc; i++) { + if (!strcmp(argv[i], "--")) { + has_dashdash = 1; + break; + } + } + prefix = setup_git_directory(); git_config(git_default_config, NULL); for (i = 1; i < argc; i++) { @@ -765,6 +773,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix) } if (verify) die_no_single_rev(quiet); + if (has_dashdash) + die("bad revision '%s'", arg); as_is = 1; if (!show_file(arg, output_prefix)) continue; diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh index f950c10128..613d9bfe1b 100755 --- a/t/t1506-rev-parse-diagnosis.sh +++ b/t/t1506-rev-parse-diagnosis.sh @@ -196,4 +196,28 @@ test_expect_success 'dotdot is not an empty set' ' test_cmp expect actual ' +test_expect_success 'arg before dashdash must be a revision (missing)' ' + test_must_fail git rev-parse foobar -- 2>stderr && + test_i18ngrep "bad revision" stderr +' + +test_expect_success 'arg before dashdash must be a revision (file)' ' + >foobar && + test_must_fail git rev-parse foobar -- 2>stderr && + test_i18ngrep "bad revision" stderr +' + +test_expect_success 'arg before dashdash must be a revision (ambiguous)' ' + >foobar && + git update-ref refs/heads/foobar HEAD && + { + # we do not want to use rev-parse here, because + # we are testing it + cat .git/refs/heads/foobar && + printf "%s\n" -- + } >expect && + git rev-parse foobar -- >actual && + test_cmp expect actual +' + test_done From 62f162f8e7c28cd60f0c18f5e69933efd72659e0 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 6 Dec 2013 17:07:52 -0500 Subject: [PATCH 2/2] rev-parse: be more careful with munging arguments When rev-parse looks at whether an argument like "foo..bar" or "foobar^@" is a difference or parent-shorthand, it internally munges the arguments so that it can pass the individual rev arguments to get_sha1(). However, we do not consistently un-munge the result. For cases where we do not match (e.g., "doesnotexist..HEAD"), we would then want to try to treat the argument as a filename. try_difference gets() this right, and always unmunges in this case. However, try_parent_shorthand() never unmunges, leading to incorrect error messages, or even incorrect results: $ git rev-parse foobar^@ foobar fatal: ambiguous argument 'foobar': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git [...] -- [...]' $ >foobar $ git rev-parse foobar^@ foobar For cases where we do match, neither function unmunges. This does not currently matter, since we are done with the argument. However, a future patch will do further processing, and this prepares for it. In addition, it's simply a confusing interface for some cases to modify the const argument, and others not to. Signed-off-by: Jeff King Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- builtin/rev-parse.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c index 8be641e077..c4b768ffda 100644 --- a/builtin/rev-parse.c +++ b/builtin/rev-parse.c @@ -279,6 +279,7 @@ static int try_difference(const char *arg) exclude = n; } } + *dotdot = '.'; return 1; } *dotdot = '.'; @@ -302,8 +303,10 @@ static int try_parent_shorthands(const char *arg) return 0; *dotdot = 0; - if (get_sha1_committish(arg, sha1)) + if (get_sha1_committish(arg, sha1)) { + *dotdot = '^'; return 0; + } if (!parents_only) show_rev(NORMAL, sha1, arg); @@ -312,6 +315,7 @@ static int try_parent_shorthands(const char *arg) show_rev(parents_only ? NORMAL : REVERSED, parents->item->object.sha1, arg); + *dotdot = '^'; return 1; }