threads / patch / 50758

patchsend-email: don't cc *-by lines with '-' prefix

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

## tl;dr

18 messages between Mar 16, 2019 and Apr 4, 2019. Diffs are folded; open one to read it.

replies: 17people: 5as markdown or json

Baruch Siach· Mar 16, 2019, 19:26 UTC · lore

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(-)
Show changes to git-send-email.perl +1 −1
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
Joe Perches· Mar 16, 2019, 19:30 UTC · re: Baruch Siach · lore

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

On Sat, 2019-03-16 at 21:26 +0200, Baruch Siach wrote:
Show 7 quoted lines
> 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?

Show 24 quoted lines
> 
> 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:
Baruch Siach· Mar 16, 2019, 19:49 UTC · re: Joe Perches · lore

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

Hi Joe,
On Sat, Mar 16 2019, Joe Perches wrote:
Show 11 quoted lines
> 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
Show 23 quoted lines
>> 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 -
Joe Perches· Mar 16, 2019, 19:59 UTC · re: Baruch Siach · lore

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

On Sat, 2019-03-16 at 21:49 +0200, Baruch Siach wrote:
Show 17 quoted lines
> 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.
Show 9 quoted lines
> 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.
Baruch Siach· Mar 16, 2019, 20:14 UTC · re: Joe Perches · lore

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

Hi Joe,
On Sat, Mar 16 2019, Joe Perches wrote:
Show 39 quoted lines
> 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 -
Joe Perches· Mar 16, 2019, 20:23 UTC · re: Baruch Siach · lore

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

On Sat, 2019-03-16 at 22:14 +0200, Baruch Siach wrote:
> Hi Joe,
Hello Baruch.
Show 22 quoted lines
> 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 '^---'.

Rasmus Villemoes· Mar 17, 2019, 19:27 UTC · re: Baruch Siach · lore

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

On 16/03/2019 20.26, Baruch Siach wrote:
Show 7 quoted lines
> 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
Joe Perches· Mar 18, 2019, 01:56 UTC · re: Rasmus Villemoes · lore

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

On Sun, 2019-03-17 at 20:27 +0100, Rasmus Villemoes wrote:
Show 22 quoted lines
> 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(-)
Show changes to git-send-email.perl +4 −1
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;
Junio C Hamano· Mar 18, 2019, 06:28 UTC · re: Joe Perches · lore

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

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.

Show 29 quoted lines
> ---
>  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;
Joe Perches· Mar 18, 2019, 07:02 UTC · re: Junio C Hamano · lore

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

On Mon, 2019-03-18 at 15:28 +0900, Junio C Hamano wrote:
Show 8 quoted lines
> 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.
Show 9 quoted lines
> 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.
Baruch Siach· Apr 4, 2019, 07:38 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On Mon, Mar 18 2019, Junio C. Hamano wrote:
Show 18 quoted lines
> 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

Show 29 quoted lines
>> ---
>>  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 -
Junio C Hamano· Apr 4, 2019, 09:20 UTC · re: Baruch Siach · lore

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

Baruch Siach <baruch@tkos.co.il> writes:
Show 9 quoted lines
>> 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 ;-)

Baruch Siach· Apr 4, 2019, 09:27 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On Thu, Apr 04 2019, Junio C. Hamano wrote:
Show 17 quoted lines
> 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 -
Junio C Hamano· Apr 4, 2019, 09:41 UTC · re: Baruch Siach · lore

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

Baruch Siach <baruch@tkos.co.il> writes:
Show 27 quoted lines
> 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.

Rasmus Villemoes· Apr 4, 2019, 09:42 UTC · re: Baruch Siach · lore

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

On 04/04/2019 11.27, Baruch Siach wrote:
Show 27 quoted lines
> 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
Junio C Hamano· Apr 4, 2019, 09:47 UTC · re: Rasmus Villemoes · lore

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

Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:
Show 10 quoted lines
> 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 ;-)
Jeff King· Apr 4, 2019, 12:14 UTC · re: Rasmus Villemoes · lore

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

On Thu, Apr 04, 2019 at 11:42:23AM +0200, Rasmus Villemoes wrote:
Show 10 quoted lines
> 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
Junio C Hamano· Apr 4, 2019, 09:49 UTC · re: Baruch Siach · lore

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

Baruch Siach <baruch@tkos.co.il> writes:
Show 31 quoted lines
> 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.

← back to recent threads