Re: [PATCH] {master} Tests: normalize use of the 'errexit' shell flag.

[Stefano Lattarini]

On Monday 03 January 2011, Ralf Wildenhues wrote:
> Hi Stefano,
> 
> * Stefano Lattarini wrote on Mon, Jan 03, 2011 at 03:04:05PM CET:
> > * tests/maken3.test (check_targets): Remove redundant call to
> > 'set -e'.
> > * tests/maken4.test: Likewise.
> > * tests/ansi5.test: Call 'set -e' just after './defs' has been
> > sourced.
> > * tests/ansi6.test: Likewise.
> > * tests/ansi7.test: Likewise.
> > * tests/cond16.test: Likewise.
> > * tests/cond17.test: Likewise.
> > * tests/cond18.test: Likewise.
> > * tests/cond19.test: Likewise.
> > * tests/cond20.test: Likewise.
> > * tests/cond21.test: Likewise.
> > * tests/instdat2.test: Likewise.
> > * tests/instdir-texi.test: Likewise.
> > * tests/parallel-tests3.test: Likewise.
> > * tests/remake1a.test: Likewise.
> > * tests/ccnoco.test: Likewise, and add trailing `:' command.
> > * tests/comment4.test: Likewise.
> > * tests/gcj4.test: Likewise.
> > * tests/nodist2.test: Likewise.
> > * tests/nodist3.test: Enable 'errexit' shell flag (this should
> > have been done in commit v1.11-248-g317e17b, but the relevant
> > hunk has been forgotten somehow).
> > * tests/output.test: Likewise.
> > * tests/gnits2.test: Likewise, and display captured stderr to
> > script's stderr, not to script's stdout.
> > * tests/gnits3.test: Likewise.  Also, prefer 'cat' over 'echo'
> > to append to Makefile.am, and really check that the exit status
> > of "make installcheck" indicates failure.
> [...]
> > --- a/tests/maken3.test
> > +++ b/tests/maken3.test
> 
> > @@ -122,7 +122,6 @@ $AUTOCONF
> >  
> >  check_targets ()
> >  {
> > -  set -e
> >    for target in \
> >      all install install-strip uninstall clean distclean check \
> >      info html dvi pdf ps \
> 
> > --- a/tests/maken4.test
> > +++ b/tests/maken4.test
> 
> > @@ -124,7 +124,6 @@ $AUTOCONF
> >  
> >  check_targets ()
> >  {
> > -  set -e
> >    for target in \
> >      all install install-strip uninstall clean distclean check \
> >      info html dvi pdf ps \
> 
> These two were added by me in fear of shells resetting errexit upon
> function entry (like some shells do with -x aka xtrace).  Luckily I
> couldn't find any shells which do this upon testing (by looking for 'e'
> in the output of)
>   sh -ec 'f () { echo $-; }; f'
> 
> but generally, here's a nice (and more complete than autoconf.texi) list
> of issues with set -e: http://www.in-ulm.de/~mascheck/various/set-e/
> 
> Please, before removing such seemingly superfluous code, consider
> first whether it may have been added for a reason.
>
> If you do this research, then I don't have to, and can review more
> patches.
> 
> Patch is OK.
> 
> Thanks,
> Ralf
> 
JFTR: I considered the removal of those `set -e' just obvious because all
the other testcases which use shell functions to do their checks do not
bother to call `set -e' upon entering such functions.  Moreover, `git diff'
showed me that those `set -e' calls in maken{3,4}.test had been there from
the very first version of the tests, which made clear that they had not
been added in response to a real failure or to some weird shell bug.

Regards,
  Stefano