test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'

This series proposes two small quality-of-life improvements (in my opinion) to the 'test_pause' and 'debug' test functions: using the original values of HOME and TERM (before they are changed by the test framework) and using SHELL instead of SHELL_PATH.

The later might be too big of a change, but I think it makes sense. We could add a new GIT_TEST_* to conditionnaly change the behaviour, but I kept it simple for v1.

Cheers, Philippe.

Philippe Blain (2):
test-lib-functions: use user's SHELL, HOME and TERM for 'test_pause'
test-lib-functions: use user's TERM and HOME for 'debug'

 t/test-lib-functions.sh | 4 ++--
 t/test-lib.sh           | 6 ++++--
 2 files changed, 6 insertions(+), 4 deletions(-)

base-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1022%2Fphil-blain%2Ftest-pause-and-debug-easier-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1022/phil-blain/test-pause-and-debug-easier-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1022

gitgitgadget

From: Philippe Blain <[email protected]>

The 'test_pause' function, which is designed to help interactive debugging and exploration of tests, currently inherits the value of HOME and TERM set by 'test-lib.sh': HOME="$TRASH_DIRECTORY" and TERM=dumb. It also invokes the shell defined by SHELL_PATH, which defaults to /bin/sh.

Changing the value of HOME means that any customization configured in a developers' shell startup files and any Git aliases defined in their global Git configuration file are not available in the shell invoked by 'test_pause'.

Changing the value of TERM to 'dumb' means that colored output is disabled for all commands in that shell.

Using /bin/sh as the shell invoked by 'test_pause' is not ideal since some platforms (i.e. Debian and derivatives) use Dash as /bin/sh, and this shell is usually compiled without readline support, which makes for a poor interactive command line experience.

To make the interactive command line experience in the shell invoked by 'test_pause' more pleasant, save the values of HOME and TERM in USER_HOME and USER_TERM before changing them in test-lib.sh, and use these variables to invoke the shell in 'test_pause'. Also, invoke SHELL instead of SHELL_PATH, so that developer's interactive shell is used.

---
 t/test-lib-functions.sh | 2 +-
 t/test-lib.sh           | 6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

index e28411bb75a..662cfc4c3e0 100644
@@ -139,7 +139,7 @@ test_tick () {
 # Be sure to remove all invocations of this command before submitting.
 
 test_pause () {
-	"$SHELL_PATH" <&6 >&5 2>&7
+	TERM="$USER_TERM" HOME="$USER_HOME" "$SHELL" <&6 >&5 2>&7
 }
 
 # Wrap git with a debugger. Adding this to a command can make it easier

index abcfbed6d61..132618991e2 100644
@@ -585,8 +585,9 @@ else
 	}
 fi
 
+USER_TERM="$TERM"
 TERM=dumb
-export TERM
+export TERM USER_TERM
 
 error () {
 	say_color error "error: $*"

@@ -1380,9 +1381,10 @@ then
 fi
 
 # Last-minute variable setup
+USER_HOME="$HOME"
 HOME="$TRASH_DIRECTORY"
 GNUPGHOME="$HOME/gnupg-home-not-used"
-export HOME GNUPGHOME
+export HOME GNUPGHOME USER_HOME
 
 # Test repository
 rm -fr "$TRASH_DIRECTORY" || {

gitgitgadget

From: Philippe Blain <[email protected]>

The 'debug' function in test-lib-functions.sh is used to invoke a debugger at a specific line in a test. It inherits the value of HOME and TERM set by 'test-lib.sh': HOME="$TRASH_DIRECTORY" and TERM=dumb.

Changing the value of HOME means that any customization configured in a developers' debugger configuration file (like $HOME/.gdbinit or $HOME/.lldbinit) are not available in the debugger invoked by 'test_pause'.

Changing the value of TERM to 'dumb' means that colored output is disabled in the debugger.

To make the debugging experience with 'debug' more pleasant, leverage the variables USER_HOME and USER_TERM, added in the previous commit, to set HOME and TERM before invoking the debugger.

---
 t/test-lib-functions.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 662cfc4c3e0..86680b1177d 100644
@@ -163,7 +163,7 @@ debug () {
 		GIT_DEBUGGER=1
 		;;
 	esac &&
-	GIT_DEBUGGER="${GIT_DEBUGGER}" "$@" <&6 >&5 2>&7
+	TERM="$USER_TERM" HOME="$USER_HOME" GIT_DEBUGGER="${GIT_DEBUGGER}" "$@" <&6 >&5 2>&7
 }
 
 # Usage: test_commit [options] <message> [<file> [<contents> [<tag>]]]

gitgitgadget

I also find the test_pause() user-experience suboptimal and appreciate the idea of improving it. However, this approach seems fatally flawed. In particular, setting HOME to the user's real home directory could lead to undesirable results. When I'm using test_pause() to debug a problem with a test, I'm not just inspecting the test state, but I quite often interact with the state using the same Git commands as the test itself would use. Hence, it is very common for me to pause the test just before the suspect commands and then run those commands manually (instead of allowing the test script to do so). In such a scenario, HOME must be pointing at the test's home directory, not at my real home directory.

Perhaps there's some way to achieve your goal for at least certain shells by detecting the shell and taking advantage of special shell features which allow you to launch the shell and run some canned commands to set up the shell as desired before dropping into an interactive session[1] -- but it's just an idle thought.

[1]: For Bash, for instance, a quick bit of Googling leads to:
https://serverfault.com/a/586272

Yes, I do agree with you that it is a valid concern.

I wonder if the developers can configure tools used during debugging session with XDG so that HOME can be kept for the "fake user that ran the test suite, with the fake user's configuration"?

I agree, but I worry that it's not just HOME. I'd think the point of test_pause is to let you interact with the repository state while getting the same results that the test framework would, and I think some tests could be affected by TERM and SHELL too (e.g. perhaps the recent issues with COLUMNS). Granted, I suspect far fewer tests would be affected by those, but I'm not sure I like the idea of inability to reproduce the same issues.

Maybe we just need a different construct or special flags to test_pause?

Oh, indeed. I didn't mean to imply that HOME is the only problematic one; they all are since, as you say, they can impact correctness and reproducibility of the tests themselves. I called out HOME specially because of the potential danger involved with pointing it at the user's real home directory since it could very well lead to clobbering of precious files and other settings belonging to the user.

I haven't studied XDG deeply enough to answer, though it would not help macOS or Windows users (but that's not an argument against pursuing an XDG solution -- it might still help some developers).

that is a bug, it should have been TEST_SHELL_PATH instead.

goes without saying, that if you don't really need that shell for your interactive session, nothing prevents you from calling bash and resetting TERM or even HOME as needed

Carlo

instead of changing $HOME the needed dot files could be as well linked into the current $HOME.
this will of course need extra code and specific knowledge of the debugger that was invoked but will be IMHO safer and accomplish the same objective.

Carlo

PS. I remember once wondering what GIT_DEBUGGER=1 meant and how it was meant to be used, AFAIK that and the first use case were always broken otherwise, maybe someone who knows and uses this code better could chime in.

Hi everyone,

Le 2021-08-19 à 16:11, Eric Sunshine a écrit :

Thanks everyone for sharing their input and concerns. I understand that the behaviour change might not be wanted all the time, or by everyone.

I also did not think about the implications of changing $HOME that could lead to the test framework overwriting stuff in my home. I checked the tests and there are only a handful of them that seem to reference HOME, but still, for those tests it would be undesirable to reset HOME.

In light of this I'm thinking of simply adding flags to 'test_pause' and 'debug' to signal that one wants to use their original TERM, HOME and SHELL, with appropriate caveats in the description of the functions:

test_pause     # original behaviour
test_pause -t  # use USER_TERM
test_pause -s  # use SHELL instead of TEST_SHELL_PATH
test_pause -h  # use USER_HOME

and combinations of these three.

For 'debug', Carlo's idea of just symlinking/copying gdbinit and/or llldbinit to the test HOME might be easier, and would cover the majority of developers, I think. As for TERM, we could do 'debug -t' as above, or use USER_TERM always...

I'll explore these ideas before sending v2.

Thanks,

Philippe.

Hi Carlo,

Le 2021-08-19 à 23:08, Carlo Arenas a écrit :

Right. I'll make that change unconditionnally as a preparatory step.

Yes. I'm just trying to streamline to experience :)

It's not just tests which reference HOME explicitly which are problematic. Git commands themselves access files and configuration pointed at by HOME. Worse, Git commands invoked by tests can alter configuration, so the potential for damage is wider than the few tests which reference HOME explicitly.

A (very) stray thought I had: Rather than mucking with the environment variables -- which can impact test reproducibility and correctness and can potentially damage the user's precious files and configuration -- another possibility might be to detect which shell is being used (whether it be bash, zsh, etc.), and then enable certain useful options specific to the shell, such as tab-completion, colors, etc. This approach doesn't give developers the customized shell experience they're used to (it wouldn't have their aliases or configuration, for instance), but it at least might make the test_pause() experience a bit more pleasant.

