"git shortlog -sn --follow -- <path>" counts all commits to entire repo

"git shortlog -sn --follow -- <path>" counts all commits to entire repo

[Валентин]

Hi,

I'll be short as shortlog :)

"git shortlog -sn -- <path>"
counts all commits to the specified path, as expected.

"git shortlog -sn --follow -- <path>"
counts all commits to the entire repo, which looks like a bug.

"--follow" switch is not listed on
https://git-scm.com/docs/git-shortlog so maybe it's not supported. In
this case I would expect error message.

Tried the following versions:
"git version 2.14.1.windows.1" on Windows 7
"git version 2.7.4" on Ubuntu 16.04

Thanks,
Valentine

Re: "git shortlog -sn --follow -- <path>" counts all commits to entire repo

[Stefan Beller]

On Thu, Sep 7, 2017 at 11:13 AM, Валентин <valiko.ua@gmail.com> wrote:
> Hi,
>
> I'll be short as shortlog :)
>
> "git shortlog -sn -- <path>"
> counts all commits to the specified path, as expected.
>
> "git shortlog -sn --follow -- <path>"
> counts all commits to the entire repo, which looks like a bug.
>
> "--follow" switch is not listed on
> https://git-scm.com/docs/git-shortlog so maybe it's not supported. In
> this case I would expect error message.
>
> Tried the following versions:
> "git version 2.14.1.windows.1" on Windows 7
> "git version 2.7.4" on Ubuntu 16.04

The shortlog takes most (all?) options that git-log
does, e.g. in git.git:

    $ git shortlog -sne --author=Peter

    74  Peter Krefting <peter@softwolves.pp.se>
    43  H. Peter Anvin <hpa@zytor.com>
    23  Peter Eriksen <s022018@student.dtu.dk>
     7  Peter Hagervall <hager@cs.umu.se>
     6  Peter Collingbourne <peter@pcc.me.uk>
     4  Peter Baumann <waste.manager@gmx.de>
     3  Peter Oberndorfer <kumbayo84@arcor.de>
     3  Peter Valdemar Mørch <peter@morch.com>
     2  Peter Colberg <peter@colberg.org>
     2  Peter Eisentraut <peter@eisentraut.org>
     2  Peter Harris <git@peter.is-a-geek.org>
     2  Peter van der Does <peter@avirtualhome.com>
     1  Peter Hutterer <peter.hutterer@who-t.net>
     1  Peter Law <PeterJCLaw@gmail.com>
     1  Peter Stuge <peter@stuge.se>
     1  Peter Wu <lekensteyn@gmail.com>
     1  Peter van Zetten <peter.van.zetten@cgi.com>

Maybe we'd to state in the man page explicitly that
shortlog is part of the log family hence taking all
log related options.

Thanks,
Stefan

Re: "git shortlog -sn --follow -- <path>" counts all commits to entire repo

[Jeff King]

On Thu, Sep 07, 2017 at 12:30:01PM -0700, Stefan Beller wrote:

> > "--follow" switch is not listed on
> > https://git-scm.com/docs/git-shortlog so maybe it's not supported. In
> > this case I would expect error message.
> >
> > Tried the following versions:
> > "git version 2.14.1.windows.1" on Windows 7
> > "git version 2.7.4" on Ubuntu 16.04
> 
> The shortlog takes most (all?) options that git-log
> does, e.g. in git.git:

That's definitely the intent, but I think --follow is just buggy here.
We don't seem to trigger a diff, which is where "git log" does the
follow check (because of the way it's bolted onto the diff, and not the
actual pathspec-pruning mechanism).

Something like the patch below seems to work, but there are a lot of
open questions. And it would probably want to do something to prevent
nonsense like "shortlog -p" from showing actual diffs.

I suspect a better solution might involve actually building on
log-tree.c to do the traversal (since this internal traversal is
supposed to be equivalent to "git log | git shortlog").

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 43c4799ea9..31274a92f5 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -175,8 +175,31 @@ static void get_from_rev(struct rev_info *rev, struct shortlog *log)
 
 	if (prepare_revision_walk(rev))
 		die(_("revision walk setup failed"));
-	while ((commit = get_revision(rev)) != NULL)
-		shortlog_add_commit(log, commit);
+	while ((commit = get_revision(rev)) != NULL) {
+		int show_commit;
+
+		/* trigger a diff to give --follow a chance to kick in */
+		if (!commit->parents) {
+			/* should diff against empty tree or respect --root? */
+			show_commit = 1;
+		} else if (commit->parents->next) {
+			/* how to handle merge commits? */
+			show_commit = 1;
+		} else {
+			struct commit *parent = commit->parents->item;
+
+			parse_commit_or_die(parent);
+			parse_commit_or_die(commit);
+			diff_tree_oid(&parent->tree->object.oid,
+				      &commit->tree->object.oid,
+				      "", &rev->diffopt);
+			show_commit = !diff_queue_is_empty();
+			diff_flush(&rev->diffopt);
+		}
+
+		if (show_commit)
+			shortlog_add_commit(log, commit);
+	}
 }
 
 static int parse_uint(char const **arg, int comma, int defval)

