threads / discuss / 46726
Subject: "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
Stefan BellerRe: "git shortlog -sn --follow -- <path>" counts all commits to entire repo
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
Jeff KingRe: "git shortlog -sn --follow -- <path>" counts all commits to entire repo
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)
Junio C HamanoRe: "git shortlog -sn --follow -- <path>" counts all commits to entire repo
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.
Jeff KingRe: "git shortlog -sn --follow -- <path>" counts all commits to entire repo
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
Junio C HamanoRe: "git shortlog -sn --follow -- <path>" counts all commits to entire repo
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.
Jeff KingRe: "git shortlog -sn --follow -- <path>" counts all commits to entire repo
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
Junio C HamanoRe: "git shortlog -sn --follow -- <path>" counts all commits to entire repo
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.