{"thread":{"id":"19119","subject":"[PATCH 0/6] cleanups for git-send-email","startedAt":"2009-04-29T13:12:17Z","lastAt":"2009-05-04T07:41:08Z","messageCount":24,"participants":["Bill Pemberton","Nicolas Sebrecht","Junio C Hamano","Andreas Ericsson","Jeff King","Francis Galiegue","Jay Soffian","H.Merijn Brand"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"112626","messageId":"1241010743-7020-1-git-send-email-wfp5p@virginia.edu","threadId":"19119","inReplyTo":null,"subject":"[PATCH 0/6] cleanups for git-send-email","fromName":"Bill Pemberton","fromEmail":"wfp5p@virginia.edu","sentAt":"2009-04-29T13:12:17Z","receivedAt":"2009-04-29T13:12:17Z","isPatch":true,"sender":{"key":"wfp5p@virginia.edu","avatar":null},"body":"The following are some code cleanups for git-send-email.perl. They're\nbased on suggestions by perlcritc.\n\nBill Pemberton (6):\n Remove return undef from validate_patch\n Remove function prototypes from git-send-email.perl\n Remove return undef from ask()\n Add explict return to end of subroutines\n Remove mix of high and low-precedence booleans\n Remove bareword filehandles in git-send-email.perl\n\n git-send-email.perl | 63 ++++++++++++++++++++++++++------------------------\n 1 files changed, 33 insertions(+), 30 deletions(-)\n"},{"id":"112629","messageId":"1241010743-7020-2-git-send-email-wfp5p@virginia.edu","threadId":"19119","inReplyTo":"1241010743-7020-1-git-send-email-wfp5p@virginia.edu","subject":"[PATCH 1/6] Remove return undef from validate_patch","fromName":"Bill Pemberton","fromEmail":"wfp5p@virginia.edu","sentAt":"2009-04-29T13:12:18Z","receivedAt":"2009-04-29T13:12:18Z","isPatch":true,"sender":{"key":"wfp5p@virginia.edu","avatar":null},"body":"Returning undef is rarely the correct way to return a failure.\nReplace it with return 0\n\nSigned-off-by: Bill Pemberton \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 cccbf45..4f62c59 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1182,7 +1182,7 @@ sub validate_patch {\n \t\t\treturn \"$.: patch contains a line longer than 998 characters\";\n \t\t}\n \t}\n-\treturn undef;\n+\treturn 0;\n }\n \n sub file_has_nonascii {\n-- \n1.6.0.6\n"},{"id":"112628","messageId":"1241010743-7020-3-git-send-email-wfp5p@virginia.edu","threadId":"19119","inReplyTo":"1241010743-7020-2-git-send-email-wfp5p@virginia.edu","subject":"[PATCH 2/6] Remove function prototypes from git-send-email.perl","fromName":"Bill Pemberton","fromEmail":"wfp5p@virginia.edu","sentAt":"2009-04-29T13:12:19Z","receivedAt":"2009-04-29T13:12:19Z","isPatch":true,"sender":{"key":"wfp5p@virginia.edu","avatar":null},"body":"Use of function prototypes is considered bad practice in perl. The\nones used here didn't accomplish anything anyhow, so they've been\nremoved.\n\nSigned-off-by: Bill Pemberton \n---\n git-send-email.perl | 11 ++++-------\n 1 files changed, 4 insertions(+), 7 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 4f62c59..067aaf0 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -131,9 +131,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 my (@to,@cc,@initial_cc,@bcclist,@xh,\n \t$initial_reply_to,$initial_subject,@files,\n@@ -443,7 +440,7 @@ if (@alias_files and $aliasfiletype and defined $parse_alias{$aliasfiletype}) {\n ($sender) = expand_aliases($sender) if defined $sender;\n \n # returns 1 if the conflict must be solved using it as a format-patch argument\n-sub check_file_rev_conflict($) {\n+sub check_file_rev_conflict {\n \treturn unless $repo;\n \tmy $f = shift;\n \ttry {\n@@ -509,7 +506,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@@ -1150,13 +1147,13 @@ 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 $smtp->quit if $smtp;\n \n-sub unique_email_list(@) {\n+sub unique_email_list {\n \tmy %seen;\n \tmy @emails;\n \n-- \n1.6.0.6\n"},{"id":"112630","messageId":"1241010743-7020-4-git-send-email-wfp5p@virginia.edu","threadId":"19119","inReplyTo":"1241010743-7020-3-git-send-email-wfp5p@virginia.edu","subject":"[PATCH 3/6] Remove return undef from ask()","fromName":"Bill Pemberton","fromEmail":"wfp5p@virginia.edu","sentAt":"2009-04-29T13:12:20Z","receivedAt":"2009-04-29T13:12:20Z","isPatch":true,"sender":{"key":"wfp5p@virginia.edu","avatar":null},"body":"Returning undef is rarely the correct way to return a failure.\nReplace it with bare return\n\nSigned-off-by: Bill Pemberton \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 067aaf0..1ed5869 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -633,7 +633,7 @@ sub ask {\n \t\t\treturn $resp;\n \t\t}\n \t}\n-\treturn undef;\n+\treturn;\n }\n \n my $prompting = 0;\n-- \n1.6.0.6\n"},{"id":"112627","messageId":"1241010743-7020-5-git-send-email-wfp5p@virginia.edu","threadId":"19119","inReplyTo":"1241010743-7020-4-git-send-email-wfp5p@virginia.edu","subject":"[PATCH 4/6] Add explict return to end of subroutines","fromName":"Bill Pemberton","fromEmail":"wfp5p@virginia.edu","sentAt":"2009-04-29T13:12:21Z","receivedAt":"2009-04-29T13:12:21Z","isPatch":true,"sender":{"key":"wfp5p@virginia.edu","avatar":null},"body":"In perl a subroutine that ends without an explicit return will return\nthe value of the last expression evalutated. This can lead to\nunexpected return values.\n\nSigned-off-by: Bill Pemberton \n---\n git-send-email.perl | 6 ++++++\n 1 files changed, 6 insertions(+), 0 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 1ed5869..c24e0df 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -160,6 +160,7 @@ my $compose_filename;\n # Handle interactive edition of files.\n my $multiedit;\n my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, \"core.editor\") || $ENV{VISUAL} || $ENV{EDITOR} || \"vi\";\n+\n sub do_edit {\n \tif (defined($multiedit) && !$multiedit) {\n \t\tmap {\n@@ -174,6 +175,7 @@ sub do_edit {\n \t\t\tdie(\"the editor exited uncleanly, aborting everything\");\n \t\t}\n \t}\n+ return;\n }\n \n # Variables with corresponding config settings\n@@ -304,6 +306,7 @@ sub read_config {\n \t\t\t$smtp_encryption = 'ssl';\n \t\t}\n \t}\n+ return;\n }\n \n # read configuration from [sendemail \"$identity\"], fall back on [sendemail]\n@@ -745,6 +748,7 @@ sub make_message_id\n \tmy $message_id_template = \"<%s-git-send-email-%s>\";\n \t$message_id = sprintf($message_id_template, $uniq, $du_part);\n \t#print \"new message id = $message_id\\n\"; # Was useful for debugging\n+ return;\n }\n \n \n@@ -971,6 +975,7 @@ X-Mailer: git-send-email $gitversion\n \t\t\tprint \"Result: OK\\n\";\n \t\t}\n \t}\n+ return;\n }\n \n $reply_to = $initial_reply_to;\n@@ -1149,6 +1154,7 @@ cleanup_compose_files();\n \n sub cleanup_compose_files {\n \tunlink($compose_filename, $compose_filename . \".final\") if $compose;\n+ return;\n }\n \n $smtp->quit if $smtp;\n-- \n1.6.0.6\n"},{"id":"112632","messageId":"1241010743-7020-6-git-send-email-wfp5p@virginia.edu","threadId":"19119","inReplyTo":"1241010743-7020-5-git-send-email-wfp5p@virginia.edu","subject":"[PATCH 5/6] Remove mix of high and low-precedence booleans","fromName":"Bill Pemberton","fromEmail":"wfp5p@virginia.edu","sentAt":"2009-04-29T13:12:22Z","receivedAt":"2009-04-29T13:12:22Z","isPatch":true,"sender":{"key":"wfp5p@virginia.edu","avatar":null},"body":"Booleans such as &&, ||, ! have higher precedence than and, or, not.\nThey should not be mixed.\n\nSigned-off-by: Bill Pemberton \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 c24e0df..5e7295d 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -471,14 +471,14 @@ while (defined(my $f = shift @ARGV)) {\n \tif ($f eq \"--\") {\n \t\tpush @rev_list_opts, \"--\", @ARGV;\n \t\t@ARGV = ();\n-\t} elsif (-d $f and !check_file_rev_conflict($f)) {\n+\t} elsif (-d $f && !check_file_rev_conflict($f)) {\n \t\topendir(DH,$f)\n \t\t\tor die \"Failed to opendir $f: $!\";\n \n \t\tpush @files, grep { -f $_ } map { +$f . \"/\" . $_ }\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} elsif ((-f $f || -p $f) && !check_file_rev_conflict($f)) {\n \t\tpush @files, $f;\n \t} else {\n \t\tpush @rev_list_opts, $f;\n@@ -632,7 +632,7 @@ sub ask {\n \t\tif ($resp eq '' and defined $default) {\n \t\t\treturn $default;\n \t\t}\n-\t\tif (!defined $valid_re or $resp =~ /$valid_re/) {\n+\t\tif (!defined $valid_re || $resp =~ /$valid_re/) {\n \t\t\treturn $resp;\n \t\t}\n \t}\n-- \n1.6.0.6\n"},{"id":"112631","messageId":"1241010743-7020-7-git-send-email-wfp5p@virginia.edu","threadId":"19119","inReplyTo":"1241010743-7020-6-git-send-email-wfp5p@virginia.edu","subject":"[PATCH 6/6] Remove bareword filehandles in git-send-email.perl","fromName":"Bill Pemberton","fromEmail":"wfp5p@virginia.edu","sentAt":"2009-04-29T13:12:23Z","receivedAt":"2009-04-29T13:12:23Z","isPatch":true,"sender":{"key":"wfp5p@virginia.edu","avatar":null},"body":"The script was using bareword filehandles. This is considered a bad\npractice so they have been changed to indirect filehandles.\n\nSigned-off-by: Bill Pemberton \n---\n git-send-email.perl | 36 ++++++++++++++++++------------------\n 1 files changed, 18 insertions(+), 18 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 5e7295d..7068041 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -527,7 +527,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@@ -535,7 +535,7 @@ if ($compose) {\n \tmy $tpl_subject = $initial_subject || '';\n \tmy $tpl_reply_to = $initial_reply_to || '';\n \n-\tprint C <\",$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() {\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@@ -601,10 +601,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@@ -984,7 +984,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 $F,'<',$t or die \"can't open file $t\";\n \n \tmy $author = undef;\n \tmy $author_encoding;\n@@ -997,7 +997,7 @@ foreach my $t (@files) {\n \t$message = \"\";\n \t$message_num++;\n \t# First unfold multiline header fields\n-\twhile() {\n+\twhile(<$F>) {\n \t\tlast if /^\\s*$/;\n \t\tif (/^\\s+\\S/ and @header) {\n \t\t\tchomp($header[$#header]);\n@@ -1073,7 +1073,7 @@ foreach my $t (@files) {\n \t\t}\n \t}\n \t# Now parse the message body\n-\twhile() {\n+\twhile(<$F>) {\n \t\t$message .= $_;\n \t\tif (/^(Signed-off-by|Cc): (.*)$/i) {\n \t\t\tchomp;\n@@ -1090,12 +1090,12 @@ foreach my $t (@files) {\n \t\t\t\t$c, $_) unless $quiet;\n \t\t}\n \t}\n-\tclose F;\n+\tclose $F;\n \n \tif (defined $cc_cmd && !$suppress_cc{'cccmd'}) {\n-\t\topen(F, \"$cc_cmd $t |\")\n+\t\topen my $F, '-|', \"$cc_cmd $t\"\n \t\t\tor die \"(cc-cmd) Could not execute '$cc_cmd'\";\n-\t\twhile() {\n+\t\twhile(<$F>) {\n \t\t\tmy $c = $_;\n \t\t\t$c =~ s/^\\s*//g;\n \t\t\t$c =~ s/\\n$//g;\n@@ -1104,7 +1104,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 $F\n \t\t\tor die \"(cc-cmd) failed to close pipe to '$cc_cmd'\";\n \t}\n \n-- \n1.6.0.6\n"},{"id":"112650","messageId":"20090429165407.GB12908@vidovic","threadId":"19119","inReplyTo":"1241010743-7020-6-git-send-email-wfp5p@virginia.edu","subject":"[PATCH 5/6] Re: Remove mix of high and low-precedence booleans","fromName":"Nicolas Sebrecht","fromEmail":"nicolas.s.dev@gmail.com","sentAt":"2009-04-29T16:54:07Z","receivedAt":"2009-04-29T16:54:07Z","isPatch":true,"sender":{"key":"nicolas.s.dev@gmail.com","avatar":null},"body":"On Wed, Apr 29, 2009 at 09:12:22AM -0400, Bill Pemberton wrote:\n\n> Booleans such as &&, ||, ! have higher precedence than and, or, not.\n> They should not be mixed.\n\nBut round brackets have higher precedence than both '&&' and 'and',\nright? If so, why thoses changes?\n\n> -\t} elsif (-d $f and !check_file_rev_conflict($f)) {\n> +\t} elsif (-d $f && !check_file_rev_conflict($f)) {\n\n> -\t} elsif ((-f $f or -p $f) and !check_file_rev_conflict($f)) {\n> +\t} elsif ((-f $f || -p $f) && !check_file_rev_conflict($f)) {\n\n> -\t\tif (!defined $valid_re or $resp =~ /$valid_re/) {\n> +\t\tif (!defined $valid_re || $resp =~ /$valid_re/) {\n\n-- \nNicolas Sebrecht\n"},{"id":"112651","messageId":"20090429170032.59F0B57034@viridian.itc.Virginia.EDU","threadId":"19119","inReplyTo":"20090429165407.GB12908@vidovic","subject":"Re: [PATCH 5/6] Re: Remove mix of high and low-precedence booleans","fromName":"Bill Pemberton","fromEmail":"wfp5p@viridian.itc.virginia.edu","sentAt":"2009-04-29T17:00:32Z","receivedAt":"2009-04-29T17:00:32Z","isPatch":true,"sender":{"key":"wfp5p@viridian.itc.virginia.edu","avatar":null},"body":"> > Booleans such as &&, ||, ! have higher precedence than and, or, not.\n> > They should not be mixed.\n> \n> But round brackets have higher precedence than both '&&' and 'and',\n> right? If so, why thoses changes?\n> \n\nWhile those particular statements may be correct, mixing the low\nprecedence logical operators with the high precedence ones is likely\nto introduce bugs down the road. See the book \"Perl Best Practices\"\nfor more information on this.\n\n-- \nBill\n"},{"id":"112673","messageId":"7vws939skl.fsf@gitster.siamese.dyndns.org","threadId":"19119","inReplyTo":"1241010743-7020-1-git-send-email-wfp5p@virginia.edu","subject":"Re: [PATCH 0/6] cleanups for git-send-email","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-29T19:18:50Z","receivedAt":"2009-04-29T19:18:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bill Pemberton writes:\n\n> The following are some code cleanups for git-send-email.perl. They're\n> based on suggestions by perlcritc.\n>\n> Bill Pemberton (6):\n> Remove return undef from validate_patch\n> Remove function prototypes from git-send-email.perl\n> Remove return undef from ask()\n> Add explict return to end of subroutines\n> Remove mix of high and low-precedence booleans\n> Remove bareword filehandles in git-send-email.perl\n\nPerl styles are highly personal.\n\nWhile I admit that the changes these patches bring in match my personal\ntaste more or less exactly [*1*], there was somebody (sorry I lost track)\nwho volunteered to take the maintenance responsibility of that program\nover to clean up or even rewrite it, so I would rather have that person\nhave a say in the overall styles of the (rewritten) program.\n\n[Footnote]\n\n*1* ...except for the \"and/or vs &&/||\" bits, even though I prefer the\nlatter myself solely because I am old fashioned.\n\nI think it is simply silly to say \"precedence of ! and and/or does not\nmix\". \"!\" and \"&&\" have different precedence and rewriting (A and !B)\ninto (A && !B) would not make things any better nor worse. After all,\nnobody would have problems with \"$a + $b * $c\" even though + and * have\ndifferent precedence.\n\nOh, I also do not agree with \"always explicitly return\". If the change\nand explanation were limited to the subs whose return values are _used_, I\nwould agree with the change, though.\n"},{"id":"112676","messageId":"20090429194852.0976257034@viridian.itc.Virginia.EDU","threadId":"19119","inReplyTo":"7vws939skl.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 0/6] cleanups for git-send-email","fromName":"Bill Pemberton","fromEmail":"wfp5p@viridian.itc.virginia.edu","sentAt":"2009-04-29T19:48:51Z","receivedAt":"2009-04-29T19:48:51Z","isPatch":true,"sender":{"key":"wfp5p@viridian.itc.virginia.edu","avatar":null},"body":"> Perl styles are highly personal.\n> \n\nSo are C styles, but the kernel and git doesn't allow all sorts of\nmixed styles. My changes are also not just coding style, they have\nactual meaning in perl.\n\nMy changes come directly from the book \"Perl Best Practices\". Just as\nyou do things like \"don't allow assignment in conditionals\" in C, even\nthough it's legal. There are good reasons to do these things in perl\nto prevent bugs down the road.\n\n> \n> *1* ...except for the \"and/or vs &&/||\" bits, even though I prefer the\n> latter myself solely because I am old fashioned.\n> \n\nAgain, it prevents bugs. People use \"and\" vs \"&&\" as the same thing,\nwhen they are not. The have different precedence in perl.\n\nFor example, \n\nnext if not $finished || $x < 5;\nnext if !$finished || $x < 5;\n\ndo not mean the same thing.\n\n\n> I think it is simply silly to say \"precedence of ! and and/or does not\n> mix\". \"!\" and \"&&\" have different precedence and rewriting (A and !B)\n> into (A && !B) would not make things any better nor worse. After all,\n> nobody would have problems with \"$a + $b * $c\" even though + and * have\n> different precedence.\n> \n\nIt's not that ! and && have different precedence. It's that \"not\" and\n! have different precedence. Using your math example, it would be\nlike having an operator named plus that had a higher precedence than\n\"*\". Now if you wrote \"$a plus $b * $c\" it would have different\nresult than \"$a + $b * $c\".\n\n\n> Oh, I also do not agree with \"always explicitly return\". If the change\n> and explanation were limited to the subs whose return values are _used_, I\n> would agree with the change, though.\n> \n\nAgain, it prevents potential bugs down the road. Currently those\nfunctions return something. While they are not used, the something\nthey return can be interpreted by developers as an intentional return\nvalue and that property may get used. If some other developer changes\nthe original function in some way that the implicit return becomes\nsomething else, it'll create a bug. If a subroutine isn't supposed to\nreturn a meaningful value, it should do it explicitly.\n\n\n-- \nBill Pemberton wfp5p@virginia.edu\nITC/Unix Systems flash@virginia.edu\nUniversity of Virginia \n"},{"id":"112685","messageId":"7v4ow79pk6.fsf@gitster.siamese.dyndns.org","threadId":"19119","inReplyTo":"20090429194852.0976257034@viridian.itc.Virginia.EDU","subject":"Re: [PATCH 0/6] cleanups for git-send-email","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-29T20:23:53Z","receivedAt":"2009-04-29T20:23:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"wfp5p@viridian.itc.Virginia.EDU (Bill Pemberton) writes:\n\n> My changes come directly from the book \"Perl Best Practices\". Just as\n> ...\n> Again, it prevents bugs. People use \"and\" vs \"&&\" as the same thing,\n> when they are not. The have different precedence in perl.\n>\n> For example, \n>\n> next if not $finished || $x < 5;\n> next if !$finished || $x < 5;\n>\n> do not mean the same thing.\n> ...\n> Again, it prevents potential bugs down the road....\n\nEarlier I did guide the community not to use \"more advanced\" (aka\n\"obscure\") Perl features so that people not so familiar with Perl can\nstill tweak scripts without breaking them; the tricks in your patches that\n\"prevent potential bugs\" are in line with that, and that is why I said my\npersonal taste more or less agrees with your patch already.\n\nBut the line between \"more advanced and tricky\" and \"if you are coding in\nPerl you should know your language\" is not so black and white as you seem\nto think. I'd rather defer that decision to whoever is taking send-email\nover.\n"},{"id":"112713","messageId":"20090429222711.GC12908@vidovic","threadId":"19119","inReplyTo":"20090429194852.0976257034@viridian.itc.Virginia.EDU","subject":"[PATCH 0/6] Re: cleanups for git-send-email","fromName":"Nicolas Sebrecht","fromEmail":"nicolas.s.dev@gmail.com","sentAt":"2009-04-29T22:27:11Z","receivedAt":"2009-04-29T22:27:11Z","isPatch":true,"sender":{"key":"nicolas.s.dev@gmail.com","avatar":null},"body":"On Wed, Apr 29, 2009 at 03:48:51PM -0400, Bill Pemberton wrote:\n\n> Again, it prevents bugs. People use \"and\" vs \"&&\" as the same thing,\n> when they are not. The have different precedence in perl.\n\nI agree with you except that the chapter 4.16 from the Perl Best\nPractices book does not apply here. FMPOV, we don't really mix booleans\nbecause the precedence is explicitly given by the parentheses.\n\n[ Notice _how_ the author raises the ambiguity to explain his point in\n the book: he uses parentheses. ]\n\n> For example, \n> \n> next if not $finished || $x < 5;\n> next if !$finished || $x < 5;\n> \n> do not mean the same thing.\n\nTrue. But the lines we are talking about are different. We have:\n\n\tnext if ($finished or $x < 5);\n\nIf we add a \"not\"/\"!\" or append a \"&&\"/\"and\" - or whatever -, we do know what will\nbe evaluated easily:\n\n\tnext if !($finished or $x < 5);\n\nlooks rather different from\n\n\tnext if (!$finished or $x < 5);\n\n-- \nNicolas Sebrecht\n"},{"id":"112743","messageId":"49F95D23.3050101@op5.se","threadId":"19119","inReplyTo":"20090429222711.GC12908@vidovic","subject":"Re: [PATCH 0/6] Re: cleanups for git-send-email","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2009-04-30T08:11:15Z","receivedAt":"2009-04-30T08:11:15Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Nicolas Sebrecht wrote:\n> On Wed, Apr 29, 2009 at 03:48:51PM -0400, Bill Pemberton wrote:\n> \n>> Again, it prevents bugs. People use \"and\" vs \"&&\" as the same thing,\n>> when they are not. The have different precedence in perl.\n> \n> I agree with you except that the chapter 4.16 from the Perl Best\n> Practices book does not apply here. FMPOV, we don't really mix booleans\n> because the precedence is explicitly given by the parentheses.\n> \n> [ Notice _how_ the author raises the ambiguity to explain his point in\n> the book: he uses parentheses. ]\n> \n>> For example, \n>>\n>> next if not $finished || $x < 5;\n>> next if !$finished || $x < 5;\n>>\n>> do not mean the same thing.\n> \n> True. But the lines we are talking about are different. We have:\n> \n> \tnext if ($finished or $x < 5);\n> \n> If we add a \"not\"/\"!\" or append a \"&&\"/\"and\" - or whatever -, we do know what will\n> be evaluated easily:\n> \n> \tnext if !($finished or $x < 5);\n> \n> looks rather different from\n> \n> \tnext if (!$finished or $x < 5);\n> \n\nI'm rather clueless when it comes to perl coding, but I know what I\ndon't know, so I've got enough sense to look these things up whenever\nI have to hack some perl. I hope others do the same.\n\nPersonally, I've found that never using 'or', 'not' or 'and', and\noverparenthesize when I'm uncertain seems to work rather nicely,\neven though perl gurus would probably shunt my code as overly explicit.\n\n-- \nAndreas Ericsson andreas.ericsson@op5.se\nOP5 AB www.op5.se\nTel: +46 8-230225 Fax: +46 8-230231\n\nRegister now for Nordic Meet on Nagios, June 3-4 in Stockholm\n http://nordicmeetonnagios.op5.org/\n\nConsidering the successes of the wars on alcohol, poverty, drugs and\nterror, I think we should give some serious thought to declaring war\non peace.\n"},{"id":"112930","messageId":"20090503194600.GB20468@coredump.intra.peff.net","threadId":"19119","inReplyTo":"1241010743-7020-2-git-send-email-wfp5p@virginia.edu","subject":"Re: [PATCH 1/6] Remove return undef from validate_patch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-05-03T19:46:00Z","receivedAt":"2009-05-03T19:46:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 29, 2009 at 09:12:18AM -0400, Bill Pemberton wrote:\n\n> Returning undef is rarely the correct way to return a failure.\n> Replace it with return 0\n\nNo, it's the right way to return failure here. The function returns\neither an error string, prefixed with the problematic line number, or\nundef. So 'undef' is working as a sentinel value here, not as part of a\nboolean.\n\nThat being said, the _calling_ code is a bit sloppy in checking \"$error\"\ninstead of \"defined($error)\". It is not an actual bug because the\nbeginning of the string is always a line number >= 1, so it always\ntriggers as desired. However, it would probably be more clear to write\nit like this:\n\n---\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex cccbf45..168b2c2 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -495,7 +495,7 @@ if ($validate) {\n \tforeach my $f (@files) {\n \t\tunless (-p $f) {\n \t\t\tmy $error = validate_patch($f);\n-\t\t\t$error and die \"fatal: $f: $error\\nwarning: no patches were sent\\n\";\n+\t\t\tdefined($error) and die \"fatal: $f: $error\\nwarning: no patches were sent\\n\";\n \t\t}\n \t}\n }\n"},{"id":"112931","messageId":"20090503202625.GC20468@coredump.intra.peff.net","threadId":"19119","inReplyTo":"1241010743-7020-4-git-send-email-wfp5p@virginia.edu","subject":"Re: [PATCH 3/6] Remove return undef from ask()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-05-03T20:26:25Z","receivedAt":"2009-05-03T20:26:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 29, 2009 at 09:12:20AM -0400, Bill Pemberton wrote:\n\n> Returning undef is rarely the correct way to return a failure.\n> Replace it with bare return\n\nI'm not sure this makes sense. The function is meant to be (and is\nalways) called in a scalar context, so \"return\" and \"return undef\"\nproduce exactly the same results. I find the latter much more clear,\nbecause it is explicit about what the function is doing: return the\nuser's desire; if that fails, return the default; if no default, return\nundef.\n\nReturning undef can be a problem for programs which are meant to be used\nin list context; you generally want to return the empty list in that\ncase (not a list with a single undef in it). So that is the reason for\nthe advice you are quoting.\n\nI am not sure it is worth making this function behave properly in a list\ncontext; it is not a library function, so we can see that all of the\nexisting callers use it in a scalar context (and we would have to define\nsensible semantics for a list context). But even if we _did_ want to do\nthat, your patch is only the first step. You would have to also fix the\nother returns which can return undef instead of an empty list. So there\nis not much point to your patch as it stands.\n\nOn a side note, while looking at this function, I wonder if that \"return\nundef\" is correct after all. We get there only if the user has failed to\ngive valid input 10 times, so presumably it is a sanity check to\nprevent runaway input errors (and I am cc'ing Jay, who added the\nfunction not too long ago). Should we be respecting the default here, as\nwe do when we get EOF? Although I tend to think if the user is\nrepeatedly giving us bogus input that we should not just proceed, but\nshould probably die. Because otherwise we are guessing at what they\nmight have wanted.\n\n-Peff\n"},{"id":"112932","messageId":"20090503202714.GD20468@coredump.intra.peff.net","threadId":"19119","inReplyTo":"1241010743-7020-3-git-send-email-wfp5p@virginia.edu","subject":"Re: [PATCH 2/6] Remove function prototypes from git-send-email.perl","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-05-03T20:27:14Z","receivedAt":"2009-05-03T20:27:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 29, 2009 at 09:12:19AM -0400, Bill Pemberton wrote:\n\n> Use of function prototypes is considered bad practice in perl. The\n> ones used here didn't accomplish anything anyhow, so they've been\n> removed.\n> \n> Signed-off-by: Bill Pemberton \n\nI think this one is sensible.\n\n-Peff\n"},{"id":"112933","messageId":"20090503203126.GE20468@coredump.intra.peff.net","threadId":"19119","inReplyTo":"1241010743-7020-5-git-send-email-wfp5p@virginia.edu","subject":"Re: [PATCH 4/6] Add explict return to end of subroutines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-05-03T20:31:26Z","receivedAt":"2009-05-03T20:31:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 29, 2009 at 09:12:21AM -0400, Bill Pemberton wrote:\n\n> In perl a subroutine that ends without an explicit return will return\n> the value of the last expression evalutated. This can lead to\n> unexpected return values.\n\nI am not opposed to this, but it is really not helping anything. These\nfunctions aren't meant to return any value, and their return values are\nnot used. In C, they would simply be declared \"void\", but there is no\nway (AFAIK) to do that in perl.\n\nBut for some of them, I wonder if you would do better instead of\nreturning _nothing_ to return something that might make a little bit of\nsense. For example, make_message_id munges a global variable. A\nuseful refactoring might be to have it return the value of the\nglobal variable, and then perhaps even the global could go away, which\n_would_ be a real improvement.\n\n-Peff\n"},{"id":"112935","messageId":"20090503205826.GF20468@coredump.intra.peff.net","threadId":"19119","inReplyTo":"1241010743-7020-7-git-send-email-wfp5p@virginia.edu","subject":"Re: [PATCH 6/6] Remove bareword filehandles in git-send-email.perl","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-05-03T20:58:26Z","receivedAt":"2009-05-03T20:58:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 29, 2009 at 09:12:23AM -0400, Bill Pemberton wrote:\n\n> The script was using bareword filehandles. This is considered a bad\n> practice so they have been changed to indirect filehandles.\n\nI think this is a real improvement; using indirect filehandles mean they\nget scoped properly, which can avoid errors (especially forgetting to\nclose() them, which happens automagically when they go out of scope).\nAssuming, of course, that the scoping added by your change is correct,\nand doesn't close a handle during a loop that we may have wanted to keep\nopen (I didn't check carefully).\n\nBut in the patch itself:\n\n> -\topen(C,\">\",$compose_filename)\n> +\topen my $C,'>',$compose_filename\n\nThere are actually two things happening here:\n\n 1. s/C/my $C/, which I think is good\n\n 2. losing the parentheses around open(). This is a style issue, but I\n think we usually prefer the parenthesized form of most perl\n builtins (and certainly in the absence of other information, it\n should be left as-is).\n\nAnd the style thing that probably _should_ be changed is the spacing: we\ntypically have a space between function arguments. So:\n\n open(my $C, '>', $compose_file)\n\n> -\tprint C < +\tprint {$C} < On Wed, Apr 29, 2009 at 09:12:23AM -0400, Bill Pemberton wrote:\n> > The script was using bareword filehandles. This is considered a bad\n> > practice so they have been changed to indirect filehandles.\n>\n> I think this is a real improvement; using indirect filehandles mean they\n> get scoped properly, which can avoid errors (especially forgetting to\n> close() them, which happens automagically when they go out of scope).\n> Assuming, of course, that the scoping added by your change is correct,\n> and doesn't close a handle during a loop that we may have wanted to keep\n> open (I didn't check carefully).\n>\n> But in the patch itself:\n> > -\topen(C,\">\",$compose_filename)\n> > +\topen my $C,'>',$compose_filename\n>\n> There are actually two things happening here:\n>\n> 1. s/C/my $C/, which I think is good\n>\n> 2. losing the parentheses around open(). This is a style issue, but I\n> think we usually prefer the parenthesized form of most perl\n> builtins (and certainly in the absence of other information, it\n> should be left as-is).\n>\n\nAnd why not go the full way and using IO::File?\n\nmy $fh = new IO::File;\n\n$fh->open(\"/the/file\", O_RDONLY|...)\n\n-- \nFrancis Galiegue\nfge@one2team.com\nIngénieur système\nMob : +33 (0) 683 877 875\nTel : +33 (0) 178 945 552\nOne2team\n40 avenue Raymond Poincaré\n75116 Paris\n"},{"id":"112944","messageId":"76718490905031926i771b0234ua7b45d5e0d827913@mail.gmail.com","threadId":"19119","inReplyTo":"20090503202625.GC20468@coredump.intra.peff.net","subject":"Re: [PATCH 3/6] Remove return undef from ask()","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-05-04T02:26:25Z","receivedAt":"2009-05-04T02:26:25Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Sun, May 3, 2009 at 4:26 PM, Jeff King wrote:\n> On a side note, while looking at this function, I wonder if that \"return\n> undef\" is correct after all. We get there only if the user has failed to\n> give valid input 10 times, so presumably it is a sanity check to\n> prevent runaway input errors\n\nCorrect, that is why it is there.\n\n> (and I am cc'ing Jay, who added the\n> function not too long ago). Should we be respecting the default here, as\n> we do when we get EOF?\n\nThe original motivation was a user who was running send-email from\ncron and it was looping forever. That case is now actually handled\nbefore the loop, and all other normal cases are handled inside the\nloop.\n\nSo the only thing that can cause the loop to exit (AFAIK) is when\n$valid_re is passed in and the user provides invalid input 10x.\n\n> Although I tend to think if the user is\n> repeatedly giving us bogus input that we should not just proceed, but\n> should probably die. Because otherwise we are guessing at what they\n> might have wanted.\n\nWell, it returns undef, at which point it's up to the caller to figure\nout what to do. You'll notice the one caller which passes in $valid_re\ndies:\n\ndie \"Send this email reply required\" unless defined $_;\n\nLetting the caller decide what to do provides more flexibility.\n\nj.\n"},{"id":"112947","messageId":"20090504081254.64e289fd@pc09.procura.nl","threadId":"19119","inReplyTo":"200905032334.03286.fge@one2team.com","subject":"Re: [PATCH 6/6] Remove bareword filehandles in git-send-email.perl","fromName":"H.Merijn Brand","fromEmail":"h.m.brand@xs4all.nl","sentAt":"2009-05-04T06:12:54Z","receivedAt":"2009-05-04T06:12:54Z","isPatch":true,"sender":{"key":"h.m.brand@xs4all.nl","avatar":"https://gravatar.com/avatar/5b8f83ee35c427a646cbea3b104346e00ab3663b99bbf435cddeb75cd4b3857b?d=mp&s=160"},"body":"On Sun, 3 May 2009 23:34:03 +0200, Francis Galiegue \nwrote:\n\n> Le Sunday 03 May 2009 22:58:26 Jeff King, vous avez écrit :\n> > On Wed, Apr 29, 2009 at 09:12:23AM -0400, Bill Pemberton wrote:\n> > > The script was using bareword filehandles. This is considered a bad\n> > > practice so they have been changed to indirect filehandles.\n> >\n> > I think this is a real improvement; using indirect filehandles mean they\n> > get scoped properly, which can avoid errors (especially forgetting to\n> > close() them, which happens automagically when they go out of scope).\n> > Assuming, of course, that the scoping added by your change is correct,\n> > and doesn't close a handle during a loop that we may have wanted to keep\n> > open (I didn't check carefully).\n> >\n> > But in the patch itself:\n> > > -\topen(C,\">\",$compose_filename)\n> > > +\topen my $C,'>',$compose_filename\n> >\n> > There are actually two things happening here:\n> >\n> > 1. s/C/my $C/, which I think is good\n> >\n> > 2. losing the parentheses around open(). This is a style issue, but I\n> > think we usually prefer the parenthesized form of most perl\n> > builtins (and certainly in the absence of other information, it\n> > should be left as-is).\n> >\n> \n> And why not go the full way and using IO::File?\n\nBecause that would be travelling back in time.\nThe most efficient and preferred way is three-arg lexical:\n\n open my $fh, \"<\", $filename or die \"$filename: $!\";\n while (<$fh>) {\n # ...\n }\n close $fh or die \"$filename: $!\";\n\ngive or take quoting style, some spaces and/or indents\n\n> my $fh = new IO::File;\n> \n> $fh->open(\"/the/file\", O_RDONLY|...)\n\nWhy use a module for something that is neatly buit in?\n\n-- \nH.Merijn Brand http://tux.nl Perl Monger http://amsterdam.pm.org/\nusing & porting perl 5.6.2, 5.8.x, 5.10.x, 5.11.x on HP-UX 10.20, 11.00,\n11.11, 11.23, and 11.31, OpenSuSE 10.3, 11.0, and 11.1, AIX 5.2 and 5.3.\nhttp://mirrors.develooper.com/hpux/ http://www.test-smoke.org/\nhttp://qa.perl.org http://www.goldmark.org/jeff/stupid-disclaimers/\n"},{"id":"112957","messageId":"200905040853.45186.fge@one2team.com","threadId":"19119","inReplyTo":"20090504081254.64e289fd@pc09.procura.nl","subject":"Re: [PATCH 6/6] Remove bareword filehandles in git-send-email.perl","fromName":"Francis Galiegue","fromEmail":"fge@one2team.com","sentAt":"2009-05-04T06:53:44Z","receivedAt":"2009-05-04T06:53:44Z","isPatch":true,"sender":{"key":"fge@one2team.com","avatar":null},"body":"Le lundi 04 mai 2009, vous avez écrit :\n[...]\n> > \n> > And why not go the full way and using IO::File?\n> \n> Because that would be travelling back in time.\n> The most efficient and preferred way is three-arg lexical:\n> \n> open my $fh, \"<\", $filename or die \"$filename: $!\";\n> while (<$fh>) {\n> # ...\n> }\n> close $fh or die \"$filename: $!\";\n> \n\nI don't see how using IO::File is going back in time at all. It's a standard \nperl module, even in 5.10.\n\n> \n> > my $fh = new IO::File;\n> > \n> > $fh->open(\"/the/file\", O_RDONLY|...)\n> \n> Why use a module for something that is neatly buit in?\n> \n\nBecause it reads better? YMMV, of course. I prefer using IO::File because perl \nhas too many keywords for its own good :p\n\n-- \nFrancis Galiegue\nONE2TEAM\nIngénieur système\nMob : +33 (0) 683 877 875\nTel : +33 (0) 178 945 552\nfge@one2team.com\n40 avenue Raymond Poincaré\n75116 Paris\n"},{"id":"112962","messageId":"20090504094108.0bf52762@pc09.procura.nl","threadId":"19119","inReplyTo":"200905040853.45186.fge@one2team.com","subject":"Re: [PATCH 6/6] Remove bareword filehandles in git-send-email.perl","fromName":"H.Merijn Brand","fromEmail":"h.m.brand@xs4all.nl","sentAt":"2009-05-04T07:41:08Z","receivedAt":"2009-05-04T07:41:08Z","isPatch":true,"sender":{"key":"h.m.brand@xs4all.nl","avatar":"https://gravatar.com/avatar/5b8f83ee35c427a646cbea3b104346e00ab3663b99bbf435cddeb75cd4b3857b?d=mp&s=160"},"body":"On Mon, 4 May 2009 08:53:44 +0200, Francis Galiegue \nwrote:\n\n> Le lundi 04 mai 2009, vous avez écrit :\n> [...]\n> > > \n> > > And why not go the full way and using IO::File?\n> > \n> > Because that would be travelling back in time.\n> > The most efficient and preferred way is three-arg lexical:\n> > \n> > open my $fh, \"<\", $filename or die \"$filename: $!\";\n> > while (<$fh>) {\n> > # ...\n> > }\n> > close $fh or die \"$filename: $!\";\n> > \n> \n> I don't see how using IO::File is going back in time at all. It's\n> a standard perl module, even in 5.10.\n\nIt was written to be able to have lexical handles and pass them around.\nWith the above implementation you don't need to read a 600+ line module\nto have every file action go through methods that are not needed at\nall, and you also do not have to read the documentation for the\ndeviating syntax. I'd rather stick to simple, easy and standard.\n\n> > > my $fh = new IO::File;\n> > > $fh->open(\"/the/file\", O_RDONLY|...)\n> > \n> > Why use a module for something that is neatly buit in?\n> \n> Because it reads better? YMMV, of course.\n\nBecause it is more efficient? And yes, it reads better, because it is\nused in a zillion places already.\n\n> I prefer using IO::File because perl has too many keywords for its\n> own good :p\n\nIn the syntax I wrote down are now new keywords at all.\n\n-- \nH.Merijn Brand http://tux.nl Perl Monger http://amsterdam.pm.org/\nusing & porting perl 5.6.2, 5.8.x, 5.10.x, 5.11.x on HP-UX 10.20, 11.00,\n11.11, 11.23, and 11.31, OpenSuSE 10.3, 11.0, and 11.1, AIX 5.2 and 5.3.\nhttp://mirrors.develooper.com/hpux/ http://www.test-smoke.org/\nhttp://qa.perl.org http://www.goldmark.org/jeff/stupid-disclaimers/\n"}]}