Re: [PATCH v2 1/2] t1510: remove need for "test_untraceable", retain coverage

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Feb 6, 2022, 21:40 UTC
Message-ID: <20220206214024.GC1936@szeder.dev>
In-Reply-To: <YbMiK1wHzBfYvK2a@coredump.intra.peff.net>

On Fri, Dec 10, 2021 at 04:47:23AM -0500, Jeff King wrote:

Show 18 quoted lines

> On Thu, Dec 02, 2021 at 08:16:35PM +0100, SZEDER Gábor wrote:
> 
> > > @@ -62,7 +59,7 @@ test_repo () {
> > >  			export GIT_WORK_TREE
> > >  		fi &&
> > >  		rm -f trace &&
> > > -		GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null &&
> > > +		GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null 2>>stderr &&
> > 
> > I suspect that it's lines like this that make Peff argue for
> > BASH_XTRACEFD :)
> > 
> > While this is not a compound command, it does contain a command
> > substitution, and the trace generated when executing the command in
> > that command substitution goes to the command's stderr, and then,
> > because of the redirection, to the 'stderr' file.
> 
> Better still, the behavior varies between shells:

Indeed, although POSIX seems to be quite clear about what should happen in this case: the specs for simple commands [1] state that redirections should be performed before variable assignments are expanded for, among other things, command substitution.

Show 12 quoted lines

>   $ bash -c 'set -x; FOO=$(echo foo) echo main >stdout 2>stderr; set +x; grep . stdout stderr'
>   ++ echo foo
>   + FOO=foo
>   + echo main
>   + set +x
>   stdout:main
> 
>   $ dash -c 'set -x; FOO=$(echo foo) echo main >stdout 2>stderr; set +x; grep . stdout stderr'
>   + FOO=foo echo main
>   + set +x
>   stdout:main
>   stderr:+ echo foo

So in case of these commands the shell should first redirect stdout and stderr, then expand the command substitution, thus it should write the trace for the 'echo foo' within to stderr _while_ stderr is redirected. It seems that dash, for once, does conform to POSIX.

Although the standard-conform behavior is potentially more problematic for us, because it would cause test failure with tracing enabled when used like this:

  GIT_TRACE="$(pwd)/trace" git cmd --opts 2>actual.err &&
  test_cmp expected.err actual.err

because the "+ pwd" trace output from the command substitution would go to 'err.actual'; 9b2ac68f27 (t5526: use $TRASH_DIRECTORY to specify the path of GIT_TRACE log file, 2018-02-24) fixed a case like this.

[1] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01