Re: [PATCH 2/2] test: cope better with use of return for errors

[Jeff King <peff@peff.net>]

On Tue, Aug 09, 2011 at 10:46:34AM +0200, Johannes Sixt wrote:

> Am 8/8/2011 3:17, schrieb Jonathan Nieder:
> > +test_eval_ () {
> > +	# This is a separate function because some tests use
> > +	# "return" to end a test_expect_success block early.
> > +	eval >&3 2>&4 "$*"
> > +}
> > +
> >  test_run_ () {
> >  	test_cleanup=:
> >  	expecting_failure=$2
> > -	eval >&3 2>&4 "$1"
> > +	test_eval_ "$1"
> >  	eval_ret=$?
> >  
> >  	if test -z "$immediate" || test $eval_ret = 0 || test -n "$expecting_failure"
> >  	then
> > -		eval >&3 2>&4 "$test_cleanup"
> > +		test_eval_ "$test_cleanup"
> >  	fi
> >  	if test "$verbose" = "t" && test -n "$HARNESS_ACTIVE"; then
> >  		echo ""
> 
> This invalidates at least t3900.29, which accesses an unexpanded $3
> from the test script. The patch below fixes this case.

Hmm. Isn't t3900 already broken, even without this patch? It does
something like this:

  foo() {
    test_expect_success 'bar' 'echo $3'
  }

That can't possibly access the third positional parameter of foo, as we
are already inside the test_expect_success function, no matter what
functions we call after that.

If there is any regression here, it would be that we used to get the
positional parameters of test_run_, and now we get them from test_eval_.
So accessing "$2" used to get us $expecting_failure, but now gets
nothing. I doubt it matters, and tests would be insane to rely on
something internal like that. If it did, the right fix would be:

  -       test_eval_ "$1"
  +       test_eval_ "$@"

in test_run_.

So I don't see a problem in Jonathan's patches. But clearly t3900 is
poorly written and should be fixed.

But:

> diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
> index c06a5ee..3265fac 100755
> --- a/t/t3900-i18n-commit.sh
> +++ b/t/t3900-i18n-commit.sh
> @@ -136,6 +136,7 @@ done
>  test_commit_autosquash_flags () {
>  	H=$1
>  	flag=$2
> +	mopt=$3
>  	test_expect_success "commit --$flag with $H encoding" '
>  		git config i18n.commitencoding $H &&
>  		git checkout -b $H-$flag C0 &&
> @@ -147,7 +148,7 @@ test_commit_autosquash_flags () {
>  		git commit -a -m "intermediate commit" &&
>  		test_tick &&
>  		echo $H $flag >>F &&
> -		git commit -a --$flag HEAD~1 $3 &&
> +		git commit -a --$flag HEAD~1 $mopt &&
>  		E=$(git cat-file commit '$H-$flag' |
>  			sed -ne "s/^encoding //p") &&
>  		test "z$E" = "z$H" &&
> @@ -160,6 +161,6 @@ test_commit_autosquash_flags () {
>  
>  test_commit_autosquash_flags eucJP fixup
>  
> -test_commit_autosquash_flags ISO-2022-JP squash '-m "squash message"'
> +test_commit_autosquash_flags ISO-2022-JP squash '-m squash_message'

Can't we just drop $3 here? I don't see how it's actually doing
anything. It makes us call:

  git commit --squash HEAD~1 -m squash_message

instead of just:

  git commit --squash HEAD~1

but then we never actually check to see that the included message
becomes part of the squashed commit, anyway.

Also, a side note. These tests put their shell snippet inside single
quotes. And then access the variables sometimes directly (assuming they
are left unchanged inside the test functions), and sometimes using
single quotes. Which actually places them outside the snippet's single
quotes, and expands them in place. Which happens to be OK because they
don't contain any spaces, but would otherwise pass extra arguments to
test_expect_success.

So it's not a bug, but it does look unintentional and poor style.

Anyway. Your patch is fine and sufficient to solve the problem. Those
were all just thoughtst I had while trying to figure out what the test
was actually trying to do.

-Peff