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

From: Junio C Hamano <[email-masked]>
Date: Thu, 15 Apr 2021 11:35:48 -0700

Ævar Arnfjörð Bjarmason <[email-masked]> writes:

> On Mon, Apr 12 2021, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason  <[email-masked]> 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?

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