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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 15, 2021, 11:32 UTC
Message-ID: <87blafvl49.fsf@evledraar.gmail.com>
In-Reply-To: <xmqqa6q3cn0s.fsf@gitster.g>

On Mon, Apr 12 2021, Junio C Hamano wrote:

Show 26 quoted lines

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

And then say:

    File COPYING Makefile doesn't exist

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!"
	 		echo "File $@ doesn't exist"
	 		return 1
	 	fi

But I could do that if you think it's better.

We could also just emit $1 in the "echo". I don't feel strongly about
that, but think that's arguably worse, since having it be $@ means the
developer is more likely to notice the invalid usage right away.