Re: [WIP/PATCH v4 6/8] for-each-ref: rename some functions and make them public
On June 1, 2015 8:23:51 PM GMT+05:30, Junio C Hamano <gitster@pobox.com> wrote:
Show 19 quoted lines
>Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Could be achieved using a simple wrapper around 'filter_refs()'
>> something like this perhaps.
>>
>> int filter_refs_with_pattern(struct ref_array *ref, int
>> (*for_each_ref_fn)(each_ref_fn, void *), char **patterns)
>> {
>> int i;
>> struct ref_filter_cbdata data;
>> data.filter.name_patterns = patterns;
>> filter_refs(for_each_ref_fn, &data);
>
>I presume that this is
>
> filter_refs(&refs, for_each_ref_fn, &data);
>
>as you would need to have some way to get the result back ;-)
>No it's meant to be that way, see as it takes a struct ref_filter_cbdata, the filters are put in the data.filter.* and we obtain the refs in the data.array.items. So no need for another ref_array to hold the array of items.
Show 39 quoted lines
>> refs->nr = data.array.nr;
>> for(i = 0; i < refs->nr; i++) {
>> /* copy over the refs */
>> }
>> return 0;
>> }
>>
>> Is this on the lines of what you had in mind? If it is, than I could
>> just create a new patch which would make ref_filter_handler() private
>> and introduce filter_refs() as shown.
>
>Yeah. Even though I suggested
>
> filter_refs(&for_each_ref, ...);
>
>I actually would think the external interface should not mention
>for_each_ref() like I did. The primary reason why I felt that it is
>bad for the API to export a generic callback function the caller can
>use to call for_each_ref() or for_each_rawref()" in the longer term
>is because it forces us to always iterate all refs; for_each_ref()
>does not know what the callback filter function wants to do. The
>most common way to filter in the context of your GSoC project is "we
>limit only to refs/heads/*, and then we may also filter by other
>criteria" (that is "git branch" "-l" or possibly with "--contains",
>etc.), and it is very wasteful for that codepath to allow
>for_each_ref() to even enumerate and feed all refs outside the
>refs/heads/* area to your callback, which would involve reading all
>entries in packed-refs (which is a fixed cost so not an overhead)
>and then reading everything in .git/refs/* (which is an overhead we
>could and should avoid when we know we are only interested in the
>branches that live in refs/heads*).
>
>Your first implementation of course can just call for_each_ref() or
>for_each_rawref(), and at the end of GSoC, the code may still do so.
>But by keeping the external interface free of for_each_ref(), you
>could later optimize.
>
>And your above sample only takes for_each_ref_fn without exposing the
>internal use of for_each_ref(), which is good.That seems good, thanks for the guidance.
Karthik Nayak <karthik.188@gmail.com>