Re: [PATCH] send-pack: use OBJECT_INFO_QUICK to check negative objects

From: Jonathan Tan <jonathantanmy@google.com>
Date: Dec 4, 2019, 20:53 UTC
Message-ID: <20191204205353.118168-1-jonathantanmy@google.com>
In-Reply-To: <20191203232007.GA30535@sigill.intra.peff.net>

Show 26 quoted lines

> On Sat, Nov 30, 2019 at 09:08:32AM -0800, Junio C Hamano wrote:
> 
> > > Interestingly, upload-pack does not use OBJECT_INFO_QUICK when it's
> > > getting oids from the other side. But I think it could possibly benefit
> > > in the same way. Nobody seems to have noticed. Perhaps it simply comes
> > > up less, as servers would tend to have more objects than their clients?
> > 
> > Makes me wonder how many times we wre bitten by the fact that
> > INFO_SKIP_FETCH_OBJECT does not imply INFO_QUICK.  Perhaps most of
> > the users of INFO_SKIP_FETCH_OBJECT wants to use INFO_FOR_PREFETCH?
> 
> We seem to be discussing this about once a month lately. :)
> 
> There's some good digging by Jonathan in:
> 
>   https://public-inbox.org/git/20191011220822.154063-1-jonathantanmy@google.com/
> 
> That thread is also about this exact same spot, which is why I cc'd him
> originally.
> 
> I do tend to think that QUICK ought to imply SKIP_FETCH. I'm slightly
> negative on sprinkling FOR_PREFETCH everywhere, because the name implies
> to me that we are telling object_info() that we are pre-fetching. Which
> yes, has the effect we want, but I think is misleading. So we'd want to
> change the name of the combined flag, I think, or just let QUICK imply
> SKIP_FETCH and use that more thoroughly.

If we want to make QUICK imply SKIP_FETCH but not the other way around, then note that we'll have to make sure that we do not repeat the error that 31f5256c82 ("sha1-file: split OBJECT_INFO_FOR_PREFETCH", 2019-05-28) fixes.

As for whether we should do it in the first place, I think that having 2 orthogonal flags is clearer than having 1 that controls fetch (SKIP_FETCH) and 1 that does both (QUICK) without any that recheck packed, but I'm quite familiar with this part of the code, so perhaps I'm the wrong person to ask - we should optimize for the typical Git developer who just wants to access the object store. Judging from the frequency in which we encounter this issue (as Peff says), maybe we should go ahead and make QUICK imply SKIP_FETCH but not the other way around. (It is also possible just to make both imply the other and unify QUICK and SKIP_FETCH into one flag - I am not opposed to that - although read my email that Peff linked to see why bidirectional implication is correct in one sense but incorrect in another.)