[PATCH 0/6] cleanups for git-send-email

[PATCH 0/6] cleanups for git-send-email

[Bill Pemberton]

The following are some code cleanups for git-send-email.perl.  They're
based on suggestions by perlcritc.

Bill Pemberton (6):
  Remove return undef from validate_patch
  Remove function prototypes from git-send-email.perl
  Remove return undef from ask()
  Add explict return to end of subroutines
  Remove mix of high and low-precedence booleans
  Remove bareword filehandles in git-send-email.perl

 git-send-email.perl |   63 ++++++++++++++++++++++++++------------------------
 1 files changed, 33 insertions(+), 30 deletions(-)

[PATCH 1/6] Remove return undef from validate_patch

[Bill Pemberton]

Returning undef is rarely the correct way to return a failure.
Replace it with return 0

Signed-off-by: Bill Pemberton <wfp5p@virginia.edu>
---
 git-send-email.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index cccbf45..4f62c59 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1182,7 +1182,7 @@ sub validate_patch {
 			return "$.: patch contains a line longer than 998 characters";
 		}
 	}
-	return undef;
+	return 0;
 }
 
 sub file_has_nonascii {
-- 
1.6.0.6

[PATCH 2/6] Remove function prototypes from git-send-email.perl

[Bill Pemberton]

Use of function prototypes is considered bad practice in perl.  The
ones used here didn't accomplish anything anyhow, so they've been
removed.

Signed-off-by: Bill Pemberton <wfp5p@virginia.edu>
---
 git-send-email.perl |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 4f62c59..067aaf0 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -131,9 +131,6 @@ my $have_mail_address = eval { require Mail::Address; 1 };
 my $smtp;
 my $auth;
 
-sub unique_email_list(@);
-sub cleanup_compose_files();
-
 # Variables we fill in automatically, or via prompting:
 my (@to,@cc,@initial_cc,@bcclist,@xh,
 	$initial_reply_to,$initial_subject,@files,
@@ -443,7 +440,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 {
@@ -509,7 +506,7 @@ if (@files) {
 	usage();
 }
 
-sub get_patch_subject($) {
+sub get_patch_subject {
 	my $fn = shift;
 	open (my $fh, '<', $fn);
 	while (my $line = <$fh>) {
@@ -1150,13 +1147,13 @@ foreach my $t (@files) {
 
 cleanup_compose_files();
 
-sub cleanup_compose_files() {
+sub cleanup_compose_files {
 	unlink($compose_filename, $compose_filename . ".final") if $compose;
 }
 
 $smtp->quit if $smtp;
 
-sub unique_email_list(@) {
+sub unique_email_list {
 	my %seen;
 	my @emails;
 
-- 
1.6.0.6

[PATCH 3/6] Remove return undef from ask()

[Bill Pemberton]

Returning undef is rarely the correct way to return a failure.
Replace it with bare return

Signed-off-by: Bill Pemberton <wfp5p@virginia.edu>
---
 git-send-email.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 067aaf0..1ed5869 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -633,7 +633,7 @@ sub ask {
 			return $resp;
 		}
 	}
-	return undef;
+	return;
 }
 
 my $prompting = 0;
-- 
1.6.0.6

[PATCH 4/6] Add explict return to end of subroutines

[Bill Pemberton]

In perl a subroutine that ends without an explicit return will return
the value of the last expression evalutated.  This can lead to
unexpected return values.

Signed-off-by: Bill Pemberton <wfp5p@virginia.edu>
---
 git-send-email.perl |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 1ed5869..c24e0df 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -160,6 +160,7 @@ my $compose_filename;
 # Handle interactive edition of files.
 my $multiedit;
 my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, "core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+
 sub do_edit {
 	if (defined($multiedit) && !$multiedit) {
 		map {
@@ -174,6 +175,7 @@ sub do_edit {
 			die("the editor exited uncleanly, aborting everything");
 		}
 	}
+    return;
 }
 
 # Variables with corresponding config settings
@@ -304,6 +306,7 @@ sub read_config {
 			$smtp_encryption = 'ssl';
 		}
 	}
+    return;
 }
 
 # read configuration from [sendemail "$identity"], fall back on [sendemail]
@@ -745,6 +748,7 @@ sub make_message_id
 	my $message_id_template = "<%s-git-send-email-%s>";
 	$message_id = sprintf($message_id_template, $uniq, $du_part);
 	#print "new message id = $message_id\n"; # Was useful for debugging
+    return;
 }
 
 
@@ -971,6 +975,7 @@ X-Mailer: git-send-email $gitversion
 			print "Result: OK\n";
 		}
 	}
+   return;
 }
 
 $reply_to = $initial_reply_to;
@@ -1149,6 +1154,7 @@ cleanup_compose_files();
 
 sub cleanup_compose_files {
 	unlink($compose_filename, $compose_filename . ".final") if $compose;
+        return;
 }
 
 $smtp->quit if $smtp;
-- 
1.6.0.6

[PATCH 5/6] Remove mix of high and low-precedence booleans

[Bill Pemberton]

Booleans such as &&, ||, ! have higher precedence than and, or, not.
They should not be mixed.

Signed-off-by: Bill Pemberton <wfp5p@virginia.edu>
---
 git-send-email.perl |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index c24e0df..5e7295d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -471,14 +471,14 @@ while (defined(my $f = shift @ARGV)) {
 	if ($f eq "--") {
 		push @rev_list_opts, "--", @ARGV;
 		@ARGV = ();
-	} elsif (-d $f and !check_file_rev_conflict($f)) {
+	} elsif (-d $f && !check_file_rev_conflict($f)) {
 		opendir(DH,$f)
 			or die "Failed to opendir $f: $!";
 
 		push @files, grep { -f $_ } map { +$f . "/" . $_ }
 				sort readdir(DH);
 		closedir(DH);
-	} elsif ((-f $f or -p $f) and !check_file_rev_conflict($f)) {
+	} elsif ((-f $f || -p $f) && !check_file_rev_conflict($f)) {
 		push @files, $f;
 	} else {
 		push @rev_list_opts, $f;
@@ -632,7 +632,7 @@ sub ask {
 		if ($resp eq '' and defined $default) {
 			return $default;
 		}
-		if (!defined $valid_re or $resp =~ /$valid_re/) {
+		if (!defined $valid_re || $resp =~ /$valid_re/) {
 			return $resp;
 		}
 	}
-- 
1.6.0.6

[PATCH 6/6] Remove bareword filehandles in git-send-email.perl

[Bill Pemberton]

The script was using bareword filehandles.  This is considered a bad
practice so they have been changed to indirect filehandles.

Signed-off-by: Bill Pemberton <wfp5p@virginia.edu>
---
 git-send-email.perl |   36 ++++++++++++++++++------------------
 1 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 5e7295d..7068041 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -527,7 +527,7 @@ if ($compose) {
 	$compose_filename = ($repo ?
 		tempfile(".gitsendemail.msg.XXXXXX", DIR => $repo->repo_path()) :
 		tempfile(".gitsendemail.msg.XXXXXX", DIR => "."))[1];
-	open(C,">",$compose_filename)
+	open my $C,'>',$compose_filename
 		or die "Failed to open for writing $compose_filename: $!";
 
 
@@ -535,7 +535,7 @@ if ($compose) {
 	my $tpl_subject = $initial_subject || '';
 	my $tpl_reply_to = $initial_reply_to || '';
 
-	print C <<EOT;
+	print {$C} <<EOT;
 From $tpl_sender # This line is ignored.
 GIT: Lines beginning in "GIT: " will be removed.
 GIT: Consider including an overall diffstat or table of contents
@@ -548,9 +548,9 @@ In-Reply-To: $tpl_reply_to
 
 EOT
 	for my $f (@files) {
-		print C get_patch_subject($f);
+		print {$C} get_patch_subject($f);
 	}
-	close(C);
+	close($C);
 
 	my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, "core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
 
@@ -560,23 +560,23 @@ EOT
 		do_edit($compose_filename);
 	}
 
-	open(C2,">",$compose_filename . ".final")
+	open my $C2,'>',$compose_filename . ".final"
 		or die "Failed to open $compose_filename.final : " . $!;
 
-	open(C,"<",$compose_filename)
+	open $C, '<',$compose_filename
 		or die "Failed to open $compose_filename : " . $!;
 
 	my $need_8bit_cte = file_has_nonascii($compose_filename);
 	my $in_body = 0;
 	my $summary_empty = 1;
-	while(<C>) {
+	while(<$C>) {
 		next if m/^GIT: /;
 		if ($in_body) {
 			$summary_empty = 0 unless (/^\n$/);
 		} elsif (/^\n$/) {
 			$in_body = 1;
 			if ($need_8bit_cte) {
-				print C2 "MIME-Version: 1.0\n",
+				print {$C2} "MIME-Version: 1.0\n",
 					 "Content-Type: text/plain; ",
 					   "charset=utf-8\n",
 					 "Content-Transfer-Encoding: 8bit\n";
@@ -601,10 +601,10 @@ EOT
 			print "To/Cc/Bcc fields are not interpreted yet, they have been ignored\n";
 			next;
 		}
-		print C2 $_;
+		print {$C2} $_;
 	}
-	close(C);
-	close(C2);
+	close($C);
+	close($C2);
 
 	if ($summary_empty) {
 		print "Summary email is empty, skipping it\n";
@@ -984,7 +984,7 @@ $subject = $initial_subject;
 $message_num = 0;
 
 foreach my $t (@files) {
-	open(F,"<",$t) or die "can't open file $t";
+	open my $F,'<',$t or die "can't open file $t";
 
 	my $author = undef;
 	my $author_encoding;
@@ -997,7 +997,7 @@ foreach my $t (@files) {
 	$message = "";
 	$message_num++;
 	# First unfold multiline header fields
-	while(<F>) {
+	while(<$F>) {
 		last if /^\s*$/;
 		if (/^\s+\S/ and @header) {
 			chomp($header[$#header]);
@@ -1073,7 +1073,7 @@ foreach my $t (@files) {
 		}
 	}
 	# Now parse the message body
-	while(<F>) {
+	while(<$F>) {
 		$message .=  $_;
 		if (/^(Signed-off-by|Cc): (.*)$/i) {
 			chomp;
@@ -1090,12 +1090,12 @@ foreach my $t (@files) {
 				$c, $_) unless $quiet;
 		}
 	}
-	close F;
+	close $F;
 
 	if (defined $cc_cmd && !$suppress_cc{'cccmd'}) {
-		open(F, "$cc_cmd $t |")
+		open my $F, '-|', "$cc_cmd $t"
 			or die "(cc-cmd) Could not execute '$cc_cmd'";
-		while(<F>) {
+		while(<$F>) {
 			my $c = $_;
 			$c =~ s/^\s*//g;
 			$c =~ s/\n$//g;
@@ -1104,7 +1104,7 @@ foreach my $t (@files) {
 			printf("(cc-cmd) Adding cc: %s from: '%s'\n",
 				$c, $cc_cmd) unless $quiet;
 		}
-		close F
+		close $F
 			or die "(cc-cmd) failed to close pipe to '$cc_cmd'";
 	}
 
-- 
1.6.0.6

[PATCH 5/6] Re: Remove mix of high and low-precedence booleans

[Nicolas Sebrecht]

On Wed, Apr 29, 2009 at 09:12:22AM -0400, Bill Pemberton wrote:

> Booleans such as &&, ||, ! have higher precedence than and, or, not.
> They should not be mixed.

But round brackets have higher precedence than both '&&' and 'and',
right?  If so, why thoses changes?

> -	} elsif (-d $f and !check_file_rev_conflict($f)) {
> +	} elsif (-d $f && !check_file_rev_conflict($f)) {

> -	} elsif ((-f $f or -p $f) and !check_file_rev_conflict($f)) {
> +	} elsif ((-f $f || -p $f) && !check_file_rev_conflict($f)) {

> -		if (!defined $valid_re or $resp =~ /$valid_re/) {
> +		if (!defined $valid_re || $resp =~ /$valid_re/) {

-- 
Nicolas Sebrecht

Re: [PATCH 5/6] Re: Remove mix of high and low-precedence booleans

[Bill Pemberton]

> > Booleans such as &&, ||, ! have higher precedence than and, or, not.
> > They should not be mixed.
> 
> But round brackets have higher precedence than both '&&' and 'and',
> right?  If so, why thoses changes?
> 

While those particular statements may be correct, mixing the low
precedence logical operators with the high precedence ones is likely
to introduce bugs down the road.  See the book "Perl Best Practices"
for more information on this.

-- 
Bill

Re: [PATCH 0/6] cleanups for git-send-email

[Junio C Hamano]

Bill Pemberton <wfp5p@virginia.edu> writes:

> The following are some code cleanups for git-send-email.perl.  They're
> based on suggestions by perlcritc.
>
> Bill Pemberton (6):
>   Remove return undef from validate_patch
>   Remove function prototypes from git-send-email.perl
>   Remove return undef from ask()
>   Add explict return to end of subroutines
>   Remove mix of high and low-precedence booleans
>   Remove bareword filehandles in git-send-email.perl

Perl styles are highly personal.

While I admit that the changes these patches bring in match my personal
taste more or less exactly [*1*], there was somebody (sorry I lost track)
who volunteered to take the maintenance responsibility of that program
over to clean up or even rewrite it, so I would rather have that person
have a say in the overall styles of the (rewritten) program.

[Footnote]

*1* ...except for the "and/or vs &&/||" bits, even though I prefer the
latter myself solely because I am old fashioned.

I think it is simply silly to say "precedence of ! and and/or does not
mix".  "!" and "&&" have different precedence and rewriting (A and !B)
into (A && !B) would not make things any better nor worse.  After all,
nobody would have problems with "$a + $b * $c" even though + and * have
different precedence.

Oh, I also do not agree with "always explicitly return".  If the change
and explanation were limited to the subs whose return values are _used_, I
would agree with the change, though.

Re: [PATCH 0/6] cleanups for git-send-email

[Bill Pemberton]

> Perl styles are highly personal.
> 

So are C styles, but the kernel and git doesn't allow all sorts of
mixed styles.  My changes are also not just coding style, they have
actual meaning in perl.

My changes come directly from the book "Perl Best Practices".  Just as
you do things like "don't allow assignment in conditionals" in C, even
though it's legal.  There are good reasons to do these things in perl
to prevent bugs down the road.

> 
> *1* ...except for the "and/or vs &&/||" bits, even though I prefer the
> latter myself solely because I am old fashioned.
> 

Again, it prevents bugs.  People use "and" vs "&&" as the same thing,
when they are not.  The have different precedence in perl.

For example, 

next if not $finished || $x < 5;
next if !$finished || $x < 5;

do not mean the same thing.


> I think it is simply silly to say "precedence of ! and and/or does not
> mix".  "!" and "&&" have different precedence and rewriting (A and !B)
> into (A && !B) would not make things any better nor worse.  After all,
> nobody would have problems with "$a + $b * $c" even though + and * have
> different precedence.
> 

It's not that ! and && have different precedence.  It's that "not" and
! have different precedence.  Using your math example, it would be
like having an operator named plus that had a higher precedence than
"*".  Now if you wrote "$a plus $b * $c" it would have different
result than "$a + $b * $c".


> Oh, I also do not agree with "always explicitly return".  If the change
> and explanation were limited to the subs whose return values are _used_, I
> would agree with the change, though.
> 

Again, it prevents potential bugs down the road.  Currently those
functions return something.  While they are not used, the something
they return can be interpreted by developers as an intentional return
value and that property may get used.  If some other developer changes
the original function in some way that the implicit return becomes
something else, it'll create a bug.  If a subroutine isn't supposed to
return a meaningful value, it should do it explicitly.


-- 
Bill Pemberton                                 wfp5p@virginia.edu
ITC/Unix Systems                               flash@virginia.edu
University of Virginia                    

Re: [PATCH 0/6] cleanups for git-send-email

[Junio C Hamano]

wfp5p@viridian.itc.Virginia.EDU (Bill Pemberton) writes:

> My changes come directly from the book "Perl Best Practices".  Just as
> ...
> Again, it prevents bugs.  People use "and" vs "&&" as the same thing,
> when they are not.  The have different precedence in perl.
>
> For example, 
>
> next if not $finished || $x < 5;
> next if !$finished || $x < 5;
>
> do not mean the same thing.
> ...
> Again, it prevents potential bugs down the road....

Earlier I did guide the community not to use "more advanced" (aka
"obscure") Perl features so that people not so familiar with Perl can
still tweak scripts without breaking them; the tricks in your patches that
"prevent potential bugs" are in line with that, and that is why I said my
personal taste more or less agrees with your patch already.

But the line between "more advanced and tricky" and "if you are coding in
Perl you should know your language" is not so black and white as you seem
to think.  I'd rather defer that decision to whoever is taking send-email
over.

[PATCH 0/6] Re: cleanups for git-send-email

[Nicolas Sebrecht]

On Wed, Apr 29, 2009 at 03:48:51PM -0400, Bill Pemberton wrote:

> Again, it prevents bugs.  People use "and" vs "&&" as the same thing,
> when they are not.  The have different precedence in perl.

I agree with you except that the chapter 4.16 from the Perl Best
Practices book does not apply here. FMPOV, we don't really mix booleans
because the precedence is explicitly given by the parentheses.

[ Notice _how_ the author raises the ambiguity to explain his point in
  the book: he uses parentheses. ]

> For example, 
> 
> next if not $finished || $x < 5;
> next if !$finished || $x < 5;
> 
> do not mean the same thing.

True. But the lines we are talking about are different. We have:

	next if ($finished or $x < 5);

If we add a "not"/"!" or append a "&&"/"and" - or whatever -, we do know what will
be evaluated easily:

	next if !($finished or $x < 5);

looks rather different from

	next if (!$finished or $x < 5);

-- 
Nicolas Sebrecht

Re: [PATCH 0/6] Re: cleanups for git-send-email

[Andreas Ericsson]

Nicolas Sebrecht wrote:
> On Wed, Apr 29, 2009 at 03:48:51PM -0400, Bill Pemberton wrote:
> 
>> Again, it prevents bugs.  People use "and" vs "&&" as the same thing,
>> when they are not.  The have different precedence in perl.
> 
> I agree with you except that the chapter 4.16 from the Perl Best
> Practices book does not apply here. FMPOV, we don't really mix booleans
> because the precedence is explicitly given by the parentheses.
> 
> [ Notice _how_ the author raises the ambiguity to explain his point in
>   the book: he uses parentheses. ]
> 
>> For example, 
>>
>> next if not $finished || $x < 5;
>> next if !$finished || $x < 5;
>>
>> do not mean the same thing.
> 
> True. But the lines we are talking about are different. We have:
> 
> 	next if ($finished or $x < 5);
> 
> If we add a "not"/"!" or append a "&&"/"and" - or whatever -, we do know what will
> be evaluated easily:
> 
> 	next if !($finished or $x < 5);
> 
> looks rather different from
> 
> 	next if (!$finished or $x < 5);
> 

I'm rather clueless when it comes to perl coding, but I know what I
don't know, so I've got enough sense to look these things up whenever
I have to hack some perl. I hope others do the same.

Personally, I've found that never using 'or', 'not' or 'and', and
overparenthesize when I'm uncertain seems to work rather nicely,
even though perl gurus would probably shunt my code as overly explicit.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Register now for Nordic Meet on Nagios, June 3-4 in Stockholm
 http://nordicmeetonnagios.op5.org/

Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.

Re: [PATCH 1/6] Remove return undef from validate_patch

[Jeff King]

On Wed, Apr 29, 2009 at 09:12:18AM -0400, Bill Pemberton wrote:

> Returning undef is rarely the correct way to return a failure.
> Replace it with return 0

No, it's the right way to return failure here. The function returns
either an error string, prefixed with the problematic line number, or
undef. So 'undef' is working as a sentinel value here, not as part of a
boolean.

That being said, the _calling_ code is a bit sloppy in checking "$error"
instead of "defined($error)". It is not an actual bug because the
beginning of the string is always a line number >= 1, so it always
triggers as desired. However, it would probably be more clear to write
it like this:

---
diff --git a/git-send-email.perl b/git-send-email.perl
index cccbf45..168b2c2 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -495,7 +495,7 @@ if ($validate) {
 	foreach my $f (@files) {
 		unless (-p $f) {
 			my $error = validate_patch($f);
-			$error and die "fatal: $f: $error\nwarning: no patches were sent\n";
+			defined($error) and die "fatal: $f: $error\nwarning: no patches were sent\n";
 		}
 	}
 }

Re: [PATCH 3/6] Remove return undef from ask()

[Jeff King]

On Wed, Apr 29, 2009 at 09:12:20AM -0400, Bill Pemberton wrote:

> Returning undef is rarely the correct way to return a failure.
> Replace it with bare return

I'm not sure this makes sense. The function is meant to be (and is
always) called in a scalar context, so "return" and "return undef"
produce exactly the same results. I find the latter much more clear,
because it is explicit about what the function is doing: return the
user's desire; if that fails, return the default; if no default, return
undef.

Returning undef can be a problem for programs which are meant to be used
in list context; you generally want to return the empty list in that
case (not a list with a single undef in it). So that is the reason for
the advice you are quoting.

I am not sure it is worth making this function behave properly in a list
context; it is not a library function, so we can see that all of the
existing callers use it in a scalar context (and we would have to define
sensible semantics for a list context). But even if we _did_ want to do
that, your patch is only the first step. You would have to also fix the
other returns which can return undef instead of an empty list. So there
is not much point to your patch as it stands.

On a side note, while looking at this function, I wonder if that "return
undef" is correct after all. We get there only if the user has failed to
give valid input 10 times, so presumably it is a sanity check to
prevent runaway input errors (and I am cc'ing Jay, who added the
function not too long ago). Should we be respecting the default here, as
we do when we get EOF? Although I tend to think if the user is
repeatedly giving us bogus input that we should not just proceed, but
should probably die. Because otherwise we are guessing at what they
might have wanted.

-Peff

Re: [PATCH 2/6] Remove function prototypes from git-send-email.perl

[Jeff King]

On Wed, Apr 29, 2009 at 09:12:19AM -0400, Bill Pemberton wrote:

> Use of function prototypes is considered bad practice in perl.  The
> ones used here didn't accomplish anything anyhow, so they've been
> removed.
> 
> Signed-off-by: Bill Pemberton <wfp5p@virginia.edu>

I think this one is sensible.

-Peff

Re: [PATCH 4/6] Add explict return to end of subroutines

[Jeff King]

On Wed, Apr 29, 2009 at 09:12:21AM -0400, Bill Pemberton wrote:

> In perl a subroutine that ends without an explicit return will return
> the value of the last expression evalutated.  This can lead to
> unexpected return values.

I am not opposed to this, but it is really not helping anything. These
functions aren't meant to return any value, and their return values are
not used. In C, they would simply be declared "void", but there is no
way (AFAIK) to do that in perl.

But for some of them, I wonder if you would do better instead of
returning _nothing_ to return something that might make a little bit of
sense. For example, make_message_id munges a global variable. A
useful refactoring might be to have it return the value of the
global variable, and then perhaps even the global could go away, which
_would_ be a real improvement.

-Peff

Re: [PATCH 6/6] Remove bareword filehandles in git-send-email.perl

[Jeff King]

On Wed, Apr 29, 2009 at 09:12:23AM -0400, Bill Pemberton wrote:

> The script was using bareword filehandles.  This is considered a bad
> practice so they have been changed to indirect filehandles.

I think this is a real improvement; using indirect filehandles mean they
get scoped properly, which can avoid errors (especially forgetting to
close() them, which happens automagically when they go out of scope).
Assuming, of course, that the scoping added by your change is correct,
and doesn't close a handle during a loop that we may have wanted to keep
open (I didn't check carefully).

But in the patch itself:

> -	open(C,">",$compose_filename)
> +	open my $C,'>',$compose_filename

There are actually two things happening here:

  1. s/C/my $C/, which I think is good

  2. losing the parentheses around open(). This is a style issue, but I
     think we usually prefer the parenthesized form of most perl
     builtins (and certainly in the absence of other information, it
     should be left as-is).

And the style thing that probably _should_ be changed is the spacing: we
typically have a space between function arguments. So:

  open(my $C, '>', $compose_file)

> -	print C <<EOT;
> +	print {$C} <<EOT;

Are the braces really necessary here? Is there a version of perl on
which

  print $C <<EOT;

will not work?

-Peff

Re: [PATCH 6/6] Remove bareword filehandles in git-send-email.perl

[Francis Galiegue]

Le Sunday 03 May 2009 22:58:26 Jeff King, vous avez écrit :
> On Wed, Apr 29, 2009 at 09:12:23AM -0400, Bill Pemberton wrote:
> > The script was using bareword filehandles.  This is considered a bad
> > practice so they have been changed to indirect filehandles.
>
> I think this is a real improvement; using indirect filehandles mean they
> get scoped properly, which can avoid errors (especially forgetting to
> close() them, which happens automagically when they go out of scope).
> Assuming, of course, that the scoping added by your change is correct,
> and doesn't close a handle during a loop that we may have wanted to keep
> open (I didn't check carefully).
>
> But in the patch itself:
> > -	open(C,">",$compose_filename)
> > +	open my $C,'>',$compose_filename
>
> There are actually two things happening here:
>
>   1. s/C/my $C/, which I think is good
>
>   2. losing the parentheses around open(). This is a style issue, but I
>      think we usually prefer the parenthesized form of most perl
>      builtins (and certainly in the absence of other information, it
>      should be left as-is).
>

And why not go the full way and using IO::File?

my $fh = new IO::File;

$fh->open("/the/file", O_RDONLY|...)

-- 
Francis Galiegue
fge@one2team.com
Ingénieur système
Mob : +33 (0) 683 877 875
Tel : +33 (0) 178 945 552
One2team
40 avenue Raymond Poincaré
75116 Paris

Re: [PATCH 3/6] Remove return undef from ask()

[Jay Soffian]

On Sun, May 3, 2009 at 4:26 PM, Jeff King <peff@peff.net> wrote:
> On a side note, while looking at this function, I wonder if that "return
> undef" is correct after all. We get there only if the user has failed to
> give valid input 10 times, so presumably it is a sanity check to
> prevent runaway input errors

Correct, that is why it is there.

> (and I am cc'ing Jay, who added the
> function not too long ago). Should we be respecting the default here, as
> we do when we get EOF?

The original motivation was a user who was running send-email from
cron and it was looping forever. That case is now actually handled
before the loop, and all other normal cases are handled inside the
loop.

So the only thing that can cause the loop to exit (AFAIK) is when
$valid_re is passed in and the user provides invalid input 10x.

> Although I tend to think if the user is
> repeatedly giving us bogus input that we should not just proceed, but
> should probably die. Because otherwise we are guessing at what they
> might have wanted.

Well, it returns undef, at which point it's up to the caller to figure
out what to do. You'll notice the one caller which passes in $valid_re
dies:

die "Send this email reply required" unless defined $_;

Letting the caller decide what to do provides more flexibility.

j.

Re: [PATCH 6/6] Remove bareword filehandles in git-send-email.perl

[H.Merijn Brand]

On Sun, 3 May 2009 23:34:03 +0200, Francis Galiegue <fge@one2team.com>
wrote:

> Le Sunday 03 May 2009 22:58:26 Jeff King, vous avez écrit :
> > On Wed, Apr 29, 2009 at 09:12:23AM -0400, Bill Pemberton wrote:
> > > The script was using bareword filehandles.  This is considered a bad
> > > practice so they have been changed to indirect filehandles.
> >
> > I think this is a real improvement; using indirect filehandles mean they
> > get scoped properly, which can avoid errors (especially forgetting to
> > close() them, which happens automagically when they go out of scope).
> > Assuming, of course, that the scoping added by your change is correct,
> > and doesn't close a handle during a loop that we may have wanted to keep
> > open (I didn't check carefully).
> >
> > But in the patch itself:
> > > -	open(C,">",$compose_filename)
> > > +	open my $C,'>',$compose_filename
> >
> > There are actually two things happening here:
> >
> >   1. s/C/my $C/, which I think is good
> >
> >   2. losing the parentheses around open(). This is a style issue, but I
> >      think we usually prefer the parenthesized form of most perl
> >      builtins (and certainly in the absence of other information, it
> >      should be left as-is).
> >
> 
> And why not go the full way and using IO::File?

Because that would be travelling back in time.
The most efficient and preferred way is three-arg lexical:

    open my $fh, "<", $filename or die "$filename: $!";
    while (<$fh>) {
        # ...
        }
    close $fh or die "$filename: $!";

give or take quoting style, some spaces and/or indents

> my $fh = new IO::File;
> 
> $fh->open("/the/file", O_RDONLY|...)

Why use a module for something that is neatly buit in?

-- 
H.Merijn Brand  http://tux.nl      Perl Monger  http://amsterdam.pm.org/
using & porting perl 5.6.2, 5.8.x, 5.10.x, 5.11.x on HP-UX 10.20, 11.00,
11.11, 11.23, and 11.31, OpenSuSE 10.3, 11.0, and 11.1, AIX 5.2 and 5.3.
http://mirrors.develooper.com/hpux/           http://www.test-smoke.org/
http://qa.perl.org      http://www.goldmark.org/jeff/stupid-disclaimers/

Re: [PATCH 6/6] Remove bareword filehandles in git-send-email.perl

[Francis Galiegue]

Le lundi 04 mai 2009, vous avez écrit :
[...]
> > 
> > And why not go the full way and using IO::File?
> 
> Because that would be travelling back in time.
> The most efficient and preferred way is three-arg lexical:
> 
>     open my $fh, "<", $filename or die "$filename: $!";
>     while (<$fh>) {
>         # ...
>         }
>     close $fh or die "$filename: $!";
> 

I don't see how using IO::File is going back in time at all. It's a standard 
perl module, even in 5.10.

> 
> > my $fh = new IO::File;
> > 
> > $fh->open("/the/file", O_RDONLY|...)
> 
> Why use a module for something that is neatly buit in?
> 

Because it reads better? YMMV, of course. I prefer using IO::File because perl 
has too many keywords for its own good :p

-- 
Francis Galiegue
ONE2TEAM
Ingénieur système
Mob : +33 (0) 683 877 875
Tel : +33 (0) 178 945 552
fge@one2team.com
40 avenue Raymond Poincaré
75116 Paris

Re: [PATCH 6/6] Remove bareword filehandles in git-send-email.perl

[H.Merijn Brand]

On Mon, 4 May 2009 08:53:44 +0200, Francis Galiegue <fge@one2team.com>
wrote:

> Le lundi 04 mai 2009, vous avez écrit :
> [...]
> > > 
> > > And why not go the full way and using IO::File?
> > 
> > Because that would be travelling back in time.
> > The most efficient and preferred way is three-arg lexical:
> > 
> >     open my $fh, "<", $filename or die "$filename: $!";
> >     while (<$fh>) {
> >         # ...
> >         }
> >     close $fh or die "$filename: $!";
> > 
> 
> I don't see how using IO::File is going back in time at all. It's
> a standard perl module, even in 5.10.

It was written to be able to have lexical handles and pass them around.
With the above implementation you don't need to read a 600+ line module
to have every file action go through methods that are not needed at
all, and you also do not have to read the documentation for the
deviating syntax. I'd rather stick to simple, easy and standard.

> > > my $fh = new IO::File;
> > > $fh->open("/the/file", O_RDONLY|...)
> > 
> > Why use a module for something that is neatly buit in?
> 
> Because it reads better? YMMV, of course.

Because it is more efficient? And yes, it reads better, because it is
used in a zillion places already.

> I prefer using IO::File because perl has too many keywords for its
> own good :p

In the syntax I wrote down are now new keywords at all.

-- 
H.Merijn Brand  http://tux.nl      Perl Monger  http://amsterdam.pm.org/
using & porting perl 5.6.2, 5.8.x, 5.10.x, 5.11.x on HP-UX 10.20, 11.00,
11.11, 11.23, and 11.31, OpenSuSE 10.3, 11.0, and 11.1, AIX 5.2 and 5.3.
http://mirrors.develooper.com/hpux/           http://www.test-smoke.org/
http://qa.perl.org      http://www.goldmark.org/jeff/stupid-disclaimers/