# [PATCH/RFC] test-lib: clean up trash* directories on SIGINT 7 messages from 2010-07-11 to 2010-07-12. Participants: Ævar Arnfjörð Bjarmason, Andreas Schwab, Nicolas Pitre, Jonathan Nieder. Thread: https://gitlist.dev/t/24367 ## Ævar Arnfjörð Bjarmason, 2010-07-11 09:51 Subject: [PATCH/RFC] test-lib: clean up trash* directories on SIGINT Message-ID: URL: https://gitlist.dev/e/AANLkTimnrLlprdScYDKHs_lIvFKc3K0n8U5vTzG_df2k%40mail.gmail.com ``` Is there any reason not to remove trash directories on SIGINT? It's annoying that trash directories are left when I cancel a test run. I can submit an actual patch along these lines once Junio applies some of my other stuff which will probably conflict with any finalized patch. Is "trap 'foo' INT" a bashism? Is "trap 'foo' SIGINT" more portable? How about the exit code from an INT handler, what should it be? diff --git a/t/test-lib.sh b/t/test-lib.sh index ac496aa..e4a0fc9 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -222,6 +222,17 @@ die () { GIT_EXIT_OK= trap 'die' EXIT +git_interrupted () { + test -d "$remove_trash" && + cd "$(dirname "$remove_trash")" && + rm -rf "$(basename "$remove_trash")" + + # What my shell normal exits on on sigint + exit 130 +} + +trap 'git_interrupted' INT + # The semantics of the editor variables are that of invoking # sh -c "$EDITOR \"$@\"" files ... # ``` ## Andreas Schwab, 2010-07-11 11:55 Subject: Re: [PATCH/RFC] test-lib: clean up trash* directories on SIGINT Message-ID: URL: https://gitlist.dev/e/m21vba9qrq.fsf%40igel.home In-Reply-To: ``` Ævar Arnfjörð Bjarmason writes: > Is "trap 'foo' INT" a bashism? It's the norm. > Is "trap 'foo' SIGINT" more portable? "Implementations may permit names with the SIG prefix or ignore case in signal names as an extension." Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ``` ## Nicolas Pitre, 2010-07-12 03:32 Subject: Re: [PATCH/RFC] test-lib: clean up trash* directories on SIGINT Message-ID: URL: https://gitlist.dev/e/alpine.LFD.2.00.1007112327150.10598%40xanadu.home In-Reply-To: ``` On Sun, 11 Jul 2010, Ævar Arnfjörð Bjarmason wrote: > Is there any reason not to remove trash directories on SIGINT? It's > annoying that trash directories are left when I cancel a test run. What about tests that would fail due to some infinite loop? The test would never complete until you manually interrupt it, and in that case the trash directory content might be valuable. You may do 'make clean' in the test directory to clean it. Nicolas ``` ## Ævar Arnfjörð Bjarmason, 2010-07-12 05:42 Subject: Re: [PATCH/RFC] test-lib: clean up trash* directories on SIGINT Message-ID: URL: https://gitlist.dev/e/AANLkTindliEmS2XhH8B1dUyo-v-CzpLQBqZakOaNnARW%40mail.gmail.com In-Reply-To: ``` On Mon, Jul 12, 2010 at 03:32, Nicolas Pitre wrote: > On Sun, 11 Jul 2010, Ævar Arnfjörð Bjarmason wrote: > >> Is there any reason not to remove trash directories on SIGINT? It's >> annoying that trash directories are left when I cancel a test run. > > What about tests that would fail due to some infinite loop?  The test > would never complete until you manually interrupt it, and in that case > the trash directory content might be valuable. Unless that's due to some unreproducable heisenbug you could get the trash directory by running the test with --debug. > You may do 'make clean' in the test directory to clean it. I know, but I think not cleaning them up on SIGINT is a misfeature. These trash directories are explicitly temporary, and should be cleaned up as the shell exists. ``` ## Jonathan Nieder, 2010-07-12 06:16 Subject: Re: [PATCH/RFC] test-lib: clean up trash* directories on SIGINT Message-ID: <20100712061608.GA6140@burratino> URL: https://gitlist.dev/e/20100712061608.GA6140%40burratino In-Reply-To: ``` Hi Ævar, Ævar Arnfjörð Bjarmason wrote: > Unless that's due to some unreproducable heisenbug you could get the > trash directory by running the test with --debug. So what should I do after noticing a heisenbug? I guess I can suspend with ^Z, but I would prefer not to have to train my fingers away from ^C. > These trash directories are explicitly temporary, and > should be cleaned up as the shell exists. When tests learned to remove the trash directory in test_done in v1.6.1-rc1~336^2~1 (Enable parallel tests, 2008-08-08), that was not the rationale; it was rather to avoid too many trash directories piling up. If a test exits by mistake with ‘exit’ or fails with ‘-i’, the one or two scratch directories involved are still kept for debugging. So you can see why a person would be reluctant to change this for aesthetic reasons only. ``` ## Ævar Arnfjörð Bjarmason, 2010-07-12 08:16 Subject: Re: [PATCH/RFC] test-lib: clean up trash* directories on SIGINT Message-ID: URL: https://gitlist.dev/e/AANLkTinGsIqy-rG8um1E7_zfla1KwsO2Z26JnchXaRLd%40mail.gmail.com In-Reply-To: <20100712061608.GA6140@burratino> ``` On Mon, Jul 12, 2010 at 06:16, Jonathan Nieder wrote: > Hi Ævar, > > Ævar Arnfjörð Bjarmason wrote: > >> Unless that's due to some unreproducable heisenbug you could get the >> trash directory by running the test with --debug. > > So what should I do after noticing a heisenbug? Run the test again with --debug. The test library and the commands it's calling don't live in the same address space. A "heisenbug" will probably be just as reproducable with --debug and without --debug. >> These trash directories are explicitly temporary, and >> should be cleaned up as the shell exists. > > When tests learned to remove the trash directory in test_done in > v1.6.1-rc1~336^2~1 (Enable parallel tests, 2008-08-08), that was not > the rationale; it was rather to avoid too many trash directories > piling up.  If a test exits by mistake with ‘exit’ or fails with ‘-i’, > the one or two scratch directories involved are still kept for > debugging. That's the original rationale, which is fair enough. I think the behavior should be to clean up temporary files when the test exits, not just when it's done executing. > So you can see why a person would be reluctant to change this for > aesthetic reasons only. The reason I want to change it is that as I'm hacking Git I often run the tests but kill them before they've completed, because if I'm e.g. testing the test library itself I'm content with having only 10 tests pass before I continue improving it. If I keep doing that (along with --jobs 20 and --shuffle) I'll eventually pile up ~200 MB of trash. That'll exceed the limits of my ramdisk and I'll have to grudgingly do rm -rf trash* again. But maybe people such as yourself depend on the current behavior with regards to SIGINT. I don't feel strongly about it, and having to remove trash is a minor annoyance. If it's being used it should be kept. I just assumed that it was an omission in the cleanup routines. ``` ## Jonathan Nieder, 2010-07-12 08:56 Subject: Re: [PATCH/RFC] test-lib: clean up trash* directories on SIGINT Message-ID: <20100712085652.GA6837@burratino> URL: https://gitlist.dev/e/20100712085652.GA6837%40burratino In-Reply-To: ``` Ævar Arnfjörð Bjarmason wrote: > On Mon, Jul 12, 2010 at 06:16, Jonathan Nieder wrote: >> So what should I do after noticing a heisenbug? > > Run the test again with --debug. Sorry, I wasn’t clear. The sequence I was imagining (and have enountered) is: $ make test [...] not ok - 35 something that really should have been okay ... more text streams by ... -- wait, what? I’ve never seen that fail before. ^C If I catch it early enough, I can inspect the trash directory and figure out what happened. Some bugs are hard to reproduce (and it avoids waiting for tests to run again). But I can use ^Z instead. > If I keep doing that (along with --jobs 20 and --shuffle) I'll > eventually pile up ~200 MB of trash. That'll exceed the limits of my > ramdisk and I'll have to grudgingly do rm -rf trash* again. Given this rationale, I don’t personally mind the change (though it is still not clear why not to run “make clean” from t/ between runs instead). I would be more worried if you were proposing making the --immediate option remove the trash directory, too. I often use that option with the express intent of populating a test repository which is no more temporary than the .o files and copies of binaries in the build tree are. Thanks for the explanation, Jonathan ```