Re: [PATCH] diff: skip GITLINK when lazy fetching missing objs

From: Jeff King <peff@peff.net>
Date: Aug 22, 2019, 17:10 UTC
Message-ID: <20190822171018.GB26163@sigill.intra.peff.net>
In-Reply-To: <20190822162534.17365-1-jonathantanmy@google.com>

On Thu, Aug 22, 2019 at 09:25:34AM -0700, Jonathan Tan wrote:

Show 10 quoted lines

> > The batch fetch is purely an optimization, because we'd eventually fetch
> > the individual objects on demand. If the batch one fails, should we
> > continue with the operation? That leaves any error-handling for the
> > overall operation to the "real" code. And it would also mean that this
> > bug became an annoying error message,
> 
> On the one hand, if the batch fetch fails, then the individual
> prefetching would likely fail as well. But on the other hand, as you
> said below, we sometimes extraneously fetch objects, so making the batch
> fetch non-fatal might be a good idea too.

Yeah, I'd expect it to fail again on the individual fetch[1]. But then we are leaving that code to handle the error as it sees fit, which might not be to die(). Though TBH, the diff code is pretty eager to die() on missing objects anyway.

Of course the die() we see in this case is not really in your new code at all, but somewhere deep in the fetch stack. So there's probably a fair bit of work in making a failed fetch a recoverable error in the first place.

[1] You'd incur two attempts to fetch, which are expensive, but if
    we care about that it would be easy enough to keep an in-memory list
    of "I tried to fetch this once already and could not", and just
    quickly return failure on the second attempt.

-Peff