As an even more different alternative, what do you think about making it easier to break out of the test suite at a particular state? That would let you inspect it from your regular shell environment.

E.g., most of my test debugging happens via:

  ./t1234-foo.sh -i
  cd trash\ directory-1234-foo/
  gdb --args ../../git some-command

That implies that the test you're interested in is actually failing (though I have been known to insert "false &&" to make it so), and that running the test doesn't wreck the on-disk state.

But what if we could do something like:

  ./t1234-foo.sh --debug=42

to stop _before_ running test 42, print out the commands it would run, and leave the trash directory so that you can inspect it yourself?

That does not have the "resume after I'm done inspecting" property that test_pause, but IMHO that is not really that useful for debugging.

It will also occasionally cause headaches when a test relies on other parts of the environment (e.g., environment variables defined previously, or functions defined in the script). But I find those are usually a minority of cases.

-Peff

Changes since v1:

* added 1/3 as a preliminary step to use TEST_SHELL_PATH in test_pause instead of SHELL_PATH, as suggested by Carlos
* implemented the change in behaviour through optional flags in both test_pause and debug. This seemed to be the simplest way to keep the current behaviour but also provide a way to improve the UX.

v1: This series proposes two small quality-of-life improvements (in my opinion) to the 'test_pause' and 'debug' test functions: using the original values of HOME and TERM (before they are changed by the test framework) and using SHELL instead of SHELL_PATH.

The later might be too big of a change, but I think it makes sense. We could add a new GIT_TEST_* to conditionnaly change the behaviour, but I kept it simple for v1.

Cheers, Philippe.

Philippe Blain (3):
test-lib-functions: use 'TEST_SHELL_PATH' in 'test_pause'
test-lib-functions: optionally keep HOME, TERM and SHELL in
'test_pause'
test-lib-functions: optionally keep HOME and TERM in 'debug'

 t/test-lib-functions.sh | 91 ++++++++++++++++++++++++++++++++++-------
 t/test-lib.sh           |  6 ++-
 2 files changed, 80 insertions(+), 17 deletions(-)

base-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1022%2Fphil-blain%2Ftest-pause-and-debug-easier-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1022/phil-blain/test-pause-and-debug-easier-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1022

Range-diff vs v1:

 -:  ----------- > 1:  2f566f330e0 test-lib-functions: use 'TEST_SHELL_PATH' in 'test_pause'
 1:  bf916ad98cc ! 2:  00211457ece test-lib-functions: use user's SHELL, HOME and TERM for 'test_pause'

@@ Metadata
Author: Philippe Blain <[email protected]>

## Commit message ##
- test-lib-functions: use user's SHELL, HOME and TERM for 'test_pause'
+ test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'

The 'test_pause' function, which is designed to help interactive debugging and exploration of tests, currently inherits the value of HOME and TERM set by 'test-lib.sh': HOME="$TRASH_DIRECTORY" and TERM=dumb. It
- also invokes the shell defined by SHELL_PATH, which defaults to /bin/sh.
+ also invokes the shell defined by TEST_SHELL_PATH, which defaults to
+ /bin/sh (through SHELL_PATH).

Changing the value of HOME means that any customization configured in a developers' shell startup files and any Git aliases defined in their
@@ Commit message

To make the interactive command line experience in the shell invoked by 'test_pause' more pleasant, save the values of HOME and TERM in
- USER_HOME and USER_TERM before changing them in test-lib.sh, and use
- these variables to invoke the shell in 'test_pause'. Also, invoke SHELL
- instead of SHELL_PATH, so that developer's interactive shell is used.
+ USER_HOME and USER_TERM before changing them in test-lib.sh, and add
+ options to 'test_pause' to optionally use these variables to invoke the
+ shell. Also add an option to invoke SHELL instead of TEST_SHELL_PATH, so
+ that developer's interactive shell is used.
+
+ We use options instead of changing the behaviour unconditionally since
+ these three variables can break test reproducibility. Moreover, using the
+ original HOME means tests could overwrite files in a user's home
+ directory. Be explicit about these caveats in the new 'Usage' section in
+ test-lib-functions.sh.

## t/test-lib-functions.sh ##
@@ t/test-lib-functions.sh: test_tick () {
+ # Stop execution and start a shell. This is useful for debugging tests.
+ #
# Be sure to remove all invocations of this command before submitting.
++#
++# Usage: test_pause [options]
++# -t
++# Use your original TERM instead of test-lib.sh's "dumb".
++# This usually restores color output in the invoked shell.
++# WARNING: this can break test reproducibility.
++# -s
++# Invoke $SHELL instead of $TEST_SHELL_PATH
++# WARNING: this can break test reproducibility.
++# -h
++# Use your original HOME instead of test-lib.sh's "$TRASH_DIRECTORY".
++# This allows you to use your regular shell environment and Git aliases.
++# WARNING: this can break test reproducibility.
++# CAUTION: this can overwrite files in your HOME.

test_pause () {
-- "$SHELL_PATH" <&6 >&5 2>&7
-+ TERM="$USER_TERM" HOME="$USER_HOME" "$SHELL" <&6 >&5 2>&7
+- "$TEST_SHELL_PATH" <&6 >&5 2>&7
++ PAUSE_TERM=$TERM &&
++ PAUSE_SHELL=$TEST_SHELL_PATH &&
++ PAUSE_HOME=$HOME &&
++ while test $# != 0
++ do
++ case "$1" in
++ -t)
++ PAUSE_TERM="$USER_TERM"
++ ;;
++ -s)
++ PAUSE_SHELL="$SHELL"
++ ;;
++ -h)
++ PAUSE_HOME="$USER_HOME"
++ ;;
++ *)
++ break
++ ;;
++ esac
++ shift
++ done &&
++ TERM="$PAUSE_TERM" HOME="$PAUSE_HOME" "$PAUSE_SHELL" <&6 >&5 2>&7
}

# Wrap git with a debugger. Adding this to a command can make it easier

 2:  d51d0db6e25 < -:  ----------- test-lib-functions: use user's TERM and HOME for 'debug'
 -:  ----------- > 3:  1fac9baec1d test-lib-functions: optionally keep HOME and TERM in 'debug'

gitgitgadget

From: Philippe Blain <[email protected]>

3f824e91c8 (t/Makefile: introduce TEST_SHELL_PATH, 2017-12-08) made it easy to use a different shell for the tests than 'SHELL_PATH' used at compile time. But 'test_pause' still invokes 'SHELL_PATH'.

If TEST_SHELL_PATH is set, invoke that shell in 'test_pause' for consistency.

---
 t/test-lib-functions.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index e28411bb75a..1884177e293 100644
