Re: [PATCH] Portability: returning void

On Tue, Mar 29, 2011 at 10:57:33PM -0500, Jonathan Nieder wrote:

Jeff King wrote:

quoted

While we're at it, let's make the "kill" process a little
more robust. Specifically:

  1. Wrap the "kill" in a test_when_finished, since we want
     to clean up the process whether the test succeeds or not.

  2. The "kill" is part of our && chain for test success. It
     probably won't fail, but it can if the process has
     expired before we manage to kill it. So let's mark it
     as OK to fail.

Signed-off-by: Jeff King <redacted>
---
Actually, my (2) above is unlikely to trigger, since the test would have
to hang for 100 seconds first, which probably means it is failing
anyway. But I did run across it when I screwed up my fix.

That is actually how the tests distinguish between success and
failure.  Any idea about a better way?

Ah. I was trying not to look too hard at how the tests actually work, so
I completely missed that. Yes, if the "kill" is part of the actual test,
then it should stay in the test. We can also put in a test_when_finished
to kill it should the test fail to make it that far. If the cleanup one
does an extra kill, it shouldn't be a big deal.

I was in the process of writing a commit message for the same "exec"
trick, but I'm glad you got to it first. ;-)

I don't know why I didn't think of it sooner. :)

quoted

Also, is it just me, or does it seem weird that test_when_finished
blocks failing can produce test failure? I would think you would be able
to put any old cleanup crap into them and not care whether it worked or
not.

I'm generally happy that it catches mistakes in cleanup code, but I
could easily be convinced to change it anyway.

I don't think it's a big deal. It just surprised me.

Maybe it would be good to factor out a helper to make future
improvements a little easier, like so:

-- 8< --
Subject: t0081: introduce helper to fill a pipe in the background

The fill_input function generates a fifo and runs a command to write
to it and wait a while.  The intended use is to test programs that
need to be able to cope with input of limited length without relying
on EOF or SIGPIPE to detect its end.

Yeah, that looks much nicer. Do you want to just re-roll that on top of
what's in 'master', with the "exec" magic and the defensive
test_when_finished in it (or as a separate patch on top if you want to
split the refactor and fix)? Feel free to borrow from my commit message.

-Peff