Re: [PATCH v5 10/12] tests: when run in Bash, annotate test failures with file name/line number

From: "Carlo Marcelo Arenas Belón" <carenas@gmail.com>
To: Danh Doan <congdanhqx@gmail.com>
Cc: git@vger.kernel.org, Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [PATCH v5 10/12] tests: when run in Bash, annotate test failures with file name/line number
Date: Mon, 4 May 2020 17:54:05 -0700	[thread overview]
Message-ID: <20200505005405.GA1218@Carlos-MBP> (raw)
In-Reply-To: <20200504232511.GB29599@danh.dev>

On Tue, May 05, 2020 at 06:25:11AM +0700, Danh Doan wrote:
> On 2020-05-04 10:46:36-0700, Carlo Marcelo Arenas Belón <carenas@gmail.com> wrote:
> > On Sat, Apr 11, 2020 at 12:18:12AM +0700, Đoàn Trần Công Danh wrote:
> > > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > > index 0ea1e5a05e..40a00983f7 100644
> > > --- a/t/test-lib.sh
> > > +++ b/t/test-lib.sh
> > > @@ -657,6 +657,18 @@ die () {
> > >  	fi
> > >  }
> > >  
> > > +file_lineno () {
> > > +	test -z "$GIT_TEST_FRAMEWORK_SELFTEST" && test -n "$BASH" || return 0
> > > +	local i
> > > +	for i in ${!BASH_SOURCE[*]}
> > 
> > this line breaks with NetBSD's sh (and probably other POSIX complaint shells)
> > 
> > the Coding Guidelines mention "no shell arrays" and while the tests are more
> > relaxed against that rule, usually workarounds are needed, as it is shown by:
> > 5826b7b595 (test-lib: check Bash version for '-x' without using shell arrays,
> > 2019-01-03)
> 
> This function will be called in CI only, and when the the shell used
> is bash, to annotate the faulty line.

that part is correct, but the test is meant to be able to also run outside
of CI (specially considering that there are several systems that are not
covered by it yet)

> We have a test guarding it already.
> So, I think it's fine.

no; because the shell has to parse the whole script (regardless if it
executes the line or not), so if you checkout master in a NetBSD machine
and try to run the tests you get :

** t0000-basic.sh ***
./test-lib.sh: 681: Syntax error: Bad substitution
gmake[2]: *** [Makefile:56: t0000-basic.sh] Error 2

the coding guidelines warns specifically against using arrays for this
reason, and therefore it will be ideal that we follow such guidelines
by rewriting the code not to use shell arrays (which is not a POSIX sh
feature) as it affects not only NetBSD but anyone else not running bash.

but there is an "easy" workaround as shown by the reference I sent, so a
naive and very ugly "fix" would be:

--- >8 ---

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1b221951a8..b6b7175dfe 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -677,6 +677,7 @@ die () {
 
 file_lineno () {
 	test -z "$GIT_TEST_FRAMEWORK_SELFTEST" && test -n "$BASH" || return 0
+	eval '
 	local i
 	for i in ${!BASH_SOURCE[*]}
 	do
@@ -685,6 +686,7 @@ file_lineno () {
 		*,t[0-9]*.sh) echo "t/${BASH_SOURCE[$i]}:${BASH_LINENO[$(($i-1))]}: ${1+$1: }"; return;;
 		esac
 	done
+	'
 }
 
 GIT_EXIT_OK=
