From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 03 Oct 2024 16:26:23 GMT
Subject: Re: [PATCH v2 3/3] diff: modify output_prefix function pointer
Message-ID: <xmqqsetdi24w.fsf@gitster.g>
In-Reply-To: <e1d825ad212d91505eee9d911abbd3ba6bc170b1.1727956724.git.gitgitgadget@gmail.com>

"Jeff King via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Jeff King <peff@peff.net>
>
> The uses of the output_prefix function pointer in the diff_options
> struct is currently difficult to work with by returning a pointer to a
> strbuf. There is only one use that cares about the length of the string,
> which appears to be the only justification of the return type.
>
> We already noticed confusing memory issues around this return type, so
> use a const char * return type to make it clear that the caller does not
> own this string buffer.
>
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---
>  diff-lib.c   | 4 ++--
>  diff.c       | 8 +++-----
>  diff.h       | 2 +-
>  graph.c      | 4 ++--
>  log-tree.c   | 4 ++--
>  range-diff.c | 4 ++--
>  6 files changed, 12 insertions(+), 14 deletions(-)

Very nice.

>  			if (opt->diffopt.output_prefix) {
> -				struct strbuf *msg = NULL;
> +				const char *msg;
>  				msg = opt->diffopt.output_prefix(&opt->diffopt,
>  					opt->diffopt.output_prefix_data);
> -				fwrite(msg->buf, msg->len, 1, opt->diffopt.file);
> +				fwrite(msg, strlen(msg), 1, opt->diffopt.file);
>  			}

OK.  We are not relying on the strbuf being able to have embedded
NUL in the buffer, and this looks very sensible.

Thanks.