[PATCH] git-send-email.perl: Fix handling of suppresscc option.

[PATCH] git-send-email.perl: Fix handling of suppresscc option.

[Jens Stimpfle]

Signed-off-by: Jens Stimpfle <debian@jstimpfle.de>
---

Notes:
    This patch makes sure that "sob", "cc" and "bodycc" values for
    sendemail.suppresscc option are handled, even when the email-addresses in
    question are equal to the sender and "self" in not configured in
    sendemail.suppresscc.
    
    Sounds complicated, I know. But the current behaviour is really confusing: For
    example, when sending, git-send-email logs
    
    (mbox) Adding cc: Jens Stimpfle <debian@jstimpfle.de> from line 'From: Jens Stimpfle <debian@jstimpfle.de>'
    (body) Adding cc: Jens Stimpfle <debian@jstimpfle.de> from line 'Signed-off-by: Jens Stimpfle <debian@jstimpfle.de>'
    
    even though I have "sob" configured in sendemail.suppresscc.
    
    With this patch, the suppression handling is also made consistent with the
    handling of the "author" value.

 git-send-email.perl | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 9949db0..452a783 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1377,11 +1377,8 @@ foreach my $t (@files) {
 				foreach my $addr (parse_address_line($1)) {
 					my $qaddr = unquote_rfc2047($addr);
 					my $saddr = sanitize_address($qaddr);
-					if ($saddr eq $sender) {
-						next if ($suppress_cc{'self'});
-					} else {
-						next if ($suppress_cc{'cc'});
-					}
+					next if $suppress_cc{'cc'};
+					next if $suppress_cc{'self'} and $saddr eq $sender;
 					printf("(mbox) Adding cc: %s from line '%s'\n",
 						$addr, $_) unless $quiet;
 					push @cc, $addr;
@@ -1425,12 +1422,9 @@ foreach my $t (@files) {
 			my ($what, $c) = ($1, $2);
 			chomp $c;
 			my $sc = sanitize_address($c);
-			if ($sc eq $sender) {
-				next if ($suppress_cc{'self'});
-			} else {
-				next if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i;
-				next if $suppress_cc{'bodycc'} and $what =~ /Cc/i;
-			}
+			next if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i;
+			next if $suppress_cc{'bodycc'} and $what =~ /Cc/i;
+			next if $suppress_cc{'self'} and $sc eq $sender;
 			push @cc, $c;
 			printf("(body) Adding cc: %s from line '%s'\n",
 				$c, $_) unless $quiet;
-- 
2.1.1

Re: [PATCH] git-send-email.perl: Fix handling of suppresscc option.

[Junio C Hamano]

Jens Stimpfle <debian@jstimpfle.de> writes:

> Signed-off-by: Jens Stimpfle <debian@jstimpfle.de>
> ---

Thanks.

Please do better than saying "Fix" to explain your changes in your
log message.

Also, on the Subject:, s/Fix/fix/; s/option./option/ to match other
entries in "git shortlog" message.

"What you think is broken" is clear (i.e. "supresscc option" is
broken) with the subject line alone, but "How it is broken", "How it
should behave instead", and "What are the differences between the
broken and the correct behaviour" should be explained in the log
message.

In other words, most of what you wrote below should come before your
S-o-b: line.

> Notes:
> ...

> diff --git a/git-send-email.perl b/git-send-email.perl
> index 9949db0..452a783 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1377,11 +1377,8 @@ foreach my $t (@files) {
>  				foreach my $addr (parse_address_line($1)) {
>  					my $qaddr = unquote_rfc2047($addr);
>  					my $saddr = sanitize_address($qaddr);
> -					if ($saddr eq $sender) {
> -						next if ($suppress_cc{'self'});
> -					} else {
> -						next if ($suppress_cc{'cc'});
> -					}
> +					next if $suppress_cc{'cc'};
> +					next if $suppress_cc{'self'} and $saddr eq $sender;

This smells more like a change in behaviour than bugfix from a
cursory look, though.  It used to be that I could receive a copy by
adding me to cc as long as I did not suppress 'self', even I
squelched everybody else by suppressing 'cc'.  I do not use such a
configuration myself but I wonder if people rely on this behaviour
as a feature.

> @@ -1425,12 +1422,9 @@ foreach my $t (@files) {
>  			my ($what, $c) = ($1, $2);
>  			chomp $c;
>  			my $sc = sanitize_address($c);
> -			if ($sc eq $sender) {
> -				next if ($suppress_cc{'self'});
> -			} else {
> -				next if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i;
> -				next if $suppress_cc{'bodycc'} and $what =~ /Cc/i;
> -			}
> +			next if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i;
> +			next if $suppress_cc{'bodycc'} and $what =~ /Cc/i;
> +			next if $suppress_cc{'self'} and $sc eq $sender;

Likewise.

I do like the updated logic flow in both hunks, though.

"When I say addresses on Cc: does not matter, it doesn't.  No matter
what the address in question is" (likewise for S-o-b:) is what the
updated logic says.  It is easier to explain than the traditional
"The way to squelch my address is by 'suppress self'; for all other
addresses on Cc:/S-o-b:, there are separate suppression methods".

But I have a slight suspicion that this special casing of 'self' was
done on purpose, and people may be relying on it.

Re: [PATCH] git-send-email.perl: Fix handling of suppresscc option.

[Jens Stimpfle]

Junio, thanks for reviewing.

On Wed, Nov 12, 2014 at 10:25:14AM -0800, Junio C Hamano wrote:
> "When I say addresses on Cc: does not matter, it doesn't.  No matter
> what the address in question is" (likewise for S-o-b:) is what the
> updated logic says.  It is easier to explain than the traditional
> "The way to squelch my address is by 'suppress self'; for all other
> addresses on Cc:/S-o-b:, there are separate suppression methods".
> 
> But I have a slight suspicion that this special casing of 'self' was
> done on purpose, and people may be relying on it.

It seems the behaviour is exactly described in the manpage. Sorry for
having missed that.

The behaviour is super-complex and confusing, though. It's easy to
imagine much simpler, intuitive and "transparent" ones.

But I have no experience with distributed git workflows so I'm
absolutely not in the position to call it broken or provide a "fix".

Sorry for the noise.