Allow "git shortlog" to group by committer information

In some situations you may want to group the commits not by author,
but by committer instead.

For example, when I just wanted to look up what I'm still missing from
linux-next in the current merge window, I don't care so much about who
wrote a patch, as what git tree it came from, which generally boils
down to "who committed it".

So make git shortlog take a "-c" or "--committer" option to switch
grouping to that.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

On Tue, Oct 11, 2016 at 11:45:58AM -0700, Linus Torvalds wrote:

In some situations you may want to group the commits not by author,
but by committer instead.

For example, when I just wanted to look up what I'm still missing from
linux-next in the current merge window, I don't care so much about who
wrote a patch, as what git tree it came from, which generally boils
down to "who committed it".

So make git shortlog take a "-c" or "--committer" option to switch
grouping to that.

I made a very similar patch as part of a larger series:

  http://public-inbox.org/git/20151229073515.GK8842@sigill.intra.peff.net/

but never followed through with it because it wasn't clear that grouping
by anything besides author was actually useful to anybody.

My implementation is a little more complicated because it's also setting
things up for grouping by trailers (so you can group by "signed-off-by",
for example). I don't know if that's useful to your or not.

I'm fine with this less invasive version, but a few suggestions:

 - do you want to call it --group-by=committer (with --group-by=author
   as the default), which could later extend naturally to other forms of
   grouping?

 - you might want to steal the tests and documentation from my patch
   (though obviously they would need tweaked to match your interface)

-Peff

On Tue, Oct 11, 2016 at 12:01 PM, Jeff King [off-list ref] wrote:

My implementation is a little more complicated because it's also setting
things up for grouping by trailers (so you can group by "signed-off-by",
for example). I don't know if that's useful to your or not.

Hmm. Maybe in theory. But probably not in reality - it's just not
unique enough (ie there are generally multiple, and if you choose the
first/last, it should be the same as author/committer, so it doesn't
actually add anything).

