Re: [PATCH] git-send-email.perl: Fix handling of suppresscc option.
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 12, 2014, 18:25 UTC
- Message-ID
- <xmqqoasc46ph.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1415801891-28471-1-git-send-email-debian@jstimpfle.de>
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: > ...
Show 15 quoted lines
> 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.
Show 13 quoted lines
> @@ -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.