Re: [PATCH 3/5] odb/source-files: make the second read an implementation detail

[Justin Tobler <jltobler@gmail.com>]

On 26/10/14 08:55AM, Patrick Steinhardt wrote:
> On Tue, Oct 13, 2026 at 04:28:37PM -0500, Justin Tobler wrote:
> > On 26/10/13 09:36AM, Patrick Steinhardt wrote:
> > > Note that the "packed" source also used this flag to figure out whether
> > > it should try to surface an object via a packfile that should have the
> > > object as determined by a multi-pack index, but where the packfile has
> > > vanished. The multi-pack index is stale in that case, and that scenario
> > > should really only happen when the process is racing with a concurrent
> > > maintenance task.
> > > 
> > > This code is adapted to instead use `OBJECT_INFO_QUICK`, which means
> > > that the fallback may trigger on the first read already, but not in a
> > > case where callers know to handle false negatives already. In the worst
> > > case this could cause another scan through all packfiles covered by the
> > > multi-pack index. But that scan should be fast enough anyway, and it
> > > only triggers when we hit the race. So the effect thereof should be
> > > limited.
> > 
> > Would it be more correct to say that we are now using the absence of the
> > OBJECT_INFO_QUICK flag trigger the fallback?
> 
> I dunno. Whether we check for absence or presence, in either case we use
> the flag. So I'm inclined to leave this as-is as it starts to read a bit
> awkward otherwise.
> 
> Let me know in case you feel strongly though.

I don't feel too strongly either way. When I read it the first time, my
first thought was that we were using the OBJECT_INFO_QUICK flag to mean
the same thing as OBJECT_INFO_SECOND_READ which confused me. Then I
realized it was in fact the exact opposite.

> > > diff --git a/odb/source-files.c b/odb/source-files.c
> > > index 54235821d5..9788a5adf4 100644
> > > --- a/odb/source-files.c
> > > +++ b/odb/source-files.c
> > > @@ -444,6 +449,9 @@ static enum odb_read_status odb_source_files_read_object_info(struct odb_source
> > >  	}
> > >  
> > >  	for (struct odb_files_dir *dir = files->dirs; dir; dir = dir->next) {
> > > +		if (retry)
> > > +			break;
> > 
> > IIUC, we don't need to bother checking the loose objects a second time
> > so we exit the look early.
> > 
> > Nit: Mostly just a style preference, but instead of entering the loop
> > just to break out early for retries, could we just return early prior
> > the loop starting? It looks like the rest of the function is skipped on
> > the second read anyways.
> 
> Yes. But I've got a follow-up patch series that has some fixes for how
> we reload alternates, and as part of that I'll start to iterate over
> loose object directories that have been added after re-reading
> ".git/objects/info/alternates". So this is a bit awkward right now, but
> makes the follow-up series a bit easier.

Ok, this sounds reasonable to me.

-Justin