Re: Tag peeling peculiarities

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mar 14, 2013, 11:28 UTC
Message-ID: <5141B475.1000707@alum.mit.edu>
In-Reply-To: <20130314052448.GA2300@sigill.intra.peff.net>

On 03/14/2013 06:24 AM, Jeff King wrote:

Show 16 quoted lines

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

Show 19 quoted lines

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