Re: [RFC PATCH] ref-filter: sort detached HEAD lines firstly
- From
Matthew DeVore <matvore@comcast.net>
- Date
- Jun 10, 2019, 23:49 UTC
- Message-ID
- <20190610234918.GA10396@comcast.net>
- In-Reply-To
- <nycvar.QRO.7.76.6.1906090954510.789@QRFXGBC-DHN364S.ybpnyqbznva>
On Sun, Jun 09, 2019 at 10:17:19AM +0200, Johannes Schindelin wrote:
Show 18 quoted lines
> > 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)
> > + } else if (cmp_type == FIELD_STR) {
>
> I find that it makes sense in general to suppress one's urges regarding
> introducing `{ ... }` around one-liners when the patch does not actually
> require it.
>
> For example, I found this patch harder than necessary to read because of
> it.I understand the desire to make the patch itself clean, and I sometimes try to do that to a fault, but the style as I understand it is to put { } around all if branches if only one branch requires it. Since I'm already modifying the "else if (cmp_type == FIELD_STR)" line, I decided to put the } at the start of the line and modify the if (s->version) line as well. So only one line was modified "in excess." I think the temporary cost of the verbose patch is justified to keep the style consistent in narrow code fragments.
Show 13 quoted lines
>
> > + if ((a->kind & FILTER_REFS_DETACHED_HEAD) !=
> > + (b->kind & FILTER_REFS_DETACHED_HEAD)) {
>
> So in case that both are detached...
>
> > + return (a->kind & FILTER_REFS_DETACHED_HEAD) ? -1 : 1;
> > + }
> > cmp = cmp_fn(va->s, vb->s);
>
> ... we compare their commit hashes, is that right? Might be worth a code
> comment.
> It should not be possible to have two detached head lines, so adding a comment about that may be distracting.
Show 11 quoted lines
> > - else {
> > + } else {
>
> FWIW it would have been a much more obvious patch if it had done
>
> if (s->version)
> [...]
> + else if (cmp_type == FIELD_STR &&
> + (a->kind & FILTER_REFS_DETACHED_HEAD ||
> + b->kind & FILTER_REFS_DETACHED_HEAD))
> + return (a->kind & FILTER_REFS_DETACHED_HEAD) ? -1 : 1;But that means that if a is detached, it is always a < b, even if both are detached? That's probably right in practice, since there should only be one detached head, but it's jarring to read a brittle cmp function.
Show 12 quoted lines
> else if (cmp_type == FIELD_STR) > [...] > > Maybe still worth doing. > > FWIW I was *so* tempted to write > > ((a->kind ^ b->kind) & FILTER_REFS_DETACHED_HEAD) > > to make this code DRYer, but then, readers not intimately familiar with > Boolean arithmetic might not even know about the `^` operator, making the > code harder to read than necessary, too.
I think I found a readable way to DRY:
} 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);
} ...Show 28 quoted lines
> > > > 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,15 @@ 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 -z "$zh_CN_locale"; then > > + say "# lib-gettext: No zh_CN UTF-8 locale available" > > + fi > > I wonder why this hunk, unlike the previous one, does not imitate the > is_IS handling closely.
It was because I didn't have gettext set up properly and it was causing GIT_INTERNAL_GETTEXT_SH_SCHEME to be "fallthrough", despite the actual Git output being translated. My set-up was quite broken so I don't think our intl utility and test code needs any fixing. I've handled this and the next version of the patch will have that fixed. (incidentally, this was why I originally marked my patch RFC. Also incidentally, I'm using a Mac for development since the most powerful machine I have access to is a Mac, so I've been jumping through some hoops to make that work.)
Show 17 quoted lines
> > > diff --git a/t/t3207-branch-intl.sh b/t/t3207-branch-intl.sh > > new file mode 100755 > > index 0000000000..9f6fcc7481 > > --- /dev/null > > +++ b/t/t3207-branch-intl.sh > > @@ -0,0 +1,38 @@ > > +#!/bin/sh > > + > > +test_description='git branch internationalization tests' > > + > > +. ./lib-gettext.sh > > + > > +test_expect_success 'init repo' ' > > + git init r1 && > > Why?
You mean why make a test repo?
Show 7 quoted lines
> > > + touch r1/foo && > > + git -C r1 add foo && > > + git -C r1 commit -m foo > > +' > > Why not simply `test_commit foo`?
Good idea, I'll use that.
Show 12 quoted lines
>
> > +test_expect_success 'detached head sorts before other 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.
> > +
> > + git -C r1 checkout HEAD^{} -- &&
> > + git -C r1 branch !should_be_after_detached HEAD &&
>
> I am not sure that `!` is a wise choice, as it might not be a legal file
> name character everywhere. A `.` or `-` might make more sense.The ! is actually meaningless, as I have come to observed, since the actual ref name used for comparison is not "!should_be_after_detached" but "refs/heads/!should_be_after_detached". So I'll remove it.
Show 7 quoted lines
> > > + LC_ALL=$zh_CN_locale LC_MESSAGES=$zh_CN_locale \ > > + git -C r1 branch >actual && > > + git -C r1 checkout - && > > Why call `checkout` after `branch`? That's unnecessary, we do not verify > anything after that call.
It's to get the repo into a neutral state in case an additional testcase is added in the future.
Show 46 quoted lines
>
> > + awk "
> > + # We need full-width or half-width parens on the first line.
> > + NR == 1 && (/[(].*[)]/ || /\xef\xbc\x88.*\xef\xbc\x89/) {
> > + found_head = 1;
> > + }
> > + /!should_be_after_detached/ {
> > + found_control_branch = 1;
> > + }
> > + END { exit !found_head || !found_control_branch }
> > + " actual
>
> This might look beautiful for a fan of `awk`. For the vast majority of us,
> this is not a good idea.
>
> Remember, you do *not* write those tests for your own pleasure, you do
> *not* write those tests in order to help you catch problems while you
> develop your patches, you do *not* develop these tests in order to just
> catch future breakages.
>
> You *do* write those tests for *other* developers who you try to help in
> preventing introducing regressions.
>
> As such, you *want* the tests to be
>
> - easy to understand for as wide a range of developers as you can make,
>
> - quick,
>
> - covering regressions, and *only* regressions,
>
> - helping diagnose *and* fix regressions.
>
> In the ideal case you won't even hear when developers found your test
> helpful, and you will never, ever learn about regressions that have been
> prevented.
>
> You most frequently will hear about your tests when they did not do their
> job well.
>
> In this instance, I would have expected something like
>
> test_expect_lines = 3 actual &&
>
> head -n 1 <actual >first &&
> test_i18ngrep "detached HEAD" first &&Tangential to your point, but the test_i18ngrep actually won't work since all it does is *not* grep when the text is translated. But I want the text to use the genuine zh_CN translation.
Show 5 quoted lines
> > tail -n 1 <actual >last && > grep should_be_after last > > instead of the "awk-ward" code above.
All fair points, and I may have been carried away when I wrote the awk code. I liked using awk since it matched my mental model at the time, which was procedural, as opposed to your proposed pure sh implementation, which is more declarative.
Here is the new version of the test:
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. git -C r1 checkout HEAD^{} -- &&
LC_ALL=$zh_CN_locale LC_MESSAGES=$zh_CN_locale \
git -C r1 branch >actual &&
git -C r1 checkout - &&head -n 1 actual >first && # The first line should be enclosed by full-width parenthesis. grep '$'\xef\xbc\x88.*\xef\xbc\x89'' first && grep master actual '
> > Ciao, > Johannes >
Thank you for the thoughtful feedback.