On Sat, May 23, 2015 at 4:42 PM, Matthieu Moy
<Matthieu.Moy@xxxxxxxxxxxxxxx> wrote:
> karthik nayak <karthik.188@xxxxxxxxx> 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;
> };
Matthieu, I think you forgot to remove "const char **name_patterns;"
in the above struct, as you put it in the "ref_filter" struct below:
> struct ref_filter {
> const char **name_patterns;
> /* There will be more here later */
> };
I agree that it might be clearer to separate both. In this case
instead of "ref_list" the struct might be called "ref_filter_array" as
we already have "argv_array" in argv-array.h and "sha1_array" in
"sha1-array.h".
--
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