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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]