Re: Re* [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references

Jeff King [off-list ref] writes:

It looks like we also use it in remote.c:count_refspec_match, but I
haven't figured out if that can trigger a warning or not.

It starts sounding like that the ill-thought-out warning should be ripped
out regardless of what other things we do.

On Tue, Oct 11, 2011 at 04:50:46PM -0700, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

It looks like we also use it in remote.c:count_refspec_match, but I
haven't figured out if that can trigger a warning or not.

It starts sounding like that the ill-thought-out warning should be ripped
out regardless of what other things we do.

Maybe. I think it is not the warning that is wrong, but that it is
exposing a slightly hack-ish part of git (that we consider things like
$GIT_DIR/config as possible refs, and just silently reject them because
they happen not to have the right format).

On the other hand, it has been working fine that way for years, so maybe
it is not worth changing now.

At any rate, I think the changes should be all or nothing. If the
warning goes away, fine. But if the warning stays, and dwim_ref is going
to have special rules for looking in the top-level $GIT_DIR, then things
like shorten_unambiguous_ref need to respect those rules, or we've just
created a new bug.

-Peff

On 10/12/2011 01:50 AM, Junio C Hamano wrote:

It starts sounding like that the ill-thought-out warning should be ripped
out regardless of what other things we do.

ISTM that the warning is proving its worth already by illuminating some
questionable practices (treating any file in $GIT_DIR as a potential
reference) :-)

However, it might be that fixing 100% of the questionable practices is
too much work.  In that case, I suggest that the warning not be ripped
out altogether, but rather that the warning only be emitted for invalid
references whose names start with "refs/".

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Jeff King [off-list ref] writes:

At any rate, I think the changes should be all or nothing. If the
warning goes away, fine. But if the warning stays, and dwim_ref is going
to have special rules for looking in the top-level $GIT_DIR, then things
like shorten_unambiguous_ref need to respect those rules, or we've just
created a new bug.

Whether we remove the warning or not, I think it would be an improvement
not to look at random files directly underneath $GIT_DIR/. I am not sure
how we can be confident that we caught everything, though.

In other words, is shorten-unambiguous-refs the last one that needs
fixing? How would we know for certain?

Also I tend to think Michael's "only warn in refs/" is probably not the
right solution. When a caller asks to resolve_ref(MERGE_HEAD), one of
these things can be true:

 - A file $GIT_DIR/MERGE_HEAD does not exist; this is not inherently an
   error unless we were supposed to be in the middle of a conflicted
   merge.

 - A file $GIT_DIR/MERGE_HEAD exists, and records a correct 40-hexadecimal
   get_sha1_hex() can grok. This is perfectly normal.

 - A file $GIT_DIR/MERGE_HEAD exists, but get_sha1_hex() does not grok
   it. Michael warns against this twice, and I think it is a wrong thing
   to pass this unnoticed.

Once we tighten all the "too loose accesses to $GIT_DIR/$random_filename",
we might even want to have an option to cause the caller to die() in the
error case, and the logic is the same for refs under $GIT_DIR/refs/, not
just the ref-like-things directly under $GIT_DIR.

A regular ref, can also appear in $GIT_DIR/packed-refs, but a corruption
of an entry in the file will be caught when the file is read and outside
the scope of this discussion, I think.

On Tue, Oct 11, 2011 at 09:41:39PM -0700, Junio C Hamano wrote:

Whether we remove the warning or not, I think it would be an improvement
not to look at random files directly underneath $GIT_DIR/. I am not sure
how we can be confident that we caught everything, though.

In other words, is shorten-unambiguous-refs the last one that needs
fixing? How would we know for certain?

My assumption was that the set of rules is defined by
ref_rev_parse_rules, so grepping for functions that access that would be
sufficient.

It looks like there is also ref_fetch_rules, which serves a similar
purpose. Uses of that would also need to be audited.

I _think_ that should be enough for arbitrary lookup. I'm sure other
callsites directly say things like 'git_path("MERGE_HEAD")', but that's
not a problem. They would be doing so with a well-defined top-level
refname. This is really just about the ref_rev_parse_rules lookups.

-Peff