Re: v2.46.0-rc0 test failures on cygwin
- From
Jeff King <peff@peff.net>
- Date
- Jul 23, 2024, 20:49 UTC
- Message-ID
- <20240723204953.GA6779@coredump.intra.peff.net>
- In-Reply-To
- <Zp-GQ0MQmuIyAear@tanuki>
On Tue, Jul 23, 2024 at 12:30:27PM +0200, Patrick Steinhardt wrote:
Show 9 quoted lines
> > So my biggest question is just whether there is any downside to doing > > the release/init pair rather than trying to reuse the existing struct. > > There shouldn't be any downside, but it is wasted effort. The main ref > store should always be accessed via `get_main_ref_store()`, and that > function knows to lazily initialize the refdb as required. So instead, I > think the preferable fix is to release the new ref store after we have > populated it and set the main ref store of the repository to `NULL` > instead of re-initializing it.
Ah, that is even better. I'm not too worried about wasted effort (this is a one-time thing during the ref backend migration), but I think we are better off leaving as much to the "regular" ref code as possible.
> So the following should be sufficient: > [...]
Yeah, that looks nice.
Show 11 quoted lines
> > I do think it probably causes a small memory leak. The "init" function > > allocates the actual ref_store struct, but the "release" function > > doesn't seem to free it. So we are probably leaking the store that > > points to the temp directory. But that is also true of "old_refs", or of > > "new_refs" if we hit an error. So I think the solution is probably for > > init/release to be symmetric, and for the latter to clean up everything. > > But again, I'd prefer to get input from Patrick there. > > Yeah, we'd have to free the new ref store struct, as well. I wouldn't > make `release()` free the structure as that would be uncustomarily > named.
We usually don't free in the release() function because it was not allocated on the init() side in the first place. The real sin here is that the init/release pair is not symmetric, regardless of what they are named.
In the patch above you worked around it by just doing a manual free() of the struct. That's crossing the abstraction barrier a little bit, but I think is OK in this instance. We don't generally expect a ref stores to be created and released a lot. If this were a general purpose data structure like a strbuf that gets used everywhere, I'd be more concerned.
-Peff