[PATCH 0/2] 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 <redacted>

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.

Signed-off-by: Philippe Blain <redacted>
---
 t/test-lib-functions.sh | 2 +-
 t/test-lib.sh           | 6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index e28411bb75a..662cfc4c3e0 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh

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

diff --git a/t/test-lib.sh b/t/test-lib.sh
index abcfbed6d61..132618991e2 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

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

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.

Signed-off-by: Philippe Blain <redacted>
---
 t/test-lib-functions.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 662cfc4c3e0..86680b1177d 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh

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

On Thu, Aug 19, 2021 at 1:16 PM Philippe Blain via GitGitGadget
[off-list ref] wrote:

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.

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

On Thu, Aug 19, 2021 at 05:16:35PM +0000, Philippe Blain via GitGitGadget wrote:

I also share some concerns about setting $HOME here (though less than in
test_pause), but forwarding $USER_TERM down would be so nice. I have a
muscle memory of 'tui enable' for anything besides absolutely trivial
debugging, and it's always so frustrating to see:

    (gdb) tui enable
    Cannot enable the TUI: terminal doesn't support cursor addressing [TERM=dumb]

So I would welcome even just that part of this change.

Thanks,
Taylor

On Thu, Aug 19, 2021 at 11:10 AM Eric Sunshine [off-list ref] wrote:

On Thu, Aug 19, 2021 at 1:16 PM Philippe Blain via GitGitGadget
[off-list ref] wrote:

quoted

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.

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.

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?

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

On Thu, Aug 19, 2021 at 4:03 PM Elijah Newren [off-list ref] wrote:

On Thu, Aug 19, 2021 at 11:10 AM Eric Sunshine [off-list ref] wrote:

quoted

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.

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.

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.

On Thu, Aug 19, 2021 at 10:17 AM Philippe Blain via GitGitGadget
[off-list ref] wrote:

From: Philippe Blain <redacted>

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.

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

On Thu, Aug 19, 2021 at 12:25 PM Taylor Blau [off-list ref] wrote:

On Thu, Aug 19, 2021 at 05:16:35PM +0000, Philippe Blain via GitGitGadget wrote:

quoted

Signed-off-by: Philippe Blain <redacted>
---
 t/test-lib-functions.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 662cfc4c3e0..86680b1177d 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh

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

I also share some concerns about setting $HOME here (though less than in
test_pause)

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 :

On Thu, Aug 19, 2021 at 4:03 PM Elijah Newren [off-list ref] wrote:

quoted

On Thu, Aug 19, 2021 at 11:10 AM Eric Sunshine [off-list ref] wrote:

quoted

quoted

In such a
scenario, HOME must be pointing at the test's home directory, not at
my real home directory.

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.

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.

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 :

On Thu, Aug 19, 2021 at 10:17 AM Philippe Blain via GitGitGadget
[off-list ref] wrote:

quoted

From: Philippe Blain <redacted>

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.

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

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

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

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

On Fri, Aug 20, 2021 at 8:12 AM Philippe Blain
[off-list ref] wrote:

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

quoted

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

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.

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

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.

On Fri, Aug 20, 2021 at 08:12:50AM -0400, Philippe Blain wrote:

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.

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 [off-list ref]
      
       ## 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.
      
          Signed-off-by: Philippe Blain [off-list ref]
      
       ## 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 <redacted>

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.

Suggested-by: Carlo Marcelo Arenas Belón <redacted>
Signed-off-by: Philippe Blain <redacted>
---
 t/test-lib-functions.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index e28411bb75a..1884177e293 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh

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

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.

Signed-off-by: Philippe Blain <redacted>
---
 t/test-lib-functions.sh | 37 ++++++++++++++++++++++++++++++++++++-
 t/test-lib.sh           |  6 ++++--
 2 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 1884177e293..1b775343adc 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh

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

diff --git a/t/test-lib.sh b/t/test-lib.sh
index abcfbed6d61..132618991e2 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

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

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.

Signed-off-by: Philippe Blain <redacted>
---
 t/test-lib-functions.sh | 54 ++++++++++++++++++++++++++++++-----------
 1 file changed, 40 insertions(+), 14 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 1b775343adc..0f6d30e2b3f 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh

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

On Fri, Aug 27, 2021 at 5:47 PM Philippe Blain via GitGitGadget
[off-list ref] wrote:

Do you want to add an option that implies the other three, so that
folks can do something like `test_pause -a` (or some other single
letter) rather than `test_pause -t -s -h`?

Hi Elijah,

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

On Fri, Aug 27, 2021 at 5:47 PM Philippe Blain via GitGitGadget
[off-list ref] wrote:

quoted

+#
+# 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.

Do you want to add an option that implies the other three, so that
folks can do something like `test_pause -a` (or some other single
letter) rather than `test_pause -t -s -h`?

quoted

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

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.

Signed-off-by: Philippe Blain <redacted>
---
 t/test-lib-functions.sh | 44 ++++++++++++++++++++++++++++++++++++++++-
 t/test-lib.sh           |  6 ++++--
 2 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 1884177e293..4b667dc7e76 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh

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

diff --git a/t/test-lib.sh b/t/test-lib.sh
index abcfbed6d61..132618991e2 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

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

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.

Suggested-by: Carlo Marcelo Arenas Belón <redacted>
Signed-off-by: Philippe Blain <redacted>
---
 t/test-lib-functions.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index e28411bb75a..1884177e293 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh

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

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.