@@ -139,7 +139,7 @@ test_tick () {
 # Be sure to remove all invocations of this command before submitting.
 
 test_pause () {
-	"$SHELL_PATH" <&6 >&5 2>&7
+	"$TEST_SHELL_PATH" <&6 >&5 2>&7
 }
 
 # Wrap git with a debugger. Adding this to a command can make it easier

gitgitgadget

From: Philippe Blain <[email protected]>

The 'test_pause' function, which is designed to help interactive debugging and exploration of tests, currently inherits the value of HOME and TERM set by 'test-lib.sh': HOME="$TRASH_DIRECTORY" and TERM=dumb. It also invokes the shell defined by TEST_SHELL_PATH, which defaults to /bin/sh (through SHELL_PATH).

Changing the value of HOME means that any customization configured in a developers' shell startup files and any Git aliases defined in their global Git configuration file are not available in the shell invoked by 'test_pause'.

Changing the value of TERM to 'dumb' means that colored output is disabled for all commands in that shell.

Using /bin/sh as the shell invoked by 'test_pause' is not ideal since some platforms (i.e. Debian and derivatives) use Dash as /bin/sh, and this shell is usually compiled without readline support, which makes for a poor interactive command line experience.

To make the interactive command line experience in the shell invoked by 'test_pause' more pleasant, save the values of HOME and TERM in USER_HOME and USER_TERM before changing them in test-lib.sh, and add options to 'test_pause' to optionally use these variables to invoke the shell. Also add an option to invoke SHELL instead of TEST_SHELL_PATH, so that developer's interactive shell is used.

We use options instead of changing the behaviour unconditionally since these three variables can break test reproducibility. Moreover, using the original HOME means tests could overwrite files in a user's home directory. Be explicit about these caveats in the new 'Usage' section in test-lib-functions.sh.

---
 t/test-lib-functions.sh | 37 ++++++++++++++++++++++++++++++++++++-
 t/test-lib.sh           |  6 ++++--
 2 files changed, 40 insertions(+), 3 deletions(-)

index 1884177e293..1b775343adc 100644
@@ -137,9 +137,44 @@ test_tick () {
 # Stop execution and start a shell. This is useful for debugging tests.
 #
 # Be sure to remove all invocations of this command before submitting.
+#
+# Usage: test_pause [options]
+#   -t
+#	Use your original TERM instead of test-lib.sh's "dumb".
+#	This usually restores color output in the invoked shell.
+#	WARNING: this can break test reproducibility.
+#   -s
+#	Invoke $SHELL instead of $TEST_SHELL_PATH
+#	WARNING: this can break test reproducibility.
+#   -h
+#	Use your original HOME instead of test-lib.sh's "$TRASH_DIRECTORY".
+#	This allows you to use your regular shell environment and Git aliases.
+#	WARNING: this can break test reproducibility.
+#	CAUTION: this can overwrite files in your HOME.
 
 test_pause () {
-	"$TEST_SHELL_PATH" <&6 >&5 2>&7
+	PAUSE_TERM=$TERM &&
+	PAUSE_SHELL=$TEST_SHELL_PATH &&
+	PAUSE_HOME=$HOME &&
+	while test $# != 0
+	do
+		case "$1" in
+		-t)
+			PAUSE_TERM="$USER_TERM"
+			;;
+		-s)
+			PAUSE_SHELL="$SHELL"
+			;;
+		-h)
+			PAUSE_HOME="$USER_HOME"
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done &&
+	TERM="$PAUSE_TERM" HOME="$PAUSE_HOME" "$PAUSE_SHELL" <&6 >&5 2>&7
 }
 
 # Wrap git with a debugger. Adding this to a command can make it easier

index abcfbed6d61..132618991e2 100644
@@ -585,8 +585,9 @@ else
 	}
 fi
 
+USER_TERM="$TERM"
 TERM=dumb
-export TERM
+export TERM USER_TERM
 
 error () {
 	say_color error "error: $*"

@@ -1380,9 +1381,10 @@ then
 fi
 
 # Last-minute variable setup
+USER_HOME="$HOME"
 HOME="$TRASH_DIRECTORY"
 GNUPGHOME="$HOME/gnupg-home-not-used"
-export HOME GNUPGHOME
+export HOME GNUPGHOME USER_HOME
 
 # Test repository
 rm -fr "$TRASH_DIRECTORY" || {

gitgitgadget

From: Philippe Blain <[email protected]>

The 'debug' function in test-lib-functions.sh is used to invoke a debugger at a specific line in a test. It inherits the value of HOME and TERM set by 'test-lib.sh': HOME="$TRASH_DIRECTORY" and TERM=dumb.

Changing the value of HOME means that any customization configured in a developers' debugger configuration file (like $HOME/.gdbinit or $HOME/.lldbinit) are not available in the debugger invoked by 'test_pause'.

Changing the value of TERM to 'dumb' means that colored output is disabled in the debugger.

To make the debugging experience with 'debug' more pleasant, leverage the variables USER_HOME and USER_TERM, added in the previous commit, to optionally set HOME and TERM before invoking the debugger.

Add the same warnings as for 'test_pause' in the "Usage" section.

---
 t/test-lib-functions.sh | 54 ++++++++++++++++++++++++++++++-----------
 1 file changed, 40 insertions(+), 14 deletions(-)

index 1b775343adc..0f6d30e2b3f 100644
@@ -180,25 +180,51 @@ test_pause () {
 # Wrap git with a debugger. Adding this to a command can make it easier
 # to understand what is going on in a failing test.
 #
+# Usage: debug [options] [git command]
+#   -d <debugger>
+#   --debugger=<debugger>
+#	Use <debugger> instead of GDB
+#   -t
+#	Use your original TERM instead of test-lib.sh's "dumb".
+#	This usually restores color output in the debugger.
+#	WARNING: this can break test reproducibility.
+#   -h
+#	Use your original HOME instead of test-lib.sh's "$TRASH_DIRECTORY".
+#	This allows your debugger to find its config file in your home.
+#	WARNING: this can break test reproducibility.
+#	CAUTION: this can overwrite files in your HOME.
+#
 # Examples:
 #     debug git checkout master
 #     debug --debugger=nemiver git $ARGS
 #     debug -d "valgrind --tool=memcheck --track-origins=yes" git $ARGS
 debug () {
-	case "$1" in
-	-d)
-		GIT_DEBUGGER="$2" &&
-		shift 2
-		;;
-	--debugger=*)
-		GIT_DEBUGGER="${1#*=}" &&
-		shift 1
-		;;
-	*)
-		GIT_DEBUGGER=1
-		;;
-	esac &&
-	GIT_DEBUGGER="${GIT_DEBUGGER}" "$@" <&6 >&5 2>&7
+	GIT_DEBUGGER=1 &&
+	DEBUG_TERM=$TERM &&
+	DEBUG_HOME=$HOME &&
+	while test $# != 0
+	do
+		case "$1" in
+		-t)
+			DEBUG_TERM="$USER_TERM"
+			;;
+		-h)
+			DEBUG_HOME="$USER_HOME"
+			;;
+		-d)
+			GIT_DEBUGGER="$2" &&
+			shift
+			;;
+		--debugger=*)
+			GIT_DEBUGGER="${1#*=}"
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done &&
+	TERM="$DEBUG_TERM" HOME="$DEBUG_HOME" GIT_DEBUGGER="${GIT_DEBUGGER}" "$@" <&6 >&5 2>&7
 }
 
 # Usage: test_commit [options] <message> [<file> [<contents> [<tag>]]]

gitgitgadget

Hi Elijah,

Le 2021-08-28 à 03:27, Elijah Newren a écrit :

Yes, that would be even better. I'll add that for next version.

Thanks,

Philippe.

Changes since v2:

* Added '-a' flag as suggested by Elijah, equivalent to '-t -s -h' for 'test_pause' and to '-t -h' for 'debug'

