Karthik Nayak <karthik.188@xxxxxxxxx> writes:
> On Tue, Jul 28, 2015 at 7:47 PM, Matthieu Moy
> <Matthieu.Moy@xxxxxxxxxxxxxxx> 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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html