Signed-off-by: Philippe Blain <redacted>
---
 t/test-lib-functions.sh | 60 +++++++++++++++++++++++++++++++----------
 1 file changed, 46 insertions(+), 14 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 4b667dc7e76..537fd88e686 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh

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

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/xmqqa6kvoptx.fsf@gitster.g/

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 [off-list ref]
          Signed-off-by: Philippe Blain [off-list ref]
      
     + ## 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 [off-list ref]
      
       ## 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 [off-list ref]
          Signed-off-by: Philippe Blain [off-list ref]
      
     + ## 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 <redacted>

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.

Suggested-by: Carlo Marcelo Arenas Belón <redacted>
Signed-off-by: Philippe Blain <redacted>
---
 t/test-lib-functions.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index e28411bb75a..1884177e293 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh

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

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.

Helped-by: Elijah Newren [off-list ref]
Signed-off-by: Philippe Blain <redacted>
---
 t/README                |  5 +++--
 t/test-lib-functions.sh | 47 ++++++++++++++++++++++++++++++++++++++++-
 t/test-lib.sh           |  6 ++++--
 3 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/t/README b/t/README
index 9e701223020..cc8be6e67ad 100644
--- a/t/README
+++ b/t/README

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

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 1884177e293..5bed34e47e0 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh

@@ -137,9 +137,54 @@ 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.
+#   -s
+#	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.
+#	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
 
 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

diff --git a/t/test-lib.sh b/t/test-lib.sh
index abcfbed6d61..132618991e2 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

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

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

Helped-by: Carlo Marcelo Arenas Belón [off-list ref]
Signed-off-by: Philippe Blain <redacted>
---
 t/README                |  6 ++--
 t/test-lib-functions.sh | 66 ++++++++++++++++++++++++++++++++---------
 2 files changed, 56 insertions(+), 16 deletions(-)

diff --git a/t/README b/t/README
index cc8be6e67ad..e924bd81e2d 100644
--- a/t/README
+++ b/t/README

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

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 5bed34e47e0..8cec0e986e1 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh

@@ -190,25 +190,63 @@ 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: the command being debugged might behave differently than when
+#	running the test.
+#
 # 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 &&
+	while test $# != 0
+	do
+		case "$1" in
+		-t)
+			DEBUG_TERM="$USER_TERM"
+			;;
+		-d)
+			GIT_DEBUGGER="$2" &&
+			shift
+			;;
+		--debugger=*)
+			GIT_DEBUGGER="${1#*=}"
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done &&
+
+	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

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/xmqqa6kvoptx.fsf@gitster.g/

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

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.

Suggested-by: Carlo Marcelo Arenas Belón <redacted>
Signed-off-by: Philippe Blain <redacted>
---
 t/test-lib-functions.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index e28411bb75a..1884177e293 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh

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

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.

Helped-by: Elijah Newren [off-list ref]
Signed-off-by: Philippe Blain <redacted>
---
 t/README                |  5 +++--
 t/test-lib-functions.sh | 47 ++++++++++++++++++++++++++++++++++++++++-
 t/test-lib.sh           |  6 ++++--
 3 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/t/README b/t/README
index 9e701223020..cc8be6e67ad 100644
--- a/t/README
+++ b/t/README

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

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 1884177e293..5bed34e47e0 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh

@@ -137,9 +137,54 @@ 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.
+#   -s
+#	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.
+#	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
 
 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

diff --git a/t/test-lib.sh b/t/test-lib.sh
index abcfbed6d61..132618991e2 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

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

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

Helped-by: Carlo Marcelo Arenas Belón [off-list ref]
Signed-off-by: Philippe Blain <redacted>
---
 t/README                |  6 +++--
 t/test-lib-functions.sh | 60 +++++++++++++++++++++++++++++++----------
 2 files changed, 50 insertions(+), 16 deletions(-)

diff --git a/t/README b/t/README
index cc8be6e67ad..e924bd81e2d 100644
--- a/t/README
+++ b/t/README

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

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 5bed34e47e0..eef2262a360 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh

@@ -190,25 +190,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: the command being debugged might behave differently than when
+#	running the test.
+#
 # 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 &&
+	while test $# != 0
+	do
+		case "$1" in
+		-t)
+			DEBUG_TERM="$USER_TERM"
+			;;
+		-d)
+			GIT_DEBUGGER="$2" &&
+			shift
+			;;
+		--debugger=*)
+			GIT_DEBUGGER="${1#*=}"
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done &&
+
+	dotfiles=".gdbinit .lldbinit"
+
+	for dotfile in $dotfiles
+	do
+		dotfile="$USER_HOME/$dotfile" &&
+		test -f "$dotfile" && cp "$dotfile" "$HOME" || :
+	done &&
+
+	TERM="$DEBUG_TERM" GIT_DEBUGGER="${GIT_DEBUGGER}" "$@" <&6 >&5 2>&7 &&
+
+	for dotfile in $dotfiles
+	do
+		rm -f "$HOME/$dotfile"
+	done
 }
 
 # Usage: test_commit [options] <message> [<file> [<contents> [<tag>]]]

-- 
gitgitgadget

On Sun, Sep 5, 2021 at 9:39 PM Philippe Blain via GitGitGadget
[off-list ref] wrote:

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/xmqqa6kvoptx.fsf@gitster.g/

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.

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

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