On Mon, Apr 08, 2013 at 09:40:04AM -0500, Felipe Contreras wrote:

> 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

Thanks, I think this patch is definitely the right direction.

It seems like there is a lot of back-story that had to be clarified
during the review/discussion. Is there a reason not to summarize it here
so later readers of this commit are enlightened?

I'm thinking something like:

  If a push fails because the remote-helper died (with fast-export), the
  user does not see any error message. We do correctly die with a failed
  exit code, as we notice that the helper has died while reading back
  the ref status from the helper. However, we don't print any message.
  This is OK if the helper itself printed a useful error message, but we
  cannot count on that; let's let the user know that the helper failed.

  In the long run, it may make more sense to propagate the error back up
  to push, so that it can present the usual status table and give a
  nicer message. But this is a much simpler fix that can help

  While we're adding tests, let's also confirm that the remote-helper
  dying is also detect when importing refs. We currently do so robustly
  when the helper uses the "done" feature (and that is what we test). We
  cannot do so reliably when the helper does not use the "done" feature,
  but it is not even worth testing; the right solution is for the helper
  to start using "done".

>       export)
> +             if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
> +             then
> +                     sleep 1 # don't let fast-export get SIGPIPE
> +                     exit 1
> +             fi

We can do away with this sleep with:

  while read line; do
          test "$line" = "done" && break

The version I posted yesterday had both the read and the sleep, but the
sleep was only necessary there to demonstrate the race with

> +# We sleep to give fast-export a chance to catch the SIGPIPE
> +test_expect_success 'proper failure checks for pushing' '

I think we can drop this comment now, right?

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to