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

Jeff King [off-list ref] writes:

On Fri, Oct 10, 2014 at 02:21:56AM -0400, Jeff King wrote:

quoted

diff --git a/t/test-lib.sh b/t/test-lib.sh
index a60ec75..81ceb23 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -237,7 +237,11 @@ do
 		shift ;;
 	-x)
 		test_eval_start_='set -x'
-		test_eval_end_='set +x'
+		test_eval_end_='
+			set +x
+			test "$test_eval_ret_" = 0 ||
+			  say_color error >&4 "last command exited with \$?=$?"

That should be \$?=$test_eval_ret_, of course. The patch below fixes it.

Rerolled patch is below. Sorry for all the emails. I'll stop looking at
it now to give you guys a chance to find any remaining mistakes. ;)

Does 1308 pass with this patch for you (running it without "-x")?

The original that expects a hardcoded line number (not relative to
the original or something) is a bad taste, and also the test setup
procedure is broken (see below for a fix of that breakage, which
does not fix the breakage this patch seems to bring in anyway).

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

 t/t1308-config-set.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index ea0bce2..462bb64 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh

@@ -23,7 +23,7 @@ check_config () {
 }
 
 test_expect_success 'setup default config' '
-	cat >.git/config <<\EOF
+	cat >.git/config <<-\EOF
 	[case]
 		penguin = very blue
 		Movie = BadPhysics

On Mon, Oct 13, 2014 at 03:22:50PM -0700, Junio C Hamano wrote:

quoted

Rerolled patch is below. Sorry for all the emails. I'll stop looking at
it now to give you guys a chance to find any remaining mistakes. ;)

Does 1308 pass with this patch for you (running it without "-x")?

Hmph. It does not. I know that "make test" passed with an earlier
iteration, but I must have gotten so wrapped up in testing "make
GIT_TEST_OPTS=-x test" that I never ran a vanilla "make test" on
what I finally posted. Sorry.

The original that expects a hardcoded line number (not relative to
the original or something) is a bad taste, and also the test setup
procedure is broken (see below for a fix of that breakage, which
does not fix the breakage this patch seems to bring in anyway).

Yeah, I agree, and your patch below looks reasonable.

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. :)

-Peff

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

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