[PATCH 0/3] making test-suite tracing more useful

[PATCH 0/3] making test-suite tracing more useful

[Jeff King]

I sometimes run git's test suite as part of an automated testing
process. I was hoping to add "-x" support to get more details when a
test fails (since failures are sometimes hard to reproduce). But I hit a
few small snags:

  - you have to run with bash, since BASH_XTRACEFD is required to avoid
    failures in some tests when we capture the stderr of shell functions
    or subshells (which get polluted with the "set -x" outupt). This
    requirement isn't a big deal for me, but it showed some other
    issues.

  - the output with BASH_XTRACEFD is a little confusing; fixed by patch
    1.

  - there's one test that _only_ fails with BASH_XTRACEFD. That's fixed
    in patch 2.

  - I wanted to use "prove" since the output of "make -j64 test" is
    unreadable. But that means using "--verbose-log", which was
    incompatible with "-x". That's patch 3.

With these patches, I can now do:

  make -j64 test \
    SHELL_PATH=/bin/bash \
    GIT_TEST_OPTS="--verbose-log -x" \
    DEFAULT_TEST_TARGET=prove \
    GIT_PROVE_OPTS=-j64 || {
          echo "Failing tests:"
          echo "--------------"
	  grep -l '[^0]' t/test-results/*.exit |
	  while read failed; do
	          base=${failed%.exit}
		  name=${base#t/test-results/}
		  echo "==> $name"
		  cat "$base.out"
	  done
	  exit 1
    }

and get fairly readable output (a nice summary from prove, and then a
dump of any failing test output).

Johannes, I've seen that you do "-x" in the tests that the
git-for-windows bot uses to comment on GitHub. You may have seen the
bogus failure in t5615, which this series should fix (you may also have
seen the "set +x" cruft at the end of each test, which is fixed here,
too).

Lars, I think with this it should be possible to turn on "-x" for the
Travis build.

  [1/3]: test-lib: silence "-x" cleanup under bash
  [2/3]: t5615: avoid re-using descriptor 4
  [3/3]: test-lib: make "-x" work with "--verbose-log"

 t/t5615-alternate-env.sh |  6 +++---
 t/test-lib.sh            | 44 ++++++++++++++++++++++++++++++++------------
 2 files changed, 35 insertions(+), 15 deletions(-)

-Peff

[PATCH 1/3] test-lib: silence "-x" cleanup under bash

[Jeff King]

When the test suite's "-x" option is used with bash, we end
up seeing cleanup cruft in the output:

  $ bash t0001-init.sh -x
  [...]
  ++ diff -u expected actual
  + test_eval_ret_=0
  + want_trace
  + test t = t
  + test t = t
  + set +x
  ok 42 - re-init from a linked worktree

This ranges from mildly annoying (for a successful test) to
downright confusing (when we say "last command exited with
error", but it's really 5 commands back).

We normally are able to suppress this cleanup. As the
in-code comment explains, we can't convince the shell not to
print it, but we can redirect its stderr elsewhere.

But since d88785e424 (test-lib: set BASH_XTRACEFD
automatically, 2016-05-11), that doesn't hold for bash. It
sends the "set -x" output directly to descriptor 4, not to
stderr.

We can fix this by also redirecting descriptor 4, and
paying close attention to which commands redirect and
which do not (see the updated comment).

Two alternatives I considered and rejected:

  - unsetting and setting BASH_XTRACEFD; doing so closes the
    descriptor, which we must avoid

  - we could keep everything in a single block as before,
    redirect 4>/dev/null there, but retain 5>&4 as a copy.
    And then selectively restore 4>&5 for commands which
    should be allowed to trace. This would work, but the
    descriptor swapping seems unnecessarily confusing.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/test-lib.sh | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9b61f16f7a..3399fc3c88 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -600,26 +600,40 @@ test_eval_inner_ () {
 }
 
 test_eval_ () {
-	# We run this block with stderr redirected to avoid extra cruft
-	# during a "-x" trace. Once in "set -x" mode, we cannot prevent
+	# If "-x" tracing is in effect, then we want to avoid polluting stderr
+	# with non-test commands. But once in "set -x" mode, we cannot prevent
 	# the shell from printing the "set +x" to turn it off (nor the saving
 	# of $? before that). But we can make sure that the output goes to
 	# /dev/null.
 	#
-	# The test itself is run with stderr put back to &4 (so either to
-	# /dev/null, or to the original stderr if --verbose was used).
+	# There are a few subtleties here:
+	#
+	#   - we have to redirect descriptor 4 in addition to 2, to cover
+	#     BASH_XTRACEFD
+	#
+	#   - the actual eval has to come before the redirection block (since
+	#     it needs to see descriptor 4 to set up its stderr)
+	#
+	#   - likewise, any error message we print must be outside the block to
+	#     access descriptor 4
+	#
+	#   - checking $? has to come immediately after the eval, but it must
+	#     be _inside_ the block to avoid polluting the "set -x" output
+	#
+
+	test_eval_inner_ "$@" </dev/null >&3 2>&4
 	{
-		test_eval_inner_ "$@" </dev/null >&3 2>&4
 		test_eval_ret_=$?
 		if want_trace
 		then
 			set +x
-			if test "$test_eval_ret_" != 0
-			then
-				say_color error >&4 "error: last command exited with \$?=$test_eval_ret_"
-			fi
 		fi
-	} 2>/dev/null
+	} 2>/dev/null 4>&2
+
+	if test "$test_eval_ret_" != 0 && want_trace
+	then
+		say_color error >&4 "error: last command exited with \$?=$test_eval_ret_"
+	fi
 	return $test_eval_ret_
 }
 
-- 
2.15.0.rc1.560.g5f0609e481

[PATCH 2/3] t5615: avoid re-using descriptor 4

[Jeff King]

File descriptors 3 and 4 are special in our test suite, as
they link back to the test script's original stdout and
stderr. Normally this isn't something tests need to worry
about: they are free to clobber these descriptors for
sub-commands without affecting the overall script.

But there's one very special thing about descriptor 4: since
d88785e424 (test-lib: set BASH_XTRACEFD automatically,
2016-05-11), we ask bash to output "set -x" output to it by
number. This goes to _any_ descriptor 4, even if it no
longer points to the place it did when we set BASH_XTRACEFD.

But in t5615, we run a shell loop with descriptor 4
redirected.  As a result, t5615 works with non-bash shells
even with "-x". And it works with bash without "-x". But the
combination of "bash t5615-alternate-env.sh -x" gets a test
failure (because our "set -x" output pollutes one of the
files).

We can fix this by using any descriptor _except_ the magical
4. So let's switch arbitrarily to using 5/6 in this loop,
not 3/4.

Signed-off-by: Jeff King <peff@peff.net>
---
I also considered trying to bump the "set -x" output descriptor to "9".
That just moves the problem around, but presumably scripts are less
likely to go that high. :)

It would also be possible to pick something insanely high, like "999".
Many shells choke on descriptors higher than 9, but since the issue is
related to BASH_XTRACEFD, we could make it a bash-only thing. I don't
know if it's worth the trouble. I hate leaving a subtle "don't use
descriptor 4 in a subshell or your script will break" hand-grenade like
this lying around, but we do seem to have only one instance of it over
the whole test suite.

 t/t5615-alternate-env.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t5615-alternate-env.sh b/t/t5615-alternate-env.sh
index d2d883f3a1..b4905b822c 100755
--- a/t/t5615-alternate-env.sh
+++ b/t/t5615-alternate-env.sh
@@ -7,9 +7,9 @@ check_obj () {
 	alt=$1; shift
 	while read obj expect
 	do
-		echo "$obj" >&3 &&
-		echo "$obj $expect" >&4
-	done 3>input 4>expect &&
+		echo "$obj" >&5 &&
+		echo "$obj $expect" >&6
+	done 5>input 6>expect &&
 	GIT_ALTERNATE_OBJECT_DIRECTORIES=$alt \
 		git "$@" cat-file --batch-check='%(objectname) %(objecttype)' \
 		<input >actual &&
-- 
2.15.0.rc1.560.g5f0609e481

Re: [PATCH 2/3] t5615: avoid re-using descriptor 4

[Stefan Beller]

> I also considered trying to bump the "set -x" output descriptor to "9".
> That just moves the problem around, but presumably scripts are less
> likely to go that high. :)
>
> It would also be possible to pick something insanely high, like "999".
> Many shells choke on descriptors higher than 9, but since the issue is
> related to BASH_XTRACEFD, we could make it a bash-only thing. I don't
> know if it's worth the trouble. I hate leaving a subtle "don't use
> descriptor 4 in a subshell or your script will break" hand-grenade like
> this lying around, but we do seem to have only one instance of it over
> the whole test suite.

I would imagine that a higher fd for BASH_XTRACEFD
would be less explosive than requiring the tests to skip
the low number 4. (It is not *that* low. In e.g. git-submodule.sh
we make use of 3 in cmd_foreach, not needing more.)

Thanks,
Stefan

Re: [PATCH 2/3] t5615: avoid re-using descriptor 4

[Jeff King]

On Thu, Oct 19, 2017 at 02:46:33PM -0700, Stefan Beller wrote:

> > I also considered trying to bump the "set -x" output descriptor to "9".
> > That just moves the problem around, but presumably scripts are less
> > likely to go that high. :)
> >
> > It would also be possible to pick something insanely high, like "999".
> > Many shells choke on descriptors higher than 9, but since the issue is
> > related to BASH_XTRACEFD, we could make it a bash-only thing. I don't
> > know if it's worth the trouble. I hate leaving a subtle "don't use
> > descriptor 4 in a subshell or your script will break" hand-grenade like
> > this lying around, but we do seem to have only one instance of it over
> > the whole test suite.
> 
> I would imagine that a higher fd for BASH_XTRACEFD
> would be less explosive than requiring the tests to skip
> the low number 4. (It is not *that* low. In e.g. git-submodule.sh
> we make use of 3 in cmd_foreach, not needing more.)

So one trick is that we can't just set it to a higher number. We have to
also open and manage that descriptor. It might be enough to do:

  if test -n "$BASH_VERSION"
  then
	exec 999>&4
	BASH_XTRACEFD=999
  fi

but since we fiddle with descriptors on a per-test basis, I'm not sure
if that's it or not. I think for convincing output to go to it, it
probably is. We tend to point things _at_ descriptor 4, not point
descriptor 4 elsewhere. But the fix in patch 1 is an exception, where we
try to suppress the "set +x" output. For that we have to redirect 999,
too (which is hard because ">&999" is syntactically invalid in other
shells, so we have to follow a separate code path for bash or get into
evals).

Or start playing games with unsetting BASH_XTRACEFD (which closes 999,
and then we re-open 999>&4 and reset XTRACEFD).

I think it might be workable, but I'm worried we're opening a can of
worms. Or continuing to dig into the can of worms from the original
BASH_XTRACEFD, if you prefer. :)

-Peff

Re: [PATCH 2/3] t5615: avoid re-using descriptor 4

[Jeff King]

On Thu, Oct 19, 2017 at 07:23:37PM -0400, Jeff King wrote:

> So one trick is that we can't just set it to a higher number. We have to
> also open and manage that descriptor. It might be enough to do:
> 
>   if test -n "$BASH_VERSION"
>   then
> 	exec 999>&4
> 	BASH_XTRACEFD=999
>   fi
> [...]
> I think it might be workable, but I'm worried we're opening a can of
> worms. Or continuing to dig into the can of worms from the original
> BASH_XTRACEFD, if you prefer. :)

So this is the best I came up with (on top of my other patches for
ease of reading, though I'd re-work them if this is the route we're
going to go):

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 14fac6d6f2..833ceaefd2 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -377,7 +377,12 @@ fi
 #
 # Note also that we don't need or want to export it. The tracing is local to
 # this shell, and we would not want to influence any shells we exec.
-BASH_XTRACEFD=4
+if test -n "$BASH_VERSION"
+then
+	exec 999>&4
+	BASH_XTRACEFD=999
+	silence_trace="999>/dev/null"
+fi
 
 test_failure=0
 test_count=0
@@ -627,14 +632,16 @@ test_eval_ () {
 	#     be _inside_ the block to avoid polluting the "set -x" output
 	#
 
-	test_eval_inner_ "$@" </dev/null >&3 2>&4
-	{
-		test_eval_ret_=$?
-		if want_trace
-		then
-			set +x
-		fi
-	} 2>/dev/null 4>&2
+	eval '
+		test_eval_inner_ "$@" </dev/null >&3 2>&4
+		{
+			test_eval_ret_=$?
+			if want_trace
+			then
+				set +x
+			fi
+		} 2>/dev/null '"$silence_trace"'
+	'
 
 	if test "$test_eval_ret_" != 0 && want_trace
 	then

I really hate that extra eval, since it adds an extra layer of "+" to
the tracing output in bash.

But I can't find a way to make it work without it. The "999>/dev/null"
is syntactically bogus in non-bash shells, so it _must_ appear as part
of an eval.  We can't conditionally stick it at the end of the eval run
by test_eval_inner_, because that one is running the actual test code
(so it may bail early with "return", for example).

TBH, I'm not 100% sure that the initial "exec 999>&4" won't cause
complaints in some shells. Dash seems to handle it, but others might
not. That can be worked around with another eval, though.

So I dunno. It does solve the problem in a way that the individual test
scripts wouldn't have to care about. But it's a lot of eval trickery.

-Peff

Re: [PATCH 2/3] t5615: avoid re-using descriptor 4

[Stefan Beller]

On Thu, Oct 19, 2017 at 10:50 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Oct 19, 2017 at 07:23:37PM -0400, Jeff King wrote:
>
>> So one trick is that we can't just set it to a higher number. We have to
>> also open and manage that descriptor. It might be enough to do:
>>
>>   if test -n "$BASH_VERSION"
>>   then
>>       exec 999>&4
>>       BASH_XTRACEFD=999
>>   fi
>> [...]
>> I think it might be workable, but I'm worried we're opening a can of
>> worms. Or continuing to dig into the can of worms from the original
>> BASH_XTRACEFD, if you prefer. :)
>
> So this is the best I came up with (on top of my other patches for
> ease of reading, though I'd re-work them if this is the route we're
> going to go):
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 14fac6d6f2..833ceaefd2 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -377,7 +377,12 @@ fi
>  #
>  # Note also that we don't need or want to export it. The tracing is local to
>  # this shell, and we would not want to influence any shells we exec.
> -BASH_XTRACEFD=4
> +if test -n "$BASH_VERSION"
> +then
> +       exec 999>&4
> +       BASH_XTRACEFD=999
> +       silence_trace="999>/dev/null"
> +fi
>
>  test_failure=0
>  test_count=0
> @@ -627,14 +632,16 @@ test_eval_ () {
>         #     be _inside_ the block to avoid polluting the "set -x" output
>         #
>
> -       test_eval_inner_ "$@" </dev/null >&3 2>&4
> -       {
> -               test_eval_ret_=$?
> -               if want_trace
> -               then
> -                       set +x
> -               fi
> -       } 2>/dev/null 4>&2
> +       eval '
> +               test_eval_inner_ "$@" </dev/null >&3 2>&4
> +               {
> +                       test_eval_ret_=$?
> +                       if want_trace
> +                       then
> +                               set +x
> +                       fi
> +               } 2>/dev/null '"$silence_trace"'
> +       '
>
>         if test "$test_eval_ret_" != 0 && want_trace
>         then
>
> I really hate that extra eval, since it adds an extra layer of "+" to
> the tracing output in bash.
..
> So I dunno. It does solve the problem in a way that the individual test
> scripts wouldn't have to care about. But it's a lot of eval trickery.
>

I agree. Maybe just stick with the original patch?

Thanks,
Stefan

Re: [PATCH 2/3] t5615: avoid re-using descriptor 4

[Jeff King]

On Fri, Oct 20, 2017 at 02:27:40PM -0700, Stefan Beller wrote:

> > So I dunno. It does solve the problem in a way that the individual test
> > scripts wouldn't have to care about. But it's a lot of eval trickery.
> 
> I agree. Maybe just stick with the original patch?

OK. Why don't we live with that for now, then. The only advantage of the
"999" trickery is that it's less likely to come up again. If it doesn't,
then we're happy. If it does, then we can always switch then.

I also noticed that our GIT_TRACE=4 trickery has the same problem (it
didn't trigger in the t5615 case because it doesn't actually run git
inside the subshell). If we end up doing a "999" descriptor eventually,
we should probably point GIT_TRACE at it, too.

-Peff

Re: [PATCH 2/3] t5615: avoid re-using descriptor 4

[Simon Ruderich]

On Fri, Oct 20, 2017 at 06:46:08PM -0400, Jeff King wrote:
>> I agree. Maybe just stick with the original patch?
>
> OK. Why don't we live with that for now, then. The only advantage of the
> "999" trickery is that it's less likely to come up again. If it doesn't,
> then we're happy. If it does, then we can always switch then.

I think switching the 4 to 9 (which you already brought up in
this thread) is a good idea. It makes accidental conflicts less
likely (it's rare to use so many file descriptors) and is easy to
implement.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

Re: [PATCH 2/3] t5615: avoid re-using descriptor 4

[Junio C Hamano]

Simon Ruderich <simon@ruderich.org> writes:

> On Fri, Oct 20, 2017 at 06:46:08PM -0400, Jeff King wrote:
>>> I agree. Maybe just stick with the original patch?
>>
>> OK. Why don't we live with that for now, then. The only advantage of the
>> "999" trickery is that it's less likely to come up again. If it doesn't,
>> then we're happy. If it does, then we can always switch then.
>
> I think switching the 4 to 9 (which you already brought up in
> this thread) is a good idea. It makes accidental conflicts less
> likely (it's rare to use so many file descriptors) and is easy to
> implement.

Yeah, I like the simplicity of implementation, and I more like the
fact that it is simpler to reason about its limitation.

Re: [PATCH 2/3] t5615: avoid re-using descriptor 4

[Jeff King]

On Sat, Oct 21, 2017 at 02:19:16AM +0200, Simon Ruderich wrote:

> On Fri, Oct 20, 2017 at 06:46:08PM -0400, Jeff King wrote:
> >> I agree. Maybe just stick with the original patch?
> >
> > OK. Why don't we live with that for now, then. The only advantage of the
> > "999" trickery is that it's less likely to come up again. If it doesn't,
> > then we're happy. If it does, then we can always switch then.
> 
> I think switching the 4 to 9 (which you already brought up in
> this thread) is a good idea. It makes accidental conflicts less
> likely (it's rare to use so many file descriptors) and is easy to
> implement.

I'm not sure it does make accidental conflicts less likely. Grepping for
'9>' shows a problematic one in t0008, and one in t9300. That's two
versus the one for "4". :)

We often use descriptors 8 or 9 as "high and not taken for any specific
use" in our tests, and do things like:

  mkfifo foo
  exec 9>foo
  ...
  exec 9>&-

This is unlike a redirection in a sub-command (like "foo 9>bar") because
it effects the whole test suite's state. So it would break the test
under "-x" (because we'd get random cruft sent to the fifo), as well as
breaking the "-x" output itself (because we close the descriptor).

I actually think "7" is the safest descriptor right now. It's not used
for anything, and it's not high enough for tests to think "this probably
isn't used for anything".

-Peff

[PATCH 3/3] test-lib: make "-x" work with "--verbose-log"

[Jeff King]

The "-x" tracing option implies "--verbose". This is a
problem when running under a TAP harness like "prove", where
we need to use "--verbose-log" instead. Instead, let's
handle this the same way we do for --valgrind, including the
recent fix from 88c6e9d31c (test-lib: --valgrind should not
override --verbose-log, 2017-09-05). Namely, let's enable
--verbose only when we there isn't a more specific verbosity
option indicated.

Note that we also have to tweak `want_trace` to turn it on
(we re-check $verbose in each test because that's how we
handle --verbose-only, but --verbose-log is always on for
all tests).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/test-lib.sh | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 3399fc3c88..14fac6d6f2 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -263,7 +263,6 @@ do
 		shift ;;
 	-x)
 		trace=t
-		verbose=t
 		shift ;;
 	--verbose-log)
 		verbose_log=t
@@ -282,6 +281,11 @@ then
 	test -z "$verbose_log" && verbose=t
 fi
 
+if test -n "$trace" && test -z "$verbose_log"
+then
+	verbose=t
+fi
+
 if test -n "$color"
 then
 	# Save the color control sequences now rather than run tput
@@ -585,7 +589,9 @@ maybe_setup_valgrind () {
 }
 
 want_trace () {
-	test "$trace" = t && test "$verbose" = t
+	test "$trace" = t && {
+		test "$verbose" = t || test "$verbose_log" = t
+	}
 }
 
 # This is a separate function because some tests use
-- 
2.15.0.rc1.560.g5f0609e481

Re: [PATCH 3/3] test-lib: make "-x" work with "--verbose-log"

[Johannes Schindelin]

Hi Peff,

On Thu, 19 Oct 2017, Jeff King wrote:

> [...]
> override --verbose-log, 2017-09-05). Namely, let's enable
> --verbose only when we there isn't a more specific verbosity
> option indicated.

s/we there/we know that there/

Thanks,
Dscho

[PATCH 4/3] t/Makefile: introduce TEST_SHELL_PATH

[Jeff King]

On Thu, Oct 19, 2017 at 05:01:40PM -0400, Jeff King wrote:

> I sometimes run git's test suite as part of an automated testing
> process. I was hoping to add "-x" support to get more details when a
> test fails (since failures are sometimes hard to reproduce). But I hit a
> few small snags:
> 
>   - you have to run with bash, since BASH_XTRACEFD is required to avoid
>     failures in some tests when we capture the stderr of shell functions
>     or subshells (which get polluted with the "set -x" outupt). This
>     requirement isn't a big deal for me, but it showed some other
>     issues.

Actually, this did lead me to one more fix.

I was building with SHELL_PATH=/bin/bash to make BASH_XTRACEFD work. But
that impacts not only the test scripts, but also the build itself.
I'd prefer to test something closer to my normal builds (which use
bin/sh). This patch lets me do:

  make \
    TEST_SHELL_PATH=/bin/bash \
    GIT_TEST_OPTS="--verbose-log -x" \
    test

to run the whole test suite using /bin/sh for the build, for getting the
benefits of bash's "-x" tracing.

-- >8 --
Subject: [PATCH] t/Makefile: introduce TEST_SHELL_PATH

You may want to run the test suite with a different shell
than you use to build Git. For instance, you may build with
SHELL_PATH=/bin/sh (because it's faster, or it's what you
expect to exist on systems where the build will be used) but
want to run the test suite with bash (e.g., since that
allows using "-x" reliably across the whole test suite).
There's currently no good way to do this.

You might think that doing two separate make invocations,
like:

  make &&
  make -C t SHELL_PATH=/bin/bash

would work. And it _almost_ does. The second make will see
our bash SHELL_PATH, and we'll use that to run the
individual test scripts (or tell prove to use it to do so).
So far so good.

But this breaks down when "--tee" or "--verbose-log" is
used. Those options cause the test script to actually
re-exec itself using $SHELL_PATH. But wait, wouldn't our
second make invocation have set SHELL_PATH correctly in the
environment?

Yes, but test-lib.sh sources GIT-BUILD-OPTIONS, which we
built during the first "make". And that overrides the
environment, giving us the original SHELL_PATH again.

Let's introduce a new variable that lets you specify a
specific shell to be run for the test scripts. Note that we
have to touch both the main and t/ Makefiles, since we have
to record it in GIT-BUILD-OPTIONS in one, and use it in the
latter.

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile      | 8 ++++++++
 t/Makefile    | 6 ++++--
 t/test-lib.sh | 2 +-
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index cd75985991..9baa3c4b50 100644
--- a/Makefile
+++ b/Makefile
@@ -425,6 +425,10 @@ all::
 #
 # to say "export LESS=FRX (and LV=-c) if the environment variable
 # LESS (and LV) is not set, respectively".
+#
+# Define TEST_SHELL_PATH if you want to use a shell besides SHELL_PATH for
+# running the test scripts (e.g., bash has better support for "set -x"
+# tracing).
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -727,6 +731,8 @@ endif
 export PERL_PATH
 export PYTHON_PATH
 
+TEST_SHELL_PATH = $(SHELL_PATH)
+
 LIB_FILE = libgit.a
 XDIFF_LIB = xdiff/lib.a
 VCSSVN_LIB = vcs-svn/lib.a
@@ -1721,6 +1727,7 @@ prefix_SQ = $(subst ','\'',$(prefix))
 gitwebdir_SQ = $(subst ','\'',$(gitwebdir))
 
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
+TEST_SHELL_PATH_SQ = $(subst ','\'',$(TEST_SHELL_PATH))
 PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
 PYTHON_PATH_SQ = $(subst ','\'',$(PYTHON_PATH))
 TCLTK_PATH_SQ = $(subst ','\'',$(TCLTK_PATH))
@@ -2350,6 +2357,7 @@ GIT-LDFLAGS: FORCE
 # and the first level quoting from the shell that runs "echo".
 GIT-BUILD-OPTIONS: FORCE
 	@echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' >$@+
+	@echo TEST_SHELL_PATH=\''$(subst ','\'',$(TEST_SHELL_PATH_SQ))'\' >$@+
 	@echo PERL_PATH=\''$(subst ','\'',$(PERL_PATH_SQ))'\' >>$@+
 	@echo DIFF=\''$(subst ','\'',$(subst ','\'',$(DIFF)))'\' >>$@+
 	@echo PYTHON_PATH=\''$(subst ','\'',$(PYTHON_PATH_SQ))'\' >>$@+
diff --git a/t/Makefile b/t/Makefile
index 1bb06c36f2..96317a35f4 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -8,6 +8,7 @@
 
 #GIT_TEST_OPTS = --verbose --debug
 SHELL_PATH ?= $(SHELL)
+TEST_SHELL_PATH ?= $(SHELL_PATH)
 PERL_PATH ?= /usr/bin/perl
 TAR ?= $(TAR)
 RM ?= rm -f
@@ -23,6 +24,7 @@ endif
 
 # Shell quote;
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
+TEST_SHELL_PATH_SQ = $(subst ','\'',$(TEST_SHELL_PATH))
 PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
 TEST_RESULTS_DIRECTORY_SQ = $(subst ','\'',$(TEST_RESULTS_DIRECTORY))
 
@@ -42,11 +44,11 @@ failed:
 	test -z "$$failed" || $(MAKE) $$failed
 
 prove: pre-clean $(TEST_LINT)
-	@echo "*** prove ***"; $(PROVE) --exec '$(SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
+	@echo "*** prove ***"; $(PROVE) --exec '$(TEST_SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
 	$(MAKE) clean-except-prove-cache
 
 $(T):
-	@echo "*** $@ ***"; '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
+	@echo "*** $@ ***"; '$(TEST_SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
 
 pre-clean:
 	$(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)'
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 14fac6d6f2..e373ad1e12 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -80,7 +80,7 @@ done,*)
 	# from any previous runs.
 	>"$GIT_TEST_TEE_OUTPUT_FILE"
 
-	(GIT_TEST_TEE_STARTED=done ${SHELL_PATH} "$0" "$@" 2>&1;
+	(GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} "$0" "$@" 2>&1;
 	 echo $? >"$BASE.exit") | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
 	test "$(cat "$BASE.exit")" = 0
 	exit
-- 
2.15.0.rc2.500.g047c67c30e

Re: [PATCH 4/3] t/Makefile: introduce TEST_SHELL_PATH

[SZEDER Gábor]

> On Thu, Oct 19, 2017 at 05:01:40PM -0400, Jeff King wrote:
> 
> > I sometimes run git's test suite as part of an automated testing
> > process. I was hoping to add "-x" support to get more details when a
> > test fails (since failures are sometimes hard to reproduce).

Would it make sense (or be feasible) to enable "-x" on Travis CI?


> diff --git a/Makefile b/Makefile
> index cd75985991..9baa3c4b50 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -425,6 +425,10 @@ all::
>  #
>  # to say "export LESS=FRX (and LV=-c) if the environment variable
>  # LESS (and LV) is not set, respectively".
> +#
> +# Define TEST_SHELL_PATH if you want to use a shell besides SHELL_PATH for
> +# running the test scripts (e.g., bash has better support for "set -x"
> +# tracing).
>  
>  GIT-VERSION-FILE: FORCE
>  	@$(SHELL_PATH) ./GIT-VERSION-GEN
> @@ -727,6 +731,8 @@ endif
>  export PERL_PATH
>  export PYTHON_PATH
>  
> +TEST_SHELL_PATH = $(SHELL_PATH)
> +
>  LIB_FILE = libgit.a
>  XDIFF_LIB = xdiff/lib.a
>  VCSSVN_LIB = vcs-svn/lib.a
> @@ -1721,6 +1727,7 @@ prefix_SQ = $(subst ','\'',$(prefix))
>  gitwebdir_SQ = $(subst ','\'',$(gitwebdir))
>  
>  SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
> +TEST_SHELL_PATH_SQ = $(subst ','\'',$(TEST_SHELL_PATH))
>  PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
>  PYTHON_PATH_SQ = $(subst ','\'',$(PYTHON_PATH))
>  TCLTK_PATH_SQ = $(subst ','\'',$(TCLTK_PATH))
> @@ -2350,6 +2357,7 @@ GIT-LDFLAGS: FORCE
>  # and the first level quoting from the shell that runs "echo".
>  GIT-BUILD-OPTIONS: FORCE
>  	@echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' >$@+
> +	@echo TEST_SHELL_PATH=\''$(subst ','\'',$(TEST_SHELL_PATH_SQ))'\' >$@+

This redirection overwrites, loosing the just written SHELL_PATH.  It
should append like the lines below.

>  	@echo PERL_PATH=\''$(subst ','\'',$(PERL_PATH_SQ))'\' >>$@+
>  	@echo DIFF=\''$(subst ','\'',$(subst ','\'',$(DIFF)))'\' >>$@+
>  	@echo PYTHON_PATH=\''$(subst ','\'',$(PYTHON_PATH_SQ))'\' >>$@+


Gábor

Re: [PATCH 4/3] t/Makefile: introduce TEST_SHELL_PATH

[Jeff King]

On Sat, Oct 21, 2017 at 01:50:01AM +0200, SZEDER Gábor wrote:

> > On Thu, Oct 19, 2017 at 05:01:40PM -0400, Jeff King wrote:
> > 
> > > I sometimes run git's test suite as part of an automated testing
> > > process. I was hoping to add "-x" support to get more details when a
> > > test fails (since failures are sometimes hard to reproduce).
> 
> Would it make sense (or be feasible) to enable "-x" on Travis CI?

Yes, after this series I think it may be workable to do so.

> > @@ -2350,6 +2357,7 @@ GIT-LDFLAGS: FORCE
> >  # and the first level quoting from the shell that runs "echo".
> >  GIT-BUILD-OPTIONS: FORCE
> >  	@echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' >$@+
> > +	@echo TEST_SHELL_PATH=\''$(subst ','\'',$(TEST_SHELL_PATH_SQ))'\' >$@+
> 
> This redirection overwrites, loosing the just written SHELL_PATH.  It
> should append like the lines below.

Doh, thank you. It's interesting that nothing broke with this error. But
I think when run via Make that we end up with SHELL_PATH via the
environment (set to the default /bin/sh, which was suitable for my
system).

I'll fix it.

-Peff

Re: [PATCH 4/3] t/Makefile: introduce TEST_SHELL_PATH

[Johannes Schindelin]

Hi Peff,

On Fri, 20 Oct 2017, Jeff King wrote:

> @@ -2350,6 +2357,7 @@ GIT-LDFLAGS: FORCE
>  # and the first level quoting from the shell that runs "echo".
>  GIT-BUILD-OPTIONS: FORCE
>  	@echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' >$@+
> +	@echo TEST_SHELL_PATH=\''$(subst ','\'',$(TEST_SHELL_PATH_SQ))'\' >$@+

Do we really want to force the test shell path to be hardcoded at runtime?
It may be a better idea not to write this into GIT-BUILD-OPTIONS.

Or alternatively we could prefix the assignment by

	test -n "$TEST_SHELL_PATH" ||

or use the pattern

	TEST_SHELL_PATH="${TEST_SHELL_PATH:-[...]}"

Ciao,
Dscho

Re: [PATCH 4/3] t/Makefile: introduce TEST_SHELL_PATH

[Jeff King]

On Mon, Oct 23, 2017 at 01:01:42PM +0200, Johannes Schindelin wrote:

> On Fri, 20 Oct 2017, Jeff King wrote:
> 
> > @@ -2350,6 +2357,7 @@ GIT-LDFLAGS: FORCE
> >  # and the first level quoting from the shell that runs "echo".
> >  GIT-BUILD-OPTIONS: FORCE
> >  	@echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' >$@+
> > +	@echo TEST_SHELL_PATH=\''$(subst ','\'',$(TEST_SHELL_PATH_SQ))'\' >$@+
> 
> Do we really want to force the test shell path to be hardcoded at runtime?
> It may be a better idea not to write this into GIT-BUILD-OPTIONS.

My intent was to make it work "out of the box" in the same way as
SHELL_PATH does now. So that:

  echo TEST_SHELL_PATH=whatever >>config.mak
  make test
  cd t && ./t1234-*

both respect it. Without going through BUILD-OPTIONS, I don't think it
makes it into the environment via config.mak (it _does_ if you specify
it on the command-line of "make", though).

For my purposes it would be fine to just use the environment, but I was
trying to have it match the other variables for consistency.

> Or alternatively we could prefix the assignment by
> 
> 	test -n "$TEST_SHELL_PATH" ||
> 
> or use the pattern
> 
> 	TEST_SHELL_PATH="${TEST_SHELL_PATH:-[...]}"

I'm not quite sure what this is fixing.  Is there a case where we
wouldn't have TEST_SHELL_PATH set when running the tests? I think there
are already other bits that assume that "make" has been run (including
the existing reference to $SHELL_PATH, I think).

-Peff

Re: [PATCH 4/3] t/Makefile: introduce TEST_SHELL_PATH

[Johannes Schindelin]

Hi Peff,

On Mon, 23 Oct 2017, Jeff King wrote:

> On Mon, Oct 23, 2017 at 01:01:42PM +0200, Johannes Schindelin wrote:
> 
> > On Fri, 20 Oct 2017, Jeff King wrote:
> > 
> > > @@ -2350,6 +2357,7 @@ GIT-LDFLAGS: FORCE
> > >  # and the first level quoting from the shell that runs "echo".
> > >  GIT-BUILD-OPTIONS: FORCE
> > >  	@echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' >$@+
> > > +	@echo TEST_SHELL_PATH=\''$(subst ','\'',$(TEST_SHELL_PATH_SQ))'\' >$@+
> > 
> > Do we really want to force the test shell path to be hardcoded at runtime?
> > It may be a better idea not to write this into GIT-BUILD-OPTIONS.
> 
> My intent was to make it work "out of the box" in the same way as
> SHELL_PATH does now. So that:
> 
>   echo TEST_SHELL_PATH=whatever >>config.mak
>   make test
>   cd t && ./t1234-*
> 
> both respect it. Without going through BUILD-OPTIONS, I don't think it
> makes it into the environment via config.mak (it _does_ if you specify
> it on the command-line of "make", though).
> 
> For my purposes it would be fine to just use the environment, but I was
> trying to have it match the other variables for consistency.

Makes sense.

> > Or alternatively we could prefix the assignment by
> > 
> > 	test -n "$TEST_SHELL_PATH" ||
> > 
> > or use the pattern
> > 
> > 	TEST_SHELL_PATH="${TEST_SHELL_PATH:-[...]}"
> 
> I'm not quite sure what this is fixing.  Is there a case where we
> wouldn't have TEST_SHELL_PATH set when running the tests? I think there
> are already other bits that assume that "make" has been run (including
> the existing reference to $SHELL_PATH, I think).

The way I read your patch, setting the environment variable differnently
at test time than at build time would be ignored: GIT-BUILD-OPTIONS is
sourced and would override whatever you told the test suite to use.

I guess it does not really matter all that much in practice.

Ciao,
Dscho

Re: [PATCH 4/3] t/Makefile: introduce TEST_SHELL_PATH

[Jeff King]

On Wed, Oct 25, 2017 at 11:35:44PM +0200, Johannes Schindelin wrote:

> > > Or alternatively we could prefix the assignment by
> > > 
> > > 	test -n "$TEST_SHELL_PATH" ||
> > > 
> > > or use the pattern
> > > 
> > > 	TEST_SHELL_PATH="${TEST_SHELL_PATH:-[...]}"
> > 
> > I'm not quite sure what this is fixing.  Is there a case where we
> > wouldn't have TEST_SHELL_PATH set when running the tests? I think there
> > are already other bits that assume that "make" has been run (including
> > the existing reference to $SHELL_PATH, I think).
> 
> The way I read your patch, setting the environment variable differnently
> at test time than at build time would be ignored: GIT-BUILD-OPTIONS is
> sourced and would override whatever you told the test suite to use.
> 
> I guess it does not really matter all that much in practice.

Right. I find that behavior mildly irritating at times, but it's
consistent with other items like NO_PERL, etc. E.g., you cannot do:

  make NO_PERL=
  cd t
  NO_PERL=Nope ./t3701-*

and disable perl. It's testing what got built.

-Peff

Re: [PATCH 4/3] t/Makefile: introduce TEST_SHELL_PATH

[Johannes Schindelin]

Hi Peff,

On Wed, 25 Oct 2017, Jeff King wrote:

> On Wed, Oct 25, 2017 at 11:35:44PM +0200, Johannes Schindelin wrote:
> 
> > > > Or alternatively we could prefix the assignment by
> > > > 
> > > > 	test -n "$TEST_SHELL_PATH" ||
> > > > 
> > > > or use the pattern
> > > > 
> > > > 	TEST_SHELL_PATH="${TEST_SHELL_PATH:-[...]}"
> > > 
> > > I'm not quite sure what this is fixing.  Is there a case where we
> > > wouldn't have TEST_SHELL_PATH set when running the tests? I think there
> > > are already other bits that assume that "make" has been run (including
> > > the existing reference to $SHELL_PATH, I think).
> > 
> > The way I read your patch, setting the environment variable differnently
> > at test time than at build time would be ignored: GIT-BUILD-OPTIONS is
> > sourced and would override whatever you told the test suite to use.
> > 
> > I guess it does not really matter all that much in practice.
> 
> Right. I find that behavior mildly irritating at times, but it's
> consistent with other items like NO_PERL, etc. E.g., you cannot do:
> 
>   make NO_PERL=
>   cd t
>   NO_PERL=Nope ./t3701-*
> 
> and disable perl. It's testing what got built.

The difference between NO_PERL and TEST_SHELL_PATH, of course, is that
NO_PERL affects the build and the test phase, while TEST_SHELL_PATH really
has no impact whatsoever on the build phase.

Ciao,
Dscho

Re: [PATCH 0/3] making test-suite tracing more useful

[Johannes Schindelin]

Hi Peff,

On Thu, 19 Oct 2017, Jeff King wrote:

> I sometimes run git's test suite as part of an automated testing
> process. I was hoping to add "-x" support to get more details when a
> test fails (since failures are sometimes hard to reproduce).

Thank you for working on this.

> Johannes, I've seen that you do "-x" in the tests that the
> git-for-windows bot uses to comment on GitHub. You may have seen the
> bogus failure in t5615, which this series should fix (you may also have
> seen the "set +x" cruft at the end of each test, which is fixed here,
> too).

Surprisingly enough, I did not see any test failure in that test. I do
re-run failed tests after the complete test run to make sure that the
failure is not due to any resource scarcity, and that re-run is performed
with -x (to double as documentation what really went wrong).

Ciao,
Dscho

Re: [PATCH 0/3] making test-suite tracing more useful

[Jeff King]

On Mon, Oct 23, 2017 at 01:04:45PM +0200, Johannes Schindelin wrote:

> > Johannes, I've seen that you do "-x" in the tests that the
> > git-for-windows bot uses to comment on GitHub. You may have seen the
> > bogus failure in t5615, which this series should fix (you may also have
> > seen the "set +x" cruft at the end of each test, which is fixed here,
> > too).
> 
> Surprisingly enough, I did not see any test failure in that test. I do
> re-run failed tests after the complete test run to make sure that the
> failure is not due to any resource scarcity, and that re-run is performed
> with -x (to double as documentation what really went wrong).

Ah, OK, that explains it. If t5615 never failed in the non-x run, then you
wouldn't have run into it. So it was waiting to bite you, but that test
script is simple enough that it never failed. :)

-Peff

[PATCH v2 0/4] making test-suite tracing more useful

[Jeff King]

This series fixes some rough edges in the "-x" feature of the test
suite. With it, it should be possible to turn on tracing for CI runs.

This is mostly a repost of v1 at:

  https://public-inbox.org/git/20171019210140.64lb52cqtgdh22ew@sigill.intra.peff.net

which had some discussion, but wasn't picked up.

I fixed a few typos pointed out by reviewers, and I tried to summarize
the discussion around "magic descriptor 4" in the commit message of
patch 2.  My conclusion there was that none of the options is
particularly good.  The only thing thing that might make sense is making
"7" the magical descriptor. But given that "4" only had one troubling
call, I'm inclined to just leave it as-is and see if this ever comes up
again (especially if we start using "-x" in the Travis builds, then we'd
catch any problems).

  [1/4]: test-lib: silence "-x" cleanup under bash
  [2/4]: t5615: avoid re-using descriptor 4
  [3/4]: test-lib: make "-x" work with "--verbose-log"
  [4/4]: t/Makefile: introduce TEST_SHELL_PATH

 Makefile                 |  8 ++++++++
 t/Makefile               |  6 ++++--
 t/t5615-alternate-env.sh |  6 +++---
 t/test-lib.sh            | 46 +++++++++++++++++++++++++++++++++-------------
 4 files changed, 48 insertions(+), 18 deletions(-)

-Peff

[PATCH v2 1/4] test-lib: silence "-x" cleanup under bash

[Jeff King]

When the test suite's "-x" option is used with bash, we end
up seeing cleanup cruft in the output:

  $ bash t0001-init.sh -x
  [...]
  ++ diff -u expected actual
  + test_eval_ret_=0
  + want_trace
  + test t = t
  + test t = t
  + set +x
  ok 42 - re-init from a linked worktree

This ranges from mildly annoying (for a successful test) to
downright confusing (when we say "last command exited with
error", but it's really 5 commands back).

We normally are able to suppress this cleanup. As the
in-code comment explains, we can't convince the shell not to
print it, but we can redirect its stderr elsewhere.

But since d88785e424 (test-lib: set BASH_XTRACEFD
automatically, 2016-05-11), that doesn't hold for bash. It
sends the "set -x" output directly to descriptor 4, not to
stderr.

We can fix this by also redirecting descriptor 4, and
paying close attention to which commands redirected and
which are not (see the updated comment).

Two alternatives I considered and rejected:

  - unsetting and setting BASH_XTRACEFD; doing so closes the
    descriptor, which we must avoid

  - we could keep everything in a single block as before,
    redirect 4>/dev/null there, but retain 5>&4 as a copy.
    And then selectively restore 4>&5 for commands which
    should be allowed to trace. This would work, but the
    descriptor swapping seems unnecessarily confusing.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/test-lib.sh | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 116bd6a70c..7914453a3b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -601,26 +601,40 @@ test_eval_inner_ () {
 }
 
 test_eval_ () {
-	# We run this block with stderr redirected to avoid extra cruft
-	# during a "-x" trace. Once in "set -x" mode, we cannot prevent
+	# If "-x" tracing is in effect, then we want to avoid polluting stderr
+	# with non-test commands. But once in "set -x" mode, we cannot prevent
 	# the shell from printing the "set +x" to turn it off (nor the saving
 	# of $? before that). But we can make sure that the output goes to
 	# /dev/null.
 	#
-	# The test itself is run with stderr put back to &4 (so either to
-	# /dev/null, or to the original stderr if --verbose was used).
+	# There are a few subtleties here:
+	#
+	#   - we have to redirect descriptor 4 in addition to 2, to cover
+	#     BASH_XTRACEFD
+	#
+	#   - the actual eval has to come before the redirection block (since
+	#     it needs to see descriptor 4 to set up its stderr)
+	#
+	#   - likewise, any error message we print must be outside the block to
+	#     access descriptor 4
+	#
+	#   - checking $? has to come immediately after the eval, but it must
+	#     be _inside_ the block to avoid polluting the "set -x" output
+	#
+
+	test_eval_inner_ "$@" </dev/null >&3 2>&4
 	{
-		test_eval_inner_ "$@" </dev/null >&3 2>&4
 		test_eval_ret_=$?
 		if want_trace
 		then
 			set +x
-			if test "$test_eval_ret_" != 0
-			then
-				say_color error >&4 "error: last command exited with \$?=$test_eval_ret_"
-			fi
 		fi
-	} 2>/dev/null
+	} 2>/dev/null 4>&2
+
+	if test "$test_eval_ret_" != 0 && want_trace
+	then
+		say_color error >&4 "error: last command exited with \$?=$test_eval_ret_"
+	fi
 	return $test_eval_ret_
 }
 
-- 
2.15.1.659.g8bd2eae3ea

[PATCH v2 2/4] t5615: avoid re-using descriptor 4

[Jeff King]

File descriptors 3 and 4 are special in our test suite, as
they link back to the test script's original stdout and
stderr. Normally this isn't something tests need to worry
about: they are free to clobber these descriptors for
sub-commands without affecting the overall script.

But there's one very special thing about descriptor 4: since
d88785e424 (test-lib: set BASH_XTRACEFD automatically,
2016-05-11), we ask bash to output "set -x" output to it by
number. This goes to _any_ descriptor 4, even if it no
longer points to the place it did when we set BASH_XTRACEFD.

But in t5615, we run a shell loop with descriptor 4
redirected.  As a result, t5615 works with non-bash shells
even with "-x". And it works with bash without "-x". But the
combination of "bash t5615-alternate-env.sh -x" gets a test
failure (because our "set -x" output pollutes one of the
files).

We can fix this by using any descriptor _except_ the magical
4. So let's switch arbitrarily to using 5/6 in this loop,
not 3/4.

Another alternative is to use a different descriptor for
BASH_XTRACEFD. But picking an unused one turns out to be
hard. Most shells limit us to 9 numbered descriptors. Bash
can handle more, but:

  - while the BASH_XTRACEFD is specific to bash, GIT_TRACE=4
    has a similar problem, and would affect all shells

  - constructs like "999>/dev/null" are synticatically
    invalid to non-bash shells. So we have to actually bury
    it inside an eval, which creates more complications.

Of the numbers 1-9, you might think that "9" would be less
used than "4". But it's not; many of our scripts use
descriptors 8 and 9 (probably under the assumption that they
are high and therefore unused). The least-used descriptor is
currently "7". We could switch to that, but we're just
trading one magic number for another.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5615-alternate-env.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t5615-alternate-env.sh b/t/t5615-alternate-env.sh
index d2d883f3a1..b4905b822c 100755
--- a/t/t5615-alternate-env.sh
+++ b/t/t5615-alternate-env.sh
@@ -7,9 +7,9 @@ check_obj () {
 	alt=$1; shift
 	while read obj expect
 	do
-		echo "$obj" >&3 &&
-		echo "$obj $expect" >&4
-	done 3>input 4>expect &&
+		echo "$obj" >&5 &&
+		echo "$obj $expect" >&6
+	done 5>input 6>expect &&
 	GIT_ALTERNATE_OBJECT_DIRECTORIES=$alt \
 		git "$@" cat-file --batch-check='%(objectname) %(objecttype)' \
 		<input >actual &&
-- 
2.15.1.659.g8bd2eae3ea

[PATCH v2 3/4] test-lib: make "-x" work with "--verbose-log"

[Jeff King]

The "-x" tracing option implies "--verbose". This is a
problem when running under a TAP harness like "prove", where
we need to use "--verbose-log" instead. Instead, let's
handle this the same way we do for --valgrind, including the
recent fix from 88c6e9d31c (test-lib: --valgrind should not
override --verbose-log, 2017-09-05). Namely, let's enable
--verbose only when we know there isn't a more specific
verbosity option indicated.

Note that we also have to tweak `want_trace` to turn it on
(previously we just lumped $verbose_log in with $verbose,
but now we don't necessarily auto-set the latter).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/test-lib.sh | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 7914453a3b..b8dd5e79ac 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -264,7 +264,6 @@ do
 		shift ;;
 	-x)
 		trace=t
-		verbose=t
 		shift ;;
 	--verbose-log)
 		verbose_log=t
@@ -283,6 +282,11 @@ then
 	test -z "$verbose_log" && verbose=t
 fi
 
+if test -n "$trace" && test -z "$verbose_log"
+then
+	verbose=t
+fi
+
 if test -n "$color"
 then
 	# Save the color control sequences now rather than run tput
@@ -586,7 +590,9 @@ maybe_setup_valgrind () {
 }
 
 want_trace () {
-	test "$trace" = t && test "$verbose" = t
+	test "$trace" = t && {
+		test "$verbose" = t || test "$verbose_log" = t
+	}
 }
 
 # This is a separate function because some tests use
-- 
2.15.1.659.g8bd2eae3ea

[PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH

[Jeff King]

You may want to run the test suite with a different shell
than you use to build Git. For instance, you may build with
SHELL_PATH=/bin/sh (because it's faster, or it's what you
expect to exist on systems where the build will be used) but
want to run the test suite with bash (e.g., since that
allows using "-x" reliably across the whole test suite).
There's currently no good way to do this.

You might think that doing two separate make invocations,
like:

  make &&
  make -C t SHELL_PATH=/bin/bash

would work. And it _almost_ does. The second make will see
our bash SHELL_PATH, and we'll use that to run the
individual test scripts (or tell prove to use it to do so).
So far so good.

But this breaks down when "--tee" or "--verbose-log" is
used. Those options cause the test script to actually
re-exec itself using $SHELL_PATH. But wait, wouldn't our
second make invocation have set SHELL_PATH correctly in the
environment?

Yes, but test-lib.sh sources GIT-BUILD-OPTIONS, which we
built during the first "make". And that overrides the
environment, giving us the original SHELL_PATH again.

Let's introduce a new variable that lets you specify a
specific shell to be run for the test scripts. Note that we
have to touch both the main and t/ Makefiles, since we have
to record it in GIT-BUILD-OPTIONS in one, and use it in the
latter.

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile      | 8 ++++++++
 t/Makefile    | 6 ++++--
 t/test-lib.sh | 2 +-
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index fef9c8d272..8a21c4d8f1 100644
--- a/Makefile
+++ b/Makefile
@@ -425,6 +425,10 @@ all::
 #
 # to say "export LESS=FRX (and LV=-c) if the environment variable
 # LESS (and LV) is not set, respectively".
+#
+# Define TEST_SHELL_PATH if you want to use a shell besides SHELL_PATH for
+# running the test scripts (e.g., bash has better support for "set -x"
+# tracing).
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -729,6 +733,8 @@ endif
 export PERL_PATH
 export PYTHON_PATH
 
+TEST_SHELL_PATH = $(SHELL_PATH)
+
 LIB_FILE = libgit.a
 XDIFF_LIB = xdiff/lib.a
 VCSSVN_LIB = vcs-svn/lib.a
@@ -1725,6 +1731,7 @@ prefix_SQ = $(subst ','\'',$(prefix))
 gitwebdir_SQ = $(subst ','\'',$(gitwebdir))
 
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
+TEST_SHELL_PATH_SQ = $(subst ','\'',$(TEST_SHELL_PATH))
 PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
 PYTHON_PATH_SQ = $(subst ','\'',$(PYTHON_PATH))
 TCLTK_PATH_SQ = $(subst ','\'',$(TCLTK_PATH))
@@ -2355,6 +2362,7 @@ GIT-LDFLAGS: FORCE
 # and the first level quoting from the shell that runs "echo".
 GIT-BUILD-OPTIONS: FORCE
 	@echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' >$@+
+	@echo TEST_SHELL_PATH=\''$(subst ','\'',$(TEST_SHELL_PATH_SQ))'\' >>$@+
 	@echo PERL_PATH=\''$(subst ','\'',$(PERL_PATH_SQ))'\' >>$@+
 	@echo DIFF=\''$(subst ','\'',$(subst ','\'',$(DIFF)))'\' >>$@+
 	@echo PYTHON_PATH=\''$(subst ','\'',$(PYTHON_PATH_SQ))'\' >>$@+
diff --git a/t/Makefile b/t/Makefile
index 1bb06c36f2..96317a35f4 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -8,6 +8,7 @@
 
 #GIT_TEST_OPTS = --verbose --debug
 SHELL_PATH ?= $(SHELL)
+TEST_SHELL_PATH ?= $(SHELL_PATH)
 PERL_PATH ?= /usr/bin/perl
 TAR ?= $(TAR)
 RM ?= rm -f
@@ -23,6 +24,7 @@ endif
 
 # Shell quote;
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
+TEST_SHELL_PATH_SQ = $(subst ','\'',$(TEST_SHELL_PATH))
 PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
 TEST_RESULTS_DIRECTORY_SQ = $(subst ','\'',$(TEST_RESULTS_DIRECTORY))
 
@@ -42,11 +44,11 @@ failed:
 	test -z "$$failed" || $(MAKE) $$failed
 
 prove: pre-clean $(TEST_LINT)
-	@echo "*** prove ***"; $(PROVE) --exec '$(SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
+	@echo "*** prove ***"; $(PROVE) --exec '$(TEST_SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
 	$(MAKE) clean-except-prove-cache
 
 $(T):
-	@echo "*** $@ ***"; '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
+	@echo "*** $@ ***"; '$(TEST_SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
 
 pre-clean:
 	$(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)'
diff --git a/t/test-lib.sh b/t/test-lib.sh
index b8dd5e79ac..1ae0fc02d0 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -80,7 +80,7 @@ done,*)
 	# from any previous runs.
 	>"$GIT_TEST_TEE_OUTPUT_FILE"
 
-	(GIT_TEST_TEE_STARTED=done ${SHELL_PATH} "$0" "$@" 2>&1;
+	(GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} "$0" "$@" 2>&1;
 	 echo $? >"$BASE.exit") | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
 	test "$(cat "$BASE.exit")" = 0
 	exit
-- 
2.15.1.659.g8bd2eae3ea

Re: [PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH

[Johannes Schindelin]

Hi Peff,

the other three patches look good to me.

On Fri, 8 Dec 2017, Jeff King wrote:

> You may want to run the test suite with a different shell
> than you use to build Git. For instance, you may build with
> SHELL_PATH=/bin/sh (because it's faster, or it's what you
> expect to exist on systems where the build will be used) but
> want to run the test suite with bash (e.g., since that
> allows using "-x" reliably across the whole test suite).
> There's currently no good way to do this.
> 
> You might think that doing two separate make invocations,
> like:
> 
>   make &&
>   make -C t SHELL_PATH=/bin/bash
> 
> would work. And it _almost_ does. The second make will see
> our bash SHELL_PATH, and we'll use that to run the
> individual test scripts (or tell prove to use it to do so).
> So far so good.
> 
> But this breaks down when "--tee" or "--verbose-log" is
> used. Those options cause the test script to actually
> re-exec itself using $SHELL_PATH. But wait, wouldn't our
> second make invocation have set SHELL_PATH correctly in the
> environment?
> 
> Yes, but test-lib.sh sources GIT-BUILD-OPTIONS, which we
> built during the first "make". And that overrides the
> environment, giving us the original SHELL_PATH again.

... and we could simply see whether the environment variable
TEST_SHELL_PATH (which we would set in t/Makefile from the passed-in
SHELL_PATH) is set, and override it again.

I still think we can do without recording test-phase details in the
build-phase (which may, or may not, know what the test-phase wants to do).

In other words, I believe that we can make the invocation you mentioned
above work, by touching only t/Makefile (to pass SHELL_PATH as
TEST_SHELL_PATH) and t/test-lib.sh (to override the SHELL_PATH from
GIT-BUILD-OPTIONS with TEST_SHELL_PATH, if set).

Ciao,
Dscho

Re: [PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH

[Jeff King]

On Fri, Dec 08, 2017 at 04:08:19PM +0100, Johannes Schindelin wrote:

> > Yes, but test-lib.sh sources GIT-BUILD-OPTIONS, which we
> > built during the first "make". And that overrides the
> > environment, giving us the original SHELL_PATH again.
> 
> ... and we could simply see whether the environment variable
> TEST_SHELL_PATH (which we would set in t/Makefile from the passed-in
> SHELL_PATH) is set, and override it again.
> 
> I still think we can do without recording test-phase details in the
> build-phase (which may, or may not, know what the test-phase wants to do).
> 
> In other words, I believe that we can make the invocation you mentioned
> above work, by touching only t/Makefile (to pass SHELL_PATH as
> TEST_SHELL_PATH) and t/test-lib.sh (to override the SHELL_PATH from
> GIT-BUILD-OPTIONS with TEST_SHELL_PATH, if set).

We could do that, but it makes TEST_SHELL_PATH inconsistent with all of
the other config.mak variables.

-Peff

Re: [PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH

[Johannes Schindelin]

Hi Peff,

On Fri, 8 Dec 2017, Jeff King wrote:

> On Fri, Dec 08, 2017 at 04:08:19PM +0100, Johannes Schindelin wrote:
> 
> > > Yes, but test-lib.sh sources GIT-BUILD-OPTIONS, which we
> > > built during the first "make". And that overrides the
> > > environment, giving us the original SHELL_PATH again.
> > 
> > ... and we could simply see whether the environment variable
> > TEST_SHELL_PATH (which we would set in t/Makefile from the passed-in
> > SHELL_PATH) is set, and override it again.
> > 
> > I still think we can do without recording test-phase details in the
> > build-phase (which may, or may not, know what the test-phase wants to do).
> > 
> > In other words, I believe that we can make the invocation you mentioned
> > above work, by touching only t/Makefile (to pass SHELL_PATH as
> > TEST_SHELL_PATH) and t/test-lib.sh (to override the SHELL_PATH from
> > GIT-BUILD-OPTIONS with TEST_SHELL_PATH, if set).
> 
> We could do that, but it makes TEST_SHELL_PATH inconsistent with all of
> the other config.mak variables.

It is already inconsistent with the other variables because its scope is
the "test" phase, not the "build" phase.

Ciao,
Dscho

Re: [PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH

[Jeff King]

On Sat, Dec 09, 2017 at 02:44:44PM +0100, Johannes Schindelin wrote:

> > > ... and we could simply see whether the environment variable
> > > TEST_SHELL_PATH (which we would set in t/Makefile from the passed-in
> > > SHELL_PATH) is set, and override it again.
> > > 
> > > I still think we can do without recording test-phase details in the
> > > build-phase (which may, or may not, know what the test-phase wants to do).
> > > 
> > > In other words, I believe that we can make the invocation you mentioned
> > > above work, by touching only t/Makefile (to pass SHELL_PATH as
> > > TEST_SHELL_PATH) and t/test-lib.sh (to override the SHELL_PATH from
> > > GIT-BUILD-OPTIONS with TEST_SHELL_PATH, if set).
> > 
> > We could do that, but it makes TEST_SHELL_PATH inconsistent with all of
> > the other config.mak variables.
> 
> It is already inconsistent with the other variables because its scope is
> the "test" phase, not the "build" phase.

I'm not sure that's true. Look at what already goes into
GIT-BUILD-OPTIONS: TEST_OUTPUT_DIRECTORY, GIT_TEST_CMP, GIT_PERF_*, etc.

Interestingly, many of those do something like this in the Makefile:

  ifdef GIT_TEST_CMP
	@echo GIT_TEST_OPTS=... >>$@+
  endif

which seems utterly confusing to me. Because it means that if you build
with those options set, then they will override anything in the
environment. But if you don't, then you _may_ override them in the
environment. In other words:

  make
  cd t
  GIT_TEST_CMP=foo ./t0000-*

will respect that variable. But:

  make GIT_TEST_CMP=foo
  cd t
  GIT_TEST_CMP=bar ./t0000-*

will not. Which seems weird.  But I guess we could follow that pattern
with TEST_SHELL_PATH.

-Peff

Re: [PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'm not sure that's true. Look at what already goes into
> GIT-BUILD-OPTIONS: TEST_OUTPUT_DIRECTORY, GIT_TEST_CMP, GIT_PERF_*, etc.
>
> Interestingly, many of those do something like this in the Makefile:
>
>   ifdef GIT_TEST_CMP
> 	@echo GIT_TEST_OPTS=... >>$@+
>   endif
>
> which seems utterly confusing to me. Because it means that if you build
> with those options set, then they will override anything in the
> environment. But if you don't, then you _may_ override them in the
> environment. In other words:
>
>   make
>   cd t
>   GIT_TEST_CMP=foo ./t0000-*
>
> will respect that variable. But:
>
>   make GIT_TEST_CMP=foo
>   cd t
>   GIT_TEST_CMP=bar ./t0000-*
>
> will not. Which seems weird.  But I guess we could follow that pattern
> with TEST_SHELL_PATH.

Or perhaps we can start setting a better example with the new
variable, and migrate those weird existing ones over to the new way
of not forbidding run-time overriding?

There is a long outstanding NEEDSWORK comment in help.c that wonders
if we want to embed contents from GIT-BUILD-OPTIONS in the resulting
binary, and the distinction Dscho brought up between "build" and
"test" phases would start to matter even more once we go in that
direction.

Re: [PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH

[Jeff King]

On Mon, Dec 11, 2017 at 12:37:42PM -0800, Junio C Hamano wrote:

> > Interestingly, many of those do something like this in the Makefile:
> >
> >   ifdef GIT_TEST_CMP
> > 	@echo GIT_TEST_OPTS=... >>$@+
> >   endif
> >
> > which seems utterly confusing to me. Because it means that if you build
> > with those options set, then they will override anything in the
> > environment. But if you don't, then you _may_ override them in the
> > environment. In other words:
> >
> >   make
> >   cd t
> >   GIT_TEST_CMP=foo ./t0000-*
> >
> > will respect that variable. But:
> >
> >   make GIT_TEST_CMP=foo
> >   cd t
> >   GIT_TEST_CMP=bar ./t0000-*
> >
> > will not. Which seems weird.  But I guess we could follow that pattern
> > with TEST_SHELL_PATH.
> 
> Or perhaps we can start setting a better example with the new
> variable, and migrate those weird existing ones over to the new way
> of not forbidding run-time overriding?

This turns out to be quite tricky, because GIT-BUILD-OPTIONS must be
parsed as both a Makefile and shell-script.

So we can do:

  1. Omit them from GIT-BUILD-OPTIONS, which means they don't work for
     cases where the tests aren't started from the Makefile (which would
     have put them in the environment). I think this is a non-starter.

  2. Add a Makefile-level ifdef when generating GIT-BUILD-OPTIONS, so
     that unused options can be overridden by the environment That's the
     "weird" thing above that we do now for some variables.

  3. Add something like:

       test -z "$FOO" && FOO=...

     when building GIT-BUILD-OPTIONS (instead of just FOO=...). But that
     doesn't work because it must parse as Makefile.

  4. In test-lib.sh, save and restore each such variable so that the
     existing environment takes precedence. Like:

       FOO_save=$FOO
       BAR_save=$BAR
       ...etc for each...

       . GIT-BUILD-OPTIONS

       test -n "$FOO_save" && FOO=$FOO_save
       test -n "$BAR_save" && BAR=$BAR_save
       ...etc...

      We have to do the save/restore dance rather than just reordering
      the assignments, because the existing environment is being passed
      into us (so we can't just "assign" it to overwrite what's in the
      build options file).

      This could be made slightly less tedious with a loop and an eval.
      It could possibly be made very succinct but very magical with
      something like:

        saved=$(export)
	. GIT-BUILD-OPTIONS
	eval "$saved"

  5. Give up on the dual nature of GIT-BUILD-OPTIONS, and generate two
     such files (with identical values but different syntax).

I think (4) and (5) are the only things that actually change the
behavior in a meaningful way. But they're a bit more hacky and
repetitive than I'd like. Especially given that I'm not really sure
we're solving an interesting problem. I'm happy enough with the patch as
shown, and I do not recall anybody complaining about the current
behavior of these options.

> There is a long outstanding NEEDSWORK comment in help.c that wonders
> if we want to embed contents from GIT-BUILD-OPTIONS in the resulting
> binary, and the distinction Dscho brought up between "build" and
> "test" phases would start to matter even more once we go in that
> direction.

I guess you're implying having a GIT-BUILD-OPTIONS and a
GIT-TEST-OPTIONS here. But that doesn't save us from the Makefile/shell
duality, unfortunately. Some of the test options need to be read by
t/Makefile, and some need to be read by test-lib.sh (and I suspect there
are some needed in both places).

-Peff

Re: [PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think (4) and (5) are the only things that actually change the
> behavior in a meaningful way. But they're a bit more hacky and
> repetitive than I'd like. Especially given that I'm not really sure
> we're solving an interesting problem. I'm happy enough with the patch as
> shown, and I do not recall anybody complaining about the current
> behavior of these options.

OK.  Thanks for thinking it through.

>> There is a long outstanding NEEDSWORK comment in help.c that wonders
>> if we want to embed contents from GIT-BUILD-OPTIONS in the resulting
>> binary, and the distinction Dscho brought up between "build" and
>> "test" phases would start to matter even more once we go in that
>> direction.
>
> I guess you're implying having a GIT-BUILD-OPTIONS and a
> GIT-TEST-OPTIONS here.

I admit that my thinking did not go that far to introduce the
latter, as "git version --how-did-we-build-this-exact-git" only
needs the former.  But you're right that some information given
at the top-level must be stored somewhere t/test-lib.sh reads in
order to allow us run tests from outside Makefile (your point 1.)

Re: [PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH

[Jeff King]

On Fri, Dec 15, 2017 at 08:58:22AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I think (4) and (5) are the only things that actually change the
> > behavior in a meaningful way. But they're a bit more hacky and
> > repetitive than I'd like. Especially given that I'm not really sure
> > we're solving an interesting problem. I'm happy enough with the patch as
> > shown, and I do not recall anybody complaining about the current
> > behavior of these options.
> 
> OK.  Thanks for thinking it through.

I took one final look at this, wondering if it ought to follow the
"write to BUILD-OPTIONS only if set" pattern that some other variables
do. But I think that just ends up more confusing, because of the way we
use the variable from both the Makefile and test-lib.sh. So it makes
this work:

  make
  make -C t TEST_SHELL_PATH=whatever

but not quite this:

  make TEST_SHELL_PATH=one
  make -C t TEST_SHELL_PATH=two

because in the second case, we use "two" to invoke the test script, but
a "--tee" re-exec would use "one". Which is pretty subtle.

I really wish there was a way for a shell script to find out which
interpreter it was currently using, but I couldn't come up with a
portable way to do so (on some systems, /proc/$$/exe works, but that's
obviously not something we should count on).

So anyway. I think I'm OK with the series as-is.

-Peff