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