git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH] tests: create an interactive gdb session with the 'debug' helper
@ 2017-03-17 14:46 SZEDER Gábor
  2017-03-17 14:55 ` Jeff King
  0 siblings, 1 reply; 9+ messages in thread
From: SZEDER Gábor @ 2017-03-17 14:46 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, Johannes Schindelin, git, SZEDER Gábor

The 'debug' test helper is supposed to facilitate debugging by running
a command of the test suite under gdb.  Unfortunately, its usefulness
is severely limited, because that gdb session is not interactive,
since the test's, and thus gdb's standard input is attached to
/dev/null (for a good reason, see 781f76b15 (test-lib: redirect stdin
of tests, 2011-12-15)).

Re-attach the original standard input in the debug helper, thus
creating an interactive gdb session, which is much much more useful.

Signed-off-by: SZEDER Gábor <szeder.dev@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 bd357704c..9567dc986 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -154,7 +154,7 @@ test_pause () {
 #
 # Example: "debug git checkout master".
 debug () {
-	 GIT_TEST_GDB=1 "$@"
+	 GIT_TEST_GDB=1 "$@" <&6
 }
 
 # Call test_commit with the arguments
-- 
2.12.0.375.g7a2ebd56b


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] tests: create an interactive gdb session with the 'debug' helper
  2017-03-17 14:46 [PATCH] tests: create an interactive gdb session with the 'debug' helper SZEDER Gábor
@ 2017-03-17 14:55 ` Jeff King
  2017-03-18 16:10   ` SZEDER Gábor
  0 siblings, 1 reply; 9+ messages in thread
From: Jeff King @ 2017-03-17 14:55 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Junio C Hamano, Johannes Schindelin, git

On Fri, Mar 17, 2017 at 03:46:46PM +0100, SZEDER Gábor wrote:

> The 'debug' test helper is supposed to facilitate debugging by running
> a command of the test suite under gdb.  Unfortunately, its usefulness
> is severely limited, because that gdb session is not interactive,
> since the test's, and thus gdb's standard input is attached to
> /dev/null (for a good reason, see 781f76b15 (test-lib: redirect stdin
> of tests, 2011-12-15)).
> 
> Re-attach the original standard input in the debug helper, thus
> creating an interactive gdb session, which is much much more useful.

Yeah, I think I've had to do a similar hack manually. This is an obvious
improvement. Though...

> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index bd357704c..9567dc986 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -154,7 +154,7 @@ test_pause () {
>  #
>  # Example: "debug git checkout master".
>  debug () {
> -	 GIT_TEST_GDB=1 "$@"
> +	 GIT_TEST_GDB=1 "$@" <&6
>  }

Your stderr and stdout may be redirected, too. That's usually not a big
problem because you'll be using "-v", but perhaps this should add:

  >&3 2>&4

to be on the safe side. That covers running without "-v", as well as
cases where the test script is redirecting output (I _actually_ wish gdb
behaved like perl's debugger and unconditionally used the terminal,
retaining existing redirections, but I AFAIK there's no easy way to get
it to do that).

-Peff

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] tests: create an interactive gdb session with the 'debug' helper
  2017-03-17 14:55 ` Jeff King
