{"thread":{"id":"52095","subject":"[PATCH] pretty: Add \"%aU\"|\"%au\" option to output author's username","startedAt":"2019-10-22T23:29:04Z","lastAt":"2019-10-24T12:44:02Z","messageCount":9,"participants":["Prarit Bhargava","Junio C Hamano","brian m. carlson","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"384639","messageId":"20191022232847.5212-1-prarit@redhat.com","threadId":"52095","inReplyTo":null,"subject":"[PATCH] pretty: Add \"%aU\"|\"%au\" option to output author's username","fromName":"Prarit Bhargava","fromEmail":"prarit@redhat.com","sentAt":"2019-10-22T23:28:47Z","receivedAt":"2019-10-22T23:29:04Z","isPatch":true,"sender":{"key":"prarit@redhat.com","avatar":"https://avatars.githubusercontent.com/u/11943259?v=4"},"body":"In many projects the number of contributors is low enough that users know\neach other and the full email address doesn't need to be displayed.\nDisplaying only the author's username saves a lot of columns on the screen.\nFor example displaying \"prarit\" instead of \"prarit@redhat.com\" saves 11\ncolumns.\n\nAdd a \"%aU\"|\"%au\" option that outputs the author's email username.\n\nAlso add tests for \"%ae\" and \"%an\".\n\nSigned-off-by: Prarit Bhargava <prarit@redhat.com>\n---\n Documentation/pretty-formats.txt |  3 +++\n pretty.c                         |  5 +++++\n t/t4202-log.sh                   | 16 ++++++++++++++++\n 3 files changed, 24 insertions(+)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex b87e2e83e6d0..479a15a8ab12 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -163,6 +163,9 @@ The placeholders are:\n '%ae':: author email\n '%aE':: author email (respecting .mailmap, see linkgit:git-shortlog[1]\n \tor linkgit:git-blame[1])\n+'%au':: author username\n+'%aU':: author username (respecting .mailmap, see linkgit:git-shortlog[1]\n+\tor linkgit:git-blame[1])\n '%ad':: author date (format respects --date= option)\n '%aD':: author date, RFC2822 style\n '%ar':: author date, relative\ndiff --git a/pretty.c b/pretty.c\nindex b32f0369531c..2a5b93022050 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -706,6 +706,11 @@ static size_t format_person_part(struct strbuf *sb, char part,\n \t\tstrbuf_add(sb, mail, maillen);\n \t\treturn placeholder_len;\n \t}\n+\tif (part == 'u' || part == 'U') {\t/* username */\n+\t\tmaillen = strstr(s.mail_begin, \"@\") - s.mail_begin;\n+\t\tstrbuf_add(sb, mail, maillen);\n+\t\treturn placeholder_len;\n+\t}\n \n \tif (!s.date_begin)\n \t\tgoto skip;\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex e803ba402e9e..2fee0c067197 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1729,4 +1729,20 @@ test_expect_success 'log --end-of-options' '\n        test_cmp expect actual\n '\n \n+test_expect_success 'log pretty %an %ae %au' '\n+\tgit checkout -b anaeau &&\n+\ttest_commit anaeau_test anaeau_test_file &&\n+\tgit log --pretty=\"%an\" > actual &&\n+\tgit log --pretty=\"%ae\" >> actual &&\n+\tgit log --pretty=\"%au\" >> actual &&\n+\tgit log > full &&\n+\tname=$(cat full | grep \"^Author: \" | awk -F \"Author: \" \" { print \\$2 } \" | awk -F \" <\" \" { print \\$1 } \") &&\n+\temail=$(cat full | grep \"^Author: \" | awk -F \"<\" \" { print \\$2 } \" | awk -F \">\" \" { print \\$1 } \") &&\n+\tusername=$(cat full | grep \"^Author: \" | awk -F \"<\" \" { print \\$2 } \" | awk -F \">\" \" { print \\$1 } \" | awk -F \"@\" \" { print \\$1 } \" ) &&\n+\techo \"${name}\" > expect &&\n+\techo \"${email}\" >> expect &&\n+\techo \"${username}\" >> expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.21.0\n\n"},{"id":"384647","messageId":"xmqqy2xcjqc8.fsf@gitster-ct.c.googlers.com","threadId":"52095","inReplyTo":"20191022232847.5212-1-prarit@redhat.com","subject":"Re: [PATCH] pretty: Add \"%aU\"|\"%au\" option to output author's username","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-22T23:46:47Z","receivedAt":"2019-10-22T23:46:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Prarit Bhargava <prarit@redhat.com> writes:\n\n> Subject: Re: [PATCH] pretty: Add \"%aU\"|\"%au\" option to output author's username\n\nDowncase \"Add\" (see \"git shortlog --no-merges -100 master\" and\nmimick the project convention).\n\n> Add a \"%aU\"|\"%au\" option that outputs the author's email username.\n\nEven though I personally do not see the use for it, I agree it would\nmake sense to have an option to show the local part only where the\ne-mail address is shown.  \n\nI do not know if u/U is a good mnemonic; it hints too strongly that\nit may come from GIT_{AUTHOR/COMMITTER}_NAME but that is not what\nyou are doing---isn't there a letter that better conveys that this\nis about RFC 2822 local-part (cf. page 16 ieft.org/rfc/rfc2822.txt)?\n\n> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n> index b87e2e83e6d0..479a15a8ab12 100644\n> --- a/Documentation/pretty-formats.txt\n> +++ b/Documentation/pretty-formats.txt\n> @@ -163,6 +163,9 @@ The placeholders are:\n>  '%ae':: author email\n>  '%aE':: author email (respecting .mailmap, see linkgit:git-shortlog[1]\n>  \tor linkgit:git-blame[1])\n> +'%au':: author username\n> +'%aU':: author username (respecting .mailmap, see linkgit:git-shortlog[1]\n> +\tor linkgit:git-blame[1])\n>  '%ad':: author date (format respects --date= option)\n>  '%aD':: author date, RFC2822 style\n>  '%ar':: author date, relative\n\n> diff --git a/pretty.c b/pretty.c\n> index b32f0369531c..2a5b93022050 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -706,6 +706,11 @@ static size_t format_person_part(struct strbuf *sb, char part,\n>  \t\tstrbuf_add(sb, mail, maillen);\n>  \t\treturn placeholder_len;\n>  \t}\n> +\tif (part == 'u' || part == 'U') {\t/* username */\n> +\t\tmaillen = strstr(s.mail_begin, \"@\") - s.mail_begin;\n> +\t\tstrbuf_add(sb, mail, maillen);\n> +\t\treturn placeholder_len;\n> +\t}\n\nI think users get %eu and %eU for free with this change, which should\nbe documented.\n"},{"id":"384648","messageId":"20191022234822.f4ixqcpfytimiscn@camp.crustytoothpaste.net","threadId":"52095","inReplyTo":"20191022232847.5212-1-prarit@redhat.com","subject":"Re: [PATCH] pretty: Add \"%aU\"|\"%au\" option to output author's username","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-10-22T23:48:44Z","receivedAt":"2019-10-22T23:48:54Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2019-10-22 at 23:28:47, Prarit Bhargava wrote:\n> In many projects the number of contributors is low enough that users know\n> each other and the full email address doesn't need to be displayed.\n> Displaying only the author's username saves a lot of columns on the screen.\n> For example displaying \"prarit\" instead of \"prarit@redhat.com\" saves 11\n> columns.\n> \n> Add a \"%aU\"|\"%au\" option that outputs the author's email username.\n\nI have no position on whether or not this is a useful change.  I don't\nthink I'll end up using it, but I can imagine cases where it is useful,\nsuch as certain corporate environments.  It would be interesting to see\nwhat others think.\n\n> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n> index b87e2e83e6d0..479a15a8ab12 100644\n> --- a/Documentation/pretty-formats.txt\n> +++ b/Documentation/pretty-formats.txt\n> @@ -163,6 +163,9 @@ The placeholders are:\n>  '%ae':: author email\n>  '%aE':: author email (respecting .mailmap, see linkgit:git-shortlog[1]\n>  \tor linkgit:git-blame[1])\n> +'%au':: author username\n> +'%aU':: author username (respecting .mailmap, see linkgit:git-shortlog[1]\n> +\tor linkgit:git-blame[1])\n\nI think we need to actually document what \"username\" means here.  I\nexpect you'll have people think that this magically means their\n$HOSTING_PLATFORM username, which it is not.\n\n> diff --git a/pretty.c b/pretty.c\n> index b32f0369531c..2a5b93022050 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -706,6 +706,11 @@ static size_t format_person_part(struct strbuf *sb, char part,\n>  \t\tstrbuf_add(sb, mail, maillen);\n>  \t\treturn placeholder_len;\n>  \t}\n> +\tif (part == 'u' || part == 'U') {\t/* username */\n> +\t\tmaillen = strstr(s.mail_begin, \"@\") - s.mail_begin;\n> +\t\tstrbuf_add(sb, mail, maillen);\n> +\t\treturn placeholder_len;\n> +\t}\n\nThis branch doesn't appear to do anything different for the mailmap and\nnon-mailmap cases.  Perhaps adding an additional test that demonstrates\nthe difference would be a good idea.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"384675","messageId":"20191023050206.GA29779@sigill.intra.peff.net","threadId":"52095","inReplyTo":"20191022232847.5212-1-prarit@redhat.com","subject":"Re: [PATCH] pretty: Add \"%aU\"|\"%au\" option to output author's username","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-23T05:02:06Z","receivedAt":"2019-10-23T05:02:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 22, 2019 at 07:28:47PM -0400, Prarit Bhargava wrote:\n\n> In many projects the number of contributors is low enough that users know\n> each other and the full email address doesn't need to be displayed.\n> Displaying only the author's username saves a lot of columns on the screen.\n> For example displaying \"prarit\" instead of \"prarit@redhat.com\" saves 11\n> columns.\n> \n> Add a \"%aU\"|\"%au\" option that outputs the author's email username.\n\nLike others, this seems potentially useful even if I probably wouldn't\nuse it myself. Another more complicated way to think of it would be to\ngive a list of domains to omit (so if 90% of the committers are\n@redhat.com, we can skip that, but the one-off contributor from another\ndomain gets their fully qualified name.\n\nBut that's a lot more complicated. I don't mind doing the easy thing\nnow, and even if we later grew the more complicated thing, I wouldn't be\nsad to still have this easy one as an option.\n\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -706,6 +706,11 @@ static size_t format_person_part(struct strbuf *sb, char part,\n>  \t\tstrbuf_add(sb, mail, maillen);\n>  \t\treturn placeholder_len;\n>  \t}\n> +\tif (part == 'u' || part == 'U') {\t/* username */\n> +\t\tmaillen = strstr(s.mail_begin, \"@\") - s.mail_begin;\n> +\t\tstrbuf_add(sb, mail, maillen);\n> +\t\treturn placeholder_len;\n> +\t}\n\nWhat happens if the email doesn't have an \"@\"? I think you'd either end\nup printing a bunch of extra cruft (because you're not limiting the\nsearch for \"@\" to the boundaries from split_ident_line) or you'd\ncrash (if there's no \"@\" at all, and you'd get a huge maillen).\n\nThere's also no need to use the slower strstr() when looking for a\nsingle character. So perhaps:\n\n  const char *at = memchr(mail, '@', maillen);\n  if (at)\n          maillen = at - mail;\n  strbuf_add(sb, mail, maillen);\n\n> +test_expect_success 'log pretty %an %ae %au' '\n\nAs others noted, this could cover %aU, too (which is broken; you need to\nhandle 'U' alongside 'E' and 'N' earlier in format_person_part()).\n\n> +\tgit checkout -b anaeau &&\n> +\ttest_commit anaeau_test anaeau_test_file &&\n> +\tgit log --pretty=\"%an\" > actual &&\n> +\tgit log --pretty=\"%ae\" >> actual &&\n> +\tgit log --pretty=\"%au\" >> actual &&\n\nMaybe:\n\n  git log --pretty=\"%an %ae %au\"\n\nor\n\n  git log --pretty=\"%an%n%ae%n%au\"\n\nwhich is shorter and runs more efficiently?\n\n> +\tgit log > full &&\n> +\tname=$(cat full | grep \"^Author: \" | awk -F \"Author: \" \" { print \\$2 } \" | awk -F \" <\" \" { print \\$1 } \") &&\n> +\temail=$(cat full | grep \"^Author: \" | awk -F \"<\" \" { print \\$2 } \" | awk -F \">\" \" { print \\$1 } \") &&\n> +\tusername=$(cat full | grep \"^Author: \" | awk -F \"<\" \" { print \\$2 } \" | awk -F \">\" \" { print \\$1 } \" | awk -F \"@\" \" { print \\$1 } \" ) &&\n> +\techo \"${name}\" > expect &&\n> +\techo \"${email}\" >> expect &&\n> +\techo \"${username}\" >> expect &&\n\nThese values come from our hard-coded test setup, so it would be more\nreadable to just expect those:\n\n  {\n\techo \"$GIT_AUTHOR_NAME\" &&\n\techo \"$GIT_AUTHOR_EMAIL\" &&\n\techo \"$GIT_AUTHOR_EMAIL\" | sed \"s/@.*//\"\n  } >expect\n\nFor the last one, also I wouldn't be upset to see test-lib.sh do\nsomething like:\n\n  TEST_AUTHOR_USERNAME=author\n  TEST_AUTHOR_DOMAIN=example.com\n  GIT_AUTHOR_NAME=$TEST_AUTHOR_USERNAME@$TEST_AUTHOR_DOMAIN\n\nto let tests like this pick out the individual values if they want.\n\n-Peff\n"},{"id":"384706","messageId":"987b7743-df0c-dbcf-6336-23f2903c061a@redhat.com","threadId":"52095","inReplyTo":"xmqqy2xcjqc8.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] pretty: Add \"%aU\"|\"%au\" option to output author's username","fromName":"Prarit Bhargava","fromEmail":"prarit@redhat.com","sentAt":"2019-10-23T15:03:50Z","receivedAt":"2019-10-23T15:03:58Z","isPatch":true,"sender":{"key":"prarit@redhat.com","avatar":"https://avatars.githubusercontent.com/u/11943259?v=4"},"body":"\n\nOn 10/22/19 7:46 PM, Junio C Hamano wrote:\n> Prarit Bhargava <prarit@redhat.com> writes:\n> \n>> Subject: Re: [PATCH] pretty: Add \"%aU\"|\"%au\" option to output author's username\n> \n> Downcase \"Add\" (see \"git shortlog --no-merges -100 master\" and\n> mimick the project convention).\n\nI'll fix that.\n\n> \n>> Add a \"%aU\"|\"%au\" option that outputs the author's email username.\n> \n> Even though I personally do not see the use for it, I agree it would\n> make sense to have an option to show the local part only where the\n> e-mail address is shown.  \n> \n> I do not know if u/U is a good mnemonic; it hints too strongly that\n> it may come from GIT_{AUTHOR/COMMITTER}_NAME but that is not what\n> you are doing---isn't there a letter that better conveys that this\n> is about RFC 2822 local-part (cf. page 16 ieft.org/rfc/rfc2822.txt)?\n\nI'll go with \"l\" and \"L\" for local-part as defined on page 16.  I will also\ninclude a comment in braces that says (the portion of the email address\npreceding the '@' symbol)\".  Admittedly that doesn't convey the meaning of the\nmailbox concept of the email address it does tell a user what is going to be output.\n\n> \n>> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n>> index b87e2e83e6d0..479a15a8ab12 100644\n>> --- a/Documentation/pretty-formats.txt\n>> +++ b/Documentation/pretty-formats.txt\n>> @@ -163,6 +163,9 @@ The placeholders are:\n>>  '%ae':: author email\n>>  '%aE':: author email (respecting .mailmap, see linkgit:git-shortlog[1]\n>>  \tor linkgit:git-blame[1])\n>> +'%au':: author username\n>> +'%aU':: author username (respecting .mailmap, see linkgit:git-shortlog[1]\n>> +\tor linkgit:git-blame[1])\n>>  '%ad':: author date (format respects --date= option)\n>>  '%aD':: author date, RFC2822 style\n>>  '%ar':: author date, relative\n> \n>> diff --git a/pretty.c b/pretty.c\n>> index b32f0369531c..2a5b93022050 100644\n>> --- a/pretty.c\n>> +++ b/pretty.c\n>> @@ -706,6 +706,11 @@ static size_t format_person_part(struct strbuf *sb, char part,\n>>  \t\tstrbuf_add(sb, mail, maillen);\n>>  \t\treturn placeholder_len;\n>>  \t}\n>> +\tif (part == 'u' || part == 'U') {\t/* username */\n>> +\t\tmaillen = strstr(s.mail_begin, \"@\") - s.mail_begin;\n>> +\t\tstrbuf_add(sb, mail, maillen);\n>> +\t\treturn placeholder_len;\n>> +\t}\n> \n> I think users get %eu and %eU for free with this change, which should\n> be documented.\n> \n\nDid you mean %cu and %cU?  Or am I missing something with \"%e\"?\n\nP.\n\n"},{"id":"384708","messageId":"83b6ad72-9077-a4ce-09b7-84ce6aabf84f@redhat.com","threadId":"52095","inReplyTo":"20191022234822.f4ixqcpfytimiscn@camp.crustytoothpaste.net","subject":"Re: [PATCH] pretty: Add \"%aU\"|\"%au\" option to output author's username","fromName":"Prarit Bhargava","fromEmail":"prarit@redhat.com","sentAt":"2019-10-23T15:08:14Z","receivedAt":"2019-10-23T15:08:22Z","isPatch":true,"sender":{"key":"prarit@redhat.com","avatar":"https://avatars.githubusercontent.com/u/11943259?v=4"},"body":"\n\nOn 10/22/19 7:48 PM, brian m. carlson wrote:\n> On 2019-10-22 at 23:28:47, Prarit Bhargava wrote:\n>> In many projects the number of contributors is low enough that users know\n>> each other and the full email address doesn't need to be displayed.\n>> Displaying only the author's username saves a lot of columns on the screen.\n>> For example displaying \"prarit\" instead of \"prarit@redhat.com\" saves 11\n>> columns.\n>>\n>> Add a \"%aU\"|\"%au\" option that outputs the author's email username.\n> \n> I have no position on whether or not this is a useful change.  I don't\n> think I'll end up using it, but I can imagine cases where it is useful,\n> such as certain corporate environments.  It would be interesting to see\n> what others think.\n> \n>> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n>> index b87e2e83e6d0..479a15a8ab12 100644\n>> --- a/Documentation/pretty-formats.txt\n>> +++ b/Documentation/pretty-formats.txt\n>> @@ -163,6 +163,9 @@ The placeholders are:\n>>  '%ae':: author email\n>>  '%aE':: author email (respecting .mailmap, see linkgit:git-shortlog[1]\n>>  \tor linkgit:git-blame[1])\n>> +'%au':: author username\n>> +'%aU':: author username (respecting .mailmap, see linkgit:git-shortlog[1]\n>> +\tor linkgit:git-blame[1])\n> \n> I think we need to actually document what \"username\" means here.  I\n> expect you'll have people think that this magically means their\n> $HOSTING_PLATFORM username, which it is not.\n> \n\nBased on Junio's input, I'm going to change the option to \"al\" and \"aL\" where L\nmeans \"local-part\" as defined by the rfc2822.txt specification, and include a\ncomment that says \"(the portion of the email address preceding the '@' symbol)\".\n Admittedly that doesn't convey the meaning of the mailbox concept of the email\naddress it does tell a user what is going to be output.\n\n\n>> diff --git a/pretty.c b/pretty.c\n>> index b32f0369531c..2a5b93022050 100644\n>> --- a/pretty.c\n>> +++ b/pretty.c\n>> @@ -706,6 +706,11 @@ static size_t format_person_part(struct strbuf *sb, char part,\n>>  \t\tstrbuf_add(sb, mail, maillen);\n>>  \t\treturn placeholder_len;\n>>  \t}\n>> +\tif (part == 'u' || part == 'U') {\t/* username */\n>> +\t\tmaillen = strstr(s.mail_begin, \"@\") - s.mail_begin;\n>> +\t\tstrbuf_add(sb, mail, maillen);\n>> +\t\treturn placeholder_len;\n>> +\t}\n> \n> This branch doesn't appear to do anything different for the mailmap and\n> non-mailmap cases.  Perhaps adding an additional test that demonstrates\n> the difference would be a good idea.\n> \n\nThanks for the suggestion.  I'll look into it for v2.\n\nP.\n\n"},{"id":"384709","messageId":"8d57a33b-0f0e-4e4b-1cd3-069ee5d7520e@redhat.com","threadId":"52095","inReplyTo":"20191023050206.GA29779@sigill.intra.peff.net","subject":"Re: [PATCH] pretty: Add \"%aU\"|\"%au\" option to output author's username","fromName":"Prarit Bhargava","fromEmail":"prarit@redhat.com","sentAt":"2019-10-23T15:58:17Z","receivedAt":"2019-10-23T15:58:25Z","isPatch":true,"sender":{"key":"prarit@redhat.com","avatar":"https://avatars.githubusercontent.com/u/11943259?v=4"},"body":"\n\nOn 10/23/19 1:02 AM, Jeff King wrote:\n> On Tue, Oct 22, 2019 at 07:28:47PM -0400, Prarit Bhargava wrote:\n> \n>> In many projects the number of contributors is low enough that users know\n>> each other and the full email address doesn't need to be displayed.\n>> Displaying only the author's username saves a lot of columns on the screen.\n>> For example displaying \"prarit\" instead of \"prarit@redhat.com\" saves 11\n>> columns.\n>>\n>> Add a \"%aU\"|\"%au\" option that outputs the author's email username.\n> \n> Like others, this seems potentially useful even if I probably wouldn't\n> use it myself. Another more complicated way to think of it would be to\n> give a list of domains to omit (so if 90% of the committers are\n> @redhat.com, we can skip that, but the one-off contributor from another\n> domain gets their fully qualified name.>\n> But that's a lot more complicated. I don't mind doing the easy thing\n> now, and even if we later grew the more complicated thing, I wouldn't be\n> sad to still have this easy one as an option.\n\nFWIW, I went through the exact same thought process as you did and came to the\nsame conclusion.\n\n> \n>> --- a/pretty.c\n>> +++ b/pretty.c\n>> @@ -706,6 +706,11 @@ static size_t format_person_part(struct strbuf *sb, char part,\n>>  \t\tstrbuf_add(sb, mail, maillen);\n>>  \t\treturn placeholder_len;\n>>  \t}\n>> +\tif (part == 'u' || part == 'U') {\t/* username */\n>> +\t\tmaillen = strstr(s.mail_begin, \"@\") - s.mail_begin;\n>> +\t\tstrbuf_add(sb, mail, maillen);\n>> +\t\treturn placeholder_len;\n>> +\t}\n> \n> What happens if the email doesn't have an \"@\"? I think you'd either end\n> up printing a bunch of extra cruft (because you're not limiting the\n> search for \"@\" to the boundaries from split_ident_line) or you'd\n> crash (if there's no \"@\" at all, and you'd get a huge maillen).\n> \n> There's also no need to use the slower strstr() when looking for a\n> single character. So perhaps:\n> \n>   const char *at = memchr(mail, '@', maillen);\n>   if (at)\n>           maillen = at - mail;\n>   strbuf_add(sb, mail, maillen);\n\nTBH I had assumed that the email address was RFC2822 compliant.  Thanks for the\ncode.  I've incorporated it into v2.\n\n> \n>> +test_expect_success 'log pretty %an %ae %au' '\n> \n> As others noted, this could cover %aU, too (which is broken; you need to\n> handle 'U' alongside 'E' and 'N' earlier in format_person_part()).\n\nWhups.  Thanks for the pointer.  Also fixed in v2.\n\n> \n>> +\tgit checkout -b anaeau &&\n>> +\ttest_commit anaeau_test anaeau_test_file &&\n>> +\tgit log --pretty=\"%an\" > actual &&\n>> +\tgit log --pretty=\"%ae\" >> actual &&\n>> +\tgit log --pretty=\"%au\" >> actual &&\n> \n> Maybe:\n> \n>   git log --pretty=\"%an %ae %au\"\n> \n> or\n> \n>   git log --pretty=\"%an%n%ae%n%au\"\n> \n> which is shorter and runs more efficiently?\n> \n>> +\tgit log > full &&\n>> +\tname=$(cat full | grep \"^Author: \" | awk -F \"Author: \" \" { print \\$2 } \" | awk -F \" <\" \" { print \\$1 } \") &&\n>> +\temail=$(cat full | grep \"^Author: \" | awk -F \"<\" \" { print \\$2 } \" | awk -F \">\" \" { print \\$1 } \") &&\n>> +\tusername=$(cat full | grep \"^Author: \" | awk -F \"<\" \" { print \\$2 } \" | awk -F \">\" \" { print \\$1 } \" | awk -F \"@\" \" { print \\$1 } \" ) &&\n>> +\techo \"${name}\" > expect &&\n>> +\techo \"${email}\" >> expect &&\n>> +\techo \"${username}\" >> expect &&\n> \n> These values come from our hard-coded test setup, so it would be more\n> readable to just expect those:\n> \n>   {\n> \techo \"$GIT_AUTHOR_NAME\" &&\n> \techo \"$GIT_AUTHOR_EMAIL\" &&\n> \techo \"$GIT_AUTHOR_EMAIL\" | sed \"s/@.*//\"\n>   } >expect\n\nAdded to v2 (along with Brian's suggestion to test %aE, %aN, and %aL).\n\n> \n> For the last one, also I wouldn't be upset to see test-lib.sh do\n> something like:\n> \n>   TEST_AUTHOR_USERNAME=author\n>   TEST_AUTHOR_DOMAIN=example.com\n>   GIT_AUTHOR_NAME=$TEST_AUTHOR_USERNAME@$TEST_AUTHOR_DOMAIN\n\nI like this suggestion a lot.  I'm incorporating into v2 as well as similar\nchanges for the COMMITTER fields.\n\nP.\n\n> \n> to let tests like this pick out the individual values if they want.\n> \n> -Peff\n> \n\n"},{"id":"384741","messageId":"xmqqa79qho4v.fsf@gitster-ct.c.googlers.com","threadId":"52095","inReplyTo":"20191022234822.f4ixqcpfytimiscn@camp.crustytoothpaste.net","subject":"Re: [PATCH] pretty: Add \"%aU\"|\"%au\" option to output author's username","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-24T02:29:36Z","receivedAt":"2019-10-24T02:29:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n>> +\tif (part == 'u' || part == 'U') {\t/* username */\n>> +\t\tmaillen = strstr(s.mail_begin, \"@\") - s.mail_begin;\n>> +\t\tstrbuf_add(sb, mail, maillen);\n>> +\t\treturn placeholder_len;\n>> +\t}\n>\n> This branch doesn't appear to do anything different for the mailmap and\n> non-mailmap cases.  Perhaps adding an additional test that demonstrates\n> the difference would be a good idea.\n\nYes, and the bug that would be exposed is the lack of call to\nmailmap.\n\nPerhaps along this line (I may have off-by-one or two tho)?\n\n pretty.c | 20 ++++++++++++++++----\n 1 file changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex e4ed14effe..4b76d022c6 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -696,15 +696,27 @@ static size_t format_person_part(struct strbuf *sb, char part,\n \tmail = s.mail_begin;\n \tmaillen = s.mail_end - s.mail_begin;\n \n-\tif (part == 'N' || part == 'E') /* mailmap lookup */\n+\tif (part == 'N' || part == 'E' || part == 'L') /* mailmap lookup */\n \t\tmailmap_name(&mail, &maillen, &name, &namelen);\n-\tif (part == 'n' || part == 'N') {\t/* name */\n+\n+\tswitch (part) {\n+\tcase 'n': case 'N':\n \t\tstrbuf_add(sb, name, namelen);\n \t\treturn placeholder_len;\n-\t}\n-\tif (part == 'e' || part == 'E') {\t/* email */\n+\tcase 'l': case 'L':\n+\t\t{\n+\t\t\tconst char *at = memchr(mail, '@', maillen);\n+\t\t\tif (at) {\n+\t\t\t\tmaillen -= at - mail + 1;\n+\t\t\t\tmail = at + 1;\n+\t\t\t}\n+\t\t}\n+\t\t/* fall through */\n+\tcase 'e': case 'E': \n \t\tstrbuf_add(sb, mail, maillen);\n \t\treturn placeholder_len;\n+\tdefault:\n+\t\tbreak;\n \t}\n \n \tif (!s.date_begin)\n"},{"id":"384768","messageId":"4bc612f9-0b9a-1bab-9746-15ff25bd6f5f@redhat.com","threadId":"52095","inReplyTo":"xmqqa79qho4v.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] pretty: Add \"%aU\"|\"%au\" option to output author's username","fromName":"Prarit Bhargava","fromEmail":"prarit@redhat.com","sentAt":"2019-10-24T12:43:54Z","receivedAt":"2019-10-24T12:44:02Z","isPatch":true,"sender":{"key":"prarit@redhat.com","avatar":"https://avatars.githubusercontent.com/u/11943259?v=4"},"body":"\n\nOn 10/23/19 10:29 PM, Junio C Hamano wrote:\n> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n> \n>>> +\tif (part == 'u' || part == 'U') {\t/* username */\n>>> +\t\tmaillen = strstr(s.mail_begin, \"@\") - s.mail_begin;\n>>> +\t\tstrbuf_add(sb, mail, maillen);\n>>> +\t\treturn placeholder_len;\n>>> +\t}\n>>\n>> This branch doesn't appear to do anything different for the mailmap and\n>> non-mailmap cases.  Perhaps adding an additional test that demonstrates\n>> the difference would be a good idea.\n> \n> Yes, and the bug that would be exposed is the lack of call to\n> mailmap.\n> \n> Perhaps along this line (I may have off-by-one or two tho)?\n> \n>  pretty.c | 20 ++++++++++++++++----\n>  1 file changed, 16 insertions(+), 4 deletions(-)\n> \n> diff --git a/pretty.c b/pretty.c\n> index e4ed14effe..4b76d022c6 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -696,15 +696,27 @@ static size_t format_person_part(struct strbuf *sb, char part,\n>  \tmail = s.mail_begin;\n>  \tmaillen = s.mail_end - s.mail_begin;\n>  \n> -\tif (part == 'N' || part == 'E') /* mailmap lookup */\n> +\tif (part == 'N' || part == 'E' || part == 'L') /* mailmap lookup */\n>  \t\tmailmap_name(&mail, &maillen, &name, &namelen);\n> -\tif (part == 'n' || part == 'N') {\t/* name */\n> +\n> +\tswitch (part) {\n> +\tcase 'n': case 'N':\n>  \t\tstrbuf_add(sb, name, namelen);\n>  \t\treturn placeholder_len;\n> -\t}\n> -\tif (part == 'e' || part == 'E') {\t/* email */\n> +\tcase 'l': case 'L':\n> +\t\t{\n> +\t\t\tconst char *at = memchr(mail, '@', maillen);\n> +\t\t\tif (at) {\n> +\t\t\t\tmaillen -= at - mail + 1;\n> +\t\t\t\tmail = at + 1;\n\n^^^  If the mail is 'prarit@redhat.com' the output of these lines is\n\n\tmaillen = maillen - at - mail - 1\n\n\twhich is (16 - [6] - 1) = 9.\n\t\n\tand mail = at + 1 points to \"redhat.com\"\n\nThis is the reverse of what we want.  IMO I suggest (sorry for the\ncut-and-paste) which keeps the code easy to read.\n\ndiff --git a/pretty.c b/pretty.c\nindex b32f0369531c..93eb6e837071 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -696,7 +696,7 @@ static size_t format_person_part(struct strbuf *sb, char par\nt,\n        mail = s.mail_begin;\n        maillen = s.mail_end - s.mail_begin;\n\n-       if (part == 'N' || part == 'E') /* mailmap lookup */\n+       if (part == 'N' || part == 'E' || part == 'L') /* mailmap lookup */\n                mailmap_name(&mail, &maillen, &name, &namelen);\n        if (part == 'n' || part == 'N') {       /* name */\n                strbuf_add(sb, name, namelen);\n@@ -706,6 +706,13 @@ static size_t format_person_part(struct strbuf *sb, char pa\nrt,\n                strbuf_add(sb, mail, maillen);\n                return placeholder_len;\n        }\n+       if (part == 'l' || part == 'L') {       /* local-part */\n+               const char *at = memchr(mail, '@', maillen);\n+               if (at)\n+                       maillen = at - mail;\n+               strbuf_add(sb, mail, maillen);\n+               return placeholder_len;\n+       }\n\n        if (!s.date_begin)\n\nLet me submit a v2 with all the suggested changes so that you can review my work\nso far.  We can continue the discussion there.\n\nP.\n\n> +\t\t\t}\n\n> +\t\t}\n> +\t\t/* fall through */\n> +\tcase 'e': case 'E': \n>  \t\tstrbuf_add(sb, mail, maillen);\n>  \t\treturn placeholder_len;\n> +\tdefault:\n> +\t\tbreak;\n>  \t}\n>  \n>  \tif (!s.date_begin)\n> \n\n"}]}