Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs

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

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

> @@ -458,7 +345,7 @@ static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,
>  	}
>  
>  	if (item->kind == REF_LOCAL_BRANCH)
> -		fill_tracking_info(&stat, item->refname, filter->verbose > 1);
> +		fill_tracking_info(&stat, refname, filter->verbose > 1);

Why can't you continue using item->refname?

(It's a real question)

> @@ -635,14 +495,21 @@ static void print_ref_list(struct ref_filter *filter)
>  	/* Print detached heads before sorting and printing the rest */
>  	if (filter->detached) {
>  		print_ref_item(array.items[index - 1], maxwidth, filter, remote_prefix);
> -		index -= 1;
> +		array.nr--;
>  	}
>  
> -	qsort(array.items, index, sizeof(struct ref_array_item *), ref_cmp);
> +	if (!sorting) {
> +		def_sorting.next = NULL;
> +		def_sorting.atom = parse_ref_filter_atom(sort_type,
> +							 sort_type + strlen(sort_type));
> +		sorting = &def_sorting;
> +	}
> +	ref_array_sort(sorting, &array);

Does this belong to print_ref_list()? Is it not possible to extract it
to get a code closer to the simple:

	filter_refs(...);
	ref_array_sort(...);
	print_ref_list(...);

?

> -	for (i = 0; i < index; i++)
> +	for (i = 0; i < array.nr; i++)
>  		print_ref_item(array.items[i], maxwidth, filter, remote_prefix);

Now that we have show_ref_array_item, it may make sense to rename
print_ref_item to something that make the difference between these
functions more explicit. Well, ideally, you'd get rid of it an actually
use show_ref_array_item, but if you are to keep it, maybe
print_ref_item_default_branch_format (or something shorter)?

> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -49,7 +49,6 @@ struct ref_sorting {
>  struct ref_array_item {
>  	unsigned char objectname[20];
>  	int flag, kind;
> -	int ignore : 1;

You should explain why you needed it and why you don't need it anymore
(I guess, because it was used to implement --merge and you now get it
from ref-filter).

> --- a/t/t1430-bad-ref-name.sh
> +++ b/t/t1430-bad-ref-name.sh
> @@ -38,7 +38,7 @@ test_expect_success 'fast-import: fail on invalid branch name "bad[branch]name"'
>  	test_must_fail git fast-import <input
>  '
>  
> -test_expect_success 'git branch shows badly named ref' '
> +test_expect_failure 'git branch does not show badly named ref' '

I'm not sure what's the convention, but I think the test description
should give the expected behavior even with test_expect_failure.

And please help the reviewers by saying what's the status wrt this test
(any plan on how to fix it?).

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