"Detailed diagnosis" perhaps broken

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