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
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.
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html