{"thread":{"id":"52109","subject":"[PATCH v2] pretty: add \"%aL\"|\"%al|%cL|%cl\" option to output local-part of email addresses","startedAt":"2019-10-24T12:53:50Z","lastAt":"2019-10-25T10:56:49Z","messageCount":7,"participants":["Prarit Bhargava","Jeff King","SZEDER Gábor","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"384769","messageId":"20191024125332.29958-1-prarit@redhat.com","threadId":"52109","inReplyTo":null,"subject":"[PATCH v2] pretty: add \"%aL\"|\"%al|%cL|%cl\" option to output local-part of email addresses","fromName":"Prarit Bhargava","fromEmail":"prarit@redhat.com","sentAt":"2019-10-24T12:53:32Z","receivedAt":"2019-10-24T12:53:50Z","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 \"%aL\"|\"%al|%cL|%cl\" option that output the local-part of an email\naddress.\n\nAlso add tests for \"%ae\",\"%an\", \"%ce\", and \"%cn\".\n\nChanges in v2:\n- Changed option to 'L' based on https://www.ietf.org/rfc/rfc2822.txt\n  definition of 'local-part' of email addresses.\n- added additional information to documentation for %cL and %cl\n- added mailmap output test\n- modified code to use mailmap output for \"L\" option\n- modified code to check if email address field has '@' symbol\n- modified tests based on input from Peff\n\nSigned-off-by: Prarit Bhargava <prarit@redhat.com>\nCc: Junio C Hamano <gitster@pobox.com>\nCc: brian m. carlson <sandals@crustytoothpaste.net>\nCc: Jeff King <peff@peff.net>\n---\n Documentation/pretty-formats.txt | 10 ++++++++++\n pretty.c                         |  9 ++++++++-\n t/t4202-log.sh                   | 30 ++++++++++++++++++++++++++++++\n t/test-lib.sh                    | 10 ++++++++--\n 4 files changed, 56 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex b87e2e83e6d0..9a1f900f114a 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -163,6 +163,11 @@ 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+'%al':: author local-part (the portion of the email address preceding the '@'\n+\tsymbol)\n+'%aL':: author local-part (the portion of the email address preceding the '@'\n+\tsymbol, respecting .mailmap, see linkgit:git-shortlog[1] or\n+\tlinkgit:git-blame[1])\n '%ad':: author date (format respects --date= option)\n '%aD':: author date, RFC2822 style\n '%ar':: author date, relative\n@@ -175,6 +180,11 @@ The placeholders are:\n '%ce':: committer email\n '%cE':: committer email (respecting .mailmap, see\n \tlinkgit:git-shortlog[1] or linkgit:git-blame[1])\n+'%cl':: author local-part (the portion of the email address preceding the '@'\n+\tsymbol)\n+'%cL':: author local-part (the portion of the email address preceding the '@'\n+\tsymbol, respecting .mailmap, see linkgit:git-shortlog[1] or\n+\tlinkgit:git-blame[1])\n '%cd':: committer date (format respects --date= option)\n '%cD':: committer date, RFC2822 style\n '%cr':: committer date, relative\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 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 \t\tstrbuf_add(sb, name, namelen);\n@@ -706,6 +706,13 @@ 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 == 'l' || part == 'L') {\t/* local-part */\n+\t\tconst char *at = memchr(mail, '@', maillen);\n+\t\tif (at)\n+\t\t\tmaillen = at - mail;\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..fa6ecf3588b7 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1729,4 +1729,34 @@ test_expect_success 'log --end-of-options' '\n        test_cmp expect actual\n '\n \n+test_expect_success 'log pretty %an %ae %al %aN %aE %aL' '\n+\tgit checkout -b anaeal &&\n+\ttest_commit anaeal_test anaeal_test_file &&\n+\tgit log --pretty=\"%an%n%ae%n%al%n%aN%n%aE%n%aL\" -1 > actual &&\n+\t{\n+\t\techo \"${GIT_AUTHOR_NAME}\" &&\n+\t\techo \"${GIT_AUTHOR_EMAIL}\" &&\n+\t\techo \"${TEST_AUTHOR_LOCALNAME}\"\n+\t\techo \"${GIT_AUTHOR_NAME}\" &&\n+\t\techo \"${GIT_AUTHOR_EMAIL}\" &&\n+\t\techo \"${TEST_AUTHOR_LOCALNAME}\"\n+\t} > expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'log pretty %cn %ce %cl %cN %cE %cL' '\n+\tgit checkout -b cncecl &&\n+\ttest_commit cncecl_test cncecl_test_file &&\n+\tgit log --pretty=\"%cn%n%ce%n%cl%n%cN%n%cE%n%cL\" -1 > actual &&\n+\t{\n+\t\techo \"${GIT_COMMITTER_NAME}\" &&\n+\t\techo \"${GIT_COMMITTER_EMAIL}\" &&\n+\t\techo \"${TEST_COMMITTER_LOCALNAME}\"\n+\t\techo \"${GIT_COMMITTER_NAME}\" &&\n+\t\techo \"${GIT_COMMITTER_EMAIL}\" &&\n+\t\techo \"${TEST_COMMITTER_LOCALNAME}\"\n+\t} > expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex e06fa02a0eec..5ef0ad8c1a2a 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -404,14 +404,20 @@ unset VISUAL EMAIL LANGUAGE COLUMNS $(\"$PERL_PATH\" -e '\n unset XDG_CACHE_HOME\n unset XDG_CONFIG_HOME\n unset GITPERLLIB\n-GIT_AUTHOR_EMAIL=author@example.com\n+TEST_AUTHOR_LOCALNAME=author\n+TEST_AUTHOR_DOMAIN=example.com\n+GIT_AUTHOR_EMAIL=${TEST_AUTHOR_LOCALNAME}@${TEST_AUTHOR_DOMAIN}\n GIT_AUTHOR_NAME='A U Thor'\n-GIT_COMMITTER_EMAIL=committer@example.com\n+TEST_COMMITTER_LOCALNAME=committer\n+TEST_COMMITTER_DOMAIN=example.com\n+GIT_COMMITTER_EMAIL=${TEST_COMMITTER_LOCALNAME}@${TEST_COMMITTER_DOMAIN}\n GIT_COMMITTER_NAME='C O Mitter'\n GIT_MERGE_VERBOSITY=5\n GIT_MERGE_AUTOEDIT=no\n export GIT_MERGE_VERBOSITY GIT_MERGE_AUTOEDIT\n+export TEST_AUTHOR_LOCALNAME TEST_AUTHOR_DOMAIN\n export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME\n+export TEST_COMMITTER_LOCALNAME TEST_COMMITTER_DOMAIN\n export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME\n export EDITOR\n \n-- \n2.21.0\n\n"},{"id":"384775","messageId":"20191024161610.GA8448@sigill.intra.peff.net","threadId":"52109","inReplyTo":"20191024125332.29958-1-prarit@redhat.com","subject":"Re: [PATCH v2] pretty: add \"%aL\"|\"%al|%cL|%cl\" option to output local-part of email addresses","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-24T16:16:11Z","receivedAt":"2019-10-24T16:16:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 24, 2019 at 08:53:32AM -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 \"%aL\"|\"%al|%cL|%cl\" option that output the local-part of an email\n> address.\n> \n> Also add tests for \"%ae\",\"%an\", \"%ce\", and \"%cn\".\n\nThanks, this is looking better, but I think there are still a few minor\nbits to address.\n\n> Changes in v2:\n> - Changed option to 'L' based on https://www.ietf.org/rfc/rfc2822.txt\n>   definition of 'local-part' of email addresses.\n> - added additional information to documentation for %cL and %cl\n> - added mailmap output test\n> - modified code to use mailmap output for \"L\" option\n> - modified code to check if email address field has '@' symbol\n> - modified tests based on input from Peff\n\nThis change list is very welcome, but it should generally go after the\n\"---\", so that it's not part of the commit message (i.e., it is for\npeople reading this email and reviewing now, but people reading \"git\nlog\" later would not know or care about v1).\n\n> Signed-off-by: Prarit Bhargava <prarit@redhat.com>\n> Cc: Junio C Hamano <gitster@pobox.com>\n> Cc: brian m. carlson <sandals@crustytoothpaste.net>\n> Cc: Jeff King <peff@peff.net>\n\nLikewise we do not generally use \"cc:\" trailers in this project.\n\n> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n> index b87e2e83e6d0..9a1f900f114a 100644\n> --- a/Documentation/pretty-formats.txt\n> +++ b/Documentation/pretty-formats.txt\n> @@ -163,6 +163,11 @@ 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> +'%al':: author local-part (the portion of the email address preceding the '@'\n> +\tsymbol)\n\nI'm not sure if it is worth saying something like \"preceding the @\nsymbol, or the whole address if no @ symbol\". It's a pretty rare case,\nI'd think, and it does clutter up the wording. So just a thought.\n\n> +'%aL':: author local-part (the portion of the email address preceding the '@'\n> +\tsymbol, respecting .mailmap, see linkgit:git-shortlog[1] or\n> +\tlinkgit:git-blame[1])\n\nThis description gets pretty long. I wonder if we can simplify by\nreferring to earlier formats, which would also make clear to the user\nthe relationship between the formats. Perhaps:\n\n  '%aL':: author local-part (see '%al'), respecting .mailmap (see '%aE')\n\nAnd ditto for %cL.\n\n> diff --git a/pretty.c b/pretty.c\n> index 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 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\nI think this is sufficient to fix the mailmap issue. Good.\n\n> @@ -706,6 +706,13 @@ 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 == 'l' || part == 'L') {\t/* local-part */\n> +\t\tconst char *at = memchr(mail, '@', maillen);\n> +\t\tif (at)\n> +\t\t\tmaillen = at - mail;\n> +\t\tstrbuf_add(sb, mail, maillen);\n> +\t\treturn placeholder_len;\n> +\t}\n\nAnd I think this does the counting correctly (mail[maillen] is the \"@\",\nso we wouldn't include it; good).\n\n> +test_expect_success 'log pretty %an %ae %al %aN %aE %aL' '\n> +\tgit checkout -b anaeal &&\n> +\ttest_commit anaeal_test anaeal_test_file &&\n\nNot a big deal, but we could simplify this a bit, I think. There's no\nneed to make a new branch. And there's no need to specify the filename\nto test_commit. IMHO it's worth being as simple as possible in the\ntests, because then a reader doesn't have to think about whether those\ndetails are important or not.\n\n> +\tgit log --pretty=\"%an%n%ae%n%al%n%aN%n%aE%n%aL\" -1 > actual &&\n\nI didn't think about this before, but...surely we're testing %an, etc\nalready?\n\nIndeed, it looks like t6006 already covers that. Maybe you should be\nadding to that test? I note that it just hardcodes \"author@example.com\"\nin the expectation. I'd be OK with either following the lead there, or\ndoing a separate preparatory patch to use $GIT_AUTHOR_EMAIL, etc.\n\n> +\t{\n> +\t\techo \"${GIT_AUTHOR_NAME}\" &&\n> +\t\techo \"${GIT_AUTHOR_EMAIL}\" &&\n> +\t\techo \"${TEST_AUTHOR_LOCALNAME}\"\n> +\t\techo \"${GIT_AUTHOR_NAME}\" &&\n> +\t\techo \"${GIT_AUTHOR_EMAIL}\" &&\n> +\t\techo \"${TEST_AUTHOR_LOCALNAME}\"\n> +\t} > expect &&\n\nThe expectation for %aE is the same as for %ae. So we're not really\ntesting that we actually applied the mailmap. It looks like t4203 has\nsome tests for %aE; you'd probably want to check %aL there.\n\n> +test_expect_success 'log pretty %cn %ce %cl %cN %cE %cL' '\n\nLikewise, both of the spots I mentioned cover the committer formats,\ntoo.\n\n> +TEST_AUTHOR_LOCALNAME=author\n> +TEST_AUTHOR_DOMAIN=example.com\n> +GIT_AUTHOR_EMAIL=${TEST_AUTHOR_LOCALNAME}@${TEST_AUTHOR_DOMAIN}\n\nIf you follow the lead of t6006 and just hard-code these, then this hunk\ncan go away. But if you do keep it, note that...\n\n>  export GIT_MERGE_VERBOSITY GIT_MERGE_AUTOEDIT\n> +export TEST_AUTHOR_LOCALNAME TEST_AUTHOR_DOMAIN\n>  export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME\n> +export TEST_COMMITTER_LOCALNAME TEST_COMMITTER_DOMAIN\n\nThese lines are unnecessary. We have to export GIT_AUTHOR_EMAIL, etc, so\nthat the child git-commit process can read it. But there's no need to do\nso for TEST_*, which are meant to be used by the test scripts\nthemselves.\n\n-Peff\n"},{"id":"384788","messageId":"28711c54-0635-41e1-1c3f-1fc2751c8c4e@redhat.com","threadId":"52109","inReplyTo":"20191024161610.GA8448@sigill.intra.peff.net","subject":"Re: [PATCH v2] pretty: add \"%aL\"|\"%al|%cL|%cl\" option to output local-part of email addresses","fromName":"Prarit Bhargava","fromEmail":"prarit@redhat.com","sentAt":"2019-10-24T20:14:39Z","receivedAt":"2019-10-24T20:14:48Z","isPatch":true,"sender":{"key":"prarit@redhat.com","avatar":"https://avatars.githubusercontent.com/u/11943259?v=4"},"body":"\n\nOn 10/24/19 12:16 PM, Jeff King wrote:\n> On Thu, Oct 24, 2019 at 08:53:32AM -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 \"%aL\"|\"%al|%cL|%cl\" option that output the local-part of an email\n>> address.\n>>\n>> Also add tests for \"%ae\",\"%an\", \"%ce\", and \"%cn\".\n>\n> Thanks, this is looking better, but I think there are still a few minor\n> bits to address.\n>\n\nThanks :)\n\n>> Changes in v2:\n>> - Changed option to 'L' based on https://www.ietf.org/rfc/rfc2822.txt\n>>   definition of 'local-part' of email addresses.\n>> - added additional information to documentation for %cL and %cl\n>> - added mailmap output test\n>> - modified code to use mailmap output for \"L\" option\n>> - modified code to check if email address field has '@' symbol\n>> - modified tests based on input from Peff\n>\n> This change list is very welcome, but it should generally go after the\n> \"---\", so that it's not part of the commit message (i.e., it is for\n> people reading this email and reviewing now, but people reading \"git\n> log\" later would not know or care about v1).\n>\n>> Signed-off-by: Prarit Bhargava <prarit@redhat.com>\n>> Cc: Junio C Hamano <gitster@pobox.com>\n>> Cc: brian m. carlson <sandals@crustytoothpaste.net>\n>> Cc: Jeff King <peff@peff.net>\n>\n> Likewise we do not generally use \"cc:\" trailers in this project.\n>\n>> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n>> index b87e2e83e6d0..9a1f900f114a 100644\n>> --- a/Documentation/pretty-formats.txt\n>> +++ b/Documentation/pretty-formats.txt\n>> @@ -163,6 +163,11 @@ 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>> +'%al':: author local-part (the portion of the email address preceding the '@'\n>> +\tsymbol)\n>\n> I'm not sure if it is worth saying something like \"preceding the @\n> symbol, or the whole address if no @ symbol\". It's a pretty rare case,\n> I'd think, and it does clutter up the wording. So just a thought.\n>\n\nAfter adding the text I've decided to keep it the same.  As you pointed out it\nended up being too long.\n\n>> +'%aL':: author local-part (the portion of the email address preceding the '@'\n>> +\tsymbol, respecting .mailmap, see linkgit:git-shortlog[1] or\n>> +\tlinkgit:git-blame[1])\n>\n> This description gets pretty long. I wonder if we can simplify by\n> referring to earlier formats, which would also make clear to the user\n> the relationship between the formats. Perhaps:\n>\n>   '%aL':: author local-part (see '%al'), respecting .mailmap (see '%aE')\n>\n> And ditto for %cL.\n\nI have added this for v3.\n\n<snip>\n\n> I didn't think about this before, but...surely we're testing %an, etc\n> already?\n>\n> Indeed, it looks like t6006 already covers that. Maybe you should be\n> adding to that test? I note that it just hardcodes \"author@example.com\"\n> in the expectation. I'd be OK with either following the lead there, or\n> doing a separate preparatory patch to use $GIT_AUTHOR_EMAIL, etc.\n>\n\nI've done separate preparatory patches for both t6006 and t4203 for v3.\n\n>> +\t{\n>> +\t\techo \"${GIT_AUTHOR_NAME}\" &&\n>> +\t\techo \"${GIT_AUTHOR_EMAIL}\" &&\n>> +\t\techo \"${TEST_AUTHOR_LOCALNAME}\"\n>> +\t\techo \"${GIT_AUTHOR_NAME}\" &&\n>> +\t\techo \"${GIT_AUTHOR_EMAIL}\" &&\n>> +\t\techo \"${TEST_AUTHOR_LOCALNAME}\"\n>> +\t} > expect &&\n>\n> The expectation for %aE is the same as for %ae. So we're not really\n> testing that we actually applied the mailmap. It looks like t4203 has\n> some tests for %aE; you'd probably want to check %aL there.\n>\n>> +test_expect_success 'log pretty %cn %ce %cl %cN %cE %cL' '\n>\n> Likewise, both of the spots I mentioned cover the committer formats,\n> too.\n>\n>> +TEST_AUTHOR_LOCALNAME=author\n>> +TEST_AUTHOR_DOMAIN=example.com\n>> +GIT_AUTHOR_EMAIL=${TEST_AUTHOR_LOCALNAME}@${TEST_AUTHOR_DOMAIN}\n>\n> If you follow the lead of t6006 and just hard-code these, then this hunk\n> can go away. But if you do keep it, note that...\n>\n\nI implemented the suggested tests in both test files in v3.\n\n>>  export GIT_MERGE_VERBOSITY GIT_MERGE_AUTOEDIT\n>> +export TEST_AUTHOR_LOCALNAME TEST_AUTHOR_DOMAIN\n>>  export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME\n>> +export TEST_COMMITTER_LOCALNAME TEST_COMMITTER_DOMAIN\n>\n> These lines are unnecessary. We have to export GIT_AUTHOR_EMAIL, etc, so\n> that the child git-commit process can read it. But there's no need to do\n> so for TEST_*, which are meant to be used by the test scripts\n> themselves.\n>\n\nRemoved in v3.  I will post after running the test suite.\n\nThanks,\n\nP.\n\n"},{"id":"384789","messageId":"20191024202947.GN4348@szeder.dev","threadId":"52109","inReplyTo":"20191024125332.29958-1-prarit@redhat.com","subject":"Re: [PATCH v2] pretty: add \"%aL\"|\"%al|%cL|%cl\" option to output local-part of email addresses","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-10-24T20:29:47Z","receivedAt":"2019-10-24T20:29:53Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nJust a couple of test nits:\n\nOn Thu, Oct 24, 2019 at 08:53:32AM -0400, Prarit Bhargava wrote:\n> +test_expect_success 'log pretty %an %ae %al %aN %aE %aL' '\n> +\tgit checkout -b anaeal &&\n> +\ttest_commit anaeal_test anaeal_test_file &&\n> +\tgit log --pretty=\"%an%n%ae%n%al%n%aN%n%aE%n%aL\" -1 > actual &&\n\nStyle: no space between redirection and filename, i.e. >actual\n\n> +\t{\n> +\t\techo \"${GIT_AUTHOR_NAME}\" &&\n> +\t\techo \"${GIT_AUTHOR_EMAIL}\" &&\n> +\t\techo \"${TEST_AUTHOR_LOCALNAME}\"\n\nBroken &&-chain (though with just a bunch of echos it won't really\nmake much of a difference)\n\n> +\t\techo \"${GIT_AUTHOR_NAME}\" &&\n> +\t\techo \"${GIT_AUTHOR_EMAIL}\" &&\n> +\t\techo \"${TEST_AUTHOR_LOCALNAME}\"\n\nAll these variables stand on their own, so the curly braces around\nthem are unnecessary.\n\n> +\t} > expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'log pretty %cn %ce %cl %cN %cE %cL' '\n> +\tgit checkout -b cncecl &&\n> +\ttest_commit cncecl_test cncecl_test_file &&\n> +\tgit log --pretty=\"%cn%n%ce%n%cl%n%cN%n%cE%n%cL\" -1 > actual &&\n> +\t{\n> +\t\techo \"${GIT_COMMITTER_NAME}\" &&\n> +\t\techo \"${GIT_COMMITTER_EMAIL}\" &&\n> +\t\techo \"${TEST_COMMITTER_LOCALNAME}\"\n> +\t\techo \"${GIT_COMMITTER_NAME}\" &&\n> +\t\techo \"${GIT_COMMITTER_EMAIL}\" &&\n> +\t\techo \"${TEST_COMMITTER_LOCALNAME}\"\n> +\t} > expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n"},{"id":"384803","messageId":"5d9b8cbe-489e-b3eb-873c-4aee2e2015d0@redhat.com","threadId":"52109","inReplyTo":"20191024202947.GN4348@szeder.dev","subject":"Re: [PATCH v2] pretty: add \"%aL\"|\"%al|%cL|%cl\" option to output local-part of email addresses","fromName":"Prarit Bhargava","fromEmail":"prarit@redhat.com","sentAt":"2019-10-24T23:05:00Z","receivedAt":"2019-10-24T23:05:07Z","isPatch":true,"sender":{"key":"prarit@redhat.com","avatar":"https://avatars.githubusercontent.com/u/11943259?v=4"},"body":"\n\nOn 10/24/19 4:29 PM, SZEDER Gábor wrote:\n> \n> Just a couple of test nits:\n> \n> On Thu, Oct 24, 2019 at 08:53:32AM -0400, Prarit Bhargava wrote:\n>> +test_expect_success 'log pretty %an %ae %al %aN %aE %aL' '\n>> +\tgit checkout -b anaeal &&\n>> +\ttest_commit anaeal_test anaeal_test_file &&\n>> +\tgit log --pretty=\"%an%n%ae%n%al%n%aN%n%aE%n%aL\" -1 > actual &&\n> \n> Style: no space between redirection and filename, i.e. >actual\n> \n>> +\t{\n>> +\t\techo \"${GIT_AUTHOR_NAME}\" &&\n>> +\t\techo \"${GIT_AUTHOR_EMAIL}\" &&\n>> +\t\techo \"${TEST_AUTHOR_LOCALNAME}\"\n> \n> Broken &&-chain (though with just a bunch of echos it won't really\n> make much of a difference)\n> \n>> +\t\techo \"${GIT_AUTHOR_NAME}\" &&\n>> +\t\techo \"${GIT_AUTHOR_EMAIL}\" &&\n>> +\t\techo \"${TEST_AUTHOR_LOCALNAME}\"\n> \n> All these variables stand on their own, so the curly braces around\n> them are unnecessary.\n\nI've changed this code (based on Peff's suggestions) to other tests, however,\nAFAIK using braces around is considered \"good practice\".\n\nIt can't harm anything to have braces but if the preferred git coding style is\nto only use them when necessary I will remove them.\n\nP.\n\n> \n>> +\t} > expect &&\n>> +\ttest_cmp expect actual\n>> +'\n>> +\n>> +test_expect_success 'log pretty %cn %ce %cl %cN %cE %cL' '\n>> +\tgit checkout -b cncecl &&\n>> +\ttest_commit cncecl_test cncecl_test_file &&\n>> +\tgit log --pretty=\"%cn%n%ce%n%cl%n%cN%n%cE%n%cL\" -1 > actual &&\n>> +\t{\n>> +\t\techo \"${GIT_COMMITTER_NAME}\" &&\n>> +\t\techo \"${GIT_COMMITTER_EMAIL}\" &&\n>> +\t\techo \"${TEST_COMMITTER_LOCALNAME}\"\n>> +\t\techo \"${GIT_COMMITTER_NAME}\" &&\n>> +\t\techo \"${GIT_COMMITTER_EMAIL}\" &&\n>> +\t\techo \"${TEST_COMMITTER_LOCALNAME}\"\n>> +\t} > expect &&\n>> +\ttest_cmp expect actual\n>> +'\n>> +\n\n"},{"id":"384842","messageId":"xmqqwoctcszm.fsf@gitster-ct.c.googlers.com","threadId":"52109","inReplyTo":"5d9b8cbe-489e-b3eb-873c-4aee2e2015d0@redhat.com","subject":"Re: [PATCH v2] pretty: add \"%aL\"|\"%al|%cL|%cl\" option to output local-part of email addresses","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-25T05:08:13Z","receivedAt":"2019-10-25T05:08:17Z","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> It can't harm anything to have braces but if the preferred git coding style is\n> to only use them when necessary I will remove them.\n\nIt harms readability.  Don't try to be different just for the sake\nof being different, especially when you think \"it can't harm\nanything\" aka \"there is no strong reason to do so (or not to)\".\nInstead, help others by making sure that their eyes do not have to\nget distracted for such meaningless differences from existing parts\nof the system.\n\nThanks.\n"},{"id":"384853","messageId":"8baf0152-e38a-eaad-b181-72e2d3d2f00d@redhat.com","threadId":"52109","inReplyTo":"xmqqwoctcszm.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] pretty: add \"%aL\"|\"%al|%cL|%cl\" option to output local-part of email addresses","fromName":"Prarit Bhargava","fromEmail":"prarit@redhat.com","sentAt":"2019-10-25T10:56:40Z","receivedAt":"2019-10-25T10:56:49Z","isPatch":true,"sender":{"key":"prarit@redhat.com","avatar":"https://avatars.githubusercontent.com/u/11943259?v=4"},"body":"\n\nOn 10/25/19 1:08 AM, Junio C Hamano wrote:\n> Prarit Bhargava <prarit@redhat.com> writes:\n> \n>> It can't harm anything to have braces but if the preferred git coding style is\n>> to only use them when necessary I will remove them.\n> \n> It harms readability.  Don't try to be different just for the sake\n> of being different, especially when you think \"it can't harm\n> anything\" aka \"there is no strong reason to do so (or not to)\".\n> Instead, help others by making sure that their eyes do not have to\n> get distracted for such meaningless differences from existing parts\n> of the system.\n\nHa :)  I did pull them for v3 after doing a search.  I didn't reply properly and\nshould have said \"I'll do a search\".  Admittedly I'm concerned in a few spots in\nthe code where it *seems* like something could go wrong if the code is changed.\nBut nothing worth patching over.\n\nP.\n\n> \n> Thanks.\n> \n\n"}]}