{"thread":{"id":"50758","subject":"[PATCH] send-email: don't cc *-by lines with '-' prefix","startedAt":"2019-03-16T19:27:43Z","lastAt":"2019-04-04T12:14:20Z","messageCount":18,"participants":["Baruch Siach","Joe Perches","Rasmus Villemoes","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"371658","messageId":"eec56beab016182fb78fbd367fcfa97f2ca6a5ff.1552764410.git.baruch@tkos.co.il","threadId":"50758","inReplyTo":null,"subject":"[PATCH] send-email: don't cc *-by lines with '-' prefix","fromName":"Baruch Siach","fromEmail":"baruch@tkos.co.il","sentAt":"2019-03-16T19:26:50Z","receivedAt":"2019-03-16T19:27:43Z","isPatch":true,"sender":{"key":"baruch@tkos.co.il","avatar":"https://avatars.githubusercontent.com/u/8298220?v=4"},"body":"Since commit ef0cc1df90f6b (\"send-email: also pick up cc addresses from\n-by trailers\") in git version 2.20, git send-email adds to cc list\naddresses from all *-by lines. As a side effect a line with\n'-Signed-off-by' is now also added to cc. This makes send-email pick\nlines from patches that remove patch files from the git repo. This is\ncommon in the Buildroot project that often removes (and adds) patch\nfiles that have 'Signed-off-by' in their patch description part.\n\nConsider only *-by lines that start with [a-z] (case insensitive) to\navoid unrelated addresses in cc.\n\nCc: Joe Perches \nCc: Rasmus Villemoes \nSigned-off-by: Baruch Siach \n---\n git-send-email.perl | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 8eb63b5a2f8d..5656ba83d9b1 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1693,7 +1693,7 @@ sub process_file {\n \t# Now parse the message body\n \twhile(<$fh>) {\n \t\t$message .= $_;\n-\t\tif (/^([a-z-]*-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-- \n2.20.1\n\n"},{"id":"371659","messageId":"8e28e622af4143b13a9bfa5c7a6df33d8baf1b5e.camel@perches.com","threadId":"50758","inReplyTo":"eec56beab016182fb78fbd367fcfa97f2ca6a5ff.1552764410.git.baruch@tkos.co.il","subject":"Re: [PATCH] send-email: don't cc *-by lines with '-' prefix","fromName":"Joe Perches","fromEmail":"joe@perches.com","sentAt":"2019-03-16T19:30:35Z","receivedAt":"2019-03-16T19:37:11Z","isPatch":true,"sender":{"key":"joe@perches.com","avatar":"https://avatars.githubusercontent.com/u/13122723?v=4"},"body":"On Sat, 2019-03-16 at 21:26 +0200, Baruch Siach wrote:\n> Since commit ef0cc1df90f6b (\"send-email: also pick up cc addresses from\n> -by trailers\") in git version 2.20, git send-email adds to cc list\n> addresses from all *-by lines. As a side effect a line with\n> '-Signed-off-by' is now also added to cc. This makes send-email pick\n> lines from patches that remove patch files from the git repo. This is\n> common in the Buildroot project that often removes (and adds) patch\n> files that have 'Signed-off-by' in their patch description part.\n\nWhy is such a line used and why shouldn't an author\nof a to-be-removed patch be cc'd?\n\n> \n> Consider only *-by lines that start with [a-z] (case insensitive) to\n> avoid unrelated addresses in cc.\n> \n> Cc: Joe Perches \n> Cc: Rasmus Villemoes \n> Signed-off-by: Baruch Siach \n> ---\n> git-send-email.perl | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 8eb63b5a2f8d..5656ba83d9b1 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -1693,7 +1693,7 @@ sub process_file {\n> \t# Now parse the message body\n> \twhile(<$fh>) {\n> \t\t$message .= $_;\n> -\t\tif (/^([a-z-]*-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":"371660","messageId":"87zhpuzjke.fsf@tarshish","threadId":"50758","inReplyTo":"8e28e622af4143b13a9bfa5c7a6df33d8baf1b5e.camel@perches.com","subject":"Re: [PATCH] send-email: don't cc *-by lines with '-' prefix","fromName":"Baruch Siach","fromEmail":"baruch@tkos.co.il","sentAt":"2019-03-16T19:49:05Z","receivedAt":"2019-03-16T19:49:09Z","isPatch":true,"sender":{"key":"baruch@tkos.co.il","avatar":"https://avatars.githubusercontent.com/u/8298220?v=4"},"body":"Hi Joe,\n\nOn Sat, Mar 16 2019, Joe Perches wrote:\n> On Sat, 2019-03-16 at 21:26 +0200, Baruch Siach wrote:\n>> Since commit ef0cc1df90f6b (\"send-email: also pick up cc addresses from\n>> -by trailers\") in git version 2.20, git send-email adds to cc list\n>> addresses from all *-by lines. As a side effect a line with\n>> '-Signed-off-by' is now also added to cc. This makes send-email pick\n>> lines from patches that remove patch files from the git repo. This is\n>> common in the Buildroot project that often removes (and adds) patch\n>> files that have 'Signed-off-by' in their patch description part.\n>\n> Why is such a line used and why shouldn't an author\n> of a to-be-removed patch be cc'd?\n\nThese lines are currently used because the '^([a-z-]*-by)' regexp\nmatches.\n\nBuildroot is a tool that build various software packages. The patches\nbeing removed are usually for packages that Buildroot patches to fix the\nbuild. These patches are often pulled from upstream git repo of\nrespective package. When the package version updates, the patch is\ndropped.\n\nWe don't cc patch authors when we add the patch in the first place,\nbecause the regexp does not match '+Signed-off-by'. I see not reason to\ncc them when we remove the patch.\n\nbaruch\n\n>> Consider only *-by lines that start with [a-z] (case insensitive) to\n>> avoid unrelated addresses in cc.\n>>\n>> Cc: Joe Perches \n>> Cc: Rasmus Villemoes \n>> Signed-off-by: Baruch Siach \n>> ---\n>> git-send-email.perl | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/git-send-email.perl b/git-send-email.perl\n>> index 8eb63b5a2f8d..5656ba83d9b1 100755\n>> --- a/git-send-email.perl\n>> +++ b/git-send-email.perl\n>> @@ -1693,7 +1693,7 @@ sub process_file {\n>> \t# Now parse the message body\n>> \twhile(<$fh>) {\n>> \t\t$message .= $_;\n>> -\t\tif (/^([a-z-]*-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\n--\n http://baruch.siach.name/blog/ ~. .~ Tk Open Systems\n=}------------------------------------------------ooO--U--Ooo------------{=\n - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -\n"},{"id":"371661","messageId":"fc14d671d5c3ff43fc43095930d4c762fe6f5002.camel@perches.com","threadId":"50758","inReplyTo":"87zhpuzjke.fsf@tarshish","subject":"Re: [PATCH] send-email: don't cc *-by lines with '-' prefix","fromName":"Joe Perches","fromEmail":"joe@perches.com","sentAt":"2019-03-16T19:59:55Z","receivedAt":"2019-03-16T20:00:00Z","isPatch":true,"sender":{"key":"joe@perches.com","avatar":"https://avatars.githubusercontent.com/u/13122723?v=4"},"body":"On Sat, 2019-03-16 at 21:49 +0200, Baruch Siach wrote:\n> Hi Joe,\n> \n> On Sat, Mar 16 2019, Joe Perches wrote:\n> > On Sat, 2019-03-16 at 21:26 +0200, Baruch Siach wrote:\n> > > Since commit ef0cc1df90f6b (\"send-email: also pick up cc addresses from\n> > > -by trailers\") in git version 2.20, git send-email adds to cc list\n> > > addresses from all *-by lines. As a side effect a line with\n> > > '-Signed-off-by' is now also added to cc. This makes send-email pick\n> > > lines from patches that remove patch files from the git repo. This is\n> > > common in the Buildroot project that often removes (and adds) patch\n> > > files that have 'Signed-off-by' in their patch description part.\n> > \n> > Why is such a line used and why shouldn't an author\n> > of a to-be-removed patch be cc'd?\n> \n> These lines are currently used because the '^([a-z-]*-by)' regexp\n> matches.\n\nThat part I already understood.\n\nI am not a buildroot user.\n\n> Buildroot is a tool that build various software packages. The patches\n> being removed are usually for packages that Buildroot patches to fix the\n> build. These patches are often pulled from upstream git repo of\n> respective package. When the package version updates, the patch is\n> dropped.\n> \n> We don't cc patch authors when we add the patch in the first place,\n> because the regexp does not match '+Signed-off-by'. I see not reason to\n> cc them when we remove the patch.\n\nSo buildroot uses '+Signed-off-by:' and '-Signed-off-by:' lines\nfor some internal purpose?\n\nWhy?\n\nhttps://buildroot.org/downloads/manual/manual.html\n\ndoesn't mention it.\n\n\n"},{"id":"371668","messageId":"87y35eziel.fsf@tarshish","threadId":"50758","inReplyTo":"fc14d671d5c3ff43fc43095930d4c762fe6f5002.camel@perches.com","subject":"Re: [PATCH] send-email: don't cc *-by lines with '-' prefix","fromName":"Baruch Siach","fromEmail":"baruch@tkos.co.il","sentAt":"2019-03-16T20:14:10Z","receivedAt":"2019-03-16T20:14:14Z","isPatch":true,"sender":{"key":"baruch@tkos.co.il","avatar":"https://avatars.githubusercontent.com/u/8298220?v=4"},"body":"Hi Joe,\n\nOn Sat, Mar 16 2019, Joe Perches wrote:\n> On Sat, 2019-03-16 at 21:49 +0200, Baruch Siach wrote:\n>> On Sat, Mar 16 2019, Joe Perches wrote:\n>> > On Sat, 2019-03-16 at 21:26 +0200, Baruch Siach wrote:\n>> > > Since commit ef0cc1df90f6b (\"send-email: also pick up cc addresses from\n>> > > -by trailers\") in git version 2.20, git send-email adds to cc list\n>> > > addresses from all *-by lines. As a side effect a line with\n>> > > '-Signed-off-by' is now also added to cc. This makes send-email pick\n>> > > lines from patches that remove patch files from the git repo. This is\n>> > > common in the Buildroot project that often removes (and adds) patch\n>> > > files that have 'Signed-off-by' in their patch description part.\n>> >\n>> > Why is such a line used and why shouldn't an author\n>> > of a to-be-removed patch be cc'd?\n>>\n>> These lines are currently used because the '^([a-z-]*-by)' regexp\n>> matches.\n>\n> That part I already understood.\n>\n> I am not a buildroot user.\n>\n>> Buildroot is a tool that build various software packages. The patches\n>> being removed are usually for packages that Buildroot patches to fix the\n>> build. These patches are often pulled from upstream git repo of\n>> respective package. When the package version updates, the patch is\n>> dropped.\n>>\n>> We don't cc patch authors when we add the patch in the first place,\n>> because the regexp does not match '+Signed-off-by'. I see not reason to\n>> cc them when we remove the patch.\n>\n> So buildroot uses '+Signed-off-by:' and '-Signed-off-by:' lines\n> for some internal purpose?\n>\n> Why?\n>\n> https://buildroot.org/downloads/manual/manual.html\n>\n> doesn't mention it.\n\nNo. Patches to the Buildroot project often add or remove patch\nfiles. See this one for example:\n\n http://lists.busybox.net/pipermail/buildroot/2019-March/244762.html\n\nIn this case 'git send-email' added Peter Korsgaard to cc because a\npatch file with his sign-off is removed.\n\n(mbox) Adding cc: Baruch Siach from line 'From: Baruch Siach '\n(body) Adding cc: Petr Vorel from line 'Cc: Petr Vorel '\n(body) Adding cc: Baruch Siach from line 'Signed-off-by: Baruch Siach '\n(body) Adding cc: Peter Korsgaard from line '-Signed-off-by: Peter Korsgaard '\n\nThe same Buildroot patch also adds a patch file that carries a number of\nsign-off lines. But 'git send-email didn't add these addresses to cc.\n\nIn both cases I see not point in adding these addresses to cc, since\nthey have little to do with the Buildroot patch. But only patch removal\ntriggers the regexp.\n\nbaruch\n\n--\n http://baruch.siach.name/blog/ ~. .~ Tk Open Systems\n=}------------------------------------------------ooO--U--Ooo------------{=\n - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -\n"},{"id":"371669","messageId":"2220d599992885eca1709f316a3e862c671301ff.camel@perches.com","threadId":"50758","inReplyTo":"87y35eziel.fsf@tarshish","subject":"Re: [PATCH] send-email: don't cc *-by lines with '-' prefix","fromName":"Joe Perches","fromEmail":"joe@perches.com","sentAt":"2019-03-16T20:23:30Z","receivedAt":"2019-03-16T20:23:36Z","isPatch":true,"sender":{"key":"joe@perches.com","avatar":"https://avatars.githubusercontent.com/u/13122723?v=4"},"body":"On Sat, 2019-03-16 at 22:14 +0200, Baruch Siach wrote:\n> Hi Joe,\n\nHello Baruch.\n\n> On Sat, Mar 16 2019, Joe Perches wrote:\n> > So buildroot uses '+Signed-off-by:' and '-Signed-off-by:' lines\n> > for some internal purpose?\n> > \n> > Why?\n> > \n> > https://buildroot.org/downloads/manual/manual.html\n> > \n> > doesn't mention it.\n> \n> No. Patches to the Buildroot project often add or remove patch\n> files. See this one for example:\n> \n> http://lists.busybox.net/pipermail/buildroot/2019-March/244762.html\n> \n> In this case 'git send-email' added Peter Korsgaard to cc because a\n> patch file with his sign-off is removed.\n> \n> (mbox) Adding cc: Baruch Siach from line 'From: Baruch Siach '\n> (body) Adding cc: Petr Vorel from line 'Cc: Petr Vorel '\n> (body) Adding cc: Baruch Siach from line 'Signed-off-by: Baruch Siach '\n> (body) Adding cc: Peter Korsgaard from line '-Signed-off-by: Peter Korsgaard '\n\nI see.\n\nIMO git send-email should not really be adding -by: lines from\nactual patch content but only from lines before any '^---'.\n\n\n"},{"id":"371748","messageId":"bc20070b-437a-9875-efd0-b4cad1413233@rasmusvillemoes.dk","threadId":"50758","inReplyTo":"eec56beab016182fb78fbd367fcfa97f2ca6a5ff.1552764410.git.baruch@tkos.co.il","subject":"Re: [PATCH] send-email: don't cc *-by lines with '-' prefix","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2019-03-17T19:27:57Z","receivedAt":"2019-03-17T19:28:02Z","isPatch":true,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"On 16/03/2019 20.26, Baruch Siach wrote:\n> Since commit ef0cc1df90f6b (\"send-email: also pick up cc addresses from\n> -by trailers\") in git version 2.20, git send-email adds to cc list\n> addresses from all *-by lines. As a side effect a line with\n> '-Signed-off-by' is now also added to cc. This makes send-email pick\n> lines from patches that remove patch files from the git repo. This is\n> common in the Buildroot project that often removes (and adds) patch\n> files that have 'Signed-off-by' in their patch description part.\n\nYocto/OpenEmbedded and other projects do the same\n\n> Consider only *-by lines that start with [a-z] (case insensitive) to\n> avoid unrelated addresses in cc.\n\nWhile I agree with Joe in principle that we really should not look\ninside the diff part, all lines there start with [ +-], so we wouldn't\nnormally pick up anything from that due to the anchoring. Except for the\nmisc-by regexp that added hyphens to grab Reported-and-tested-by and\nsimilar. So this is by far the simplest fix that doesn't hurt the common\nuse cases the misc-by handling was added to support, so\n\nAcked-by: Rasmus Villemoes \n\nRasmus\n"},{"id":"371759","messageId":"604795fe60991f22273cbb652eeeedc17985bc65.camel@perches.com","threadId":"50758","inReplyTo":"bc20070b-437a-9875-efd0-b4cad1413233@rasmusvillemoes.dk","subject":"Re: [PATCH] send-email: don't cc *-by lines with '-' prefix","fromName":"Joe Perches","fromEmail":"joe@perches.com","sentAt":"2019-03-18T01:56:08Z","receivedAt":"2019-03-18T01:56:14Z","isPatch":true,"sender":{"key":"joe@perches.com","avatar":"https://avatars.githubusercontent.com/u/13122723?v=4"},"body":"On Sun, 2019-03-17 at 20:27 +0100, Rasmus Villemoes wrote:\n> On 16/03/2019 20.26, Baruch Siach wrote:\n> > Since commit ef0cc1df90f6b (\"send-email: also pick up cc addresses from\n> > -by trailers\") in git version 2.20, git send-email adds to cc list\n> > addresses from all *-by lines. As a side effect a line with\n> > '-Signed-off-by' is now also added to cc. This makes send-email pick\n> > lines from patches that remove patch files from the git repo. This is\n> > common in the Buildroot project that often removes (and adds) patch\n> > files that have 'Signed-off-by' in their patch description part.\n> \n> Yocto/OpenEmbedded and other projects do the same\n> \n> > Consider only *-by lines that start with [a-z] (case insensitive) to\n> > avoid unrelated addresses in cc.\n> \n> While I agree with Joe in principle that we really should not look\n> inside the diff part, all lines there start with [ +-], so we wouldn't\n> normally pick up anything from that due to the anchoring. Except for the\n> misc-by regexp that added hyphens to grab Reported-and-tested-by and\n> similar. So this is by far the simplest fix that doesn't hurt the common\n> use cases the misc-by handling was added to support, so\n> \n> Acked-by: Rasmus Villemoes \n\nMy preference would be for correctness.\nI presume something like this isn't too onerous.\n---\n git-send-email.perl | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 8200d58cdc..83b0429576 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1697,9 +1697,10 @@ sub process_file {\n \t\t}\n \t}\n \t# Now parse the message body\n+\tmy $in_patch = 0;\n \twhile(<$fh>) {\n \t\t$message .= $_;\n-\t\tif (/^([a-z-]*-by|Cc): (.*)/i) {\n+\t\tif (!$in_patch && /^([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@@ -1725,6 +1726,8 @@ sub process_file {\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+\t\t} elsif (/^---/) {\n+\t\t\t$in_patch = 1;\n \t\t}\n \t}\n \tclose $fh;\n\n\n\n"},{"id":"371781","messageId":"xmqqh8c03dcz.fsf@gitster-ct.c.googlers.com","threadId":"50758","inReplyTo":"604795fe60991f22273cbb652eeeedc17985bc65.camel@perches.com","subject":"Re: [PATCH] send-email: don't cc *-by lines with '-' prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-18T06:28:44Z","receivedAt":"2019-03-18T06:28:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joe Perches writes:\n\n> My preference would be for correctness.\n> I presume something like this isn't too onerous.\n\nI am guessing that /^---/ is to stop at the three-dash line *OR*\nafter the initial handful of lines of the first diff header (as the\nlast resort) and that is why it is not looking for /^---$/.\n\nIf that is the case, I think it makes a lot of sense. It is a\ngeneral improvement not tied to the case that triggered this thread.\n\nIndependently, I think it makes sense to do something like\n\n\t/^([a-z][a-z-]*-by|Cc): (.*)/i\n\nto tighten the match to exclude a non-trailer; that would have been\nsufficient for the original case that triggered this thread.\n\n\n\n> ---\n> git-send-email.perl | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 8200d58cdc..83b0429576 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -1697,9 +1697,10 @@ sub process_file {\n> \t\t}\n> \t}\n> \t# Now parse the message body\n> +\tmy $in_patch = 0;\n> \twhile(<$fh>) {\n> \t\t$message .= $_;\n> -\t\tif (/^([a-z-]*-by|Cc): (.*)/i) {\n> +\t\tif (!$in_patch && /^([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> @@ -1725,6 +1726,8 @@ sub process_file {\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> +\t\t} elsif (/^---/) {\n> +\t\t\t$in_patch = 1;\n> \t\t}\n> \t}\n> \tclose $fh;\n"},{"id":"371788","messageId":"e2c7cc07618ecc753fe5939a90b562c97885af39.camel@perches.com","threadId":"50758","inReplyTo":"xmqqh8c03dcz.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] send-email: don't cc *-by lines with '-' prefix","fromName":"Joe Perches","fromEmail":"joe@perches.com","sentAt":"2019-03-18T07:02:50Z","receivedAt":"2019-03-18T07:02:55Z","isPatch":true,"sender":{"key":"joe@perches.com","avatar":"https://avatars.githubusercontent.com/u/13122723?v=4"},"body":"On Mon, 2019-03-18 at 15:28 +0900, Junio C Hamano wrote:\n> Joe Perches writes:\n> \n> > My preference would be for correctness.\n> > I presume something like this isn't too onerous.\n> \n> I am guessing that /^---/ is to stop at the three-dash line *OR*\n> after the initial handful of lines of the first diff header (as the\n> last resort) and that is why it is not looking for /^---$/.\n\nRight.\n\n> If that is the case, I think it makes a lot of sense. It is a\n> general improvement not tied to the case that triggered this thread.\n> \n> Independently, I think it makes sense to do something like\n> \n> \t/^([a-z][a-z-]*-by|Cc): (.*)/i\n> \n> to tighten the match to exclude a non-trailer; that would have been\n> sufficient for the original case that triggered this thread.\n\n\n"},{"id":"373095","messageId":"874l7ekynt.fsf@tarshish","threadId":"50758","inReplyTo":"xmqqh8c03dcz.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] send-email: don't cc *-by lines with '-' prefix","fromName":"Baruch Siach","fromEmail":"baruch@tkos.co.il","sentAt":"2019-04-04T07:38:46Z","receivedAt":"2019-04-04T07:38:51Z","isPatch":true,"sender":{"key":"baruch@tkos.co.il","avatar":"https://avatars.githubusercontent.com/u/8298220?v=4"},"body":"Hi Junio,\n\nOn Mon, Mar 18 2019, Junio C. Hamano wrote:\n> Joe Perches writes:\n>\n>> My preference would be for correctness.\n>> I presume something like this isn't too onerous.\n>\n> I am guessing that /^---/ is to stop at the three-dash line *OR*\n> after the initial handful of lines of the first diff header (as the\n> last resort) and that is why it is not looking for /^---$/.\n>\n> If that is the case, I think it makes a lot of sense. It is a\n> general improvement not tied to the case that triggered this thread.\n>\n> Independently, I think it makes sense to do something like\n>\n> \t/^([a-z][a-z-]*-by|Cc): (.*)/i\n>\n> to tighten the match to exclude a non-trailer; that would have been\n> sufficient for the original case that triggered this thread.\n\nIs there anything I need to do more to get this fix applied for the next\ngit release?\n\nThanks,\nbaruch\n\n>> ---\n>> git-send-email.perl | 5 ++++-\n>> 1 file changed, 4 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/git-send-email.perl b/git-send-email.perl\n>> index 8200d58cdc..83b0429576 100755\n>> --- a/git-send-email.perl\n>> +++ b/git-send-email.perl\n>> @@ -1697,9 +1697,10 @@ sub process_file {\n>> \t\t}\n>> \t}\n>> \t# Now parse the message body\n>> +\tmy $in_patch = 0;\n>> \twhile(<$fh>) {\n>> \t\t$message .= $_;\n>> -\t\tif (/^([a-z-]*-by|Cc): (.*)/i) {\n>> +\t\tif (!$in_patch && /^([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>> @@ -1725,6 +1726,8 @@ sub process_file {\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>> +\t\t} elsif (/^---/) {\n>> +\t\t\t$in_patch = 1;\n>> \t\t}\n>> \t}\n>> \tclose $fh;\n\n\n--\n http://baruch.siach.name/blog/ ~. .~ Tk Open Systems\n=}------------------------------------------------ooO--U--Ooo------------{=\n - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -\n"},{"id":"373103","messageId":"xmqqk1gaf7oe.fsf@gitster-ct.c.googlers.com","threadId":"50758","inReplyTo":"874l7ekynt.fsf@tarshish","subject":"Re: [PATCH] send-email: don't cc *-by lines with '-' prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-04T09:20:33Z","receivedAt":"2019-04-04T09:20:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Baruch Siach writes:\n\n>> Independently, I think it makes sense to do something like\n>>\n>> \t/^([a-z][a-z-]*-by|Cc): (.*)/i\n>>\n>> to tighten the match to exclude a non-trailer; that would have been\n>> sufficient for the original case that triggered this thread.\n>\n> Is there anything I need to do more to get this fix applied for the next\n> git release?\n\nGet \"this\" fix applied? I think we should tighten the regexp to\nexclude a non-trailer, which would have been sufficient for the\noriginal case without anything else in \"this\" fix. So in short, I\ndo not think \"this\" fix won't be applied without further tweaking\n;-)\n"},{"id":"373104","messageId":"87zhp6jf2o.fsf@tarshish","threadId":"50758","inReplyTo":"xmqqk1gaf7oe.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] send-email: don't cc *-by lines with '-' prefix","fromName":"Baruch Siach","fromEmail":"baruch@tkos.co.il","sentAt":"2019-04-04T09:27:11Z","receivedAt":"2019-04-04T09:27:16Z","isPatch":true,"sender":{"key":"baruch@tkos.co.il","avatar":"https://avatars.githubusercontent.com/u/8298220?v=4"},"body":"Hi Junio,\n\nOn Thu, Apr 04 2019, Junio C. Hamano wrote:\n> Baruch Siach writes:\n>\n>>> Independently, I think it makes sense to do something like\n>>>\n>>> \t/^([a-z][a-z-]*-by|Cc): (.*)/i\n>>>\n>>> to tighten the match to exclude a non-trailer; that would have been\n>>> sufficient for the original case that triggered this thread.\n>>\n>> Is there anything I need to do more to get this fix applied for the next\n>> git release?\n>\n> Get \"this\" fix applied? I think we should tighten the regexp to\n> exclude a non-trailer, which would have been sufficient for the\n> original case without anything else in \"this\" fix. So in short, I\n> do not think \"this\" fix won't be applied without further tweaking\n> ;-)\n\nThis is exactly what \"this\" patch (referenced in the title of \"this\"\nthread) is doing:\n\n https://public-inbox.org/git/eec56beab016182fb78fbd367fcfa97f2ca6a5ff.1552764410.git.baruch@tkos.co.il/\n\nAm I missing something?\n\nbaruch\n\n--\n http://baruch.siach.name/blog/ ~. .~ Tk Open Systems\n=}------------------------------------------------ooO--U--Ooo------------{=\n - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -\n"},{"id":"373107","messageId":"xmqq7ecaf6pr.fsf@gitster-ct.c.googlers.com","threadId":"50758","inReplyTo":"87zhp6jf2o.fsf@tarshish","subject":"Re: [PATCH] send-email: don't cc *-by lines with '-' prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-04T09:41:20Z","receivedAt":"2019-04-04T09:41:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Baruch Siach writes:\n\n> Hi Junio,\n>\n> On Thu, Apr 04 2019, Junio C. Hamano wrote:\n>> Baruch Siach writes:\n>>\n>>>> Independently, I think it makes sense to do something like\n>>>>\n>>>> \t/^([a-z][a-z-]*-by|Cc): (.*)/i\n>>>>\n>>>> to tighten the match to exclude a non-trailer; that would have been\n>>>> sufficient for the original case that triggered this thread.\n>>>\n>>> Is there anything I need to do more to get this fix applied for the next\n>>> git release?\n>>\n>> Get \"this\" fix applied? I think we should tighten the regexp to\n>> exclude a non-trailer, which would have been sufficient for the\n>> original case without anything else in \"this\" fix. So in short, I\n>> do not think \"this\" fix won't be applied without further tweaking\n>> ;-)\n>\n> This is exactly what \"this\" patch (referenced in the title of \"this\"\n> thread) is doing:\n>\n> https://public-inbox.org/git/eec56beab016182fb78fbd367fcfa97f2ca6a5ff.1552764410.git.baruch@tkos.co.il/\n>\n> Am I missing something?\n\nThat is totally outside of the in-reply-to/references trail of your\nping message, and what I saw in the message you were quoting in your\n'ping' was\n\n>> \t\t$message .= $_;\n>> -\t\tif (/^([a-z-]*-by|Cc): (.*)/i) {\n>> +\t\tif (!$in_patch && /^([a-z-]*-by|Cc): (.*)/i) {\n>> \t\t\tchomp;\n\nwhich is a lot looser than the suggested \"the beginning must be\nalpha\" pattern.\n\n"},{"id":"373108","messageId":"dd8160f8-0e5e-1024-53c1-1a9f23423af5@rasmusvillemoes.dk","threadId":"50758","inReplyTo":"87zhp6jf2o.fsf@tarshish","subject":"Re: [PATCH] send-email: don't cc *-by lines with '-' prefix","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2019-04-04T09:42:23Z","receivedAt":"2019-04-04T09:42:30Z","isPatch":true,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"On 04/04/2019 11.27, Baruch Siach wrote:\n> Hi Junio,\n> \n> On Thu, Apr 04 2019, Junio C. Hamano wrote:\n>> Baruch Siach writes:\n>>\n>>>> Independently, I think it makes sense to do something like\n>>>>\n>>>> \t/^([a-z][a-z-]*-by|Cc): (.*)/i\n>>>>\n>>>> to tighten the match to exclude a non-trailer; that would have been\n>>>> sufficient for the original case that triggered this thread.\n>>>\n>>> Is there anything I need to do more to get this fix applied for the next\n>>> git release?\n>>\n>> Get \"this\" fix applied? I think we should tighten the regexp to\n>> exclude a non-trailer, which would have been sufficient for the\n>> original case without anything else in \"this\" fix. So in short, I\n>> do not think \"this\" fix won't be applied without further tweaking\n>> ;-)\n> \n> This is exactly what \"this\" patch (referenced in the title of \"this\"\n> thread) is doing:\n> \n> https://public-inbox.org/git/eec56beab016182fb78fbd367fcfa97f2ca6a5ff.1552764410.git.baruch@tkos.co.il/\n> \n> Am I missing something?\n\nMy ack for Baruch's original patch, which AFAICT is identical with\nJunio's suggestion, still stands. FWIW, I'm against Joe's suggestion of\nstopping at a line matching /^---/, since it's not unlikely somebody\ndoes something like\n\n---- dmesg output ----\nbla bla\n----\n\nin the commit message.\n\nSince all lines (except for some of the diff header lines) in the patch\npart begin with space, - or +, insisting on a the line starting with a\nletter should be sufficient for excluding any random Foo-by lines that\nmay appear in the patch part.\n\nRasmus\n"},{"id":"373109","messageId":"xmqq36myf6fg.fsf@gitster-ct.c.googlers.com","threadId":"50758","inReplyTo":"dd8160f8-0e5e-1024-53c1-1a9f23423af5@rasmusvillemoes.dk","subject":"Re: [PATCH] send-email: don't cc *-by lines with '-' prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-04T09:47:31Z","receivedAt":"2019-04-04T09:47:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rasmus Villemoes writes:\n\n> My ack for Baruch's original patch, which AFAICT is identical with\n> Junio's suggestion, still stands. FWIW, I'm against Joe's suggestion of\n> stopping at a line matching /^---/, since it's not unlikely somebody\n> does something like\n>\n> ---- dmesg output ----\n> bla bla\n> ----\n>\n> in the commit message.\n\nHmph. That does make sort-of sense ;-)\n\n"},{"id":"373110","messageId":"xmqqy34qdrr2.fsf@gitster-ct.c.googlers.com","threadId":"50758","inReplyTo":"eec56beab016182fb78fbd367fcfa97f2ca6a5ff.1552764410.git.baruch@tkos.co.il","subject":"Re: [PATCH] send-email: don't cc *-by lines with '-' prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-04T09:49:53Z","receivedAt":"2019-04-04T09:50:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Baruch Siach writes:\n\n> Since commit ef0cc1df90f6b (\"send-email: also pick up cc addresses from\n> -by trailers\") in git version 2.20, git send-email adds to cc list\n> addresses from all *-by lines. As a side effect a line with\n> '-Signed-off-by' is now also added to cc. This makes send-email pick\n> lines from patches that remove patch files from the git repo. This is\n> common in the Buildroot project that often removes (and adds) patch\n> files that have 'Signed-off-by' in their patch description part.\n>\n> Consider only *-by lines that start with [a-z] (case insensitive) to\n> avoid unrelated addresses in cc.\n>\n> Cc: Joe Perches \n> Cc: Rasmus Villemoes \n> Signed-off-by: Baruch Siach \n> ---\n> git-send-email.perl | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 8eb63b5a2f8d..5656ba83d9b1 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -1693,7 +1693,7 @@ sub process_file {\n> \t# Now parse the message body\n> \twhile(<$fh>) {\n> \t\t$message .= $_;\n> -\t\tif (/^([a-z-]*-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\nOK, this fell through the cracks (and it did not help that a recent\nping message did not come as a response to it, but as a response to\nanother thread with an alternative implementation). Will apply and\ncook in 'next' to see what happens.\n\nFYI, being in 'next' does not mean it will be in the next release.\nBeing in 'master' usually does, though.\n"},{"id":"373116","messageId":"20190404121416.GC22324@sigill.intra.peff.net","threadId":"50758","inReplyTo":"dd8160f8-0e5e-1024-53c1-1a9f23423af5@rasmusvillemoes.dk","subject":"Re: [PATCH] send-email: don't cc *-by lines with '-' prefix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-04T12:14:16Z","receivedAt":"2019-04-04T12:14:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 04, 2019 at 11:42:23AM +0200, Rasmus Villemoes wrote:\n\n> My ack for Baruch's original patch, which AFAICT is identical with\n> Junio's suggestion, still stands. FWIW, I'm against Joe's suggestion of\n> stopping at a line matching /^---/, since it's not unlikely somebody\n> does something like\n> \n> ---- dmesg output ----\n> bla bla\n> ----\n> \n> in the commit message.\n\nKeep in mind that on the receiving end, we are going to stop reading the\ncommit message at a triple-dash, too, which is done as (from\nmailinfo.c's patchbreak()):\n\n /^---( [^\\s]|\\s*$)/\n\nSo it might make sense to use the same rule here. That said:\n\n> Since all lines (except for some of the diff header lines) in the patch\n> part begin with space, - or +, insisting on a the line starting with a\n> letter should be sufficient for excluding any random Foo-by lines that\n> may appear in the patch part.\n\nYeah, I think this mostly makes it a non-issue, unless we care about\nefficiency (and I doubt it is even measurable).\n\nTechnically you could have other cruft after the diff, too. But I think\nputting \"signed-off-by: somebody\" in your email sig is a case of \"if it\nhurts, don't do it\".\n\n-Peff\n"}]}