Re: [PATCH 01/10] ref-filter: add option to filter only branches

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

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

> From: Karthik Nayak <karthik.188@gmail.com>
>
> Add an option in 'filter_refs()' to use 'for_each_branch_ref()'
> and filter refs. This type checking is done by adding a
> 'FILTER_REFS_BRANCHES' in 'ref-filter.h'.
>
> Add an option in 'ref_filter_handler()' to filter different
> types of branches by calling 'filter_branch_kind()' which
> checks for the type of branch needed.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  ref-filter.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  ref-filter.h | 10 ++++++++--
>  2 files changed, 55 insertions(+), 2 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index de84dd4..c573109 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1044,6 +1044,46 @@ static const unsigned char *match_points_at(struct sha1_array *points_at,
>  	return NULL;
>  }
>  
> +/*
> + * Checks if a given refname is a branch and returns the kind of
> + * branch it is. If not a branch, 0 is returned.
> + */
> +static int filter_branch_kind(struct ref_filter *filter, const char *refname)
> +{
> +	int kind, i;
> +
> +	static struct {
> +		const char *prefix;
> +		int kind;

Make a mental note that this is signed int.

> +	} ref_kind[] = {
> +		{ "refs/heads/" , REF_LOCAL_BRANCH },
> +		{ "refs/remotes/" , REF_REMOTE_BRANCH },
> +	};
> +
> +	/*  If no kind is specified, no need to filter */
> +	if (!filter->branch_kind)
> +		return REF_NO_BRANCH_FILTERING;
> +
> +	for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
> +		if (starts_with(refname, ref_kind[i].prefix)) {
> +			kind = ref_kind[i].kind;
> +			break;
> +		}
> +	}
> +
> +	if (ARRAY_SIZE(ref_kind) <= i) {
> +		if (!strcmp(refname, "HEAD"))
> +			kind = REF_DETACHED_HEAD;
> +		else
> +			return 0;
> +	}
> +
> +	if ((filter->branch_kind & kind) == 0)
> +		return 0;
> +
> +	return kind;
> +}

While this looks fine, I am not sure if this is a good interface for
filtering, though.

If you start from already having everything and want to limit things
down to "refs/heads/", this might make sense but it would be far
more efficient, if you know that you want to limit to "refs/heads/"
upfront, not to collect everything but just limit the collection to
those under "refs/heads/" without wasting cycles in the first place.

> diff --git a/ref-filter.h b/ref-filter.h
> index 5be3e35..b5a13e8 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -16,6 +16,12 @@
>  #define FILTER_REFS_INCLUDE_BROKEN 0x1
>  #define FILTER_REFS_ALL 0x2
>  #define FILTER_REFS_TAGS 0x4
> +#define FILTER_REFS_BRANCHES 0x8

Is this a sensible set of bits?  Does it make sense to have ALL and
TAGS at the same time (and what does that mean?)?

> +#define REF_DETACHED_HEAD   0x01
> +#define REF_LOCAL_BRANCH    0x02
> +#define REF_REMOTE_BRANCH   0x04
> +#define REF_NO_BRANCH_FILTERING 0x08

Where do these values go?  It is a returned by filter-branch-kind
for each ref, i.e. given "refs/heads/bar", it answers "Yeah, that is
a local branch".  Why are these values pretending to be a set of
bits that can be combined together, as if a branch can be both LOCAL
and REMOTE?  This does not make _any_ sense.


>  #define ALIGN_LEFT 0x01
>  #define ALIGN_RIGHT 0x02
> @@ -50,7 +56,7 @@ struct ref_sorting {
>  
>  struct ref_array_item {
>  	unsigned char objectname[20];
> -	int flag;
> +	int flag, kind;

For readability, do not define multiple structure fields on a single
line.

If you are storing a set of bits in an integer, use unsigned.  If it
is an enumeration, int is fine.

>  	const char *symref;
>  	struct commit *commit;
>  	struct atom_value *value;
> @@ -76,7 +82,7 @@ struct ref_filter {
>  
>  	unsigned int with_commit_tag_algo : 1,
>  		match_as_path : 1;
> -	unsigned int lines;
> +	unsigned int lines, branch_kind;

For readability, do not define multiple structure fields on a single
line.

>  };
>  
>  struct ref_filter_cbdata {