[RFC PATCH] ref-filter: sort detached HEAD lines firstly

[RFC PATCH] ref-filter: sort detached HEAD lines firstly

[Matthew DeVore]

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

diff --git a/ref-filter.c b/ref-filter.c
index 8500671bc6..cbfae790f9 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2157,25 +2157,29 @@ 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)
+	} else if (cmp_type == FIELD_STR) {
+		if ((a->kind & FILTER_REFS_DETACHED_HEAD) !=
+				(b->kind & FILTER_REFS_DETACHED_HEAD)) {
+			return (a->kind & FILTER_REFS_DETACHED_HEAD) ? -1 : 1;
+		}
 		cmp = cmp_fn(va->s, vb->s);
-	else {
+	} 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..de08d109dc 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,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
 fi
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 &&
+	touch r1/foo &&
+	git -C r1 add foo &&
+	git -C r1 commit -m foo
+'
+
+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 &&
+	LC_ALL=$zh_CN_locale LC_MESSAGES=$zh_CN_locale \
+		git -C r1 branch >actual &&
+	git -C r1 checkout - &&
+
+	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
+'
+
+test_done
-- 
2.21.0

Re: [RFC PATCH] ref-filter: sort detached HEAD lines firstlyxy

[Johannes Schindelin]

Hi Matthew,

On Thu, 6 Jun 2019, Matthew DeVore wrote:

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

This description is pretty convincing!

> diff --git a/ref-filter.c b/ref-filter.c
> index 8500671bc6..cbfae790f9 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -2157,25 +2157,29 @@ 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)
> +	} 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.

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

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

> diff --git a/t/lib-gettext.sh b/t/lib-gettext.sh
> index 2139b427ca..de08d109dc 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,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.

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

> +	touch r1/foo &&
> +	git -C r1 add foo &&
> +	git -C r1 commit -m foo
> +'

Why not simply `test_commit foo`?

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

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

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

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

instead of the "awk-ward" code above.

Ciao,
Johannes

> +'
> +
> +test_done
> --
> 2.21.0
>
>

Re: [RFC PATCH] ref-filter: sort detached HEAD lines firstlyxy

[Johannes Schindelin]

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:

> Hi Matthew,
>
> On Thu, 6 Jun 2019, Matthew DeVore wrote:
>
> > 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.
>
> This description is pretty convincing!
>
> > diff --git a/ref-filter.c b/ref-filter.c
> > index 8500671bc6..cbfae790f9 100644
> > --- a/ref-filter.c
> > +++ b/ref-filter.c
> > @@ -2157,25 +2157,29 @@ 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)
> > +	} 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.
>
> > +		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.
>
> > -	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;
>  	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.
>
> > diff --git a/t/lib-gettext.sh b/t/lib-gettext.sh
> > index 2139b427ca..de08d109dc 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,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.
>
> > 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?
>
> > +	touch r1/foo &&
> > +	git -C r1 add foo &&
> > +	git -C r1 commit -m foo
> > +'
>
> Why not simply `test_commit foo`?
>
> > +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.
>
> > +	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.
>
> > +	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 &&
>
> 	tail -n 1 <actual >last &&
> 	grep should_be_after last
>
> instead of the "awk-ward" code above.
>
> Ciao,
> Johannes
>
> > +'
> > +
> > +test_done
> > --
> > 2.21.0
> >
> >
>

Re: [RFC PATCH] ref-filter: sort detached HEAD lines firstly

[Matthew DeVore]

On Sun, Jun 09, 2019 at 10:17:19AM +0200, Johannes Schindelin wrote:
> >  	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.

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

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

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

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

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

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

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

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

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

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

Re: [RFC PATCH] ref-filter: sort detached HEAD lines firstly

[Jonathan Nieder]

Hi,

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

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

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

[...]
>>> +	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.

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.

[...]
> 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 &&

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

Re: [RFC PATCH] ref-filter: sort detached HEAD lines firstly

[Matthew DeVore]

On Mon, Jun 10, 2019 at 05:41:06PM -0700, Jonathan Nieder 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). :)

Thanks for finding that.

> 
> [...]
> >>> +	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.
> 
> 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.

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

> > 	head -n 1 actual >first &&
> > 	# The first line should be enclosed by full-width parenthesis.
> > 	grep '$'\xef\xbc\x88.*\xef\xbc\x89'' first &&
> 
> 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).

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

[PATCH v2 0/1] Sort detached HEAD lines firstly

[Matthew DeVore]

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

-- 
2.21.0

[PATCH v2 1/1] ref-filter: sort detached HEAD lines firstly

[Matthew DeVore]

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

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

Re: [RFC PATCH] ref-filter: sort detached HEAD lines firstly

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

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.

Re: [PATCH v2 1/1] ref-filter: sort detached HEAD lines firstly

[Junio C Hamano]

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.

Re: [PATCH v2 1/1] ref-filter: sort detached HEAD lines firstly

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 8112 bytes --]

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

Re: [PATCH v2 1/1] ref-filter: sort detached HEAD lines firstly

[Junio C Hamano]

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

Re: [PATCH v2 1/1] ref-filter: sort detached HEAD lines firstly

[Matthew DeVore]

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

Re: [PATCH v2 1/1] ref-filter: sort detached HEAD lines firstly

[Matthew DeVore]

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.

Re: [PATCH v2 1/1] ref-filter: sort detached HEAD lines firstly

[Jeff King]

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

[PATCH v3 0/1] Sort detached heads line firstly

[Matthew DeVore]

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

-- 
2.21.0

[PATCH v3 1/1] ref-filter: sort detached HEAD lines firstly

[Matthew DeVore]

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