There are possibly other things that *could* be grouped by and might be useful:

 - main subdirectory it touches (I've often wanted that)

 - rough size of diff or number of files it touches

but realistically both are painful enough that it probably doesn't
make sense to do in some low-level helper.

I'm fine with this less invasive version, but a few suggestions:

 - do you want to call it --group-by=committer (with --group-by=author
   as the default), which could later extend naturally to other forms of
   grouping?

Honestly, it's probably the more generic one, but especially for
one-off commands that aren't that common, it's a pain to write. When
testing it, I literally just used "-c" for that reason.

I wrote the patch because I've wanted this before, but it's a "once or
twice a merge window" thing for me, so ..

 - you might want to steal the tests and documentation from my patch
   (though obviously they would need tweaked to match your interface)

Heh. Yes.

          Linus

On Tue, Oct 11, 2016 at 12:07:40PM -0700, Linus Torvalds wrote:

On Tue, Oct 11, 2016 at 12:01 PM, Jeff King [off-list ref] wrote:

quoted

My implementation is a little more complicated because it's also setting
things up for grouping by trailers (so you can group by "signed-off-by",
for example). I don't know if that's useful to your or not.

Hmm. Maybe in theory. But probably not in reality - it's just not
unique enough (ie there are generally multiple, and if you choose the
first/last, it should be the same as author/committer, so it doesn't
actually add anything).

The implementation I did credited each commit multiple times if the
trailer appeared more than once. If you want to play with it, you can
fetch it from:

  git://github.com/peff jk/shortlog-ident

and then something like:

  git shortlog --ident=reviewed-by --format='...reviewed %an'

works. I haven't found it to really be useful for more than toy
statistic gathering, though.

There are possibly other things that *could* be grouped by and might be useful:

 - main subdirectory it touches (I've often wanted that)

 - rough size of diff or number of files it touches

but realistically both are painful enough that it probably doesn't
make sense to do in some low-level helper.

Yeah, I think there's a lot of policy there in what counts as "main",
the rough sizes, etc. I've definitely done queries like that before, but
usually by piping "log --numstat" into perl. It's a minor pain to get
the data into perl data structures, but once you have it, you have a lot
more flexibility in what you can compute.

That might be aided by providing more structured machine-readable output
from git, like JSON (which I don't particularly like, but it's kind-of a
standard, and it sure as hell beats XML). But obviously that's another
topic entirely.

quoted

I'm fine with this less invasive version, but a few suggestions:

 - do you want to call it --group-by=committer (with --group-by=author
   as the default), which could later extend naturally to other forms of
   grouping?

Honestly, it's probably the more generic one, but especially for
one-off commands that aren't that common, it's a pain to write. When
testing it, I literally just used "-c" for that reason.

It's not the end of the world to call it "-c" now, and later define "-c"
as a shorthand for "--group-by=committer", if and when the latter comes
into existence.

Keep in mind that shortlog takes arbitrary revision options, too, and
"-c" is defined there for combined diffs. I can't think of a good reason
to want to pass it to shortlog, though, so I don't think it's a big
loss.

-Peff

Just a ping on this patch..

On Tue, Oct 11, 2016 at 11:45 AM, Linus Torvalds
[off-list ref] wrote:

In some situations you may want to group the commits not by author,
but by committer instead.

For example, when I just wanted to look up what I'm still missing from
linux-next in the current merge window [..]

It's another merge window later for the kernel, and I just re-applied
this patch to my git tree because I still want to know teh committer
information rather than the authorship information, and it still seems
to be the simplest way to do that.

Jeff had apparently done something similar as part of a bigger
patch-series, but I don't see that either. I really don't care very
much how this is done, but I do find this very useful, I do things
like

   git shortlog -cnse linus..next |
        head -20 |
        cut -f2 |
        sed 's/$/,/'

to generate a nice list of the top-20 committers that I haven't gotten
pull requests from yet.

Yes, I can just maintain this myself, and maybe nobody else needs it,
but it's pretty simple and straightforward, and there didn't seem to
be any real reason not to have the option..

                 Linus

On Thu, Dec 15, 2016 at 01:29:47PM -0800, Linus Torvalds wrote:

On Tue, Oct 11, 2016 at 11:45 AM, Linus Torvalds
[off-list ref] wrote:

quoted

In some situations you may want to group the commits not by author,
but by committer instead.

For example, when I just wanted to look up what I'm still missing from
linux-next in the current merge window [..]

It's another merge window later for the kernel, and I just re-applied
this patch to my git tree because I still want to know teh committer
information rather than the authorship information, and it still seems
to be the simplest way to do that.

Jeff had apparently done something similar as part of a bigger
patch-series, but I don't see that either. I really don't care very
much how this is done, but I do find this very useful, I do things
like

Sorry if I de-railed the earlier conversation. The shortlog
group-by-trailer work didn't seem useful enough for me to make it a
priority.

I'm OK with the approach your patch takes, but I think there were some
unresolved issues:

  - are we OK taking the short "-c" for this, or do we want
    "--group-by=committer" or something like it?

  - no tests; you can steal the general form from my [1]

  - no documentation (can also be stolen from [1], though the syntax is
    quite different)

-Peff

[1] http://public-inbox.org/git/20151229073515.GK8842@sigill.intra.peff.net/

On Fri, Dec 16, 2016 at 08:39:40AM -0500, Jeff King wrote:

I'm OK with the approach your patch takes, but I think there were some
unresolved issues:

  - are we OK taking the short "-c" for this, or do we want
    "--group-by=committer" or something like it?

  - no tests; you can steal the general form from my [1]

  - no documentation (can also be stolen from [1], though the syntax is
    quite different)

Being moved by the holiday spirit, I wrote a patch to address the latter
two. ;)

It obviously would need updating if we switch away from "-c", but I
think I am OK with the short "-c" (even if we add a more exotic grouping
option later, this can remain as a short synonym).

-- >8 --
Subject: [PATCH] shortlog: test and document --committer option

This puts the final touches on the feature added by
fbfda15fb8 (shortlog: group by committer information,
2016-10-11).

Signed-off-by: Jeff King <redacted>
---
 Documentation/git-shortlog.txt |  4 ++++
 t/t4201-shortlog.sh            | 13 +++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt
index 31af7f2736..ee6c5476c1 100644
--- a/Documentation/git-shortlog.txt
+++ b/Documentation/git-shortlog.txt

@@ -47,6 +47,10 @@ OPTIONS
 
 	Each pretty-printed commit will be rewrapped before it is shown.
 
+-c::
+--committer::
+	Collect and show committer identities instead of authors.
+
 -w[<width>[,<indent1>[,<indent2>]]]::
 	Linewrap the output by wrapping each line at `width`.  The first
 	line of each entry is indented by `indent1` spaces, and the second

diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index ae08b57712..6c7c637481 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh

@@ -190,4 +190,17 @@ test_expect_success 'shortlog with --output=<file>' '
 	test_line_count = 3 shortlog
 '
 
+test_expect_success 'shortlog --committer (internal)' '
+	cat >expect <<-\EOF &&
+	     3	C O Mitter
+	EOF
+	git shortlog -nsc HEAD >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'shortlog --committer (external)' '
+	git log --format=full | git shortlog -nsc >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
2.11.0.348.g960a0b554

Am 16.12.2016 um 14:51 schrieb Jeff King:

May I kindly ask you to make this work on Windows, too? Just

sed -i -e s/MINGW/MINGW,HAVENOT/ t4201-shortlog.sh

on your Linux box and make it pass the tests.

Thank you so much in advance.

-- Hannes