"Detailed diagnosis" perhaps broken

"Detailed diagnosis" perhaps broken

[Junio C Hamano]

009fee4 (Detailed diagnosis when parsing an object name fails., 2009-12-07)
added a logic to try to see what kind of breakage the object name
user gave in the $treeish:$path or /:$search syntax.

This logic however triggers in a funny way, leading to an idiotic
error message.  You can try this in the git repository:

	$ git log COPYING HEAD^:COPYING
	fatal: Path 'COPYING' exists on disk, but not in 'HEAD^'.

When setup_revisions() tries to parse the command line arguments, it
notices that there is no "--", and it makes sure that earlier
arguments are all revs that cannot possibly refer to working tree
files, and later arguments all refer to working tree files and
cannot possibly refer to objects.

It first looks at "COPYING", notices that it is _not_ a rev.  Which
means that anything that follows must _not_ be an object name.  It
calls verify_filename() on "COPYING", which is a happy filename, and
then calls verify_filename() on "HEAD^:COPYING", and then calls
die_verify_filename() on it, assuming that it is being fed a badly
formed rev.

Obviously, this assumption is *WRONG*.

Then get_sha1_with_mode_1() is called with "gently" set to 0 (in
later code, we have a fix to flip the value of this flag and renamed
it to "only-to-die"---we are calling this function only to let it die
with diagnostics).  It tries to interpret HEAD^:COPYING as a rev,
and even though get_tree_entry() call for "COPYING" inside "HEAD^"
tree-ish succeeds, it ignores the fact that it successfully names an
object, and calls diagnose_invalid_sha1_path() to die.

	Side note.  The original motivation of the "detailed diag"
	patch was to catch something like

	$ git rev-parse HEAD^:COPYIGN

	and the codepath is called when the caller _knows_
	HEAD^:COPYIGN must resolve to an object name, and made sure
	that it does not, so in that case, it results in expected
	behaviour.  But for this "earlier must be all revs, later
	must be all paths" caller, it was a wrong conversion.

That causes "Path exists on disk, but not in 'HEAD^'", which is only
half correct (because the caller just checked and it knows the path
exists in 'HEAD^', but it ignored the result of the check), and more
importantly, completely misses the point.

At least, the following patch seems to work around _this_ particular
codepath, but I suspect that we need to check other places that can
reach diagnose_invalid_foo() functions your commit introduced for
similar breakages.

Ideas for better fix?

 sha1_name.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sha1_name.c b/sha1_name.c
index c633113..574c68a 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1127,7 +1127,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
 			if (new_filename)
 				filename = new_filename;
 			ret = get_tree_entry(tree_sha1, filename, sha1, &oc->mode);
