Re: [PATCH v4] transport-helper: report errors properly

From: Jeff King <peff@peff.net>
Date: Apr 9, 2013, 21:50 UTC
Message-ID: <20130409215018.GA28271@sigill.intra.peff.net>
In-Reply-To: <87ip3v1j2a.fsf@hexa.v.cablecom.net>

On Tue, Apr 09, 2013 at 11:38:05PM +0200, Thomas Rast wrote:

Show 20 quoted lines

> Two out of six of these loops quit within 1 and 2 iterations,
> respectively, both with an error along the lines of:
> 
>   expecting success: 
>           (GIT_REMOTE_TESTGIT_FAILURE=1 &&
>           export GIT_REMOTE_TESTGIT_FAILURE &&
>           cd local &&
>           test_must_fail git push --all 2> error &&
>           cat error &&
>           grep -q "Reading from remote helper failed" error
>           )
> 
>   error: fast-export died of signal 13
>   fatal: Error while running fast-export
>   not ok 21 - proper failure checks for pushing
> 
> I haven't been able to reproduce outside of valgrind tests.  Is this an
> expected issue, caused by overrunning the sleep somehow?  If so, can you
> increase the sleep delay under valgrind so as to not cause intermittent
> failures in the test suite?

Yeah, I am not too surprised. The failing helper sleeps before exiting so that fast-export puts all of its data into the pipe buffer before the helper dies, and does not get SIGPIPE. But obviously the sleep is just delaying the problem if your fast-export runs really slowly (which, if you are running under valgrind, is a possibility).

The helper should instead just consume all of fast-export's input before exiting, which accomplishes the same thing, finishes sooner in the normal case, and doesn't race. And I think it also simulates a reasonable real-world setup (a helper reads and converts the data, but then dies while writing the output to disk, the network, or whatever).

I posted review comments, including that, and I'm assuming that Felipe is going to re-roll at some point.

-Peff