| Message ID | cf0246a5cce6cbd9b4a1fd1eefa0f5cbc2cfcaf0.1560277373.git.matvore@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Sort detached HEAD lines firstly | expand |
Matthew DeVore <matvore@google.com> writes: > - if (s->version) > + if (s->version) { > cmp = versioncmp(va->s, vb->s); > - else if (cmp_type == FIELD_STR) > - cmp = cmp_fn(va->s, vb->s); > - else { Ah, this must be the patch noise Jonathan was (half) complaining about. It does make it a bit distracting to read the patch but the resulting code is of course easier to follow ;-). > + } else if (cmp_type == FIELD_STR) { > + const int a_detached = a->kind & FILTER_REFS_DETACHED_HEAD; > + > + /* > + * When sorting by name, we should put "detached" head lines, > + * which are all the lines in parenthesis, before all others. > + * This usually is automatic, since "(" is before "refs/" and > + * "remotes/", but this does not hold for zh_CN, which uses > + * full-width parenthesis, so make the ordering explicit. > + */ > + if (a_detached != (b->kind & FILTER_REFS_DETACHED_HEAD)) > + cmp = a_detached ? -1 : 1; So, comparing a detached and an undetached ones, the detached side always sorts lower. Good. And ... > + else > + cmp = cmp_fn(va->s, vb->s); ... otherwise we compare the string using the given function. Sounds sensible. Will queue.
Hi Matthew, On Tue, 11 Jun 2019, Matthew DeVore wrote: > diff --git a/ref-filter.c b/ref-filter.c > index 8500671bc6..056d21d666 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -2157,25 +2157,37 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru > cmp_type cmp_type = used_atom[s->atom].type; > int (*cmp_fn)(const char *, const char *); > struct strbuf err = STRBUF_INIT; > > if (get_ref_atom_value(a, s->atom, &va, &err)) > die("%s", err.buf); > if (get_ref_atom_value(b, s->atom, &vb, &err)) > die("%s", err.buf); > strbuf_release(&err); > cmp_fn = s->ignore_case ? strcasecmp : strcmp; > - if (s->version) > + if (s->version) { > cmp = versioncmp(va->s, vb->s); > - else if (cmp_type == FIELD_STR) > - cmp = cmp_fn(va->s, vb->s); > - else { > + } else if (cmp_type == FIELD_STR) { I still think that this slipped-in `{` makes this patch harder to read than necessary. Your argument that you introduce the first curlies in an `else` block does not hold, as the removed `else {` line above demonstrates quite clearly. But you seem dead set to do it nevertheless, so I'll save my breath. > + const int a_detached = a->kind & FILTER_REFS_DETACHED_HEAD; > + > + /* > + * When sorting by name, we should put "detached" head lines, > + * which are all the lines in parenthesis, before all others. > + * This usually is automatic, since "(" is before "refs/" and > + * "remotes/", but this does not hold for zh_CN, which uses > + * full-width parenthesis, so make the ordering explicit. > + */ > + if (a_detached != (b->kind & FILTER_REFS_DETACHED_HEAD)) > + cmp = a_detached ? -1 : 1; > + else > + cmp = cmp_fn(va->s, vb->s); > + } else { > if (va->value < vb->value) > cmp = -1; > else if (va->value == vb->value) > cmp = cmp_fn(a->refname, b->refname); > else > cmp = 1; > } > > return (s->reverse) ? -cmp : cmp; > } > diff --git a/t/lib-gettext.sh b/t/lib-gettext.sh > index 2139b427ca..1adf1d4c31 100644 > --- a/t/lib-gettext.sh > +++ b/t/lib-gettext.sh > @@ -25,23 +25,29 @@ then > p > q > }') > # is_IS.ISO8859-1 on Solaris and FreeBSD, is_IS.iso88591 on Debian > is_IS_iso_locale=$(locale -a 2>/dev/null | > sed -n '/^is_IS\.[iI][sS][oO]8859-*1$/{ > p > q > }') > > - # Export them as an environment variable so the t0202/test.pl Perl > - # test can use it too > - export is_IS_locale is_IS_iso_locale > + zh_CN_locale=$(locale -a 2>/dev/null | > + sed -n '/^zh_CN\.[uU][tT][fF]-*8$/{ > + p > + q > + }') > + > + # Export them as environment variables so other tests can use them > + # too > + export is_IS_locale is_IS_iso_locale zh_CN_locale > > if test -n "$is_IS_locale" && > test $GIT_INTERNAL_GETTEXT_SH_SCHEME != "fallthrough" > then > # Some of the tests need the reference Icelandic locale > test_set_prereq GETTEXT_LOCALE > > # Exporting for t0202/test.pl > GETTEXT_LOCALE=1 > export GETTEXT_LOCALE > @@ -53,11 +59,21 @@ then > if test -n "$is_IS_iso_locale" && > test $GIT_INTERNAL_GETTEXT_SH_SCHEME != "fallthrough" > then > # Some of the tests need the reference Icelandic locale > test_set_prereq GETTEXT_ISO_LOCALE > > say "# lib-gettext: Found '$is_IS_iso_locale' as an is_IS ISO-8859-1 locale" > else > say "# lib-gettext: No is_IS ISO-8859-1 locale available" > fi > + > + if test -n "$zh_CN_locale" && > + test $GIT_INTERNAL_GETTEXT_SH_SCHEME != "fallthrough" > + then > + test_set_prereq GETTEXT_ZH_LOCALE > + > + say "# lib-gettext: Found '$zh_CN_locale' as a zh_CN UTF-8 locale" > + else > + say "# lib-gettext: No zh_CN UTF-8 locale available" > + fi > fi > diff --git a/t/t3207-branch-intl.sh b/t/t3207-branch-intl.sh > new file mode 100755 > index 0000000000..a46538188c > --- /dev/null > +++ b/t/t3207-branch-intl.sh > @@ -0,0 +1,41 @@ > +#!/bin/sh > + > +test_description='git branch internationalization tests' > + > +. ./lib-gettext.sh > + > +test_expect_success 'init repo' ' > + git init r1 && > + test_commit -C r1 first > +' I still see absolutely no need for initializing `r1`. Every test script in Git's test suite starts out with a fully initialized repository, no `git init` necessary. Therefore, this test case seems to have an unnecessary `git init` and multiple unnecessary `-C r1` options that make the script quite noisy. I mean, you initialize that `r1`, work on it exclusively, and completely ignore the repository that has been initialized in `.git` for you. > +test_expect_success GETTEXT_ZH_LOCALE 'detached head sorts before branches' ' > + # Ref sorting logic should put detached heads before the other > + # branches, but this is not automatic when a branch name sorts > + # lexically before "(" or the full-width "(" (Unicode codepoint FF08). > + # The latter case is nearly guaranteed for the Chinese locale. > + > + test_when_finished "git -C r1 checkout master" && > + > + git -C r1 checkout HEAD^{} -- && `HEAD^0` is a much more canonical way to say this. However, if you want your test case to be easy to understand (and that is your goal, too, right, not only mine?), you will instead use git checkout --detach > + LC_ALL=$zh_CN_locale LC_MESSAGES=$zh_CN_locale \ > + git -C r1 branch >actual && > + > + head -n 1 actual >first && > + # The first line should be enclosed by full-width parenthesis. > + grep "（.*）" first && I wonder whether it is a good idea to pretend that we can pass arbitrary byte sequences to `grep`, independent of the current locale. On Windows, this does not hold true, for example. It would probably make more sense to store a support file in t/t3207/, much like it is done in t3900. And once you do that, you can simply `test_cmp t3207/first first`. No need to `grep` for `master` in addition: > + grep master actual > +' > + > +test_expect_success 'detached head honors reverse sorting' ' > + test_when_finished "git -C r1 checkout master" && Hmm. I see you also did that in the previous test case, but since you immediately detach the HEAD, I have to ask: - why? Why do you insist on switching back to `master` after the test case finished? - Why even bother to call `git checkout --detach` in anything but the very first test case, whose purpose it is to set things up for the subsequent test cases, after all? > + > + git -C r1 checkout HEAD^{} -- && > + git -C r1 branch --sort=-refname >actual && > + > + head -n 1 actual >first && > + grep master first && > + test_i18ngrep "HEAD detached" actual Funny, reading the test case's title, I would have expected to read instead: echo "* HEAD detached" >expect && tail -n 1 actual >last && test_cmp expect last In all, the test script should read more like this: test_expect_success 'setup' ' test_commit first && git checkout --detach ' # [... long comment here, does not need to be hidden and indented # inside...] test_expect_success GETTEXT_ZH_LOCALE 'detached HEAD sorts first' ' LC_ALL=$zh_CN_locale LC_MESSAGES=$zh_CN_locale git branch >actual && head -n 1 <actual >first && test_cmp "$TEST_DIRECTORY/../t3207/first" first ' test_expect_success 'detached HEAD reverse-sorts last' ' git branch --sort=-refname >actual && echo "* HEAD detached" >expect && tail -n 1 actual >last && test_cmp expect last ' It is quite possible that this can be simplified even further, i.e. made even easier to understand for developers in the unfortunate situation of having to debug a regression (which is the entire goal of a well-written regression test: to help, rather than just to force, developers to debug regressions). Granted, the simpler form might look like it took less effort to write than the complicated one. People with some experience in software development will understand the opposite to be true, though. Ciao, Dscho > +' > + > +test_done > -- > 2.21.0 > >
Junio C Hamano <gitster@pobox.com> writes: >> + /* >> + * When sorting by name, we should put "detached" head lines, >> + * which are all the lines in parenthesis, before all others. >> + * This usually is automatic, since "(" is before "refs/" and >> + * "remotes/", but this does not hold for zh_CN, which uses >> + * full-width parenthesis, so make the ordering explicit. >> + */ >> + if (a_detached != (b->kind & FILTER_REFS_DETACHED_HEAD)) >> + cmp = a_detached ? -1 : 1; > > So, comparing a detached and an undetached ones, the detached side > always sorts lower. Good. And ... > >> + else >> + cmp = cmp_fn(va->s, vb->s); > > ... otherwise we compare the string using the given function. > > Sounds sensible. Will queue. Stepping back a bit, why are we even allowing the surrounding () pair to be futzed by the translators? IOW, shouldn't our code more like this from the beginning, with or without Chinese translation? With a bit more work, we may even be able to lose "make sure this matches the one in wt-status.c" comment as losing the leading '(' would take us one step closer to have an identical string here as we have in wt-status.c ref-filter.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index 8500671bc6..7e4705fcb2 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1459,20 +1459,22 @@ char *get_head_description(void) strbuf_addf(&desc, _("(no branch, bisect started on %s)"), state.branch); else if (state.detached_from) { + strbuf_addch(&desc, '('); if (state.detached_at) /* * TRANSLATORS: make sure this matches "HEAD * detached at " in wt-status.c */ - strbuf_addf(&desc, _("(HEAD detached at %s)"), - state.detached_from); + strbuf_addf(&desc, _("HEAD detached at %s"), + state.detached_from); else /* * TRANSLATORS: make sure this matches "HEAD * detached from " in wt-status.c */ - strbuf_addf(&desc, _("(HEAD detached from %s)"), + strbuf_addf(&desc, _("HEAD detached from %s"), state.detached_from); + strbuf_addch(&desc, ')'); } else strbuf_addstr(&desc, _("(no branch)"));
On Wed, Jun 12, 2019 at 02:09:53PM -0700, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > >> + /* > >> + * When sorting by name, we should put "detached" head lines, > >> + * which are all the lines in parenthesis, before all others. > >> + * This usually is automatic, since "(" is before "refs/" and > >> + * "remotes/", but this does not hold for zh_CN, which uses > >> + * full-width parenthesis, so make the ordering explicit. > >> + */ > >> + if (a_detached != (b->kind & FILTER_REFS_DETACHED_HEAD)) > >> + cmp = a_detached ? -1 : 1; > > > > So, comparing a detached and an undetached ones, the detached side > > always sorts lower. Good. And ... > > > >> + else > >> + cmp = cmp_fn(va->s, vb->s); > > > > ... otherwise we compare the string using the given function. > > > > Sounds sensible. Will queue. > > Stepping back a bit, why are we even allowing the surrounding () > pair to be futzed by the translators? I was thinking about removing () from the translated strings, but decided against it since there are a lot of full-width parenthesis in the translated strings already: $ cd po; git grep -B 1 'msgstr.*（' ... 246 matches in zh_CN ... and it seems strange to force only a few pairs of parens to be half-width to make the code simpler. I don't know if that's a great argument, since it is somewhat aesthetic. I would have liked half-width parens more if it were closing off purely ASCII text. But it is in fact surrounding Chinese text: $ git branch * （头指针分离于 cf0246a5cc） > > IOW, shouldn't our code more like this from the beginning, with or > without Chinese translation? > > With a bit more work, we may even be able to lose "make sure this > matches the one in wt-status.c" comment as losing the leading '(' > would take us one step closer to have an identical string here as we > have in wt-status.c > > ref-filter.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/ref-filter.c b/ref-filter.c > index 8500671bc6..7e4705fcb2 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -1459,20 +1459,22 @@ char *get_head_description(void) > strbuf_addf(&desc, _("(no branch, bisect started on %s)"), > state.branch); > else if (state.detached_from) { > + strbuf_addch(&desc, '('); > if (state.detached_at) > /* > * TRANSLATORS: make sure this matches "HEAD > * detached at " in wt-status.c > */ > - strbuf_addf(&desc, _("(HEAD detached at %s)"), > - state.detached_from); > + strbuf_addf(&desc, _("HEAD detached at %s"), > + state.detached_from); > else > /* > * TRANSLATORS: make sure this matches "HEAD > * detached from " in wt-status.c > */ > - strbuf_addf(&desc, _("(HEAD detached from %s)"), > + strbuf_addf(&desc, _("HEAD detached from %s"), > state.detached_from); > + strbuf_addch(&desc, ')'); > } > else > strbuf_addstr(&desc, _("(no branch)")); > >
On Wed, Jun 12, 2019 at 02:09:53PM -0700, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > Stepping back a bit, why are we even allowing the surrounding () > pair to be futzed by the translators? > > IOW, shouldn't our code more like this from the beginning, with or > without Chinese translation? > > With a bit more work, we may even be able to lose "make sure this > matches the one in wt-status.c" comment as losing the leading '(' > would take us one step closer to have an identical string here as we > have in wt-status.c I think my previous e-mail didn't make it to the public list, maybe since it contained non-ASCII text. The gist of that mail was that we have full-width parens in various zh_CN strings so it seems hacky to make just this one be half-width for the sake of code simplicity. Giving this a bit more thought now, perhaps the fact that we want to be in-sync with the string in wt-status.c, combined with the fact that we already have "*" as half-width metacharacter in the "git branch" output, is a good enough excuse to drop "()" out of the translatable string, as your patch does.
On Wed, Jun 12, 2019 at 09:51:33PM +0200, Johannes Schindelin wrote: > > + head -n 1 actual >first && > > + # The first line should be enclosed by full-width parenthesis. > > + grep "（.*）" first && > > I wonder whether it is a good idea to pretend that we can pass arbitrary > byte sequences to `grep`, independent of the current locale. On Windows, > this does not hold true, for example. > > It would probably make more sense to store a support file in t/t3207/, > much like it is done in t3900. > > And once you do that, you can simply `test_cmp t3207/first first`. No > need to `grep` for `master` in addition: I was just writing a similar response in another part of the thread, and found this. :) In addition to grep portability problems, IMHO the source with the raw UTF-8 characters is harder to read. Even if your editor and terminal support UTF-8, people without the right fonts will just get a bunch of empty boxes. And when debugging, you often care about the raw bytes anyway (e.g., when there are multiple representations of the same glyph). Adding a support file is fine, but for small cases like this, it may be easier to do: printf '\357\274\210...' >expect but note that this _must_ be octal, not hex, as many versions of printf only handle the former. -Peff
diff --git a/ref-filter.c b/ref-filter.c index 8500671bc6..056d21d666 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -2157,25 +2157,37 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru cmp_type cmp_type = used_atom[s->atom].type; int (*cmp_fn)(const char *, const char *); struct strbuf err = STRBUF_INIT; if (get_ref_atom_value(a, s->atom, &va, &err)) die("%s", err.buf); if (get_ref_atom_value(b, s->atom, &vb, &err)) die("%s", err.buf); strbuf_release(&err); cmp_fn = s->ignore_case ? strcasecmp : strcmp; - if (s->version) + if (s->version) { cmp = versioncmp(va->s, vb->s); - else if (cmp_type == FIELD_STR) - cmp = cmp_fn(va->s, vb->s); - else { + } else if (cmp_type == FIELD_STR) { + const int a_detached = a->kind & FILTER_REFS_DETACHED_HEAD; + + /* + * When sorting by name, we should put "detached" head lines, + * which are all the lines in parenthesis, before all others. + * This usually is automatic, since "(" is before "refs/" and + * "remotes/", but this does not hold for zh_CN, which uses + * full-width parenthesis, so make the ordering explicit. + */ + if (a_detached != (b->kind & FILTER_REFS_DETACHED_HEAD)) + cmp = a_detached ? -1 : 1; + else + cmp = cmp_fn(va->s, vb->s); + } else { if (va->value < vb->value) cmp = -1; else if (va->value == vb->value) cmp = cmp_fn(a->refname, b->refname); else cmp = 1; } return (s->reverse) ? -cmp : cmp; } diff --git a/t/lib-gettext.sh b/t/lib-gettext.sh index 2139b427ca..1adf1d4c31 100644 --- a/t/lib-gettext.sh +++ b/t/lib-gettext.sh @@ -25,23 +25,29 @@ then p q }') # is_IS.ISO8859-1 on Solaris and FreeBSD, is_IS.iso88591 on Debian is_IS_iso_locale=$(locale -a 2>/dev/null | sed -n '/^is_IS\.[iI][sS][oO]8859-*1$/{ p q }') - # Export them as an environment variable so the t0202/test.pl Perl - # test can use it too - export is_IS_locale is_IS_iso_locale + zh_CN_locale=$(locale -a 2>/dev/null | + sed -n '/^zh_CN\.[uU][tT][fF]-*8$/{ + p + q + }') + + # Export them as environment variables so other tests can use them + # too + export is_IS_locale is_IS_iso_locale zh_CN_locale if test -n "$is_IS_locale" && test $GIT_INTERNAL_GETTEXT_SH_SCHEME != "fallthrough" then # Some of the tests need the reference Icelandic locale test_set_prereq GETTEXT_LOCALE # Exporting for t0202/test.pl GETTEXT_LOCALE=1 export GETTEXT_LOCALE @@ -53,11 +59,21 @@ then if test -n "$is_IS_iso_locale" && test $GIT_INTERNAL_GETTEXT_SH_SCHEME != "fallthrough" then # Some of the tests need the reference Icelandic locale test_set_prereq GETTEXT_ISO_LOCALE say "# lib-gettext: Found '$is_IS_iso_locale' as an is_IS ISO-8859-1 locale" else say "# lib-gettext: No is_IS ISO-8859-1 locale available" fi + + if test -n "$zh_CN_locale" && + test $GIT_INTERNAL_GETTEXT_SH_SCHEME != "fallthrough" + then + test_set_prereq GETTEXT_ZH_LOCALE + + say "# lib-gettext: Found '$zh_CN_locale' as a zh_CN UTF-8 locale" + else + say "# lib-gettext: No zh_CN UTF-8 locale available" + fi fi diff --git a/t/t3207-branch-intl.sh b/t/t3207-branch-intl.sh new file mode 100755 index 0000000000..a46538188c --- /dev/null +++ b/t/t3207-branch-intl.sh @@ -0,0 +1,41 @@ +#!/bin/sh + +test_description='git branch internationalization tests' + +. ./lib-gettext.sh + +test_expect_success 'init repo' ' + git init r1 && + test_commit -C r1 first +' + +test_expect_success GETTEXT_ZH_LOCALE 'detached head sorts before branches' ' + # Ref sorting logic should put detached heads before the other + # branches, but this is not automatic when a branch name sorts + # lexically before "(" or the full-width "(" (Unicode codepoint FF08). + # The latter case is nearly guaranteed for the Chinese locale. + + test_when_finished "git -C r1 checkout master" && + + git -C r1 checkout HEAD^{} -- && + LC_ALL=$zh_CN_locale LC_MESSAGES=$zh_CN_locale \ + git -C r1 branch >actual && + + head -n 1 actual >first && + # The first line should be enclosed by full-width parenthesis. + grep "（.*）" first && + grep master actual +' + +test_expect_success 'detached head honors reverse sorting' ' + test_when_finished "git -C r1 checkout master" && + + git -C r1 checkout HEAD^{} -- && + git -C r1 branch --sort=-refname >actual && + + head -n 1 actual >first && + grep master first && + test_i18ngrep "HEAD detached" actual +' + +test_done
Before this patch, "git branch" would put "(HEAD detached...)" and "(no branch, rebasing...)" lines before all the other branches *in most cases* and only because of the fact that "(" is a low codepoint. This would not hold in the Chinese locale, which uses a full-width "(" symbol (codepoint FF08). This meant that the detached HEAD line would appear after all local refs and even after the remote refs if there were any. Deliberately sort the detached HEAD refs before other refs when sorting by refname rather than rely on codepoint subtleties. Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Helped-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Matthew DeVore <matvore@google.com> --- ref-filter.c | 20 ++++++++++++++++---- t/lib-gettext.sh | 22 +++++++++++++++++++--- t/t3207-branch-intl.sh | 41 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 7 deletions(-) create mode 100755 t/t3207-branch-intl.sh