Re: [BUG] redundant error message

From

Jeff King <peff@peff.net>

Date

Dec 5, 2013, 21:28 UTC

Message-ID

<20131205212851.GA21776@sigill.intra.peff.net>

In-Reply-To

<20131205210000.GA19617@sigill.intra.peff.net>

On Thu, Dec 05, 2013 at 04:00:00PM -0500, Jeff King wrote:

> Yes, I do expect an error. But it should not be "-- after filename". It
> should be "foobar is not a revision".
> [...]
> It would be nice to get the error messages right, though. I do not see
> any reason why it could not follow the same steps as "git log",
> converting revisions (or throwing an error as appropriate) on the left
> side of the "--", and passing through the right side untouched.

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index c76b89d..845eab9 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -476,6 +476,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;
@@ -489,6 +490,14 @@ 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 +774,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;


BTW, the raw looping to find "--" made me wonder how we handle:

  git log --grep -- HEAD

I'd expect it to be equivalent to:

  git log --grep=-- HEAD

but it's not; we truncate the arguments and complain that --grep is
missing its argument. Which is probably good enough, given that the
alternative is doing a pass that understands all of the options. But it
does mean that the "--long-opt=arg" form is safer than the split form if
you are passing along an arbitrary "arg".

-Peff