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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 11, 2013, 18:44 UTC
Message-ID: <7vfvywj4au.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CAMP44s02K5ydKLNi0umMkuAicoVTWyCdVfjs0yssCa2oyFShGQ@mail.gmail.com>

Felipe Contreras <felipe.contreras@gmail.com> writes:

Show 5 quoted lines

> On Wed, Apr 10, 2013 at 4:15 PM, Jeff King <peff@peff.net> wrote:
>> From: Felipe Contreras <felipe.contreras@gmail.com>
>>
>> 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.

Show 7 quoted lines

>> 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.

Show 11 quoted lines

>> +               if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
>> +               then
>> +                       # consume input so fast-export doesn't get SIGPIPE;
>
> I think this is explanation enough.
>
>> +                       # 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".