From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Sat, 01 Jul 2017 17:11:37 GMT
Subject: Re: [PATCH v2 28/29] repack_without_refs(): don't lock or unlock the packed refs
Message-ID: <71e58288-483b-3cfb-00db-523a28c78549@alum.mit.edu>
In-Reply-To: <20170623195600.cvn26gt7o5rtywy6@sigill.intra.peff.net>

On 06/23/2017 09:56 PM, Jeff King wrote:
> On Fri, Jun 23, 2017 at 09:01:46AM +0200, Michael Haggerty wrote:
> 
>> Change `repack_without_refs()` to expect the packed-refs lock to be
>> held already, and not to release the lock before returning. Change the
>> callers to deal with lock management.
>>
>> This change makes it possible for callers to hold the packed-refs lock
>> for a longer span of time, a possibility that will eventually make it
>> possible to fix some longstanding races.
> 
> This is the sort of clue I was thinking about in my last email. :)

I'll try to be better about that in the future.

And the reason to make `packed_ref_store` fulfill the `ref_store`
interface is mostly indirect at the moment. It was important (to my
sanity, if nothing else) to simplify the interface to the packed-refs
code before rewriting its innards. Since the `ref_store` interface is
familiar and is fairly close to what is needed, it seemed logical to use it.

>> The only semantic change here is that `repack_without_refs()` used to
>> forgot to release the lock in the `if (!removed)` exit path. That
>> omission is now fixed.
> 
> s/used to forgot/previously forgot/ or similar?

Thanks; will fix.

Michael