Re: [PATCH 1/2] transport-helper: report errors properly

Felipe Contreras [off-list ref] writes:

On Wed, Apr 10, 2013 at 4:15 PM, Jeff King [off-list ref] wrote:

quoted

From: Felipe Contreras <redacted>

If a push fails because the remote-helper died (with
fast-export), the user does not see any error message. We do

I agree with you that s/does not see/may not see/ would be more
helpful here, so I'll squash it in while queuing.

quoted

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.

Yes it might, and it might make sense to rewrite much of this code,
but that's not relevant.

It is a good reminder for people who later inspect this part of the
code and wonder if it was a conscious design choice not to propagate
the error or just being "simple and sufficient for now", I think.
It would help them by making it clear that it is the latter, no?

... I think it might
be possible enforce remote-helpers to implement the "done" feature,
and we might want to do that later.

Yes, all these are possible and I think writing it down explicitly
will serve as a reminder for our future selves, I think.

quoted

+               if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
+               then
+                       # consume input so fast-export doesn't get SIGPIPE;

I think this is explanation enough.

quoted

+                       # git would also notice that case, but we want
+                       # to make sure we are exercising the later
+                       # error checks

I don't understand what is being said here. What is "that case"?

In my first reading, it felt to me that it was natural to interpret
that this is "even if we didn't have this loop that avoids killing
fast-export with SIGPIPE, we would notice death of fast-export by
SIGPIPE".