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

From: Jeff King <peff@peff.net>
Date: Sep 7, 2017, 04:33 UTC
Message-ID: <20170907043354.fwxiifzbqf3yt5rr@sigill.intra.peff.net>
In-Reply-To: <xmqqtw0f61xr.fsf@gitster.mtv.corp.google.com>

On Thu, Sep 07, 2017 at 04:51:12AM +0900, Junio C Hamano wrote:

Show 19 quoted lines

> > 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.

> 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.

-Peff