Re: [PATCH 2/2] bugreport: slightly better memory management

[Junio C Hamano <gitster@pobox.com>]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> 238b439d69 (bugreport: add tool to generate debugging info, 2020-04-16)
> introduces an UNLEAK for a strbuf that contains the buffer that gets
> flushed to disk earlier, instead of simply cleaning the buffer.
>
> do so, and while at it, move the free() call for another temporary string
> closer to its creator, so it is easier to keep track of.

'do so' -> 'Do so'.

>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  builtin/bugreport.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index 17042381c3..a9bedde1e8 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -152,6 +152,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
>  	strbuf_addstr(&report_path, "git-bugreport-");
>  	strbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);
>  	strbuf_addstr(&report_path, ".txt");
> +	free(prefixed_filename);

Correct, but it can be raised even further.  We can free it after we
addstr to report_path.

I looked at the existing callers of prefix_filename(), hoping that
many of them might take it in a strbuf they have, in which case we
may be able to expose an alternative interface to take a caller
supplied strbuf to clean this one up.  But it seems this is the only
one, so "store in a temporary, strbuf_addstr it, and immediately
free the temporary" here would be good.

>  	switch (safe_create_leading_directories(report_path.buf)) {
>  	case SCLD_OK:
> @@ -181,6 +182,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
>  		die_errno(_("unable to write to %s"), report_path.buf);
>  
>  	close(report);
> +	strbuf_release(&buffer);

We are done with the strbuf once write_in_full() returns, but this
is close enough.

> @@ -191,8 +193,6 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
>  	fprintf(stderr, _("Created new report at '%s'.\n"),
>  		user_relative_path);
>  
> -	free(prefixed_filename);
> -	UNLEAK(buffer);
>  	UNLEAK(report_path);
>  	return !!launch_editor(report_path.buf, NULL, NULL);

Having reviewed all, I am not sure if my reaction is "good, now we
are cleaner" or "meh, for the same reason why report_path can be
left alive, it is fine to leave buffer alive, too".