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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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