ref-filter: apply fallback refname sort only after all user sorts

Commit 9e468334b4 (ref-filter: fallback on alphabetical comparison,
2015-10-30) taught ref-filter's sort to fallback to comparing refnames.
But it did it at the wrong level, overriding the comparison result for a
single "--sort" key from the user, rather than after all sort keys have
been exhausted.

This worked correctly for a single "--sort" option, but not for multiple
ones. We'd break any ties in the first key with the refname and never
evaluate the second key at all.

To make matters even more interesting, we only applied this fallback
sometimes! For a field like "taggeremail" which requires a string
comparison, we'd truly return the result of strcmp(), even if it was 0.
But for numerical "value" fields like "taggerdate", we did apply the
fallback. And that's why our multiple-sort test missed this: it uses
taggeremail as the main comparison.

So let's start by adding a much more rigorous test. We'll have a set of
commits expressing every combination of two tagger emails, dates, and
refnames. Then we can confirm that our sort is applied with the correct
precedence, and we'll be hitting both the string and value comparators.

That does show the bug, and the fix is simple: moving the fallback to
the outer compare_refs() function, after all ref_sorting keys have been
exhausted.

Note that in the outer function we don't have an "ignore_case" flag, as
it's part of each individual ref_sorting element. It's debatable what
such a fallback should do, since we didn't use the user's keys to match.
But until now we have been trying to respect that flag, so the
least-invasive thing is to try to continue to do so. Since all callers
in the current code either set the flag for all keys or for none, we can
just pull the flag from the first key. In a hypothetical world where the
user really can flip the case-insensitivity of keys separately, we may
want to extend the code to distinguish that case from a blanket
"--ignore-case".

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/ref-filter.c b/ref-filter.c
index 05a62914ca3d42dd53b8462ffd48705d03f6ec29..2dc046ffb689934ac9b557aebd6b65b117265f79 100644 (file)
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2141,7 +2141,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
                if (va->value < vb->value)
                        cmp = -1;
                else if (va->value == vb->value)
-                       cmp = cmp_fn(a->refname, b->refname);
+                       cmp = 0;
                else
                        cmp = 1;
        }
@@ -2160,7 +2160,10 @@ static int compare_refs(const void *a_, const void *b_, void *ref_sorting)
                if (cmp)
                        return cmp;
        }
-       return 0;
+       s = ref_sorting;
+       return s && s->ignore_case ?
+               strcasecmp(a->refname, b->refname) :
+               strcmp(a->refname, b->refname);
 }
 
 void ref_sorting_icase_all(struct ref_sorting *sorting, int flag)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index b347bba0870d3825d8426e2057bb2ed74425f6c1..abf34d6e8e3e23e3d0e96eead87fda086d795b27 100755 (executable)
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -583,17 +583,59 @@ test_atom refs/tags/signed-long contents "subject line
 body contents
 $sig"
 
-cat >expected <<EOF
-$(git rev-parse refs/tags/bogo) <committer@example.com> refs/tags/bogo
-$(git rev-parse refs/tags/master) <committer@example.com> refs/tags/master
-EOF
+test_expect_success 'set up multiple-sort tags' '
+       for when in 100000 200000
+       do
+               for email in user1 user2
+               do
+                       for ref in ref1 ref2
+                       do
+                               GIT_COMMITTER_DATE="@$when +0000" \
+                               GIT_COMMITTER_EMAIL="$email@example.com" \
+                               git tag -m "tag $ref-$when-$email" \
+                               multi-$ref-$when-$email || return 1
+                       done
+               done
+       done
+'
 
 test_expect_success 'Verify sort with multiple keys' '
-       git for-each-ref --format="%(objectname) %(taggeremail) %(refname)" --sort=objectname --sort=taggeremail \
-               refs/tags/bogo refs/tags/master > actual &&
+       cat >expected <<-\EOF &&
+       100000 <user1@example.com> refs/tags/multi-ref2-100000-user1
+       100000 <user1@example.com> refs/tags/multi-ref1-100000-user1
+       100000 <user2@example.com> refs/tags/multi-ref2-100000-user2
+       100000 <user2@example.com> refs/tags/multi-ref1-100000-user2
+       200000 <user1@example.com> refs/tags/multi-ref2-200000-user1
+       200000 <user1@example.com> refs/tags/multi-ref1-200000-user1
+       200000 <user2@example.com> refs/tags/multi-ref2-200000-user2
+       200000 <user2@example.com> refs/tags/multi-ref1-200000-user2
+       EOF
+       git for-each-ref \
+               --format="%(taggerdate:unix) %(taggeremail) %(refname)" \
+               --sort=-refname \
+               --sort=taggeremail \
+               --sort=taggerdate \
+               "refs/tags/multi-*" >actual &&
        test_cmp expected actual
 '
 
+test_expect_success 'equivalent sorts fall back on refname' '
+       cat >expected <<-\EOF &&
+       100000 <user1@example.com> refs/tags/multi-ref1-100000-user1
+       100000 <user2@example.com> refs/tags/multi-ref1-100000-user2
+       100000 <user1@example.com> refs/tags/multi-ref2-100000-user1
+       100000 <user2@example.com> refs/tags/multi-ref2-100000-user2
+       200000 <user1@example.com> refs/tags/multi-ref1-200000-user1
+       200000 <user2@example.com> refs/tags/multi-ref1-200000-user2
+       200000 <user1@example.com> refs/tags/multi-ref2-200000-user1
+       200000 <user2@example.com> refs/tags/multi-ref2-200000-user2
+       EOF
+       git for-each-ref \
+               --format="%(taggerdate:unix) %(taggeremail) %(refname)" \
+               --sort=taggerdate \
+               "refs/tags/multi-*" >actual &&
+       test_cmp expected actual
+'
 
 test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' '
        test_when_finished "git checkout master" &&