[PATCH 0/3] send-email: Also pick up cc addresses from -by trailers

[PATCH 0/3] send-email: Also pick up cc addresses from -by trailers

[Rasmus Villemoes]

This has been attempted multiple times before, but I hope that this
can make it in this time around. That *-by addresses are not
automatically Cc'ed certainly still surprises people from time to
time.

I hope that this addresses all the concerns Junio had in
https://lkml.org/lkml/2016/8/31/768 .

For the name, I chose 'misc-by', since that has -by in its name. I am
fine with absolutely any other name (bodyby, body-by, by-trailers,
...). I doubt we can find a short token that is completely
self-explanatory, and note that one has to look in the man page anyway
to know what 'sob' means in this line from 'git send-email -h':

    --suppress-cc           <str>  * author, self, sob, cc, cccmd, body, bodycc, all.

Rasmus Villemoes (3):
  Documentation/git-send-email.txt: style fixes
  send-email: only consider lines containing @ or <> for automatic
    Cc'ing
  send-email: also pick up cc addresses from -by trailers

 Documentation/git-send-email.txt | 11 +++++++----
 git-send-email.perl              | 19 +++++++++++++------
 2 files changed, 20 insertions(+), 10 deletions(-)

-- 
2.19.1.6.g084f1d7761

[PATCH 1/3] Documentation/git-send-email.txt: style fixes

[Rasmus Villemoes]

For consistency, add full stops in a few places and outdent a line by
one space.

Signed-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>
---
 Documentation/git-send-email.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 465a4ecbed..ea6ea512fe 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -321,16 +321,16 @@ Automating
 	auto-cc of:
 +
 --
-- 'author' will avoid including the patch author
-- 'self' will avoid including the sender
+- 'author' will avoid including the patch author.
+- 'self' will avoid including the sender.
 - 'cc' will avoid including anyone mentioned in Cc lines in the patch header
   except for self (use 'self' for that).
 - 'bodycc' will avoid including anyone mentioned in Cc lines in the
   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).
+  for self (use 'self' for that).
 - 'cccmd' will avoid running the --cc-cmd.
-- 'body' is equivalent to 'sob' + 'bodycc'
+- 'body' is equivalent to 'sob' + 'bodycc'.
 - 'all' will suppress all auto cc values.
 --
 +
-- 
2.19.1.6.g084f1d7761

[PATCH 2/3] send-email: only consider lines containing @ or <> for automatic Cc'ing

[Rasmus Villemoes]

While the address sanitizations routines do accept local addresses, that
is almost never what is meant in a Cc or Signed-off-by trailer.

Looking through all the signed-off-by lines in the linux kernel tree
without a @, there are mostly two patterns: Either just a full name, or
a full name followed by <user at domain.com> (i.e., with the word at
instead of a @), and minor variations. For cc lines, the same patterns
appear, along with lots of "cc stable" variations that do not actually
name stable@vger.kernel.org

  Cc: stable # introduced pre-git times
  cc: stable.kernel.org

In the <user at domain.com> cases, one gets a chance to interactively
fix it. But when there is no <> pair, it seems we end up just using the
first word as a (local) address.

As the number of cases where a local address really was meant is
likely (and anecdotally) quite small compared to the number of cases
where we end up cc'ing a garbage address, insist on at least a @ or a <>
pair being present.

This is also preparation for the next patch, where we are likely to
encounter even more non-addresses in -by lines, such as

  Reported-by: Coverity
  Patch-generated-by: Coccinelle

Signed-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>
---
 git-send-email.perl | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/git-send-email.perl b/git-send-email.perl
index 2be5dac337..1916159d2a 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1694,6 +1694,11 @@ sub process_file {
 				next if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i;
 				next if $suppress_cc{'bodycc'} and $what =~ /Cc/i;
 			}
+			if ($c !~ /.+@.+|<.+>/) {
+				printf("(body) Ignoring %s from line '%s'\n",
+					$what, $_) unless $quiet;
+				next;
+			}
 			push @cc, $c;
 			printf(__("(body) Adding cc: %s from line '%s'\n"),
 				$c, $_) unless $quiet;
-- 
2.19.1.6.g084f1d7761

Re: [PATCH 2/3] send-email: only consider lines containing @ or <> for automatic Cc'ing

