Re: [PATCH 17/23] get_packed_ref_cache(): assume "packed-refs" won't change while locked
- Date: Thu, 18 May 2017 09:58:53 -0700
- From: Stefan Beller <sbeller@xxxxxxxxxx>
- Subject: 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
>> 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
> 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.