{"thread":{"id":"51256","subject":"[RFC PATCH] ref-filter: sort detached HEAD lines firstly","startedAt":"2019-06-06T21:39:15Z","lastAt":"2021-01-07T23:25:55Z","messageCount":36,"participants":["Matthew DeVore","Johannes Schindelin","Jonathan Nieder","Junio C Hamano","Jeff King","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"376784","messageId":"faaa9a3d6ba66d77cc2a8eab438d1bfc8f762fa1.1559857032.git.matvore@google.com","threadId":"51256","inReplyTo":null,"subject":"[RFC PATCH] ref-filter: sort detached HEAD lines firstly","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2019-06-06T21:38:20Z","receivedAt":"2019-06-06T21:39:15Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"Before this patch, \"git branch\" would put \"(HEAD detached...)\" and \"(no\nbranch, rebasing...)\" lines before all the other branches *in most\ncases* and only because of the fact that \"(\" is a low codepoint. This\nwould not hold in the Chinese locale, which uses a full-width \"(\" symbol\n(codepoint FF08). This meant that the detached HEAD line would appear\nafter all local refs and even after the remote refs if there were any.\n\nDeliberately sort the detached HEAD refs before other refs when sorting\nby refname rather than rely on codepoint subtleties.\n\nSigned-off-by: Matthew DeVore \n---\n ref-filter.c | 10 +++++++---\n t/lib-gettext.sh | 16 +++++++++++++---\n t/t3207-branch-intl.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 3 files changed, 58 insertions(+), 6 deletions(-)\n create mode 100755 t/t3207-branch-intl.sh\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 8500671bc6..cbfae790f9 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2157,25 +2157,29 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n \tcmp_type cmp_type = used_atom[s->atom].type;\n \tint (*cmp_fn)(const char *, const char *);\n \tstruct strbuf err = STRBUF_INIT;\n \n \tif (get_ref_atom_value(a, s->atom, &va, &err))\n \t\tdie(\"%s\", err.buf);\n \tif (get_ref_atom_value(b, s->atom, &vb, &err))\n \t\tdie(\"%s\", err.buf);\n \tstrbuf_release(&err);\n \tcmp_fn = s->ignore_case ? strcasecmp : strcmp;\n-\tif (s->version)\n+\tif (s->version) {\n \t\tcmp = versioncmp(va->s, vb->s);\n-\telse if (cmp_type == FIELD_STR)\n+\t} else if (cmp_type == FIELD_STR) {\n+\t\tif ((a->kind & FILTER_REFS_DETACHED_HEAD) !=\n+\t\t\t\t(b->kind & FILTER_REFS_DETACHED_HEAD)) {\n+\t\t\treturn (a->kind & FILTER_REFS_DETACHED_HEAD) ? -1 : 1;\n+\t\t}\n \t\tcmp = cmp_fn(va->s, vb->s);\n-\telse {\n+\t} else {\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\telse\n \t\t\tcmp = 1;\n \t}\n \n \treturn (s->reverse) ? -cmp : cmp;\n }\ndiff --git a/t/lib-gettext.sh b/t/lib-gettext.sh\nindex 2139b427ca..de08d109dc 100644\n--- a/t/lib-gettext.sh\n+++ b/t/lib-gettext.sh\n@@ -25,23 +25,29 @@ then\n \t\tp\n \t\tq\n \t}')\n \t# is_IS.ISO8859-1 on Solaris and FreeBSD, is_IS.iso88591 on Debian\n \tis_IS_iso_locale=$(locale -a 2>/dev/null |\n \t\tsed -n '/^is_IS\\.[iI][sS][oO]8859-*1$/{\n \t\tp\n \t\tq\n \t}')\n \n-\t# Export them as an environment variable so the t0202/test.pl Perl\n-\t# test can use it too\n-\texport is_IS_locale is_IS_iso_locale\n+\tzh_CN_locale=$(locale -a 2>/dev/null |\n+\t\tsed -n '/^zh_CN\\.[uU][tT][fF]-*8$/{\n+\t\tp\n+\t\tq\n+\t}')\n+\n+\t# Export them as environment variables so other tests can use them\n+\t# too\n+\texport is_IS_locale is_IS_iso_locale zh_CN_locale\n \n \tif test -n \"$is_IS_locale\" &&\n \t\ttest $GIT_INTERNAL_GETTEXT_SH_SCHEME != \"fallthrough\"\n \tthen\n \t\t# Some of the tests need the reference Icelandic locale\n \t\ttest_set_prereq GETTEXT_LOCALE\n \n \t\t# Exporting for t0202/test.pl\n \t\tGETTEXT_LOCALE=1\n \t\texport GETTEXT_LOCALE\n@@ -53,11 +59,15 @@ then\n \tif test -n \"$is_IS_iso_locale\" &&\n \t\ttest $GIT_INTERNAL_GETTEXT_SH_SCHEME != \"fallthrough\"\n \tthen\n \t\t# Some of the tests need the reference Icelandic locale\n \t\ttest_set_prereq GETTEXT_ISO_LOCALE\n \n \t\tsay \"# lib-gettext: Found '$is_IS_iso_locale' as an is_IS ISO-8859-1 locale\"\n \telse\n \t\tsay \"# lib-gettext: No is_IS ISO-8859-1 locale available\"\n \tfi\n+\n+\tif test -z \"$zh_CN_locale\"; then\n+\t\tsay \"# lib-gettext: No zh_CN UTF-8 locale available\"\n+\tfi\n fi\ndiff --git a/t/t3207-branch-intl.sh b/t/t3207-branch-intl.sh\nnew file mode 100755\nindex 0000000000..9f6fcc7481\n--- /dev/null\n+++ b/t/t3207-branch-intl.sh\n@@ -0,0 +1,38 @@\n+#!/bin/sh\n+\n+test_description='git branch internationalization tests'\n+\n+. ./lib-gettext.sh\n+\n+test_expect_success 'init repo' '\n+\tgit init r1 &&\n+\ttouch r1/foo &&\n+\tgit -C r1 add foo &&\n+\tgit -C r1 commit -m foo\n+'\n+\n+test_expect_success 'detached head sorts before other branches' '\n+\t# Ref sorting logic should put detached heads before the other\n+\t# branches, but this is not automatic when a branch name sorts\n+\t# lexically before \"(\" or the full-width \"(\" (Unicode codepoint FF08).\n+\t# The latter case is nearly guaranteed for the Chinese locale.\n+\n+\tgit -C r1 checkout HEAD^{} -- &&\n+\tgit -C r1 branch !should_be_after_detached HEAD &&\n+\tLC_ALL=$zh_CN_locale LC_MESSAGES=$zh_CN_locale \\\n+\t\tgit -C r1 branch >actual &&\n+\tgit -C r1 checkout - &&\n+\n+\tawk \"\n+\t# We need full-width or half-width parens on the first line.\n+\tNR == 1 && (/[(].*[)]/ || /\\xef\\xbc\\x88.*\\xef\\xbc\\x89/) {\n+\t\tfound_head = 1;\n+\t}\n+\t/!should_be_after_detached/ {\n+\t\tfound_control_branch = 1;\n+\t}\n+\tEND { exit !found_head || !found_control_branch }\n+\t\" actual\n+'\n+\n+test_done\n-- \n2.21.0\n\n"},{"id":"376875","messageId":"nycvar.QRO.7.76.6.1906090954510.789@QRFXGBC-DHN364S.ybpnyqbznva","threadId":"51256","inReplyTo":"faaa9a3d6ba66d77cc2a8eab438d1bfc8f762fa1.1559857032.git.matvore@google.com","subject":"Re: [RFC PATCH] ref-filter: sort detached HEAD lines firstlyxy","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-06-09T08:17:19Z","receivedAt":"2019-06-09T08:17:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Matthew,\n\nOn Thu, 6 Jun 2019, Matthew DeVore wrote:\n\n> Before this patch, \"git branch\" would put \"(HEAD detached...)\" and \"(no\n> branch, rebasing...)\" lines before all the other branches *in most\n> cases* and only because of the fact that \"(\" is a low codepoint. This\n> would not hold in the Chinese locale, which uses a full-width \"(\" symbol\n> (codepoint FF08). This meant that the detached HEAD line would appear\n> after all local refs and even after the remote refs if there were any.\n>\n> Deliberately sort the detached HEAD refs before other refs when sorting\n> by refname rather than rely on codepoint subtleties.\n\nThis description is pretty convincing!\n\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 8500671bc6..cbfae790f9 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -2157,25 +2157,29 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n> \tcmp_type cmp_type = used_atom[s->atom].type;\n> \tint (*cmp_fn)(const char *, const char *);\n> \tstruct strbuf err = STRBUF_INIT;\n>\n> \tif (get_ref_atom_value(a, s->atom, &va, &err))\n> \t\tdie(\"%s\", err.buf);\n> \tif (get_ref_atom_value(b, s->atom, &vb, &err))\n> \t\tdie(\"%s\", err.buf);\n> \tstrbuf_release(&err);\n> \tcmp_fn = s->ignore_case ? strcasecmp : strcmp;\n> -\tif (s->version)\n> +\tif (s->version) {\n> \t\tcmp = versioncmp(va->s, vb->s);\n> -\telse if (cmp_type == FIELD_STR)\n> +\t} else if (cmp_type == FIELD_STR) {\n\nI find that it makes sense in general to suppress one's urges regarding\nintroducing `{ ... }` around one-liners when the patch does not actually\nrequire it.\n\nFor example, I found this patch harder than necessary to read because of\nit.\n\n> +\t\tif ((a->kind & FILTER_REFS_DETACHED_HEAD) !=\n> +\t\t\t\t(b->kind & FILTER_REFS_DETACHED_HEAD)) {\n\nSo in case that both are detached...\n\n> +\t\t\treturn (a->kind & FILTER_REFS_DETACHED_HEAD) ? -1 : 1;\n> +\t\t}\n> \t\tcmp = cmp_fn(va->s, vb->s);\n\n... we compare their commit hashes, is that right? Might be worth a code\ncomment.\n\n> -\telse {\n> +\t} else {\n\nFWIW it would have been a much more obvious patch if it had done\n\n \tif (s->version)\n\t\t[...]\n+\telse if (cmp_type == FIELD_STR &&\n+\t\t (a->kind & FILTER_REFS_DETACHED_HEAD ||\n+\t\t b->kind & FILTER_REFS_DETACHED_HEAD))\n+\t\treturn (a->kind & FILTER_REFS_DETACHED_HEAD) ? -1 : 1;\n \telse if (cmp_type == FIELD_STR)\n\t\t[...]\n\nMaybe still worth doing.\n\nFWIW I was *so* tempted to write\n\n\t((a->kind ^ b->kind) & FILTER_REFS_DETACHED_HEAD)\n\nto make this code DRYer, but then, readers not intimately familiar with\nBoolean arithmetic might not even know about the `^` operator, making the\ncode harder to read than necessary, too.\n\n> diff --git a/t/lib-gettext.sh b/t/lib-gettext.sh\n> index 2139b427ca..de08d109dc 100644\n> --- a/t/lib-gettext.sh\n> +++ b/t/lib-gettext.sh\n> @@ -25,23 +25,29 @@ then\n> \t\tp\n> \t\tq\n> \t}')\n> \t# is_IS.ISO8859-1 on Solaris and FreeBSD, is_IS.iso88591 on Debian\n> \tis_IS_iso_locale=$(locale -a 2>/dev/null |\n> \t\tsed -n '/^is_IS\\.[iI][sS][oO]8859-*1$/{\n> \t\tp\n> \t\tq\n> \t}')\n>\n> -\t# Export them as an environment variable so the t0202/test.pl Perl\n> -\t# test can use it too\n> -\texport is_IS_locale is_IS_iso_locale\n> +\tzh_CN_locale=$(locale -a 2>/dev/null |\n> +\t\tsed -n '/^zh_CN\\.[uU][tT][fF]-*8$/{\n> +\t\tp\n> +\t\tq\n> +\t}')\n> +\n> +\t# Export them as environment variables so other tests can use them\n> +\t# too\n> +\texport is_IS_locale is_IS_iso_locale zh_CN_locale\n>\n> \tif test -n \"$is_IS_locale\" &&\n> \t\ttest $GIT_INTERNAL_GETTEXT_SH_SCHEME != \"fallthrough\"\n> \tthen\n> \t\t# Some of the tests need the reference Icelandic locale\n> \t\ttest_set_prereq GETTEXT_LOCALE\n>\n> \t\t# Exporting for t0202/test.pl\n> \t\tGETTEXT_LOCALE=1\n> \t\texport GETTEXT_LOCALE\n> @@ -53,11 +59,15 @@ then\n> \tif test -n \"$is_IS_iso_locale\" &&\n> \t\ttest $GIT_INTERNAL_GETTEXT_SH_SCHEME != \"fallthrough\"\n> \tthen\n> \t\t# Some of the tests need the reference Icelandic locale\n> \t\ttest_set_prereq GETTEXT_ISO_LOCALE\n>\n> \t\tsay \"# lib-gettext: Found '$is_IS_iso_locale' as an is_IS ISO-8859-1 locale\"\n> \telse\n> \t\tsay \"# lib-gettext: No is_IS ISO-8859-1 locale available\"\n> \tfi\n> +\n> +\tif test -z \"$zh_CN_locale\"; then\n> +\t\tsay \"# lib-gettext: No zh_CN UTF-8 locale available\"\n> +\tfi\n\nI wonder why this hunk, unlike the previous one, does not imitate the\nis_IS handling closely.\n\n> diff --git a/t/t3207-branch-intl.sh b/t/t3207-branch-intl.sh\n> new file mode 100755\n> index 0000000000..9f6fcc7481\n> --- /dev/null\n> +++ b/t/t3207-branch-intl.sh\n> @@ -0,0 +1,38 @@\n> +#!/bin/sh\n> +\n> +test_description='git branch internationalization tests'\n> +\n> +. ./lib-gettext.sh\n> +\n> +test_expect_success 'init repo' '\n> +\tgit init r1 &&\n\nWhy?\n\n> +\ttouch r1/foo &&\n> +\tgit -C r1 add foo &&\n> +\tgit -C r1 commit -m foo\n> +'\n\nWhy not simply `test_commit foo`?\n\n> +test_expect_success 'detached head sorts before other branches' '\n> +\t# Ref sorting logic should put detached heads before the other\n> +\t# branches, but this is not automatic when a branch name sorts\n> +\t# lexically before \"(\" or the full-width \"(\" (Unicode codepoint FF08).\n> +\t# The latter case is nearly guaranteed for the Chinese locale.\n> +\n> +\tgit -C r1 checkout HEAD^{} -- &&\n> +\tgit -C r1 branch !should_be_after_detached HEAD &&\n\nI am not sure that `!` is a wise choice, as it might not be a legal file\nname character everywhere. A `.` or `-` might make more sense.\n\n> +\tLC_ALL=$zh_CN_locale LC_MESSAGES=$zh_CN_locale \\\n> +\t\tgit -C r1 branch >actual &&\n> +\tgit -C r1 checkout - &&\n\nWhy call `checkout` after `branch`? That's unnecessary, we do not verify\nanything after that call.\n\n> +\tawk \"\n> +\t# We need full-width or half-width parens on the first line.\n> +\tNR == 1 && (/[(].*[)]/ || /\\xef\\xbc\\x88.*\\xef\\xbc\\x89/) {\n> +\t\tfound_head = 1;\n> +\t}\n> +\t/!should_be_after_detached/ {\n> +\t\tfound_control_branch = 1;\n> +\t}\n> +\tEND { exit !found_head || !found_control_branch }\n> +\t\" actual\n\nThis might look beautiful for a fan of `awk`. For the vast majority of us,\nthis is not a good idea.\n\nRemember, you do *not* write those tests for your own pleasure, you do\n*not* write those tests in order to help you catch problems while you\ndevelop your patches, you do *not* develop these tests in order to just\ncatch future breakages.\n\nYou *do* write those tests for *other* developers who you try to help in\npreventing introducing regressions.\n\nAs such, you *want* the tests to be\n\n- easy to understand for as wide a range of developers as you can make,\n\n- quick,\n\n- covering regressions, and *only* regressions,\n\n- helping diagnose *and* fix regressions.\n\nIn the ideal case you won't even hear when developers found your test\nhelpful, and you will never, ever learn about regressions that have been\nprevented.\n\nYou most frequently will hear about your tests when they did not do their\njob well.\n\nIn this instance, I would have expected something like\n\n\ttest_expect_lines = 3 actual &&\n\n\thead -n 1 first &&\n\ttest_i18ngrep \"detached HEAD\" first &&\n\n\ttail -n 1 last &&\n\tgrep should_be_after last\n\ninstead of the \"awk-ward\" code above.\n\nCiao,\nJohannes\n\n> +'\n> +\n> +test_done\n> --\n> 2.21.0\n>\n>\n"},{"id":"376887","messageId":"nycvar.QRO.7.76.6.1906091838580.789@QRFXGBC-DHN364S.ybpnyqbznva","threadId":"51256","inReplyTo":"nycvar.QRO.7.76.6.1906090954510.789@QRFXGBC-DHN364S.ybpnyqbznva","subject":"Re: [RFC PATCH] ref-filter: sort detached HEAD lines firstlyxy","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-06-09T16:39:35Z","receivedAt":"2019-06-09T16:39:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nsorry for top-posting, but I just noticed the \"firstlyxy\" typo in the\nsubject ;-)\n\nCiao,\nDscho\n\nOn Sun, 9 Jun 2019, Johannes Schindelin wrote:\n\n> Hi Matthew,\n>\n> On Thu, 6 Jun 2019, Matthew DeVore wrote:\n>\n> > Before this patch, \"git branch\" would put \"(HEAD detached...)\" and \"(no\n> > branch, rebasing...)\" lines before all the other branches *in most\n> > cases* and only because of the fact that \"(\" is a low codepoint. This\n> > would not hold in the Chinese locale, which uses a full-width \"(\" symbol\n> > (codepoint FF08). This meant that the detached HEAD line would appear\n> > after all local refs and even after the remote refs if there were any.\n> >\n> > Deliberately sort the detached HEAD refs before other refs when sorting\n> > by refname rather than rely on codepoint subtleties.\n>\n> This description is pretty convincing!\n>\n> > diff --git a/ref-filter.c b/ref-filter.c\n> > index 8500671bc6..cbfae790f9 100644\n> > --- a/ref-filter.c\n> > +++ b/ref-filter.c\n> > @@ -2157,25 +2157,29 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n> > \tcmp_type cmp_type = used_atom[s->atom].type;\n> > \tint (*cmp_fn)(const char *, const char *);\n> > \tstruct strbuf err = STRBUF_INIT;\n> >\n> > \tif (get_ref_atom_value(a, s->atom, &va, &err))\n> > \t\tdie(\"%s\", err.buf);\n> > \tif (get_ref_atom_value(b, s->atom, &vb, &err))\n> > \t\tdie(\"%s\", err.buf);\n> > \tstrbuf_release(&err);\n> > \tcmp_fn = s->ignore_case ? strcasecmp : strcmp;\n> > -\tif (s->version)\n> > +\tif (s->version) {\n> > \t\tcmp = versioncmp(va->s, vb->s);\n> > -\telse if (cmp_type == FIELD_STR)\n> > +\t} else if (cmp_type == FIELD_STR) {\n>\n> I find that it makes sense in general to suppress one's urges regarding\n> introducing `{ ... }` around one-liners when the patch does not actually\n> require it.\n>\n> For example, I found this patch harder than necessary to read because of\n> it.\n>\n> > +\t\tif ((a->kind & FILTER_REFS_DETACHED_HEAD) !=\n> > +\t\t\t\t(b->kind & FILTER_REFS_DETACHED_HEAD)) {\n>\n> So in case that both are detached...\n>\n> > +\t\t\treturn (a->kind & FILTER_REFS_DETACHED_HEAD) ? -1 : 1;\n> > +\t\t}\n> > \t\tcmp = cmp_fn(va->s, vb->s);\n>\n> ... we compare their commit hashes, is that right? Might be worth a code\n> comment.\n>\n> > -\telse {\n> > +\t} else {\n>\n> FWIW it would have been a much more obvious patch if it had done\n>\n> \tif (s->version)\n> \t\t[...]\n> +\telse if (cmp_type == FIELD_STR &&\n> +\t\t (a->kind & FILTER_REFS_DETACHED_HEAD ||\n> +\t\t b->kind & FILTER_REFS_DETACHED_HEAD))\n> +\t\treturn (a->kind & FILTER_REFS_DETACHED_HEAD) ? -1 : 1;\n> \telse if (cmp_type == FIELD_STR)\n> \t\t[...]\n>\n> Maybe still worth doing.\n>\n> FWIW I was *so* tempted to write\n>\n> \t((a->kind ^ b->kind) & FILTER_REFS_DETACHED_HEAD)\n>\n> to make this code DRYer, but then, readers not intimately familiar with\n> Boolean arithmetic might not even know about the `^` operator, making the\n> code harder to read than necessary, too.\n>\n> > diff --git a/t/lib-gettext.sh b/t/lib-gettext.sh\n> > index 2139b427ca..de08d109dc 100644\n> > --- a/t/lib-gettext.sh\n> > +++ b/t/lib-gettext.sh\n> > @@ -25,23 +25,29 @@ then\n> > \t\tp\n> > \t\tq\n> > \t}')\n> > \t# is_IS.ISO8859-1 on Solaris and FreeBSD, is_IS.iso88591 on Debian\n> > \tis_IS_iso_locale=$(locale -a 2>/dev/null |\n> > \t\tsed -n '/^is_IS\\.[iI][sS][oO]8859-*1$/{\n> > \t\tp\n> > \t\tq\n> > \t}')\n> >\n> > -\t# Export them as an environment variable so the t0202/test.pl Perl\n> > -\t# test can use it too\n> > -\texport is_IS_locale is_IS_iso_locale\n> > +\tzh_CN_locale=$(locale -a 2>/dev/null |\n> > +\t\tsed -n '/^zh_CN\\.[uU][tT][fF]-*8$/{\n> > +\t\tp\n> > +\t\tq\n> > +\t}')\n> > +\n> > +\t# Export them as environment variables so other tests can use them\n> > +\t# too\n> > +\texport is_IS_locale is_IS_iso_locale zh_CN_locale\n> >\n> > \tif test -n \"$is_IS_locale\" &&\n> > \t\ttest $GIT_INTERNAL_GETTEXT_SH_SCHEME != \"fallthrough\"\n> > \tthen\n> > \t\t# Some of the tests need the reference Icelandic locale\n> > \t\ttest_set_prereq GETTEXT_LOCALE\n> >\n> > \t\t# Exporting for t0202/test.pl\n> > \t\tGETTEXT_LOCALE=1\n> > \t\texport GETTEXT_LOCALE\n> > @@ -53,11 +59,15 @@ then\n> > \tif test -n \"$is_IS_iso_locale\" &&\n> > \t\ttest $GIT_INTERNAL_GETTEXT_SH_SCHEME != \"fallthrough\"\n> > \tthen\n> > \t\t# Some of the tests need the reference Icelandic locale\n> > \t\ttest_set_prereq GETTEXT_ISO_LOCALE\n> >\n> > \t\tsay \"# lib-gettext: Found '$is_IS_iso_locale' as an is_IS ISO-8859-1 locale\"\n> > \telse\n> > \t\tsay \"# lib-gettext: No is_IS ISO-8859-1 locale available\"\n> > \tfi\n> > +\n> > +\tif test -z \"$zh_CN_locale\"; then\n> > +\t\tsay \"# lib-gettext: No zh_CN UTF-8 locale available\"\n> > +\tfi\n>\n> I wonder why this hunk, unlike the previous one, does not imitate the\n> is_IS handling closely.\n>\n> > diff --git a/t/t3207-branch-intl.sh b/t/t3207-branch-intl.sh\n> > new file mode 100755\n> > index 0000000000..9f6fcc7481\n> > --- /dev/null\n> > +++ b/t/t3207-branch-intl.sh\n> > @@ -0,0 +1,38 @@\n> > +#!/bin/sh\n> > +\n> > +test_description='git branch internationalization tests'\n> > +\n> > +. ./lib-gettext.sh\n> > +\n> > +test_expect_success 'init repo' '\n> > +\tgit init r1 &&\n>\n> Why?\n>\n> > +\ttouch r1/foo &&\n> > +\tgit -C r1 add foo &&\n> > +\tgit -C r1 commit -m foo\n> > +'\n>\n> Why not simply `test_commit foo`?\n>\n> > +test_expect_success 'detached head sorts before other branches' '\n> > +\t# Ref sorting logic should put detached heads before the other\n> > +\t# branches, but this is not automatic when a branch name sorts\n> > +\t# lexically before \"(\" or the full-width \"(\" (Unicode codepoint FF08).\n> > +\t# The latter case is nearly guaranteed for the Chinese locale.\n> > +\n> > +\tgit -C r1 checkout HEAD^{} -- &&\n> > +\tgit -C r1 branch !should_be_after_detached HEAD &&\n>\n> I am not sure that `!` is a wise choice, as it might not be a legal file\n> name character everywhere. A `.` or `-` might make more sense.\n>\n> > +\tLC_ALL=$zh_CN_locale LC_MESSAGES=$zh_CN_locale \\\n> > +\t\tgit -C r1 branch >actual &&\n> > +\tgit -C r1 checkout - &&\n>\n> Why call `checkout` after `branch`? That's unnecessary, we do not verify\n> anything after that call.\n>\n> > +\tawk \"\n> > +\t# We need full-width or half-width parens on the first line.\n> > +\tNR == 1 && (/[(].*[)]/ || /\\xef\\xbc\\x88.*\\xef\\xbc\\x89/) {\n> > +\t\tfound_head = 1;\n> > +\t}\n> > +\t/!should_be_after_detached/ {\n> > +\t\tfound_control_branch = 1;\n> > +\t}\n> > +\tEND { exit !found_head || !found_control_branch }\n> > +\t\" actual\n>\n> This might look beautiful for a fan of `awk`. For the vast majority of us,\n> this is not a good idea.\n>\n> Remember, you do *not* write those tests for your own pleasure, you do\n> *not* write those tests in order to help you catch problems while you\n> develop your patches, you do *not* develop these tests in order to just\n> catch future breakages.\n>\n> You *do* write those tests for *other* developers who you try to help in\n> preventing introducing regressions.\n>\n> As such, you *want* the tests to be\n>\n> - easy to understand for as wide a range of developers as you can make,\n>\n> - quick,\n>\n> - covering regressions, and *only* regressions,\n>\n> - helping diagnose *and* fix regressions.\n>\n> In the ideal case you won't even hear when developers found your test\n> helpful, and you will never, ever learn about regressions that have been\n> prevented.\n>\n> You most frequently will hear about your tests when they did not do their\n> job well.\n>\n> In this instance, I would have expected something like\n>\n> \ttest_expect_lines = 3 actual &&\n>\n> \thead -n 1 first &&\n> \ttest_i18ngrep \"detached HEAD\" first &&\n>\n> \ttail -n 1 last &&\n> \tgrep should_be_after last\n>\n> instead of the \"awk-ward\" code above.\n>\n> Ciao,\n> Johannes\n>\n> > +'\n> > +\n> > +test_done\n> > --\n> > 2.21.0\n> >\n> >\n>\n"},{"id":"376989","messageId":"20190610234918.GA10396@comcast.net","threadId":"51256","inReplyTo":"nycvar.QRO.7.76.6.1906090954510.789@QRFXGBC-DHN364S.ybpnyqbznva","subject":"Re: [RFC PATCH] ref-filter: sort detached HEAD lines firstly","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-06-10T23:49:18Z","receivedAt":"2019-06-10T23:50:03Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"On Sun, Jun 09, 2019 at 10:17:19AM +0200, Johannes Schindelin wrote:\n> > \tif (get_ref_atom_value(a, s->atom, &va, &err))\n> > \t\tdie(\"%s\", err.buf);\n> > \tif (get_ref_atom_value(b, s->atom, &vb, &err))\n> > \t\tdie(\"%s\", err.buf);\n> > \tstrbuf_release(&err);\n> > \tcmp_fn = s->ignore_case ? strcasecmp : strcmp;\n> > -\tif (s->version)\n> > +\tif (s->version) {\n> > \t\tcmp = versioncmp(va->s, vb->s);\n> > -\telse if (cmp_type == FIELD_STR)\n> > +\t} else if (cmp_type == FIELD_STR) {\n> \n> I find that it makes sense in general to suppress one's urges regarding\n> introducing `{ ... }` around one-liners when the patch does not actually\n> require it.\n> \n> For example, I found this patch harder than necessary to read because of\n> it.\n\nI understand the desire to make the patch itself clean, and I sometimes try to\ndo that to a fault, but the style as I understand it is to put { } around all\nif branches if only one branch requires it. Since I'm already modifying the\n\"else if (cmp_type == FIELD_STR)\" line, I decided to put the } at the start of\nthe line and modify the if (s->version) line as well. So only one line was\nmodified \"in excess.\" I think the temporary cost of the verbose patch is\njustified to keep the style consistent in narrow code fragments.\n\n> \n> > +\t\tif ((a->kind & FILTER_REFS_DETACHED_HEAD) !=\n> > +\t\t\t\t(b->kind & FILTER_REFS_DETACHED_HEAD)) {\n> \n> So in case that both are detached...\n> \n> > +\t\t\treturn (a->kind & FILTER_REFS_DETACHED_HEAD) ? -1 : 1;\n> > +\t\t}\n> > \t\tcmp = cmp_fn(va->s, vb->s);\n> \n> ... we compare their commit hashes, is that right? Might be worth a code\n> comment.\n> \n\nIt should not be possible to have two detached head lines, so adding a comment\nabout that may be distracting.\n\n> > -\telse {\n> > +\t} else {\n> \n> FWIW it would have been a much more obvious patch if it had done\n> \n> \tif (s->version)\n> \t\t[...]\n> +\telse if (cmp_type == FIELD_STR &&\n> +\t\t (a->kind & FILTER_REFS_DETACHED_HEAD ||\n> +\t\t b->kind & FILTER_REFS_DETACHED_HEAD))\n> +\t\treturn (a->kind & FILTER_REFS_DETACHED_HEAD) ? -1 : 1;\n\nBut that means that if a is detached, it is always a < b, even if both are\ndetached? That's probably right in practice, since there should only be one\ndetached head, but it's jarring to read a brittle cmp function.\n\n> \telse if (cmp_type == FIELD_STR)\n> \t\t[...]\n> \n> Maybe still worth doing.\n> \n> FWIW I was *so* tempted to write\n> \n> \t((a->kind ^ b->kind) & FILTER_REFS_DETACHED_HEAD)\n> \n> to make this code DRYer, but then, readers not intimately familiar with\n> Boolean arithmetic might not even know about the `^` operator, making the\n> code harder to read than necessary, too.\n\nI think I found a readable way to DRY:\n\n\t} else if (cmp_type == FIELD_STR) {\n\t\tconst int a_detached = a->kind & FILTER_REFS_DETACHED_HEAD;\n\n\t\t/*\n\t\t * When sorting by name, we should put \"detached\" head lines,\n\t\t * which are all the lines in parenthesis, before all others.\n\t\t * This usually is automatic, since \"(\" is before \"refs/\" and\n\t\t * \"remotes/\", but this does not hold for zh_CN, which uses\n\t\t * full-width parenthesis, so make the ordering explicit.\n\t\t */\n\t\tif (a_detached != (b->kind & FILTER_REFS_DETACHED_HEAD))\n\t\t\tcmp = a_detached ? -1 : 1;\n\t\telse\n\t\t\tcmp = cmp_fn(va->s, vb->s);\n\t} ...\n\n> >\n> > \tif test -n \"$is_IS_locale\" &&\n> > \t\ttest $GIT_INTERNAL_GETTEXT_SH_SCHEME != \"fallthrough\"\n> > \tthen\n> > \t\t# Some of the tests need the reference Icelandic locale\n> > \t\ttest_set_prereq GETTEXT_LOCALE\n> >\n> > \t\t# Exporting for t0202/test.pl\n> > \t\tGETTEXT_LOCALE=1\n> > \t\texport GETTEXT_LOCALE\n> > @@ -53,11 +59,15 @@ then\n> > \tif test -n \"$is_IS_iso_locale\" &&\n> > \t\ttest $GIT_INTERNAL_GETTEXT_SH_SCHEME != \"fallthrough\"\n> > \tthen\n> > \t\t# Some of the tests need the reference Icelandic locale\n> > \t\ttest_set_prereq GETTEXT_ISO_LOCALE\n> >\n> > \t\tsay \"# lib-gettext: Found '$is_IS_iso_locale' as an is_IS ISO-8859-1 locale\"\n> > \telse\n> > \t\tsay \"# lib-gettext: No is_IS ISO-8859-1 locale available\"\n> > \tfi\n> > +\n> > +\tif test -z \"$zh_CN_locale\"; then\n> > +\t\tsay \"# lib-gettext: No zh_CN UTF-8 locale available\"\n> > +\tfi\n> \n> I wonder why this hunk, unlike the previous one, does not imitate the\n> is_IS handling closely.\n\nIt was because I didn't have gettext set up properly and it was causing\nGIT_INTERNAL_GETTEXT_SH_SCHEME to be \"fallthrough\", despite the actual Git\noutput being translated. My set-up was quite broken so I don't think our intl\nutility and test code needs any fixing. I've handled this and the next version\nof the patch will have that fixed. (incidentally, this was why I originally\nmarked my patch RFC. Also incidentally, I'm using a Mac for development since\nthe most powerful machine I have access to is a Mac, so I've been jumping\nthrough some hoops to make that work.)\n\n> \n> > diff --git a/t/t3207-branch-intl.sh b/t/t3207-branch-intl.sh\n> > new file mode 100755\n> > index 0000000000..9f6fcc7481\n> > --- /dev/null\n> > +++ b/t/t3207-branch-intl.sh\n> > @@ -0,0 +1,38 @@\n> > +#!/bin/sh\n> > +\n> > +test_description='git branch internationalization tests'\n> > +\n> > +. ./lib-gettext.sh\n> > +\n> > +test_expect_success 'init repo' '\n> > +\tgit init r1 &&\n> \n> Why?\n\nYou mean why make a test repo?\n\n> \n> > +\ttouch r1/foo &&\n> > +\tgit -C r1 add foo &&\n> > +\tgit -C r1 commit -m foo\n> > +'\n> \n> Why not simply `test_commit foo`?\n\nGood idea, I'll use that.\n\n> \n> > +test_expect_success 'detached head sorts before other branches' '\n> > +\t# Ref sorting logic should put detached heads before the other\n> > +\t# branches, but this is not automatic when a branch name sorts\n> > +\t# lexically before \"(\" or the full-width \"(\" (Unicode codepoint FF08).\n> > +\t# The latter case is nearly guaranteed for the Chinese locale.\n> > +\n> > +\tgit -C r1 checkout HEAD^{} -- &&\n> > +\tgit -C r1 branch !should_be_after_detached HEAD &&\n> \n> I am not sure that `!` is a wise choice, as it might not be a legal file\n> name character everywhere. A `.` or `-` might make more sense.\n\nThe ! is actually meaningless, as I have come to observed, since the actual ref\nname used for comparison is not \"!should_be_after_detached\" but\n\"refs/heads/!should_be_after_detached\". So I'll remove it.\n\n> \n> > +\tLC_ALL=$zh_CN_locale LC_MESSAGES=$zh_CN_locale \\\n> > +\t\tgit -C r1 branch >actual &&\n> > +\tgit -C r1 checkout - &&\n> \n> Why call `checkout` after `branch`? That's unnecessary, we do not verify\n> anything after that call.\n\nIt's to get the repo into a neutral state in case an additional testcase is\nadded in the future.\n\n> \n> > +\tawk \"\n> > +\t# We need full-width or half-width parens on the first line.\n> > +\tNR == 1 && (/[(].*[)]/ || /\\xef\\xbc\\x88.*\\xef\\xbc\\x89/) {\n> > +\t\tfound_head = 1;\n> > +\t}\n> > +\t/!should_be_after_detached/ {\n> > +\t\tfound_control_branch = 1;\n> > +\t}\n> > +\tEND { exit !found_head || !found_control_branch }\n> > +\t\" actual\n> \n> This might look beautiful for a fan of `awk`. For the vast majority of us,\n> this is not a good idea.\n> \n> Remember, you do *not* write those tests for your own pleasure, you do\n> *not* write those tests in order to help you catch problems while you\n> develop your patches, you do *not* develop these tests in order to just\n> catch future breakages.\n> \n> You *do* write those tests for *other* developers who you try to help in\n> preventing introducing regressions.\n> \n> As such, you *want* the tests to be\n> \n> - easy to understand for as wide a range of developers as you can make,\n> \n> - quick,\n> \n> - covering regressions, and *only* regressions,\n> \n> - helping diagnose *and* fix regressions.\n> \n> In the ideal case you won't even hear when developers found your test\n> helpful, and you will never, ever learn about regressions that have been\n> prevented.\n> \n> You most frequently will hear about your tests when they did not do their\n> job well.\n> \n> In this instance, I would have expected something like\n> \n> \ttest_expect_lines = 3 actual &&\n> \n> \thead -n 1 first &&\n> \ttest_i18ngrep \"detached HEAD\" first &&\n\nTangential to your point, but the test_i18ngrep actually won't work since all\nit does is *not* grep when the text is translated. But I want the text to use\nthe genuine zh_CN translation.\n\n> \n> \ttail -n 1 last &&\n> \tgrep should_be_after last\n> \n> instead of the \"awk-ward\" code above.\n\nAll fair points, and I may have been carried away when I wrote the awk code. I\nliked using awk since it matched my mental model at the time, which was\nprocedural, as opposed to your proposed pure sh implementation, which is more\ndeclarative.\n\nHere is the new version of the test:\n\ntest_expect_success GETTEXT_ZH_LOCALE 'detached head sorts before branches' '\n\t# Ref sorting logic should put detached heads before the other\n\t# branches, but this is not automatic when a branch name sorts\n\t# lexically before \"(\" or the full-width \"(\" (Unicode codepoint FF08).\n\t# The latter case is nearly guaranteed for the Chinese locale.\n\n\tgit -C r1 checkout HEAD^{} -- &&\n\tLC_ALL=$zh_CN_locale LC_MESSAGES=$zh_CN_locale \\\n\t\tgit -C r1 branch >actual &&\n\tgit -C r1 checkout - &&\n\n\thead -n 1 actual >first &&\n\t# The first line should be enclosed by full-width parenthesis.\n\tgrep '$'\\xef\\xbc\\x88.*\\xef\\xbc\\x89'' first &&\n\tgrep master actual\n'\n\n> \n> Ciao,\n> Johannes\n>\n\nThank you for the thoughtful feedback.\n"},{"id":"376991","messageId":"20190611004106.GB64137@google.com","threadId":"51256","inReplyTo":"20190610234918.GA10396@comcast.net","subject":"Re: [RFC PATCH] ref-filter: sort detached HEAD lines firstly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-06-11T00:41:06Z","receivedAt":"2019-06-11T00:41:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMatthew DeVore wrote:\n> On Sun, Jun 09, 2019 at 10:17:19AM +0200, Johannes Schindelin wrote:\n\n>> I find that it makes sense in general to suppress one's urges regarding\n>> introducing `{ ... }` around one-liners when the patch does not actually\n>> require it.\n>>\n>> For example, I found this patch harder than necessary to read because of\n>> it.\n>\n> I understand the desire to make the patch itself clean, and I sometimes try to\n> do that to a fault, but the style as I understand it is to put { } around all\n> if branches if only one branch requires it. Since I'm already modifying the\n> \"else if (cmp_type == FIELD_STR)\" line, I decided to put the } at the start of\n> the line and modify the if (s->version) line as well. So only one line was\n> modified \"in excess.\" I think the temporary cost of the verbose patch is\n> justified to keep the style consistent in narrow code fragments.\n\nGit seems to be inconsistent about this. Documentation/CodingGuidelines\nsays\n\n - When there are multiple arms to a conditional and some of them\n require braces, enclose even a single line block in braces for\n consistency. E.g.:\n\nso you have some cover from there (and it matches what I'm used to,\ntoo). :)\n\n[...]\n>>> +\tLC_ALL=$zh_CN_locale LC_MESSAGES=$zh_CN_locale \\\n>>> +\t\tgit -C r1 branch >actual &&\n>>> +\tgit -C r1 checkout - &&\n>>\n>> Why call `checkout` after `branch`? That's unnecessary, we do not verify\n>> anything after that call.\n>\n> It's to get the repo into a neutral state in case an additional testcase is\n> added in the future.\n\nFor this kind of thing, we tend to use test_when_finished so that the\ntest ends up in a clean state even if it fails.\n\n[...]\n> test_expect_success GETTEXT_ZH_LOCALE 'detached head sorts before branches' '\n> \t# Ref sorting logic should put detached heads before the other\n> \t# branches, but this is not automatic when a branch name sorts\n> \t# lexically before \"(\" or the full-width \"(\" (Unicode codepoint FF08).\n> \t# The latter case is nearly guaranteed for the Chinese locale.\n>\n> \tgit -C r1 checkout HEAD^{} -- &&\n> \tLC_ALL=$zh_CN_locale LC_MESSAGES=$zh_CN_locale \\\n> \t\tgit -C r1 branch >actual &&\n> \tgit -C r1 checkout - &&\n>\n> \thead -n 1 actual >first &&\n> \t# The first line should be enclosed by full-width parenthesis.\n> \tgrep '$'\\xef\\xbc\\x88.*\\xef\\xbc\\x89'' first &&\n\nnit: older shells do not know how to do $'\\x01' interpolation.\nProbably best to use the raw UTF-8 directly here (it will be more\nreadable anyway).\n\nThanks,\nJonathan\n"},{"id":"377014","messageId":"20190611164834.GA58112@comcast.net","threadId":"51256","inReplyTo":"20190611004106.GB64137@google.com","subject":"Re: [RFC PATCH] ref-filter: sort detached HEAD lines firstly","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-06-11T16:48:34Z","receivedAt":"2019-06-11T16:49:16Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"On Mon, Jun 10, 2019 at 05:41:06PM -0700, Jonathan Nieder wrote:\n> Git seems to be inconsistent about this. Documentation/CodingGuidelines\n> says\n> \n> - When there are multiple arms to a conditional and some of them\n> require braces, enclose even a single line block in braces for\n> consistency. E.g.:\n> \n> so you have some cover from there (and it matches what I'm used to,\n> too). :)\n\nThanks for finding that.\n\n> \n> [...]\n> >>> +\tLC_ALL=$zh_CN_locale LC_MESSAGES=$zh_CN_locale \\\n> >>> +\t\tgit -C r1 branch >actual &&\n> >>> +\tgit -C r1 checkout - &&\n> >>\n> >> Why call `checkout` after `branch`? That's unnecessary, we do not verify\n> >> anything after that call.\n> >\n> > It's to get the repo into a neutral state in case an additional testcase is\n> > added in the future.\n> \n> For this kind of thing, we tend to use test_when_finished so that the\n> test ends up in a clean state even if it fails.\n\nDone. That will be used in the next roll-up.\n\n> > \thead -n 1 actual >first &&\n> > \t# The first line should be enclosed by full-width parenthesis.\n> > \tgrep '$'\\xef\\xbc\\x88.*\\xef\\xbc\\x89'' first &&\n> \n> nit: older shells do not know how to do $'\\x01' interpolation.\n> Probably best to use the raw UTF-8 directly here (it will be more\n> readable anyway).\n\nGood point. I suppose we don't have to worry about dev's editors screwing up\nencoding since modern editors make this kind of thing easy to configure (and I\nsuspect that all sane editors use UTF-8 as the default or at least won't mangle\nit...)\n"},{"id":"377016","messageId":"cover.1560277373.git.matvore@google.com","threadId":"51256","inReplyTo":"faaa9a3d6ba66d77cc2a8eab438d1bfc8f762fa1.1559857032.git.matvore@google.com","subject":"[PATCH v2 0/1] Sort detached HEAD lines firstly","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2019-06-11T18:28:17Z","receivedAt":"2019-06-11T18:28:34Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"This version improves upon the previous:\n\n - fixes a bug where reverse-sorting didn't work with (HEAD detached) lines, and\n adds a test\n - makes implementation code a little DRYer\n - uses test_when_finished where applicable\n - makes zh_CN locale detection look more like the is_IS locale detection\n\nThanks,\n\nMatthew DeVore (1):\n ref-filter: sort detached HEAD lines firstly\n\n ref-filter.c | 20 ++++++++++++++++----\n t/lib-gettext.sh | 22 +++++++++++++++++++---\n t/t3207-branch-intl.sh | 41 +++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 76 insertions(+), 7 deletions(-)\n create mode 100755 t/t3207-branch-intl.sh\n\n-- \n2.21.0\n\n"},{"id":"377017","messageId":"cf0246a5cce6cbd9b4a1fd1eefa0f5cbc2cfcaf0.1560277373.git.matvore@google.com","threadId":"51256","inReplyTo":"cover.1560277373.git.matvore@google.com","subject":"[PATCH v2 1/1] ref-filter: sort detached HEAD lines firstly","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2019-06-11T18:28:18Z","receivedAt":"2019-06-11T18:28:40Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"Before this patch, \"git branch\" would put \"(HEAD detached...)\" and \"(no\nbranch, rebasing...)\" lines before all the other branches *in most\ncases* and only because of the fact that \"(\" is a low codepoint. This\nwould not hold in the Chinese locale, which uses a full-width \"(\" symbol\n(codepoint FF08). This meant that the detached HEAD line would appear\nafter all local refs and even after the remote refs if there were any.\n\nDeliberately sort the detached HEAD refs before other refs when sorting\nby refname rather than rely on codepoint subtleties.\n\nHelped-by: Johannes Schindelin \nHelped-by: Jonathan Nieder \nSigned-off-by: Matthew DeVore \n---\n ref-filter.c | 20 ++++++++++++++++----\n t/lib-gettext.sh | 22 +++++++++++++++++++---\n t/t3207-branch-intl.sh | 41 +++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 76 insertions(+), 7 deletions(-)\n create mode 100755 t/t3207-branch-intl.sh\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 8500671bc6..056d21d666 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2157,25 +2157,37 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n \tcmp_type cmp_type = used_atom[s->atom].type;\n \tint (*cmp_fn)(const char *, const char *);\n \tstruct strbuf err = STRBUF_INIT;\n \n \tif (get_ref_atom_value(a, s->atom, &va, &err))\n \t\tdie(\"%s\", err.buf);\n \tif (get_ref_atom_value(b, s->atom, &vb, &err))\n \t\tdie(\"%s\", err.buf);\n \tstrbuf_release(&err);\n \tcmp_fn = s->ignore_case ? strcasecmp : strcmp;\n-\tif (s->version)\n+\tif (s->version) {\n \t\tcmp = versioncmp(va->s, vb->s);\n-\telse if (cmp_type == FIELD_STR)\n-\t\tcmp = cmp_fn(va->s, vb->s);\n-\telse {\n+\t} else if (cmp_type == FIELD_STR) {\n+\t\tconst int a_detached = a->kind & FILTER_REFS_DETACHED_HEAD;\n+\n+\t\t/*\n+\t\t * When sorting by name, we should put \"detached\" head lines,\n+\t\t * which are all the lines in parenthesis, before all others.\n+\t\t * This usually is automatic, since \"(\" is before \"refs/\" and\n+\t\t * \"remotes/\", but this does not hold for zh_CN, which uses\n+\t\t * full-width parenthesis, so make the ordering explicit.\n+\t\t */\n+\t\tif (a_detached != (b->kind & FILTER_REFS_DETACHED_HEAD))\n+\t\t\tcmp = a_detached ? -1 : 1;\n+\t\telse\n+\t\t\tcmp = cmp_fn(va->s, vb->s);\n+\t} else {\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\telse\n \t\t\tcmp = 1;\n \t}\n \n \treturn (s->reverse) ? -cmp : cmp;\n }\ndiff --git a/t/lib-gettext.sh b/t/lib-gettext.sh\nindex 2139b427ca..1adf1d4c31 100644\n--- a/t/lib-gettext.sh\n+++ b/t/lib-gettext.sh\n@@ -25,23 +25,29 @@ then\n \t\tp\n \t\tq\n \t}')\n \t# is_IS.ISO8859-1 on Solaris and FreeBSD, is_IS.iso88591 on Debian\n \tis_IS_iso_locale=$(locale -a 2>/dev/null |\n \t\tsed -n '/^is_IS\\.[iI][sS][oO]8859-*1$/{\n \t\tp\n \t\tq\n \t}')\n \n-\t# Export them as an environment variable so the t0202/test.pl Perl\n-\t# test can use it too\n-\texport is_IS_locale is_IS_iso_locale\n+\tzh_CN_locale=$(locale -a 2>/dev/null |\n+\t\tsed -n '/^zh_CN\\.[uU][tT][fF]-*8$/{\n+\t\tp\n+\t\tq\n+\t}')\n+\n+\t# Export them as environment variables so other tests can use them\n+\t# too\n+\texport is_IS_locale is_IS_iso_locale zh_CN_locale\n \n \tif test -n \"$is_IS_locale\" &&\n \t\ttest $GIT_INTERNAL_GETTEXT_SH_SCHEME != \"fallthrough\"\n \tthen\n \t\t# Some of the tests need the reference Icelandic locale\n \t\ttest_set_prereq GETTEXT_LOCALE\n \n \t\t# Exporting for t0202/test.pl\n \t\tGETTEXT_LOCALE=1\n \t\texport GETTEXT_LOCALE\n@@ -53,11 +59,21 @@ then\n \tif test -n \"$is_IS_iso_locale\" &&\n \t\ttest $GIT_INTERNAL_GETTEXT_SH_SCHEME != \"fallthrough\"\n \tthen\n \t\t# Some of the tests need the reference Icelandic locale\n \t\ttest_set_prereq GETTEXT_ISO_LOCALE\n \n \t\tsay \"# lib-gettext: Found '$is_IS_iso_locale' as an is_IS ISO-8859-1 locale\"\n \telse\n \t\tsay \"# lib-gettext: No is_IS ISO-8859-1 locale available\"\n \tfi\n+\n+\tif test -n \"$zh_CN_locale\" &&\n+\t\ttest $GIT_INTERNAL_GETTEXT_SH_SCHEME != \"fallthrough\"\n+\tthen\n+\t\ttest_set_prereq GETTEXT_ZH_LOCALE\n+\n+\t\tsay \"# lib-gettext: Found '$zh_CN_locale' as a zh_CN UTF-8 locale\"\n+\telse\n+\t\tsay \"# lib-gettext: No zh_CN UTF-8 locale available\"\n+\tfi\n fi\ndiff --git a/t/t3207-branch-intl.sh b/t/t3207-branch-intl.sh\nnew file mode 100755\nindex 0000000000..a46538188c\n--- /dev/null\n+++ b/t/t3207-branch-intl.sh\n@@ -0,0 +1,41 @@\n+#!/bin/sh\n+\n+test_description='git branch internationalization tests'\n+\n+. ./lib-gettext.sh\n+\n+test_expect_success 'init repo' '\n+\tgit init r1 &&\n+\ttest_commit -C r1 first\n+'\n+\n+test_expect_success GETTEXT_ZH_LOCALE 'detached head sorts before branches' '\n+\t# Ref sorting logic should put detached heads before the other\n+\t# branches, but this is not automatic when a branch name sorts\n+\t# lexically before \"(\" or the full-width \"(\" (Unicode codepoint FF08).\n+\t# The latter case is nearly guaranteed for the Chinese locale.\n+\n+\ttest_when_finished \"git -C r1 checkout master\" &&\n+\n+\tgit -C r1 checkout HEAD^{} -- &&\n+\tLC_ALL=$zh_CN_locale LC_MESSAGES=$zh_CN_locale \\\n+\t\tgit -C r1 branch >actual &&\n+\n+\thead -n 1 actual >first &&\n+\t# The first line should be enclosed by full-width parenthesis.\n+\tgrep \"（.*）\" first &&\n+\tgrep master actual\n+'\n+\n+test_expect_success 'detached head honors reverse sorting' '\n+\ttest_when_finished \"git -C r1 checkout master\" &&\n+\n+\tgit -C r1 checkout HEAD^{} -- &&\n+\tgit -C r1 branch --sort=-refname >actual &&\n+\n+\thead -n 1 actual >first &&\n+\tgrep master first &&\n+\ttest_i18ngrep \"HEAD detached\" actual\n+'\n+\n+test_done\n-- \n2.21.0\n\n"},{"id":"377022","messageId":"xmqqftoflx09.fsf@gitster-ct.c.googlers.com","threadId":"51256","inReplyTo":"20190611004106.GB64137@google.com","subject":"Re: [RFC PATCH] ref-filter: sort detached HEAD lines firstly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-11T19:53:10Z","receivedAt":"2019-06-11T19:53:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Git seems to be inconsistent about this. Documentation/CodingGuidelines\n> says\n>\n> - When there are multiple arms to a conditional and some of them\n> require braces, enclose even a single line block in braces for\n> consistency. E.g.:\n>\n> so you have some cover from there (and it matches what I'm used to,\n> too). :)\n\nYup, it took us for quite some time before we settled on that rule\nand wrote it down, so there are some lines that predate it *and*\nhave survived.\n"},{"id":"377025","messageId":"xmqq7e9rlw72.fsf@gitster-ct.c.googlers.com","threadId":"51256","inReplyTo":"cf0246a5cce6cbd9b4a1fd1eefa0f5cbc2cfcaf0.1560277373.git.matvore@google.com","subject":"Re: [PATCH v2 1/1] ref-filter: sort detached HEAD lines firstly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-11T20:10:41Z","receivedAt":"2019-06-11T20:10:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthew DeVore writes:\n\n> -\tif (s->version)\n> +\tif (s->version) {\n> \t\tcmp = versioncmp(va->s, vb->s);\n> -\telse if (cmp_type == FIELD_STR)\n> -\t\tcmp = cmp_fn(va->s, vb->s);\n> -\telse {\n\nAh, this must be the patch noise Jonathan was (half) complaining\nabout. It does make it a bit distracting to read the patch but the\nresulting code is of course easier to follow ;-).\n\n> +\t} else if (cmp_type == FIELD_STR) {\n> +\t\tconst int a_detached = a->kind & FILTER_REFS_DETACHED_HEAD;\n> +\n> +\t\t/*\n> +\t\t * When sorting by name, we should put \"detached\" head lines,\n> +\t\t * which are all the lines in parenthesis, before all others.\n> +\t\t * This usually is automatic, since \"(\" is before \"refs/\" and\n> +\t\t * \"remotes/\", but this does not hold for zh_CN, which uses\n> +\t\t * full-width parenthesis, so make the ordering explicit.\n> +\t\t */\n> +\t\tif (a_detached != (b->kind & FILTER_REFS_DETACHED_HEAD))\n> +\t\t\tcmp = a_detached ? -1 : 1;\n\nSo, comparing a detached and an undetached ones, the detached side\nalways sorts lower. Good. And ...\n\n> +\t\telse\n> +\t\t\tcmp = cmp_fn(va->s, vb->s);\n\n... otherwise we compare the string using the given function.\n\nSounds sensible. Will queue.\n"},{"id":"377110","messageId":"nycvar.QRO.7.76.6.1906122118380.789@QRFXGBC-DHN364S.ybpnyqbznva","threadId":"51256","inReplyTo":"cf0246a5cce6cbd9b4a1fd1eefa0f5cbc2cfcaf0.1560277373.git.matvore@google.com","subject":"Re: [PATCH v2 1/1] ref-filter: sort detached HEAD lines firstly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-06-12T19:51:33Z","receivedAt":"2019-06-12T19:51:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Matthew,\n\nOn Tue, 11 Jun 2019, Matthew DeVore wrote:\n\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 8500671bc6..056d21d666 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -2157,25 +2157,37 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n> \tcmp_type cmp_type = used_atom[s->atom].type;\n> \tint (*cmp_fn)(const char *, const char *);\n> \tstruct strbuf err = STRBUF_INIT;\n>\n> \tif (get_ref_atom_value(a, s->atom, &va, &err))\n> \t\tdie(\"%s\", err.buf);\n> \tif (get_ref_atom_value(b, s->atom, &vb, &err))\n> \t\tdie(\"%s\", err.buf);\n> \tstrbuf_release(&err);\n> \tcmp_fn = s->ignore_case ? strcasecmp : strcmp;\n> -\tif (s->version)\n> +\tif (s->version) {\n> \t\tcmp = versioncmp(va->s, vb->s);\n> -\telse if (cmp_type == FIELD_STR)\n> -\t\tcmp = cmp_fn(va->s, vb->s);\n> -\telse {\n> +\t} else if (cmp_type == FIELD_STR) {\n\nI still think that this slipped-in `{` makes this patch harder to read\nthan necessary.\n\nYour argument that you introduce the first curlies in an `else` block does\nnot hold, as the removed `else {` line above demonstrates quite clearly.\n\nBut you seem dead set to do it nevertheless, so I'll save my breath.\n\n> +\t\tconst int a_detached = a->kind & FILTER_REFS_DETACHED_HEAD;\n> +\n> +\t\t/*\n> +\t\t * When sorting by name, we should put \"detached\" head lines,\n> +\t\t * which are all the lines in parenthesis, before all others.\n> +\t\t * This usually is automatic, since \"(\" is before \"refs/\" and\n> +\t\t * \"remotes/\", but this does not hold for zh_CN, which uses\n> +\t\t * full-width parenthesis, so make the ordering explicit.\n> +\t\t */\n> +\t\tif (a_detached != (b->kind & FILTER_REFS_DETACHED_HEAD))\n> +\t\t\tcmp = a_detached ? -1 : 1;\n> +\t\telse\n> +\t\t\tcmp = cmp_fn(va->s, vb->s);\n> +\t} else {\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\telse\n> \t\t\tcmp = 1;\n> \t}\n>\n> \treturn (s->reverse) ? -cmp : cmp;\n> }\n> diff --git a/t/lib-gettext.sh b/t/lib-gettext.sh\n> index 2139b427ca..1adf1d4c31 100644\n> --- a/t/lib-gettext.sh\n> +++ b/t/lib-gettext.sh\n> @@ -25,23 +25,29 @@ then\n> \t\tp\n> \t\tq\n> \t}')\n> \t# is_IS.ISO8859-1 on Solaris and FreeBSD, is_IS.iso88591 on Debian\n> \tis_IS_iso_locale=$(locale -a 2>/dev/null |\n> \t\tsed -n '/^is_IS\\.[iI][sS][oO]8859-*1$/{\n> \t\tp\n> \t\tq\n> \t}')\n>\n> -\t# Export them as an environment variable so the t0202/test.pl Perl\n> -\t# test can use it too\n> -\texport is_IS_locale is_IS_iso_locale\n> +\tzh_CN_locale=$(locale -a 2>/dev/null |\n> +\t\tsed -n '/^zh_CN\\.[uU][tT][fF]-*8$/{\n> +\t\tp\n> +\t\tq\n> +\t}')\n> +\n> +\t# Export them as environment variables so other tests can use them\n> +\t# too\n> +\texport is_IS_locale is_IS_iso_locale zh_CN_locale\n>\n> \tif test -n \"$is_IS_locale\" &&\n> \t\ttest $GIT_INTERNAL_GETTEXT_SH_SCHEME != \"fallthrough\"\n> \tthen\n> \t\t# Some of the tests need the reference Icelandic locale\n> \t\ttest_set_prereq GETTEXT_LOCALE\n>\n> \t\t# Exporting for t0202/test.pl\n> \t\tGETTEXT_LOCALE=1\n> \t\texport GETTEXT_LOCALE\n> @@ -53,11 +59,21 @@ then\n> \tif test -n \"$is_IS_iso_locale\" &&\n> \t\ttest $GIT_INTERNAL_GETTEXT_SH_SCHEME != \"fallthrough\"\n> \tthen\n> \t\t# Some of the tests need the reference Icelandic locale\n> \t\ttest_set_prereq GETTEXT_ISO_LOCALE\n>\n> \t\tsay \"# lib-gettext: Found '$is_IS_iso_locale' as an is_IS ISO-8859-1 locale\"\n> \telse\n> \t\tsay \"# lib-gettext: No is_IS ISO-8859-1 locale available\"\n> \tfi\n> +\n> +\tif test -n \"$zh_CN_locale\" &&\n> +\t\ttest $GIT_INTERNAL_GETTEXT_SH_SCHEME != \"fallthrough\"\n> +\tthen\n> +\t\ttest_set_prereq GETTEXT_ZH_LOCALE\n> +\n> +\t\tsay \"# lib-gettext: Found '$zh_CN_locale' as a zh_CN UTF-8 locale\"\n> +\telse\n> +\t\tsay \"# lib-gettext: No zh_CN UTF-8 locale available\"\n> +\tfi\n> fi\n> diff --git a/t/t3207-branch-intl.sh b/t/t3207-branch-intl.sh\n> new file mode 100755\n> index 0000000000..a46538188c\n> --- /dev/null\n> +++ b/t/t3207-branch-intl.sh\n> @@ -0,0 +1,41 @@\n> +#!/bin/sh\n> +\n> +test_description='git branch internationalization tests'\n> +\n> +. ./lib-gettext.sh\n> +\n> +test_expect_success 'init repo' '\n> +\tgit init r1 &&\n> +\ttest_commit -C r1 first\n> +'\n\nI still see absolutely no need for initializing `r1`. Every test script in\nGit's test suite starts out with a fully initialized repository, no `git\ninit` necessary. Therefore, this test case seems to have an unnecessary\n`git init` and multiple unnecessary `-C r1` options that make the script\nquite noisy.\n\nI mean, you initialize that `r1`, work on it exclusively, and completely\nignore the repository that has been initialized in `.git` for you.\n\n> +test_expect_success GETTEXT_ZH_LOCALE 'detached head sorts before branches' '\n> +\t# Ref sorting logic should put detached heads before the other\n> +\t# branches, but this is not automatic when a branch name sorts\n> +\t# lexically before \"(\" or the full-width \"(\" (Unicode codepoint FF08).\n> +\t# The latter case is nearly guaranteed for the Chinese locale.\n> +\n> +\ttest_when_finished \"git -C r1 checkout master\" &&\n> +\n> +\tgit -C r1 checkout HEAD^{} -- &&\n\n`HEAD^0` is a much more canonical way to say this. However, if you want\nyour test case to be easy to understand (and that is your goal, too,\nright, not only mine?), you will instead use\n\n\tgit checkout --detach\n\n> +\tLC_ALL=$zh_CN_locale LC_MESSAGES=$zh_CN_locale \\\n> +\t\tgit -C r1 branch >actual &&\n> +\n> +\thead -n 1 actual >first &&\n> +\t# The first line should be enclosed by full-width parenthesis.\n> +\tgrep \"（.*）\" first &&\n\nI wonder whether it is a good idea to pretend that we can pass arbitrary\nbyte sequences to `grep`, independent of the current locale. On Windows,\nthis does not hold true, for example.\n\nIt would probably make more sense to store a support file in t/t3207/,\nmuch like it is done in t3900.\n\nAnd once you do that, you can simply `test_cmp t3207/first first`. No\nneed to `grep` for `master` in addition:\n\n> +\tgrep master actual\n> +'\n> +\n> +test_expect_success 'detached head honors reverse sorting' '\n> +\ttest_when_finished \"git -C r1 checkout master\" &&\n\nHmm. I see you also did that in the previous test case, but since you\nimmediately detach the HEAD, I have to ask:\n\n- why? Why do you insist on switching back to `master` after the test case\n finished?\n- Why even bother to call `git checkout --detach` in anything but the very\n first test case, whose purpose it is to set things up for the subsequent\n test cases, after all?\n\n> +\n> +\tgit -C r1 checkout HEAD^{} -- &&\n> +\tgit -C r1 branch --sort=-refname >actual &&\n> +\n> +\thead -n 1 actual >first &&\n> +\tgrep master first &&\n> +\ttest_i18ngrep \"HEAD detached\" actual\n\nFunny, reading the test case's title, I would have expected to read\ninstead:\n\n\techo \"* HEAD detached\" >expect &&\n\ttail -n 1 actual >last &&\n\ttest_cmp expect last\n\nIn all, the test script should read more like this:\n\n\ttest_expect_success 'setup' '\n\t\ttest_commit first &&\n\t\tgit checkout --detach\n\t'\n\n\t# [... long comment here, does not need to be hidden and indented\n\t# inside...]\n\ttest_expect_success GETTEXT_ZH_LOCALE 'detached HEAD sorts first' '\n\t\tLC_ALL=$zh_CN_locale LC_MESSAGES=$zh_CN_locale git branch >actual &&\n\n\t\thead -n 1 first &&\n\t\ttest_cmp \"$TEST_DIRECTORY/../t3207/first\" first\n\t'\n\n\ttest_expect_success 'detached HEAD reverse-sorts last' '\n\t\tgit branch --sort=-refname >actual &&\n\n\t\techo \"* HEAD detached\" >expect &&\n\t\ttail -n 1 actual >last &&\n\t\ttest_cmp expect last\n\t'\n\nIt is quite possible that this can be simplified even further, i.e. made\neven easier to understand for developers in the unfortunate situation of\nhaving to debug a regression (which is the entire goal of a well-written\nregression test: to help, rather than just to force, developers to debug\nregressions).\n\nGranted, the simpler form might look like it took less effort to write\nthan the complicated one. People with some experience in software\ndevelopment will understand the opposite to be true, though.\n\nCiao,\nDscho\n\n> +'\n> +\n> +test_done\n> --\n> 2.21.0\n>\n>\n"},{"id":"377115","messageId":"xmqqo932ik7y.fsf@gitster-ct.c.googlers.com","threadId":"51256","inReplyTo":"xmqq7e9rlw72.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/1] ref-filter: sort detached HEAD lines firstly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-12T21:09:53Z","receivedAt":"2019-06-12T21:10:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +\t\t/*\n>> +\t\t * When sorting by name, we should put \"detached\" head lines,\n>> +\t\t * which are all the lines in parenthesis, before all others.\n>> +\t\t * This usually is automatic, since \"(\" is before \"refs/\" and\n>> +\t\t * \"remotes/\", but this does not hold for zh_CN, which uses\n>> +\t\t * full-width parenthesis, so make the ordering explicit.\n>> +\t\t */\n>> +\t\tif (a_detached != (b->kind & FILTER_REFS_DETACHED_HEAD))\n>> +\t\t\tcmp = a_detached ? -1 : 1;\n>\n> So, comparing a detached and an undetached ones, the detached side\n> always sorts lower. Good. And ...\n>\n>> +\t\telse\n>> +\t\t\tcmp = cmp_fn(va->s, vb->s);\n>\n> ... otherwise we compare the string using the given function.\n>\n> Sounds sensible. Will queue.\n\nStepping back a bit, why are we even allowing the surrounding ()\npair to be futzed by the translators?\n\nIOW, shouldn't our code more like this from the beginning, with or\nwithout Chinese translation?\n\nWith a bit more work, we may even be able to lose \"make sure this\nmatches the one in wt-status.c\" comment as losing the leading '('\nwould take us one step closer to have an identical string here as we\nhave in wt-status.c\n\n ref-filter.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 8500671bc6..7e4705fcb2 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1459,20 +1459,22 @@ char *get_head_description(void)\n \t\tstrbuf_addf(&desc, _(\"(no branch, bisect started on %s)\"),\n \t\t\t state.branch);\n \telse if (state.detached_from) {\n+\t\tstrbuf_addch(&desc, '(');\n \t\tif (state.detached_at)\n \t\t\t/*\n \t\t\t * TRANSLATORS: make sure this matches \"HEAD\n \t\t\t * detached at \" in wt-status.c\n \t\t\t */\n-\t\t\tstrbuf_addf(&desc, _(\"(HEAD detached at %s)\"),\n-\t\t\t\tstate.detached_from);\n+\t\t\tstrbuf_addf(&desc, _(\"HEAD detached at %s\"),\n+\t\t\t\t state.detached_from);\n \t\telse\n \t\t\t/*\n \t\t\t * TRANSLATORS: make sure this matches \"HEAD\n \t\t\t * detached from \" in wt-status.c\n \t\t\t */\n-\t\t\tstrbuf_addf(&desc, _(\"(HEAD detached from %s)\"),\n+\t\t\tstrbuf_addf(&desc, _(\"HEAD detached from %s\"),\n \t\t\t\tstate.detached_from);\n+\t\tstrbuf_addch(&desc, ')');\n \t}\n \telse\n \t\tstrbuf_addstr(&desc, _(\"(no branch)\"));\n\n\n"},{"id":"377143","messageId":"20190613015616.GG58112@comcast.net","threadId":"51256","inReplyTo":"xmqqo932ik7y.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/1] ref-filter: sort detached HEAD lines firstly","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-06-13T01:56:16Z","receivedAt":"2019-06-13T16:55:37Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"On Wed, Jun 12, 2019 at 02:09:53PM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> Stepping back a bit, why are we even allowing the surrounding ()\n> pair to be futzed by the translators?\n> \n> IOW, shouldn't our code more like this from the beginning, with or\n> without Chinese translation?\n> \n> With a bit more work, we may even be able to lose \"make sure this\n> matches the one in wt-status.c\" comment as losing the leading '('\n> would take us one step closer to have an identical string here as we\n> have in wt-status.c\n\nI think my previous e-mail didn't make it to the public list, maybe since it\ncontained non-ASCII text. The gist of that mail was that we have full-width\nparens in various zh_CN strings so it seems hacky to make just this one be\nhalf-width for the sake of code simplicity.\n\nGiving this a bit more thought now, perhaps the fact that we want to be in-sync\nwith the string in wt-status.c, combined with the fact that we already have \"*\"\nas half-width metacharacter in the \"git branch\" output, is a good enough excuse\nto drop \"()\" out of the translatable string, as your patch does.\n"},{"id":"377144","messageId":"20190613165809.GA13031@sigill.intra.peff.net","threadId":"51256","inReplyTo":"nycvar.QRO.7.76.6.1906122118380.789@QRFXGBC-DHN364S.ybpnyqbznva","subject":"Re: [PATCH v2 1/1] ref-filter: sort detached HEAD lines firstly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-13T16:58:09Z","receivedAt":"2019-06-13T16:58:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 12, 2019 at 09:51:33PM +0200, Johannes Schindelin wrote:\n\n> > +\thead -n 1 actual >first &&\n> > +\t# The first line should be enclosed by full-width parenthesis.\n> > +\tgrep \"（.*）\" first &&\n> \n> I wonder whether it is a good idea to pretend that we can pass arbitrary\n> byte sequences to `grep`, independent of the current locale. On Windows,\n> this does not hold true, for example.\n> \n> It would probably make more sense to store a support file in t/t3207/,\n> much like it is done in t3900.\n> \n> And once you do that, you can simply `test_cmp t3207/first first`. No\n> need to `grep` for `master` in addition:\n\nI was just writing a similar response in another part of the thread, and\nfound this. :)\n\nIn addition to grep portability problems, IMHO the source with the raw\nUTF-8 characters is harder to read. Even if your editor and terminal\nsupport UTF-8, people without the right fonts will just get a bunch of\nempty boxes. And when debugging, you often care about the raw bytes\nanyway (e.g., when there are multiple representations of the same\nglyph).\n\nAdding a support file is fine, but for small cases like this, it may be\neasier to do:\n\n printf '\\357\\274\\210...' >expect\n\nbut note that this _must_ be octal, not hex, as many versions of printf\nonly handle the former.\n\n-Peff\n"},{"id":"377155","messageId":"20190612212111.GF58112@comcast.net","threadId":"51256","inReplyTo":"xmqqo932ik7y.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/1] ref-filter: sort detached HEAD lines firstly","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-06-12T21:21:11Z","receivedAt":"2019-06-13T17:17:07Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"On Wed, Jun 12, 2019 at 02:09:53PM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> >> +\t\t/*\n> >> +\t\t * When sorting by name, we should put \"detached\" head lines,\n> >> +\t\t * which are all the lines in parenthesis, before all others.\n> >> +\t\t * This usually is automatic, since \"(\" is before \"refs/\" and\n> >> +\t\t * \"remotes/\", but this does not hold for zh_CN, which uses\n> >> +\t\t * full-width parenthesis, so make the ordering explicit.\n> >> +\t\t */\n> >> +\t\tif (a_detached != (b->kind & FILTER_REFS_DETACHED_HEAD))\n> >> +\t\t\tcmp = a_detached ? -1 : 1;\n> >\n> > So, comparing a detached and an undetached ones, the detached side\n> > always sorts lower. Good. And ...\n> >\n> >> +\t\telse\n> >> +\t\t\tcmp = cmp_fn(va->s, vb->s);\n> >\n> > ... otherwise we compare the string using the given function.\n> >\n> > Sounds sensible. Will queue.\n> \n> Stepping back a bit, why are we even allowing the surrounding ()\n> pair to be futzed by the translators?\n\nI was thinking about removing () from the translated strings, but decided\nagainst it since there are a lot of full-width parenthesis in the translated\nstrings already:\n\n$ cd po; git grep -B 1 'msgstr.*（'\n... 246 matches in zh_CN ...\n\nand it seems strange to force only a few pairs of parens to be half-width to\nmake the code simpler. I don't know if that's a great argument, since it is\nsomewhat aesthetic. I would have liked half-width parens more if it were\nclosing off purely ASCII text. But it is in fact surrounding Chinese text:\n\n$ git branch\n* （头指针分离于 cf0246a5cc）\n\n> \n> IOW, shouldn't our code more like this from the beginning, with or\n> without Chinese translation?\n> \n> With a bit more work, we may even be able to lose \"make sure this\n> matches the one in wt-status.c\" comment as losing the leading '('\n> would take us one step closer to have an identical string here as we\n> have in wt-status.c\n> \n> ref-filter.c | 8 +++++---\n> 1 file changed, 5 insertions(+), 3 deletions(-)\n> \n> diff --git a/ref-filter.c b/ref-filter.c\n> index 8500671bc6..7e4705fcb2 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1459,20 +1459,22 @@ char *get_head_description(void)\n> \t\tstrbuf_addf(&desc, _(\"(no branch, bisect started on %s)\"),\n> \t\t\t state.branch);\n> \telse if (state.detached_from) {\n> +\t\tstrbuf_addch(&desc, '(');\n> \t\tif (state.detached_at)\n> \t\t\t/*\n> \t\t\t * TRANSLATORS: make sure this matches \"HEAD\n> \t\t\t * detached at \" in wt-status.c\n> \t\t\t */\n> -\t\t\tstrbuf_addf(&desc, _(\"(HEAD detached at %s)\"),\n> -\t\t\t\tstate.detached_from);\n> +\t\t\tstrbuf_addf(&desc, _(\"HEAD detached at %s\"),\n> +\t\t\t\t state.detached_from);\n> \t\telse\n> \t\t\t/*\n> \t\t\t * TRANSLATORS: make sure this matches \"HEAD\n> \t\t\t * detached from \" in wt-status.c\n> \t\t\t */\n> -\t\t\tstrbuf_addf(&desc, _(\"(HEAD detached from %s)\"),\n> +\t\t\tstrbuf_addf(&desc, _(\"HEAD detached from %s\"),\n> \t\t\t\tstate.detached_from);\n> +\t\tstrbuf_addch(&desc, ')');\n> \t}\n> \telse\n> \t\tstrbuf_addstr(&desc, _(\"(no branch)\"));\n> \n> \n"},{"id":"377461","messageId":"cover.1560895672.git.matvore@google.com","threadId":"51256","inReplyTo":"faaa9a3d6ba66d77cc2a8eab438d1bfc8f762fa1.1559857032.git.matvore@google.com","subject":"[PATCH v3 0/1] Sort detached heads line firstly","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2019-06-18T22:29:14Z","receivedAt":"2019-06-18T22:29:23Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"In the interest of simplifying the code and the job of translators, I've\ndecided to simply remove the ( from the translatable strings as suggested by\nJunio in a response to v2 of this patchset.\n\nI thought that a regression test would be a bit overkill for a fix of this\nnature. Instead, I've added a cautionary comment to not add the ( back to the\ntranslatable string.\n\nThank you,\n\nMatthew DeVore (1):\n ref-filter: sort detached HEAD lines firstly\n\n ref-filter.c | 32 ++++++++++++++++----------------\n wt-status.c | 4 ++--\n wt-status.h | 3 +++\n 3 files changed, 21 insertions(+), 18 deletions(-)\n\n-- \n2.21.0\n\n"},{"id":"377463","messageId":"9bd85516f91c3e2fdefdafd51df71f75603e51f6.1560895672.git.matvore@google.com","threadId":"51256","inReplyTo":"cover.1560895672.git.matvore@google.com","subject":"[PATCH v3 1/1] ref-filter: sort detached HEAD lines firstly","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2019-06-18T22:29:15Z","receivedAt":"2019-06-18T22:29:26Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"Before this patch, \"git branch\" would put \"(HEAD detached...)\" and \"(no\nbranch, rebasing...)\" lines before all the other branches *in most\ncases* except for when using Chinese-language messages. zh_CN generally\nuses a full-width \"(\" symbol (codepoint FF08) to match the full-width\nproportions of Chinese characters, and the translated strings we had did\nuse them. This meant that the detached HEAD line would appear after all\nlocal refs and even after the remote refs if there were any.\n\nAFAIK, it is sometimes not jarring to see the half-width parenthesis in\n\"full-width\" text as in the CJK languages, for instance when there are\nno characters preceding or following the parenthesized text fragment. By\nremoving the parenthesis from the localizable text, we can share strings\nwith wt-status.c and remove a cautionary comment to translators.\n\nRemove the ( from the localizable portion of messages so the sorting\nhappens properly regardless of locale.\n\nHelped-by: Johannes Schindelin \nHelped-by: Jonathan Nieder \nSigned-off-by: Matthew DeVore \n---\n ref-filter.c | 32 ++++++++++++++++----------------\n wt-status.c | 4 ++--\n wt-status.h | 3 +++\n 3 files changed, 21 insertions(+), 18 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 8500671bc6..87aa6b4774 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1440,49 +1440,49 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n \t} else\n \t\tBUG(\"unhandled RR_* enum\");\n }\n \n char *get_head_description(void)\n {\n \tstruct strbuf desc = STRBUF_INIT;\n \tstruct wt_status_state state;\n \tmemset(&state, 0, sizeof(state));\n \twt_status_get_state(the_repository, &state, 1);\n+\n+\t/*\n+\t * The ( character must be hard-coded and not part of a localizable\n+\t * string, since the description is used as a sort key and compared\n+\t * with ref names.\n+\t */\n+\tstrbuf_addch(&desc, '(');\n \tif (state.rebase_in_progress ||\n \t state.rebase_interactive_in_progress) {\n \t\tif (state.branch)\n-\t\t\tstrbuf_addf(&desc, _(\"(no branch, rebasing %s)\"),\n+\t\t\tstrbuf_addf(&desc, _(\"no branch, rebasing %s\"),\n \t\t\t\t state.branch);\n \t\telse\n-\t\t\tstrbuf_addf(&desc, _(\"(no branch, rebasing detached HEAD %s)\"),\n+\t\t\tstrbuf_addf(&desc, _(\"no branch, rebasing detached HEAD %s\"),\n \t\t\t\t state.detached_from);\n \t} else if (state.bisect_in_progress)\n-\t\tstrbuf_addf(&desc, _(\"(no branch, bisect started on %s)\"),\n+\t\tstrbuf_addf(&desc, _(\"no branch, bisect started on %s\"),\n \t\t\t state.branch);\n \telse if (state.detached_from) {\n \t\tif (state.detached_at)\n-\t\t\t/*\n-\t\t\t * TRANSLATORS: make sure this matches \"HEAD\n-\t\t\t * detached at \" in wt-status.c\n-\t\t\t */\n-\t\t\tstrbuf_addf(&desc, _(\"(HEAD detached at %s)\"),\n-\t\t\t\tstate.detached_from);\n+\t\t\tstrbuf_addstr(&desc, HEAD_DETACHED_AT);\n \t\telse\n-\t\t\t/*\n-\t\t\t * TRANSLATORS: make sure this matches \"HEAD\n-\t\t\t * detached from \" in wt-status.c\n-\t\t\t */\n-\t\t\tstrbuf_addf(&desc, _(\"(HEAD detached from %s)\"),\n-\t\t\t\tstate.detached_from);\n+\t\t\tstrbuf_addstr(&desc, HEAD_DETACHED_FROM);\n+\t\tstrbuf_addstr(&desc, state.detached_from);\n \t}\n \telse\n-\t\tstrbuf_addstr(&desc, _(\"(no branch)\"));\n+\t\tstrbuf_addstr(&desc, _(\"no branch\"));\n+\tstrbuf_addch(&desc, ')');\n+\n \tfree(state.branch);\n \tfree(state.onto);\n \tfree(state.detached_from);\n \treturn strbuf_detach(&desc, NULL);\n }\n \n static const char *get_symref(struct used_atom *atom, struct ref_array_item *ref)\n {\n \tif (!ref->symref)\n \t\treturn xstrdup(\"\");\ndiff --git a/wt-status.c b/wt-status.c\nindex 0bccef542f..c29e4bf091 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1669,23 +1669,23 @@ static void wt_longstatus_print(struct wt_status *s)\n \t\t\tif (s->state.rebase_in_progress ||\n \t\t\t s->state.rebase_interactive_in_progress) {\n \t\t\t\tif (s->state.rebase_interactive_in_progress)\n \t\t\t\t\ton_what = _(\"interactive rebase in progress; onto \");\n \t\t\t\telse\n \t\t\t\t\ton_what = _(\"rebase in progress; onto \");\n \t\t\t\tbranch_name = s->state.onto;\n \t\t\t} else if (s->state.detached_from) {\n \t\t\t\tbranch_name = s->state.detached_from;\n \t\t\t\tif (s->state.detached_at)\n-\t\t\t\t\ton_what = _(\"HEAD detached at \");\n+\t\t\t\t\ton_what = HEAD_DETACHED_AT;\n \t\t\t\telse\n-\t\t\t\t\ton_what = _(\"HEAD detached from \");\n+\t\t\t\t\ton_what = HEAD_DETACHED_FROM;\n \t\t\t} else {\n \t\t\t\tbranch_name = \"\";\n \t\t\t\ton_what = _(\"Not currently on any branch.\");\n \t\t\t}\n \t\t} else\n \t\t\tskip_prefix(branch_name, \"refs/heads/\", &branch_name);\n \t\tstatus_printf(s, color(WT_STATUS_HEADER, s), \"%s\", \"\");\n \t\tstatus_printf_more(s, branch_status_color, \"%s\", on_what);\n \t\tstatus_printf_more(s, branch_color, \"%s\\n\", branch_name);\n \t\tif (!s->is_initial)\ndiff --git a/wt-status.h b/wt-status.h\nindex 64f1ddc9fd..b0cfdc8011 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -58,20 +58,23 @@ struct wt_status_change_data {\n enum wt_status_format {\n \tSTATUS_FORMAT_NONE = 0,\n \tSTATUS_FORMAT_LONG,\n \tSTATUS_FORMAT_SHORT,\n \tSTATUS_FORMAT_PORCELAIN,\n \tSTATUS_FORMAT_PORCELAIN_V2,\n \n \tSTATUS_FORMAT_UNSPECIFIED\n };\n \n+#define HEAD_DETACHED_AT _(\"HEAD detached at \")\n+#define HEAD_DETACHED_FROM _(\"HEAD detached from \")\n+\n struct wt_status_state {\n \tint merge_in_progress;\n \tint am_in_progress;\n \tint am_empty_patch;\n \tint rebase_in_progress;\n \tint rebase_interactive_in_progress;\n \tint cherry_pick_in_progress;\n \tint bisect_in_progress;\n \tint revert_in_progress;\n \tint detached_at;\n-- \n2.21.0\n\n"},{"id":"377536","messageId":"xmqqv9x1pp9i.fsf@gitster-ct.c.googlers.com","threadId":"51256","inReplyTo":"9bd85516f91c3e2fdefdafd51df71f75603e51f6.1560895672.git.matvore@google.com","subject":"Re: [PATCH v3 1/1] ref-filter: sort detached HEAD lines firstly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-19T15:29:29Z","receivedAt":"2019-06-19T15:29:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthew DeVore writes:\n\n> ... By\n> removing the parenthesis from the localizable text, we can share strings\n> with wt-status.c and remove a cautionary comment to translators.\n...\n> -\t\t\t/*\n> -\t\t\t * TRANSLATORS: make sure this matches \"HEAD\n> -\t\t\t * detached at \" in wt-status.c\n> -\t\t\t */\n> -\t\t\tstrbuf_addf(&desc, _(\"(HEAD detached at %s)\"),\n> -\t\t\t\tstate.detached_from);\n> +\t\t\tstrbuf_addstr(&desc, HEAD_DETACHED_AT);\n> \t\telse\n> -\t\t\t/*\n> -\t\t\t * TRANSLATORS: make sure this matches \"HEAD\n> -\t\t\t * detached from \" in wt-status.c\n> -\t\t\t */\n> -\t\t\tstrbuf_addf(&desc, _(\"(HEAD detached from %s)\"),\n> -\t\t\t\tstate.detached_from);\n> +\t\t\tstrbuf_addstr(&desc, HEAD_DETACHED_FROM);\n\nVery nice ;-)\n\n> +\t\tstrbuf_addstr(&desc, state.detached_from);\n> \t}\n> \telse\n> -\t\tstrbuf_addstr(&desc, _(\"(no branch)\"));\n> +\t\tstrbuf_addstr(&desc, _(\"no branch\"));\n> +\tstrbuf_addch(&desc, ')');\n> +\n> \tfree(state.branch);\n> \tfree(state.onto);\n> \tfree(state.detached_from);\n> \treturn strbuf_detach(&desc, NULL);\n> }\n\n> diff --git a/wt-status.h b/wt-status.h\n> index 64f1ddc9fd..b0cfdc8011 100644\n> --- a/wt-status.h\n> +++ b/wt-status.h\n> @@ -58,20 +58,23 @@ struct wt_status_change_data {\n> ...\n> \n> +#define HEAD_DETACHED_AT _(\"HEAD detached at \")\n> +#define HEAD_DETACHED_FROM _(\"HEAD detached from \")\n> +\n> struct wt_status_state {\n\nThese too.\n"},{"id":"413555","messageId":"20210106100139.14651-1-avarab@gmail.com","threadId":"51256","inReplyTo":"9bd85516f91c3e2fdefdafd51df71f75603e51f6.1560895672.git.matvore@google.com","subject":"[PATCH 0/5] branch: --sort improvements","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-01-06T10:01:34Z","receivedAt":"2021-01-06T10:02:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This started out as a reading of ref-filter.c where I wondered why we\nneeded this i18n lego. I'm not sure whether in Chinese what Matthew\nDeVore said in [1] is true, but in any case it seems better to leave\nthat to the translators, re-using the string is a relatively small\ngain.\n\nSo I think that change was really just \"ASCII sort is easier than\nchecking a flag in a sort callback\", fair enough. But I thought I'd\ntry to see how hard that patch would be. Turned out it's rather easy &\nI think results in better code, 4/5 gets us to that point.\n\nBut 5/5 I think makes this more generally interesting. In all locales\n(including LC_ALL=C) we list the \"HEAD detached\" entry last in \"git\nbranch -l\" output if you're doing a reverse sort. I don't think this\nmakes any sense, it's a notice, not a refname to be sorted. Using the\nnew sorting function for treating detached HEAD specially makes this\ntrivial to fix.\n\n1. https://lore.kernel.org/git/9bd85516f91c3e2fdefdafd51df71f75603e51f6.1560895672.git.matvore@google.com/\n\nÆvar Arnfjörð Bjarmason (5):\n branch: change \"--local\" to \"--list\" in comment\n branch tests: add to --sort tests\n ref-filter: add a \"detached_head_first\" sorting option\n branch: use the \"detached_head_first\" sorting option\n branch: show \"HEAD detached\" first under reverse sort\n\n builtin/branch.c | 3 ++-\n ref-filter.c | 54 +++++++++++++++++++++++++---------------\n ref-filter.h | 3 +++\n t/t3203-branch-output.sh | 51 ++++++++++++++++++++++++++++++++++++-\n wt-status.c | 4 +--\n wt-status.h | 2 --\n 6 files changed, 91 insertions(+), 26 deletions(-)\n\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"413556","messageId":"20210106100139.14651-2-avarab@gmail.com","threadId":"51256","inReplyTo":"9bd85516f91c3e2fdefdafd51df71f75603e51f6.1560895672.git.matvore@google.com","subject":"[PATCH 1/5] branch: change \"--local\" to \"--list\" in comment","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-01-06T10:01:35Z","receivedAt":"2021-01-06T10:03:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"There has never been a \"git branch --local\", this is just a typo for\n\"--list\". Fixes a comment added in 23e714df91c (branch: roll\nshow_detached HEAD into regular ref_list, 2015-09-23).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/branch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 9b68591addf..045866a51ae 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -726,7 +726,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tprint_current_branch_name();\n \t\treturn 0;\n \t} else if (list) {\n-\t\t/* git branch --local also shows HEAD when it is detached */\n+\t\t/* git branch --list also shows HEAD when it is detached */\n \t\tif ((filter.kind & FILTER_REFS_BRANCHES) && filter.detached)\n \t\t\tfilter.kind |= FILTER_REFS_DETACHED_HEAD;\n \t\tfilter.name_patterns = argv;\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"413557","messageId":"20210106100139.14651-3-avarab@gmail.com","threadId":"51256","inReplyTo":"9bd85516f91c3e2fdefdafd51df71f75603e51f6.1560895672.git.matvore@google.com","subject":"[PATCH 2/5] branch tests: add to --sort tests","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-01-06T10:01:36Z","receivedAt":"2021-01-06T10:03:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Further stress the --sort callback in ref-filter.c. The implementation\nuses certain short-circuiting logic, let's make sure it behaves the\nsame way on e.g. name & version sort. Improves a test added in\naedcb7dc75e (branch.c: use 'ref-filter' APIs, 2015-09-23).\n\nI don't think all of this output makes sense, but let's test for the\nbehavior as-is, we can fix bugs in it in a later commit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t3203-branch-output.sh | 51 +++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 50 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\nindex b945faf4702..f92fb3aab9d 100755\n--- a/t/t3203-branch-output.sh\n+++ b/t/t3203-branch-output.sh\n@@ -210,7 +210,7 @@ EOF\n \ttest_i18ncmp expect actual\n '\n \n-test_expect_success 'git branch `--sort` option' '\n+test_expect_success 'git branch `--sort=[-]objectsize` option' '\n \tcat >expect <<-\\EOF &&\n \t* (HEAD detached from fromtag)\n \t branch-two\n@@ -218,6 +218,55 @@ test_expect_success 'git branch `--sort` option' '\n \t main\n \tEOF\n \tgit branch --sort=objectsize >actual &&\n+\ttest_i18ncmp expect actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\t branch-one\n+\t main\n+\t* (HEAD detached from fromtag)\n+\t branch-two\n+\tEOF\n+\tgit branch --sort=-objectsize >actual &&\n+\ttest_i18ncmp expect actual\n+'\n+\n+test_expect_success 'git branch `--sort=[-]type` option' '\n+\tcat >expect <<-\\EOF &&\n+\t* (HEAD detached from fromtag)\n+\t branch-one\n+\t branch-two\n+\t main\n+\tEOF\n+\tgit branch --sort=type >actual &&\n+\ttest_i18ncmp expect actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\t* (HEAD detached from fromtag)\n+\t branch-one\n+\t branch-two\n+\t main\n+\tEOF\n+\tgit branch --sort=-type >actual &&\n+\ttest_i18ncmp expect actual\n+'\n+\n+test_expect_success 'git branch `--sort=[-]version:refname` option' '\n+\tcat >expect <<-\\EOF &&\n+\t* (HEAD detached from fromtag)\n+\t branch-one\n+\t branch-two\n+\t main\n+\tEOF\n+\tgit branch --sort=version:refname >actual &&\n+\ttest_i18ncmp expect actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\t main\n+\t branch-two\n+\t branch-one\n+\t* (HEAD detached from fromtag)\n+\tEOF\n+\tgit branch --sort=-version:refname >actual &&\n \ttest_i18ncmp expect actual\n '\n \n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"413558","messageId":"20210106100139.14651-5-avarab@gmail.com","threadId":"51256","inReplyTo":"9bd85516f91c3e2fdefdafd51df71f75603e51f6.1560895672.git.matvore@google.com","subject":"[PATCH 4/5] branch: use the \"detached_head_first\" sorting option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-01-06T10:01:38Z","receivedAt":"2021-01-06T10:03:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Use the new ref_sorting_detached_head_first_all() sorting option in\nref-filter.c to revert and amend 28438e84e04 (ref-filter: sort\ndetached HEAD lines firstly, 2019-06-18).\n\nIn Chinese the fullwidth versions of punctuation like \"()\" are\ntypically written as (U+FF08 fullwidth left parenthesis), (U+FF09\nfullwidth right parenthesis) instead. This form is used in both\npo/zh_{CN,TW}.po in most cases where \"()\" is translated in a string.\n\nIn 28438e84e04 the ability to translate this as part of the \"git\nbranch -l\" output was removed because we'd like the detached line to\nappear first at the start of \"git branch -l\", e.g.:\n\n $ git branch -l\n * (HEAD detached at )\n master\n\nLet's instead use the new ref_sorting_detached_head_first_all() in\nbranch.c to say that we'd like these sorted before other entries.\n\nAs seen in the amended tests this made reverse sorting a bit more\nconsistent. Before this we'd sometimes sort this message in the\nmiddle, now it's consistently at the beginning or end. Having it at\nthe end doesn't make much sense either, but at least it behaves\nconsistently now. A follow-up commit will make this behavior even\nbetter.\n\nI'm removing the \"TRANSLATORS\" comments that were in the old code\nwhile I'm at it. Those were added in d4919bb288e (ref-filter: move\nget_head_description() from branch.c, 2017-01-10). I think it's\nobvious from context, string and translation memory in typical\ntranslation tools that these are the same or similar string.\n\n1. https://en.wikipedia.org/wiki/Chinese_punctuation#Marks_similar_to_European_punctuation\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/branch.c | 1 +\n ref-filter.c | 27 +++++++++------------------\n t/t3203-branch-output.sh | 4 ++--\n wt-status.c | 4 ++--\n wt-status.h | 2 --\n 5 files changed, 14 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 045866a51ae..92221bdf8a6 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -740,6 +740,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tif (!sorting)\n \t\t\tsorting = ref_default_sorting();\n \t\tref_sorting_icase_all(sorting, icase);\n+\t\tref_sorting_detached_head_first_all(sorting, 1);\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/ref-filter.c b/ref-filter.c\nindex 94ab3f86a53..7e0289cb659 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1536,36 +1536,27 @@ char *get_head_description(void)\n \tstruct wt_status_state state;\n \tmemset(&state, 0, sizeof(state));\n \twt_status_get_state(the_repository, &state, 1);\n-\n-\t/*\n-\t * The ( character must be hard-coded and not part of a localizable\n-\t * string, since the description is used as a sort key and compared\n-\t * with ref names.\n-\t */\n-\tstrbuf_addch(&desc, '(');\n \tif (state.rebase_in_progress ||\n \t state.rebase_interactive_in_progress) {\n \t\tif (state.branch)\n-\t\t\tstrbuf_addf(&desc, _(\"no branch, rebasing %s\"),\n+\t\t\tstrbuf_addf(&desc, _(\"(no branch, rebasing %s)\"),\n \t\t\t\t state.branch);\n \t\telse\n-\t\t\tstrbuf_addf(&desc, _(\"no branch, rebasing detached HEAD %s\"),\n+\t\t\tstrbuf_addf(&desc, _(\"(no branch, rebasing detached HEAD %s)\"),\n \t\t\t\t state.detached_from);\n \t} else if (state.bisect_in_progress)\n-\t\tstrbuf_addf(&desc, _(\"no branch, bisect started on %s\"),\n+\t\tstrbuf_addf(&desc, _(\"(no branch, bisect started on %s)\"),\n \t\t\t state.branch);\n \telse if (state.detached_from) {\n \t\tif (state.detached_at)\n-\t\t\tstrbuf_addstr(&desc, HEAD_DETACHED_AT);\n+\t\t\tstrbuf_addf(&desc, _(\"(HEAD detached at %s)\"),\n+\t\t\t\tstate.detached_from);\n \t\telse\n-\t\t\tstrbuf_addstr(&desc, HEAD_DETACHED_FROM);\n-\t\tstrbuf_addstr(&desc, state.detached_from);\n-\t}\n-\telse\n-\t\tstrbuf_addstr(&desc, _(\"no branch\"));\n-\tstrbuf_addch(&desc, ')');\n+\t\t\tstrbuf_addf(&desc, _(\"(HEAD detached from %s)\"),\n+\t\t\t\tstate.detached_from);\n+\t} else\n+\t\tstrbuf_addstr(&desc, _(\"(no branch)\"));\n \n-\twt_status_state_free_buffers(&state);\n \treturn strbuf_detach(&desc, NULL);\n }\n \ndiff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\nindex f92fb3aab9d..8f53b081365 100755\n--- a/t/t3203-branch-output.sh\n+++ b/t/t3203-branch-output.sh\n@@ -223,8 +223,8 @@ test_expect_success 'git branch `--sort=[-]objectsize` option' '\n \tcat >expect <<-\\EOF &&\n \t branch-one\n \t main\n-\t* (HEAD detached from fromtag)\n \t branch-two\n+\t* (HEAD detached from fromtag)\n \tEOF\n \tgit branch --sort=-objectsize >actual &&\n \ttest_i18ncmp expect actual\n@@ -241,10 +241,10 @@ test_expect_success 'git branch `--sort=[-]type` option' '\n \ttest_i18ncmp expect actual &&\n \n \tcat >expect <<-\\EOF &&\n-\t* (HEAD detached from fromtag)\n \t branch-one\n \t branch-two\n \t main\n+\t* (HEAD detached from fromtag)\n \tEOF\n \tgit branch --sort=-type >actual &&\n \ttest_i18ncmp expect actual\ndiff --git a/wt-status.c b/wt-status.c\nindex 7074bbdd53c..40b59be478c 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1742,9 +1742,9 @@ static void wt_longstatus_print(struct wt_status *s)\n \t\t\t} else if (s->state.detached_from) {\n \t\t\t\tbranch_name = s->state.detached_from;\n \t\t\t\tif (s->state.detached_at)\n-\t\t\t\t\ton_what = HEAD_DETACHED_AT;\n+\t\t\t\t\ton_what = _(\"HEAD detached at \");\n \t\t\t\telse\n-\t\t\t\t\ton_what = HEAD_DETACHED_FROM;\n+\t\t\t\t\ton_what = _(\"HEAD detached from \");\n \t\t\t} else {\n \t\t\t\tbranch_name = \"\";\n \t\t\t\ton_what = _(\"Not currently on any branch.\");\ndiff --git a/wt-status.h b/wt-status.h\nindex 35b44c388ed..0d32799b28e 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -77,8 +77,6 @@ enum wt_status_format {\n \tSTATUS_FORMAT_UNSPECIFIED\n };\n \n-#define HEAD_DETACHED_AT _(\"HEAD detached at \")\n-#define HEAD_DETACHED_FROM _(\"HEAD detached from \")\n #define SPARSE_CHECKOUT_DISABLED -1\n \n struct wt_status_state {\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"413559","messageId":"20210106100139.14651-4-avarab@gmail.com","threadId":"51256","inReplyTo":"9bd85516f91c3e2fdefdafd51df71f75603e51f6.1560895672.git.matvore@google.com","subject":"[PATCH 3/5] ref-filter: add a \"detached_head_first\" sorting option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-01-06T10:01:37Z","receivedAt":"2021-01-06T10:03:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a \"detached_head_first\" sorting option for eventual use by the\n\"git branch\" command. When listing branches we want to list the\ndetached HEAD \"ref\" at the start of the list. As shown in\n28438e84e04 (ref-filter: sort detached HEAD lines firstly, 2019-06-18)\nthis currently relies on \"(\" sorting before any other refname by\nstrcmp().\n\nThis boxes translators into using ASCII parentheses, a subsequent\ncommit will amend get_head_description() to get rid of this\nlimitation.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n ref-filter.c | 23 ++++++++++++++++++++++-\n ref-filter.h | 3 +++\n 2 files changed, 25 insertions(+), 1 deletion(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex aa260bfd099..94ab3f86a53 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2350,6 +2350,16 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n \treturn ret;\n }\n \n+static int compare_detached_head(struct ref_array_item *a, struct ref_array_item *b)\n+{\n+\tif (a->kind & FILTER_REFS_DETACHED_HEAD)\n+\t\treturn -1;\n+\telse if (b->kind & FILTER_REFS_DETACHED_HEAD)\n+\t\treturn 1;\n+\tBUG(\"compare_detached_head() is guarded by an xor on [ab]->kind & FILTER_REFS_DETACHED_HEAD\");\n+\treturn 0;\n+}\n+\n static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, struct ref_array_item *b)\n {\n \tstruct atom_value *va, *vb;\n@@ -2364,7 +2374,12 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n \t\tdie(\"%s\", err.buf);\n \tstrbuf_release(&err);\n \tcmp_fn = s->ignore_case ? strcasecmp : strcmp;\n-\tif (s->version)\n+\tif (s->detached_head_first &&\n+\t ((a->kind & FILTER_REFS_DETACHED_HEAD)\n+\t ^\n+\t (b->kind & FILTER_REFS_DETACHED_HEAD))) {\n+\t\tcmp = compare_detached_head(a, b);\n+\t} else if (s->version)\n \t\tcmp = versioncmp(va->s, vb->s);\n \telse if (cmp_type == FIELD_STR)\n \t\tcmp = cmp_fn(va->s, vb->s);\n@@ -2403,6 +2418,12 @@ void ref_sorting_icase_all(struct ref_sorting *sorting, int flag)\n \t\tsorting->ignore_case = !!flag;\n }\n \n+void ref_sorting_detached_head_first_all(struct ref_sorting *sorting, int flag)\n+{\n+\tfor (; sorting; sorting = sorting->next)\n+\t\tsorting->detached_head_first = !!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 feaef4a8fde..3b92e0f2696 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -30,6 +30,7 @@ struct ref_sorting {\n \tint atom; /* index into used_atom array (internal) */\n \tunsigned reverse : 1,\n \t\tignore_case : 1,\n+\t\tdetached_head_first : 1,\n \t\tversion : 1;\n };\n \n@@ -111,6 +112,8 @@ int verify_ref_format(struct ref_format *format);\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+/* Set the detached_head_first flag for all elements of a sorting list */\n+void ref_sorting_detached_head_first_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-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"413560","messageId":"20210106100139.14651-6-avarab@gmail.com","threadId":"51256","inReplyTo":"9bd85516f91c3e2fdefdafd51df71f75603e51f6.1560895672.git.matvore@google.com","subject":"[PATCH 5/5] branch: show \"HEAD detached\" first under reverse sort","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-01-06T10:01:39Z","receivedAt":"2021-01-06T10:03:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the output of the likes of \"git branch -l --sort=-objectsize\"\nto show the \"(HEAD detached at )\" message at the start of the\noutput. Before the compare_detached_head() function added in a\npreceding commit we'd emit this output as an emergent effect.\n\nIt doesn't make any sense to consider the objectsize, type or other\nnon-attribute of the \"(HEAD detached at )\" message for the\npurposes of sorting. Let's always emit it at the top instead. The only\nreason it was sorted in the first place is because we're injecting it\ninto the ref-filter machinery so builtin/branch.c doesn't need to do\nits own \"am I detached?\" detection.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n ref-filter.c | 4 +++-\n t/t3203-branch-output.sh | 6 +++---\n 2 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 7e0289cb659..5bbdc46c1f9 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2355,6 +2355,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n {\n \tstruct atom_value *va, *vb;\n \tint cmp;\n+\tint cmp_detached_head = 0;\n \tcmp_type cmp_type = used_atom[s->atom].type;\n \tint (*cmp_fn)(const char *, const char *);\n \tstruct strbuf err = STRBUF_INIT;\n@@ -2370,6 +2371,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n \t ^\n \t (b->kind & FILTER_REFS_DETACHED_HEAD))) {\n \t\tcmp = compare_detached_head(a, b);\n+\t\tcmp_detached_head = 1;\n \t} else if (s->version)\n \t\tcmp = versioncmp(va->s, vb->s);\n \telse if (cmp_type == FIELD_STR)\n@@ -2383,7 +2385,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n \t\t\tcmp = 1;\n \t}\n \n-\treturn (s->reverse) ? -cmp : cmp;\n+\treturn (s->reverse && !cmp_detached_head) ? -cmp : cmp;\n }\n \n static int compare_refs(const void *a_, const void *b_, void *ref_sorting)\ndiff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\nindex 8f53b081365..5e0577d5c7f 100755\n--- a/t/t3203-branch-output.sh\n+++ b/t/t3203-branch-output.sh\n@@ -221,10 +221,10 @@ test_expect_success 'git branch `--sort=[-]objectsize` option' '\n \ttest_i18ncmp expect actual &&\n \n \tcat >expect <<-\\EOF &&\n+\t* (HEAD detached from fromtag)\n \t branch-one\n \t main\n \t branch-two\n-\t* (HEAD detached from fromtag)\n \tEOF\n \tgit branch --sort=-objectsize >actual &&\n \ttest_i18ncmp expect actual\n@@ -241,10 +241,10 @@ test_expect_success 'git branch `--sort=[-]type` option' '\n \ttest_i18ncmp expect actual &&\n \n \tcat >expect <<-\\EOF &&\n+\t* (HEAD detached from fromtag)\n \t branch-one\n \t branch-two\n \t main\n-\t* (HEAD detached from fromtag)\n \tEOF\n \tgit branch --sort=-type >actual &&\n \ttest_i18ncmp expect actual\n@@ -261,10 +261,10 @@ test_expect_success 'git branch `--sort=[-]version:refname` option' '\n \ttest_i18ncmp expect actual &&\n \n \tcat >expect <<-\\EOF &&\n+\t* (HEAD detached from fromtag)\n \t main\n \t branch-two\n \t branch-one\n-\t* (HEAD detached from fromtag)\n \tEOF\n \tgit branch --sort=-version:refname >actual &&\n \ttest_i18ncmp expect actual\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"413624","messageId":"xmqq5z49ps7n.fsf@gitster.c.googlers.com","threadId":"51256","inReplyTo":"20210106100139.14651-3-avarab@gmail.com","subject":"Re: [PATCH 2/5] branch tests: add to --sort tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-06T23:21:16Z","receivedAt":"2021-01-06T23:22:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Further stress the --sort callback in ref-filter.c. The implementation\n> uses certain short-circuiting logic, let's make sure it behaves the\n> same way on e.g. name & version sort. Improves a test added in\n> aedcb7dc75e (branch.c: use 'ref-filter' APIs, 2015-09-23).\n>\n> I don't think all of this output makes sense, but let's test for the\n> behavior as-is, we can fix bugs in it in a later commit.\n\nOK.\n\nI wondered if 'type' and '-type' tests and 'version:refname' and\n'-version:refname' tests, should be separate, so that the latter\nhalf of the latter pair can expect to have HEAD at the beginning\nwith test_expect_failure until it gets fixed. But \"document the\nstatus quo, and then change the behaviour and demonstrate how the\nnew behaviour is superiour with the change in the expectation in the\npatch\" is a reasonable approach, too.\n\nWill queue; thanks.\n\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> t/t3203-branch-output.sh | 51 +++++++++++++++++++++++++++++++++++++++-\n> 1 file changed, 50 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\n> index b945faf4702..f92fb3aab9d 100755\n> --- a/t/t3203-branch-output.sh\n> +++ b/t/t3203-branch-output.sh\n> @@ -210,7 +210,7 @@ EOF\n> \ttest_i18ncmp expect actual\n> '\n> \n> -test_expect_success 'git branch `--sort` option' '\n> +test_expect_success 'git branch `--sort=[-]objectsize` option' '\n> \tcat >expect <<-\\EOF &&\n> \t* (HEAD detached from fromtag)\n> \t branch-two\n> @@ -218,6 +218,55 @@ test_expect_success 'git branch `--sort` option' '\n> \t main\n> \tEOF\n> \tgit branch --sort=objectsize >actual &&\n> +\ttest_i18ncmp expect actual &&\n> +\n> +\tcat >expect <<-\\EOF &&\n> +\t branch-one\n> +\t main\n> +\t* (HEAD detached from fromtag)\n> +\t branch-two\n> +\tEOF\n> +\tgit branch --sort=-objectsize >actual &&\n> +\ttest_i18ncmp expect actual\n> +'\n> +\n> +test_expect_success 'git branch `--sort=[-]type` option' '\n> +\tcat >expect <<-\\EOF &&\n> +\t* (HEAD detached from fromtag)\n> +\t branch-one\n> +\t branch-two\n> +\t main\n> +\tEOF\n> +\tgit branch --sort=type >actual &&\n> +\ttest_i18ncmp expect actual &&\n> +\n> +\tcat >expect <<-\\EOF &&\n> +\t* (HEAD detached from fromtag)\n> +\t branch-one\n> +\t branch-two\n> +\t main\n> +\tEOF\n> +\tgit branch --sort=-type >actual &&\n> +\ttest_i18ncmp expect actual\n> +'\n> +\n> +test_expect_success 'git branch `--sort=[-]version:refname` option' '\n> +\tcat >expect <<-\\EOF &&\n> +\t* (HEAD detached from fromtag)\n> +\t branch-one\n> +\t branch-two\n> +\t main\n> +\tEOF\n> +\tgit branch --sort=version:refname >actual &&\n> +\ttest_i18ncmp expect actual &&\n> +\n> +\tcat >expect <<-\\EOF &&\n> +\t main\n> +\t branch-two\n> +\t branch-one\n> +\t* (HEAD detached from fromtag)\n> +\tEOF\n> +\tgit branch --sort=-version:refname >actual &&\n> \ttest_i18ncmp expect actual\n> '\n"},{"id":"413625","messageId":"xmqqy2h5oci9.fsf@gitster.c.googlers.com","threadId":"51256","inReplyTo":"20210106100139.14651-4-avarab@gmail.com","subject":"Re: [PATCH 3/5] ref-filter: add a \"detached_head_first\" sorting option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-06T23:45:50Z","receivedAt":"2021-01-06T23:46:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> +static int compare_detached_head(struct ref_array_item *a, struct ref_array_item *b)\n> +{\n> +\tif (a->kind & FILTER_REFS_DETACHED_HEAD)\n> +\t\treturn -1;\n> +\telse if (b->kind & FILTER_REFS_DETACHED_HEAD)\n> +\t\treturn 1;\n> +\tBUG(\"compare_detached_head() is guarded by an xor on [ab]->kind & FILTER_REFS_DETACHED_HEAD\");\n> +\treturn 0;\n> +}\n\nOK.\n\n> +void ref_sorting_detached_head_first_all(struct ref_sorting *sorting, int flag)\n> +{\n> +\tfor (; sorting; sorting = sorting->next)\n> +\t\tsorting->detached_head_first = !!flag;\n> +}\n\nThis, taken together with existing ref_sorting_icase_all(), looks\nsomewhat ugly, especially when you ponder how you would add a third\nsimilar option to the mix.\n\nPerhaps \"ignore_case\" and \"detached_head_first\" shouldn't be\nseparate bitfields, but bits in the same flag word member in the\n\"struct ref_sorting\", and \"set/unset these flags to all the sort\nops\" helper function should just take a flags word that has two\nbits?\n\nOr maybe it is good enough for now. I hesitate to say so myself,\nthough, after already saying it is \"somewhat ugly\" ;-)\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 feaef4a8fde..3b92e0f2696 100644\n> --- a/ref-filter.h\n> +++ b/ref-filter.h\n> @@ -30,6 +30,7 @@ struct ref_sorting {\n> \tint atom; /* index into used_atom array (internal) */\n> \tunsigned reverse : 1,\n> \t\tignore_case : 1,\n> +\t\tdetached_head_first : 1,\n> \t\tversion : 1;\n> };\n> \n> @@ -111,6 +112,8 @@ int verify_ref_format(struct ref_format *format);\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> +/* Set the detached_head_first flag for all elements of a sorting list */\n> +void ref_sorting_detached_head_first_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"},{"id":"413626","messageId":"xmqqturtoccv.fsf@gitster.c.googlers.com","threadId":"51256","inReplyTo":"20210106100139.14651-6-avarab@gmail.com","subject":"Re: [PATCH 5/5] branch: show \"HEAD detached\" first under reverse sort","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-06T23:49:04Z","receivedAt":"2021-01-06T23:49:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> \tstruct atom_value *va, *vb;\n> \tint cmp;\n> +\tint cmp_detached_head = 0;\n> \tcmp_type cmp_type = used_atom[s->atom].type;\n> \tint (*cmp_fn)(const char *, const char *);\n> \tstruct strbuf err = STRBUF_INIT;\n> @@ -2370,6 +2371,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n> \t ^\n> \t (b->kind & FILTER_REFS_DETACHED_HEAD))) {\n> \t\tcmp = compare_detached_head(a, b);\n> +\t\tcmp_detached_head = 1;\n> \t} else if (s->version)\n> \t\tcmp = versioncmp(va->s, vb->s);\n> \telse if (cmp_type == FIELD_STR)\n> @@ -2383,7 +2385,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n> \t\t\tcmp = 1;\n> \t}\n> \n> -\treturn (s->reverse) ? -cmp : cmp;\n> +\treturn (s->reverse && !cmp_detached_head) ? -cmp : cmp;\n> }\n\nOK. Other criteria would honor the \"reverse\" bit, but when we work\non the set that includes \"HEAD\" ref (which only happens when \"branch -l\"\ndeals with a detached head), it always tries to sort it before all other\nrefs, regardless of the reverse bit. Makes sense.\n\n> static int compare_refs(const void *a_, const void *b_, void *ref_sorting)\n> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\n> index 8f53b081365..5e0577d5c7f 100755\n> --- a/t/t3203-branch-output.sh\n> +++ b/t/t3203-branch-output.sh\n> @@ -221,10 +221,10 @@ test_expect_success 'git branch `--sort=[-]objectsize` option' '\n> \ttest_i18ncmp expect actual &&\n> \n> \tcat >expect <<-\\EOF &&\n> +\t* (HEAD detached from fromtag)\n> \t branch-one\n> \t main\n> \t branch-two\n> -\t* (HEAD detached from fromtag)\n> \tEOF\n> \tgit branch --sort=-objectsize >actual &&\n> \ttest_i18ncmp expect actual\n> @@ -241,10 +241,10 @@ test_expect_success 'git branch `--sort=[-]type` option' '\n> \ttest_i18ncmp expect actual &&\n> \n> \tcat >expect <<-\\EOF &&\n> +\t* (HEAD detached from fromtag)\n> \t branch-one\n> \t branch-two\n> \t main\n> -\t* (HEAD detached from fromtag)\n> \tEOF\n> \tgit branch --sort=-type >actual &&\n> \ttest_i18ncmp expect actual\n> @@ -261,10 +261,10 @@ test_expect_success 'git branch `--sort=[-]version:refname` option' '\n> \ttest_i18ncmp expect actual &&\n> \n> \tcat >expect <<-\\EOF &&\n> +\t* (HEAD detached from fromtag)\n> \t main\n> \t branch-two\n> \t branch-one\n> -\t* (HEAD detached from fromtag)\n> \tEOF\n> \tgit branch --sort=-version:refname >actual &&\n> \ttest_i18ncmp expect actual\n"},{"id":"413667","messageId":"20210107095153.4753-2-avarab@gmail.com","threadId":"51256","inReplyTo":"20210106100139.14651-1-avarab@gmail.com","subject":"[PATCH v2 1/7] branch: change \"--local\" to \"--list\" in comment","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-01-07T09:51:47Z","receivedAt":"2021-01-07T09:53:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"There has never been a \"git branch --local\", this is just a typo for\n\"--list\". Fixes a comment added in 23e714df91c (branch: roll\nshow_detached HEAD into regular ref_list, 2015-09-23).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/branch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 9b68591addf..045866a51ae 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -726,7 +726,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tprint_current_branch_name();\n \t\treturn 0;\n \t} else if (list) {\n-\t\t/* git branch --local also shows HEAD when it is detached */\n+\t\t/* git branch --list also shows HEAD when it is detached */\n \t\tif ((filter.kind & FILTER_REFS_BRANCHES) && filter.detached)\n \t\t\tfilter.kind |= FILTER_REFS_DETACHED_HEAD;\n \t\tfilter.name_patterns = argv;\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"413668","messageId":"20210107095153.4753-1-avarab@gmail.com","threadId":"51256","inReplyTo":"20210106100139.14651-1-avarab@gmail.com","subject":"[PATCH v2 0/7] branch: --sort improvements","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-01-07T09:51:46Z","receivedAt":"2021-01-07T09:53:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Addresses a comment Junio had on 3/5 in v1. Now there's leading\npatches moving the ref_sorting flags to a bitfield, which (agreeing\nwith Junio) I think looks a bit better & should be more maintainable\ngoing forward.\n\nWhile I was at it I squashed the 3/5 and 4/5 patches into one. It's\nless confusing to look at when we add the ref-filter.c sorting code in\nthe same commit we're using it in. I also made the bitfield/xor\nchecking/sanity BUG a bit less verbose & simpler to understand.\n\nÆvar Arnfjörð Bjarmason (7):\n branch: change \"--local\" to \"--list\" in comment\n branch tests: add to --sort tests\n ref-filter: add braces to if/else if/else chain\n ref-filter: move \"cmp_fn\" assignment into \"else if\" arm\n ref-filter: move ref_sorting flags to a bitfield\n branch: sort detached HEAD based on a flag\n branch: show \"HEAD detached\" first under reverse sort\n\n builtin/branch.c | 6 ++--\n builtin/for-each-ref.c | 2 +-\n builtin/tag.c | 2 +-\n ref-filter.c | 75 ++++++++++++++++++++++++----------------\n ref-filter.h | 13 ++++---\n t/t3203-branch-output.sh | 51 ++++++++++++++++++++++++++-\n wt-status.c | 4 +--\n wt-status.h | 2 --\n 8 files changed, 111 insertions(+), 44 deletions(-)\n\nRange-diff:\n1: c74e75dea90 = 1: c74e75dea90 branch: change \"--local\" to \"--list\" in comment\n2: 1fea125c7a6 = 2: 1fea125c7a6 branch tests: add to --sort tests\n3: 11e6f274d2d < -: ----------- ref-filter: add a \"detached_head_first\" sorting option\n-: ----------- > 3: 5cb44f0be40 ref-filter: add braces to if/else if/else chain\n-: ----------- > 4: 3e26cebe545 ref-filter: move \"cmp_fn\" assignment into \"else if\" arm\n-: ----------- > 5: ad598fdc87c ref-filter: move ref_sorting flags to a bitfield\n4: faf9e23a13f ! 6: af0c884b506 branch: use the \"detached_head_first\" sorting option\n @@ Metadata\n Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - branch: use the \"detached_head_first\" sorting option\n + branch: sort detached HEAD based on a flag\n \n - Use the new ref_sorting_detached_head_first_all() sorting option in\n - ref-filter.c to revert and amend 28438e84e04 (ref-filter: sort\n - detached HEAD lines firstly, 2019-06-18).\n + Change the ref-filter sorting of detached HEAD to check the\n + FILTER_REFS_DETACHED_HEAD flag, instead of relying on the ref\n + description filled-in by get_head_description() to start with \"(\",\n + which in turn we expect to ASCII-sort before any other reference.\n \n - In Chinese the fullwidth versions of punctuation like \"()\" are\n - typically written as (U+FF08 fullwidth left parenthesis), (U+FF09\n - fullwidth right parenthesis) instead. This form is used in both\n - po/zh_{CN,TW}.po in most cases where \"()\" is translated in a string.\n -\n - In 28438e84e04 the ability to translate this as part of the \"git\n - branch -l\" output was removed because we'd like the detached line to\n - appear first at the start of \"git branch -l\", e.g.:\n + For context, we'd like the detached line to appear first at the start\n + of \"git branch -l\", e.g.:\n \n $ git branch -l\n * (HEAD detached at )\n master\n \n - Let's instead use the new ref_sorting_detached_head_first_all() in\n - branch.c to say that we'd like these sorted before other entries.\n + This doesn't change that, but improves on a fix made in\n + 28438e84e04 (ref-filter: sort detached HEAD lines firstly, 2019-06-18)\n + and gives the Chinese translation the ability to use its preferred\n + punctuation marks again.\n +\n + In Chinese the fullwidth versions of punctuation like \"()\" are\n + typically written as (U+FF08 fullwidth left parenthesis), (U+FF09\n + fullwidth right parenthesis) instead[1]. This form is used in both\n + po/zh_{CN,TW}.po in most cases where \"()\" is translated in a string.\n +\n + Aside from that improvement to the Chinese translation, it also just\n + makes for cleaner code that we mark any special cases in the ref_array\n + we're sorting with flags and make the sort function aware of them,\n + instead of piggy-backing on the general-case of strcmp() doing the\n + right thing.\n \n As seen in the amended tests this made reverse sorting a bit more\n consistent. Before this we'd sometimes sort this message in the\n - middle, now it's consistently at the beginning or end. Having it at\n - the end doesn't make much sense either, but at least it behaves\n - consistently now. A follow-up commit will make this behavior even\n - better.\n + middle, now it's consistently at the beginning or end, depending on\n + whether we're doing a normal or reverse sort. Having it at the end\n + doesn't make much sense either, but at least it behaves consistently\n + now. A follow-up commit will make this behavior under reverse sorting\n + even better.\n \n I'm removing the \"TRANSLATORS\" comments that were in the old code\n while I'm at it. Those were added in d4919bb288e (ref-filter: move\n @@ builtin/branch.c\n @@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tif (!sorting)\n \t\t\tsorting = ref_default_sorting();\n - \t\tref_sorting_icase_all(sorting, icase);\n -+\t\tref_sorting_detached_head_first_all(sorting, 1);\n + \t\tref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);\n ++\t\tref_sorting_set_sort_flags_all(\n ++\t\t\tsorting, REF_SORTING_DETACHED_HEAD_FIRST, 1);\n \t\tprint_ref_list(&filter, sorting, &format);\n \t\tprint_columns(&output, colopts, NULL);\n \t\tstring_list_clear(&output, 0);\n @@ ref-filter.c: char *get_head_description(void)\n \treturn strbuf_detach(&desc, NULL);\n }\n \n +@@ ref-filter.c: int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n + \treturn ret;\n + }\n + \n ++static int compare_detached_head(struct ref_array_item *a, struct ref_array_item *b)\n ++{\n ++\tif (!(a->kind ^ b->kind))\n ++\t\tBUG(\"ref_kind_from_refname() should only mark one ref as HEAD\");\n ++\tif (a->kind & FILTER_REFS_DETACHED_HEAD)\n ++\t\treturn -1;\n ++\telse if (b->kind & FILTER_REFS_DETACHED_HEAD)\n ++\t\treturn 1;\n ++\tBUG(\"should have died in the xor check above\");\n ++\treturn 0;\n ++}\n ++\n + static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, struct ref_array_item *b)\n + {\n + \tstruct atom_value *va, *vb;\n +@@ ref-filter.c: static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n + \tif (get_ref_atom_value(b, s->atom, &vb, &err))\n + \t\tdie(\"%s\", err.buf);\n + \tstrbuf_release(&err);\n +-\tif (s->sort_flags & REF_SORTING_VERSION) {\n ++\tif (s->sort_flags & REF_SORTING_DETACHED_HEAD_FIRST &&\n ++\t ((a->kind | b->kind) & FILTER_REFS_DETACHED_HEAD)) {\n ++\t\tcmp = compare_detached_head(a, b);\n ++\t} else if (s->sort_flags & REF_SORTING_VERSION) {\n + \t\tcmp = versioncmp(va->s, vb->s);\n + \t} else if (cmp_type == FIELD_STR) {\n + \t\tint (*cmp_fn)(const char *, const char *);\n +\n + ## ref-filter.h ##\n +@@ ref-filter.h: struct ref_sorting {\n + \t\tREF_SORTING_REVERSE = 1<<0,\n + \t\tREF_SORTING_ICASE = 1<<1,\n + \t\tREF_SORTING_VERSION = 1<<2,\n ++\t\tREF_SORTING_DETACHED_HEAD_FIRST = 1<<3,\n + \t} sort_flags;\n + };\n + \n \n ## t/t3203-branch-output.sh ##\n @@ t/t3203-branch-output.sh: test_expect_success 'git branch `--sort=[-]objectsize` option' '\n5: b14a7b32cbf ! 7: 2497fffebf6 branch: show \"HEAD detached\" first under reverse sort\n @@ ref-filter.c: static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array\n \tint cmp;\n +\tint cmp_detached_head = 0;\n \tcmp_type cmp_type = used_atom[s->atom].type;\n - \tint (*cmp_fn)(const char *, const char *);\n \tstruct strbuf err = STRBUF_INIT;\n + \n @@ ref-filter.c: static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n - \t ^\n - \t (b->kind & FILTER_REFS_DETACHED_HEAD))) {\n + \tif (s->sort_flags & REF_SORTING_DETACHED_HEAD_FIRST &&\n + \t ((a->kind | b->kind) & FILTER_REFS_DETACHED_HEAD)) {\n \t\tcmp = compare_detached_head(a, b);\n +\t\tcmp_detached_head = 1;\n - \t} else if (s->version)\n + \t} else if (s->sort_flags & REF_SORTING_VERSION) {\n \t\tcmp = versioncmp(va->s, vb->s);\n - \telse if (cmp_type == FIELD_STR)\n + \t} else if (cmp_type == FIELD_STR) {\n @@ ref-filter.c: static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n \t\t\tcmp = 1;\n \t}\n \n --\treturn (s->reverse) ? -cmp : cmp;\n -+\treturn (s->reverse && !cmp_detached_head) ? -cmp : cmp;\n +-\treturn (s->sort_flags & REF_SORTING_REVERSE) ? -cmp : cmp;\n ++\treturn (s->sort_flags & REF_SORTING_REVERSE && !cmp_detached_head)\n ++\t\t? -cmp : cmp;\n }\n \n static int compare_refs(const void *a_, const void *b_, void *ref_sorting)\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"413669","messageId":"20210107095153.4753-5-avarab@gmail.com","threadId":"51256","inReplyTo":"20210106100139.14651-1-avarab@gmail.com","subject":"[PATCH v2 4/7] ref-filter: move \"cmp_fn\" assignment into \"else if\" arm","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-01-07T09:51:50Z","receivedAt":"2021-01-07T09:53:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Further amend code changed in 7c5045fc180 (ref-filter: apply fallback\nrefname sort only after all user sorts, 2020-05-03) to move an\nassignment only used in the \"else if\" arm to happen there. Before that\ncommit the cmp_fn would be used outside of it.\n\nWe could also just skip the \"cmp_fn\" assignment and use\nstrcasecmp/strcmp directly in a ternary statement here, but this is\nprobably more readable.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n ref-filter.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex e4c162a8c34..8882128cd3e 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2355,7 +2355,6 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n \tstruct atom_value *va, *vb;\n \tint cmp;\n \tcmp_type cmp_type = used_atom[s->atom].type;\n-\tint (*cmp_fn)(const char *, const char *);\n \tstruct strbuf err = STRBUF_INIT;\n \n \tif (get_ref_atom_value(a, s->atom, &va, &err))\n@@ -2363,10 +2362,11 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n \tif (get_ref_atom_value(b, s->atom, &vb, &err))\n \t\tdie(\"%s\", err.buf);\n \tstrbuf_release(&err);\n-\tcmp_fn = s->ignore_case ? strcasecmp : strcmp;\n \tif (s->version) {\n \t\tcmp = versioncmp(va->s, vb->s);\n \t} else if (cmp_type == FIELD_STR) {\n+\t\tint (*cmp_fn)(const char *, const char *);\n+\t\tcmp_fn = s->ignore_case ? strcasecmp : strcmp;\n \t\tcmp = cmp_fn(va->s, vb->s);\n \t} else {\n \t\tif (va->value < vb->value)\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"413670","messageId":"20210107095153.4753-3-avarab@gmail.com","threadId":"51256","inReplyTo":"20210106100139.14651-1-avarab@gmail.com","subject":"[PATCH v2 2/7] branch tests: add to --sort tests","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-01-07T09:51:48Z","receivedAt":"2021-01-07T09:53:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Further stress the --sort callback in ref-filter.c. The implementation\nuses certain short-circuiting logic, let's make sure it behaves the\nsame way on e.g. name & version sort. Improves a test added in\naedcb7dc75e (branch.c: use 'ref-filter' APIs, 2015-09-23).\n\nI don't think all of this output makes sense, but let's test for the\nbehavior as-is, we can fix bugs in it in a later commit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t3203-branch-output.sh | 51 +++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 50 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\nindex b945faf4702..f92fb3aab9d 100755\n--- a/t/t3203-branch-output.sh\n+++ b/t/t3203-branch-output.sh\n@@ -210,7 +210,7 @@ EOF\n \ttest_i18ncmp expect actual\n '\n \n-test_expect_success 'git branch `--sort` option' '\n+test_expect_success 'git branch `--sort=[-]objectsize` option' '\n \tcat >expect <<-\\EOF &&\n \t* (HEAD detached from fromtag)\n \t branch-two\n@@ -218,6 +218,55 @@ test_expect_success 'git branch `--sort` option' '\n \t main\n \tEOF\n \tgit branch --sort=objectsize >actual &&\n+\ttest_i18ncmp expect actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\t branch-one\n+\t main\n+\t* (HEAD detached from fromtag)\n+\t branch-two\n+\tEOF\n+\tgit branch --sort=-objectsize >actual &&\n+\ttest_i18ncmp expect actual\n+'\n+\n+test_expect_success 'git branch `--sort=[-]type` option' '\n+\tcat >expect <<-\\EOF &&\n+\t* (HEAD detached from fromtag)\n+\t branch-one\n+\t branch-two\n+\t main\n+\tEOF\n+\tgit branch --sort=type >actual &&\n+\ttest_i18ncmp expect actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\t* (HEAD detached from fromtag)\n+\t branch-one\n+\t branch-two\n+\t main\n+\tEOF\n+\tgit branch --sort=-type >actual &&\n+\ttest_i18ncmp expect actual\n+'\n+\n+test_expect_success 'git branch `--sort=[-]version:refname` option' '\n+\tcat >expect <<-\\EOF &&\n+\t* (HEAD detached from fromtag)\n+\t branch-one\n+\t branch-two\n+\t main\n+\tEOF\n+\tgit branch --sort=version:refname >actual &&\n+\ttest_i18ncmp expect actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\t main\n+\t branch-two\n+\t branch-one\n+\t* (HEAD detached from fromtag)\n+\tEOF\n+\tgit branch --sort=-version:refname >actual &&\n \ttest_i18ncmp expect actual\n '\n \n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"413671","messageId":"20210107095153.4753-4-avarab@gmail.com","threadId":"51256","inReplyTo":"20210106100139.14651-1-avarab@gmail.com","subject":"[PATCH v2 3/7] ref-filter: add braces to if/else if/else chain","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-01-07T09:51:49Z","receivedAt":"2021-01-07T09:53:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Per the CodingGuidelines add braces to an if/else if/else chain where\nonly the \"else\" had braces. This is in preparation for a subsequent\nchange where the \"else if\" will have lines added to it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n ref-filter.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex aa260bfd099..e4c162a8c34 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2364,11 +2364,11 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n \t\tdie(\"%s\", err.buf);\n \tstrbuf_release(&err);\n \tcmp_fn = s->ignore_case ? strcasecmp : strcmp;\n-\tif (s->version)\n+\tif (s->version) {\n \t\tcmp = versioncmp(va->s, vb->s);\n-\telse if (cmp_type == FIELD_STR)\n+\t} else if (cmp_type == FIELD_STR) {\n \t\tcmp = cmp_fn(va->s, vb->s);\n-\telse {\n+\t} else {\n \t\tif (va->value < vb->value)\n \t\t\tcmp = -1;\n \t\telse if (va->value == vb->value)\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"413672","messageId":"20210107095153.4753-7-avarab@gmail.com","threadId":"51256","inReplyTo":"20210106100139.14651-1-avarab@gmail.com","subject":"[PATCH v2 6/7] branch: sort detached HEAD based on a flag","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-01-07T09:51:52Z","receivedAt":"2021-01-07T09:53:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the ref-filter sorting of detached HEAD to check the\nFILTER_REFS_DETACHED_HEAD flag, instead of relying on the ref\ndescription filled-in by get_head_description() to start with \"(\",\nwhich in turn we expect to ASCII-sort before any other reference.\n\nFor context, we'd like the detached line to appear first at the start\nof \"git branch -l\", e.g.:\n\n $ git branch -l\n * (HEAD detached at )\n master\n\nThis doesn't change that, but improves on a fix made in\n28438e84e04 (ref-filter: sort detached HEAD lines firstly, 2019-06-18)\nand gives the Chinese translation the ability to use its preferred\npunctuation marks again.\n\nIn Chinese the fullwidth versions of punctuation like \"()\" are\ntypically written as (U+FF08 fullwidth left parenthesis), (U+FF09\nfullwidth right parenthesis) instead[1]. This form is used in both\npo/zh_{CN,TW}.po in most cases where \"()\" is translated in a string.\n\nAside from that improvement to the Chinese translation, it also just\nmakes for cleaner code that we mark any special cases in the ref_array\nwe're sorting with flags and make the sort function aware of them,\ninstead of piggy-backing on the general-case of strcmp() doing the\nright thing.\n\nAs seen in the amended tests this made reverse sorting a bit more\nconsistent. Before this we'd sometimes sort this message in the\nmiddle, now it's consistently at the beginning or end, depending on\nwhether we're doing a normal or reverse sort. Having it at the end\ndoesn't make much sense either, but at least it behaves consistently\nnow. A follow-up commit will make this behavior under reverse sorting\neven better.\n\nI'm removing the \"TRANSLATORS\" comments that were in the old code\nwhile I'm at it. Those were added in d4919bb288e (ref-filter: move\nget_head_description() from branch.c, 2017-01-10). I think it's\nobvious from context, string and translation memory in typical\ntranslation tools that these are the same or similar string.\n\n1. https://en.wikipedia.org/wiki/Chinese_punctuation#Marks_similar_to_European_punctuation\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/branch.c | 2 ++\n ref-filter.c | 44 +++++++++++++++++++++++-----------------\n ref-filter.h | 1 +\n t/t3203-branch-output.sh | 4 ++--\n wt-status.c | 4 ++--\n wt-status.h | 2 --\n 6 files changed, 32 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 2dd51a8653b..8c0b428104d 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -740,6 +740,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tif (!sorting)\n \t\t\tsorting = ref_default_sorting();\n \t\tref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);\n+\t\tref_sorting_set_sort_flags_all(\n+\t\t\tsorting, REF_SORTING_DETACHED_HEAD_FIRST, 1);\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/ref-filter.c b/ref-filter.c\nindex fe587afb80b..8d0739b9972 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1536,36 +1536,27 @@ char *get_head_description(void)\n \tstruct wt_status_state state;\n \tmemset(&state, 0, sizeof(state));\n \twt_status_get_state(the_repository, &state, 1);\n-\n-\t/*\n-\t * The ( character must be hard-coded and not part of a localizable\n-\t * string, since the description is used as a sort key and compared\n-\t * with ref names.\n-\t */\n-\tstrbuf_addch(&desc, '(');\n \tif (state.rebase_in_progress ||\n \t state.rebase_interactive_in_progress) {\n \t\tif (state.branch)\n-\t\t\tstrbuf_addf(&desc, _(\"no branch, rebasing %s\"),\n+\t\t\tstrbuf_addf(&desc, _(\"(no branch, rebasing %s)\"),\n \t\t\t\t state.branch);\n \t\telse\n-\t\t\tstrbuf_addf(&desc, _(\"no branch, rebasing detached HEAD %s\"),\n+\t\t\tstrbuf_addf(&desc, _(\"(no branch, rebasing detached HEAD %s)\"),\n \t\t\t\t state.detached_from);\n \t} else if (state.bisect_in_progress)\n-\t\tstrbuf_addf(&desc, _(\"no branch, bisect started on %s\"),\n+\t\tstrbuf_addf(&desc, _(\"(no branch, bisect started on %s)\"),\n \t\t\t state.branch);\n \telse if (state.detached_from) {\n \t\tif (state.detached_at)\n-\t\t\tstrbuf_addstr(&desc, HEAD_DETACHED_AT);\n+\t\t\tstrbuf_addf(&desc, _(\"(HEAD detached at %s)\"),\n+\t\t\t\tstate.detached_from);\n \t\telse\n-\t\t\tstrbuf_addstr(&desc, HEAD_DETACHED_FROM);\n-\t\tstrbuf_addstr(&desc, state.detached_from);\n-\t}\n-\telse\n-\t\tstrbuf_addstr(&desc, _(\"no branch\"));\n-\tstrbuf_addch(&desc, ')');\n+\t\t\tstrbuf_addf(&desc, _(\"(HEAD detached from %s)\"),\n+\t\t\t\tstate.detached_from);\n+\t} else\n+\t\tstrbuf_addstr(&desc, _(\"(no branch)\"));\n \n-\twt_status_state_free_buffers(&state);\n \treturn strbuf_detach(&desc, NULL);\n }\n \n@@ -2350,6 +2341,18 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n \treturn ret;\n }\n \n+static int compare_detached_head(struct ref_array_item *a, struct ref_array_item *b)\n+{\n+\tif (!(a->kind ^ b->kind))\n+\t\tBUG(\"ref_kind_from_refname() should only mark one ref as HEAD\");\n+\tif (a->kind & FILTER_REFS_DETACHED_HEAD)\n+\t\treturn -1;\n+\telse if (b->kind & FILTER_REFS_DETACHED_HEAD)\n+\t\treturn 1;\n+\tBUG(\"should have died in the xor check above\");\n+\treturn 0;\n+}\n+\n static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, struct ref_array_item *b)\n {\n \tstruct atom_value *va, *vb;\n@@ -2362,7 +2365,10 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n \tif (get_ref_atom_value(b, s->atom, &vb, &err))\n \t\tdie(\"%s\", err.buf);\n \tstrbuf_release(&err);\n-\tif (s->sort_flags & REF_SORTING_VERSION) {\n+\tif (s->sort_flags & REF_SORTING_DETACHED_HEAD_FIRST &&\n+\t ((a->kind | b->kind) & FILTER_REFS_DETACHED_HEAD)) {\n+\t\tcmp = compare_detached_head(a, b);\n+\t} else if (s->sort_flags & REF_SORTING_VERSION) {\n \t\tcmp = versioncmp(va->s, vb->s);\n \t} else if (cmp_type == FIELD_STR) {\n \t\tint (*cmp_fn)(const char *, const char *);\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 6296ae8bb27..19ea4c41340 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -32,6 +32,7 @@ struct ref_sorting {\n \t\tREF_SORTING_REVERSE = 1<<0,\n \t\tREF_SORTING_ICASE = 1<<1,\n \t\tREF_SORTING_VERSION = 1<<2,\n+\t\tREF_SORTING_DETACHED_HEAD_FIRST = 1<<3,\n \t} sort_flags;\n };\n \ndiff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\nindex f92fb3aab9d..8f53b081365 100755\n--- a/t/t3203-branch-output.sh\n+++ b/t/t3203-branch-output.sh\n@@ -223,8 +223,8 @@ test_expect_success 'git branch `--sort=[-]objectsize` option' '\n \tcat >expect <<-\\EOF &&\n \t branch-one\n \t main\n-\t* (HEAD detached from fromtag)\n \t branch-two\n+\t* (HEAD detached from fromtag)\n \tEOF\n \tgit branch --sort=-objectsize >actual &&\n \ttest_i18ncmp expect actual\n@@ -241,10 +241,10 @@ test_expect_success 'git branch `--sort=[-]type` option' '\n \ttest_i18ncmp expect actual &&\n \n \tcat >expect <<-\\EOF &&\n-\t* (HEAD detached from fromtag)\n \t branch-one\n \t branch-two\n \t main\n+\t* (HEAD detached from fromtag)\n \tEOF\n \tgit branch --sort=-type >actual &&\n \ttest_i18ncmp expect actual\ndiff --git a/wt-status.c b/wt-status.c\nindex 7074bbdd53c..40b59be478c 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1742,9 +1742,9 @@ static void wt_longstatus_print(struct wt_status *s)\n \t\t\t} else if (s->state.detached_from) {\n \t\t\t\tbranch_name = s->state.detached_from;\n \t\t\t\tif (s->state.detached_at)\n-\t\t\t\t\ton_what = HEAD_DETACHED_AT;\n+\t\t\t\t\ton_what = _(\"HEAD detached at \");\n \t\t\t\telse\n-\t\t\t\t\ton_what = HEAD_DETACHED_FROM;\n+\t\t\t\t\ton_what = _(\"HEAD detached from \");\n \t\t\t} else {\n \t\t\t\tbranch_name = \"\";\n \t\t\t\ton_what = _(\"Not currently on any branch.\");\ndiff --git a/wt-status.h b/wt-status.h\nindex 35b44c388ed..0d32799b28e 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -77,8 +77,6 @@ enum wt_status_format {\n \tSTATUS_FORMAT_UNSPECIFIED\n };\n \n-#define HEAD_DETACHED_AT _(\"HEAD detached at \")\n-#define HEAD_DETACHED_FROM _(\"HEAD detached from \")\n #define SPARSE_CHECKOUT_DISABLED -1\n \n struct wt_status_state {\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"413673","messageId":"20210107095153.4753-8-avarab@gmail.com","threadId":"51256","inReplyTo":"20210106100139.14651-1-avarab@gmail.com","subject":"[PATCH v2 7/7] branch: show \"HEAD detached\" first under reverse sort","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-01-07T09:51:53Z","receivedAt":"2021-01-07T09:53:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the output of the likes of \"git branch -l --sort=-objectsize\"\nto show the \"(HEAD detached at )\" message at the start of the\noutput. Before the compare_detached_head() function added in a\npreceding commit we'd emit this output as an emergent effect.\n\nIt doesn't make any sense to consider the objectsize, type or other\nnon-attribute of the \"(HEAD detached at )\" message for the\npurposes of sorting. Let's always emit it at the top instead. The only\nreason it was sorted in the first place is because we're injecting it\ninto the ref-filter machinery so builtin/branch.c doesn't need to do\nits own \"am I detached?\" detection.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n ref-filter.c | 5 ++++-\n t/t3203-branch-output.sh | 6 +++---\n 2 files changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 8d0739b9972..ee337df232a 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2357,6 +2357,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n {\n \tstruct atom_value *va, *vb;\n \tint cmp;\n+\tint cmp_detached_head = 0;\n \tcmp_type cmp_type = used_atom[s->atom].type;\n \tstruct strbuf err = STRBUF_INIT;\n \n@@ -2368,6 +2369,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n \tif (s->sort_flags & REF_SORTING_DETACHED_HEAD_FIRST &&\n \t ((a->kind | b->kind) & FILTER_REFS_DETACHED_HEAD)) {\n \t\tcmp = compare_detached_head(a, b);\n+\t\tcmp_detached_head = 1;\n \t} else if (s->sort_flags & REF_SORTING_VERSION) {\n \t\tcmp = versioncmp(va->s, vb->s);\n \t} else if (cmp_type == FIELD_STR) {\n@@ -2384,7 +2386,8 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n \t\t\tcmp = 1;\n \t}\n \n-\treturn (s->sort_flags & REF_SORTING_REVERSE) ? -cmp : cmp;\n+\treturn (s->sort_flags & REF_SORTING_REVERSE && !cmp_detached_head)\n+\t\t? -cmp : cmp;\n }\n \n static int compare_refs(const void *a_, const void *b_, void *ref_sorting)\ndiff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\nindex 8f53b081365..5e0577d5c7f 100755\n--- a/t/t3203-branch-output.sh\n+++ b/t/t3203-branch-output.sh\n@@ -221,10 +221,10 @@ test_expect_success 'git branch `--sort=[-]objectsize` option' '\n \ttest_i18ncmp expect actual &&\n \n \tcat >expect <<-\\EOF &&\n+\t* (HEAD detached from fromtag)\n \t branch-one\n \t main\n \t branch-two\n-\t* (HEAD detached from fromtag)\n \tEOF\n \tgit branch --sort=-objectsize >actual &&\n \ttest_i18ncmp expect actual\n@@ -241,10 +241,10 @@ test_expect_success 'git branch `--sort=[-]type` option' '\n \ttest_i18ncmp expect actual &&\n \n \tcat >expect <<-\\EOF &&\n+\t* (HEAD detached from fromtag)\n \t branch-one\n \t branch-two\n \t main\n-\t* (HEAD detached from fromtag)\n \tEOF\n \tgit branch --sort=-type >actual &&\n \ttest_i18ncmp expect actual\n@@ -261,10 +261,10 @@ test_expect_success 'git branch `--sort=[-]version:refname` option' '\n \ttest_i18ncmp expect actual &&\n \n \tcat >expect <<-\\EOF &&\n+\t* (HEAD detached from fromtag)\n \t main\n \t branch-two\n \t branch-one\n-\t* (HEAD detached from fromtag)\n \tEOF\n \tgit branch --sort=-version:refname >actual &&\n \ttest_i18ncmp expect actual\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"413674","messageId":"20210107095153.4753-6-avarab@gmail.com","threadId":"51256","inReplyTo":"20210106100139.14651-1-avarab@gmail.com","subject":"[PATCH v2 5/7] ref-filter: move ref_sorting flags to a bitfield","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-01-07T09:51:51Z","receivedAt":"2021-01-07T09:53:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the reverse/ignore_case/version sort flags in the ref_sorting\nstruct into a bitfield. Having three of them was already a bit\nunwieldy, but it would be even more so if another flag needed a\nfunction like ref_sorting_icase_all() introduced in\n76f9e569adb (ref-filter: apply --ignore-case to all sorting keys,\n2020-05-03).\n\nA follow-up change will introduce such a flag, so let's move this over\nto a bitfield. Instead of using the usual '#define' pattern I'm using\nthe \"enum\" pattern from builtin/rebase.c's b4c8eb024af (builtin\nrebase: support --quiet, 2018-09-04).\n\nPerhaps there's a more idiomatic way of doing the \"for each in list\namend mask\" pattern than this \"mask/on\" variable combo. This function\ndoesn't allow us to e.g. do any arbitrary changes to the bitfield for\nmultiple flags, but I think in this case that's fine. The common case\nis that we're calling this with a list of one.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/branch.c | 2 +-\n builtin/for-each-ref.c | 2 +-\n builtin/tag.c | 2 +-\n ref-filter.c | 24 +++++++++++++++---------\n ref-filter.h | 12 +++++++-----\n 5 files changed, 25 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 045866a51ae..2dd51a8653b 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\tref_sorting_icase_all(sorting, icase);\n+\t\tref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, 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 9d1ecda2b8f..cb9c81a0460 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-\tref_sorting_icase_all(sorting, icase);\n+\tref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);\n \tfilter.ignore_case = icase;\n \n \tfilter.name_patterns = argv;\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex ecf011776dc..24d35b746d1 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-\tref_sorting_icase_all(sorting, icase);\n+\tref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, 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 8882128cd3e..fe587afb80b 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2362,11 +2362,12 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n \tif (get_ref_atom_value(b, s->atom, &vb, &err))\n \t\tdie(\"%s\", err.buf);\n \tstrbuf_release(&err);\n-\tif (s->version) {\n+\tif (s->sort_flags & REF_SORTING_VERSION) {\n \t\tcmp = versioncmp(va->s, vb->s);\n \t} else if (cmp_type == FIELD_STR) {\n \t\tint (*cmp_fn)(const char *, const char *);\n-\t\tcmp_fn = s->ignore_case ? strcasecmp : strcmp;\n+\t\tcmp_fn = s->sort_flags & REF_SORTING_ICASE\n+\t\t\t? strcasecmp : strcmp;\n \t\tcmp = cmp_fn(va->s, vb->s);\n \t} else {\n \t\tif (va->value < vb->value)\n@@ -2377,7 +2378,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n \t\t\tcmp = 1;\n \t}\n \n-\treturn (s->reverse) ? -cmp : cmp;\n+\treturn (s->sort_flags & REF_SORTING_REVERSE) ? -cmp : cmp;\n }\n \n static int compare_refs(const void *a_, const void *b_, void *ref_sorting)\n@@ -2392,15 +2393,20 @@ static int compare_refs(const void *a_, const void *b_, void *ref_sorting)\n \t\t\treturn cmp;\n \t}\n \ts = ref_sorting;\n-\treturn s && s->ignore_case ?\n+\treturn s && s->sort_flags & REF_SORTING_ICASE ?\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)\n+void ref_sorting_set_sort_flags_all(struct ref_sorting *sorting,\n+\t\t\t\t unsigned int mask, int on)\n {\n-\tfor (; sorting; sorting = sorting->next)\n-\t\tsorting->ignore_case = !!flag;\n+\tfor (; sorting; sorting = sorting->next) {\n+\t\tif (on)\n+\t\t\tsorting->sort_flags |= mask;\n+\t\telse\n+\t\t\tsorting->sort_flags &= ~mask;\n+\t}\n }\n \n void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)\n@@ -2537,12 +2543,12 @@ void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)\n \t*sorting_tail = s;\n \n \tif (*arg == '-') {\n-\t\ts->reverse = 1;\n+\t\ts->sort_flags |= REF_SORTING_REVERSE;\n \t\targ++;\n \t}\n \tif (skip_prefix(arg, \"version:\", &arg) ||\n \t skip_prefix(arg, \"v:\", &arg))\n-\t\ts->version = 1;\n+\t\ts->sort_flags |= REF_SORTING_VERSION;\n \ts->atom = parse_sorting_atom(arg);\n }\n \ndiff --git a/ref-filter.h b/ref-filter.h\nindex feaef4a8fde..6296ae8bb27 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -28,9 +28,11 @@ struct atom_value;\n struct ref_sorting {\n \tstruct ref_sorting *next;\n \tint atom; /* index into used_atom array (internal) */\n-\tunsigned reverse : 1,\n-\t\tignore_case : 1,\n-\t\tversion : 1;\n+\tenum {\n+\t\tREF_SORTING_REVERSE = 1<<0,\n+\t\tREF_SORTING_ICASE = 1<<1,\n+\t\tREF_SORTING_VERSION = 1<<2,\n+\t} sort_flags;\n };\n \n struct ref_array_item {\n@@ -109,8 +111,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+/* Set REF_SORTING_* sort_flags for all elements of a sorting list */\n+void ref_sorting_set_sort_flags_all(struct ref_sorting *sorting, unsigned int mask, int on);\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-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"413770","messageId":"xmqqpn2gjpoa.fsf@gitster.c.googlers.com","threadId":"51256","inReplyTo":"20210107095153.4753-6-avarab@gmail.com","subject":"Re: [PATCH v2 5/7] ref-filter: move ref_sorting flags to a bitfield","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-07T23:24:53Z","receivedAt":"2021-01-07T23:25:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Perhaps there's a more idiomatic way of doing the \"for each in list\n> amend mask\" pattern than this \"mask/on\" variable combo. This function\n> doesn't allow us to e.g. do any arbitrary changes to the bitfield for\n> multiple flags, but I think in this case that's fine. The common case\n> is that we're calling this with a list of one.\n\nAn obvious alternative would be to pass two masks, one for setting\nand the other for clearing, instead of passing a mask and a bool\nthat says if the mask is for setting or clearing.\n\nThe helper that follows such a design would be:\n\n\tvoid ref_sorting_tweak_flags(struct ref_sorting *sorting,\n\t\t\t\t unsigned set, unsigned clear)\n\t{\n\t\twhile (sorting) {\n\t\t\tsorting->sort_flags |= set;\n\t\t\tsorting->sort_flags &= ~clear;\n\t\t\tsorting = sorting->next;\n\t\t}\n\t}\n\nand the caller in the endgame would become\n\n\t...\n\t} else if (list) {\n\t\tunsigned set = REF_SORTING_DETACHED_HEAD_FIRST;\n\t\tunsigned clear = 0;\n\n *(icase ? &set : &clear) |= REF_SORTING_ICASE;\n\t\tref_sorting_tweak_flags(sorting, set, clear);\n\nwhich may be more lines but probably copes better when adding new\nbits.\n\nThanks.\n"}]}