From: Phillip Wood Date: Tue, 14 Sep 2021 10:31:41 GMT Subject: Re: [PATCH v2 08/11] rebase: remove redundant strbuf Message-ID: <0c067b02-44e4-f719-42d9-7ceac5804e4a@gmail.com> In-Reply-To: On 13/09/2021 23:40, Junio C Hamano wrote: > René Scharfe writes: > >> Am 13.09.21 um 17:19 schrieb Phillip Wood via GitGitGadget: >>> From: Phillip Wood >>> >>> 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. >> 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. > 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. >