[PATCH] send-email: don't cc *-by lines with '-' prefix

[PATCH] send-email: don't cc *-by lines with '-' prefix

[Baruch Siach]

Since commit ef0cc1df90f6b ("send-email: also pick up cc addresses from
-by trailers") in git version 2.20, git send-email adds to cc list
addresses from all *-by lines. As a side effect a line with
'-Signed-off-by' is now also added to cc. This makes send-email pick
lines from patches that remove patch files from the git repo. This is
common in the Buildroot project that often removes (and adds) patch
files that have 'Signed-off-by' in their patch description part.

Consider only *-by lines that start with [a-z] (case insensitive) to
avoid unrelated addresses in cc.

Cc: Joe Perches <joe@perches.com>
Cc: Rasmus Villemoes <rv@rasmusvillemoes.dk>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 git-send-email.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 8eb63b5a2f8d..5656ba83d9b1 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1693,7 +1693,7 @@ sub process_file {
 	# Now parse the message body
 	while(<$fh>) {
 		$message .=  $_;
-		if (/^([a-z-]*-by|Cc): (.*)/i) {
+		if (/^([a-z][a-z-]*-by|Cc): (.*)/i) {
 			chomp;
 			my ($what, $c) = ($1, $2);
 			# strip garbage for the address we'll use:
-- 
2.20.1

Re: [PATCH] send-email: don't cc *-by lines with '-' prefix

[Joe Perches]

On Sat, 2019-03-16 at 21:26 +0200, Baruch Siach wrote:
> Since commit ef0cc1df90f6b ("send-email: also pick up cc addresses from
> -by trailers") in git version 2.20, git send-email adds to cc list
> addresses from all *-by lines. As a side effect a line with
> '-Signed-off-by' is now also added to cc. This makes send-email pick
> lines from patches that remove patch files from the git repo. This is
> common in the Buildroot project that often removes (and adds) patch
> files that have 'Signed-off-by' in their patch description part.

Why is such a line used and why shouldn't an author
of a to-be-removed patch be cc'd?

> 
> Consider only *-by lines that start with [a-z] (case insensitive) to
> avoid unrelated addresses in cc.
> 
> Cc: Joe Perches <joe@perches.com>
> Cc: Rasmus Villemoes <rv@rasmusvillemoes.dk>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  git-send-email.perl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 8eb63b5a2f8d..5656ba83d9b1 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1693,7 +1693,7 @@ sub process_file {
>  	# Now parse the message body
>  	while(<$fh>) {
>  		$message .=  $_;
> -		if (/^([a-z-]*-by|Cc): (.*)/i) {
> +		if (/^([a-z][a-z-]*-by|Cc): (.*)/i) {
>  			chomp;
>  			my ($what, $c) = ($1, $2);
>  			# strip garbage for the address we'll use:

Re: [PATCH] send-email: don't cc *-by lines with '-' prefix

[Baruch Siach]

Hi Joe,

On Sat, Mar 16 2019, Joe Perches wrote:
> On Sat, 2019-03-16 at 21:26 +0200, Baruch Siach wrote:
>> Since commit ef0cc1df90f6b ("send-email: also pick up cc addresses from
>> -by trailers") in git version 2.20, git send-email adds to cc list
>> addresses from all *-by lines. As a side effect a line with
>> '-Signed-off-by' is now also added to cc. This makes send-email pick
>> lines from patches that remove patch files from the git repo. This is
>> common in the Buildroot project that often removes (and adds) patch
>> files that have 'Signed-off-by' in their patch description part.
>
> Why is such a line used and why shouldn't an author
> of a to-be-removed patch be cc'd?

These lines are currently used because the '^([a-z-]*-by)' regexp
matches.

Buildroot is a tool that build various software packages. The patches
being removed are usually for packages that Buildroot patches to fix the
build. These patches are often pulled from upstream git repo of
respective package. When the package version updates, the patch is
dropped.

We don't cc patch authors when we add the patch in the first place,
because the regexp does not match '+Signed-off-by'. I see not reason to
cc them when we remove the patch.

baruch

>> Consider only *-by lines that start with [a-z] (case insensitive) to
>> avoid unrelated addresses in cc.
>>
>> Cc: Joe Perches <joe@perches.com>
>> Cc: Rasmus Villemoes <rv@rasmusvillemoes.dk>
>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>> ---
>>  git-send-email.perl | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index 8eb63b5a2f8d..5656ba83d9b1 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -1693,7 +1693,7 @@ sub process_file {
>>  	# Now parse the message body
>>  	while(<$fh>) {
>>  		$message .=  $_;
>> -		if (/^([a-z-]*-by|Cc): (.*)/i) {
>> +		if (/^([a-z][a-z-]*-by|Cc): (.*)/i) {
>>  			chomp;
>>  			my ($what, $c) = ($1, $2);
>>  			# strip garbage for the address we'll use:


--
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [PATCH] send-email: don't cc *-by lines with '-' prefix

[Joe Perches]

On Sat, 2019-03-16 at 21:49 +0200, Baruch Siach wrote:
> Hi Joe,
> 
> On Sat, Mar 16 2019, Joe Perches wrote:
> > On Sat, 2019-03-16 at 21:26 +0200, Baruch Siach wrote:
> > > Since commit ef0cc1df90f6b ("send-email: also pick up cc addresses from
> > > -by trailers") in git version 2.20, git send-email adds to cc list
> > > addresses from all *-by lines. As a side effect a line with
> > > '-Signed-off-by' is now also added to cc. This makes send-email pick
> > > lines from patches that remove patch files from the git repo. This is
> > > common in the Buildroot project that often removes (and adds) patch
> > > files that have 'Signed-off-by' in their patch description part.
> > 
> > Why is such a line used and why shouldn't an author
> > of a to-be-removed patch be cc'd?
> 
> These lines are currently used because the '^([a-z-]*-by)' regexp
> matches.

That part I already understood.

I am not a buildroot user.

> Buildroot is a tool that build various software packages. The patches
> being removed are usually for packages that Buildroot patches to fix the
> build. These patches are often pulled from upstream git repo of
> respective package. When the package version updates, the patch is
> dropped.
> 
> We don't cc patch authors when we add the patch in the first place,
> because the regexp does not match '+Signed-off-by'. I see not reason to
> cc them when we remove the patch.

So buildroot uses '+Signed-off-by:' and '-Signed-off-by:' lines
for some internal purpose?

Why?

https://buildroot.org/downloads/manual/manual.html

doesn't mention it.

Re: [PATCH] send-email: don't cc *-by lines with '-' prefix

[Baruch Siach]

Hi Joe,

On Sat, Mar 16 2019, Joe Perches wrote:
> On Sat, 2019-03-16 at 21:49 +0200, Baruch Siach wrote:
>> On Sat, Mar 16 2019, Joe Perches wrote:
>> > On Sat, 2019-03-16 at 21:26 +0200, Baruch Siach wrote:
>> > > Since commit ef0cc1df90f6b ("send-email: also pick up cc addresses from
>> > > -by trailers") in git version 2.20, git send-email adds to cc list
>> > > addresses from all *-by lines. As a side effect a line with
>> > > '-Signed-off-by' is now also added to cc. This makes send-email pick
>> > > lines from patches that remove patch files from the git repo. This is
>> > > common in the Buildroot project that often removes (and adds) patch
>> > > files that have 'Signed-off-by' in their patch description part.
>> >
>> > Why is such a line used and why shouldn't an author
>> > of a to-be-removed patch be cc'd?
>>
>> These lines are currently used because the '^([a-z-]*-by)' regexp
>> matches.
>
> That part I already understood.
>
> I am not a buildroot user.
>
>> Buildroot is a tool that build various software packages. The patches
>> being removed are usually for packages that Buildroot patches to fix the
>> build. These patches are often pulled from upstream git repo of
>> respective package. When the package version updates, the patch is
>> dropped.
>>
>> We don't cc patch authors when we add the patch in the first place,
>> because the regexp does not match '+Signed-off-by'. I see not reason to
>> cc them when we remove the patch.
>
> So buildroot uses '+Signed-off-by:' and '-Signed-off-by:' lines
> for some internal purpose?
>
> Why?
>
> https://buildroot.org/downloads/manual/manual.html
>
> doesn't mention it.

No. Patches to the Buildroot project often add or remove patch
files. See this one for example:

  http://lists.busybox.net/pipermail/buildroot/2019-March/244762.html

In this case 'git send-email' added Peter Korsgaard to cc because a
patch file with his sign-off is removed.

(mbox) Adding cc: Baruch Siach <baruch@tkos.co.il> from line 'From: Baruch Siach <baruch@tkos.co.il>'
(body) Adding cc: Petr Vorel <petr.vorel@gmail.com> from line 'Cc: Petr Vorel <petr.vorel@gmail.com>'
(body) Adding cc: Baruch Siach <baruch@tkos.co.il> from line 'Signed-off-by: Baruch Siach <baruch@tkos.co.il>'
(body) Adding cc: Peter Korsgaard <peter@korsgaard.com> from line '-Signed-off-by: Peter Korsgaard <peter@korsgaard.com>'

The same Buildroot patch also adds a patch file that carries a number of
sign-off lines. But 'git send-email didn't add these addresses to cc.

In both cases I see not point in adding these addresses to cc, since
they have little to do with the Buildroot patch. But only patch removal
triggers the regexp.

baruch

--
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [PATCH] send-email: don't cc *-by lines with '-' prefix

[Joe Perches]

On Sat, 2019-03-16 at 22:14 +0200, Baruch Siach wrote:
> Hi Joe,

Hello Baruch.

> On Sat, Mar 16 2019, Joe Perches wrote:
> > So buildroot uses '+Signed-off-by:' and '-Signed-off-by:' lines
> > for some internal purpose?
> > 
> > Why?
> > 
> > https://buildroot.org/downloads/manual/manual.html
> > 
> > doesn't mention it.
> 
> No. Patches to the Buildroot project often add or remove patch
> files. See this one for example:
> 
>   http://lists.busybox.net/pipermail/buildroot/2019-March/244762.html
> 
> In this case 'git send-email' added Peter Korsgaard to cc because a
> patch file with his sign-off is removed.
> 
> (mbox) Adding cc: Baruch Siach <baruch@tkos.co.il> from line 'From: Baruch Siach <baruch@tkos.co.il>'
> (body) Adding cc: Petr Vorel <petr.vorel@gmail.com> from line 'Cc: Petr Vorel <petr.vorel@gmail.com>'
> (body) Adding cc: Baruch Siach <baruch@tkos.co.il> from line 'Signed-off-by: Baruch Siach <baruch@tkos.co.il>'
> (body) Adding cc: Peter Korsgaard <peter@korsgaard.com> from line '-Signed-off-by: Peter Korsgaard <peter@korsgaard.com>'

I see.

IMO git send-email should not really be adding -by: lines from
actual patch content but only from lines before any '^---'.

Re: [PATCH] send-email: don't cc *-by lines with '-' prefix

[Rasmus Villemoes]

On 16/03/2019 20.26, Baruch Siach wrote:
> Since commit ef0cc1df90f6b ("send-email: also pick up cc addresses from
> -by trailers") in git version 2.20, git send-email adds to cc list
> addresses from all *-by lines. As a side effect a line with
> '-Signed-off-by' is now also added to cc. This makes send-email pick
> lines from patches that remove patch files from the git repo. This is
> common in the Buildroot project that often removes (and adds) patch
> files that have 'Signed-off-by' in their patch description part.

Yocto/OpenEmbedded and other projects do the same

> Consider only *-by lines that start with [a-z] (case insensitive) to
> avoid unrelated addresses in cc.

While I agree with Joe in principle that we really should not look
inside the diff part, all lines there start with [ +-], so we wouldn't
normally pick up anything from that due to the anchoring. Except for the
misc-by regexp that added hyphens to grab Reported-and-tested-by and
similar. So this is by far the simplest fix that doesn't hurt the common
use cases the misc-by handling was added to support, so

Acked-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>

Rasmus

Re: [PATCH] send-email: don't cc *-by lines with '-' prefix

[Joe Perches]

On Sun, 2019-03-17 at 20:27 +0100, Rasmus Villemoes wrote:
> On 16/03/2019 20.26, Baruch Siach wrote:
> > Since commit ef0cc1df90f6b ("send-email: also pick up cc addresses from
> > -by trailers") in git version 2.20, git send-email adds to cc list
> > addresses from all *-by lines. As a side effect a line with
> > '-Signed-off-by' is now also added to cc. This makes send-email pick
> > lines from patches that remove patch files from the git repo. This is
> > common in the Buildroot project that often removes (and adds) patch
> > files that have 'Signed-off-by' in their patch description part.
> 
> Yocto/OpenEmbedded and other projects do the same
> 
> > Consider only *-by lines that start with [a-z] (case insensitive) to
> > avoid unrelated addresses in cc.
> 
> While I agree with Joe in principle that we really should not look
> inside the diff part, all lines there start with [ +-], so we wouldn't
> normally pick up anything from that due to the anchoring. Except for the
> misc-by regexp that added hyphens to grab Reported-and-tested-by and
> similar. So this is by far the simplest fix that doesn't hurt the common
> use cases the misc-by handling was added to support, so
> 
> Acked-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>

My preference would be for correctness.
I presume something like this isn't too onerous.
---
 git-send-email.perl | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 8200d58cdc..83b0429576 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1697,9 +1697,10 @@ sub process_file {
 		}
 	}
 	# Now parse the message body
+	my $in_patch = 0;
 	while(<$fh>) {
 		$message .=  $_;
-		if (/^([a-z-]*-by|Cc): (.*)/i) {
+		if (!$in_patch && /^([a-z-]*-by|Cc): (.*)/i) {
 			chomp;
 			my ($what, $c) = ($1, $2);
 			# strip garbage for the address we'll use:
@@ -1725,6 +1726,8 @@ sub process_file {
 			push @cc, $c;
 			printf(__("(body) Adding cc: %s from line '%s'\n"),
 				$c, $_) unless $quiet;
+		} elsif (/^---/) {
+			$in_patch = 1;
 		}
 	}
 	close $fh;

Re: [PATCH] send-email: don't cc *-by lines with '-' prefix

[Junio C Hamano]

Joe Perches <joe@perches.com> writes:

> My preference would be for correctness.
> I presume something like this isn't too onerous.

I am guessing that /^---/ is to stop at the three-dash line *OR*
after the initial handful of lines of the first diff header (as the
last resort) and that is why it is not looking for /^---$/.

If that is the case, I think it makes a lot of sense.  It is a
general improvement not tied to the case that triggered this thread.

Independently, I think it makes sense to do something like

	/^([a-z][a-z-]*-by|Cc): (.*)/i

to tighten the match to exclude a non-trailer; that would have been
sufficient for the original case that triggered this thread.



> ---
>  git-send-email.perl | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 8200d58cdc..83b0429576 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1697,9 +1697,10 @@ sub process_file {
>  		}
>  	}
>  	# Now parse the message body
> +	my $in_patch = 0;
>  	while(<$fh>) {
>  		$message .=  $_;
> -		if (/^([a-z-]*-by|Cc): (.*)/i) {
> +		if (!$in_patch && /^([a-z-]*-by|Cc): (.*)/i) {
>  			chomp;
>  			my ($what, $c) = ($1, $2);
>  			# strip garbage for the address we'll use:
> @@ -1725,6 +1726,8 @@ sub process_file {
>  			push @cc, $c;
>  			printf(__("(body) Adding cc: %s from line '%s'\n"),
>  				$c, $_) unless $quiet;
> +		} elsif (/^---/) {
> +			$in_patch = 1;
>  		}
>  	}
>  	close $fh;

Re: [PATCH] send-email: don't cc *-by lines with '-' prefix

[Joe Perches]

On Mon, 2019-03-18 at 15:28 +0900, Junio C Hamano wrote:
> Joe Perches <joe@perches.com> writes:
> 
> > My preference would be for correctness.
> > I presume something like this isn't too onerous.
> 
> I am guessing that /^---/ is to stop at the three-dash line *OR*
> after the initial handful of lines of the first diff header (as the
> last resort) and that is why it is not looking for /^---$/.

Right.

> If that is the case, I think it makes a lot of sense.  It is a
> general improvement not tied to the case that triggered this thread.
> 
> Independently, I think it makes sense to do something like
> 
> 	/^([a-z][a-z-]*-by|Cc): (.*)/i
> 
> to tighten the match to exclude a non-trailer; that would have been
> sufficient for the original case that triggered this thread.

Re: [PATCH] send-email: don't cc *-by lines with '-' prefix

[Baruch Siach]

Hi Junio,

On Mon, Mar 18 2019, Junio C. Hamano wrote:
> Joe Perches <joe@perches.com> writes:
>
>> My preference would be for correctness.
>> I presume something like this isn't too onerous.
>
> I am guessing that /^---/ is to stop at the three-dash line *OR*
> after the initial handful of lines of the first diff header (as the
> last resort) and that is why it is not looking for /^---$/.
>
> If that is the case, I think it makes a lot of sense.  It is a
> general improvement not tied to the case that triggered this thread.
>
> Independently, I think it makes sense to do something like
>
> 	/^([a-z][a-z-]*-by|Cc): (.*)/i
>
> to tighten the match to exclude a non-trailer; that would have been
> sufficient for the original case that triggered this thread.

Is there anything I need to do more to get this fix applied for the next
git release?

Thanks,
baruch

>> ---
>>  git-send-email.perl | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index 8200d58cdc..83b0429576 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -1697,9 +1697,10 @@ sub process_file {
>>  		}
>>  	}
>>  	# Now parse the message body
>> +	my $in_patch = 0;
>>  	while(<$fh>) {
>>  		$message .=  $_;
>> -		if (/^([a-z-]*-by|Cc): (.*)/i) {
>> +		if (!$in_patch && /^([a-z-]*-by|Cc): (.*)/i) {
>>  			chomp;
>>  			my ($what, $c) = ($1, $2);
>>  			# strip garbage for the address we'll use:
>> @@ -1725,6 +1726,8 @@ sub process_file {
>>  			push @cc, $c;
>>  			printf(__("(body) Adding cc: %s from line '%s'\n"),
>>  				$c, $_) unless $quiet;
>> +		} elsif (/^---/) {
>> +			$in_patch = 1;
>>  		}
>>  	}
>>  	close $fh;


--
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [PATCH] send-email: don't cc *-by lines with '-' prefix

[Junio C Hamano]

Baruch Siach <baruch@tkos.co.il> writes:

>> Independently, I think it makes sense to do something like
>>
>> 	/^([a-z][a-z-]*-by|Cc): (.*)/i
>>
>> to tighten the match to exclude a non-trailer; that would have been
>> sufficient for the original case that triggered this thread.
>
> Is there anything I need to do more to get this fix applied for the next
> git release?

Get "this" fix applied?  I think we should tighten the regexp to
exclude a non-trailer, which would have been sufficient for the
original case without anything else in "this" fix.  So in short, I
do not think "this" fix won't be applied without further tweaking
;-)

Re: [PATCH] send-email: don't cc *-by lines with '-' prefix

[Baruch Siach]

Hi Junio,

On Thu, Apr 04 2019, Junio C. Hamano wrote:
> Baruch Siach <baruch@tkos.co.il> writes:
>
>>> Independently, I think it makes sense to do something like
>>>
>>> 	/^([a-z][a-z-]*-by|Cc): (.*)/i
>>>
>>> to tighten the match to exclude a non-trailer; that would have been
>>> sufficient for the original case that triggered this thread.
>>
>> Is there anything I need to do more to get this fix applied for the next
>> git release?
>
> Get "this" fix applied?  I think we should tighten the regexp to
> exclude a non-trailer, which would have been sufficient for the
> original case without anything else in "this" fix.  So in short, I
> do not think "this" fix won't be applied without further tweaking
> ;-)

This is exactly what "this" patch (referenced in the title of "this"
thread) is doing:

  https://public-inbox.org/git/eec56beab016182fb78fbd367fcfa97f2ca6a5ff.1552764410.git.baruch@tkos.co.il/

Am I missing something?

baruch

--
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [PATCH] send-email: don't cc *-by lines with '-' prefix

[Junio C Hamano]

Baruch Siach <baruch@tkos.co.il> writes:

> Hi Junio,
>
> On Thu, Apr 04 2019, Junio C. Hamano wrote:
>> Baruch Siach <baruch@tkos.co.il> writes:
>>
>>>> Independently, I think it makes sense to do something like
>>>>
>>>> 	/^([a-z][a-z-]*-by|Cc): (.*)/i
>>>>
>>>> to tighten the match to exclude a non-trailer; that would have been
>>>> sufficient for the original case that triggered this thread.
>>>
>>> Is there anything I need to do more to get this fix applied for the next
>>> git release?
>>
>> Get "this" fix applied?  I think we should tighten the regexp to
>> exclude a non-trailer, which would have been sufficient for the
>> original case without anything else in "this" fix.  So in short, I
>> do not think "this" fix won't be applied without further tweaking
>> ;-)
>
> This is exactly what "this" patch (referenced in the title of "this"
> thread) is doing:
>
>   https://public-inbox.org/git/eec56beab016182fb78fbd367fcfa97f2ca6a5ff.1552764410.git.baruch@tkos.co.il/
>
> Am I missing something?

That is totally outside of the in-reply-to/references trail of your
ping message, and what I saw in the message you were quoting in your
'ping' was

>>  		$message .=  $_;
>> -		if (/^([a-z-]*-by|Cc): (.*)/i) {
>> +		if (!$in_patch && /^([a-z-]*-by|Cc): (.*)/i) {
>>  			chomp;

which is a lot looser than the suggested "the beginning must be
alpha" pattern.

Re: [PATCH] send-email: don't cc *-by lines with '-' prefix

[Rasmus Villemoes]

On 04/04/2019 11.27, Baruch Siach wrote:
> Hi Junio,
> 
> On Thu, Apr 04 2019, Junio C. Hamano wrote:
>> Baruch Siach <baruch@tkos.co.il> writes:
>>
>>>> Independently, I think it makes sense to do something like
>>>>
>>>> 	/^([a-z][a-z-]*-by|Cc): (.*)/i
>>>>
>>>> to tighten the match to exclude a non-trailer; that would have been
>>>> sufficient for the original case that triggered this thread.
>>>
>>> Is there anything I need to do more to get this fix applied for the next
>>> git release?
>>
>> Get "this" fix applied?  I think we should tighten the regexp to
>> exclude a non-trailer, which would have been sufficient for the
>> original case without anything else in "this" fix.  So in short, I
>> do not think "this" fix won't be applied without further tweaking
>> ;-)
> 
> This is exactly what "this" patch (referenced in the title of "this"
> thread) is doing:
> 
>   https://public-inbox.org/git/eec56beab016182fb78fbd367fcfa97f2ca6a5ff.1552764410.git.baruch@tkos.co.il/
> 
> Am I missing something?

My ack for Baruch's original patch, which AFAICT is identical with
Junio's suggestion, still stands. FWIW, I'm against Joe's suggestion of
stopping at a line matching /^---/, since it's not unlikely somebody
does something like

---- dmesg output ----
bla bla
----

in the commit message.

Since all lines (except for some of the diff header lines) in the patch
part begin with space, - or +, insisting on a the line starting with a
letter should be sufficient for excluding any random Foo-by lines that
may appear in the patch part.

Rasmus

Re: [PATCH] send-email: don't cc *-by lines with '-' prefix

[Junio C Hamano]

Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:

> My ack for Baruch's original patch, which AFAICT is identical with
> Junio's suggestion, still stands. FWIW, I'm against Joe's suggestion of
> stopping at a line matching /^---/, since it's not unlikely somebody
> does something like
>
> ---- dmesg output ----
> bla bla
> ----
>
> in the commit message.

Hmph.  That does make sort-of sense ;-)

Re: [PATCH] send-email: don't cc *-by lines with '-' prefix

[Junio C Hamano]

Baruch Siach <baruch@tkos.co.il> writes:

> Since commit ef0cc1df90f6b ("send-email: also pick up cc addresses from
> -by trailers") in git version 2.20, git send-email adds to cc list
> addresses from all *-by lines. As a side effect a line with
> '-Signed-off-by' is now also added to cc. This makes send-email pick
> lines from patches that remove patch files from the git repo. This is
> common in the Buildroot project that often removes (and adds) patch
> files that have 'Signed-off-by' in their patch description part.
>
> Consider only *-by lines that start with [a-z] (case insensitive) to
> avoid unrelated addresses in cc.
>
> Cc: Joe Perches <joe@perches.com>
> Cc: Rasmus Villemoes <rv@rasmusvillemoes.dk>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  git-send-email.perl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 8eb63b5a2f8d..5656ba83d9b1 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1693,7 +1693,7 @@ sub process_file {
>  	# Now parse the message body
>  	while(<$fh>) {
>  		$message .=  $_;
> -		if (/^([a-z-]*-by|Cc): (.*)/i) {
> +		if (/^([a-z][a-z-]*-by|Cc): (.*)/i) {
>  			chomp;
>  			my ($what, $c) = ($1, $2);
>  			# strip garbage for the address we'll use:

OK, this fell through the cracks (and it did not help that a recent
ping message did not come as a response to it, but as a response to
another thread with an alternative implementation).  Will apply and
cook in 'next' to see what happens.

FYI, being in 'next' does not mean it will be in the next release.
Being in 'master' usually does, though.

Re: [PATCH] send-email: don't cc *-by lines with '-' prefix

[Jeff King]

On Thu, Apr 04, 2019 at 11:42:23AM +0200, Rasmus Villemoes wrote:

> My ack for Baruch's original patch, which AFAICT is identical with
> Junio's suggestion, still stands. FWIW, I'm against Joe's suggestion of
> stopping at a line matching /^---/, since it's not unlikely somebody
> does something like
> 
> ---- dmesg output ----
> bla bla
> ----
> 
> in the commit message.

Keep in mind that on the receiving end, we are going to stop reading the
commit message at a triple-dash, too, which is done as (from
mailinfo.c's patchbreak()):

  /^---( [^\s]|\s*$)/

So it might make sense to use the same rule here. That said:

> Since all lines (except for some of the diff header lines) in the patch
> part begin with space, - or +, insisting on a the line starting with a
> letter should be sufficient for excluding any random Foo-by lines that
> may appear in the patch part.

Yeah, I think this mostly makes it a non-issue, unless we care about
efficiency (and I doubt it is even measurable).

Technically you could have other cruft after the diff, too. But I think
putting "signed-off-by: somebody" in your email sig is a case of "if it
hurts, don't do it".

-Peff