@ 2017-03-18 16:10   ` SZEDER Gábor
  2017-03-18 16:13     ` [PATCHv2 1/2] " SZEDER Gábor
  2017-03-20  4:29     ` [PATCH] " Jeff King
  0 siblings, 2 replies; 9+ messages in thread
From: SZEDER Gábor @ 2017-03-18 16:10 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, Johannes Schindelin, Git mailing list

On Fri, Mar 17, 2017 at 3:55 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Mar 17, 2017 at 03:46:46PM +0100, SZEDER Gábor wrote:
>
>> The 'debug' test helper is supposed to facilitate debugging by running
>> a command of the test suite under gdb.  Unfortunately, its usefulness
>> is severely limited, because that gdb session is not interactive,
>> since the test's, and thus gdb's standard input is attached to
>> /dev/null (for a good reason, see 781f76b15 (test-lib: redirect stdin
>> of tests, 2011-12-15)).
>>
>> Re-attach the original standard input in the debug helper, thus
>> creating an interactive gdb session, which is much much more useful.
>
> Yeah, I think I've had to do a similar hack manually. This is an obvious
> improvement. Though...
>
>> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
>> index bd357704c..9567dc986 100644
>> --- a/t/test-lib-functions.sh
>> +++ b/t/test-lib-functions.sh
>> @@ -154,7 +154,7 @@ test_pause () {
>>  #
>>  # Example: "debug git checkout master".
>>  debug () {
>> -      GIT_TEST_GDB=1 "$@"
>> +      GIT_TEST_GDB=1 "$@" <&6
>>  }
>
> Your stderr and stdout may be redirected, too. That's usually not a big
> problem because you'll be using "-v",

Yeah, I didn't consider using this helper in non-verbose mode at all,
and the resulting behaviour is not quite pleasant, indeed: gdb starts
in interactive mode, but as its stdout goes straight to /dev/null the
user has no idea, and gdb does not quit on Ctrl-C.  A possible
solution would be to forbid using the 'debug' helper without '-v',
like in the 'test_pause' helper just above.

However...

> but perhaps this should add:
>
>   >&3 2>&4
>
> to be on the safe side. That covers running without "-v", as well as
> cases where the test script is redirecting output

That wouldn't buy us much.  First, file descriptors 3 and 4 are the
test's stdout and stderr, i.e. any process started within the test
would be connected to those fds anyway without those explicit
redirections.  Second, fds 3 and 4 are redirected to /dev/null in
non-verbose mode, so it would still not work without '-v'.

Perhaps you meant '>&5 2>&7' here (and in the bash example in the
commit message of 781f76b15 (test-lib: redirect stdin of tests,
2011-12-15), for that matter)?  Those redirections do in fact make
'debug' work even without '-v'.  Furthermore, those redirections do
make 'test_pause' work without '-v', too.

So this is what v2 will do, then.

Gábor

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCHv2 1/2] tests: create an interactive gdb session with the 'debug' helper
  2017-03-18 16:10   ` SZEDER Gábor
@ 2017-03-18 16:13     ` " SZEDER Gábor
  2017-03-18 16:14       ` [PATCHv2 2/2] tests: make the 'test_pause' helper work in non-verbose mode SZEDER Gábor
                         ` (2 more replies)
  2017-03-20  4:29     ` [PATCH] " Jeff King
  1 sibling, 3 replies; 9+ messages in thread
From: SZEDER Gábor @ 2017-03-18 16:13 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, Johannes Schindelin, git, SZEDER Gábor

The 'debug' test helper is supposed to facilitate debugging by running
a command of the test suite under gdb.  Unfortunately, its usefulness
is severely limited, because that gdb session is not interactive,
since the test's, and thus gdb's standard input is redirected from
/dev/null (for a good reason, see 781f76b15 (test-lib: redirect stdin
of tests, 2011-12-15)).

