RFC patchref-filter: sort detached HEAD lines firstly

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.

Signed-off-by: Matthew DeVore <matvore@google.com>
---
 ref-filter.c           | 10 +++++++---
 t/lib-gettext.sh       | 16 +++++++++++++---
 t/t3207-branch-intl.sh | 38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 6 deletions(-)
 create mode 100755 t/t3207-branch-intl.sh

Hi Matthew,

On Thu, 6 Jun 2019, Matthew DeVore wrote:

This description is pretty convincing!

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.

So in case that both are detached...

... we compare their commit hashes, is that right? Might be worth a code comment.

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;
 	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 wonder why this hunk, unlike the previous one, does not imitate the is_IS handling closely.

Why?

Why not simply `test_commit foo`?

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.

Why call `checkout` after `branch`? That's unnecessary, we do not verify anything after that call.

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 &&

	tail -n 1 <actual >last &&
	grep should_be_after last

instead of the "awk-ward" code above.

Ciao, Johannes

Hi,

sorry for top-posting, but I just noticed the "firstlyxy" typo in the subject ;-)

Ciao, Dscho

On Sun, 9 Jun 2019, Johannes Schindelin wrote:

On Sun, Jun 09, 2019 at 10:17:19AM +0200, Johannes Schindelin wrote:

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.

It should not be possible to have two detached head lines, so adding a comment about that may be distracting.

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.

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);
	} ...

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.)

You mean why make a test repo?

Good idea, I'll use that.

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.

It's to get the repo into a neutral state in case an additional testcase is added in the future.

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.

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
'

Thank you for the thoughtful feedback.

Hi,

Matthew DeVore wrote:

Git seems to be inconsistent about this. Documentation/CodingGuidelines says

        - When there are multiple arms to a conditional and some of them
          require braces, enclose even a single line block in braces for
          consistency. E.g.:

