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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sep 14, 2021, 10:31 UTC
Message-ID: <0c067b02-44e4-f719-42d9-7ceac5804e4a@gmail.com>
In-Reply-To: <xmqqczpccd57.fsf@gitster.g>

On 13/09/2021 23:40, Junio C Hamano wrote:

Show 13 quoted lines

> René Scharfe <l.s.r@web.de> writes:
> 
>> Am 13.09.21 um 17:19 schrieb Phillip Wood via GitGitGadget:
>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>
>>> 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.

For better or worse reusing the same strbuf is a common pattern and when I saw the code switch to using a different variable I wondered if it was because the value of buf was being used later. It is also confusing to free msg in a different place to all the other variables. rebase_cmd() being so long does not help (msg is defined 763 lines before its first use) as it makes it harder to see what is going on.

Show 8 quoted lines

>> 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.

It is confusing to the reader though, I spent time checking why the strbuf_release() call was there before concluding it was a mistake.

Show 14 quoted lines

> 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.

In a short function where it is easy to see what happens between the variable declaration and its use I'd agree but here everything is so spread out I actually found the switch to a second variable confusing.

Best Wishes

Phillip

> Thanks.
>