v2:

* added 1/3 as a preliminary step to use TEST_SHELL_PATH in test_pause instead of SHELL_PATH, as suggested by Carlo
* implemented the change in behaviour through optional flags in both test_pause and debug. This seemed to be the simplest way to keep the current behaviour but also provide a way to improve the UX.

v1: This series proposes two small quality-of-life improvements (in my opinion) to the 'test_pause' and 'debug' test functions: using the original values of HOME and TERM (before they are changed by the test framework) and using SHELL instead of SHELL_PATH.

The later might be too big of a change, but I think it makes sense. We could add a new GIT_TEST_* to conditionnaly change the behaviour, but I kept it simple for v1.

Cheers, Philippe.

Philippe Blain (3):
test-lib-functions: use 'TEST_SHELL_PATH' in 'test_pause'
test-lib-functions: optionally keep HOME, TERM and SHELL in
'test_pause'
test-lib-functions: optionally keep HOME and TERM in 'debug'

 t/test-lib-functions.sh | 104 ++++++++++++++++++++++++++++++++++------
 t/test-lib.sh           |   6 ++-
 2 files changed, 93 insertions(+), 17 deletions(-)

base-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1022%2Fphil-blain%2Ftest-pause-and-debug-easier-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1022/phil-blain/test-pause-and-debug-easier-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1022

Range-diff vs v2:

 1:  2f566f330e0 = 1:  2f566f330e0 test-lib-functions: use 'TEST_SHELL_PATH' in 'test_pause'
 2:  00211457ece ! 2:  328b5d6e76f test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'
     @@ t/test-lib-functions.sh: test_tick () {
      +#	This allows you to use your regular shell environment and Git aliases.
      +#	WARNING: this can break test reproducibility.
      +#	CAUTION: this can overwrite files in your HOME.
     ++#   -a
     ++#	Shortcut for -t -s -h
       
       test_pause () {
      -	"$TEST_SHELL_PATH" <&6 >&5 2>&7
     @@ t/test-lib-functions.sh: test_tick () {
      +		-h)
      +			PAUSE_HOME="$USER_HOME"
      +			;;
     ++		-a)
     ++			PAUSE_TERM="$USER_TERM"
     ++			PAUSE_SHELL="$SHELL"
     ++			PAUSE_HOME="$USER_HOME"
     ++			;;
      +		*)
      +			break
      +			;;
 3:  1fac9baec1d ! 3:  4e43bd086b5 test-lib-functions: optionally keep HOME and TERM in 'debug'
     @@ t/test-lib-functions.sh: test_pause () {
      +#	This allows your debugger to find its config file in your home.
      +#	WARNING: this can break test reproducibility.
      +#	CAUTION: this can overwrite files in your HOME.
     ++#   -a
     ++#	Shortcut for -t -h
      +#
       # Examples:
       #     debug git checkout master
     @@ t/test-lib-functions.sh: test_pause () {
      +		-h)
      +			DEBUG_HOME="$USER_HOME"
      +			;;
     ++		-a)
     ++			DEBUG_TERM="$USER_TERM"
     ++			DEBUG_HOME="$USER_HOME"
     ++			;;
      +		-d)
      +			GIT_DEBUGGER="$2" &&
      +			shift

gitgitgadget

From: Philippe Blain <[email protected]>

The 'test_pause' function, which is designed to help interactive debugging and exploration of tests, currently inherits the value of HOME and TERM set by 'test-lib.sh': HOME="$TRASH_DIRECTORY" and TERM=dumb. It also invokes the shell defined by TEST_SHELL_PATH, which defaults to /bin/sh (through SHELL_PATH).

Changing the value of HOME means that any customization configured in a developers' shell startup files and any Git aliases defined in their global Git configuration file are not available in the shell invoked by 'test_pause'.

Changing the value of TERM to 'dumb' means that colored output is disabled for all commands in that shell.

Using /bin/sh as the shell invoked by 'test_pause' is not ideal since some platforms (i.e. Debian and derivatives) use Dash as /bin/sh, and this shell is usually compiled without readline support, which makes for a poor interactive command line experience.

To make the interactive command line experience in the shell invoked by 'test_pause' more pleasant, save the values of HOME and TERM in USER_HOME and USER_TERM before changing them in test-lib.sh, and add options to 'test_pause' to optionally use these variables to invoke the shell. Also add an option to invoke SHELL instead of TEST_SHELL_PATH, so that developer's interactive shell is used.

We use options instead of changing the behaviour unconditionally since these three variables can break test reproducibility. Moreover, using the original HOME means tests could overwrite files in a user's home directory. Be explicit about these caveats in the new 'Usage' section in test-lib-functions.sh.

---
 t/test-lib-functions.sh | 44 ++++++++++++++++++++++++++++++++++++++++-
 t/test-lib.sh           |  6 ++++--
 2 files changed, 47 insertions(+), 3 deletions(-)

index 1884177e293..4b667dc7e76 100644
@@ -137,9 +137,51 @@ test_tick () {
 # Stop execution and start a shell. This is useful for debugging tests.
 #
 # Be sure to remove all invocations of this command before submitting.
+#
+# Usage: test_pause [options]
+#   -t
+#	Use your original TERM instead of test-lib.sh's "dumb".
+#	This usually restores color output in the invoked shell.
+#	WARNING: this can break test reproducibility.
+#   -s
+#	Invoke $SHELL instead of $TEST_SHELL_PATH
+#	WARNING: this can break test reproducibility.
+#   -h
+#	Use your original HOME instead of test-lib.sh's "$TRASH_DIRECTORY".
+#	This allows you to use your regular shell environment and Git aliases.
+#	WARNING: this can break test reproducibility.
+#	CAUTION: this can overwrite files in your HOME.
+#   -a
+#	Shortcut for -t -s -h
 
 test_pause () {
-	"$TEST_SHELL_PATH" <&6 >&5 2>&7
+	PAUSE_TERM=$TERM &&
+	PAUSE_SHELL=$TEST_SHELL_PATH &&
+	PAUSE_HOME=$HOME &&
+	while test $# != 0
+	do
+		case "$1" in
+		-t)
+			PAUSE_TERM="$USER_TERM"
+			;;
+		-s)
+			PAUSE_SHELL="$SHELL"
+			;;
+		-h)
+			PAUSE_HOME="$USER_HOME"
+			;;
+		-a)
+			PAUSE_TERM="$USER_TERM"
+			PAUSE_SHELL="$SHELL"
+			PAUSE_HOME="$USER_HOME"
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done &&
+	TERM="$PAUSE_TERM" HOME="$PAUSE_HOME" "$PAUSE_SHELL" <&6 >&5 2>&7
 }
 
 # Wrap git with a debugger. Adding this to a command can make it easier

index abcfbed6d61..132618991e2 100644
@@ -585,8 +585,9 @@ else
 	}
 fi
 
+USER_TERM="$TERM"
 TERM=dumb
