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

Karthik Nayak [off-list ref] writes:

On Tue, Jul 28, 2015 at 7:47 PM, Matthieu Moy
[off-list ref] wrote:

quoted

quoted

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

quoted

quoted

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