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

[Junio C Hamano <gitster@pobox.com>]

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

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

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