{"thread":{"id":"46726","subject":"\"git shortlog -sn --follow -- \" counts all commits to entire repo","startedAt":"2017-09-07T18:13:39Z","lastAt":"2017-09-10T07:36:43Z","messageCount":8,"participants":["Валентин","Stefan Beller","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"327724","messageId":"CAJrPYn8_n1JD3hL1LSAVz2Khs=dZRVaZ-gpru5AV+ottv5WfAw@mail.gmail.com","threadId":"46726","inReplyTo":null,"subject":"\"git shortlog -sn --follow -- \" counts all commits to entire repo","fromName":"Валентин","fromEmail":"valiko.ua@gmail.com","sentAt":"2017-09-07T18:13:27Z","receivedAt":"2017-09-07T18:13:39Z","isPatch":false,"sender":{"key":"valiko.ua@gmail.com","avatar":null},"body":"Hi,\n\nI'll be short as shortlog :)\n\n\"git shortlog -sn -- \"\ncounts all commits to the specified path, as expected.\n\n\"git shortlog -sn --follow -- \"\ncounts all commits to the entire repo, which looks like a bug.\n\n\"--follow\" switch is not listed on\nhttps://git-scm.com/docs/git-shortlog so maybe it's not supported. In\nthis case I would expect error message.\n\nTried the following versions:\n\"git version 2.14.1.windows.1\" on Windows 7\n\"git version 2.7.4\" on Ubuntu 16.04\n\nThanks,\nValentine\n"},{"id":"327728","messageId":"CAGZ79kZAeCERKn6he2LzAj97BXEJ5U+Fy4sorAPNmW_XaCS9Vg@mail.gmail.com","threadId":"46726","inReplyTo":"CAJrPYn8_n1JD3hL1LSAVz2Khs=dZRVaZ-gpru5AV+ottv5WfAw@mail.gmail.com","subject":"Re: \"git shortlog -sn --follow -- \" counts all commits to entire repo","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-07T19:30:01Z","receivedAt":"2017-09-07T19:30:12Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 7, 2017 at 11:13 AM, Валентин wrote:\n> Hi,\n>\n> I'll be short as shortlog :)\n>\n> \"git shortlog -sn -- \"\n> counts all commits to the specified path, as expected.\n>\n> \"git shortlog -sn --follow -- \"\n> counts all commits to the entire repo, which looks like a bug.\n>\n> \"--follow\" switch is not listed on\n> https://git-scm.com/docs/git-shortlog so maybe it's not supported. In\n> this case I would expect error message.\n>\n> Tried the following versions:\n> \"git version 2.14.1.windows.1\" on Windows 7\n> \"git version 2.7.4\" on Ubuntu 16.04\n\nThe shortlog takes most (all?) options that git-log\ndoes, e.g. in git.git:\n\n $ git shortlog -sne --author=Peter\n\n 74 Peter Krefting \n 43 H. Peter Anvin \n 23 Peter Eriksen \n 7 Peter Hagervall \n 6 Peter Collingbourne \n 4 Peter Baumann \n 3 Peter Oberndorfer \n 3 Peter Valdemar Mørch \n 2 Peter Colberg \n 2 Peter Eisentraut \n 2 Peter Harris \n 2 Peter van der Does \n 1 Peter Hutterer \n 1 Peter Law \n 1 Peter Stuge \n 1 Peter Wu \n 1 Peter van Zetten \n\nMaybe we'd to state in the man page explicitly that\nshortlog is part of the log family hence taking all\nlog related options.\n\nThanks,\nStefan\n"},{"id":"327750","messageId":"20170908051015.ybq4egdrddecl4se@sigill.intra.peff.net","threadId":"46726","inReplyTo":"CAGZ79kZAeCERKn6he2LzAj97BXEJ5U+Fy4sorAPNmW_XaCS9Vg@mail.gmail.com","subject":"Re: \"git shortlog -sn --follow -- \" counts all commits to entire repo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-08T05:10:15Z","receivedAt":"2017-09-08T05:10:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 07, 2017 at 12:30:01PM -0700, Stefan Beller wrote:\n\n> > \"--follow\" switch is not listed on\n> > https://git-scm.com/docs/git-shortlog so maybe it's not supported. In\n> > this case I would expect error message.\n> >\n> > Tried the following versions:\n> > \"git version 2.14.1.windows.1\" on Windows 7\n> > \"git version 2.7.4\" on Ubuntu 16.04\n> \n> The shortlog takes most (all?) options that git-log\n> does, e.g. in git.git:\n\nThat's definitely the intent, but I think --follow is just buggy here.\nWe don't seem to trigger a diff, which is where \"git log\" does the\nfollow check (because of the way it's bolted onto the diff, and not the\nactual pathspec-pruning mechanism).\n\nSomething like the patch below seems to work, but there are a lot of\nopen questions. And it would probably want to do something to prevent\nnonsense like \"shortlog -p\" from showing actual diffs.\n\nI suspect a better solution might involve actually building on\nlog-tree.c to do the traversal (since this internal traversal is\nsupposed to be equivalent to \"git log | git shortlog\").\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 43c4799ea9..31274a92f5 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -175,8 +175,31 @@ static void get_from_rev(struct rev_info *rev, struct shortlog *log)\n \n \tif (prepare_revision_walk(rev))\n \t\tdie(_(\"revision walk setup failed\"));\n-\twhile ((commit = get_revision(rev)) != NULL)\n-\t\tshortlog_add_commit(log, commit);\n+\twhile ((commit = get_revision(rev)) != NULL) {\n+\t\tint show_commit;\n+\n+\t\t/* trigger a diff to give --follow a chance to kick in */\n+\t\tif (!commit->parents) {\n+\t\t\t/* should diff against empty tree or respect --root? */\n+\t\t\tshow_commit = 1;\n+\t\t} else if (commit->parents->next) {\n+\t\t\t/* how to handle merge commits? */\n+\t\t\tshow_commit = 1;\n+\t\t} else {\n+\t\t\tstruct commit *parent = commit->parents->item;\n+\n+\t\t\tparse_commit_or_die(parent);\n+\t\t\tparse_commit_or_die(commit);\n+\t\t\tdiff_tree_oid(&parent->tree->object.oid,\n+\t\t\t\t &commit->tree->object.oid,\n+\t\t\t\t \"\", &rev->diffopt);\n+\t\t\tshow_commit = !diff_queue_is_empty();\n+\t\t\tdiff_flush(&rev->diffopt);\n+\t\t}\n+\n+\t\tif (show_commit)\n+\t\t\tshortlog_add_commit(log, commit);\n+\t}\n }\n \n static int parse_uint(char const **arg, int comma, int defval)\n"},{"id":"327751","messageId":"xmqqo9qlzodi.fsf@gitster.mtv.corp.google.com","threadId":"46726","inReplyTo":"20170908051015.ybq4egdrddecl4se@sigill.intra.peff.net","subject":"Re: \"git shortlog -sn --follow -- \" counts all commits to entire repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-08T06:38:17Z","receivedAt":"2017-09-08T06:38:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> That's definitely the intent, but I think --follow is just buggy here.\n> We don't seem to trigger a diff, which is where \"git log\" does the\n> follow check (because of the way it's bolted onto the diff, and not the\n> actual pathspec-pruning mechanism).\n\nYeah, at the same time, the usual caveat about \"--follow\" that it\nwould not work in general in a history with forks and merges due to\nits keeping track of a single pathspec that is globally swapped out.\n\n> Something like the patch below seems to work, but there are a lot of\n> open questions. And it would probably want to do something to prevent\n> nonsense like \"shortlog -p\" from showing actual diffs.\n>\n> I suspect a better solution might involve actually building on\n> log-tree.c to do the traversal (since this internal traversal is\n> supposed to be equivalent to \"git log | git shortlog\").\n\nProbably. That approach would also have an added benefit that when\n\"--follow\" is fixed to keep track of which path it is following per\ntraversal for \"git log\", the result from \"git shortlog --follow\"\nwould automatically become correct, I guess?\n\nThanks.\n"},{"id":"327760","messageId":"20170908074910.reoyb47navgsg6kt@sigill.intra.peff.net","threadId":"46726","inReplyTo":"xmqqo9qlzodi.fsf@gitster.mtv.corp.google.com","subject":"Re: \"git shortlog -sn --follow -- \" counts all commits to entire repo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-08T07:49:10Z","receivedAt":"2017-09-08T07:49:20Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 08, 2017 at 03:38:17PM +0900, Junio C Hamano wrote:\n\n> > I suspect a better solution might involve actually building on\n> > log-tree.c to do the traversal (since this internal traversal is\n> > supposed to be equivalent to \"git log | git shortlog\").\n> \n> Probably. That approach would also have an added benefit that when\n> \"--follow\" is fixed to keep track of which path it is following per\n> traversal for \"git log\", the result from \"git shortlog --follow\"\n> would automatically become correct, I guess?\n\nYeah. It depends on exactly how such a fix is made. I think one\nimprovement would be to actually bump --follow handling into the\nlimit_list() stage, so that we properly handle history simplification\nover followed paths. In which case get_revision() would just never\nreturn the uninteresting commits, and the current shortlog code would\nJust Work.\n\nThat said, I don't think we can go wrong by making shortlog's traversal\nmore like log's. Any changes we make to --follow will be aimed at and\ntested with git-log, so the more code they share the more likely it is\nthat shortlog won't bitrot.\n\n-Peff\n"},{"id":"327786","messageId":"xmqqbmmlytv3.fsf@gitster.mtv.corp.google.com","threadId":"46726","inReplyTo":"20170908074910.reoyb47navgsg6kt@sigill.intra.peff.net","subject":"Re: \"git shortlog -sn --follow -- \" counts all commits to entire repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-08T17:37:20Z","receivedAt":"2017-09-08T17:37:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah. It depends on exactly how such a fix is made. I think one\n> improvement would be to actually bump --follow handling into the\n> limit_list() stage, so that we properly handle history simplification\n> over followed paths. In which case get_revision() would just never\n> return the uninteresting commits, and the current shortlog code would\n> Just Work.\n>\n> That said, I don't think we can go wrong by making shortlog's traversal\n> more like log's. Any changes we make to --follow will be aimed at and\n> tested with git-log, so the more code they share the more likely it is\n> that shortlog won't bitrot.\n\nBoth true. \n\nUsing log-tree traversal machinery instead of just get_revision()\nwould probably mean we would slow it down quite a bit unless we are\ncareful, but at the same time, things like \"git shortlog -G\"\nwould suddenly start working, so this is not just helping the\n\"--follow\" hack.\n\n"},{"id":"327807","messageId":"20170909065247.wob3nuxvox4f65gg@sigill.intra.peff.net","threadId":"46726","inReplyTo":"xmqqbmmlytv3.fsf@gitster.mtv.corp.google.com","subject":"Re: \"git shortlog -sn --follow -- \" counts all commits to entire repo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-09T06:52:47Z","receivedAt":"2017-09-09T06:52:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 09, 2017 at 02:37:20AM +0900, Junio C Hamano wrote:\n\n> > That said, I don't think we can go wrong by making shortlog's traversal\n> > more like log's. Any changes we make to --follow will be aimed at and\n> > tested with git-log, so the more code they share the more likely it is\n> > that shortlog won't bitrot.\n> \n> Both true. \n> \n> Using log-tree traversal machinery instead of just get_revision()\n> would probably mean we would slow it down quite a bit unless we are\n> careful, but at the same time, things like \"git shortlog -G\"\n> would suddenly start working, so this is not just helping the\n> \"--follow\" hack.\n\nI didn't notice that, but I'm not surprised that there are more options\nthat shortlog doesn't quite work with.\n\nI don't plan on working on this myself any time soon, so maybe it's a\ngood #leftoverbits candidate (though it's perhaps a little more involved\nthan some).\n\n-Peff\n"},{"id":"327824","messageId":"xmqqh8wbxaww.fsf@gitster.mtv.corp.google.com","threadId":"46726","inReplyTo":"20170909065247.wob3nuxvox4f65gg@sigill.intra.peff.net","subject":"Re: \"git shortlog -sn --follow -- \" counts all commits to entire repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-10T07:36:31Z","receivedAt":"2017-09-10T07:36:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sat, Sep 09, 2017 at 02:37:20AM +0900, Junio C Hamano wrote:\n>\n>> Using log-tree traversal machinery instead of just get_revision()\n>> would probably mean we would slow it down quite a bit unless we are\n>> careful, but at the same time, things like \"git shortlog -G\"\n>> would suddenly start working, so this is not just helping the\n>> \"--follow\" hack.\n>\n> I didn't notice that, but I'm not surprised that there are more options\n> that shortlog doesn't quite work with.\n>\n> I don't plan on working on this myself any time soon, so maybe it's a\n> good #leftoverbits candidate (though it's perhaps a little more involved\n> than some).\n\nI agree that I do not mind seeing those who haven't really touched\nthe inner core of the system to try this change, so marking this\ndiscussion with #leftoverbits may be a good idea, but I have this\ngut feeling that \"a little more involved\" might be a bit of an\nunderstatement ;-)\n\nBut still I think it is a very good suggestion to allow log-tree to\nfilter things more so that \"shortlog $args\" can become a more\nfaithful imitation of \"log $args | shortlog\".\n\nThanks.\n"}]}