so you have some cover from there (and it matches what I'm used to, too). :)

[...]

For this kind of thing, we tend to use test_when_finished so that the test ends up in a clean state even if it fails.

[...]

nit: older shells do not know how to do $'\x01' interpolation. Probably best to use the raw UTF-8 directly here (it will be more readable anyway).

Thanks, Jonathan

On Mon, Jun 10, 2019 at 05:41:06PM -0700, Jonathan Nieder wrote:

Thanks for finding that.

Done. That will be used in the next roll-up.

Good point. I suppose we don't have to worry about dev's editors screwing up encoding since modern editors make this kind of thing easy to configure (and I suspect that all sane editors use UTF-8 as the default or at least won't mangle it...)

Jonathan Nieder <jrnieder@gmail.com> writes:

Yup, it took us for quite some time before we settled on that rule and wrote it down, so there are some lines that predate it *and* have survived.

This version improves upon the previous:

 - fixes a bug where reverse-sorting didn't work with (HEAD detached) lines, and
   adds a test
 - makes implementation code a little DRYer
 - uses test_when_finished where applicable
 - makes zh_CN locale detection look more like the is_IS locale detection

Thanks,

Matthew DeVore (1):
  ref-filter: sort detached HEAD lines firstly

 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

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

Matthew DeVore <matvore@google.com> writes:

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 ;-).

So, comparing a detached and an undetached ones, the detached side always sorts lower. Good. And ...

... otherwise we compare the string using the given function.

Sounds sensible.  Will queue.

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

 ref-filter.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

On Wed, Jun 12, 2019 at 02:09:53PM -0700, Junio C Hamano wrote:

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 02:09:53PM -0700, Junio C Hamano wrote:

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）

Hi Matthew,

On Tue, 11 Jun 2019, Matthew DeVore wrote:

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.

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.

`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

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:

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?

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

On Wed, Jun 12, 2019 at 09:51:33PM +0200, Johannes Schindelin wrote:

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

In the interest of simplifying the code and the job of translators, I've decided to simply remove the ( from the translatable strings as suggested by Junio in a response to v2 of this patchset.

I thought that a regression test would be a bit overkill for a fix of this nature. Instead, I've added a cautionary comment to not add the ( back to the translatable string.

Thank you,

Matthew DeVore (1):
  ref-filter: sort detached HEAD lines firstly

 ref-filter.c | 32 ++++++++++++++++----------------
 wt-status.c  |  4 ++--
 wt-status.h  |  3 +++
 3 files changed, 21 insertions(+), 18 deletions(-)

Before this patch, "git branch" would put "(HEAD detached...)" and "(no branch, rebasing...)" lines before all the other branches *in most cases* except for when using Chinese-language messages. zh_CN generally uses a full-width "(" symbol (codepoint FF08) to match the full-width proportions of Chinese characters, and the translated strings we had did use them. This meant that the detached HEAD line would appear after all local refs and even after the remote refs if there were any.

AFAIK, it is sometimes not jarring to see the half-width parenthesis in "full-width" text as in the CJK languages, for instance when there are no characters preceding or following the parenthesized text fragment. By removing the parenthesis from the localizable text, we can share strings with wt-status.c and remove a cautionary comment to translators.

Remove the ( from the localizable portion of messages so the sorting happens properly regardless of locale.

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 | 32 ++++++++++++++++----------------
 wt-status.c  |  4 ++--
 wt-status.h  |  3 +++
 3 files changed, 21 insertions(+), 18 deletions(-)

Matthew DeVore <matvore@google.com> writes:

...

Very nice ;-)

These too.

This started out as a reading of ref-filter.c where I wondered why we needed this i18n lego. I'm not sure whether in Chinese what Matthew DeVore said in [1] is true, but in any case it seems better to leave that to the translators, re-using the string is a relatively small gain.

So I think that change was really just "ASCII sort is easier than checking a flag in a sort callback", fair enough. But I thought I'd try to see how hard that patch would be. Turned out it's rather easy & I think results in better code, 4/5 gets us to that point.

But 5/5 I think makes this more generally interesting. In all locales (including LC_ALL=C) we list the "HEAD detached" entry last in "git branch -l" output if you're doing a reverse sort. I don't think this makes any sense, it's a notice, not a refname to be sorted. Using the new sorting function for treating detached HEAD specially makes this trivial to fix.

1. https://lore.kernel.org/git/9bd85516f91c3e2fdefdafd51df71f75603e51f6.1560895672.git.matvore@google.com/

Ævar Arnfjörð Bjarmason (5):
  branch: change "--local" to "--list" in comment
  branch tests: add to --sort tests
  ref-filter: add a "detached_head_first" sorting option
  branch: use the "detached_head_first" sorting option
  branch: show "HEAD detached" first under reverse sort

 builtin/branch.c         |  3 ++-
 ref-filter.c             | 54 +++++++++++++++++++++++++---------------
 ref-filter.h             |  3 +++
 t/t3203-branch-output.sh | 51 ++++++++++++++++++++++++++++++++++++-
 wt-status.c              |  4 +--
 wt-status.h              |  2 --
 6 files changed, 91 insertions(+), 26 deletions(-)

There has never been a "git branch --local", this is just a typo for "--list". Fixes a comment added in 23e714df91c (branch: roll show_detached HEAD into regular ref_list, 2015-09-23).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/branch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Addresses a comment Junio had on 3/5 in v1. Now there's leading patches moving the ref_sorting flags to a bitfield, which (agreeing with Junio) I think looks a bit better & should be more maintainable going forward.

While I was at it I squashed the 3/5 and 4/5 patches into one. It's less confusing to look at when we add the ref-filter.c sorting code in the same commit we're using it in. I also made the bitfield/xor checking/sanity BUG a bit less verbose & simpler to understand.

Ævar Arnfjörð Bjarmason (7):
  branch: change "--local" to "--list" in comment
  branch tests: add to --sort tests
  ref-filter: add braces to if/else if/else chain
  ref-filter: move "cmp_fn" assignment into "else if" arm
  ref-filter: move ref_sorting flags to a bitfield
  branch: sort detached HEAD based on a flag
  branch: show "HEAD detached" first under reverse sort

 builtin/branch.c         |  6 ++--
 builtin/for-each-ref.c   |  2 +-
 builtin/tag.c            |  2 +-
 ref-filter.c             | 75 ++++++++++++++++++++++++----------------
 ref-filter.h             | 13 ++++---
 t/t3203-branch-output.sh | 51 ++++++++++++++++++++++++++-
 wt-status.c              |  4 +--
 wt-status.h              |  2 --
 8 files changed, 111 insertions(+), 44 deletions(-)

Range-diff:
1:  c74e75dea90 = 1:  c74e75dea90 branch: change "--local" to "--list" in comment
2:  1fea125c7a6 = 2:  1fea125c7a6 branch tests: add to --sort tests
3:  11e6f274d2d < -:  ----------- ref-filter: add a "detached_head_first" sorting option
-:  ----------- > 3:  5cb44f0be40 ref-filter: add braces to if/else if/else chain
-:  ----------- > 4:  3e26cebe545 ref-filter: move "cmp_fn" assignment into "else if" arm
-:  ----------- > 5:  ad598fdc87c ref-filter: move ref_sorting flags to a bitfield
4:  faf9e23a13f ! 6:  af0c884b506 branch: use the "detached_head_first" sorting option
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    branch: use the "detached_head_first" sorting option
    +    branch: sort detached HEAD based on a flag
     
    -    Use the new ref_sorting_detached_head_first_all() sorting option in
    -    ref-filter.c to revert and amend 28438e84e04 (ref-filter: sort
    -    detached HEAD lines firstly, 2019-06-18).
    +    Change the ref-filter sorting of detached HEAD to check the
    +    FILTER_REFS_DETACHED_HEAD flag, instead of relying on the ref
    +    description filled-in by get_head_description() to start with "(",
    +    which in turn we expect to ASCII-sort before any other reference.
     
    -    In Chinese the fullwidth versions of punctuation like "()" are
    -    typically written as (U+FF08 fullwidth left parenthesis), (U+FF09
    -    fullwidth right parenthesis) instead. This form is used in both
    -    po/zh_{CN,TW}.po in most cases where "()" is translated in a string.
    -
    -    In 28438e84e04 the ability to translate this as part of the "git
    -    branch -l" output was removed because we'd like the detached line to
    -    appear first at the start of "git branch -l", e.g.:
    +    For context, we'd like the detached line to appear first at the start
    +    of "git branch -l", e.g.:
     
             $ git branch -l
             * (HEAD detached at <hash>)
               master
     
    -    Let's instead use the new ref_sorting_detached_head_first_all() in
    -    branch.c to say that we'd like these sorted before other entries.
    +    This doesn't change that, but improves on a fix made in
    +    28438e84e04 (ref-filter: sort detached HEAD lines firstly, 2019-06-18)
    +    and gives the Chinese translation the ability to use its preferred
    +    punctuation marks again.
    +
    +    In Chinese the fullwidth versions of punctuation like "()" are
    +    typically written as (U+FF08 fullwidth left parenthesis), (U+FF09
    +    fullwidth right parenthesis) instead[1]. This form is used in both
    +    po/zh_{CN,TW}.po in most cases where "()" is translated in a string.
    +
    +    Aside from that improvement to the Chinese translation, it also just
    +    makes for cleaner code that we mark any special cases in the ref_array
    +    we're sorting with flags and make the sort function aware of them,
    +    instead of piggy-backing on the general-case of strcmp() doing the
    +    right thing.
     
         As seen in the amended tests this made reverse sorting a bit more
         consistent. Before this we'd sometimes sort this message in the
    -    middle, now it's consistently at the beginning or end. Having it at
    -    the end doesn't make much sense either, but at least it behaves
    -    consistently now. A follow-up commit will make this behavior even
    -    better.
    +    middle, now it's consistently at the beginning or end, depending on
    +    whether we're doing a normal or reverse sort. Having it at the end
    +    doesn't make much sense either, but at least it behaves consistently
    +    now. A follow-up commit will make this behavior under reverse sorting
    +    even better.
     
         I'm removing the "TRANSLATORS" comments that were in the old code
         while I'm at it. Those were added in d4919bb288e (ref-filter: move
    @@ builtin/branch.c
     @@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix)
      		if (!sorting)
      			sorting = ref_default_sorting();
    - 		ref_sorting_icase_all(sorting, icase);
    -+		ref_sorting_detached_head_first_all(sorting, 1);
    + 		ref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);
    ++		ref_sorting_set_sort_flags_all(
    ++			sorting, REF_SORTING_DETACHED_HEAD_FIRST, 1);
      		print_ref_list(&filter, sorting, &format);
      		print_columns(&output, colopts, NULL);
      		string_list_clear(&output, 0);
    @@ ref-filter.c: char *get_head_description(void)
      	return strbuf_detach(&desc, NULL);
      }
      
    +@@ ref-filter.c: int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
    + 	return ret;
    + }
    + 
    ++static int compare_detached_head(struct ref_array_item *a, struct ref_array_item *b)
    ++{
    ++	if (!(a->kind ^ b->kind))
    ++		BUG("ref_kind_from_refname() should only mark one ref as HEAD");
    ++	if (a->kind & FILTER_REFS_DETACHED_HEAD)
    ++		return -1;
    ++	else if (b->kind & FILTER_REFS_DETACHED_HEAD)
    ++		return 1;
    ++	BUG("should have died in the xor check above");
    ++	return 0;
    ++}
    ++
    + static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, struct ref_array_item *b)
    + {
    + 	struct atom_value *va, *vb;
    +@@ ref-filter.c: static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
    + 	if (get_ref_atom_value(b, s->atom, &vb, &err))
    + 		die("%s", err.buf);
    + 	strbuf_release(&err);
    +-	if (s->sort_flags & REF_SORTING_VERSION) {
    ++	if (s->sort_flags & REF_SORTING_DETACHED_HEAD_FIRST &&
    ++	    ((a->kind | b->kind) & FILTER_REFS_DETACHED_HEAD)) {
    ++		cmp = compare_detached_head(a, b);
    ++	} else if (s->sort_flags & REF_SORTING_VERSION) {
    + 		cmp = versioncmp(va->s, vb->s);
    + 	} else if (cmp_type == FIELD_STR) {
    + 		int (*cmp_fn)(const char *, const char *);
    +
    + ## ref-filter.h ##
    +@@ ref-filter.h: struct ref_sorting {
    + 		REF_SORTING_REVERSE = 1<<0,
    + 		REF_SORTING_ICASE = 1<<1,
    + 		REF_SORTING_VERSION = 1<<2,
    ++		REF_SORTING_DETACHED_HEAD_FIRST = 1<<3,
    + 	} sort_flags;
    + };
    + 
     
      ## t/t3203-branch-output.sh ##
     @@ t/t3203-branch-output.sh: test_expect_success 'git branch `--sort=[-]objectsize` option' '
5:  b14a7b32cbf ! 7:  2497fffebf6 branch: show "HEAD detached" first under reverse sort
    @@ ref-filter.c: static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array
      	int cmp;
     +	int cmp_detached_head = 0;
      	cmp_type cmp_type = used_atom[s->atom].type;
    - 	int (*cmp_fn)(const char *, const char *);
      	struct strbuf err = STRBUF_INIT;
    + 
     @@ ref-filter.c: static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
    - 	     ^
    - 	     (b->kind & FILTER_REFS_DETACHED_HEAD))) {
    + 	if (s->sort_flags & REF_SORTING_DETACHED_HEAD_FIRST &&
    + 	    ((a->kind | b->kind) & FILTER_REFS_DETACHED_HEAD)) {
      		cmp = compare_detached_head(a, b);
     +		cmp_detached_head = 1;
    - 	} else if (s->version)
    + 	} else if (s->sort_flags & REF_SORTING_VERSION) {
      		cmp = versioncmp(va->s, vb->s);
    - 	else if (cmp_type == FIELD_STR)
    + 	} else if (cmp_type == FIELD_STR) {
     @@ ref-filter.c: static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
      			cmp = 1;
      	}
      
    --	return (s->reverse) ? -cmp : cmp;
    -+	return (s->reverse && !cmp_detached_head) ? -cmp : cmp;
    +-	return (s->sort_flags & REF_SORTING_REVERSE) ? -cmp : cmp;
    ++	return (s->sort_flags & REF_SORTING_REVERSE && !cmp_detached_head)
    ++		? -cmp : cmp;
      }
      
      static int compare_refs(const void *a_, const void *b_, void *ref_sorting)

Further amend code changed in 7c5045fc180 (ref-filter: apply fallback refname sort only after all user sorts, 2020-05-03) to move an assignment only used in the "else if" arm to happen there. Before that commit the cmp_fn would be used outside of it.

We could also just skip the "cmp_fn" assignment and use strcasecmp/strcmp directly in a ternary statement here, but this is probably more readable.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 ref-filter.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Further stress the --sort callback in ref-filter.c. The implementation uses certain short-circuiting logic, let's make sure it behaves the same way on e.g. name & version sort. Improves a test added in aedcb7dc75e (branch.c: use 'ref-filter' APIs, 2015-09-23).

I don't think all of this output makes sense, but let's test for the behavior as-is, we can fix bugs in it in a later commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t3203-branch-output.sh | 51 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

Per the CodingGuidelines add braces to an if/else if/else chain where only the "else" had braces. This is in preparation for a subsequent change where the "else if" will have lines added to it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 ref-filter.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Change the ref-filter sorting of detached HEAD to check the FILTER_REFS_DETACHED_HEAD flag, instead of relying on the ref description filled-in by get_head_description() to start with "(", which in turn we expect to ASCII-sort before any other reference.

For context, we'd like the detached line to appear first at the start of "git branch -l", e.g.:

    $ git branch -l
    * (HEAD detached at <hash>)
      master

This doesn't change that, but improves on a fix made in 28438e84e04 (ref-filter: sort detached HEAD lines firstly, 2019-06-18) and gives the Chinese translation the ability to use its preferred punctuation marks again.

In Chinese the fullwidth versions of punctuation like "()" are typically written as (U+FF08 fullwidth left parenthesis), (U+FF09 fullwidth right parenthesis) instead[1]. This form is used in both po/zh_{CN,TW}.po in most cases where "()" is translated in a string.

Aside from that improvement to the Chinese translation, it also just makes for cleaner code that we mark any special cases in the ref_array we're sorting with flags and make the sort function aware of them, instead of piggy-backing on the general-case of strcmp() doing the right thing.

As seen in the amended tests this made reverse sorting a bit more consistent. Before this we'd sometimes sort this message in the middle, now it's consistently at the beginning or end, depending on whether we're doing a normal or reverse sort. Having it at the end doesn't make much sense either, but at least it behaves consistently now. A follow-up commit will make this behavior under reverse sorting even better.

I'm removing the "TRANSLATORS" comments that were in the old code while I'm at it. Those were added in d4919bb288e (ref-filter: move get_head_description() from branch.c, 2017-01-10). I think it's obvious from context, string and translation memory in typical translation tools that these are the same or similar string.

1. https://en.wikipedia.org/wiki/Chinese_punctuation#Marks_similar_to_European_punctuation

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/branch.c         |  2 ++
 ref-filter.c             | 44 +++++++++++++++++++++++-----------------
 ref-filter.h             |  1 +
 t/t3203-branch-output.sh |  4 ++--
 wt-status.c              |  4 ++--
 wt-status.h              |  2 --
 6 files changed, 32 insertions(+), 25 deletions(-)

Change the output of the likes of "git branch -l --sort=-objectsize" to show the "(HEAD detached at <hash>)" message at the start of the output. Before the compare_detached_head() function added in a preceding commit we'd emit this output as an emergent effect.

It doesn't make any sense to consider the objectsize, type or other non-attribute of the "(HEAD detached at <hash>)" message for the purposes of sorting. Let's always emit it at the top instead. The only reason it was sorted in the first place is because we're injecting it into the ref-filter machinery so builtin/branch.c doesn't need to do its own "am I detached?" detection.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 ref-filter.c             | 5 ++++-
 t/t3203-branch-output.sh | 6 +++---
 2 files changed, 7 insertions(+), 4 deletions(-)

Change the reverse/ignore_case/version sort flags in the ref_sorting struct into a bitfield. Having three of them was already a bit unwieldy, but it would be even more so if another flag needed a function like ref_sorting_icase_all() introduced in 76f9e569adb (ref-filter: apply --ignore-case to all sorting keys, 2020-05-03).

A follow-up change will introduce such a flag, so let's move this over to a bitfield. Instead of using the usual '#define' pattern I'm using the "enum" pattern from builtin/rebase.c's b4c8eb024af (builtin rebase: support --quiet, 2018-09-04).

Perhaps there's a more idiomatic way of doing the "for each in list amend mask" pattern than this "mask/on" variable combo. This function doesn't allow us to e.g. do any arbitrary changes to the bitfield for multiple flags, but I think in this case that's fine. The common case is that we're calling this with a list of one.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/branch.c       |  2 +-
 builtin/for-each-ref.c |  2 +-
 builtin/tag.c          |  2 +-
 ref-filter.c           | 24 +++++++++++++++---------
 ref-filter.h           | 12 +++++++-----
 5 files changed, 25 insertions(+), 17 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

An obvious alternative would be to pass two masks, one for setting and the other for clearing, instead of passing a mask and a bool that says if the mask is for setting or clearing.

The helper that follows such a design would be:

	void ref_sorting_tweak_flags(struct ref_sorting *sorting,
				     unsigned set, unsigned clear)
	{
		while (sorting) {
			sorting->sort_flags |= set;
			sorting->sort_flags &= ~clear;
			sorting = sorting->next;
		}
	}

and the caller in the endgame would become

	...
	} else if (list) {
		unsigned set = REF_SORTING_DETACHED_HEAD_FIRST;
		unsigned clear = 0;

                *(icase ? &set : &clear) |= REF_SORTING_ICASE;
		ref_sorting_tweak_flags(sorting, set, clear);

which may be more lines but probably copes better when adding new bits.

Thanks.

There has never been a "git branch --local", this is just a typo for "--list". Fixes a comment added in 23e714df91c (branch: roll show_detached HEAD into regular ref_list, 2015-09-23).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/branch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Further stress the --sort callback in ref-filter.c. The implementation uses certain short-circuiting logic, let's make sure it behaves the same way on e.g. name & version sort. Improves a test added in aedcb7dc75e (branch.c: use 'ref-filter' APIs, 2015-09-23).

I don't think all of this output makes sense, but let's test for the behavior as-is, we can fix bugs in it in a later commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t3203-branch-output.sh | 51 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

OK.

I wondered if 'type' and '-type' tests and 'version:refname' and '-version:refname' tests, should be separate, so that the latter half of the latter pair can expect to have HEAD at the beginning with test_expect_failure until it gets fixed. But "document the status quo, and then change the behaviour and demonstrate how the new behaviour is superiour with the change in the expectation in the patch" is a reasonable approach, too.

Will queue; thanks.

Use the new ref_sorting_detached_head_first_all() sorting option in ref-filter.c to revert and amend 28438e84e04 (ref-filter: sort detached HEAD lines firstly, 2019-06-18).

In Chinese the fullwidth versions of punctuation like "()" are typically written as (U+FF08 fullwidth left parenthesis), (U+FF09 fullwidth right parenthesis) instead. This form is used in both po/zh_{CN,TW}.po in most cases where "()" is translated in a string.

In 28438e84e04 the ability to translate this as part of the "git branch -l" output was removed because we'd like the detached line to appear first at the start of "git branch -l", e.g.:

    $ git branch -l
    * (HEAD detached at <hash>)
      master

Let's instead use the new ref_sorting_detached_head_first_all() in branch.c to say that we'd like these sorted before other entries.

As seen in the amended tests this made reverse sorting a bit more consistent. Before this we'd sometimes sort this message in the middle, now it's consistently at the beginning or end. Having it at the end doesn't make much sense either, but at least it behaves consistently now. A follow-up commit will make this behavior even better.

I'm removing the "TRANSLATORS" comments that were in the old code while I'm at it. Those were added in d4919bb288e (ref-filter: move get_head_description() from branch.c, 2017-01-10). I think it's obvious from context, string and translation memory in typical translation tools that these are the same or similar string.

1. https://en.wikipedia.org/wiki/Chinese_punctuation#Marks_similar_to_European_punctuation

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/branch.c         |  1 +
 ref-filter.c             | 27 +++++++++------------------
 t/t3203-branch-output.sh |  4 ++--
 wt-status.c              |  4 ++--
 wt-status.h              |  2 --
 5 files changed, 14 insertions(+), 24 deletions(-)

Add a "detached_head_first" sorting option for eventual use by the "git branch" command. When listing branches we want to list the detached HEAD "ref" at the start of the list. As shown in 28438e84e04 (ref-filter: sort detached HEAD lines firstly, 2019-06-18) this currently relies on "(" sorting before any other refname by strcmp().

This boxes translators into using ASCII parentheses, a subsequent commit will amend get_head_description() to get rid of this limitation.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 ref-filter.c | 23 ++++++++++++++++++++++-
 ref-filter.h |  3 +++
 2 files changed, 25 insertions(+), 1 deletion(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

OK.

This, taken together with existing ref_sorting_icase_all(), looks somewhat ugly, especially when you ponder how you would add a third similar option to the mix.

Perhaps "ignore_case" and "detached_head_first" shouldn't be separate bitfields, but bits in the same flag word member in the "struct ref_sorting", and "set/unset these flags to all the sort ops" helper function should just take a flags word that has two bits?

Or maybe it is good enough for now. I hesitate to say so myself, though, after already saying it is "somewhat ugly" ;-)

Change the output of the likes of "git branch -l --sort=-objectsize" to show the "(HEAD detached at <hash>)" message at the start of the output. Before the compare_detached_head() function added in a preceding commit we'd emit this output as an emergent effect.

It doesn't make any sense to consider the objectsize, type or other non-attribute of the "(HEAD detached at <hash>)" message for the purposes of sorting. Let's always emit it at the top instead. The only reason it was sorted in the first place is because we're injecting it into the ref-filter machinery so builtin/branch.c doesn't need to do its own "am I detached?" detection.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 ref-filter.c             | 4 +++-
 t/t3203-branch-output.sh | 6 +++---
 2 files changed, 6 insertions(+), 4 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

OK. Other criteria would honor the "reverse" bit, but when we work on the set that includes "HEAD" ref (which only happens when "branch -l" deals with a detached head), it always tries to sort it before all other refs, regardless of the reverse bit. Makes sense.