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

From: Karthik Nayak <karthik.188@gmail.com>
Date: May 23, 2015, 17:52 UTC
Message-ID: <5560BE73.8020801@gmail.com>
In-Reply-To: <vpq8ucffj8h.fsf@anie.imag.fr>

Show 12 quoted lines

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

Was kinda confused, This clears out things, Thanks.

Show 20 quoted lines

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

This seems cleaner, agreed.

 >
 > 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".
 >

Somehow ref_list seems more real to me, list of refs.

 >
 > And I do not think an array of things that are operated on should
 > not be named "ref_filter_item".
 >
 > Surely, the latter "set of operations to be applied" may currently
 > be only filtering, but who says it has to stay that way?  "I have a
 > set of refs that represent my local branches I am interested
 > in. Please map them to their corresponding @{upstream}" is a
 > reasonable request once you have an infrastructure to represent "set
 > of refs to be worked on" and "set of operations to apply", and at
 > that point, the items are no longer filter-items (map-items?).
 >

That's also a good point to consider, I shall rename and restructure the code as discussed here, thanks.

-- 
Regards,
Karthik