Re: [PATCH v2 3/3] test-lib.sh: support -x option for shell-tracing

From: Jeff King <peff@peff.net>
Date: Oct 13, 2014, 22:43 UTC
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:

Show 28 quoted lines

> > 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