Re: [PATCH] t/test_lib: avoid naked bash arrays in file_lineno

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 2891 bytes --]

Hi Carlo,

On Wed, 6 May 2020, Carlo Marcelo Arenas Belón wrote:

> 662f9cf154 (tests: when run in Bash, annotate test failures with file
> name/line number, 2020-04-11), introduces a way to report the location
> (file:lineno) of a failed test case by traversing the bash callstack.
>
> The implementation requires bash and is therefore protected by a guard
> but NetBSD sh will still have to parse the function and therefore will
> result in:
>
>   ** t0000-basic.sh ***
>   ./test-lib.sh: 681: Syntax error: Bad substitution
>
> Enclose the bash specific code inside an eval to avoid parsing errors
> and while at it, simplify the logic so that instead of traversing the
> callstack just pop the two topmost entries that are required.

I would be okay with that, but that's not what the patch does:

> Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  t/test-lib.sh | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 1b221951a8..60b8e70678 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -677,14 +677,13 @@ die () {
>
>  file_lineno () {
>  	test -z "$GIT_TEST_FRAMEWORK_SELFTEST" && test -n "$BASH" || return 0
> -	local i
> -	for i in ${!BASH_SOURCE[*]}
> -	do
> -		case $i,"${BASH_SOURCE[$i]##*/}" in
> -		0,t[0-9]*.sh) echo "t/${BASH_SOURCE[$i]}:$LINENO: ${1+$1: }"; return;;
> -		*,t[0-9]*.sh) echo "t/${BASH_SOURCE[$i]}:${BASH_LINENO[$(($i-1))]}: ${1+$1: }"; return;;
> -		esac
> -	done

Note how this `for` loop stops as soon as it finds a `t/[0-9]*.sh` in the
backtrace?

> +	eval '
> +		local n=$(echo ${#BASH_SOURCE[*]})
> +		if test $n -ge 2
> +		then
> +			echo "${BASH_SOURCE[$n - 1]}:${BASH_LINENO[$n - 2]}: $1: "
> +		fi

This loop is completely lost here.

Now, you could argue that your version is better because it avoids the
loop and always shows the ultimate caller in the backtrace.

However, I would then immediately point you to `t/t0027-auto-crlf.sh` and
ask whether it is all that useful to the `commit_chk_wrnNNO` call rather
than one of the five `test_expect_success` calls contained in that
function.

Besides, you simply replaces `${1+$1: }` by `$1: `.

Again, you could argue that your version is better because there is now
only one caller, and it always passes `error` as first parameter.

I would not be _too_ happy about losing the logic that allows calling
`file_lineno` in other circumstances (I found it valuable for debugging),
but _in the least_ you need to talk about this change in the commit
message. Just changing the behavior without even describing, let alone
justifying, it in the commit message is a bad idea.

Ciao,
Dscho

> +	'
>  }
>
>  GIT_EXIT_OK=
> --
> 2.26.2.686.gfaf46a9ccd
>
>