Web lists-archives.com

Re: [PATCH 17/23] get_packed_ref_cache(): assume "packed-refs" won't change while locked




On Wed, May 17, 2017 at 6:15 PM, Jeff King <peff@xxxxxxxx> wrote:
> On Wed, May 17, 2017 at 10:57:34AM -0700, Stefan Beller wrote:
>
>> On Wed, May 17, 2017 at 5:05 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote:
>> > If we've got the "packed-refs" file locked, then it can't change;
>> > there's no need to keep calling `stat_validity_check()` on it.
>>
>> This change will work in a world where all Git implementations
>> obey a lock. If there is at least one implementation that doesn't
>> care about the existence of lock files we may introduce a race
>> here.
>>
>> I am not sure if it is worth to be extra careful in the common case
>> though. But I could imagine some people using a git repo on an
>> NFS concurrently with different implementations and one of them
>> is an old / careless lock-ignoring implementation.
>>
>> My opinion is not strong enough that I'd veto such a patch
>> just food for thought.
>
> You're so unbelievably screwed if somebody is not respecting the lock
> that I don't think it's worth considering.
>
> This change just drops the stat_validity_check(), so you may fail to
> realize that somebody racily (without holding the lock!) changed the
> packed refs, and may omit a ref from your traversal if it moved from
> loose to packed. That _can_ have lasting corruption effects if your
> operation is something like "git prune" that is computing full
> reachability.
>
> But even without this change, somebody writing the packed-refs file
> without lock is likely to hose over simultaneous writes and lose ref
> updates (or even lose refs entirely). So anybody who doesn't respect the
> locks is broken, period, and needs to be fixed.

ok, in that case I see no objection for this patch.

Stefan