[PATCH RFC 1/6] send-email: Add --delay for separating emails

[PATCH RFC 1/6] send-email: Add --delay for separating emails

[Michael Witten]

When sending a patch series, the emails often arrive at the final
destination out of order; though these emails should be chained
via the In-Reply-To headers, some mail-viewing systems display
by order of arrival instead.

The --delay option provides a means for specifying that there
should be a certain number of seconds of delay between sending
emails, so that the arrival order can be controlled better.

Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 Documentation/git-send-email.txt |    5 +++++
 git-send-email.perl              |   17 +++++++++++++----
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 10dfd66..4b656ca 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -97,6 +97,11 @@ The --to option must be repeated for each user you want on the to list.
 Sending
 ~~~~~~~
 
+--delay::
+	Specify the minimum number of seconds of delay that should occur
+	between sending emails. This number should be an integer >= zero.
+	Default is the value of the 'sendemail.delay' configuration variable.
+
 --envelope-sender::
 	Specify the envelope sender used to send the emails.
 	This is useful if your default address is not the address that is
diff --git a/git-send-email.perl b/git-send-email.perl
index 172b53c..273c8c7 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -56,6 +56,7 @@ git send-email [options] <file | directory | rev-list options >
     --compose                      * Open an editor for introduction.
 
   Sending:
+    --delay                 <int>  * Delay (seconds) between sending emails.
     --envelope-sender       <str>  * Email envelope sender.
     --smtp-server       <str:int>  * Outgoing SMTP server to use. The port
                                      is optional. Default 'localhost'.
@@ -180,7 +181,7 @@ sub do_edit {
 }
 
 # Variables with corresponding config settings
-my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc, $cc_cmd);
+my ($delay, $thread, $chain_reply_to, $suppress_from, $signed_off_by_cc, $cc_cmd);
 my ($smtp_server, $smtp_server_port, $smtp_authuser, $smtp_encryption);
 my ($identity, $aliasfiletype, @alias_files, @smtp_host_parts);
 my ($validate, $confirm);
