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

From: Jeff King <peff@peff.net>
Date: Jul 27, 2020, 17:11 UTC
Message-ID: <20200727171143.GA789310@coredump.intra.peff.net>
In-Reply-To: <xmqqft9fi4hm.fsf@gitster.c.googlers.com>

On Sat, Jul 25, 2020 at 04:01:25PM -0700, Junio C Hamano wrote:

Show 22 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > 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).
> 
> Yeah, I almost 80%+ agree.
> 
> The remainder of 20% is that I am not so convinced that (fmt_string
> + va_list) that isn't pretty much usable for anything but generating
> human readable error messages is enough.  It is certainly a good
> enough replacement for (and much better than) the approach to
> prepare an error string in a "struct strbuf err" that was passeed by
> the caller, but I am not sure if that is a good bar to aim to pass
> to begin with ;-).

Yeah, I certainly agree that just passing around strings does not provide as much information as some callers would like. But I think introducing a system of machine-readable error tokens would be cumbersome for a lot of cases, to the point that it may tip this from "seems like an easy enough change" to something that nobody wants to take on.

One in-between step that would be pretty easy is to record errno when available (i.e., if error_errno() becomes error_ctx_errno(), then we just stuff that errno value into the error_context struct). That would let us reliably propagate errno up the stack, or even give intermediate callers the opportunity to munge it (e.g., if a ref lookup failed with EISDIR while opening a file, it might convert that to ENOENT as it propagates up the stack).

-Peff