On Tue, Apr 20 2021, Eric Sunshine wrote:
> On Tue, Apr 20, 2021 at 8:29 AM Ævar Arnfjörð Bjarmason
> <avarab@xxxxxxxxx> wrote:
>> [...]
>> The goal here is to get rid of the verbosity of having e.g. a "test 2
>> -ne 2" line for every "test_cmp". We use "$@" as an argument to "test"
>> to intentionally feed the "test" operator too many arguments if the
>> functions are called with too many arguments, thus piggy-backing on it
>> to check the number of arguments we get.
>> [...]
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
>> ---
>> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
>> @@ -756,41 +756,43 @@ test_external_without_stderr () {
>> # debugging-friendly alternatives to "test [-f|-d|-e]"
>> +# The commands test the existence or non-existence of
>> +# a given argument.
>> +#
>> +# The pattern of using "$@" to "test" instead of "$1" is not a bug. We
>> +# are counting on "test" to error on too many arguments if more than
>> +# one is given. Checking "$#" explicitly would lead to overly verbose
>> +# -x output.
>> test_path_is_file () {
>> + if ! test -f "$@"
>
> Thanks. The new comment makes the intent of "$@" clear.
>
> If you do re-roll for some reason, it might make sense to move the new
> comment (the one starting "The pattern of...") into the function
> itself just before the use of "$@". The reasons I suggest this are:
> (1) the comment explains an implementation detail, thus is intended
> for people who might change this function, whereas all the text above
> the new paragraph is API documentation for callers of the function who
> need only the black-box description; (2) it places the comment closer
> to the relevant code, thus is less likely to be overlooked by someone
> changing the function.
I think moving it would be worse / wouldn't make sense.
The comment doesn't just apply to test_path_is_file(), but the next N
functions that use the "$@" pattern. Just like the existing
"debugging-friendly alternatives" comment does. I.e. test_path_is_file
is just the "-f" part of thta comment, the following test_path_is_dir is
the "-d" etc.
