[PATCH] git-send-email.perl: implement suggestions made by perlcritic

[PATCH] git-send-email.perl: implement suggestions made by perlcritic

[Ramkumar Ramachandra]

Running perlcritic with gentle severity reports six problems.  The
following lists the line numbers on which the problems occur, along
with a description of the problem.  This patch fixes them all.

516: Contrary to common belief, subroutine prototypes do not enable
compile-time checks for proper arguments.  They serve the singular
purpose of defining functions that behave like built-in functions, but
check_file_rev_conflict was never intended to behave like one.

714, 836, 855, 1487: Returning `undef' upon failure from a subroutine
is pretty common. But if the subroutine is called in list context, an
explicit `return undef;' statement will return a one-element list
containing `(undef)'. Now if that list is subsequently put in a
boolean context to test for failure, then it evaluates to true. But
you probably wanted it to be false.  The solution is to just use a
bare `return' statement whenever you want to return failure. In list
context, Perl will then give you an empty list (which is false), and
`undef' in scalar context (which is also false).

1441: The three-argument form of `open' (introduced in Perl 5.6)
prevents subtle bugs that occur when the filename starts with funny
characters like '>' or '<'.  It's also more explicitly clear to define
the input mode of the file.  This policy will not complain if the file
explicitly states that it is compatible with a version of Perl prior
to 5.6 via an include statement (see 'use 5.008' near the top of the
file).

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 I was looking at git-send-email.perl after I posted that bug report
 about SSL_verify.  What better way to get started than Perl Best
 Practices?  Here's a nice patch to fix some obvious style issues.  I
 didn't run perlcritic on a higher severity, because I don't know
 enough Perl to have a well-formed opinion of my own; I'm just fixing
 the glaringly obvious problems.

 git-send-email.perl | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index be809e5..e974b11 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -513,7 +513,7 @@ if (@alias_files and $aliasfiletype and defined $parse_alias{$aliasfiletype}) {
 ($sender) = expand_aliases($sender) if defined $sender;
 
 # returns 1 if the conflict must be solved using it as a format-patch argument
-sub check_file_rev_conflict($) {
+sub check_file_rev_conflict {
 	return unless $repo;
 	my $f = shift;
 	try {
@@ -711,7 +711,7 @@ sub ask {
 			}
 		}
 	}
-	return undef;
+	return;
 }
 
 my %broken_encoding;
@@ -833,7 +833,7 @@ sub extract_valid_address {
 	# less robust/correct than the monster regexp in Email::Valid,
 	# but still does a 99% job, and one less dependency
 	return $1 if $address =~ /($local_part_regexp\@$domain_regexp)/;
-	return undef;
+	return;
 }
 
 sub extract_valid_address_or_die {
@@ -852,7 +852,7 @@ sub validate_address {
 			valid_re => qr/^(?:quit|q|drop|d|edit|e)/i,
 			default => 'q');
 		if (/^d/i) {
-			return undef;
+			return;
 		} elsif (/^q/i) {
 			cleanup_compose_files();
 			exit(0);
@@ -1438,7 +1438,7 @@ sub recipients_cmd {
 
 	my $sanitized_sender = sanitize_address($sender);
 	my @addresses = ();
-	open my $fh, "$cmd \Q$file\E |"
+	open my $fh, q{-|}, "$cmd \Q$file\E"
 	    or die "($prefix) Could not execute '$cmd'";
 	while (my $address = <$fh>) {
 		$address =~ s/^\s*//g;
@@ -1484,7 +1484,7 @@ sub validate_patch {
 			return "$.: patch contains a line longer than 998 characters";
 		}
 	}
-	return undef;
+	return;
 }
 
 sub file_has_nonascii {
-- 
1.8.2.62.ga35d936.dirty

Re: [PATCH] git-send-email.perl: implement suggestions made by perlcritic

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> diff --git a/git-send-email.perl b/git-send-email.perl
> index be809e5..e974b11 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -513,7 +513,7 @@ if (@alias_files and $aliasfiletype and defined $parse_alias{$aliasfiletype}) {
>  ($sender) = expand_aliases($sender) if defined $sender;
>  
>  # returns 1 if the conflict must be solved using it as a format-patch argument
> -sub check_file_rev_conflict($) {
> +sub check_file_rev_conflict {

Have you verified that the callers of this sub are OK with this
change?  It used to force scalar context on its arguments but now it
does not.

I am not saying I know the callers will get broken.  I am trying to
make sure that this is *not* the result of blindly following
perlcritic output without understanding the ramifications of the
change.

> @@ -1438,7 +1438,7 @@ sub recipients_cmd {
>  
>  	my $sanitized_sender = sanitize_address($sender);
>  	my @addresses = ();
> -	open my $fh, "$cmd \Q$file\E |"
> +	open my $fh, q{-|}, "$cmd \Q$file\E"

Strange quoting (why not just say "-|"?) aside, if you are moving
away from the two-param form of open(), it would be a sane thing to
do to also stop concatenating the arguments to commands to avoid
shell metacharacter gotcha.  It will make the resulting code much
safer.

Re: [PATCH] git-send-email.perl: implement suggestions made by perlcritic

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index be809e5..e974b11 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -513,7 +513,7 @@ if (@alias_files and $aliasfiletype and defined $parse_alias{$aliasfiletype}) {
>>  ($sender) = expand_aliases($sender) if defined $sender;
>>
>>  # returns 1 if the conflict must be solved using it as a format-patch argument
>> -sub check_file_rev_conflict($) {
>> +sub check_file_rev_conflict {
>
> Have you verified that the callers of this sub are OK with this
> change?  It used to force scalar context on its arguments but now it
> does not.
>
> I am not saying I know the callers will get broken.  I am trying to
> make sure that this is *not* the result of blindly following
> perlcritic output without understanding the ramifications of the
> change.

No Junio, I haven't blindly followed the perlcritic output.  I've
considered the ramifications, audited the callers, and run the tests
to make sure that nothing breaks.

>> @@ -1438,7 +1438,7 @@ sub recipients_cmd {
>>
>>       my $sanitized_sender = sanitize_address($sender);
>>       my @addresses = ();
>> -     open my $fh, "$cmd \Q$file\E |"
>> +     open my $fh, q{-|}, "$cmd \Q$file\E"
>
> Strange quoting (why not just say "-|"?) aside

Intentional.  I thought it was a pretty way to differentiate between a
mode string (which can only be <, >, or -|) and a filename string.  If
you don't share my taste, feel free to use quotes instead.

> , if you are moving
> away from the two-param form of open(), it would be a sane thing to
> do to also stop concatenating the arguments to commands to avoid
> shell metacharacter gotcha.  It will make the resulting code much
> safer.

Yes, the file can be improved in many ways, but that is not the topic
of this series.  This series just makes the changes suggested by
perlcritic gentle.

Re: [PATCH] git-send-email.perl: implement suggestions made by perlcritic

[Ramkumar Ramachandra]

Junio,

I don't see this queued in 'pu'.  Do you have any objections to the
patch, or did you just forget?

Thanks.

Re: [PATCH] git-send-email.perl: implement suggestions made by perlcritic

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> I don't see this queued in 'pu'.  Do you have any objections to the
> patch, or did you just forget?

I do not recall the details but I think I was expecting a re-roll
updating the log message (if the original invited questions, there
must have been some room for improvement to avoid such confusions,
right?) for a few days since the exchange, and then forgot.

[PATCH v2] git-send-email.perl: implement suggestions made by perlcritic

[Ramkumar Ramachandra]

Running perlcritic with gentle severity reports six problems.  The
following lists the line numbers on which the problems occur, along
with a description of the problem.  This patch fixes them all, after
carefully considering the consequences.

516: Contrary to common belief, subroutine prototypes do not enable
compile-time checks for proper arguments.  They serve the singular
purpose of defining functions that behave like built-in functions, but
check_file_rev_conflict was never intended to behave like one.  We
have verified that the callers of the subroutines are alright with the
change.

714, 836, 855, 1487: Returning `undef' upon failure from a subroutine
is pretty common. But if the subroutine is called in list context, an
explicit `return undef;' statement will return a one-element list
containing `(undef)'. Now if that list is subsequently put in a
boolean context to test for failure, then it evaluates to true. But
you probably wanted it to be false.  The solution is to just use a
bare `return' statement whenever you want to return failure. In list
context, Perl will then give you an empty list (which is false), and
`undef' in scalar context (which is also false).

1441: The three-argument form of `open' (introduced in Perl 5.6)
prevents subtle bugs that occur when the filename starts with funny
characters like '>' or '<'.  It's also more explicitly clear to define
the input mode of the file.  This policy will not complain if the file
explicitly states that it is compatible with a version of Perl prior
to 5.6 via an include statement (see 'use 5.008' near the top of the
file).

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Junio: In future, please tell me explicitly that you're expecting a
 re-roll with an updated commit message.  It wasn't obvious to me at
 all.

 git-send-email.perl | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index c3501d9..633f447 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -513,7 +513,7 @@ if (@alias_files and $aliasfiletype and defined $parse_alias{$aliasfiletype}) {
 ($sender) = expand_aliases($sender) if defined $sender;
 
 # returns 1 if the conflict must be solved using it as a format-patch argument
-sub check_file_rev_conflict($) {
+sub check_file_rev_conflict {
 	return unless $repo;
 	my $f = shift;
 	try {
@@ -711,7 +711,7 @@ sub ask {
 			}
 		}
 	}
-	return undef;
+	return;
 }
 
 my %broken_encoding;
@@ -833,7 +833,7 @@ sub extract_valid_address {
 	# less robust/correct than the monster regexp in Email::Valid,
 	# but still does a 99% job, and one less dependency
 	return $1 if $address =~ /($local_part_regexp\@$domain_regexp)/;
-	return undef;
+	return;
 }
 
 sub extract_valid_address_or_die {
@@ -852,7 +852,7 @@ sub validate_address {
 			valid_re => qr/^(?:quit|q|drop|d|edit|e)/i,
 			default => 'q');
 		if (/^d/i) {
-			return undef;
+			return;
 		} elsif (/^q/i) {
 			cleanup_compose_files();
 			exit(0);
@@ -1453,7 +1453,7 @@ sub recipients_cmd {
 
 	my $sanitized_sender = sanitize_address($sender);
 	my @addresses = ();
-	open my $fh, "$cmd \Q$file\E |"
+	open my $fh, q{-|}, "$cmd \Q$file\E"
 	    or die "($prefix) Could not execute '$cmd'";
 	while (my $address = <$fh>) {
 		$address =~ s/^\s*//g;
@@ -1499,7 +1499,7 @@ sub validate_patch {
 			return "$.: patch contains a line longer than 998 characters";
 		}
 	}
-	return undef;
+	return;
 }
 
 sub file_has_nonascii {
-- 
1.8.2.141.g07926c6

Re: [PATCH v2] git-send-email.perl: implement suggestions made by perlcritic

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

>  Junio: In future, please tell me explicitly that you're expecting a
>  re-roll with an updated commit message.  It wasn't obvious to me at
>  all.

When there are questions in response to a patch, there are two
possibilities:

 * temporary brainfart --- sorry for the bother
 * the clarity of the patch or commit message has room for improvement

This wasn't a case of the former, so a seasoned contributor could
safely assume that it was definitely a case of the latter.

The explanation in Linux kernel's Documentation/SubmittingPatches
item 10 ("Don't get discouraged.  Re-submit") has some fitting advice.

Hope that helps,
Jonathan

Re: [PATCH v2] git-send-email.perl: implement suggestions made by perlcritic

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Running perlcritic with gentle severity reports six problems.  The
> following lists the line numbers on which the problems occur, along
> with a description of the problem.  This patch fixes them all, 

Thanks.

> after
> carefully considering the consequences.

Hmmmm.....

> 516: Contrary to common belief, subroutine prototypes do not enable
> compile-time checks for proper arguments.  They serve the singular
> purpose of defining functions that behave like built-in functions, but
> check_file_rev_conflict was never intended to behave like one.  We
> have verified that the callers of the subroutines are alright with the
> change.

This, together with the "carefully considering", does not build any
confidence on the part of the reader.  Subroutine prototypes are not
for compile-time checks (correct), and they were introduced to the
language only for the purpose of letting you write subroutine that
emulate built-ins like "pop @ary" (again correct),

The most important fact that is not described in the above is that
by using prototype, the subroutine enforces a non-default context at
the callsite when the arguments are evaluated. That is why you can
write an emulated "pop"; otherwise the first @ary will simply be
flattened and you won't grab an element from the array.

Saying "check_file_rev_conflict is not emulating any Perl built-in
function" is irrelevant as a justification for the change.  A
subroutine that does not emulate a built-in can still be relying on
the non-default context in which its arguments are evaluated.

    sub foo ($) { my ($arg) = @_; print "$arg\n"; }
    sub bar     { my ($arg) = @_; print "$arg\n"; }
    my @baz = (100, 101, 102);
    foo @baz; # says 3
    bar @baz; # says 100

In general, writing subroutines without prototypes is much
preferred. I do not dispute that and would not argue against
perlcritic. But if you blindly _fix_ the above "foo" subroutine by
dropping its prototype, it changes behaviour if the callers passed
an array variable. You need to check the callers and they are not
doing something to cause the prototyped and unprototyped versions
to behave differently.

And that is what needs to be explained in the log message; not these
handwavy "carefully considering consequences" (what consequences did
you consider?), "was never intended to behave like one" (how does
that matter?) and "the callers of the subroutines are alright" (why
do you think so?).

Without that, the reviewer needs to go and check the callers.  Your
log message is _not_ helping them.

Same for the remainder.

In general, you do not have to copy and paste the output from
perlcritic.  Treat it more as a learning tool, and use the knowledge
you learned from its output to explain why your changes are
improvements.  Not just "because perlcritic said so".

For "ask" subroutine, all its callers assign the returned value to a
single scaler variable, so the difference between "return undef" and
just "return" does not matter. If somebody starts doing

	@foo = ask(...);
	if (@foo) { ... we got an answer ... } else { ... we did not ... }

then "return undef;" form will break.  So it is less error prone if
we dropped the explicit "undef".  The same goes for extract_valid_address,
whose current callers all assign the returned value to a single scalar,
or apply "!" operator inside "while" conditional.

The change to validate_address is correct, but it is correct only
because its only caller, validate_address_list, filters out "undef"
returned from map() that calls this subroutine.  By dropping the
explicit "undef" from there, it seems to me that validate_address no
longer returns "undef" so validate_address_list loses any need to
filter its return value.  Seeing a patch that does not change that
caller while changing the callee makes reviewers wonder what is
going on.  Perhaps even with this patch, there still is a need to
filter in validate_address_list, and if so, that needs to be
explained.

If I were doing this change, I would rather leave this subroutine
as-is.  Nothing is broken and we are risking new breakages by
changing it.

> 1441: The three-argument form of `open' (introduced in Perl 5.6)
> prevents subtle bugs that occur when the filename starts with funny
> characters like '>' or '<'.

Correct, and this patch is about using the three-or-more-arg form to
take advantage of its safety.  So why are we still using the shell
invocation form inherited back from the days we used two-arg form?

Again, seeing a patch that only turns "open FILEHANDLE,EXPR" into
"open FILEHANDLE,MODE,EXPR" when EXPR is not a simple command name
and not into "open FILEHANDLE,MODE,EXPR,LIST" form makes reviewers
wonder why the patch stops in the middle.

>  Junio: In future, please tell me explicitly that you're expecting a
>  re-roll with an updated commit message.  It wasn't obvious to me at
>  all.

It wasn't obvious to me, either ;-).

I said the patch was not explained well, but it may not be the only
problem with it.  Other people may have inputs to it, and you may
have been waiting for the input from others before initiating a
reroll.

It wasn't like "please fix these and then the result will be perfect
and I'll promise to apply".  A maintainer cannot work like that.

I try to make sure there aren't leftover bits from time to time by
sweeping archived e-mails, and also I try to handhold newer
contributors by pinging them about their progress every once in a
while (but you are not exactly new).

This patch fell under the cracks, and reminding me with a "what
happened to it?" was the right thing to do.  Literally, that is what
I ask in the "Notes from the maintainer" message.

Re: [PATCH v2] git-send-email.perl: implement suggestions made by perlcritic

[Ramkumar Ramachandra]

Junio C Hamano wrote:
>     sub foo ($) { my ($arg) = @_; print "$arg\n"; }
>     sub bar     { my ($arg) = @_; print "$arg\n"; }
>     my @baz = (100, 101, 102);
>     foo @baz; # says 3
>     bar @baz; # says 100

Ouch.  Please drop this patch; I'll resubmit when I feel confident
about my change.

> This patch fell under the cracks, and reminding me with a "what
> happened to it?" was the right thing to do.  Literally, that is what
> I ask in the "Notes from the maintainer" message.

Right.  Thanks for clarifying.  I'll actively track the patches I submit.

Re: [PATCH v2] git-send-email.perl: implement suggestions made by perlcritic

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>>     sub foo ($) { my ($arg) = @_; print "$arg\n"; }
>>     sub bar     { my ($arg) = @_; print "$arg\n"; }
>>     my @baz = (100, 101, 102);
>>     foo @baz; # says 3
>>     bar @baz; # says 100
>
> Ouch.  Please drop this patch; I'll resubmit when I feel confident
> about my change.

No, let's not do that.  I will forget and end up spending time to
read the same patch again.

[PATCH 1/3] send-email: use "return;" not "return undef;" on error codepaths

[Junio C Hamano]

From: Ramkumar Ramachandra <artagnon@gmail.com>

All the callers of "ask", "extract_valid_address", and "validate_patch"
subroutines assign the return values from them to a single scaler:

	$var = subr(...);

and "return undef;" in these subroutine can safely be turned into a
simpler "return;".  Doing so will also future-proof a new caller that
mistakenly does this:

    @foo = ask(...);
    if (@foo) { ... we got an answer ... } else { ... we did not ... }

Note that we leave "return undef;" in validate_address on purpose,
even though Perlcritic may complain.  The primary "return" site of
the function returns whatever is in the scaler variable $address, so
it is pointless to change only the other "return undef;" to "return".
The caller must be prepared to see an array with a single undef as
the return value from this subroutine anyway.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-send-email.perl | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index be809e5..79cc5be 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -711,7 +711,7 @@ sub ask {
 			}
 		}
 	}
-	return undef;
+	return;
 }
 
 my %broken_encoding;
@@ -833,7 +833,7 @@ sub extract_valid_address {
 	# less robust/correct than the monster regexp in Email::Valid,
 	# but still does a 99% job, and one less dependency
 	return $1 if $address =~ /($local_part_regexp\@$domain_regexp)/;
-	return undef;
+	return;
 }
 
 sub extract_valid_address_or_die {
@@ -1484,7 +1484,7 @@ sub validate_patch {
 			return "$.: patch contains a line longer than 998 characters";
 		}
 	}
-	return undef;
+	return;
 }
 
 sub file_has_nonascii {
-- 
1.8.2-441-g6e6f07b

[PATCH 2/3] send-email: drop misleading function prototype

[Junio C Hamano]

From: Ramkumar Ramachandra <artagnon@gmail.com>

The subroutine check_file_rev_conflict() is called from two places,
both of which expects to pass a single scaler variable and see if
that can be interpreted as a pathname or a revision name.  It is
defined with a function prototype ($) to force a scaler context
while evaluating the arguments at the calling site but it does not
help the current calling sites.  The only effect it has is to hurt
future calling sites that may want to build an argument list in an
array variable and call it as check_file_rev_confict(@args).

Drop the misleading prototype, as Perlcritic suggests.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-send-email.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 79cc5be..86dd593 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -513,7 +513,7 @@ sub split_addrs {
 ($sender) = expand_aliases($sender) if defined $sender;
 
 # returns 1 if the conflict must be solved using it as a format-patch argument
-sub check_file_rev_conflict($) {
+sub check_file_rev_conflict {
 	return unless $repo;
 	my $f = shift;
 	try {
-- 
1.8.2-441-g6e6f07b

[PATCH 3/3] send-email: use the three-arg form of open in recipients_cmd

[Junio C Hamano]

From: Ramkumar Ramachandra <artagnon@gmail.com>

Perlcritic does not want to see the trailing pipe in the two-args
form of open(), i.e.

	open my $fh, "$cmd \Q$file\E |";

If $cmd were a single-token command name, it would make a lot more
sense to use four-or-more-args form "open FILEHANDLE,MODE,CMD,ARGS"
to avoid shell from expanding metacharacters in $file, but we do
expect multi-word string in $to_cmd and $cc_cmd to be expanded by
the shell, so we cannot rewrite it to

	open my $fh, "-|", $cmd, $file;

for extra safety.  At least, by using this in the three-arg form:

	open my $fh, "-|", "$cmd \Q$file\E";

we can silence Perlcritique, even though we do not gain much safety
by doing so.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-send-email.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 86dd593..fb92227 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1438,7 +1438,7 @@ sub recipients_cmd {
 
 	my $sanitized_sender = sanitize_address($sender);
 	my @addresses = ();
-	open my $fh, "$cmd \Q$file\E |"
+	open my $fh, "-|", "$cmd \Q$file\E"
 	    or die "($prefix) Could not execute '$cmd'";
 	while (my $address = <$fh>) {
 		$address =~ s/^\s*//g;
-- 
1.8.2-441-g6e6f07b

Re: [PATCH 2/3] send-email: drop misleading function prototype

[Jonathan Nieder]

Junio C Hamano wrote:

> From: Ramkumar Ramachandra <artagnon@gmail.com>
>
> The subroutine check_file_rev_conflict() is called from two places,
> both of which expects to pass a single scaler variable and see if
> that can be interpreted as a pathname or a revision name.  It is
> defined with a function prototype ($) to force a scaler context
> while evaluating the arguments at the calling site but it does not
> help the current calling sites.  The only effect it has is to hurt
> future calling sites that may want to build an argument list in an
> array variable and call it as check_file_rev_confict(@args).
>
> Drop the misleading prototype, as Perlcritic suggests.

Nice explanation.  Here's a similar patch I wrote before I noticed
your patch, with commit message stolen from the above.

-- >8 --
Subject: send-email: drop misleading function prototype

The subroutine check_file_rev_conflict() is called from two places,
both of which expects to pass a single scaler variable and see if
that can be interpreted as a pathname or a revision name.  It is
defined with a function prototype ($) to force a scaler context
while evaluating the arguments at the calling site but it does not
help the current calling sites.  The only effect it has is to hurt
future calling sites that may want to build an argument list in an
array variable and call it as check_file_rev_confict(@args).

Drop the misleading prototype, as Perlcritic suggests.

While at it, rename the function to avoid new call sites unaware of
this change arising and add a comment clarifying what this function
is for.

Reported-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 git-send-email.perl | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index c3501d98..d6b3c32b 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -512,8 +512,9 @@ sub split_addrs {
 
 ($sender) = expand_aliases($sender) if defined $sender;
 
-# returns 1 if the conflict must be solved using it as a format-patch argument
-sub check_file_rev_conflict($) {
+# is_format_patch_arg($f) returns 0 if $f names a patch, or 1 if
+# $f is a revision list specification to be passed to format-patch.
+sub is_format_patch_arg {
 	return unless $repo;
 	my $f = shift;
 	try {
@@ -529,6 +530,7 @@ ($)
     * Giving --format-patch option if you mean a range.
 EOF
 	} catch Git::Error::Command with {
+		# Not a valid revision.  Treat it as a filename.
 		return 0;
 	}
 }
@@ -540,14 +542,14 @@ ($)
 	if ($f eq "--") {
 		push @rev_list_opts, "--", @ARGV;
 		@ARGV = ();
-	} elsif (-d $f and !check_file_rev_conflict($f)) {
+	} elsif (-d $f and !is_format_patch_arg($f)) {
 		opendir my $dh, $f
 			or die "Failed to opendir $f: $!";
 
 		push @files, grep { -f $_ } map { catfile($f, $_) }
 				sort readdir $dh;
 		closedir $dh;
-	} elsif ((-f $f or -p $f) and !check_file_rev_conflict($f)) {
+	} elsif ((-f $f or -p $f) and !is_format_patch_arg($f)) {
 		push @files, $f;
 	} else {
 		push @rev_list_opts, $f;
-- 
1.8.2

Re: [PATCH 3/3] send-email: use the three-arg form of open in recipients_cmd

[Jonathan Nieder]

Junio C Hamano wrote:

>               we cannot rewrite it to
>
> 	open my $fh, "-|", $cmd, $file;
>
> for extra safety.  At least, by using this in the three-arg form:
>
> 	open my $fh, "-|", "$cmd \Q$file\E";
>
> we can silence Perlcritique, even though we do not gain much safety
> by doing so.

Yeah, I think this is the right thing to do.

This means that if some later code refactoring parses $tocmd once and
passes an array around, it would be easy to change this to

	open my $fh, "-|", @$cmd, $file;

and there would be no temptation to do something involving pasting
@$cmd back together into a single string.  Of course such a
refactoring is not very likely, but that kind of thing is a good
reason to prefer this style in general.

So for what it's worth, I like this patch.

Re: [PATCH 1/3] send-email: use "return;" not "return undef;" on error codepaths

[Eric Sunshine]

On Sun, Mar 31, 2013 at 9:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> All the callers of "ask", "extract_valid_address", and "validate_patch"
> subroutines assign the return values from them to a single scaler:

s/scaler/scalar/g

(note the /g)

>
>         $var = subr(...);
>
> and "return undef;" in these subroutine can safely be turned into a
> simpler "return;".  Doing so will also future-proof a new caller that
> mistakenly does this:
>
>     @foo = ask(...);
>     if (@foo) { ... we got an answer ... } else { ... we did not ... }
>
> Note that we leave "return undef;" in validate_address on purpose,
> even though Perlcritic may complain.  The primary "return" site of
> the function returns whatever is in the scaler variable $address, so
> it is pointless to change only the other "return undef;" to "return".
> The caller must be prepared to see an array with a single undef as
> the return value from this subroutine anyway.

Re: [PATCH 2/3] send-email: drop misleading function prototype

[Eric Sunshine]

On Sun, Mar 31, 2013 at 9:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> The subroutine check_file_rev_conflict() is called from two places,
> both of which expects to pass a single scaler variable and see if

s/scaler/scalar/g

(note the /g)

> that can be interpreted as a pathname or a revision name.  It is
> defined with a function prototype ($) to force a scaler context
> while evaluating the arguments at the calling site but it does not
> help the current calling sites.  The only effect it has is to hurt
> future calling sites that may want to build an argument list in an
> array variable and call it as check_file_rev_confict(@args).
>
> Drop the misleading prototype, as Perlcritic suggests.

Re: [PATCH v2] git-send-email.perl: implement suggestions made by perlcritic

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>> Ouch.  Please drop this patch; I'll resubmit when I feel confident
>> about my change.
>
> No, let's not do that.  I will forget and end up spending time to
> read the same patch again.

All three look good to me.  Thanks for doing this: I learnt quite a
bit from your commit messages.

Re: [PATCH 3/3] send-email: use the three-arg form of open in recipients_cmd

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> we can silence Perlcritique, even though we do not gain much safety
> by doing so.

Nit: it's perlcritic; critique is used to refer to the output of a critic.

Re: [PATCH 1/3] send-email: use "return;" not "return undef;" on error codepaths

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Note that we leave "return undef;" in validate_address on purpose,
> even though Perlcritic may complain.  The primary "return" site of
> the function returns whatever is in the scaler variable $address, so
> it is pointless to change only the other "return undef;" to "return".
> The caller must be prepared to see an array with a single undef as
> the return value from this subroutine anyway.

The way I understood it:
All callsites only ever call validate_address via
validate_address_list, which in turn maps a list of addresses by
calling validate_address on each address.  Therefore, validate_address
returning an undef in one codepath is correct.

Re: [PATCH 1/3] send-email: use "return;" not "return undef;" on error codepaths

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> Note that we leave "return undef;" in validate_address on purpose,
>> even though Perlcritic may complain.  The primary "return" site of
>> the function returns whatever is in the scaler variable $address, so
>> it is pointless to change only the other "return undef;" to "return".
>> The caller must be prepared to see an array with a single undef as
>> the return value from this subroutine anyway.
>
> The way I understood it:
> All callsites only ever call validate_address via
> validate_address_list, which in turn maps a list of addresses by
> calling validate_address on each address.  Therefore, validate_address
> returning an undef in one codepath is correct.

I think we are in agreement then.

The implication of what you just said is that anybody that calls
this subroutine _must_ be (and indeed is) prepared to see a single
'undef' returned from it, hence the use pattern,

	@foo = validate_address(...);
        if (@foo) { ... we got a valid one ... } else { ... we did not ... }

cannot be used for the subroutine _anyway_.  @foo may get an invalid
address, even with the "don't say 'return undef;' say 'return;'"
wisdom is applied to the other return site in the subroutine.

Now, the subroutine's body essentially is:

	while (!extract_valid($address)) {
		...
                if (...) {
                	return undef; # or just "return;"
		}
	}
	return $address;

You can prove that the "return $address" on the last line will never
return 'undef' by proving that extract_valid() never returns true
for 'undef' input.  With that, we can safely change the error return
to do "return;" in the loop.

When it is done, the subroutine's new interface becomes "Single
address goes in, either a single valid address comes out and that
will never be an undef, or nothing (not even an undef) comes out
upon error".

Which means the sole caller of the subroutine needs to be updated,
which currently does this:

	return grep { defined $_ } map { validate_address($_) } @_;

As the subroutine never returns an undef as "no this is not valid",
but acts more like a filter to cull bad ones from the input, the
outer grep { defined $_ } becomes unnecessary.  

And I think that change logically belongs to the same patch as the
one that inspects how validate_address behaves and updates its
"sometimes we return undef to signal a bad input" to return nothing.

That is why validate_adress was excluded from [1/3] which deals with
other simpler cases.