Re: [PATCH 27/34] shortlog: release strbuf after use in insert_one_record()

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> On Thu, Sep 07, 2017 at 04:51:12AM +0900, Junio C Hamano wrote:
>
>> > diff --git a/builtin/shortlog.c b/builtin/shortlog.c
>> > index 43c4799ea9..48af16c681 100644
>> > --- a/builtin/shortlog.c
>> > +++ b/builtin/shortlog.c
>> > @@ -50,66 +50,67 @@ static int compare_by_list(const void *a1, const void *a2)
>> >  static void insert_one_record(struct shortlog *log,
>> >  			      const char *author,
>> >  			      const char *oneline)
>> >  {
>> > ...
>> >  	item = string_list_insert(&log->list, namemailbuf.buf);
>> > +	strbuf_release(&namemailbuf);
>> 
>> As log->list.strdup_strings is set to true in shortlog_init(),
>> namemailbuf.buf will leak without this.
>> 
>> An alterative, as this is the only place we add to log->list, could
>> be to make log->list take ownership of the string by not adding a
>> _release() here but instead _detach(), I guess.
>
> I agree that would be better, but I think it's a little tricky. The
> string_list_insert() call may make a new entry, or it may return an
> existing one. We'd still need to free in the latter case. I'm not sure
> the string_list interface makes it easy to tell the difference.

True; I do not think string_list API does.  But for this particular
application, I suspect that we can by looking at the util field of
the item returned.  A newly created one has NULL, but we always make
it non-NULL before leaving this function.

>> It is also curious that at the end of shortlog_output(), we set the
>> log->list.strdup_strings again to true immediately before calling
>> string_list_clear() on it.  I suspect that is unnecessary?
>
> I think so. I was curious whether I might have introduced this weirdness
> when I refactored this code a year or so ago. But no, it looks like it
> dates all the way back to the very first version of shortlog.c.

Hmph.