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

From: Jonathan Nieder <jrnieder@gmail.com>
To: Matthew DeVore <matvore@comcast.net>
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Matthew DeVore <matvore@google.com>,
	git@vger.kernel.org, avarab@gmail.com, git@matthieu-moy.fr,
	olyatelezhnaya@gmail.com, samuel.maftoul@gmail.com,
	gitster@pobox.com, karthik.188@gmail.com, pclouds@gmail.com,
	sunshine@sunshineco.com, emilyshaffer@google.com
Subject: Re: [RFC PATCH] ref-filter: sort detached HEAD lines firstly
Date: Mon, 10 Jun 2019 17:41:06 -0700	[thread overview]
Message-ID: <20190611004106.GB64137@google.com> (raw)
In-Reply-To: <20190610234918.GA10396@comcast.net>

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