{"thread":{"id":"48319","subject":"[PATCH] git-send-email: Cc more people","startedAt":"2018-04-18T14:05:09Z","lastAt":"2018-04-20T15:27:44Z","messageCount":9,"participants":["Matthew Wilcox","Steven Rostedt","Mathieu Desnoyers","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"344983","messageId":"20180418140503.GD27475@bombadil.infradead.org","threadId":"48319","inReplyTo":null,"subject":"[PATCH] git-send-email: Cc more people","fromName":"Matthew Wilcox","fromEmail":"willy@infradead.org","sentAt":"2018-04-18T14:05:03Z","receivedAt":"2018-04-18T14:05:09Z","isPatch":true,"sender":{"key":"willy@infradead.org","avatar":null},"body":"From: Matthew Wilcox \n\nSeveral of my colleagues (and myself) have expressed surprise and\nannoyance that git-send-email doesn't automatically pick up people who\nare listed in patches as Reported-by: or Reviewed-by: or ... many other\ntags that would seem (to us) to indicate that person might be interested.\nThis patch to git-send-email tries to pick up all Foo-by: tags.\n\nSigned-off-by: Matthew Wilcox \n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 2fa7818ca..926815329 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1665,7 +1665,7 @@ foreach my $t (@files) {\n \t# Now parse the message body\n \twhile(<$fh>) {\n \t\t$message .= $_;\n-\t\tif (/^(Signed-off-by|Cc): (.*)/i) {\n+\t\tif (/^([A-Z-a-z]*-by|Cc): (.*)/i) {\n \t\t\tchomp;\n \t\t\tmy ($what, $c) = ($1, $2);\n \t\t\t# strip garbage for the address we'll use:\n\n"},{"id":"344985","messageId":"20180418103339.30bae9bc@gandalf.local.home","threadId":"48319","inReplyTo":"20180418140503.GD27475@bombadil.infradead.org","subject":"Re: [PATCH] git-send-email: Cc more people","fromName":"Steven Rostedt","fromEmail":"rostedt@goodmis.org","sentAt":"2018-04-18T14:33:39Z","receivedAt":"2018-04-18T14:33:46Z","isPatch":true,"sender":{"key":"rostedt@goodmis.org","avatar":"https://gravatar.com/avatar/cc188bf330d625ec6a7a2d0b6f4829dc777963e8dab83d943691dc31c5095227?d=mp&s=160"},"body":"On Wed, 18 Apr 2018 07:05:03 -0700\nMatthew Wilcox wrote:\n\n> From: Matthew Wilcox \n> \n> Several of my colleagues (and myself) have expressed surprise and\n> annoyance that git-send-email doesn't automatically pick up people who\n> are listed in patches as Reported-by: or Reviewed-by: or ... many other\n> tags that would seem (to us) to indicate that person might be interested.\n> This patch to git-send-email tries to pick up all Foo-by: tags.\n\nAcked-by: Steven Rostedt (VMware) \n\nNote, this is one of the reasons I still use quilt to send my email.\nI've modified my quilt scripts to do what Matthew does here below.\n\n-- Steve\n\n\n> \n> Signed-off-by: Matthew Wilcox \n> \n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 2fa7818ca..926815329 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -1665,7 +1665,7 @@ foreach my $t (@files) {\n> \t# Now parse the message body\n> \twhile(<$fh>) {\n> \t\t$message .= $_;\n> -\t\tif (/^(Signed-off-by|Cc): (.*)/i) {\n> +\t\tif (/^([A-Z-a-z]*-by|Cc): (.*)/i) {\n> \t\t\tchomp;\n> \t\t\tmy ($what, $c) = ($1, $2);\n> \t\t\t# strip garbage for the address we'll use:\n\n"},{"id":"344996","messageId":"1481207245.12764.1524072308610.JavaMail.zimbra@efficios.com","threadId":"48319","inReplyTo":"20180418103339.30bae9bc@gandalf.local.home","subject":"Re: [PATCH] git-send-email: Cc more people","fromName":"Mathieu Desnoyers","fromEmail":"mathieu.desnoyers@efficios.com","sentAt":"2018-04-18T17:25:08Z","receivedAt":"2018-04-18T17:25:14Z","isPatch":true,"sender":{"key":"mathieu.desnoyers@efficios.com","avatar":"https://gravatar.com/avatar/c3872f90e251e72cba6a65443eb6a4144e73ab1b2e08149cf78eb5d4779a88dd?d=mp&s=160"},"body":"----- On Apr 18, 2018, at 10:33 AM, rostedt rostedt@goodmis.org wrote:\n\n> On Wed, 18 Apr 2018 07:05:03 -0700\n> Matthew Wilcox wrote:\n> \n>> From: Matthew Wilcox \n>> \n>> Several of my colleagues (and myself) have expressed surprise and\n>> annoyance that git-send-email doesn't automatically pick up people who\n>> are listed in patches as Reported-by: or Reviewed-by: or ... many other\n>> tags that would seem (to us) to indicate that person might be interested.\n>> This patch to git-send-email tries to pick up all Foo-by: tags.\n> \n> Acked-by: Steven Rostedt (VMware) \n> \n> Note, this is one of the reasons I still use quilt to send my email.\n> I've modified my quilt scripts to do what Matthew does here below.\n\nAcked-by: Mathieu Desnoyers \n\nI find it really surprising and unexpected that people listed as\n\"Reviewed-by\" don't end up being CC'd.\n\nThanks,\n\nMathieu\n\n> \n> -- Steve\n> \n> \n>> \n>> Signed-off-by: Matthew Wilcox \n>> \n>> diff --git a/git-send-email.perl b/git-send-email.perl\n>> index 2fa7818ca..926815329 100755\n>> --- a/git-send-email.perl\n>> +++ b/git-send-email.perl\n>> @@ -1665,7 +1665,7 @@ foreach my $t (@files) {\n>> \t# Now parse the message body\n>> \twhile(<$fh>) {\n>> \t\t$message .= $_;\n>> -\t\tif (/^(Signed-off-by|Cc): (.*)/i) {\n>> +\t\tif (/^([A-Z-a-z]*-by|Cc): (.*)/i) {\n>> \t\t\tchomp;\n>> \t\t\tmy ($what, $c) = ($1, $2);\n> > \t\t\t# strip garbage for the address we'll use:\n\n-- \nMathieu Desnoyers\nEfficiOS Inc.\nhttp://www.efficios.com\n"},{"id":"345007","messageId":"87tvs8e174.fsf@evledraar.gmail.com","threadId":"48319","inReplyTo":"20180418140503.GD27475@bombadil.infradead.org","subject":"Re: [PATCH] git-send-email: Cc more people","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-04-18T19:58:39Z","receivedAt":"2018-04-18T19:58:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Apr 18 2018, Matthew Wilcox wrote:\n\n> From: Matthew Wilcox \n>\n> Several of my colleagues (and myself) have expressed surprise and\n> annoyance that git-send-email doesn't automatically pick up people who\n> are listed in patches as Reported-by: or Reviewed-by: or ... many other\n> tags that would seem (to us) to indicate that person might be interested.\n> This patch to git-send-email tries to pick up all Foo-by: tags.\n>\n> Signed-off-by: Matthew Wilcox \n>\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 2fa7818ca..926815329 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -1665,7 +1665,7 @@ foreach my $t (@files) {\n> \t# Now parse the message body\n> \twhile(<$fh>) {\n> \t\t$message .= $_;\n> -\t\tif (/^(Signed-off-by|Cc): (.*)/i) {\n> +\t\tif (/^([A-Z-a-z]*-by|Cc): (.*)/i) {\n> \t\t\tchomp;\n> \t\t\tmy ($what, $c) = ($1, $2);\n> \t\t\t# strip garbage for the address we'll use:\n\nI like this direction, I've actually been meaning to take this further\nand try to parse out SHA1s in the commit message, look those up, and add\ntheir authors to CC one of these days.\n\nBut IMO this patch is really lacking a few things before being ready:\n\n1. You have no tests for this. See t/t9001-send-email.sh for examples,\n i.e. stuff like\n\n (body) Adding cc: C O Mitter from line\n 'Signed-off-by: C O Mitter '\n\n Should have corresponding tests for \"Reviewed-by\" \"Seen-by\"\n etc. These are easy to add, just edit the raw messages and test that\n for the output about adding CCs.\n\n2. Just a few lines down from your quoted hunk we have this:\n\n\t# strip garbage for the address we'll use:\n\t$c = strip_garbage_one_address($c);\n\t# sanitize a bit more to decide whether to suppress the address:\n\tmy $sc = sanitize_address($c);\n\tif ($sc eq $sender) {\n\t\tnext if ($suppress_cc{'self'});\n\t} else {\n\t\tnext if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i;\n\t\tnext if $suppress_cc{'bodycc'} and $what =~ /Cc/i;\n\t}\n\tpush @cc, $c;\n\tprintf(__(\"(body) Adding cc: %s from line '%s'\\n\"),\n\t\t$c, $_) unless $quiet;\n\n So before we just supported Signed-off-by as a special case, but now\n your patch adds WHAT-EVER-by without updating the the corresponding\n --[no-]signed-off-by-cc command-line options.\n\n Your change should at least describe why those aren't being updated,\n but probably we should add some other command-line option for\n ignoring these wildcards, e.g. --[no-]wildcard-by-cc=reviewed\n --[no-]wildcard-by-cc=seen etc, and we can make --[no-]signed-off-by\n a historical alias for --[no-]wildcard-by-cc=signed-off.\n\n3. Ditto all the documentation in \"man git-send-email\" about\n \"signed-off-by\", \"sob\" etc, and the \"signedoffbycc\" variable\n documented both there and in \"man git-config\".\n\nStyle comment: First time I've seen someone write a charclass as\n[A-Z-a-z] and mean it, usually it's usually it's [A-Za-z-] to clarify\nthat the \"-\" isn't a range. Makes sense (to me) to have ranges first &\nstray chars last.\n"},{"id":"345021","messageId":"xmqqr2ncgqhl.fsf@gitster-ct.c.googlers.com","threadId":"48319","inReplyTo":"87tvs8e174.fsf@evledraar.gmail.com","subject":"Re: [PATCH] git-send-email: Cc more people","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-18T21:21:42Z","receivedAt":"2018-04-18T21:21:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> But IMO this patch is really lacking a few things before being ready:\n>\n> 1. You have no tests for this. See t/t9001-send-email.sh for examples,\n> ...\n> 2. Just a few lines down from your quoted hunk we have this:\n> ... code about $supress_cc{} ...\n> Your change should at least describe why those aren't being updated,\n> but probably we should add some other command-line option for\n> ignoring these wildcards, e.g. --[no-]wildcard-by-cc=reviewed\n> --[no-]wildcard-by-cc=seen etc, and we can make --[no-]signed-off-by\n> a historical alias for --[no-]wildcard-by-cc=signed-off.\n> 3. Ditto all the documentation in \"man git-send-email\" about\n> ...\n\nThanks, I agree that 2. (the lack of suppression) is a showstopper.\nI'd further say that these new CC-sources should be disabled by\ndefault and made opt-in to avoid surprising existing users.\n\nOne thing we also need to be very careful about is that some of the\nfields may not even have an e-mail address. We can expect that\nS-o-b and Cc would be of form \"human readable name \"\nby their nature, but it is perfectly fine to write only human\nreadable name without address on random lines like \"suggeted-by\" and\n\"helped-by\". There needs a way for the end-user to avoid using data\nfound on such lines as if they are valid e-mail addresses.\n\n\n"},{"id":"345090","messageId":"20180419121024.GD5556@bombadil.infradead.org","threadId":"48319","inReplyTo":"xmqqr2ncgqhl.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] git-send-email: Cc more people","fromName":"Matthew Wilcox","fromEmail":"willy@infradead.org","sentAt":"2018-04-19T12:10:24Z","receivedAt":"2018-04-19T12:10:30Z","isPatch":true,"sender":{"key":"willy@infradead.org","avatar":null},"body":"On Thu, Apr 19, 2018 at 06:21:42AM +0900, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n> \n> > But IMO this patch is really lacking a few things before being ready:\n> >\n> > 1. You have no tests for this. See t/t9001-send-email.sh for examples,\n> > ...\n> > 2. Just a few lines down from your quoted hunk we have this:\n> > ... code about $supress_cc{} ...\n> > Your change should at least describe why those aren't being updated,\n> > but probably we should add some other command-line option for\n> > ignoring these wildcards, e.g. --[no-]wildcard-by-cc=reviewed\n> > --[no-]wildcard-by-cc=seen etc, and we can make --[no-]signed-off-by\n> > a historical alias for --[no-]wildcard-by-cc=signed-off.\n> > 3. Ditto all the documentation in \"man git-send-email\" about\n> > ...\n> \n> Thanks, I agree that 2. (the lack of suppression) is a showstopper.\n\nI agree with that (and the lack of tests, obviously)\n\n> I'd further say that these new CC-sources should be disabled by\n> default and made opt-in to avoid surprising existing users.\n\nBut I disagree with this. The current behaviour is surprising to\nexisting users, to the point where people are writing their own scripts\nto replace git send-email (which seems crazy to me).\n\n> One thing we also need to be very careful about is that some of the\n> fields may not even have an e-mail address. We can expect that\n> S-o-b and Cc would be of form \"human readable name \"\n> by their nature, but it is perfectly fine to write only human\n> readable name without address on random lines like \"suggeted-by\" and\n> \"helped-by\". There needs a way for the end-user to avoid using data\n> found on such lines as if they are valid e-mail addresses.\n\nI also agree with this. I'll add some test-cases and make sure we only\nadd these if they're valid email addresses.\n"},{"id":"345109","messageId":"646938104.13100.1524141300699.JavaMail.zimbra@efficios.com","threadId":"48319","inReplyTo":"20180419121024.GD5556@bombadil.infradead.org","subject":"Re: [PATCH] git-send-email: Cc more people","fromName":"Mathieu Desnoyers","fromEmail":"mathieu.desnoyers@efficios.com","sentAt":"2018-04-19T12:35:00Z","receivedAt":"2018-04-19T12:35:05Z","isPatch":true,"sender":{"key":"mathieu.desnoyers@efficios.com","avatar":"https://gravatar.com/avatar/c3872f90e251e72cba6a65443eb6a4144e73ab1b2e08149cf78eb5d4779a88dd?d=mp&s=160"},"body":"----- On Apr 19, 2018, at 8:10 AM, Matthew Wilcox willy@infradead.org wrote:\n\n> On Thu, Apr 19, 2018 at 06:21:42AM +0900, Junio C Hamano wrote:\n>> Ævar Arnfjörð Bjarmason writes:\n>> \n>> > But IMO this patch is really lacking a few things before being ready:\n>> >\n>> > 1. You have no tests for this. See t/t9001-send-email.sh for examples,\n>> > ...\n>> > 2. Just a few lines down from your quoted hunk we have this:\n>> > ... code about $supress_cc{} ...\n>> > Your change should at least describe why those aren't being updated,\n>> > but probably we should add some other command-line option for\n>> > ignoring these wildcards, e.g. --[no-]wildcard-by-cc=reviewed\n>> > --[no-]wildcard-by-cc=seen etc, and we can make --[no-]signed-off-by\n>> > a historical alias for --[no-]wildcard-by-cc=signed-off.\n>> > 3. Ditto all the documentation in \"man git-send-email\" about\n>> > ...\n>> \n>> Thanks, I agree that 2. (the lack of suppression) is a showstopper.\n> \n> I agree with that (and the lack of tests, obviously)\n> \n>> I'd further say that these new CC-sources should be disabled by\n>> default and made opt-in to avoid surprising existing users.\n> \n> But I disagree with this. The current behaviour is surprising to\n> existing users, to the point where people are writing their own scripts\n> to replace git send-email (which seems crazy to me).\n\nWe could perhaps go with a whitelist approach. The four\nmain match I would be tempted to add are: Acked-by, Reported-by,\nReviewed-by, and Tested-by.\n\nMy workflow is to initially CC a bunch of relevant maintainers\nwhen sending out a patch, and as the Acked, Reviewed and Tested\nby tags come it, I replace those CC with the relevant tag.\nI never expected them to stop being CC'd when switching between\nthose categories.\n\nThanks,\n\nMathieu\n\n> \n>> One thing we also need to be very careful about is that some of the\n>> fields may not even have an e-mail address. We can expect that\n>> S-o-b and Cc would be of form \"human readable name \"\n>> by their nature, but it is perfectly fine to write only human\n>> readable name without address on random lines like \"suggeted-by\" and\n>> \"helped-by\". There needs a way for the end-user to avoid using data\n>> found on such lines as if they are valid e-mail addresses.\n> \n> I also agree with this. I'll add some test-cases and make sure we only\n> add these if they're valid email addresses.\n\n-- \nMathieu Desnoyers\nEfficiOS Inc.\nhttp://www.efficios.com\n"},{"id":"345178","messageId":"xmqqtvs6d9r6.fsf@gitster-ct.c.googlers.com","threadId":"48319","inReplyTo":"646938104.13100.1524141300699.JavaMail.zimbra@efficios.com","subject":"Re: [PATCH] git-send-email: Cc more people","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-20T00:03:41Z","receivedAt":"2018-04-20T00:03:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mathieu Desnoyers writes:\n\n>>> I'd further say that these new CC-sources should be disabled by\n>>> default and made opt-in to avoid surprising existing users.\n>> \n>> But I disagree with this. The current behaviour is surprising to\n>> existing users, to the point where people are writing their own scripts\n>> to replace git send-email (which seems crazy to me).\n>\n> We could perhaps go with a whitelist approach. The four\n> main match I would be tempted to add are: Acked-by, Reported-by,\n> Reviewed-by, and Tested-by.\n\nA tool that suddenly starts sending e-mails to more addresses\nwithout letting the end-users know when and why the change in\nbehaviour happened is a source of irritated \"somebody made a stupid\nchange to git-send-email without telling us that caused unwanted\ne-mails sent to unexpected places and embarrassed me\" bug reports.\nI do agree with a whitelist approach from that point of view, and in\nthe initial rollout of the feature, that whitelist should be limited\nto what we already send out.\n\nThe users who learn about this new feature can opt into whitelisting\nthe common 4 above before we enable them by default. FWIW, I\npersonally think these will be a sensible default (in addition to\nwhat we already Cc). I however prefer an approach to introduce\nthese more gradually.\n\n"},{"id":"345229","messageId":"666133236.13971.1524238059705.JavaMail.zimbra@efficios.com","threadId":"48319","inReplyTo":"xmqqtvs6d9r6.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] git-send-email: Cc more people","fromName":"Mathieu Desnoyers","fromEmail":"mathieu.desnoyers@efficios.com","sentAt":"2018-04-20T15:27:39Z","receivedAt":"2018-04-20T15:27:44Z","isPatch":true,"sender":{"key":"mathieu.desnoyers@efficios.com","avatar":"https://gravatar.com/avatar/c3872f90e251e72cba6a65443eb6a4144e73ab1b2e08149cf78eb5d4779a88dd?d=mp&s=160"},"body":"----- On Apr 19, 2018, at 8:03 PM, Junio C Hamano gitster@pobox.com wrote:\n\n> Mathieu Desnoyers writes:\n> \n>>>> I'd further say that these new CC-sources should be disabled by\n>>>> default and made opt-in to avoid surprising existing users.\n>>> \n>>> But I disagree with this. The current behaviour is surprising to\n>>> existing users, to the point where people are writing their own scripts\n>>> to replace git send-email (which seems crazy to me).\n>>\n>> We could perhaps go with a whitelist approach. The four\n>> main match I would be tempted to add are: Acked-by, Reported-by,\n>> Reviewed-by, and Tested-by.\n> \n> A tool that suddenly starts sending e-mails to more addresses\n> without letting the end-users know when and why the change in\n> behaviour happened is a source of irritated \"somebody made a stupid\n> change to git-send-email without telling us that caused unwanted\n> e-mails sent to unexpected places and embarrassed me\" bug reports.\n> I do agree with a whitelist approach from that point of view, and in\n> the initial rollout of the feature, that whitelist should be limited\n> to what we already send out.\n> \n> The users who learn about this new feature can opt into whitelisting\n> the common 4 above before we enable them by default. FWIW, I\n> personally think these will be a sensible default (in addition to\n> what we already Cc). I however prefer an approach to introduce\n> these more gradually.\n\nSure, introducing changes like this needs to be done gradually.\n\nThanks!\n\nMathieu\n\n\n-- \nMathieu Desnoyers\nEfficiOS Inc.\nhttp://www.efficios.com\n"}]}