On 12/13/2011 08:12 PM, Michael Haggerty wrote:
> On 12/13/2011 07:37 AM, Junio C Hamano wrote:
>> 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.
I forgot to mention that I think it would be better to separate the
handling of extra refs even more than do the patches
[PATCH 0/6] Handle extra_refs separately from ref_caches
Namely, we should figure out what code wants to set extra refs and or
wants to include extra refs in its iteration over references. The
setters don't have to be changed, but the readers should be. After the
above patch series, the readers call for_each_ref(), which always calls
do_for_each_extra_ref(). The call to do_for_each_extra_ref() should be
taken out of for_each_ref() and called directly by any readers who want
to support extra_refs. Other readers should only call for_each_ref().
(do_for_each_extra_ref(), probably under a different name, would have to
become public.) That way, for_each_ref() could ignore the extra refs
entirely, and other callers of for_each_ref() wouldn't risk getting
extra refs by accident.
Today I spent some time trying to figure out what callers of
for_each_ref() could possible want to deal with extra refs, but I got
lost in the deep call trees and undocumented function declarations. At
the top it is pretty clear that all such code paths start in cmd_clone()
or cmd_receive_pack(). But there are so many calls to the
for_each_ref*() family of functions that I wasn't able to determine
exactly which should allow for extra refs and which shouldn't. (I even
wonder whether extra refs might be used inadvertently in some places
where they aren't wanted, for example in one of the myriad calls to
setup_revisions().) If somebody could put his finger on the
for_each_ref() callers who want to support extra refs, that would be
very helpful.
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