{"thread":{"id":"37939","subject":"[PATCH] git-send-email.perl: Fix handling of suppresscc option.","startedAt":"2014-11-12T14:18:11Z","lastAt":"2014-11-13T11:41:04Z","messageCount":3,"participants":["Jens Stimpfle","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"251756","messageId":"1415801891-28471-1-git-send-email-debian@jstimpfle.de","threadId":"37939","inReplyTo":null,"subject":"[PATCH] git-send-email.perl: Fix handling of suppresscc option.","fromName":"Jens Stimpfle","fromEmail":"debian@jstimpfle.de","sentAt":"2014-11-12T14:18:11Z","receivedAt":"2014-11-12T14:18:11Z","isPatch":true,"sender":{"key":"debian@jstimpfle.de","avatar":null},"body":"Signed-off-by: Jens Stimpfle <debian@jstimpfle.de>\n---\n\nNotes:\n    This patch makes sure that \"sob\", \"cc\" and \"bodycc\" values for\n    sendemail.suppresscc option are handled, even when the email-addresses in\n    question are equal to the sender and \"self\" in not configured in\n    sendemail.suppresscc.\n    \n    Sounds complicated, I know. But the current behaviour is really confusing: For\n    example, when sending, git-send-email logs\n    \n    (mbox) Adding cc: Jens Stimpfle <debian@jstimpfle.de> from line 'From: Jens Stimpfle <debian@jstimpfle.de>'\n    (body) Adding cc: Jens Stimpfle <debian@jstimpfle.de> from line 'Signed-off-by: Jens Stimpfle <debian@jstimpfle.de>'\n    \n    even though I have \"sob\" configured in sendemail.suppresscc.\n    \n    With this patch, the suppression handling is also made consistent with the\n    handling of the \"author\" value.\n\n git-send-email.perl | 16 +++++-----------\n 1 file changed, 5 insertions(+), 11 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 9949db0..452a783 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1377,11 +1377,8 @@ foreach my $t (@files) {\n \t\t\t\tforeach my $addr (parse_address_line($1)) {\n \t\t\t\t\tmy $qaddr = unquote_rfc2047($addr);\n \t\t\t\t\tmy $saddr = sanitize_address($qaddr);\n-\t\t\t\t\tif ($saddr eq $sender) {\n-\t\t\t\t\t\tnext if ($suppress_cc{'self'});\n-\t\t\t\t\t} else {\n-\t\t\t\t\t\tnext if ($suppress_cc{'cc'});\n-\t\t\t\t\t}\n+\t\t\t\t\tnext if $suppress_cc{'cc'};\n+\t\t\t\t\tnext if $suppress_cc{'self'} and $saddr eq $sender;\n \t\t\t\t\tprintf(\"(mbox) Adding cc: %s from line '%s'\\n\",\n \t\t\t\t\t\t$addr, $_) unless $quiet;\n \t\t\t\t\tpush @cc, $addr;\n@@ -1425,12 +1422,9 @@ foreach my $t (@files) {\n \t\t\tmy ($what, $c) = ($1, $2);\n \t\t\tchomp $c;\n \t\t\tmy $sc = sanitize_address($c);\n-\t\t\tif ($sc eq $sender) {\n-\t\t\t\tnext if ($suppress_cc{'self'});\n-\t\t\t} else {\n-\t\t\t\tnext if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i;\n-\t\t\t\tnext if $suppress_cc{'bodycc'} and $what =~ /Cc/i;\n-\t\t\t}\n+\t\t\tnext if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i;\n+\t\t\tnext if $suppress_cc{'bodycc'} and $what =~ /Cc/i;\n+\t\t\tnext if $suppress_cc{'self'} and $sc eq $sender;\n \t\t\tpush @cc, $c;\n \t\t\tprintf(\"(body) Adding cc: %s from line '%s'\\n\",\n \t\t\t\t$c, $_) unless $quiet;\n-- \n2.1.1\n"},{"id":"251764","messageId":"xmqqoasc46ph.fsf@gitster.dls.corp.google.com","threadId":"37939","inReplyTo":"1415801891-28471-1-git-send-email-debian@jstimpfle.de","subject":"Re: [PATCH] git-send-email.perl: Fix handling of suppresscc option.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-12T18:25:14Z","receivedAt":"2014-11-12T18:25:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Stimpfle <debian@jstimpfle.de> writes:\n\n> Signed-off-by: Jens Stimpfle <debian@jstimpfle.de>\n> ---\n\nThanks.\n\nPlease do better than saying \"Fix\" to explain your changes in your\nlog message.\n\nAlso, on the Subject:, s/Fix/fix/; s/option./option/ to match other\nentries in \"git shortlog\" message.\n\n\"What you think is broken\" is clear (i.e. \"supresscc option\" is\nbroken) with the subject line alone, but \"How it is broken\", \"How it\nshould behave instead\", and \"What are the differences between the\nbroken and the correct behaviour\" should be explained in the log\nmessage.\n\nIn other words, most of what you wrote below should come before your\nS-o-b: line.\n\n> Notes:\n> ...\n\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 9949db0..452a783 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -1377,11 +1377,8 @@ foreach my $t (@files) {\n>  \t\t\t\tforeach my $addr (parse_address_line($1)) {\n>  \t\t\t\t\tmy $qaddr = unquote_rfc2047($addr);\n>  \t\t\t\t\tmy $saddr = sanitize_address($qaddr);\n> -\t\t\t\t\tif ($saddr eq $sender) {\n> -\t\t\t\t\t\tnext if ($suppress_cc{'self'});\n> -\t\t\t\t\t} else {\n> -\t\t\t\t\t\tnext if ($suppress_cc{'cc'});\n> -\t\t\t\t\t}\n> +\t\t\t\t\tnext if $suppress_cc{'cc'};\n> +\t\t\t\t\tnext if $suppress_cc{'self'} and $saddr eq $sender;\n\nThis smells more like a change in behaviour than bugfix from a\ncursory look, though.  It used to be that I could receive a copy by\nadding me to cc as long as I did not suppress 'self', even I\nsquelched everybody else by suppressing 'cc'.  I do not use such a\nconfiguration myself but I wonder if people rely on this behaviour\nas a feature.\n\n> @@ -1425,12 +1422,9 @@ foreach my $t (@files) {\n>  \t\t\tmy ($what, $c) = ($1, $2);\n>  \t\t\tchomp $c;\n>  \t\t\tmy $sc = sanitize_address($c);\n> -\t\t\tif ($sc eq $sender) {\n> -\t\t\t\tnext if ($suppress_cc{'self'});\n> -\t\t\t} else {\n> -\t\t\t\tnext if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i;\n> -\t\t\t\tnext if $suppress_cc{'bodycc'} and $what =~ /Cc/i;\n> -\t\t\t}\n> +\t\t\tnext if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i;\n> +\t\t\tnext if $suppress_cc{'bodycc'} and $what =~ /Cc/i;\n> +\t\t\tnext if $suppress_cc{'self'} and $sc eq $sender;\n\nLikewise.\n\nI do like the updated logic flow in both hunks, though.\n\n\"When I say addresses on Cc: does not matter, it doesn't.  No matter\nwhat the address in question is\" (likewise for S-o-b:) is what the\nupdated logic says.  It is easier to explain than the traditional\n\"The way to squelch my address is by 'suppress self'; for all other\naddresses on Cc:/S-o-b:, there are separate suppression methods\".\n\nBut I have a slight suspicion that this special casing of 'self' was\ndone on purpose, and people may be relying on it.\n"},{"id":"251807","messageId":"20141113114104.GA27098@jstimpfle.de","threadId":"37939","inReplyTo":"xmqqoasc46ph.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-send-email.perl: Fix handling of suppresscc option.","fromName":"Jens Stimpfle","fromEmail":"debian@jstimpfle.de","sentAt":"2014-11-13T11:41:04Z","receivedAt":"2014-11-13T11:41:04Z","isPatch":true,"sender":{"key":"debian@jstimpfle.de","avatar":null},"body":"Junio, thanks for reviewing.\n\nOn Wed, Nov 12, 2014 at 10:25:14AM -0800, Junio C Hamano wrote:\n> \"When I say addresses on Cc: does not matter, it doesn't.  No matter\n> what the address in question is\" (likewise for S-o-b:) is what the\n> updated logic says.  It is easier to explain than the traditional\n> \"The way to squelch my address is by 'suppress self'; for all other\n> addresses on Cc:/S-o-b:, there are separate suppression methods\".\n> \n> But I have a slight suspicion that this special casing of 'self' was\n> done on purpose, and people may be relying on it.\n\nIt seems the behaviour is exactly described in the manpage. Sorry for\nhaving missed that.\n\nThe behaviour is super-complex and confusing, though. It's easy to\nimagine much simpler, intuitive and \"transparent\" ones.\n\nBut I have no experience with distributed git workflows so I'm\nabsolutely not in the position to call it broken or provide a \"fix\".\n\nSorry for the noise.\n"}]}