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

[Jeff King <peff@peff.net>]

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@alum.mit.edu> 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.

-Peff