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@xxxxxx>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@xxxxxxxxx>
> ---
> 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
>
>
