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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 1, 2015, 21:30 UTC
Message-ID: <xmqqa8t5rfng.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1441131994-13508-7-git-send-email-Karthik.188@gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

Show 11 quoted lines

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

Show 22 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

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