[PATCH v2 1/3] rev-parse: correctly diagnose revision errors before "--"

[Jeff King <peff@peff.net>]

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 <command> [<revision>...] -- [<file>...]'

  $ >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 <peff@peff.net>
---
 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 1d9ecaf..bb694ab 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -488,6 +488,7 @@ N_("git rev-parse --parseopt [options] -- [<args>...]\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;
@@ -501,6 +502,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++) {
@@ -788,6 +796,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 f950c10..613d9bf 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
-- 
1.8.5.524.g6743da6