diff --git a/ref-filter.c b/ref-filter.c
index 8500671bc6..87aa6b4774 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1440,49 +1440,49 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
 	} else
 		BUG("unhandled RR_* enum");
 }
 
 char *get_head_description(void)
 {
 	struct strbuf desc = STRBUF_INIT;
 	struct wt_status_state state;
 	memset(&state, 0, sizeof(state));
 	wt_status_get_state(the_repository, &state, 1);
+
+	/*
+	 * The ( character must be hard-coded and not part of a localizable
+	 * string, since the description is used as a sort key and compared
+	 * with ref names.
+	 */
+	strbuf_addch(&desc, '(');
 	if (state.rebase_in_progress ||
 	    state.rebase_interactive_in_progress) {
 		if (state.branch)
-			strbuf_addf(&desc, _("(no branch, rebasing %s)"),
+			strbuf_addf(&desc, _("no branch, rebasing %s"),
 				    state.branch);
 		else
-			strbuf_addf(&desc, _("(no branch, rebasing detached HEAD %s)"),
+			strbuf_addf(&desc, _("no branch, rebasing detached HEAD %s"),
 				    state.detached_from);
 	} else if (state.bisect_in_progress)
-		strbuf_addf(&desc, _("(no branch, bisect started on %s)"),
+		strbuf_addf(&desc, _("no branch, bisect started on %s"),
 			    state.branch);
 	else if (state.detached_from) {
 		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_addstr(&desc, HEAD_DETACHED_AT);
 		else
-			/*
-			 * TRANSLATORS: make sure this matches "HEAD
-			 * detached from " in wt-status.c
-			 */
-			strbuf_addf(&desc, _("(HEAD detached from %s)"),
-				state.detached_from);
+			strbuf_addstr(&desc, HEAD_DETACHED_FROM);
+		strbuf_addstr(&desc, state.detached_from);
 	}
 	else
-		strbuf_addstr(&desc, _("(no branch)"));
+		strbuf_addstr(&desc, _("no branch"));
+	strbuf_addch(&desc, ')');
+
 	free(state.branch);
 	free(state.onto);
 	free(state.detached_from);
 	return strbuf_detach(&desc, NULL);
 }
 
 static const char *get_symref(struct used_atom *atom, struct ref_array_item *ref)
 {
 	if (!ref->symref)
 		return xstrdup("");
diff --git a/wt-status.c b/wt-status.c
index 0bccef542f..c29e4bf091 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1669,23 +1669,23 @@ static void wt_longstatus_print(struct wt_status *s)
 			if (s->state.rebase_in_progress ||
 			    s->state.rebase_interactive_in_progress) {
 				if (s->state.rebase_interactive_in_progress)
 					on_what = _("interactive rebase in progress; onto ");
 				else
 					on_what = _("rebase in progress; onto ");
 				branch_name = s->state.onto;
 			} else if (s->state.detached_from) {
 				branch_name = s->state.detached_from;
 				if (s->state.detached_at)
-					on_what = _("HEAD detached at ");
+					on_what = HEAD_DETACHED_AT;
 				else
-					on_what = _("HEAD detached from ");
+					on_what = HEAD_DETACHED_FROM;
 			} else {
 				branch_name = "";
 				on_what = _("Not currently on any branch.");
 			}
 		} else
 			skip_prefix(branch_name, "refs/heads/", &branch_name);
 		status_printf(s, color(WT_STATUS_HEADER, s), "%s", "");
 		status_printf_more(s, branch_status_color, "%s", on_what);
 		status_printf_more(s, branch_color, "%s\n", branch_name);
 		if (!s->is_initial)
diff --git a/wt-status.h b/wt-status.h
index 64f1ddc9fd..b0cfdc8011 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -58,20 +58,23 @@ struct wt_status_change_data {
 enum wt_status_format {
 	STATUS_FORMAT_NONE = 0,
 	STATUS_FORMAT_LONG,
 	STATUS_FORMAT_SHORT,
 	STATUS_FORMAT_PORCELAIN,
 	STATUS_FORMAT_PORCELAIN_V2,
 
 	STATUS_FORMAT_UNSPECIFIED
 };
 
+#define HEAD_DETACHED_AT _("HEAD detached at ")
+#define HEAD_DETACHED_FROM _("HEAD detached from ")
+
 struct wt_status_state {
 	int merge_in_progress;
 	int am_in_progress;
 	int am_empty_patch;
 	int rebase_in_progress;
 	int rebase_interactive_in_progress;
 	int cherry_pick_in_progress;
 	int bisect_in_progress;
 	int revert_in_progress;
 	int detached_at;
-- 
2.21.0

Re: [PATCH v3 1/1] ref-filter: sort detached HEAD lines firstly

[Junio C Hamano]

Matthew DeVore <matvore@google.com> writes:

> ... By
> removing the parenthesis from the localizable text, we can share strings
> with wt-status.c and remove a cautionary comment to translators.
...
> -			/*
> -			 * TRANSLATORS: make sure this matches "HEAD
> -			 * detached at " in wt-status.c
> -			 */
> -			strbuf_addf(&desc, _("(HEAD detached at %s)"),
> -				state.detached_from);
> +			strbuf_addstr(&desc, HEAD_DETACHED_AT);
>  		else
> -			/*
> -			 * TRANSLATORS: make sure this matches "HEAD
> -			 * detached from " in wt-status.c
> -			 */
> -			strbuf_addf(&desc, _("(HEAD detached from %s)"),
> -				state.detached_from);
> +			strbuf_addstr(&desc, HEAD_DETACHED_FROM);

Very nice ;-)

> +		strbuf_addstr(&desc, state.detached_from);
>  	}
>  	else
> -		strbuf_addstr(&desc, _("(no branch)"));
> +		strbuf_addstr(&desc, _("no branch"));
> +	strbuf_addch(&desc, ')');
> +
>  	free(state.branch);
>  	free(state.onto);
>  	free(state.detached_from);
>  	return strbuf_detach(&desc, NULL);
>  }

> diff --git a/wt-status.h b/wt-status.h
> index 64f1ddc9fd..b0cfdc8011 100644
> --- a/wt-status.h
> +++ b/wt-status.h
> @@ -58,20 +58,23 @@ struct wt_status_change_data {
> ...
>  
> +#define HEAD_DETACHED_AT _("HEAD detached at ")
> +#define HEAD_DETACHED_FROM _("HEAD detached from ")
> +
>  struct wt_status_state {

These too.

[PATCH 0/5] branch: --sort improvements

[Ævar Arnfjörð Bjarmason]

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

-- 
2.29.2.222.g5d2a92d10f8

[PATCH 1/5] branch: change "--local" to "--list" in comment

[Ævar Arnfjörð Bjarmason]

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

diff --git a/builtin/branch.c b/builtin/branch.c
index 9b68591addf..045866a51ae 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -726,7 +726,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		print_current_branch_name();
 		return 0;
 	} else if (list) {
-		/*  git branch --local also shows HEAD when it is detached */
+		/*  git branch --list also shows HEAD when it is detached */
 		if ((filter.kind & FILTER_REFS_BRANCHES) && filter.detached)
 			filter.kind |= FILTER_REFS_DETACHED_HEAD;
 		filter.name_patterns = argv;
-- 
2.29.2.222.g5d2a92d10f8

[PATCH 2/5] branch tests: add to --sort tests

[Ævar Arnfjörð Bjarmason]

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

diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index b945faf4702..f92fb3aab9d 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -210,7 +210,7 @@ EOF
 	test_i18ncmp expect actual
 '
 
-test_expect_success 'git branch `--sort` option' '
+test_expect_success 'git branch `--sort=[-]objectsize` option' '
 	cat >expect <<-\EOF &&
 	* (HEAD detached from fromtag)
 	  branch-two
@@ -218,6 +218,55 @@ test_expect_success 'git branch `--sort` option' '
 	  main
 	EOF
 	git branch --sort=objectsize >actual &&
+	test_i18ncmp expect actual &&
+
+	cat >expect <<-\EOF &&
+	  branch-one
+	  main
+	* (HEAD detached from fromtag)
+	  branch-two
+	EOF
+	git branch --sort=-objectsize >actual &&
+	test_i18ncmp expect actual
+'
+
+test_expect_success 'git branch `--sort=[-]type` option' '
+	cat >expect <<-\EOF &&
+	* (HEAD detached from fromtag)
+	  branch-one
+	  branch-two
+	  main
+	EOF
+	git branch --sort=type >actual &&
+	test_i18ncmp expect actual &&
+
+	cat >expect <<-\EOF &&
+	* (HEAD detached from fromtag)
+	  branch-one
+	  branch-two
+	  main
+	EOF
+	git branch --sort=-type >actual &&
+	test_i18ncmp expect actual
+'
+
+test_expect_success 'git branch `--sort=[-]version:refname` option' '
+	cat >expect <<-\EOF &&
+	* (HEAD detached from fromtag)
+	  branch-one
+	  branch-two
+	  main
+	EOF
+	git branch --sort=version:refname >actual &&
+	test_i18ncmp expect actual &&
+
+	cat >expect <<-\EOF &&
+	  main
+	  branch-two
+	  branch-one
+	* (HEAD detached from fromtag)
+	EOF
+	git branch --sort=-version:refname >actual &&
 	test_i18ncmp expect actual
 '
 
-- 
2.29.2.222.g5d2a92d10f8

[PATCH 3/5] ref-filter: add a "detached_head_first" sorting option

[Ævar Arnfjörð Bjarmason]

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

diff --git a/ref-filter.c b/ref-filter.c
index aa260bfd099..94ab3f86a53 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2350,6 +2350,16 @@ 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 & FILTER_REFS_DETACHED_HEAD)
+		return -1;
+	else if (b->kind & FILTER_REFS_DETACHED_HEAD)
+		return 1;
+	BUG("compare_detached_head() is guarded by an xor on [ab]->kind & FILTER_REFS_DETACHED_HEAD");
+	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;
@@ -2364,7 +2374,12 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
 		die("%s", err.buf);
 	strbuf_release(&err);
 	cmp_fn = s->ignore_case ? strcasecmp : strcmp;
-	if (s->version)
+	if (s->detached_head_first &&
+	    ((a->kind & FILTER_REFS_DETACHED_HEAD)
+	     ^
+	     (b->kind & FILTER_REFS_DETACHED_HEAD))) {
+		cmp = compare_detached_head(a, b);
+	} else if (s->version)
 		cmp = versioncmp(va->s, vb->s);
 	else if (cmp_type == FIELD_STR)
 		cmp = cmp_fn(va->s, vb->s);
@@ -2403,6 +2418,12 @@ void ref_sorting_icase_all(struct ref_sorting *sorting, int flag)
 		sorting->ignore_case = !!flag;
 }
 
+void ref_sorting_detached_head_first_all(struct ref_sorting *sorting, int flag)
+{
+	for (; sorting; sorting = sorting->next)
+		sorting->detached_head_first = !!flag;
+}
+
 void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
 {
 	QSORT_S(array->items, array->nr, compare_refs, sorting);
diff --git a/ref-filter.h b/ref-filter.h
index feaef4a8fde..3b92e0f2696 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -30,6 +30,7 @@ struct ref_sorting {
 	int atom; /* index into used_atom array (internal) */
 	unsigned reverse : 1,
 		ignore_case : 1,
+		detached_head_first : 1,
 		version : 1;
 };
 
@@ -111,6 +112,8 @@ int verify_ref_format(struct ref_format *format);
 void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
 /*  Set the ignore_case flag for all elements of a sorting list */
 void ref_sorting_icase_all(struct ref_sorting *sorting, int flag);
+/*  Set the detached_head_first flag for all elements of a sorting list */
+void ref_sorting_detached_head_first_all(struct ref_sorting *sorting, int flag);
 /*  Based on the given format and quote_style, fill the strbuf */
 int format_ref_array_item(struct ref_array_item *info,
 			  const struct ref_format *format,
-- 
2.29.2.222.g5d2a92d10f8

[PATCH 4/5] branch: use the "detached_head_first" sorting option

[Ævar Arnfjörð Bjarmason]

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

diff --git a/builtin/branch.c b/builtin/branch.c
index 045866a51ae..92221bdf8a6 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -740,6 +740,7 @@ 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);
 		print_ref_list(&filter, sorting, &format);
 		print_columns(&output, colopts, NULL);
 		string_list_clear(&output, 0);
diff --git a/ref-filter.c b/ref-filter.c
index 94ab3f86a53..7e0289cb659 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1536,36 +1536,27 @@ char *get_head_description(void)
 	struct wt_status_state state;
 	memset(&state, 0, sizeof(state));
 	wt_status_get_state(the_repository, &state, 1);
-
-	/*
-	 * The ( character must be hard-coded and not part of a localizable
-	 * string, since the description is used as a sort key and compared
-	 * with ref names.
-	 */
-	strbuf_addch(&desc, '(');
 	if (state.rebase_in_progress ||
 	    state.rebase_interactive_in_progress) {
 		if (state.branch)
-			strbuf_addf(&desc, _("no branch, rebasing %s"),
+			strbuf_addf(&desc, _("(no branch, rebasing %s)"),
 				    state.branch);
 		else
-			strbuf_addf(&desc, _("no branch, rebasing detached HEAD %s"),
+			strbuf_addf(&desc, _("(no branch, rebasing detached HEAD %s)"),
 				    state.detached_from);
 	} else if (state.bisect_in_progress)
-		strbuf_addf(&desc, _("no branch, bisect started on %s"),
+		strbuf_addf(&desc, _("(no branch, bisect started on %s)"),
 			    state.branch);
 	else if (state.detached_from) {
 		if (state.detached_at)
-			strbuf_addstr(&desc, HEAD_DETACHED_AT);
+			strbuf_addf(&desc, _("(HEAD detached at %s)"),
+				state.detached_from);
 		else
-			strbuf_addstr(&desc, HEAD_DETACHED_FROM);
-		strbuf_addstr(&desc, state.detached_from);
-	}
-	else
-		strbuf_addstr(&desc, _("no branch"));
-	strbuf_addch(&desc, ')');
+			strbuf_addf(&desc, _("(HEAD detached from %s)"),
+				state.detached_from);
+	} else
+		strbuf_addstr(&desc, _("(no branch)"));
 
-	wt_status_state_free_buffers(&state);
 	return strbuf_detach(&desc, NULL);
 }
 
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index f92fb3aab9d..8f53b081365 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -223,8 +223,8 @@ test_expect_success 'git branch `--sort=[-]objectsize` option' '
 	cat >expect <<-\EOF &&
 	  branch-one
 	  main
-	* (HEAD detached from fromtag)
 	  branch-two
+	* (HEAD detached from fromtag)
 	EOF
 	git branch --sort=-objectsize >actual &&
 	test_i18ncmp expect actual
@@ -241,10 +241,10 @@ test_expect_success 'git branch `--sort=[-]type` option' '
 	test_i18ncmp expect actual &&
 
 	cat >expect <<-\EOF &&
-	* (HEAD detached from fromtag)
 	  branch-one
 	  branch-two
 	  main
+	* (HEAD detached from fromtag)
 	EOF
 	git branch --sort=-type >actual &&
 	test_i18ncmp expect actual
diff --git a/wt-status.c b/wt-status.c
index 7074bbdd53c..40b59be478c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1742,9 +1742,9 @@ static void wt_longstatus_print(struct wt_status *s)
 			} else if (s->state.detached_from) {
 				branch_name = s->state.detached_from;
 				if (s->state.detached_at)
-					on_what = HEAD_DETACHED_AT;
+					on_what = _("HEAD detached at ");
 				else
-					on_what = HEAD_DETACHED_FROM;
+					on_what = _("HEAD detached from ");
 			} else {
 				branch_name = "";
 				on_what = _("Not currently on any branch.");
diff --git a/wt-status.h b/wt-status.h
index 35b44c388ed..0d32799b28e 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -77,8 +77,6 @@ enum wt_status_format {
 	STATUS_FORMAT_UNSPECIFIED
 };
 
-#define HEAD_DETACHED_AT _("HEAD detached at ")
-#define HEAD_DETACHED_FROM _("HEAD detached from ")
 #define SPARSE_CHECKOUT_DISABLED -1
 
 struct wt_status_state {
-- 
2.29.2.222.g5d2a92d10f8

[PATCH 5/5] branch: show "HEAD detached" first under reverse sort

[Ævar Arnfjörð Bjarmason]

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

diff --git a/ref-filter.c b/ref-filter.c
index 7e0289cb659..5bbdc46c1f9 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2355,6 +2355,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
 {
 	struct atom_value *va, *vb;
 	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;
@@ -2370,6 +2371,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
 	     ^
 	     (b->kind & FILTER_REFS_DETACHED_HEAD))) {
 		cmp = compare_detached_head(a, b);
+		cmp_detached_head = 1;
 	} else if (s->version)
 		cmp = versioncmp(va->s, vb->s);
 	else if (cmp_type == FIELD_STR)
@@ -2383,7 +2385,7 @@ 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;
 }
 
 static int compare_refs(const void *a_, const void *b_, void *ref_sorting)
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index 8f53b081365..5e0577d5c7f 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -221,10 +221,10 @@ test_expect_success 'git branch `--sort=[-]objectsize` option' '
 	test_i18ncmp expect actual &&
 
 	cat >expect <<-\EOF &&
+	* (HEAD detached from fromtag)
 	  branch-one
 	  main
 	  branch-two
-	* (HEAD detached from fromtag)
 	EOF
 	git branch --sort=-objectsize >actual &&
 	test_i18ncmp expect actual
@@ -241,10 +241,10 @@ test_expect_success 'git branch `--sort=[-]type` option' '
 	test_i18ncmp expect actual &&
 
 	cat >expect <<-\EOF &&
+	* (HEAD detached from fromtag)
 	  branch-one
 	  branch-two
 	  main
-	* (HEAD detached from fromtag)
 	EOF
 	git branch --sort=-type >actual &&
 	test_i18ncmp expect actual
@@ -261,10 +261,10 @@ test_expect_success 'git branch `--sort=[-]version:refname` option' '
 	test_i18ncmp expect actual &&
 
 	cat >expect <<-\EOF &&
+	* (HEAD detached from fromtag)
 	  main
 	  branch-two
 	  branch-one
-	* (HEAD detached from fromtag)
 	EOF
 	git branch --sort=-version:refname >actual &&
 	test_i18ncmp expect actual
-- 
2.29.2.222.g5d2a92d10f8

Re: [PATCH 2/5] branch tests: add to --sort tests

[Junio C Hamano]

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

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

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.

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/t3203-branch-output.sh | 51 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
> index b945faf4702..f92fb3aab9d 100755
> --- a/t/t3203-branch-output.sh
> +++ b/t/t3203-branch-output.sh
> @@ -210,7 +210,7 @@ EOF
>  	test_i18ncmp expect actual
>  '
>  
> -test_expect_success 'git branch `--sort` option' '
> +test_expect_success 'git branch `--sort=[-]objectsize` option' '
>  	cat >expect <<-\EOF &&
>  	* (HEAD detached from fromtag)
>  	  branch-two
> @@ -218,6 +218,55 @@ test_expect_success 'git branch `--sort` option' '
>  	  main
>  	EOF
>  	git branch --sort=objectsize >actual &&
> +	test_i18ncmp expect actual &&
> +
> +	cat >expect <<-\EOF &&
> +	  branch-one
> +	  main
> +	* (HEAD detached from fromtag)
> +	  branch-two
> +	EOF
> +	git branch --sort=-objectsize >actual &&
> +	test_i18ncmp expect actual
> +'
> +
> +test_expect_success 'git branch `--sort=[-]type` option' '
> +	cat >expect <<-\EOF &&
> +	* (HEAD detached from fromtag)
> +	  branch-one
> +	  branch-two
> +	  main
> +	EOF
> +	git branch --sort=type >actual &&
> +	test_i18ncmp expect actual &&
> +
> +	cat >expect <<-\EOF &&
> +	* (HEAD detached from fromtag)
> +	  branch-one
> +	  branch-two
> +	  main
> +	EOF
> +	git branch --sort=-type >actual &&
> +	test_i18ncmp expect actual
> +'
> +
> +test_expect_success 'git branch `--sort=[-]version:refname` option' '
> +	cat >expect <<-\EOF &&
> +	* (HEAD detached from fromtag)
> +	  branch-one
> +	  branch-two
> +	  main
> +	EOF
> +	git branch --sort=version:refname >actual &&
> +	test_i18ncmp expect actual &&
> +
> +	cat >expect <<-\EOF &&
> +	  main
> +	  branch-two
> +	  branch-one
> +	* (HEAD detached from fromtag)
> +	EOF
> +	git branch --sort=-version:refname >actual &&
>  	test_i18ncmp expect actual
>  '

Re: [PATCH 3/5] ref-filter: add a "detached_head_first" sorting option

[Junio C Hamano]

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

> +static int compare_detached_head(struct ref_array_item *a, struct ref_array_item *b)
> +{
> +	if (a->kind & FILTER_REFS_DETACHED_HEAD)
> +		return -1;
> +	else if (b->kind & FILTER_REFS_DETACHED_HEAD)
> +		return 1;
> +	BUG("compare_detached_head() is guarded by an xor on [ab]->kind & FILTER_REFS_DETACHED_HEAD");
> +	return 0;
> +}

OK.

> +void ref_sorting_detached_head_first_all(struct ref_sorting *sorting, int flag)
> +{
> +	for (; sorting; sorting = sorting->next)
> +		sorting->detached_head_first = !!flag;
> +}

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

>  void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
>  {
>  	QSORT_S(array->items, array->nr, compare_refs, sorting);
> diff --git a/ref-filter.h b/ref-filter.h
> index feaef4a8fde..3b92e0f2696 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -30,6 +30,7 @@ struct ref_sorting {
>  	int atom; /* index into used_atom array (internal) */
>  	unsigned reverse : 1,
>  		ignore_case : 1,
> +		detached_head_first : 1,
>  		version : 1;
>  };
>  
> @@ -111,6 +112,8 @@ int verify_ref_format(struct ref_format *format);
>  void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
>  /*  Set the ignore_case flag for all elements of a sorting list */
>  void ref_sorting_icase_all(struct ref_sorting *sorting, int flag);
> +/*  Set the detached_head_first flag for all elements of a sorting list */
> +void ref_sorting_detached_head_first_all(struct ref_sorting *sorting, int flag);
>  /*  Based on the given format and quote_style, fill the strbuf */
>  int format_ref_array_item(struct ref_array_item *info,
>  			  const struct ref_format *format,

Re: [PATCH 5/5] branch: show "HEAD detached" first under reverse sort

[Junio C Hamano]

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

>  	struct atom_value *va, *vb;
>  	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;
> @@ -2370,6 +2371,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
>  	     ^
>  	     (b->kind & FILTER_REFS_DETACHED_HEAD))) {
>  		cmp = compare_detached_head(a, b);
> +		cmp_detached_head = 1;
>  	} else if (s->version)
>  		cmp = versioncmp(va->s, vb->s);
>  	else if (cmp_type == FIELD_STR)
> @@ -2383,7 +2385,7 @@ 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;
>  }

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.

>  static int compare_refs(const void *a_, const void *b_, void *ref_sorting)
> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
> index 8f53b081365..5e0577d5c7f 100755
> --- a/t/t3203-branch-output.sh
> +++ b/t/t3203-branch-output.sh
> @@ -221,10 +221,10 @@ test_expect_success 'git branch `--sort=[-]objectsize` option' '
>  	test_i18ncmp expect actual &&
>  
>  	cat >expect <<-\EOF &&
> +	* (HEAD detached from fromtag)
>  	  branch-one
>  	  main
>  	  branch-two
> -	* (HEAD detached from fromtag)
>  	EOF
>  	git branch --sort=-objectsize >actual &&
>  	test_i18ncmp expect actual
> @@ -241,10 +241,10 @@ test_expect_success 'git branch `--sort=[-]type` option' '
>  	test_i18ncmp expect actual &&
>  
>  	cat >expect <<-\EOF &&
> +	* (HEAD detached from fromtag)
>  	  branch-one
>  	  branch-two
>  	  main
> -	* (HEAD detached from fromtag)
>  	EOF
>  	git branch --sort=-type >actual &&
>  	test_i18ncmp expect actual
> @@ -261,10 +261,10 @@ test_expect_success 'git branch `--sort=[-]version:refname` option' '
>  	test_i18ncmp expect actual &&
>  
>  	cat >expect <<-\EOF &&
> +	* (HEAD detached from fromtag)
>  	  main
>  	  branch-two
>  	  branch-one
> -	* (HEAD detached from fromtag)
>  	EOF
>  	git branch --sort=-version:refname >actual &&
>  	test_i18ncmp expect actual

[PATCH v2 0/7] branch: --sort improvements

[Ævar Arnfjörð Bjarmason]

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

[PATCH v2 1/7] branch: change "--local" to "--list" in comment

[Ævar Arnfjörð Bjarmason]

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

diff --git a/builtin/branch.c b/builtin/branch.c
index 9b68591addf..045866a51ae 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -726,7 +726,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		print_current_branch_name();
 		return 0;
 	} else if (list) {
-		/*  git branch --local also shows HEAD when it is detached */
+		/*  git branch --list also shows HEAD when it is detached */
 		if ((filter.kind & FILTER_REFS_BRANCHES) && filter.detached)
 			filter.kind |= FILTER_REFS_DETACHED_HEAD;
 		filter.name_patterns = argv;
-- 
2.29.2.222.g5d2a92d10f8

[PATCH v2 2/7] branch tests: add to --sort tests

[Ævar Arnfjörð Bjarmason]

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

diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index b945faf4702..f92fb3aab9d 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -210,7 +210,7 @@ EOF
 	test_i18ncmp expect actual
 '
 
-test_expect_success 'git branch `--sort` option' '
+test_expect_success 'git branch `--sort=[-]objectsize` option' '
 	cat >expect <<-\EOF &&
 	* (HEAD detached from fromtag)
 	  branch-two
@@ -218,6 +218,55 @@ test_expect_success 'git branch `--sort` option' '
 	  main
 	EOF
 	git branch --sort=objectsize >actual &&
+	test_i18ncmp expect actual &&
+
+	cat >expect <<-\EOF &&
+	  branch-one
+	  main
+	* (HEAD detached from fromtag)
+	  branch-two
+	EOF
+	git branch --sort=-objectsize >actual &&
+	test_i18ncmp expect actual
+'
+
+test_expect_success 'git branch `--sort=[-]type` option' '
+	cat >expect <<-\EOF &&
+	* (HEAD detached from fromtag)
+	  branch-one
+	  branch-two
+	  main
+	EOF
+	git branch --sort=type >actual &&
+	test_i18ncmp expect actual &&
+
+	cat >expect <<-\EOF &&
+	* (HEAD detached from fromtag)
+	  branch-one
+	  branch-two
+	  main
+	EOF
+	git branch --sort=-type >actual &&
+	test_i18ncmp expect actual
+'
+
+test_expect_success 'git branch `--sort=[-]version:refname` option' '
+	cat >expect <<-\EOF &&
+	* (HEAD detached from fromtag)
+	  branch-one
+	  branch-two
+	  main
+	EOF
+	git branch --sort=version:refname >actual &&
+	test_i18ncmp expect actual &&
+
+	cat >expect <<-\EOF &&
+	  main
+	  branch-two
+	  branch-one
+	* (HEAD detached from fromtag)
+	EOF
+	git branch --sort=-version:refname >actual &&
 	test_i18ncmp expect actual
 '
 
-- 
2.29.2.222.g5d2a92d10f8

[PATCH v2 3/7] ref-filter: add braces to if/else if/else chain

[Ævar Arnfjörð Bjarmason]

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

diff --git a/ref-filter.c b/ref-filter.c
index aa260bfd099..e4c162a8c34 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2364,11 +2364,11 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
 		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) {
 		cmp = cmp_fn(va->s, vb->s);
-	else {
+	} else {
 		if (va->value < vb->value)
 			cmp = -1;
 		else if (va->value == vb->value)
-- 
2.29.2.222.g5d2a92d10f8

[PATCH v2 4/7] ref-filter: move "cmp_fn" assignment into "else if" arm

[Ævar Arnfjörð Bjarmason]

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

diff --git a/ref-filter.c b/ref-filter.c
index e4c162a8c34..8882128cd3e 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2355,7 +2355,6 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
 	struct atom_value *va, *vb;
 	int cmp;
 	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))
@@ -2363,10 +2362,11 @@ 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);
-	cmp_fn = s->ignore_case ? strcasecmp : strcmp;
 	if (s->version) {
 		cmp = versioncmp(va->s, vb->s);
 	} else if (cmp_type == FIELD_STR) {
+		int (*cmp_fn)(const char *, const char *);
+		cmp_fn = s->ignore_case ? strcasecmp : strcmp;
 		cmp = cmp_fn(va->s, vb->s);
 	} else {
 		if (va->value < vb->value)
-- 
2.29.2.222.g5d2a92d10f8

[PATCH v2 5/7] ref-filter: move ref_sorting flags to a bitfield

[Ævar Arnfjörð Bjarmason]

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

diff --git a/builtin/branch.c b/builtin/branch.c
index 045866a51ae..2dd51a8653b 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -739,7 +739,7 @@ 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_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);
 		print_ref_list(&filter, sorting, &format);
 		print_columns(&output, colopts, NULL);
 		string_list_clear(&output, 0);
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 9d1ecda2b8f..cb9c81a0460 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -70,7 +70,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 
 	if (!sorting)
 		sorting = ref_default_sorting();
