Re: [PATCH] t/test_lib: avoid naked bash arrays in file_lineno
From: Johannes Schindelin <hidden>
Date: 2020-05-07 19:52:22
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:
quoted hunk ↗ jump to hunk
Helped-by: Johannes Schindelin [off-list ref] Signed-off-by: Carlo Marcelo Arenas Belón <redacted> --- 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