Re: [WIP/PATCH v4 6/8] for-each-ref: rename some functions and make them public

From: Karthik Nayak <karthik.188@gmail.com>
Date: May 31, 2015, 19:34 UTC
Message-ID: <556B6234.2010809@gmail.com>
In-Reply-To: <xmqqmw0kab94.fsf@gitster.dls.corp.google.com>

On 05/31/2015 11:18 PM, Junio C Hamano wrote:

Show 43 quoted lines

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>   /*
>> - * A call-back given to for_each_ref().  Filter refs and keep them for
>> + * A call-back given to for_each_ref(). Filter refs and keep them for
>>    * later object processing.
>>    */
>> -static int grab_single_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
>> +int ref_filter_handler(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
>>   {
>
> I am not sure if this is a good external interface, i.e. to expect
> callers of ref-filter API to do this:
>
> 	prepare cbdata;
> 	for_each_ref(ref_filter_handler, &cbdata);
>
> It might be better to expect callers to do this instead:
>
> 	prepare cbdata;
>          filter_refs(for_each_ref, &cbdata);
>
> i.e. introducing a new "filter_refs()" function as the entry point
> to the ref-filter API.  The filter_refs() entry point may internally
> use ref_filter_handler() that will be file-scope static to ref-filter.c
> and at that point the overly generic "-handler" name would not bother
> anybody ;-) but more importantly, then you can extend the function
> signature of filter_refs() not to be so tied to for_each_ref() API.
> It could be that the internals of cbdata may not be something the
> callers of filter-refs API does not even have to know about, in
> which case the call might even become something like:
>
> 	struct ref_array refs = REF_ARRAY_INIT;
>          const char **ref_patterns = { "refs/heads/*", "refs/tags/*", NULL};
>
> 	filter_refs(&refs, for_each_rawref, ref_patterns);
>
>          /* now "refs" has the result, the caller uses them */
> 	for (i = 0; i < refs.nr; i++)
>          	refs.item[i];
>
> Just a thought.
>

Thats brilliant, How about I introduce something of this sort

int filter_refs(int (*for_each_ref_fn)(each_ref_fn, void *), 
ref_filter_cbdata *cbdata)
{
	return for_each_ref_fn(ref_filter_handler, cbdata);
}

where its the most basic form, and things like

 >
 > 	struct ref_array refs = REF_ARRAY_INIT;
 >          const char **ref_patterns = { "refs/heads/*", "refs/tags/*", 
NULL};
 >
 > 	filter_refs(&refs, for_each_rawref, ref_patterns);
 >
 >          /* now "refs" has the result, the caller uses them */
 > 	for (i = 0; i < refs.nr; i++)
 >          	refs.item[i];
 >

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

-- 
Regards,
Karthik