Subject: Re: [PATCH v2 08/11] rebase: remove redundant strbuf

From: Junio C Hamano <[email-masked]>
Date: Mon, 13 Sep 2021 15:40:52 -0700

René Scharfe <[email-masked]> writes:

> Am 13.09.21 um 17:19 schrieb Phillip Wood via GitGitGadget:
>> From: Phillip Wood <[email-masked]>
>>
>> There is already an strbuf that can be reused for creating messages.
>
> Reminds me of a terrible joke from elementary school: In Peter's class
> everybody is called Klaus, except Franz -- his name is Michael.
>
> Why would we want to use the same variable for multiple purposes?  This
> makes the code harder to read.  And the allocation overhead for these
> few cases should be negligible.
>
> The most important question: Is this patch really needed to support
> tags (the purpose of this series)?
>
>> msg is not freed if there is an error and there is a logic error where
>> we call strbuf_release(&msg) followed by strbuf_reset(&msg) and
>> strbuf_addf(&msg).
>
> strbuf_reset() after strbuf_release() is redundant but legal.

All good points.

I do not care too deeply either way, in the sense that it probably
is better for this function to have two variables (with at least one
of them having a meaningful name "msg" that tells readers what it is
about), if the original submission to rewrite "rebase" in C used a
single strbuf for both of these and given it a name (like "tmp")
that makes it clear that the buffer is merely a temporary area
without any longer term significance, I probably wouldn't have told
the submitter to rewrite it to use separate strbuf variables.

But if existing code already uses two variables, with at least one
of them having a meaningful name that tells what it is used for, I
see no reason why we want to rewrite it to use a single one.

Thanks.