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

On Thu, May 07, 2020 at 09:52:12PM +0200, Johannes Schindelin wrote:

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

quoted

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:

FWIW that was the intention, but luckily Junio quickly predicted it was
most likely buggy and so has been since made obsolete by:

  https://lore.kernel.org/git/20200507175706.19986-1-carenas@gmail.com/ (local)

quoted

+	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.

as I said originally, I thought it would had been even better to be specific
up to the line number of the instruction that failed, after all we already
have the whole function code for context in the message.

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

I couldn't figure out why that was needed, but while I am just slow, this
one is actually genius!; you are using a shell parameter expansion (which
is even POSIX sh compatible) to do string concatenation!

conditionally, noneless and not even using a silly if (like I would have
done) or a bash += concatenation operator.

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.

fair enough; but in my defense, I couldn't had been made aware this was
being used also for debugging, or called without the parameter or even
called in a way where it will report its own LINENO (which you clearly
explained before as being the wrong value to report and that lead to
my confused first attempt at porting it)

thanks again, for your insightful feedback, and guess then there is no
more point on trying to simplify it further, and if you could ACK my
other proposal, hopefully neither a need to find workarounds to running
tests on NetBSD for the current master.

if we decide later into improving the accuracy of the report, might be
worth documenting some of what I learned in the commit message, for the
future reviewers, who hopefully will be less thick headed.

Carlo

PS. I know I botched the v2 git send-email, so you didn't get a CC, or even
    have the v2 in the subject, hope it would be still good enough but let
    me know otherwise.