{"thread":{"id":"50016","subject":"[PATCH] shortlog: pass the mailmap into the revision walker","startedAt":"2018-12-12T16:49:08Z","lastAt":"2018-12-12T19:19:33Z","messageCount":4,"participants":["CB Bailey","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"365179","messageId":"20181212164134.12922-1-cb@hashpling.org","threadId":"50016","inReplyTo":null,"subject":"[PATCH] shortlog: pass the mailmap into the revision walker","fromName":"CB Bailey","fromEmail":"cb@hashpling.org","sentAt":"2018-12-12T16:41:34Z","receivedAt":"2018-12-12T16:49:08Z","isPatch":true,"sender":{"key":"cb@hashpling.org","avatar":null},"body":"From: CB Bailey \n\nshortlog always respects the mailmap in its output. Pass the mailmap\ninto the revision walker to allow the mailmap to be used with revision\nlimiting options such as '--author'.\n\nThis removes some apparently inconsistent behaviors when using\n'--author', such as not finding some or all commits for a given author\nwhich do appear under that author in an unrestricted invocation of\nshortlog or commits being summarized under a different author than the\nspecified author.\n---\n builtin/shortlog.c | 2 ++\n t/t4203-mailmap.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 30 insertions(+)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 88f88e97b2..a6fb00ade8 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -188,6 +188,8 @@ static void get_from_rev(struct rev_info *rev, struct shortlog *log)\n {\n \tstruct commit *commit;\n \n+\trev->mailmap = &log->mailmap;\n+\n \tif (prepare_revision_walk(rev))\n \t\tdie(_(\"revision walk setup failed\"));\n \twhile ((commit = get_revision(rev)) != NULL)\ndiff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh\nindex 43b1522ea2..9bee35b06c 100755\n--- a/t/t4203-mailmap.sh\n+++ b/t/t4203-mailmap.sh\n@@ -383,6 +383,34 @@ test_expect_success 'Shortlog output (complex mapping)' '\n \n '\n \n+test_expect_success 'Shortlog output (complex mapping, filtered)' '\n+\n+\tprintf \" 1\\tA U Thor \\n\" >expect &&\n+\n+\tgit shortlog -es --author=\"A U Thor\" HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tprintf \" 1\\tCTO \\n\" >expect &&\n+\n+\tgit shortlog -es --author=CTO HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tprintf \" 2\\tOther Author \\n\" >expect &&\n+\n+\tgit shortlog -es --author=\"Other Author\" HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tprintf \" 2\\tSanta Claus \\n\" >expect &&\n+\n+\tgit shortlog -es --author=\"Santa Claus\" HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tprintf \" 1\\tSome Dude \\n\" >expect &&\n+\n+\tgit shortlog -es --author=\"Some Dude\" HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n # git log with --pretty format which uses the name and email mailmap placemarkers\n cat >expect <<\\EOF\n Author CTO maps to CTO \n-- \n2.17.0.rc0\n\n"},{"id":"365183","messageId":"20181212171052.13415-1-cb@hashpling.org","threadId":"50016","inReplyTo":"20181212164134.12922-1-cb@hashpling.org","subject":"[PATCH] shortlog: pass the mailmap into the revision walker","fromName":"CB Bailey","fromEmail":"cb@hashpling.org","sentAt":"2018-12-12T17:10:52Z","receivedAt":"2018-12-12T17:10:56Z","isPatch":true,"sender":{"key":"cb@hashpling.org","avatar":null},"body":"From: CB Bailey \n\nshortlog always respects the mailmap in its output. Pass the mailmap\ninto the revision walker to allow the mailmap to be used with revision\nlimiting options such as '--author'.\n\nThis removes some apparently inconsistent behaviors when using\n'--author', such as not finding some or all commits for a given author\nwhich do appear under that author in an unrestricted invocation of\nshortlog or commits being summarized under a different author than the\nspecified author.\n\nSigned-off-by: CB Bailey \n---\n\nResending with omitted s-o-b.\n\n builtin/shortlog.c | 2 ++\n t/t4203-mailmap.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 30 insertions(+)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 88f88e97b2..a6fb00ade8 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -188,6 +188,8 @@ static void get_from_rev(struct rev_info *rev, struct shortlog *log)\n {\n \tstruct commit *commit;\n \n+\trev->mailmap = &log->mailmap;\n+\n \tif (prepare_revision_walk(rev))\n \t\tdie(_(\"revision walk setup failed\"));\n \twhile ((commit = get_revision(rev)) != NULL)\ndiff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh\nindex 43b1522ea2..9bee35b06c 100755\n--- a/t/t4203-mailmap.sh\n+++ b/t/t4203-mailmap.sh\n@@ -383,6 +383,34 @@ test_expect_success 'Shortlog output (complex mapping)' '\n \n '\n \n+test_expect_success 'Shortlog output (complex mapping, filtered)' '\n+\n+\tprintf \" 1\\tA U Thor \\n\" >expect &&\n+\n+\tgit shortlog -es --author=\"A U Thor\" HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tprintf \" 1\\tCTO \\n\" >expect &&\n+\n+\tgit shortlog -es --author=CTO HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tprintf \" 2\\tOther Author \\n\" >expect &&\n+\n+\tgit shortlog -es --author=\"Other Author\" HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tprintf \" 2\\tSanta Claus \\n\" >expect &&\n+\n+\tgit shortlog -es --author=\"Santa Claus\" HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tprintf \" 1\\tSome Dude \\n\" >expect &&\n+\n+\tgit shortlog -es --author=\"Some Dude\" HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n # git log with --pretty format which uses the name and email mailmap placemarkers\n cat >expect <<\\EOF\n Author CTO maps to CTO \n-- \n2.17.0.rc0\n\n"},{"id":"365189","messageId":"87va3ymwai.fsf@evledraar.gmail.com","threadId":"50016","inReplyTo":"20181212171052.13415-1-cb@hashpling.org","subject":"Re: [PATCH] shortlog: pass the mailmap into the revision walker","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-12T18:31:33Z","receivedAt":"2018-12-12T18:31:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Dec 12 2018, CB Bailey wrote:\n\n> From: CB Bailey \n>\n> shortlog always respects the mailmap in its output. Pass the mailmap\n> into the revision walker to allow the mailmap to be used with revision\n> limiting options such as '--author'.\n>\n> This removes some apparently inconsistent behaviors when using\n> '--author', such as not finding some or all commits for a given author\n> which do appear under that author in an unrestricted invocation of\n> shortlog or commits being summarized under a different author than the\n> specified author.\n>\n> Signed-off-by: CB Bailey \n> ---\n>\n> Resending with omitted s-o-b.\n>\n> builtin/shortlog.c | 2 ++\n> t/t4203-mailmap.sh | 28 ++++++++++++++++++++++++++++\n> 2 files changed, 30 insertions(+)\n>\n> diff --git a/builtin/shortlog.c b/builtin/shortlog.c\n> index 88f88e97b2..a6fb00ade8 100644\n> --- a/builtin/shortlog.c\n> +++ b/builtin/shortlog.c\n> @@ -188,6 +188,8 @@ static void get_from_rev(struct rev_info *rev, struct shortlog *log)\n> {\n> \tstruct commit *commit;\n>\n> +\trev->mailmap = &log->mailmap;\n> +\n> \tif (prepare_revision_walk(rev))\n> \t\tdie(_(\"revision walk setup failed\"));\n> \twhile ((commit = get_revision(rev)) != NULL)\n> diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh\n> index 43b1522ea2..9bee35b06c 100755\n> --- a/t/t4203-mailmap.sh\n> +++ b/t/t4203-mailmap.sh\n> @@ -383,6 +383,34 @@ test_expect_success 'Shortlog output (complex mapping)' '\n>\n> '\n>\n> +test_expect_success 'Shortlog output (complex mapping, filtered)' '\n> +\n> +\tprintf \" 1\\tA U Thor \\n\" >expect &&\n> +\n> +\tgit shortlog -es --author=\"A U Thor\" HEAD >actual &&\n> +\ttest_cmp expect actual &&\n> +\n> +\tprintf \" 1\\tCTO \\n\" >expect &&\n> +\n> +\tgit shortlog -es --author=CTO HEAD >actual &&\n> +\ttest_cmp expect actual &&\n> +\n> +\tprintf \" 2\\tOther Author \\n\" >expect &&\n> +\n> +\tgit shortlog -es --author=\"Other Author\" HEAD >actual &&\n> +\ttest_cmp expect actual &&\n> +\n> +\tprintf \" 2\\tSanta Claus \\n\" >expect &&\n> +\n> +\tgit shortlog -es --author=\"Santa Claus\" HEAD >actual &&\n> +\ttest_cmp expect actual &&\n> +\n> +\tprintf \" 1\\tSome Dude \\n\" >expect &&\n> +\n> +\tgit shortlog -es --author=\"Some Dude\" HEAD >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> # git log with --pretty format which uses the name and email mailmap placemarkers\n> cat >expect <<\\EOF\n> Author CTO maps to CTO \n\nMakes sense. Not saying this is how it should be, just an equivalently\nworking patch that I came up with on top while poing at this:\n\n diff --git a/builtin/shortlog.c b/builtin/shortlog.c\n index a6fb00ade8..ad84d70d07 100644\n --- a/builtin/shortlog.c\n +++ b/builtin/shortlog.c\n @@ -188,10 +188,9 @@ static void get_from_rev(struct rev_info *rev, struct shortlog *log)\n {\n \tstruct commit *commit;\n\n -\trev->mailmap = &log->mailmap;\n -\n \tif (prepare_revision_walk(rev))\n \t\tdie(_(\"revision walk setup failed\"));\n +\trev->mailmap = &log->mailmap;\n \twhile ((commit = get_revision(rev)) != NULL)\n \t\tshortlog_add_commit(log, commit);\n }\n diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh\n index 9bee35b06c..74a269052d 100755\n --- a/t/t4203-mailmap.sh\n +++ b/t/t4203-mailmap.sh\n @@ -384,17 +384,6 @@ test_expect_success 'Shortlog output (complex mapping)' '\n '\n\n test_expect_success 'Shortlog output (complex mapping, filtered)' '\n -\n -\tprintf \" 1\\tA U Thor \\n\" >expect &&\n -\n -\tgit shortlog -es --author=\"A U Thor\" HEAD >actual &&\n -\ttest_cmp expect actual &&\n -\n -\tprintf \" 1\\tCTO \\n\" >expect &&\n -\n -\tgit shortlog -es --author=CTO HEAD >actual &&\n -\ttest_cmp expect actual &&\n -\n \tprintf \" 2\\tOther Author \\n\" >expect &&\n\n \tgit shortlog -es --author=\"Other Author\" HEAD >actual &&\n\nI.e. we just need the assignment after prepare_revision_walk() and the\nfirst 2x tests were things that passed before this change.\n\nSo that's not a \"let's squash that on top\" but \"I was poking at this and\nhere's stuff that I fiddled with or surprised me slightly\".\n"},{"id":"365192","messageId":"xmqq5zvyimdb.fsf@gitster-ct.c.googlers.com","threadId":"50016","inReplyTo":"20181212171052.13415-1-cb@hashpling.org","subject":"Re: [PATCH] shortlog: pass the mailmap into the revision walker","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-12T19:19:28Z","receivedAt":"2018-12-12T19:19:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"CB Bailey writes:\n\n> From: CB Bailey \n>\n> shortlog always respects the mailmap in its output. Pass the mailmap\n> into the revision walker to allow the mailmap to be used with revision\n> limiting options such as '--author'.\n\nI am a bit torn on this. \n\nIf an author of interest has entries in the mailmap, with the same\nname but three or more different e-mail addresses, you could do\n\n $ git shortlog --author=junio@kernel.org --author=junio@twinsun.com\n\nto find commits by the author under these two addresses, and exclude\ncommits by the same author made under other addresses. With this\npatch applied, it becomes no longer possible, and you can only look\nfor commits written under all of my identities, or none of them,\nwhich is a minor annoyance.\n\nBut more importantly, the user now needs to know which one is the\n\"canonical\" spelling of the author's ident [*1*]. Asking for\ncommits written by junio@kernel.org will not yield any result with\nthe patch applied, no?\n\nFor the author in the above example, luckily, AUTHOR_NAME is the\nsame and unique, so without your patch, you could still do\n\n $ git shortlog --author=\"Junio C\"\n\nto grab all of them, but there may be authors for whom there is no\ncatch-all substring that would match all of the idents they ever\nused but still unique enough to match only that author, and I do\nagree that the new feature proposed by the patch in question would\nhave uses in such a case.\n\nWhich nudges me to say that there needs a way to ask for the\noriginal behaviour, disabling the rewriting of commit author\nidentity before --author filter kicks in.\n\nThere may also need a way to ask omitting mailmap processing even at\nthe output phase. I do not think it is a particularly interesting\nfeature to be able to ask for my commits under only two of my\nidentities [*2*], but as long as that \"feature\" exists, it also\nshould be possible to see the result of that \"feature\" more clearly\nby not merging them into a single list. \"shortlog --no-use-mailmap\"\nmay be a way to do so, but unlike \"log\", the command currently does\nnot take the \"--[no-]use-mailmap\" option.\n\n\n[Footnote]\n\n*1* Passing the query string given as --author=... through the\n mailmap machinery _might_ be a possible ingredient to solve \"the\n user needs to know the caonical spelling of the author ident\"\n issue this patch has, but I do not think it generally is\n possible (e.g. how would you rewrite --author='.*@kernel.org').\n\n*2* In other words, I think the loss of \"find only my @kernel.org\n and @twinsun.com commits\" is mere annoyance and people can live\n with it. But I think \"you must look for gitster@pobox.com, and\n looking for junio@kernel.org will not find my commits made under\n that identity\" is a show stopper. That is why I use a very weak\n \"may also need\" to this \"optionally not using mailmap at all\"\n feature.\n"}]}