Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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?
