From: Jeff King <peff@peff.net>
Date: Sat, 25 Jul 2020 21:41:18 GMT
Subject: Re: [RFC PATCH] Modify fetch-pack to no longer die on error?
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:

> 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