patch, 3 partssend-email: Also pick up cc addresses from -by trailers

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

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

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

On Wed, Oct 10 2018, Rasmus Villemoes wrote:

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?

On 2018-10-10 14:57, Ævar Arnfjörð Bjarmason wrote:

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

Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:

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?

On 2018-10-11 08:06, Junio C Hamano wrote:

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

Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:

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.

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

On Wed, Oct 10 2018, Rasmus Villemoes wrote:

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:

So this picks up anything-by not just s-o-by, which sort of makes sense.

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 ...

... must make sure what we have is _exactly_ "signed-off-by" when 'sob' is suppressed. Makes sense.

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;
		...
	}

On 2018-10-11 08:18, Junio C Hamano wrote:

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...

...yes, that's probably more readable.

Thanks, Rasmus

Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:

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:

Will do, I was just waiting for more comments to come in.

 It may be surprising to existing users that

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:

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.

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

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

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

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

Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:

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.