Re: [BUG] redundant error message
On Fri, Dec 06, 2013 at 08:15:52AM +0700, Duy Nguyen wrote: > On Fri, Dec 6, 2013 at 4:28 AM, Jeff King wrote: > > 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". > > Maybe we could make setup_revisions() use parse_options() someday, > which understands about arguments and dashdash. > > $ ./git grep -e -- foo > fatal: ambiguous argument 'foo': both revision and filename > Use '--' to separate paths from revisions, like this: > 'git [...] -- [...]' > $ ./git grep -e -- -- foo Yes, although we use it some in handle_revision_opt, I believe. The problem isn't inherent to parse_options or not, though. To do it correctly, we need to either: 1. make two passes with the code that actually understands the options (be it parse_options or not); the first looking for "--", and the second to do the actual parsing. Right now our first pass does not understand the options at all. 2. store the non-option arguments (including "--"), and only resolve and verify them after we have gone through the whole command-line and know whether we hit a "--" or not. I suspect the second option would be simpler, as neither parse-options nor the current revision code is safe to run through twice (e.g., parse-options may have callbacks that add to a list, and we would need to add some kind of "dry-run" flag). It's something that would be nice to fix, but I don't see myself working on it anytime soon. It's a lot of work for very little benefit. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] redundant error message
On Fri, Dec 6, 2013 at 4:28 AM, Jeff King wrote: > 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". Maybe we could make setup_revisions() use parse_options() someday, which understands about arguments and dashdash. $ ./git grep -e -- foo fatal: ambiguous argument 'foo': both revision and filename Use '--' to separate paths from revisions, like this: 'git [...] -- [...]' $ ./git grep -e -- -- foo $ -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] redundant error message
Jeff King writes: > 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". ;-) Good flow of thought. As to your rev-parse change, I don't immediately think of a hole/flaw offhand; it looked a good straight-forward change to me. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] redundant error message
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.
IOW, the patch below, which is the same strategy that setup_revisions
uses:
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] -- [...]\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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] redundant error message
On Thu, Dec 05, 2013 at 12:15:07PM -0800, Junio C Hamano wrote: > >> Hmph, it looks like it is following the usual "zero-or-more dashed > >> options, zero-or-more revs and then zero-or-one double-dash and then > >> zero-or-more paths" rule to parse the thing. "foobar" is a file and > >> not a rev, so "--" should not be there, no? > >> > >> Confused why you think it is not right... > > > > Because once you say "--", then all ambiguity goes away, no? > > But it is tricky (not from implementation but from semantics point > of view) to make rev-parse follow that "-- separates revs and paths" > rule literally. The primary use of rev-parse is to convert revs in > extended SHA-1 expressions into concrete object names, so that > scripts do not have to worry about having to deal with object names > in a format that is not 40-hexdecimal. "git rev-parse foobar --" > that gives > > foobar > -- > > without any error, because 'foobar' cannot be made into an object > name, would be behaving in a way unexpected by the calling script, > no? Yes, I do expect an error. But it should not be "-- after filename". It should be "foobar is not a revision". Thinking on it more, though, the problem is purely limited to wrong error messages. If "foobar" exists as a rev, we do parse it correctly. If it does not, we are in the wrong code path, but it _must_ be an error at that point (either because foobar does not exist as a file, or it does and has "--" after it). 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. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] redundant error message
Jeff King writes: > On Thu, Dec 05, 2013 at 12:00:16PM -0800, Junio C Hamano wrote: > >> Jeff King writes: >> >> > $ >foobar >> > $ git rev-parse foobar -- >> > foobar >> > -- >> > fatal: bad flag '--' used after filename >> > >> > That's not right. >> >> Hmph, it looks like it is following the usual "zero-or-more dashed >> options, zero-or-more revs and then zero-or-one double-dash and then >> zero-or-more paths" rule to parse the thing. "foobar" is a file and >> not a rev, so "--" should not be there, no? >> >> Confused why you think it is not right... > > Because once you say "--", then all ambiguity goes away, no? But it is tricky (not from implementation but from semantics point of view) to make rev-parse follow that "-- separates revs and paths" rule literally. The primary use of rev-parse is to convert revs in extended SHA-1 expressions into concrete object names, so that scripts do not have to worry about having to deal with object names in a format that is not 40-hexdecimal. "git rev-parse foobar --" that gives foobar -- without any error, because 'foobar' cannot be made into an object name, would be behaving in a way unexpected by the calling script, no? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] redundant error message
On Thu, Dec 05, 2013 at 12:00:16PM -0800, Junio C Hamano wrote: > Jeff King writes: > > > $ >foobar > > $ git rev-parse foobar -- > > foobar > > -- > > fatal: bad flag '--' used after filename > > > > That's not right. > > Hmph, it looks like it is following the usual "zero-or-more dashed > options, zero-or-more revs and then zero-or-one double-dash and then > zero-or-more paths" rule to parse the thing. "foobar" is a file and > not a rev, so "--" should not be there, no? > > Confused why you think it is not right... Because once you say "--", then all ambiguity goes away, no? Everything to the left is a rev, not a filename, and everything to the right is a filename. E.g.: $ git log foobar -- fatal: bad revision 'foobar' I think rev-parse is not following the same rules that the rest of the revision-parsing programs do. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] redundant error message
Jeff King writes: > $ >foobar > $ git rev-parse foobar -- > foobar > -- > fatal: bad flag '--' used after filename > > That's not right. Hmph, it looks like it is following the usual "zero-or-more dashed options, zero-or-more revs and then zero-or-one double-dash and then zero-or-more paths" rule to parse the thing. "foobar" is a file and not a rev, so "--" should not be there, no? Confused why you think it is not right... -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] redundant error message
On Thu, Dec 05, 2013 at 05:07:31PM +0700, Duy Nguyen wrote: > $ 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 [...] -- [...]' > > I already put "--" there. So it should shut up. I think it is more than just a bad error message; if we get that message, it means we are following the wrong code path in interpreting "foobar": $ >foobar $ git rev-parse foobar -- foobar -- fatal: bad flag '--' used after filename That's not right. It looks like this isn't a new breakage, though. v1.5.0 produces the same result. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
