Re: [PATCH] hook: test -a|o is not POSIX

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 9, 2022, 18:32 UTC
Message-ID: <xmqqmtk43glm.fsf@gitster.g>
In-Reply-To: <9c7aeb5b-87b2-a51e-c08d-8dfc75ecf588@gmail.com>

Bagas Sanjaya <bagasdotme@gmail.com> writes:

Show 8 quoted lines

>> -		if [ "$oldrev" = "$zero" -a "$denycreatebranch" = "true" ]; then
>> +		if [ "$oldrev" = "$zero" ] && [ "$denycreatebranch" = "true" ]; then
>>   			echo "*** Creating a branch is not allowed in this repository" >&2
>>   			exit 1
>>   		fi
>> base-commit: 2ae0a9cb8298185a94e5998086f380a355dd8907
>
> From the patch above, the title should be "replace non-POSIX test -a & test -o with logical operator".

I think "should" is a bit too strong a word. While yours is a bit more explicit on _what_ the solution is, "hook: test -a|o is not POSIX" already implies that we are fixing that non-posix-ness by rewriting, and it is obvious (cf. Documentation/CodingGuidelines) what the right rewrite should be.

One thing the original does a bit better tha yours is that it tell us _where_ the problem is. So, perhaps

    sample hook: use "test ... &&/|| test ..." instead of "test -a/-o"

But I find the original just fine.