From: Jeff King <peff@peff.net>
Date: Mon, 13 Oct 2014 22:43:43 GMT
Subject: Re: [PATCH v2 3/3] test-lib.sh: support -x option for shell-tracing
Message-ID: <20141013224342.GA22290@peff.net>
In-Reply-To: <20141013223303.GA17045@peff.net>

On Mon, Oct 13, 2014 at 06:33:03PM -0400, Jeff King wrote:

> > But still it is disturbing to see that there is a blank line
> > difference with and without this change in the file created by the
> > test (i.e. the client of the code this patch touches).
> 
> This fixes it:
> 
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 4dab575..059bb25 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -528,8 +528,7 @@ maybe_setup_valgrind () {
>  test_eval_inner_ () {
>  	eval "
>  		test \"$trace\" = t && set -x
> -		$*
> -	"
> +		$*"
>  }
>  
>  test_eval_ () {
> 
> 
> My patch definitely expands the snippet with an extra trailing newline.
> But what I really don't understand is why that would impact the
> _contents_ of the config file.
> 
> I'll dig further, but I'm about to leave the computer for dinner for a
> few hours, so please don't hold your breath. :)

OK, I lied. I couldn't resist spending 5 more minutes on it.

If you instrument t1308 on master to look at the contents of .git/config
directly after the setup step, you'll see that the file ends with (tabs
marked as ^I):

  [...]
  ^I^Ihorns
  ^IEOF

Which makes sense. We forgot the tab-eating "<<-" in the here-doc, so
the tab-indented EOF was not counted as the end of the input. So this
test is bogus and broken, and the breakage introduced by my patch is
only triggered because of that (which isn't to say we shouldn't
necessarily adjust my patch, but we definitely should fix this test).

What really surprises me is that the shell is fine with a here-doc
ending inside an eval. Bash at least warns:

  $ bash -c "eval 'cat <<EOF
    content'"
  bash: line 2: warning: here-document at line 1 delimited by end-of-file (wanted `EOF')
      content

but dash silently accepts it:
  $ dash -c "eval 'cat <<EOF
    content'"
      content

Maybe this is something that every shell does, but it certainly seems
like something we should not be relying on (and it was definitely not
something the test meant to rely on, as evidenced by the bogus EOF
marker it included).

-Peff