@@ -196,6 +197,7 @@ my %config_bool_settings = (
 );
 
 my %config_settings = (
+    "delay" => \$delay,
     "smtpserver" => \$smtp_server,
     "smtpserverport" => \$smtp_server_port,
     "smtpuser" => \$smtp_authuser,
@@ -247,6 +249,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
 		    "cc=s" => \@initial_cc,
 		    "bcc=s" => \@bcclist,
 		    "chain-reply-to!" => \$chain_reply_to,
+		    "delay=i" => \$delay,
 		    "smtp-server=s" => \$smtp_server,
 		    "smtp-server-port=s" => \$smtp_server_port,
 		    "smtp-user=s" => \$smtp_authuser,
@@ -973,8 +976,9 @@ $references = $initial_reply_to || '';
 $subject = $initial_subject;
 $message_num = 0;
 
-foreach my $t (@files) {
-	open(F,"<",$t) or die "can't open file $t";
+for (my $index = 0; $index < @files; $index++) {
+	my $file = $files[$index];
+	open(F,"<",$file) or die "can't open file $file";
 
 	my $author = undef;
 	my $author_encoding;
@@ -1083,7 +1087,7 @@ foreach my $t (@files) {
 	close F;
 
 	if (defined $cc_cmd && !$suppress_cc{'cccmd'}) {
-		open(F, "$cc_cmd $t |")
+		open(F, "$cc_cmd $file |")
 			or die "(cc-cmd) Could not execute '$cc_cmd'";
 		while(<F>) {
 			my $c = $_;
@@ -1128,6 +1132,11 @@ foreach my $t (@files) {
 
 	send_message();
 
+	if ($delay && $index < $#files) {
+		my $this_long = $delay;
+		while (($this_long -= sleep $this_long) > 0) {}
+	}
+
 	# set up for the next message
 	if ($chain_reply_to || !defined $reply_to || length($reply_to) == 0) {
 		$reply_to = $message_id;
-- 
1.6.2.2.448.g61445.dirty

[PATCH RFC 2/6] send-email: --smtp-server-port should take an integer

[Michael Witten]

Signed-off-by: Michael Witten <mfwitten@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 273c8c7..63d6063 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -251,7 +251,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
 		    "chain-reply-to!" => \$chain_reply_to,
 		    "delay=i" => \$delay,
 		    "smtp-server=s" => \$smtp_server,
-		    "smtp-server-port=s" => \$smtp_server_port,
+		    "smtp-server-port=i" => \$smtp_server_port,
 		    "smtp-user=s" => \$smtp_authuser,
 		    "smtp-pass:s" => \$smtp_authpass,
 		    "smtp-ssl" => sub { $smtp_encryption = 'ssl' },
-- 
1.6.2.2.448.g61445.dirty

[PATCH RFC 3/6] send-email: Handle "GIT:" rather than "GIT: " during --compose

[Michael Witten]

This should make things a little more robust in terms of user input;
before, even the program got it wrong by outputting a line with only
"GIT:", which was left in place as a header, because there would be
no following space character.

Also, I cleaned up get_patch_subject().

Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 git-send-email.perl |   23 ++++++++++++-----------
 1 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 63d6063..098c620 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -505,15 +505,16 @@ if (@files) {
 }
 
 sub get_patch_subject($) {
-	my $fn = shift;
-	open (my $fh, '<', $fn);
-	while (my $line = <$fh>) {
-		next unless ($line =~ /^Subject: (.*)$/);
-		close $fh;
-		return "GIT: $1\n";
+
+	my $patch = shift;
+	open (my $fh, '<', $patch);
+
+	while (<$fh>) {
+		next unless (/^Subject: (.*)$/);
+		return $1;
 	}
-	close $fh;
-	die "No subject line in $fn ?";
+
+	die "'Subject:' line expected in '$patch'";
 }
 
 if ($compose) {
@@ -532,7 +533,7 @@ if ($compose) {
 
 	print C <<EOT;
 From $tpl_sender # This line is ignored.
-GIT: Lines beginning in "GIT: " will be removed.
+GIT: Lines beginning in "GIT:" will be removed.
 GIT: Consider including an overall diffstat or table of contents
 GIT: for the patch you are writing.
 GIT:
@@ -543,7 +544,7 @@ In-Reply-To: $tpl_reply_to
 
 EOT
 	for my $f (@files) {
-		print C get_patch_subject($f);
+		print C "GIT: ", get_patch_subject($f), "\n";
 	}
 	close(C);
 
@@ -565,7 +566,7 @@ EOT
 	my $in_body = 0;
 	my $summary_empty = 1;
 	while(<C>) {
-		next if m/^GIT: /;
+		next if m/^GIT:/;
 		if ($in_body) {
 			$summary_empty = 0 unless (/^\n$/);
 		} elsif (/^\n$/) {
-- 
1.6.2.2.448.g61445.dirty

[PATCH RFC 4/6] send-email: --compose takes optional argument to existing file

[Michael Witten]

Now, a user may specify an existing (in-progress) file to use as
the introductory/summary email.

The file is opened for any additional editing as usual.

Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 Documentation/git-send-email.txt |    7 ++-
 git-send-email.perl              |  112 ++++++++++++++++++++++----------------
 2 files changed, 71 insertions(+), 48 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 4b656ca..bc9ff13 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -57,8 +57,11 @@ The --cc option must be repeated for each user you want on the cc list.
 	or one for all of them at once.
 
 --compose::
-	Use $GIT_EDITOR, core.editor, $VISUAL, or $EDITOR to edit an
-	introductory message for the patch series.
+	Use $GIT_EDITOR, core.editor, $VISUAL, or $EDITOR, or vi to edit an
+	introductory message for the patch series. An existing file may be
+	specified as the basis for the introductory email; it will be opened
+	for editing directly. Otherwise, a new temporary file is created with
+	some default contents.
 +
 When '--compose' is used, git send-email will use the From, Subject, and
 In-Reply-To headers specified in the message. If the body of the message
diff --git a/git-send-email.perl b/git-send-email.perl
index 098c620..481bf36 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -53,7 +53,7 @@ git send-email [options] <file | directory | rev-list options >
     --subject               <str>  * Email "Subject:"
     --in-reply-to           <str>  * Email "In-Reply-To:"
     --annotate                     * Review each patch that will be sent in an editor.
-    --compose                      * Open an editor for introduction.
+    --compose           opt <str>  * Open an editor for introduction.
 
   Sending:
     --delay                 <int>  * Delay (seconds) between sending emails.
@@ -62,7 +62,7 @@ git send-email [options] <file | directory | rev-list options >
                                      is optional. Default 'localhost'.
     --smtp-server-port      <int>  * Outgoing SMTP server port.
     --smtp-user             <str>  * Username for SMTP-AUTH.
-    --smtp-pass             <str>  * Password for SMTP-AUTH; not necessary.
+    --smtp-pass         opt <str>  * Password for SMTP-AUTH; not necessary.
     --smtp-encryption       <str>  * tls or ssl; anything else disables.
     --smtp-ssl                     * Deprecated. Use '--smtp-encryption ssl'.
 
@@ -159,7 +159,7 @@ if ($@) {
 # Behavior modification variables
 my ($quiet, $dry_run) = (0, 0);
 my $format_patch;
-my $compose_filename;
+my ($compose_filename, $compose_final_filename, $compose_final_is_not_empty);
 
 # Handle interactive edition of files.
 my $multiedit;
@@ -224,13 +224,12 @@ sub signal_handler {
 	system "stty echo";
 
 	# tmp files from --compose
-	if (defined $compose_filename) {
-		if (-e $compose_filename) {
-			print "'$compose_filename' contains an intermediate version of the email you were composing.\n";
-		}
-		if (-e ($compose_filename . ".final")) {
-			print "'$compose_filename.final' contains the composed email.\n"
-		}
+	if (defined $compose_filename and -f $compose_filename) {
+		print "'$compose_filename' contains an intermediate version of the email you were composing.\n";
+	}
+
+	if (defined $compose_final_filename and -f $compose_final_filename) {
+		print "'$compose_final_filename' contains the composed email.\n"
 	}
 
 	exit;
@@ -258,7 +257,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
 		    "smtp-encryption=s" => \$smtp_encryption,
 		    "identity=s" => \$identity,
 		    "annotate" => \$annotate,
-		    "compose" => \$compose,
+		    "compose:s" => \$compose,
 		    "quiet" => \$quiet,
 		    "cc-cmd=s" => \$cc_cmd,
 		    "suppress-from!" => \$suppress_from,
@@ -517,21 +516,36 @@ sub get_patch_subject($) {
 	die "'Subject:' line expected in '$patch'";
 }
 
-if ($compose) {
-	# Note that this does not need to be secure, but we will make a small
-	# effort to have it be unique
-	$compose_filename = ($repo ?
-		tempfile(".gitsendemail.msg.XXXXXX", DIR => $repo->repo_path()) :
-		tempfile(".gitsendemail.msg.XXXXXX", DIR => "."))[1];
-	open(C,">",$compose_filename)
-		or die "Failed to open for writing $compose_filename: $!";
+if (defined $compose) {
+
+	my ($tmp_file, $tmp_filename) = tempfile(".gitsendemail.msg.XXXXXX", DIR => ($repo ? $repo->repo_path() : "."));
 
+	my $compose_file;
+	my $compose_final_file;
+
+	if ($compose ne '') {
+
+		$compose_filename = $compose;
+
+		$compose_final_filename = $tmp_filename;
+		$compose_final_file     = $tmp_file
+
+	} else {
 
-	my $tpl_sender = $sender || $repoauthor || $repocommitter || '';
-	my $tpl_subject = $initial_subject || '';
-	my $tpl_reply_to = $initial_reply_to || '';
+		$compose_filename = $tmp_filename;
+		$compose_file     = $tmp_file;
 
-	print C <<EOT;
+		$compose_final_filename = "$compose_filename.final";
+		open $compose_final_file, ">", $compose_final_filename
+			or die "Failed to open '$compose_final_filename' for writing: $!";
+
+		# Help the user out with some instruction and initial headers:
+
+		my $tpl_sender = $sender || $repoauthor || $repocommitter || '';
+		my $tpl_subject = $initial_subject || '';
+		my $tpl_reply_to = $initial_reply_to || '';
+
+		print $compose_file <<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
@@ -543,10 +557,10 @@ Subject: $tpl_subject
 In-Reply-To: $tpl_reply_to
 
 EOT
-	for my $f (@files) {
-		print C "GIT: ", get_patch_subject($f), "\n";
+		for my $f (@files) {
+			print $compose_file "GIT: ", get_patch_subject($f), "\n";
+		}
 	}
-	close(C);
 
 	my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, "core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
 
@@ -556,23 +570,28 @@ EOT
 		do_edit($compose_filename);
 	}
 
-	open(C2,">",$compose_filename . ".final")
-		or die "Failed to open $compose_filename.final : " . $!;
+	# Now transform the user-edited introduction into something
+	# suitable for sending via email; the user's editor may have
+	# unlinked the original file and replaced it with an entirely
+	# new one. If this be the case, then it wouldn't do just to
+	# seek to the beginning and start reading, because then only
+	# the original content would be retrieved. Consequently, the
+	# file must be reopened to be safe (note, the original
+	# filehandle is closed automatically).
 
-	open(C,"<",$compose_filename)
-		or die "Failed to open $compose_filename : " . $!;
+	open $compose_file, "<", $compose_filename
+		or die "Failed to open '$compose_filename' for reading: $!";
 
 	my $need_8bit_cte = file_has_nonascii($compose_filename);
 	my $in_body = 0;
-	my $summary_empty = 1;
-	while(<C>) {
+	while(<$compose_file>) {
 		next if m/^GIT:/;
-		if ($in_body) {
-			$summary_empty = 0 unless (/^\n$/);
+		if ($in_body && not /^\n$/) {
+			 $compose_final_is_not_empty = 1;
 		} elsif (/^\n$/) {
 			$in_body = 1;
 			if ($need_8bit_cte) {
-				print C2 "MIME-Version: 1.0\n",
+				print $compose_final_file "MIME-Version: 1.0\n",
 					 "Content-Type: text/plain; ",
 					   "charset=utf-8\n",
 					 "Content-Transfer-Encoding: 8bit\n";
@@ -597,15 +616,12 @@ EOT
 			print "To/Cc/Bcc fields are not interpreted yet, they have been ignored\n";
 			next;
 		}
-		print C2 $_;
-	}
-	close(C);
-	close(C2);
 
-	if ($summary_empty) {
-		print "Summary email is empty, skipping it\n";
-		$compose = -1;
+		print $compose_final_file $_;
 	}
+
+	print "Summary email is empty, skipping it\n" unless ($compose_final_is_not_empty);
+
 } elsif ($annotate) {
 	do_edit(@files);
 }
@@ -685,9 +701,7 @@ if (!defined $smtp_server) {
 	$smtp_server ||= 'localhost'; # could be 127.0.0.1, too... *shrug*
 }
 
-if ($compose && $compose > 0) {
-	@files = ($compose_filename . ".final", @files);
-}
+unshift(@files, $compose_final_filename) if ($compose_final_is_not_empty);
 
 # Variables we set as part of the loop over files
 our ($message_id, %mail, $subject, $reply_to, $references, $message,
@@ -1153,7 +1167,13 @@ for (my $index = 0; $index < @files; $index++) {
 cleanup_compose_files();
 
 sub cleanup_compose_files() {
-	unlink($compose_filename, $compose_filename . ".final") if $compose;
+	if (defined $compose) {
+		unlink(
+			$compose_final_filename,
+			# Don't delete user-supplied file.
+			$compose ? () : $compose_filename
+		);
+	}
 }
 
 $smtp->quit if $smtp;
-- 
1.6.2.2.448.g61445.dirty

[PATCH RFC 5/6] send-email: Cleanup the usage text a bit

[Michael Witten]

All lines should be < 80 characters.

Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 git-send-email.perl |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 481bf36..c3e3598 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -42,6 +42,9 @@ package main;
 
 
 sub usage {
+
+	# All printed lines should be less than 80 characters.
+
 	print <<EOT;
 git send-email [options] <file | directory | rev-list options >
 
@@ -52,7 +55,8 @@ git send-email [options] <file | directory | rev-list options >
     --bcc                   <str>  * Email Bcc:
     --subject               <str>  * Email "Subject:"
     --in-reply-to           <str>  * Email "In-Reply-To:"
-    --annotate                     * Review each patch that will be sent in an editor.
+    --annotate                     * Review each patch that will be sent in
+                                     an editor.
     --compose           opt <str>  * Open an editor for introduction.
 
   Sending:
@@ -69,8 +73,10 @@ git send-email [options] <file | directory | rev-list options >
   Automating:
     --identity              <str>  * Use the sendemail.<id> options.
     --cc-cmd                <str>  * Email Cc: via `<str> \$patch_path`
-    --suppress-cc           <str>  * author, self, sob, cc, cccmd, body, bodycc, all.
-    --[no-]signed-off-by-cc        * Send to Signed-off-by: addresses. Default on.
+    --suppress-cc           <str>  * author, self, sob, cc, cccmd, body,
+                                     bodycc, all.
+    --[no-]signed-off-by-cc        * Send to Signed-off-by: addresses.
+                                     Default on.
     --[no-]suppress-from           * Send to self. Default off.
     --[no-]chain-reply-to          * Chain In-Reply-To: fields. Default on.
     --[no-]thread                  * Use In-Reply-To: field. Default on.
@@ -81,8 +87,8 @@ git send-email [options] <file | directory | rev-list options >
     --quiet                        * Output one line of info per email.
     --dry-run                      * Don't actually send the emails.
     --[no-]validate                * Perform patch sanity checks. Default on.
-    --[no-]format-patch            * understand any non optional arguments as
-                                     `git format-patch` ones.
+    --[no-]format-patch            * Understand any non-optional arguments as
+                                     `git format-patch' arguments.
 
 EOT
 	exit(1);
-- 
1.6.2.2.448.g61445.dirty

[PATCH RFC 6/6] send-email: Remove horrible mix of tabs and spaces

[Michael Witten]

For the most part, I ran a search for all the lines
that match:

    ^[\t]*[ ]+

and then I manually replaced the offending text with
an appropriate number of tabs.

While scanning through the file, I also tried to format
some of the code so as to obviate future mixing; I also
fixed one horrendously egregious section of code, where
someone was trying to be unnecessarily compact.

Currently, no lines match the following:

    [\t]+[ ]+
    [ ]+[\t]+

So, it should be reasonably clean.

The whole file is still horrendous.

Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 git-send-email.perl |  282 +++++++++++++++++++++++++++++++--------------------
 1 files changed, 170 insertions(+), 112 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index c3e3598..a4c24f3 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -16,6 +16,14 @@
 #    and second line is the subject of the message.
 #
 
+## WARNING! ACHTUNG! ATTENTION! ADVERTENCIA!
+##   Currently, this file uses tabs (like the rest of git source) to
+##   delineate code structure. Do NOT under any circumstances mix tabs
+##   and spaces across lines that share a relationship in terms of layout.
+##   In fact, it would currently be best to use only tabs, so please set
+##   your editor(s) accordingly. This code is already trashy enough. Please
+##   don't make it worse.
+
 use strict;
 use warnings;
 use Term::ReadLine;
@@ -118,19 +126,20 @@ sub format_2822_time {
 		die ("local time offset greater than or equal to 24 hours\n");
 	}
 
-	return sprintf("%s, %2d %s %d %02d:%02d:%02d %s%02d%02d",
-		       qw(Sun Mon Tue Wed Thu Fri Sat)[$localtm[6]],
-		       $localtm[3],
-		       qw(Jan Feb Mar Apr May Jun
-			  Jul Aug Sep Oct Nov Dec)[$localtm[4]],
-		       $localtm[5]+1900,
-		       $localtm[2],
-		       $localtm[1],
-		       $localtm[0],
-		       ($offset >= 0) ? '+' : '-',
-		       abs($offhour),
-		       $offmin,
-		       );
+	return sprintf(
+		"%s, %2d %s %d %02d:%02d:%02d %s%02d%02d",
+		qw(Sun Mon Tue Wed Thu Fri Sat)[$localtm[6]],
+		$localtm[3],
+		qw(Jan Feb Mar Apr May Jun
+		Jul Aug Sep Oct Nov Dec)[$localtm[4]],
+		$localtm[5]+1900,
+		$localtm[2],
+		$localtm[1],
+		$localtm[0],
+		($offset >= 0) ? '+' : '-',
+		abs($offhour),
+		$offmin,
+	);
 }
 
 my $have_email_valid = eval { require Email::Valid; 1 };
@@ -194,30 +203,30 @@ my ($validate, $confirm);
 my (@suppress_cc);
 
 my %config_bool_settings = (
-    "thread" => [\$thread, 1],
-    "chainreplyto" => [\$chain_reply_to, 1],
-    "suppressfrom" => [\$suppress_from, undef],
-    "signedoffbycc" => [\$signed_off_by_cc, undef],
-    "signedoffcc" => [\$signed_off_by_cc, undef],      # Deprecated
-    "validate" => [\$validate, 1],
+	"thread" => [\$thread, 1],
+	"chainreplyto" => [\$chain_reply_to, 1],
+	"suppressfrom" => [\$suppress_from, undef],
+	"signedoffbycc" => [\$signed_off_by_cc, undef],
+	"signedoffcc" => [\$signed_off_by_cc, undef],      # Deprecated
+	"validate" => [\$validate, 1],
 );
 
 my %config_settings = (
-    "delay" => \$delay,
-    "smtpserver" => \$smtp_server,
-    "smtpserverport" => \$smtp_server_port,
-    "smtpuser" => \$smtp_authuser,
-    "smtppass" => \$smtp_authpass,
-    "to" => \@to,
-    "cc" => \@initial_cc,
-    "cccmd" => \$cc_cmd,
-    "aliasfiletype" => \$aliasfiletype,
-    "bcc" => \@bcclist,
-    "aliasesfile" => \@alias_files,
-    "suppresscc" => \@suppress_cc,
-    "envelopesender" => \$envelope_sender,
-    "multiedit" => \$multiedit,
-    "confirm"   => \$confirm,
+	"delay" => \$delay,
+	"smtpserver" => \$smtp_server,
+	"smtpserverport" => \$smtp_server_port,
+	"smtpuser" => \$smtp_authuser,
+	"smtppass" => \$smtp_authpass,
+	"to" => \@to,
+	"cc" => \@initial_cc,
+	"cccmd" => \$cc_cmd,
+	"aliasfiletype" => \$aliasfiletype,
+	"bcc" => \@bcclist,
+	"aliasesfile" => \@alias_files,
+	"suppresscc" => \@suppress_cc,
+	"envelopesender" => \$envelope_sender,
+	"multiedit" => \$multiedit,
+	"confirm"   => \$confirm,
 );
 
 # Handle Uncouth Termination
@@ -247,38 +256,39 @@ $SIG{INT}  = \&signal_handler;
 # Begin by accumulating all the variables (defined above), that we will end up
 # needing, first, from the command line:
 
-my $rc = GetOptions("sender|from=s" => \$sender,
-                    "in-reply-to=s" => \$initial_reply_to,
-		    "subject=s" => \$initial_subject,
-		    "to=s" => \@to,
-		    "cc=s" => \@initial_cc,
-		    "bcc=s" => \@bcclist,
-		    "chain-reply-to!" => \$chain_reply_to,
-		    "delay=i" => \$delay,
-		    "smtp-server=s" => \$smtp_server,
-		    "smtp-server-port=i" => \$smtp_server_port,
-		    "smtp-user=s" => \$smtp_authuser,
-		    "smtp-pass:s" => \$smtp_authpass,
-		    "smtp-ssl" => sub { $smtp_encryption = 'ssl' },
-		    "smtp-encryption=s" => \$smtp_encryption,
-		    "identity=s" => \$identity,
-		    "annotate" => \$annotate,
-		    "compose:s" => \$compose,
-		    "quiet" => \$quiet,
-		    "cc-cmd=s" => \$cc_cmd,
-		    "suppress-from!" => \$suppress_from,
-		    "suppress-cc=s" => \@suppress_cc,
-		    "signed-off-cc|signed-off-by-cc!" => \$signed_off_by_cc,
-		    "confirm=s" => \$confirm,
-		    "dry-run" => \$dry_run,
-		    "envelope-sender=s" => \$envelope_sender,
-		    "thread!" => \$thread,
-		    "validate!" => \$validate,
-		    "format-patch!" => \$format_patch,
-	 );
+my $rc = GetOptions(
+	"sender|from=s" => \$sender,
+	"in-reply-to=s" => \$initial_reply_to,
+	"subject=s" => \$initial_subject,
+	"to=s" => \@to,
+	"cc=s" => \@initial_cc,
+	"bcc=s" => \@bcclist,
+	"chain-reply-to!" => \$chain_reply_to,
+	"delay=i" => \$delay,
+	"smtp-server=s" => \$smtp_server,
+	"smtp-server-port=i" => \$smtp_server_port,
+	"smtp-user=s" => \$smtp_authuser,
+	"smtp-pass:s" => \$smtp_authpass,
+	"smtp-ssl" => sub { $smtp_encryption = 'ssl' },
+	"smtp-encryption=s" => \$smtp_encryption,
+	"identity=s" => \$identity,
+	"annotate" => \$annotate,
+	"compose:s" => \$compose,
+	"quiet" => \$quiet,
+	"cc-cmd=s" => \$cc_cmd,
+	"suppress-from!" => \$suppress_from,
+	"suppress-cc=s" => \@suppress_cc,
+	"signed-off-cc|signed-off-by-cc!" => \$signed_off_by_cc,
+	"confirm=s" => \$confirm,
+	"dry-run" => \$dry_run,
+	"envelope-sender=s" => \$envelope_sender,
+	"thread!" => \$thread,
+	"validate!" => \$validate,
+	"format-patch!" => \$format_patch,
+);
 
 unless ($rc) {
-    usage();
+	usage();
 }
 
 die "Cannot run git format-patch from outside a repository\n"
@@ -407,29 +417,55 @@ sub split_addrs {
 my %aliases;
 my %parse_alias = (
 	# multiline formats can be supported in the future
-	mutt => sub { my $fh = shift; while (<$fh>) {
-		if (/^\s*alias\s+(\S+)\s+(.*)$/) {
-			my ($alias, $addr) = ($1, $2);
-			$addr =~ s/#.*$//; # mutt allows # comments
-			 # commas delimit multiple addresses
-			$aliases{$alias} = [ split_addrs($addr) ];
-		}}},
-	mailrc => sub { my $fh = shift; while (<$fh>) {
-		if (/^alias\s+(\S+)\s+(.*)$/) {
-			# spaces delimit multiple addresses
-			$aliases{$1} = [ split(/\s+/, $2) ];
-		}}},
-	pine => sub { my $fh = shift; my $f='\t[^\t]*';
-	        for (my $x = ''; defined($x); $x = $_) {
+	mutt => sub {
+
+		my $fh = shift;
+
+		while (<$fh>) {
+			if (/^\s*alias\s+(\S+)\s+(.*)$/) {
+				my ($alias, $addr) = ($1, $2);
+				$addr =~ s/#.*$//; # mutt allows # comments
+				# commas delimit multiple addresses
+				$aliases{$alias} = [ split_addrs($addr) ];
+			}
+		}
+	},
+
+	mailrc => sub {
+
+		my $fh = shift;
+
+		while (<$fh>) {
+			if (/^alias\s+(\S+)\s+(.*)$/) {
+				# spaces delimit multiple addresses
+				$aliases{$1} = [ split(/\s+/, $2) ];
+			}
+		}
+	},
+
+	pine => sub {
+
+		my $fh = shift;
+		my $f='\t[^\t]*';
+
+		for (my $x = ''; defined($x); $x = $_) {
 			chomp $x;
-		        $x .= $1 while(defined($_ = <$fh>) && /^ +(.*)$/);
+			$x .= $1 while(defined($_ = <$fh>) && /^ +(.*)$/);
 			$x =~ /^(\S+)$f\t\(?([^\t]+?)\)?(:?$f){0,2}$/ or next;
 			$aliases{$1} = [ split_addrs($2) ];
-		}},
-	gnus => sub { my $fh = shift; while (<$fh>) {
-		if (/\(define-mail-alias\s+"(\S+?)"\s+"(\S+?)"\)/) {
-			$aliases{$1} = [ $2 ];
-		}}}
+		}
+	},
+
+	gnus => sub {
+
+		my $fh = shift;
+
+		while (<$fh>) {
+			if (/\(define-mail-alias\s+"(\S+?)"\s+"(\S+?)"\)/) {
+				$aliases{$1} = [ $2 ];
+			}
+		}
+	}
 );
 
 if (@alias_files and $aliasfiletype and defined $parse_alias{$aliasfiletype}) {
@@ -597,10 +633,11 @@ EOT
 		} elsif (/^\n$/) {
 			$in_body = 1;
 			if ($need_8bit_cte) {
-				print $compose_final_file "MIME-Version: 1.0\n",
-					 "Content-Type: text/plain; ",
-					   "charset=utf-8\n",
-					 "Content-Transfer-Encoding: 8bit\n";
+				print $compose_final_file
+					"MIME-Version: 1.0\n",
+					"Content-Type: text/plain; ",
+					"charset=utf-8\n",
+					"Content-Transfer-Encoding: 8bit\n";
 			}
 		} elsif (/^MIME-Version:/i) {
 			$need_8bit_cte = 0;
@@ -609,8 +646,8 @@ EOT
 			my $subject = $initial_subject;
 			$_ = "Subject: " .
 				($subject =~ /[^[:ascii:]]/ ?
-				 quote_rfc2047($subject) :
-				 $subject) .
+				quote_rfc2047($subject) :
+				$subject) .
 				"\n";
 		} elsif (/^In-Reply-To:\s*(.+)\s*$/i) {
 			$initial_reply_to = $1;
@@ -639,8 +676,10 @@ sub ask {
 	my $resp;
 	my $i = 0;
 	return defined $default ? $default : undef
-		unless defined $term->IN and defined fileno($term->IN) and
-		       defined $term->OUT and defined fileno($term->OUT);
+		unless defined $term->IN
+			and defined fileno($term->IN)
+			and defined $term->OUT
+			and defined fileno($term->OUT);
 	while ($i++ < 10) {
 		$resp = $term->readline($prompt);
 		if (!defined $resp) { # EOF
@@ -660,8 +699,12 @@ sub ask {
 my $prompting = 0;
 if (!defined $sender) {
 	$sender = $repoauthor || $repocommitter || '';
-	$sender = ask("Who should the emails appear to be from? [$sender] ",
-	              default => $sender);
+
+	$sender = ask(
+		"Who should the emails appear to be from? [$sender] ",
+		default => $sender
+	);
+
 	print "Emails will be sent from: ", $sender, "\n";
 	$prompting++;
 }
@@ -689,7 +732,8 @@ sub expand_aliases {
 
 if ($thread && !defined $initial_reply_to && $prompting) {
 	$initial_reply_to = ask(
-		"Message-ID to be used as In-Reply-To for the first email? ");
+		"Message-ID to be used as In-Reply-To for the first email? "
+	);
 }
 if (defined $initial_reply_to) {
 	$initial_reply_to =~ s/^\s*<?//;
@@ -823,18 +867,22 @@ sub sanitize_address
 sub send_message
 {
 	my @recipients = unique_email_list(@to);
-	@cc = (grep { my $cc = extract_valid_address($_);
-		      not grep { $cc eq $_ } @recipients
-		    }
-	       map { sanitize_address($_) }
-	       @cc);
+
+	@cc = (grep
+		{
+			my $cc = extract_valid_address($_);
+			not grep { $cc eq $_ } @recipients
+		}
+		map { sanitize_address($_) } @cc
+	);
+
 	my $to = join (",\n\t", @recipients);
 	@recipients = unique_email_list(@recipients,@cc,@bcclist);
 	@recipients = (map { extract_valid_address($_) } @recipients);
 	my $date = format_2822_time($time++);
 	my $gitversion = '@@GIT_VERSION@@';
 	if ($gitversion =~ m/..GIT_VERSION../) {
-	    $gitversion = Git::version();
+		$gitversion = Git::version();
 	}
 
 	my $cc = join(", ", unique_email_list(@cc));
@@ -883,9 +931,13 @@ X-Mailer: git-send-email $gitversion
 			print "    To retain the current behavior, but squelch this message,\n";
 			print "    run 'git config --global sendemail.confirm auto'.\n\n";
 		}
-		$_ = ask("Send this email? ([y]es|[n]o|[q]uit|[a]ll): ",
-		         valid_re => qr/^(?:yes|y|no|n|quit|q|all|a)/i,
-		         default => $ask_default);
+
+		$_ = ask(
+			"Send this email? ([y]es|[n]o|[q]uit|[a]ll): ",
+			valid_re => qr/^(?:yes|y|no|n|quit|q|all|a)/i,
+			default => $ask_default
+		);
+
 		die "Send this email reply required" unless defined $_;
 		if (/^n/i) {
 			return;
@@ -920,9 +972,13 @@ X-Mailer: git-send-email $gitversion
 		}
 		else {
 			require Net::SMTP;
-			$smtp ||= Net::SMTP->new((defined $smtp_server_port)
-						 ? "$smtp_server:$smtp_server_port"
-						 : $smtp_server);
+
+			$smtp ||= Net::SMTP->new(
+				(defined $smtp_server_port)
+					? "$smtp_server:$smtp_server_port"
+					: $smtp_server
+			);
+
 			if ($smtp_encryption eq 'tls') {
 				require Net::SMTP::SSL;
 				$smtp->command('STARTTLS');
@@ -1018,7 +1074,7 @@ for (my $index = 0; $index < @files; $index++) {
 			chomp($header[$#header]);
 			s/^\s+/ /;
 			$header[$#header] .= $_;
-	    } else {
+		} else {
 			push(@header, $_);
 		}
 	}
@@ -1136,9 +1192,9 @@ for (my $index = 0; $index < @files; $index++) {
 			}
 			else {
 				push @xh,
-				  'MIME-Version: 1.0',
-				  "Content-Type: text/plain; charset=$author_encoding",
-				  'Content-Transfer-Encoding: 8bit';
+					'MIME-Version: 1.0',
+					"Content-Type: text/plain; charset=$author_encoding",
+					'Content-Transfer-Encoding: 8bit';
 			}
 		}
 	}
@@ -1146,7 +1202,9 @@ for (my $index = 0; $index < @files; $index++) {
 	$needs_confirm = (
 		$confirm eq "always" or
 		($confirm =~ /^(?:auto|cc)$/ && @cc) or
-		($confirm =~ /^(?:auto|compose)$/ && $compose && $message_num == 1));
+		($confirm =~ /^(?:auto|compose)$/ && $compose && $message_num == 1)
+	);
+
 	$needs_confirm = "inform" if ($needs_confirm && $confirm_unconfigured && @cc);
 
 	@cc = (@initial_cc, @cc);
-- 
1.6.2.2.448.g61445.dirty

Re: [PATCH RFC 6/6] send-email: Remove horrible mix of tabs and spaces

[demerphq]

2009/4/7 Michael Witten <mfwitten@gmail.com>:
> +## WARNING! ACHTUNG! ATTENTION! ADVERTENCIA!
> +##   Currently, this file uses tabs (like the rest of git source) to
> +##   delineate code structure. Do NOT under any circumstances mix tabs
> +##   and spaces across lines that share a relationship in terms of layout.
> +##   In fact, it would currently be best to use only tabs, so please set
> +##   your editor(s) accordingly. This code is already trashy enough. Please
> +##   don't make it worse.

Perltidy the file?

Yves


-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

Re: [PATCH RFC 6/6] send-email: Remove horrible mix of tabs and spaces

[Michael Witten]

On Tue, Apr 7, 2009 at 16:35, demerphq <demerphq@gmail.com> wrote:
> 2009/4/7 Michael Witten <mfwitten@gmail.com>:
>> +## WARNING! ACHTUNG! ATTENTION! ADVERTENCIA!
>> +##   Currently, this file uses tabs (like the rest of git source) to
>> +##   delineate code structure. Do NOT under any circumstances mix tabs
>> +##   and spaces across lines that share a relationship in terms of layout.
>> +##   In fact, it would currently be best to use only tabs, so please set
>> +##   your editor(s) accordingly. This code is already trashy enough. Please
>> +##   don't make it worse.
>
> Perltidy the file?
>
> Yves

Oooh, that's sexy!

I'll have to give that a try. However, I've been entertaining the idea
of rewriting the whole thing anyway; it's in need of much more than
reformatting.

As a side note, Yves, I sent the patches to perl5-porters again about
1.6666666 hours ago, but nothing seems to have come through; I'll try
again later.

Michael

Re: [PATCH RFC 6/6] send-email: Remove horrible mix of tabs and spaces

[demerphq]

2009/4/7 Michael Witten <mfwitten@gmail.com>:
> On Tue, Apr 7, 2009 at 16:35, demerphq <demerphq@gmail.com> wrote:
>> 2009/4/7 Michael Witten <mfwitten@gmail.com>:
>>> +## WARNING! ACHTUNG! ATTENTION! ADVERTENCIA!
>>> +##   Currently, this file uses tabs (like the rest of git source) to
>>> +##   delineate code structure. Do NOT under any circumstances mix tabs
>>> +##   and spaces across lines that share a relationship in terms of layout.
>>> +##   In fact, it would currently be best to use only tabs, so please set
>>> +##   your editor(s) accordingly. This code is already trashy enough. Please
>>> +##   don't make it worse.
>>
>> Perltidy the file?
>>
>> Yves
>
> Oooh, that's sexy!
>
> I'll have to give that a try. However, I've been entertaining the idea
> of rewriting the whole thing anyway; it's in need of much more than
> reformatting.

Yes. I notice evilness in there. Not necessarily dire burn your house
down evilness, but evilness none the less.

> As a side note, Yves, I sent the patches to perl5-porters again about
> 1.6666666 hours ago, but nothing seems to have come through; I'll try
> again later.

You mean the 14 patches I just applied and pushed to blead?

:-)

cheers,
Yves



-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

Re: [PATCH RFC 6/6] send-email: Remove horrible mix of tabs and spaces

[demerphq]

2009/4/7 demerphq <demerphq@gmail.com>:
> 2009/4/7 Michael Witten <mfwitten@gmail.com>:
>> On Tue, Apr 7, 2009 at 16:35, demerphq <demerphq@gmail.com> wrote:
>>> 2009/4/7 Michael Witten <mfwitten@gmail.com>:
>>>> +## WARNING! ACHTUNG! ATTENTION! ADVERTENCIA!
>>>> +##   Currently, this file uses tabs (like the rest of git source) to
>>>> +##   delineate code structure. Do NOT under any circumstances mix tabs
>>>> +##   and spaces across lines that share a relationship in terms of layout.
>>>> +##   In fact, it would currently be best to use only tabs, so please set
>>>> +##   your editor(s) accordingly. This code is already trashy enough. Please
>>>> +##   don't make it worse.
>>>
>>> Perltidy the file?
>>>
>>> Yves
>>
>> Oooh, that's sexy!
>>
>> I'll have to give that a try. However, I've been entertaining the idea
>> of rewriting the whole thing anyway; it's in need of much more than
>> reformatting.
>
> Yes. I notice evilness in there. Not necessarily dire burn your house
> down evilness, but evilness none the less.

BTW, I'd be glad to help out if you want, as I suspect would some of
the p5p folks.

Yves

-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

Re: [PATCH RFC 6/6] send-email: Remove horrible mix of tabs and spaces

[Jeff King]

On Tue, Apr 07, 2009 at 04:42:36PM -0500, Michael Witten wrote:

> I'll have to give that a try. However, I've been entertaining the idea
> of rewriting the whole thing anyway; it's in need of much more than
> reformatting.

Just my two cents, but if you are considering re-writing send-email, I
would suggest two things:

  1. Make much heavier use of existing CPAN libraries. A lot of the ugly
     code is trying to handle corner cases in rfc2822 and mime parsing
     and generation. And I would not be surprised if there were still
     bugs in that ugly code.

  2. Make a new command to compete with send-email instead of using the
     same name. This means that people who are really put off by
     CPAN dependencies from (1) above won't be negatively impacted. And
     you can drop any historical interface warts if you want to.

-Peff

Re: [PATCH RFC 6/6] send-email: Remove horrible mix of tabs and spaces

[Andreas Ericsson]

Jeff King wrote:
> On Tue, Apr 07, 2009 at 04:42:36PM -0500, Michael Witten wrote:
> 
>> I'll have to give that a try. However, I've been entertaining the idea
>> of rewriting the whole thing anyway; it's in need of much more than
>> reformatting.
> 
> Just my two cents, but if you are considering re-writing send-email, I
> would suggest two things:
> 
>   1. Make much heavier use of existing CPAN libraries. A lot of the ugly
>      code is trying to handle corner cases in rfc2822 and mime parsing
>      and generation. And I would not be surprised if there were still
>      bugs in that ugly code.
> 
>   2. Make a new command to compete with send-email instead of using the
>      same name. This means that people who are really put off by
>      CPAN dependencies from (1) above won't be negatively impacted. And
>      you can drop any historical interface warts if you want to.
> 

3. Make it capable of sending email directly from commits rather than
   than having to generate them as files first. For bonus-points, use
   git sequencer or some other "git rebase -i"-esque mangling thing
   first, with capabilities of adding a cover-letter for patch-series.

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

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 RFC 6/6] send-email: Remove horrible mix of tabs and spaces

[Tomas Carnecky]

On Apr 8, 2009, at 12:10 AM, Andreas Ericsson wrote:
> 3. Make it capable of sending email directly from commits rather than
>  than having to generate them as files first. For bonus-points, use

This is already possible:
   git send-email [options] <file|directory|rev-list options>
You can pass it a rev-list and it will generate the patches on its own.

>  git sequencer or some other "git rebase -i"-esque mangling thing
>  first, with capabilities of adding a cover-letter for patch-series.


tom

Re: [PATCH RFC 6/6] send-email: Remove horrible mix of tabs and spaces

[Jeff King]

On Wed, Apr 08, 2009 at 12:10:37AM +0200, Andreas Ericsson wrote:

>>   2. Make a new command to compete with send-email instead of using the
>>      same name. This means that people who are really put off by
>>      CPAN dependencies from (1) above won't be negatively impacted. And
>>      you can drop any historical interface warts if you want to.
>>
>
> 3. Make it capable of sending email directly from commits rather than
>   than having to generate them as files first. For bonus-points, use
>   git sequencer or some other "git rebase -i"-esque mangling thing
>   first, with capabilities of adding a cover-letter for patch-series.

As Tomas noted, this is already possible (this was due to some patches
from Pierre a few months ago, I think). But IIRC, there are some corner
cases where send-email has to guess whether you mean rev-list options or
a file (or maybe there is some way to signal, I don't remember). And
that is exactly the sort of thing that you could clean up via (2) above.

-Peff

Re: [PATCH RFC 5/6] send-email: Cleanup the usage text a bit

[Junio C Hamano]

Michael Witten <mfwitten@gmail.com> writes:

> All lines should be < 80 characters.
>
> Signed-off-by: Michael Witten <mfwitten@gmail.com>
> ---
>  git-send-email.perl |   16 +++++++++++-----
>  1 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 481bf36..c3e3598 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -42,6 +42,9 @@ package main;
>  
>  
>  sub usage {
> +
> +	# All printed lines should be less than 80 characters.
> +

Perhaps a good idea.  It would have been nicer if this were early in the
series.

Re: [PATCH RFC 4/6] send-email: --compose takes optional argument to existing file

[Junio C Hamano]

Michael Witten <mfwitten@gmail.com> writes:

> Now, a user may specify an existing (in-progress) file to use as
> the introductory/summary email.
>
> The file is opened for any additional editing as usual.
>
> Signed-off-by: Michael Witten <mfwitten@gmail.com>
> ---
>  Documentation/git-send-email.txt |    7 ++-
>  git-send-email.perl              |  112 ++++++++++++++++++++++----------------
>  2 files changed, 71 insertions(+), 48 deletions(-)
>
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index 4b656ca..bc9ff13 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -57,8 +57,11 @@ The --cc option must be repeated for each user you want on the cc list.
>  	or one for all of them at once.
>  
>  --compose::
> -	Use $GIT_EDITOR, core.editor, $VISUAL, or $EDITOR to edit an
> -	introductory message for the patch series.
> +	Use $GIT_EDITOR, core.editor, $VISUAL, or $EDITOR, or vi to edit an

I think addition of ", or vi" makes sense but the ", or $EDITOR" needs to
change, and also the language needs to be clarified to say the first one
of these is used.

The name of the new variable $compose_file that is used as a filehandle
was confusing to read (almost everybody else in the program uses $fh, and
it looked as if $compose_file is talking about the name of the file or
indirectly pointing at an existing filehandle, but it was not).

Re: [PATCH RFC 3/6] send-email: Handle "GIT:" rather than "GIT: " during --compose

[Junio C Hamano]

Michael Witten <mfwitten@gmail.com> writes:

> This should make things a little more robust in terms of user input;
> before, even the program got it wrong by outputting a line with only
> "GIT:", which was left in place as a header, because there would be
> no following space character.

An alternative could be to add an extra space after the "GIT:" on the
lines the compose template generated by this program, but people can set
their editors to strip trailing whitespaces, so I think yours is a better
approach.  I suspect this patch comes from your own experience of getting
bitten by this once, perhaps?

> Also, I cleaned up get_patch_subject().

Which is a bit iffy.  It does not belong to the primary topic of the patch
to begin with, so it shouldn't be in here even if it weren't iffy.

> diff --git a/git-send-email.perl b/git-send-email.perl
> index 63d6063..098c620 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -505,15 +505,16 @@ if (@files) {
>  }
>  
>  sub get_patch_subject($) {
> -	my $fn = shift;
> -	open (my $fh, '<', $fn);
> -	while (my $line = <$fh>) {
> -		next unless ($line =~ /^Subject: (.*)$/);
> -		close $fh;
> -		return "GIT: $1\n";
> +
> +	my $patch = shift;
> +	open (my $fh, '<', $patch);
> +
> +	while (<$fh>) {
> +		next unless (/^Subject: (.*)$/);
> +		return $1;
>  	}
> -	close $fh;
> -	die "No subject line in $fn ?";
> +
> +	die "'Subject:' line expected in '$patch'";
>  }

Because "while (<>)" does not localize $_, you are clobbering it in the
caller's context.  I do not know if any of the the existing callers cares,
but it is a change in behaviour.

$ cat >/var/tmp/j.perl <<\EOF
#!/usr/bin/perl -w
use strict;
sub foo($) {
	my $name = shift;
	open my $fh, "<$name";
	while (my $line = <$fh>) {
		chomp $line;
		close $fh;
		return $line;
	}
	close $fh;
	return undef;
}
sub bar($) {
	my $name = shift;
	open my $fh, "<$name";
	while (<$fh>) {
		chomp;
		close $fh;
		return $_;
	}
	close $fh;
	return undef;
}
$_ = 'original';
foo($0);
print "after running foo: $_\n";

$_ = 'original';
bar($0);
print "after running bar: $_\n";
EOF
$ perl /var/tmp/j.perl
after running foo: original
after running bar: #!/usr/bin/perl -w
$ exit

Re: [PATCH RFC 3/6] send-email: Handle "GIT:" rather than "GIT: " during --compose

[Michael Witten]

On Sat, Apr 11, 2009 at 14:22, Junio C Hamano <gitster@pobox.com> wrote:
> Michael Witten <mfwitten@gmail.com> writes:
>
>> This should make things a little more robust in terms of user input;
>> before, even the program got it wrong by outputting a line with only
>> "GIT:", which was left in place as a header, because there would be
>> no following space character.
>
> An alternative could be to add an extra space after the "GIT:" on the
> lines the compose template generated by this program, but people can set
> their editors to strip trailing whitespaces, so I think yours is a better
> approach.  I suspect this patch comes from your own experience of getting
> bitten by this once, perhaps?

My first thought was indeed just to add an extra space, but it occurred to me
that it's not easily remembered. Consider the original documentation:

> If the body of the message (what you type after the headers and a blank line) only contains blank (or GIT: prefixed) lines the summary won't be sent

>> Also, I cleaned up get_patch_subject().
>
> Which is a bit iffy.  It does not belong to the primary topic of the patch
> to begin with, so it shouldn't be in here even if it weren't iffy.

I can split it into another patch.

> Because "while (<>)" does not localize $_, you are clobbering it in the
> caller's context.  I do not know if any of the the existing callers cares,
> but it is a change in behaviour.

How about:

    while (local $_ = <$fh>)

Or, in our case, this:

    while (my $_ = <$fh>)

In testing these, I came across behavior that I think is incorrect, and I
have a mind to complain about it to the perl guys:

# Well! the print `function' doesn't seem to play by the rules.

# Example 0
# I expect the output to be:
# 1
# 1
# 3
# and I am right!

$_ = 3;

{
  local $_ = 1;
  print; print "\n";
  print $_; print "\n";
}

print; print "\n";

##############################################

# Example 1
# I expect the output to be:
# 3
# 1
# 3
# But it is:
# 1
# 1
# 3

$_ = 3;

{
  my $_ = 1;
  print; print "\n";
  print $_; print "\n";
}

print; print "\n";

###############################################

# Example 2
# I expect the output to be:
# 1
# 1
# 3
# and I am right!

sub my_print {
  print(shift or $_);
}

$_ = 3;

{
  local $_ = 1;
  my_print; print "\n";
  my_print $_; print "\n";
}

my_print; print "\n";

###############################################

# Example 3
# I expect the output to be:
# 3
# 1
# 3
# and I am right this time!

sub my_print {
  print(shift or $_);
}

$_ = 3;

{
  my $_ = 1;
  my_print; print "\n";
  my_print $_; print "\n";
}

my_print; print "\n";

Re: [PATCH RFC 3/6] send-email: Handle "GIT:" rather than "GIT: " during --compose

[Junio C Hamano]

Michael Witten <mfwitten@gmail.com> writes:

>> Because "while (<>)" does not localize $_, you are clobbering it in the
>> caller's context.  I do not know if any of the the existing callers cares,
>> but it is a change in behaviour.
>
> How about:
>
>     while (local $_ = <$fh>)
>
> Or, in our case, this:
>
>     while (my $_ = <$fh>)

Special variables like $_ cannot be made into lexicals, unless you know
you will only run with a very recent version of Perl (5.9.1, I think).

If you do not want to worry about portability, typically it is easiest to
say "local ($_)" upfront in the beginning of a sub.

I do not understand why you want to change the original

	while (my $line = <$fh>) {
        	...
	}

though.

Re: [PATCH RFC 3/6] send-email: Handle "GIT:" rather than "GIT: " during --compose

[Michael Witten]

On Sat, Apr 11, 2009 at 19:59, Junio C Hamano <gitster@pobox.com> wrote:
> Special variables like $_ cannot be made into lexicals, unless you know
> you will only run with a very recent version of Perl (5.9.1, I think).

Ah. I wish Perl's docs were a little more forthcoming about such
things. I ended up just trying it out, and indeed I am running 5.10.0.

> If you do not want to worry about portability, typically it is easiest to
> say "local ($_)" upfront in the beginning of a sub.
>
> I do not understand why you want to change the original
>
>        while (my $line = <$fh>) {
>                ...
>        }
>
> though.

I'm not terribly bent on it; I had already reshaped that function and
I was curious about making what I had done work out of intellectual
curiosity. In fact, my commits no longer have the refactored version.

Re: [PATCH RFC 2/6] send-email: --smtp-server-port should take an integer

[Junio C Hamano]

I think this makes sense but you made it depend on your --delay patch, so
it won't apply without wiggling the patch by hand.

I can do that, but a good rule of thumb when preparing a series is to have
this kind of obvious clean-up first, leaving enhancement patches later in
the series.

Re: [PATCH RFC 2/6] send-email: --smtp-server-port should take an integer

[Junio C Hamano]

Michael Witten <mfwitten@gmail.com> writes:

> Signed-off-by: Michael Witten <mfwitten@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 273c8c7..63d6063 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -251,7 +251,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
>  		    "chain-reply-to!" => \$chain_reply_to,
>  		    "delay=i" => \$delay,
>  		    "smtp-server=s" => \$smtp_server,
> -		    "smtp-server-port=s" => \$smtp_server_port,
> +		    "smtp-server-port=i" => \$smtp_server_port,
>  		    "smtp-user=s" => \$smtp_authuser,
>  		    "smtp-pass:s" => \$smtp_authpass,
>  		    "smtp-ssl" => sub { $smtp_encryption = 'ssl' },

Hmm, I have to wonder if there somebody who is using symbolic names for
ports, e.g. --smtp-server-port=ssmtp which this patch may start
rejecting.

Re: [PATCH RFC 2/6] send-email: --smtp-server-port should take an integer

[Wesley J. Landaker]

On Saturday 11 April 2009 13:22:29 Junio C Hamano wrote:
> Hmm, I have to wonder if there somebody who is using symbolic names for
> ports, e.g. --smtp-server-port=ssmtp which this patch may start
> rejecting.

An common example is --smtp-server-port=submission (for 587).

Re: [PATCH RFC 2/6] send-email: --smtp-server-port should take an integer

[Michael Witten]

On Sat, Apr 11, 2009 at 16:01, Wesley J. Landaker <wjl@icecavern.net> wrote:
> On Saturday 11 April 2009 13:22:29 Junio C Hamano wrote:
>> Hmm, I have to wonder if there somebody who is using symbolic names for
>> ports, e.g. --smtp-server-port=ssmtp which this patch may start
>> rejecting.
>
> An common example is --smtp-server-port=submission (for 587).

Clearly, then, this patch is no good. Consider it thrown out.

Re: [PATCH RFC 1/6] send-email: Add --delay for separating emails

[Jeff King]

On Tue, Apr 07, 2009 at 04:25:17PM -0500, Michael Witten wrote:

> When sending a patch series, the emails often arrive at the final
> destination out of order; though these emails should be chained
> via the In-Reply-To headers, some mail-viewing systems display
> by order of arrival instead.
> 
> The --delay option provides a means for specifying that there
> should be a certain number of seconds of delay between sending
> emails, so that the arrival order can be controlled better.
> 
> Signed-off-by: Michael Witten <mfwitten@gmail.com>

I'm a little dubious how well this works in practice. Have you done any
experiments?

The reason I am dubious is that you are presumably delaying only a few
seconds (since anything more would be quite annoying to the user). This
may deal with a short race condition in your local mail server. But what
is the real cause of out-of-order delivery? Is it the local mail server
seeing two messages essentially "simultaneously" and then reordering
them randomly? Or is it other random delays that happen _after_ that,
like network congestion, DNS lookups, down or congested servers, time it
takes to deliver the actual message body (e.g., if your mail server
sends two simultaneously, but the first one is much larger and takes
longer to complete), etc.

Those delays can be much larger than a few seconds, and this won't help
at all there.

I think it may still be reasonable to implement a solution that only
covers some of the cases, but I what I am asking is if we know what
percentage of the cases that is. If we are preventing only 1% of
out-of-order deliveries with this, I question whether it is worth the
bother.

-Peff

[PATCH RFC 1/6] Re: send-email: Add --delay for separating emails

[Nicolas Sebrecht]

On Tue, Apr 07, 2009 at 05:51:43PM -0400, Jeff King wrote:

> > When sending a patch series, the emails often arrive at the final
> > destination out of order; though these emails should be chained
> > via the In-Reply-To headers, some mail-viewing systems display
> > by order of arrival instead.
> > 
> > The --delay option provides a means for specifying that there
> > should be a certain number of seconds of delay between sending
> > emails, so that the arrival order can be controlled better.
> > 
> > Signed-off-by: Michael Witten <mfwitten@gmail.com>

> I think it may still be reasonable to implement a solution that only
> covers some of the cases, but I what I am asking is if we know what
> percentage of the cases that is. If we are preventing only 1% of
> out-of-order deliveries with this, I question whether it is worth the
> bother.

IMHO, this improvement is broken by design. We try to fix a
receiver-only issue by a sender side fix.

If the receiver wants the patch series be in a good ordered _for sure_, he
has to switch to a client mail supporting the In-Reply-To chains.

-- 
Nicolas Sebrecht

Re: [PATCH RFC 1/6] Re: send-email: Add --delay for separating emails

[Andreas Ericsson]

Nicolas Sebrecht wrote:
> On Tue, Apr 07, 2009 at 05:51:43PM -0400, Jeff King wrote:
> 
>>> When sending a patch series, the emails often arrive at the final
>>> destination out of order; though these emails should be chained
>>> via the In-Reply-To headers, some mail-viewing systems display
>>> by order of arrival instead.
>>>
>>> The --delay option provides a means for specifying that there
>>> should be a certain number of seconds of delay between sending
>>> emails, so that the arrival order can be controlled better.
>>>
>>> Signed-off-by: Michael Witten <mfwitten@gmail.com>
> 
>> I think it may still be reasonable to implement a solution that only
>> covers some of the cases, but I what I am asking is if we know what
>> percentage of the cases that is. If we are preventing only 1% of
>> out-of-order deliveries with this, I question whether it is worth the
>> bother.
> 
> IMHO, this improvement is broken by design. We try to fix a
> receiver-only issue by a sender side fix.
> 
> If the receiver wants the patch series be in a good ordered _for sure_, he
> has to switch to a client mail supporting the In-Reply-To chains.
> 

The biggest problem with in-reply-to chains is that they're absolutely
horrible for patch-series of more than five or so messages. The "worst"
one this week was a series of 14 patches, I believe. If any of the
deeper nested patches gets any sort of commentary, it usually eats so
much horizontal screen estate that it becomes hopeless to actually
find anything.

Besides that, most mua's I've worked with list emails in a thread
like this:

 First
  +------ second
  |         +------ third
  |         |
  |         +---- reply to second
  |                 +
  |                 |
  |                 + reply to reply to second
  |
  +-- reply to first

etc. etc, but when asked for "next unread message in thread", they
jump to the *deepest* message in the thread first, so you end up
reading the replies to the patches in the wrong order anyway.

For those two reasons, I absolutely loathe deeply nested
in-reply-to chains.

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

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 RFC 1/6] Re: send-email: Add --delay for separating emails

[Jeff King]

On Wed, Apr 08, 2009 at 12:17:54AM +0200, Andreas Ericsson wrote:

> For those two reasons, I absolutely loathe deeply nested
> in-reply-to chains.

Agreed.  I don't know if you saw:

  http://article.gmane.org/gmane.comp.version-control.git/109790

a while back.

-Peff

Re: [PATCH RFC 1/6] Re: send-email: Add --delay for separating emails

[Jeff King]

On Wed, Apr 08, 2009 at 12:08:54AM +0200, Nicolas Sebrecht wrote:

> > I think it may still be reasonable to implement a solution that only
> > covers some of the cases, but I what I am asking is if we know what
> > percentage of the cases that is. If we are preventing only 1% of
> > out-of-order deliveries with this, I question whether it is worth the
> > bother.
> 
> IMHO, this improvement is broken by design. We try to fix a
> receiver-only issue by a sender side fix.

I almost said the same thing: it is really the receiver's problem.
However, that doesn't mean the sender can't do simple things to help
hint the right thing to the receiver. For example, we already munge the
date fields to make sure the timestamp in each patch is increasing.

So there is precedent for giving hints to help the receiver sort the
patches. But munging the date fields is relatively transparent to the
sener. A multi-second delay is downright annoying. As a sender, I don't
think I would enable this option.

> If the receiver wants the patch series be in a good ordered _for sure_, he
> has to switch to a client mail supporting the In-Reply-To chains.

That's not enough for shallow-style patch series, like:

  PATCH 0/3
    \->PATCH 1/3
    \->PATCH 2/3
    \->PATCH 3/3

which is the proposed default for v1.6.3.

Many readers will sort by rfc822 date within a single thread level,
which is sufficient with what send-email currently generates. Sorting by
subject should also work fine.

But apparently many readers sort by date received. See this subthread:

  http://article.gmane.org/gmane.comp.version-control.git/110097

I am generally of the opinion that if it is a big problem for people,
they should get a better mail client. But I am also open to suggestions
for helping receivers on crappy mail clients as long as those
suggestions do not put a burden on the sender.

-Peff

Re: [PATCH RFC 1/6] send-email: Add --delay for separating emails

[Junio C Hamano]

Michael Witten <mfwitten@gmail.com> writes:

> When sending a patch series, the emails often arrive at the final
> destination out of order; though these emails should be chained
> via the In-Reply-To headers, some mail-viewing systems display
> by order of arrival instead.
>
> The --delay option provides a means for specifying that there
> should be a certain number of seconds of delay between sending
> emails, so that the arrival order can be controlled better.

If you are trying to force the order of messages that the client MUA
physically receives the messages, I do not think giving N second
interval at the sending end would help much in the real world.  Between
your submitting MUA (that's "git-send-email") and the client MUA, there
are many hops involved:

 * Your outgoing MSA (typically your ISP's sendmail) your MUA hands
   messages to;

 * Your ISP's internal mail routing chain of MTAs that forward the
   messages around;

 * The recipient's ISP's incoming MTA that receives the messages from
   your ISP's outgoing MTA;

 * The recipient's ISP's internal mail routing chain of MTAs that
   forward the messages around, until they reach...

 * ... the mailbox at recipient's ISP that stores the messages until the
   recipient picks them up;

 * And finally the recipient's MUA that reads from the mailbox.

Messages your MUA sends out can take different paths in the above chain
even though the final destination (mailbox at the recipient's ISP) may
be the same, and different mailpaths can and do have different
latencies.  Even if all the messages sent out by a single invocation of
your submitting MUA happened to take the same mailpath, any single hop
can batch the messages that arrive within a small time window before
passing them to the next hop, and it can reorder the messages when it
does so.

In short, the only thing your --delay can control is the arrival
interval at your outgoing MSA.  The arrival interval and order of
messages are outside your control for later hops.

On the other hand, I think send-email already has hacks to timestamp the
messages at least one-second apart by shifting the Date: field, so that
the recipient MUA can sort by the departure timestamp if it wants to (and
if it can), instead of the arrival timestamp.  Is it not working well for
you?

Re: [PATCH RFC 1/6] send-email: Add --delay for separating emails

[Michael Witten]

On Tue, Apr 07, 2009 at 04:25:17PM -0500, Michael Witten wrote:
>> When sending a patch series, the emails often arrive at the final
>> destination out of order; though these emails should be chained
>> via the In-Reply-To headers, some mail-viewing systems display
>> by order of arrival instead.
>>
>> The --delay option provides a means for specifying that there
>> should be a certain number of seconds of delay between sending
>> emails, so that the arrival order can be controlled better.

On Tue, Apr 7, 2009 at 16:51, Jeff King wrote:
> I'm a little dubious how well this works in practice. Have you done
> any experiments?

I have indeed, and I got better results. Whether that was a fluke,
I can't say for sure.

> [Delivery delays] can be much larger than a few seconds, and this
> won't help at all there.

and

On Tue, Apr 7, 2009 at 18:17, Junio C Hamano wrote:
> I do not think giving N second interval at the sending end would
> help much in the real world. Between your submitting MUA (that's
> "git-send-email") and the client MUA, there are many hops involved...
> any single hop can batch the messages that arrive within a small time
> window before passing them to the next hop, and it can reorder the
> messages when it does so.
>
> In short, the only thing your --delay can control is the arrival
> interval at your outgoing MSA. The arrival interval and order of
> messages are outside your control for later hops.

For the most part, yes, I am operating under the assumption that email
is sent as soon as it is received by any intervening hop (no batch
accumulation). However, I'm also postulating that there always exists
an N that serves as an upperbound on the transit time, regardless of
batching.

That's where these comment comes into play:

On Tue, Apr 7, 2009 at 16:51, Jeff King wrote:
> The reason I am dubious is that you are presumably delaying only a few
> seconds (since anything more would be quite annoying to the user).

On Wed, Apr 8, 2009 at 01:03:
> A multi-second delay is downright annoying. As a sender, I don't think
> I would enable this option.

However, this sentiment doesn't make sense to me.

Firstly, I presume that someone is electing to use this option, so it is
almost by definition not annoying for that person.

Secondly, it seems reasonable to drop into another VC, screen window, or
terminal instance, and then set send-email a-running. For instance, with
a 14-patch series, one could set `--delay 60' and then let send-email
run happily for the next 14 minutes with nary a thought.

However, I think you're coming at it from a different angle:

On Wed, Apr 8, 2009 at 01:03, Jeff King wrote:
> But apparently many readers sort by date received. See this subthread:
> 
>  http://article.gmane.org/gmane.comp.version-control.git/110097
> 
> I am generally of the opinion that if it is a big problem for people,
> they should get a better mail client. But I am also open to suggestions
> for helping receivers on crappy mail clients as long as those
> suggestions do not put a burden on the sender.

and

on Tue, Apr 7, 2009 at 17:08, Nicolas Sebrecht wrote:
> If the receiver wants the patch series be in a good ordered _for
> sure_, he has to switch to a client mail supporting the In-Reply-To
> chains.

Frankly, I don't care how other people's patch series appear to me. I care
about how mine appear to others. Is this irrational? Probably, but I'm kind
of OC; I want my patch series to look like it's in order for everyone.

This has nothing to do with what the receiver wants. This has everything
to do with what the sender wants. I want my patch series to be in order
even for wrongheaded receivers.

So, I never had any intention of forcing a delay on the sending end. It is
strictly for the sending end to use if desired.

On Tue, Apr 7, 2009 at 18:17, Junio C Hamano wrote:
> I think send-email already has hacks to timestamp the messages at
> least one-second apart by shifting the Date: field, so that the
> recipient MUA can sort by the departure timestamp if it wants to (and
> if it can), instead of the arrival timestamp. Is it not working well
> for you?

I have worked with 2 major mail clients that both display by date received:

  * Mac OS X's Mail
  * Gmail

I assume that a not-insignificant number of others also use these clients.
I don't mind letting send-email run in the background for a few minutes if
it means my patch series appear in order.

As you say, Jeff:

> On Wed, Apr 08, 2009 at 12:08:54AM +0200, Nicolas Sebrecht wrote:
>> IMHO, this improvement is broken by design. We try to fix a
>> receiver-only issue by a sender side fix.
> 
> I almost said the same thing: it is really the receiver's problem.
> However, that doesn't mean the sender can't do simple things to help
> hint the right thing to the receiver. For example, we already munge the
> date fields to make sure the timestamp in each patch is increasing.

There's nothing simpler than slowing the rate of outgoing email. It doesn't
even have to be used; it is completely unintrusive and fully automated. It
even works with the confirmation. Best of all, it didn't take much code to
implement.

Sincerely,
Michael Witten

[PATCH RFC 1/6] Re: send-email: Add --delay for separating emails

[Nicolas Sebrecht]

On Wed, Apr 08, 2009 at 09:25:25AM -0500, Michael Witten wrote:


> This has nothing to do with what the receiver wants. This has everything
> to do with what the sender wants. I want my patch series to be in order
> even for wrongheaded receivers.

The --delay option may have an undesirable side effect. In case of
non-chained emails, unrelated mails could be insterted between patches
where *all* MUA would be affected. It's not only true for very high
volume message mailing-lists (million monkeys receiving...). FMPOV, it's
worse than all display issues we already know or have with the current
behaviour. 

-- 
Nicolas Sebrecht

Re: [PATCH RFC 1/6] Re: send-email: Add --delay for separating emails

[Michael Witten]

On Thu, Apr 9, 2009 at 11:17, Nicolas Sebrecht
<nicolas.s-dev@laposte.net> wrote:
>
> The --delay option may have an undesirable side effect. In case of
> non-chained emails, unrelated mails could be insterted between patches
> where *all* MUA would be affected. It's not only true for very high
> volume message mailing-lists (million monkeys receiving...). FMPOV, it's
> worse than all display issues we already know or have with the current
> behaviour.

But it's already impossible to protect against this scenario. In that
situation, the smallest delay possible is desired, so --delay wouldn't
even be used (that is, its value would be zero). However, the transit
delay could never be small enough to guarantee that no other emails
are inserted into the patch series, so the only solution is to chain
them. At this point, we're back to the problem of arrival time, and
hence --delay becomes useful.

:-D

Re: [PATCH RFC 1/6] Re: send-email: Add --delay for separating emails

[Michael Witten]

On Thu, Apr 9, 2009 at 12:27, Michael Witten <mfwitten@gmail.com> wrote:
> On Thu, Apr 9, 2009 at 11:17, Nicolas Sebrecht
> <nicolas.s-dev@laposte.net> wrote:
>>
>> The --delay option may have an undesirable side effect. In case of
>> non-chained emails, unrelated mails could be insterted between patches
>> where *all* MUA would be affected. It's not only true for very high
>> volume message mailing-lists (million monkeys receiving...). FMPOV, it's
>> worse than all display issues we already know or have with the current
>> behaviour.
>
> But it's already impossible to protect against this scenario. In that
> situation, the smallest delay possible is desired, so --delay wouldn't
> even be used (that is, its value would be zero). However, the transit
> delay could never be small enough to guarantee that no other emails
> are inserted into the patch series, so the only solution is to chain
> them. At this point, we're back to the problem of arrival time, and
> hence --delay becomes useful.

I do agree that --delay could exacerbate the spreading out of patches.

[PATCH RFC 1/6] Re: send-email: Add --delay for separating emails

[Nicolas Sebrecht]

On Thu, Apr 09, 2009 at 12:27:18PM -0500, Michael Witten wrote:
> 
> On Thu, Apr 9, 2009 at 11:17, Nicolas Sebrecht
> <nicolas.s-dev@laposte.net> wrote:
> >
> > The --delay option may have an undesirable side effect. In case of
> > non-chained emails, unrelated mails could be insterted between patches
> > where *all* MUA would be affected. It's not only true for very high
> > volume message mailing-lists (million monkeys receiving...). FMPOV, it's
> > worse than all display issues we already know or have with the current
> > behaviour.
> 
> But it's already impossible to protect against this scenario.

But we could forbid the use of --delay with non-chained emails, no? Or
at least, warn it should not be used.

-- 
Nicolas Sebrecht

Re: [PATCH RFC 1/6] Re: send-email: Add --delay for separating emails

[Michael Witten]

On Thu, Apr 9, 2009 at 12:45, Nicolas Sebrecht
<nicolas.s-dev@laposte.net> wrote:
> But we could forbid the use of --delay with non-chained emails, no? Or
> at least, warn it should not be used.

Ah! Good point! I'll add that.