[BUG] redundant error message

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

I already put "--" there. So it should shut up.

On Thu, Dec 05, 2013 at 05:07:31PM +0700, Duy Nguyen wrote:

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

Jeff King <peff@peff.net> writes:

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...

On Thu, Dec 05, 2013 at 12:00:16PM -0800, Junio C Hamano wrote:

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

Jeff King <peff@peff.net> writes:

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?

On Thu, Dec 05, 2013 at 12:15:07PM -0800, Junio C Hamano wrote:

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

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

IOW, the patch below, which is the same strategy that setup_revisions uses:

Jeff King <peff@peff.net> writes:

;-) 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.

On Thu, Dec 05, 2013 at 01:44:12PM -0800, Junio C Hamano wrote:

Here it is with tests and a commit message. While writing the tests, I noticed that also do not handle true ambiguity the same (we prefer revs, rather than complaining). That's fixed in the second commit.

  [1/2]: rev-parse: correctly diagnose revision errors before "--"
  [2/2]: rev-parse: diagnose ambiguous revision/filename arguments

-Peff

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(+)

If you have both a file and a branch named "foo", running:

  git log foo

will complain. We should do the same in rev-parse, and demand that it be disambiguated with:

  git rev-parse foo --

or

  git rev-parse -- foo

Signed-off-by: Jeff King <peff@peff.net>
---
Hmm, looking at this again, I guess we need to give the same treatment
to the try_difference code path above, as "foo..bar" can be ambiguous
with a filename (and "git log" seems to diagnose that).

 builtin/rev-parse.c            | 5 ++++-
 t/t1506-rev-parse-diagnosis.sh | 7 +++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

On Fri, Dec 06, 2013 at 04:15:09PM -0500, Jeff King wrote:

Here's a revised series which handles that:

  [1/3]: rev-parse: correctly diagnose revision errors before "--"
  [2/3]: rev-parse: be more careful with munging arguments
  [3/3]: rev-parse: diagnose ambiguous revision/filename arguments

While fixing this commit, I noticed another bug whose fix is a prerequisite. That's now patch 2 in the series. I think the first two patches are correct and should be applied.

Patch 3 is the revised version of this patch which notices ambiguity. However, I'm having second thoughts on it. I think it's the right thing to do if you want to help people build something like "git log" themselves. But it does mean that we are breaking somebody who does:

  echo foo >HEAD
  commit=$(git rev-parse HEAD)

I'm tempted to say that people who did that are stupid and wrong (and ugly, too). They should probably be using "--verify" in this case. But it has been that way for a long time, and there are two instances in our test scripts that are broken by the patch.

Is it better for "rev-parse" to be more careful, and to behave more like the rest of git? Or is better to be historically compatible?

One thing to note is that "git rev-parse HEAD" is slightly broken there already. Because "git rev-parse $some_branch" may do very different things than what the caller expects if $some_branch does not exist (but there is a file with the same name). So maybe we are doing a favor by calling out the problem; if they want a rev, they should be using "--verify" (or "--").

-Peff

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(+)

Jeff King wrote:

Neat.

[...]

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>

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

  $ >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 <peff@peff.net>
---
As an aside, this is one of those places where C's string functions do
gross things with const. We modify a const argument to the function
without any casts or warnings, because the return value from strstr
implicitly drops the const. Time for us all to move to C++ with
templated strstr, right?

 builtin/rev-parse.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Jeff King wrote:

Yes, yuck.

