{"thread":{"id":"53378","subject":"git for-each-ref - sorting by multiple keys","startedAt":"2020-05-02T20:32:05Z","lastAt":"2020-05-05T21:30:57Z","messageCount":21,"participants":["clime","Jeff King","Danh Doan","Junio C Hamano","Carlo Marcelo Arenas Belón","Taylor Blau"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"396845","messageId":"CAGqZTUvuJgZH-YUxDTYunng3QD4-wwPgquZX1P=1P1R=Ku0s2g@mail.gmail.com","threadId":"53378","inReplyTo":null,"subject":"git for-each-ref - sorting by multiple keys","fromName":"clime","fromEmail":"clime7@gmail.com","sentAt":"2020-05-02T20:31:50Z","receivedAt":"2020-05-02T20:32:05Z","isPatch":false,"sender":{"key":"clime7@gmail.com","avatar":"https://gravatar.com/avatar/d99ae46a5b0f57415bb74fc6c8ecd7eec304ff769f298c3be00fabf961b215ff?d=mp&s=160"},"body":"Hello,\n\nI have the following command:\n\n/usr/bin/git for-each-ref --merged=\"${GIT_HEAD-HEAD}\"\n--sort='-taggerdate' --sort='-*committerdate'\n--format=\"%(*committerdate)|%(taggerdate)|%(tag)\" refs/tags\n\nI thought this will use: -*commiterdate as a primary key and\n-taggerdate as a secondary. According to man page for --sort: \"You may\nuse the --sort=<key> option multiple times, in which case the last key\nbecomes the primary key.\"\n\nBut that doesn't seem to be the case. I created a repo with a single\ncommit and created annotated tags on the commit in the following\norder:\n\n$ git tag -a -m \"foo\" B\n$ git tag -a -m \"foo\" C\n$ git tag -a -m \"foo\" A\n\nYet the order I am getting after running the command is:\n\nSat May 2 22:10:30 2020 +0200|Sat May 2 22:14:49 2020 +0200|C\nSat May 2 22:10:30 2020 +0200|Sat May 2 22:14:45 2020 +0200|B\nSat May 2 22:10:30 2020 +0200|Sat May 2 22:14:51 2020 +0200|A\n\nIs it a mistake in man pages? Is there any way to sort by multiple keys?\n\nThank you\nclime\n"},{"id":"396860","messageId":"20200503090952.GA170768@coredump.intra.peff.net","threadId":"53378","inReplyTo":"CAGqZTUvuJgZH-YUxDTYunng3QD4-wwPgquZX1P=1P1R=Ku0s2g@mail.gmail.com","subject":"Re: git for-each-ref - sorting by multiple keys","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-03T09:09:52Z","receivedAt":"2020-05-03T09:09:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 02, 2020 at 10:31:50PM +0200, clime wrote:\n\n> I have the following command:\n> \n> /usr/bin/git for-each-ref --merged=\"${GIT_HEAD-HEAD}\"\n> --sort='-taggerdate' --sort='-*committerdate'\n> --format=\"%(*committerdate)|%(taggerdate)|%(tag)\" refs/tags\n> \n> I thought this will use: -*commiterdate as a primary key and\n> -taggerdate as a secondary. According to man page for --sort: \"You may\n> use the --sort=<key> option multiple times, in which case the last key\n> becomes the primary key.\"\n> \n> But that doesn't seem to be the case. I created a repo with a single\n> commit and created annotated tags on the commit in the following\n> order:\n\nIt looks like this has been quite broken since 2015 and nobody noticed. :(\n\nThanks for your reproduction recipe; using times is a critical part of\nthe bug. Here's a fix, plus a fix for a related bug I noticed while\nworking on it (the second one fixes your bug).\n\n  [1/2]: ref-filter: apply --ignore-case to all sorting keys\n  [2/2]: ref-filter: apply fallback refname sort only after all user sorts\n\n builtin/branch.c        |  2 +-\n builtin/for-each-ref.c  |  2 +-\n builtin/tag.c           |  2 +-\n ref-filter.c            | 13 +++++-\n ref-filter.h            |  2 +\n t/t6300-for-each-ref.sh | 94 ++++++++++++++++++++++++++++++++++++++---\n 6 files changed, 104 insertions(+), 11 deletions(-)\n\n-Peff\n"},{"id":"396861","messageId":"20200503091157.GA170902@coredump.intra.peff.net","threadId":"53378","inReplyTo":"20200503090952.GA170768@coredump.intra.peff.net","subject":"[PATCH 1/2] ref-filter: apply --ignore-case to all sorting keys","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-03T09:11:57Z","receivedAt":"2020-05-03T09:12:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"All of the ref-filter users (for-each-ref, branch, and tag) take an\n--ignore-case option which makes filtering and sorting case-insensitive.\nHowever, this option was applied only to the first element of the\nref_sorting list. So:\n\n  git for-each-ref --ignore-case --sort=refname\n\nwould do what you expect, but:\n\n  git for-each-ref --ignore-case --sort=refname --sort=taggername\n\nwould sort the primary key (taggername) case-insensitively, but sort the\nrefname case-sensitively. We have two options here:\n\n  - teach callers to set ignore_case on the whole list\n\n  - replace the ref_sorting list with a struct that contains both the\n    list of sorting keys, as well as options that apply to _all_\n    keys\n\nI went with the first one here, as it gives more flexibility if we later\nwant to let the users set the flag per-key (presumably through some\nspecial syntax when defining the key; for now it's all or nothing\nthrough --ignore-case).\n\nThe new test covers this by sorting on both tagger and subject\ncase-insensitively, which should compare \"a\" and \"A\" identically, but\nstill sort them before \"b\" and \"B\". We'll break ties by sorting on the\nrefname to give ourselves a stable output (this is actually supposed to\nbe done automatically, but there's another bug which will be fixed in\nthe next commit).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/branch.c        |  2 +-\n builtin/for-each-ref.c  |  2 +-\n builtin/tag.c           |  2 +-\n ref-filter.c            |  6 ++++++\n ref-filter.h            |  2 ++\n t/t6300-for-each-ref.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 51 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex d8297f80ff..86341cc835 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -739,7 +739,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t */\n \t\tif (!sorting)\n \t\t\tsorting = ref_default_sorting();\n-\t\tsorting->ignore_case = icase;\n+\t\tref_sorting_icase_all(sorting, icase);\n \t\tprint_ref_list(&filter, sorting, &format);\n \t\tprint_columns(&output, colopts, NULL);\n \t\tstring_list_clear(&output, 0);\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 465153e853..57489e4eab 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -70,7 +70,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \n \tif (!sorting)\n \t\tsorting = ref_default_sorting();\n-\tsorting->ignore_case = icase;\n+\tref_sorting_icase_all(sorting, icase);\n \tfilter.ignore_case = icase;\n \n \tfilter.name_patterns = argv;\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex dd160b49c7..ff7610b5c8 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -485,7 +485,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t}\n \tif (!sorting)\n \t\tsorting = ref_default_sorting();\n-\tsorting->ignore_case = icase;\n+\tref_sorting_icase_all(sorting, icase);\n \tfilter.ignore_case = icase;\n \tif (cmdmode == 'l') {\n \t\tint ret;\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 35776838f4..bdb3535ce5 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2317,6 +2317,12 @@ static int compare_refs(const void *a_, const void *b_, void *ref_sorting)\n \treturn 0;\n }\n \n+void ref_sorting_icase_all(struct ref_sorting *sorting, int flag)\n+{\n+\tfor (; sorting; sorting = sorting->next)\n+\t\tsorting->ignore_case = !!flag;\n+}\n+\n void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)\n {\n \tQSORT_S(array->items, array->nr, compare_refs, sorting);\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 64330e9601..8ecc33cdfa 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -114,6 +114,8 @@ void ref_array_clear(struct ref_array *array);\n int verify_ref_format(struct ref_format *format);\n /*  Sort the given ref_array as per the ref_sorting provided */\n void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);\n+/*  Set the ignore_case flag for all elements of a sorting list */\n+void ref_sorting_icase_all(struct ref_sorting *sorting, int flag);\n /*  Based on the given format and quote_style, fill the strbuf */\n int format_ref_array_item(struct ref_array_item *info,\n \t\t\t  const struct ref_format *format,\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex b3c1092338..c9caf26327 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -895,4 +895,44 @@ test_expect_success 'for-each-ref --ignore-case ignores case' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' '\n+\t# name refs numerically to avoid case-insensitive filesystem conflicts\n+\tnr=0 &&\n+\tfor email in a A b B\n+\tdo\n+\t\tfor subject in a A b B\n+\t\tdo\n+\t\t\tGIT_COMMITTER_EMAIL=\"$email@example.com\" \\\n+\t\t\tgit tag -m \"tag $subject\" icase-$(printf %02d $nr) &&\n+\t\t\tnr=$((nr+1))||\n+\t\t\treturn 1\n+\t\tdone\n+\tdone &&\n+\tgit for-each-ref --ignore-case \\\n+\t\t--format=\"%(taggeremail) %(subject) %(refname)\" \\\n+\t\t--sort=refname \\\n+\t\t--sort=subject \\\n+\t\t--sort=taggeremail \\\n+\t\trefs/tags/icase-* >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t<a@example.com> tag a refs/tags/icase-00\n+\t<a@example.com> tag A refs/tags/icase-01\n+\t<A@example.com> tag a refs/tags/icase-04\n+\t<A@example.com> tag A refs/tags/icase-05\n+\t<a@example.com> tag b refs/tags/icase-02\n+\t<a@example.com> tag B refs/tags/icase-03\n+\t<A@example.com> tag b refs/tags/icase-06\n+\t<A@example.com> tag B refs/tags/icase-07\n+\t<b@example.com> tag a refs/tags/icase-08\n+\t<b@example.com> tag A refs/tags/icase-09\n+\t<B@example.com> tag a refs/tags/icase-12\n+\t<B@example.com> tag A refs/tags/icase-13\n+\t<b@example.com> tag b refs/tags/icase-10\n+\t<b@example.com> tag B refs/tags/icase-11\n+\t<B@example.com> tag b refs/tags/icase-14\n+\t<B@example.com> tag B refs/tags/icase-15\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.26.2.957.g6dc93e954a\n\n"},{"id":"396862","messageId":"20200503091309.GB170902@coredump.intra.peff.net","threadId":"53378","inReplyTo":"20200503090952.GA170768@coredump.intra.peff.net","subject":"[PATCH 2/2] ref-filter: apply fallback refname sort only after all user sorts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-03T09:13:09Z","receivedAt":"2020-05-03T09:13:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 9e468334b4 (ref-filter: fallback on alphabetical comparison,\n2015-10-30) taught ref-filter's sort to fallback to comparing refnames.\nBut it did it at the wrong level, overriding the comparison result for a\nsingle \"--sort\" key from the user, rather than after all sort keys have\nbeen exhausted.\n\nThis worked correctly for a single \"--sort\" option, but not for multiple\nones. We'd break any ties in the first key with the refname and never\nevaluate the second key at all.\n\nTo make matters even more interesting, we only applied this fallback\nsometimes! For a field like \"taggeremail\" which requires a string\ncomparison, we'd truly return the result of strcmp(), even if it was 0.\nBut for numerical \"value\" fields like \"taggerdate\", we did apply the\nfallback. And that's why our multiple-sort test missed this: it uses\ntaggeremail as the main comparison.\n\nSo let's start by adding a much more rigorous test. We'll have a set of\ncommits expressing every combination of two tagger emails, dates, and\nrefnames. Then we can confirm that our sort is applied with the correct\nprecedence, and we'll be hitting both the string and value comparators.\n\nThat does show the bug, and the fix is simple: moving the fallback to\nthe outer compare_refs() function, after all ref_sorting keys have been\nexhausted.\n\nNote that in the outer function we don't have an \"ignore_case\" flag, as\nit's part of each individual ref_sorting element. It's debatable what\nsuch a fallback should do, since we didn't use the user's keys to match.\nBut until now we have been trying to respect that flag, so the\nleast-invasive thing is to try to continue to do so. Since all callers\nin the current code either set the flag for all keys or for none, we can\njust pull the flag from the first key. In a hypothetical world where the\nuser really can flip the case-insensitivity of keys separately, we may\nwant to extend the code to distinguish that case from a blanket\n\"--ignore-case\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n ref-filter.c            |  7 ++++--\n t/t6300-for-each-ref.sh | 54 ++++++++++++++++++++++++++++++++++++-----\n 2 files changed, 53 insertions(+), 8 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex bdb3535ce5..bf7b70299b 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2295,7 +2295,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n \t\tif (va->value < vb->value)\n \t\t\tcmp = -1;\n \t\telse if (va->value == vb->value)\n-\t\t\tcmp = cmp_fn(a->refname, b->refname);\n+\t\t\tcmp = 0;\n \t\telse\n \t\t\tcmp = 1;\n \t}\n@@ -2314,7 +2314,10 @@ static int compare_refs(const void *a_, const void *b_, void *ref_sorting)\n \t\tif (cmp)\n \t\t\treturn cmp;\n \t}\n-\treturn 0;\n+\ts = ref_sorting;\n+\treturn s && s->ignore_case ?\n+\t\tstrcasecmp(a->refname, b->refname) :\n+\t\tstrcmp(a->refname, b->refname);\n }\n \n void ref_sorting_icase_all(struct ref_sorting *sorting, int flag)\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex c9caf26327..da59fadc5d 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -650,17 +650,59 @@ test_atom refs/tags/signed-long contents \"subject line\n body contents\n $sig\"\n \n-sort >expected <<EOF\n-$(git rev-parse refs/tags/bogo) <committer@example.com> refs/tags/bogo\n-$(git rev-parse refs/tags/master) <committer@example.com> refs/tags/master\n-EOF\n+test_expect_success 'set up multiple-sort tags' '\n+\tfor when in 100000 200000\n+\tdo\n+\t\tfor email in user1 user2\n+\t\tdo\n+\t\t\tfor ref in ref1 ref2\n+\t\t\tdo\n+\t\t\t\tGIT_COMMITTER_DATE=\"@$when +0000\" \\\n+\t\t\t\tGIT_COMMITTER_EMAIL=\"$email@example.com\" \\\n+\t\t\t\tgit tag -m \"tag $ref-$when-$email\" \\\n+\t\t\t\tmulti-$ref-$when-$email || return 1\n+\t\t\tdone\n+\t\tdone\n+\tdone\n+'\n \n test_expect_success 'Verify sort with multiple keys' '\n-\tgit for-each-ref --format=\"%(objectname) %(taggeremail) %(refname)\" --sort=objectname --sort=taggeremail \\\n-\t\trefs/tags/bogo refs/tags/master > actual &&\n+\tcat >expected <<-\\EOF &&\n+\t100000 <user1@example.com> refs/tags/multi-ref2-100000-user1\n+\t100000 <user1@example.com> refs/tags/multi-ref1-100000-user1\n+\t100000 <user2@example.com> refs/tags/multi-ref2-100000-user2\n+\t100000 <user2@example.com> refs/tags/multi-ref1-100000-user2\n+\t200000 <user1@example.com> refs/tags/multi-ref2-200000-user1\n+\t200000 <user1@example.com> refs/tags/multi-ref1-200000-user1\n+\t200000 <user2@example.com> refs/tags/multi-ref2-200000-user2\n+\t200000 <user2@example.com> refs/tags/multi-ref1-200000-user2\n+\tEOF\n+\tgit for-each-ref \\\n+\t\t--format=\"%(taggerdate:unix) %(taggeremail) %(refname)\" \\\n+\t\t--sort=-refname \\\n+\t\t--sort=taggeremail \\\n+\t\t--sort=taggerdate \\\n+\t\t\"refs/tags/multi-*\" >actual &&\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'equivalent sorts fall back on refname' '\n+\tcat >expected <<-\\EOF &&\n+\t100000 <user1@example.com> refs/tags/multi-ref1-100000-user1\n+\t100000 <user2@example.com> refs/tags/multi-ref1-100000-user2\n+\t100000 <user1@example.com> refs/tags/multi-ref2-100000-user1\n+\t100000 <user2@example.com> refs/tags/multi-ref2-100000-user2\n+\t200000 <user1@example.com> refs/tags/multi-ref1-200000-user1\n+\t200000 <user2@example.com> refs/tags/multi-ref1-200000-user2\n+\t200000 <user1@example.com> refs/tags/multi-ref2-200000-user1\n+\t200000 <user2@example.com> refs/tags/multi-ref2-200000-user2\n+\tEOF\n+\tgit for-each-ref \\\n+\t\t--format=\"%(taggerdate:unix) %(taggeremail) %(refname)\" \\\n+\t\t--sort=taggerdate \\\n+\t\t\"refs/tags/multi-*\" >actual &&\n+\ttest_cmp expected actual\n+'\n \n test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' '\n \ttest_when_finished \"git checkout master\" &&\n-- \n2.26.2.957.g6dc93e954a\n"},{"id":"396869","messageId":"CAGqZTUvaiDQbiQ1dOoqLcy+GHZg+BuXY=Z+S=Dpsq=wm44dGaQ@mail.gmail.com","threadId":"53378","inReplyTo":"20200503090952.GA170768@coredump.intra.peff.net","subject":"Re: git for-each-ref - sorting by multiple keys","fromName":"clime","fromEmail":"clime7@gmail.com","sentAt":"2020-05-03T10:16:44Z","receivedAt":"2020-05-03T10:16:58Z","isPatch":false,"sender":{"key":"clime7@gmail.com","avatar":"https://gravatar.com/avatar/d99ae46a5b0f57415bb74fc6c8ecd7eec304ff769f298c3be00fabf961b215ff?d=mp&s=160"},"body":"Awesome, thanks!\n\nOn Sun, 3 May 2020 at 11:09, Jeff King <peff@peff.net> wrote:\n>\n> On Sat, May 02, 2020 at 10:31:50PM +0200, clime wrote:\n>\n> > I have the following command:\n> >\n> > /usr/bin/git for-each-ref --merged=\"${GIT_HEAD-HEAD}\"\n> > --sort='-taggerdate' --sort='-*committerdate'\n> > --format=\"%(*committerdate)|%(taggerdate)|%(tag)\" refs/tags\n> >\n> > I thought this will use: -*commiterdate as a primary key and\n> > -taggerdate as a secondary. According to man page for --sort: \"You may\n> > use the --sort=<key> option multiple times, in which case the last key\n> > becomes the primary key.\"\n> >\n> > But that doesn't seem to be the case. I created a repo with a single\n> > commit and created annotated tags on the commit in the following\n> > order:\n>\n> It looks like this has been quite broken since 2015 and nobody noticed. :(\n>\n> Thanks for your reproduction recipe; using times is a critical part of\n> the bug. Here's a fix, plus a fix for a related bug I noticed while\n> working on it (the second one fixes your bug).\n>\n>   [1/2]: ref-filter: apply --ignore-case to all sorting keys\n>   [2/2]: ref-filter: apply fallback refname sort only after all user sorts\n>\n>  builtin/branch.c        |  2 +-\n>  builtin/for-each-ref.c  |  2 +-\n>  builtin/tag.c           |  2 +-\n>  ref-filter.c            | 13 +++++-\n>  ref-filter.h            |  2 +\n>  t/t6300-for-each-ref.sh | 94 ++++++++++++++++++++++++++++++++++++++---\n>  6 files changed, 104 insertions(+), 11 deletions(-)\n>\n> -Peff\n"},{"id":"396870","messageId":"20200503114351.GA28680@danh.dev","threadId":"53378","inReplyTo":"20200503091157.GA170902@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] ref-filter: apply --ignore-case to all sorting keys","fromName":"Danh Doan","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-05-03T11:44:02Z","receivedAt":"2020-05-03T11:44:07Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-05-03 05:11:57-0400, Jeff King <peff@peff.net> wrote:\n> +test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' '\n> +\t# name refs numerically to avoid case-insensitive filesystem conflicts\n> +\tnr=0 &&\n> +\tfor email in a A b B\n> +\tdo\n> +\t\tfor subject in a A b B\n> +\t\tdo\n> +\t\t\tGIT_COMMITTER_EMAIL=\"$email@example.com\" \\\n> +\t\t\tgit tag -m \"tag $subject\" icase-$(printf %02d $nr) &&\n> +\t\t\tnr=$((nr+1))||\n\nThe CodingGuidelines said we want to spell `$nr` instead of `nr`\ninside arithmetic expansion for dash older than 0.5.4\n\nI'm not sure if we should go with just `$((nr+1))` or it's better to\nloosen our Guidelines. Since Debian Jessie (oldest supported Debian)\nships 0.5.7. I don't know about other systems.\n\n\n-- \nDanh\n"},{"id":"396907","messageId":"20200504151351.GC11373@coredump.intra.peff.net","threadId":"53378","inReplyTo":"20200503114351.GA28680@danh.dev","subject":"Re: [PATCH 1/2] ref-filter: apply --ignore-case to all sorting keys","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-04T15:13:51Z","receivedAt":"2020-05-04T15:13:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 03, 2020 at 06:44:02PM +0700, Danh Doan wrote:\n\n> On 2020-05-03 05:11:57-0400, Jeff King <peff@peff.net> wrote:\n> > +test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' '\n> > +\t# name refs numerically to avoid case-insensitive filesystem conflicts\n> > +\tnr=0 &&\n> > +\tfor email in a A b B\n> > +\tdo\n> > +\t\tfor subject in a A b B\n> > +\t\tdo\n> > +\t\t\tGIT_COMMITTER_EMAIL=\"$email@example.com\" \\\n> > +\t\t\tgit tag -m \"tag $subject\" icase-$(printf %02d $nr) &&\n> > +\t\t\tnr=$((nr+1))||\n> \n> The CodingGuidelines said we want to spell `$nr` instead of `nr`\n> inside arithmetic expansion for dash older than 0.5.4\n> \n> I'm not sure if we should go with just `$((nr+1))` or it's better to\n> loosen our Guidelines. Since Debian Jessie (oldest supported Debian)\n> ships 0.5.7. I don't know about other systems.\n\nHmm, somehow I didn't know about that rule. We have many cases already\nin the test suite and elsewhere (try grepping for '$(([a-z]', which\nisn't exhaustive but turns up many examples).\n\nMaybe it's time to loosen the rule?\n\nI've actually seen style guides suggesting to never use \"$\" there for a\nfew reasons:\n\n  - it's slightly cleaner to read (this is the recommendation and\n    rationale in Google's shell style guide)\n\n  - it's less surprising if you somehow end up with a non-number in your\n    variable:\n\n      $ foo=bar\n      $ bar=41\n      $ echo $((foo + 1))\n      dash: 8: Illegal number: bar\n      $ echo $(($foo + 1))\n      42\n\n    That's using dash. With bash, both produce the answer 42! Clearly\n    this isn't something we should be doing either way, but I'd much\n    rather see \"illegal number\" in some cases which would alert us that\n    something confusing is going on.\n\n-Peff\n"},{"id":"396908","messageId":"xmqqh7wviud9.fsf@gitster.c.googlers.com","threadId":"53378","inReplyTo":"20200504151351.GC11373@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] ref-filter: apply --ignore-case to all sorting keys","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-04T15:37:38Z","receivedAt":"2020-05-04T15:37:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Hmm, somehow I didn't know about that rule. We have many cases already\n> in the test suite and elsewhere (try grepping for '$(([a-z]', which\n> isn't exhaustive but turns up many examples).\n>\n> Maybe it's time to loosen the rule?\n\nLet's do that.  It's time.\n"},{"id":"396920","messageId":"20200504160709.GB12842@coredump.intra.peff.net","threadId":"53378","inReplyTo":"xmqqh7wviud9.fsf@gitster.c.googlers.com","subject":"[PATCH] CodingGuidelines: drop arithmetic expansion advice to use \"$x\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-04T16:07:09Z","receivedAt":"2020-05-04T16:07:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 04, 2020 at 08:37:38AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Hmm, somehow I didn't know about that rule. We have many cases already\n> > in the test suite and elsewhere (try grepping for '$(([a-z]', which\n> > isn't exhaustive but turns up many examples).\n> >\n> > Maybe it's time to loosen the rule?\n> \n> Let's do that.  It's time.\n\nHere it is in patch form for your convenience.\n\n-- >8 --\nSubject: CodingGuidelines: drop arithmetic expansion advice to use \"$x\"\n\nThe advice to use \"$x\" rather than \"x\" in arithmetric expansion was\nworking around a dash bug fixed in 0.5.4. Even Debian oldstable has\n0.5.7 these days. And in the meantime, we've added almost two dozen\ninstances of the \"x\" form which you can find with:\n\n  git grep '$(([a-z]'\n\nand nobody seems to have complained. Let's declare this workaround\nobsolete and simplify our style guide.\n\nHelped-by: Danh Doan <congdanhqx@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/CodingGuidelines | 4 ----\n 1 file changed, 4 deletions(-)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 390ceece52..a89e8dcfbc 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -95,10 +95,6 @@ For shell scripts specifically (not exhaustive):\n \n  - We use Arithmetic Expansion $(( ... )).\n \n- - Inside Arithmetic Expansion, spell shell variables with $ in front\n-   of them, as some shells do not grok $((x)) while accepting $(($x))\n-   just fine (e.g. dash older than 0.5.4).\n-\n  - We do not use Process Substitution <(list) or >(list).\n \n  - Do not write control structures on a single line with semicolon.\n-- \n2.26.2.957.g6dc93e954a\n\n"},{"id":"396924","messageId":"20200504162844.GE86805@Carlos-MBP","threadId":"53378","inReplyTo":"20200504160709.GB12842@coredump.intra.peff.net","subject":"Re: [PATCH] CodingGuidelines: drop arithmetic expansion advice to use \"$x\"","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-04T16:28:44Z","receivedAt":"2020-05-04T16:28:50Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, May 04, 2020 at 12:07:09PM -0400, Jeff King wrote:\n> -- >8 --\n> Subject: CodingGuidelines: drop arithmetic expansion advice to use \"$x\"\n> \n> The advice to use \"$x\" rather than \"x\" in arithmetric expansion was\n> working around a dash bug fixed in 0.5.4. Even Debian oldstable has\n> 0.5.7 these days.\n\nthat would be oldoldstable, oldstable is actually in 0.5.8 ;)\n\n> Helped-by: Danh Doan <congdanhqx@gmail.com>\n> Signed-off-by: Jeff King <peff@peff.net>\n\nReviewed-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n\nCarlo\n"},{"id":"396927","messageId":"20200504163325.GA18103@coredump.intra.peff.net","threadId":"53378","inReplyTo":"20200504162844.GE86805@Carlos-MBP","subject":"Re: [PATCH] CodingGuidelines: drop arithmetic expansion advice to use \"$x\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-04T16:33:25Z","receivedAt":"2020-05-04T16:33:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 04, 2020 at 09:28:44AM -0700, Carlo Marcelo Arenas Belón wrote:\n\n> On Mon, May 04, 2020 at 12:07:09PM -0400, Jeff King wrote:\n> > -- >8 --\n> > Subject: CodingGuidelines: drop arithmetic expansion advice to use \"$x\"\n> > \n> > The advice to use \"$x\" rather than \"x\" in arithmetric expansion was\n> > working around a dash bug fixed in 0.5.4. Even Debian oldstable has\n> > 0.5.7 these days.\n> \n> that would be oldoldstable, oldstable is actually in 0.5.8 ;)\n\nOh, you're right. I forgot they're doing an extra layer these days. It\nwill officially become obsolete in less than 2 months. :)\n\n-Peff\n"},{"id":"396974","messageId":"xmqqy2q7fpo7.fsf@gitster.c.googlers.com","threadId":"53378","inReplyTo":"20200504163325.GA18103@coredump.intra.peff.net","subject":"Re: [PATCH] CodingGuidelines: drop arithmetic expansion advice to use \"$x\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-04T19:47:20Z","receivedAt":"2020-05-04T19:47:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, May 04, 2020 at 09:28:44AM -0700, Carlo Marcelo Arenas Belón wrote:\n>\n>> On Mon, May 04, 2020 at 12:07:09PM -0400, Jeff King wrote:\n>> > -- >8 --\n>> > Subject: CodingGuidelines: drop arithmetic expansion advice to use \"$x\"\n>> > \n>> > The advice to use \"$x\" rather than \"x\" in arithmetric expansion was\n>> > working around a dash bug fixed in 0.5.4. Even Debian oldstable has\n>> > 0.5.7 these days.\n>> \n>> that would be oldoldstable, oldstable is actually in 0.5.8 ;)\n>\n> Oh, you're right. I forgot they're doing an extra layer these days. It\n> will officially become obsolete in less than 2 months. :)\n\nThanks, both.  I'll just do s/0.5.7/0.5.8/ when I add Carlo's\nreviewed-by to queue the patch.\n"},{"id":"396986","messageId":"xmqqh7wvfmar.fsf@gitster.c.googlers.com","threadId":"53378","inReplyTo":"20200503091157.GA170902@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] ref-filter: apply --ignore-case to all sorting keys","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-04T21:00:12Z","receivedAt":"2020-05-04T21:00:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> would sort the primary key (taggername) case-insensitively, but sort the\n> refname case-sensitively. We have two options here:\n>\n>   - teach callers to set ignore_case on the whole list\n>\n>   - replace the ref_sorting list with a struct that contains both the\n>     list of sorting keys, as well as options that apply to _all_\n>     keys\n>\n> I went with the first one here, as it gives more flexibility if we later\n> want to let the users set the flag per-key (presumably through some\n> special syntax when defining the key; for now it's all or nothing\n> through --ignore-case).\n\nA good design decision I would fully support.\n\n> +test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' '\n> +\t# name refs numerically to avoid case-insensitive filesystem conflicts\n\nVery considerate.  If I were writing these nested loops, I am sure I\nwould have used \"tag-$email-$subject\" to be cute.\n\nQueued.  Thanks.\n\n> +\tnr=0 &&\n> +\tfor email in a A b B\n> +\tdo\n> +\t\tfor subject in a A b B\n> +\t\tdo\n> +\t\t\tGIT_COMMITTER_EMAIL=\"$email@example.com\" \\\n> +\t\t\tgit tag -m \"tag $subject\" icase-$(printf %02d $nr) &&\n> +\t\t\tnr=$((nr+1))||\n> +\t\t\treturn 1\n> +\t\tdone\n> +\tdone &&\n> +\tgit for-each-ref --ignore-case \\\n> +\t\t--format=\"%(taggeremail) %(subject) %(refname)\" \\\n> +\t\t--sort=refname \\\n> +\t\t--sort=subject \\\n> +\t\t--sort=taggeremail \\\n> +\t\trefs/tags/icase-* >actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\t<a@example.com> tag a refs/tags/icase-00\n> +\t<a@example.com> tag A refs/tags/icase-01\n> +\t<A@example.com> tag a refs/tags/icase-04\n> +\t<A@example.com> tag A refs/tags/icase-05\n> +\t<a@example.com> tag b refs/tags/icase-02\n> +\t<a@example.com> tag B refs/tags/icase-03\n> +\t<A@example.com> tag b refs/tags/icase-06\n> +\t<A@example.com> tag B refs/tags/icase-07\n> +\t<b@example.com> tag a refs/tags/icase-08\n> +\t<b@example.com> tag A refs/tags/icase-09\n> +\t<B@example.com> tag a refs/tags/icase-12\n> +\t<B@example.com> tag A refs/tags/icase-13\n> +\t<b@example.com> tag b refs/tags/icase-10\n> +\t<b@example.com> tag B refs/tags/icase-11\n> +\t<B@example.com> tag b refs/tags/icase-14\n> +\t<B@example.com> tag B refs/tags/icase-15\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_done\n"},{"id":"396987","messageId":"xmqqd07jfm2b.fsf@gitster.c.googlers.com","threadId":"53378","inReplyTo":"20200503091309.GB170902@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] ref-filter: apply fallback refname sort only after all user sorts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-04T21:05:16Z","receivedAt":"2020-05-04T21:05:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Commit 9e468334b4 (ref-filter: fallback on alphabetical comparison,\n> 2015-10-30) taught ref-filter's sort to fallback to comparing refnames.\n> But it did it at the wrong level, overriding the comparison result for a\n> single \"--sort\" key from the user, rather than after all sort keys have\n> been exhausted.\n\nGood find.  It does look like that the fallback was broken from the\nvery first version when it was introduced, as we did have multiple\nkeys support back in that version already.\n\nThe fix looks good to me.\n\nThanks.\n"},{"id":"397015","messageId":"20200504233255.GC29599@danh.dev","threadId":"53378","inReplyTo":"20200504160709.GB12842@coredump.intra.peff.net","subject":"Re: [PATCH] CodingGuidelines: drop arithmetic expansion advice to use \"$x\"","fromName":"Danh Doan","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-05-04T23:32:55Z","receivedAt":"2020-05-04T23:33:01Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-05-04 12:07:09-0400, Jeff King <peff@peff.net> wrote:\n> On Mon, May 04, 2020 at 08:37:38AM -0700, Junio C Hamano wrote:\n> \n> Subject: CodingGuidelines: drop arithmetic expansion advice to use \"$x\"\n> \n> The advice to use \"$x\" rather than \"x\" in arithmetric expansion was\n> working around a dash bug fixed in 0.5.4. Even Debian oldstable has\n> 0.5.7 these days. And in the meantime, we've added almost two dozen\n> instances of the \"x\" form which you can find with:\n> \n>   git grep '$(([a-z]'\n> \n> and nobody seems to have complained. Let's declare this workaround\n> obsolete and simplify our style guide.\n> \n> Helped-by: Danh Doan <congdanhqx@gmail.com>\n> Signed-off-by: Jeff King <peff@peff.net>\n\nI see this patch hasn't been merged to pu yet.\n\nPlease have my name as (if it's not too much trouble for you):\n\n\tĐoàn Trần Công Danh <congdanhqx@gmail.com>\n\n(I'm going to change my name in email setting)\n\n-- \nDanh\n"},{"id":"397019","messageId":"20200505001104.GA40276@coredump.intra.peff.net","threadId":"53378","inReplyTo":"xmqqh7wvfmar.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] ref-filter: apply --ignore-case to all sorting keys","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-05T00:11:04Z","receivedAt":"2020-05-05T00:11:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 04, 2020 at 02:00:12PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > would sort the primary key (taggername) case-insensitively, but sort the\n> > refname case-sensitively. We have two options here:\n> >\n> >   - teach callers to set ignore_case on the whole list\n> >\n> >   - replace the ref_sorting list with a struct that contains both the\n> >     list of sorting keys, as well as options that apply to _all_\n> >     keys\n> >\n> > I went with the first one here, as it gives more flexibility if we later\n> > want to let the users set the flag per-key (presumably through some\n> > special syntax when defining the key; for now it's all or nothing\n> > through --ignore-case).\n> \n> A good design decision I would fully support.\n\nI admit I had second thoughts when dealing with the \"oops, we have to\nchoose ignore_case from the first one\" part of the second patch. But I\nthink it works OK in practice, and I did like having a less invasive\ndiff. :)\n\n-Peff\n"},{"id":"397020","messageId":"20200505001336.GK45250@syl.local","threadId":"53378","inReplyTo":"20200503091157.GA170902@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] ref-filter: apply --ignore-case to all sorting keys","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-05T00:13:36Z","receivedAt":"2020-05-05T00:13:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, May 03, 2020 at 05:11:57AM -0400, Jeff King wrote:\n> All of the ref-filter users (for-each-ref, branch, and tag) take an\n> --ignore-case option which makes filtering and sorting case-insensitive.\n> However, this option was applied only to the first element of the\n> ref_sorting list. So:\n>\n>   git for-each-ref --ignore-case --sort=refname\n>\n> would do what you expect, but:\n>\n>   git for-each-ref --ignore-case --sort=refname --sort=taggername\n>\n> would sort the primary key (taggername) case-insensitively, but sort the\n> refname case-sensitively. We have two options here:\n>\n>   - teach callers to set ignore_case on the whole list\n>\n>   - replace the ref_sorting list with a struct that contains both the\n>     list of sorting keys, as well as options that apply to _all_\n>     keys\n>\n> I went with the first one here, as it gives more flexibility if we later\n> want to let the users set the flag per-key (presumably through some\n> special syntax when defining the key; for now it's all or nothing\n> through --ignore-case).\n\nMakes sense, I think that this will provide us more flexibility in the\nfuture in case we want to have per-flag keys or some such.\n\n> The new test covers this by sorting on both tagger and subject\n> case-insensitively, which should compare \"a\" and \"A\" identically, but\n> still sort them before \"b\" and \"B\". We'll break ties by sorting on the\n> refname to give ourselves a stable output (this is actually supposed to\n> be done automatically, but there's another bug which will be fixed in\n> the next commit).\n\nThanks for adding a test.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/branch.c        |  2 +-\n>  builtin/for-each-ref.c  |  2 +-\n>  builtin/tag.c           |  2 +-\n>  ref-filter.c            |  6 ++++++\n>  ref-filter.h            |  2 ++\n>  t/t6300-for-each-ref.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n>  6 files changed, 51 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index d8297f80ff..86341cc835 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -739,7 +739,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n>  \t\t */\n>  \t\tif (!sorting)\n>  \t\t\tsorting = ref_default_sorting();\n> -\t\tsorting->ignore_case = icase;\n> +\t\tref_sorting_icase_all(sorting, icase);\n>  \t\tprint_ref_list(&filter, sorting, &format);\n>  \t\tprint_columns(&output, colopts, NULL);\n>  \t\tstring_list_clear(&output, 0);\n> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> index 465153e853..57489e4eab 100644\n> --- a/builtin/for-each-ref.c\n> +++ b/builtin/for-each-ref.c\n> @@ -70,7 +70,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n>\n>  \tif (!sorting)\n>  \t\tsorting = ref_default_sorting();\n> -\tsorting->ignore_case = icase;\n> +\tref_sorting_icase_all(sorting, icase);\n>  \tfilter.ignore_case = icase;\n>\n>  \tfilter.name_patterns = argv;\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index dd160b49c7..ff7610b5c8 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -485,7 +485,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>  \t}\n>  \tif (!sorting)\n>  \t\tsorting = ref_default_sorting();\n> -\tsorting->ignore_case = icase;\n> +\tref_sorting_icase_all(sorting, icase);\n>  \tfilter.ignore_case = icase;\n>  \tif (cmdmode == 'l') {\n>  \t\tint ret;\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 35776838f4..bdb3535ce5 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -2317,6 +2317,12 @@ static int compare_refs(const void *a_, const void *b_, void *ref_sorting)\n>  \treturn 0;\n>  }\n>\n> +void ref_sorting_icase_all(struct ref_sorting *sorting, int flag)\n> +{\n> +\tfor (; sorting; sorting = sorting->next)\n> +\t\tsorting->ignore_case = !!flag;\n> +}\n> +\n>  void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)\n>  {\n>  \tQSORT_S(array->items, array->nr, compare_refs, sorting);\n> diff --git a/ref-filter.h b/ref-filter.h\n> index 64330e9601..8ecc33cdfa 100644\n> --- a/ref-filter.h\n> +++ b/ref-filter.h\n> @@ -114,6 +114,8 @@ void ref_array_clear(struct ref_array *array);\n>  int verify_ref_format(struct ref_format *format);\n>  /*  Sort the given ref_array as per the ref_sorting provided */\n>  void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);\n> +/*  Set the ignore_case flag for all elements of a sorting list */\n> +void ref_sorting_icase_all(struct ref_sorting *sorting, int flag);\n>  /*  Based on the given format and quote_style, fill the strbuf */\n>  int format_ref_array_item(struct ref_array_item *info,\n>  \t\t\t  const struct ref_format *format,\n> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n> index b3c1092338..c9caf26327 100755\n> --- a/t/t6300-for-each-ref.sh\n> +++ b/t/t6300-for-each-ref.sh\n> @@ -895,4 +895,44 @@ test_expect_success 'for-each-ref --ignore-case ignores case' '\n>  \ttest_cmp expect actual\n>  '\n>\n> +test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' '\n> +\t# name refs numerically to avoid case-insensitive filesystem conflicts\n> +\tnr=0 &&\n> +\tfor email in a A b B\n> +\tdo\n> +\t\tfor subject in a A b B\n> +\t\tdo\n> +\t\t\tGIT_COMMITTER_EMAIL=\"$email@example.com\" \\\n> +\t\t\tgit tag -m \"tag $subject\" icase-$(printf %02d $nr) &&\n> +\t\t\tnr=$((nr+1))||\n> +\t\t\treturn 1\n> +\t\tdone\n> +\tdone &&\n> +\tgit for-each-ref --ignore-case \\\n> +\t\t--format=\"%(taggeremail) %(subject) %(refname)\" \\\n> +\t\t--sort=refname \\\n> +\t\t--sort=subject \\\n> +\t\t--sort=taggeremail \\\n> +\t\trefs/tags/icase-* >actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\t<a@example.com> tag a refs/tags/icase-00\n> +\t<a@example.com> tag A refs/tags/icase-01\n> +\t<A@example.com> tag a refs/tags/icase-04\n> +\t<A@example.com> tag A refs/tags/icase-05\n> +\t<a@example.com> tag b refs/tags/icase-02\n> +\t<a@example.com> tag B refs/tags/icase-03\n> +\t<A@example.com> tag b refs/tags/icase-06\n> +\t<A@example.com> tag B refs/tags/icase-07\n> +\t<b@example.com> tag a refs/tags/icase-08\n> +\t<b@example.com> tag A refs/tags/icase-09\n> +\t<B@example.com> tag a refs/tags/icase-12\n> +\t<B@example.com> tag A refs/tags/icase-13\n> +\t<b@example.com> tag b refs/tags/icase-10\n> +\t<b@example.com> tag B refs/tags/icase-11\n> +\t<B@example.com> tag b refs/tags/icase-14\n> +\t<B@example.com> tag B refs/tags/icase-15\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_done\n> --\n> 2.26.2.957.g6dc93e954a\n\nAll looks very reasonable, so:\n\n  Reviewed-by: Taylor Blau <me@ttaylorr.com>\n\nThanks,\nTaylor\n"},{"id":"397021","messageId":"20200505001455.GA64230@syl.local","threadId":"53378","inReplyTo":"xmqqd07jfm2b.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/2] ref-filter: apply fallback refname sort only after all user sorts","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-05T00:14:55Z","receivedAt":"2020-05-05T00:15:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, May 04, 2020 at 02:05:16PM -0700, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n>\n> > Commit 9e468334b4 (ref-filter: fallback on alphabetical comparison,\n> > 2015-10-30) taught ref-filter's sort to fallback to comparing refnames.\n> > But it did it at the wrong level, overriding the comparison result for a\n> > single \"--sort\" key from the user, rather than after all sort keys have\n> > been exhausted.\n>\n> Good find.  It does look like that the fallback was broken from the\n> very first version when it was introduced, as we did have multiple\n> keys support back in that version already.\n>\n> The fix looks good to me.\n\nMe too.\n\n  Reviewed-by: Taylor Blau <me@ttaylorr.com>\n\n> Thanks.\n\nThanks,\nTaylor\n"},{"id":"397134","messageId":"xmqqr1vy9kv0.fsf@gitster.c.googlers.com","threadId":"53378","inReplyTo":"20200504160709.GB12842@coredump.intra.peff.net","subject":"Re: [PATCH] CodingGuidelines: drop arithmetic expansion advice to use \"$x\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-05T20:40:03Z","receivedAt":"2020-05-05T20:40:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> index 390ceece52..a89e8dcfbc 100644\n> --- a/Documentation/CodingGuidelines\n> +++ b/Documentation/CodingGuidelines\n> @@ -95,10 +95,6 @@ For shell scripts specifically (not exhaustive):\n>  \n>   - We use Arithmetic Expansion $(( ... )).\n>  \n> - - Inside Arithmetic Expansion, spell shell variables with $ in front\n> -   of them, as some shells do not grok $((x)) while accepting $(($x))\n> -   just fine (e.g. dash older than 0.5.4).\n> -\n>   - We do not use Process Substitution <(list) or >(list).\n>  \n>   - Do not write control structures on a single line with semicolon.\n\nA new entry in the \"What's cooking\" report has this:\n\n    * jk/arith-expansion-coding-guidelines (2020-05-04) 1 commit\n     - CodingGuidelines: drop arithmetic expansion advice to use \"$x\"\n\n     The coding guideline for shell scripts instructed to refer to a\n     variable with dollar-sign inside airthmetic expansion to work\n     around a bug in old versions of bash, which is a thing of the past.\n     Now we are not forbidden from writing $((var+1)).\n\nWriting the last sentence made me wonder if we should go one step\nfurther and actually encourage actively omitting the dollar-sign\nfrom variable reference instead.\n"},{"id":"397137","messageId":"20200505210741.GB645290@coredump.intra.peff.net","threadId":"53378","inReplyTo":"xmqqr1vy9kv0.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] CodingGuidelines: drop arithmetic expansion advice to use \"$x\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-05T21:07:41Z","receivedAt":"2020-05-05T21:07:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 05, 2020 at 01:40:03PM -0700, Junio C Hamano wrote:\n\n> A new entry in the \"What's cooking\" report has this:\n> \n>     * jk/arith-expansion-coding-guidelines (2020-05-04) 1 commit\n>      - CodingGuidelines: drop arithmetic expansion advice to use \"$x\"\n> \n>      The coding guideline for shell scripts instructed to refer to a\n>      variable with dollar-sign inside airthmetic expansion to work\n>      around a bug in old versions of bash, which is a thing of the past.\n>      Now we are not forbidden from writing $((var+1)).\n\ns/bash/dash/, I think\n\n> Writing the last sentence made me wonder if we should go one step\n> further and actually encourage actively omitting the dollar-sign\n> from variable reference instead.\n\nI don't have a strong preference either way. I gave a few reasons to\nprefer the dollar-less version in:\n\n  https://lore.kernel.org/git/20200504151351.GC11373@coredump.intra.peff.net/\n\nbut I couldn't find a case where the difference really matters in\npractice for otherwise-correct code. If we don't care much either way,\nI'd just as soon not have a rule. We have enough rules as it is, and I\ndon't think either is obvious enough that somebody who is used to one\nstyle will get confused by the other.\n\n-Peff\n"},{"id":"397139","messageId":"xmqqh7wu9iie.fsf@gitster.c.googlers.com","threadId":"53378","inReplyTo":"20200505210741.GB645290@coredump.intra.peff.net","subject":"Re: [PATCH] CodingGuidelines: drop arithmetic expansion advice to use \"$x\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-05T21:30:49Z","receivedAt":"2020-05-05T21:30:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I'd just as soon not have a rule. We have enough rules as it is, and I\n> don't think either is obvious enough that somebody who is used to one\n> style will get confused by the other.\n\nOK.  I very much welcome one fewer paragraphs in the file ;-)\n"}]}