| Message ID | 20181010111351.5045-4-rv@rasmusvillemoes.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | send-email: Also pick up cc addresses from -by trailers | expand |
On Wed, Oct 10 2018, Rasmus Villemoes wrote: > - next if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i; > + next if $suppress_cc{'sob'} and $what =~ /^Signed-off-by$/i; > + next if $suppress_cc{'misc-by'} > + and $what =~ /-by$/i and $what !~ /^Signed-off-by$/i; > next if $suppress_cc{'bodycc'} and $what =~ /Cc/i; Looks good, FWIW I was curious if this could be: next if $suppress_cc{'misc-by'} and $what =~ /(?<!^Signed-off)-by$/; But found that as soon as you add a /i Perl will barf on it, and in any case makes sense to be less clever about regex features.
Rasmus Villemoes <rv@rasmusvillemoes.dk> writes: > @@ -1681,7 +1681,7 @@ sub process_file { > # Now parse the message body > while(<$fh>) { > $message .= $_; > - if (/^(Signed-off-by|Cc): (.*)/i) { > + if (/^([a-z-]*-by|Cc): (.*)/i) { So this picks up anything-by not just s-o-by, which sort of makes sense. > @@ -1691,7 +1691,9 @@ sub process_file { > if ($sc eq $sender) { > next if ($suppress_cc{'self'}); > } else { > - next if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i; We used to only grab CC or Signed-off-by (and specifically not something like "Not-Signed-off-by") upfront above, so matching /Signed-off-by/ was sufficient (it would have been sufficient to just look for 's'). But to suppress s-o-b and keep allowing via misc-by a trailer "Not-signed-off-by:", we now ... > + next if $suppress_cc{'sob'} and $what =~ /^Signed-off-by$/i; ... must make sure what we have is _exactly_ "signed-off-by" when 'sob' is suppressed. Makes sense. > + next if $suppress_cc{'misc-by'} > + and $what =~ /-by$/i and $what !~ /^Signed-off-by$/i; And this is the opposite side of the same coin, which also makes sense. I wonder if it would make it easier to grok if we made the logic inside out, i.e. if ($sc eq $sender) { ... } else { if ($what =~ /^Signed-off-by$/i) { next if $suppress_cc{'sob'}; } elsif ($what =~ /-by$/i) { next if $suppress_cc{'misc'}; } elsif ($what =~ /^Cc$/i) { next if $suppress_cc{'bodycc'}; } push @cc, $c; ... } > next if $suppress_cc{'bodycc'} and $what =~ /Cc/i; > } > if ($c !~ /.+@.+|<.+>/) {
On 2018-10-11 08:18, Junio C Hamano wrote: > Rasmus Villemoes <rv@rasmusvillemoes.dk> writes: > we now ... > >> + next if $suppress_cc{'sob'} and $what =~ /^Signed-off-by$/i; > > ... must make sure what we have is _exactly_ "signed-off-by" when > 'sob' is suppressed. Makes sense. > >> + next if $suppress_cc{'misc-by'} >> + and $what =~ /-by$/i and $what !~ /^Signed-off-by$/i; > > And this is the opposite side of the same coin, which also makes sense. Yup, I started by just adding the misc-by line, then remembered that people sometimes use not-signed-off-by variants, and went back and anchored the s-o-b case. So now it's no longer so minimal, and... > I wonder if it would make it easier to grok if we made the logic > inside out, i.e. > > if ($sc eq $sender) { > ... > } else { > if ($what =~ /^Signed-off-by$/i) { > next if $suppress_cc{'sob'}; > } elsif ($what =~ /-by$/i) { > next if $suppress_cc{'misc'}; > } elsif ($what =~ /^Cc$/i) { > next if $suppress_cc{'bodycc'};> } ...yes, that's probably more readable. Thanks, Rasmus
Rasmus Villemoes <rv@rasmusvillemoes.dk> writes: >> I wonder if it would make it easier to grok if we made the logic >> inside out, i.e. >> >> if ($sc eq $sender) { >> ... >> } else { >> if ($what =~ /^Signed-off-by$/i) { >> next if $suppress_cc{'sob'}; >> } elsif ($what =~ /-by$/i) { >> next if $suppress_cc{'misc'}; >> } elsif ($what =~ /^Cc$/i) { >> next if $suppress_cc{'bodycc'};> } > > ...yes, that's probably more readable. OK, unless there is more comments and suggestions for improvements, can you send in a final version sometime not in so distant future so that we won't forget? It may be surprising to existing users that the command now suddenly adds more addresses the user did not think would be added, but it would probably be easy enough for them to work around. I'll need to prepare a note in the draft release notes to describe backward (in)compatibility to warn users.
On 2018-10-16 07:57, Junio C Hamano wrote: > Rasmus Villemoes <rv@rasmusvillemoes.dk> writes: > >>> I wonder if it would make it easier to grok if we made the logic >>> inside out, i.e. >>> >>> if ($sc eq $sender) { >>> ... >>> } else { >>> if ($what =~ /^Signed-off-by$/i) { >>> next if $suppress_cc{'sob'}; >>> } elsif ($what =~ /-by$/i) { >>> next if $suppress_cc{'misc'}; >>> } elsif ($what =~ /^Cc$/i) { >>> next if $suppress_cc{'bodycc'};> } >> >> ...yes, that's probably more readable. > > OK, unless there is more comments and suggestions for improvements, > can you send in a final version sometime not in so distant future so > that we won't forget? Will do, I was just waiting for more comments to come in. It may be surprising to existing users that > the command now suddenly adds more addresses the user did not think > would be added, but it would probably be easy enough for them to > work around. Yeah, I thought about that, but unfortunately the whole auto-cc business is not built around some config options where we can add a new and default false. Also note that there are also cases currently where the user sends off a patch series and is surprised that lots of intended recipients were not cc'ed (that's how I picked this subject up again; I had a long series where I had put specific Cc's in each patch, at v2, some of those had given a Reviewed-by, so I changed the tags, and a --dry-run told me they wouldn't get the new version). I suppose one could make use of -by addresses dependent on a new opt-in config option, but that's not very elegant. Another option is, for a release or two, to make a little (more) noise when picking up a -by address - something like setting a flag in the ($what =~ /-by/) branch, and testing that flag somewhere in send_message(). But I suppose the message printed when needs_confirm eq "inform" is generic enough. Rasmus
Rasmus Villemoes <rv@rasmusvillemoes.dk> writes: >> It may be surprising to existing users that >> the command now suddenly adds more addresses the user did not think >> would be added, but it would probably be easy enough for them to >> work around. > > Yeah, I thought about that, but unfortunately the whole auto-cc business > is not built around some config options where we can add a new and > default false. Also note that there are also cases currently where the > user sends off a patch series and is surprised that lots of intended > recipients were not cc'ed (that's how I picked this subject up again; I That "also note ... people who are not familiar are surprised" is, quite honestly, irrelevant. The behaviour is documented, and the users are supposed to be used to it. Changing the behaviour in quite a different way from what existing users are used to is a very different matter. No matter how you cut it, change of behaviour like this is a regression for some existing users, while helping others, and it does not matter if it helps many more users than it hurts---a regression is a regression to those who are affected negatively. At least this is a deliberate one we are making, and I think it is OK as long as both the change in behaviour and the way to get back the old behaviour are advertised properly. Thanks.
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt index ea6ea512fe..f6010ac68b 100644 --- a/Documentation/git-send-email.txt +++ b/Documentation/git-send-email.txt @@ -329,8 +329,11 @@ Automating patch body (commit message) except for self (use 'self' for that). - 'sob' will avoid including anyone mentioned in Signed-off-by lines except for self (use 'self' for that). +- 'misc-by' will avoid including anyone mentioned in Acked-by, + Reviewed-by, Tested-by and other "-by" lines in the patch body, + except Signed-off-by (use 'sob' for that). - 'cccmd' will avoid running the --cc-cmd. -- 'body' is equivalent to 'sob' + 'bodycc'. +- 'body' is equivalent to 'sob' + 'bodycc' + 'misc-by'. - 'all' will suppress all auto cc values. -- + diff --git a/git-send-email.perl b/git-send-email.perl index 1916159d2a..7a6391e5d8 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -94,7 +94,7 @@ sub usage { --identity <str> * Use the sendemail.<id> options. --to-cmd <str> * Email To: via `<str> \$patch_path` --cc-cmd <str> * Email Cc: via `<str> \$patch_path` - --suppress-cc <str> * author, self, sob, cc, cccmd, body, bodycc, all. + --suppress-cc <str> * author, self, sob, cc, cccmd, body, bodycc, misc-by, all. --[no-]cc-cover * Email Cc: addresses in the cover letter. --[no-]to-cover * Email To: addresses in the cover letter. --[no-]signed-off-by-cc * Send to Signed-off-by: addresses. Default on. @@ -454,13 +454,13 @@ sub read_config { if (@suppress_cc) { foreach my $entry (@suppress_cc) { die sprintf(__("Unknown --suppress-cc field: '%s'\n"), $entry) - unless $entry =~ /^(?:all|cccmd|cc|author|self|sob|body|bodycc)$/; + unless $entry =~ /^(?:all|cccmd|cc|author|self|sob|body|bodycc|misc-by)$/; $suppress_cc{$entry} = 1; } } if ($suppress_cc{'all'}) { - foreach my $entry (qw (cccmd cc author self sob body bodycc)) { + foreach my $entry (qw (cccmd cc author self sob body bodycc misc-by)) { $suppress_cc{$entry} = 1; } delete $suppress_cc{'all'}; @@ -471,7 +471,7 @@ sub read_config { $suppress_cc{'sob'} = !$signed_off_by_cc if defined $signed_off_by_cc; if ($suppress_cc{'body'}) { - foreach my $entry (qw (sob bodycc)) { + foreach my $entry (qw (sob bodycc misc-by)) { $suppress_cc{$entry} = 1; } delete $suppress_cc{'body'}; @@ -1681,7 +1681,7 @@ sub process_file { # Now parse the message body while(<$fh>) { $message .= $_; - if (/^(Signed-off-by|Cc): (.*)/i) { + if (/^([a-z-]*-by|Cc): (.*)/i) { chomp; my ($what, $c) = ($1, $2); # strip garbage for the address we'll use: @@ -1691,7 +1691,9 @@ sub process_file { 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{'sob'} and $what =~ /^Signed-off-by$/i; + next if $suppress_cc{'misc-by'} + and $what =~ /-by$/i and $what !~ /^Signed-off-by$/i; next if $suppress_cc{'bodycc'} and $what =~ /Cc/i; } if ($c !~ /.+@.+|<.+>/) {
When rerolling a patch series, including various Reviewed-by etc. that may have come in, it is quite convenient to have git-send-email automatically cc those people. So pick up any *-by lines, with a new suppression category 'misc-by', but special-case Signed-off-by, since that already has its own suppression category. It seems natural to make 'misc-by' implied by 'body'. Based-on-patch-by: Joe Perches <joe@perches.com> Signed-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk> --- Documentation/git-send-email.txt | 5 ++++- git-send-email.perl | 14 ++++++++------ 2 files changed, 12 insertions(+), 7 deletions(-)