RE: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Johannes Schindelin]

Hi Victor,

On Sat, 31 Oct 2015, Victor Leschuk wrote:

> > >   +if test -n "$TEST_GDB_GIT"
> > > +then
> > > +   exec gdb -args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
> > Maybe we could make $TEST_GDB_GIT not just a boolean flag? It would be 
> > useful
> > to contain "gdb" executable name. It would allow to set path to GDB when it
> > is not in $PATH, set different debuggers (for example, I usually use cgdb),
> > or even set it to /path/to/gdb_wrapper.sh which could contain different gdb
> > options and tunings.
> 
> > Sure, as long as TEST_GDB_GIT=1 still works. Why don't you make an add-on
> > patch and submit it?
> 
> Sure, I will prepare the patch as soon as this one is included in master.

Excuse my asking: why do you want to wait?

Ciao,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Victor Leschuk]

> >   +if test -n "$TEST_GDB_GIT"
> > +then
> > +   exec gdb -args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
> Maybe we could make $TEST_GDB_GIT not just a boolean flag? It would be useful
> to contain "gdb" executable name. It would allow to set path to GDB when it
> is not in $PATH, set different debuggers (for example, I usually use cgdb),
> or even set it to /path/to/gdb_wrapper.sh which could contain different gdb
> options and tunings.

> Sure, as long as TEST_GDB_GIT=1 still works. Why don't you make an add-on
> patch and submit it?

Hello Johannes,

Sure, I will prepare the patch as soon as this one is included in master.
--
Victor
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Jonathan Nieder]

Jeff King wrote:

> Somebody suggested elsewhere that the name "gdb" be configurable. We
> could stick that in the same variable, like:
>
>   test "$GIT_TEST_GDB" = 1 && GIT_TEST_GDB=gdb
>   exec ${GIT_TEST_GDB} --args ...
>
> but that does not play well with the "debug" function, which does not
> know which value to set it to. I guess we would need GIT_TEST_GDB_PATH
> or something.

*nod* I think having a separate variable like GIT_TEST_GDB_COMMAND
would be fine.

An interested person could also replace 'gdb --args' with 'lldb --' if
they want to.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Junio C Hamano]

Jeff King  writes:

> At the risk of repeating what I just said elsewhere in the thread, I
> think this patch is the best of the proposed solutions.

OK, will queue.  I agree that more could be built on top, instead of
polishing this further in place.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Johannes Schindelin]

Hi Jonathan,

On Fri, 30 Oct 2015, Jonathan Nieder wrote:

> Junio C Hamano wrote:
> > Johannes Schindelin  writes:
> >> On Tue, 27 Oct 2015, Junio C Hamano wrote:
> 
> >>> It can be called GDB=1, perhaps?
> >>
> >> No, this is way too generic. As I only test that the environment
> >> variable's existence, even something like GDB=/usr/opt/gdb/bin/gdb would
> >> trigger it.
> >>
> >> I could be talked into GDB_GIT=1, though.
> >
> > As I said in another message, I have no preference myself over the
> > name of this variable (or making it a shell function like Duy
> > mentioned, which incidentally may give us more visual pleasantness
> > by losing '=').
> >
> > I'd just be happy as long as the feature becomes available, and I'd
> > leave the choice of consistent and convenient naming to others who
> > have stronger opinions ;-)
> 
> Here's a suggested patch.

I am fine with this patch as a replacement for my original version.

Thanks,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Jeff King]

On Fri, Oct 30, 2015 at 07:25:24PM +0100, Johannes Schindelin wrote:

> > I agree doing so would be crazy. But would:
> > 
> >   ./t1234-frotz.sh --gdb=17
> > 
> > be sane to run gdb only inside test 17?
> 
> It would probably be sane, but I never encountered the need for something
> like that. It was always much easier to run the test using `sh -x t... -i
> -v` to find out what command was behaving funnily (mind you, that can be a
> pretty hard thing todo, we have some quite convoluted test scripts in our
> code base) and then edit the test.
> 
> I would expect that `--gdb=` thing to drive me crazy: first, I would
> choose the wrong number. Next, I would probably forget that test_commit
> and other commands *also* calls Git.

Yeah, good points. You somehow have to say "debug _this_ git
invocation", and there is probably not a more precise way to do that
than sticking something in the code on the right line.

I do think I like Junio's "debug git foo" rather than setting the
environment variable, as its syntactically a little simpler to type (and
of course it would probably be implemented with an environment variable,
so one could whichever style they prefer).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Johannes Schindelin]

Hi Victor,

On Thu, 29 Oct 2015, Victor Leschuk wrote:

> >   +if test -n "$TEST_GDB_GIT"
> > +then
> > +   exec gdb -args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
> Maybe we could make $TEST_GDB_GIT not just a boolean flag? It would be useful
> to contain "gdb" executable name. It would allow to set path to GDB when it
> is not in $PATH, set different debuggers (for example, I usually use cgdb),
> or even set it to /path/to/gdb_wrapper.sh which could contain different gdb
> options and tunings.

Sure, as long as TEST_GDB_GIT=1 still works. Why don't you make an add-on
patch and submit it?

Ciao,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Jonathan Nieder]

Johannes Schindelin wrote:
> On Tue, 27 Oct 2015, Johannes Schindelin wrote:
>> On Mon, 26 Oct 2015, Jonathan Nieder wrote:

>>> Does the 'exec' after the fi need this as well?  exec is supposed to
>>> itself print a message and exit when it runs into an error.
[...]
> Actually, after reading the patch again, I think it is better to be less
> intrusive and add the error message *just* for the gdb case, as it is
> right now:

Why?  Unlike the C library function of the same name, the shell
builtin 'exec' prints an error message and exits on error.

Sorry for the lack of clarity,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Jonathan Nieder]

Junio C Hamano wrote:
> Johannes Schindelin  writes:
>> On Tue, 27 Oct 2015, Junio C Hamano wrote:

>>> It can be called GDB=1, perhaps?
>>
>> No, this is way too generic. As I only test that the environment
>> variable's existence, even something like GDB=/usr/opt/gdb/bin/gdb would
>> trigger it.
>>
>> I could be talked into GDB_GIT=1, though.
>
> As I said in another message, I have no preference myself over the
> name of this variable (or making it a shell function like Duy
> mentioned, which incidentally may give us more visual pleasantness
> by losing '=').
>
> I'd just be happy as long as the feature becomes available, and I'd
> leave the choice of consistent and convenient naming to others who
> have stronger opinions ;-)

Here's a suggested patch.

-- >8 --
From: Johannes Schindelin 
Subject: Facilitate debugging Git executables in tests with gdb

When prefixing a Git call in the test suite with 'debug ', it will now
be run with GDB, allowing the developer to debug test failures more
conveniently.

Signed-off-by: Johannes Schindelin 
Signed-off-by: Jonathan Nieder 
---
 t/README| 5 +
 t/test-lib-functions.sh | 8 
 wrap-for-bin.sh | 8 +++-
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/t/README b/t/README
index 35438bc..1dc908e 100644
--- a/t/README
+++ b/t/README
@@ -563,6 +563,11 @@ library for your script to use.
argument.  This is primarily meant for use during the
development of a new test script.
 
+ - debug 
+
+   Run a git command inside a debugger. This is primarily meant for
+   use when debugging a failing test script.
+
  - test_done
 
Your test script must have test_done at the end.  Its purpose
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 6dffb8b..73e37a1 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -145,6 +145,14 @@ test_pause () {
fi
 }
 
+# Wrap git in gdb. Adding this to a command can make it easier to
+# understand what is going on in a failing test.
+#
+# Example: "debug git checkout master".
+debug () {
+GIT_TEST_GDB=1 "$@"
+}
+
 # Call test_commit with the arguments " [ [ []]]"
 #
 # This will commit a file with the given contents and the given commit
diff --git a/wrap-for-bin.sh b/wrap-for-bin.sh
index 701d233..db0ec6a 100644
--- a/wrap-for-bin.sh
+++ b/wrap-for-bin.sh
@@ -19,4 +19,10 @@ GIT_TEXTDOMAINDIR='@@BUILD_DIR@@/po/build/locale'
 PATH='@@BUILD_DIR@@/bin-wrappers:'"$PATH"
 export GIT_EXEC_PATH GITPERLLIB PATH GIT_TEXTDOMAINDIR
 
-exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"
+if test -n "$GIT_TEST_GDB"
+then
+   unset GIT_TEST_GDB
+   exec gdb --args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
+else
+   exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"
+fi
-- 
2.6.0.rc2.230.g3dd15c0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Johannes Schindelin]

Hi Peff,

On Tue, 27 Oct 2015, Jeff King wrote:

> On Tue, Oct 27, 2015 at 09:34:48AM -0700, Junio C Hamano wrote:
> 
> > Yeah, that was my first reaction when I saw this patch.  Instead of
> > having to munge that line to "gdb -whatever-args git", you can do a
> > single-shot debugging in a convenient way.  And quite honestly,
> > because nobody sane will run:
> > 
> >  $ cd t && TEST_GDB_GIT=1 sh ./t1234-frotz.sh
> > 
> > and can drive all the "git" running under gdb at the same time, I
> > think what you showed would be the _only_ practical use case.
> 
> I agree doing so would be crazy. But would:
> 
>   ./t1234-frotz.sh --gdb=17
> 
> be sane to run gdb only inside test 17?

It would probably be sane, but I never encountered the need for something
like that. It was always much easier to run the test using `sh -x t... -i
-v` to find out what command was behaving funnily (mind you, that can be a
pretty hard thing todo, we have some quite convoluted test scripts in our
code base) and then edit the test.

I would expect that `--gdb=` thing to drive me crazy: first, I would
choose the wrong number. Next, I would probably forget that test_commit
and other commands *also* calls Git.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Johannes Schindelin]

Hi Jonathan,

On Tue, 27 Oct 2015, Johannes Schindelin wrote:

> On Mon, 26 Oct 2015, Jonathan Nieder wrote:
> 
> > Does the 'exec' after the fi need this as well?  exec is supposed to
> > itself print a message and exit when it runs into an error.  Would
> > including an 'else' with the if make the control flow clearer?  E.g.
> > 
> > if test -n "$TEST_GDB_GIT"
> > then
> > exec gdb --args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
> > else
> > exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"
> > fi
> 
> I suppose you're right! The `exec` can fail easily, e.g. when `gdb` was
> not found.

Actually, after reading the patch again, I think it is better to be less
intrusive and add the error message *just* for the gdb case, as it is
right now:

-- snipsnap --
 export GIT_EXEC_PATH GITPERLLIB PATH GIT_TEXTDOMAINDIR
 
+if test -n "$TEST_GDB_GIT"
+then
+   exec gdb -args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
+   echo "Could not run gdb -args ${GIT_EXEC_PATH}/@@PROG@@ $*" >&2
+   exit 1
+fi
+
 exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Johannes Schindelin]

Hi Junio,

On Tue, 27 Oct 2015, Junio C Hamano wrote:

> It can be called GDB=1, perhaps?

No, this is way too generic. As I only test that the environment
variable's existence, even something like GDB=/usr/opt/gdb/bin/gdb would
trigger it.

I could be talked into GDB_GIT=1, though.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Junio C Hamano]

Johannes Schindelin  writes:

> Hi Junio,
>
> On Tue, 27 Oct 2015, Junio C Hamano wrote:
>
>> It can be called GDB=1, perhaps?
>
> No, this is way too generic. As I only test that the environment
> variable's existence, even something like GDB=/usr/opt/gdb/bin/gdb would
> trigger it.
>
> I could be talked into GDB_GIT=1, though.

As I said in another message, I have no preference myself over the
name of this variable (or making it a shell function like Duy
mentioned, which incidentally may give us more visual pleasantness
by losing '=').

I'd just be happy as long as the feature becomes available, and I'd
leave the choice of consistent and convenient naming to others who
have stronger opinions ;-)



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Jeff King]

On Fri, Oct 30, 2015 at 12:02:56PM -0700, Jonathan Nieder wrote:

> > I'd just be happy as long as the feature becomes available, and I'd
> > leave the choice of consistent and convenient naming to others who
> > have stronger opinions ;-)
> 
> Here's a suggested patch.
> 
> -- >8 --
> From: Johannes Schindelin 
> Subject: Facilitate debugging Git executables in tests with gdb
> 
> When prefixing a Git call in the test suite with 'debug ', it will now
> be run with GDB, allowing the developer to debug test failures more
> conveniently.

At the risk of repeating what I just said elsewhere in the thread, I
think this patch is the best of the proposed solutions.

> --- a/wrap-for-bin.sh
> +++ b/wrap-for-bin.sh
> @@ -19,4 +19,10 @@ GIT_TEXTDOMAINDIR='@@BUILD_DIR@@/po/build/locale'
>  PATH='@@BUILD_DIR@@/bin-wrappers:'"$PATH"
>  export GIT_EXEC_PATH GITPERLLIB PATH GIT_TEXTDOMAINDIR
>  
> -exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"
> +if test -n "$GIT_TEST_GDB"
> +then
> + unset GIT_TEST_GDB
> + exec gdb --args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
> +else
> + exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"
> +fi

Somebody suggested elsewhere that the name "gdb" be configurable. We
could stick that in the same variable, like:

  test "$GIT_TEST_GDB" = 1 && GIT_TEST_GDB=gdb
  exec ${GIT_TEST_GDB} --args ...

but that does not play well with the "debug" function, which does not
know which value to set it to. I guess we would need GIT_TEST_GDB_PATH
or something.

I am happy to let that get added later by interested parties (I am happy
with "gdb" myself). I just wanted to mention it to make sure we are not
painting ourselves into any corners.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Junio C Hamano]

Duy Nguyen  writes:

> On Mon, Oct 26, 2015 at 2:15 PM, Johannes Schindelin
>  wrote:
>> When prefixing a Git call in the test suite with 'TEST_GDB_GIT=1 ', it
>> will now be run with GDB, allowing the developer to debug test failures
>> more conveniently.
>
> I'm very slowly catching up with git traffic. Apologies if it's
> already mentioned elsewhere since I have only read this mail thread.
>
> Is it more convenient to add a sh function "gdb" instead?

Changing a line of git invocation you want to debug from

git frotz &&

to

debug git frotz &&

indeed is slightly more pleasing to the eye than

TEST_GDB_GIT=1 git frotz &&

I do not terribly care either way, as long as that feature is
availble ;-)

Either way these tweaks are temporary changes we make while figuring
out where things go wrong, and from that point of view, (1) the
longer and more cumbersome to type, the more cumbersome to use, but
(2) the longer and more visually identifiable, the easier to spot in
"diff" a tweak you forgot to revert before committing.

> We can even go further with supporting gdbserver function, to launch
> gdbserver, then I can debug from outside, works even without -v -i.

Yes, that may be useful, but you can do so whether you use your
shell function or TEST_GDB_GIT=1 that trigeers inside the "git"
wrapper in bin-wrappers, I would think.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Victor Leschuk]

  
+if test -n "$TEST_GDB_GIT"

+then
+   exec gdb -args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
Maybe we could make $TEST_GDB_GIT not just a boolean flag? It would be 
useful to contain "gdb" executable name. It would allow to set path to 
GDB when it is not in $PATH, set different debuggers (for example, I 
usually use cgdb), or even set it to /path/to/gdb_wrapper.sh which could 
contain different gdb options and tunings.


