From patchwork Sun May 3 09:11:57 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 11524281 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 1BCA8139A for ; Sun, 3 May 2020 09:12:01 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id EFEA420787 for ; Sun, 3 May 2020 09:12:00 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727871AbgECJL7 (ORCPT ); Sun, 3 May 2020 05:11:59 -0400 Received: from cloud.peff.net ([104.130.231.41]:35004 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1727116AbgECJL7 (ORCPT ); Sun, 3 May 2020 05:11:59 -0400 Received: (qmail 21736 invoked by uid 109); 3 May 2020 09:11:58 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Sun, 03 May 2020 09:11:58 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 4791 invoked by uid 111); 3 May 2020 09:12:00 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Sun, 03 May 2020 05:12:00 -0400 Authentication-Results: peff.net; auth=none Date: Sun, 3 May 2020 05:11:57 -0400 From: Jeff King To: clime Cc: Git List Subject: [PATCH 1/2] ref-filter: apply --ignore-case to all sorting keys Message-ID: <20200503091157.GA170902@coredump.intra.peff.net> References: <20200503090952.GA170768@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200503090952.GA170768@coredump.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org All of the ref-filter users (for-each-ref, branch, and tag) take an --ignore-case option which makes filtering and sorting case-insensitive. However, this option was applied only to the first element of the ref_sorting list. So: git for-each-ref --ignore-case --sort=refname would do what you expect, but: git for-each-ref --ignore-case --sort=refname --sort=taggername would sort the primary key (taggername) case-insensitively, but sort the refname case-sensitively. We have two options here: - teach callers to set ignore_case on the whole list - replace the ref_sorting list with a struct that contains both the list of sorting keys, as well as options that apply to _all_ keys I went with the first one here, as it gives more flexibility if we later want to let the users set the flag per-key (presumably through some special syntax when defining the key; for now it's all or nothing through --ignore-case). The new test covers this by sorting on both tagger and subject case-insensitively, which should compare "a" and "A" identically, but still sort them before "b" and "B". We'll break ties by sorting on the refname to give ourselves a stable output (this is actually supposed to be done automatically, but there's another bug which will be fixed in the next commit). Signed-off-by: Jeff King --- builtin/branch.c | 2 +- builtin/for-each-ref.c | 2 +- builtin/tag.c | 2 +- ref-filter.c | 6 ++++++ ref-filter.h | 2 ++ t/t6300-for-each-ref.sh | 40 ++++++++++++++++++++++++++++++++++++++++ 6 files changed, 51 insertions(+), 3 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index d8297f80ff..86341cc835 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -739,7 +739,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) */ if (!sorting) sorting = ref_default_sorting(); - sorting->ignore_case = icase; + ref_sorting_icase_all(sorting, icase); print_ref_list(&filter, sorting, &format); print_columns(&output, colopts, NULL); string_list_clear(&output, 0); diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index 465153e853..57489e4eab 100644 --- a/builtin/for-each-ref.c +++ b/builtin/for-each-ref.c @@ -70,7 +70,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) if (!sorting) sorting = ref_default_sorting(); - sorting->ignore_case = icase; + ref_sorting_icase_all(sorting, icase); filter.ignore_case = icase; filter.name_patterns = argv; diff --git a/builtin/tag.c b/builtin/tag.c index dd160b49c7..ff7610b5c8 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -485,7 +485,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix) } if (!sorting) sorting = ref_default_sorting(); - sorting->ignore_case = icase; + ref_sorting_icase_all(sorting, icase); filter.ignore_case = icase; if (cmdmode == 'l') { int ret; diff --git a/ref-filter.c b/ref-filter.c index 35776838f4..bdb3535ce5 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -2317,6 +2317,12 @@ static int compare_refs(const void *a_, const void *b_, void *ref_sorting) return 0; } +void ref_sorting_icase_all(struct ref_sorting *sorting, int flag) +{ + for (; sorting; sorting = sorting->next) + sorting->ignore_case = !!flag; +} + void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array) { QSORT_S(array->items, array->nr, compare_refs, sorting); diff --git a/ref-filter.h b/ref-filter.h index 64330e9601..8ecc33cdfa 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -114,6 +114,8 @@ void ref_array_clear(struct ref_array *array); int verify_ref_format(struct ref_format *format); /* Sort the given ref_array as per the ref_sorting provided */ void ref_array_sort(struct ref_sorting *sort, struct ref_array *array); +/* Set the ignore_case flag for all elements of a sorting list */ +void ref_sorting_icase_all(struct ref_sorting *sorting, int flag); /* Based on the given format and quote_style, fill the strbuf */ int format_ref_array_item(struct ref_array_item *info, const struct ref_format *format, diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index b3c1092338..c9caf26327 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -895,4 +895,44 @@ test_expect_success 'for-each-ref --ignore-case ignores case' ' test_cmp expect actual ' +test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' ' + # name refs numerically to avoid case-insensitive filesystem conflicts + nr=0 && + for email in a A b B + do + for subject in a A b B + do + GIT_COMMITTER_EMAIL="$email@example.com" \ + git tag -m "tag $subject" icase-$(printf %02d $nr) && + nr=$((nr+1))|| + return 1 + done + done && + git for-each-ref --ignore-case \ + --format="%(taggeremail) %(subject) %(refname)" \ + --sort=refname \ + --sort=subject \ + --sort=taggeremail \ + refs/tags/icase-* >actual && + cat >expect <<-\EOF && + tag a refs/tags/icase-00 + tag A refs/tags/icase-01 + tag a refs/tags/icase-04 + tag A refs/tags/icase-05 + tag b refs/tags/icase-02 + tag B refs/tags/icase-03 + tag b refs/tags/icase-06 + tag B refs/tags/icase-07 + tag a refs/tags/icase-08 + tag A refs/tags/icase-09 + tag a refs/tags/icase-12 + tag A refs/tags/icase-13 + tag b refs/tags/icase-10 + tag B refs/tags/icase-11 + tag b refs/tags/icase-14 + tag B refs/tags/icase-15 + EOF + test_cmp expect actual +' + test_done From patchwork Sun May 3 09:13:09 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 11524283 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 8E938913 for ; Sun, 3 May 2020 09:13:12 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 7AAD620721 for ; Sun, 3 May 2020 09:13:12 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727845AbgECJNL (ORCPT ); Sun, 3 May 2020 05:13:11 -0400 Received: from cloud.peff.net ([104.130.231.41]:35012 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1727116AbgECJNL (ORCPT ); Sun, 3 May 2020 05:13:11 -0400 Received: (qmail 21759 invoked by uid 109); 3 May 2020 09:13:10 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Sun, 03 May 2020 09:13:10 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 4815 invoked by uid 111); 3 May 2020 09:13:12 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Sun, 03 May 2020 05:13:12 -0400 Authentication-Results: peff.net; auth=none Date: Sun, 3 May 2020 05:13:09 -0400 From: Jeff King To: clime Cc: Git List Subject: [PATCH 2/2] ref-filter: apply fallback refname sort only after all user sorts Message-ID: <20200503091309.GB170902@coredump.intra.peff.net> References: <20200503090952.GA170768@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200503090952.GA170768@coredump.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 --- ref-filter.c | 7 ++++-- t/t6300-for-each-ref.sh | 54 ++++++++++++++++++++++++++++++++++++----- 2 files changed, 53 insertions(+), 8 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index bdb3535ce5..bf7b70299b 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -2295,7 +2295,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; } @@ -2314,7 +2314,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 c9caf26327..da59fadc5d 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -650,17 +650,59 @@ test_atom refs/tags/signed-long contents "subject line body contents $sig" -sort >expected < refs/tags/bogo -$(git rev-parse refs/tags/master) 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 refs/tags/multi-ref2-100000-user1 + 100000 refs/tags/multi-ref1-100000-user1 + 100000 refs/tags/multi-ref2-100000-user2 + 100000 refs/tags/multi-ref1-100000-user2 + 200000 refs/tags/multi-ref2-200000-user1 + 200000 refs/tags/multi-ref1-200000-user1 + 200000 refs/tags/multi-ref2-200000-user2 + 200000 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 refs/tags/multi-ref1-100000-user1 + 100000 refs/tags/multi-ref1-100000-user2 + 100000 refs/tags/multi-ref2-100000-user1 + 100000 refs/tags/multi-ref2-100000-user2 + 200000 refs/tags/multi-ref1-200000-user1 + 200000 refs/tags/multi-ref1-200000-user2 + 200000 refs/tags/multi-ref2-200000-user1 + 200000 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" &&