Re: "Detailed diagnosis" perhaps broken

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jun 17, 2012, 18:34 UTC
Message-ID: <vpq395tvlc0.fsf@bauges.imag.fr>
In-Reply-To: <7vpq91cjby.fsf@alter.siamese.dyndns.org>

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

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

Oops!

Show 11 quoted lines

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