Re: [PATCH v5 7/8] branch.c: use 'ref-filter' APIs

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

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

> --- a/Documentation/git-branch.txt
> +++ b/Documentation/git-branch.txt
> @@ -231,6 +231,13 @@ start-point is either a local or remote-tracking branch.
>  	The new name for an existing branch. The same restrictions as for
>  	<branchname> apply.
>  
> +--sort=<key>::
> +	Sort based on the key given. Prefix `-` to sort in descending
> +	order of the value. You may use the --sort=<key> option
> +	multiple times, in which case the last key becomes the primary
> +	key. The keys supported are the same as those in `git
> +	for-each-ref`. Sort order defaults to sorting based on branch
> +	type.

The last sentence is no longer true. Perhaps something like:

Sort order defaults to sorting based on the full refname (including
`refs/...` prefix). This lists detached HEAD (if present) first, then
local branches and finally remote-tracking branches.

> -static void print_ref_list(struct ref_filter *filter)
> +static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting)
>  {
>  	int i;
>  	struct ref_array array;
> -	struct ref_filter_cbdata data;
>  	int maxwidth = 0;
>  	const char *remote_prefix = "";
> -	struct rev_info revs;
> +	struct ref_sorting def_sorting;
> +	const char *sort_type = "refname";

You are using refname without special-casing HEAD at all. So, this is
relying on the fact that HEAD comes alphabetically before refs/..., and
that we are only listing refs/... and HEAD.

As I mentionned earlyer, if we ever allow branch to list e.g.
FETCH_HEAD, then the detached HEAD will come in the middle. I first
thought that this was too fragile, but after thinking about it, I think
this is actually sensible. After all, if we ever allow FETCH_HEAD, then
keeping the alphabetical order still makes sense.

So, your code is OK, but a bit too subtle to my taste: you should add a
comment explaining why sorting according to refname is sufficient (I
would use a comment, but a better commit message may be OK too).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/