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

[Brandon Williams <bmwill@google.com>]

On 05/17, Jeff King wrote:
> On Wed, May 17, 2017 at 05:17:17PM -0700, Brandon Williams wrote:
> 
> > > 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.
> 
> One used, you are not allowed to free a lockfile struct (actually these
> days it's just the "tempfile" part of it), because it lives on the
> cleanup-handler's tempfile_list forever.
> 
> This is a holdover from the early days of the lockfile code, but I think
> we could loosen it (and that's the right solution in the long run).
> 

Ah ok, thanks for the info!

> > > 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.
> 
> Oh, I agree it's a probable thing for the future. I was mostly wondering
> about the immediate change. I think this patch makes things slightly
> worse for that future, but the right fix is to remove the weird tempfile
> lifetime requirement in the first place.
> 
> -Peff

-- 
Brandon Williams