Re: [PATCH 15/16] Revert and amend "test-lib-functions: assert correct parameter count"

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 15, 2021, 18:35 UTC
Message-ID: <xmqqh7k7qttn.fsf@gitster.g>
In-Reply-To: <87blafvl49.fsf@evledraar.gmail.com>

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

Show 32 quoted lines

> On Mon, Apr 12 2021, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>
>>>  test_path_is_file () {
>>> -	test "$#" -ne 1 && BUG "1 param"
>>> -	if ! test -f "$1"
>>> +	if ! test -f "$@"
>>>  	then
>>> -		echo "File $1 doesn't exist"
>>> +		echo "File $@ doesn't exist"
>>>  		return 1
>>
>> What does it even mean to call
>>
>> 	test_path_is_file Documentation/ Makefile
>>
>> with this patch applied?
>>
>> If there were three files "COPYING Makefile", "COPYING", and
>> "Makefile", what would happen when you did
>>
>> 	test_path_is_file COPYING Makefile
>>
>> (without dq around them)?
>>
>> I think this particular medicine is far worse than the symptom it
>> tries to cure.
>
> We'll error with:
>
>     test: foo: unexpected operator

Ah, so use of "$@" was intentional. That's clever (I thought it was a common typo people make when they mean "$*").

Of course, it would not work if the caller did a nonsense like so:

	test_path_is_file foo -o ok

but as long as we trust that the callers would not make stupid mistakes, this is OK. Is that the reasoning behind this removal of the BUG?

Show 13 quoted lines

> I thought guarding just for the one-off development error of not using
> the function correctly wasn't worth it, but I thought it made sense not
> to litter all of this with:
> 	
> 	diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> 	index 28b8826e565..0bd7367a07e 100644
> 	--- a/t/test-lib-functions.sh
> 	+++ b/t/test-lib-functions.sh
> 	@@ -690,6 +690,7 @@ test_expect_success () {
> 	 test_path_is_file () {
> 	 	if ! test -f "$@"
> 	 	then
> 	+		test $# -eq 1 || BUG "Do not call test_path_is_file() with more than one argument!"

But this breaks our assumption that the caller would not be making stupid mistakes, so I am not sure if it is worth it. If we were to have a sanity check, shouldn't we do the check upfront, like the original?

Thanks.