threads / discuss / 44277

Allow "git shortlog" to group by committer information

Subject: Allow "git shortlog" to group by committer information

## tl;dr

24 messages between Oct 11, 2016 and Dec 21, 2016.

replies: 23people: 6as markdown or json

Linus Torvalds· Oct 11, 2016, 18:45 UTC · lore

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>
 builtin/shortlog.c | 15 ++++++++++++---
 shortlog.h         |  1 +
 2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index ba0e1154a..c9585d475 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -117,11 +117,15 @@ static void read_from_stdin(struct shortlog *log)
 {
 	struct strbuf author = STRBUF_INIT;
 	struct strbuf oneline = STRBUF_INIT;
+	static const char *author_match[2] = { "Author: ", "author " };
+	static const char *committer_match[2] = { "Commit: ", "committer " };
+	const char **match;
 
+	match = log->committer ? committer_match : author_match;
 	while (strbuf_getline_lf(&author, stdin) != EOF) {
 		const char *v;
-		if (!skip_prefix(author.buf, "Author: ", &v) &&
-		    !skip_prefix(author.buf, "author ", &v))
+		if (!skip_prefix(author.buf, match[0], &v) &&
+		    !skip_prefix(author.buf, match[1], &v))
 			continue;
 		while (strbuf_getline_lf(&oneline, stdin) != EOF &&
 		       oneline.len)
@@ -140,6 +144,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)
 	struct strbuf author = STRBUF_INIT;
 	struct strbuf oneline = STRBUF_INIT;
 	struct pretty_print_context ctx = {0};
+	const char *fmt;
 
 	ctx.fmt = CMIT_FMT_USERFORMAT;
 	ctx.abbrev = log->abbrev;
@@ -148,7 +153,9 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)
 	ctx.date_mode.type = DATE_NORMAL;
 	ctx.output_encoding = get_log_output_encoding();
 
