RE: [PATCH v3 2/4] Revert and amend "test-lib-functions: assert correct parameter count"

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Apr 29, 2021, 04:02 UTC
Message-ID: <608a2fbe5e70e_ac4020821@natae.notmuch>
In-Reply-To: <patch-2.4-3a570321770-20210423T072824Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason wrote:

Show 16 quoted lines

> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 2914b398470..0adb9fd124d 100644
> --- 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 $1
> +# 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.

I would consider adding an extra line. In my mind a comment without a line is for the next function, one with a line is for the section.

  # function comment
  function_1 ()

Versus:

  # section comment

  function_1 ()
  function_2 ()

But I agree with the other comments that it's better inside the function. It's an implementation detail.

While I think git tries to follow the self-documenting code style, sometimes it's better to just write the comment.

>  test_path_is_file () {
> -	test "$#" -ne 1 && BUG "1 param"
> -	if ! test -f "$1"
> +	if ! test -f "$@"

  if ! test -f "$@" # Rely on test to ensure 1 param

Show 7 quoted lines

>  	then
> -		echo "File $1 doesn't exist"
> -		false
> +		echo "File $* doesn't exist"
> +		return 1
>  	fi
>  }

And yes, I would add that comment to every instance where this trick is used. That way people reading the second or third function don't have to scratch their heads, or miraculously read a comment far above.

Cheers.

-- 
Felipe Contreras