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