-	format_commit_message(commit, "%an <%ae>", &author, &ctx);
+	fmt = log->committer ? "%cn <%ce>" : "%an <%ae>";
+
+	format_commit_message(commit, fmt, &author, &ctx);
 	if (!log->summary) {
 		if (log->user_format)
 			pretty_print_commit(&ctx, commit, &oneline);
@@ -238,6 +245,8 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
 	int nongit = !startup_info->have_repository;
 
 	const struct option options[] = {
+		OPT_BOOL('c', "committer", &log.committer,
+			 N_("Group by committer rather than author")),
 		OPT_BOOL('n', "numbered", &log.sort_by_number,
 			 N_("sort output according to the number of commits per author")),
 		OPT_BOOL('s', "summary", &log.summary,
diff --git a/shortlog.h b/shortlog.h
index 5a326c686..5d64cfe92 100644
--- a/shortlog.h
+++ b/shortlog.h
@@ -13,6 +13,7 @@ struct shortlog {
 	int in2;
 	int user_format;
 	int abbrev;
+	int committer;
 
 	char *common_repo_prefix;
 	int email;
Jeff King· Oct 11, 2016, 19:01 UTC · re: Linus Torvalds · lore

Re: Allow "git shortlog" to group by committer information

On Tue, Oct 11, 2016 at 11:45:58AM -0700, Linus Torvalds wrote:
Show 10 quoted lines
> 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
Linus Torvalds· Oct 11, 2016, 19:07 UTC · re: Jeff King · lore

Re: Allow "git shortlog" to group by committer information

On Tue, Oct 11, 2016 at 12:01 PM, Jeff King <peff@peff.net> 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.

Show 5 quoted lines
> 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
Jeff King· Oct 11, 2016, 19:17 UTC · re: Linus Torvalds · lore

Re: Allow "git shortlog" to group by committer information

On Tue, Oct 11, 2016 at 12:07:40PM -0700, Linus Torvalds wrote:
Show 10 quoted lines
> On Tue, Oct 11, 2016 at 12:01 PM, Jeff King <peff@peff.net> 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).

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.

Show 8 quoted lines
> 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.

Show 9 quoted lines
> > 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
Linus Torvalds· Dec 15, 2016, 21:29 UTC · re: Linus Torvalds · lore

Re: Allow "git shortlog" to group by committer information

Just a ping on this patch..

On Tue, Oct 11, 2016 at 11:45 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote:

Show 5 quoted lines
> 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
Junio C Hamano· Dec 16, 2016, 00:19 UTC · re: Linus Torvalds · lore

Re: Allow "git shortlog" to group by committer information

Linus Torvalds <torvalds@linux-foundation.org> writes:
Show 9 quoted lines
> Just a ping on this patch..
> 
> 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, ...
>
> 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..

This fell off the radar partly because of the distractions like "there are other attempts and other ways", and also because the message was not a text-plain that can be reviewed inline. Let me try to dig it up from the mail archive to see if I can find it.

Linus Torvalds· Dec 16, 2016, 01:39 UTC · re: Junio C Hamano · lore

Re: Allow "git shortlog" to group by committer information

On Thu, Dec 15, 2016 at 4:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
>
> This fell off the radar partly because of the distractions like
> "there are other attempts and other ways", and also because the
> message was not a text-plain that can be reviewed inline.  Let me
> try to dig it up from the mail archive to see if I can find it.

Sorry, I'll just re-send it without the attachment. I prefer inline myself, but I thought you didn't care (and gmail makes it unnecessarily hard).

                Linus
Junio C Hamano· Dec 16, 2016, 04:56 UTC · re: Linus Torvalds · lore

Re: Allow "git shortlog" to group by committer information

Linus Torvalds <torvalds@linux-foundation.org> writes:
Show 10 quoted lines
> On Thu, Dec 15, 2016 at 4:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> This fell off the radar partly because of the distractions like
>> "there are other attempts and other ways", and also because the
>> message was not a text-plain that can be reviewed inline.  Let me
>> try to dig it up from the mail archive to see if I can find it.
>
> Sorry, I'll just re-send it without the attachment. I prefer inline
> myself, but I thought you didn't care (and gmail makes it
> unnecessarily hard).
Thanks.

I do care, but I try to be lenient for inexperienced contriburors, which you don't qualify ;-) Experienced ones are held to a higher standard.

Linus Torvalds· Dec 16, 2016, 01:45 UTC · re: Junio C Hamano · lore

[PATCH 1/1] Allow "git shortlog" to group by committer information

From: Linus Torvalds <torvalds@linux-foundation.org>
Subject: Allow "git shortlog" to group by committer information
Date: Tue, 11 Oct 2016 11:45:58 -0700

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. During the merge window this allows me to do things like

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

to easily create a list of the top-20 committers that I haven't gotten pull requests from yet (the committer is not necessarily the person who will send the pull request, but it's a reasonably good approximation).

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 builtin/shortlog.c | 15 ++++++++++++---
 shortlog.h         |  1 +
 2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index ba0e1154a..c9585d475 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -117,11 +117,15 @@ static void read_from_stdin(struct shortlog *log)
 {
 	struct strbuf author = STRBUF_INIT;
 	struct strbuf oneline = STRBUF_INIT;
+	static const char *author_match[2] = { "Author: ", "author " };
+	static const char *committer_match[2] = { "Commit: ", "committer " };
+	const char **match;
 
+	match = log->committer ? committer_match : author_match;
 	while (strbuf_getline_lf(&author, stdin) != EOF) {
 		const char *v;
-		if (!skip_prefix(author.buf, "Author: ", &v) &&
-		    !skip_prefix(author.buf, "author ", &v))
+		if (!skip_prefix(author.buf, match[0], &v) &&
+		    !skip_prefix(author.buf, match[1], &v))
 			continue;
 		while (strbuf_getline_lf(&oneline, stdin) != EOF &&
 		       oneline.len)
@@ -140,6 +144,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)
 	struct strbuf author = STRBUF_INIT;
 	struct strbuf oneline = STRBUF_INIT;
 	struct pretty_print_context ctx = {0};
+	const char *fmt;
 
 	ctx.fmt = CMIT_FMT_USERFORMAT;
 	ctx.abbrev = log->abbrev;
@@ -148,7 +153,9 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)
 	ctx.date_mode.type = DATE_NORMAL;
 	ctx.output_encoding = get_log_output_encoding();
 
-	format_commit_message(commit, "%an <%ae>", &author, &ctx);
+	fmt = log->committer ? "%cn <%ce>" : "%an <%ae>";
+
+	format_commit_message(commit, fmt, &author, &ctx);
 	if (!log->summary) {
 		if (log->user_format)
 			pretty_print_commit(&ctx, commit, &oneline);
@@ -238,6 +245,8 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
 	int nongit = !startup_info->have_repository;
 
 	const struct option options[] = {
+		OPT_BOOL('c', "committer", &log.committer,
+			 N_("Group by committer rather than author")),
 		OPT_BOOL('n', "numbered", &log.sort_by_number,
 			 N_("sort output according to the number of commits per author")),
 		OPT_BOOL('s', "summary", &log.summary,
diff --git a/shortlog.h b/shortlog.h
index 5a326c686..5d64cfe92 100644
--- a/shortlog.h
+++ b/shortlog.h
@@ -13,6 +13,7 @@ struct shortlog {
 	int in2;
 	int user_format;
 	int abbrev;
+	int committer;
 
 	char *common_repo_prefix;
 	int email;
Stephen & Linda Smith· Dec 16, 2016, 01:51 UTC · re: Junio C Hamano · lore

Re: Allow "git shortlog" to group by committer information

On Thursday, December 15, 2016 5:39:53 PM MST Linus Torvalds wrote:
Show 7 quoted lines
> On Thu, Dec 15, 2016 at 4:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Sorry, I'll just re-send it without the attachment. I prefer inline
> myself, but I thought you didn't care (and gmail makes it
> unnecessarily hard).
> 
>                 Linus
Why does gmail make it unnecessarily hard?

I thought that a good percentage of the kernel maintainers use git send-email. what would make that command easier to use with gmail?

sps
Linus Torvalds· Dec 16, 2016, 02:00 UTC · re: Stephen & Linda Smith · lore

Re: Allow "git shortlog" to group by committer information

On Thu, Dec 15, 2016 at 5:51 PM, Stephen & Linda Smith <ischis2@cox.net> wrote:
>
> Why does gmail make it unnecessarily hard?

I read email with the gmail web interface, which is wonderful because of the server-side searching etc. The only real downside is the weak threading, but you get used to it.

I personally find IMAP and POP to be a tool of the devil, and have never had a good experience with them as a mail interface. In theory IMAP is supposed to support server-side searches, in practice it never worked for me.

But the problem with sending patches using the web interface is that you cannot attach things inline without gmail screwing up whitespace.

I suggested to some googler that a "attach inline" checkbox in the would be a wonderful option for text attachments, but considering that the android gmail app still has no text-only option I don't think that suggestion went anywhere.

> I thought that a good percentage of the kernel maintainers use git send-email.
> what would make that command easier to use with gmail?

Oh, I can send inline stuff (as I just re-sent that patch), but then I have to fire up alpine and do it the old-fashioned way. So it's an extra step. So since I spend all my time at the gmail web interface _anyway_, the attachment model ends up being the slightly more convenient one.

And sure, I could use git-send-email as that extra step instead, but I'd rather just use alpine. That's the extra step I do for some other things (ie the "200-email patch-bomb from Andrew Morton" things - I'm not using the web interface for _that_).

                 Linus
Jeff King· Dec 16, 2016, 13:39 UTC · re: Linus Torvalds · lore

Re: Allow "git shortlog" to group by committer information

On Thu, Dec 15, 2016 at 01:29:47PM -0800, Linus Torvalds wrote:
Show 17 quoted lines
> On Tue, Oct 11, 2016 at 11:45 AM, Linus Torvalds
> <torvalds@linux-foundation.org> 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

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/
Jeff King· Dec 16, 2016, 13:51 UTC · re: Jeff King · lore

Re: Allow "git shortlog" to group by committer information

On Fri, Dec 16, 2016 at 08:39:40AM -0500, Jeff King wrote:
Show 10 quoted lines
> 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 <peff@peff.net>
---
 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
Junio C Hamano· Dec 16, 2016, 17:27 UTC · re: Jeff King · lore

Re: Allow "git shortlog" to group by committer information

Jeff King <peff@peff.net> writes:
> 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).
Yeah, I think it probably is OK.

As it is very clear that "group by author" is the default, there is no need to add the corresponding "-a/--author" option, either. The fact that "--no-committer" can countermand an earlier "--committer" on the command line is just how options work, so it probably does not deserve a separate mention, either.

Thanks.
Show 50 quoted lines
> -- >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 <peff@peff.net>
> ---
>  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
Johannes Sixt· Dec 20, 2016, 18:12 UTC · re: Jeff King · lore

Re: Allow "git shortlog" to group by committer information

Am 16.12.2016 um 14:51 schrieb Jeff King:
Show 23 quoted lines
> 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
>
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
Junio C Hamano· Dec 20, 2016, 18:19 UTC · re: Johannes Sixt · lore

Re: Allow "git shortlog" to group by committer information

Johannes Sixt <j6t@kdbg.org> writes:
Show 28 quoted lines
> Am 16.12.2016 um 14:51 schrieb Jeff King:
>> 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
>>
>
> May I kindly ask you to make this work on Windows, too? Just
>
> sed -i -e s/MINGW/MINGW,HAVENOT/ t4201-shortlog.sh
HAVENOT???
Show 6 quoted lines
>
> on your Linux box and make it pass the tests.
>
> Thank you so much in advance.
>
> -- Hannes
Junio C Hamano· Dec 20, 2016, 18:24 UTC · re: Junio C Hamano · lore

Re: Allow "git shortlog" to group by committer information

Junio C Hamano <gitster@pobox.com> writes:
Show 9 quoted lines
>> May I kindly ask you to make this work on Windows, too? Just
>>
>> sed -i -e s/MINGW/MINGW,HAVENOT/ t4201-shortlog.sh
>
> HAVENOT???
>>
>> on your Linux box and make it pass the tests.
>>
>> Thank you so much in advance.
Ah, I think I am slower than my usual today.
Junio C Hamano· Dec 20, 2016, 18:35 UTC · re: Junio C Hamano · lore

Re: Allow "git shortlog" to group by committer information

Junio C Hamano <gitster@pobox.com> writes:
Show 13 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> May I kindly ask you to make this work on Windows, too? Just
>>>
>>> sed -i -e s/MINGW/MINGW,HAVENOT/ t4201-shortlog.sh
>>
>> HAVENOT???
>>>
>>> on your Linux box and make it pass the tests.
>>>
>>> Thank you so much in advance.
>
> Ah, I think I am slower than my usual today.
-- >8 --
Subject: SQUASH???

Make sure the test does not depend on the result of the previous tests; with MINGW prerequisite satisfied, a "reset to original and rebuild" in an earlier test was skipped, resulting in different history being tested with this and the next tests.

---
 t/t4201-shortlog.sh | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index 6c7c637481..9df054bf05 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -191,8 +191,14 @@ test_expect_success 'shortlog with --output=<file>' '
 '
 
 test_expect_success 'shortlog --committer (internal)' '
+	git checkout --orphan side &&
+	git commit --allow-empty -m one &&
+	git commit --allow-empty -m two &&
+	GIT_COMMITTER_NAME="Sin Nombre" git commit --allow-empty -m three &&
+
 	cat >expect <<-\EOF &&
-	     3	C O Mitter
+	     2	C O Mitter
+	     1	Sin Nombre
 	EOF
 	git shortlog -nsc HEAD >actual &&
 	test_cmp expect actual
Johannes Sixt· Dec 20, 2016, 18:52 UTC · re: Junio C Hamano · lore

Re: Allow "git shortlog" to group by committer information

Am 20.12.2016 um 19:35 schrieb Junio C Hamano:
Show 5 quoted lines
>  test_expect_success 'shortlog --committer (internal)' '
> +	git checkout --orphan side &&
> +	git commit --allow-empty -m one &&
> +	git commit --allow-empty -m two &&
> +	GIT_COMMITTER_NAME="Sin Nombre" git commit --allow-empty -m three &&
Clever! Thank you. Will test in 12 hours.
Show 9 quoted lines
> +
>  	cat >expect <<-\EOF &&
> -	     3	C O Mitter
> +	     2	C O Mitter
> +	     1	Sin Nombre
>  	EOF
>  	git shortlog -nsc HEAD >actual &&
>  	test_cmp expect actual
>
Johannes Sixt· Dec 21, 2016, 21:09 UTC · re: Johannes Sixt · lore

Re: Allow "git shortlog" to group by committer information

Am 20.12.2016 um 19:52 schrieb Johannes Sixt:
Show 19 quoted lines
> Am 20.12.2016 um 19:35 schrieb Junio C Hamano:
>>  test_expect_success 'shortlog --committer (internal)' '
>> +    git checkout --orphan side &&
>> +    git commit --allow-empty -m one &&
>> +    git commit --allow-empty -m two &&
>> +    GIT_COMMITTER_NAME="Sin Nombre" git commit --allow-empty -m three &&
>
> Clever! Thank you. Will test in 12 hours.
>
>> +
>>      cat >expect <<-\EOF &&
>> -         3    C O Mitter
>> +         2    C O Mitter
>> +         1    Sin Nombre
>>      EOF
>>      git shortlog -nsc HEAD >actual &&
>>      test_cmp expect actual
>>
>
I confirm that t4201 now passes on Windows with this fixup.
-- Hannes
Jeff King· Dec 21, 2016, 03:22 UTC · re: Junio C Hamano · lore

Re: Allow "git shortlog" to group by committer information

On Tue, Dec 20, 2016 at 10:35:36AM -0800, Junio C Hamano wrote:
Show 7 quoted lines
> -- >8 --
> Subject: SQUASH???
> 
> Make sure the test does not depend on the result of the previous
> tests; with MINGW prerequisite satisfied, a "reset to original and
> rebuild" in an earlier test was skipped, resulting in different
> history being tested with this and the next tests.
Yeah, this looks good, and obviously correct.

I do wonder if in general it should be the responsibility of skippable tests to make sure we end up with the same state whether they are run or not. That might manage the complexity more. But I certainly don't mind tests being defensive like you have here.

-Peff
Jacob Keller· Dec 21, 2016, 07:55 UTC · re: Jeff King · lore

Re: Allow "git shortlog" to group by committer information

On Tue, Dec 20, 2016 at 7:22 PM, Jeff King <peff@peff.net> wrote:
Show 18 quoted lines
> On Tue, Dec 20, 2016 at 10:35:36AM -0800, Junio C Hamano wrote:
>
>> -- >8 --
>> Subject: SQUASH???
>>
>> Make sure the test does not depend on the result of the previous
>> tests; with MINGW prerequisite satisfied, a "reset to original and
>> rebuild" in an earlier test was skipped, resulting in different
>> history being tested with this and the next tests.
>
> Yeah, this looks good, and obviously correct.
>
> I do wonder if in general it should be the responsibility of skippable
> tests to make sure we end up with the same state whether they are run or
> not. That might manage the complexity more. But I certainly don't mind
> tests being defensive like you have here.
>
> -Peff

That seems like a good idea, but I'm not sure how you would implement it in practice? Would we just "rely" on a skipable test having a "do this if we skip, instead" block? That would be easier to spot but I think still relies on the skip-able tests being careful?

Thanks, Jake

Jeff King· Dec 21, 2016, 16:04 UTC · re: Jacob Keller · lore

Re: Allow "git shortlog" to group by committer information

On Tue, Dec 20, 2016 at 11:55:45PM -0800, Jacob Keller wrote:
Show 11 quoted lines
> > I do wonder if in general it should be the responsibility of skippable
> > tests to make sure we end up with the same state whether they are run or
> > not. That might manage the complexity more. But I certainly don't mind
> > tests being defensive like you have here.
> >
> > -Peff
> 
> That seems like a good idea, but I'm not sure how you would implement
> it in practice? Would we just "rely" on a skipable test having a "do
> this if we skip, instead" block? That would be easier to spot but I
> think still relies on the skip-able tests being careful?

Yes, it definitely means the skip-able tests would have to be careful. But it's putting the onus on them, rather than on all the other tests.

If the rule is "the on-disk state must be the same whether or not the test runs", then I suspect many tests could get by with a test_when_finished, like:

  test_expect_success FOO 'test --foo knob' '
	git commit -m "new commit for test" &&
	test_when_finished "git reset --hard HEAD^" &&
	git log --foo >actual &&
	test_cmp expect actual
  '

It gets harder if you have multiple such tests that rely on intermediate state. Probably you'd have:

  test_expect_success FOO 'clean up FOO state' '
	git reset --hard HEAD^
  '
at the end of the sequence.

I dunno. It is unclear to me whether such a rule is worth it. Preemptively fighting these state-based bugs before they occur is a nice thought, but I think it may end up being more work to write the tests that way than it is to simply find and fix the bugs when they occur. Of course it also changes where the work falls (the test writers versus the bug hunters, and given that !MINGW is our most common prereq, I think Windows devs are over-represented in the latter case).

-Peff
Junio C Hamano· Dec 21, 2016, 20:44 UTC · re: Jeff King · lore

Re: Allow "git shortlog" to group by committer information

Jeff King <peff@peff.net> writes:
> I do wonder if in general it should be the responsibility of skippable
> tests to make sure we end up with the same state whether they are run or
> not. That might manage the complexity more. But I certainly don't mind
> tests being defensive like you have here.

If we speak "in general", I would say that any test should be prepared to be turned into a skippable one, and they should all make sure they leave the same state whether they are skipped, they succeed, or they fail in the middle.

That can theoretically be achievable (e.g. you assume you would always start from an empty repository, do your thing and arrange to leave an empty repository by doing test_when_finished), and the cognitive cost of developers to do so can be reduced by teaching test_expect_{success/failure} helpers to be responsible for the "arrange to leave an empty repository" part. But it is quite a big departure from the way our tests are currently done, i.e. prepare the environment once and then each of multiple tests observes one thing in that environment (e.g. "does it work well with --dry-run? how about without?").

Also it will make the runtime cost of the tests a lot larger, as setup and teardown need to happen for each individual test. So I do not think it is a good goal in practice.

Perhaps what you suggest may be a good middle-ground. When you add prerequisite to an existing test, it will become your responsibility to make sure the test will leave the same state. That way, you would know that tests that come later will not be affected by your change.

← back to recent threads