-			if (only_to_die) {
+			if (only_to_die && ret) {
 				diagnose_invalid_sha1_path(prefix, filename,
 							   tree_sha1, object_name);
 				free(object_name);

Re: "Detailed diagnosis" perhaps broken

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> 	$ git log COPYING HEAD^:COPYING
> 	fatal: Path 'COPYING' exists on disk, but not in 'HEAD^'.

Oops!

> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -1127,7 +1127,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
>  			if (new_filename)
>  				filename = new_filename;
>  			ret = get_tree_entry(tree_sha1, filename, sha1, &oc->mode);
> -			if (only_to_die) {
> +			if (only_to_die && ret) {
>  				diagnose_invalid_sha1_path(prefix, filename,
>  							   tree_sha1, object_name);
>  				free(object_name);

This is one obvious thing to do. We should never call
diagnose_invalid_sha1_path if the search done by get_tree_entry
succeeded. A patch follows with a proper test-case.

But that isn't sufficient unfortunately. The other question here is: why
did we even try calling get_tree_entry, if we're not looking for an
object at all? Indeed, if get_tree_entry fails, we get:

  $ git log COPYING HEAD:foo
  fatal: Path 'foo' does not exist in 'HEAD'

At least, the message is correct in that foo does not exist in HEAD, but
not accurate in the sense that it is not the reason for the error.

So, the other fix should be to distinguish from the caller of
verify_filename whether we should try the detailed diagnosis including
sha1_name_*, or just die and complain about path not being in the
working tree. Another patch follows doing that.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH 1/2] sha1_name: don't trigger detailed diagnosis for file arguments

[Matthieu Moy]

diagnose_invalid_sha1_path is normally meant to be called to diagnose
<treeish>:<pathname> when <pathname> does not exist in <treeish>.
However, the current code may call it if <treeish>:<pathname> is invalid
(which triggers another call with only_to_die == 1), but for another
reason. This happens when calling e.g.

  git log existing-file HEAD:existing-file

(because existing-file is a file and not a revision, the next arguments
are assumed to be files too), leading to incorrect message like
"existing-file does not exist in HEAD".

Check that the search for <pathname> in <treeish> fails before triggering
the diagnosis.

Bug report and code fix by: Junio C Hamano <gitster@pobox.com>
Test by: Matthieu Moy <Matthieu.Moy@imag.fr>

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---

This patch is very simple and should be rather uncontroversial.

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

diff --git a/sha1_name.c b/sha1_name.c
index c633113..5d81ea0 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1127,7 +1127,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
 			if (new_filename)
 				filename = new_filename;
 			ret = get_tree_entry(tree_sha1, filename, sha1, &oc->mode);
-			if (only_to_die) {
+			if (ret && only_to_die) {
 				diagnose_invalid_sha1_path(prefix, filename,
 							   tree_sha1, object_name);
 				free(object_name);
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index 0843a1c..4a39ac5 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -171,4 +171,15 @@ test_expect_success 'relative path when startup_info is NULL' '
 	grep "BUG: startup_info struct is not initialized." error
 '
 
+test_expect_success '<commit>:file correctly diagnosed after a pathname' '
+	test_must_fail git rev-parse file.txt HEAD:file.txt 1>actual 2>error &&
+	test_i18ngrep ! "exists on disk" error &&
+	test_i18ngrep "unknown revision or path not in the working tree" error &&
+	cat >expect <<EOF &&
+file.txt
+HEAD:file.txt
+EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
1.7.11.rc0.57.g84a04c7

[PATCH 2/2 RFC] verify_filename: ask the caller to chose the kind of diagnosis

[Matthieu Moy]

verify_filename can be called in two different contexts. Either we just
tried to interpret a string as an object name, and it fails, so we try
looking for a working tree file as a fallback, or we _know_ that we are
looking for a filename, and shouldn't even try interpreting the string as
an object name.

For example, with this change, we get:

  $ git log COPYING HEAD:inexistant
  fatal: HEAD:inexistant: no such path in the working tree.
  $ git log HEAD:inexistant
  fatal: Path 'inexistant' does not exist in 'HEAD'

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---

This one is much less straightforward, hence the RFC. I did check all
the call sites of verify_filename, but I may not have fully understood
the logic at each call site.

A mistake in the diagnose_rev argument is not _that_ serious however,
since it does not change the potential failures, but only the error
message.

 builtin/grep.c                 | 7 +++++--
 builtin/reset.c                | 2 +-
 builtin/rev-parse.c            | 4 ++--
 cache.h                        | 9 ++++++++-
 revision.c                     | 5 +++--
 setup.c                        | 8 +++++---
 t/t1506-rev-parse-diagnosis.sh | 2 +-
 7 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index fe1726f..41924dc 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -927,8 +927,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	/* The rest are paths */
 	if (!seen_dashdash) {
 		int j;
-		for (j = i; j < argc; j++)
-			verify_filename(prefix, argv[j]);
+		if (i < argc) {
+			verify_filename(prefix, argv[i], 1);
+			for (j = i + 1; j < argc; j++)
+				verify_filename(prefix, argv[j], 0);
+		}
 	}
 
 	paths = get_pathspec(prefix, argv + i);
diff --git a/builtin/reset.c b/builtin/reset.c
index 8c2c1d5..4cc34c9 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -285,7 +285,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 			rev = argv[i++];
 		} else {
 			/* Otherwise we treat this as a filename */
-			verify_filename(prefix, argv[i]);
+			verify_filename(prefix, argv[i], 1);
 		}
 	}
 
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 733f626..13495b8 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -486,7 +486,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 
 		if (as_is) {
 			if (show_file(arg) && as_is < 2)
-				verify_filename(prefix, arg);
+				verify_filename(prefix, arg, 0);
 			continue;
 		}
 		if (!strcmp(arg,"-n")) {
@@ -734,7 +734,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 		as_is = 1;
 		if (!show_file(arg))
 			continue;
-		verify_filename(prefix, arg);
+		verify_filename(prefix, arg, 1);
 	}
 	if (verify) {
 		if (revs_count == 1) {
diff --git a/cache.h b/cache.h
index 06413e1..d1a4d9e 100644
--- a/cache.h
+++ b/cache.h
@@ -409,7 +409,14 @@ extern const char *setup_git_directory(void);
 extern char *prefix_path(const char *prefix, int len, const char *path);
 extern const char *prefix_filename(const char *prefix, int len, const char *path);
 extern int check_filename(const char *prefix, const char *name);
-extern void verify_filename(const char *prefix, const char *name);
+/*
+ * Verify that "name" is a filename.
+ * The "diagnose_rev" is used to provide a user-friendly diagnosis. If
+ * 0, the diagnosis will try to diagnose "name" as an invalid object
+ * name (e.g. HEAD:foo). If non-zero, the diagnosis will only complain
+ * about an inexisting file.
+ */
+extern void verify_filename(const char *prefix, const char *name, int diagnose_rev);
 extern void verify_non_filename(const char *prefix, const char *name);
 
 #define INIT_DB_QUIET 0x0001
diff --git a/revision.c b/revision.c
index 935e7a7..756196a 100644
--- a/revision.c
+++ b/revision.c
@@ -1780,8 +1780,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			 *     as a valid filename.
 			 * but the latter we have checked in the main loop.
 			 */
-			for (j = i; j < argc; j++)
-				verify_filename(revs->prefix, argv[j]);
+			verify_filename(revs->prefix, arg, 1);			
+			for (j = i + 1; j < argc; j++)
+				verify_filename(revs->prefix, argv[j], 0);
 
 			append_prune_data(&prune_data, argv + i);
 			break;
diff --git a/setup.c b/setup.c
index 731851a..ca33092 100644
--- a/setup.c
+++ b/setup.c
@@ -53,11 +53,13 @@ int check_filename(const char *prefix, const char *arg)
 	die_errno("failed to stat '%s'", arg);
 }
 
-static void NORETURN die_verify_filename(const char *prefix, const char *arg)
+static void NORETURN die_verify_filename(const char *prefix, const char *arg, int diagnose_rev)
 {
 	unsigned char sha1[20];
 	unsigned mode;
 
+	if (!diagnose_rev)
+		die("%s: no such path in the working tree.", arg);
 	/*
 	 * Saying "'(icase)foo' does not exist in the index" when the
 	 * user gave us ":(icase)foo" is just stupid.  A magic pathspec
@@ -81,13 +83,13 @@ static void NORETURN die_verify_filename(const char *prefix, const char *arg)
  * it to be preceded by the "--" marker (or we want the user to
  * use a format like "./-filename")
  */
-void verify_filename(const char *prefix, const char *arg)
+void verify_filename(const char *prefix, const char *arg, int diagnose_rev)
 {
 	if (*arg == '-')
 		die("bad flag '%s' used after filename", arg);
 	if (check_filename(prefix, arg))
 		return;
-	die_verify_filename(prefix, arg);
+	die_verify_filename(prefix, arg, diagnose_rev);
 }
 
 /*
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index 4a39ac5..a3f11e8 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -174,7 +174,7 @@ test_expect_success 'relative path when startup_info is NULL' '
 test_expect_success '<commit>:file correctly diagnosed after a pathname' '
 	test_must_fail git rev-parse file.txt HEAD:file.txt 1>actual 2>error &&
 	test_i18ngrep ! "exists on disk" error &&
-	test_i18ngrep "unknown revision or path not in the working tree" error &&
+	test_i18ngrep "no such path in the working tree" error &&
 	cat >expect <<EOF &&
 file.txt
 HEAD:file.txt
-- 
1.7.11.rc0.57.g84a04c7

Re: [PATCH 2/2 RFC] verify_filename: ask the caller to chose the kind of diagnosis

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> +/*
> + * Verify that "name" is a filename.
> + * The "diagnose_rev" is used to provide a user-friendly diagnosis. If
> + * 0, the diagnosis will try to diagnose "name" as an invalid object
> + * name (e.g. HEAD:foo). If non-zero, the diagnosis will only complain
> + * about an inexisting file.
> + */
> +extern void verify_filename(const char *prefix, const char *name, int diagnose_rev);

The whole point of verify_filename() is to make sure, because the
user did not have disambiguating "--" on the command line, that the
first non-rev argument is a path and also it cannot be interpreted
as a valid rev.  It somehow feels wrong to make it also responsible,
for a possibly misspelled rev.  The caller can mistakenly throw 0 or
1 at random but the _only_ right value for this parameter is to set
it to true only for the first non-rev, no?

Let's look at the patched sites.

> diff --git a/builtin/grep.c b/builtin/grep.c
> index fe1726f..41924dc 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -927,8 +927,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  	/* The rest are paths */
>  	if (!seen_dashdash) {
>  		int j;
> -		for (j = i; j < argc; j++)
> -			verify_filename(prefix, argv[j]);
> +		if (i < argc) {
> +			verify_filename(prefix, argv[i], 1);
> +			for (j = i + 1; j < argc; j++)
> +				verify_filename(prefix, argv[j], 0);
> +		}

This is exactly

	verify_filename(prefix, argv[j], j == first_non_rev)

> diff --git a/builtin/reset.c b/builtin/reset.c
> index 8c2c1d5..4cc34c9 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -285,7 +285,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>  			rev = argv[i++];
>  		} else {
>  			/* Otherwise we treat this as a filename */
> -			verify_filename(prefix, argv[i]);
> +			verify_filename(prefix, argv[i], 1);

This is also checking the first non-rev, too.  We just saw
"florbl^{triee}" in "git reset florbl^{triee} hello.c" is not a
valid rev.  If "florbl^{triee}" is indeed a file, we shouldn't
complain and die with "This may be a misspelled rev", but take it as
a path.

> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index 733f626..13495b8 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -486,7 +486,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>  
>  		if (as_is) {
>  			if (show_file(arg) && as_is < 2)
> -				verify_filename(prefix, arg);
> +				verify_filename(prefix, arg, 0);
>  			continue;
>  		}
>  		if (!strcmp(arg,"-n")) {
> @@ -734,7 +734,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>  		as_is = 1;
>  		if (!show_file(arg))
>  			continue;
> -		verify_filename(prefix, arg);
> +		verify_filename(prefix, arg, 1);
>  	}

These are, too.

> diff --git a/revision.c b/revision.c
> index 935e7a7..756196a 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1780,8 +1780,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  			 *     as a valid filename.
>  			 * but the latter we have checked in the main loop.
>  			 */
> -			for (j = i; j < argc; j++)
> -				verify_filename(revs->prefix, argv[j]);
> +			verify_filename(revs->prefix, arg, 1);			
> +			for (j = i + 1; j < argc; j++)
> +				verify_filename(revs->prefix, argv[j], 0);

Likewise.

> @@ -81,13 +83,13 @@ static void NORETURN die_verify_filename(const char *prefix, const char *arg)
>   * it to be preceded by the "--" marker (or we want the user to
>   * use a format like "./-filename")
>   */
> -void verify_filename(const char *prefix, const char *arg)
> +void verify_filename(const char *prefix, const char *arg, int diagnose_rev)
>  {
>  	if (*arg == '-')
>  		die("bad flag '%s' used after filename", arg);
>  	if (check_filename(prefix, arg))
>  		return;
> -	die_verify_filename(prefix, arg);
> +	die_verify_filename(prefix, arg, diagnose_rev);

And this implements the "if it is path, don't complain, but
otherwise diagnose misspelled rev if the caller asked us to".

I think the patch is not wrong per-se, but diagnose_rev is probably
misnamed.  It tells the callee what to do, but gives little hint to
the caller when to set it.  s/diagnose_rev/first_non_rev/ or
something might make it easier to understand for future callers.

Thanks.

Re: [PATCH 2/2 RFC] verify_filename: ask the caller to chose the kind of diagnosis

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> +/*
>> + * Verify that "name" is a filename.
>> + * The "diagnose_rev" is used to provide a user-friendly diagnosis. If
>> + * 0, the diagnosis will try to diagnose "name" as an invalid object
>> + * name (e.g. HEAD:foo). If non-zero, the diagnosis will only complain
>> + * about an inexisting file.
>> + */
>> +extern void verify_filename(const char *prefix, const char *name, int diagnose_rev);
>
> The whole point of verify_filename() is to make sure, because the
> user did not have disambiguating "--" on the command line, that the
> first non-rev argument is a path and also it cannot be interpreted
> as a valid rev.  It somehow feels wrong to make it also responsible,
> for a possibly misspelled rev.

verify_filename will check the same thing in both cases. If the caller
looks like

if (name is not a valid object name) {
        verify_filename(name);
}

then it should ask for a detailed diagnosis. If the caller knows that an
object name would not be accepted anyway, it should not.

> The caller can mistakenly throw 0 or 1 at random but the _only_ right
> value for this parameter is to set it to true only for the first
> non-rev, no?

In general, this is the case, but that's a consequence of "an object
name would not be accepted anyway". I don't think there is any such call
in Git's code source right now, but we could imagine a caller trying to
verify that something is actually a file, and "verify_filename" would be
a correct way to do it, provided you pass diagnose_rev == 0.

>> --- a/builtin/grep.c
>> +++ b/builtin/grep.c
>> @@ -927,8 +927,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>>  	/* The rest are paths */
>>  	if (!seen_dashdash) {
>>  		int j;
>> -		for (j = i; j < argc; j++)
>> -			verify_filename(prefix, argv[j]);
>> +		if (i < argc) {
>> +			verify_filename(prefix, argv[i], 1);
>> +			for (j = i + 1; j < argc; j++)
>> +				verify_filename(prefix, argv[j], 0);
>> +		}
>
> This is exactly
>
> 	verify_filename(prefix, argv[j], j == first_non_rev)

I buy that.

>> diff --git a/builtin/reset.c b/builtin/reset.c
>> index 8c2c1d5..4cc34c9 100644
>> --- a/builtin/reset.c
>> +++ b/builtin/reset.c
>> @@ -285,7 +285,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>>  			rev = argv[i++];
>>  		} else {
>>  			/* Otherwise we treat this as a filename */
>> -			verify_filename(prefix, argv[i]);
>> +			verify_filename(prefix, argv[i], 1);
>
> This is also checking the first non-rev, too.  We just saw
> "florbl^{triee}" in "git reset florbl^{triee} hello.c" is not a
> valid rev.  If "florbl^{triee}" is indeed a file, we shouldn't
> complain and die with "This may be a misspelled rev", but take it as
> a path.

Yes, and this is what we are doing already. This verify_filename is only
called for the first argument. We have exactly the right pattern here:

		/*
		 * Otherwise, argv[i] could be either <rev> or <paths> and
		 * has to be unambiguous.
		 */
		else if (!get_sha1(argv[i], sha1)) {
			verify_non_filename(prefix, argv[i]);
		} else {
			/* Otherwise we treat this as a filename */
			verify_filename(prefix, argv[i], 1);
		}

Clearly, if "argv[i]" is a filename, it's OK and we take it as it is,
but if it is not, then the failure is due to both "verify_filename" and
"git_sha1" failures, and we should take that into account in the
diagnosis. To me, the fact that this is called for the first non-rev
argument is a detail, the real reason to pass 1 here is that we wouldn't
have called verify_filename if it was a revision.

>> @@ -81,13 +83,13 @@ static void NORETURN die_verify_filename(const char *prefix, const char *arg)
>>   * it to be preceded by the "--" marker (or we want the user to
>>   * use a format like "./-filename")
>>   */
>> -void verify_filename(const char *prefix, const char *arg)
>> +void verify_filename(const char *prefix, const char *arg, int diagnose_rev)
>>  {
>>  	if (*arg == '-')
>>  		die("bad flag '%s' used after filename", arg);
>>  	if (check_filename(prefix, arg))
>>  		return;
>> -	die_verify_filename(prefix, arg);
>> +	die_verify_filename(prefix, arg, diagnose_rev);
>
> And this implements the "if it is path, don't complain, but
> otherwise diagnose misspelled rev if the caller asked us to".
>
> I think the patch is not wrong per-se, but diagnose_rev is probably
> misnamed.  It tells the callee what to do, but gives little hint to
> the caller when to set it.  s/diagnose_rev/first_non_rev/ or
> something might make it easier to understand for future callers.

I considered "could_have_been_a_rev" or
"would_have_been_ok_if_it_was_a_rev" ;-).

I think it would be better to document that as a comment, like this in
cache.h:

   * In most cases, the caller will want diagnose_rev == 1 when
   * verifying the first non_rev argument, and diagnose_rev == 0 for the
   * next ones (because we already saw a filename, there's not ambiguity
   * anymore).
   */
  extern void verify_filename(const char *prefix, const char *name, int diagnose_rev);
  
but keep a param name that is more general.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 2/2 RFC] verify_filename: ask the caller to chose the kind of diagnosis

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> The caller can mistakenly throw 0 or 1 at random but the _only_ right
>> value for this parameter is to set it to true only for the first
>> non-rev, no?
>
> In general, this is the case, but that's a consequence of "an object
> name would not be accepted anyway". I don't think there is any such call
> in Git's code source right now, but we could imagine a caller trying to
> verify that something is actually a file, and "verify_filename" would be
> a correct way to do it, provided you pass diagnose_rev == 0.

While I do not think that is likely to happen, I did think about
that, but then it started to feel somewhat troublesome that we do
not have symmetry "syntactically this place we have to have rev but
it could be a misspelled pathname" and I stopped.

If I mentally rephrase diagnose_rev to could_have_been_a_rev as you
later explained, it sort-of makes sense, and "diagnose_rev" hints
that, so probably it is not too bad.  Maybe "diagnose_misspelt_rev"
would have given a stronger hint, though.

> I think it would be better to document that as a comment, like this in
> cache.h:
>
>    * In most cases, the caller will want diagnose_rev == 1 when
>    * verifying the first non_rev argument, and diagnose_rev == 0 for the
>    * next ones (because we already saw a filename, there's not ambiguity
>    * anymore).
>    */
>   extern void verify_filename(const char *prefix, const char *name, int diagnose_rev);

I agree a better documentation is called for.  It is not "in most
cases", but "in all cases where the caller has to guess the boundary
between revs (that come earlier on the command line) and paths (that
come later), pass 1 in diagnose_misspelt_rev when checking the first
argument that was not a rev, because it is not a path but is a rev
that the user misspelled" or something like that.

I also thought that verify_filename_or_diagnose_misspelt_rev() as a
separate helper function might be cleaner, but by changing the
signature of an existing function you made sure that all the
exicting calls appear in the same patch and get eyeballed for the
correctness of the converison, which was good.

Re: [PATCH 1/2] sha1_name: don't trigger detailed diagnosis for file arguments

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> diagnose_invalid_sha1_path is normally meant to be called to diagnose
> <treeish>:<pathname> when <pathname> does not exist in <treeish>.
> However, the current code may call it if <treeish>:<pathname> is invalid
> (which triggers another call with only_to_die == 1), but for another
> reason. This happens when calling e.g.
>
>   git log existing-file HEAD:existing-file
>
> (because existing-file is a file and not a revision, the next arguments
> are assumed to be files too), leading to incorrect message like
> "existing-file does not exist in HEAD".
>
> Check that the search for <pathname> in <treeish> fails before triggering
> the diagnosis.
>
> Bug report and code fix by: Junio C Hamano <gitster@pobox.com>
> Test by: Matthieu Moy <Matthieu.Moy@imag.fr>
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
>
> This patch is very simple and should be rather uncontroversial.

I am not so sure about that.  The "only-to-die" caller is not even
expecting that the call to this codepath would successfully return.

Or at least, it shouldn't.

So it might not be a bad idea to actually catch this as a
programming error and do

	if (only_to_die) {
        	if (!ret)
                	die("BUG");
		diagnose_invalid_sha1_path(...);
	}

instead, especially since we will have a more fundamental fix with
your 2/2 patch as a longer-term fix.

>  sha1_name.c                    |  2 +-
>  t/t1506-rev-parse-diagnosis.sh | 11 +++++++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/sha1_name.c b/sha1_name.c
> index c633113..5d81ea0 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -1127,7 +1127,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
>  			if (new_filename)
>  				filename = new_filename;
>  			ret = get_tree_entry(tree_sha1, filename, sha1, &oc->mode);
> -			if (only_to_die) {
> +			if (ret && only_to_die) {
>  				diagnose_invalid_sha1_path(prefix, filename,
>  							   tree_sha1, object_name);
>  				free(object_name);
> diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
> index 0843a1c..4a39ac5 100755
> --- a/t/t1506-rev-parse-diagnosis.sh
> +++ b/t/t1506-rev-parse-diagnosis.sh
> @@ -171,4 +171,15 @@ test_expect_success 'relative path when startup_info is NULL' '
>  	grep "BUG: startup_info struct is not initialized." error
>  '
>  
> +test_expect_success '<commit>:file correctly diagnosed after a pathname' '
> +	test_must_fail git rev-parse file.txt HEAD:file.txt 1>actual 2>error &&
> +	test_i18ngrep ! "exists on disk" error &&
> +	test_i18ngrep "unknown revision or path not in the working tree" error &&
> +	cat >expect <<EOF &&
> +file.txt
> +HEAD:file.txt
> +EOF
> +	test_cmp expect actual
> +'
> +
>  test_done

Re: [PATCH 1/2] sha1_name: don't trigger detailed diagnosis for file arguments

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> I am not so sure about that.  The "only-to-die" caller is not even
> expecting that the call to this codepath would successfully return.
>
> Or at least, it shouldn't.
>
> So it might not be a bad idea to actually catch this as a
> programming error and do
>
> 	if (only_to_die) {
>         	if (!ret)
>                 	die("BUG");
> 		diagnose_invalid_sha1_path(...);
> 	}

I disagree.

The only-to-die caller can expect that get_sha1_with_context_1 never
returns when called with only-to-die, but it's a stronger assumption to
expect that this particular place will trigger the failure.

In this case, the assumption is correct because there's a "return ret;"
a bit later in the code, but I don't think we should have to look at
this to check the correctness of the code (for example, if something
like "if (ret) try_some_fallback_method();" is later added before
"return ret;", then the assumption would become false).

My version reads as

  try something;
  if (it failed && I'm only here to report an error)
          report_error();

which I find easier to understand.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 1/2] sha1_name: don't trigger detailed diagnosis for file arguments

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> My version reads as
>
>   try something;
>   if (it failed && I'm only here to report an error)
>           report_error();
>
> which I find easier to understand.

I agree that _this_ part is easy to understand when written that
way.  But then shouldn't there be a blanket "The caller is here only
to report an error, but all the previous code didn't find any error,
so there is something wrong" check much later in the code before it
returns a success?  Or am I being too paranoid?

Re: [PATCH 1/2] sha1_name: don't trigger detailed diagnosis for file arguments

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> My version reads as
>>
>>   try something;
>>   if (it failed && I'm only here to report an error)
>>           report_error();
>>
>> which I find easier to understand.
>
> I agree that _this_ part is easy to understand when written that
> way.  But then shouldn't there be a blanket "The caller is here only
> to report an error, but all the previous code didn't find any error,
> so there is something wrong" check much later in the code before it
> returns a success?  Or am I being too paranoid?

Currently, there's no problem if get_sha1_with_context_1 returns without
dying, because the caller does:

	if (!(arg[0] == ':' && !isalnum(arg[1])))
		/* try a detailed diagnostic ... */
		get_sha1_with_mode_1(arg, sha1, &mode, 1, prefix);

	/* ... or fall back the most general message. */
	die("ambiguous argument '%s': unknown revision or path not in the working tree.\n"
	    "Use '--' to separate paths from revisions", arg);

If we failed to give a nice diagnosis, we give the generic error
message.

We could add this check within get_sha1_with_context_1 to simplify the
task of the caller, but that would require adding it before each
"return" statement, which I think is overkill.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 1/2] sha1_name: don't trigger detailed diagnosis for file arguments

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>>
>>> My version reads as
>>>
>>>   try something;
>>>   if (it failed && I'm only here to report an error)
>>>           report_error();
>>>
>>> which I find easier to understand.
>>
>> I agree that _this_ part is easy to understand when written that
>> way.  But then shouldn't there be a blanket "The caller is here only
>> to report an error, but all the previous code didn't find any error,
>> so there is something wrong" check much later in the code before it
>> returns a success?  Or am I being too paranoid?
>
> Currently, there's no problem if get_sha1_with_context_1 returns without
> dying, because the caller does:
>
> 	if (!(arg[0] == ':' && !isalnum(arg[1])))
> 		/* try a detailed diagnostic ... */
> 		get_sha1_with_mode_1(arg, sha1, &mode, 1, prefix);
>
> 	/* ... or fall back the most general message. */
> 	die("ambiguous argument '%s': unknown revision or path not in the working tree.\n"
> 	    "Use '--' to separate paths from revisions", arg);
>
> If we failed to give a nice diagnosis, we give the generic error
> message.
>
> We could add this check within get_sha1_with_context_1 to simplify the
> task of the caller, but that would require adding it before each
> "return" statement, which I think is overkill.

OK.

[PATCH 1/2 v2] sha1_name: don't trigger detailed diagnosis for file arguments

[Matthieu Moy]

diagnose_invalid_sha1_path is normally meant to be called to diagnose
<treeish>:<pathname> when <pathname> does not exist in <treeish>.
However, the current code may call it if <treeish>:<pathname> is invalid
(which triggers another call with only_to_die == 1), but for another
reason. This happens when calling e.g.

  git log existing-file HEAD:existing-file

(because existing-file is a file and not a revision, the next arguments
are assumed to be files too), leading to incorrect message like
"existing-file does not exist in HEAD".

Check that the search for <pathname> in <treeish> fails before triggering
the diagnosis.

Bug report and code fix by: Junio C Hamano <gitster@pobox.com>
Test by: Matthieu Moy <Matthieu.Moy@imag.fr>

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
No change since v1

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

diff --git a/sha1_name.c b/sha1_name.c
index c633113..5d81ea0 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1127,7 +1127,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
 			if (new_filename)
 				filename = new_filename;
 			ret = get_tree_entry(tree_sha1, filename, sha1, &oc->mode);
-			if (only_to_die) {
+			if (ret && only_to_die) {
 				diagnose_invalid_sha1_path(prefix, filename,
 							   tree_sha1, object_name);
 				free(object_name);
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index 0843a1c..4a39ac5 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -171,4 +171,15 @@ test_expect_success 'relative path when startup_info is NULL' '
 	grep "BUG: startup_info struct is not initialized." error
 '
 
+test_expect_success '<commit>:file correctly diagnosed after a pathname' '
+	test_must_fail git rev-parse file.txt HEAD:file.txt 1>actual 2>error &&
+	test_i18ngrep ! "exists on disk" error &&
+	test_i18ngrep "unknown revision or path not in the working tree" error &&
+	cat >expect <<EOF &&
+file.txt
+HEAD:file.txt
+EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
1.7.11.rc0.57.g84a04c7

[PATCH 2/2 v2] verify_filename: ask the caller to chose the kind of diagnosis

[Matthieu Moy]

verify_filename can be called in two different contexts. Either we just
tried to interpret a string as an object name, and it fails, so we try
looking for a working tree file as a fallback, or we _know_ that we are
looking for a filename, and shouldn't even try interpreting the string as
an object name.

For example, with this change, we get:

  $ git log COPYING HEAD:inexistant
  fatal: HEAD:inexistant: no such path in the working tree.
  Use '-- <path>...' to specify paths that do not exist locally.
  $ git log HEAD:inexistant
  fatal: Path 'inexistant' does not exist in 'HEAD'

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Changes since v1 :

* Simplify the patch when verify_filename is called in a for loop
  (Suggested by Junio)

* Improved documentation, rename "diagnose_rev" to
  "diagnose_misspelt_rev".

* Added "Use '-- <path>...' to specify paths that do not exist locally."
  advice in addition to "no such path in the working tree" (useful for
  example with "git log -- old-file.txt")

 builtin/grep.c                 |  2 +-
 builtin/reset.c                |  2 +-
 builtin/rev-parse.c            |  4 ++--
 cache.h                        |  4 +++-
 revision.c                     |  2 +-
 setup.c                        | 26 +++++++++++++++++++++++---
 t/t1506-rev-parse-diagnosis.sh |  2 +-
 7 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index fe1726f..29adb0a 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -928,7 +928,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	if (!seen_dashdash) {
 		int j;
 		for (j = i; j < argc; j++)
-			verify_filename(prefix, argv[j]);
+			verify_filename(prefix, argv[j], j == i);
 	}
 
 	paths = get_pathspec(prefix, argv + i);
diff --git a/builtin/reset.c b/builtin/reset.c
index 8c2c1d5..4cc34c9 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -285,7 +285,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 			rev = argv[i++];
 		} else {
 			/* Otherwise we treat this as a filename */
-			verify_filename(prefix, argv[i]);
+			verify_filename(prefix, argv[i], 1);
 		}
 	}
 
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 733f626..13495b8 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -486,7 +486,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 
 		if (as_is) {
 			if (show_file(arg) && as_is < 2)
-				verify_filename(prefix, arg);
+				verify_filename(prefix, arg, 0);
 			continue;
 		}
 		if (!strcmp(arg,"-n")) {
@@ -734,7 +734,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 		as_is = 1;
 		if (!show_file(arg))
 			continue;
-		verify_filename(prefix, arg);
+		verify_filename(prefix, arg, 1);
 	}
 	if (verify) {
 		if (revs_count == 1) {
diff --git a/cache.h b/cache.h
index 06413e1..4079037 100644
--- a/cache.h
+++ b/cache.h
@@ -409,7 +409,9 @@ extern const char *setup_git_directory(void);
 extern char *prefix_path(const char *prefix, int len, const char *path);
 extern const char *prefix_filename(const char *prefix, int len, const char *path);
 extern int check_filename(const char *prefix, const char *name);
-extern void verify_filename(const char *prefix, const char *name);
+extern void verify_filename(const char *prefix, 
+			    const char *name,
+			    int diagnose_misspelt_rev);
 extern void verify_non_filename(const char *prefix, const char *name);
 
 #define INIT_DB_QUIET 0x0001
diff --git a/revision.c b/revision.c
index 935e7a7..39cc6b0 100644
--- a/revision.c
+++ b/revision.c
@@ -1781,7 +1781,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			 * but the latter we have checked in the main loop.
 			 */
 			for (j = i; j < argc; j++)
-				verify_filename(revs->prefix, argv[j]);
+				verify_filename(revs->prefix, argv[j], j == i);
 
 			append_prune_data(&prune_data, argv + i);
 			break;
diff --git a/setup.c b/setup.c
index 731851a..7b196f8 100644
--- a/setup.c
+++ b/setup.c
@@ -53,11 +53,17 @@ int check_filename(const char *prefix, const char *arg)
 	die_errno("failed to stat '%s'", arg);
 }
 
-static void NORETURN die_verify_filename(const char *prefix, const char *arg)
+static void NORETURN die_verify_filename(const char *prefix,
+					 const char *arg,
+					 int diagnose_misspelt_rev)
 {
 	unsigned char sha1[20];
 	unsigned mode;
 
+	if (!diagnose_misspelt_rev)
+		die("%s: no such path in the working tree.\n"
+		    "Use '-- <path>...' to specify paths that do not exist locally.",
+		    arg);
 	/*
 	 * Saying "'(icase)foo' does not exist in the index" when the
 	 * user gave us ":(icase)foo" is just stupid.  A magic pathspec
@@ -80,14 +86,28 @@ static void NORETURN die_verify_filename(const char *prefix, const char *arg)
  * as true, because even if such a filename were to exist, we want
  * it to be preceded by the "--" marker (or we want the user to
  * use a format like "./-filename")
+ *
+ * The "diagnose_misspelt_rev" is used to provide a user-friendly
+ * diagnosis. If 0, the diagnosis will try to diagnose "name" as an
+ * invalid object name (e.g. HEAD:foo). If non-zero, the diagnosis
+ * will only complain about an inexisting file.
+ *
+ * This function is typically called to check that a "file or rev"
+ * argument is unambiguous. In this case, the caller will want
+ * diagnose_misspelt_rev == 1 when verifying the first non-rev
+ * argument (which could have been a revision), and
+ * diagnose_misspelt_rev == 0 for the next ones (because we already
+ * saw a filename, there's not ambiguity anymore).
  */
-void verify_filename(const char *prefix, const char *arg)
+void verify_filename(const char *prefix, 
+		     const char *arg,
+		     int diagnose_misspelt_rev)
 {
 	if (*arg == '-')
 		die("bad flag '%s' used after filename", arg);
 	if (check_filename(prefix, arg))
 		return;
-	die_verify_filename(prefix, arg);
+	die_verify_filename(prefix, arg, diagnose_misspelt_rev);
 }
 
 /*
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index 4a39ac5..a3f11e8 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -174,7 +174,7 @@ test_expect_success 'relative path when startup_info is NULL' '
 test_expect_success '<commit>:file correctly diagnosed after a pathname' '
 	test_must_fail git rev-parse file.txt HEAD:file.txt 1>actual 2>error &&
 	test_i18ngrep ! "exists on disk" error &&
-	test_i18ngrep "unknown revision or path not in the working tree" error &&
+	test_i18ngrep "no such path in the working tree" error &&
 	cat >expect <<EOF &&
 file.txt
 HEAD:file.txt
-- 
1.7.11.rc0.57.g84a04c7

Re: [PATCH 2/2 v2] verify_filename: ask the caller to chose the kind of diagnosis

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> verify_filename can be called in two different contexts. Either we just
> tried to interpret a string as an object name, and it fails, so we try
> looking for a working tree file as a fallback, or we _know_ that we are
> looking for a filename, and shouldn't even try interpreting the string as
> an object name.
>
> For example, with this change, we get:
>
>   $ git log COPYING HEAD:inexistant
>   fatal: HEAD:inexistant: no such path in the working tree.
>   Use '-- <path>...' to specify paths that do not exist locally.
>   $ git log HEAD:inexistant
>   fatal: Path 'inexistant' does not exist in 'HEAD'
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---

Thanks; both patches look sensible (modulo minor nits below).

Will queue with a local fix-up.

> @@ -80,14 +86,28 @@ static void NORETURN die_verify_filename(const char *prefix, const char *arg)
>   * as true, because even if such a filename were to exist, we want
>   * it to be preceded by the "--" marker (or we want the user to
>   * use a format like "./-filename")
> + *
> + * The "diagnose_misspelt_rev" is used to provide a user-friendly
> + * diagnosis. If 0, the diagnosis will try to diagnose "name" as an
> + * invalid object name (e.g. HEAD:foo). If non-zero, the diagnosis
> + * will only complain about an inexisting file.

I have a feeling that "if 0/if non-zero" above are backwards.

Re: [PATCH 2/2 v2] verify_filename: ask the caller to chose the kind of diagnosis

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

>> @@ -80,14 +86,28 @@ static void NORETURN die_verify_filename(const char *prefix, const char *arg)
>>   * as true, because even if such a filename were to exist, we want
>>   * it to be preceded by the "--" marker (or we want the user to
>>   * use a format like "./-filename")
>> + *
>> + * The "diagnose_misspelt_rev" is used to provide a user-friendly
>> + * diagnosis. If 0, the diagnosis will try to diagnose "name" as an
>> + * invalid object name (e.g. HEAD:foo). If non-zero, the diagnosis
>> + * will only complain about an inexisting file.
>
> I have a feeling that "if 0/if non-zero" above are backwards.

Oops, right. Thanks,

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/