On 19/08/2020 16:51, Junio C Hamano wrote:
Phillip Wood <phillip.wood123@xxxxxxxxx> writes:
For am and the sequencer fmt_ident() is in a function which is called
from a loop and it is convenient not to have to worry about passing an
strbuf around or allocating and freeing it on each function call
That's strbuf on caller's stack, right? The actual string pointed
by the strbuf's buf pointer will be needed and on the heap with or
without the clean-up on top of the patch we are discussing, so I do
not think alloc/free would be in the picture when considering the
pros-and-cons.
At the moment we use the same allocation for the whole program (ignoring
reallocation if the string grows). If we were to add an strbuf as a
local variable in the function that gets called by the commit picking
loop we either need to make it static and accept the leak or alloc and
free the string each time the function is called. Either way I'd be
amazed if it has any effect on the performance given we're performing a
merge with each loop iteration.
The callers in ident (fmt_name(), git_author_info() and
git_committer_info()) return the string so they would need their own
strbuf or have to be changed so the caller passed one in. There are
quite a few callers of those functions and they seem to either
immediately call split_ident_line() or duplicate the returned string
(mostly the latter).
So it would be a bit of work to do it but not an enormous amount
(assuming we don't change the signature of git_author_info() etc in
ident.c, although given the pattern of callers it might be worth doing
that if they are mostly duplicating the returned string anyway)
I'd say that is more than "while at it" clean-up. It would be
easier to review and slightly easier to do if done before this patch
introduces rotating strbuf, but ...
I'm going to be off line for 10-14 days from the beginning of next week,
I could take a look at it after that, or we could leave it for a future
improvement - what do you think?
... I can be talked into punting it for later, at least for now.
I'm tempted to say leave it for now but if this isn't in next by the
time I'm back online I'll look at rerolling.
Best Wishes
Phillip
Thanks.
