Re: [PATCH 2/3] send-email: drop misleading function prototype
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Apr 1, 2013, 02:20 UTC
- Message-ID
- <20130401022024.GC11704@elie.Belkin>
- In-Reply-To
- <1364780442-6015-2-git-send-email-gitster@pobox.com>
Junio C Hamano wrote:
Show 12 quoted lines
> 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