From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 02 Sep 2025 08:50:25 GMT
Subject: Re: [PATCH v2 09/16] packfile: split up responsibilities of `reprepare_packed_git()`
Message-ID: <aLav0TRRg6mH4Umw@pks.im>
In-Reply-To: <aK0XeiRz13O7j+HO@nand.local>

On Mon, Aug 25, 2025 at 10:10:02PM -0400, Taylor Blau wrote:
> On Thu, Aug 21, 2025 at 09:39:07AM +0200, Patrick Steinhardt wrote:
> > diff --git a/odb.c b/odb.c
> > index 80ec6fc1fa..37ed21f53b 100644
> > --- a/odb.c
> > +++ b/odb.c
> > @@ -694,7 +694,7 @@ static int do_oid_object_info_extended(struct object_database *odb,
> >
> >  		/* Not a loose object; someone else may have just packed it. */
> >  		if (!(flags & OBJECT_INFO_QUICK)) {
> > -			reprepare_packed_git(odb->repo);
> > +			odb_reprepare(odb->repo->objects);
> >  			if (find_pack_entry(odb->repo, real, &e))
> >  				break;
> >  		}
> > @@ -1039,3 +1039,26 @@ void odb_clear(struct object_database *o)
> >
> >  	string_list_clear(&o->submodule_source_paths, 0);
> >  }
> > +
> > +void odb_reprepare(struct object_database *o)
> 
> OK; so here is the new location for the non-packfile related portions of
> the former reprepare_packed_git() function. That makes sense, but...
> 
> > +{
> > +	struct odb_source *source;
> > +
> > +	/*
> > +	 * Reprepare alt odbs, in case the alternates file was modified
> > +	 * during the course of this process. This only _adds_ odbs to
> > +	 * the linked list, so existing odbs will continue to exist for
> > +	 * the lifetime of the process.
> > +	 */
> > +	o->loaded_alternates = 0;
> > +	odb_prepare_alternates(o);
> > +
> > +	for (source = o->sources; source; source = source->next)
> > +		odb_clear_loose_cache(source);
> > +
> > +	o->approximate_object_count_valid = 0;
> > +
> > +	packfile_store_reprepare(o->packfiles);
> > +
> > +	obj_read_unlock();
> 
> ...I think I am missing where we call odb_read_lock(). The function
> packfile_store_reprepare() has a comment that it must be called under
> the odb_read_lock(), but I don't see where we acquire that lock here.
> 
> Are the callers of odb_reprepare() supposed to acquire that lock? If so,
> it seems a little awkward that the caller is supposed to acquire the
> lock, but the callee is the one to release it. Is this function missing
> a odb_read_lock() at the top?
> 
> I looked at a few callers here and none of them seem to be holding this
> lock. pthread_mutex_unlock() is supposed to check that the mutex lock is
> held for recursive and error-checking mutexes. IIRC we initialize the
> the odb mutex with PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP, so I am a
> little surprised that this did not cause a runtime error.

Oh, that's a very good catch. And yes, surprising indeed that this does
not cause an error anywhere. But if you look at the code it ultimately
isn't all that surprising: there is only a single command in our code
base that performs concurrent reads via the ODB, namely git-grep(1).
That command explicitly opts into `obj_read_use_lock` by calling
`enable_obj_read_lock()`. All the other commands never perform any
locking whatsoever.

In any case, I think I merely missed to add the locking call into
`odb_reprepare()`. It shouldn't be the responsibility of the caller to
care about locking, it should be handled automatically.

Will fix.

Patrick