From: Jeff King <peff@peff.net>
Date: Mon, 04 Jan 2016 10:23:00 GMT
Subject: Re: [PATCH 11/14] shortlog: allow grouping by committer ident
Message-ID: <20160104102300.GB32086@sigill.intra.peff.net>
In-Reply-To: <CAPig+cT1pNXt2U1bd-KVapiUD=-6TQ8KRBNC8y+0wLAtuqUhQQ@mail.gmail.com>

On Mon, Jan 04, 2016 at 04:44:00AM -0500, Eric Sunshine wrote:

> > diff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt
> > @@ -47,6 +47,14 @@ OPTIONS
> > +--ident=<type>::
> 
> Should this be called --group-by?

Yeah, that may be a more sensible name. For committer/author, I think
--ident is accurate. But for a trailer, we technically do not know what
is in it. I expect people would use this primarily with ident-based
trailers (like "reviewed-by" or whatever), but there's no reason you
could not collate references to a bug id, or something.

> > +       By default, `shortlog` collects and collates author identities;
> > +       using `--ident` will collect other types of identity. If
> > +       `<type>` is:
> > ++
> > + - `author`, commits are grouped by author (this is the default)
> > + - `committer`, commits are grouped by committer
> 
> There is a bit of redundancy here. I wonder if it could be described
> more succinctly. For instance, instead of all the above, perhaps:
> 
>     Group commits by `<type>`, which can be one of:
> 
>     - `author` (default)
>     - `committer`

I avoided that kind of parallel structure because I knew that later in
the series I would be adding a new bullet that would need more
explanation. I'm still pondering `trailer:<key>` as the syntax, which
would involve reworking this text. I'll keep it in mind when I do.

> > +       case SHORTLOG_IDENT_COMMITTER:
> > +               format_commit_message(commit, "%cn <%ce>", &ident, &ctx);
> > +               if (ident.len <= 3) {
> > +                       warning(_("Missing committer: %s"),
> > +                               oid_to_hex(&commit->object.oid));
> > +                       return;
> 
> Is this leaking strbuf 'ident'?
> 
> (Ditto for the "author" case as mentioned already in the patch 6/14 review.)

Yep. :) I think these can become "goto out" per my previous fixup.

-Peff