Re: [PATCH v4 11/12] refs: implement logic to migrate between ref storage formats

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 1655 bytes --]

On Wed, Jun 05, 2024 at 09:59:14AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > Coverity complains here of a leak of the xstrdup(). The return from
> > mkdtemp() should generally point to the same buffer we passed in, but if
> > it sees an error it will return NULL and the new heap buffer will be
> > lost.
> >
> > Probably unlikely, but since you are on a leak-checking kick, I thought
> > I'd mention it. ;)
> >
> > Since you have a writable strbuf already, maybe:
> >
> >   new_gitdir = mkdtemp(buf.buf);
> >   if (!new_gitdir)
> > 	...
> >   new_gitdir = strbuf_detach(&buf, NULL); /* same pointer, but now we own it */
> >
> > Or since "buf" is not used for anything else, we could just leave it
> > attached to the strbuf. And probably give it a better name. Maybe:
> > ...
> 
> Hmph, I think this is the second one we want to amend on the topic
> and it seems that I merged it a bit too prematurely.
> 
> I do not mind reverting the topic out of 'next' and actually would
> prefer replacing it with a corrected version, which would allow us
> to merge the clean copy to the next release.

I wouldn't exactly say prematurely, given that it likely wouldn't have
gotten a review without the merge because it was spurred by Coverity :)
I really wish that the Coverity tooling was available to run at will and
locally in our pipelines so that we can stop reacting to it, but instead
address whatever it flags _before_ the code hits the target branch. But,
well, that's not how Coverity works.

Anyway, I'll send a revised version in a bit. Thanks for your extra
review, Peff!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]