Re: [PATCH 10/23] files_ref_store: put the packed files lock directly in this struct

From: Brandon Williams <bmwill@google.com>
Date: May 18, 2017, 00:17 UTC
Message-ID: <20170518001717.GF185461@google.com>
In-Reply-To: <20170517131753.rditx62clmkrdmeq@sigill.intra.peff.net>

On 05/17, Jeff King wrote:

Show 15 quoted lines

> On Wed, May 17, 2017 at 02:05:33PM +0200, Michael Haggerty wrote:
> 
> > Instead of using a global `lock_file` instance for the main
> > "packed-refs" file and using a pointer in `files_ref_store` to keep
> > track of whether it is locked, embed the `lock_file` instance directly
> > in the `files_ref_store` struct and use the new
> > `is_lock_file_locked()` function to keep track of whether it is
> > locked. This keeps related data together and makes the main reference
> > store less of a special case.
> 
> This made me wonder how we handle the locking for ref_stores besides the
> main one (e.g., for submodules). The lockfile structs have to remain
> valid for the length of the program. Previously those stores could have
> xcalloc()'d a lockfile and just leaked it. Now they'll need to xcalloc()
> and leak their whole structs.

Wait, why would this be the case? I would assume that you would allocate a ref_store (including a lockfile for it) and then once you are done with the ref_store, you free it (and unlock and free the lockfile). I'm definitely not versed in ref handling code though so I may be missing something.

> 
> I suspect the answer is "we don't ever lock anything except the main ref
> store because that is the only one we write to", so it doesn't matter
> anyway.

Really? I can envision a case in the future where we'd want to update a ref, or create a ref inside a submodule.

-- 
Brandon Williams