Re: [PATCH v2 1/3] rev-parse: correctly diagnose revision errors before "--"
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Dec 6, 2013, 23:34 UTC
- Message-ID
- <20131206233410.GO29959@google.com>
- In-Reply-To
- <20131206220547.GA25620@sigill.intra.peff.net>
Jeff King wrote:
Show 18 quoted lines
> 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.
Neat.
[...]
Show 31 quoted lines
> --- 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;Yep, this is the "fall back to looking for a file" part of rev-parse, so erroring out if there as a dashdash coming is the right thing to do. And a quick code search for "rev-parse.*\ --\ " reveals that most callers would simply not be affected by this.
Thanks for a pleasant patch.
For what it's worth, Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>