Allow "git shortlog" to group by committer information

Allow "git shortlog" to group by committer information

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 482 bytes --]

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>

[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 2398 bytes --]

 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;

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

[Jeff King]

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

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

[Linus Torvalds]

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.

> 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

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

[Jeff King]

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

> 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.

> 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.

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

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

[Linus Torvalds]

Just a ping on this patch..

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

   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

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

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> 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.

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

[Linus Torvalds]

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).

                Linus

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

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> 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.

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

[Linus Torvalds]

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;

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

[Stephen & Linda Smith]

On Thursday, December 15, 2016 5:39:53 PM MST Linus Torvalds wrote:
> 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

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

[Linus Torvalds]

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

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

[Jeff King]

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

> 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/

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

[Jeff King]

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

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

[Junio C Hamano]

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.

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

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

[Johannes Sixt]

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

on your Linux box and make it pass the tests.

Thank you so much in advance.

-- Hannes

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

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> 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???

>
> on your Linux box and make it pass the tests.
>
> Thank you so much in advance.
>
> -- Hannes

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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

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

[Johannes Sixt]

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
>

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

[Johannes Sixt]

Am 20.12.2016 um 19:52 schrieb Johannes Sixt:
> 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

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

[Jeff King]

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

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

[Jacob Keller]

On Tue, Dec 20, 2016 at 7:22 PM, Jeff King <peff@peff.net> wrote:
> 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

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

[Jeff King]

On Tue, Dec 20, 2016 at 11:55:45PM -0800, Jacob Keller wrote:

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

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

[Junio C Hamano]

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.