On Wed, Sep 2, 2015 at 3:00 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Karthik Nayak <karthik.188@xxxxxxxxx> 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?
>
>> + 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.
>
> #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
--
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