On 13/09/2021 23:40, Junio C Hamano wrote:
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.
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.
