{"thread":{"id":"25299","subject":"[PATCH 00/16] git-send-email cleanups","startedAt":"2010-09-30T13:42:53Z","lastAt":"2010-10-01T05:40:29Z","messageCount":29,"participants":["Ævar Arnfjörð Bjarmason","Brian Gernhardt","Jay Soffian","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":16},"messages":[{"id":"152135","messageId":"1285854189-10240-1-git-send-email-avarab@gmail.com","threadId":"25299","inReplyTo":null,"subject":"[PATCH 00/16] git-send-email cleanups","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-30T13:42:53Z","receivedAt":"2010-09-30T13:42:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As threatened here's a series that cleans up some of the glaring warts\nin git-send-email.\n\nÆvar Arnfjörð Bjarmason (16):\n  send-email: use lexical filehandle for opendir\n  send-email: use lexical filehandles for $compose\n  send-email: use lexical filehandles during sending\n  send-email: get_patch_subject doesn't need a prototype\n  send-email: file_declares_8bit_cte doesn't need a prototype\n  send-email: unique_email_list doesn't need a prototype\n  send-email: cleanup_compose_files doesn't need a prototype\n  send-email: use \\E***\\Q instead of \\*\\*\\*\n  send-email: sanitize_address use $foo, not \"$foo\"\n  send-email: sanitize_address use qq[\"foo\"], not \"\\\"foo\\\"\"\n  send-email: use (?:) instead of () if no  match variables are needed\n  send-email: is_rfc2047_quoted use qr// regexes\n  send-email: extract_valid_address use qr// regexes and /o\n  send-email: send_message die on $!, not $?\n  send-email: make_message_id use \"require\" instead of \"use\"\n  send-email: use Perl idioms in while loop\n\n git-send-email.perl   |   80 +++++++++++++++++++++++-------------------------\n t/t9001-send-email.sh |    2 +-\n 2 files changed, 39 insertions(+), 43 deletions(-)\n\n-- \n1.7.3.159.g610493\n"},{"id":"152136","messageId":"1285854189-10240-2-git-send-email-avarab@gmail.com","threadId":"25299","inReplyTo":"1285854189-10240-1-git-send-email-avarab@gmail.com","subject":"[PATCH 01/16] send-email: use lexical filehandle for opendir","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-30T13:42:54Z","receivedAt":"2010-09-30T13:42:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n git-send-email.perl |    6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 8cc4161..2f18d83 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -512,12 +512,12 @@ while (defined(my $f = shift @ARGV)) {\n \t\tpush @rev_list_opts, \"--\", @ARGV;\n \t\t@ARGV = ();\n \t} elsif (-d $f and !check_file_rev_conflict($f)) {\n-\t\topendir(DH,$f)\n+\t\topendir my $dh, $f\n \t\t\tor die \"Failed to opendir $f: $!\";\n \n \t\tpush @files, grep { -f $_ } map { catfile($f, $_) }\n-\t\t\t\tsort readdir(DH);\n-\t\tclosedir(DH);\n+\t\t\t\tsort readdir $dh;\n+\t\tclosedir $dh;\n \t} elsif ((-f $f or -p $f) and !check_file_rev_conflict($f)) {\n \t\tpush @files, $f;\n \t} else {\n-- \n1.7.3.159.g610493\n"},{"id":"152137","messageId":"1285854189-10240-3-git-send-email-avarab@gmail.com","threadId":"25299","inReplyTo":"1285854189-10240-1-git-send-email-avarab@gmail.com","subject":"[PATCH 02/16] send-email: use lexical filehandles for $compose","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-30T13:42:55Z","receivedAt":"2010-09-30T13:42:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n git-send-email.perl |   22 +++++++++++-----------\n 1 files changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 2f18d83..634835c 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -567,7 +567,7 @@ if ($compose) {\n \t$compose_filename = ($repo ?\n \t\ttempfile(\".gitsendemail.msg.XXXXXX\", DIR => $repo->repo_path()) :\n \t\ttempfile(\".gitsendemail.msg.XXXXXX\", DIR => \".\"))[1];\n-\topen(C,\">\",$compose_filename)\n+\topen my $c, \">\", $compose_filename\n \t\tor die \"Failed to open for writing $compose_filename: $!\";\n \n \n@@ -575,7 +575,7 @@ if ($compose) {\n \tmy $tpl_subject = $initial_subject || '';\n \tmy $tpl_reply_to = $initial_reply_to || '';\n \n-\tprint C <<EOT;\n+\tprint $c <<EOT;\n From $tpl_sender # This line is ignored.\n GIT: Lines beginning in \"GIT:\" will be removed.\n GIT: Consider including an overall diffstat or table of contents\n@@ -588,9 +588,9 @@ In-Reply-To: $tpl_reply_to\n \n EOT\n \tfor my $f (@files) {\n-\t\tprint C get_patch_subject($f);\n+\t\tprint $c get_patch_subject($f);\n \t}\n-\tclose(C);\n+\tclose $c;\n \n \tif ($annotate) {\n \t\tdo_edit($compose_filename, @files);\n@@ -598,23 +598,23 @@ EOT\n \t\tdo_edit($compose_filename);\n \t}\n \n-\topen(C2,\">\",$compose_filename . \".final\")\n+\topen my $c2, \">\", $compose_filename . \".final\"\n \t\tor die \"Failed to open $compose_filename.final : \" . $!;\n \n-\topen(C,\"<\",$compose_filename)\n+\topen $c, \"<\", $compose_filename\n \t\tor die \"Failed to open $compose_filename : \" . $!;\n \n \tmy $need_8bit_cte = file_has_nonascii($compose_filename);\n \tmy $in_body = 0;\n \tmy $summary_empty = 1;\n-\twhile(<C>) {\n+\twhile(<$c>) {\n \t\tnext if m/^GIT:/;\n \t\tif ($in_body) {\n \t\t\t$summary_empty = 0 unless (/^\\n$/);\n \t\t} elsif (/^\\n$/) {\n \t\t\t$in_body = 1;\n \t\t\tif ($need_8bit_cte) {\n-\t\t\t\tprint C2 \"MIME-Version: 1.0\\n\",\n+\t\t\t\tprint $c2 \"MIME-Version: 1.0\\n\",\n \t\t\t\t\t \"Content-Type: text/plain; \",\n \t\t\t\t\t   \"charset=UTF-8\\n\",\n \t\t\t\t\t \"Content-Transfer-Encoding: 8bit\\n\";\n@@ -639,10 +639,10 @@ EOT\n \t\t\tprint \"To/Cc/Bcc fields are not interpreted yet, they have been ignored\\n\";\n \t\t\tnext;\n \t\t}\n-\t\tprint C2 $_;\n+\t\tprint $c2 $_;\n \t}\n-\tclose(C);\n-\tclose(C2);\n+\tclose $c;\n+\tclose $c2;\n \n \tif ($summary_empty) {\n \t\tprint \"Summary email is empty, skipping it\\n\";\n-- \n1.7.3.159.g610493\n"},{"id":"152138","messageId":"1285854189-10240-4-git-send-email-avarab@gmail.com","threadId":"25299","inReplyTo":"1285854189-10240-1-git-send-email-avarab@gmail.com","subject":"[PATCH 03/16] send-email: use lexical filehandles during sending","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-30T13:42:56Z","receivedAt":"2010-09-30T13:42:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n git-send-email.perl |   14 +++++++-------\n 1 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 634835c..488d894 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1144,7 +1144,7 @@ $subject = $initial_subject;\n $message_num = 0;\n \n foreach my $t (@files) {\n-\topen(F,\"<\",$t) or die \"can't open file $t\";\n+\topen my $fh, \"<\", $t or die \"can't open file $t\";\n \n \tmy $author = undef;\n \tmy $author_encoding;\n@@ -1157,7 +1157,7 @@ foreach my $t (@files) {\n \t$message = \"\";\n \t$message_num++;\n \t# First unfold multiline header fields\n-\twhile(<F>) {\n+\twhile(<$fh>) {\n \t\tlast if /^\\s*$/;\n \t\tif (/^\\s+\\S/ and @header) {\n \t\t\tchomp($header[$#header]);\n@@ -1233,7 +1233,7 @@ foreach my $t (@files) {\n \t\t}\n \t}\n \t# Now parse the message body\n-\twhile(<F>) {\n+\twhile(<$fh>) {\n \t\t$message .=  $_;\n \t\tif (/^(Signed-off-by|Cc): (.*)$/i) {\n \t\t\tchomp;\n@@ -1250,12 +1250,12 @@ foreach my $t (@files) {\n \t\t\t\t$c, $_) unless $quiet;\n \t\t}\n \t}\n-\tclose F;\n+\tclose $fh;\n \n \tif (defined $cc_cmd && !$suppress_cc{'cccmd'}) {\n-\t\topen(F, \"$cc_cmd \\Q$t\\E |\")\n+\t\topen my $fh, \"$cc_cmd \\Q$t\\E |\"\n \t\t\tor die \"(cc-cmd) Could not execute '$cc_cmd'\";\n-\t\twhile(<F>) {\n+\t\twhile(<$fh>) {\n \t\t\tmy $c = $_;\n \t\t\t$c =~ s/^\\s*//g;\n \t\t\t$c =~ s/\\n$//g;\n@@ -1264,7 +1264,7 @@ foreach my $t (@files) {\n \t\t\tprintf(\"(cc-cmd) Adding cc: %s from: '%s'\\n\",\n \t\t\t\t$c, $cc_cmd) unless $quiet;\n \t\t}\n-\t\tclose F\n+\t\tclose $fh\n \t\t\tor die \"(cc-cmd) failed to close pipe to '$cc_cmd'\";\n \t}\n \n-- \n1.7.3.159.g610493\n"},{"id":"152139","messageId":"1285854189-10240-5-git-send-email-avarab@gmail.com","threadId":"25299","inReplyTo":"1285854189-10240-1-git-send-email-avarab@gmail.com","subject":"[PATCH 04/16] send-email: get_patch_subject doesn't need a prototype","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-30T13:42:57Z","receivedAt":"2010-09-30T13:42:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n git-send-email.perl |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 488d894..b50c963 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -549,7 +549,7 @@ if (@files) {\n \tusage();\n }\n \n-sub get_patch_subject($) {\n+sub get_patch_subject {\n \tmy $fn = shift;\n \topen (my $fh, '<', $fn);\n \twhile (my $line = <$fh>) {\n-- \n1.7.3.159.g610493\n"},{"id":"152140","messageId":"1285854189-10240-6-git-send-email-avarab@gmail.com","threadId":"25299","inReplyTo":"1285854189-10240-1-git-send-email-avarab@gmail.com","subject":"[PATCH 05/16] send-email: file_declares_8bit_cte doesn't need a prototype","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-30T13:42:58Z","receivedAt":"2010-09-30T13:42:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n git-send-email.perl |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex b50c963..f471888 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -679,7 +679,7 @@ sub ask {\n \n my %broken_encoding;\n \n-sub file_declares_8bit_cte($) {\n+sub file_declares_8bit_cte {\n \tmy $fn = shift;\n \topen (my $fh, '<', $fn);\n \twhile (my $line = <$fh>) {\n-- \n1.7.3.159.g610493\n"},{"id":"152141","messageId":"1285854189-10240-7-git-send-email-avarab@gmail.com","threadId":"25299","inReplyTo":"1285854189-10240-1-git-send-email-avarab@gmail.com","subject":"[PATCH 06/16] send-email: unique_email_list doesn't need a prototype","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-30T13:42:59Z","receivedAt":"2010-09-30T13:42:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n git-send-email.perl |    3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex f471888..90b777a 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -136,7 +136,6 @@ my $have_mail_address = eval { require Mail::Address; 1 };\n my $smtp;\n my $auth;\n \n-sub unique_email_list(@);\n sub cleanup_compose_files();\n \n # Variables we fill in automatically, or via prompting:\n@@ -1332,7 +1331,7 @@ sub cleanup_compose_files() {\n \n $smtp->quit if $smtp;\n \n-sub unique_email_list(@) {\n+sub unique_email_list {\n \tmy %seen;\n \tmy @emails;\n \n-- \n1.7.3.159.g610493\n"},{"id":"152142","messageId":"1285854189-10240-8-git-send-email-avarab@gmail.com","threadId":"25299","inReplyTo":"1285854189-10240-1-git-send-email-avarab@gmail.com","subject":"[PATCH 07/16] send-email: cleanup_compose_files doesn't need a prototype","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-30T13:43:00Z","receivedAt":"2010-09-30T13:43:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n git-send-email.perl |    4 +---\n 1 files changed, 1 insertions(+), 3 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 90b777a..ce9b5eb 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -136,8 +136,6 @@ my $have_mail_address = eval { require Mail::Address; 1 };\n my $smtp;\n my $auth;\n \n-sub cleanup_compose_files();\n-\n # Variables we fill in automatically, or via prompting:\n my (@to,$no_to,@cc,$no_cc,@initial_cc,@bcclist,$no_bcc,@xh,\n \t$initial_reply_to,$initial_subject,@files,\n@@ -1325,7 +1323,7 @@ foreach my $t (@files) {\n \n cleanup_compose_files();\n \n-sub cleanup_compose_files() {\n+sub cleanup_compose_files {\n \tunlink($compose_filename, $compose_filename . \".final\") if $compose;\n }\n \n-- \n1.7.3.159.g610493\n"},{"id":"152150","messageId":"1285854189-10240-9-git-send-email-avarab@gmail.com","threadId":"25299","inReplyTo":"1285854189-10240-1-git-send-email-avarab@gmail.com","subject":"[PATCH 08/16] send-email: use \\E***\\Q instead of \\*\\*\\*","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-30T13:43:01Z","receivedAt":"2010-09-30T13:43:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the regex introduced in a03bc5b to use the \\E...\\Q escape\nsyntax instead of using backslashes. It's more readable like this, and\neasier to grep for.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n git-send-email.perl |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex ce9b5eb..1218bbe 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -705,7 +705,7 @@ if (!defined $auto_8bit_encoding && scalar %broken_encoding) {\n \n if (!$force) {\n \tfor my $f (@files) {\n-\t\tif (get_patch_subject($f) =~ /\\*\\*\\* SUBJECT HERE \\*\\*\\*/) {\n+\t\tif (get_patch_subject($f) =~ /\\Q*** SUBJECT HERE ***\\E/) {\n \t\t\tdie \"Refusing to send because the patch\\n\\t$f\\n\"\n \t\t\t\t. \"has the template subject '*** SUBJECT HERE ***'. \"\n \t\t\t\t. \"Pass --force if you really want to send.\\n\";\n-- \n1.7.3.159.g610493\n"},{"id":"152151","messageId":"1285854189-10240-10-git-send-email-avarab@gmail.com","threadId":"25299","inReplyTo":"1285854189-10240-1-git-send-email-avarab@gmail.com","subject":"[PATCH 09/16] send-email: sanitize_address use $foo, not \"$foo\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-30T13:43:02Z","receivedAt":"2010-09-30T13:43:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"There's no reason to explicitly stringify a variable in Perl unless\nit's an overloaded object and you want to call overload::StrVal,\notherwise it's just creating a new scalar redundantly.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n git-send-email.perl |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 1218bbe..1bf090a 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -864,7 +864,7 @@ sub sanitize_address {\n \tmy ($recipient_name, $recipient_addr) = ($recipient =~ /^(.*?)\\s*(<.*)/);\n \n \tif (not $recipient_name) {\n-\t\treturn \"$recipient\";\n+\t\treturn $recipient;\n \t}\n \n \t# if recipient_name is already quoted, do nothing\n-- \n1.7.3.159.g610493\n"},{"id":"152143","messageId":"1285854189-10240-11-git-send-email-avarab@gmail.com","threadId":"25299","inReplyTo":"1285854189-10240-1-git-send-email-avarab@gmail.com","subject":"[PATCH 10/16] send-email: sanitize_address use qq[\"foo\"], not \"\\\"foo\\\"\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-30T13:43:03Z","receivedAt":"2010-09-30T13:43:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Perl provides an alternate quote syntax which can make using \"\" inside\ninterpolated strings easier to read.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n git-send-email.perl |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 1bf090a..c012b95 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -881,7 +881,7 @@ sub sanitize_address {\n \t# double quotes are needed if specials or CTLs are included\n \telsif ($recipient_name =~ /[][()<>@,;:\\\\\".\\000-\\037\\177]/) {\n \t\t$recipient_name =~ s/([\"\\\\\\r])/\\\\$1/g;\n-\t\t$recipient_name = \"\\\"$recipient_name\\\"\";\n+\t\t$recipient_name = qq[\"$recipient_name\"];\n \t}\n \n \treturn \"$recipient_name $recipient_addr\";\n-- \n1.7.3.159.g610493\n"},{"id":"152144","messageId":"1285854189-10240-12-git-send-email-avarab@gmail.com","threadId":"25299","inReplyTo":"1285854189-10240-1-git-send-email-avarab@gmail.com","subject":"[PATCH 11/16] send-email: use (?:) instead of () if no match variables are needed","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-30T13:43:04Z","receivedAt":"2010-09-30T13:43:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n git-send-email.perl |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex c012b95..5a0c4a8 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -365,7 +365,7 @@ my(%suppress_cc);\n if (@suppress_cc) {\n \tforeach my $entry (@suppress_cc) {\n \t\tdie \"Unknown --suppress-cc field: '$entry'\\n\"\n-\t\t\tunless $entry =~ /^(all|cccmd|cc|author|self|sob|body|bodycc)$/;\n+\t\t\tunless $entry =~ /^(?:all|cccmd|cc|author|self|sob|body|bodycc)$/;\n \t\t$suppress_cc{$entry} = 1;\n \t}\n }\n-- \n1.7.3.159.g610493\n"},{"id":"152146","messageId":"1285854189-10240-13-git-send-email-avarab@gmail.com","threadId":"25299","inReplyTo":"1285854189-10240-1-git-send-email-avarab@gmail.com","subject":"[PATCH 12/16] send-email: is_rfc2047_quoted use qr// regexes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-30T13:43:05Z","receivedAt":"2010-09-30T13:43:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n git-send-email.perl |    4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 5a0c4a8..b87c3f2 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -852,8 +852,8 @@ sub quote_rfc2047 {\n \n sub is_rfc2047_quoted {\n \tmy $s = shift;\n-\tmy $token = '[^][()<>@,;:\"\\/?.= \\000-\\037\\177-\\377]+';\n-\tmy $encoded_text = '[!->@-~]+';\n+\tmy $token = qr/[^][()<>@,;:\"\\/?.= \\000-\\037\\177-\\377]+/;\n+\tmy $encoded_text = qr/[!->@-~]+/;\n \tlength($s) <= 75 &&\n \t$s =~ m/^(?:\"[[:ascii:]]*\"|=\\?$token\\?$token\\?$encoded_text\\?=)$/o;\n }\n-- \n1.7.3.159.g610493\n"},{"id":"152145","messageId":"1285854189-10240-14-git-send-email-avarab@gmail.com","threadId":"25299","inReplyTo":"1285854189-10240-1-git-send-email-avarab@gmail.com","subject":"[PATCH 13/16] send-email: extract_valid_address use qr// regexes and /o","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-30T13:43:06Z","receivedAt":"2010-09-30T13:43:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the regex fragment in extract_valid_address to use the qr//\nsyntax for compiled regexes, and when they're used add a /o flag so\nthey're only compiled once for the lifetime of the program.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n git-send-email.perl |    8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex b87c3f2..47d86ad 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -777,11 +777,11 @@ our ($message_id, %mail, $subject, $reply_to, $references, $message,\n \n sub extract_valid_address {\n \tmy $address = shift;\n-\tmy $local_part_regexp = '[^<>\"\\s@]+';\n-\tmy $domain_regexp = '[^.<>\"\\s@]+(?:\\.[^.<>\"\\s@]+)+';\n+\tmy $local_part_regexp = qr/[^<>\"\\s@]+/;\n+\tmy $domain_regexp = qr/[^.<>\"\\s@]+(?:\\.[^.<>\"\\s@]+)+/;\n \n \t# check for a local address:\n-\treturn $address if ($address =~ /^($local_part_regexp)$/);\n+\treturn $address if ($address =~ /^($local_part_regexp)$/o);\n \n \t$address =~ s/^\\s*<(.*)>\\s*$/$1/;\n \tif ($have_email_valid) {\n@@ -789,7 +789,7 @@ sub extract_valid_address {\n \t} else {\n \t\t# less robust/correct than the monster regexp in Email::Valid,\n \t\t# but still does a 99% job, and one less dependency\n-\t\t$address =~ /($local_part_regexp\\@$domain_regexp)/;\n+\t\t$address =~ /($local_part_regexp\\@$domain_regexp)/o;\n \t\treturn $1;\n \t}\n }\n-- \n1.7.3.159.g610493\n"},{"id":"152147","messageId":"1285854189-10240-15-git-send-email-avarab@gmail.com","threadId":"25299","inReplyTo":"1285854189-10240-1-git-send-email-avarab@gmail.com","subject":"[PATCH 14/16] send-email: send_message die on $!, not $?","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-30T13:43:07Z","receivedAt":"2010-09-30T13:43:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"If close fails we want to emit errno, not the return code of whatever\nhappened to be the child process run.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n git-send-email.perl |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 47d86ad..9cb6aa6 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1035,7 +1035,7 @@ X-Mailer: git-send-email $gitversion\n \t\t\texec($smtp_server, @sendmail_parameters) or die $!;\n \t\t}\n \t\tprint $sm \"$header\\n$message\";\n-\t\tclose $sm or die $?;\n+\t\tclose $sm or die $!;\n \t} else {\n \n \t\tif (!defined $smtp_server) {\n-- \n1.7.3.159.g610493\n"},{"id":"152149","messageId":"1285854189-10240-16-git-send-email-avarab@gmail.com","threadId":"25299","inReplyTo":"1285854189-10240-1-git-send-email-avarab@gmail.com","subject":"[PATCH 15/16] send-email: make_message_id use \"require\" instead of \"use\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-30T13:43:08Z","receivedAt":"2010-09-30T13:43:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the use of Sys::Hostname from a \"use\" to a \"require\". The\nformer happens in an implicit BEGIN block and is thus immune from the\nif block it's contained in, so it's always loaded.\n\nThis should speed up the invocation of git-send-email by a few\nmilliseconds.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n git-send-email.perl |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 9cb6aa6..5a19d3d 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -819,7 +819,7 @@ sub make_message_id {\n \t\tlast if (defined $du_part and $du_part ne '');\n \t}\n \tif (not defined $du_part or $du_part eq '') {\n-\t\tuse Sys::Hostname qw();\n+\t\trequire Sys::Hostname;\n \t\t$du_part = 'user@' . Sys::Hostname::hostname();\n \t}\n \tmy $message_id_template = \"<%s-git-send-email-%s>\";\n-- \n1.7.3.159.g610493\n"},{"id":"152148","messageId":"1285854189-10240-17-git-send-email-avarab@gmail.com","threadId":"25299","inReplyTo":"1285854189-10240-1-git-send-email-avarab@gmail.com","subject":"[PATCH 16/16] send-email: use Perl idioms in while loop","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-30T13:43:09Z","receivedAt":"2010-09-30T13:43:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change `while(<$fh>) { my $c = $_' to `while(my $c = <$fh>) {', and\nuse `chomp $c' instead of `$c =~ s/\\n$//g;', the two are equivalent in\nthis case.\n\nI've also changed the --cccmd test so that we test for the stripping\nof whitespace at the beginning of the lines returned from the\n--cccmd. I think we probably shouldn't do this, but it was there\nalready so I haven't changed the behavior.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n git-send-email.perl   |    5 ++---\n t/t9001-send-email.sh |    2 +-\n 2 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 5a19d3d..445d2ec 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1252,10 +1252,9 @@ foreach my $t (@files) {\n \tif (defined $cc_cmd && !$suppress_cc{'cccmd'}) {\n \t\topen my $fh, \"$cc_cmd \\Q$t\\E |\"\n \t\t\tor die \"(cc-cmd) Could not execute '$cc_cmd'\";\n-\t\twhile(<$fh>) {\n-\t\t\tmy $c = $_;\n+\t\twhile(my $c = <$fh>) {\n+\t\t\tchomp $c;\n \t\t\t$c =~ s/^\\s*//g;\n-\t\t\t$c =~ s/\\n$//g;\n \t\t\tnext if ($c eq $sender and $suppress_from);\n \t\t\tpush @cc, $c;\n \t\t\tprintf(\"(cc-cmd) Adding cc: %s from: '%s'\\n\",\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 6f67da4..99a16d5 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -204,7 +204,7 @@ test_expect_success $PREREQ 'Prompting works' '\n test_expect_success $PREREQ 'cccmd works' '\n \tclean_fake_sendmail &&\n \tcp $patches cccmd.patch &&\n-\techo cccmd--cccmd@example.com >>cccmd.patch &&\n+\techo \"cccmd--  cccmd@example.com\" >>cccmd.patch &&\n \t{\n \t  echo \"#!$SHELL_PATH\"\n \t  echo sed -n -e s/^cccmd--//p \\\"\\$1\\\"\n-- \n1.7.3.159.g610493\n"},{"id":"152156","messageId":"18E0A903-D625-4C7A-A575-AC5C5EF448C9@gernhardtsoftware.com","threadId":"25299","inReplyTo":"1285854189-10240-1-git-send-email-avarab@gmail.com","subject":"Re: [PATCH 00/16] git-send-email cleanups","fromName":"Brian Gernhardt","fromEmail":"brian@gernhardtsoftware.com","sentAt":"2010-09-30T14:30:31Z","receivedAt":"2010-09-30T14:30:31Z","isPatch":true,"sender":{"key":"brian@gernhardtsoftware.com","avatar":"https://avatars.githubusercontent.com/u/133455?v=4"},"body":"\nOn Sep 30, 2010, at 9:42 AM, Ævar Arnfjörð Bjarmason wrote:\n\n>  send-email: get_patch_subject doesn't need a prototype\n>  send-email: file_declares_8bit_cte doesn't need a prototype\n>  send-email: unique_email_list doesn't need a prototype\n>  send-email: cleanup_compose_files doesn't need a prototype\n\nNone of these subroutines strictly need the prototype, but it does allow Perl to warn us if we send incorrect arguments.  Why remove them?  Are they causing problems somewhere?\n\n~~ Brian\n"},{"id":"152157","messageId":"AANLkTimi-8jotJwubC5FpNx0NfVB0zQ_+dKaeEW0y4hN@mail.gmail.com","threadId":"25299","inReplyTo":"1285854189-10240-1-git-send-email-avarab@gmail.com","subject":"Re: [PATCH 00/16] git-send-email cleanups","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2010-09-30T14:30:42Z","receivedAt":"2010-09-30T14:30:42Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Thu, Sep 30, 2010 at 9:42 AM, Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> As threatened here's a series that cleans up some of the glaring warts\n> in git-send-email.\n>\n> Ævar Arnfjörð Bjarmason (16):\n>  send-email: use lexical filehandle for opendir\n>  send-email: use lexical filehandles for $compose\n>  send-email: use lexical filehandles during sending\n>  send-email: get_patch_subject doesn't need a prototype\n>  send-email: file_declares_8bit_cte doesn't need a prototype\n>  send-email: unique_email_list doesn't need a prototype\n>  send-email: cleanup_compose_files doesn't need a prototype\n>  send-email: use \\E***\\Q instead of \\*\\*\\*\n>  send-email: sanitize_address use $foo, not \"$foo\"\n>  send-email: sanitize_address use qq[\"foo\"], not \"\\\"foo\\\"\"\n>  send-email: use (?:) instead of () if no  match variables are needed\n>  send-email: is_rfc2047_quoted use qr// regexes\n>  send-email: extract_valid_address use qr// regexes and /o\n>  send-email: send_message die on $!, not $?\n>  send-email: make_message_id use \"require\" instead of \"use\"\n>  send-email: use Perl idioms in while loop\n>\n>  git-send-email.perl   |   80 +++++++++++++++++++++++-------------------------\n>  t/t9001-send-email.sh |    2 +-\n>  2 files changed, 39 insertions(+), 43 deletions(-)\n\nThese all look pretty uncontroversial and sane by me, though I don't\nthink you touched any of the lines I contributed to git-send-email. If\nanything, perhaps this series is a bit too granular -- I might combine\nthe commits that get rid of the function prototypes into one for\nexample. Otherwise, thanks for the cleanup.\n\nAcked-by: Jay Soffian <jaysoffian@gmail.com>\n\nj.\n"},{"id":"152160","messageId":"20100930145218.GB32728@sigill.intra.peff.net","threadId":"25299","inReplyTo":"18E0A903-D625-4C7A-A575-AC5C5EF448C9@gernhardtsoftware.com","subject":"Re: [PATCH 00/16] git-send-email cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-09-30T14:52:18Z","receivedAt":"2010-09-30T14:52:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 30, 2010 at 10:30:31AM -0400, Brian Gernhardt wrote:\n\n> >  send-email: get_patch_subject doesn't need a prototype\n> >  send-email: file_declares_8bit_cte doesn't need a prototype\n> >  send-email: unique_email_list doesn't need a prototype\n> >  send-email: cleanup_compose_files doesn't need a prototype\n> \n> None of these subroutines strictly need the prototype, but it does\n> allow Perl to warn us if we send incorrect arguments.  Why remove\n> them?  Are they causing problems somewhere?\n\nThey don't necessarily do what you want:\n\n  perl -e 'sub want_scalar($) { print \"got $_[0]\\n\" }\n           want_scalar(\"ok\");\n           my @a = qw(totally broken);\n           want_scalar(@a);\n  '\n\nI get:\n\n  got ok\n  got 2\n\nAnd using \"sub want_list(@)\" basically does nothing at all (you can pass\nnothing, a scalar, or a list).\n\nFor more details, read:\n\n http://www.perlmonks.org/?node_id=861966\n\n(If you're impatient, skip to the section \"Problems with Regular\nPrototypes\").\n\n-Peff\n"},{"id":"152162","messageId":"AANLkTikDFJ8jWnuSc9U3hZCRSA971h5Zc2FfJmNiqTUN@mail.gmail.com","threadId":"25299","inReplyTo":"18E0A903-D625-4C7A-A575-AC5C5EF448C9@gernhardtsoftware.com","subject":"Re: [PATCH 00/16] git-send-email cleanups","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-30T15:11:43Z","receivedAt":"2010-09-30T15:11:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Thu, Sep 30, 2010 at 14:30, Brian Gernhardt\n<brian@gernhardtsoftware.com> wrote:\n\n> None of these subroutines strictly need the prototype, but it does\n> allow Perl to warn us if we send incorrect arguments.  Why remove\n> them?  Are they causing problems somewhere?\n\nAs Jeff pointed out prototypes are troublesome. If you want to be\nwarned about too many arguments a better way is:\n\n    sub foo {\n        warn sprintf \"You gave me %d arguments\", scalar @_ if @_ != 1;\n\nOr something like that, but there's no reason to do that for these\nsubs in particular. There are 32 subroutines in git-send-email.perl,\nthese weren't in any way more special than the rest.\n\nThey probably had prototypes in the first place because they were\nadded by someone who was under the mistaken impression that Perl\nprototypes were remotely similar to C-like prototypes, they're not.\n\nThe purpose of Perl prototypes is to rewrite the *caller* code, so\nthat e.g. if you have:\n\n    sub blah ($$) { ... }\n\nPerl will rewrite this call:\n\n    blah @foo, @bar;\n\nAs:\n\n    blah(scalar(@foo), scalar(@bar))\n\nWhile a blah without prototypes would just be:\n\n    blah(@foo, @bar);\n\nUsing prototypes superfluously like this makes it harder to read the\ncode, because you end up checking every call site for every subroutine\ncall that uses prototypes to see if rewriting the argument list like\nthis is producing some unexpected logic error.\n"},{"id":"152164","messageId":"7703AA30-C1A1-455D-9856-022588EF00B4@gernhardtsoftware.com","threadId":"25299","inReplyTo":"20100930145218.GB32728@sigill.intra.peff.net","subject":"Re: [PATCH 00/16] git-send-email cleanups","fromName":"Brian Gernhardt","fromEmail":"brian@gernhardtsoftware.com","sentAt":"2010-09-30T15:15:19Z","receivedAt":"2010-09-30T15:15:19Z","isPatch":true,"sender":{"key":"brian@gernhardtsoftware.com","avatar":"https://avatars.githubusercontent.com/u/133455?v=4"},"body":"\nOn Sep 30, 2010, at 10:52 AM, Jeff King wrote:\n\n> They don't necessarily do what you want:\n> \n>  perl -e 'sub want_scalar($) { print \"got $_[0]\\n\" }\n>           want_scalar(\"ok\");\n>           my @a = qw(totally broken);\n>           want_scalar(@a);\n>  '\n> \n> I get:\n> \n>  got ok\n>  got 2\n> \n> And using \"sub want_list(@)\" basically does nothing at all (you can pass\n> nothing, a scalar, or a list).\n\nI'm not surprised by that at all, but I've spent too much time in scripting languages recently.\n\n> For more details, read:\n> \n> http://www.perlmonks.org/?node_id=861966\n> \n> (If you're impatient, skip to the section \"Problems with Regular\n> Prototypes\").\n\nHuh.  Somewhere along the way I had picked up using prototypes as a solution to some problem and got used to them.  Interesting to see the full discussion why you shouldn't.  I guess I'm reading too much Ruby and Perl6 and didn't think about how that would work with Perl5.\n\n~~ Brian\n"},{"id":"152170","messageId":"20100930161912.GA8707@sigill.intra.peff.net","threadId":"25299","inReplyTo":"1285854189-10240-14-git-send-email-avarab@gmail.com","subject":"Re: [PATCH 13/16] send-email: extract_valid_address use qr// regexes and /o","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-09-30T16:19:12Z","receivedAt":"2010-09-30T16:19:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 30, 2010 at 01:43:06PM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> Change the regex fragment in extract_valid_address to use the qr//\n> syntax for compiled regexes, and when they're used add a /o flag so\n> they're only compiled once for the lifetime of the program.\n> [...]\n>  sub extract_valid_address {\n>  \tmy $address = shift;\n> -\tmy $local_part_regexp = '[^<>\"\\s@]+';\n> -\tmy $domain_regexp = '[^.<>\"\\s@]+(?:\\.[^.<>\"\\s@]+)+';\n> +\tmy $local_part_regexp = qr/[^<>\"\\s@]+/;\n> +\tmy $domain_regexp = qr/[^.<>\"\\s@]+(?:\\.[^.<>\"\\s@]+)+/;\n>  \n\nHmm. But these are lexical variables, so won't we recompile them each\ntime we enter the subroutine? I don't think it affects correctness, as\nthis \"/o\":\n\n> +\treturn $address if ($address =~ /^($local_part_regexp)$/o);\n\nmeans that we will compile and use the value from the first time we run\nthe function.\n\nBut we are unnecessarily compiling the sub-regexes each time. Not that\nthis is probably a performance critical piece of code, but your \"/o\" is\ndoing very little, and this is exactly the sort perl wankery that I find\ninteresting.\n\nSadly, there is no real perl equivalent of C static local variables,\nwhich is what you really want.  Usually I would do:\n\n  {\n    my $local_part_regexp = qr/.../;\n    sub extract_valid_address {\n      ...\n    }\n  }\n\nbut beware of the execution order. That works well in a module, where\nthe module code is executed before anybody calls the function. But it\nbreaks in something like this:\n\n  foo();\n  {\n    my $foo_static_local = 5;\n    sub foo {\n      print \"$foo_static_local\\n\";\n    }\n  }\n\nI think you could get by with:\n\n  {\n    my $local_part_regexp;\n    sub extract_valid_address {\n      $local_part_regexp ||= qr/.../;\n      ...\n    }\n  }\n\n-Peff\n"},{"id":"152171","messageId":"20100930162153.GB8707@sigill.intra.peff.net","threadId":"25299","inReplyTo":"1285854189-10240-1-git-send-email-avarab@gmail.com","subject":"Re: [PATCH 00/16] git-send-email cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-09-30T16:21:54Z","receivedAt":"2010-09-30T16:21:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 30, 2010 at 01:42:53PM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> As threatened here's a series that cleans up some of the glaring warts\n> in git-send-email.\n> \n> Ævar Arnfjörð Bjarmason (16):\n>   send-email: use lexical filehandle for opendir\n>   send-email: use lexical filehandles for $compose\n>   send-email: use lexical filehandles during sending\n>   send-email: get_patch_subject doesn't need a prototype\n>   send-email: file_declares_8bit_cte doesn't need a prototype\n>   send-email: unique_email_list doesn't need a prototype\n>   send-email: cleanup_compose_files doesn't need a prototype\n>   send-email: use \\E***\\Q instead of \\*\\*\\*\n>   send-email: sanitize_address use $foo, not \"$foo\"\n>   send-email: sanitize_address use qq[\"foo\"], not \"\\\"foo\\\"\"\n>   send-email: use (?:) instead of () if no  match variables are needed\n>   send-email: is_rfc2047_quoted use qr// regexes\n>   send-email: extract_valid_address use qr// regexes and /o\n>   send-email: send_message die on $!, not $?\n>   send-email: make_message_id use \"require\" instead of \"use\"\n>   send-email: use Perl idioms in while loop\n\nWith the exception of my comments on the \"/o\" regexes, these all look\nsane to me.\n\n-Peff\n"},{"id":"152172","messageId":"AANLkTimSHxU8EhuOHn77kw+ASCCeg_Bjg2or7L=G_o=M@mail.gmail.com","threadId":"25299","inReplyTo":"20100930161912.GA8707@sigill.intra.peff.net","subject":"Re: [PATCH 13/16] send-email: extract_valid_address use qr// regexes and /o","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-30T16:33:59Z","receivedAt":"2010-09-30T16:33:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Thu, Sep 30, 2010 at 16:19, Jeff King <peff@peff.net> wrote:\n> On Thu, Sep 30, 2010 at 01:43:06PM +0000, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Change the regex fragment in extract_valid_address to use the qr//\n>> syntax for compiled regexes, and when they're used add a /o flag so\n>> they're only compiled once for the lifetime of the program.\n>> [...]\n>>  sub extract_valid_address {\n>>       my $address = shift;\n>> -     my $local_part_regexp = '[^<>\"\\s@]+';\n>> -     my $domain_regexp = '[^.<>\"\\s@]+(?:\\.[^.<>\"\\s@]+)+';\n>> +     my $local_part_regexp = qr/[^<>\"\\s@]+/;\n>> +     my $domain_regexp = qr/[^.<>\"\\s@]+(?:\\.[^.<>\"\\s@]+)+/;\n>>\n>\n> Hmm. But these are lexical variables, so won't we recompile them each\n> time we enter the subroutine? I don't think it affects correctness, as\n> this \"/o\":\n>\n>> +     return $address if ($address =~ /^($local_part_regexp)$/o);\n>\n> means that we will compile and use the value from the first time we run\n> the function.\n>\n> But we are unnecessarily compiling the sub-regexes each time. Not that\n> this is probably a performance critical piece of code, but your \"/o\" is\n> doing very little, and this is exactly the sort perl wankery that I find\n> interesting.\n\nIIRC different perl versions treat this differently. In more recent\nversions doing:\n\n    perl -Mre=debug -E 'sub x { my $x = qr/foo/; my $y = qr/bar/;\n/$x$y/ } x($_) for 1..2'\n\nWill only compile all of those regexes once, since perl can see that\nthey're constant. So the /o does nothing.\n\nWe might want to keep it for self-documentation purposes to indicate\nthat the $local_part_regexp would never change, but it's probably best\nto drop that /o altogether.\n\n> Sadly, there is no real perl equivalent of C static local variables,\n> which is what you really want.  Usually I would do:\n>\n>  {\n>    my $local_part_regexp = qr/.../;\n>    sub extract_valid_address {\n>      ...\n>    }\n>  }\n>\n> but beware of the execution order. That works well in a module, where\n> the module code is executed before anybody calls the function. But it\n> breaks in something like this:\n>\n>  foo();\n>  {\n>    my $foo_static_local = 5;\n>    sub foo {\n>      print \"$foo_static_local\\n\";\n>    }\n>  }\n>\n> I think you could get by with:\n>\n>  {\n>    my $local_part_regexp;\n>    sub extract_valid_address {\n>      $local_part_regexp ||= qr/.../;\n>      ...\n>    }\n>  }\n\nPerl has static variables like that, just not in the ancient version\nwe're pinned to:\n\n    sub foo {\n        state $foo = qr/$_[0]/\n    }\n"},{"id":"152179","messageId":"7vocbfjg7x.fsf@alter.siamese.dyndns.org","threadId":"25299","inReplyTo":"20100930161912.GA8707@sigill.intra.peff.net","subject":"Re: [PATCH 13/16] send-email: extract_valid_address use qr// regexes and /o","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-09-30T17:25:54Z","receivedAt":"2010-09-30T17:25:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> ...\n> But we are unnecessarily compiling the sub-regexes each time. Not that\n> this is probably a performance critical piece of code, but your \"/o\" is\n> doing very little, and this is exactly the sort perl wankery that I find\n> interesting.\n\nWell, isn't the _sole_ point of using qr// to optimize by avoiding\nrecompilation?  If this is not a performance critical section of the code,\nwhat is the point of this change?\n\nThis [PATCH 13/16] and also [PATCH 12/16] rewrite strings using qr// but\nthe patterns thus compiled are used exactly once before the control leaves\nthe scope of the variables, so...\n\nIt is a different story if the patch instead introduced module-level\nglobal variables to hold a pre-compiled regexp objects, but that is not\nwhat we are seeing here.\n"},{"id":"152185","messageId":"AANLkTin_b6eSnHgY2YCOfN7E83gEzrOpktUD3heOnfQw@mail.gmail.com","threadId":"25299","inReplyTo":"7vocbfjg7x.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 13/16] send-email: extract_valid_address use qr// regexes and /o","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-30T18:56:17Z","receivedAt":"2010-09-30T18:56:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Thu, Sep 30, 2010 at 17:25, Junio C Hamano <gitster@pobox.com> wrote:\n> Jeff King <peff@peff.net> writes:\n>\n>> ...\n>> But we are unnecessarily compiling the sub-regexes each time. Not that\n>> this is probably a performance critical piece of code, but your \"/o\" is\n>> doing very little, and this is exactly the sort perl wankery that I find\n>> interesting.\n>\n> Well, isn't the _sole_ point of using qr// to optimize by avoiding\n> recompilation?  If this is not a performance critical section of the code,\n> what is the point of this change?\n>\n> This [PATCH 13/16] and also [PATCH 12/16] rewrite strings using qr// but\n> the patterns thus compiled are used exactly once before the control leaves\n> the scope of the variables, so...\n\nThe point is to use Perl's data-types for what they're supposed to be\nused for. In perl you *can* write:\n\n    my $two = \"1\" + \"1\";\n\nTo get 2, but that's silly. You should just use integers instead of\nstring coercion:\n\n    my $two = 1 + 1;\n\nSimilarly you *can* put regexes in strings, but perl has a first-class\ndatatype for it, so you should do:\n\n    my $rx = qr/foo/;\n\nNot:\n\n    my $rx = \"foo\";\n\nSo it's just a change to use a better style. There are some tangible\nadvantages to using qr// over qq// (also known as \"\"), e.g. Emacs and\nVim will know to highlight the string as a regexp, and when the $rx is\ninterpolated Perl won't need to recompile it because it can just add a\npointer to the existing regex in the new pattern.\n\nBut the main reason here is to not write code that looks like it\ntargets Perl 5.5, but 5.8.\n"},{"id":"152187","messageId":"1285873411-14045-1-git-send-email-avarab@gmail.com","threadId":"25299","inReplyTo":"1285854189-10240-14-git-send-email-avarab@gmail.com","subject":"[PATCH v2 13/16] send-email: extract_valid_address use qr// regexes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-30T19:03:31Z","receivedAt":"2010-09-30T19:03:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nHere's v2 which omits the confusing /o addition and just uses the more\ncorrect qr// syntax over qq//.\n\n git-send-email.perl |    4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex b87c3f2..30000b9 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -777,8 +777,8 @@ our ($message_id, %mail, $subject, $reply_to, $references, $message,\n \n sub extract_valid_address {\n \tmy $address = shift;\n-\tmy $local_part_regexp = '[^<>\"\\s@]+';\n-\tmy $domain_regexp = '[^.<>\"\\s@]+(?:\\.[^.<>\"\\s@]+)+';\n+\tmy $local_part_regexp = qr/[^<>\"\\s@]+/;\n+\tmy $domain_regexp = qr/[^.<>\"\\s@]+(?:\\.[^.<>\"\\s@]+)+/;\n \n \t# check for a local address:\n \treturn $address if ($address =~ /^($local_part_regexp)$/);\n-- \n1.7.3.159.g610493\n"},{"id":"152216","messageId":"20101001054029.GA18674@sigill.intra.peff.net","threadId":"25299","inReplyTo":"AANLkTimSHxU8EhuOHn77kw+ASCCeg_Bjg2or7L=G_o=M@mail.gmail.com","subject":"Re: [PATCH 13/16] send-email: extract_valid_address use qr// regexes and /o","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-10-01T05:40:29Z","receivedAt":"2010-10-01T05:40:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 30, 2010 at 04:33:59PM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> > But we are unnecessarily compiling the sub-regexes each time. Not that\n> > this is probably a performance critical piece of code, but your \"/o\" is\n> > doing very little, and this is exactly the sort perl wankery that I find\n> > interesting.\n> \n> IIRC different perl versions treat this differently. In more recent\n> versions doing:\n> \n>     perl -Mre=debug -E 'sub x { my $x = qr/foo/; my $y = qr/bar/;\n> /$x$y/ } x($_) for 1..2'\n\nAh, right. I am used to using qr// with variable input (since that is\nthe intended use), but of course perl is smart enough to just need to\ncompile qr/constant/ once.\n\n> Will only compile all of those regexes once, since perl can see that\n> they're constant. So the /o does nothing.\n\nNice. I didn't think it would be smart enough to handle the layer of\nindirection, but obviously it does. Go perl.\n\n> We might want to keep it for self-documentation purposes to indicate\n> that the $local_part_regexp would never change, but it's probably best\n> to drop that /o altogether.\n\nYeah, I would just drop the /o. It will in all probability never change,\nbut it it _did_, I think having the /o just makes it buggy.\n\n> Perl has static variables like that, just not in the ancient version\n> we're pinned to:\n> \n>     sub foo {\n>         state $foo = qr/$_[0]/\n>     }\n\nNice, I obviously have not kept up on my perl-changelog-reading. ;) But\nyeah, it's not in 5.8. It's a moot point anyway, since as you\ndemonstrated above, perl is already doing the optimization.\n\n-Peff\n"}]}