Re: [PATCH] object-file: reprepare alternates when necessary

[Derrick Stolee <derrickstolee@github.com>]

On 3/7/2023 12:29 PM, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
>> But in fact we've been doing the locking since 6c307626f1e (grep:
>> protect packed_git [re-]initialization, 2020-01-15). So the only thing
>> that really needs justification here is that putting the alternates
>> re-reading under the same lock
>>
>> There is a really interesting potential caveat here which you're not
>> discussing, which is...
>>> ...
>>> +void reprepare_alt_odb(struct repository *r)
>>> +{
>>> +	r->objects->loaded_alternates = 0;
>>> +	prepare_alt_odb(r);
>>> +}
>>> +
>>> ...
>> This seems reasonable, but wouldn't this do the same without introducing
>> an API function just for this one use-case?
>>
>> That's of course a nit, and you seem to have been adding this for
>> consistency with reprepare_packed_git(), but it already "owns" the
>> "approximate_object_count_valid" and "packed_git_initialized" flags in
>> "struct raw_object_store".
>>
>> So as we'll only need this from reprepare_packed_git() isn't it better
>> to declare that "loaded_alternates" is another such flag?
> 
> I am not sure I got what you want to say 100%, but if you are saying
> that this "drop the 'loaded' flag and force prepare_*() function to
> redo its thing" must not be done only in reprepare_packed_git(), and
> that inlining the code there without introducing a helper function
> that anybody can casually call without thinking its consequenced
> through, then I tend to agree in principle.  But it is just as easy
> to lift two lines of code from the rewritten/inlined code to a new
> place---to ensure people follow the obj_read_lock() rule, the
> comment before it may have to be a bit stronger, I wonder?

The fact that we do it in a lock in reprepare_packed_git() in the
only current caller does raise suspicion that someone could call it
later and not realize that the lock could be helpful. Inlining the
change within reprepare_packed_git() makes the most sense here
instead of mimicking the pattern.
 
>> Perhaps not, but as the resulting patch is much shorter it seems worth
>> considering.

The shortness of the patch is metric of quality, to me. The other
reason (we might introduce a footgun) is more interesting.

>> ...but to continue the above, the *really* important thing here (and
>> correct me if I'm wrong) is that we really need to *first* prepare the
>> alternates, and *then* do the rest, as our new alternates might point to
>> new loose objects and packs.
> 
> Yes, and as Derrick explained in another message, we only have to
> worry about new ones getting added, not existing ones going away.

I'll be sure to clarify that in my next version.

>> So with both of the above (the same could be done with your new helper)
>> something like this would IMO make that much clearer:
>>
>> 	diff --git a/packfile.c b/packfile.c
>> 	index 79e21ab18e7..50cb46ca4b7 100644
>> 	--- a/packfile.c
>> 	+++ b/packfile.c
>> 	@@ -1008,6 +1008,13 @@ void reprepare_packed_git(struct repository *r)
>> 	 	struct object_directory *odb;
>> 	 
>> 	 	obj_read_lock();
>> 	+	/*
>> 	+	 * New alternates might point to new loose & pack dirs, so we
>> 	+	 * must first read those.
>> 	+	 */
>> 	+	r->objects->loaded_alternates = 0;
>> 	+	prepare_alt_odb(r);
>> 	+
>> 	 	for (odb = r->objects->odb; odb; odb = odb->next)
>> 	 		odb_clear_loose_cache(odb);
>>
>> And, I think this is an exsiting edge case, but we're only locking the
>> ODB of the "parent" repository in this case, so if we have alternates in
>> play aren't we going to racily compute the rest here, the loose objects
>> and packs of the alternates we're about to consult aren't under the same
>> lock?

I don't understand what you are saying here. odb_read_lock() does not
specify a repository and is instead a global lock on reading from any
object database.

Here is its implementation:

extern int obj_read_use_lock;
extern pthread_mutex_t obj_read_mutex;

static inline void obj_read_lock(void)
{
	if(obj_read_use_lock)
		pthread_mutex_lock(&obj_read_mutex);
}

static inline void obj_read_unlock(void)
{
	if(obj_read_use_lock)
		pthread_mutex_unlock(&obj_read_mutex);
}

So I don't believe that your proposed edge case exists.

Thanks,
-Stolee