Re: [PATCH 03/20] pretty: fix memory leaks when parsing pretty formats

[Patrick Steinhardt <ps@pks.im>]

On Tue, Aug 20, 2024 at 01:36:11PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > When parsing pretty formats from the config we leak the name and user
> > format whenever these are set multiple times. This is because we do not
> > free any already-set value in case there is one.
> >
> > Plugging this leak for the name is trivial. For the user format we need
> > to be a bit more careful, because we may end up assigning a pointer into
> > the allocated region when the string is prefixed with either "format" or
> > "tformat:". In order to make it safe to unconditionally free the user
> > format we thus strdup the stripped string into the field instead of a
> > pointer into the string.
> 
> Yup, the stripped one is trickier, but the change looks correct.
> 
> Will queue.
> 
> By the way, we tend to prefer no spaces after (cast) in our
> codebase, but I just noticed that it is not spelled out in the
> coding guidelines.  Comparing
> 
>     $ git grep -E -e '\((int|char \*)\) ' \*.c ':!compat/' ':!contrib/'
>     $ git grep -E -e '\((int|char \*)\)[^ ]' \*.c ':!compat/' ':!contrib/'
> 
> tells me that the extra space after the (cast) is found mostly in
> borrowed or imported sources and majority of culprits are found in
> reftable library X-<.

Not entirely surprising. I myself have typically favored adding a space
after the cast, and I remember wondering about the coding style several
times here. I never wondered enough to actually check though.

So thanks for clarifying and thanks for updating the coding guidelines
around this.

Patrick