Hi Carlo,
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.
That's what I wanted to point out: this needs to be fixed.
> > 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.
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.
> > Even worse, as `$0` does _not_ contain `test-lib.sh` at this point,
> > the printed information is totally bogus.
>
> not sure I understand what you mean here, at least when runnning with bash
> the original code shows $0 correctly as t????.sh when I tried to force a
> failure to test.
Yes, $0 shows the correct file. But since the line number that is printed
is from a totally different file, the combination <file>:<lineno> is
completely and utterly bogus. Misleading. Less than useless.
Ciao,
Dscho
