Re: Tag peeling peculiarities

[Michael Haggerty <mhagger@alum.mit.edu>]

On 03/14/2013 06:24 AM, Jeff King wrote:
> On Thu, Mar 14, 2013 at 05:41:58AM +0100, Michael Haggerty wrote:
> 
>> Here is analysis of our options as I see them:
>>
>> 1. Accept that tags outside of refs/tags are not reliably advertised in
>>    their peeled form.  Document this deficiency and either:
>>
>>    a. Don't even bother trying to peel refs outside of refs/tags (the
>>       status quo).
> 
> When you say "not reliably advertised" you mean from upload-pack, right?
> What about other callers? From my reading of peel_ref, anybody who calls
> it to get a packed ref may actually get a wrong answer. So this is not
> just about tag auto-following over fetch, but about other places, too
> (however, a quick grep does not make it look like this other places are
> all that commonly used).

Yes, this is a good point.  I didn't audit all of the callers to see
whether any of them need 100% reliability, but I guess I should:

upload-pack.c:763: in send_ref(): This is the caller that we have been
talking about.

builtin/pack-objects.c:559: in mark_tagged(): This function is only
called for references under refs/tags.

builtin/pack-objects.c:2035: in add_ref_tag(): peel_ref() is called only
for refnames under refs/tags.

builtin/describe.c:147: in get_name(): peel_ref() is called only for
refnames under refs/tags.

builtin/show-ref.c:81: in show_ref(): this is broken too, as I showed in
my original email.  This breakage was also introduced in 435c833.

Perhaps if peel_ref() *were* 100% reliable, we might be able to use it
to avoid object lookups in some other places.

> Another fun fact: upload-pack did not use peel_ref until recently
> (435c833, in v1.8.1). So while it is tempting to say "well, this was
> always broken, and nobody cared", it was not really; it is a fairly
> recent regression in 435c833.

I didn't realize that; thanks for pointing it out.  Although peel_ref()
itself has been broken since it was introduced, at least in the sense
that it gives wrong answers for tags outside of refs/tags, prior to
435c833 it appears to only have been used for refs/tags.

>> I think the best option would be 1b.  Even though there would never be a
>> guarantee that all peeled references are recorded and advertised
>> correctly, the behavior would asymptotically approach correctness as old
>> Git versions are retired, and the situations where it fails are probably
>> rare and no worse than the status quo.
> 
> Thanks for laying out the options. I think 1b or 2c are the only sane
> paths forward. With either option, a newer writer produces something
> that all implementations, old and new, should get right, and that is
> primarily what we care about.
> 
> So the only question is how much work we want to put into making sure
> the new reader handles the old writer correctly. Doing 2c is obviously
> more rigorous, and it is not that much work to add the fully-packed
> flag, but I kind of wonder if anybody even cares. We can just say "it's
> a bug fix; run `git pack-refs` again if you care" and call it a day
> (i.e., 1b).
> 
> I could go either way.

On 03/14/2013 06:32 AM, Jeff King wrote:
> [...]
> So it's really not that much code. The bigger question is whether we
> want to have to carry the "fully-peeled" tag forever, and how other
> implementations would treat it.

I agree that 2c is also an attractive option.  Its biggest advantage is
that it make peel_ref() reliable and therefore potentially usable for
other purposes.  And probably the effort of carrying forward the
"fully-peeled" tag is no more work than the alternative of carrying
forward the knowledge that peel_ref() is unreliable.

Your patch looks about right aside from its lack of comments :-).  I was
going to implement approximately the same thing in a patch series that I
am working on, but if you prefer then go ahead and submit your patch and
I will rebase my branch on top of it.

Michael

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