Redirect gdb's standard file descriptors from/to the test
environment's stdin, stdout and stderr in the 'debug' helper, thus
creating an interactive gdb session (even in non-verbose mode), which
is much, much more useful.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/test-lib-functions.sh | 2 +-
 t/test-lib.sh           | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index bd357704c..4a50a6104 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -154,7 +154,7 @@ test_pause () {
 #
 # Example: "debug git checkout master".
 debug () {
-	 GIT_TEST_GDB=1 "$@"
+	 GIT_TEST_GDB=1 "$@" <&6 >&5 2>&7
 }
 
 # Call test_commit with the arguments
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 86d77c16d..23c29bce6 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -342,6 +342,7 @@ fi
 
 exec 5>&1
 exec 6<&0
+exec 7>&2
 if test "$verbose_log" = "t"
 then
 	exec 3>>"$GIT_TEST_TEE_OUTPUT_FILE" 4>&3
-- 
2.12.0.377.g15f6ffe90


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCHv2 2/2] tests: make the 'test_pause' helper work in non-verbose mode
  2017-03-18 16:13     ` [PATCHv2 1/2] " SZEDER Gábor
@ 2017-03-18 16:14       ` SZEDER Gábor
  2017-03-20  4:32         ` Jeff King
  2017-03-20  4:31       ` [PATCHv2 1/2] tests: create an interactive gdb session with the 'debug' helper Jeff King
  2017-03-21 11:07       ` Johannes Schindelin
  2 siblings, 1 reply; 9+ messages in thread
From: SZEDER Gábor @ 2017-03-18 16:14 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, Johannes Schindelin, git, SZEDER Gábor

When the 'test_pause' helper function invokes the shell mid-test, it
explicitly redirects the shell's stdout and stderr to file descriptors
3 and 4, which are the stdout and stderr of the tests (i.e. where they
would be connected anyway without those redirections).  These file
descriptors are only attached to the terminal in verbose mode, hence
the restriction of 'test_pause' to work only with '-v'.

Redirect the shell's stdout and stderr to the test environment's
original stdout and stderr, allowing it to work properly even in
non-verbose mode, and the restriction can be lifted.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/test-lib-functions.sh | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 4a50a6104..5ee124332 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -136,17 +136,12 @@ test_tick () {
 	export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
 }
 
-# Stop execution and start a shell. This is useful for debugging tests and
-# only makes sense together with "-v".
+# Stop execution and start a shell. This is useful for debugging tests.
 #
 # Be sure to remove all invocations of this command before submitting.
 
 test_pause () {
-	if test "$verbose" = t; then
-		"$SHELL_PATH" <&6 >&3 2>&4
-	else
-		error >&5 "test_pause requires --verbose"
-	fi
+	"$SHELL_PATH" <&6 >&5 2>&7
 }
 
 # Wrap git in gdb. Adding this to a command can make it easier to
-- 
2.12.0.377.g15f6ffe90


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] tests: create an interactive gdb session with the 'debug' helper
  2017-03-18 16:10   ` SZEDER Gábor
  2017-03-18 16:13     ` [PATCHv2 1/2] " SZEDER Gábor
@ 2017-03-20  4:29     ` " Jeff King
  1 sibling, 0 replies; 9+ messages in thread
From: Jeff King @ 2017-03-20  4:29 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Junio C Hamano, Johannes Schindelin, Git mailing list

On Sat, Mar 18, 2017 at 05:10:18PM +0100, SZEDER Gábor wrote:

> > but perhaps this should add:
> >
> >   >&3 2>&4
> >
> > to be on the safe side. That covers running without "-v", as well as
> > cases where the test script is redirecting output
> 
> That wouldn't buy us much.  First, file descriptors 3 and 4 are the
> test's stdout and stderr, i.e. any process started within the test
> would be connected to those fds anyway without those explicit
> redirections.  Second, fds 3 and 4 are redirected to /dev/null in
> non-verbose mode, so it would still not work without '-v'.
> 
> Perhaps you meant '>&5 2>&7' here (and in the bash example in the
> commit message of 781f76b15 (test-lib: redirect stdin of tests,
> 2011-12-15), for that matter)?  Those redirections do in fact make
> 'debug' work even without '-v'.  Furthermore, those redirections do
> make 'test_pause' work without '-v', too.

Yeah, sorry, I got my descriptors mixed up. ">&5 2>&1" is the right
invocation (unless you add a new "7" to save the original stderr).

-Peff

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCHv2 1/2] tests: create an interactive gdb session with the 'debug' helper
  2017-03-18 16:13     ` [PATCHv2 1/2] " SZEDER Gábor
  2017-03-18 16:14       ` [PATCHv2 2/2] tests: make the 'test_pause' helper work in non-verbose mode SZEDER Gábor
@ 2017-03-20  4:31       ` Jeff King
  2017-03-21 11:07       ` Johannes Schindelin
  2 siblings, 0 replies; 9+ messages in thread
From: Jeff King @ 2017-03-20  4:31 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Junio C Hamano, Johannes Schindelin, git

On Sat, Mar 18, 2017 at 05:13:59PM +0100, SZEDER Gábor wrote:

> The 'debug' test helper is supposed to facilitate debugging by running
> a command of the test suite under gdb.  Unfortunately, its usefulness
> is severely limited, because that gdb session is not interactive,
> since the test's, and thus gdb's standard input is redirected from
> /dev/null (for a good reason, see 781f76b15 (test-lib: redirect stdin
> of tests, 2011-12-15)).
> 
> Redirect gdb's standard file descriptors from/to the test
> environment's stdin, stdout and stderr in the 'debug' helper, thus
> creating an interactive gdb session (even in non-verbose mode), which
> is much, much more useful.
>
> [...]
>
>  debug () {
> -	 GIT_TEST_GDB=1 "$@"
> +	 GIT_TEST_GDB=1 "$@" <&6 >&5 2>&7
>  }

