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