threads / rfc / 24367

RFC patchtest-lib: clean up trash* directories on SIGINT

Subject: [PATCH/RFC] test-lib: clean up trash* directories on SIGINT

## tl;dr

7 messages between Jul 11, 2010 and Jul 12, 2010. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Ævar Arnfjörð Bjarmason· Jul 11, 2010, 09:51 UTC · lore

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?

Show changes to t/test-lib.sh +11 −0

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· Jul 11, 2010, 11:55 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH/RFC] test-lib: clean up trash* directories on SIGINT

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Is "trap 'foo' INT" a bashism?

It's the norm.

> Is "trap 'foo' SIGINT" more portable?

<http://www.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_28_03> "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· Jul 12, 2010, 03:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH/RFC] test-lib: clean up trash* directories on SIGINT

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· Jul 12, 2010, 05:42 UTC · re: Nicolas Pitre · lore

Re: [PATCH/RFC] test-lib: clean up trash* directories on SIGINT

On Mon, Jul 12, 2010 at 03:32, Nicolas Pitre <nico@fluxnic.net> wrote:

Show 8 quoted lines

> 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· Jul 12, 2010, 06:16 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH/RFC] test-lib: clean up trash* directories on SIGINT

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· Jul 12, 2010, 08:16 UTC · re: Jonathan Nieder · lore

Re: [PATCH/RFC] test-lib: clean up trash* directories on SIGINT

On Mon, Jul 12, 2010 at 06:16, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 8 quoted lines

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

Show 9 quoted lines

>> 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· Jul 12, 2010, 08:56 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH/RFC] test-lib: clean up trash* directories on SIGINT

Ævar Arnfjörð Bjarmason wrote:

> On Mon, Jul 12, 2010 at 06:16, Jonathan Nieder <jrnieder@gmail.com> 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