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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux