On 12/13/2011 07:37 AM, Junio C Hamano wrote: > Michael Haggerty <mhagger@xxxxxxxxxxxx> writes: > >> Apropos testing, it is unsettling that our test suite doesn't show any >> failures after my changes. The dir entries in extra_refs are now always >> sorted and de-duped when do_for_each_ref() is called. Could it be that >> duplicate ".have" references never come up in the test suite? It sounds >> like an important code path is not being tested. In particular, I won't >> be able to test whether my fix works :-/ > > I doubt anybody thought of using more than one --reference while cloning > or having more than one entry in $GIT_DIR/objects/info/alternates in a > repository that is being pushed into, even though we might have tests for > simpler single --reference and single alternate cases. Even with a single alternate database, multiple references are advertised with the same name ".have". But the tests never push from a repository with more than one reference in its alternates (verified by instrumenting code). That is why my changes didn't cause test failures. When I, for example, change the setup function in t5519 to push *two* references to alice-pub, then master works fine, my ref-api branch fails, and my fixup patch fixes the failure. > As to the order of the changes, my gut feeling is that it would make it > harder to debug your series to delay the removal of "extra_ref" hackery, > as it would be a corner case that your "hierarchical" structure never has > to worry about in the end result. > > Another possibility is to keep the extra_ref interface in refs.c, without > any of your changes (i.e. keep it just as a flat list, with the original > interface to append to it without any dedup and other fancy features) and > also keep the special casing of it in for_each_ref(). AFAIK, that is the > only iterator that should care about the extra refs. Thinking about it a > bit more, removal of add_extra_ref() API may be unnecessarily complex with > no real gain. For example, extra refs added in builtin/clone.c is used by > builtin/fetch-pack.c, meaning that the codepath that discovers and > remembers these extra history anchor points and the codepath that uses > them while walking the history are not localized and we would need some > sort of "extra ref API" anyway. I am leaning towards this option. In a few minutes I will post a series of patches that store extra_refs in a linked list separate from the reference caches, and iterates over them only from for_each_ref(). I could rebase my ref-api-D changes on top of this patch series in such a way that the extra refs are kept in non-hierarchical storage. But I leave for vacation on Thursday so it is quite likely that I won't be able to get it finished before I return in the new year. An alternative is to use my one-patch fix on top of the ref-api-D changes (plus another patch to beef up t5519). The fix is isolated and I'm confident that it is safe (though I would inspect and test it better before formally submitting it). The advantage is that is less work, so it can be ready tomorrow instead of in two weeks. The disadvantage is that there would be an interval of commits on the feature branch (from the middle of the ref-api-D patch series until the fix) that are non-functional, albeit in a way that the test suite doesn't detect. 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