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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jun 11, 2019, 00:41 UTC
Message-ID: <20190611004106.GB64137@google.com>
In-Reply-To: <20190610234918.GA10396@comcast.net>

Hi,

Matthew DeVore wrote:

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

Show 14 quoted lines

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

[...]

Show 9 quoted lines

>>> +	LC_ALL=$zh_CN_locale LC_MESSAGES=$zh_CN_locale \
>>> +		git -C r1 branch >actual &&
>>> +	git -C r1 checkout - &&
>>
>> Why call `checkout` after `branch`? That's unnecessary, we do not verify
>> anything after that call.
>
> It's to get the repo into a neutral state in case an additional testcase is
> added in the future.

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.

[...]

Show 14 quoted lines

> 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