Re: [PATCH v15 06/13] ref-filter: add option to filter out tags, branches and remotes

From: Karthik Nayak <karthik.188@gmail.com>
Date: Sep 2, 2015, 01:27 UTC
Message-ID: <CAOLa=ZQQia58_9c+KDh9mZ_UdcpUn96yuPOdABYvs1gno04CmQ@mail.gmail.com>
In-Reply-To: <xmqqa8t5rfng.fsf@gitster.mtv.corp.google.com>

On Wed, Sep 2, 2015 at 3:00 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +     if (!filter->kind)
>>               die("filter_refs: invalid type");
>> +     else {
>> +             if (filter->kind == FILTER_REFS_BRANCHES)
>> +                     ret = for_each_fullref_in("refs/heads/", ref_filter_handler, &ref_cbdata, broken);
>> +             else if (filter->kind == FILTER_REFS_REMOTES)
>> +                     ret = for_each_fullref_in("refs/remotes/", ref_filter_handler, &ref_cbdata, broken);
>> +             else if (filter->kind == FILTER_REFS_TAGS)
>> +                     ret = for_each_fullref_in("refs/tags/", ref_filter_handler, &ref_cbdata, broken);
>> +             else if (filter->kind & FILTER_REFS_ALL)
>> +                     ret = for_each_fullref_in("", ref_filter_handler, &ref_cbdata, broken);
>
> This if/else if/else if/ cascade and ...

Did you notice the "==" for others and "&" for the ALL?

Show 43 quoted lines

>
>> +             if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))
>> +                     head_ref(ref_filter_handler, &ref_cbdata);
>> +     }
>> +
>>
>>       /*  Filters that need revision walking */
>>       if (filter->merge_commit)
>> diff --git a/ref-filter.h b/ref-filter.h
>> index 45026d0..0913ba9 100644
>> --- a/ref-filter.h
>> +++ b/ref-filter.h
>> @@ -13,8 +13,15 @@
>>  #define QUOTE_PYTHON 4
>>  #define QUOTE_TCL 8
>>
>> -#define FILTER_REFS_INCLUDE_BROKEN 0x1
>> -#define FILTER_REFS_ALL 0x2
>> +#define FILTER_REFS_INCLUDE_BROKEN 0x0001
>> +#define FILTER_REFS_TAGS           0x0002
>> +#define FILTER_REFS_BRANCHES       0x0004
>> +#define FILTER_REFS_REMOTES        0x0008
>> +#define FILTER_REFS_OTHERS         0x0010
>
> ... the bit assignment here conceptually do not mesh well with each
> other.  These bits look as if I can ask for both tags and branches
> by passing 0x0006, and if the code above were
>
>         empty the result set;
>         if (filter->kind & FILTER_REFS_BRANCHES)
>                 add in things from refs/heads/ to the result set;
>         if (filter->kind & FILTER_REFS_TAGS)
>                 add in things from refs/tags/ to the result set;
>         ...
>
> without "else if", that would conceptually make sense.
>
> Alternatively, if the code does not (and will not ever) support such
> an arbitrary mixing of bits and intends to only allow "one of
> BRANCHES, REMOTES, TAGS or ALL, and no other choice", then it is
> wrong to pretend as if they can be mixed by defining the constant
> with values with non-overlapping bit patterns.  If you defined these
> constants as

Hmm, The code does support mixing if you see, whenever we mix and match these bits (consider 0x0006) we satisfy the ` else if (filter->kind & FILTER_REFS_ALL)` condition. Which would then go through the entire set of refs and pick only refs we need using filter_ref_kind(). This may seem a little confusing but this avoids ref type filtering when we do not mix bits up.

Show 13 quoted lines

>
> #define FILTER_REFS_TAGS           100
> #define FILTER_REFS_BRANCHES       101
> #define FILTER_REFS_REMOTES        102
> #define FILTER_REFS_OTHERS         103
>
> then nobody would be think that the function can collect both tags
> and branches by passing FILTER_REFS_TAGS|FILTER_REFS_BRANCHES.
>
> The former, i.e. keep the bits distinct and allow them to be OR'ed
> together by updating the code to allow such callers, would be more
> preferrable, of course.
>

Just to confirm, I even changed the function call of for-each-ref to
    filter_refs(&array, &filter, FILTER_REFS_BRANCHES |
FILTER_REFS_TAGS | FILTER_REFS_INCLUDE_BROKEN);
and it printed only "refs/heads/" and "refs/tags/".
Thanks :)

-- 
Regards,
Karthik Nayak