Re: [PATCH v2 08/11] rebase: remove redundant strbuf
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 13, 2021, 22:40 UTC
- Message-ID
- <xmqqczpccd57.fsf@gitster.g>
- In-Reply-To
- <3c7c5560-2cae-a4cf-a298-6d77a3fb9226@web.de>
René Scharfe <l.s.r@web.de> writes:
Show 20 quoted lines
> 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. > > 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.