[PATCH] send-email: handle multiple Cc addresses when reading mbox message

[PATCH] send-email: handle multiple Cc addresses when reading mbox message

[Jay Soffian]

From: Jay Soffian <jaysoffian@gmail.com>

When git format-patch is given multiple --cc arguments, it generates a
Cc header that looks like:

 Cc: first@example.com,
     second@example.com,
     third@example.com

Before this commit, send-email was unable to handle such a message.
First, it didn't know how to handle a header split across multiple
lines. Second, it couldn't handle multiple comma-separated addresses,
even if they were provided all on a single Cc: line.

This patch teaches it to unfold header lines by pre-processing the
header before extracting any of its fields. It also teaches it to split
the Cc line using Mail::Address if available, otherwise it just splits
on a simple \s*,\s* regular expression.

Also, conditionally use Mail::Address to handle the response to the "Who
should the emails be sent to?" prompt.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
This message which you are reading was generated with:
$ git format-patch --cc="Ryan Anderson <ryan@michonline.com>" \
  --cc=gitster@pobox.com -s -1

And sent with:
$ git send-email --from 'Jay Soffian <jaysoffian@gmail.com>' \
  --to git@vger.kernel.org  --cc 'Jay Soffian <jaysoffian@gmail.com>'

Before this patch, send-email would not have sent it properly. :-)

j.

 git-send-email.perl   |  150 +++++++++++++++++++++++++++---------------------
 t/t9001-send-email.sh |   22 +++++--
 2 files changed, 99 insertions(+), 73 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 77ca8fe..cde294c 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -126,6 +126,7 @@ sub format_2822_time {
 }
 
 my $have_email_valid = eval { require Email::Valid; 1 };
+my $have_mail_address = eval { require Mail::Address; 1 };
 my $smtp;
 my $auth;
 
@@ -360,6 +361,14 @@ foreach my $entry (@bcclist) {
 	die "Comma in --bcclist entry: $entry'\n" unless $entry !~ m/,/;
 }
 
+sub parse_address_line {
+	if ($have_mail_address) {
+		return map { $_->format } Mail::Address->parse($_[0]);
+	} else {
+		return split_addrs($_[0]);
+	}
+}
+
 sub split_addrs {
 	return quotewords('\s*,\s*', 1, @_);
 }
@@ -593,7 +602,7 @@ if (!@to) {
 	}
 
 	my $to = $_;
-	push @to, split_addrs($to);
+	push @to, parse_address_line($to);
 	$prompting++;
 }
 
