threads / patch / 17780

patchsend-email: handle multiple Cc addresses when reading mbox message

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

## tl;dr

20 messages between Feb 13, 2009 and Feb 14, 2009. Diffs are folded; open one to read it.

replies: 19people: 4as markdown or json

Jay Soffian· Feb 13, 2009, 23:05 UTC · lore
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(-)
Show changes to 2 files +99 −73

git-send-email.perl, t/t9001-send-email.sh

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
Thomas Rast· Feb 13, 2009, 23:32 UTC · re: Jay Soffian · lore

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

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
Jay Soffian· Feb 13, 2009, 23:39 UTC · re: Thomas Rast · lore

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

On Fri, Feb 13, 2009 at 6:32 PM, Thomas Rast <trast@student.ethz.ch> wrote:
Show 12 quoted lines
> 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.
Jay Soffian· Feb 13, 2009, 23:44 UTC · re: Jay Soffian · lore

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

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.
Jay Soffian· Feb 14, 2009, 03:51 UTC · re: Thomas Rast · lore

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

From: Jay Soffian <jaysoffian@gmail.com>
On Fri, Feb 13, 2009 at 6:32 PM, Thomas Rast <trast@student.ethz.ch> wrote:
Show 12 quoted lines
> 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(-)
Jay Soffian· Feb 14, 2009, 03:51 UTC · re: Jay Soffian · lore

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

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(-)
Show changes to git-send-email.perl +3 −5
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
Jay Soffian· Feb 14, 2009, 03:51 UTC · re: Jay Soffian · lore

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

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(-)
Show changes to git-send-email.perl +1 −1
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
Jay Soffian· Feb 14, 2009, 03:51 UTC · re: Jay Soffian · lore

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

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(-)
Show changes to 3 files +69 −23

Documentation/git-send-email.txt, git-send-email.perl, t/t9001-send-email.sh

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
Jay Soffian· Feb 14, 2009, 05:37 UTC · re: Jay Soffian · lore

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

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(-)
Show changes to 3 files +60 −15

Documentation/git-send-email.txt, git-send-email.perl, t/t9001-send-email.sh

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
Junio C Hamano· Feb 14, 2009, 06:36 UTC · re: Jay Soffian · lore

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

Jay Soffian <jaysoffian@gmail.com> writes:
Show 9 quoted lines
> 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.

Show 6 quoted lines
>  * '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?

Show 16 quoted lines
> 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.

Show 25 quoted lines
> 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..

Show 17 quoted lines
>  # 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.
Show 16 quoted lines
> 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.
Show 8 quoted lines
> @@ -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.

Jay Soffian· Feb 14, 2009, 05:50 UTC · re: Jay Soffian · lore

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

On Fri, Feb 13, 2009 at 10:51 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
Show 24 quoted lines
> 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.
Jay Soffian· Feb 14, 2009, 06:21 UTC · re: Jay Soffian · lore

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

--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(-)
Show changes to git-send-email.perl +6 −7
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
Junio C Hamano· Feb 14, 2009, 06:52 UTC · re: Jay Soffian · lore

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

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.

Jay Soffian· Feb 14, 2009, 07:15 UTC · re: Junio C Hamano · lore

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

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.
Junio C Hamano· Feb 14, 2009, 07:35 UTC · re: Jay Soffian · lore

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

Jay Soffian <jaysoffian@gmail.com> writes:
> Who let the original code in w/o tests, hmm? :-)
Reviewers and list participants, including you.
Thomas Rast· Feb 14, 2009, 16:57 UTC · re: Jay Soffian · lore

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

Jay Soffian wrote:
Show 18 quoted lines
> 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
Jeff King· Feb 14, 2009, 00:31 UTC · re: Jay Soffian · lore

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

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
Jay Soffian· Feb 14, 2009, 00:42 UTC · re: Jeff King · lore

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

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.
Jeff King· Feb 14, 2009, 03:37 UTC · re: Jay Soffian · lore

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

On Fri, Feb 13, 2009 at 07:42:16PM -0500, Jay Soffian wrote:
Show 10 quoted lines
> 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.

Show 9 quoted lines
> 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
Junio C Hamano· Feb 14, 2009, 08:16 UTC · re: Jay Soffian · lore

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

Jay Soffian <jaysoffian@gmail.com> writes:
Show 9 quoted lines
> 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.

← back to recent threads