[PATCH 0/2] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'

[PATCH 0/2] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'

[Philippe Blain via GitGitGadget]

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

[PATCH 1/2] test-lib-functions: use user's SHELL, HOME and TERM for 'test_pause'

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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 <levraiphilippeblain@gmail.com>
---
 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

Re: [PATCH 1/2] test-lib-functions: use user's SHELL, HOME and TERM for 'test_pause'

[Carlo Arenas]

On Thu, Aug 19, 2021 at 10:17 AM Philippe Blain via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> 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

Re: [PATCH 1/2] test-lib-functions: use user's SHELL, HOME and TERM for 'test_pause'

[Philippe Blain]

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
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Philippe Blain <levraiphilippeblain@gmail.com>
>>
>> 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 :)

[PATCH 2/2] test-lib-functions: use user's TERM and HOME for 'debug'

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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 <levraiphilippeblain@gmail.com>
---
 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

Re: [PATCH 2/2] test-lib-functions: use user's TERM and HOME for 'debug'

[Taylor Blau]

On Thu, Aug 19, 2021 at 05:16:35PM +0000, Philippe Blain via GitGitGadget wrote:
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
>  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), 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

Re: [PATCH 2/2] test-lib-functions: use user's TERM and HOME for 'debug'

[Carlo Arenas]

On Thu, Aug 19, 2021 at 12:25 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Thu, Aug 19, 2021 at 05:16:35PM +0000, Philippe Blain via GitGitGadget wrote:
> > Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> > ---
> >  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.

Re: [PATCH 0/2] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'

[Eric Sunshine]

On Thu, Aug 19, 2021 at 1:16 PM Philippe Blain via GitGitGadget
<gitgitgadget@gmail.com> 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

Re: [PATCH 0/2] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

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

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

Re: [PATCH 0/2] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'

[Eric Sunshine]

On Thu, Aug 19, 2021 at 3:58 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > 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.
>
> 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 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).

Re: [PATCH 0/2] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'

[Elijah Newren]

On Thu, Aug 19, 2021 at 11:10 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Thu, Aug 19, 2021 at 1:16 PM Philippe Blain via GitGitGadget
> <gitgitgadget@gmail.com> 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.

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

Re: [PATCH 0/2] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'

[Eric Sunshine]

On Thu, Aug 19, 2021 at 4:03 PM Elijah Newren <newren@gmail.com> wrote:
> On Thu, Aug 19, 2021 at 11:10 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > 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.

Re: [PATCH 0/2] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'

[Philippe Blain]

Hi everyone,

Le 2021-08-19 à 16:11, Eric Sunshine a écrit :
> On Thu, Aug 19, 2021 at 4:03 PM Elijah Newren <newren@gmail.com> wrote:
>> On Thu, Aug 19, 2021 at 11:10 AM Eric Sunshine <sunshine@sunshineco.com> wrote:

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

Re: [PATCH 0/2] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'

[Eric Sunshine]

On Fri, Aug 20, 2021 at 8:12 AM Philippe Blain
<levraiphilippeblain@gmail.com> wrote:
> Le 2021-08-19 à 16:11, Eric Sunshine a écrit :
> > 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.

Re: [PATCH 0/2] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'

[Jeff King]

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

[PATCH v2 0/3] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'

[Philippe Blain via GitGitGadget]

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 <levraiphilippeblain@gmail.com>
      
       ## 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 <levraiphilippeblain@gmail.com>
      
       ## 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

[PATCH v2 1/3] test-lib-functions: use 'TEST_SHELL_PATH' in 'test_pause'

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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 <carenas@gmail.com>
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 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

[PATCH v2 2/3] test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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 <levraiphilippeblain@gmail.com>
---
 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

Re: [PATCH v2 2/3] test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'

[Elijah Newren]

On Fri, Aug 27, 2021 at 5:47 PM Philippe Blain via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> 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 <levraiphilippeblain@gmail.com>
> ---
>  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.

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

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

Re: [PATCH v2 2/3] test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'

[Philippe Blain]

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
> <gitgitgadget@gmail.com> wrote:
>> +#
>> +# 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`?
> 
>>
Yes, that would be even better. I'll add that for next version.

Thanks,

Philippe.

[PATCH v2 3/3] test-lib-functions: optionally keep HOME and TERM in 'debug'

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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 <levraiphilippeblain@gmail.com>
---
 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

[PATCH v3 0/3] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'

[Philippe Blain via GitGitGadget]

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

[PATCH v3 1/3] test-lib-functions: use 'TEST_SHELL_PATH' in 'test_pause'

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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 <carenas@gmail.com>
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 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

Re: [PATCH v3 1/3] test-lib-functions: use 'TEST_SHELL_PATH' in 'test_pause'

[Junio C Hamano]

"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> 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 <carenas@gmail.com>
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
>  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

Sensible.

[PATCH v3 2/3] test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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 <levraiphilippeblain@gmail.com>
---
 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

Re: [PATCH v3 2/3] test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'

[Junio C Hamano]

"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

As this is not end-user facing but facing our developer, I do not
deeply care, but I find the warnings in this help text problematic.

Because a new process instance of $PAUSE_SHELL is run, the options
you add when inserting test_pause does not affect what happens in
the tests after you exit the $PAUSE_SHELL [*], right?