[Ævar Arnfjörð Bjarmason]

On Wed, Oct 10 2018, Rasmus Villemoes wrote:

> +			if ($c !~ /.+@.+|<.+>/) {
> +				printf("(body) Ignoring %s from line '%s'\n",
> +					$what, $_) unless $quiet;
> +				next;
> +			}
>  			push @cc, $c;
>  			printf(__("(body) Adding cc: %s from line '%s'\n"),
>  				$c, $_) unless $quiet;

There's a extract_valid_address() function in git-send-email already,
shouldn't this be:

    if (!extract_valid_address($c)) {
    [...]

Or is there a good reason not to use that function in this case?

Re: [PATCH 2/3] send-email: only consider lines containing @ or <> for automatic Cc'ing

[Rasmus Villemoes]

On 2018-10-10 14:57, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Oct 10 2018, Rasmus Villemoes wrote:
> 
>> +			if ($c !~ /.+@.+|<.+>/) {
>> +				printf("(body) Ignoring %s from line '%s'\n",
>> +					$what, $_) unless $quiet;
>> +				next;
>> +			}
>>  			push @cc, $c;
>>  			printf(__("(body) Adding cc: %s from line '%s'\n"),
>>  				$c, $_) unless $quiet;
> 
> There's a extract_valid_address() function in git-send-email already,
> shouldn't this be:
> 
>     if (!extract_valid_address($c)) {
>     [...]
> 
> Or is there a good reason not to use that function in this case?
> 

I considered that (and also had a version where I simply insisted on a @
being present), but that means the user no longer would get prompted
about the cases where the address was just slightly obfuscated, e.g. the

Cc: John Doe <john at doe.com>

cases, which would be a regression, I guess. So I do want to pass such
cases through, and have them be dealt with when process_address_list
gets called.

So this is just a rather minimal and simple heuristic, which should
still be able to handle the vast majority of cases correctly, and at
least almost never exclude anything that might have a chance of becoming
a real address.

Rasmus

Re: [PATCH 2/3] send-email: only consider lines containing @ or <> for automatic Cc'ing

[Junio C Hamano]

Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:

> I considered that (and also had a version where I simply insisted on a @
> being present), but that means the user no longer would get prompted
> about the cases where the address was just slightly obfuscated, e.g. the
>
> Cc: John Doe <john at doe.com>
>
> cases, which would be a regression, I guess. So I do want to pass such
> cases through, and have them be dealt with when process_address_list
> gets called.

We are only tightening with this patch, and we were passing any
random things through with the original code anyway, so without
[PATCH 3/3], this step must be making it only better, but I have to
wonder one thing.

You keep saying "get prompted" but are we sure we always stop and
ask (and preferrably---fail and abort when the end user is not
available at the terminal to interact) when we have such a
questionable address?

Re: [PATCH 2/3] send-email: only consider lines containing @ or <> for automatic Cc'ing

[Rasmus Villemoes]

On 2018-10-11 08:06, Junio C Hamano wrote:
> Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:
> 
>> I considered that (and also had a version where I simply insisted on a @
>> being present), but that means the user no longer would get prompted
>> about the cases where the address was just slightly obfuscated, e.g. the
>>
>> Cc: John Doe <john at doe.com>
>>
>> cases, which would be a regression, I guess. So I do want to pass such
>> cases through, and have them be dealt with when process_address_list
>> gets called.
> 
> We are only tightening with this patch, and we were passing any
> random things through with the original code anyway, so without
> [PATCH 3/3], this step must be making it only better, but I have to
> wonder one thing.
> 
> You keep saying "get prompted" but are we sure we always stop and
> ask (and preferrably---fail and abort when the end user is not
> available at the terminal to interact) when we have such a
> questionable address?
> 

I dunno. I guess I've never considered non-interactive use of
send-email. But the ask() in validate_address does have default q[uit],
which I suppose gets used if stdin is /dev/null? I did do an experiment
adding a bunch of the random odd patterns found in kernel commit
messages to see how send-email reacted before/after this, and the only
things that got filtered away (i.e., no longer prompted about) were
things where the user probably couldn't easily fix it anyway. In the
cases where there was a "Cc: stable" that might be fixed to the proper
stable@vger.kernel.org, the logic in extract_valid_address simply saw
that as a local address, so we didn't use to be prompted, but simply
sent to stable@localhost. Now we simply don't pass that through. So, for
non-interactive use, I guess the effect of this patch is to allow more
cases to complete succesfully, since we filter away (some) cases where
extract_valid_address would cause us to prompt (and thus quit).

So, it seems you're ok with this tightening, but some comment on the
non-interactive use case should be made in the commit log? Or am I
misunderstanding?

Thanks,
Rasmus

Re: [PATCH 2/3] send-email: only consider lines containing @ or <> for automatic Cc'ing

[Junio C Hamano]

Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:

> So, it seems you're ok with this tightening, but some comment on the
> non-interactive use case should be made in the commit log? Or am I
> misunderstanding?

I do not think we need any immediate action on this step.  I was
just wondering if we want two classes of "I am not running you
interactively, so assume I said 'yes' when you need to ask me any
confirmation on X and Y" and "I am not running you interactively,
so assume I said 'no' for safety when you need to ask me any
confirmation on Z" supported in the future.  Lines with both @ and
<> fall into the first class, while lines with only <> fall into the
second camp, I would guess.

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

[Rasmus Villemoes]

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(-)

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

Re: [PATCH 3/3] send-email: also pick up cc addresses from -by trailers

[Ævar Arnfjörð Bjarmason]

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.

Re: [PATCH 3/3] send-email: also pick up cc addresses from -by trailers

[Junio C Hamano]

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

Re: [PATCH 3/3] send-email: also pick up cc addresses from -by trailers

[Rasmus Villemoes]

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

Re: [PATCH 3/3] send-email: also pick up cc addresses from -by trailers

[Junio C Hamano]

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.

Re: [PATCH 3/3] send-email: also pick up cc addresses from -by trailers

[Rasmus Villemoes]

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

Re: [PATCH 3/3] send-email: also pick up cc addresses from -by trailers

[Junio C Hamano]

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 v2 0/3] send-email: Also pick up cc addresses from -by trailers

[Rasmus Villemoes]

This series extends the logic in git-send-email so that addresses
appearing in various *-by: trailers (e.g. Reviewed-by, Acked-by,
Tested-by) are picked up and added to the Cc list, in addition to the
current logic that picks up Cc: and Signed-off-by: lines.

This deliberately only picks up -by trailers (as opposed to any
trailer), based on the heuristic that the -by suffix strongly suggests
there's a (name +) email address after the colon. This avoids having
to deal with BugID:, Link:, or other such tags. Still, widening to any
-by trailer does increase the risk that we will pick up stuff that is
not an email address, such as

  Reported-by: Coverity
  Patch-generated-by: Coccinelle

where send-email then ends up cc'ing the local 'coverity' user. Patch
2 tries to weed out the obvious no-email-address-here cases, which
should also help avoid cc'ing garbage (local) addresses for malformed
cc and signed-off-by lines. Patch 3 is then mostly mechanical,
introducing the misc-by suppression category and changing the regexp
for matching trailer lines to include .*-by.

Changes in v2: Rework logic in patch 3 as suggested by Junio.

v1 cover letter:

This has been attempted multiple times before, but I hope that this
can make it in this time around. That *-by addresses are not
automatically Cc'ed certainly still surprises people from time to
time.

I hope that this addresses all the concerns Junio had in
https://lkml.org/lkml/2016/8/31/768 .

For the name, I chose 'misc-by', since that has -by in its name. I am
fine with absolutely any other name (bodyby, body-by, by-trailers,
...). I doubt we can find a short token that is completely
self-explanatory, and note that one has to look in the man page anyway
to know what 'sob' means in this line from 'git send-email -h':

    --suppress-cc           <str>  * author, self, sob, cc, cccmd, body, bodycc, all.


Rasmus Villemoes (3):
  Documentation/git-send-email.txt: style fixes
  send-email: only consider lines containing @ or <> for automatic
    Cc'ing
  send-email: also pick up cc addresses from -by trailers

 Documentation/git-send-email.txt | 11 +++++++----
 git-send-email.perl              | 24 +++++++++++++++++-------
 2 files changed, 24 insertions(+), 11 deletions(-)

-- 
2.19.1.6.gbde171bbf5

[PATCH v2 1/3] Documentation/git-send-email.txt: style fixes

[Rasmus Villemoes]

For consistency, add full stops in a few places and outdent a line by
one space.

Signed-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>
---
 Documentation/git-send-email.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 465a4ecbed..ea6ea512fe 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -321,16 +321,16 @@ Automating
 	auto-cc of:
 +
 --
-- 'author' will avoid including the patch author
-- 'self' will avoid including the sender
+- 'author' will avoid including the patch author.
+- 'self' will avoid including the sender.
 - 'cc' will avoid including anyone mentioned in Cc lines in the patch header
   except for self (use 'self' for that).
 - 'bodycc' will avoid including anyone mentioned in Cc lines in the
   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).
