[PATCH 00/16] git-send-email cleanups

[PATCH 00/16] git-send-email cleanups

[Ævar Arnfjörð Bjarmason]

As threatened here's a series that cleans up some of the glaring warts
in git-send-email.

Ævar Arnfjörð Bjarmason (16):
  send-email: use lexical filehandle for opendir
  send-email: use lexical filehandles for $compose
  send-email: use lexical filehandles during sending
  send-email: get_patch_subject doesn't need a prototype
  send-email: file_declares_8bit_cte doesn't need a prototype
  send-email: unique_email_list doesn't need a prototype
  send-email: cleanup_compose_files doesn't need a prototype
  send-email: use \E***\Q instead of \*\*\*
  send-email: sanitize_address use $foo, not "$foo"
  send-email: sanitize_address use qq["foo"], not "\"foo\""
  send-email: use (?:) instead of () if no  match variables are needed
  send-email: is_rfc2047_quoted use qr// regexes
  send-email: extract_valid_address use qr// regexes and /o
  send-email: send_message die on $!, not $?
  send-email: make_message_id use "require" instead of "use"
  send-email: use Perl idioms in while loop

 git-send-email.perl   |   80 +++++++++++++++++++++++-------------------------
 t/t9001-send-email.sh |    2 +-
 2 files changed, 39 insertions(+), 43 deletions(-)

-- 
1.7.3.159.g610493

[PATCH 01/16] send-email: use lexical filehandle for opendir

