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

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

[Ævar Arnfjörð Bjarmason]

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

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

[Andreas Schwab]

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

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

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 455 bytes --]

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

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

[Ævar Arnfjörð Bjarmason]

On Mon, Jul 12, 2010 at 03:32, Nicolas Pitre <nico@fluxnic.net> 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.

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

[Jonathan Nieder]

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.

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

[Ævar Arnfjörð Bjarmason]

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

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

[Jonathan Nieder]

Æ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