[BUG] redundant error message

[BUG] redundant error message

[Duy Nguyen]

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

Re: [BUG] redundant error message

[Jeff King]

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

Re: [BUG] redundant error message

[Junio C Hamano]

Jeff King <peff@peff.net> 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...

Re: [BUG] redundant error message

[Jeff King]

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

> Jeff King <peff@peff.net> 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

Re: [BUG] redundant error message

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Dec 05, 2013 at 12:00:16PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> 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?

Re: [BUG] redundant error message

[Jeff King]

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

Re: [BUG] redundant error message

[Jeff King]

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] -- [<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

Re: [BUG] redundant error message

[Junio C Hamano]

Jeff King <peff@peff.net> 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.

Re: [BUG] redundant error message

[Duy Nguyen]

On Fri, Dec 6, 2013 at 4:28 AM, Jeff King <peff@peff.net> 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 <command> [<revision>...] -- [<file>...]'
$ ./git grep -e -- -- foo
$
-- 
Duy

[PATCH 0/2] rev-parse and "--"

[Jeff King]

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

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

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

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

[Jeff King]

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

[PATCH 2/2] rev-parse: diagnose ambiguous revision/filename arguments

[Jeff King]

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

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index bb694ab..3ff4f8a 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -790,8 +790,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 		if (!get_sha1(name, sha1)) {
 			if (verify)
 				revs_count++;
-			else
+			else {
 				show_rev(type, sha1, name);
+				if (!has_dashdash)
+					verify_non_filename(prefix, arg);
+			}
 			continue;
 		}
 		if (verify)
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index 613d9bf..7ccbcf4 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -220,4 +220,11 @@ test_expect_success 'arg before dashdash must be a revision (ambiguous)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'ambiguous arg without dashdash' '
+	>foobar &&
+	git update-ref refs/heads/foobar HEAD &&
+	test_must_fail git rev-parse foobar 2>stderr &&
+	test_i18ngrep ambiguous stderr
+'
+
 test_done
-- 
1.8.5.524.g6743da6

[PATCH v2 0/3] rev-parse and "--"

[Jeff King]

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

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

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

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

[Jeff King]

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

[PATCH v2 2/3] rev-parse: be more careful with munging arguments

[Jeff King]

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

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index bb694ab..a60fcd3 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -286,6 +286,7 @@ static int try_difference(const char *arg)
 				exclude = n;
 			}
 		}
+		*dotdot = '.';
 		return 1;
 	}
 	*dotdot = '.';
@@ -309,8 +310,10 @@ static int try_parent_shorthands(const char *arg)
 		return 0;
 
 	*dotdot = 0;
-	if (get_sha1_committish(arg, sha1))
+	if (get_sha1_committish(arg, sha1)) {
+		*dotdot = '^';
 		return 0;
+	}
 
 	if (!parents_only)
 		show_rev(NORMAL, sha1, arg);
@@ -319,6 +322,7 @@ static int try_parent_shorthands(const char *arg)
 		show_rev(parents_only ? NORMAL : REVERSED,
 				parents->item->object.sha1, arg);
 
+	*dotdot = '^';
 	return 1;
 }
 
-- 
1.8.5.524.g6743da6

[PATCH v2 3/3] rev-parse: diagnose ambiguous revision/filename arguments

[Jeff King]

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

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index a60fcd3..a3a58bf 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -781,10 +781,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 		}
 
 		/* Not a flag argument */