-export TERM
+export TERM USER_TERM
 
 error () {
 	say_color error "error: $*"

@@ -1380,9 +1381,10 @@ then
 fi
 
 # Last-minute variable setup
+USER_HOME="$HOME"
 HOME="$TRASH_DIRECTORY"
 GNUPGHOME="$HOME/gnupg-home-not-used"
-export HOME GNUPGHOME
+export HOME GNUPGHOME USER_HOME
 
 # Test repository
 rm -fr "$TRASH_DIRECTORY" || {

gitgitgadget

From: Philippe Blain <[email protected]>

3f824e91c8 (t/Makefile: introduce TEST_SHELL_PATH, 2017-12-08) made it easy to use a different shell for the tests than 'SHELL_PATH' used at compile time. But 'test_pause' still invokes 'SHELL_PATH'.

If TEST_SHELL_PATH is set, invoke that shell in 'test_pause' for consistency.

---
 t/test-lib-functions.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index e28411bb75a..1884177e293 100644
@@ -139,7 +139,7 @@ test_tick () {
 # Be sure to remove all invocations of this command before submitting.
 
 test_pause () {
-	"$SHELL_PATH" <&6 >&5 2>&7
+	"$TEST_SHELL_PATH" <&6 >&5 2>&7
 }
 
 # Wrap git with a debugger. Adding this to a command can make it easier

gitgitgadget

From: Philippe Blain <[email protected]>

The 'debug' function in test-lib-functions.sh is used to invoke a debugger at a specific line in a test. It inherits the value of HOME and TERM set by 'test-lib.sh': HOME="$TRASH_DIRECTORY" and TERM=dumb.

Changing the value of HOME means that any customization configured in a developers' debugger configuration file (like $HOME/.gdbinit or $HOME/.lldbinit) are not available in the debugger invoked by 'test_pause'.

Changing the value of TERM to 'dumb' means that colored output is disabled in the debugger.

To make the debugging experience with 'debug' more pleasant, leverage the variables USER_HOME and USER_TERM, added in the previous commit, to optionally set HOME and TERM before invoking the debugger.

Add the same warnings as for 'test_pause' in the "Usage" section.

---
 t/test-lib-functions.sh | 60 +++++++++++++++++++++++++++++++----------
 1 file changed, 46 insertions(+), 14 deletions(-)

index 4b667dc7e76..537fd88e686 100644
@@ -187,25 +187,57 @@ test_pause () {
 # Wrap git with a debugger. Adding this to a command can make it easier
 # to understand what is going on in a failing test.
 #
+# Usage: debug [options] [git command]
+#   -d <debugger>
+#   --debugger=<debugger>
+#	Use <debugger> instead of GDB
+#   -t
+#	Use your original TERM instead of test-lib.sh's "dumb".
+#	This usually restores color output in the debugger.
+#	WARNING: this can break test reproducibility.
+#   -h
+#	Use your original HOME instead of test-lib.sh's "$TRASH_DIRECTORY".
+#	This allows your debugger to find its config file in your home.
+#	WARNING: this can break test reproducibility.
+#	CAUTION: this can overwrite files in your HOME.
+#   -a
+#	Shortcut for -t -h
+#
 # Examples:
 #     debug git checkout master
 #     debug --debugger=nemiver git $ARGS
 #     debug -d "valgrind --tool=memcheck --track-origins=yes" git $ARGS
 debug () {
-	case "$1" in
-	-d)
-		GIT_DEBUGGER="$2" &&
-		shift 2
-		;;
-	--debugger=*)
-		GIT_DEBUGGER="${1#*=}" &&
-		shift 1
-		;;
-	*)
-		GIT_DEBUGGER=1
-		;;
-	esac &&
-	GIT_DEBUGGER="${GIT_DEBUGGER}" "$@" <&6 >&5 2>&7
+	GIT_DEBUGGER=1 &&
+	DEBUG_TERM=$TERM &&
+	DEBUG_HOME=$HOME &&
+	while test $# != 0
+	do
+		case "$1" in
+		-t)
+			DEBUG_TERM="$USER_TERM"
+			;;
+		-h)
+			DEBUG_HOME="$USER_HOME"
+			;;
+		-a)
+			DEBUG_TERM="$USER_TERM"
+			DEBUG_HOME="$USER_HOME"
+			;;
+		-d)
+			GIT_DEBUGGER="$2" &&
+			shift
+			;;
+		--debugger=*)
+			GIT_DEBUGGER="${1#*=}"
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done &&
+	TERM="$DEBUG_TERM" HOME="$DEBUG_HOME" GIT_DEBUGGER="${GIT_DEBUGGER}" "$@" <&6 >&5 2>&7
 }
 
 # Usage: test_commit [options] <message> [<file> [<contents> [<tag>]]]

gitgitgadget

I think the warning was less about what happens after test_pause is complete and the testcase resumes, and more intended to convey that if the user tries to manually copy & paste snippets of code from the test into $PAUSE_SHELL, then the use of these flags can cause the result of those pasted commands to differ. If so, a small rewording might be in order, e.g. "WARNING: this can cause commands run in the paused shell to give different results". I'd also say the CAUTION would perhaps benefit from some rewording (since the option itself doesn't cause files to be overwritten), e.g. "CAUTION: using this option and copying commands from the test script into the paused shell might result in files in your HOME being overwritten".

But the difference of TERM and SHELL are much smaller issue when one cuts and pastes the lines from the test script. Even if you do not use -s or -t options, shell variables and helpers won't be available to the interactive session given by test_pause, and that is a much bigger difference between the "real" shell environment that is running the tests and the user's shell environment test_pause gives us. That is why I think the WARNING should be attached _directly_ to the description of test_pause and not to individual "this uses different X environment compared to the real shell environment running the tests" options.

The updated $HOME does deserve a special mention, as .gitconfig from it would further affect the tests and hide the bug you are hunting for, and the CAUTION is a good thing to have, even if you do not plan to modify anything in the test_pause sight-seeing session.

Hi Elijah and Junio,

Le 2021-09-01 à 19:09, Junio C Hamano a écrit :

Yes, I added the warnings based on the reticence I got in v1 to change the behaviour without adding flags.

OK, I can make that tweak.

Le 2021-09-01 à 17:52, Elijah Newren a écrit :
-- snip --

Thanks for these suggestions, that wording does seem clearer.

Philippe.

Changes since v3:

* 2/3: improved the wording for the warning and caution as suggested by Elijah,, and moved the warning so it relates to the use of test_pause itself, not just the new flags, as suggested by Junio. Adapted the commit messages accordingly.
* 3/3: changed the approach: instead of changing HOME, just copy ~/.gdbinit and ~/.lldbinit to the test HOME, as suggested by Carlo. This seems safer as this way $USER_HOME/.gitconfig does not interfere with the behaviour of the command being debugged (as Junio remarked in [1], but for test_pause). If other config files are needed for other debuggers, they can be added when the need arises.
* [23]/3: also adapted the synopsys of 'test_pause' and 'debug' in t/README for better discoverability of the new features.

[1] https://lore.kernel.org/git/[email protected]/

v3:

* Added '-a' flag as suggested by Elijah, equivalent to '-t -s -h' for 'test_pause' and to '-t -h' for 'debug'

v2:

* added 1/3 as a preliminary step to use TEST_SHELL_PATH in test_pause instead of SHELL_PATH, as suggested by Carlo
* implemented the change in behaviour through optional flags in both test_pause and debug. This seemed to be the simplest way to keep the current behaviour but also provide a way to improve the UX.

v1: This series proposes two small quality-of-life improvements (in my opinion) to the 'test_pause' and 'debug' test functions: using the original values of HOME and TERM (before they are changed by the test framework) and using SHELL instead of SHELL_PATH.

The later might be too big of a change, but I think it makes sense. We could add a new GIT_TEST_* to conditionnaly change the behaviour, but I kept it simple for v1.

Cheers, Philippe.

Philippe Blain (3):
test-lib-functions: use 'TEST_SHELL_PATH' in 'test_pause'
test-lib-functions: optionally keep HOME, TERM and SHELL in
'test_pause'
test-lib-functions: keep user's debugger config files and TERM in

    'debug'

 t/README                |  11 ++--
 t/test-lib-functions.sh | 113 ++++++++++++++++++++++++++++++++++------
 t/test-lib.sh           |   6 ++-
 3 files changed, 109 insertions(+), 21 deletions(-)

base-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1022%2Fphil-blain%2Ftest-pause-and-debug-easier-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1022/phil-blain/test-pause-and-debug-easier-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1022

Range-diff vs v3:

 1:  2f566f330e0 = 1:  2f566f330e0 test-lib-functions: use 'TEST_SHELL_PATH' in 'test_pause'
 2:  328b5d6e76f ! 2:  a231d560e68 test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'