Re: "git shortlog -sn --follow -- <path>" counts all commits to entire repo

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> That's definitely the intent, but I think --follow is just buggy here.
> We don't seem to trigger a diff, which is where "git log" does the
> follow check (because of the way it's bolted onto the diff, and not the
> actual pathspec-pruning mechanism).

Yeah, at the same time, the usual caveat about "--follow" that it
would not work in general in a history with forks and merges due to
its keeping track of a single pathspec that is globally swapped out.

> Something like the patch below seems to work, but there are a lot of
> open questions. And it would probably want to do something to prevent
> nonsense like "shortlog -p" from showing actual diffs.
>
> I suspect a better solution might involve actually building on
> log-tree.c to do the traversal (since this internal traversal is
> supposed to be equivalent to "git log | git shortlog").

Probably.  That approach would also have an added benefit that when
"--follow" is fixed to keep track of which path it is following per
traversal for "git log", the result from "git shortlog --follow"
would automatically become correct, I guess?

Thanks.

Re: "git shortlog -sn --follow -- <path>" counts all commits to entire repo

[Jeff King]

On Fri, Sep 08, 2017 at 03:38:17PM +0900, Junio C Hamano wrote:

> > I suspect a better solution might involve actually building on
> > log-tree.c to do the traversal (since this internal traversal is
> > supposed to be equivalent to "git log | git shortlog").
> 
> Probably.  That approach would also have an added benefit that when
> "--follow" is fixed to keep track of which path it is following per
> traversal for "git log", the result from "git shortlog --follow"
> would automatically become correct, I guess?

Yeah. It depends on exactly how such a fix is made. I think one
improvement would be to actually bump --follow handling into the
limit_list() stage, so that we properly handle history simplification
over followed paths. In which case get_revision() would just never
return the uninteresting commits, and the current shortlog code would
Just Work.

That said, I don't think we can go wrong by making shortlog's traversal
more like log's. Any changes we make to --follow will be aimed at and
tested with git-log, so the more code they share the more likely it is
that shortlog won't bitrot.

-Peff

Re: "git shortlog -sn --follow -- <path>" counts all commits to entire repo

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yeah. It depends on exactly how such a fix is made. I think one
> improvement would be to actually bump --follow handling into the
> limit_list() stage, so that we properly handle history simplification
> over followed paths. In which case get_revision() would just never
> return the uninteresting commits, and the current shortlog code would
> Just Work.
>
> That said, I don't think we can go wrong by making shortlog's traversal
> more like log's. Any changes we make to --follow will be aimed at and
> tested with git-log, so the more code they share the more likely it is
> that shortlog won't bitrot.

Both true.  

Using log-tree traversal machinery instead of just get_revision()
would probably mean we would slow it down quite a bit unless we are
careful, but at the same time, things like "git shortlog -G<string>"
would suddenly start working, so this is not just helping the
"--follow" hack.

Re: "git shortlog -sn --follow -- <path>" counts all commits to entire repo

[Jeff King]

On Sat, Sep 09, 2017 at 02:37:20AM +0900, Junio C Hamano wrote:

> > That said, I don't think we can go wrong by making shortlog's traversal
> > more like log's. Any changes we make to --follow will be aimed at and
> > tested with git-log, so the more code they share the more likely it is
> > that shortlog won't bitrot.
> 
> Both true.  
> 
> Using log-tree traversal machinery instead of just get_revision()
> would probably mean we would slow it down quite a bit unless we are
> careful, but at the same time, things like "git shortlog -G<string>"
> would suddenly start working, so this is not just helping the
> "--follow" hack.

I didn't notice that, but I'm not surprised that there are more options
that shortlog doesn't quite work with.

I don't plan on working on this myself any time soon, so maybe it's a
good #leftoverbits candidate (though it's perhaps a little more involved
than some).

-Peff

Re: "git shortlog -sn --follow -- <path>" counts all commits to entire repo

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Sep 09, 2017 at 02:37:20AM +0900, Junio C Hamano wrote:
>
>> Using log-tree traversal machinery instead of just get_revision()
>> would probably mean we would slow it down quite a bit unless we are
>> careful, but at the same time, things like "git shortlog -G<string>"
>> would suddenly start working, so this is not just helping the
>> "--follow" hack.
>
> I didn't notice that, but I'm not surprised that there are more options
> that shortlog doesn't quite work with.
>
> I don't plan on working on this myself any time soon, so maybe it's a
> good #leftoverbits candidate (though it's perhaps a little more involved
> than some).

I agree that I do not mind seeing those who haven't really touched
the inner core of the system to try this change, so marking this
discussion with #leftoverbits may be a good idea, but I have this
gut feeling that "a little more involved" might be a bit of an
understatement ;-)

But still I think it is a very good suggestion to allow log-tree to
filter things more so that "shortlog $args" can become a more
faithful imitation of "log $args | shortlog".

Thanks.