Re: [PATCH 10/23] files_ref_store: put the packed files lock directly in this struct
- Date: Wed, 17 May 2017 21:11:53 -0400
- From: Jeff King <peff@xxxxxxxx>
- Subject: Re: [PATCH 10/23] files_ref_store: put the packed files lock directly in this struct
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.