-		if (try_difference(arg))
-			continue;
-		if (try_parent_shorthands(arg))
+		if (try_difference(arg) || try_parent_shorthands(arg)) {
+			if (!has_dashdash)
+				verify_non_filename(prefix, arg);
 			continue;
+		}
 		name = arg;
 		type = NORMAL;
 		if (*arg == '^') {
@@ -794,8 +795,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 		if (!get_sha1(name, sha1)) {
 			if (verify)
 				revs_count++;
-			else
+			else {
 				show_rev(type, sha1, name);
+				if (!has_dashdash)
+					verify_non_filename(prefix, arg);
+			}
 			continue;
 		}
 		if (verify)
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index 613d9bf..dc1f1dc 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -220,4 +220,26 @@ test_expect_success 'arg before dashdash must be a revision (ambiguous)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'ambiguous arg without dashdash (normal)' '
+	>foobar &&
+	git update-ref refs/heads/foobar HEAD &&
+	test_must_fail git rev-parse foobar 2>stderr &&
+	test_i18ngrep ambiguous stderr
+'
+
+test_expect_success 'ambiguous arg without dashdash (difference)' '
+	>one..two &&
+	git update-ref refs/heads/one HEAD &&
+	git update-ref refs/heads/two HEAD &&
+	test_must_fail git rev-parse one..two 2>stderr &&
+	test_i18ngrep ambiguous stderr
+'
+
+test_expect_success 'ambiguous arg without dashdash (parents)' '
+	>"foobar^@" &&
+	git update-ref refs/heads/foobar HEAD &&
+	test_must_fail git rev-parse foobar^@ 2>stderr &&
+	test_i18ngrep ambiguous stderr
+'
+
 test_done
-- 
1.8.5.524.g6743da6

Re: [BUG] redundant error message

[Jeff King]

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

> On Fri, Dec 6, 2013 at 4:28 AM, Jeff King <peff@peff.net> 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 <command> [<revision>...] -- [<file>...]'
> $ ./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

Re: [PATCH v2 0/3] rev-parse and "--"

[Jonathan Nieder]

Jeff King wrote:

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

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

Re: [PATCH v2 0/3] rev-parse and "--"

[Jeff King]

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

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

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

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

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

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

[Jonathan Nieder]

Jeff King wrote:

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

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

Re: [PATCH v2 2/3] rev-parse: be more careful with munging arguments

[Jonathan Nieder]

Jeff King wrote:

> As an aside, this is one of those places where C's string functions do
> gross things with const.

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.

diff --git i/builtin/rev-parse.c w/builtin/rev-parse.c
index c76b89d..98262b3 100644
--- i/builtin/rev-parse.c
+++ w/builtin/rev-parse.c
@@ -234,37 +234,35 @@ static int try_difference(const char *arg)
 	unsigned char sha1[20];
 	unsigned char end[20];
 	const char *next;
-	const char *this;
+	struct strbuf this = STRBUF_INIT;
 	int symmetric;
-	static const char head_by_default[] = "HEAD";
 
 	if (!(dotdot = strstr(arg, "..")))
 		return 0;
 	next = dotdot + 2;
-	this = arg;
 	symmetric = (*next == '.');
-
-	*dotdot = 0;
 	next += symmetric;
 
-	if (!*next)
-		next = head_by_default;
-	if (dotdot == arg)
-		this = head_by_default;
-
-	if (this == head_by_default && next == head_by_default &&
-	    !symmetric) {
+	if (!*next && dotdot == arg && !symmetric)
 		/*
 		 * Just ".."?  That is not a range but the
 		 * pathspec for the parent directory.
 		 */
-		*dotdot = '.';
 		return 0;
+
+	strbuf_add(&this, arg, dotdot - arg);
+
+	if (!*next)
+		next = "HEAD";
+	if (dotdot == arg) {
+		strbuf_reset(&this);
+		strbuf_addstr(&this, "HEAD");
 	}
 
-	if (!get_sha1_committish(this, sha1) && !get_sha1_committish(next, end)) {
+	if (!get_sha1_committish(this.buf, sha1)
+	    && !get_sha1_committish(next, end)) {
 		show_rev(NORMAL, end, next);
-		show_rev(symmetric ? NORMAL : REVERSED, sha1, this);
+		show_rev(symmetric ? NORMAL : REVERSED, sha1, this.buf);
 		if (symmetric) {
 			struct commit_list *exclude;
 			struct commit *a, *b;
@@ -279,9 +277,10 @@ static int try_difference(const char *arg)
 				exclude = n;
 			}
 		}
+		strbuf_release(&this);
 		return 1;
 	}
-	*dotdot = '.';
+	strbuf_release(&this);
 	return 0;
 }
 

Re: [PATCH v2 0/3] rev-parse and "--"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

Thanks.

Re: [PATCH v2 0/3] rev-parse and "--"

[Jonathan Nieder]

Junio C Hamano wrote:

>>                                      So maybe we are doing a favor by
>> calling out the problem; if they want a rev, they should be using
>> "--verify" (or "--").
>
> I tend to agree with the reasoning in the last sentence. Let's cook
> it for a while and see what happens.

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

Re: [PATCH v2 0/3] rev-parse and "--"

[Jonathan Nieder]

Jonathan Nieder wrote:

> Isn't this essentially breaking a contract

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

Re: [PATCH v2 0/3] rev-parse and "--"

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>
>>>                                      So maybe we are doing a favor by
>>> calling out the problem; if they want a rev, they should be using
>>> "--verify" (or "--").
>>
>> I tend to agree with the reasoning in the last sentence. Let's cook
>> it for a while and see what happens.
>
> 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,

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

Re: [PATCH v2 0/3] rev-parse and "--"

[Jonathan Nieder]

Junio C Hamano wrote:

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

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.

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

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

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

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

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

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

Re: [PATCH v2 0/3] rev-parse and "--"

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

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.

Re: [PATCH v2 2/3] rev-parse: be more careful with munging arguments

[Eric Sunshine]

On Fri, Dec 6, 2013 at 5:07 PM, Jeff King <peff@peff.net> wrote:
> 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

s/wouuld/would/

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