On Thu, Apr 15 2021, Junio C Hamano wrote:
> Æ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?
The reasoning is to get rid of verbosity in the trace output, while
still effectively retaining the error checking.
Yes you could do "foo -o ok", but as my already-on-master fixes to the
few misuses showed we only realistically have to worry about them being
used with many normal looking file names (if that).
>> 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?
It's fine to do sanity checking if it's in the "else" branch where we're
already emitting an error, and we only do so in the BUG case.
I.e. if it's written like e.g. this:
test_path_is_file () {
if test $# -ne 1
then
BUG "Do not call test_path_is_file() with more than one argument!"
elif ! test -f "$@"
then
echo "File $@ doesn't exist"
Then with e.g.:
./t3600-rm.sh --run=1-3 -vx
We get:
+ test_path_is_file foo
+ test 1 -ne 1
+ test -f foo
+ git ls-files --error-unmatch foo
But if we, as my patch does, piggy-pack on the test-built in to panic on
too many arguments we get the much more succinct:
+ test_path_is_file foo
+ test -f foo
+ git ls-files --error-unmatch foo
I think that's the only trace output that matters, having "test 2 -ne 1"
or whatever in the case where we're just about to invoke BUG anyway is
fine.
