Re: [RFC PATCH] Modify fetch-pack to no longer die on error?

From: Jeff King <peff@peff.net>
Date: Jul 25, 2020, 21:41 UTC
Message-ID: <20200725214118.GA4508@coredump.intra.peff.net>
In-Reply-To: <xmqq7dusjyos.fsf@gitster.c.googlers.com>

On Fri, Jul 24, 2020 at 04:11:31PM -0700, Junio C Hamano wrote:

Show 13 quoted lines

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> > I think the best way for easy authorship and review is to convert each
> > possibly-dying function into a function that either returns a
> > possibly-null error string or returns success/failure and writes the
> > error string into an "out" parameter.
> 
> Ugly.
> 
> Rather, if we were to pass an extra parameter through the entire
> callchain, I'd rather see that parameter to be what to do when we
> see an error.  Depending on that, the callee can die(), return
> error(), or silently return -1, or do something else.

Agreed. I had a proposal a while ago to pass a "struct error_context" which was basically a function pointer and any accompanying data. That lets the caller instruct us to die(), call error(), stuff the error in a strbuf owned by the caller, and so on.

I think it was this one:

  https://lore.kernel.org/git/20160927191955.mympqgylrxhkp24n@sigill.intra.peff.net/

I still think it's a good idea, but the hard part is lib-ifying all of the functions that call die() to instead return an error up the stack (and handle cleanup, etc). Which is why I never really pushed it further. But if we're going to be lib-ifying such calls anyway, I think it's nice to do this flexible thing (from their perspective it's no more work to trigger the callback than it is to call error() and return).

That said, I do think for this particular case, your "just run it in a sub-process" suggestion may be simpler and more robust.

-Peff