{"thread":{"id":"49525","subject":"[PATCH 0/3] send-email: Also pick up cc addresses from -by trailers","startedAt":"2018-10-10T11:14:05Z","lastAt":"2018-10-16T07:57:46Z","messageCount":20,"participants":["Rasmus Villemoes","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"360001","messageId":"20181010111351.5045-1-rv@rasmusvillemoes.dk","threadId":"49525","inReplyTo":null,"subject":"[PATCH 0/3] send-email: Also pick up cc addresses from -by trailers","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2018-10-10T11:13:48Z","receivedAt":"2018-10-10T11:14:05Z","isPatch":true,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"This has been attempted multiple times before, but I hope that this\ncan make it in this time around. That *-by addresses are not\nautomatically Cc'ed certainly still surprises people from time to\ntime.\n\nI hope that this addresses all the concerns Junio had in\nhttps://lkml.org/lkml/2016/8/31/768 .\n\nFor the name, I chose 'misc-by', since that has -by in its name. I am\nfine with absolutely any other name (bodyby, body-by, by-trailers,\n...). I doubt we can find a short token that is completely\nself-explanatory, and note that one has to look in the man page anyway\nto know what 'sob' means in this line from 'git send-email -h':\n\n --suppress-cc * author, self, sob, cc, cccmd, body, bodycc, all.\n\nRasmus Villemoes (3):\n Documentation/git-send-email.txt: style fixes\n send-email: only consider lines containing @ or <> for automatic\n Cc'ing\n send-email: also pick up cc addresses from -by trailers\n\n Documentation/git-send-email.txt | 11 +++++++----\n git-send-email.perl | 19 +++++++++++++------\n 2 files changed, 20 insertions(+), 10 deletions(-)\n\n-- \n2.19.1.6.g084f1d7761\n\n"},{"id":"360002","messageId":"20181010111351.5045-2-rv@rasmusvillemoes.dk","threadId":"49525","inReplyTo":"20181010111351.5045-1-rv@rasmusvillemoes.dk","subject":"[PATCH 1/3] Documentation/git-send-email.txt: style fixes","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2018-10-10T11:13:49Z","receivedAt":"2018-10-10T11:14:08Z","isPatch":true,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"For consistency, add full stops in a few places and outdent a line by\none space.\n\nSigned-off-by: Rasmus Villemoes \n---\n Documentation/git-send-email.txt | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\nindex 465a4ecbed..ea6ea512fe 100644\n--- a/Documentation/git-send-email.txt\n+++ b/Documentation/git-send-email.txt\n@@ -321,16 +321,16 @@ Automating\n \tauto-cc of:\n +\n --\n-- 'author' will avoid including the patch author\n-- 'self' will avoid including the sender\n+- 'author' will avoid including the patch author.\n+- 'self' will avoid including the sender.\n - 'cc' will avoid including anyone mentioned in Cc lines in the patch header\n except for self (use 'self' for that).\n - 'bodycc' will avoid including anyone mentioned in Cc lines in the\n patch body (commit message) except for self (use 'self' for that).\n - 'sob' will avoid including anyone mentioned in Signed-off-by lines except\n- for self (use 'self' for that).\n+ for self (use 'self' for that).\n - 'cccmd' will avoid running the --cc-cmd.\n-- 'body' is equivalent to 'sob' + 'bodycc'\n+- 'body' is equivalent to 'sob' + 'bodycc'.\n - 'all' will suppress all auto cc values.\n --\n +\n-- \n2.19.1.6.g084f1d7761\n\n"},{"id":"360003","messageId":"20181010111351.5045-3-rv@rasmusvillemoes.dk","threadId":"49525","inReplyTo":"20181010111351.5045-1-rv@rasmusvillemoes.dk","subject":"[PATCH 2/3] send-email: only consider lines containing @ or <> for automatic Cc'ing","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2018-10-10T11:13:50Z","receivedAt":"2018-10-10T11:14:10Z","isPatch":true,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"While the address sanitizations routines do accept local addresses, that\nis almost never what is meant in a Cc or Signed-off-by trailer.\n\nLooking through all the signed-off-by lines in the linux kernel tree\nwithout a @, there are mostly two patterns: Either just a full name, or\na full name followed by (i.e., with the word at\ninstead of a @), and minor variations. For cc lines, the same patterns\nappear, along with lots of \"cc stable\" variations that do not actually\nname stable@vger.kernel.org\n\n Cc: stable # introduced pre-git times\n cc: stable.kernel.org\n\nIn the cases, one gets a chance to interactively\nfix it. But when there is no <> pair, it seems we end up just using the\nfirst word as a (local) address.\n\nAs the number of cases where a local address really was meant is\nlikely (and anecdotally) quite small compared to the number of cases\nwhere we end up cc'ing a garbage address, insist on at least a @ or a <>\npair being present.\n\nThis is also preparation for the next patch, where we are likely to\nencounter even more non-addresses in -by lines, such as\n\n Reported-by: Coverity\n Patch-generated-by: Coccinelle\n\nSigned-off-by: Rasmus Villemoes \n---\n git-send-email.perl | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 2be5dac337..1916159d2a 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1694,6 +1694,11 @@ sub process_file {\n \t\t\t\tnext if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i;\n \t\t\t\tnext if $suppress_cc{'bodycc'} and $what =~ /Cc/i;\n \t\t\t}\n+\t\t\tif ($c !~ /.+@.+|<.+>/) {\n+\t\t\t\tprintf(\"(body) Ignoring %s from line '%s'\\n\",\n+\t\t\t\t\t$what, $_) unless $quiet;\n+\t\t\t\tnext;\n+\t\t\t}\n \t\t\tpush @cc, $c;\n \t\t\tprintf(__(\"(body) Adding cc: %s from line '%s'\\n\"),\n \t\t\t\t$c, $_) unless $quiet;\n-- \n2.19.1.6.g084f1d7761\n\n"},{"id":"360004","messageId":"20181010111351.5045-4-rv@rasmusvillemoes.dk","threadId":"49525","inReplyTo":"20181010111351.5045-1-rv@rasmusvillemoes.dk","subject":"[PATCH 3/3] send-email: also pick up cc addresses from -by trailers","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2018-10-10T11:13:51Z","receivedAt":"2018-10-10T11:14:13Z","isPatch":true,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"When rerolling a patch series, including various Reviewed-by etc. that\nmay have come in, it is quite convenient to have git-send-email\nautomatically cc those people.\n\nSo pick up any *-by lines, with a new suppression category 'misc-by',\nbut special-case Signed-off-by, since that already has its own\nsuppression category. It seems natural to make 'misc-by' implied by\n'body'.\n\nBased-on-patch-by: Joe Perches \nSigned-off-by: Rasmus Villemoes \n---\n Documentation/git-send-email.txt | 5 ++++-\n git-send-email.perl | 14 ++++++++------\n 2 files changed, 12 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\nindex ea6ea512fe..f6010ac68b 100644\n--- a/Documentation/git-send-email.txt\n+++ b/Documentation/git-send-email.txt\n@@ -329,8 +329,11 @@ Automating\n patch body (commit message) except for self (use 'self' for that).\n - 'sob' will avoid including anyone mentioned in Signed-off-by lines except\n for self (use 'self' for that).\n+- 'misc-by' will avoid including anyone mentioned in Acked-by,\n+ Reviewed-by, Tested-by and other \"-by\" lines in the patch body,\n+ except Signed-off-by (use 'sob' for that).\n - 'cccmd' will avoid running the --cc-cmd.\n-- 'body' is equivalent to 'sob' + 'bodycc'.\n+- 'body' is equivalent to 'sob' + 'bodycc' + 'misc-by'.\n - 'all' will suppress all auto cc values.\n --\n +\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 1916159d2a..7a6391e5d8 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -94,7 +94,7 @@ sub usage {\n --identity * Use the sendemail. options.\n --to-cmd * Email To: via ` \\$patch_path`\n --cc-cmd * Email Cc: via ` \\$patch_path`\n- --suppress-cc * author, self, sob, cc, cccmd, body, bodycc, all.\n+ --suppress-cc * author, self, sob, cc, cccmd, body, bodycc, misc-by, all.\n --[no-]cc-cover * Email Cc: addresses in the cover letter.\n --[no-]to-cover * Email To: addresses in the cover letter.\n --[no-]signed-off-by-cc * Send to Signed-off-by: addresses. Default on.\n@@ -454,13 +454,13 @@ sub read_config {\n if (@suppress_cc) {\n \tforeach my $entry (@suppress_cc) {\n \t\tdie sprintf(__(\"Unknown --suppress-cc field: '%s'\\n\"), $entry)\n-\t\t\tunless $entry =~ /^(?:all|cccmd|cc|author|self|sob|body|bodycc)$/;\n+\t\t\tunless $entry =~ /^(?:all|cccmd|cc|author|self|sob|body|bodycc|misc-by)$/;\n \t\t$suppress_cc{$entry} = 1;\n \t}\n }\n \n if ($suppress_cc{'all'}) {\n-\tforeach my $entry (qw (cccmd cc author self sob body bodycc)) {\n+\tforeach my $entry (qw (cccmd cc author self sob body bodycc misc-by)) {\n \t\t$suppress_cc{$entry} = 1;\n \t}\n \tdelete $suppress_cc{'all'};\n@@ -471,7 +471,7 @@ sub read_config {\n $suppress_cc{'sob'} = !$signed_off_by_cc if defined $signed_off_by_cc;\n \n if ($suppress_cc{'body'}) {\n-\tforeach my $entry (qw (sob bodycc)) {\n+\tforeach my $entry (qw (sob bodycc misc-by)) {\n \t\t$suppress_cc{$entry} = 1;\n \t}\n \tdelete $suppress_cc{'body'};\n@@ -1681,7 +1681,7 @@ sub process_file {\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-]*-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@@ -1691,7 +1691,9 @@ sub process_file {\n \t\t\tif ($sc eq $sender) {\n \t\t\t\tnext if ($suppress_cc{'self'});\n \t\t\t} else {\n-\t\t\t\tnext if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i;\n+\t\t\t\tnext if $suppress_cc{'sob'} and $what =~ /^Signed-off-by$/i;\n+\t\t\t\tnext if $suppress_cc{'misc-by'}\n+\t\t\t\t\tand $what =~ /-by$/i and $what !~ /^Signed-off-by$/i;\n \t\t\t\tnext if $suppress_cc{'bodycc'} and $what =~ /Cc/i;\n \t\t\t}\n \t\t\tif ($c !~ /.+@.+|<.+>/) {\n-- \n2.19.1.6.g084f1d7761\n\n"},{"id":"360015","messageId":"8736teezha.fsf@evledraar.gmail.com","threadId":"49525","inReplyTo":"20181010111351.5045-4-rv@rasmusvillemoes.dk","subject":"Re: [PATCH 3/3] send-email: also pick up cc addresses from -by trailers","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-10T12:51:45Z","receivedAt":"2018-10-10T12:51:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Oct 10 2018, Rasmus Villemoes wrote:\n\n> -\t\t\t\tnext if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i;\n> +\t\t\t\tnext if $suppress_cc{'sob'} and $what =~ /^Signed-off-by$/i;\n> +\t\t\t\tnext if $suppress_cc{'misc-by'}\n> +\t\t\t\t\tand $what =~ /-by$/i and $what !~ /^Signed-off-by$/i;\n> \t\t\t\tnext if $suppress_cc{'bodycc'} and $what =~ /Cc/i;\n\nLooks good, FWIW I was curious if this could be:\n\n next if $suppress_cc{'misc-by'} and $what =~ /(? for automatic Cc'ing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-10T12:57:51Z","receivedAt":"2018-10-10T12:57:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Oct 10 2018, Rasmus Villemoes wrote:\n\n> +\t\t\tif ($c !~ /.+@.+|<.+>/) {\n> +\t\t\t\tprintf(\"(body) Ignoring %s from line '%s'\\n\",\n> +\t\t\t\t\t$what, $_) unless $quiet;\n> +\t\t\t\tnext;\n> +\t\t\t}\n> \t\t\tpush @cc, $c;\n> \t\t\tprintf(__(\"(body) Adding cc: %s from line '%s'\\n\"),\n> \t\t\t\t$c, $_) unless $quiet;\n\nThere's a extract_valid_address() function in git-send-email already,\nshouldn't this be:\n\n if (!extract_valid_address($c)) {\n [...]\n\nOr is there a good reason not to use that function in this case?\n"},{"id":"360022","messageId":"7b03da07-4301-1b42-b8a2-a29e4e1f80d0@rasmusvillemoes.dk","threadId":"49525","inReplyTo":"871s8yez74.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/3] send-email: only consider lines containing @ or <> for automatic Cc'ing","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2018-10-10T13:29:56Z","receivedAt":"2018-10-10T13:30:02Z","isPatch":true,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"On 2018-10-10 14:57, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Oct 10 2018, Rasmus Villemoes wrote:\n> \n>> +\t\t\tif ($c !~ /.+@.+|<.+>/) {\n>> +\t\t\t\tprintf(\"(body) Ignoring %s from line '%s'\\n\",\n>> +\t\t\t\t\t$what, $_) unless $quiet;\n>> +\t\t\t\tnext;\n>> +\t\t\t}\n>> \t\t\tpush @cc, $c;\n>> \t\t\tprintf(__(\"(body) Adding cc: %s from line '%s'\\n\"),\n>> \t\t\t\t$c, $_) unless $quiet;\n> \n> There's a extract_valid_address() function in git-send-email already,\n> shouldn't this be:\n> \n> if (!extract_valid_address($c)) {\n> [...]\n> \n> Or is there a good reason not to use that function in this case?\n> \n\nI considered that (and also had a version where I simply insisted on a @\nbeing present), but that means the user no longer would get prompted\nabout the cases where the address was just slightly obfuscated, e.g. the\n\nCc: John Doe \n\ncases, which would be a regression, I guess. So I do want to pass such\ncases through, and have them be dealt with when process_address_list\ngets called.\n\nSo this is just a rather minimal and simple heuristic, which should\nstill be able to handle the vast majority of cases correctly, and at\nleast almost never exclude anything that might have a chance of becoming\na real address.\n\nRasmus\n"},{"id":"360138","messageId":"xmqqk1mpggpq.fsf@gitster-ct.c.googlers.com","threadId":"49525","inReplyTo":"7b03da07-4301-1b42-b8a2-a29e4e1f80d0@rasmusvillemoes.dk","subject":"Re: [PATCH 2/3] send-email: only consider lines containing @ or <> for automatic Cc'ing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-11T06:06:25Z","receivedAt":"2018-10-11T06:06:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rasmus Villemoes writes:\n\n> I considered that (and also had a version where I simply insisted on a @\n> being present), but that means the user no longer would get prompted\n> about the cases where the address was just slightly obfuscated, e.g. the\n>\n> Cc: John Doe \n>\n> cases, which would be a regression, I guess. So I do want to pass such\n> cases through, and have them be dealt with when process_address_list\n> gets called.\n\nWe are only tightening with this patch, and we were passing any\nrandom things through with the original code anyway, so without\n[PATCH 3/3], this step must be making it only better, but I have to\nwonder one thing.\n\nYou keep saying \"get prompted\" but are we sure we always stop and\nask (and preferrably---fail and abort when the end user is not\navailable at the terminal to interact) when we have such a\nquestionable address?\n\n"},{"id":"360139","messageId":"xmqqd0shgg66.fsf@gitster-ct.c.googlers.com","threadId":"49525","inReplyTo":"20181010111351.5045-4-rv@rasmusvillemoes.dk","subject":"Re: [PATCH 3/3] send-email: also pick up cc addresses from -by trailers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-11T06:18:09Z","receivedAt":"2018-10-11T06:18:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rasmus Villemoes writes:\n\n> @@ -1681,7 +1681,7 @@ sub process_file {\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-]*-by|Cc): (.*)/i) {\n\nSo this picks up anything-by not just s-o-by, which sort of makes sense.\n\n> @@ -1691,7 +1691,9 @@ sub process_file {\n> \t\t\tif ($sc eq $sender) {\n> \t\t\t\tnext if ($suppress_cc{'self'});\n> \t\t\t} else {\n> -\t\t\t\tnext if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i;\n\nWe used to only grab CC or Signed-off-by (and specifically not\nsomething like \"Not-Signed-off-by\") upfront above, so matching\n/Signed-off-by/ was sufficient (it would have been sufficient to\njust look for 's'). But to suppress s-o-b and keep allowing via\nmisc-by a trailer \"Not-signed-off-by:\", we now ...\n\n> +\t\t\t\tnext if $suppress_cc{'sob'} and $what =~ /^Signed-off-by$/i;\n\n... must make sure what we have is _exactly_ \"signed-off-by\" when\n'sob' is suppressed. Makes sense.\n\n> +\t\t\t\tnext if $suppress_cc{'misc-by'}\n> +\t\t\t\t\tand $what =~ /-by$/i and $what !~ /^Signed-off-by$/i;\n\nAnd this is the opposite side of the same coin, which also makes sense.\n\nI wonder if it would make it easier to grok if we made the logic\ninside out, i.e.\n\n\tif ($sc eq $sender) {\n\t\t...\n\t} else {\n\t\tif ($what =~ /^Signed-off-by$/i) {\n\t\t\tnext if $suppress_cc{'sob'};\n\t\t} elsif ($what =~ /-by$/i) {\n\t\t\tnext if $suppress_cc{'misc'};\n\t\t} elsif ($what =~ /^Cc$/i) {\n\t\t\tnext if $suppress_cc{'bodycc'};\n\t\t}\n\t\tpush @cc, $c;\n\t\t...\n\t}\n\n> \t\t\t\tnext if $suppress_cc{'bodycc'} and $what =~ /Cc/i;\n> \t\t\t}\n> \t\t\tif ($c !~ /.+@.+|<.+>/) {\n"},{"id":"360142","messageId":"f5c401a6-f329-1076-ab4b-2f52915047bf@rasmusvillemoes.dk","threadId":"49525","inReplyTo":"xmqqk1mpggpq.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/3] send-email: only consider lines containing @ or <> for automatic Cc'ing","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2018-10-11T07:06:10Z","receivedAt":"2018-10-11T07:06:17Z","isPatch":true,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"On 2018-10-11 08:06, Junio C Hamano wrote:\n> Rasmus Villemoes writes:\n> \n>> I considered that (and also had a version where I simply insisted on a @\n>> being present), but that means the user no longer would get prompted\n>> about the cases where the address was just slightly obfuscated, e.g. the\n>>\n>> Cc: John Doe \n>>\n>> cases, which would be a regression, I guess. So I do want to pass such\n>> cases through, and have them be dealt with when process_address_list\n>> gets called.\n> \n> We are only tightening with this patch, and we were passing any\n> random things through with the original code anyway, so without\n> [PATCH 3/3], this step must be making it only better, but I have to\n> wonder one thing.\n> \n> You keep saying \"get prompted\" but are we sure we always stop and\n> ask (and preferrably---fail and abort when the end user is not\n> available at the terminal to interact) when we have such a\n> questionable address?\n> \n\nI dunno. I guess I've never considered non-interactive use of\nsend-email. But the ask() in validate_address does have default q[uit],\nwhich I suppose gets used if stdin is /dev/null? I did do an experiment\nadding a bunch of the random odd patterns found in kernel commit\nmessages to see how send-email reacted before/after this, and the only\nthings that got filtered away (i.e., no longer prompted about) were\nthings where the user probably couldn't easily fix it anyway. In the\ncases where there was a \"Cc: stable\" that might be fixed to the proper\nstable@vger.kernel.org, the logic in extract_valid_address simply saw\nthat as a local address, so we didn't use to be prompted, but simply\nsent to stable@localhost. Now we simply don't pass that through. So, for\nnon-interactive use, I guess the effect of this patch is to allow more\ncases to complete succesfully, since we filter away (some) cases where\nextract_valid_address would cause us to prompt (and thus quit).\n\nSo, it seems you're ok with this tightening, but some comment on the\nnon-interactive use case should be made in the commit log? Or am I\nmisunderstanding?\n\nThanks,\nRasmus\n"},{"id":"360143","messageId":"5651069a-4879-096d-082d-58d8f0158958@rasmusvillemoes.dk","threadId":"49525","inReplyTo":"xmqqd0shgg66.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/3] send-email: also pick up cc addresses from -by trailers","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2018-10-11T07:11:42Z","receivedAt":"2018-10-11T07:11:48Z","isPatch":true,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"On 2018-10-11 08:18, Junio C Hamano wrote:\n> Rasmus Villemoes writes:\n\n> we now ...\n> \n>> +\t\t\t\tnext if $suppress_cc{'sob'} and $what =~ /^Signed-off-by$/i;\n> \n> ... must make sure what we have is _exactly_ \"signed-off-by\" when\n> 'sob' is suppressed. Makes sense.\n> \n>> +\t\t\t\tnext if $suppress_cc{'misc-by'}\n>> +\t\t\t\t\tand $what =~ /-by$/i and $what !~ /^Signed-off-by$/i;\n> \n> And this is the opposite side of the same coin, which also makes sense.\n\nYup, I started by just adding the misc-by line, then remembered that\npeople sometimes use not-signed-off-by variants, and went back and\nanchored the s-o-b case. So now it's no longer so minimal, and...\n\n> I wonder if it would make it easier to grok if we made the logic\n> inside out, i.e.\n> \n> \tif ($sc eq $sender) {\n> \t\t...\n> \t} else {\n> \t\tif ($what =~ /^Signed-off-by$/i) {\n> \t\t\tnext if $suppress_cc{'sob'};\n> \t\t} elsif ($what =~ /-by$/i) {\n> \t\t\tnext if $suppress_cc{'misc'};\n> \t\t} elsif ($what =~ /^Cc$/i) {\n> \t\t\tnext if $suppress_cc{'bodycc'};> \t\t}\n\n...yes, that's probably more readable.\n\nThanks,\nRasmus\n"},{"id":"360151","messageId":"xmqqr2gwgaef.fsf@gitster-ct.c.googlers.com","threadId":"49525","inReplyTo":"f5c401a6-f329-1076-ab4b-2f52915047bf@rasmusvillemoes.dk","subject":"Re: [PATCH 2/3] send-email: only consider lines containing @ or <> for automatic Cc'ing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-11T08:22:48Z","receivedAt":"2018-10-11T08:22:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rasmus Villemoes writes:\n\n> So, it seems you're ok with this tightening, but some comment on the\n> non-interactive use case should be made in the commit log? Or am I\n> misunderstanding?\n\nI do not think we need any immediate action on this step. I was\njust wondering if we want two classes of \"I am not running you\ninteractively, so assume I said 'yes' when you need to ask me any\nconfirmation on X and Y\" and \"I am not running you interactively,\nso assume I said 'no' for safety when you need to ask me any\nconfirmation on Z\" supported in the future. Lines with both @ and\n<> fall into the first class, while lines with only <> fall into the\nsecond camp, I would guess.\n\n"},{"id":"360584","messageId":"xmqqo9buzb67.fsf@gitster-ct.c.googlers.com","threadId":"49525","inReplyTo":"5651069a-4879-096d-082d-58d8f0158958@rasmusvillemoes.dk","subject":"Re: [PATCH 3/3] send-email: also pick up cc addresses from -by trailers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-16T05:57:04Z","receivedAt":"2018-10-16T05:57:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rasmus Villemoes writes:\n\n>> I wonder if it would make it easier to grok if we made the logic\n>> inside out, i.e.\n>> \n>> \tif ($sc eq $sender) {\n>> \t\t...\n>> \t} else {\n>> \t\tif ($what =~ /^Signed-off-by$/i) {\n>> \t\t\tnext if $suppress_cc{'sob'};\n>> \t\t} elsif ($what =~ /-by$/i) {\n>> \t\t\tnext if $suppress_cc{'misc'};\n>> \t\t} elsif ($what =~ /^Cc$/i) {\n>> \t\t\tnext if $suppress_cc{'bodycc'};> \t\t}\n>\n> ...yes, that's probably more readable.\n\nOK, unless there is more comments and suggestions for improvements,\ncan you send in a final version sometime not in so distant future so\nthat we won't forget? It may be surprising to existing users that\nthe command now suddenly adds more addresses the user did not think\nwould be added, but it would probably be easy enough for them to\nwork around. I'll need to prepare a note in the draft release notes\nto describe backward (in)compatibility to warn users.\n"},{"id":"360589","messageId":"ef80210d-f440-06c5-a1eb-7b3737f73adb@rasmusvillemoes.dk","threadId":"49525","inReplyTo":"xmqqo9buzb67.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/3] send-email: also pick up cc addresses from -by trailers","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2018-10-16T07:17:46Z","receivedAt":"2018-10-16T07:17:52Z","isPatch":true,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"On 2018-10-16 07:57, Junio C Hamano wrote:\n> Rasmus Villemoes writes:\n> \n>>> I wonder if it would make it easier to grok if we made the logic\n>>> inside out, i.e.\n>>>\n>>> \tif ($sc eq $sender) {\n>>> \t\t...\n>>> \t} else {\n>>> \t\tif ($what =~ /^Signed-off-by$/i) {\n>>> \t\t\tnext if $suppress_cc{'sob'};\n>>> \t\t} elsif ($what =~ /-by$/i) {\n>>> \t\t\tnext if $suppress_cc{'misc'};\n>>> \t\t} elsif ($what =~ /^Cc$/i) {\n>>> \t\t\tnext if $suppress_cc{'bodycc'};> \t\t}\n>>\n>> ...yes, that's probably more readable.\n> \n> OK, unless there is more comments and suggestions for improvements,\n> can you send in a final version sometime not in so distant future so\n> that we won't forget?\n\nWill do, I was just waiting for more comments to come in.\n\n It may be surprising to existing users that\n> the command now suddenly adds more addresses the user did not think\n> would be added, but it would probably be easy enough for them to\n> work around. \n\nYeah, I thought about that, but unfortunately the whole auto-cc business\nis not built around some config options where we can add a new and\ndefault false. Also note that there are also cases currently where the\nuser sends off a patch series and is surprised that lots of intended\nrecipients were not cc'ed (that's how I picked this subject up again; I\nhad a long series where I had put specific Cc's in each patch, at v2,\nsome of those had given a Reviewed-by, so I changed the tags, and a\n--dry-run told me they wouldn't get the new version).\n\nI suppose one could make use of -by addresses dependent on a new opt-in\nconfig option, but that's not very elegant. Another option is, for a\nrelease or two, to make a little (more) noise when picking up a -by\naddress - something like setting a flag in the ($what =~ /-by/) branch,\nand testing that flag somewhere in send_message(). But I suppose the\nmessage printed when needs_confirm eq \"inform\" is generic enough.\n\nRasmus\n"},{"id":"360590","messageId":"20181016073923.28134-1-rv@rasmusvillemoes.dk","threadId":"49525","inReplyTo":"20181010111351.5045-1-rv@rasmusvillemoes.dk","subject":"[PATCH v2 0/3] send-email: Also pick up cc addresses from -by trailers","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2018-10-16T07:39:20Z","receivedAt":"2018-10-16T07:39:33Z","isPatch":true,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"This series extends the logic in git-send-email so that addresses\nappearing in various *-by: trailers (e.g. Reviewed-by, Acked-by,\nTested-by) are picked up and added to the Cc list, in addition to the\ncurrent logic that picks up Cc: and Signed-off-by: lines.\n\nThis deliberately only picks up -by trailers (as opposed to any\ntrailer), based on the heuristic that the -by suffix strongly suggests\nthere's a (name +) email address after the colon. This avoids having\nto deal with BugID:, Link:, or other such tags. Still, widening to any\n-by trailer does increase the risk that we will pick up stuff that is\nnot an email address, such as\n\n Reported-by: Coverity\n Patch-generated-by: Coccinelle\n\nwhere send-email then ends up cc'ing the local 'coverity' user. Patch\n2 tries to weed out the obvious no-email-address-here cases, which\nshould also help avoid cc'ing garbage (local) addresses for malformed\ncc and signed-off-by lines. Patch 3 is then mostly mechanical,\nintroducing the misc-by suppression category and changing the regexp\nfor matching trailer lines to include .*-by.\n\nChanges in v2: Rework logic in patch 3 as suggested by Junio.\n\nv1 cover letter:\n\nThis has been attempted multiple times before, but I hope that this\ncan make it in this time around. That *-by addresses are not\nautomatically Cc'ed certainly still surprises people from time to\ntime.\n\nI hope that this addresses all the concerns Junio had in\nhttps://lkml.org/lkml/2016/8/31/768 .\n\nFor the name, I chose 'misc-by', since that has -by in its name. I am\nfine with absolutely any other name (bodyby, body-by, by-trailers,\n...). I doubt we can find a short token that is completely\nself-explanatory, and note that one has to look in the man page anyway\nto know what 'sob' means in this line from 'git send-email -h':\n\n --suppress-cc * author, self, sob, cc, cccmd, body, bodycc, all.\n\n\nRasmus Villemoes (3):\n Documentation/git-send-email.txt: style fixes\n send-email: only consider lines containing @ or <> for automatic\n Cc'ing\n send-email: also pick up cc addresses from -by trailers\n\n Documentation/git-send-email.txt | 11 +++++++----\n git-send-email.perl | 24 +++++++++++++++++-------\n 2 files changed, 24 insertions(+), 11 deletions(-)\n\n-- \n2.19.1.6.gbde171bbf5\n\n"},{"id":"360591","messageId":"20181016073923.28134-2-rv@rasmusvillemoes.dk","threadId":"49525","inReplyTo":"20181016073923.28134-1-rv@rasmusvillemoes.dk","subject":"[PATCH v2 1/3] Documentation/git-send-email.txt: style fixes","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2018-10-16T07:39:21Z","receivedAt":"2018-10-16T07:39:35Z","isPatch":true,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"For consistency, add full stops in a few places and outdent a line by\none space.\n\nSigned-off-by: Rasmus Villemoes \n---\n Documentation/git-send-email.txt | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\nindex 465a4ecbed..ea6ea512fe 100644\n--- a/Documentation/git-send-email.txt\n+++ b/Documentation/git-send-email.txt\n@@ -321,16 +321,16 @@ Automating\n \tauto-cc of:\n +\n --\n-- 'author' will avoid including the patch author\n-- 'self' will avoid including the sender\n+- 'author' will avoid including the patch author.\n+- 'self' will avoid including the sender.\n - 'cc' will avoid including anyone mentioned in Cc lines in the patch header\n except for self (use 'self' for that).\n - 'bodycc' will avoid including anyone mentioned in Cc lines in the\n patch body (commit message) except for self (use 'self' for that).\n - 'sob' will avoid including anyone mentioned in Signed-off-by lines except\n- for self (use 'self' for that).\n+ for self (use 'self' for that).\n - 'cccmd' will avoid running the --cc-cmd.\n-- 'body' is equivalent to 'sob' + 'bodycc'\n+- 'body' is equivalent to 'sob' + 'bodycc'.\n - 'all' will suppress all auto cc values.\n --\n +\n-- \n2.19.1.6.gbde171bbf5\n\n"},{"id":"360592","messageId":"20181016073923.28134-3-rv@rasmusvillemoes.dk","threadId":"49525","inReplyTo":"20181016073923.28134-1-rv@rasmusvillemoes.dk","subject":"[PATCH v2 2/3] send-email: only consider lines containing @ or <> for automatic Cc'ing","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2018-10-16T07:39:22Z","receivedAt":"2018-10-16T07:39:44Z","isPatch":true,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"While the address sanitizations routines do accept local addresses, that\nis almost never what is meant in a Cc or Signed-off-by trailer.\n\nLooking through all the signed-off-by lines in the linux kernel tree\nwithout a @, there are mostly two patterns: Either just a full name, or\na full name followed by (i.e., with the word at\ninstead of a @), and minor variations. For cc lines, the same patterns\nappear, along with lots of \"cc stable\" variations that do not actually\nname stable@vger.kernel.org\n\n Cc: stable # introduced pre-git times\n cc: stable.kernel.org\n\nIn the cases, one gets a chance to interactively\nfix it. But when there is no <> pair, it seems we end up just using the\nfirst word as a (local) address.\n\nAs the number of cases where a local address really was meant is\nlikely (and anecdotally) quite small compared to the number of cases\nwhere we end up cc'ing a garbage address, insist on at least a @ or a <>\npair being present.\n\nThis is also preparation for the next patch, where we are likely to\nencounter even more non-addresses in -by lines, such as\n\n Reported-by: Coverity\n Patch-generated-by: Coccinelle\n\nSigned-off-by: Rasmus Villemoes \n---\n git-send-email.perl | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 2be5dac337..1916159d2a 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1694,6 +1694,11 @@ sub process_file {\n \t\t\t\tnext if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i;\n \t\t\t\tnext if $suppress_cc{'bodycc'} and $what =~ /Cc/i;\n \t\t\t}\n+\t\t\tif ($c !~ /.+@.+|<.+>/) {\n+\t\t\t\tprintf(\"(body) Ignoring %s from line '%s'\\n\",\n+\t\t\t\t\t$what, $_) unless $quiet;\n+\t\t\t\tnext;\n+\t\t\t}\n \t\t\tpush @cc, $c;\n \t\t\tprintf(__(\"(body) Adding cc: %s from line '%s'\\n\"),\n \t\t\t\t$c, $_) unless $quiet;\n-- \n2.19.1.6.gbde171bbf5\n\n"},{"id":"360593","messageId":"20181016073923.28134-4-rv@rasmusvillemoes.dk","threadId":"49525","inReplyTo":"20181016073923.28134-1-rv@rasmusvillemoes.dk","subject":"[PATCH v2 3/3] send-email: also pick up cc addresses from -by trailers","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2018-10-16T07:39:23Z","receivedAt":"2018-10-16T07:39:50Z","isPatch":true,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"When rerolling a patch series, including various Reviewed-by etc. that\nmay have come in, it is quite convenient to have git-send-email\nautomatically cc those people.\n\nSo pick up any *-by lines, with a new suppression category 'misc-by',\nbut special-case Signed-off-by, since that already has its own\nsuppression category. It seems natural to make 'misc-by' implied by\n'body'.\n\nBased-on-patch-by: Joe Perches \nSigned-off-by: Rasmus Villemoes \n---\n Documentation/git-send-email.txt | 5 ++++-\n git-send-email.perl | 19 ++++++++++++-------\n 2 files changed, 16 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\nindex ea6ea512fe..f6010ac68b 100644\n--- a/Documentation/git-send-email.txt\n+++ b/Documentation/git-send-email.txt\n@@ -329,8 +329,11 @@ Automating\n patch body (commit message) except for self (use 'self' for that).\n - 'sob' will avoid including anyone mentioned in Signed-off-by lines except\n for self (use 'self' for that).\n+- 'misc-by' will avoid including anyone mentioned in Acked-by,\n+ Reviewed-by, Tested-by and other \"-by\" lines in the patch body,\n+ except Signed-off-by (use 'sob' for that).\n - 'cccmd' will avoid running the --cc-cmd.\n-- 'body' is equivalent to 'sob' + 'bodycc'.\n+- 'body' is equivalent to 'sob' + 'bodycc' + 'misc-by'.\n - 'all' will suppress all auto cc values.\n --\n +\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 1916159d2a..58c6aa9d0e 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -94,7 +94,7 @@ sub usage {\n --identity * Use the sendemail. options.\n --to-cmd * Email To: via ` \\$patch_path`\n --cc-cmd * Email Cc: via ` \\$patch_path`\n- --suppress-cc * author, self, sob, cc, cccmd, body, bodycc, all.\n+ --suppress-cc * author, self, sob, cc, cccmd, body, bodycc, misc-by, all.\n --[no-]cc-cover * Email Cc: addresses in the cover letter.\n --[no-]to-cover * Email To: addresses in the cover letter.\n --[no-]signed-off-by-cc * Send to Signed-off-by: addresses. Default on.\n@@ -454,13 +454,13 @@ sub read_config {\n if (@suppress_cc) {\n \tforeach my $entry (@suppress_cc) {\n \t\tdie sprintf(__(\"Unknown --suppress-cc field: '%s'\\n\"), $entry)\n-\t\t\tunless $entry =~ /^(?:all|cccmd|cc|author|self|sob|body|bodycc)$/;\n+\t\t\tunless $entry =~ /^(?:all|cccmd|cc|author|self|sob|body|bodycc|misc-by)$/;\n \t\t$suppress_cc{$entry} = 1;\n \t}\n }\n \n if ($suppress_cc{'all'}) {\n-\tforeach my $entry (qw (cccmd cc author self sob body bodycc)) {\n+\tforeach my $entry (qw (cccmd cc author self sob body bodycc misc-by)) {\n \t\t$suppress_cc{$entry} = 1;\n \t}\n \tdelete $suppress_cc{'all'};\n@@ -471,7 +471,7 @@ sub read_config {\n $suppress_cc{'sob'} = !$signed_off_by_cc if defined $signed_off_by_cc;\n \n if ($suppress_cc{'body'}) {\n-\tforeach my $entry (qw (sob bodycc)) {\n+\tforeach my $entry (qw (sob bodycc misc-by)) {\n \t\t$suppress_cc{$entry} = 1;\n \t}\n \tdelete $suppress_cc{'body'};\n@@ -1681,7 +1681,7 @@ sub process_file {\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-]*-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@@ -1691,8 +1691,13 @@ sub process_file {\n \t\t\tif ($sc eq $sender) {\n \t\t\t\tnext if ($suppress_cc{'self'});\n \t\t\t} else {\n-\t\t\t\tnext if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i;\n-\t\t\t\tnext if $suppress_cc{'bodycc'} and $what =~ /Cc/i;\n+\t\t\t\tif ($what =~ /^Signed-off-by$/i) {\n+\t\t\t\t\tnext if $suppress_cc{'sob'};\n+\t\t\t\t} elsif ($what =~ /-by$/i) {\n+\t\t\t\t\tnext if $suppress_cc{'misc-by'};\n+\t\t\t\t} elsif ($what =~ /Cc/i) {\n+\t\t\t\t\tnext if $suppress_cc{'bodycc'};\n+\t\t\t\t}\n \t\t\t}\n \t\t\tif ($c !~ /.+@.+|<.+>/) {\n \t\t\t\tprintf(\"(body) Ignoring %s from line '%s'\\n\",\n-- \n2.19.1.6.gbde171bbf5\n\n"},{"id":"360594","messageId":"xmqq7eiiz64d.fsf@gitster-ct.c.googlers.com","threadId":"49525","inReplyTo":"ef80210d-f440-06c5-a1eb-7b3737f73adb@rasmusvillemoes.dk","subject":"Re: [PATCH 3/3] send-email: also pick up cc addresses from -by trailers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-16T07:46:10Z","receivedAt":"2018-10-16T07:46:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rasmus Villemoes writes:\n\n>> It may be surprising to existing users that\n>> the command now suddenly adds more addresses the user did not think\n>> would be added, but it would probably be easy enough for them to\n>> work around. \n>\n> Yeah, I thought about that, but unfortunately the whole auto-cc business\n> is not built around some config options where we can add a new and\n> default false. Also note that there are also cases currently where the\n> user sends off a patch series and is surprised that lots of intended\n> recipients were not cc'ed (that's how I picked this subject up again; I\n\nThat \"also note ... people who are not familiar are surprised\" is,\nquite honestly, irrelevant. The behaviour is documented, and the\nusers are supposed to be used to it. Changing the behaviour in\nquite a different way from what existing users are used to is a very\ndifferent matter. No matter how you cut it, change of behaviour\nlike this is a regression for some existing users, while helping\nothers, and it does not matter if it helps many more users than it\nhurts---a regression is a regression to those who are affected\nnegatively. \n\nAt least this is a deliberate one we are making, and I think it is\nOK as long as both the change in behaviour and the way to get back\nthe old behaviour are advertised properly.\n\nThanks.\n\n"},{"id":"360596","messageId":"xmqq36t6z5l6.fsf@gitster-ct.c.googlers.com","threadId":"49525","inReplyTo":"20181016073923.28134-1-rv@rasmusvillemoes.dk","subject":"Re: [PATCH v2 0/3] send-email: Also pick up cc addresses from -by trailers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-16T07:57:41Z","receivedAt":"2018-10-16T07:57:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rasmus Villemoes writes:\n\n> This series extends the logic in git-send-email so that addresses\n> appearing in various *-by: trailers (e.g. Reviewed-by, Acked-by,\n> Tested-by) are picked up and added to the Cc list, in addition to the\n> current logic that picks up Cc: and Signed-off-by: lines.\n\nThanks. Will replace. I think this is ready for 'next' so let's\nsee if somebody else have more comments for a few days and then\nstart merging it down.\n\n"}]}