@@ Commit message
that developer's interactive shell is used.

We use options instead of changing the behaviour unconditionally since
- these three variables can break test reproducibility. Moreover, using the
- original HOME means tests could overwrite files in a user's home
- directory. Be explicit about these caveats in the new 'Usage' section in
- test-lib-functions.sh.
+ these three variables can slightly change command behaviour. Moreover,
+ using the original HOME means commands could overwrite files in a user's
+ home directory. Be explicit about these caveats in the new 'Usage'
+ section in test-lib-functions.sh.

+ Finally, add '[options]' to the test_pause synopsys in t/README, and
+ mention that the full list of helper functions and their options can be
+ found in test-lib-functions.sh.
+
+ Helped-by: Elijah Newren <[email protected]>

+ ## t/README ##
+@@ t/README: Test harness library
+ --------------------
+
+ There are a handful helper functions defined in the test harness +-library for your script to use.
++library for your script to use. Some of them are listed below; ++see test-lib-functions.sh for the full list and their options.
+
+ - test_expect_success [<prereq>] <message> <script>
+
+@@ t/README: library for your script to use.
+ EOF
+
+
+- - test_pause
++ - test_pause [options]
+
+ This command is useful for writing and debugging tests and must be
+ removed before submitting. It halts the execution of the test and
+
## t/test-lib-functions.sh ##
@@ t/test-lib-functions.sh: test_tick () {
# Stop execution and start a shell. This is useful for debugging tests. #
# Be sure to remove all invocations of this command before submitting.
++# WARNING: the shell invoked by this helper does not have the same environment ++# as the one running the tests (shell variables and functions are not ++# available, and the options below further modify the environment). As such, ++# commands copied from a test script might behave differently than when ++# running the test.
+#
+# Usage: test_pause [options]
+# -t
+# Use your original TERM instead of test-lib.sh's "dumb".
+# This usually restores color output in the invoked shell.
-+# WARNING: this can break test reproducibility.
+# -s
-+# Invoke $SHELL instead of $TEST_SHELL_PATH
-+# WARNING: this can break test reproducibility.
++# Invoke $SHELL instead of $TEST_SHELL_PATH.
+# -h
+# Use your original HOME instead of test-lib.sh's "$TRASH_DIRECTORY".
+# This allows you to use your regular shell environment and Git aliases.
-+# WARNING: this can break test reproducibility.
-+# CAUTION: this can overwrite files in your HOME.
++# CAUTION: running commands copied from a test script into the paused shell
++# might result in files in your HOME being overwritten.
+# -a
+# Shortcut for -t -s -h

3: 4e43bd086b5 ! 3: a8b12788fa4 test-lib-functions: optionally keep HOME and TERM in 'debug'

     @@ Metadata
      Author: Philippe Blain <[email protected]>
      
       ## Commit message ##
     -    test-lib-functions: optionally keep HOME and TERM in 'debug'
     +    test-lib-functions: keep user's debugger config files and TERM in 'debug'
      
          The 'debug' function in test-lib-functions.sh is used to invoke a
          debugger at a specific line in a test. It inherits the value of HOME and
     @@ Commit message
          is disabled in the debugger.
      
          To make the debugging experience with 'debug' more pleasant, leverage
     -    the variables USER_HOME and USER_TERM, added in the previous commit, to
     -    optionally set HOME and TERM before invoking the debugger.
     +    the variable USER_HOME, added in the previous commit, to copy a
     +    developer's ~/.gdbinit and ~/.lldbinit to the test HOME. We do not set
     +    HOME to USER_HOME as in 'test_pause' to avoid user configuration in
     +    $USER_HOME/.gitconfig from interfering with the command being debugged.
      
     -    Add the same warnings as for 'test_pause' in the "Usage" section.
     +    Note that we use a while loop and a heredoc to protect against
     +    $USER_HOME containing spaces.
      
     +    Also, add a flag to launch the debugger with the original value of
     +    TERM, and add the same warning as for 'test_pause'.
     +
     +    Helped-by: Carlo Marcelo Arenas Belón <[email protected]>
          Signed-off-by: Philippe Blain <[email protected]>
      
     + ## t/README ##
     +@@ t/README: see test-lib-functions.sh for the full list and their options.
     +    argument.  This is primarily meant for use during the
     +    development of a new test script.
     + 
     +- - debug <git-command>
     ++ - debug [options] <git-command>
     + 
     +    Run a git command inside a debugger. This is primarily meant for
     +-   use when debugging a failing test script.
     ++   use when debugging a failing test script. With '-t', use your
     ++   original TERM instead of test-lib.sh's "dumb", so that your
     ++   debugger interface has colors.
     + 
     +  - test_done
     + 
     +
       ## t/test-lib-functions.sh ##
      @@ t/test-lib-functions.sh: test_pause () {
       # Wrap git with a debugger. Adding this to a command can make it easier
       # to understand what is going on in a failing test.
       #
     -+# Usage: debug [options] [git command]
     ++# Usage: debug [options] <git command>
      +#   -d <debugger>
      +#   --debugger=<debugger>
      +#	Use <debugger> instead of GDB
      +#   -t
      +#	Use your original TERM instead of test-lib.sh's "dumb".
      +#	This usually restores color output in the debugger.
     -+#	WARNING: this can break test reproducibility.
     -+#   -h
     -+#	Use your original HOME instead of test-lib.sh's "$TRASH_DIRECTORY".
     -+#	This allows your debugger to find its config file in your home.
     -+#	WARNING: this can break test reproducibility.
     -+#	CAUTION: this can overwrite files in your HOME.
     -+#   -a
     -+#	Shortcut for -t -h
     ++#	WARNING: the command being debugged might behave differently than when
     ++#	running the test.
      +#
       # Examples:
       #     debug git checkout master
     @@ t/test-lib-functions.sh: test_pause () {
      -	GIT_DEBUGGER="${GIT_DEBUGGER}" "$@" <&6 >&5 2>&7
      +	GIT_DEBUGGER=1 &&
      +	DEBUG_TERM=$TERM &&
     -+	DEBUG_HOME=$HOME &&
      +	while test $# != 0
      +	do
      +		case "$1" in
      +		-t)
      +			DEBUG_TERM="$USER_TERM"
      +			;;
     -+		-h)
     -+			DEBUG_HOME="$USER_HOME"
     -+			;;
     -+		-a)
     -+			DEBUG_TERM="$USER_TERM"
     -+			DEBUG_HOME="$USER_HOME"
     -+			;;
      +		-d)
      +			GIT_DEBUGGER="$2" &&
      +			shift
     @@ t/test-lib-functions.sh: test_pause () {
      +		esac
      +		shift
      +	done &&
     -+	TERM="$DEBUG_TERM" HOME="$DEBUG_HOME" GIT_DEBUGGER="${GIT_DEBUGGER}" "$@" <&6 >&5 2>&7
     ++
     ++	dotfiles="
     ++	.gdbinit
     ++	.lldbinit
     ++	"
     ++	while read -r dotfile
     ++	do
     ++		dotfile="$USER_HOME/$dotfile" &&
     ++		test -f "$dotfile" && cp "$dotfile" "$HOME" || :
     ++	done <<-EOF &&
     ++	$dotfiles
     ++	EOF
     ++
     ++	TERM="$DEBUG_TERM" GIT_DEBUGGER="${GIT_DEBUGGER}" "$@" <&6 >&5 2>&7 &&
     ++
     ++	while read -r dotfile
     ++	do
     ++		rm -f "$HOME/$dotfile"
     ++	done <<-EOF
     ++	$dotfiles
     ++	EOF
       }
       
       # Usage: test_commit [options] <message> [<file> [<contents> [<tag>]]]

gitgitgadget

From: Philippe Blain <[email protected]>