Of course, you can modify the repository or the working tree files
used in the test in the $PAUSE_SHELL, and that can break "test
reproducibility"---if you run "git ls-files -s" and take its output
in a temporary file, a step later in the test that runs "git status"
will see an extra untracked file, for example, and such a change may
(or may not) unnecessarily break the tests.

But it is not anything new introduced by these options.  It is
inherent to test_pause itself.

If we want to add a warning to the help text here, I think it should
be written in such a way that it is clear that the warning applies
to any use of the test_pause helper, not just to the form with the
options.

Thanks.


[Footnote]

* If we had an alternative implementation of test_pause that does
  not use a separate $PAUSE_SHELL process, but instead like
  inserting a read-eval-print loop, that would be far more powerful
  and useful debugging aid.  You can not just stop the execution of
  the test, and observe the files in the test repository and the
  environment variables---you can also access shell variables and
  functions.

  Such a test_pause from another world would deserve a "if you touch
  anything, the damage is permanent" warning even more than the
  current one, because you can modify even a shell variable to
  affect the execution of the test after you leave the paused state.

Re: [PATCH v3 2/3] test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'

[Elijah Newren]

On Wed, Sep 1, 2021 at 1:26 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +# 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
>
> As this is not end-user facing but facing our developer, I do not
> deeply care, but I find the warnings in this help text problematic.
>
> Because a new process instance of $PAUSE_SHELL is run, the options
> you add when inserting test_pause does not affect what happens in
> the tests after you exit the $PAUSE_SHELL [*], right?

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

> Of course, you can modify the repository or the working tree files
> used in the test in the $PAUSE_SHELL, and that can break "test
> reproducibility"---if you run "git ls-files -s" and take its output
> in a temporary file, a step later in the test that runs "git status"
> will see an extra untracked file, for example, and such a change may
> (or may not) unnecessarily break the tests.
>
> But it is not anything new introduced by these options.  It is
> inherent to test_pause itself.
>
> If we want to add a warning to the help text here, I think it should
> be written in such a way that it is clear that the warning applies
> to any use of the test_pause helper, not just to the form with the
> options.
>
> Thanks.
>
>
> [Footnote]
>
> * If we had an alternative implementation of test_pause that does
>   not use a separate $PAUSE_SHELL process, but instead like
>   inserting a read-eval-print loop, that would be far more powerful
>   and useful debugging aid.  You can not just stop the execution of
>   the test, and observe the files in the test repository and the
>   environment variables---you can also access shell variables and
>   functions.
>
>   Such a test_pause from another world would deserve a "if you touch
>   anything, the damage is permanent" warning even more than the
>   current one, because you can modify even a shell variable to
>   affect the execution of the test after you leave the paused state.

Re: [PATCH v3 2/3] test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

>> Because a new process instance of $PAUSE_SHELL is run, the options
>> you add when inserting test_pause does not affect what happens in
>> the tests after you exit the $PAUSE_SHELL [*], right?
>
> 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.

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.

Re: [PATCH v3 2/3] test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'

[Philippe Blain]

Hi Elijah and Junio,

Le 2021-09-01 à 19:09, Junio C Hamano a écrit :
> Elijah Newren <newren@gmail.com> writes:
> 
>>> Because a new process instance of $PAUSE_SHELL is run, the options
>>> you add when inserting test_pause does not affect what happens in
>>> the tests after you exit the $PAUSE_SHELL [*], right?
>>
>> 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.

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

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

OK, I can make that tweak.


Le 2021-09-01 à 17:52, Elijah Newren a écrit :
-- snip --
> 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".

Thanks for these suggestions, that wording does seem clearer.

Philippe.

[PATCH v3 3/3] test-lib-functions: optionally keep HOME and TERM in 'debug'

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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 <levraiphilippeblain@gmail.com>
---
 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

[PATCH v4 0/3] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'

[Philippe Blain via 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 <newren@gmail.com>
          Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
      
     + ## 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 <levraiphilippeblain@gmail.com>
      
       ## 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 <carenas@gmail.com>
          Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
      
     + ## 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

[PATCH v4 1/3] test-lib-functions: use 'TEST_SHELL_PATH' in 'test_pause'

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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 <carenas@gmail.com>
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 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

[PATCH v4 2/3] test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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 <newren@gmail.com>
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 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

[PATCH v4 3/3] test-lib-functions: keep user's debugger config files and TERM in 'debug'

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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 <carenas@gmail.com>
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 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

[PATCH v5 0/3] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'

[Philippe Blain via 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

[PATCH v5 1/3] test-lib-functions: use 'TEST_SHELL_PATH' in 'test_pause'

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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 <carenas@gmail.com>
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 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

[PATCH v5 2/3] test-lib-functions: optionally keep HOME, TERM and SHELL in 'test_pause'

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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 <newren@gmail.com>
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 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

[PATCH v5 3/3] test-lib-functions: keep user's debugger config files and TERM in 'debug'

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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 <carenas@gmail.com>
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 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

Re: [PATCH v5 0/3] test-lib-functions.sh: keep user's HOME, TERM and SHELL for 'test_pause' and 'debug'

[Elijah Newren]

On Sun, Sep 5, 2021 at 9:39 PM Philippe Blain via GitGitGadget
<gitgitgadget@gmail.com> 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