[Ævar Arnfjörð Bjarmason]

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 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 8cc4161..2f18d83 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -512,12 +512,12 @@ while (defined(my $f = shift @ARGV)) {
 		push @rev_list_opts, "--", @ARGV;
 		@ARGV = ();
 	} elsif (-d $f and !check_file_rev_conflict($f)) {
-		opendir(DH,$f)
+		opendir my $dh, $f
 			or die "Failed to opendir $f: $!";
 
 		push @files, grep { -f $_ } map { catfile($f, $_) }
-				sort readdir(DH);
-		closedir(DH);
+				sort readdir $dh;
+		closedir $dh;
 	} elsif ((-f $f or -p $f) and !check_file_rev_conflict($f)) {
 		push @files, $f;
 	} else {
-- 
1.7.3.159.g610493

[PATCH 02/16] send-email: use lexical filehandles for $compose

[Ævar Arnfjörð Bjarmason]

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 2f18d83..634835c 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -567,7 +567,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: $!";
 
 
@@ -575,7 +575,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
@@ -588,9 +588,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;
 
 	if ($annotate) {
 		do_edit($compose_filename, @files);
@@ -598,23 +598,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";
@@ -639,10 +639,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";
-- 
1.7.3.159.g610493

[PATCH 03/16] send-email: use lexical filehandles during sending

[Ævar Arnfjörð Bjarmason]

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 634835c..488d894 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1144,7 +1144,7 @@ $subject = $initial_subject;
 $message_num = 0;
 
 foreach my $t (@files) {
-	open(F,"<",$t) or die "can't open file $t";
+	open my $fh, "<", $t or die "can't open file $t";
 
 	my $author = undef;
 	my $author_encoding;
@@ -1157,7 +1157,7 @@ foreach my $t (@files) {
 	$message = "";
 	$message_num++;
 	# First unfold multiline header fields
-	while(<F>) {
+	while(<$fh>) {
 		last if /^\s*$/;
 		if (/^\s+\S/ and @header) {
 			chomp($header[$#header]);
@@ -1233,7 +1233,7 @@ foreach my $t (@files) {
 		}
 	}
 	# Now parse the message body
-	while(<F>) {
+	while(<$fh>) {
 		$message .=  $_;
 		if (/^(Signed-off-by|Cc): (.*)$/i) {
 			chomp;
@@ -1250,12 +1250,12 @@ foreach my $t (@files) {
 				$c, $_) unless $quiet;
 		}
 	}
-	close F;
+	close $fh;
 
 	if (defined $cc_cmd && !$suppress_cc{'cccmd'}) {
-		open(F, "$cc_cmd \Q$t\E |")
+		open my $fh, "$cc_cmd \Q$t\E |"
 			or die "(cc-cmd) Could not execute '$cc_cmd'";
-		while(<F>) {
+		while(<$fh>) {
 			my $c = $_;
 			$c =~ s/^\s*//g;
 			$c =~ s/\n$//g;
@@ -1264,7 +1264,7 @@ foreach my $t (@files) {
 			printf("(cc-cmd) Adding cc: %s from: '%s'\n",
 				$c, $cc_cmd) unless $quiet;
 		}
-		close F
+		close $fh
 			or die "(cc-cmd) failed to close pipe to '$cc_cmd'";
 	}
 
-- 
1.7.3.159.g610493

[PATCH 04/16] send-email: get_patch_subject doesn't need a prototype

[Ævar Arnfjörð Bjarmason]

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 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 488d894..b50c963 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -549,7 +549,7 @@ if (@files) {
 	usage();
 }
 
-sub get_patch_subject($) {
+sub get_patch_subject {
 	my $fn = shift;
 	open (my $fh, '<', $fn);
 	while (my $line = <$fh>) {
-- 
1.7.3.159.g610493

[PATCH 05/16] send-email: file_declares_8bit_cte doesn't need a prototype

[Ævar Arnfjörð Bjarmason]

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 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 b50c963..f471888 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -679,7 +679,7 @@ sub ask {
 
 my %broken_encoding;
 
-sub file_declares_8bit_cte($) {
+sub file_declares_8bit_cte {
 	my $fn = shift;
 	open (my $fh, '<', $fn);
 	while (my $line = <$fh>) {
-- 
1.7.3.159.g610493

[PATCH 06/16] send-email: unique_email_list doesn't need a prototype

[Ævar Arnfjörð Bjarmason]

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index f471888..90b777a 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -136,7 +136,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:
@@ -1332,7 +1331,7 @@ sub cleanup_compose_files() {
 
 $smtp->quit if $smtp;
 
-sub unique_email_list(@) {
+sub unique_email_list {
 	my %seen;
 	my @emails;
 
-- 
1.7.3.159.g610493

[PATCH 07/16] send-email: cleanup_compose_files doesn't need a prototype

[Ævar Arnfjörð Bjarmason]

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 90b777a..ce9b5eb 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -136,8 +136,6 @@ my $have_mail_address = eval { require Mail::Address; 1 };
 my $smtp;
 my $auth;
 
-sub cleanup_compose_files();
-
 # Variables we fill in automatically, or via prompting:
 my (@to,$no_to,@cc,$no_cc,@initial_cc,@bcclist,$no_bcc,@xh,
 	$initial_reply_to,$initial_subject,@files,
@@ -1325,7 +1323,7 @@ foreach my $t (@files) {
 
 cleanup_compose_files();
 
-sub cleanup_compose_files() {
+sub cleanup_compose_files {
 	unlink($compose_filename, $compose_filename . ".final") if $compose;
 }
 
-- 
1.7.3.159.g610493

[PATCH 08/16] send-email: use \E***\Q instead of \*\*\*

[Ævar Arnfjörð Bjarmason]

Change the regex introduced in a03bc5b to use the \E...\Q escape
syntax instead of using backslashes. It's more readable like this, and
easier to grep for.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 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 ce9b5eb..1218bbe 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -705,7 +705,7 @@ if (!defined $auto_8bit_encoding && scalar %broken_encoding) {
 
 if (!$force) {
 	for my $f (@files) {
-		if (get_patch_subject($f) =~ /\*\*\* SUBJECT HERE \*\*\*/) {
+		if (get_patch_subject($f) =~ /\Q*** SUBJECT HERE ***\E/) {
 			die "Refusing to send because the patch\n\t$f\n"
 				. "has the template subject '*** SUBJECT HERE ***'. "
 				. "Pass --force if you really want to send.\n";
-- 
1.7.3.159.g610493

[PATCH 09/16] send-email: sanitize_address use $foo, not "$foo"

[Ævar Arnfjörð Bjarmason]

There's no reason to explicitly stringify a variable in Perl unless
it's an overloaded object and you want to call overload::StrVal,
otherwise it's just creating a new scalar redundantly.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 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 1218bbe..1bf090a 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -864,7 +864,7 @@ sub sanitize_address {
 	my ($recipient_name, $recipient_addr) = ($recipient =~ /^(.*?)\s*(<.*)/);
 
 	if (not $recipient_name) {
-		return "$recipient";
+		return $recipient;
 	}
 
 	# if recipient_name is already quoted, do nothing
-- 
1.7.3.159.g610493

[PATCH 10/16] send-email: sanitize_address use qq["foo"], not "\"foo\""

[Ævar Arnfjörð Bjarmason]

Perl provides an alternate quote syntax which can make using "" inside
interpolated strings easier to read.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 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 1bf090a..c012b95 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -881,7 +881,7 @@ sub sanitize_address {
 	# double quotes are needed if specials or CTLs are included
 	elsif ($recipient_name =~ /[][()<>@,;:\\".\000-\037\177]/) {
 		$recipient_name =~ s/(["\\\r])/\\$1/g;
-		$recipient_name = "\"$recipient_name\"";
+		$recipient_name = qq["$recipient_name"];
 	}
 
 	return "$recipient_name $recipient_addr";
-- 
1.7.3.159.g610493

[PATCH 11/16] send-email: use (?:) instead of () if no match variables are needed

[Ævar Arnfjörð Bjarmason]

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 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 c012b95..5a0c4a8 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -365,7 +365,7 @@ my(%suppress_cc);
 if (@suppress_cc) {
 	foreach my $entry (@suppress_cc) {
 		die "Unknown --suppress-cc field: '$entry'\n"
-			unless $entry =~ /^(all|cccmd|cc|author|self|sob|body|bodycc)$/;
+			unless $entry =~ /^(?:all|cccmd|cc|author|self|sob|body|bodycc)$/;
 		$suppress_cc{$entry} = 1;
 	}
 }
-- 
1.7.3.159.g610493

[PATCH 12/16] send-email: is_rfc2047_quoted use qr// regexes

[Ævar Arnfjörð Bjarmason]

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 5a0c4a8..b87c3f2 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -852,8 +852,8 @@ sub quote_rfc2047 {
 
 sub is_rfc2047_quoted {
 	my $s = shift;
-	my $token = '[^][()<>@,;:"\/?.= \000-\037\177-\377]+';
-	my $encoded_text = '[!->@-~]+';
+	my $token = qr/[^][()<>@,;:"\/?.= \000-\037\177-\377]+/;
+	my $encoded_text = qr/[!->@-~]+/;
 	length($s) <= 75 &&
 	$s =~ m/^(?:"[[:ascii:]]*"|=\?$token\?$token\?$encoded_text\?=)$/o;
 }
-- 
1.7.3.159.g610493

[PATCH 13/16] send-email: extract_valid_address use qr// regexes and /o

[Ævar Arnfjörð Bjarmason]

Change the regex fragment in extract_valid_address to use the qr//
syntax for compiled regexes, and when they're used add a /o flag so
they're only compiled once for the lifetime of the program.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index b87c3f2..47d86ad 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -777,11 +777,11 @@ our ($message_id, %mail, $subject, $reply_to, $references, $message,
 
 sub extract_valid_address {
 	my $address = shift;
-	my $local_part_regexp = '[^<>"\s@]+';
-	my $domain_regexp = '[^.<>"\s@]+(?:\.[^.<>"\s@]+)+';
+	my $local_part_regexp = qr/[^<>"\s@]+/;
+	my $domain_regexp = qr/[^.<>"\s@]+(?:\.[^.<>"\s@]+)+/;
 
 	# check for a local address:
-	return $address if ($address =~ /^($local_part_regexp)$/);
+	return $address if ($address =~ /^($local_part_regexp)$/o);
 
 	$address =~ s/^\s*<(.*)>\s*$/$1/;
 	if ($have_email_valid) {
@@ -789,7 +789,7 @@ sub extract_valid_address {
 	} else {
 		# less robust/correct than the monster regexp in Email::Valid,
 		# but still does a 99% job, and one less dependency
-		$address =~ /($local_part_regexp\@$domain_regexp)/;
+		$address =~ /($local_part_regexp\@$domain_regexp)/o;
 		return $1;
 	}
 }
-- 
1.7.3.159.g610493

Re: [PATCH 13/16] send-email: extract_valid_address use qr// regexes and /o

[Jeff King]

On Thu, Sep 30, 2010 at 01:43:06PM +0000, Ævar Arnfjörð Bjarmason wrote:

> Change the regex fragment in extract_valid_address to use the qr//
> syntax for compiled regexes, and when they're used add a /o flag so
> they're only compiled once for the lifetime of the program.
> [...]
>  sub extract_valid_address {
>  	my $address = shift;
> -	my $local_part_regexp = '[^<>"\s@]+';
> -	my $domain_regexp = '[^.<>"\s@]+(?:\.[^.<>"\s@]+)+';
> +	my $local_part_regexp = qr/[^<>"\s@]+/;
> +	my $domain_regexp = qr/[^.<>"\s@]+(?:\.[^.<>"\s@]+)+/;
>  

Hmm. But these are lexical variables, so won't we recompile them each
time we enter the subroutine? I don't think it affects correctness, as
this "/o":

> +	return $address if ($address =~ /^($local_part_regexp)$/o);

means that we will compile and use the value from the first time we run
the function.

But we are unnecessarily compiling the sub-regexes each time. Not that
this is probably a performance critical piece of code, but your "/o" is
doing very little, and this is exactly the sort perl wankery that I find
interesting.

Sadly, there is no real perl equivalent of C static local variables,
which is what you really want.  Usually I would do:

  {
    my $local_part_regexp = qr/.../;
    sub extract_valid_address {
      ...
    }
  }

but beware of the execution order. That works well in a module, where
the module code is executed before anybody calls the function. But it
breaks in something like this:

  foo();
  {
    my $foo_static_local = 5;
    sub foo {
      print "$foo_static_local\n";
    }
  }

I think you could get by with:

  {
    my $local_part_regexp;
    sub extract_valid_address {
      $local_part_regexp ||= qr/.../;
      ...
    }
  }

-Peff

Re: [PATCH 13/16] send-email: extract_valid_address use qr// regexes and /o

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 30, 2010 at 16:19, Jeff King <peff@peff.net> wrote:
> On Thu, Sep 30, 2010 at 01:43:06PM +0000, Ævar Arnfjörð Bjarmason wrote:
>
>> Change the regex fragment in extract_valid_address to use the qr//
>> syntax for compiled regexes, and when they're used add a /o flag so
>> they're only compiled once for the lifetime of the program.
>> [...]
>>  sub extract_valid_address {
>>       my $address = shift;
>> -     my $local_part_regexp = '[^<>"\s@]+';
>> -     my $domain_regexp = '[^.<>"\s@]+(?:\.[^.<>"\s@]+)+';
>> +     my $local_part_regexp = qr/[^<>"\s@]+/;
>> +     my $domain_regexp = qr/[^.<>"\s@]+(?:\.[^.<>"\s@]+)+/;
>>
>
> Hmm. But these are lexical variables, so won't we recompile them each
> time we enter the subroutine? I don't think it affects correctness, as
> this "/o":
>
>> +     return $address if ($address =~ /^($local_part_regexp)$/o);
>
> means that we will compile and use the value from the first time we run
> the function.
>
> But we are unnecessarily compiling the sub-regexes each time. Not that
> this is probably a performance critical piece of code, but your "/o" is
> doing very little, and this is exactly the sort perl wankery that I find
> interesting.

IIRC different perl versions treat this differently. In more recent
versions doing:

    perl -Mre=debug -E 'sub x { my $x = qr/foo/; my $y = qr/bar/;
/$x$y/ } x($_) for 1..2'

Will only compile all of those regexes once, since perl can see that
they're constant. So the /o does nothing.

We might want to keep it for self-documentation purposes to indicate
that the $local_part_regexp would never change, but it's probably best
to drop that /o altogether.

> Sadly, there is no real perl equivalent of C static local variables,
> which is what you really want.  Usually I would do:
>
>  {
>    my $local_part_regexp = qr/.../;
>    sub extract_valid_address {
>      ...
>    }
>  }
>
> but beware of the execution order. That works well in a module, where
> the module code is executed before anybody calls the function. But it
> breaks in something like this:
>
>  foo();
>  {
>    my $foo_static_local = 5;
>    sub foo {
>      print "$foo_static_local\n";
>    }
>  }
>
> I think you could get by with:
>
>  {
>    my $local_part_regexp;
>    sub extract_valid_address {
>      $local_part_regexp ||= qr/.../;
>      ...
>    }
>  }

Perl has static variables like that, just not in the ancient version
we're pinned to:

    sub foo {
        state $foo = qr/$_[0]/
    }

Re: [PATCH 13/16] send-email: extract_valid_address use qr// regexes and /o

[Jeff King]

On Thu, Sep 30, 2010 at 04:33:59PM +0000, Ævar Arnfjörð Bjarmason wrote:

> > But we are unnecessarily compiling the sub-regexes each time. Not that
> > this is probably a performance critical piece of code, but your "/o" is
> > doing very little, and this is exactly the sort perl wankery that I find
> > interesting.
> 
> IIRC different perl versions treat this differently. In more recent
> versions doing:
> 
>     perl -Mre=debug -E 'sub x { my $x = qr/foo/; my $y = qr/bar/;
> /$x$y/ } x($_) for 1..2'

Ah, right. I am used to using qr// with variable input (since that is
the intended use), but of course perl is smart enough to just need to
compile qr/constant/ once.

> Will only compile all of those regexes once, since perl can see that
> they're constant. So the /o does nothing.

Nice. I didn't think it would be smart enough to handle the layer of
indirection, but obviously it does. Go perl.

> We might want to keep it for self-documentation purposes to indicate
> that the $local_part_regexp would never change, but it's probably best
> to drop that /o altogether.

Yeah, I would just drop the /o. It will in all probability never change,
but it it _did_, I think having the /o just makes it buggy.

> Perl has static variables like that, just not in the ancient version
> we're pinned to:
> 
>     sub foo {
>         state $foo = qr/$_[0]/
>     }

Nice, I obviously have not kept up on my perl-changelog-reading. ;) But
yeah, it's not in 5.8. It's a moot point anyway, since as you
demonstrated above, perl is already doing the optimization.

-Peff

Re: [PATCH 13/16] send-email: extract_valid_address use qr// regexes and /o

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ...
> But we are unnecessarily compiling the sub-regexes each time. Not that
> this is probably a performance critical piece of code, but your "/o" is
> doing very little, and this is exactly the sort perl wankery that I find
> interesting.

Well, isn't the _sole_ point of using qr// to optimize by avoiding
recompilation?  If this is not a performance critical section of the code,
what is the point of this change?

This [PATCH 13/16] and also [PATCH 12/16] rewrite strings using qr// but
the patterns thus compiled are used exactly once before the control leaves
the scope of the variables, so...

It is a different story if the patch instead introduced module-level
global variables to hold a pre-compiled regexp objects, but that is not
what we are seeing here.

Re: [PATCH 13/16] send-email: extract_valid_address use qr// regexes and /o

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 30, 2010 at 17:25, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> ...
>> But we are unnecessarily compiling the sub-regexes each time. Not that
>> this is probably a performance critical piece of code, but your "/o" is
>> doing very little, and this is exactly the sort perl wankery that I find
>> interesting.
>
> Well, isn't the _sole_ point of using qr// to optimize by avoiding
> recompilation?  If this is not a performance critical section of the code,
> what is the point of this change?
>
> This [PATCH 13/16] and also [PATCH 12/16] rewrite strings using qr// but
> the patterns thus compiled are used exactly once before the control leaves
> the scope of the variables, so...

The point is to use Perl's data-types for what they're supposed to be
used for. In perl you *can* write:

    my $two = "1" + "1";

To get 2, but that's silly. You should just use integers instead of
string coercion:

    my $two = 1 + 1;

Similarly you *can* put regexes in strings, but perl has a first-class
datatype for it, so you should do:

    my $rx = qr/foo/;

Not:

    my $rx = "foo";

So it's just a change to use a better style. There are some tangible
advantages to using qr// over qq// (also known as ""), e.g. Emacs and
Vim will know to highlight the string as a regexp, and when the $rx is
interpolated Perl won't need to recompile it because it can just add a
pointer to the existing regex in the new pattern.

But the main reason here is to not write code that looks like it
targets Perl 5.5, but 5.8.

[PATCH v2 13/16] send-email: extract_valid_address use qr// regexes

[Ævar Arnfjörð Bjarmason]

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

Here's v2 which omits the confusing /o addition and just uses the more
correct qr// syntax over qq//.

 git-send-email.perl |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index b87c3f2..30000b9 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -777,8 +777,8 @@ our ($message_id, %mail, $subject, $reply_to, $references, $message,
 
 sub extract_valid_address {
 	my $address = shift;
-	my $local_part_regexp = '[^<>"\s@]+';
-	my $domain_regexp = '[^.<>"\s@]+(?:\.[^.<>"\s@]+)+';
+	my $local_part_regexp = qr/[^<>"\s@]+/;
+	my $domain_regexp = qr/[^.<>"\s@]+(?:\.[^.<>"\s@]+)+/;
 
 	# check for a local address:
 	return $address if ($address =~ /^($local_part_regexp)$/);
-- 
1.7.3.159.g610493

[PATCH 14/16] send-email: send_message die on $!, not $?

[Ævar Arnfjörð Bjarmason]

If close fails we want to emit errno, not the return code of whatever
happened to be the child process run.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 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 47d86ad..9cb6aa6 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1035,7 +1035,7 @@ X-Mailer: git-send-email $gitversion
 			exec($smtp_server, @sendmail_parameters) or die $!;
 		}
 		print $sm "$header\n$message";
-		close $sm or die $?;
+		close $sm or die $!;
 	} else {
 
 		if (!defined $smtp_server) {
-- 
1.7.3.159.g610493

[PATCH 15/16] send-email: make_message_id use "require" instead of "use"

[Ævar Arnfjörð Bjarmason]

Change the use of Sys::Hostname from a "use" to a "require". The
former happens in an implicit BEGIN block and is thus immune from the
if block it's contained in, so it's always loaded.

This should speed up the invocation of git-send-email by a few
milliseconds.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 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 9cb6aa6..5a19d3d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -819,7 +819,7 @@ sub make_message_id {
 		last if (defined $du_part and $du_part ne '');
 	}
 	if (not defined $du_part or $du_part eq '') {
-		use Sys::Hostname qw();
+		require Sys::Hostname;
 		$du_part = 'user@' . Sys::Hostname::hostname();
 	}
 	my $message_id_template = "<%s-git-send-email-%s>";
-- 
1.7.3.159.g610493

[PATCH 16/16] send-email: use Perl idioms in while loop

[Ævar Arnfjörð Bjarmason]

Change `while(<$fh>) { my $c = $_' to `while(my $c = <$fh>) {', and
use `chomp $c' instead of `$c =~ s/\n$//g;', the two are equivalent in
this case.

I've also changed the --cccmd test so that we test for the stripping
of whitespace at the beginning of the lines returned from the
--cccmd. I think we probably shouldn't do this, but it was there
already so I haven't changed the behavior.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl   |    5 ++---
 t/t9001-send-email.sh |    2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 5a19d3d..445d2ec 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1252,10 +1252,9 @@ foreach my $t (@files) {
 	if (defined $cc_cmd && !$suppress_cc{'cccmd'}) {
 		open my $fh, "$cc_cmd \Q$t\E |"
 			or die "(cc-cmd) Could not execute '$cc_cmd'";
-		while(<$fh>) {
-			my $c = $_;
+		while(my $c = <$fh>) {
+			chomp $c;
 			$c =~ s/^\s*//g;
-			$c =~ s/\n$//g;
 			next if ($c eq $sender and $suppress_from);
 			push @cc, $c;
 			printf("(cc-cmd) Adding cc: %s from: '%s'\n",
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 6f67da4..99a16d5 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -204,7 +204,7 @@ test_expect_success $PREREQ 'Prompting works' '
 test_expect_success $PREREQ 'cccmd works' '
 	clean_fake_sendmail &&
 	cp $patches cccmd.patch &&
-	echo cccmd--cccmd@example.com >>cccmd.patch &&
+	echo "cccmd--  cccmd@example.com" >>cccmd.patch &&
 	{
 	  echo "#!$SHELL_PATH"
 	  echo sed -n -e s/^cccmd--//p \"\$1\"
-- 
1.7.3.159.g610493

Re: [PATCH 00/16] git-send-email cleanups

[Brian Gernhardt]

On Sep 30, 2010, at 9:42 AM, Ævar Arnfjörð Bjarmason wrote:

>  send-email: get_patch_subject doesn't need a prototype
>  send-email: file_declares_8bit_cte doesn't need a prototype
>  send-email: unique_email_list doesn't need a prototype
>  send-email: cleanup_compose_files doesn't need a prototype

None of these subroutines strictly need the prototype, but it does allow Perl to warn us if we send incorrect arguments.  Why remove them?  Are they causing problems somewhere?

~~ Brian

Re: [PATCH 00/16] git-send-email cleanups

[Jeff King]

On Thu, Sep 30, 2010 at 10:30:31AM -0400, Brian Gernhardt wrote:

> >  send-email: get_patch_subject doesn't need a prototype
> >  send-email: file_declares_8bit_cte doesn't need a prototype
> >  send-email: unique_email_list doesn't need a prototype
> >  send-email: cleanup_compose_files doesn't need a prototype
> 
> None of these subroutines strictly need the prototype, but it does
> allow Perl to warn us if we send incorrect arguments.  Why remove
> them?  Are they causing problems somewhere?

They don't necessarily do what you want:

  perl -e 'sub want_scalar($) { print "got $_[0]\n" }
           want_scalar("ok");
           my @a = qw(totally broken);
           want_scalar(@a);
  '

I get:

  got ok
  got 2

And using "sub want_list(@)" basically does nothing at all (you can pass
nothing, a scalar, or a list).

For more details, read:

 http://www.perlmonks.org/?node_id=861966

(If you're impatient, skip to the section "Problems with Regular
Prototypes").

-Peff

Re: [PATCH 00/16] git-send-email cleanups

[Brian Gernhardt]

On Sep 30, 2010, at 10:52 AM, Jeff King wrote:

> They don't necessarily do what you want:
> 
>  perl -e 'sub want_scalar($) { print "got $_[0]\n" }
>           want_scalar("ok");
>           my @a = qw(totally broken);
>           want_scalar(@a);
>  '
> 
> I get:
> 
>  got ok
>  got 2
> 
> And using "sub want_list(@)" basically does nothing at all (you can pass
> nothing, a scalar, or a list).

I'm not surprised by that at all, but I've spent too much time in scripting languages recently.

> For more details, read:
> 
> http://www.perlmonks.org/?node_id=861966
> 
> (If you're impatient, skip to the section "Problems with Regular
> Prototypes").

Huh.  Somewhere along the way I had picked up using prototypes as a solution to some problem and got used to them.  Interesting to see the full discussion why you shouldn't.  I guess I'm reading too much Ruby and Perl6 and didn't think about how that would work with Perl5.

~~ Brian

Re: [PATCH 00/16] git-send-email cleanups

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 30, 2010 at 14:30, Brian Gernhardt
<brian@gernhardtsoftware.com> wrote:

> None of these subroutines strictly need the prototype, but it does
> allow Perl to warn us if we send incorrect arguments.  Why remove
> them?  Are they causing problems somewhere?

As Jeff pointed out prototypes are troublesome. If you want to be
warned about too many arguments a better way is:

    sub foo {
        warn sprintf "You gave me %d arguments", scalar @_ if @_ != 1;

Or something like that, but there's no reason to do that for these
subs in particular. There are 32 subroutines in git-send-email.perl,
these weren't in any way more special than the rest.

They probably had prototypes in the first place because they were
added by someone who was under the mistaken impression that Perl
prototypes were remotely similar to C-like prototypes, they're not.

The purpose of Perl prototypes is to rewrite the *caller* code, so
that e.g. if you have:

    sub blah ($$) { ... }

Perl will rewrite this call:

    blah @foo, @bar;

As:

    blah(scalar(@foo), scalar(@bar))

While a blah without prototypes would just be:

    blah(@foo, @bar);

Using prototypes superfluously like this makes it harder to read the
code, because you end up checking every call site for every subroutine
call that uses prototypes to see if rewriting the argument list like
this is producing some unexpected logic error.

Re: [PATCH 00/16] git-send-email cleanups

[Jay Soffian]

On Thu, Sep 30, 2010 at 9:42 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> As threatened here's a series that cleans up some of the glaring warts
> in git-send-email.
>
> Ævar Arnfjörð Bjarmason (16):
>  send-email: use lexical filehandle for opendir
>  send-email: use lexical filehandles for $compose
>  send-email: use lexical filehandles during sending
>  send-email: get_patch_subject doesn't need a prototype
>  send-email: file_declares_8bit_cte doesn't need a prototype
>  send-email: unique_email_list doesn't need a prototype
>  send-email: cleanup_compose_files doesn't need a prototype
>  send-email: use \E***\Q instead of \*\*\*
>  send-email: sanitize_address use $foo, not "$foo"
>  send-email: sanitize_address use qq["foo"], not "\"foo\""
>  send-email: use (?:) instead of () if no  match variables are needed
>  send-email: is_rfc2047_quoted use qr// regexes
>  send-email: extract_valid_address use qr// regexes and /o
>  send-email: send_message die on $!, not $?
>  send-email: make_message_id use "require" instead of "use"
>  send-email: use Perl idioms in while loop
>
>  git-send-email.perl   |   80 +++++++++++++++++++++++-------------------------
>  t/t9001-send-email.sh |    2 +-
>  2 files changed, 39 insertions(+), 43 deletions(-)

These all look pretty uncontroversial and sane by me, though I don't
think you touched any of the lines I contributed to git-send-email. If
anything, perhaps this series is a bit too granular -- I might combine
the commits that get rid of the function prototypes into one for
example. Otherwise, thanks for the cleanup.

Acked-by: Jay Soffian <jaysoffian@gmail.com>

j.

Re: [PATCH 00/16] git-send-email cleanups

[Jeff King]

On Thu, Sep 30, 2010 at 01:42:53PM +0000, Ævar Arnfjörð Bjarmason wrote:

> As threatened here's a series that cleans up some of the glaring warts
> in git-send-email.
> 
> Ævar Arnfjörð Bjarmason (16):
>   send-email: use lexical filehandle for opendir
>   send-email: use lexical filehandles for $compose
>   send-email: use lexical filehandles during sending
>   send-email: get_patch_subject doesn't need a prototype
>   send-email: file_declares_8bit_cte doesn't need a prototype
>   send-email: unique_email_list doesn't need a prototype
>   send-email: cleanup_compose_files doesn't need a prototype
>   send-email: use \E***\Q instead of \*\*\*
>   send-email: sanitize_address use $foo, not "$foo"
>   send-email: sanitize_address use qq["foo"], not "\"foo\""
>   send-email: use (?:) instead of () if no  match variables are needed
>   send-email: is_rfc2047_quoted use qr// regexes
>   send-email: extract_valid_address use qr// regexes and /o
>   send-email: send_message die on $!, not $?
>   send-email: make_message_id use "require" instead of "use"
>   send-email: use Perl idioms in while loop

With the exception of my comments on the "/o" regexes, these all look
sane to me.

-Peff