3f824e91c8 (t/Makefile: introduce TEST_SHELL_PATH, 2017-12-08) made it easy to use a different shell for the tests than 'SHELL_PATH' used at compile time. But 'test_pause' still invokes 'SHELL_PATH'.

If TEST_SHELL_PATH is set, invoke that shell in 'test_pause' for consistency.

---
 t/test-lib-functions.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index e28411bb75a..1884177e293 100644
@@ -139,7 +139,7 @@ test_tick () {
 # Be sure to remove all invocations of this command before submitting.
 
 test_pause () {
-	"$SHELL_PATH" <&6 >&5 2>&7
+	"$TEST_SHELL_PATH" <&6 >&5 2>&7
 }
 
 # Wrap git with a debugger. Adding this to a command can make it easier

gitgitgadget

From: Philippe Blain <[email protected]>

The 'test_pause' function, which is designed to help interactive debugging and exploration of tests, currently inherits the value of HOME and TERM set by 'test-lib.sh': HOME="$TRASH_DIRECTORY" and TERM=dumb. It also invokes the shell defined by TEST_SHELL_PATH, which defaults to /bin/sh (through SHELL_PATH).

Changing the value of HOME means that any customization configured in a developers' shell startup files and any Git aliases defined in their global Git configuration file are not available in the shell invoked by 'test_pause'.

Changing the value of TERM to 'dumb' means that colored output is disabled for all commands in that shell.

Using /bin/sh as the shell invoked by 'test_pause' is not ideal since some platforms (i.e. Debian and derivatives) use Dash as /bin/sh, and this shell is usually compiled without readline support, which makes for a poor interactive command line experience.

To make the interactive command line experience in the shell invoked by 'test_pause' more pleasant, save the values of HOME and TERM in USER_HOME and USER_TERM before changing them in test-lib.sh, and add options to 'test_pause' to optionally use these variables to invoke the shell. Also add an option to invoke SHELL instead of TEST_SHELL_PATH, so that developer's interactive shell is used.

We use options instead of changing the behaviour unconditionally since these three variables can slightly change command behaviour. Moreover, using the original HOME means commands could overwrite files in a user's home directory. Be explicit about these caveats in the new 'Usage' section in test-lib-functions.sh.

Finally, add '[options]' to the test_pause synopsys in t/README, and mention that the full list of helper functions and their options can be found in test-lib-functions.sh.

---
 t/README                |  5 +++--
 t/test-lib-functions.sh | 47 ++++++++++++++++++++++++++++++++++++++++-
 t/test-lib.sh           |  6 ++++--
 3 files changed, 53 insertions(+), 5 deletions(-)

index 9e701223020..cc8be6e67ad 100644
@@ -753,7 +753,8 @@ Test harness library
 --------------------
 
 There are a handful helper functions defined in the test harness
-library for your script to use.
+library for your script to use. Some of them are listed below;
+see test-lib-functions.sh for the full list and their options.
 
  - test_expect_success [<prereq>] <message> <script>
 

@@ -989,7 +990,7 @@ library for your script to use.
 	EOF
 
 
- - test_pause
+ - test_pause [options]
 
 	This command is useful for writing and debugging tests and must be
 	removed before submitting. It halts the execution of the test and

index abcfbed6d61..132618991e2 100644
@@ -585,8 +585,9 @@ else
 	}
 fi
 
+USER_TERM="$TERM"
 TERM=dumb
