[3/3] send-email: also pick up cc addresses from -by trailers
diff mbox series

Message ID 20181010111351.5045-4-rv@rasmusvillemoes.dk
State New
Headers show
Series
  • send-email: Also pick up cc addresses from -by trailers
Related show

Commit Message

Rasmus Villemoes Oct. 10, 2018, 11:13 a.m. UTC
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(-)

Comments

Ævar Arnfjörð Bjarmason Oct. 10, 2018, 12:51 p.m. UTC | #1
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.
Junio C Hamano Oct. 11, 2018, 6:18 a.m. UTC | #2
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 !~ /.+@.+|<.+>/) {
Rasmus Villemoes Oct. 11, 2018, 7:11 a.m. UTC | #3
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
Junio C Hamano Oct. 16, 2018, 5:57 a.m. UTC | #4
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.
Rasmus Villemoes Oct. 16, 2018, 7:17 a.m. UTC | #5
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
Junio C Hamano Oct. 16, 2018, 7:46 a.m. UTC | #6
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.

Patch
diff mbox series

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 !~ /.+@.+|<.+>/) {