Re: v2.46.0-rc0 test failures on cygwin

On Tue, Jul 23, 2024 at 12:30:27PM +0200, Patrick Steinhardt wrote:

quoted

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.

quoted

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