-	ref_sorting_icase_all(sorting, icase);
+	ref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);
 	filter.ignore_case = icase;
 
 	filter.name_patterns = argv;
diff --git a/builtin/tag.c b/builtin/tag.c
index ecf011776dc..24d35b746d1 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -485,7 +485,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	}
 	if (!sorting)
 		sorting = ref_default_sorting();
-	ref_sorting_icase_all(sorting, icase);
+	ref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);
 	filter.ignore_case = icase;
 	if (cmdmode == 'l') {
 		int ret;
diff --git a/ref-filter.c b/ref-filter.c
index 8882128cd3e..fe587afb80b 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2362,11 +2362,12 @@ 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->version) {
+	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 *);
-		cmp_fn = s->ignore_case ? strcasecmp : strcmp;
+		cmp_fn = s->sort_flags & REF_SORTING_ICASE
+			? strcasecmp : strcmp;
 		cmp = cmp_fn(va->s, vb->s);
 	} else {
 		if (va->value < vb->value)
@@ -2377,7 +2378,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
 			cmp = 1;
 	}
 
-	return (s->reverse) ? -cmp : cmp;
+	return (s->sort_flags & REF_SORTING_REVERSE) ? -cmp : cmp;
 }
 
 static int compare_refs(const void *a_, const void *b_, void *ref_sorting)
