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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 18, 2021, 05:11 UTC
Message-ID: <CAPig+cSR_y4uqPnYt+P4EFxPrhN8LHDjLX=_5uJFWYbL1skixA@mail.gmail.com>
In-Reply-To: <patch-2.3-6f9e09a2017-20210417T125540Z-avarab@gmail.com>

On Sat, Apr 17, 2021 at 8:58 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 6 quoted lines

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

My one concern about this change is that it enters the realm of "huh?". Although it's dead-simple to understand what this code is doing:

    test "$#" -ne 1 && BUG "1 param"
    if ! test -f "$1"

the replacement code:

    if ! test -f "$@"

can easily lead to head-scratching, wondering why the author chose to use "$@" rather than the more obvious "$1". This sort of unusual local idiom might normally deserve an in-code comment explaining why the more obvious code is not employed. However, adding a comment at each site would be overkill, so it might be the sort of thing to explain in documentation somewhere ("In order to make -x output less noisy, employ "$@" rather than explicitly checking function arguments when writing new test functions...".) Otherwise, the reader is forced to consult the commit message.

Not a major objection; just voicing the bit of unease I feel about it.

> This does not to the "right" thing in cases like:

Channeling Dscho's inner Eric Sunshine[1]: s/to/do/

[1]: https://lore.kernel.org/git/nycvar.QRO.7.76.6.2103222235150.50@tvgsbejvaqbjf.bet/