@@ -920,88 +929,97 @@ foreach my $t (@files) {
 	@cc = @initial_cc;
 	@xh = ();
 	my $input_format = undef;
-	my $header_done = 0;
+	my @header = ();
 	$message = "";
+	# First unfold multiline header fields
 	while(<F>) {
-		if (!$header_done) {
-			if (/^From /) {
-				$input_format = 'mbox';
-				next;
+		last if /^\s*$/;
+		if (/^\s+\S/ and @header) {
+			chomp($header[$#header]);
+			s/^\s+/ /;
+			$header[$#header] .= $_;
+	    } else {
+			push(@header, $_);
+		}
+	}
+	# Now parse the header
+	foreach(@header) {
+		if (/^From /) {
+			$input_format = 'mbox';
+			next;
+		}
+		chomp;
+		if (!defined $input_format && /^[-A-Za-z]+:\s/) {
+			$input_format = 'mbox';
+		}
+
+		if (defined $input_format && $input_format eq 'mbox') {
+			if (/^Subject:\s+(.*)$/) {
+				$subject = $1;
 			}
-			chomp;
-			if (!defined $input_format && /^[-A-Za-z]+:\s/) {
-				$input_format = 'mbox';
+			elsif (/^From:\s+(.*)$/) {
+				($author, $author_encoding) = unquote_rfc2047($1);
+				next if ($suppress_cc{'author'});
+				printf("(mbox) Adding cc: %s from line '%s'\n",
+					$1, $_) unless $quiet;
+				push @cc, $1;
 			}
-
-			if (defined $input_format && $input_format eq 'mbox') {
-				if (/^Subject:\s+(.*)$/) {
-					$subject = $1;
-
-				} elsif (/^(Cc|From):\s+(.*)$/) {
-					if (unquote_rfc2047($2) eq $sender) {
+			elsif (/^Cc:\s+(.*)$/) {
+				foreach my $addr (parse_address_line($1)) {
+					if (unquote_rfc2047($addr) eq $sender) {
 						next if ($suppress_cc{'self'});
-					}
-					elsif ($1 eq 'From') {
-						($author, $author_encoding)
-						  = unquote_rfc2047($2);
-						next if ($suppress_cc{'author'});
 					} else {
 						next if ($suppress_cc{'cc'});
 					}
 					printf("(mbox) Adding cc: %s from line '%s'\n",
-						$2, $_) unless $quiet;
-					push @cc, $2;
+						$addr, $_) unless $quiet;
+					push @cc, $addr;
 				}
-				elsif (/^Content-type:/i) {
-					$has_content_type = 1;
-					if (/charset="?([^ "]+)/) {
-						$body_encoding = $1;
-					}
-					push @xh, $_;
-				}
-				elsif (/^Message-Id: (.*)/i) {
-					$message_id = $1;
-				}
-				elsif (!/^Date:\s/ && /^[-A-Za-z]+:\s+\S/) {
-					push @xh, $_;
-				}
-
-			} else {
-				# In the traditional
-				# "send lots of email" format,
-				# line 1 = cc
-				# line 2 = subject
-				# So let's support that, too.
-				$input_format = 'lots';
-				if (@cc == 0 && !$suppress_cc{'cc'}) {
-					printf("(non-mbox) Adding cc: %s from line '%s'\n",
-						$_, $_) unless $quiet;
-
-					push @cc, $_;
-
-				} elsif (!defined $subject) {
-					$subject = $_;
+			}
+			elsif (/^Content-type:/i) {
+				$has_content_type = 1;
+				if (/charset="?([^ "]+)/) {
+					$body_encoding = $1;
 				}
+				push @xh, $_;
 			}
-
-			# A whitespace line will terminate the headers
-			if (m/^\s*$/) {
-				$header_done = 1;
+			elsif (/^Message-Id: (.*)/i) {
+				$message_id = $1;
 			}
+			elsif (!/^Date:\s/ && /^[-A-Za-z]+:\s+\S/) {
+				push @xh, $_;
+			}
+
 		} else {
-			$message .=  $_;
-			if (/^(Signed-off-by|Cc): (.*)$/i) {
-				next if ($suppress_cc{'sob'});
-				chomp;
-				my $c = $2;
-				chomp $c;
-				next if ($c eq $sender and $suppress_cc{'self'});
-				push @cc, $c;
-				printf("(sob) Adding cc: %s from line '%s'\n",
-					$c, $_) unless $quiet;
+			# In the traditional
+			# "send lots of email" format,
+			# line 1 = cc
+			# line 2 = subject
+			# So let's support that, too.
+			$input_format = 'lots';
+			if (@cc == 0 && !$suppress_cc{'cc'}) {
+				printf("(non-mbox) Adding cc: %s from line '%s'\n",
+					$_, $_) unless $quiet;
+				push @cc, $_;
+			} elsif (!defined $subject) {
+				$subject = $_;
 			}
 		}
 	}
+	# Now parse the message body
+	while(<F>) {
+		$message .=  $_;
+		if (/^(Signed-off-by|Cc): (.*)$/i) {
+			next if ($suppress_cc{'sob'});
+			chomp;
+			my $c = $2;
+			chomp $c;
+			next if ($c eq $sender and $suppress_cc{'self'});
+			push @cc, $c;
+			printf("(sob) Adding cc: %s from line '%s'\n",
+				$c, $_) unless $quiet;
+		}
+	}
 	close F;
 
 	if (defined $cc_cmd && !$suppress_cc{'cccmd'}) {
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index cb3d183..da54835 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -32,7 +32,7 @@ clean_fake_sendmail() {
 }
 
 test_expect_success 'Extract patches' '
-    patches=`git format-patch -n HEAD^1`
+    patches=`git format-patch --cc="One <one@example.com>" --cc=two@example.com -n HEAD^1`
 '
 
 test_expect_success 'Send patches' '
@@ -42,6 +42,8 @@ test_expect_success 'Send patches' '
 cat >expected <<\EOF
 !nobody@example.com!
 !author@example.com!
+!one@example.com!
+!two@example.com!
 EOF
 test_expect_success \
     'Verify commandline' \
@@ -50,13 +52,15 @@ test_expect_success \
 cat >expected-show-all-headers <<\EOF
 0001-Second.patch
 (mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
+(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
+(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
 Dry-OK. Log says:
 Server: relay.example.com
 MAIL FROM:<from@example.com>
-RCPT TO:<to@example.com>,<cc@example.com>,<author@example.com>,<bcc@example.com>
+RCPT TO:<to@example.com>,<cc@example.com>,<author@example.com>,<one@example.com>,<two@example.com>,<bcc@example.com>
 From: Example <from@example.com>
 To: to@example.com
-Cc: cc@example.com, A <author@example.com>
+Cc: cc@example.com, A <author@example.com>, One <one@example.com>, two@example.com
 Subject: [PATCH 1/1] Second.
 Date: DATE-STRING
 Message-Id: MESSAGE-ID-STRING
@@ -170,13 +174,15 @@ test_expect_success 'second message is patch' '
 cat >expected-show-all-headers <<\EOF
 0001-Second.patch
 (mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
+(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
+(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
 Dry-OK. Log says:
 Server: relay.example.com
 MAIL FROM:<from@example.com>
-RCPT TO:<to@example.com>,<cc@example.com>,<author@example.com>
+RCPT TO:<to@example.com>,<cc@example.com>,<author@example.com>,<one@example.com>,<two@example.com>
 From: Example <from@example.com>
 To: to@example.com
-Cc: cc@example.com, A <author@example.com>
+Cc: cc@example.com, A <author@example.com>, One <one@example.com>, two@example.com
 Subject: [PATCH 1/1] Second.
 Date: DATE-STRING
 Message-Id: MESSAGE-ID-STRING
@@ -203,13 +209,15 @@ test_expect_success 'sendemail.cc set' '
 cat >expected-show-all-headers <<\EOF
 0001-Second.patch
 (mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
+(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
+(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
 Dry-OK. Log says:
 Server: relay.example.com
 MAIL FROM:<from@example.com>
-RCPT TO:<to@example.com>,<author@example.com>
+RCPT TO:<to@example.com>,<author@example.com>,<one@example.com>,<two@example.com>
 From: Example <from@example.com>
 To: to@example.com
-Cc: A <author@example.com>
+Cc: A <author@example.com>, One <one@example.com>, two@example.com
 Subject: [PATCH 1/1] Second.
 Date: DATE-STRING
 Message-Id: MESSAGE-ID-STRING
-- 
1.6.2.rc0.67.g77afc

Re: [PATCH] send-email: handle multiple Cc addresses when reading mbox message

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 567 bytes --]

Jay Soffian wrote:
> -			if (/^(Signed-off-by|Cc): (.*)$/i) {
> -				next if ($suppress_cc{'sob'});
[...]
> +		if (/^(Signed-off-by|Cc): (.*)$/i) {
> +			next if ($suppress_cc{'sob'});

Doesn't this actually look like a long-standing send-email bug?  Since
6564828 (git-send-email: Generalize auto-cc recipient mechanism.,
2007-12-25) they should go in separate categories, but the above lines
were just translated from the old $signed_off_cc setting.  It seems
they should distinguish between SOB and Cc.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] send-email: handle multiple Cc addresses when reading mbox message

[Jay Soffian]

On Fri, Feb 13, 2009 at 6:32 PM, Thomas Rast <trast@student.ethz.ch> wrote:
> Jay Soffian wrote:
>> -                     if (/^(Signed-off-by|Cc): (.*)$/i) {
>> -                             next if ($suppress_cc{'sob'});
> [...]
>> +             if (/^(Signed-off-by|Cc): (.*)$/i) {
>> +                     next if ($suppress_cc{'sob'});
>
> Doesn't this actually look like a long-standing send-email bug?  Since
> 6564828 (git-send-email: Generalize auto-cc recipient mechanism.,
> 2007-12-25) they should go in separate categories, but the above lines
> were just translated from the old $signed_off_cc setting.  It seems
> they should distinguish between SOB and Cc.

I think it would be nice if send-email had different settings for
extracting Cc lines and extracting SOB lines from the message body,
but I don't call that a bug, but rather an enhancement request.

And yes, I noticed it when I went to send this patch because
originally I had "Cc: ..." in the commit message on column 0, which
send-email annoyingly picked up. I just changed my commit message to
move the "Cc:" to column 1.

Nonetheless, it is independent of this patch.

j.

Re: [PATCH] send-email: handle multiple Cc addresses when reading mbox message

[Jay Soffian]

On Fri, Feb 13, 2009 at 6:39 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
> I think it would be nice if send-email had different settings for
> extracting Cc lines and extracting SOB lines from the message body,
> but I don't call that a bug, but rather an enhancement request.

Oh, I see it does now that I looked at that commit. Hmphf. I'll fix
that on-top of the patch I just sent.

j.

Re: [PATCH] send-email: handle multiple Cc addresses when reading mbox message

[Jay Soffian]

From: Jay Soffian <jaysoffian@gmail.com>

On Fri, Feb 13, 2009 at 6:32 PM, Thomas Rast <trast@student.ethz.ch> wrote:
> Jay Soffian wrote:
>> -                     if (/^(Signed-off-by|Cc): (.*)$/i) {
>> -                             next if ($suppress_cc{'sob'});
> [...]
>> +             if (/^(Signed-off-by|Cc): (.*)$/i) {
>> +                     next if ($suppress_cc{'sob'});
>
> Doesn't this actually look like a long-standing send-email bug?  Since
> 6564828 (git-send-email: Generalize auto-cc recipient mechanism.,
> 2007-12-25) they should go in separate categories, but the above lines
> were just translated from the old $signed_off_cc setting.  It seems
> they should distinguish between SOB and Cc.

This is fixed by the last patch in this series. While I was working on
it I noticed two other minor issues, which is the first two patches.

This is meant to go on top of
http://thread.gmane.org/gmane.comp.version-control.git/109783

Jay Soffian (3):
  send-email: correct logic error with --suppress-cc=cc
  send-email: don't call unquote_rfc2047 unnecessarily
  send-email: --suppress-cc improvements

 Documentation/git-send-email.txt |   14 ++++++----
 git-send-email.perl              |   50 ++++++++++++++++++++++----------------
 t/t9001-send-email.sh            |   38 +++++++++++++++++++++++++++-
 3 files changed, 73 insertions(+), 29 deletions(-)

Re: [PATCH] send-email: handle multiple Cc addresses when reading mbox message

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 1199 bytes --]

Jay Soffian wrote:
> From: Jay Soffian <jaysoffian@gmail.com>
> 
> On Fri, Feb 13, 2009 at 6:32 PM, Thomas Rast <trast@student.ethz.ch> wrote:
> > Jay Soffian wrote:
> >> -                     if (/^(Signed-off-by|Cc): (.*)$/i) {
> >> -                             next if ($suppress_cc{'sob'});
> > [...]
> >> +             if (/^(Signed-off-by|Cc): (.*)$/i) {
> >> +                     next if ($suppress_cc{'sob'});
> >
> > Doesn't this actually look like a long-standing send-email bug?  Since
> > 6564828 (git-send-email: Generalize auto-cc recipient mechanism.,
> > 2007-12-25) they should go in separate categories, but the above lines
> > were just translated from the old $signed_off_cc setting.  It seems
> > they should distinguish between SOB and Cc.
> 
> This is fixed by the last patch in this series. While I was working on
> it I noticed two other minor issues, which is the first two patches.

Thanks for the fix, and sorry if this was the comment that inspired
your venting earlier.  I should've tried a patch myself, but it was
already past midnight.  I'll send a slightly improved version of 3/3
shortly.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 1/3] send-email: correct logic error with --suppress-cc=cc

[Jay Soffian]

From: Jay Soffian <jaysoffian@gmail.com>

--suppress-cc=cc is supposed to suppress harvesting addresses from any
Cc: lines. However, in the case where the Cc: line contained the sender,
it would only suppress if --suppress-cc=self.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 git-send-email.perl |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index cde294c..a2e0b94 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -965,12 +965,10 @@ foreach my $t (@files) {
 				push @cc, $1;
 			}
 			elsif (/^Cc:\s+(.*)$/) {
+				next if $suppress_cc{'cc'};
 				foreach my $addr (parse_address_line($1)) {
-					if (unquote_rfc2047($addr) eq $sender) {
-						next if ($suppress_cc{'self'});
-					} else {
-						next if ($suppress_cc{'cc'});
-					}
+					next if $suppress_cc{'self'} and
+						unquote_rfc2047($addr) eq $sender;
 					printf("(mbox) Adding cc: %s from line '%s'\n",
 						$addr, $_) unless $quiet;
 					push @cc, $addr;
-- 
1.6.2.rc0.235.g1319

[PATCH 1/3 v2] send-email: correct logic error with --suppress-cc=cc

[Jay Soffian]

--suppress-cc=cc is supposed to suppress harvesting addresses from any
Cc: lines. However, in the case where the Cc: line contained the sender,
it would only suppress if --suppress-cc=self.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
And here's a re-roll of this one. Sheesh, that logic was too subtle for
my brain. I think it's a lot more obvious what is going on after this
patch.

I apologize that this thread is getting so dense. Here it is on gmane
which may help:

  http://thread.gmane.org/gmane.comp.version-control.git/109783

j.

 git-send-email.perl |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index cde294c..cef32da 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -959,18 +959,17 @@ foreach my $t (@files) {
 			}
 			elsif (/^From:\s+(.*)$/) {
 				($author, $author_encoding) = unquote_rfc2047($1);
-				next if ($suppress_cc{'author'});
+				next if $suppress_cc{'author'};
+				next if $suppress_cc{'self'} and $author eq $sender;
 				printf("(mbox) Adding cc: %s from line '%s'\n",
 					$1, $_) unless $quiet;
 				push @cc, $1;
 			}
 			elsif (/^Cc:\s+(.*)$/) {
+				next if $suppress_cc{'cc'};
 				foreach my $addr (parse_address_line($1)) {
-					if (unquote_rfc2047($addr) eq $sender) {
-						next if ($suppress_cc{'self'});
-					} else {
-						next if ($suppress_cc{'cc'});
-					}
+					next if $suppress_cc{'self'} and
+						unquote_rfc2047($addr) eq $sender;
 					printf("(mbox) Adding cc: %s from line '%s'\n",
 						$addr, $_) unless $quiet;
 					push @cc, $addr;
@@ -1038,7 +1037,7 @@ foreach my $t (@files) {
 			or die "(cc-cmd) failed to close pipe to '$cc_cmd'";
 	}
 
-	if (defined $author) {
+	if (defined $author and $author ne $sender) {
 		$message = "From: $author\n\n$message";
 		if (defined $author_encoding) {
 			if ($has_content_type) {
-- 
1.6.2.rc0.238.g0c1fe

Re: [PATCH 1/3 v2] send-email: correct logic error with --suppress-cc=cc

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> --suppress-cc=cc is supposed to suppress harvesting addresses from any
> Cc: lines. However, in the case where the Cc: line contained the sender,
> it would only suppress if --suppress-cc=self.

Perhaps I am personally slightly in favor of your interpretation, but the
current behaviour makes sort of sense, too.  To everybody, himself is
always special (and that is why we have 'self'); I do not think it is
entirely unreasonable if --suppress-cc=cc meant remove people from Cc list
but I am special.

> And here's a re-roll of this one. Sheesh, that logic was too subtle for
> my brain.

Perhaps it is easier to clarify the goal by updating the tests to define
what needs to happen first?  Working from tests often makes the end result
much more robust than randomly modifying the existing code here and there
until it happens to start doing something you think it should do.

Re: [PATCH 1/3 v2] send-email: correct logic error with --suppress-cc=cc

[Jay Soffian]

On Sat, Feb 14, 2009 at 1:52 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Perhaps it is easier to clarify the goal by updating the tests to define
> what needs to happen first?  Working from tests often makes the end result
> much more robust than randomly modifying the existing code here and there
> until it happens to start doing something you think it should do.

In this case the problem is that the existing code had no tests, so I wasn't
quite sure what it was supposed to do, and I divined the wrong thing from it.

But now that I understand what it should do, I'll add tests.

Who let the original code in w/o tests, hmm? :-)

j.

Re: [PATCH 1/3 v2] send-email: correct logic error with --suppress-cc=cc

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> Who let the original code in w/o tests, hmm? :-)

Reviewers and list participants, including you.

[PATCH 2/3] send-email: don't call unquote_rfc2047 unnecessarily

[Jay Soffian]

From: Jay Soffian <jaysoffian@gmail.com>

If --suppress-cc=author then there is no need to unquote the From:
address.

Signed-off-by: Jay Soffian <jaysoffian@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 a2e0b94..2a3e3e8 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -958,8 +958,8 @@ foreach my $t (@files) {
 				$subject = $1;
 			}
 			elsif (/^From:\s+(.*)$/) {
+				next if $suppress_cc{'author'};
 				($author, $author_encoding) = unquote_rfc2047($1);
-				next if ($suppress_cc{'author'});
 				printf("(mbox) Adding cc: %s from line '%s'\n",
 					$1, $_) unless $quiet;
 				push @cc, $1;
-- 
1.6.2.rc0.235.g1319

Re: [PATCH 2/3] send-email: don't call unquote_rfc2047 unnecessarily

[Jay Soffian]

On Fri, Feb 13, 2009 at 10:51 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
> From: Jay Soffian <jaysoffian@gmail.com>
>
> If --suppress-cc=author then there is no need to unquote the From:
> address.
>
> Signed-off-by: Jay Soffian <jaysoffian@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 a2e0b94..2a3e3e8 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -958,8 +958,8 @@ foreach my $t (@files) {
>                                $subject = $1;
>                        }
>                        elsif (/^From:\s+(.*)$/) {
> +                               next if $suppress_cc{'author'};
>                                ($author, $author_encoding) = unquote_rfc2047($1);
> -                               next if ($suppress_cc{'author'});
>                                printf("(mbox) Adding cc: %s from line '%s'\n",
>                                        $1, $_) unless $quiet;
>                                push @cc, $1;

Crap. Ignore this one from the series. $author is used later and we
have to parse it regardless. This is brittle code, but I should've
realize it.

j.

[PATCH 3/3] send-email: --suppress-cc improvements

[Jay Soffian]

From: Jay Soffian <jaysoffian@gmail.com>

Commit 656482830ddc4a4e2af132fabb118a25190439c2 added the --suppress-cc
option. However, it made --suppress-cc=sob suppress both SOB lines and
body Cc lines (but not header Cc lines), which seems contrary to how
it is named.

After this commit, 'sob' suppresses only SOB lines and --suppress-cc
takes two additional values:

 * 'body' suppresses both SOB and body Cc lines (i.e. what 'sob'
    used to do).

 * 'bodycc' suppresses body Cc lines, but not header Cc lines.

For backwards compatibility, --no-signed-off-by-cc, acts like 'body'.

Also update the documentation and add a few tests.

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

Okay, so --suppress=cc=sob now acts differently. I can hear the
backwards compatibility objection. I disagree. It will only effect a
user who does --suppress-cc=sob AND sends out a patch that has Cc: in
the message body (commit message). And If I'm not the first user to have
sent such a message earlier today, I'll bet I'm not much more than the
10th. 

OTOH, --suppress-cc=sob was obviously misnamed in the first place, and I
was confused by having send-email pick up the Cc in the body.

So I think it's worth the small risk of changing what --suppress-cc=sob
does. And as a benefit, we now offer two new values for --suppress-cc,
and for really-old-timers that are probably using --no-signed-of-by-cc,
that says the same.

j.

 Documentation/git-send-email.txt |   14 +++++++-----
 git-send-email.perl              |   40 +++++++++++++++++++++++--------------
 t/t9001-send-email.sh            |   38 ++++++++++++++++++++++++++++++++++-
 3 files changed, 69 insertions(+), 23 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index ff4aeff..d6af035 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -166,12 +166,14 @@ Automating
 	Specify an additional category of recipients to suppress the
 	auto-cc of.  'self' will avoid including the sender, 'author' will
 	avoid including the patch author, 'cc' will avoid including anyone
-	mentioned in Cc lines in the patch, 'sob' will avoid including
-	anyone mentioned in Signed-off-by lines, and 'cccmd' will avoid
-	running the --cc-cmd.  'all' will suppress all auto cc values.
-	Default is the value of 'sendemail.suppresscc' configuration value;
-	if that is unspecified, default to 'self' if --suppress-from is
-	specified, as well as 'sob' if --no-signed-off-cc is specified.
+	mentioned in Cc lines in the patch header, 'ccbody' will avoid
+	including anyone mentioned in Cc lines in the patch body (commit
+	message), 'sob' will avoid including anyone mentioned in Signed-off-by
+	lines, and 'cccmd' will avoid running the --cc-cmd. 'body' is
+	equivalent to 'sob' + 'ccbody'. 'all' will suppress all auto cc
+	values.  Default is the value of 'sendemail.suppresscc' configuration
+	value; if that is unspecified, default to 'self' if --suppress-from is
+	specified, as well as 'body' if --no-signed-off-cc is specified.
 
 --[no-]suppress-from::
 	If this is set, do not add the From: address to the cc: list.
diff --git a/git-send-email.perl b/git-send-email.perl
index 2a3e3e8..2282287 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -68,7 +68,7 @@ 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, cccmd, all.
+    --suppress-cc           <str>  * author, self, sob, cc, cccmd, body, bodycc, all.
     --[no-]signed-off-by-cc        * Send to Cc: and Signed-off-by:
                                      addresses. Default on.
     --[no-]suppress-from           * Send to self. Default off.
@@ -319,21 +319,28 @@ 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)$/;
+			unless $entry =~ /^(all|cccmd|cc|author|self|sob|body|bodycc)$/;
 		$suppress_cc{$entry} = 1;
 	}
 }
 
 if ($suppress_cc{'all'}) {
-	foreach my $entry (qw (ccmd cc author self sob)) {
+	foreach my $entry (qw (ccmd cc author self sob body bodycc)) {
 		$suppress_cc{$entry} = 1;
 	}
 	delete $suppress_cc{'all'};
 }
 
+if ($suppress_cc{'sob'} && $suppress_cc{'bodycc'}) {
+	$suppress_cc{'body'} = 1;
+}
+
 # If explicit old-style ones are specified, they trump --suppress-cc.
 $suppress_cc{'self'} = $suppress_from if defined $suppress_from;
-$suppress_cc{'sob'} = !$signed_off_by_cc if defined $signed_off_by_cc;
+# For backwards compatibility, old-style --signed-off-by-cc suppresses
+# SOB and body Cc lines, whereas --supress-cc=sob suppresses just the SOB
+# line, but not the body Cc.
+$suppress_cc{'body'} = !$signed_off_by_cc if defined $signed_off_by_cc;
 
 # Debugging, print out the suppressions.
 if (0) {
@@ -1005,17 +1012,20 @@ foreach my $t (@files) {
 		}
 	}
 	# Now parse the message body
-	while(<F>) {
-		$message .=  $_;
-		if (/^(Signed-off-by|Cc): (.*)$/i) {
-			next if ($suppress_cc{'sob'});
-			chomp;
-			my $c = $2;
-			chomp $c;
-			next if ($c eq $sender and $suppress_cc{'self'});
-			push @cc, $c;
-			printf("(sob) Adding cc: %s from line '%s'\n",
-				$c, $_) unless $quiet;
+	unless ($suppress_cc{'body'}) {
+		while(<F>) {
+			$message .=  $_;
+			if (/^(Signed-off-by|Cc): (.*)$/i) {
+				chomp;
+				my ($what, $c) = ($1, $2);
+				chomp $c;
+				next if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i;
+				next if $suppress_cc{'bodycc'} and $what =~ /Cc/i;
+				next if ($c eq $sender and $suppress_cc{'self'});
+				push @cc, $c;
+				printf("(body) Adding cc: %s from line '%s'\n",
+					$c, $_) unless $quiet;
+			}
 		}
 	}
 	close F;
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index da54835..d7766f9 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -32,11 +32,11 @@ clean_fake_sendmail() {
 }
 
 test_expect_success 'Extract patches' '
-    patches=`git format-patch --cc="One <one@example.com>" --cc=two@example.com -n HEAD^1`
+    patches=`git format-patch -s --cc="One <one@example.com>" --cc=two@example.com -n HEAD^1`
 '
 
 test_expect_success 'Send patches' '
-     git send-email --from="Example <nobody@example.com>" --to=nobody@example.com --smtp-server="$(pwd)/fake.sendmail" $patches 2>errors
+     git send-email --suppress-cc=sob --from="Example <nobody@example.com>" --to=nobody@example.com --smtp-server="$(pwd)/fake.sendmail" $patches 2>errors
 '
 
 cat >expected <<\EOF
@@ -74,6 +74,7 @@ EOF
 test_expect_success 'Show all headers' '
 	git send-email \
 		--dry-run \
+		--suppress-cc=sob \
 		--from="Example <from@example.com>" \
 		--to=to@example.com \
 		--cc=cc@example.com \
@@ -195,6 +196,7 @@ test_expect_success 'sendemail.cc set' '
 	git config sendemail.cc cc@example.com &&
 	git send-email \
 		--dry-run \
+		--suppress-cc=sob \
 		--from="Example <from@example.com>" \
 		--to=to@example.com \
 		--smtp-server relay.example.com \
@@ -230,6 +232,38 @@ test_expect_success 'sendemail.cc unset' '
 	git config --unset sendemail.cc &&
 	git send-email \
 		--dry-run \
+		--suppress-cc=sob \
+		--from="Example <from@example.com>" \
+		--to=to@example.com \
+		--smtp-server relay.example.com \
+		$patches |
+	sed	-e "s/^\(Date:\).*/\1 DATE-STRING/" \
+		-e "s/^\(Message-Id:\).*/\1 MESSAGE-ID-STRING/" \
+		-e "s/^\(X-Mailer:\).*/\1 X-MAILER-STRING/" \
+		>actual-show-all-headers &&
+	test_cmp expected-show-all-headers actual-show-all-headers
+'
+
+cat >expected-show-all-headers <<\EOF
+0001-Second.patch
+Dry-OK. Log says:
+Server: relay.example.com
+MAIL FROM:<from@example.com>
+RCPT TO:<to@example.com>
+From: Example <from@example.com>
+To: to@example.com
+Subject: [PATCH 1/1] Second.
+Date: DATE-STRING
+Message-Id: MESSAGE-ID-STRING
+X-Mailer: X-MAILER-STRING
+
+Result: OK
+EOF
+
+test_expect_success '--suppress-cc=all' '
+	git send-email \
+		--dry-run \
+		--suppress-cc=all \
 		--from="Example <from@example.com>" \
 		--to=to@example.com \
 		--smtp-server relay.example.com \
-- 
1.6.2.rc0.235.g1319

[PATCH 3/3 v2] send-email: --suppress-cc improvements

[Jay Soffian]

From: Jay Soffian <jaysoffian@gmail.com>

Commit 656482830ddc4a4e2af132fabb118a25190439c2 added the --suppress-cc
option. However, it made --suppress-cc=sob suppress both SOB lines and
body Cc lines (but not header Cc lines), which seems contrary to how
it is named.

After this commit, 'sob' suppresses only SOB lines and --suppress-cc
takes two additional values:

 * 'body' suppresses both SOB and body Cc lines (i.e. what 'sob'
    used to do).

 * 'bodycc' suppresses body Cc lines, but not header Cc lines.

For backwards compatibility, --no-signed-off-by-cc, acts like 'body'.

Also update the documentation and add a few tests.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
Sorry, please ignore the previous 3/3, it had an obvious breakage;
--suppress-cc=body was causing then entire message body not to be read
in at all.

 Documentation/git-send-email.txt |   14 ++++++++------
 git-send-email.perl              |   23 ++++++++++++++++-------
 t/t9001-send-email.sh            |   38 ++++++++++++++++++++++++++++++++++++--
 3 files changed, 60 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index ff4aeff..d6af035 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -166,12 +166,14 @@ Automating
 	Specify an additional category of recipients to suppress the
 	auto-cc of.  'self' will avoid including the sender, 'author' will
 	avoid including the patch author, 'cc' will avoid including anyone
-	mentioned in Cc lines in the patch, 'sob' will avoid including
-	anyone mentioned in Signed-off-by lines, and 'cccmd' will avoid
-	running the --cc-cmd.  'all' will suppress all auto cc values.
-	Default is the value of 'sendemail.suppresscc' configuration value;
-	if that is unspecified, default to 'self' if --suppress-from is
-	specified, as well as 'sob' if --no-signed-off-cc is specified.
+	mentioned in Cc lines in the patch header, 'ccbody' will avoid
+	including anyone mentioned in Cc lines in the patch body (commit
+	message), 'sob' will avoid including anyone mentioned in Signed-off-by
+	lines, and 'cccmd' will avoid running the --cc-cmd. 'body' is
+	equivalent to 'sob' + 'ccbody'. 'all' will suppress all auto cc
+	values.  Default is the value of 'sendemail.suppresscc' configuration
+	value; if that is unspecified, default to 'self' if --suppress-from is
+	specified, as well as 'body' if --no-signed-off-cc is specified.
 
 --[no-]suppress-from::
 	If this is set, do not add the From: address to the cc: list.
diff --git a/git-send-email.perl b/git-send-email.perl
index 2a3e3e8..23a55e2 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -68,7 +68,7 @@ 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, cccmd, all.
+    --suppress-cc           <str>  * author, self, sob, cc, cccmd, body, bodycc, all.
     --[no-]signed-off-by-cc        * Send to Cc: and Signed-off-by:
                                      addresses. Default on.
     --[no-]suppress-from           * Send to self. Default off.
@@ -319,21 +319,28 @@ 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)$/;
+			unless $entry =~ /^(all|cccmd|cc|author|self|sob|body|bodycc)$/;
 		$suppress_cc{$entry} = 1;
 	}
 }
 
 if ($suppress_cc{'all'}) {
-	foreach my $entry (qw (ccmd cc author self sob)) {
+	foreach my $entry (qw (ccmd cc author self sob body bodycc)) {
 		$suppress_cc{$entry} = 1;
 	}
 	delete $suppress_cc{'all'};
 }
 
+if ($suppress_cc{'sob'} && $suppress_cc{'bodycc'}) {
+	$suppress_cc{'body'} = 1;
+}
+
 # If explicit old-style ones are specified, they trump --suppress-cc.
 $suppress_cc{'self'} = $suppress_from if defined $suppress_from;
-$suppress_cc{'sob'} = !$signed_off_by_cc if defined $signed_off_by_cc;
+# For backwards compatibility, old-style --signed-off-by-cc suppresses
+# SOB and body Cc lines, whereas --supress-cc=sob suppresses just the SOB
+# line, but not the body Cc.
+$suppress_cc{'body'} = !$signed_off_by_cc if defined $signed_off_by_cc;
 
 # Debugging, print out the suppressions.
 if (0) {
@@ -1007,14 +1014,16 @@ foreach my $t (@files) {
 	# Now parse the message body
 	while(<F>) {
 		$message .=  $_;
+		next if $suppress_cc{'body'};
 		if (/^(Signed-off-by|Cc): (.*)$/i) {
-			next if ($suppress_cc{'sob'});
 			chomp;
-			my $c = $2;
+			my ($what, $c) = ($1, $2);
 			chomp $c;
+			next if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i;
+			next if $suppress_cc{'bodycc'} and $what =~ /Cc/i;
 			next if ($c eq $sender and $suppress_cc{'self'});
 			push @cc, $c;
-			printf("(sob) Adding cc: %s from line '%s'\n",
+			printf("(body) Adding cc: %s from line '%s'\n",
 				$c, $_) unless $quiet;
 		}
 	}
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index da54835..d7766f9 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -32,11 +32,11 @@ clean_fake_sendmail() {
 }
 
 test_expect_success 'Extract patches' '
-    patches=`git format-patch --cc="One <one@example.com>" --cc=two@example.com -n HEAD^1`
+    patches=`git format-patch -s --cc="One <one@example.com>" --cc=two@example.com -n HEAD^1`
 '
 
 test_expect_success 'Send patches' '
-     git send-email --from="Example <nobody@example.com>" --to=nobody@example.com --smtp-server="$(pwd)/fake.sendmail" $patches 2>errors
+     git send-email --suppress-cc=sob --from="Example <nobody@example.com>" --to=nobody@example.com --smtp-server="$(pwd)/fake.sendmail" $patches 2>errors
 '
 
 cat >expected <<\EOF
@@ -74,6 +74,7 @@ EOF
 test_expect_success 'Show all headers' '
 	git send-email \
 		--dry-run \
+		--suppress-cc=sob \
 		--from="Example <from@example.com>" \
 		--to=to@example.com \
 		--cc=cc@example.com \
@@ -195,6 +196,7 @@ test_expect_success 'sendemail.cc set' '
 	git config sendemail.cc cc@example.com &&
 	git send-email \
 		--dry-run \
+		--suppress-cc=sob \
 		--from="Example <from@example.com>" \
 		--to=to@example.com \
 		--smtp-server relay.example.com \
@@ -230,6 +232,38 @@ test_expect_success 'sendemail.cc unset' '
 	git config --unset sendemail.cc &&
 	git send-email \
 		--dry-run \
+		--suppress-cc=sob \
+		--from="Example <from@example.com>" \
+		--to=to@example.com \
+		--smtp-server relay.example.com \
+		$patches |
+	sed	-e "s/^\(Date:\).*/\1 DATE-STRING/" \
+		-e "s/^\(Message-Id:\).*/\1 MESSAGE-ID-STRING/" \
+		-e "s/^\(X-Mailer:\).*/\1 X-MAILER-STRING/" \
+		>actual-show-all-headers &&
+	test_cmp expected-show-all-headers actual-show-all-headers
+'
+
+cat >expected-show-all-headers <<\EOF
+0001-Second.patch
+Dry-OK. Log says:
+Server: relay.example.com
+MAIL FROM:<from@example.com>
+RCPT TO:<to@example.com>
+From: Example <from@example.com>
+To: to@example.com
+Subject: [PATCH 1/1] Second.
+Date: DATE-STRING
+Message-Id: MESSAGE-ID-STRING
+X-Mailer: X-MAILER-STRING
+
+Result: OK
+EOF
+
+test_expect_success '--suppress-cc=all' '
+	git send-email \
+		--dry-run \
+		--suppress-cc=all \
 		--from="Example <from@example.com>" \
 		--to=to@example.com \
 		--smtp-server relay.example.com \
-- 
1.6.2.rc0.238.g0c1fe

Re: [PATCH 3/3 v2] send-email: --suppress-cc improvements

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> From: Jay Soffian <jaysoffian@gmail.com>
>
> Commit 656482830ddc4a4e2af132fabb118a25190439c2 added the --suppress-cc
> option. However, it made --suppress-cc=sob suppress both SOB lines and
> body Cc lines (but not header Cc lines), which seems contrary to how
> it is named.
>
> After this commit, 'sob' suppresses only SOB lines and --suppress-cc
> takes two additional values:

I know it's just wording, but it always makes my skin tingle when I see
"Before this commit, blah, after this commit bah."  Maybe it's just me?
The logic flows more naturally if you say "X does Y.  It is not good for
such and such reasons.  This patch changes it to do Z.", at least to me.

>  * 'body' suppresses both SOB and body Cc lines (i.e. what 'sob'
>     used to do).
>
>  * 'bodycc' suppresses body Cc lines, but not header Cc lines.
>
> For backwards compatibility, --no-signed-off-by-cc, acts like 'body'.

I had to read this sentence three time.  Giving --no-signed-off-by-cc is
the same as giving --suppress-cc=body, meaning it suppresses both S-o-b
and in-body Cc addresses?

It looks very weird to say "For backwards compatibility," immediately
after you said "screw backward compatibility" by changing the meaning of
"sob" without even justifying why it is a good idea in the commit log
message.

You are changing the semantics of "sob" because you believe the new
definition is much saner (and I happen to agree with you but that is
besides the point).  When the user says --no-signed-off-by-cc, the user
asks not to Cc: people who have sign-offs in the log message; I'd say it
would be more natural to screw the backward compatibility here as well and
give it a saner and more natural semantics of suppressing S-o-b but not
in-body Cc.  No?

> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index ff4aeff..d6af035 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -166,12 +166,14 @@ Automating
>  	Specify an additional category of recipients to suppress the
>  	auto-cc of.  'self' will avoid including the sender, 'author' will
>  	avoid including the patch author, 'cc' will avoid including anyone
> +	mentioned in Cc lines in the patch header, 'ccbody' will avoid
> +	including anyone mentioned in Cc lines in the patch body (commit
> +	message), 'sob' will avoid including anyone mentioned in Signed-off-by
> +	lines, and 'cccmd' will avoid running the --cc-cmd. 'body' is
> +	equivalent to 'sob' + 'ccbody'. 'all' will suppress all auto cc
> +	values.  Default is the value of 'sendemail.suppresscc' configuration
> +	value; if that is unspecified, default to 'self' if --suppress-from is
> +	specified, as well as 'body' if --no-signed-off-cc is specified.

Perhaps with this many choices, it would be better to rewrite this into a
list form.

> diff --git a/git-send-email.perl b/git-send-email.perl
> index 2a3e3e8..23a55e2 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -319,21 +319,28 @@ 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)$/;
> +			unless $entry =~ /^(all|cccmd|cc|author|self|sob|body|bodycc)$/;
>  		$suppress_cc{$entry} = 1;
>  	}
>  }
>  
>  if ($suppress_cc{'all'}) {
> -	foreach my $entry (qw (ccmd cc author self sob)) {
> +	foreach my $entry (qw (ccmd cc author self sob body bodycc)) {
>  		$suppress_cc{$entry} = 1;
>  	}
>  	delete $suppress_cc{'all'};
>  }
>  
> +if ($suppress_cc{'sob'} && $suppress_cc{'bodycc'}) {
> +	$suppress_cc{'body'} = 1;
> +}

Hmmm.

I wonder if doing this the other way around, and treat 'body' as a mere
shorthand of setting 'sob' and 'bodycc', just like we do to 'all' (which
is just a shorthand to set all of them), and delete $suppress_cc{'body'},
would make the program much less error prone in the longer term, because
then each test needs to check the independent flags and do not have to
worry about a synthetic flag 'body'.

It's just a common sense futureproofing.  More of your code will resist
possible changes to the meaning of 'body' in the future that way..

>  # Debugging, print out the suppressions.
>  if (0) {
> @@ -1007,14 +1014,16 @@ foreach my $t (@files) {
>  	# Now parse the message body
>  	while(<F>) {
>  		$message .=  $_;
> +		next if $suppress_cc{'body'};
>  		if (/^(Signed-off-by|Cc): (.*)$/i) {
>  			chomp;
> +			my ($what, $c) = ($1, $2);
>  			chomp $c;
> +			next if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i;
> +			next if $suppress_cc{'bodycc'} and $what =~ /Cc/i;
>  			next if ($c eq $sender and $suppress_cc{'self'});
>  			push @cc, $c;
> +			printf("(body) Adding cc: %s from line '%s'\n",
>  				$c, $_) unless $quiet;

Looks sane.

> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index da54835..d7766f9 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -32,11 +32,11 @@ clean_fake_sendmail() {
>  }
>  
>  test_expect_success 'Extract patches' '
> -    patches=`git format-patch --cc="One <one@example.com>" --cc=two@example.com -n HEAD^1`
> +    patches=`git format-patch -s --cc="One <one@example.com>" --cc=two@example.com -n HEAD^1`
>  '
>  
>  test_expect_success 'Send patches' '
> -     git send-email --from="Example <nobody@example.com>" --to=nobody@example.com --smtp-server="$(pwd)/fake.sendmail" $patches 2>errors
> +     git send-email --suppress-cc=sob --from="Example <nobody@example.com>" --to=nobody@example.com --smtp-server="$(pwd)/fake.sendmail" $patches 2>errors
>  '

Looks sane.

> @@ -74,6 +74,7 @@ EOF
>  test_expect_success 'Show all headers' '
>  	git send-email \
>  		--dry-run \
> +		--suppress-cc=sob \
>  		--from="Example <from@example.com>" \
>  		--to=to@example.com \
>  		--cc=cc@example.com \

It is somewhat troublesome that only sob suppression is tested, output
from messages that do have in-body S-o-b/Cc without any --suppress-cc is
not tested anymore, and none of the new ones you added is tested.  You can
build confidence in the working of the suppression that way, but you would
also want to test that your new code is not over-suppressing things by
mistake.

When writing new tests, people tend to concentrate so much on showing the
new feature works in writing positive tests to forget that negative tests
are equally important.

[PATCH v3] send-email: --suppress-cc improvements

[Thomas Rast]

From: Jay Soffian <jaysoffian@gmail.com>

Since 6564828 (git-send-email: Generalize auto-cc recipient
mechanism., 2007-12-25) we can suppress automatic Cc generation
separately for each of the possible address sources.  However,
--suppress-cc=sob suppressed both SOB lines and body (but not header)
Cc lines, contrary to the name.

Change --suppress-cc=sob to mean only SOB lines, and add separate
choices 'bodycc' (body Cc lines) and 'body' (both 'sob' and 'bodycc').
The option --no-signed-off-by-cc now acts like --suppress-cc=sob,
which is not backwards compatible but matches the name of the option.

Also update the documentation and add a few tests.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 Documentation/git-send-email.txt |   25 ++++--
 git-send-email.perl              |   23 ++++--
 t/t9001-send-email.sh            |  150 +++++++++++++++++++++++++++++++++-----
 3 files changed, 163 insertions(+), 35 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index ff4aeff..45a092c 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -164,14 +164,23 @@ Automating
 
 --suppress-cc::
 	Specify an additional category of recipients to suppress the
-	auto-cc of.  'self' will avoid including the sender, 'author' will
-	avoid including the patch author, 'cc' will avoid including anyone
-	mentioned in Cc lines in the patch, 'sob' will avoid including
-	anyone mentioned in Signed-off-by lines, and 'cccmd' will avoid
-	running the --cc-cmd.  'all' will suppress all auto cc values.
-	Default is the value of 'sendemail.suppresscc' configuration value;
-	if that is unspecified, default to 'self' if --suppress-from is
-	specified, as well as 'sob' if --no-signed-off-cc is specified.
+	auto-cc of:
++
+--
+- 'author' will avoid including the patch author
+- 'cc' will avoid including anyone mentioned in Cc lines in the patch header
+- 'ccbody' will avoid including anyone mentioned in Cc lines in the
+  patch body (commit message)
+- 'cccmd' will avoid running the --cc-cmd.
+- 'self' will avoid including the sender
+- 'sob' will avoid including anyone mentioned in Signed-off-by lines
+- 'body' is equivalent to 'sob' + 'ccbody'
+- 'all' will suppress all auto cc values.
+--
++
+Default is the value of 'sendemail.suppresscc' configuration value; if
+that is unspecified, default to 'self' if --suppress-from is
+specified, as well as 'body' if --no-signed-off-cc is specified.
 
 --[no-]suppress-from::
 	If this is set, do not add the From: address to the cc: list.
diff --git a/git-send-email.perl b/git-send-email.perl
index 2a3e3e8..3a8c71f 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -68,9 +68,8 @@
   Automating:
     --identity              <str>  * Use the sendemail.<id> options.
     --cc-cmd                <str>  * Email Cc: via `<str> \$patch_path`
-    --suppress-cc           <str>  * author, self, sob, cccmd, all.
-    --[no-]signed-off-by-cc        * Send to Cc: and 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.
@@ -319,13 +318,13 @@
 if (@suppress_cc) {
 	foreach my $entry (@suppress_cc) {
 		die "Unknown --suppress-cc field: '$entry'\n"
-			unless $entry =~ /^(all|cccmd|cc|author|self|sob)$/;
+			unless $entry =~ /^(all|cccmd|cc|author|self|sob|body|bodycc)$/;
 		$suppress_cc{$entry} = 1;
 	}
 }
 
 if ($suppress_cc{'all'}) {
-	foreach my $entry (qw (ccmd cc author self sob)) {
+	foreach my $entry (qw (ccmd cc author self sob body bodycc)) {
 		$suppress_cc{$entry} = 1;
 	}
 	delete $suppress_cc{'all'};
@@ -335,6 +334,13 @@
 $suppress_cc{'self'} = $suppress_from if defined $suppress_from;
 $suppress_cc{'sob'} = !$signed_off_by_cc if defined $signed_off_by_cc;
 
+if ($suppress_cc{'body'}) {
+	foreach my $entry (qw (sob bodycc)) {
+		$suppress_cc{$entry} = 1;
+	}
+	delete $suppress_cc{'body'};
+}
+
 # Debugging, print out the suppressions.
 if (0) {
 	print "suppressions:\n";
@@ -1008,13 +1014,14 @@ sub get_patch_subject($) {
 	while(<F>) {
 		$message .=  $_;
 		if (/^(Signed-off-by|Cc): (.*)$/i) {
-			next if ($suppress_cc{'sob'});
 			chomp;
-			my $c = $2;
+			my ($what, $c) = ($1, $2);
 			chomp $c;
+			next if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i;
+			next if $suppress_cc{'bodycc'} and $what =~ /Cc/i;
 			next if ($c eq $sender and $suppress_cc{'self'});
 			push @cc, $c;
-			printf("(sob) Adding cc: %s from line '%s'\n",
+			printf("(body) Adding cc: %s from line '%s'\n",
 				$c, $_) unless $quiet;
 		}
 	}
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index da54835..ca006cf 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -32,11 +32,11 @@ clean_fake_sendmail() {
 }
 
 test_expect_success 'Extract patches' '
-    patches=`git format-patch --cc="One <one@example.com>" --cc=two@example.com -n HEAD^1`
+    patches=`git format-patch -s --cc="One <one@example.com>" --cc=two@example.com -n HEAD^1`
 '
 
 test_expect_success 'Send patches' '
-     git send-email --from="Example <nobody@example.com>" --to=nobody@example.com --smtp-server="$(pwd)/fake.sendmail" $patches 2>errors
+     git send-email --suppress-cc=sob --from="Example <nobody@example.com>" --to=nobody@example.com --smtp-server="$(pwd)/fake.sendmail" $patches 2>errors
 '
 
 cat >expected <<\EOF
@@ -74,6 +74,7 @@ EOF
 test_expect_success 'Show all headers' '
 	git send-email \
 		--dry-run \
+		--suppress-cc=sob \
 		--from="Example <from@example.com>" \
 		--to=to@example.com \
 		--cc=cc@example.com \
@@ -171,7 +172,7 @@ test_expect_success 'second message is patch' '
 	grep "Subject:.*Second" msgtxt2
 '
 
-cat >expected-show-all-headers <<\EOF
+cat >expected-suppress-sob <<\EOF
 0001-Second.patch
 (mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
 (mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
@@ -191,10 +192,10 @@ X-Mailer: X-MAILER-STRING
 Result: OK
 EOF
 
-test_expect_success 'sendemail.cc set' '
-	git config sendemail.cc cc@example.com &&
+test_suppression () {
 	git send-email \
 		--dry-run \
+		--suppress-cc=$1 \
 		--from="Example <from@example.com>" \
 		--to=to@example.com \
 		--smtp-server relay.example.com \
@@ -202,11 +203,16 @@ test_expect_success 'sendemail.cc set' '
 	sed	-e "s/^\(Date:\).*/\1 DATE-STRING/" \
 		-e "s/^\(Message-Id:\).*/\1 MESSAGE-ID-STRING/" \
 		-e "s/^\(X-Mailer:\).*/\1 X-MAILER-STRING/" \
-		>actual-show-all-headers &&
-	test_cmp expected-show-all-headers actual-show-all-headers
+		>actual-suppress-$1 &&
+	test_cmp expected-suppress-$1 actual-suppress-$1
+}
+
+test_expect_success 'sendemail.cc set' '
+	git config sendemail.cc cc@example.com &&
+	test_suppression sob
 '
 
-cat >expected-show-all-headers <<\EOF
+cat >expected-suppress-sob <<\EOF
 0001-Second.patch
 (mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
 (mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
@@ -228,17 +234,123 @@ EOF
 
 test_expect_success 'sendemail.cc unset' '
 	git config --unset sendemail.cc &&
-	git send-email \
-		--dry-run \
-		--from="Example <from@example.com>" \
-		--to=to@example.com \
-		--smtp-server relay.example.com \
-		$patches |
-	sed	-e "s/^\(Date:\).*/\1 DATE-STRING/" \
-		-e "s/^\(Message-Id:\).*/\1 MESSAGE-ID-STRING/" \
-		-e "s/^\(X-Mailer:\).*/\1 X-MAILER-STRING/" \
-		>actual-show-all-headers &&
-	test_cmp expected-show-all-headers actual-show-all-headers
+	test_suppression sob
+'
+
+cat >expected-suppress-all <<\EOF
+0001-Second.patch
+Dry-OK. Log says:
+Server: relay.example.com
+MAIL FROM:<from@example.com>
+RCPT TO:<to@example.com>
+From: Example <from@example.com>
+To: to@example.com
+Subject: [PATCH 1/1] Second.
+Date: DATE-STRING
+Message-Id: MESSAGE-ID-STRING
+X-Mailer: X-MAILER-STRING
+
+Result: OK
+EOF
+
+test_expect_success '--suppress-cc=all' '
+	test_suppression all
+'
+
+cat >expected-suppress-body <<\EOF
+0001-Second.patch
+(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
+(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
+(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
+Dry-OK. Log says:
+Server: relay.example.com
+MAIL FROM:<from@example.com>
+RCPT TO:<to@example.com>,<author@example.com>,<one@example.com>,<two@example.com>
+From: Example <from@example.com>
+To: to@example.com
+Cc: A <author@example.com>, One <one@example.com>, two@example.com
+Subject: [PATCH 1/1] Second.
+Date: DATE-STRING
+Message-Id: MESSAGE-ID-STRING
+X-Mailer: X-MAILER-STRING
+
+Result: OK
+EOF
+
+test_expect_success '--suppress-cc=body' '
+	test_suppression body
+'
+
+cat >expected-suppress-sob <<\EOF
+0001-Second.patch
+(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
+(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
+(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
+Dry-OK. Log says:
+Server: relay.example.com
+MAIL FROM:<from@example.com>
+RCPT TO:<to@example.com>,<author@example.com>,<one@example.com>,<two@example.com>
+From: Example <from@example.com>
+To: to@example.com
+Cc: A <author@example.com>, One <one@example.com>, two@example.com
+Subject: [PATCH 1/1] Second.
+Date: DATE-STRING
+Message-Id: MESSAGE-ID-STRING
+X-Mailer: X-MAILER-STRING
+
+Result: OK
+EOF
+
+test_expect_success '--suppress-cc=sob' '
+	test_suppression sob
+'
+
+cat >expected-suppress-bodycc <<\EOF
+0001-Second.patch
+(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
+(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
+(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
+(body) Adding cc: C O Mitter <committer@example.com> from line 'Signed-off-by: C O Mitter <committer@example.com>'
+Dry-OK. Log says:
+Server: relay.example.com
+MAIL FROM:<from@example.com>
+RCPT TO:<to@example.com>,<author@example.com>,<one@example.com>,<two@example.com>,<committer@example.com>
+From: Example <from@example.com>
+To: to@example.com
+Cc: A <author@example.com>, One <one@example.com>, two@example.com, C O Mitter <committer@example.com>
+Subject: [PATCH 1/1] Second.
+Date: DATE-STRING
+Message-Id: MESSAGE-ID-STRING
+X-Mailer: X-MAILER-STRING
+
+Result: OK
+EOF
+
+test_expect_success '--suppress-cc=bodycc' '
+	test_suppression bodycc
+'
+
+cat >expected-suppress-cc <<\EOF
+0001-Second.patch
+(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
+(body) Adding cc: C O Mitter <committer@example.com> from line 'Signed-off-by: C O Mitter <committer@example.com>'
+Dry-OK. Log says:
+Server: relay.example.com
+MAIL FROM:<from@example.com>
+RCPT TO:<to@example.com>,<author@example.com>,<committer@example.com>
+From: Example <from@example.com>
+To: to@example.com
+Cc: A <author@example.com>, C O Mitter <committer@example.com>
+Subject: [PATCH 1/1] Second.
+Date: DATE-STRING
+Message-Id: MESSAGE-ID-STRING
+X-Mailer: X-MAILER-STRING
+
+Result: OK
+EOF
+
+test_expect_success '--suppress-cc=cc' '
+	test_suppression cc
 '
 
 test_expect_success '--compose adds MIME for utf8 body' '
-- 
1.6.2.rc0.287.g66074

[Interdiff v3] send-email: --suppress-cc improvements

[Thomas Rast]

Interdiff to Jay's v2.
---
 Documentation/git-send-email.txt |   27 +++++---
 git-send-email.perl              |   20 +++---
 t/t9001-send-email.sh            |  142 +++++++++++++++++++++++++++++---------
 3 files changed, 136 insertions(+), 53 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index d6af035..45a092c 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -164,16 +164,23 @@ Automating
 
 --suppress-cc::
 	Specify an additional category of recipients to suppress the
-	auto-cc of.  'self' will avoid including the sender, 'author' will
-	avoid including the patch author, 'cc' will avoid including anyone
-	mentioned in Cc lines in the patch header, 'ccbody' will avoid
-	including anyone mentioned in Cc lines in the patch body (commit
-	message), 'sob' will avoid including anyone mentioned in Signed-off-by
-	lines, and 'cccmd' will avoid running the --cc-cmd. 'body' is
-	equivalent to 'sob' + 'ccbody'. 'all' will suppress all auto cc
-	values.  Default is the value of 'sendemail.suppresscc' configuration
-	value; if that is unspecified, default to 'self' if --suppress-from is
-	specified, as well as 'body' if --no-signed-off-cc is specified.
+	auto-cc of:
++
+--
+- 'author' will avoid including the patch author
+- 'cc' will avoid including anyone mentioned in Cc lines in the patch header
+- 'ccbody' will avoid including anyone mentioned in Cc lines in the
+  patch body (commit message)
+- 'cccmd' will avoid running the --cc-cmd.
+- 'self' will avoid including the sender
+- 'sob' will avoid including anyone mentioned in Signed-off-by lines
+- 'body' is equivalent to 'sob' + 'ccbody'
+- 'all' will suppress all auto cc values.
+--
++
+Default is the value of 'sendemail.suppresscc' configuration value; if
+that is unspecified, default to 'self' if --suppress-from is
+specified, as well as 'body' if --no-signed-off-cc is specified.
 
 --[no-]suppress-from::
 	If this is set, do not add the From: address to the cc: list.
diff --git a/git-send-email.perl b/git-send-email.perl
index 23a55e2..3a8c71f 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -69,8 +69,7 @@
     --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 Cc: and Signed-off-by:
-                                     addresses. Default on.
+    --[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.
@@ -331,16 +330,16 @@
 	delete $suppress_cc{'all'};
 }
 
-if ($suppress_cc{'sob'} && $suppress_cc{'bodycc'}) {
-	$suppress_cc{'body'} = 1;
-}
-
 # If explicit old-style ones are specified, they trump --suppress-cc.
 $suppress_cc{'self'} = $suppress_from if defined $suppress_from;
-# For backwards compatibility, old-style --signed-off-by-cc suppresses
-# SOB and body Cc lines, whereas --supress-cc=sob suppresses just the SOB
-# line, but not the body Cc.
-$suppress_cc{'body'} = !$signed_off_by_cc if defined $signed_off_by_cc;
+$suppress_cc{'sob'} = !$signed_off_by_cc if defined $signed_off_by_cc;
+
+if ($suppress_cc{'body'}) {
+	foreach my $entry (qw (sob bodycc)) {
+		$suppress_cc{$entry} = 1;
+	}
+	delete $suppress_cc{'body'};
+}
 
 # Debugging, print out the suppressions.
 if (0) {
@@ -1014,7 +1013,6 @@ sub get_patch_subject($) {
 	# Now parse the message body
 	while(<F>) {
 		$message .=  $_;
-		next if $suppress_cc{'body'};
 		if (/^(Signed-off-by|Cc): (.*)$/i) {
 			chomp;
 			my ($what, $c) = ($1, $2);
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index d7766f9..ca006cf 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -172,7 +172,7 @@ test_expect_success 'second message is patch' '
 	grep "Subject:.*Second" msgtxt2
 '
 
-cat >expected-show-all-headers <<\EOF
+cat >expected-suppress-sob <<\EOF
 0001-Second.patch
 (mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
 (mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
@@ -192,11 +192,10 @@ X-Mailer: X-MAILER-STRING
 Result: OK
 EOF
 
-test_expect_success 'sendemail.cc set' '
-	git config sendemail.cc cc@example.com &&
+test_suppression () {
 	git send-email \
 		--dry-run \
-		--suppress-cc=sob \
+		--suppress-cc=$1 \
 		--from="Example <from@example.com>" \
 		--to=to@example.com \
 		--smtp-server relay.example.com \
@@ -204,11 +203,16 @@ test_expect_success 'sendemail.cc set' '
 	sed	-e "s/^\(Date:\).*/\1 DATE-STRING/" \
 		-e "s/^\(Message-Id:\).*/\1 MESSAGE-ID-STRING/" \
 		-e "s/^\(X-Mailer:\).*/\1 X-MAILER-STRING/" \
-		>actual-show-all-headers &&
-	test_cmp expected-show-all-headers actual-show-all-headers
+		>actual-suppress-$1 &&
+	test_cmp expected-suppress-$1 actual-suppress-$1
+}
+
+test_expect_success 'sendemail.cc set' '
+	git config sendemail.cc cc@example.com &&
+	test_suppression sob
 '
 
-cat >expected-show-all-headers <<\EOF
+cat >expected-suppress-sob <<\EOF
 0001-Second.patch
 (mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
 (mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
@@ -230,21 +234,10 @@ EOF
 
 test_expect_success 'sendemail.cc unset' '
 	git config --unset sendemail.cc &&
-	git send-email \
-		--dry-run \
-		--suppress-cc=sob \
-		--from="Example <from@example.com>" \
-		--to=to@example.com \
-		--smtp-server relay.example.com \
-		$patches |
-	sed	-e "s/^\(Date:\).*/\1 DATE-STRING/" \
-		-e "s/^\(Message-Id:\).*/\1 MESSAGE-ID-STRING/" \
-		-e "s/^\(X-Mailer:\).*/\1 X-MAILER-STRING/" \
-		>actual-show-all-headers &&
-	test_cmp expected-show-all-headers actual-show-all-headers
+	test_suppression sob
 '
 
-cat >expected-show-all-headers <<\EOF
+cat >expected-suppress-all <<\EOF
 0001-Second.patch
 Dry-OK. Log says:
 Server: relay.example.com
@@ -261,18 +254,103 @@ Result: OK
 EOF
 
 test_expect_success '--suppress-cc=all' '
-	git send-email \
-		--dry-run \
-		--suppress-cc=all \
-		--from="Example <from@example.com>" \
-		--to=to@example.com \
-		--smtp-server relay.example.com \
-		$patches |
-	sed	-e "s/^\(Date:\).*/\1 DATE-STRING/" \
-		-e "s/^\(Message-Id:\).*/\1 MESSAGE-ID-STRING/" \
-		-e "s/^\(X-Mailer:\).*/\1 X-MAILER-STRING/" \
-		>actual-show-all-headers &&
-	test_cmp expected-show-all-headers actual-show-all-headers
+	test_suppression all
+'
+
+cat >expected-suppress-body <<\EOF
+0001-Second.patch
+(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
+(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
+(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
+Dry-OK. Log says:
+Server: relay.example.com
+MAIL FROM:<from@example.com>
+RCPT TO:<to@example.com>,<author@example.com>,<one@example.com>,<two@example.com>
+From: Example <from@example.com>
+To: to@example.com
+Cc: A <author@example.com>, One <one@example.com>, two@example.com
+Subject: [PATCH 1/1] Second.
+Date: DATE-STRING
+Message-Id: MESSAGE-ID-STRING
+X-Mailer: X-MAILER-STRING
+
+Result: OK
+EOF
+
+test_expect_success '--suppress-cc=body' '
+	test_suppression body
+'
+
+cat >expected-suppress-sob <<\EOF
+0001-Second.patch
+(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
+(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
+(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
+Dry-OK. Log says:
+Server: relay.example.com
+MAIL FROM:<from@example.com>
+RCPT TO:<to@example.com>,<author@example.com>,<one@example.com>,<two@example.com>
+From: Example <from@example.com>
+To: to@example.com
+Cc: A <author@example.com>, One <one@example.com>, two@example.com
+Subject: [PATCH 1/1] Second.
+Date: DATE-STRING
+Message-Id: MESSAGE-ID-STRING
+X-Mailer: X-MAILER-STRING
+
+Result: OK
+EOF
+
+test_expect_success '--suppress-cc=sob' '
+	test_suppression sob
+'
+
+cat >expected-suppress-bodycc <<\EOF
+0001-Second.patch
+(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
+(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
+(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
+(body) Adding cc: C O Mitter <committer@example.com> from line 'Signed-off-by: C O Mitter <committer@example.com>'
+Dry-OK. Log says:
+Server: relay.example.com
+MAIL FROM:<from@example.com>
+RCPT TO:<to@example.com>,<author@example.com>,<one@example.com>,<two@example.com>,<committer@example.com>
+From: Example <from@example.com>
+To: to@example.com
+Cc: A <author@example.com>, One <one@example.com>, two@example.com, C O Mitter <committer@example.com>
+Subject: [PATCH 1/1] Second.
+Date: DATE-STRING
+Message-Id: MESSAGE-ID-STRING
+X-Mailer: X-MAILER-STRING
+
+Result: OK
+EOF
+
+test_expect_success '--suppress-cc=bodycc' '
+	test_suppression bodycc
+'
+
+cat >expected-suppress-cc <<\EOF
+0001-Second.patch
+(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
+(body) Adding cc: C O Mitter <committer@example.com> from line 'Signed-off-by: C O Mitter <committer@example.com>'
+Dry-OK. Log says:
+Server: relay.example.com
+MAIL FROM:<from@example.com>
+RCPT TO:<to@example.com>,<author@example.com>,<committer@example.com>
+From: Example <from@example.com>
+To: to@example.com
+Cc: A <author@example.com>, C O Mitter <committer@example.com>
+Subject: [PATCH 1/1] Second.
+Date: DATE-STRING
+Message-Id: MESSAGE-ID-STRING
+X-Mailer: X-MAILER-STRING
+
+Result: OK
+EOF
+
+test_expect_success '--suppress-cc=cc' '
+	test_suppression cc
 '
 
 test_expect_success '--compose adds MIME for utf8 body' '
-- 
1.6.2.rc0.287.g66074

Re: [PATCH v3] send-email: --suppress-cc improvements

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 1259 bytes --]

I just noticed that I lost the comments after '---' due to repeated
format-patch'ing, so here goes:

Thomas Rast wrote:
> From: Jay Soffian <jaysoffian@gmail.com>
> 
> Since 6564828 (git-send-email: Generalize auto-cc recipient
> mechanism., 2007-12-25) we can suppress automatic Cc generation
> separately for each of the possible address sources.  However,
> --suppress-cc=sob suppressed both SOB lines and body (but not header)
> Cc lines, contrary to the name.
> 
> Change --suppress-cc=sob to mean only SOB lines, and add separate
> choices 'bodycc' (body Cc lines) and 'body' (both 'sob' and 'bodycc').
> The option --no-signed-off-by-cc now acts like --suppress-cc=sob,
> which is not backwards compatible but matches the name of the option.
> 
> Also update the documentation and add a few tests.
> 
> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---

I tried to address Junio's comments, mostly by reformatting the docs
and expanding the tests, but also changing the handling of 'body' in
the code.  I also edited the patch message to my liking.

Of course your SOB above is now forged, so you'll have to ack it.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH v3] send-email: --suppress-cc improvements

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> I just noticed that I lost the comments after '---' due to repeated
> format-patch'ing, so here goes:

Thanks; Jay did another round of updates based on this one, which
I think is the one I took, if I did things correctly ;-)

Re: [PATCH] send-email: handle multiple Cc addresses when reading mbox message

[Jeff King]

On Fri, Feb 13, 2009 at 06:05:13PM -0500, Jay Soffian wrote:

> This patch teaches it to unfold header lines by pre-processing the
> header before extracting any of its fields. It also teaches it to split
> the Cc line using Mail::Address if available, otherwise it just splits
> on a simple \s*,\s* regular expression.

Ugh. This is not in any way a comment on your patch, but it is really
painful to see time and again send-email bugs like this that could be
solved by using the wealth of existing perl modules.

I think send-email is stuck somewhere between "here is the bare minimum
to send some patches, and we don't want to require any fancy
dependencies" and "a full-fledged replacement for my MUA when dealing
with patches".

The approach of "use this module if we have it, otherwise do something
quick and dirty" might be the best approach. But I do wonder if the
"quick and dirty" code path will end up buggy due to lack of use, and/or
be surprising for people who expect send-email to be more thorough.

The other alternatives are to keep fixing bugs and improving
incrementally until it converges on being an actual MUA, or to simply
start a rewrite based on reasonable CPAN modules.

I don't know what the right solution is. I am certainly not volunteering
to rewrite; I am very happy just using my actual MUA to send patches. :)

-Peff

Re: [PATCH] send-email: handle multiple Cc addresses when reading mbox message

[Jay Soffian]

On Fri, Feb 13, 2009 at 7:31 PM, Jeff King <peff@peff.net> wrote:
> I don't know what the right solution is. I am certainly not volunteering
> to rewrite; I am very happy just using my actual MUA to send patches. :)

It is quite ugly, and I have thought about rewriting it.

But to be quite honest, I have become very discouraged over the last
two weeks about submitting patches. What seems at first to be a simple
change has required multiple revisions over frankly what feels to me
like nit-picking.

It goes like this:

Me: Notice wart
Me: Cut wart off (i.e. submit patch)
Reviewer: Sorry, you didn't cut that wart off to my standards, unless
you can do it better, I'd rather have the wart.

I dunno, maybe I'm too sensitive and don't have the fortitude for
contributing to git.

Or maybe I'm just venting on a Friday.

j.

Re: [PATCH] send-email: handle multiple Cc addresses when reading mbox message

[Jeff King]

On Fri, Feb 13, 2009 at 07:42:16PM -0500, Jay Soffian wrote:

> On Fri, Feb 13, 2009 at 7:31 PM, Jeff King <peff@peff.net> wrote:
> > I don't know what the right solution is. I am certainly not volunteering
> > to rewrite; I am very happy just using my actual MUA to send patches. :)
> 
> It is quite ugly, and I have thought about rewriting it.
> 
> But to be quite honest, I have become very discouraged over the last
> two weeks about submitting patches. What seems at first to be a simple
> change has required multiple revisions over frankly what feels to me
> like nit-picking.

I hope it wasn't my message in this thread that sent you over the edge.
I really did mean the "this isn't a comment on your patch" but was just
musing out loud over what the right direction for send-email is in
general.

> Me: Notice wart
> Me: Cut wart off (i.e. submit patch)
> Reviewer: Sorry, you didn't cut that wart off to my standards, unless
> you can do it better, I'd rather have the wart.
> 
> I dunno, maybe I'm too sensitive and don't have the fortitude for
> contributing to git.
> 
> Or maybe I'm just venting on a Friday.

I hope just venting. I think the warts you've been cutting off have been
improving git.

One of the things I have seen happening with your patches is something
like:

  Jay: Here's a patch.
  Reviewer: Good, but it should also do X to be complete.
              or
            Good, but there is one corner case where the correct
            behavior is unclear [queue endless discussion, production of
            several versions of the patch doing different behavior, or
            realization that some of the behavior would be an order of
            magnitude more work to implement]

Those sorts of reviews are good for helping git in the long run, and I
think reviewers are often thinking in terms of git's overall
progression. But I think it is also OK sometimes as a patch submitter to
say "This series scratches my itch, and I am done for now." And maybe
somebody else picks it up and builds on it. Or maybe you do, but weeks
or months later. Or maybe nobody does, because it turns out that nobody
really cares about that corner case in practice.

Which is really just "perfect is the enemy of the good" in another form,
and recognizing that git's development is incremental.

And all of that isn't to say there aren't plenty of patches that should
be rejected for incompleteness or lack of handling corner cases. Many
times an incomplete solution is worse than no solution at all. So there
is some judgement required about whether the changes are a net positive
and a net negative.

But I think it is up to the submitter to decide how far he wants to take
a given topic (and at what rate), and to say "OK, I'll go implement
that", "This behavior doesn't go all the way, but we're better off than
before", or even to just take a week off and come back to it later.

So if you're feeling burned out on submitting patches, maybe that helps.

Of course, there are also just plain nitpicks. We try to keep them to a
minimum, but they do creep in. :)

-Peff

Re: [PATCH] send-email: handle multiple Cc addresses when reading mbox message

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> It goes like this:
>
> Me: Notice wart
> Me: Cut wart off (i.e. submit patch)
> Reviewer: Sorry, you didn't cut that wart off to my standards, unless
> you can do it better, I'd rather have the wart.
>
> I dunno, maybe I'm too sensitive and don't have the fortitude for
> contributing to git.

Different people have different priorities and judging criteria.

To some authors, code is the only thing that matters and they say a crappy
commit log message is Ok if the code works.  I actually do not even read
the patch if the commit log message does not clearly express what problem
the author is trying to address, because I always have other patches to
attend to, and if the author cannot formulate his thought clearly in the
commit log message, it is likely that the code doesn't, either, and even
if it does, the code speaks only about how it does what it does, and not
much about _why_ it is so, which is the most important thing down the
road.

Some people comment more on readability of the patch and format of the
code while some others let them pass and concentrate on performance and
correctness.

All of them are important, and reviewers, together with the original
author, complement each other's weakness to work toward a better system.

The first thing to learn is to tell the difference between "you didn't cut
that wart off to my standards, unless you can do it better, I'd rather
have the wart." and "you claim you have cut the wart off, but that's minor
compared to the wart you are missing here which can be cut at the same
time without too much effort; let's do so at the same time before we all
forget".

The review comments I read on this list are more often the latter, but I
can certainly understand some authors, being so excited about and fond of
his own creation, mistake it as the former.  Two tricks I learned to avoid
that trap while working on git, and especially when git was still young
and I was a contributor, was to sleep on things, and to always consider
the possibility that I _might_ be wrong.  The latter takes some practice,
and I admit I still haven't mastered it yet, but I am trying.