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: Junio C Hamano <gitster@pobox.com>
Cc: Danh Doan <congdanhqx@gmail.com>,
	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: Wed, 6 May 2020 00:30:45 -0700	[thread overview]
Message-ID: <20200506073045.GA52959@Carlos-MBP> (raw)
In-Reply-To: <xmqqlfm7cj7s.fsf@gitster.c.googlers.com>

On Mon, May 04, 2020 at 05:35:03PM -0700, Junio C Hamano wrote:
> Danh Doan <congdanhqx@gmail.com> writes:
> 
> >> > +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)
> >>  ...
> > This function will be called in CI only, and when the the shell used
> > is bash, to annotate the faulty line.
> >
> > We have a test guarding it already.
> 
> Carlo, you said "this line breaks"---implying that you actually saw
> breakage when running our tests on affected platforms.  Is that the
> case?

correct. and as described in the followup email[1]

> It is entirely possible that some shell implementations may try to
> parse the contents of the shell function and trigger a syntax error
> on that line, even before evaluationg "are we running tests and do
> we have BASH set?", so I can believe "we have a test guarding", if
> the mention refers to the first line of the helper function, is
> insufficient and does break with some shells.

it would seem that a POSIX sh version would be enough for what we
need and as shown after the scissors.

got away also with the IMHO unnecesary rewriting of the script path
and hardcoding of "t/" as it looks cleaner.

Carlo
--- >8 ---
Subject: [PATCH] tests: make file_lineno() POSIX sh compatible

662f9cf154 (tests: when run in Bash, annotate test failures with file
name/line number, 2020-04-11), introduces a way to report the exact
location where an error was triggered.

The implementation was using bash specific syntax and was restricted
with a guard so it will only be used with bash, but NetBSD sh will
still have to parse the function and the use of arrays triggers a
syntax error as shown by:

** t0000-basic.sh ***
./test-lib.sh: 681: Syntax error: Bad substitution

Instead of guarding the code, use a simpler version written in POSIX
sh as per the Coding Guidelines.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 t/test-lib.sh | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1b221951a8..a8f8e4106b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -676,15 +676,9 @@ 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
+	test -z "$GIT_TEST_FRAMEWORK_SELFTEST" || return 0
+
+	echo "$0:$LINENO: ${1+$1: }"
 }
 
 GIT_EXIT_OK=
-- 
2.26.2.686.gfaf46a9ccd