The fundamental grossness is that argv is semantically char ** (assuming this doesn't segfault) but passed around as const char **. I wonder why we don't use the same trick in git_extract_argv0_path and when handling STRIP_EXTENSION in git.c.

Alternatively, try_difference could create a new buffer for its own use. Originally (v0.99~273, 2005-06-13) this dotdot handling code was pretty simple:

	for (i = 1; i < argc; i++) {
		char *arg = argv[i];

		...

		dotdot = strstr(arg, "..");
		if (dotdot) {
			unsigned char end[20];
			char *n = dotdot+2;
			*dotdot = 0;
			if (!get_sha1(arg, sha1)) {
				if (!*n)
					n = "HEAD";
				if (!get_sha1(n, end)) {
					printf("%s\n", sha1_to_hex(end));
					printf("^%s\n", sha1_to_hex(sha1));
					continue;
				}
			}
			*dotdot = '.';
		}
		printf("%s\n", arg);
	}

The 'continue' without making the argument right again was justified because we were done with the argument.

Now it is still not much more complicated --- the function is logically just trying to parse the argument into a "this" and "next" part and print them.

Something like the following (untested). Or if the allocation churn is too much, it could use a static strbuf.

On Fri, Dec 6, 2013 at 5:07 PM, Jeff King <peff@peff.net> wrote:

s/wouuld/would/

If you have both a file and a branch named "foo", running:

  git log foo

will complain. We should do the same in rev-parse, and demand that it be disambiguated with:

  git rev-parse foo --

or

  git rev-parse -- foo

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/rev-parse.c            | 12 ++++++++----
 t/t1506-rev-parse-diagnosis.sh | 22 ++++++++++++++++++++++
 2 files changed, 30 insertions(+), 4 deletions(-)

Jeff King wrote:

Wouldn't the same thing happen for $(git rev-parse HEAD^..HEAD) when there is a 'HEAD^..HEAD' file, too? --verify doesn't support that.

I think it's a nice idea to provide this functionality for people who need it, which would mean protected by an option (--check-ambiguity or something). I don't think it's a good change to do it unconditionally.

My two cents, Jonathan

On Fri, Dec 06, 2013 at 03:25:56PM -0800, Jonathan Nieder wrote:

True, but they can use "rev-parse HEAD^..HEAD --", which is probably a better alternative to --verify.

I don't think it's worth making it conditional. Literally nobody has asked for it. I was mostly trying to harmonize "rev-parse" and the rest of the revision parser. If that's not worth doing, I'd just as soon drop it (and people using "--" properly already get the right behavior).

-Peff

Jeff King <peff@peff.net> writes:

I tend to agree with the reasoning in the last sentence. Let's cook it for a while and see what happens.

Thanks.

Junio C Hamano wrote:

Isn't this essentially breaking a contract that would have been relied on by any script that used "git rev-parse HEAD~3..HEAD"? Worse, it's breaking that contract in a way that no one would notice until they are asked to manipulate a worktree with a file named 'HEAD~3..HEAD' --- in other words, the breakage it introduces is painfully subtle.

I agree that "git rev-parse HEAD" is better written as "git rev-parse --verify HEAD" and hence not so much worth worrying about, but I don't find it easy to believe that people should have anticipated this change and added a trailing "--" to more complex rev-parse expressions.

So to be clear, I think unless it is protected by a new option, this is a bad idea.

Hope that helps, Jonathan

Jonathan Nieder wrote:

To clarify, if there were some strong motivation --- e.g. if this were resolving some security problem --- then I would be all for breaking compatibility in this way. What's confusing to me is that I just don't see the motivation. Why wouldn't someone that wants this functionality be able to pass a flag for it?

Is the idea that without a flag it's easier to teach or something?

Hoping that clarifies, Jonathan

Jonathan Nieder <jrnieder@gmail.com> writes:

I do not share the "with --verify is better hence no problem" reasoning. My "not so much worth worrying about" comes solely from a hunch that nobody has "HEAD~3..HEAD" in their working directory, and if somebody has one, then they must be using "--verify" (or a clarifying "--"), because their "git log" and whatever they use "git rev-parse HEAD~3..HEAD" for would behave very differently otherwise.

So it is not merely "--verify is better"---in a situation where the backward incompatibility matters, I doubt the existing behaves sensibly in the first place.

But if we cook it for a while, I suspect that we will find more and more breakages of expectations in the existing scripts in and out of the tree; in general, I hate _any_ change, and I do not mind dropping it ;-).

Junio C Hamano wrote:

That's what makes it a problem. This change makes it very easy to make a general-purpose script that breaks in an edge case that the script's author is not likely to run into. Then as soon as someone adds a file with such a name to the test data in their repo, their favorite general-purpose repo munger just breaks.

If we wanted to forbid git from tracking files named HEAD~3..HEAD altogether, that would be a different story.

Isn't protecting against this kind of thing the reason we ask authors of general-purpose scripts to use "simple, do what I say and not what I mean" plumbing commands?

Another relevant detail is that using rev-parse with "--" is more painful than without, since it includes the "--" in its output. Without this change, it seems much more likely to me that someone would do

	git rev-parse <commits> |
	while read commit
	do
		...
	done

than

	git rev-parse <commits> -- |
	while read commit
	do
		if test "$commit" = "--"
		then
			continue
		fi

		...
	done

What in the former of the above two loops is broken?

Alas, probably no, because nobody has "HEAD~3..HEAD" in their working directory. That's exactly the problem --- it creates an edge case that nobody is likely to test until the once-in-a-few-years moment when they need it.

Jonathan

Jonathan Nieder <jrnieder@gmail.com> writes:

I didn't mean to say that running and encoutering the issue is the only way to find "more breakges of expectations", by the way. I am still on the fence, but leaning towards not merging it too hastily.

On Fri, Dec 6, 2013 at 4:28 AM, Jeff King <peff@peff.net> wrote:

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 <command> [<revision>...] -- [<file>...]' $ ./git grep -e -- -- foo $

On Fri, Dec 06, 2013 at 08:15:52AM +0700, Duy Nguyen wrote:

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