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