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