Re: [PATCH 2/4] ref-filter: add ref-filter API

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

karthik nayak <karthik.188@gmail.com> writes:

>> At some point, I'd expect something like
>>
>>    filtered_list_of_refs = filer(full_list_of_refs, description_of_filter);
>>
>> That would remove some refs from full_list_of_refs according to
>> description_of_filter.
>>
>> (totally invented code, only to show the idea)
>>
>> If there's a piece of code looking like this, then you need a data
>> structure to store list of refs (full_list_of_refs and
>> filtered_list_of_refs) and another to describe what you're doing with it
>> (description_of_filter).
>>
>> The name ref_filter implies to me that it contains the description of
>> the filter, but looking at the code it doesn't seem to be the case.
>>
>
> But it does just that, doesn't it?
>
> struct ref_filter {
> 	int count, alloc;
> 	struct ref_filter_item **items;
> 	const char **name_patterns;
> };
>
> If you see it does contain 'name_patterns' according to which it will
> filter the given refs,

But it also contains struct ref_filter_item **items, which as I
understand it contains a list of refs (with name, sha1 & such).

That's the part I do not find natural: the same structure contains both
the list of refs and the way it should be filtered.

Re-reading the patch, I seem to understand that you're putting both on
the same struct because of the API of for_each_ref() which takes one
'data' pointer to be casted, so you want both the input (filter
description) and the output (list of refs after filtering) to be
contained in the same struct.

But I think this could be clearer in the code (and/or comment + commit
message). Perhaps stg like:

struct ref_filter_data /* Probably not the best name */ {
        struct ref_list list;
        struct ref_filter filter;
};

struct ref_list {
 	int count, alloc;
 	struct ref_filter_item **items;
 	const char **name_patterns;
};

struct ref_filter {
	const char **name_patterns;
	/* There will be more here later */
};

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/