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

[Jeff King <peff@peff.net>]

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).

> > 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