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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 7, 2023, 17:29 UTC
Message-ID: <xmqqttyw1ndr.fsf@gitster.g>
In-Reply-To: <230307.861qm0235d.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 25 quoted lines

> 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?

Show 7 quoted lines

> Perhaps not, but as the resulting patch is much shorter it seems worth
> considering.
>
> ...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.

Show 26 quoted lines

> 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?