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

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Mon, Apr 12, 2021 at 01:09:04PM +0200, Ævar Arnfjörð Bjarmason wrote:
> This reverts and amends my my own e7884b353b7 (test-lib-functions:
> assert correct parameter count, 2021-02-12) in order to improve the -x
> output.
> 
> When I added these BUG assertions in e7884b353b7 I missed that this
> made the -x output much more verbose.
> 
> E.g. for each test_cmp invocation we'd now emit:
> 
>     + test_cmp expect actual
>     + test 2 -ne 2
>     + eval diff -u "$@"
>     + diff -u expect actual
> 
> That "test 2 -ne 2" line is new in e7884b353b7. As noted in
> 45a2686441b (test-lib-functions: remove bug-inducing "diagnostics"
> helper param, 2021-02-12) we had buggy invocations of some of these
> functions with too many parameters.
> 
> Let's instead use "$@" instead of "$1" to achieve the same ends with
> no extra -x output verbosity. The "test" operator will die with an
> error if given more than one argument in these contexts, so using "$@"
> achieves the same goal.

I prefer the current check for its explicitness over the implicit and
somewhat cryptic approach introduced in this patch.  I hope that
sooner or later I'll finish up my patch series to suppress '-x' output
from test helper functions, and then this issue will become moot
anyway.

> The same goes for "cmp" and "diff -u" (which we typically use for
> test_cmp).
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/test-lib-functions.sh | 41 ++++++++++++++++-------------------------
>  1 file changed, 16 insertions(+), 25 deletions(-)
> 
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index c46bf0ff09c..2cf72b56851 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -759,39 +759,35 @@ test_external_without_stderr () {
>  # debugging-friendly alternatives to "test [-f|-d|-e]"
>  # The commands test the existence or non-existence of $1
>  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
>  	fi
>  }
>  
>  test_path_is_dir () {
> -	test "$#" -ne 1 && BUG "1 param"
> -	if ! test -d "$1"
> +	if ! test -d "$@"
>  	then
> -		echo "Directory $1 doesn't exist"
> +		echo "Directory $@ doesn't exist"
>  		return 1
>  	fi
>  }
>  
>  test_path_exists () {
> -	test "$#" -ne 1 && BUG "1 param"
> -	if ! test -e "$1"
> +	if ! test -e "$@"
>  	then
> -		echo "Path $1 doesn't exist"
> +		echo "Path $@ doesn't exist"
>  		return 1
>  	fi
>  }
>  
>  # Check if the directory exists and is empty as expected, barf otherwise.
>  test_dir_is_empty () {
> -	test "$#" -ne 1 && BUG "1 param"
> -	test_path_is_dir "$1" &&
> -	if test -n "$(ls -a1 "$1" | egrep -v '^\.\.?$')"
> +	test_path_is_dir "$@" &&
> +	if test -n "$(ls -a1 "$@" | egrep -v '^\.\.?$')"
>  	then
> -		echo "Directory '$1' is not empty, it contains:"
> +		echo "Directory '$@' is not empty, it contains:"
>  		ls -la "$1"
>  		return 1
>  	fi
> @@ -799,17 +795,15 @@ test_dir_is_empty () {
>  
>  # Check if the file exists and has a size greater than zero
>  test_file_not_empty () {
> -	test "$#" = 2 && BUG "2 param"
> -	if ! test -s "$1"
> +	if ! test -s "$@"
>  	then
> -		echo "'$1' is not a non-empty file."
> +		echo "'$@' is not a non-empty file."
>  		return 1
>  	fi
>  }
>  
>  test_path_is_missing () {
> -	test "$#" -ne 1 && BUG "1 param"
> -	if test -e "$1"
> +	if test -e "$@"
>  	then
>  		echo "Path $1 exists!"
>  		false
> @@ -1013,7 +1007,6 @@ test_expect_code () {
>  # - not all diff versions understand "-u"
>  
>  test_cmp () {
> -	test "$#" -ne 2 && BUG "2 param"
>  	eval "$GIT_TEST_CMP" '"$@"'
>  }
>  
> @@ -1043,7 +1036,6 @@ test_cmp_config () {
>  # test_cmp_bin - helper to compare binary files
>  
>  test_cmp_bin () {
> -	test "$#" -ne 2 && BUG "2 param"
>  	cmp "$@"
>  }
>  
> @@ -1104,12 +1096,11 @@ verbose () {
>  # otherwise.
>  
>  test_must_be_empty () {
> -	test "$#" -ne 1 && BUG "1 param"
> -	test_path_is_file "$1" &&
> -	if test -s "$1"
> +	test_path_is_file "$@" &&
> +	if test -s "$@"
>  	then
> -		echo "'$1' is not empty, it contains:"
> -		cat "$1"
> +		echo "'$@' is not empty, it contains:"
> +		cat "$@"
>  		return 1
>  	fi
>  }
> -- 
> 2.31.1.634.gb41287a30b0
>