Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> On Mon, Apr 12 2021, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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.
