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

> 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;
> };