@@ -2392,15 +2393,20 @@ static int compare_refs(const void *a_, const void *b_, void *ref_sorting)
 			return cmp;
 	}
 	s = ref_sorting;
-	return s && s->ignore_case ?
+	return s && s->sort_flags & REF_SORTING_ICASE ?
 		strcasecmp(a->refname, b->refname) :
 		strcmp(a->refname, b->refname);
 }
 
-void ref_sorting_icase_all(struct ref_sorting *sorting, int flag)
+void ref_sorting_set_sort_flags_all(struct ref_sorting *sorting,
+				    unsigned int mask, int on)
 {
-	for (; sorting; sorting = sorting->next)
-		sorting->ignore_case = !!flag;
+	for (; sorting; sorting = sorting->next) {
+		if (on)
+			sorting->sort_flags |= mask;
+		else
+			sorting->sort_flags &= ~mask;
+	}
 }
 
 void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
@@ -2537,12 +2543,12 @@ void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)
 	*sorting_tail = s;
 
 	if (*arg == '-') {
-		s->reverse = 1;
+		s->sort_flags |= REF_SORTING_REVERSE;
 		arg++;
 	}
 	if (skip_prefix(arg, "version:", &arg) ||
 	    skip_prefix(arg, "v:", &arg))
-		s->version = 1;
+		s->sort_flags |= REF_SORTING_VERSION;
 	s->atom = parse_sorting_atom(arg);
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index feaef4a8fde..6296ae8bb27 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -28,9 +28,11 @@ struct atom_value;
 struct ref_sorting {
 	struct ref_sorting *next;
 	int atom; /* index into used_atom array (internal) */
-	unsigned reverse : 1,
-		ignore_case : 1,
-		version : 1;
+	enum {
+		REF_SORTING_REVERSE = 1<<0,
+		REF_SORTING_ICASE = 1<<1,
+		REF_SORTING_VERSION = 1<<2,
+	} sort_flags;
 };
 
 struct ref_array_item {
@@ -109,8 +111,8 @@ void ref_array_clear(struct ref_array *array);
 int verify_ref_format(struct ref_format *format);
 /*  Sort the given ref_array as per the ref_sorting provided */
 void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
-/*  Set the ignore_case flag for all elements of a sorting list */
-void ref_sorting_icase_all(struct ref_sorting *sorting, int flag);
+/*  Set REF_SORTING_* sort_flags for all elements of a sorting list */
+void ref_sorting_set_sort_flags_all(struct ref_sorting *sorting, unsigned int mask, int on);
 /*  Based on the given format and quote_style, fill the strbuf */
 int format_ref_array_item(struct ref_array_item *info,
 			  const struct ref_format *format,
-- 
2.29.2.222.g5d2a92d10f8

[PATCH v2 6/7] branch: sort detached HEAD based on a flag

[Ævar Arnfjörð Bjarmason]

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

diff --git a/builtin/branch.c b/builtin/branch.c
index 2dd51a8653b..8c0b428104d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -740,6 +740,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		if (!sorting)
 			sorting = ref_default_sorting();
 		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);
diff --git a/ref-filter.c b/ref-filter.c
index fe587afb80b..8d0739b9972 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1536,36 +1536,27 @@ char *get_head_description(void)
 	struct wt_status_state state;
 	memset(&state, 0, sizeof(state));
 	wt_status_get_state(the_repository, &state, 1);
-
-	/*
-	 * The ( character must be hard-coded and not part of a localizable
-	 * string, since the description is used as a sort key and compared
-	 * with ref names.
-	 */
-	strbuf_addch(&desc, '(');
 	if (state.rebase_in_progress ||
 	    state.rebase_interactive_in_progress) {
 		if (state.branch)
-			strbuf_addf(&desc, _("no branch, rebasing %s"),
+			strbuf_addf(&desc, _("(no branch, rebasing %s)"),
 				    state.branch);
 		else
-			strbuf_addf(&desc, _("no branch, rebasing detached HEAD %s"),
+			strbuf_addf(&desc, _("(no branch, rebasing detached HEAD %s)"),
 				    state.detached_from);
 	} else if (state.bisect_in_progress)
-		strbuf_addf(&desc, _("no branch, bisect started on %s"),
+		strbuf_addf(&desc, _("(no branch, bisect started on %s)"),
 			    state.branch);
 	else if (state.detached_from) {
 		if (state.detached_at)
-			strbuf_addstr(&desc, HEAD_DETACHED_AT);
+			strbuf_addf(&desc, _("(HEAD detached at %s)"),
+				state.detached_from);
 		else
-			strbuf_addstr(&desc, HEAD_DETACHED_FROM);
-		strbuf_addstr(&desc, state.detached_from);
-	}
-	else
-		strbuf_addstr(&desc, _("no branch"));
-	strbuf_addch(&desc, ')');
+			strbuf_addf(&desc, _("(HEAD detached from %s)"),
+				state.detached_from);
+	} else
+		strbuf_addstr(&desc, _("(no branch)"));
 
-	wt_status_state_free_buffers(&state);
 	return strbuf_detach(&desc, NULL);
 }
 
@@ -2350,6 +2341,18 @@ 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;
@@ -2362,7 +2365,10 @@ 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 *);
diff --git a/ref-filter.h b/ref-filter.h
index 6296ae8bb27..19ea4c41340 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -32,6 +32,7 @@ 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;
 };
 
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index f92fb3aab9d..8f53b081365 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -223,8 +223,8 @@ test_expect_success 'git branch `--sort=[-]objectsize` option' '
 	cat >expect <<-\EOF &&
 	  branch-one
 	  main
