[PATCH] git-send-email: Cc more people

[PATCH] git-send-email: Cc more people

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

Several of my colleagues (and myself) have expressed surprise and
annoyance that git-send-email doesn't automatically pick up people who
are listed in patches as Reported-by: or Reviewed-by: or ... many other
tags that would seem (to us) to indicate that person might be interested.
This patch to git-send-email tries to pick up all Foo-by: tags.

Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>

diff --git a/git-send-email.perl b/git-send-email.perl
index 2fa7818ca..926815329 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1665,7 +1665,7 @@ foreach my $t (@files) {
 	# Now parse the message body
 	while(<$fh>) {
 		$message .=  $_;
-		if (/^(Signed-off-by|Cc): (.*)/i) {
+		if (/^([A-Z-a-z]*-by|Cc): (.*)/i) {
 			chomp;
 			my ($what, $c) = ($1, $2);
 			# strip garbage for the address we'll use:

Re: [PATCH] git-send-email: Cc more people

[Steven Rostedt]

On Wed, 18 Apr 2018 07:05:03 -0700
Matthew Wilcox <willy@infradead.org> wrote:

> From: Matthew Wilcox <mawilcox@microsoft.com>
> 
> Several of my colleagues (and myself) have expressed surprise and
> annoyance that git-send-email doesn't automatically pick up people who
> are listed in patches as Reported-by: or Reviewed-by: or ... many other
> tags that would seem (to us) to indicate that person might be interested.
> This patch to git-send-email tries to pick up all Foo-by: tags.

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Note, this is one of the reasons I still use quilt to send my email.
I've modified my quilt scripts to do what Matthew does here below.

-- Steve


> 
> Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> 
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 2fa7818ca..926815329 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1665,7 +1665,7 @@ foreach my $t (@files) {
>  	# Now parse the message body
>  	while(<$fh>) {
>  		$message .=  $_;
> -		if (/^(Signed-off-by|Cc): (.*)/i) {
> +		if (/^([A-Z-a-z]*-by|Cc): (.*)/i) {
>  			chomp;
>  			my ($what, $c) = ($1, $2);
>  			# strip garbage for the address we'll use:

Re: [PATCH] git-send-email: Cc more people

[Mathieu Desnoyers]

----- On Apr 18, 2018, at 10:33 AM, rostedt rostedt@goodmis.org wrote:

> On Wed, 18 Apr 2018 07:05:03 -0700
> Matthew Wilcox <willy@infradead.org> wrote:
> 
>> From: Matthew Wilcox <mawilcox@microsoft.com>
>> 
>> Several of my colleagues (and myself) have expressed surprise and
>> annoyance that git-send-email doesn't automatically pick up people who
>> are listed in patches as Reported-by: or Reviewed-by: or ... many other
>> tags that would seem (to us) to indicate that person might be interested.
>> This patch to git-send-email tries to pick up all Foo-by: tags.
> 
> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> 
> Note, this is one of the reasons I still use quilt to send my email.
> I've modified my quilt scripts to do what Matthew does here below.

Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

I find it really surprising and unexpected that people listed as
"Reviewed-by" don't end up being CC'd.

Thanks,

Mathieu

> 
> -- Steve
> 
> 
>> 
>> Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
>> 
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index 2fa7818ca..926815329 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -1665,7 +1665,7 @@ foreach my $t (@files) {
>>  	# Now parse the message body
>>  	while(<$fh>) {
>>  		$message .=  $_;
>> -		if (/^(Signed-off-by|Cc): (.*)/i) {
>> +		if (/^([A-Z-a-z]*-by|Cc): (.*)/i) {
>>  			chomp;
>>  			my ($what, $c) = ($1, $2);
> >  			# strip garbage for the address we'll use:

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH] git-send-email: Cc more people

[Ævar Arnfjörð Bjarmason]

On Wed, Apr 18 2018, Matthew Wilcox wrote:

> From: Matthew Wilcox <mawilcox@microsoft.com>
>
> Several of my colleagues (and myself) have expressed surprise and
> annoyance that git-send-email doesn't automatically pick up people who
> are listed in patches as Reported-by: or Reviewed-by: or ... many other
> tags that would seem (to us) to indicate that person might be interested.
> This patch to git-send-email tries to pick up all Foo-by: tags.
>
> Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 2fa7818ca..926815329 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1665,7 +1665,7 @@ foreach my $t (@files) {
>  	# Now parse the message body
>  	while(<$fh>) {
>  		$message .=  $_;
> -		if (/^(Signed-off-by|Cc): (.*)/i) {
> +		if (/^([A-Z-a-z]*-by|Cc): (.*)/i) {
>  			chomp;
>  			my ($what, $c) = ($1, $2);
>  			# strip garbage for the address we'll use:

I like this direction, I've actually been meaning to take this further
and try to parse out SHA1s in the commit message, look those up, and add
their authors to CC one of these days.

But IMO this patch is really lacking a few things before being ready:

1. You have no tests for this. See t/t9001-send-email.sh for examples,
   i.e. stuff like

       (body) Adding cc: C O Mitter <committer@example.com> from line
       'Signed-off-by: C O Mitter <committer@example.com>'

   Should have corresponding tests for "Reviewed-by" "Seen-by"
   etc. These are easy to add, just edit the raw messages and test that
   for the output about adding CCs.

2. Just a few lines down from your quoted hunk we have this:

	# strip garbage for the address we'll use:
	$c = strip_garbage_one_address($c);
	# sanitize a bit more to decide whether to suppress the address:
	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;
	}
	push @cc, $c;
	printf(__("(body) Adding cc: %s from line '%s'\n"),
		$c, $_) unless $quiet;

   So before we just supported Signed-off-by as a special case, but now
   your patch adds WHAT-EVER-by without updating the the corresponding
   --[no-]signed-off-by-cc command-line options.

   Your change should at least describe why those aren't being updated,
   but probably we should add some other command-line option for
   ignoring these wildcards, e.g. --[no-]wildcard-by-cc=reviewed
   --[no-]wildcard-by-cc=seen etc, and we can make --[no-]signed-off-by
   a historical alias for --[no-]wildcard-by-cc=signed-off.

3. Ditto all the documentation in "man git-send-email" about
   "signed-off-by", "sob" etc, and the "signedoffbycc" variable
   documented both there and in "man git-config".

Style comment: First time I've seen someone write a charclass as
[A-Z-a-z] and mean it, usually it's usually it's [A-Za-z-] to clarify
that the "-" isn't a range. Makes sense (to me) to have ranges first &
stray chars last.

Re: [PATCH] git-send-email: Cc more people

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> But IMO this patch is really lacking a few things before being ready:
>
> 1. You have no tests for this. See t/t9001-send-email.sh for examples,
> ...
> 2. Just a few lines down from your quoted hunk we have this:
> ... code about $supress_cc{<token>} ...
>    Your change should at least describe why those aren't being updated,
>    but probably we should add some other command-line option for
>    ignoring these wildcards, e.g. --[no-]wildcard-by-cc=reviewed
>    --[no-]wildcard-by-cc=seen etc, and we can make --[no-]signed-off-by
>    a historical alias for --[no-]wildcard-by-cc=signed-off.
> 3. Ditto all the documentation in "man git-send-email" about
> ...

Thanks, I agree that 2. (the lack of suppression) is a showstopper.
I'd further say that these new CC-sources should be disabled by
default and made opt-in to avoid surprising existing users.

One thing we also need to be very careful about is that some of the
fields may not even have an e-mail address.  We can expect that
S-o-b and Cc would be of form "human readable name <email@addre.ss>"
by their nature, but it is perfectly fine to write only human
readable name without address on random lines like "suggeted-by" and
"helped-by".  There needs a way for the end-user to avoid using data
found on such lines as if they are valid e-mail addresses.

Re: [PATCH] git-send-email: Cc more people

[Matthew Wilcox]

On Thu, Apr 19, 2018 at 06:21:42AM +0900, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
> > But IMO this patch is really lacking a few things before being ready:
> >
> > 1. You have no tests for this. See t/t9001-send-email.sh for examples,
> > ...
> > 2. Just a few lines down from your quoted hunk we have this:
> > ... code about $supress_cc{<token>} ...
> >    Your change should at least describe why those aren't being updated,
> >    but probably we should add some other command-line option for
> >    ignoring these wildcards, e.g. --[no-]wildcard-by-cc=reviewed
> >    --[no-]wildcard-by-cc=seen etc, and we can make --[no-]signed-off-by
> >    a historical alias for --[no-]wildcard-by-cc=signed-off.
> > 3. Ditto all the documentation in "man git-send-email" about
> > ...
> 
> Thanks, I agree that 2. (the lack of suppression) is a showstopper.

I agree with that (and the lack of tests, obviously)

> I'd further say that these new CC-sources should be disabled by
> default and made opt-in to avoid surprising existing users.

But I disagree with this.  The current behaviour is surprising to
existing users, to the point where people are writing their own scripts
to replace git send-email (which seems crazy to me).

> One thing we also need to be very careful about is that some of the
> fields may not even have an e-mail address.  We can expect that
> S-o-b and Cc would be of form "human readable name <email@addre.ss>"
> by their nature, but it is perfectly fine to write only human
> readable name without address on random lines like "suggeted-by" and
> "helped-by".  There needs a way for the end-user to avoid using data
> found on such lines as if they are valid e-mail addresses.

I also agree with this.  I'll add some test-cases and make sure we only
add these if they're valid email addresses.

Re: [PATCH] git-send-email: Cc more people

[Mathieu Desnoyers]

----- On Apr 19, 2018, at 8:10 AM, Matthew Wilcox willy@infradead.org wrote:

> On Thu, Apr 19, 2018 at 06:21:42AM +0900, Junio C Hamano wrote:
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>> 
>> > But IMO this patch is really lacking a few things before being ready:
>> >
>> > 1. You have no tests for this. See t/t9001-send-email.sh for examples,
>> > ...
>> > 2. Just a few lines down from your quoted hunk we have this:
>> > ... code about $supress_cc{<token>} ...
>> >    Your change should at least describe why those aren't being updated,
>> >    but probably we should add some other command-line option for
>> >    ignoring these wildcards, e.g. --[no-]wildcard-by-cc=reviewed
>> >    --[no-]wildcard-by-cc=seen etc, and we can make --[no-]signed-off-by
>> >    a historical alias for --[no-]wildcard-by-cc=signed-off.
>> > 3. Ditto all the documentation in "man git-send-email" about
>> > ...
>> 
>> Thanks, I agree that 2. (the lack of suppression) is a showstopper.
> 
> I agree with that (and the lack of tests, obviously)
> 
>> I'd further say that these new CC-sources should be disabled by
>> default and made opt-in to avoid surprising existing users.
> 
> But I disagree with this.  The current behaviour is surprising to
> existing users, to the point where people are writing their own scripts
> to replace git send-email (which seems crazy to me).

We could perhaps go with a whitelist approach. The four
main match I would be tempted to add are: Acked-by, Reported-by,
Reviewed-by, and Tested-by.

My workflow is to initially CC a bunch of relevant maintainers
when sending out a patch, and as the Acked, Reviewed and Tested
by tags come it, I replace those CC with the relevant tag.
I never expected them to stop being CC'd when switching between
those categories.

Thanks,

Mathieu

> 
>> One thing we also need to be very careful about is that some of the
>> fields may not even have an e-mail address.  We can expect that
>> S-o-b and Cc would be of form "human readable name <email@addre.ss>"
>> by their nature, but it is perfectly fine to write only human
>> readable name without address on random lines like "suggeted-by" and
>> "helped-by".  There needs a way for the end-user to avoid using data
>> found on such lines as if they are valid e-mail addresses.
> 
> I also agree with this.  I'll add some test-cases and make sure we only
> add these if they're valid email addresses.

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH] git-send-email: Cc more people

[Junio C Hamano]

Mathieu Desnoyers <mathieu.desnoyers@efficios.com> writes:

>>> I'd further say that these new CC-sources should be disabled by
>>> default and made opt-in to avoid surprising existing users.
>> 
>> But I disagree with this.  The current behaviour is surprising to
>> existing users, to the point where people are writing their own scripts
>> to replace git send-email (which seems crazy to me).
>
> We could perhaps go with a whitelist approach. The four
> main match I would be tempted to add are: Acked-by, Reported-by,
> Reviewed-by, and Tested-by.

A tool that suddenly starts sending e-mails to more addresses
without letting the end-users know when and why the change in
behaviour happened is a source of irritated "somebody made a stupid
change to git-send-email without telling us that caused unwanted
e-mails sent to unexpected places and embarrassed me" bug reports.
I do agree with a whitelist approach from that point of view, and in
the initial rollout of the feature, that whitelist should be limited
to what we already send out.

The users who learn about this new feature can opt into whitelisting
the common 4 above before we enable them by default.  FWIW, I
personally think these will be a sensible default (in addition to
what we already Cc).  I however prefer an approach to introduce
these more gradually.

Re: [PATCH] git-send-email: Cc more people

[Mathieu Desnoyers]

----- On Apr 19, 2018, at 8:03 PM, Junio C Hamano gitster@pobox.com wrote:

> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> writes:
> 
>>>> I'd further say that these new CC-sources should be disabled by
>>>> default and made opt-in to avoid surprising existing users.
>>> 
>>> But I disagree with this.  The current behaviour is surprising to
>>> existing users, to the point where people are writing their own scripts
>>> to replace git send-email (which seems crazy to me).
>>
>> We could perhaps go with a whitelist approach. The four
>> main match I would be tempted to add are: Acked-by, Reported-by,
>> Reviewed-by, and Tested-by.
> 
> A tool that suddenly starts sending e-mails to more addresses
> without letting the end-users know when and why the change in
> behaviour happened is a source of irritated "somebody made a stupid
> change to git-send-email without telling us that caused unwanted
> e-mails sent to unexpected places and embarrassed me" bug reports.
> I do agree with a whitelist approach from that point of view, and in
> the initial rollout of the feature, that whitelist should be limited
> to what we already send out.
> 
> The users who learn about this new feature can opt into whitelisting
> the common 4 above before we enable them by default.  FWIW, I
> personally think these will be a sensible default (in addition to
> what we already Cc).  I however prefer an approach to introduce
> these more gradually.

Sure, introducing changes like this needs to be done gradually.

Thanks!

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com