{"thread":{"id":"47220","subject":"[PATCH] git-send-email: fix get_maintainer.pl regression","startedAt":"2017-11-16T15:48:24Z","lastAt":"2017-12-12T22:19:26Z","messageCount":27,"participants":["Alex Bennée","Eric Sunshine","Philip Oakley","Junio C Hamano","Thomas Adam","Matthieu Moy","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"332668","messageId":"20171116154814.23785-1-alex.bennee@linaro.org","threadId":"47220","inReplyTo":null,"subject":"[PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Alex Bennée","fromEmail":"alex.bennee@linaro.org","sentAt":"2017-11-16T15:48:14Z","receivedAt":"2017-11-16T15:48:24Z","isPatch":true,"sender":{"key":"alex.bennee@linaro.org","avatar":"https://avatars.githubusercontent.com/u/22458?v=4"},"body":"Getting rid of Mail::Address regressed behaviour with common\nget_maintainer scripts such as the Linux kernel. Fix the missed corner\ncase and add a test for it.\n\nFixes: cc9075067776ebd34cc08f31bf78bb05f12fd879\n\nSigned-off-by: Alex Bennée \n---\n perl/Git.pm | 3 +++\n t/t9000/test.pl | 4 +++-\n t/t9001-send-email.sh | 21 +++++++++++++++++++++\n 3 files changed, 27 insertions(+), 1 deletion(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex ffa09ace9..9b17de1cc 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -936,6 +936,9 @@ sub parse_mailboxes {\n \t\t\t$end_of_addr_seen = 0;\n \t\t} elsif ($token =~ /^\\(/) {\n \t\t\tpush @comment, $token;\n+\t\t} elsif ($token =~ /^\\)/) {\n+\t\t my $nested_comment = pop @comment;\n+\t\t\tpush @comment, \"$nested_comment$token\";\n \t\t} elsif ($token eq \"<\") {\n \t\t\tpush @phrase, (splice @address), (splice @buffer);\n \t\t} elsif ($token eq \">\") {\ndiff --git a/t/t9000/test.pl b/t/t9000/test.pl\nindex dfeaa9c65..f10be50cd 100755\n--- a/t/t9000/test.pl\n+++ b/t/t9000/test.pl\n@@ -35,7 +35,9 @@ my @success_list = (q[Jane],\n \tq['Jane 'Doe' ],\n \tq[Jane@:;\\.,()<>Doe ],\n \tq[Jane Doe],\n-\tq[ Jane Doe]);\n+\tq[ Jane Doe],\n+\tq[jdoe@example.com (open list:for thing (foo/bar))],\n+ );\n \n my @known_failure_list = (q[Jane\\ Doe ],\n \tq[\"Doe, Ja\"ne ],\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 4d261c2a9..0bcd7ab96 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -172,6 +172,27 @@ test_expect_success $PREREQ 'cc trailer with various syntax' '\n \ttest_cmp expected-cc commandline1\n '\n \n+test_expect_success $PREREQ 'setup get_mainter script for cc trailer' \"\n+cat >expected-cc-script.sh <<-EOF && chmod +x expected-cc-script.sh\n+#!/bin/sh\n+echo 'One Person (supporter:THIS (FOO/bar))'\n+echo 'Two Person (maintainer:THIS THING)'\n+echo 'Third List (moderated list:THIS THING (FOO/bar))'\n+echo ' (moderated list:FOR THING)'\n+echo 'five@example.com (open list:FOR THING (FOO/bar))'\n+echo 'six@example.com (open list)'\n+EOF\n+\"\n+\n+test_expect_success $PREREQ 'cc trailer with get_maintainer output' '\n+\ttest_commit cc-trailer &&\n+\tclean_fake_sendmail &&\n+\tgit send-email -1 --to=recipient@example.com \\\n+\t\t--cc-cmd=\"$(pwd)/expected-cc-script.sh\" \\\n+\t\t--smtp-server=\"$(pwd)/fake.sendmail\" &&\n+\ttest_cmp expected-cc commandline1\n+'\n+\n test_expect_success $PREREQ 'setup expect' \"\n cat >expected-show-all-headers <<\\EOF\n 0001-Second.patch\n-- \n2.15.0\n\n"},{"id":"332672","messageId":"874lpu189c.fsf@linaro.org","threadId":"47220","inReplyTo":"20171116154814.23785-1-alex.bennee@linaro.org","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Alex Bennée","fromEmail":"alex.bennee@linaro.org","sentAt":"2017-11-16T16:46:39Z","receivedAt":"2017-11-16T16:47:04Z","isPatch":true,"sender":{"key":"alex.bennee@linaro.org","avatar":"https://avatars.githubusercontent.com/u/22458?v=4"},"body":"\nAlex Bennée writes:\n\n> Getting rid of Mail::Address regressed behaviour with common\n> get_maintainer scripts such as the Linux kernel. Fix the missed corner\n> case and add a test for it.\n>\n\n> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n> index 4d261c2a9..0bcd7ab96 100755\n> --- a/t/t9001-send-email.sh\n> +++ b/t/t9001-send-email.sh\n> @@ -172,6 +172,27 @@ test_expect_success $PREREQ 'cc trailer with various syntax' '\n> \ttest_cmp expected-cc commandline1\n> '\n>\n> +test_expect_success $PREREQ 'setup get_mainter script for cc trailer' \"\n> +cat >expected-cc-script.sh <<-EOF && chmod +x expected-cc-script.sh\n> +#!/bin/sh\n> +echo 'One Person (supporter:THIS (FOO/bar))'\n> +echo 'Two Person (maintainer:THIS THING)'\n> +echo 'Third List (moderated list:THIS THING (FOO/bar))'\n> +echo ' (moderated list:FOR THING)'\n> +echo 'five@example.com (open list:FOR THING (FOO/bar))'\n> +echo 'six@example.com (open list)'\n> +EOF\n> +\"\n> +\n> +test_expect_success $PREREQ 'cc trailer with get_maintainer output' '\n> +\ttest_commit cc-trailer &&\n> +\tclean_fake_sendmail &&\n> +\tgit send-email -1 --to=recipient@example.com \\\n> +\t\t--cc-cmd=\"$(pwd)/expected-cc-script.sh\" \\\n> +\t\t--smtp-server=\"$(pwd)/fake.sendmail\" &&\n> +\ttest_cmp expected-cc commandline1\n> +'\n> +\n\nOK I'm afraid I don't fully understand the test harness as this breaks a\nbunch of other tests. If anyone can offer some pointers on how to fix\nI'd be grateful.\n\nIn the meantime I know the core change works because I tested with:\n\n#+name: send-patches-dry-run\n#+begin_src sh :results output\n# temp workaround\nexport PERL5LIB=/home/alex/src/git.git/perl/\ngit send-email --confirm=never --dry-run --quiet ${mailto} ${series}.patches/*\n#+end_src\n\nWhen I sent my last set of kernel patches to the list (the workflow that\nwas broken before by the cc9075067776ebd34cc08f31bf78bb05f12fd879 change\nlanding via my git stable PPA).\n\n--\nAlex Bennée\n"},{"id":"332841","messageId":"CAPig+cSh0tVVkh0xF9FwCfM4gngAWMSN_FXd2zhzHcy2trYXfw@mail.gmail.com","threadId":"47220","inReplyTo":"20171116154814.23785-1-alex.bennee@linaro.org","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-11-19T02:54:46Z","receivedAt":"2017-11-19T03:03:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Nov 16, 2017 at 10:48 AM, Alex Bennée wrote:\n> Getting rid of Mail::Address regressed behaviour with common\n> get_maintainer scripts such as the Linux kernel. Fix the missed corner\n> case and add a test for it.\n\nIt is not at all clear, based upon this text, what this is fixing.\nWhen you re-roll, please provide a description of the regression in\nsufficient detail for readers to easily understand the problem and the\nsolution.\n\nMore below...\n\n> Signed-off-by: Alex Bennée \n> ---\n> diff --git a/t/t9000/test.pl b/t/t9000/test.pl\n> @@ -35,7 +35,9 @@ my @success_list = (q[Jane],\n> q['Jane 'Doe' ],\n> q[Jane@:;\\.,()<>Doe ],\n> q[Jane Doe],\n> - q[ Jane Doe]);\n> + q[ Jane Doe],\n> + q[jdoe@example.com (open list:for thing (foo/bar))],\n> + );\n\nStyle nit: The dangling \");\" introduced by this change differs from\nthe other list(s) in this file which have the closing \");\" on the same\nline as the last item in the list.\n\n> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n> +test_expect_success $PREREQ 'setup get_mainter script for cc trailer' \"\n> +cat >expected-cc-script.sh <<-EOF && chmod +x expected-cc-script.sh\n> +#!/bin/sh\n> +echo 'One Person (supporter:THIS (FOO/bar))'\n> +echo 'Two Person (maintainer:THIS THING)'\n> +echo 'Third List (moderated list:THIS THING (FOO/bar))'\n> +echo ' (moderated list:FOR THING)'\n> +echo 'five@example.com (open list:FOR THING (FOO/bar))'\n> +echo 'six@example.com (open list)'\n> +EOF\n> +\"\n\nUse write_script() to create the script:\n\n--- 8< ---\nwrite_script expected-cc-script.sh <<-\\EOF &&\necho '...'\n...\nEOF\n--- 8< ---\n\nNo need for #!/bin/sh or chmod, both of which are handled by\nwrite_script(). In fact, you could fold this into the following test\n(since there doesn't seem a good reason for it to be separate).\n\n> +test_expect_success $PREREQ 'cc trailer with get_maintainer output' '\n> + test_commit cc-trailer &&\n> + clean_fake_sendmail &&\n> + git send-email -1 --to=recipient@example.com \\\n> + --cc-cmd=\"$(pwd)/expected-cc-script.sh\" \\\n> + --smtp-server=\"$(pwd)/fake.sendmail\" &&\n> + test_cmp expected-cc commandline1\n> +'\n> OK I'm afraid I don't fully understand the test harness as this breaks a\n> bunch of other tests. If anyone can offer some pointers on how to fix\n> I'd be grateful.\n\nThere are several problems:\n\n* test_commit bombs because there is already a tag named \"cc-trailer\"\ncreated by an earlier test. Fix this by choosing a different name for\nthe commit. Even better would be to avoid making the commit in the\nfirst place since it doesn't appear to be relevant to the test, and\nthe test works fine without it.\n\n* The directory in which the expected-cc-script.sh is created contains\na space; this is intentional to catch bugs in tests and Git itself. In\nthis case, your test is exposing what might be considered a bug in\ngit-send-email itself, in which it invokes the --cc-cmd as \"/path/with\nspace/expected-cc-script.sh\", which is interpreted as trying to invoke\nprogram \"/path/with\" with argument \"space/expected-cc-script.sh\". One\nfix (which you could submit as a preparatory patch, making this a\n2-patch series) would be this:\n\n--- 8< ---\ndiff --git a/git-send-email.perl b/git-send-email.perl\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1724,7 +1724,7 @@ sub recipients_cmd {\n my ($prefix, $what, $cmd, $file) = @_;\n\n my @addresses = ();\n- open my $fh, \"-|\", \"$cmd \\Q$file\\E\"\n+ open my $fh, \"-|\", \"\\Q$cmd\\E \\Q$file\\E\"\n or die sprintf(__(\"(%s) Could not execute '%s'\"), $prefix, $cmd);\n while (my $address = <$fh>) {\n $address =~ s/^\\s*//g;\n--- 8< ---\n\nHowever, it's possible that might break existing users who rely on\n--cc-cmd=\"myscript --option arg\" working. It's not clear which\nbehavior is correct.\n\n* Subsequent tests fail because you've added a commit which they are\nnot expecting. If you look at tests earlier in the file, you will see\nthat they deal with this by removing the added commit (via \"git reset\n--hard HEAD^\") by the time the test finishes. However, as noted above,\nthis new test doesn't actually need to make this commit in the first\nplace.\n\nSo, with fixes, your test might look like this:\n\n--- 8< ---\ntest_expect_success $PREREQ 'cc trailer with get_maintainer output' '\n test_commit cc-trailer-get-maintainer &&\n test_when_finished \"git reset --hard HEAD^\" &&\n clean_fake_sendmail &&\n git send-email -1 --to=recipient@example.com \\\n --cc-cmd=\"$(pwd)/expected-cc-script.sh\" \\\n --smtp-server=\"$(pwd)/fake.sendmail\" &&\n test_cmp expected-cc commandline1\n'\n--- 8< ---\n\nOr, if you drop the unnecessary test_commit():\n\n--- 8< ---\ntest_expect_success $PREREQ 'cc trailer with get_maintainer output' '\n clean_fake_sendmail &&\n git send-email -1 --to=recipient@example.com \\\n --cc-cmd=\"$(pwd)/expected-cc-script.sh\" \\\n --smtp-server=\"$(pwd)/fake.sendmail\" &&\n test_cmp expected-cc commandline1\n'\n--- 8< ---\n"},{"id":"332900","messageId":"87a7zhxm9s.fsf@linaro.org","threadId":"47220","inReplyTo":"CAPig+cSh0tVVkh0xF9FwCfM4gngAWMSN_FXd2zhzHcy2trYXfw@mail.gmail.com","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Alex Bennée","fromEmail":"alex.bennee@linaro.org","sentAt":"2017-11-20T10:44:15Z","receivedAt":"2017-11-20T10:44:25Z","isPatch":true,"sender":{"key":"alex.bennee@linaro.org","avatar":"https://avatars.githubusercontent.com/u/22458?v=4"},"body":"\nEric Sunshine writes:\n\n> On Thu, Nov 16, 2017 at 10:48 AM, Alex Bennée wrote:\n>> Getting rid of Mail::Address regressed behaviour with common\n>> get_maintainer scripts such as the Linux kernel. Fix the missed corner\n>> case and add a test for it.\n>\n> It is not at all clear, based upon this text, what this is fixing.\n> When you re-roll, please provide a description of the regression in\n> sufficient detail for readers to easily understand the problem and the\n> solution.\n\nHow about:\n\nSince the removal of Mail::Address from git-send-email certain addresses\ncommon in MAINTAINERS now fail to get correctly parsed by\nGit::parse_mailboxes. Specifically the patterns with embedded\nparenthesis fail, for example for MAINTAINERS:\n\n KERNEL VIRTUAL MACHINE FOR ARM (KVM/arm)\n L:\tlinux-arm-kernel@lists.infradead.org (moderated for non-subscribers)\n L:\tkvmarm@lists.cs.columbia.edu\n\nIs returned by get_maintainers.pl as:\n\n linux-arm-kernel@lists.infradead.org (moderated list:KERNEL VIRTUAL MACHINE FOR ARM (KVM/arm))\n kvmarm@lists.cs.columbia.edu (open list:KERNEL VIRTUAL MACHINE FOR ARM (KVM/arm))\n\nBut the parse_mailboxes code mangles the address, appending the trailing\nparenthesis to the email address to the address part causing it to fail validation:\n\n error: unable to extract a valid address from: linux-arm-kernel@lists.infradead.org) (moderated list:KERNEL VIRTUAL MACHINE FOR ARM (KVM/arm)\n error: unable to extract a valid address from: kvmarm@lists.cs.columbia.edu) (open list:KERNEL VIRTUAL MACHINE FOR ARM (KVM/arm)\n\nAs this is a common pattern which was handled by Mail::Address I've\nfixed the regression by explicitly capturing a trailing bracket and\nappending it to the comment token.\n\n>\n> More below...\n>\n>> Signed-off-by: Alex Bennée \n>> ---\n>> diff --git a/t/t9000/test.pl b/t/t9000/test.pl\n>> @@ -35,7 +35,9 @@ my @success_list = (q[Jane],\n>> q['Jane 'Doe' ],\n>> q[Jane@:;\\.,()<>Doe ],\n>> q[Jane Doe],\n>> - q[ Jane Doe]);\n>> + q[ Jane Doe],\n>> + q[jdoe@example.com (open list:for thing (foo/bar))],\n>> + );\n>\n> Style nit: The dangling \");\" introduced by this change differs from\n> the other list(s) in this file which have the closing \");\" on the same\n> line as the last item in the list.\n>\n>> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n>> +test_expect_success $PREREQ 'setup get_mainter script for cc trailer' \"\n>> +cat >expected-cc-script.sh <<-EOF && chmod +x expected-cc-script.sh\n>> +#!/bin/sh\n>> +echo 'One Person (supporter:THIS (FOO/bar))'\n>> +echo 'Two Person (maintainer:THIS THING)'\n>> +echo 'Third List (moderated list:THIS THING (FOO/bar))'\n>> +echo ' (moderated list:FOR THING)'\n>> +echo 'five@example.com (open list:FOR THING (FOO/bar))'\n>> +echo 'six@example.com (open list)'\n>> +EOF\n>> +\"\n>\n> Use write_script() to create the script:\n\nThanks for the pointers, I'll fix it up.\n\nI missed the existence of write_script.sh while I scanned through\nt/README, perhaps a stanza in in \"Test harness library\":\n\n - write_script <<-\\EOF && \n echo '...'\n ...\n EOF\n\n The write_script helper takes care of ensuring the created helper\n script has the right shebang and is executable.\n\n?\n\n>\n> --- 8< ---\n> write_script expected-cc-script.sh <<-\\EOF &&\n> echo '...'\n> ...\n> EOF\n> --- 8< ---\n>\n> No need for #!/bin/sh or chmod, both of which are handled by\n> write_script(). In fact, you could fold this into the following test\n> (since there doesn't seem a good reason for it to be separate).\n>\n>> +test_expect_success $PREREQ 'cc trailer with get_maintainer output' '\n>> + test_commit cc-trailer &&\n>> + clean_fake_sendmail &&\n>> + git send-email -1 --to=recipient@example.com \\\n>> + --cc-cmd=\"$(pwd)/expected-cc-script.sh\" \\\n>> + --smtp-server=\"$(pwd)/fake.sendmail\" &&\n>> + test_cmp expected-cc commandline1\n>> +'\n>> OK I'm afraid I don't fully understand the test harness as this breaks a\n>> bunch of other tests. If anyone can offer some pointers on how to fix\n>> I'd be grateful.\n>\n> There are several problems:\n>\n> * test_commit bombs because there is already a tag named \"cc-trailer\"\n> created by an earlier test. Fix this by choosing a different name for\n> the commit. Even better would be to avoid making the commit in the\n> first place since it doesn't appear to be relevant to the test, and\n> the test works fine without it.\n\nAhh that makes sense. Again perhaps in the t/README \"Keep in mind:\"\n\n - All the tests in a given test file run sequentially and share\n repository state. This means you should take care not to break\n assumptions of later tests as to which commits exist in the test\n repository.\n\n?\n\n>\n> * The directory in which the expected-cc-script.sh is created contains\n> a space; this is intentional to catch bugs in tests and Git itself. In\n> this case, your test is exposing what might be considered a bug in\n> git-send-email itself, in which it invokes the --cc-cmd as \"/path/with\n> space/expected-cc-script.sh\", which is interpreted as trying to invoke\n> program \"/path/with\" with argument \"space/expected-cc-script.sh\". One\n> fix (which you could submit as a preparatory patch, making this a\n> 2-patch series) would be this:\n>\n> --- 8< ---\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -1724,7 +1724,7 @@ sub recipients_cmd {\n> my ($prefix, $what, $cmd, $file) = @_;\n>\n> my @addresses = ();\n> - open my $fh, \"-|\", \"$cmd \\Q$file\\E\"\n> + open my $fh, \"-|\", \"\\Q$cmd\\E \\Q$file\\E\"\n> or die sprintf(__(\"(%s) Could not execute '%s'\"), $prefix, $cmd);\n> while (my $address = <$fh>) {\n> $address =~ s/^\\s*//g;\n> --- 8< ---\n>\n> However, it's possible that might break existing users who rely on\n> --cc-cmd=\"myscript --option arg\" working. It's not clear which\n> behavior is correct.\n>\n> * Subsequent tests fail because you've added a commit which they are\n> not expecting. If you look at tests earlier in the file, you will see\n> that they deal with this by removing the added commit (via \"git reset\n> --hard HEAD^\") by the time the test finishes. However, as noted above,\n> this new test doesn't actually need to make this commit in the first\n> place.\n>\n> So, with fixes, your test might look like this:\n>\n> --- 8< ---\n> test_expect_success $PREREQ 'cc trailer with get_maintainer output' '\n> test_commit cc-trailer-get-maintainer &&\n> test_when_finished \"git reset --hard HEAD^\" &&\n> clean_fake_sendmail &&\n> git send-email -1 --to=recipient@example.com \\\n> --cc-cmd=\"$(pwd)/expected-cc-script.sh\" \\\n> --smtp-server=\"$(pwd)/fake.sendmail\" &&\n> test_cmp expected-cc commandline1\n> '\n> --- 8< ---\n>\n> Or, if you drop the unnecessary test_commit():\n>\n> --- 8< ---\n> test_expect_success $PREREQ 'cc trailer with get_maintainer output' '\n> clean_fake_sendmail &&\n> git send-email -1 --to=recipient@example.com \\\n> --cc-cmd=\"$(pwd)/expected-cc-script.sh\" \\\n> --smtp-server=\"$(pwd)/fake.sendmail\" &&\n> test_cmp expected-cc commandline1\n> '\n> --- 8< ---\n\nThanks for the comments. v2 being rolled ;-)\n\n--\nAlex Bennée\n"},{"id":"332928","messageId":"CAPig+cSbRrGnyDkunMFiFXbWRMAsGyuAL-0FpP1QTtjSUSY2Hg@mail.gmail.com","threadId":"47220","inReplyTo":"CAPig+cSh0tVVkh0xF9FwCfM4gngAWMSN_FXd2zhzHcy2trYXfw@mail.gmail.com","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-11-20T18:57:10Z","receivedAt":"2017-11-20T18:57:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Nov 18, 2017 at 9:54 PM, Eric Sunshine wrote:\n> On Thu, Nov 16, 2017 at 10:48 AM, Alex Bennée wrote:\n>> +test_expect_success $PREREQ 'cc trailer with get_maintainer output' '\n>> + [...]\n>> + git send-email -1 --to=recipient@example.com \\\n>> + --cc-cmd=\"$(pwd)/expected-cc-script.sh\" \\\n>> + [...]\n>> +'\n>> OK I'm afraid I don't fully understand the test harness as this breaks a\n>> bunch of other tests. If anyone can offer some pointers on how to fix\n>> I'd be grateful.\n>\n> There are several problems:\n> [...]\n> * The directory in which the expected-cc-script.sh is created contains\n> a space; this is intentional to catch bugs in tests and Git itself. In\n> this case, your test is exposing what might be considered a bug in\n> git-send-email itself, in which it invokes the --cc-cmd as \"/path/with\n> space/expected-cc-script.sh\", which is interpreted as trying to invoke\n> program \"/path/with\" with argument \"space/expected-cc-script.sh\". One\n> fix (which you could submit as a preparatory patch, making this a\n> 2-patch series) would be this:\n>\n> --- 8< ---\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> @@ -1724,7 +1724,7 @@ sub recipients_cmd {\n> - open my $fh, \"-|\", \"$cmd \\Q$file\\E\"\n> + open my $fh, \"-|\", \"\\Q$cmd\\E \\Q$file\\E\"\n> --- 8< ---\n>\n> However, it's possible that might break existing users who rely on\n> --cc-cmd=\"myscript --option arg\" working. It's not clear which\n> behavior is correct.\n\nThe more I think about this, the less I consider this a bug in\ngit-send-email. As noted, people might legitimately use a complex\ncommand (--cc-cmd=\"myscript--option arg\"), so changing git-send-email\nto treat cc-cmd as an atomic string seems like a bad idea.\n\nAssuming no changes to git-send-email, to get your test working, you\ncould try to figure out how to quote the script's path you're\nspecifying with --cc-cmd, however, even easier would be to drop $(pwd)\naltogether. That is, instead of:\n\n --cc-cmd=\"$(pwd)/expected-cc-script.sh\"\n\njust use:\n\n --cc-cmd=./expected-cc-script.sh\n"},{"id":"332999","messageId":"CAPig+cTXq6jSN9f2_xyj=Jfv_cg2kUFUtA5uVkZDrRRSi2x7vg@mail.gmail.com","threadId":"47220","inReplyTo":"20171116154814.23785-1-alex.bennee@linaro.org","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-11-20T22:14:53Z","receivedAt":"2017-11-20T22:15:01Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"A few more comments/observations...\n\nOn Thu, Nov 16, 2017 at 10:48 AM, Alex Bennée wrote:\n> diff --git a/perl/Git.pm b/perl/Git.pm\n> @@ -936,6 +936,9 @@ sub parse_mailboxes {\n> $end_of_addr_seen = 0;\n> } elsif ($token =~ /^\\(/) {\n> push @comment, $token;\n> + } elsif ($token =~ /^\\)/) {\n> + my $nested_comment = pop @comment;\n> + push @comment, \"$nested_comment$token\";\n\nDue to the way tokenization works, it looks like you will only ever\nsee a \")\" as a single character. That suggests that you should be\nusing ($token eq \")\"), as is done for \"<\" and \">\", rather than ($token\n=~ /^\\)/).\n\nWhat happens if there is text before the final closing ')'? For\ninstance, \"foo@bar (bibble (bobble) smoo)\" or \"...)smoo)\". The result\nis that \"smoo\" ends up tacked onto the end of the email address\n(\"foo@barsmoo\") rather than incorporated into the comment, as\nintended.\n\nWhat happens if you encounter a \")\" but haven't yet encountered an\nopening \"(\" (that is, @comment is empty)? For example, \"foo@bar )\". In\nthat case, it unconditionally pops from the empty array, which seems\niffy at best. It might be nice to see this case taken into\nconsideration explicitly.\n\nI also was wondering if it would make more sense to take advantage of\nPerl's ability to match nested expressions (??{$nested}), however,\nthat feature apparently was added in 5.10, and Git.pm only requires\n5.8, so perhaps not (unless we want to bump the requirement higher).\n\nAside from those observations, it looks like the tokenizer in this\nfunction is broken. For any input with the address enclosed in \"<\" and\n\">\", the comment is lost entirely; it doesn't even end up in the\n@tokens array. Since you're already fixing bugs/regressions in this\ncode, perhaps that's something you'd like to tackle as well in a\nseparate patch? (\"No\" is an acceptable answer, of course.)\n\n> } elsif ($token eq \"<\") {\n> push @phrase, (splice @address), (splice @buffer);\n> } elsif ($token eq \">\") {\n"},{"id":"333017","messageId":"CAPig+cQQHMt9757aoBAQfyMtnxmDmrVJ_g8AmzvmmqTMAYaUXw@mail.gmail.com","threadId":"47220","inReplyTo":"87a7zhxm9s.fsf@linaro.org","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-11-20T22:34:50Z","receivedAt":"2017-11-20T22:34:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 20, 2017 at 5:44 AM, Alex Bennée wrote:\n> Eric Sunshine writes:\n>> It is not at all clear, based upon this text, what this is fixing.\n>> When you re-roll, please provide a description of the regression in\n>> sufficient detail for readers to easily understand the problem and the\n>> solution.\n>\n> How about:\n>\n> Since the removal of Mail::Address from git-send-email certain addresses\n> common in MAINTAINERS now fail to get correctly parsed by\n> Git::parse_mailboxes. Specifically the patterns with embedded\n> parenthesis fail, for example for MAINTAINERS:\n> [...snip...]\n\nThanks, that explanation makes the problem quite clear. It also\nallowed me to examine the fix with a more critical eye, which led to\nseveral additional comments and observations (sent in my previous\nemail).\n\n>> Use write_script() to create the script:\n>\n> Thanks for the pointers, I'll fix it up.\n>\n> I missed the existence of write_script.sh while I scanned through\n> t/README, perhaps a stanza in in \"Test harness library\":\n>\n> - write_script <<-\\EOF && \n> echo '...'\n> ...\n> EOF\n>\n> The write_script helper takes care of ensuring the created helper\n> script has the right shebang and is executable.\n> ?\n\nMentioning write_script() in the \"Test harness library\" section might\nindeed be a good idea.\n\n> Ahh that makes sense. Again perhaps in the t/README \"Keep in mind:\"\n>\n> - All the tests in a given test file run sequentially and share\n> repository state. This means you should take care not to break\n> assumptions of later tests as to which commits exist in the test\n> repository.\n> ?\n\nMaybe, maybe not. Ideally, tests should be as self-contained as\npossible. In practice, of course, this isn't always practical or even\npossible (and there is a lot of old test code which is heavily\ndependent upon state left over from earlier tests). Perfectly\nself-contained tests (or self-contained _sets_ of tests) could\ntheoretically be run in parallel, so, in general, we'd like to\nencourage people to tend toward self-contained, thus the above snippet\nmay be going in the wrong direction.\n"},{"id":"333026","messageId":"D810DA8B202742B38343EBB44BD54A1D@PhilipOakley","threadId":"47220","inReplyTo":"CAPig+cSbRrGnyDkunMFiFXbWRMAsGyuAL-0FpP1QTtjSUSY2Hg@mail.gmail.com","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2017-11-21T00:07:36Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Eric Sunshine\" \nOn Sat, Nov 18, 2017 at 9:54 PM, Eric Sunshine \nwrote:\n> On Thu, Nov 16, 2017 at 10:48 AM, Alex Bennée \n> wrote:\n>> +test_expect_success $PREREQ 'cc trailer with get_maintainer output' '\n>> + [...]\n>> + git send-email -1 --to=recipient@example.com \\\n>> + --cc-cmd=\"$(pwd)/expected-cc-script.sh\" \\\n>> + [...]\n>> +'\n>> OK I'm afraid I don't fully understand the test harness as this breaks a\n>> bunch of other tests. If anyone can offer some pointers on how to fix\n>> I'd be grateful.\n>\n> There are several problems:\n> [...]\n> * The directory in which the expected-cc-script.sh is created contains\n> a space; this is intentional to catch bugs in tests and Git itself. In\n> this case, your test is exposing what might be considered a bug in\n> git-send-email itself, in which it invokes the --cc-cmd as \"/path/with\n> space/expected-cc-script.sh\", which is interpreted as trying to invoke\n> program \"/path/with\" with argument \"space/expected-cc-script.sh\". One\n> > fix (which you could submit as a preparatory patch, making this a\n> > 2-patch series) would be this:\n> >\n> > --- 8< ---\n> > diff --git a/git-send-email.perl b/git-send-email.perl\n> > @@ -1724,7 +1724,7 @@ sub recipients_cmd {\n> > - open my $fh, \"-|\", \"$cmd \\Q$file\\E\"\n> > + open my $fh, \"-|\", \"\\Q$cmd\\E \\Q$file\\E\"\n> > --- 8< ---\n> >\n> > However, it's possible that might break existing users who rely on\n> > --cc-cmd=\"myscript --option arg\" working. It's not clear which\n> > behavior is correct.\n>\n> The more I think about this, the less I consider this a bug in\n> git-send-email. As noted, people might legitimately use a complex\n> command (--cc-cmd=\"myscript--option arg\"), so changing git-send-email\n> to treat cc-cmd as an atomic string seems like a bad idea.\n\nA while back I proposed some documentation updates\nhttps://public-inbox.org/git/1437416790-5792-1-git-send-email-philipoakley@iee.org/\nregarding what is (should be) allowed in the cc-cmd etc., and at the time\nJunio suggested that possible existing uses of the current code would be\nabuses. I didn't pursue it further, but it may be useful guidance here as to\npotential real world command lines..\n\n>\n> Assuming no changes to git-send-email, to get your test working, you\n> could try to figure out how to quote the script's path you're\n> specifying with --cc-cmd, however, even easier would be to drop $(pwd)\n> altogether. That is, instead of:\n>\n> --cc-cmd=\"$(pwd)/expected-cc-script.sh\"\n>\n> just use:\n>\n> --cc-cmd=./expected-cc-script.sh\n\n"},{"id":"333028","messageId":"CAPig+cQvjX4MxttytKu=et2tBz5QoaDNtp10oqvynDJGYLxUWQ@mail.gmail.com","threadId":"47220","inReplyTo":"D810DA8B202742B38343EBB44BD54A1D@PhilipOakley","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-11-21T00:30:28Z","receivedAt":"2017-11-21T00:30:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 20, 2017 at 7:07 PM, Philip Oakley wrote:\n> From: \"Eric Sunshine\" \n> On Sat, Nov 18, 2017 at 9:54 PM, Eric Sunshine \n> wrote:\n>> > --- 8< ---\n>> > diff --git a/git-send-email.perl b/git-send-email.perl\n>> > @@ -1724,7 +1724,7 @@ sub recipients_cmd {\n>> > - open my $fh, \"-|\", \"$cmd \\Q$file\\E\"\n>> > + open my $fh, \"-|\", \"\\Q$cmd\\E \\Q$file\\E\"\n>> > --- 8< ---\n>> >\n>> > However, it's possible that might break existing users who rely on\n>> > --cc-cmd=\"myscript --option arg\" working. It's not clear which\n>> > behavior is correct.\n>>\n>> The more I think about this, the less I consider this a bug in\n>> git-send-email. As noted, people might legitimately use a complex\n>> command (--cc-cmd=\"myscript--option arg\"), so changing git-send-email\n>> to treat cc-cmd as an atomic string seems like a bad idea.\n>\n> A while back I proposed some documentation updates\n> https://public-inbox.org/git/1437416790-5792-1-git-send-email-philipoakley@iee.org/\n> regarding what is (should be) allowed in the cc-cmd etc., and at the time\n> Junio suggested that possible existing uses of the current code would be\n> abuses. I didn't pursue it further, but it may be useful guidance here as to\n> potential real world command lines..\n\nThanks for the link. I had forgotten that discussion, but re-reading\nit brought most of it back. Given how the discussion ended -- with\nvalid use being that --cc-cmd names a program which accepts a single\nargument -- then the above patch to recipients_cmd() might indeed be\ndesirable.\n\nAnd, the documentation is still lacking, saying nothing about that\nsingle argument passed to the cc-cmd...\n"},{"id":"333029","messageId":"xmqqfu98lbe1.fsf@gitster.mtv.corp.google.com","threadId":"47220","inReplyTo":"CAPig+cSbRrGnyDkunMFiFXbWRMAsGyuAL-0FpP1QTtjSUSY2Hg@mail.gmail.com","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-21T00:32:22Z","receivedAt":"2017-11-21T00:32:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> The more I think about this, the less I consider this a bug in\n> git-send-email. As noted, people might legitimately use a complex\n> command (--cc-cmd=\"myscript--option arg\"), so changing git-send-email\n> to treat cc-cmd as an atomic string seems like a bad idea.\n\nI concur. Thanks for thinking this through.\n\n> Assuming no changes to git-send-email, to get your test working, you\n> could try to figure out how to quote the script's path you're\n> specifying with --cc-cmd, however, even easier would be to drop $(pwd)\n> altogether. That is, instead of:\n>\n> --cc-cmd=\"$(pwd)/expected-cc-script.sh\"\n>\n> just use:\n>\n> --cc-cmd=./expected-cc-script.sh\n\nSounds sensible.\n"},{"id":"333126","messageId":"87wp2jwe9o.fsf@linaro.org","threadId":"47220","inReplyTo":"CAPig+cTXq6jSN9f2_xyj=Jfv_cg2kUFUtA5uVkZDrRRSi2x7vg@mail.gmail.com","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Alex Bennée","fromEmail":"alex.bennee@linaro.org","sentAt":"2017-11-21T20:46:59Z","receivedAt":"2017-11-21T20:47:06Z","isPatch":true,"sender":{"key":"alex.bennee@linaro.org","avatar":"https://avatars.githubusercontent.com/u/22458?v=4"},"body":"\nEric Sunshine writes:\n\n> A few more comments/observations...\n>\n> On Thu, Nov 16, 2017 at 10:48 AM, Alex Bennée wrote:\n>> diff --git a/perl/Git.pm b/perl/Git.pm\n>> @@ -936,6 +936,9 @@ sub parse_mailboxes {\n>> $end_of_addr_seen = 0;\n>> } elsif ($token =~ /^\\(/) {\n>> push @comment, $token;\n>> + } elsif ($token =~ /^\\)/) {\n>> + my $nested_comment = pop @comment;\n>> + push @comment, \"$nested_comment$token\";\n>\n> Due to the way tokenization works, it looks like you will only ever\n> see a \")\" as a single character. That suggests that you should be\n> using ($token eq \")\"), as is done for \"<\" and \">\", rather than ($token\n> =~ /^\\)/).\n>\n> What happens if there is text before the final closing ')'? For\n> instance, \"foo@bar (bibble (bobble) smoo)\" or \"...)smoo)\". The result\n> is that \"smoo\" ends up tacked onto the end of the email address\n> (\"foo@barsmoo\") rather than incorporated into the comment, as\n> intended.\n>\n> What happens if you encounter a \")\" but haven't yet encountered an\n> opening \"(\" (that is, @comment is empty)? For example, \"foo@bar )\". In\n> that case, it unconditionally pops from the empty array, which seems\n> iffy at best. It might be nice to see this case taken into\n> consideration explicitly.\n\nYeah I was only really aiming for the current regression but I'm sure it\ncould be more solid. I do note that my @known_failure_list in test.pl\nhas a bunch of other cases that need fixing up.\n\n> I also was wondering if it would make more sense to take advantage of\n> Perl's ability to match nested expressions (??{$nested}), however,\n> that feature apparently was added in 5.10, and Git.pm only requires\n> 5.8, so perhaps not (unless we want to bump the requirement higher).\n\nHmm that might be a case of abusing regex to do something better suited\nto a proper tokenizer.\n\n>\n> Aside from those observations, it looks like the tokenizer in this\n> function is broken. For any input with the address enclosed in \"<\" and\n> \">\", the comment is lost entirely; it doesn't even end up in the\n> @tokens array. Since you're already fixing bugs/regressions in this\n> code, perhaps that's something you'd like to tackle as well in a\n> separate patch? (\"No\" is an acceptable answer, of course.)\n>\n>> } elsif ($token eq \"<\") {\n>> push @phrase, (splice @address), (splice @buffer);\n>> } elsif ($token eq \">\") {\n\nI can have a go but my perl-fu has weakened somewhat since I stopped\nhaving to maintain perl code for a living. It's almost as though my\nbrain was glad to dump the knowledge ;-)\n\nI guess we could maintain a nesting count and a current token type and\nuse that to more intelligently direct the nested portions to the\nappropriate bits. Maybe Matthieu or Remi (CC'ed) might want to chime in\non other options?\n\n--\nAlex Bennée\n"},{"id":"333127","messageId":"20171121205206.fvwjkkwhil4abmmk@laptop","threadId":"47220","inReplyTo":"87wp2jwe9o.fsf@linaro.org","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Thomas Adam","fromEmail":"thomas@xteddy.org","sentAt":"2017-11-21T20:52:08Z","receivedAt":"2017-11-21T20:52:16Z","isPatch":true,"sender":{"key":"thomas@xteddy.org","avatar":"https://gravatar.com/avatar/e7256db4738e501e5d2e84f00bb0bd99503165729573848a03330301fc2adc4a?d=mp&s=160"},"body":"On Tue, Nov 21, 2017 at 08:46:59PM +0000, Alex Bennée wrote:\n> \n> Eric Sunshine writes:\n> > Aside from those observations, it looks like the tokenizer in this\n> > function is broken. For any input with the address enclosed in \"<\" and\n> > \">\", the comment is lost entirely; it doesn't even end up in the\n> > @tokens array. Since you're already fixing bugs/regressions in this\n> > code, perhaps that's something you'd like to tackle as well in a\n> > separate patch? (\"No\" is an acceptable answer, of course.)\n> >\n> >> } elsif ($token eq \"<\") {\n> >> push @phrase, (splice @address), (splice @buffer);\n> >> } elsif ($token eq \">\") {\n> \n> I can have a go but my perl-fu has weakened somewhat since I stopped\n> having to maintain perl code for a living. It's almost as though my\n> brain was glad to dump the knowledge ;-)\n> \n> I guess we could maintain a nesting count and a current token type and\n> use that to more intelligently direct the nested portions to the\n> appropriate bits. Maybe Matthieu or Remi (CC'ed) might want to chime in\n> on other options?\n\nTrying to come up with a reinvention of regexps for email addresses is asking\nfor trouble, not to mention a crappy rod for your own back. Don't do that.\nThis is why people use Mail::Address.\n\nhttps://metacpan.org/pod/distribution/MailTools/lib/Mail/Address.pod\n\n-- Thomas Adam\n"},{"id":"333212","messageId":"xmqq8tezyu3g.fsf@gitster.mtv.corp.google.com","threadId":"47220","inReplyTo":"20171121205206.fvwjkkwhil4abmmk@laptop","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-22T01:34:27Z","receivedAt":"2017-11-22T01:34:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Adam writes:\n\n> Trying to come up with a reinvention of regexps for email addresses is asking\n> for trouble, not to mention a crappy rod for your own back. Don't do that.\n> This is why people use Mail::Address.\n>\n> https://metacpan.org/pod/distribution/MailTools/lib/Mail/Address.pod\n\nNow we are coming back to cc907506 (\"send-email: don't use\nMail::Address, even if available\", 2017-08-23). It argues\n\n * Having this optional Mail::Address makes it tempting to anwser \"please\n install Mail::Address\" to users instead of fixing our own code. We've\n reached the stage where bugs in our parser should be fixed, not worked\n around.\n\nbut if it costs us maintaining our substitute that much, it seems to\nme that depending on Mail::Address is not just tempting but may be a\nsensible way forward.\n\nWas there any reason why Mail::Address was _inadequate_? I know we\nhad trouble with random garbage that are *not* addresses people put\non the in-body CC: trailer in the past, but I do not recall if they\nare something Mail::Address would give worse result and we need our\nworkaround (hence our own substitute), or Mail::Address would handle\nthem just fine.\n"},{"id":"333257","messageId":"q7h97euiradu.fsf@orange.lip.ens-lyon.fr","threadId":"47220","inReplyTo":"b131cc195280498ea3a77a37eff8444e@BPMBX2013-01.univ-lyon1.fr","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@univ-lyon1.fr","sentAt":"2017-11-22T08:22:05Z","receivedAt":"2017-11-22T08:22:17Z","isPatch":true,"sender":{"key":"matthieu.moy@univ-lyon1.fr","avatar":"https://gravatar.com/avatar/8ab83b763226bd297b59ddd1463a8bfc852924577191233f73a953b86888fe0c?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> Was there any reason why Mail::Address was _inadequate_?\n\nI think the main reason was that Mail::Address is not a standard perl\nmodule, and not relying on it avoided one external dependency. AFAIK, we\ndon't really have a good way to deal with Perl dependencies, so having a\nstrong requirement on Mail::Address will probably end up in a runtime\nerror for users who compile Git from source (people using a packaged\nversion have their package manager to deal with this).\n\n> I know we had trouble with random garbage that are *not* addresses\n> people put on the in-body CC: trailer in the past, but I do not recall\n> if they are something Mail::Address would give worse result and we\n> need our workaround (hence our own substitute), or Mail::Address would\n> handle them just fine.\n\nFor a long time, we used Mail::Address if it was available and I don't\nthink we had issues with it.\n\nMy point in cc907506 (\"send-email: don't use Mail::Address, even if\navailable\", 2017-08-23) was not that Mail::Address was bad, but that\nchanging our behavior depending on whether it was there or not was\nreally bad. For example, the issue dealt with in this thread probably\nalways existed, but it was present only for *some* users.\n\n-- \nMatthieu Moy\nhttps://matthieu-moy.fr/\n"},{"id":"333258","messageId":"87vai2wumy.fsf@linaro.org","threadId":"47220","inReplyTo":"q7h97euiradu.fsf@orange.lip.ens-lyon.fr","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Alex Bennée","fromEmail":"alex.bennee@linaro.org","sentAt":"2017-11-22T09:05:41Z","receivedAt":"2017-11-22T09:07:44Z","isPatch":true,"sender":{"key":"alex.bennee@linaro.org","avatar":"https://avatars.githubusercontent.com/u/22458?v=4"},"body":"\nMatthieu Moy writes:\n\n> Junio C Hamano writes:\n>\n>> Was there any reason why Mail::Address was _inadequate_?\n>\n> I think the main reason was that Mail::Address is not a standard perl\n> module, and not relying on it avoided one external dependency. AFAIK, we\n> don't really have a good way to deal with Perl dependencies, so having a\n> strong requirement on Mail::Address will probably end up in a runtime\n> error for users who compile Git from source (people using a packaged\n> version have their package manager to deal with this).\n>\n>> I know we had trouble with random garbage that are *not* addresses\n>> people put on the in-body CC: trailer in the past, but I do not recall\n>> if they are something Mail::Address would give worse result and we\n>> need our workaround (hence our own substitute), or Mail::Address would\n>> handle them just fine.\n>\n> For a long time, we used Mail::Address if it was available and I don't\n> think we had issues with it.\n>\n> My point in cc907506 (\"send-email: don't use Mail::Address, even if\n> available\", 2017-08-23) was not that Mail::Address was bad, but that\n> changing our behavior depending on whether it was there or not was\n> really bad. For example, the issue dealt with in this thread probably\n> always existed, but it was present only for *some* users.\n\nSo I just did a little digging on my systems to illustrate the point. My\nwork machine is Ubuntu, so has a packaged git via PPA. It depends on\nlibmailtools-perl which includes the perl module and:\n\n apt-cache rdepends libmailtools-perl | wc -l\n 45\n\nSo for binary packaged systems it's not a big thing - libmailtools-perl\nseems to be quite widely relied on.\n\nOn my system at home, running Gentoo, while requiring \"perl\" doesn't\nexplicitly pull in the Mail::Address dependency. As a result the git\nsend-email stanza I was running at work manages to corrupt the\naddresses. If I manually install dev-perl/MailTools of course it\nsilently starts working again.\n\nGood job I never usually send patches from my home machine ;-)\n\nMy hacky guess about GIT's perl use calls is:\n\n find . -iname \"*.perl\" -or -iname \"*.pm\" -or -iname \"*.pl\" | xargs grep -h \"use .*::\" | sort | uniq | wc -l\n 88\n\nSo that is about 88 perl modules used in the code base. How many of them\nare not part of the core perl distribution?\n\nShould the solution be to just make Mail::Address a hard dependency and\nnot have the fallback?\n\n--\nAlex Bennée\n"},{"id":"333259","messageId":"20171122094945.2xgvzcdfom3yatpd@laptop","threadId":"47220","inReplyTo":"87vai2wumy.fsf@linaro.org","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Thomas Adam","fromEmail":"thomas@xteddy.org","sentAt":"2017-11-22T09:49:46Z","receivedAt":"2017-11-22T09:49:56Z","isPatch":true,"sender":{"key":"thomas@xteddy.org","avatar":"https://gravatar.com/avatar/e7256db4738e501e5d2e84f00bb0bd99503165729573848a03330301fc2adc4a?d=mp&s=160"},"body":"On Wed, Nov 22, 2017 at 09:05:41AM +0000, Alex Bennée wrote:\n> My hacky guess about GIT's perl use calls is:\n> \n> find . -iname \"*.perl\" -or -iname \"*.pm\" -or -iname \"*.pl\" | xargs grep -h \"use .*::\" | sort | uniq | wc -l\n> 88\n\nSo let us concentrate just on git-send-email.perl for now. In the\nModule::Extract::Use module (which happens to be what corelist uses), there's\nan example script called 'extract_modules' which will statically analyse a\nperl file and tell you the following information:\n\n% perl extract_modules ./git-send-email.perl\nModules required by ./git-send-email.perl:\n- Authen::SASL\n- Cwd (first released with Perl 5)\n- Email::Valid\n- Error\n- File::Spec::Functions (first released with Perl 5.00504)\n- File::Temp (first released with Perl 5.006001)\n- Getopt::Long (first released with Perl 5)\n- Git\n- Git::I18N\n- IO::Socket::SSL\n- MIME::Base64 (first released with Perl 5.007003)\n- MIME::QuotedPrint (first released with Perl 5.007003)\n- Net::Domain (first released with Perl 5.007003)\n- Net::SMTP (first released with Perl 5.007003)\n- Net::SMTP::SSL\n- POSIX (first released with Perl 5)\n- Sys::Hostname (first released with Perl 5)\n- Term::ANSIColor (first released with Perl 5.006)\n- Term::ReadLine (first released with Perl 5.002)\n- Text::ParseWords (first released with Perl 5)\n- strict (first released with Perl 5)\n- warnings (first released with Perl 5.006)\n\nTherefore, we have the following modules which are not standard:\n\n- Email::Valid\n- Error\n- Git\n- Git::I18N\n- IO::Socket::SSL\n- NET::SMTP::SSL\n\nLooking at the code for git-send-email.perl, it seems most of those are\neval()d at the point they're needed, which seems in many cases to be fallback\nresponses to something we've written, or a means of ensuring we don't need to\nexplicitly handle the case of it not being present at run-time.\n\n> Should the solution be to just make Mail::Address a hard dependency and\n> not have the fallback?\n\nThis seems like a slight on ensuring a running script which may or may not\nhave additional functionality depending on which modules are installed. Given\nthe pretty good state of packaging across those platforms which Git runs on, I\nwould argue we're now in a much better position to explicitly check for\nnon-core modules at BEGIN{} time, and moan loudly if they're not installed.\n\n-- Thomas Adam\n"},{"id":"333263","messageId":"xmqqtvxmsicm.fsf@gitster.mtv.corp.google.com","threadId":"47220","inReplyTo":"q7h97euiradu.fsf@orange.lip.ens-lyon.fr","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-22T10:44:41Z","receivedAt":"2017-11-22T10:44:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> My point in cc907506 (\"send-email: don't use Mail::Address, even if\n> available\", 2017-08-23) was not that Mail::Address was bad, but that\n> changing our behavior depending on whether it was there or not was\n> really bad. For example, the issue dealt with in this thread probably\n> always existed, but it was present only for *some* users.\n\nYes, I understand and agree with you that having a single\nimplementation would make things simpler for both users and bug\nwranglers than two sperate implementaions with two different sets of\nglitches.\n\nI am just wondering if we picked the right one back then, given this\nthread; that's all.\n"},{"id":"334610","messageId":"87mv2p89wu.fsf@linaro.org","threadId":"47220","inReplyTo":"xmqq8tezyu3g.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Alex Bennée","fromEmail":"alex.bennee@linaro.org","sentAt":"2017-12-11T17:13:53Z","receivedAt":"2017-12-11T17:14:02Z","isPatch":true,"sender":{"key":"alex.bennee@linaro.org","avatar":"https://avatars.githubusercontent.com/u/22458?v=4"},"body":"\nJunio C Hamano writes:\n\n> Thomas Adam writes:\n>\n>> Trying to come up with a reinvention of regexps for email addresses is asking\n>> for trouble, not to mention a crappy rod for your own back. Don't do that.\n>> This is why people use Mail::Address.\n>>\n>> https://metacpan.org/pod/distribution/MailTools/lib/Mail/Address.pod\n>\n> Now we are coming back to cc907506 (\"send-email: don't use\n> Mail::Address, even if available\", 2017-08-23). It argues\n>\n> * Having this optional Mail::Address makes it tempting to anwser \"please\n> install Mail::Address\" to users instead of fixing our own code. We've\n> reached the stage where bugs in our parser should be fixed, not worked\n> around.\n>\n> but if it costs us maintaining our substitute that much, it seems to\n> me that depending on Mail::Address is not just tempting but may be a\n> sensible way forward.\n>\n> Was there any reason why Mail::Address was _inadequate_? I know we\n> had trouble with random garbage that are *not* addresses people put\n> on the in-body CC: trailer in the past, but I do not recall if they\n> are something Mail::Address would give worse result and we need our\n> workaround (hence our own substitute), or Mail::Address would handle\n> them just fine.\n\nPing?\n\nSo have we come to a consensus about the best solution here?\n\nI'm perfectly happy to send a reversion patch because to be honest\nhacking on a bunch of perl to handle special mail cases is not my idea\nof fun spare time hacking ;-)\n\nI guess the full solution is to make Mail::Address a hard dependency?\n\n--\nAlex Bennée\n"},{"id":"334612","messageId":"20171211172615.jfsjthkvs4itjpcn@laptop","threadId":"47220","inReplyTo":"87mv2p89wu.fsf@linaro.org","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Thomas Adam","fromEmail":"thomas@xteddy.org","sentAt":"2017-12-11T17:26:16Z","receivedAt":"2017-12-11T17:26:24Z","isPatch":true,"sender":{"key":"thomas@xteddy.org","avatar":"https://gravatar.com/avatar/e7256db4738e501e5d2e84f00bb0bd99503165729573848a03330301fc2adc4a?d=mp&s=160"},"body":"On Mon, Dec 11, 2017 at 05:13:53PM +0000, Alex Bennée wrote:\n> So have we come to a consensus about the best solution here?\n> \n> I'm perfectly happy to send a reversion patch because to be honest\n> hacking on a bunch of perl to handle special mail cases is not my idea\n> of fun spare time hacking ;-)\n> \n> I guess the full solution is to make Mail::Address a hard dependency?\n\nThis is what I was suggesting, and then as a follow-up, addressing the point\nthat there's a bunch of require() hacks to also get around needing\nhard-dependencies.\n\n-- Thomas Adam\n"},{"id":"334621","messageId":"CACBZZX58KpQ7=V8GUFfxuMQq_Ar6cmmoXyPx_umUTbU19+0LCw@mail.gmail.com","threadId":"47220","inReplyTo":"20171211172615.jfsjthkvs4itjpcn@laptop","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-11T19:46:46Z","receivedAt":"2017-12-11T19:47:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, Dec 11, 2017 at 6:26 PM, Thomas Adam wrote:\n> On Mon, Dec 11, 2017 at 05:13:53PM +0000, Alex Bennée wrote:\n>> So have we come to a consensus about the best solution here?\n>>\n>> I'm perfectly happy to send a reversion patch because to be honest\n>> hacking on a bunch of perl to handle special mail cases is not my idea\n>> of fun spare time hacking ;-)\n>>\n>> I guess the full solution is to make Mail::Address a hard dependency?\n>\n> This is what I was suggesting, and then as a follow-up, addressing the point\n> that there's a bunch of require() hacks to also get around needing\n> hard-dependencies.\n\nI don't know what the right move is here, but just saying that this\ncould also be built on top of my \"Git::Error\" wrapper which I added in\n\"Makefile: replace perl/Makefile.PL with simple make rules\" which is\ncurrently cooking.\n\nI.e. we'd just ship a copy of Email::Valid and Mail::Address in\nperl/Git/FromCPAN/, use a wrapper to load them, and then we wouldn't\nneed to if/else this at the code level, just always use the module,\nand it would work even on core perl.\n"},{"id":"334668","messageId":"20171212103040.jbgkyet5rapqxi44@laptop","threadId":"47220","inReplyTo":"CACBZZX58KpQ7=V8GUFfxuMQq_Ar6cmmoXyPx_umUTbU19+0LCw@mail.gmail.com","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Thomas Adam","fromEmail":"thomas@xteddy.org","sentAt":"2017-12-12T10:30:43Z","receivedAt":"2017-12-12T10:30:53Z","isPatch":true,"sender":{"key":"thomas@xteddy.org","avatar":"https://gravatar.com/avatar/e7256db4738e501e5d2e84f00bb0bd99503165729573848a03330301fc2adc4a?d=mp&s=160"},"body":"Hi,\n\nOn Mon, Dec 11, 2017 at 08:46:46PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> I.e. we'd just ship a copy of Email::Valid and Mail::Address in\n> perl/Git/FromCPAN/, use a wrapper to load them, and then we wouldn't\n> need to if/else this at the code level, just always use the module,\n> and it would work even on core perl.\n\nI disagree with the premise of this, Ævar. As soon as you go down this route,\nit increases maintenance to ensure we keep up to date with what's on CPAN for\na tiny edge-case which I don't believe exists.\n\nYou may as well just use App::FatPacker.\n\nWe're talking about package maintenance here -- and as I said before, there's\nplenty of it around. For those distributions which ship Git (and hence also\npackage git-send-email), the dependencies are already there, too. I just\ncannot see this being a problem in relying on non-core perl modules. Every\nperl program does this, and they don't go down this route of having copies of\nvarious CPAN modules just in case. So why should we? We're not a special\nsnowflake.\n\n-- Thomas Adam\n"},{"id":"334675","messageId":"87po7kcgiu.fsf@evledraar.gmail.com","threadId":"47220","inReplyTo":"20171212103040.jbgkyet5rapqxi44@laptop","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-12T11:49:45Z","receivedAt":"2017-12-12T11:50:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 12 2017, Thomas Adam jotted:\n\n> Hi,\n>\n> On Mon, Dec 11, 2017 at 08:46:46PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> I.e. we'd just ship a copy of Email::Valid and Mail::Address in\n>> perl/Git/FromCPAN/, use a wrapper to load them, and then we wouldn't\n>> need to if/else this at the code level, just always use the module,\n>> and it would work even on core perl.\n>\n> I disagree with the premise of this, Ævar. As soon as you go down this route,\n> it increases maintenance to ensure we keep up to date with what's on CPAN for\n> a tiny edge-case which I don't believe exists.\n>\n> You may as well just use App::FatPacker.\n>\n> We're talking about package maintenance here -- and as I said before, there's\n> plenty of it around. For those distributions which ship Git (and hence also\n> package git-send-email), the dependencies are already there, too. I just\n> cannot see this being a problem in relying on non-core perl modules. Every\n> perl program does this, and they don't go down this route of having copies of\n> various CPAN modules just in case. So why should we? We're not a special\n> snowflake.\n\nSomething like FatPacker wouldn't make sense in this case, we're not\npacking stuff into an archive, but just dropping them during 'make\ninstall', but yes, it's the same idea of shipping our dependencies with\nus.\n\nI wouldn't argue for doing this from first principles, in general I\nthink we're way too conservative about adding dependencies to git.git,\nbut the general consensus on-list is to do that carefully, that's why we\nhave all this stuff in contrib/, and why we're depending on perl core\nonly.\n\nUsers or packagers of git don't care what's normal for perl programs, to\nthem the fact that git-send-email is written in perl is an\nimplementation detail.\n\nThe maintenance burden of just shipping some CPAN module as a fallback\nis trivial, for example we've shipped Error.pm since 2006-ish, and until\nI sent a patch this month nobody had touched it since 2013.\n\nIt's certainly much easier than maintaining a bunch of if/else code\nourselves, or maintaining our own stuff purely because we don't want to\nforce people to package perl dependencies for git.\n"},{"id":"334683","messageId":"87indb99xy.fsf@linaro.org","threadId":"47220","inReplyTo":"20171212103040.jbgkyet5rapqxi44@laptop","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Alex Bennée","fromEmail":"alex.bennee@linaro.org","sentAt":"2017-12-12T16:40:09Z","receivedAt":"2017-12-12T16:40:17Z","isPatch":true,"sender":{"key":"alex.bennee@linaro.org","avatar":"https://avatars.githubusercontent.com/u/22458?v=4"},"body":"\nThomas Adam writes:\n\n> Hi,\n>\n> On Mon, Dec 11, 2017 at 08:46:46PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> I.e. we'd just ship a copy of Email::Valid and Mail::Address in\n>> perl/Git/FromCPAN/, use a wrapper to load them, and then we wouldn't\n>> need to if/else this at the code level, just always use the module,\n>> and it would work even on core perl.\n>\n> I disagree with the premise of this, Ævar. As soon as you go down this route,\n> it increases maintenance to ensure we keep up to date with what's on CPAN for\n> a tiny edge-case which I don't believe exists.\n>\n> You may as well just use App::FatPacker.\n>\n> We're talking about package maintenance here -- and as I said before, there's\n> plenty of it around. For those distributions which ship Git (and hence also\n> package git-send-email), the dependencies are already there, too. I just\n> cannot see this being a problem in relying on non-core perl modules. Every\n> perl program does this, and they don't go down this route of having copies of\n> various CPAN modules just in case. So why should we? We're not a special\n> snowflake.\n\nI less bothered my the potentially shipping a git specific copy than\nensuring the packagers pick up the dependency when they do their builds.\nDo we already have a mechanism for testing for non-core perl modules\nduring the \"configure\" phase of git?\n\n--\nAlex Bennée\n"},{"id":"334687","messageId":"87fu8fddam.fsf@evledraar.gmail.com","threadId":"47220","inReplyTo":"87indb99xy.fsf@linaro.org","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-12T18:14:09Z","receivedAt":"2017-12-12T18:14:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 12 2017, Alex Bennée jotted:\n\n> Thomas Adam writes:\n>\n>> Hi,\n>>\n>> On Mon, Dec 11, 2017 at 08:46:46PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>>> I.e. we'd just ship a copy of Email::Valid and Mail::Address in\n>>> perl/Git/FromCPAN/, use a wrapper to load them, and then we wouldn't\n>>> need to if/else this at the code level, just always use the module,\n>>> and it would work even on core perl.\n>>\n>> I disagree with the premise of this, Ævar. As soon as you go down this route,\n>> it increases maintenance to ensure we keep up to date with what's on CPAN for\n>> a tiny edge-case which I don't believe exists.\n>>\n>> You may as well just use App::FatPacker.\n>>\n>> We're talking about package maintenance here -- and as I said before, there's\n>> plenty of it around. For those distributions which ship Git (and hence also\n>> package git-send-email), the dependencies are already there, too. I just\n>> cannot see this being a problem in relying on non-core perl modules. Every\n>> perl program does this, and they don't go down this route of having copies of\n>> various CPAN modules just in case. So why should we? We're not a special\n>> snowflake.\n>\n> I less bothered my the potentially shipping a git specific copy than\n> ensuring the packagers pick up the dependency when they do their builds.\n> Do we already have a mechanism for testing for non-core perl modules\n> during the \"configure\" phase of git?\n\nCurrent git.git master does two things:\n\n * For Error.pm we test at build time. See `git grep Error --\n 'perl/Make*'`. If you don't have Error.pm when you build we'll ship\n an old copy of it, and use that forever even if it's installed from\n CPAN afterwards.\n\n * For Mail::Address, Net::Domain etc. we don't ship the CPAN module,\n but some fallback code. We test at runtime, see `git grep\n eval.*require`. If you install the package from CPAN we'll start\n using it at your next invocation.\n\nMy \"Makefile: replace perl/Makefile.PL with simple make rules\" currently\ncooking in pu changes that so that:\n\n * We always at runtime test for the system CPAN module.\n\n * In the case of Error.pm we happen to ship a fallback, in the case of\n Mail::Address etc. we don't and have fallback code, but we could also\n just ship a copy and remove the fallback code.\n\nThis makes more sense, we always \"dynamically link\" as it were, we'll\njust change the target to (a presumably newer) system module in the case\nof Error.pm if it's found on the system, otherwise use our fallback.\n"},{"id":"334695","messageId":"xmqqwp1r20zx.fsf@gitster.mtv.corp.google.com","threadId":"47220","inReplyTo":"87fu8fddam.fsf@evledraar.gmail.com","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-12T19:35:14Z","receivedAt":"2017-12-12T19:35:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> My \"Makefile: replace perl/Makefile.PL with simple make rules\" currently\n> cooking in pu changes that so that:\n>\n> * We always at runtime test for the system CPAN module.\n>\n> * In the case of Error.pm we happen to ship a fallback, in the case of\n> Mail::Address etc. we don't and have fallback code, but we could also\n> just ship a copy and remove the fallback code.\n>\n> This makes more sense, we always \"dynamically link\" as it were, we'll\n> just change the target to (a presumably newer) system module in the case\n> of Error.pm if it's found on the system, otherwise use our fallback.\n\n\"When to fallback\" aside, I think the above makes sense for the\nsend-email simply because we would be replacing \"our own\" fallback\nwe may need to maintain forever with something with an upstream that\nwe do not have to worry too much about.\n\nA tangent; I thought I heard that use of Error.pm is strongly\ndiscouraged several years ago---am I mistaken, or if I am not,\nperhaps we should start looking into updating the users?\n\nThanks.\n"},{"id":"334716","messageId":"87d13jd4fd.fsf@evledraar.gmail.com","threadId":"47220","inReplyTo":"xmqqwp1r20zx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-12T21:25:42Z","receivedAt":"2017-12-12T21:25:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 12 2017, Junio C. Hamano jotted:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> My \"Makefile: replace perl/Makefile.PL with simple make rules\" currently\n>> cooking in pu changes that so that:\n>>\n>> * We always at runtime test for the system CPAN module.\n>>\n>> * In the case of Error.pm we happen to ship a fallback, in the case of\n>> Mail::Address etc. we don't and have fallback code, but we could also\n>> just ship a copy and remove the fallback code.\n>>\n>> This makes more sense, we always \"dynamically link\" as it were, we'll\n>> just change the target to (a presumably newer) system module in the case\n>> of Error.pm if it's found on the system, otherwise use our fallback.\n>\n> \"When to fallback\" aside, I think the above makes sense for the\n> send-email simply because we would be replacing \"our own\" fallback\n> we may need to maintain forever with something with an upstream that\n> we do not have to worry too much about.\n\nI'll see about submitting something that replaces the fallback with just\nusing the CPAN modules + bundling them once the Makefile patch has\ncooked to master.\n\n> A tangent; I thought I heard that use of Error.pm is strongly\n> discouraged several years ago---am I mistaken, or if I am not,\n> perhaps we should start looking into updating the users?\n\nI'm not a fan of it, 41c01693ac (\"git-svn: handle merge-base failures\",\n2010-01-06) shows how you can do that rather simply with just perl's\nbuilt-in exceptions.\n\nMy TODO list of \"perl stuff in git\" is now:\n\n - Get my Makefile.PL thing through\n - Make sure Dan Jacques's relocatable stuff is OK wrt perl on top of that\n - Upgrade the required version from 5.8 to 5.10\n - Update Error.pm itself, our copy is ancient\n - Add more stuff to Git::FromCPAN + remove fallbacks\n\nI could add \"rip out Error.pm\" to that, it looks rather easy, however\ngiven previous discussion about me needing to build a manpage from\nGit.pm I understand that Git.pm is used by code outside of Git itself.\n\nRipping out Error.pm for our few internal callers is one thing, trying\nto maintain bugwards compatibility with how it throws exceptions for\nusers expecting Error.pm objects is another. I think at that point it's\neasier to just stay with Error.pm.\n\nProbably easier to stay with it either way, don't poke sleeping dragons\nand all that, it's working code, even if we wouldn't write it like that\ntoday the churn probably isn't worth it.\n"},{"id":"334719","messageId":"xmqqa7ynzj14.fsf@gitster.mtv.corp.google.com","threadId":"47220","inReplyTo":"87d13jd4fd.fsf@evledraar.gmail.com","subject":"Re: [PATCH] git-send-email: fix get_maintainer.pl regression","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-12T22:19:19Z","receivedAt":"2017-12-12T22:19:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Ripping out Error.pm for our few internal callers is one thing, trying\n> to maintain bugwards compatibility with how it throws exceptions for\n> users expecting Error.pm objects is another. I think at that point it's\n> easier to just stay with Error.pm.\n>\n> Probably easier to stay with it either way, don't poke sleeping dragons\n> and all that, it's working code, even if we wouldn't write it like that\n> today the churn probably isn't worth it.\n\nOK, I'm inclined to defer to your judgment.\n\nTHanks.\n"}]}