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

[Jeff King <peff@peff.net>]

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

> 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