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