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

[Karthik Nayak <karthik.188@gmail.com>]

Re-sending this as it wasn't sent as plain text and failed.

On Tue, Jul 28, 2015 at 7:47 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> 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)

Because we call add_verbose_info() is called in print_ref_item() which calls
add_verbose_info() with refname=desc, where desc is a shortened refname.
And fill_tracking_info expects a shortened refname. hence we give it refname
which is nothing but desc.

>
>> @@ -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(...);
>
> ?
>

We have a ref_defaulting_sorting but that defaults to sorting by 'refname'
but what we want in branch.c is to sort by 'type' rather. Hence we
need to have this small segment of code. About its placement, IDK if
print_ref_list() is the right place. But didn't find a better place.

>> -     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)?
>

I guess it'll be converted to a helper for show_ref_array_item()
eventually so keeping that in mind maybe rename it to
get_format_and_print_ref().

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

Yeah, I'll add that into the commit message.

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

Well okay will rename the test description.
Since we use filter_refs() there's no real fix for this, ref_filter_handler()
skips over such refs. I'll mention something on these lines in the
commit message.

-- 
Regards,
Karthik Nayak