Yep, this looks fine to me.

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 86d77c16d..23c29bce6 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -342,6 +342,7 @@ fi
>  
>  exec 5>&1
>  exec 6<&0
> +exec 7>&2

And this obviously is required to keep stdout/stderr separate.

-Peff

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCHv2 2/2] tests: make the 'test_pause' helper work in non-verbose mode
  2017-03-18 16:14       ` [PATCHv2 2/2] tests: make the 'test_pause' helper work in non-verbose mode SZEDER Gábor
@ 2017-03-20  4:32         ` Jeff King
  0 siblings, 0 replies; 9+ messages in thread
From: Jeff King @ 2017-03-20  4:32 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Junio C Hamano, Johannes Schindelin, git

On Sat, Mar 18, 2017 at 05:14:00PM +0100, SZEDER Gábor wrote:

> When the 'test_pause' helper function invokes the shell mid-test, it
> explicitly redirects the shell's stdout and stderr to file descriptors
> 3 and 4, which are the stdout and stderr of the tests (i.e. where they
> would be connected anyway without those redirections).  These file
> descriptors are only attached to the terminal in verbose mode, hence
> the restriction of 'test_pause' to work only with '-v'.
> 
> Redirect the shell's stdout and stderr to the test environment's
> original stdout and stderr, allowing it to work properly even in
> non-verbose mode, and the restriction can be lifted.
> 
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  t/test-lib-functions.sh | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)

This makes sense, too. Thanks.

-Peff

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCHv2 1/2] tests: create an interactive gdb session with the 'debug' helper
  2017-03-18 16:13     ` [PATCHv2 1/2] " SZEDER Gábor
  2017-03-18 16:14       ` [PATCHv2 2/2] tests: make the 'test_pause' helper work in non-verbose mode SZEDER Gábor
  2017-03-20  4:31       ` [PATCHv2 1/2] tests: create an interactive gdb session with the 'debug' helper Jeff King
@ 2017-03-21 11:07       ` Johannes Schindelin
  2 siblings, 0 replies; 9+ messages in thread
From: Johannes Schindelin @ 2017-03-21 11:07 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Junio C Hamano, Jeff King, git

[-- Attachment #1: Type: text/plain, Size: 922 bytes --]

Hi Gábor,

On Sat, 18 Mar 2017, SZEDER Gábor wrote:

> The 'debug' test helper is supposed to facilitate debugging by running
> a command of the test suite under gdb.  Unfortunately, its usefulness
> is severely limited, because that gdb session is not interactive,
> since the test's, and thus gdb's standard input is redirected from
> /dev/null (for a good reason, see 781f76b15 (test-lib: redirect stdin
> of tests, 2011-12-15)).
> 
> Redirect gdb's standard file descriptors from/to the test
> environment's stdin, stdout and stderr in the 'debug' helper, thus
> creating an interactive gdb session (even in non-verbose mode), which
> is much, much more useful.

Thank you for working on this!

I meant to clean up my local patch that tried to avoid redirection
altogether in the case of `debug`, but I never could make it elegant
enough for my taste. Your approach is much better!

ACK,
Dscho

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-17 14:46 [PATCH] tests: create an interactive gdb session with the 'debug' helper SZEDER Gábor
2017-03-17 14:55 ` Jeff King
2017-03-18 16:10   ` SZEDER Gábor
2017-03-18 16:13     ` [PATCHv2 1/2] " SZEDER Gábor
2017-03-18 16:14       ` [PATCHv2 2/2] tests: make the 'test_pause' helper work in non-verbose mode SZEDER Gábor
2017-03-20  4:32         ` Jeff King
2017-03-20  4:31       ` [PATCHv2 1/2] tests: create an interactive gdb session with the 'debug' helper Jeff King
2017-03-21 11:07       ` Johannes Schindelin
2017-03-20  4:29     ` [PATCH] " Jeff King

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox