RE: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb
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
> > +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
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
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
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
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
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
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 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
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
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
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
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
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
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
+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
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
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
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
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
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
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
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