Re: [PATCH 01/10] ref-filter: add option to filter only branches

[Karthik Nayak <karthik.188@gmail.com>]

On Tue, Aug 11, 2015 at 11:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Add an option in 'filter_refs()' to use 'for_each_branch_ref()'
>> and filter refs. This type checking is done by adding a
>> 'FILTER_REFS_BRANCHES' in 'ref-filter.h'.
>>
>> Add an option in 'ref_filter_handler()' to filter different
>> types of branches by calling 'filter_branch_kind()' which
>> checks for the type of branch needed.
>>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  ref-filter.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>>  ref-filter.h | 10 ++++++++--
>>  2 files changed, 55 insertions(+), 2 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index de84dd4..c573109 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1044,6 +1044,46 @@ static const unsigned char *match_points_at(struct sha1_array *points_at,
>>       return NULL;
>>  }
>>
>> +/*
>> + * Checks if a given refname is a branch and returns the kind of
>> + * branch it is. If not a branch, 0 is returned.
>> + */
>> +static int filter_branch_kind(struct ref_filter *filter, const char *refname)
>> +{
>> +     int kind, i;
>> +
>> +     static struct {
>> +             const char *prefix;
>> +             int kind;
>
> Make a mental note that this is signed int.
>
>> +     } ref_kind[] = {
>> +             { "refs/heads/" , REF_LOCAL_BRANCH },
>> +             { "refs/remotes/" , REF_REMOTE_BRANCH },
>> +     };
>> +
>> +     /*  If no kind is specified, no need to filter */
>> +     if (!filter->branch_kind)
>> +             return REF_NO_BRANCH_FILTERING;
>> +
>> +     for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
>> +             if (starts_with(refname, ref_kind[i].prefix)) {
>> +                     kind = ref_kind[i].kind;
>> +                     break;
>> +             }
>> +     }
>> +
>> +     if (ARRAY_SIZE(ref_kind) <= i) {
>> +             if (!strcmp(refname, "HEAD"))
>> +                     kind = REF_DETACHED_HEAD;
>> +             else
>> +                     return 0;
>> +     }
>> +
>> +     if ((filter->branch_kind & kind) == 0)
>> +             return 0;
>> +
>> +     return kind;
>> +}
>
> While this looks fine, I am not sure if this is a good interface for
> filtering, though.
>
> If you start from already having everything and want to limit things
> down to "refs/heads/", this might make sense but it would be far
> more efficient, if you know that you want to limit to "refs/heads/"
> upfront, not to collect everything but just limit the collection to
> those under "refs/heads/" without wasting cycles in the first place.
>

Yes, considering this and what you've said below about how the
bits don't make sense, I re-wrote filter_refs() to filter based on what we want
So this part will be removed entirely. Thanks for pointing me in the
right direction.

>> diff --git a/ref-filter.h b/ref-filter.h
>> index 5be3e35..b5a13e8 100644
>> --- a/ref-filter.h
>> +++ b/ref-filter.h
>> @@ -16,6 +16,12 @@
>>  #define FILTER_REFS_INCLUDE_BROKEN 0x1
>>  #define FILTER_REFS_ALL 0x2
>>  #define FILTER_REFS_TAGS 0x4
>> +#define FILTER_REFS_BRANCHES 0x8
>
> Is this a sensible set of bits?  Does it make sense to have ALL and
> TAGS at the same time (and what does that mean?)?
>
>> +#define REF_DETACHED_HEAD   0x01
>> +#define REF_LOCAL_BRANCH    0x02
>> +#define REF_REMOTE_BRANCH   0x04
>> +#define REF_NO_BRANCH_FILTERING 0x08
>
> Where do these values go?  It is a returned by filter-branch-kind
> for each ref, i.e. given "refs/heads/bar", it answers "Yeah, that is
> a local branch".  Why are these values pretending to be a set of
> bits that can be combined together, as if a branch can be both LOCAL
> and REMOTE?  This does not make _any_ sense.
>

This was taken from branch.c, I thought of using an enum instead but that
would again require most of branch.c, hence it's been carried over
without changing
I'm thinking of changing it, any suggestions?

>
>>  #define ALIGN_LEFT 0x01
>>  #define ALIGN_RIGHT 0x02
>> @@ -50,7 +56,7 @@ struct ref_sorting {
>>
>>  struct ref_array_item {
>>       unsigned char objectname[20];
>> -     int flag;
>> +     int flag, kind;
>
> For readability, do not define multiple structure fields on a single
> line.
>
> If you are storing a set of bits in an integer, use unsigned.  If it
> is an enumeration, int is fine.
>

Thanks will change.

>>       const char *symref;
>>       struct commit *commit;
>>       struct atom_value *value;
>> @@ -76,7 +82,7 @@ struct ref_filter {
>>
>>       unsigned int with_commit_tag_algo : 1,
>>               match_as_path : 1;
>> -     unsigned int lines;
>> +     unsigned int lines, branch_kind;
>
> For readability, do not define multiple structure fields on a single
> line.
>
>>  };
>>
>>  struct ref_filter_cbdata {

Will do.

-- 
Regards,
Karthik Nayak