{"thread":{"id":"33246","subject":"[PATCH] git-send-email.perl: implement suggestions made by perlcritic","startedAt":"2013-03-21T12:39:47Z","lastAt":"2013-04-02T14:49:35Z","messageCount":21,"participants":["Ramkumar Ramachandra","Junio C Hamano","Jonathan Nieder","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"211858","messageId":"1363869587-10462-1-git-send-email-artagnon@gmail.com","threadId":"33246","inReplyTo":null,"subject":"[PATCH] git-send-email.perl: implement suggestions made by perlcritic","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-03-21T12:39:47Z","receivedAt":"2013-03-21T12:39:47Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Running perlcritic with gentle severity reports six problems. The\nfollowing lists the line numbers on which the problems occur, along\nwith a description of the problem. This patch fixes them all.\n\n516: Contrary to common belief, subroutine prototypes do not enable\ncompile-time checks for proper arguments. They serve the singular\npurpose of defining functions that behave like built-in functions, but\ncheck_file_rev_conflict was never intended to behave like one.\n\n714, 836, 855, 1487: Returning `undef' upon failure from a subroutine\nis pretty common. But if the subroutine is called in list context, an\nexplicit `return undef;' statement will return a one-element list\ncontaining `(undef)'. Now if that list is subsequently put in a\nboolean context to test for failure, then it evaluates to true. But\nyou probably wanted it to be false. The solution is to just use a\nbare `return' statement whenever you want to return failure. In list\ncontext, Perl will then give you an empty list (which is false), and\n`undef' in scalar context (which is also false).\n\n1441: The three-argument form of `open' (introduced in Perl 5.6)\nprevents subtle bugs that occur when the filename starts with funny\ncharacters like '>' or '<'. It's also more explicitly clear to define\nthe input mode of the file. This policy will not complain if the file\nexplicitly states that it is compatible with a version of Perl prior\nto 5.6 via an include statement (see 'use 5.008' near the top of the\nfile).\n\nSigned-off-by: Ramkumar Ramachandra \n---\n I was looking at git-send-email.perl after I posted that bug report\n about SSL_verify. What better way to get started than Perl Best\n Practices? Here's a nice patch to fix some obvious style issues. I\n didn't run perlcritic on a higher severity, because I don't know\n enough Perl to have a well-formed opinion of my own; I'm just fixing\n the glaringly obvious problems.\n\n git-send-email.perl | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex be809e5..e974b11 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -513,7 +513,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@@ -711,7 +711,7 @@ sub ask {\n \t\t\t}\n \t\t}\n \t}\n-\treturn undef;\n+\treturn;\n }\n \n my %broken_encoding;\n@@ -833,7 +833,7 @@ sub extract_valid_address {\n \t# less robust/correct than the monster regexp in Email::Valid,\n \t# but still does a 99% job, and one less dependency\n \treturn $1 if $address =~ /($local_part_regexp\\@$domain_regexp)/;\n-\treturn undef;\n+\treturn;\n }\n \n sub extract_valid_address_or_die {\n@@ -852,7 +852,7 @@ sub validate_address {\n \t\t\tvalid_re => qr/^(?:quit|q|drop|d|edit|e)/i,\n \t\t\tdefault => 'q');\n \t\tif (/^d/i) {\n-\t\t\treturn undef;\n+\t\t\treturn;\n \t\t} elsif (/^q/i) {\n \t\t\tcleanup_compose_files();\n \t\t\texit(0);\n@@ -1438,7 +1438,7 @@ sub recipients_cmd {\n \n \tmy $sanitized_sender = sanitize_address($sender);\n \tmy @addresses = ();\n-\topen my $fh, \"$cmd \\Q$file\\E |\"\n+\topen my $fh, q{-|}, \"$cmd \\Q$file\\E\"\n \t or die \"($prefix) Could not execute '$cmd'\";\n \twhile (my $address = <$fh>) {\n \t\t$address =~ s/^\\s*//g;\n@@ -1484,7 +1484,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;\n }\n \n sub file_has_nonascii {\n-- \n1.8.2.62.ga35d936.dirty\n"},{"id":"211874","messageId":"7vd2usbwkn.fsf@alter.siamese.dyndns.org","threadId":"33246","inReplyTo":"1363869587-10462-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH] git-send-email.perl: implement suggestions made by perlcritic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-21T15:29:28Z","receivedAt":"2013-03-21T15:29:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index be809e5..e974b11 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -513,7 +513,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\nHave you verified that the callers of this sub are OK with this\nchange? It used to force scalar context on its arguments but now it\ndoes not.\n\nI am not saying I know the callers will get broken. I am trying to\nmake sure that this is *not* the result of blindly following\nperlcritic output without understanding the ramifications of the\nchange.\n\n> @@ -1438,7 +1438,7 @@ sub recipients_cmd {\n> \n> \tmy $sanitized_sender = sanitize_address($sender);\n> \tmy @addresses = ();\n> -\topen my $fh, \"$cmd \\Q$file\\E |\"\n> +\topen my $fh, q{-|}, \"$cmd \\Q$file\\E\"\n\nStrange quoting (why not just say \"-|\"?) aside, if you are moving\naway from the two-param form of open(), it would be a sane thing to\ndo to also stop concatenating the arguments to commands to avoid\nshell metacharacter gotcha. It will make the resulting code much\nsafer.\n"},{"id":"211882","messageId":"CALkWK0kkWAkiANL-vL-OthKSDkoU2b_Q68frkGc06QNG7RbSFQ@mail.gmail.com","threadId":"33246","inReplyTo":"7vd2usbwkn.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git-send-email.perl: implement suggestions made by perlcritic","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-03-21T16:51:38Z","receivedAt":"2013-03-21T16:51:38Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> Ramkumar Ramachandra writes:\n>\n>> diff --git a/git-send-email.perl b/git-send-email.perl\n>> index be809e5..e974b11 100755\n>> --- a/git-send-email.perl\n>> +++ b/git-send-email.perl\n>> @@ -513,7 +513,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>\n> Have you verified that the callers of this sub are OK with this\n> change? It used to force scalar context on its arguments but now it\n> does not.\n>\n> I am not saying I know the callers will get broken. I am trying to\n> make sure that this is *not* the result of blindly following\n> perlcritic output without understanding the ramifications of the\n> change.\n\nNo Junio, I haven't blindly followed the perlcritic output. I've\nconsidered the ramifications, audited the callers, and run the tests\nto make sure that nothing breaks.\n\n>> @@ -1438,7 +1438,7 @@ sub recipients_cmd {\n>>\n>> my $sanitized_sender = sanitize_address($sender);\n>> my @addresses = ();\n>> - open my $fh, \"$cmd \\Q$file\\E |\"\n>> + open my $fh, q{-|}, \"$cmd \\Q$file\\E\"\n>\n> Strange quoting (why not just say \"-|\"?) aside\n\nIntentional. I thought it was a pretty way to differentiate between a\nmode string (which can only be <, >, or -|) and a filename string. If\nyou don't share my taste, feel free to use quotes instead.\n\n> , if you are moving\n> away from the two-param form of open(), it would be a sane thing to\n> do to also stop concatenating the arguments to commands to avoid\n> shell metacharacter gotcha. It will make the resulting code much\n> safer.\n\nYes, the file can be improved in many ways, but that is not the topic\nof this series. This series just makes the changes suggested by\nperlcritic gentle.\n"},{"id":"212408","messageId":"CALkWK0mvcCODcZN0s=7YQAnpVA053bFrnCr0qugpBvSK=h6Usg@mail.gmail.com","threadId":"33246","inReplyTo":"CALkWK0kkWAkiANL-vL-OthKSDkoU2b_Q68frkGc06QNG7RbSFQ@mail.gmail.com","subject":"Re: [PATCH] git-send-email.perl: implement suggestions made by perlcritic","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-03-27T17:13:50Z","receivedAt":"2013-03-27T17:13:50Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio,\n\nI don't see this queued in 'pu'. Do you have any objections to the\npatch, or did you just forget?\n\nThanks.\n"},{"id":"212417","messageId":"7vfvzgn3ob.fsf@alter.siamese.dyndns.org","threadId":"33246","inReplyTo":"CALkWK0mvcCODcZN0s=7YQAnpVA053bFrnCr0qugpBvSK=h6Usg@mail.gmail.com","subject":"Re: [PATCH] git-send-email.perl: implement suggestions made by perlcritic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-27T17:38:44Z","receivedAt":"2013-03-27T17:38:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> I don't see this queued in 'pu'. Do you have any objections to the\n> patch, or did you just forget?\n\nI do not recall the details but I think I was expecting a re-roll\nupdating the log message (if the original invited questions, there\nmust have been some room for improvement to avoid such confusions,\nright?) for a few days since the exchange, and then forgot.\n"},{"id":"212503","messageId":"1364474835-23416-1-git-send-email-artagnon@gmail.com","threadId":"33246","inReplyTo":"7vfvzgn3ob.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] git-send-email.perl: implement suggestions made by perlcritic","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-03-28T12:47:15Z","receivedAt":"2013-03-28T12:47:15Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Running perlcritic with gentle severity reports six problems. The\nfollowing lists the line numbers on which the problems occur, along\nwith a description of the problem. This patch fixes them all, after\ncarefully considering the consequences.\n\n516: Contrary to common belief, subroutine prototypes do not enable\ncompile-time checks for proper arguments. They serve the singular\npurpose of defining functions that behave like built-in functions, but\ncheck_file_rev_conflict was never intended to behave like one. We\nhave verified that the callers of the subroutines are alright with the\nchange.\n\n714, 836, 855, 1487: Returning `undef' upon failure from a subroutine\nis pretty common. But if the subroutine is called in list context, an\nexplicit `return undef;' statement will return a one-element list\ncontaining `(undef)'. Now if that list is subsequently put in a\nboolean context to test for failure, then it evaluates to true. But\nyou probably wanted it to be false. The solution is to just use a\nbare `return' statement whenever you want to return failure. In list\ncontext, Perl will then give you an empty list (which is false), and\n`undef' in scalar context (which is also false).\n\n1441: The three-argument form of `open' (introduced in Perl 5.6)\nprevents subtle bugs that occur when the filename starts with funny\ncharacters like '>' or '<'. It's also more explicitly clear to define\nthe input mode of the file. This policy will not complain if the file\nexplicitly states that it is compatible with a version of Perl prior\nto 5.6 via an include statement (see 'use 5.008' near the top of the\nfile).\n\nSigned-off-by: Ramkumar Ramachandra \n---\n Junio: In future, please tell me explicitly that you're expecting a\n re-roll with an updated commit message. It wasn't obvious to me at\n all.\n\n git-send-email.perl | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex c3501d9..633f447 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -513,7 +513,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@@ -711,7 +711,7 @@ sub ask {\n \t\t\t}\n \t\t}\n \t}\n-\treturn undef;\n+\treturn;\n }\n \n my %broken_encoding;\n@@ -833,7 +833,7 @@ sub extract_valid_address {\n \t# less robust/correct than the monster regexp in Email::Valid,\n \t# but still does a 99% job, and one less dependency\n \treturn $1 if $address =~ /($local_part_regexp\\@$domain_regexp)/;\n-\treturn undef;\n+\treturn;\n }\n \n sub extract_valid_address_or_die {\n@@ -852,7 +852,7 @@ sub validate_address {\n \t\t\tvalid_re => qr/^(?:quit|q|drop|d|edit|e)/i,\n \t\t\tdefault => 'q');\n \t\tif (/^d/i) {\n-\t\t\treturn undef;\n+\t\t\treturn;\n \t\t} elsif (/^q/i) {\n \t\t\tcleanup_compose_files();\n \t\t\texit(0);\n@@ -1453,7 +1453,7 @@ sub recipients_cmd {\n \n \tmy $sanitized_sender = sanitize_address($sender);\n \tmy @addresses = ();\n-\topen my $fh, \"$cmd \\Q$file\\E |\"\n+\topen my $fh, q{-|}, \"$cmd \\Q$file\\E\"\n \t or die \"($prefix) Could not execute '$cmd'\";\n \twhile (my $address = <$fh>) {\n \t\t$address =~ s/^\\s*//g;\n@@ -1499,7 +1499,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;\n }\n \n sub file_has_nonascii {\n-- \n1.8.2.141.g07926c6\n"},{"id":"212566","messageId":"20130328185707.GR28148@google.com","threadId":"33246","inReplyTo":"1364474835-23416-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH v2] git-send-email.perl: implement suggestions made by perlcritic","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-03-28T18:57:07Z","receivedAt":"2013-03-28T18:57:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Junio: In future, please tell me explicitly that you're expecting a\n> re-roll with an updated commit message. It wasn't obvious to me at\n> all.\n\nWhen there are questions in response to a patch, there are two\npossibilities:\n\n * temporary brainfart --- sorry for the bother\n * the clarity of the patch or commit message has room for improvement\n\nThis wasn't a case of the former, so a seasoned contributor could\nsafely assume that it was definitely a case of the latter.\n\nThe explanation in Linux kernel's Documentation/SubmittingPatches\nitem 10 (\"Don't get discouraged. Re-submit\") has some fitting advice.\n\nHope that helps,\nJonathan\n"},{"id":"212578","messageId":"7vsj3fs22i.fsf@alter.siamese.dyndns.org","threadId":"33246","inReplyTo":"1364474835-23416-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH v2] git-send-email.perl: implement suggestions made by perlcritic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-28T20:26:45Z","receivedAt":"2013-03-28T20:26:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Running perlcritic with gentle severity reports six problems. The\n> following lists the line numbers on which the problems occur, along\n> with a description of the problem. This patch fixes them all, \n\nThanks.\n\n> after\n> carefully considering the consequences.\n\nHmmmm.....\n\n> 516: Contrary to common belief, subroutine prototypes do not enable\n> compile-time checks for proper arguments. They serve the singular\n> purpose of defining functions that behave like built-in functions, but\n> check_file_rev_conflict was never intended to behave like one. We\n> have verified that the callers of the subroutines are alright with the\n> change.\n\nThis, together with the \"carefully considering\", does not build any\nconfidence on the part of the reader. Subroutine prototypes are not\nfor compile-time checks (correct), and they were introduced to the\nlanguage only for the purpose of letting you write subroutine that\nemulate built-ins like \"pop @ary\" (again correct),\n\nThe most important fact that is not described in the above is that\nby using prototype, the subroutine enforces a non-default context at\nthe callsite when the arguments are evaluated. That is why you can\nwrite an emulated \"pop\"; otherwise the first @ary will simply be\nflattened and you won't grab an element from the array.\n\nSaying \"check_file_rev_conflict is not emulating any Perl built-in\nfunction\" is irrelevant as a justification for the change. A\nsubroutine that does not emulate a built-in can still be relying on\nthe non-default context in which its arguments are evaluated.\n\n sub foo ($) { my ($arg) = @_; print \"$arg\\n\"; }\n sub bar { my ($arg) = @_; print \"$arg\\n\"; }\n my @baz = (100, 101, 102);\n foo @baz; # says 3\n bar @baz; # says 100\n\nIn general, writing subroutines without prototypes is much\npreferred. I do not dispute that and would not argue against\nperlcritic. But if you blindly _fix_ the above \"foo\" subroutine by\ndropping its prototype, it changes behaviour if the callers passed\nan array variable. You need to check the callers and they are not\ndoing something to cause the prototyped and unprototyped versions\nto behave differently.\n\nAnd that is what needs to be explained in the log message; not these\nhandwavy \"carefully considering consequences\" (what consequences did\nyou consider?), \"was never intended to behave like one\" (how does\nthat matter?) and \"the callers of the subroutines are alright\" (why\ndo you think so?).\n\nWithout that, the reviewer needs to go and check the callers. Your\nlog message is _not_ helping them.\n\nSame for the remainder.\n\nIn general, you do not have to copy and paste the output from\nperlcritic. Treat it more as a learning tool, and use the knowledge\nyou learned from its output to explain why your changes are\nimprovements. Not just \"because perlcritic said so\".\n\nFor \"ask\" subroutine, all its callers assign the returned value to a\nsingle scaler variable, so the difference between \"return undef\" and\njust \"return\" does not matter. If somebody starts doing\n\n\t@foo = ask(...);\n\tif (@foo) { ... we got an answer ... } else { ... we did not ... }\n\nthen \"return undef;\" form will break. So it is less error prone if\nwe dropped the explicit \"undef\". The same goes for extract_valid_address,\nwhose current callers all assign the returned value to a single scalar,\nor apply \"!\" operator inside \"while\" conditional.\n\nThe change to validate_address is correct, but it is correct only\nbecause its only caller, validate_address_list, filters out \"undef\"\nreturned from map() that calls this subroutine. By dropping the\nexplicit \"undef\" from there, it seems to me that validate_address no\nlonger returns \"undef\" so validate_address_list loses any need to\nfilter its return value. Seeing a patch that does not change that\ncaller while changing the callee makes reviewers wonder what is\ngoing on. Perhaps even with this patch, there still is a need to\nfilter in validate_address_list, and if so, that needs to be\nexplained.\n\nIf I were doing this change, I would rather leave this subroutine\nas-is. Nothing is broken and we are risking new breakages by\nchanging it.\n\n> 1441: The three-argument form of `open' (introduced in Perl 5.6)\n> prevents subtle bugs that occur when the filename starts with funny\n> characters like '>' or '<'.\n\nCorrect, and this patch is about using the three-or-more-arg form to\ntake advantage of its safety. So why are we still using the shell\ninvocation form inherited back from the days we used two-arg form?\n\nAgain, seeing a patch that only turns \"open FILEHANDLE,EXPR\" into\n\"open FILEHANDLE,MODE,EXPR\" when EXPR is not a simple command name\nand not into \"open FILEHANDLE,MODE,EXPR,LIST\" form makes reviewers\nwonder why the patch stops in the middle.\n\n> Junio: In future, please tell me explicitly that you're expecting a\n> re-roll with an updated commit message. It wasn't obvious to me at\n> all.\n\nIt wasn't obvious to me, either ;-).\n\nI said the patch was not explained well, but it may not be the only\nproblem with it. Other people may have inputs to it, and you may\nhave been waiting for the input from others before initiating a\nreroll.\n\nIt wasn't like \"please fix these and then the result will be perfect\nand I'll promise to apply\". A maintainer cannot work like that.\n\nI try to make sure there aren't leftover bits from time to time by\nsweeping archived e-mails, and also I try to handhold newer\ncontributors by pinging them about their progress every once in a\nwhile (but you are not exactly new).\n\nThis patch fell under the cracks, and reminding me with a \"what\nhappened to it?\" was the right thing to do. Literally, that is what\nI ask in the \"Notes from the maintainer\" message.\n"},{"id":"212755","messageId":"CALkWK0nGZUV9umbe9UoV79QU+ojCc-=j+RFpCNn_vjM_QTPyPQ@mail.gmail.com","threadId":"33246","inReplyTo":"7vsj3fs22i.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] git-send-email.perl: implement suggestions made by perlcritic","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-03-31T20:59:46Z","receivedAt":"2013-03-31T20:59:46Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> sub foo ($) { my ($arg) = @_; print \"$arg\\n\"; }\n> sub bar { my ($arg) = @_; print \"$arg\\n\"; }\n> my @baz = (100, 101, 102);\n> foo @baz; # says 3\n> bar @baz; # says 100\n\nOuch. Please drop this patch; I'll resubmit when I feel confident\nabout my change.\n\n> This patch fell under the cracks, and reminding me with a \"what\n> happened to it?\" was the right thing to do. Literally, that is what\n> I ask in the \"Notes from the maintainer\" message.\n\nRight. Thanks for clarifying. I'll actively track the patches I submit.\n"},{"id":"212763","messageId":"7vr4ivni5g.fsf@alter.siamese.dyndns.org","threadId":"33246","inReplyTo":"CALkWK0nGZUV9umbe9UoV79QU+ojCc-=j+RFpCNn_vjM_QTPyPQ@mail.gmail.com","subject":"Re: [PATCH v2] git-send-email.perl: implement suggestions made by perlcritic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-01T01:39:39Z","receivedAt":"2013-04-01T01:39:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Junio C Hamano wrote:\n>> sub foo ($) { my ($arg) = @_; print \"$arg\\n\"; }\n>> sub bar { my ($arg) = @_; print \"$arg\\n\"; }\n>> my @baz = (100, 101, 102);\n>> foo @baz; # says 3\n>> bar @baz; # says 100\n>\n> Ouch. Please drop this patch; I'll resubmit when I feel confident\n> about my change.\n\nNo, let's not do that. I will forget and end up spending time to\nread the same patch again.\n"},{"id":"212764","messageId":"1364780442-6015-1-git-send-email-gitster@pobox.com","threadId":"33246","inReplyTo":"7vr4ivni5g.fsf@alter.siamese.dyndns.org","subject":"[PATCH 1/3] send-email: use \"return;\" not \"return undef;\" on error codepaths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-01T01:40:40Z","receivedAt":"2013-04-01T01:40:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Ramkumar Ramachandra \n\nAll the callers of \"ask\", \"extract_valid_address\", and \"validate_patch\"\nsubroutines assign the return values from them to a single scaler:\n\n\t$var = subr(...);\n\nand \"return undef;\" in these subroutine can safely be turned into a\nsimpler \"return;\". Doing so will also future-proof a new caller that\nmistakenly does this:\n\n @foo = ask(...);\n if (@foo) { ... we got an answer ... } else { ... we did not ... }\n\nNote that we leave \"return undef;\" in validate_address on purpose,\neven though Perlcritic may complain. The primary \"return\" site of\nthe function returns whatever is in the scaler variable $address, so\nit is pointless to change only the other \"return undef;\" to \"return\".\nThe caller must be prepared to see an array with a single undef as\nthe return value from this subroutine anyway.\n\nSigned-off-by: Ramkumar Ramachandra \nSigned-off-by: Junio C Hamano \n---\n git-send-email.perl | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex be809e5..79cc5be 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -711,7 +711,7 @@ sub ask {\n \t\t\t}\n \t\t}\n \t}\n-\treturn undef;\n+\treturn;\n }\n \n my %broken_encoding;\n@@ -833,7 +833,7 @@ sub extract_valid_address {\n \t# less robust/correct than the monster regexp in Email::Valid,\n \t# but still does a 99% job, and one less dependency\n \treturn $1 if $address =~ /($local_part_regexp\\@$domain_regexp)/;\n-\treturn undef;\n+\treturn;\n }\n \n sub extract_valid_address_or_die {\n@@ -1484,7 +1484,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;\n }\n \n sub file_has_nonascii {\n-- \n1.8.2-441-g6e6f07b\n"},{"id":"212766","messageId":"1364780442-6015-2-git-send-email-gitster@pobox.com","threadId":"33246","inReplyTo":"1364780442-6015-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/3] send-email: drop misleading function prototype","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-01T01:40:41Z","receivedAt":"2013-04-01T01:40:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Ramkumar Ramachandra \n\nThe subroutine check_file_rev_conflict() is called from two places,\nboth of which expects to pass a single scaler variable and see if\nthat can be interpreted as a pathname or a revision name. It is\ndefined with a function prototype ($) to force a scaler context\nwhile evaluating the arguments at the calling site but it does not\nhelp the current calling sites. The only effect it has is to hurt\nfuture calling sites that may want to build an argument list in an\narray variable and call it as check_file_rev_confict(@args).\n\nDrop the misleading prototype, as Perlcritic suggests.\n\nSigned-off-by: Ramkumar Ramachandra \nSigned-off-by: Junio C Hamano \n---\n git-send-email.perl | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 79cc5be..86dd593 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -513,7 +513,7 @@ sub split_addrs {\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-- \n1.8.2-441-g6e6f07b\n"},{"id":"212765","messageId":"1364780442-6015-3-git-send-email-gitster@pobox.com","threadId":"33246","inReplyTo":"1364780442-6015-1-git-send-email-gitster@pobox.com","subject":"[PATCH 3/3] send-email: use the three-arg form of open in recipients_cmd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-01T01:40:42Z","receivedAt":"2013-04-01T01:40:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Ramkumar Ramachandra \n\nPerlcritic does not want to see the trailing pipe in the two-args\nform of open(), i.e.\n\n\topen my $fh, \"$cmd \\Q$file\\E |\";\n\nIf $cmd were a single-token command name, it would make a lot more\nsense to use four-or-more-args form \"open FILEHANDLE,MODE,CMD,ARGS\"\nto avoid shell from expanding metacharacters in $file, but we do\nexpect multi-word string in $to_cmd and $cc_cmd to be expanded by\nthe shell, so we cannot rewrite it to\n\n\topen my $fh, \"-|\", $cmd, $file;\n\nfor extra safety. At least, by using this in the three-arg form:\n\n\topen my $fh, \"-|\", \"$cmd \\Q$file\\E\";\n\nwe can silence Perlcritique, even though we do not gain much safety\nby doing so.\n\nSigned-off-by: Ramkumar Ramachandra \nSigned-off-by: Junio C Hamano \n---\n git-send-email.perl | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 86dd593..fb92227 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1438,7 +1438,7 @@ sub recipients_cmd {\n \n \tmy $sanitized_sender = sanitize_address($sender);\n \tmy @addresses = ();\n-\topen my $fh, \"$cmd \\Q$file\\E |\"\n+\topen my $fh, \"-|\", \"$cmd \\Q$file\\E\"\n \t or die \"($prefix) Could not execute '$cmd'\";\n \twhile (my $address = <$fh>) {\n \t\t$address =~ s/^\\s*//g;\n-- \n1.8.2-441-g6e6f07b\n"},{"id":"212769","messageId":"20130401022024.GC11704@elie.Belkin","threadId":"33246","inReplyTo":"1364780442-6015-2-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 2/3] send-email: drop misleading function prototype","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-04-01T02:20:24Z","receivedAt":"2013-04-01T02:20:24Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> From: Ramkumar Ramachandra \n>\n> The subroutine check_file_rev_conflict() is called from two places,\n> both of which expects to pass a single scaler variable and see if\n> that can be interpreted as a pathname or a revision name. It is\n> defined with a function prototype ($) to force a scaler context\n> while evaluating the arguments at the calling site but it does not\n> help the current calling sites. The only effect it has is to hurt\n> future calling sites that may want to build an argument list in an\n> array variable and call it as check_file_rev_confict(@args).\n>\n> Drop the misleading prototype, as Perlcritic suggests.\n\nNice explanation. Here's a similar patch I wrote before I noticed\nyour patch, with commit message stolen from the above.\n\n-- >8 --\nSubject: send-email: drop misleading function prototype\n\nThe subroutine check_file_rev_conflict() is called from two places,\nboth of which expects to pass a single scaler variable and see if\nthat can be interpreted as a pathname or a revision name. It is\ndefined with a function prototype ($) to force a scaler context\nwhile evaluating the arguments at the calling site but it does not\nhelp the current calling sites. The only effect it has is to hurt\nfuture calling sites that may want to build an argument list in an\narray variable and call it as check_file_rev_confict(@args).\n\nDrop the misleading prototype, as Perlcritic suggests.\n\nWhile at it, rename the function to avoid new call sites unaware of\nthis change arising and add a comment clarifying what this function\nis for.\n\nReported-by: Ramkumar Ramachandra \nSigned-off-by: Jonathan Nieder \n---\n git-send-email.perl | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex c3501d98..d6b3c32b 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -512,8 +512,9 @@ sub split_addrs {\n \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+# is_format_patch_arg($f) returns 0 if $f names a patch, or 1 if\n+# $f is a revision list specification to be passed to format-patch.\n+sub is_format_patch_arg {\n \treturn unless $repo;\n \tmy $f = shift;\n \ttry {\n@@ -529,6 +530,7 @@ ($)\n * Giving --format-patch option if you mean a range.\n EOF\n \t} catch Git::Error::Command with {\n+\t\t# Not a valid revision. Treat it as a filename.\n \t\treturn 0;\n \t}\n }\n@@ -540,14 +542,14 @@ ($)\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 and !is_format_patch_arg($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} elsif ((-f $f or -p $f) and !check_file_rev_conflict($f)) {\n+\t} elsif ((-f $f or -p $f) and !is_format_patch_arg($f)) {\n \t\tpush @files, $f;\n \t} else {\n \t\tpush @rev_list_opts, $f;\n-- \n1.8.2\n"},{"id":"212770","messageId":"20130401023003.GD11704@elie.Belkin","threadId":"33246","inReplyTo":"1364780442-6015-3-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 3/3] send-email: use the three-arg form of open in recipients_cmd","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-04-01T02:30:03Z","receivedAt":"2013-04-01T02:30:03Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> we cannot rewrite it to\n>\n> \topen my $fh, \"-|\", $cmd, $file;\n>\n> for extra safety. At least, by using this in the three-arg form:\n>\n> \topen my $fh, \"-|\", \"$cmd \\Q$file\\E\";\n>\n> we can silence Perlcritique, even though we do not gain much safety\n> by doing so.\n\nYeah, I think this is the right thing to do.\n\nThis means that if some later code refactoring parses $tocmd once and\npasses an array around, it would be easy to change this to\n\n\topen my $fh, \"-|\", @$cmd, $file;\n\nand there would be no temptation to do something involving pasting\n@$cmd back together into a single string. Of course such a\nrefactoring is not very likely, but that kind of thing is a good\nreason to prefer this style in general.\n\nSo for what it's worth, I like this patch.\n"},{"id":"212778","messageId":"CAPig+cTzre7D93TOv9CV_qhX6hnxSmj_zL+mewKE5c9kaP+-Pg@mail.gmail.com","threadId":"33246","inReplyTo":"1364780442-6015-1-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 1/3] send-email: use \"return;\" not \"return undef;\" on error codepaths","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-04-01T04:08:15Z","receivedAt":"2013-04-01T04:08:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 31, 2013 at 9:40 PM, Junio C Hamano wrote:\n> All the callers of \"ask\", \"extract_valid_address\", and \"validate_patch\"\n> subroutines assign the return values from them to a single scaler:\n\ns/scaler/scalar/g\n\n(note the /g)\n\n>\n> $var = subr(...);\n>\n> and \"return undef;\" in these subroutine can safely be turned into a\n> simpler \"return;\". Doing so will also future-proof a new caller that\n> mistakenly does this:\n>\n> @foo = ask(...);\n> if (@foo) { ... we got an answer ... } else { ... we did not ... }\n>\n> Note that we leave \"return undef;\" in validate_address on purpose,\n> even though Perlcritic may complain. The primary \"return\" site of\n> the function returns whatever is in the scaler variable $address, so\n> it is pointless to change only the other \"return undef;\" to \"return\".\n> The caller must be prepared to see an array with a single undef as\n> the return value from this subroutine anyway.\n"},{"id":"212779","messageId":"CAPig+cTvtrtzLeZDPuxzomz055V4u+R9cR+M86oKsEzt8+5Mhw@mail.gmail.com","threadId":"33246","inReplyTo":"1364780442-6015-2-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 2/3] send-email: drop misleading function prototype","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-04-01T04:09:13Z","receivedAt":"2013-04-01T04:09:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 31, 2013 at 9:40 PM, Junio C Hamano wrote:\n> The subroutine check_file_rev_conflict() is called from two places,\n> both of which expects to pass a single scaler variable and see if\n\ns/scaler/scalar/g\n\n(note the /g)\n\n> that can be interpreted as a pathname or a revision name. It is\n> defined with a function prototype ($) to force a scaler context\n> while evaluating the arguments at the calling site but it does not\n> help the current calling sites. The only effect it has is to hurt\n> future calling sites that may want to build an argument list in an\n> array variable and call it as check_file_rev_confict(@args).\n>\n> Drop the misleading prototype, as Perlcritic suggests.\n"},{"id":"212858","messageId":"CALkWK0mz3Oj+3zsx2JSs8ufh=yPOAZ9hiSb6a_pBJptJqdp5zg@mail.gmail.com","threadId":"33246","inReplyTo":"7vr4ivni5g.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] git-send-email.perl: implement suggestions made by perlcritic","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-02T07:41:47Z","receivedAt":"2013-04-02T07:41:47Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> Ramkumar Ramachandra writes:\n>> Ouch. Please drop this patch; I'll resubmit when I feel confident\n>> about my change.\n>\n> No, let's not do that. I will forget and end up spending time to\n> read the same patch again.\n\nAll three look good to me. Thanks for doing this: I learnt quite a\nbit from your commit messages.\n"},{"id":"212859","messageId":"CALkWK0=AmKC+U8tbjsc-6zd3R2fWq=YSfiQn8y7ukzdDAZqzcA@mail.gmail.com","threadId":"33246","inReplyTo":"1364780442-6015-3-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 3/3] send-email: use the three-arg form of open in recipients_cmd","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-02T07:46:19Z","receivedAt":"2013-04-02T07:46:19Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> we can silence Perlcritique, even though we do not gain much safety\n> by doing so.\n\nNit: it's perlcritic; critique is used to refer to the output of a critic.\n"},{"id":"212861","messageId":"CALkWK0muF2Nx=ykD6jt4pvpmp9c3R2KhXhQMjLAyMWiHu_6iPA@mail.gmail.com","threadId":"33246","inReplyTo":"1364780442-6015-1-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 1/3] send-email: use \"return;\" not \"return undef;\" on error codepaths","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-02T07:59:01Z","receivedAt":"2013-04-02T07:59:01Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> Note that we leave \"return undef;\" in validate_address on purpose,\n> even though Perlcritic may complain. The primary \"return\" site of\n> the function returns whatever is in the scaler variable $address, so\n> it is pointless to change only the other \"return undef;\" to \"return\".\n> The caller must be prepared to see an array with a single undef as\n> the return value from this subroutine anyway.\n\nThe way I understood it:\nAll callsites only ever call validate_address via\nvalidate_address_list, which in turn maps a list of addresses by\ncalling validate_address on each address. Therefore, validate_address\nreturning an undef in one codepath is correct.\n"},{"id":"212885","messageId":"7vsj39gf7k.fsf@alter.siamese.dyndns.org","threadId":"33246","inReplyTo":"CALkWK0muF2Nx=ykD6jt4pvpmp9c3R2KhXhQMjLAyMWiHu_6iPA@mail.gmail.com","subject":"Re: [PATCH 1/3] send-email: use \"return;\" not \"return undef;\" on error codepaths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-02T14:49:35Z","receivedAt":"2013-04-02T14:49:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Junio C Hamano wrote:\n>> Note that we leave \"return undef;\" in validate_address on purpose,\n>> even though Perlcritic may complain. The primary \"return\" site of\n>> the function returns whatever is in the scaler variable $address, so\n>> it is pointless to change only the other \"return undef;\" to \"return\".\n>> The caller must be prepared to see an array with a single undef as\n>> the return value from this subroutine anyway.\n>\n> The way I understood it:\n> All callsites only ever call validate_address via\n> validate_address_list, which in turn maps a list of addresses by\n> calling validate_address on each address. Therefore, validate_address\n> returning an undef in one codepath is correct.\n\nI think we are in agreement then.\n\nThe implication of what you just said is that anybody that calls\nthis subroutine _must_ be (and indeed is) prepared to see a single\n'undef' returned from it, hence the use pattern,\n\n\t@foo = validate_address(...);\n if (@foo) { ... we got a valid one ... } else { ... we did not ... }\n\ncannot be used for the subroutine _anyway_. @foo may get an invalid\naddress, even with the \"don't say 'return undef;' say 'return;'\"\nwisdom is applied to the other return site in the subroutine.\n\nNow, the subroutine's body essentially is:\n\n\twhile (!extract_valid($address)) {\n\t\t...\n if (...) {\n \treturn undef; # or just \"return;\"\n\t\t}\n\t}\n\treturn $address;\n\nYou can prove that the \"return $address\" on the last line will never\nreturn 'undef' by proving that extract_valid() never returns true\nfor 'undef' input. With that, we can safely change the error return\nto do \"return;\" in the loop.\n\nWhen it is done, the subroutine's new interface becomes \"Single\naddress goes in, either a single valid address comes out and that\nwill never be an undef, or nothing (not even an undef) comes out\nupon error\".\n\nWhich means the sole caller of the subroutine needs to be updated,\nwhich currently does this:\n\n\treturn grep { defined $_ } map { validate_address($_) } @_;\n\nAs the subroutine never returns an undef as \"no this is not valid\",\nbut acts more like a filter to cull bad ones from the input, the\nouter grep { defined $_ } becomes unnecessary. \n\nAnd I think that change logically belongs to the same patch as the\none that inspects how validate_address behaves and updates its\n\"sometimes we return undef to signal a bad input\" to return nothing.\n\nThat is why validate_adress was excluded from [1/3] which deals with\nother simpler cases.\n"}]}