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