[RFC/PATCH 2/2] git-send-email: Do not require that addresses added from body be valid

[Jakub Narebski <jnareb@gmail.com>]

On Wed, 4 May 2011, Jeff King wrote:
> On Wed, May 04, 2011 at 06:12:08PM +0200, Jakub Narebski wrote:
> > On Fri, 15 Apr 2011, Jeff King wrote:
> > >
> > > Sure. Since you are actually doing SMTP, you have much more flexibility
> > > in knowing what errors happen. Look in git-send-email.perl's
> > > send_message, around line 1118. We use the Mail::SMTP module, but we
> > > just feed it the whole recipient list and barf if any of them is
> > > rejected. You could probably remember which recipients are "important"
> > > (i.e., given on the command line) and which were pulled automatically
> > > from the commit information, and then feed each recipient individually.
> > > If important ones fail, abort the message. If an unimportant one fails,
> > > send the message anyway, but remember the bad address and report the
> > > error at the end.
> > [...]
> > This is an RFC patch preparing the way, so to speak, by remembering
> > where each Cc address came from.  We could in the future treat
> > $cc{'body'} / all_cc('body') differently from the rest of all_cc().
> > 
> > Is the approach taken here sane?
> 
> Yeah, from my cursory read, it looks like a good step forward, and I
> didn't see any obvious bugs.
> 
> You'll need still more refactoring in send_message to treat them
> differently at the SMTP level. We collapse all of the addresses down to
> a single list via unique_email_list (and we obviously want to keep this
> unique-ifying step), but that final list will have to remember where
> each address came from.

Below there is RFC patch that implements it by separating @cc and @cc_extra
and later @recipients and @recipients_extra.

How about it?

It does not warn about bad addresses from body, and there are no tests yet!

> > +sub all_cc {
> > +	my @keys = @_;
> > +	@keys = qw(initial from cc body cc-cmd) unless @keys;
> > +	return map { ref($_) ? @$_ : () } @cc{@keys};
> > +
> > +	#return map { ref($_) ? @$_ : () } values %cc;
> > +}
> 
> Nit: debugging cruft. :)

Right, I'll fix it in final version.

-- >8 ---------- >8 --
Subject: [PATCH] git-send-email: Do not require that addresses added from
 body be valid

Do not barf if any of recipients that was pulled automatically from
commit information is rejected.  This covers [currently] addresses
from 'Cc:' and 'Signed-off-by:' lines in message body.

This is possible only if we are using Net::SMTP or Net::SMTP::SSL
(it means that --smtp-server / sendemail.smtpserver is set to
internet address of outgoing SMTP server to use), because only then we
have control over which addresses are to be checked.

Currently no warnings that "unimportant" addresses were rejected are
shown; we should report errors at the end.

No test for this new behavior: perhaps we could adapt some test from
Net::SMTP module distribution...

Cc: Jeff King <peff@peff.net>
Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 git-send-email.perl |   40 +++++++++++++++++++++++++---------------
 1 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 7d75a1e..e758fd9 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -968,22 +968,32 @@ sub maildomain {
 sub send_message {
 	my @recipients = unique_email_list(@to);
 	my $to = join(",\n\t", @recipients);
-	my @cc =
-		grep {
-			my $cc = extract_valid_address($_);
-			not grep { $cc eq $_ || $_ =~ /<\Q${cc}\E>$/ } @recipients
-		}
-		map { sanitize_address($_) }
-		all_cc();
-	@recipients = unique_email_list(@recipients,@cc,@bcclist);
+	my $sanitize_cc = sub {
+		return
+			grep {
+				my $cc = extract_valid_address($_);
+				not grep { $cc eq $_ || $_ =~ /<\Q${cc}\E>$/ } @recipients
+			}
+			map { sanitize_address($_) }
+			all_cc(@_);
+	};
+	my @cc = $sanitize_cc->(qw(initial from cc cc-cmd));
+	my @cc_extra = $sanitize_cc->(qw(body));
+
+	my (%seen, @recipients_extra);
+	@recipients = unique_email_list(\%seen,@recipients,@cc,@bcclist);
 	@recipients = (map { extract_valid_address($_) } @recipients);
+	@recipients_extra =
+		map { extract_valid_address($_) }
+		unique_email_list(\%seen,@cc_extra);
+
 	my $date = format_2822_time($time++);
 	my $gitversion = '@@GIT_VERSION@@';
 	if ($gitversion =~ m/..GIT_VERSION../) {
 	    $gitversion = Git::version();
 	}
 
-	my $cc = join(",\n\t", unique_email_list(@cc));
+	my $cc = join(",\n\t", unique_email_list(@cc,@cc_extra));
 	my $ccline = "";
 	if ($cc ne '') {
 		$ccline = "\nCc: $cc";
@@ -1007,7 +1017,7 @@ X-Mailer: git-send-email $gitversion
 		$header .= join("\n", @xh) . "\n";
 	}
 
-	my @sendmail_parameters = ('-i', @recipients);
+	my @sendmail_parameters = ('-i', @recipients,@recipients_extra);
 	my $raw_from = $sanitized_sender;
 	if (defined $envelope_sender && $envelope_sender ne "auto") {
 		$raw_from = $envelope_sender;
@@ -1126,6 +1136,7 @@ X-Mailer: git-send-email $gitversion
 
 		$smtp->mail( $raw_from ) or die $smtp->message;
 		$smtp->to( @recipients ) or die $smtp->message;
+		$smtp->to( @recipients_extra, { Notify => ['NEVER'], SkipBad => 1 });
 		$smtp->data or die $smtp->message;
 		$smtp->datasend("$header\n$message") or die $smtp->message;
 		$smtp->dataend() or die $smtp->message;
@@ -1138,8 +1149,8 @@ X-Mailer: git-send-email $gitversion
 		if ($smtp_server !~ m#^/#) {
 			print "Server: $smtp_server\n";
 			print "MAIL FROM:<$raw_from>\n";
-			foreach my $entry (@recipients) {
-			    print "RCPT TO:<$entry>\n";
+			foreach my $entry (@recipients,@recipients_extra) {
+				print "RCPT TO:<$entry>\n";
 			}
 		} else {
 			print "Sendmail: $smtp_server ".join(' ',@sendmail_parameters)."\n";
@@ -1375,13 +1386,12 @@ sub cleanup_compose_files {
 $smtp->quit if $smtp;
 
 sub unique_email_list {
-	my %seen;
+	my $seen = ref $_[0] eq 'HASH' ? shift : {};
 	my @emails;
 
 	foreach my $entry (@_) {
 		if (my $clean = extract_valid_address($entry)) {
-			$seen{$clean} ||= 0;
-			next if $seen{$clean}++;
+			next if $seen->{$clean}++;
 			push @emails, $entry;
 		} else {
 			print STDERR "W: unable to extract a valid address",
-- 
1.7.5