On 3/7/2023 12:29 PM, Junio C Hamano wrote:
> Æ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?
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