--
Victor
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Johannes Schindelin]

Hi Jonathan,

On Mon, 26 Oct 2015, Jonathan Nieder wrote:

> Johannes Schindelin wrote:
> 
> > --- a/wrap-for-bin.sh
> > +++ b/wrap-for-bin.sh
> > @@ -19,4 +19,11 @@ GIT_TEXTDOMAINDIR='@@BUILD_DIR@@/po/build/locale'
> >  PATH='@@BUILD_DIR@@/bin-wrappers:'"$PATH"
> >  export GIT_EXEC_PATH GITPERLLIB PATH GIT_TEXTDOMAINDIR
> >  
> > +if test -n "$TEST_GDB_GIT"
> > +then
> > +   exec gdb -args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
> 
> Most TEST_ environment variables that git respects are under
> GIT_TEST_* --- e.g., GIT_TEST_OPTS.  Should this match that pattern
> as well, for easier debugging with commands like 'env | grep GIT_'?

I dunno. This variable is most useful when inserted into the shell scripts
in t/ themselves, not when specified via the command line. For example, if
you have something like

test_expect_success '123' '
...
# This Git call somehow fails and I have no clue why
git push remote HEAD
...
'

then prefixing the `git push` command with `TEST_GDB_GIT=1` lets you use
`gdb` when running the test with the `-i` and `-v` flags.

Please note that `TEST_GDB_GIT` is already a major step up from my initial
`DDD`.

> What happens if the child in turn calls git again?  Should this
> unset TEST_GDB_GIT in gdb's environment?

It probably would call gdb again. Which is sometimes useful. But I have to
admit that I do not know whether that works.

> The gdb manual and --help output advertise "--args".  Has "-args"
> (with a single dash) always worked?

I always used it with a single dash... So I assume that it worked for a
long time (IIRC I used it first in 1994).

> > +   echo "Could not run gdb -args ${GIT_EXEC_PATH}/@@PROG@@ $*" >&2
> > +   exit 1
> 
> Does the 'exec' after the fi need this as well?  exec is supposed to
> itself print a message and exit when it runs into an error.  Would
> including an 'else' with the if make the control flow clearer?  E.g.
> 
>   if test -n "$TEST_GDB_GIT"
>   then
>   exec gdb --args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
>   else
>   exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"
>   fi

I suppose you're right! The `exec` can fail easily, e.g. when `gdb` was
not found.

Ciao,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Junio C Hamano]

Johannes Schindelin  writes:

>> Most TEST_ environment variables that git respects are under
>> GIT_TEST_* --- e.g., GIT_TEST_OPTS.  Should this match that pattern
>> as well, for easier debugging with commands like 'env | grep GIT_'?
>
> I dunno. This variable is most useful when inserted into the shell scripts
> in t/ themselves, not when specified via the command line. For example, if
> you have something like
>
>   test_expect_success '123' '
>   ...
>   # This Git call somehow fails and I have no clue why
>   git push remote HEAD
>   ...
>   '
>
> then prefixing the `git push` command with `TEST_GDB_GIT=1` lets you use
> `gdb` when running the test with the `-i` and `-v` flags.
>
> Please note that `TEST_GDB_GIT` is already a major step up from my initial
> `DDD`.

Yeah, that was my first reaction when I saw this patch.  Instead of
having to munge that line to "gdb -whatever-args git", you can do a
single-shot debugging in a convenient way.  And quite honestly,
because nobody sane will run:

 $ cd t && TEST_GDB_GIT=1 sh ./t1234-frotz.sh

and can drive all the "git" running under gdb at the same time, I
think what you showed would be the _only_ practical use case.  I
would have thought that TEST_GDB_GIT was way too long (and so is
GIT_TEST_GDB) and was about to suggest using something short and
sweet, even shorter than DDD, that you can easily add and remove.

It can be called GDB=1, perhaps?

