On Wed, May 06, 2020 at 04:33:47PM +0200, Johannes Schindelin wrote:
> On Wed, 6 May 2020, Carlo Marcelo Arenas Belón wrote:
> > On Wed, May 06, 2020 at 02:54:38PM +0200, Johannes Schindelin wrote:
> > > On Wed, 6 May 2020, Carlo Marcelo Arenas Belón wrote:
> > > > 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: }"
> > >
> > > That suppresses the error all right.
> > >
> > > Unfortunately, it completely breaks the feature. At that point, `$LINENO`
> > > is either unset (e.g. in `dash`) or it contains the number of the line
> > > _containing the `echo`. That is totally useless information at this point,
> > > we want the line number of the caller.
> >
> > that seems like a bug in dash, which NetBSD sh doesn't have, as LINENO
> > wouldn't be unset.
>
> And your patch makes this a real problem as you no longer skip the `echo`
> in the non-Bash case.
FWIW, at least NetBSD's dash (0.5.10.2) always sets LINENO
> > > Try this, for example:
> > >
> > > ```
> > > #!/bin/sh
> > >
> > > file_lineno () {
> > > echo "$0:$LINENO: hello"
> > > }
> > >
> > > file_lineno
> > > ```
> > >
> > > When you run this, it will print `4`. What we want is `7`.
> >
> > so you need instead :
> >
> > ```
> > #!/bin/sh
> >
> > file_lineno () {
> > echo "$0:$1: hello"
> > }
> >
> > file_lineno $LINENO
>
> No.
>
> Please understand what the intention of the current (Bash-specific) code
> is: in case that there is a failure, it needs to print out the file and
> line number of the actual statement that caused that problem.
that is what I got from the commmit message but then I was puzzled not
to find the line inside the test case that faile but instead the one
where the last test function was located.
eitherway, I was careless to send the previous version without checking
more deeply; indeed the use of LINENO in the original code was part of
that confusion and is therefore gone in this new one.
> Take this example:
>
> test_expect_success 'For Carlo' '
> false
> '
>
> Obviously, this will fail, and it will print out an error message. What we
> want here is that the file that contains that `test_expect_success` and
> the actual line number of this call are printed.
>
> Your suggestion would be to clutter each and every such call with
> `$LINENO`, like so:
>
> test_expect_success $LINENO 'For Carlo' '
>
> I don't think that is sensible an idea.
>
> Besides, it would _still_ not work, for parameterized functions that call
> `test_expect_success` and that are defined in `lib-<whatever>.sh`.
>
> Example:
>
> # in t/lib-whatever.sh
> super_repetitive_test () {
> test_expect_success "first $1" '
> ...
> '
>
> test_expect_success "second $1" '
> ...
> '
>
> ...
>
> test_expect_success "gazillionth $1" '
> ...
> '
> }
>
> # in t/t1234-actual-caller.sh
> . lib-whatever.sh
>
> super_repetitive_test hello
> super_repetitive_test world
> super_repetitive_test good-bye
> super_repetitive_test dreams
>
> We will not want to print out the line number of the call in
> t/lib-whatever.sh. That is what your proposal would amount to, unless you
> want to clutter even the `super_repetitive_test` calles, which would fly
> even less.
that was very useful to understand better the constrain that was referred
in the commit message and that forced to build a bash specific solution.
I have to admit too, I was confused by the code, but with the clear guidance
you provided I think the new proposed[1] version is also clearer, and goes
away with the problem that triggered this whole discussion.
Carlo
[1] https://lore.kernel.org/git/20200507055118.69971-1-carenas@xxxxxxxxx/
