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

Felipe Contreras [off-list ref] writes:

If a push fails because the remote-helper died (with fast-export), the
user won't see any error message. So let's add one.

At the same time lets add tests to ensure this error is reported, and
while we are at it, check the error from fast-import

[...]

+# We sleep to give fast-export a chance to catch the SIGPIPE
+test_expect_success 'proper failure checks for pushing' '
+	(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
+	)
+'

There appears to be a race in the version that is in today's pu
(5eb25f737b).  I reproduced with this:

  cd git/t
  i=1
  while ./t5801-remote-helpers.sh --root=/dev/shm --valgrind
  do
    i=$(($i+1))
  done

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?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

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