I agree with all other points Jonathan made in his review, including
"Neat." part ;-)

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Duy Nguyen]

On Mon, Oct 26, 2015 at 2:15 PM, Johannes Schindelin
 wrote:
> When prefixing a Git call in the test suite with 'TEST_GDB_GIT=1 ', it
> will now be run with GDB, allowing the developer to debug test failures
> more conveniently.

I'm very slowly catching up with git traffic. Apologies if it's
already mentioned elsewhere since I have only read this mail thread.

Is it more convenient to add a sh function "gdb" instead? Most of the
time I only want to stop one command, and I put "gdb /path//" in
front of "git ...". This gdb function could just expand to that THis
would make it a lot more convenient to debug (single command, not full
.sh file).

We can even go further with supporting gdbserver function, to launch
gdbserver, then I can debug from outside, works even without -v -i.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Stefan Beller]

On Tue, Oct 27, 2015 at 4:28 PM, Jeff King  wrote:
> I agree doing so would be crazy. But would:
>
>   ./t1234-frotz.sh --gdb=17
>
> be sane to run gdb only inside test 17?

OT:
We have two ways of addressing tests, by number and by name.
Usually when a test fails ("Foo gobbles the bar correctly" failed),
I want to run tests 1,17 (1 is the correct setup and 17 is the failing test)
But coming up with that tuple is hard.
  * How do I know we need to run 1 as the setup ? (usually we do,
sometimes we don't and other times we also need 2,3 to completely
setup the tests)
  * How do I know it's test 17 which is failing? My workflow up to now
I just searched the test title in the file, such that I'd be there anyway
to inspect it further. But still I found it inconvenient to
mentally map between
17 and the test title.

Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Jeff King]

On Tue, Oct 27, 2015 at 04:39:37PM -0700, Stefan Beller wrote:

> On Tue, Oct 27, 2015 at 4:28 PM, Jeff King  wrote:
> > I agree doing so would be crazy. But would:
> >
> >   ./t1234-frotz.sh --gdb=17
> >
> > be sane to run gdb only inside test 17?
> 
> OT:
> We have two ways of addressing tests, by number and by name.

Yeah. The numbers are not stable if the script gets new test, but they
are usually fine for within a debugging session. Names are annoying to
type (and also not guaranteed unique).

> Usually when a test fails ("Foo gobbles the bar correctly" failed),
> I want to run tests 1,17 (1 is the correct setup and 17 is the failing test)
> But coming up with that tuple is hard.
>   * How do I know we need to run 1 as the setup ? (usually we do,
> sometimes we don't and other times we also need 2,3 to completely
> setup the tests)

I think trying to deduce that tuple is a fool's errand. It takes a lot
of manual work, and even if you _think_ you have it, sometimes state
left from earlier tests is accidentally important. But it's usually not
that expensive to run earlier tests at all; it's just expensive to run
them with extra debugging. That's why we have options like
"--valgrind-only=17". We still _run_ tests 1..16, but we do it quickly,
and then execute the expensive and slow valgrind git only on the
suspicious one.

And I'd propose --gdb to work the same way (run all the other tests, but
only kick in gdb for the suspicious one).

If you had multiple "git" invocations inside test 17, you could even do
something like "--gdb=17:4" to kick in only for the 4th git invocation
or something. But counting up git invocations is probably too irritating
to be worth doing manually.

>   * How do I know it's test 17 which is failing? My workflow up to now
> I just searched the test title in the file, such that I'd be there anyway
> to inspect it further. But still I found it inconvenient to
> mentally map between
> 17 and the test title.

I usually just run the test script and look at the output. Here's a
failure (which I obviously induced with an extra line):

  $ ./t4103-apply-binary.sh -v -i
  [...]
  ok 5 - check binary diff -- should fail.
  
  expecting success: 
  git checkout master &&
  echo whoops, we fail here && false &&
  test_must_fail git apply --check C.diff
  
  Already on 'master'
  whoops, we fail here
  not ok 6 - check binary diff (copy) -- should fail.
  #
  #   git checkout master &&
  #   echo whoops, we fail here && false &&
  #   test_must_fail git apply --check C.diff
  #

