Re: [PATCH 14/14] shortlog: match commit trailers with --ident

From: Jeff King <peff@peff.net>
Date: Dec 29, 2015, 07:50 UTC
Message-ID: <20151229075013.GA9191@sigill.intra.peff.net>
In-Reply-To: <20151229073832.GN8842@sigill.intra.peff.net>

A few comments on my own patch...

On Tue, Dec 29, 2015 at 02:38:32AM -0500, Jeff King wrote:

Show 5 quoted lines

> If a project uses commit trailers, this patch lets you use
> shortlog to see who is performing each action. For example,
> running:
> 
>   git shortlog -ns --ident=reviewed-by

So obviously you can do something similar by piping git-log into grep. What I think this buys us is the ability to use the trailer-parsing code rather than hacking together a regex. And it lets you do stuff like this:

> in git.git shows who has reviewed. You can even use a custom
> format to see things like who has helped whom:
> 
>   git shortlog --format="...helped %an (%ad)" --ident=helped-by

Which relies on correlating trailers and their matching commits.

Since there may be multiple entries per commit, this always ends up as:

  Helper Ident (1):
          ...helped Author Ident
	  ...helped Other Author
	  etc

It would be neat to be able to format it so that each line was a full record (which would make further stats easier). We'd need a new formatting option for that.

Show 13 quoted lines

> diff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt
> index a89a01e..16080c4 100644
> --- a/Documentation/git-shortlog.txt
> +++ b/Documentation/git-shortlog.txt
> @@ -54,6 +54,15 @@ OPTIONS
>  +
>   - `author`, commits are grouped by author (this is the default)
>   - `committer`, commits are grouped by committer
> + - any other value, the `<type>` is interpreted as a case-insensitive
> +   commit message trailer (see linkgit:git-interpret-trailers[1]). For
> +   example, if your project uses `Reviewed-by` trailers, you might want
> +   to see who has been reviewing with
> +   `git shortlog -ns --ident=reviewed-by`.

This doesn't leave us many syntactic "outs" for adding new ident types in the future (nor can you match a trailer called "Author"). I guess we could call this "--ident=trailer:reviewed-by" to be more precise, but it's more annoying to type. Perhaps we could consider that the "official" syntax, and DWIM "--ident=foo" to "--ident=trailer:foo", with the caveat that it may change in the future. I dunno.

> ++
> +Note that commits that do not include the trailer will not be counted.
> +Likewise, commits with multiple trailers (e.g., multiple signoffs) may
> +count more than once.

I think counting multiples is the only sensible thing here. If you have two helped-by fields, for example it would make sense to credit each helper. For S-o-b, where the same person may sign-off twice, it does mean you get credited twice for a single commit. I'm not sure if that's a feature or not. :)

-Peff