+  for self (use 'self' for that).
 - 'cccmd' will avoid running the --cc-cmd.
-- 'body' is equivalent to 'sob' + 'bodycc'
+- 'body' is equivalent to 'sob' + 'bodycc'.
 - 'all' will suppress all auto cc values.
 --
 +
-- 
2.19.1.6.gbde171bbf5

[PATCH v2 2/3] send-email: only consider lines containing @ or <> for automatic Cc'ing

[Rasmus Villemoes]

While the address sanitizations routines do accept local addresses, that
is almost never what is meant in a Cc or Signed-off-by trailer.

Looking through all the signed-off-by lines in the linux kernel tree
without a @, there are mostly two patterns: Either just a full name, or
a full name followed by <user at domain.com> (i.e., with the word at
instead of a @), and minor variations. For cc lines, the same patterns
appear, along with lots of "cc stable" variations that do not actually
name stable@vger.kernel.org

  Cc: stable # introduced pre-git times
  cc: stable.kernel.org

In the <user at domain.com> cases, one gets a chance to interactively
fix it. But when there is no <> pair, it seems we end up just using the
first word as a (local) address.

As the number of cases where a local address really was meant is
likely (and anecdotally) quite small compared to the number of cases
where we end up cc'ing a garbage address, insist on at least a @ or a <>
pair being present.