-export TERM
+export TERM USER_TERM
 
 error () {
 	say_color error "error: $*"

@@ -1380,9 +1381,10 @@ then
 fi
 
 # Last-minute variable setup
+USER_HOME="$HOME"
 HOME="$TRASH_DIRECTORY"
 GNUPGHOME="$HOME/gnupg-home-not-used"
-export HOME GNUPGHOME
+export HOME GNUPGHOME USER_HOME
 
 # Test repository
 rm -fr "$TRASH_DIRECTORY" || {

gitgitgadget

From: Philippe Blain <[email protected]>

The 'debug' function in test-lib-functions.sh is used to invoke a debugger at a specific line in a test. It inherits the value of HOME and TERM set by 'test-lib.sh': HOME="$TRASH_DIRECTORY" and TERM=dumb.

Changing the value of HOME means that any customization configured in a developers' debugger configuration file (like $HOME/.gdbinit or $HOME/.lldbinit) are not available in the debugger invoked by 'test_pause'.

Changing the value of TERM to 'dumb' means that colored output is disabled in the debugger.

To make the debugging experience with 'debug' more pleasant, leverage the variable USER_HOME, added in the previous commit, to copy a developer's ~/.gdbinit and ~/.lldbinit to the test HOME. We do not set HOME to USER_HOME as in 'test_pause' to avoid user configuration in $USER_HOME/.gitconfig from interfering with the command being debugged.

Note that we use a while loop and a heredoc to protect against $USER_HOME containing spaces.

Also, add a flag to launch the debugger with the original value of TERM, and add the same warning as for 'test_pause'.

---
 t/README                |  6 ++--
 t/test-lib-functions.sh | 66 ++++++++++++++++++++++++++++++++---------
 2 files changed, 56 insertions(+), 16 deletions(-)

index cc8be6e67ad..e924bd81e2d 100644
@@ -800,10 +800,12 @@ see test-lib-functions.sh for the full list and their options.
    argument.  This is primarily meant for use during the
    development of a new test script.
 
- - debug <git-command>
+ - debug [options] <git-command>
 
    Run a git command inside a debugger. This is primarily meant for
-   use when debugging a failing test script.
+   use when debugging a failing test script. With '-t', use your
+   original TERM instead of test-lib.sh's "dumb", so that your
+   debugger interface has colors.
 
  - test_done
 

gitgitgadget

Changes since v4:

* 3/3: use a for loop instead of while loop + heredoc, it's simpler and the need for the later did not match the code.

v4:

* 2/3: improved the wording for the warning and caution as suggested by Elijah,, and moved the warning so it relates to the use of test_pause itself, not just the new flags, as suggested by Junio. Adapted the commit messages accordingly.
* 3/3: changed the approach: instead of changing HOME, just copy ~/.gdbinit and ~/.lldbinit to the test HOME, as suggested by Carlo. This seems safer as this way $USER_HOME/.gitconfig does not interfere with the behaviour of the command being debugged (as Junio remarked in [1], but for test_pause). If other config files are needed for other debuggers, they can be added when the need arises.
* [23]/3: also adapted the synopsys of 'test_pause' and 'debug' in t/README for better discoverability of the new features.

[1] https://lore.kernel.org/git/[email protected]/

v3:

* Added '-a' flag as suggested by Elijah, equivalent to '-t -s -h' for 'test_pause' and to '-t -h' for 'debug'

v2:

* added 1/3 as a preliminary step to use TEST_SHELL_PATH in test_pause instead of SHELL_PATH, as suggested by Carlo
* implemented the change in behaviour through optional flags in both test_pause and debug. This seemed to be the simplest way to keep the current behaviour but also provide a way to improve the UX.

v1: This series proposes two small quality-of-life improvements (in my opinion) to the 'test_pause' and 'debug' test functions: using the original values of HOME and TERM (before they are changed by the test framework) and using SHELL instead of SHELL_PATH.

The later might be too big of a change, but I think it makes sense. We could add a new GIT_TEST_* to conditionnaly change the behaviour, but I kept it simple for v1.

Cheers, Philippe.

Philippe Blain (3):
test-lib-functions: use 'TEST_SHELL_PATH' in 'test_pause'
test-lib-functions: optionally keep HOME, TERM and SHELL in
'test_pause'
test-lib-functions: keep user's debugger config files and TERM in

    'debug'

 t/README                |  11 +++--
 t/test-lib-functions.sh | 107 ++++++++++++++++++++++++++++++++++------
 t/test-lib.sh           |   6 ++-
 3 files changed, 103 insertions(+), 21 deletions(-)

base-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1022%2Fphil-blain%2Ftest-pause-and-debug-easier-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1022/phil-blain/test-pause-and-debug-easier-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1022

Range-diff vs v4:

 1:  2f566f330e0 = 1:  2f566f330e0 test-lib-functions: use 'TEST_SHELL_PATH' in 'test_pause'
 2:  a231d560e68 = 2:  a231d560e68 test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'
 3:  a8b12788fa4 ! 3:  ebf92b6b2c3 test-lib-functions: keep user's debugger config files and TERM in 'debug'
     @@ Commit message
          HOME to USER_HOME as in 'test_pause' to avoid user configuration in
          $USER_HOME/.gitconfig from interfering with the command being debugged.
      
     -    Note that we use a while loop and a heredoc to protect against
     -    $USER_HOME containing spaces.
     -
          Also, add a flag to launch the debugger with the original value of
          TERM, and add the same warning as for 'test_pause'.
      
     @@ t/test-lib-functions.sh: test_pause () {
      +		shift
      +	done &&
      +
     -+	dotfiles="
     -+	.gdbinit
     -+	.lldbinit
     -+	"
     -+	while read -r dotfile
     ++	dotfiles=".gdbinit .lldbinit"
     ++
     ++	for dotfile in $dotfiles
      +	do
      +		dotfile="$USER_HOME/$dotfile" &&
      +		test -f "$dotfile" && cp "$dotfile" "$HOME" || :
     -+	done <<-EOF &&
     -+	$dotfiles
     -+	EOF
     ++	done &&
      +
      +	TERM="$DEBUG_TERM" GIT_DEBUGGER="${GIT_DEBUGGER}" "$@" <&6 >&5 2>&7 &&
      +
     -+	while read -r dotfile
     ++	for dotfile in $dotfiles
      +	do
      +		rm -f "$HOME/$dotfile"
     -+	done <<-EOF
     -+	$dotfiles
     -+	EOF
     ++	done
       }
       
       # Usage: test_commit [options] <message> [<file> [<contents> [<tag>]]]

gitgitgadget

From: Philippe Blain <[email protected]>

3f824e91c8 (t/Makefile: introduce TEST_SHELL_PATH, 2017-12-08) made it easy to use a different shell for the tests than 'SHELL_PATH' used at compile time. But 'test_pause' still invokes 'SHELL_PATH'.

If TEST_SHELL_PATH is set, invoke that shell in 'test_pause' for consistency.

---
 t/test-lib-functions.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index e28411bb75a..1884177e293 100644
@@ -139,7 +139,7 @@ test_tick () {
 # Be sure to remove all invocations of this command before submitting.
 
 test_pause () {
-	"$SHELL_PATH" <&6 >&5 2>&7
+	"$TEST_SHELL_PATH" <&6 >&5 2>&7
 }
 
 # Wrap git with a debugger. Adding this to a command can make it easier

gitgitgadget

From: Philippe Blain <[email protected]>

The 'test_pause' function, which is designed to help interactive debugging and exploration of tests, currently inherits the value of HOME and TERM set by 'test-lib.sh': HOME="$TRASH_DIRECTORY" and TERM=dumb. It also invokes the shell defined by TEST_SHELL_PATH, which defaults to /bin/sh (through SHELL_PATH).

Changing the value of HOME means that any customization configured in a developers' shell startup files and any Git aliases defined in their global Git configuration file are not available in the shell invoked by 'test_pause'.

Changing the value of TERM to 'dumb' means that colored output is disabled for all commands in that shell.

Using /bin/sh as the shell invoked by 'test_pause' is not ideal since some platforms (i.e. Debian and derivatives) use Dash as /bin/sh, and this shell is usually compiled without readline support, which makes for a poor interactive command line experience.

To make the interactive command line experience in the shell invoked by 'test_pause' more pleasant, save the values of HOME and TERM in USER_HOME and USER_TERM before changing them in test-lib.sh, and add options to 'test_pause' to optionally use these variables to invoke the shell. Also add an option to invoke SHELL instead of TEST_SHELL_PATH, so that developer's interactive shell is used.

We use options instead of changing the behaviour unconditionally since these three variables can slightly change command behaviour. Moreover, using the original HOME means commands could overwrite files in a user's home directory. Be explicit about these caveats in the new 'Usage' section in test-lib-functions.sh.

Finally, add '[options]' to the test_pause synopsys in t/README, and mention that the full list of helper functions and their options can be found in test-lib-functions.sh.

---
 t/README                |  5 +++--
 t/test-lib-functions.sh | 47 ++++++++++++++++++++++++++++++++++++++++-
 t/test-lib.sh           |  6 ++++--
 3 files changed, 53 insertions(+), 5 deletions(-)

index 9e701223020..cc8be6e67ad 100644
@@ -753,7 +753,8 @@ Test harness library
 --------------------
 
 There are a handful helper functions defined in the test harness
-library for your script to use.
+library for your script to use. Some of them are listed below;
+see test-lib-functions.sh for the full list and their options.
 
  - test_expect_success [<prereq>] <message> <script>
 

@@ -989,7 +990,7 @@ library for your script to use.
 	EOF
 
 
- - test_pause
+ - test_pause [options]
 
 	This command is useful for writing and debugging tests and must be
 	removed before submitting. It halts the execution of the test and

index abcfbed6d61..132618991e2 100644
@@ -585,8 +585,9 @@ else
 	}
 fi
 
+USER_TERM="$TERM"
 TERM=dumb
-export TERM
+export TERM USER_TERM
 
 error () {
 	say_color error "error: $*"

@@ -1380,9 +1381,10 @@ then
 fi
 
 # Last-minute variable setup
+USER_HOME="$HOME"
 HOME="$TRASH_DIRECTORY"
 GNUPGHOME="$HOME/gnupg-home-not-used"
-export HOME GNUPGHOME
+export HOME GNUPGHOME USER_HOME
 
 # Test repository
 rm -fr "$TRASH_DIRECTORY" || {

gitgitgadget

From: Philippe Blain <[email protected]>

The 'debug' function in test-lib-functions.sh is used to invoke a debugger at a specific line in a test. It inherits the value of HOME and TERM set by 'test-lib.sh': HOME="$TRASH_DIRECTORY" and TERM=dumb.

Changing the value of HOME means that any customization configured in a developers' debugger configuration file (like $HOME/.gdbinit or $HOME/.lldbinit) are not available in the debugger invoked by 'test_pause'.

Changing the value of TERM to 'dumb' means that colored output is disabled in the debugger.

To make the debugging experience with 'debug' more pleasant, leverage the variable USER_HOME, added in the previous commit, to copy a developer's ~/.gdbinit and ~/.lldbinit to the test HOME. We do not set HOME to USER_HOME as in 'test_pause' to avoid user configuration in $USER_HOME/.gitconfig from interfering with the command being debugged.

Also, add a flag to launch the debugger with the original value of TERM, and add the same warning as for 'test_pause'.

---
 t/README                |  6 +++--
 t/test-lib-functions.sh | 60 +++++++++++++++++++++++++++++++----------
 2 files changed, 50 insertions(+), 16 deletions(-)

index cc8be6e67ad..e924bd81e2d 100644
@@ -800,10 +800,12 @@ see test-lib-functions.sh for the full list and their options.
    argument.  This is primarily meant for use during the
    development of a new test script.
 
- - debug <git-command>
+ - debug [options] <git-command>
 
    Run a git command inside a debugger. This is primarily meant for
-   use when debugging a failing test script.
+   use when debugging a failing test script. With '-t', use your
+   original TERM instead of test-lib.sh's "dumb", so that your
+   debugger interface has colors.
 
  - test_done
 

gitgitgadget

This round looks good to me (just eyeballed, didn't download or test); you have an ack from me.