Re: [PATCH 2/2] odb/source-loose: fix existence checks for newly written objects

[Patrick Steinhardt <ps@pks.im>]

On Fri, Nov 06, 2026 at 08:02:13AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > On Thu, Nov 05, 2026 at 02:43:14PM -0500, Jeff King wrote:
> >> On Wed, Nov 04, 2026 at 01:16:35PM +0100, Patrick Steinhardt wrote:
> >> ...
> >> So I kind of wonder if prefetch_cache_entries(), along with any caller
> >> doing an OBJECT_INFO_FOR_PREFETCH check, ought to omit the QUICK flag.
> >> It would mean that the normal case of having missing partial objects
> >> (about to be fetched) would cause us to re-scan the local pack list, but
> >> that's not _that_ expensive, especially compared to the fetch we're
> >> about to do.
> >
> > Yeah, that's a valid point. Honestly, by now I'm almost urged to just
> > drop the QUICK flag altogther. It's a source of bugs, and the longer we
> > look at it the more cases we find where the existing users thereof are
> > buggy in specific edge cases. We just recently removed one user of
> > QUICK, and this here is another dubious case. I wouldn't be surprised if
> > we could find edge cases for the other, remaining users, as well.
> > ...
> >> > We could in theory fix this by flushing all the loose object caches
> >> > again, like we used to do before 5946a564cdd. But that wouldn't address
> >> > the root cause: the cache would still be stale unless somebody does a
> >> > non-QUICK read for a missing object. Instead, fix the bug by updating
> >> > the cache whenever we write a new loose object.
> 
> OK.  At least this change hides this specific failure mode.  Can we
> include some documentation that we may want to rethink the "QUICK"
> thing and audit its current uses (if we decide to keep it, that is)
> and move on with this change?
> 
> 
>  odb.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git c/odb.h w/odb.h
> index dd85b880a7..61ff443d46 100644
> --- c/odb.h
> +++ w/odb.h
> @@ -386,6 +386,9 @@ enum object_info_flags {
>  	/*
>  	 * This is meant for bulk prefetching of missing blobs in a partial
>  	 * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK.
> +	 *
> +	 * NEEDSWORK: rethink if QUICK is desired in this context.
> +	 * cf. <au23Hvzp09yrym9s@pks.im>
>  	 */
>  	OBJECT_INFO_FOR_PREFETCH = (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK),
>  };

Yeah, that'd probably be sensible. But I wonder whether that comment
should rather be applied to `OBJECT_INFO_QUICK` itself, as I think it's
way too much of a foot gun and I'm not sure whether it's ever really
sensible to use. But I'm fine with either variant.

Will you squash that in or shall I send a new version?

Thanks!

Patrick