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
immediately.
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
done
The version I posted yesterday had both the read and the sleep, but the
sleep was only necessary there to demonstrate the race with
check_command.
> +# 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?
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html