On Sat, Nov 30, 2019 at 09:08:32AM -0800, Junio C Hamano wrote:

quoted

quoted

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.)