This is also preparation for the next patch, where we are likely to
encounter even more non-addresses in -by lines, such as

  Reported-by: Coverity
  Patch-generated-by: Coccinelle

Signed-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>
---
 git-send-email.perl | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/git-send-email.perl b/git-send-email.perl
index 2be5dac337..1916159d2a 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1694,6 +1694,11 @@ sub process_file {
 				next if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i;
 				next if $suppress_cc{'bodycc'} and $what =~ /Cc/i;
 			}
+			if ($c !~ /.+@.+|<.+>/) {
+				printf("(body) Ignoring %s from line '%s'\n",
+					$what, $_) unless $quiet;
+				next;
+			}
 			push @cc, $c;
 			printf(__("(body) Adding cc: %s from line '%s'\n"),
 				$c, $_) unless $quiet;
-- 
2.19.1.6.gbde171bbf5

[PATCH v2 3/3] send-email: also pick up cc addresses from -by trailers

[Rasmus Villemoes]

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              | 19 ++++++++++++-------
 2 files changed, 16 insertions(+), 8 deletions(-)

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..58c6aa9d0e 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,8 +1691,13 @@ 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{'bodycc'} and $what =~ /Cc/i;
+				if ($what =~ /^Signed-off-by$/i) {
+					next if $suppress_cc{'sob'};
+				} elsif ($what =~ /-by$/i) {
+					next if $suppress_cc{'misc-by'};
+				} elsif ($what =~ /Cc/i) {
+					next if $suppress_cc{'bodycc'};
+				}
 			}
 			if ($c !~ /.+@.+|<.+>/) {
 				printf("(body) Ignoring %s from line '%s'\n",
-- 
2.19.1.6.gbde171bbf5

Re: [PATCH v2 0/3] send-email: Also pick up cc addresses from -by trailers

[Junio C Hamano]

Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:

> This series extends the logic in git-send-email so that addresses
> appearing in various *-by: trailers (e.g. Reviewed-by, Acked-by,
> Tested-by) are picked up and added to the Cc list, in addition to the
> current logic that picks up Cc: and Signed-off-by: lines.

Thanks.  Will replace.  I think this is ready for 'next' so let's
see if somebody else have more comments for a few days and then
start merging it down.