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:

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

Hmm, actually I think I misread the code: print_ref_list() does follow
the pattern filter_refs -> sort -> print.

Perhaps the function name is misleading, and you could make it clearer
that it computes the list and displays it. I don't know.

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

Either there's a good rationale for ignoring these refs, and you should
change the test keeping it "test_expect_success" (i.e. "the test
observed corresponds to the expected behavior"), or you should
eventually modify filter_refs() to allow not dropping them.

This

-test_expect_success '...'
+test_expect_failure '...'

explicitly says "I have a known regression in this patch".

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