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