Re: [PATCH v8 2/5] am: stop exporting GIT_COMMITTER_DATE

From: Phillip Wood <phillip.wood123@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Elijah Newren" <newren@gmail.com>,
	"Rohit Ashiwal" <rohit.ashiwal265@gmail.com>,
	"Đoàn Trần Công Danh" <congdanhqx@gmail.com>,
	"Alban Gruin" <alban.gruin@gmail.com>,
	"Git Mailing List" <git@vger.kernel.org>,
	"Phillip Wood" <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH v8 2/5] am: stop exporting GIT_COMMITTER_DATE
Date: Thu, 20 Aug 2020 16:23:33 +0100	[thread overview]
Message-ID: <8f43c23e-8472-385a-eba7-cafd24505e84@gmail.com> (raw)
In-Reply-To: <xmqqr1s2vdkq.fsf@gitster.c.googlers.com>

On 19/08/2020 16:51, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> 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.
> 