From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mon, 04 Jan 2016 09:44:00 GMT
Subject: Re: [PATCH 11/14] shortlog: allow grouping by committer ident
Message-ID: <CAPig+cT1pNXt2U1bd-KVapiUD=-6TQ8KRBNC8y+0wLAtuqUhQQ@mail.gmail.com>
In-Reply-To: <20151229073515.GK8842@sigill.intra.peff.net>

On Tue, Dec 29, 2015 at 2:35 AM, Jeff King <peff@peff.net> wrote:
> Shortlog always groups commits by the author field. It can
> be interesting to group by other fields, as well. The
> obvious other identity in each commit is the committer
> field. This might be interesting if your project follows a
> workflow where committers and authors are not the same and
> where there is not simply one maintainer picking up all of
> the patches.
>
> For instance, you can use this in git.git to see interim and
> subsystem maintainers. Likewise, you can see in linux.git
> the patches picked up by lieutenants and merged into Linus's
> tree.
>
> This patch also provides some of the necessary
> infrastructure for adding more ident types (e.g., from
> trailers) in the future.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> 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?

> +       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`

> diff --git a/builtin/shortlog.c b/builtin/shortlog.c
> index d7eb0cb..39da2d4 100644
> --- a/builtin/shortlog.c
> +++ b/builtin/shortlog.c
> @@ -147,11 +157,23 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)
> -       format_commit_message(commit, "%an <%ae>", &ident, &ctx);
> -       if (ident.len <= 3) {
> -               warning(_("Missing author: %s"),
> -                   oid_to_hex(&commit->object.oid));
> -               return;
> +       switch (log->ident_type) {
> +       case SHORTLOG_IDENT_AUTHOR:
> +               format_commit_message(commit, "%an <%ae>", &ident, &ctx);
> +               if (ident.len <= 3) {
> +                       warning(_("Missing author: %s"),
> +                               oid_to_hex(&commit->object.oid));
> +                       return;
> +               }
> +               break;
> +       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.)

> +               }
> +               break;
>         }