-	* (HEAD detached from fromtag)
 	  branch-two
+	* (HEAD detached from fromtag)
 	EOF
 	git branch --sort=-objectsize >actual &&
 	test_i18ncmp expect actual
@@ -241,10 +241,10 @@ test_expect_success 'git branch `--sort=[-]type` option' '
 	test_i18ncmp expect actual &&
 
 	cat >expect <<-\EOF &&
-	* (HEAD detached from fromtag)
 	  branch-one
 	  branch-two
 	  main
+	* (HEAD detached from fromtag)
 	EOF
 	git branch --sort=-type >actual &&
 	test_i18ncmp expect actual
diff --git a/wt-status.c b/wt-status.c
index 7074bbdd53c..40b59be478c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1742,9 +1742,9 @@ static void wt_longstatus_print(struct wt_status *s)
 			} else if (s->state.detached_from) {
 				branch_name = s->state.detached_from;
 				if (s->state.detached_at)
-					on_what = HEAD_DETACHED_AT;
+					on_what = _("HEAD detached at ");
 				else
-					on_what = HEAD_DETACHED_FROM;
+					on_what = _("HEAD detached from ");
 			} else {
 				branch_name = "";
 				on_what = _("Not currently on any branch.");
diff --git a/wt-status.h b/wt-status.h
index 35b44c388ed..0d32799b28e 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -77,8 +77,6 @@ enum wt_status_format {
 	STATUS_FORMAT_UNSPECIFIED
 };
 
-#define HEAD_DETACHED_AT _("HEAD detached at ")
-#define HEAD_DETACHED_FROM _("HEAD detached from ")
 #define SPARSE_CHECKOUT_DISABLED -1
 
 struct wt_status_state {
-- 
2.29.2.222.g5d2a92d10f8

[PATCH v2 7/7] branch: show "HEAD detached" first under reverse sort

[Ævar Arnfjörð Bjarmason]

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

diff --git a/ref-filter.c b/ref-filter.c
index 8d0739b9972..ee337df232a 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2357,6 +2357,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
 {
 	struct atom_value *va, *vb;
 	int cmp;
+	int cmp_detached_head = 0;
 	cmp_type cmp_type = used_atom[s->atom].type;
 	struct strbuf err = STRBUF_INIT;
 
@@ -2368,6 +2369,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
 	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->sort_flags & REF_SORTING_VERSION) {
 		cmp = versioncmp(va->s, vb->s);
 	} else if (cmp_type == FIELD_STR) {
@@ -2384,7 +2386,8 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
 			cmp = 1;
 	}
 
-	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)
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index 8f53b081365..5e0577d5c7f 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -221,10 +221,10 @@ test_expect_success 'git branch `--sort=[-]objectsize` option' '
 	test_i18ncmp expect actual &&
 
 	cat >expect <<-\EOF &&
+	* (HEAD detached from fromtag)
 	  branch-one
 	  main
 	  branch-two
-	* (HEAD detached from fromtag)
 	EOF
 	git branch --sort=-objectsize >actual &&
 	test_i18ncmp expect actual
@@ -241,10 +241,10 @@ test_expect_success 'git branch `--sort=[-]type` option' '
 	test_i18ncmp expect actual &&
 
 	cat >expect <<-\EOF &&
+	* (HEAD detached from fromtag)
 	  branch-one
 	  branch-two
 	  main
-	* (HEAD detached from fromtag)
 	EOF
 	git branch --sort=-type >actual &&
 	test_i18ncmp expect actual
@@ -261,10 +261,10 @@ test_expect_success 'git branch `--sort=[-]version:refname` option' '
 	test_i18ncmp expect actual &&
 
 	cat >expect <<-\EOF &&
+	* (HEAD detached from fromtag)
 	  main
 	  branch-two
 	  branch-one
-	* (HEAD detached from fromtag)
 	EOF
 	git branch --sort=-version:refname >actual &&
 	test_i18ncmp expect actual
-- 
2.29.2.222.g5d2a92d10f8

Re: [PATCH v2 5/7] ref-filter: move ref_sorting flags to a bitfield

[Junio C Hamano]

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

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

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.