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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Apr 11, 2013, 13:22 UTC
Message-ID: <CAMP44s02K5ydKLNi0umMkuAicoVTWyCdVfjs0yssCa2oyFShGQ@mail.gmail.com>
In-Reply-To: <20130410211552.GA3256@sigill.intra.peff.net>

On Wed, Apr 10, 2013 at 4:15 PM, Jeff King <peff@peff.net> wrote:

Show 10 quoted lines

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

This explained the same thing:

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

Granted, depending on the way the remote-helper died, an error might or might not been printed, so s/won't/might not/.

The fact that an exit code was returned before is not relevant, neither is how the exit was returned, and for that matter neither is all the other things that are happening in this code. It's just noise.

The only thing that is relevant is this:

-               exit(128);
+               die("Reading from remote helper failed");

It's a simple change, and simple to explain.

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

> While we're adding tests, let's also confirm that the
> remote-helper dying is also detect when importing refs.

That is enough explanation.

Show 6 quoted lines

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

This doesn't help anyone, and it's not even accurate. I think it might be possible enforce remote-helpers to implement the "done" feature, and we might want to do that later. But of course, discussing what bad things remote-helpers could do, and how we should test and babysit them is not relevant here.

If it was important to explain the subtleties and reasoning behind this change, it should be a separate patch.

> Suggested-by: Jeff King <peff@peff.net>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>

I would add:

[jk: rewrote every piece of text]

>         export)
> +               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"?

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

LGTM.

Cheers.

-- Felipe Contreras