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