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

[Junio C Hamano <gitster@pobox.com>]

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

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