I'd pull the test number from the "not ok" above (it's actually even
easier to see if you drop the "-v", but I usually start my debugging
with "-v" anyway, since error messages often make the problem obvious).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Jeff King]

On Tue, Oct 27, 2015 at 09:34:48AM -0700, Junio C Hamano wrote:

> Yeah, that was my first reaction when I saw this patch.  Instead of
> having to munge that line to "gdb -whatever-args git", you can do a
> single-shot debugging in a convenient way.  And quite honestly,
> because nobody sane will run:
> 
>  $ cd t && TEST_GDB_GIT=1 sh ./t1234-frotz.sh
> 
> and can drive all the "git" running under gdb at the same time, I
> think what you showed would be the _only_ practical use case.

I agree doing so would be crazy. But would:

  ./t1234-frotz.sh --gdb=17

be sane to run gdb only inside test 17?

I suspect it would work about half the time. Many tests will call git
only once per snippet, but many make multiple git calls, and we are only
interested in debugging one.

I dunno. Maybe that is making things more complicated than they need to
be. I usually use the "tweak the test script" approach, but I have
always found it annoying to have to untweak it later.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Jonathan Nieder]

Johannes Schindelin wrote:

> When prefixing a Git call in the test suite with 'TEST_GDB_GIT=1 ', it
> will now be run with GDB, allowing the developer to debug test failures
> more conveniently.

Neat.

[...]
> --- a/wrap-for-bin.sh
> +++ b/wrap-for-bin.sh
> @@ -19,4 +19,11 @@ GIT_TEXTDOMAINDIR='@@BUILD_DIR@@/po/build/locale'
>  PATH='@@BUILD_DIR@@/bin-wrappers:'"$PATH"
>  export GIT_EXEC_PATH GITPERLLIB PATH GIT_TEXTDOMAINDIR
>  
> +if test -n "$TEST_GDB_GIT"
> +then
> + exec gdb -args "${GIT_EXEC_PATH}/@@PROG@@" "$@"

Most TEST_ environment variables that git respects are under
GIT_TEST_* --- e.g., GIT_TEST_OPTS.  Should this match that pattern
as well, for easier debugging with commands like 'env | grep GIT_'?

What happens if the child in turn calls git again?  Should this
unset TEST_GDB_GIT in gdb's environment?

The gdb manual and --help output advertise "--args".  Has "-args"
(with a single dash) always worked?

> + echo "Could not run gdb -args ${GIT_EXEC_PATH}/@@PROG@@ $*" >&2
> + exit 1

Does the 'exec' after the fi need this as well?  exec is supposed to
itself print a message and exit when it runs into an error.  Would
including an 'else' with the if make the control flow clearer?  E.g.

if test -n "$TEST_GDB_GIT"
then
exec gdb --args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
else
exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"
fi

Thanks,
Jonathan

diff --git i/wrap-for-bin.sh w/wrap-for-bin.sh
index a151c95..db0ec6a 100644
--- i/wrap-for-bin.sh
+++ w/wrap-for-bin.sh
@@ -19,11 +19,10 @@ GIT_TEXTDOMAINDIR='@@BUILD_DIR@@/po/build/locale'
 PATH='@@BUILD_DIR@@/bin-wrappers:'"$PATH"
 export GIT_EXEC_PATH GITPERLLIB PATH GIT_TEXTDOMAINDIR
 
-if test -n "$TEST_GDB_GIT"
+if test -n "$GIT_TEST_GDB"
 then
-   exec gdb -args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
-   echo "Could not run gdb -args ${GIT_EXEC_PATH}/@@PROG@@ $*" >&2
-   exit 1
+   unset GIT_TEST_GDB
+   exec gdb --args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
+else
+   exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"
 fi
-
-exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html