* body-CC-comment regression @ 2017-02-16 17:49 Johan Hovold 2017-02-16 17:59 ` Junio C Hamano 2017-02-16 18:16 ` Matthieu Moy 0 siblings, 2 replies; 22+ messages in thread From: Johan Hovold @ 2017-02-16 17:49 UTC (permalink / raw) To: git; +Cc: Jeff King, Matthieu Moy, Kevin Daudt, Junio C Hamano, Larry Finger Hi, I recently noticed that after an upgrade, git-send-email (2.10.2) started aborting when trying to send patches that had a linux-kernel stable-tag in its body. For example, Cc: <stable@vger.kernel.org> # 4.4 was now parsed as "stable@vger.kernel.org#4.4" which resulted in Died at /usr/libexec/git-core/git-send-email line 1332, <FIN> line 1. This tends to happen in the middle of a series after the cover letter had been sent just to make things worse... I finally got around to looking into this today and discovered this thread ("Re: Formatting problem send_mail in version 2.10.0"): https://marc.info/?l=git&m=147633706724793&w=2 The problem with the resulting fixes that are now in 2.11.1 is that git-send-email no longer discards the trailing comment but rather shoves it into the name after adding some random white space: "# 3 . 3 . x : 1b9508f : sched : Rate-limit newidle" <stable@vger.kernel.org>" This example is based on the example from Documentation/process/stable-kernel-rules.rst: Cc: <stable@vger.kernel.org> # 3.3.x: 1b9508f: sched: Rate-limit newidle and this format for stable-tags has been documented at least since 2009 and 8e9b9362266d ("Doc/stable rules: add new cherry-pick logic"), and has been supported by git since 2012 and 831a488b76e0 ("git-send-email: remove garbage after email address") I believe. Can we please revert to the old behaviour of simply discarding such comments (from body-CC:s) or at least make it configurable through a configuration option? Thanks, Johan ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: body-CC-comment regression 2017-02-16 17:49 body-CC-comment regression Johan Hovold @ 2017-02-16 17:59 ` Junio C Hamano 2017-02-16 18:14 ` Johan Hovold 2017-02-16 18:16 ` Matthieu Moy 1 sibling, 1 reply; 22+ messages in thread From: Junio C Hamano @ 2017-02-16 17:59 UTC (permalink / raw) To: Johan Hovold; +Cc: git, Jeff King, Matthieu Moy, Kevin Daudt, Larry Finger Johan Hovold <johan@kernel.org> writes: > I recently noticed that after an upgrade, git-send-email (2.10.2) > started aborting when trying to send patches that had a linux-kernel > stable-tag in its body. For example, > > Cc: <stable@vger.kernel.org> # 4.4 > > was now parsed as > > "stable@vger.kernel.org#4.4" > ... It sounds like a fallout of this: https://public-inbox.org/git/41164484-309b-bfff-ddbb-55153495d41a@lwfinger.net/#t and any change to "fix" you may break the other person. > Can we please revert to the old behaviour of simply discarding such > comments (from body-CC:s) or at least make it configurable through a > configuration option? If I recall the old thread correctly, it was reported that using Mail::Address without forcing git-send-email fall back to its own non-parsing-but-paste-address-looking-things-together code would solve it, so can the "make it configurable" be just "install Mail::Address"? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: body-CC-comment regression 2017-02-16 17:59 ` Junio C Hamano @ 2017-02-16 18:14 ` Johan Hovold 0 siblings, 0 replies; 22+ messages in thread From: Johan Hovold @ 2017-02-16 18:14 UTC (permalink / raw) To: Junio C Hamano Cc: Johan Hovold, git, Jeff King, Matthieu Moy, Kevin Daudt, Larry Finger On Thu, Feb 16, 2017 at 09:59:25AM -0800, Junio C Hamano wrote: > Johan Hovold <johan@kernel.org> writes: > > > I recently noticed that after an upgrade, git-send-email (2.10.2) > > started aborting when trying to send patches that had a linux-kernel > > stable-tag in its body. For example, > > > > Cc: <stable@vger.kernel.org> # 4.4 > > > > was now parsed as > > > > "stable@vger.kernel.org#4.4" > > ... > > It sounds like a fallout of this: > > https://public-inbox.org/git/41164484-309b-bfff-ddbb-55153495d41a@lwfinger.net/#t > > and any change to "fix" you may break the other person. Yes, that's the thread I was referring to as well, and the reported breakage is the same even if the reporter used a non-standard stable-tag format (e.g. a "[4.8+]" suffix). What I'm wondering is whether the alternative fix proposed in that thread, to revert to the old behaviour of discarding trailing comments, should be considered instead of what was implemented. > > Can we please revert to the old behaviour of simply discarding such > > comments (from body-CC:s) or at least make it configurable through a > > configuration option? > > If I recall the old thread correctly, it was reported that using > Mail::Address without forcing git-send-email fall back to its own > non-parsing-but-paste-address-looking-things-together code would > solve it, so can the "make it configurable" be just "install > Mail::Address"? I believe git-send-email's parser was changed to mimic Mail::Address, and installing it does not seem to change the behaviour of including any trailing comments in the name. Thanks, Johan ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: body-CC-comment regression 2017-02-16 17:49 body-CC-comment regression Johan Hovold 2017-02-16 17:59 ` Junio C Hamano @ 2017-02-16 18:16 ` Matthieu Moy 2017-02-17 11:06 ` Johan Hovold 1 sibling, 1 reply; 22+ messages in thread From: Matthieu Moy @ 2017-02-16 18:16 UTC (permalink / raw) To: Johan Hovold; +Cc: git, Jeff King, Kevin Daudt, Junio C Hamano, Larry Finger Johan Hovold <johan@kernel.org> writes: > Hi, > > I recently noticed that after an upgrade, git-send-email (2.10.2) > started aborting when trying to send patches that had a linux-kernel > stable-tag in its body. For example, > > Cc: <stable@vger.kernel.org> # 4.4 > > was now parsed as > > "stable@vger.kernel.org#4.4" > > which resulted in > > Died at /usr/libexec/git-core/git-send-email line 1332, <FIN> line 1. This has changed in e3fdbcc8e1 (parse_mailboxes: accept extra text after <...> address, 2016-10-13), released v2.11.0 as you noticed: > The problem with the resulting fixes that are now in 2.11.1 is that > git-send-email no longer discards the trailing comment but rather > shoves it into the name after adding some random white space: > > "# 3 . 3 . x : 1b9508f : sched : Rate-limit newidle" <stable@vger.kernel.org>" > > This example is based on the example from > Documentation/process/stable-kernel-rules.rst: > > Cc: <stable@vger.kernel.org> # 3.3.x: 1b9508f: sched: Rate-limit newidle > > and this format for stable-tags has been documented at least since 2009 > and 8e9b9362266d ("Doc/stable rules: add new cherry-pick logic"), and > has been supported by git since 2012 and 831a488b76e0 ("git-send-email: > remove garbage after email address") I believe. > > Can we please revert to the old behaviour of simply discarding such > comments (from body-CC:s) or at least make it configurable through a > configuration option? The problem is that we now accept list of emails instead of just one email, so it's hard to define what "comments after the email", for example Cc: <foo@example.com> # , <boz@example.com> Is not accepted as two emails. So, just stripping whatever comes after # before parsing the list of emails would change the behavior once more, and possibly break other user's flow. Dropping the garbage after the email while parsing is possible, but only when we use our in-house parser (and we currently use Perl's Mail::Address when available). So, a proper fix is far from obvious, and unfortunately I won't have time to work on that, at least not before a while. OTOH, the current behavior isn't that bad. It accepts the input, and extracts a valid email out of it. Just the display name is admitedly suboptimal ... -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: body-CC-comment regression 2017-02-16 18:16 ` Matthieu Moy @ 2017-02-17 11:06 ` Johan Hovold 2017-02-17 13:16 ` Matthieu Moy 0 siblings, 1 reply; 22+ messages in thread From: Johan Hovold @ 2017-02-17 11:06 UTC (permalink / raw) To: Matthieu Moy Cc: Johan Hovold, git, Jeff King, Kevin Daudt, Junio C Hamano, Larry Finger On Thu, Feb 16, 2017 at 07:16:57PM +0100, Matthieu Moy wrote: > Johan Hovold <johan@kernel.org> writes: > > > Hi, > > > > I recently noticed that after an upgrade, git-send-email (2.10.2) > > started aborting when trying to send patches that had a linux-kernel > > stable-tag in its body. For example, > > > > Cc: <stable@vger.kernel.org> # 4.4 > > > > was now parsed as > > > > "stable@vger.kernel.org#4.4" > > > > which resulted in > > > > Died at /usr/libexec/git-core/git-send-email line 1332, <FIN> line 1. > > This has changed in e3fdbcc8e1 (parse_mailboxes: accept extra text after > <...> address, 2016-10-13), released v2.11.0 as you noticed: > > > The problem with the resulting fixes that are now in 2.11.1 is that > > git-send-email no longer discards the trailing comment but rather > > shoves it into the name after adding some random white space: > > > > "# 3 . 3 . x : 1b9508f : sched : Rate-limit newidle" <stable@vger.kernel.org>" > > > > This example is based on the example from > > Documentation/process/stable-kernel-rules.rst: > > > > Cc: <stable@vger.kernel.org> # 3.3.x: 1b9508f: sched: Rate-limit newidle > > > > and this format for stable-tags has been documented at least since 2009 > > and 8e9b9362266d ("Doc/stable rules: add new cherry-pick logic"), and > > has been supported by git since 2012 and 831a488b76e0 ("git-send-email: > > remove garbage after email address") I believe. > > > > Can we please revert to the old behaviour of simply discarding such > > comments (from body-CC:s) or at least make it configurable through a > > configuration option? > > The problem is that we now accept list of emails instead of just one > email, so it's hard to define what "comments after the email", for > example > > Cc: <foo@example.com> # , <boz@example.com> > > Is not accepted as two emails. > > So, just stripping whatever comes after # before parsing the list of > emails would change the behavior once more, and possibly break other > user's flow. Dropping the garbage after the email while parsing is > possible, but only when we use our in-house parser (and we currently use > Perl's Mail::Address when available). > > So, a proper fix is far from obvious, and unfortunately I won't have > time to work on that, at least not before a while. There is another option, namely to only accept a single address for tags in the body. I understand that being able to copy a CC-header to either the header section or to the command line could be useful, but I don't really see the point in allowing this in the tags in the body (a SoB always has one address, and so should a CC-tag). And since this is a regression for something that has been working for years that was introduced by a new feature, I also think it's reasonable to (partially) revert the feature. > OTOH, the current behavior isn't that bad. It accepts the input, and > extracts a valid email out of it. Just the display name is admitedly > suboptimal ... Yeah, but the display name can end up with so much noise that auto-cc is effectively broken for people submitting kernel patches (with stable tags) as the only way to avoid it is to suppress all bodycc. So what I'm proposing is to revert to the earlier behaviour of only allowing one address per body tag by simply discarding anything after the address. Something like the below seems to do the trick. Thanks, Johan From f551b4ca9926624dc7af6c286d7cf0f97af39541 Mon Sep 17 00:00:00 2001 From: Johan Hovold <johan@kernel.org> Date: Fri, 17 Feb 2017 11:55:47 +0100 Subject: [PATCH] send-email: only allow one address per body tag Adding comments after a tag in the body is a common practise (e.g. in the Linux kernel) and git-send-email has been supporting this for years by removing any trailing cruft after the address. After some recent changes, any trailing comment is now instead appended to the recipient name (with some random white space inserted) resulting in undesirable noise in the headers, for example: CC: "# 3 . 3 . x : 1b9508f : sched : Rate-limit newidle" <stable@vger.kernel.org> Revert to the earlier behaviour of discarding anything after the (first) address in a tag while parsing the body. Note that multiple addresses after are still allowed after a command-line switch (and in a CC-header). Fixes: b1c8a11c8024 ("send-email: allow multiple emails using --cc, --to and --bcc") Fixes: e3fdbcc8e164 ("parse_mailboxes: accept extra text after <...> address") Signed-off-by: Johan Hovold <johan@kernel.org> --- 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 068d60b3e698..eea0a517f71b 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -1563,7 +1563,7 @@ foreach my $t (@files) { # Now parse the message body while(<$fh>) { $message .= $_; - if (/^(Signed-off-by|Cc): (.*)$/i) { + if (/^(Signed-off-by|Cc): ([^>]*>?)/i) { chomp; my ($what, $c) = ($1, $2); chomp $c; -- 2.11.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: body-CC-comment regression 2017-02-17 11:06 ` Johan Hovold @ 2017-02-17 13:16 ` Matthieu Moy 2017-02-17 16:42 ` Johan Hovold 2017-02-17 17:38 ` body-CC-comment regression Linus Torvalds 0 siblings, 2 replies; 22+ messages in thread From: Matthieu Moy @ 2017-02-17 13:16 UTC (permalink / raw) To: Johan Hovold; +Cc: git, Jeff King, Kevin Daudt, Junio C Hamano, Larry Finger Johan Hovold <johan@kernel.org> writes: > There is another option, namely to only accept a single address for tags > in the body. I understand that being able to copy a CC-header to either > the header section or to the command line could be useful, but I don't > really see the point in allowing this in the tags in the body (a SoB > always has one address, and so should a CC-tag). I mostly agree for the SoB, but why should a Cc tag have only one email? The "multiple emails per Cc: field" has been there for a while already (b1c8a11c8024 released in 2.6.0, sept 2015), some users may have got used to it. What you are proposing breaks their flow. > And since this is a regression for something that has been working for > years that was introduced by a new feature, I also think it's reasonable > to (partially) revert the feature. I'd find it rather ironic to fix your case by breaking a feature that has been working for more than a year :-(. What would you answer to a contributor comming one year from now and proposing to revert your reversion because it breaks his flow? All that said, I think another fix would be both satisfactory for everyone and rather simple: 1) Stop calling Mail::Address even if available. It used to make sense to do that when our in-house parser was really poor, but we now have something essentially as good as Mail::Address. We test our parser against Mail::Address and we do have a few known differences (see t9000), but they are really corner-cases and shouldn't matter. A good consequence of this is that we stop depending on the way Perl is installed to parse emails. Regardless of the current issue, I think it is a good thing. 2) Modify our in-house parser to discard garbage after the >. The patch should look like (untested): --- a/perl/Git.pm +++ b/perl/Git.pm @@ -903,11 +903,11 @@ sub parse_mailboxes { my (@addr_list, @phrase, @address, @comment, @buffer) = (); foreach my $token (@tokens) { if ($token =~ /^[,;]$/) { - # if buffer still contains undeterminated strings - # append it at the end of @address or @phrase - if ($end_of_addr_seen) { - push @phrase, @buffer; - } else { + # if buffer still contains undeterminated + # strings append it at the end of @address, + # unless we already saw the closing >, in + # which case we discard it. + if (!$end_of_addr_seen) { push @address, @buffer; } What do you think? -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: body-CC-comment regression 2017-02-17 13:16 ` Matthieu Moy @ 2017-02-17 16:42 ` Johan Hovold 2017-02-17 16:58 ` Matthieu Moy 2017-02-17 17:38 ` body-CC-comment regression Linus Torvalds 1 sibling, 1 reply; 22+ messages in thread From: Johan Hovold @ 2017-02-17 16:42 UTC (permalink / raw) To: Matthieu Moy Cc: Johan Hovold, git, Jeff King, Kevin Daudt, Junio C Hamano, Larry Finger On Fri, Feb 17, 2017 at 02:16:42PM +0100, Matthieu Moy wrote: > Johan Hovold <johan@kernel.org> writes: > > > There is another option, namely to only accept a single address for tags > > in the body. I understand that being able to copy a CC-header to either > > the header section or to the command line could be useful, but I don't > > really see the point in allowing this in the tags in the body (a SoB > > always has one address, and so should a CC-tag). > > I mostly agree for the SoB, but why should a Cc tag have only one email? For symmetry (with SoB) and readability reasons (one tag per line). These are body tags, not mail headers, after all. > The "multiple emails per Cc: field" has been there for a while already > (b1c8a11c8024 released in 2.6.0, sept 2015), some users may have got > used to it. What you are proposing breaks their flow. Note that that commit never mentions multiple addresses in either headers or body-tags -- it's all about being able to specify multiple entries on the command line. There does not seem to be single commit in the kernel where multiple address are specified in a CC tag since after git-send-email started allowing it, but there are ten commits before (to my surprise), and that should be contrasted with at least 4178 commits with trailing comments including a # sign. > > And since this is a regression for something that has been working for > > years that was introduced by a new feature, I also think it's reasonable > > to (partially) revert the feature. > > I'd find it rather ironic to fix your case by breaking a feature that > has been working for more than a year :-(. What would you answer to a > contributor comming one year from now and proposing to revert your > reversion because it breaks his flow? Such conflicts are not uncommon when dealing with regressions introduced by new features, and need to be dealt with on a case-by-case basis. But the fact that trailing comments have been properly supported for more than four years should carry some weight. > All that said, I think another fix would be both satisfactory for > everyone and rather simple: > > 1) Stop calling Mail::Address even if available. It used to make sense > to do that when our in-house parser was really poor, but we now have > something essentially as good as Mail::Address. We test our parser > against Mail::Address and we do have a few known differences (see > t9000), but they are really corner-cases and shouldn't matter. > > A good consequence of this is that we stop depending on the way Perl > is installed to parse emails. Regardless of the current issue, I > think it is a good thing. Right, that sounds like the right thing to do regardless. > 2) Modify our in-house parser to discard garbage after the >. The patch > should look like (untested): > > --- a/perl/Git.pm > +++ b/perl/Git.pm > @@ -903,11 +903,11 @@ sub parse_mailboxes { > my (@addr_list, @phrase, @address, @comment, @buffer) = (); > foreach my $token (@tokens) { > if ($token =~ /^[,;]$/) { > - # if buffer still contains undeterminated strings > - # append it at the end of @address or @phrase > - if ($end_of_addr_seen) { > - push @phrase, @buffer; > - } else { > + # if buffer still contains undeterminated > + # strings append it at the end of @address, > + # unless we already saw the closing >, in > + # which case we discard it. > + if (!$end_of_addr_seen) { > push @address, @buffer; > } > > What do you think? Sounds perfectly fine to me, and seems to work too after quick test. Note however that there's another minor issue with using multiple addresses in a Cc-tag in that it breaks --suppress-cc=self, but I guess that can be fixed separately. Thanks, Johan ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: body-CC-comment regression 2017-02-17 16:42 ` Johan Hovold @ 2017-02-17 16:58 ` Matthieu Moy 2017-02-17 17:30 ` Johan Hovold 2017-02-17 18:18 ` Junio C Hamano 0 siblings, 2 replies; 22+ messages in thread From: Matthieu Moy @ 2017-02-17 16:58 UTC (permalink / raw) To: Johan Hovold; +Cc: git, Jeff King, Kevin Daudt, Junio C Hamano, Larry Finger > On Fri, Feb 17, 2017 at 02:16:42PM +0100, Matthieu Moy wrote: >> Johan Hovold <johan@kernel.org> writes: > >> The "multiple emails per Cc: field" has been there for a while already >> (b1c8a11c8024 released in 2.6.0, sept 2015), some users may have got >> used to it. What you are proposing breaks their flow. > > Note that that commit never mentions multiple addresses in either > headers or body-tags -- it's all about being able to specify multiple > entries on the command line. Indeed. I'm not the author of the patch, but I was supervising the students who wrote it and "multiple addresses in Cc:" was not the goal, but a (IMHO positive) side effect we discovered after the fact. If I had a time machine, I'd probably go back then and forbid multiple addresses there, but ... > There does not seem to be single commit in the kernel where multiple > address are specified in a CC tag since after git-send-email started > allowing it, but there are ten commits before (to my surprise), and that > should be contrasted with at least 4178 commits with trailing comments > including a # sign. Hey, there's a life outside the kernel ;-). >> 1) Stop calling Mail::Address even if available.[...] > > Right, that sounds like the right thing to do regardless. > >> 2) Modify our in-house parser to discard garbage after the >. [...] > > Sounds perfectly fine to me, and seems to work too after quick test. OK, sounds like the way to go. Do you want to work on a patch? If not, I should be able to do that myself. The code changes are straightforward, but we probably want a proper test for that. > addresses in a Cc-tag in that it breaks --suppress-cc=self, but I guess > that can be fixed separately. OK. If it's unrelated enough, please start a separate thread to explain the problem (and/or write a patch ;-) ). Thanks, -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: body-CC-comment regression 2017-02-17 16:58 ` Matthieu Moy @ 2017-02-17 17:30 ` Johan Hovold 2017-02-17 18:18 ` Junio C Hamano 1 sibling, 0 replies; 22+ messages in thread From: Johan Hovold @ 2017-02-17 17:30 UTC (permalink / raw) To: Matthieu Moy Cc: Johan Hovold, git, Jeff King, Kevin Daudt, Junio C Hamano, Larry Finger On Fri, Feb 17, 2017 at 05:58:11PM +0100, Matthieu Moy wrote: > > On Fri, Feb 17, 2017 at 02:16:42PM +0100, Matthieu Moy wrote: > >> Johan Hovold <johan@kernel.org> writes: > > > >> The "multiple emails per Cc: field" has been there for a while already > >> (b1c8a11c8024 released in 2.6.0, sept 2015), some users may have got > >> used to it. What you are proposing breaks their flow. > > > > Note that that commit never mentions multiple addresses in either > > headers or body-tags -- it's all about being able to specify multiple > > entries on the command line. > > Indeed. I'm not the author of the patch, but I was supervising the > students who wrote it and "multiple addresses in Cc:" was not the goal, > but a (IMHO positive) side effect we discovered after the fact. Yeah, and the broken --suppress-cc=self I mention below is indicative of that too. > If I had a time machine, I'd probably go back then and forbid multiple > addresses there, but ... > > > There does not seem to be single commit in the kernel where multiple > > address are specified in a CC tag since after git-send-email started > > allowing it, but there are ten commits before (to my surprise), and that > > should be contrasted with at least 4178 commits with trailing comments > > including a # sign. > > Hey, there's a life outside the kernel ;-). Sure, but it's the origin of git as well as the tags we're discussing (I believe). My point of bringing it up was that the multiple addresses in a CC-tag was indeed an unintended (and undocumented) side-effect and I doubt many people have started using it given that it's sort of counter-intuitive (again, compare with SoB). If either the trailing comments or multiple addresses in a CC-tag has to go, I think dropping the latter is clearly the best choice. > >> 1) Stop calling Mail::Address even if available.[...] > > > > Right, that sounds like the right thing to do regardless. > > > >> 2) Modify our in-house parser to discard garbage after the >. [...] > > > > Sounds perfectly fine to me, and seems to work too after quick test. > > OK, sounds like the way to go. > > Do you want to work on a patch? If not, I should be able to do that > myself. The code changes are straightforward, but we probably want a > proper test for that. Feel free to implement it this way if that's what people prefer. As long as trailing comments are supported and discarded, I don't really have a preference. > > addresses in a Cc-tag in that it breaks --suppress-cc=self, but I guess > > that can be fixed separately. > > OK. If it's unrelated enough, please start a separate thread to explain > the problem (and/or write a patch ;-) ). Well, it's related to the "offending" patch that added support for multiple addresses in tags. By disallowing that, as my fix does, the problem goes away. # Now parse the message body while(<$fh>) { $message .= $_; if (/^(Signed-off-by|Cc): (.*)$/i) { chomp; my ($what, $c) = ($1, $2); chomp $c; my $sc = sanitize_address($c); if ($sc eq $sender) { next if ($suppress_cc{'self'}); The problem here is that $sc will never match $sender when there are more than one address in a tag. For example: From: Johan Hovold <johan@kernel.org> ... Cc: alpha <test1@a.com>, Johan Hovold <johan@kernel.org> results in sc = alpha <test1@a.com>, Johan Hovold <johan@kernel.org> sender = Johan Hovold <johan@kernel.org> so that --suppress-cc=self is not honoured. Thanks, Johan ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: body-CC-comment regression 2017-02-17 16:58 ` Matthieu Moy 2017-02-17 17:30 ` Johan Hovold @ 2017-02-17 18:18 ` Junio C Hamano 2017-02-17 18:23 ` Johan Hovold 1 sibling, 1 reply; 22+ messages in thread From: Junio C Hamano @ 2017-02-17 18:18 UTC (permalink / raw) To: Matthieu Moy; +Cc: Johan Hovold, git, Jeff King, Kevin Daudt, Larry Finger Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes: >> On Fri, Feb 17, 2017 at 02:16:42PM +0100, Matthieu Moy wrote: > ... > If I had a time machine, I'd probably go back then and forbid multiple > addresses there, but ... > >> There does not seem to be single commit in the kernel where multiple >> address are specified in a CC tag since after git-send-email started >> allowing it, but there are ten commits before (to my surprise), and that >> should be contrasted with at least 4178 commits with trailing comments >> including a # sign. > > Hey, there's a life outside the kernel ;-). > ... >>> 1) Stop calling Mail::Address even if available.[...] >> >> Right, that sounds like the right thing to do regardless. >> >>> 2) Modify our in-house parser to discard garbage after the >. [...] >> >> Sounds perfectly fine to me, and seems to work too after quick test. > > OK, sounds like the way to go. > > Do you want to work on a patch? If not, I should be able to do that > myself. The code changes are straightforward, but we probably want a > proper test for that. The true headers and the things at the bottom seem to be handled in a separate loop in send-email, so treating Cc: found in the former and in the latter differently should be doable. I think it is OK to explicitly treat the latter as "these are not e-mail addresses, but just a single e-mail address possibly with non-address cruft", without losing the ability to have more than one addresses on a single CC: e-mail header. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: body-CC-comment regression 2017-02-17 18:18 ` Junio C Hamano @ 2017-02-17 18:23 ` Johan Hovold 2017-02-17 18:28 ` Junio C Hamano 0 siblings, 1 reply; 22+ messages in thread From: Johan Hovold @ 2017-02-17 18:23 UTC (permalink / raw) To: Junio C Hamano Cc: Matthieu Moy, Johan Hovold, git, Jeff King, Kevin Daudt, Larry Finger On Fri, Feb 17, 2017 at 10:18:46AM -0800, Junio C Hamano wrote: > Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes: > > >> On Fri, Feb 17, 2017 at 02:16:42PM +0100, Matthieu Moy wrote: > > ... > > If I had a time machine, I'd probably go back then and forbid multiple > > addresses there, but ... > > > >> There does not seem to be single commit in the kernel where multiple > >> address are specified in a CC tag since after git-send-email started > >> allowing it, but there are ten commits before (to my surprise), and that > >> should be contrasted with at least 4178 commits with trailing comments > >> including a # sign. > > > > Hey, there's a life outside the kernel ;-). > > ... > >>> 1) Stop calling Mail::Address even if available.[...] > >> > >> Right, that sounds like the right thing to do regardless. > >> > >>> 2) Modify our in-house parser to discard garbage after the >. [...] > >> > >> Sounds perfectly fine to me, and seems to work too after quick test. > > > > OK, sounds like the way to go. > > > > Do you want to work on a patch? If not, I should be able to do that > > myself. The code changes are straightforward, but we probably want a > > proper test for that. > > The true headers and the things at the bottom seem to be handled in > a separate loop in send-email, so treating Cc: found in the former > and in the latter differently should be doable. I think it is OK to > explicitly treat the latter as "these are not e-mail addresses, but > just a single e-mail address possibly with non-address cruft", > without losing the ability to have more than one addresses on a > single CC: e-mail header. That's precisely what the patch I posted earlier in the thread did. Thanks, Johan ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: body-CC-comment regression 2017-02-17 18:23 ` Johan Hovold @ 2017-02-17 18:28 ` Junio C Hamano 2017-02-17 18:44 ` Matthieu Moy 0 siblings, 1 reply; 22+ messages in thread From: Junio C Hamano @ 2017-02-17 18:28 UTC (permalink / raw) To: Johan Hovold; +Cc: Matthieu Moy, git, Jeff King, Kevin Daudt, Larry Finger Johan Hovold <johan@kernel.org> writes: > That's precisely what the patch I posted earlier in the thread did. That's good. I didn't see any patch yet but the message you are responding to is a response to Matthieu's message asking if you are planning to work on it, so I'd assume you are and and look forward to seeing a patch (or a series?) we can queue. Thanks. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: body-CC-comment regression 2017-02-17 18:28 ` Junio C Hamano @ 2017-02-17 18:44 ` Matthieu Moy 2017-02-17 20:05 ` Junio C Hamano 0 siblings, 1 reply; 22+ messages in thread From: Matthieu Moy @ 2017-02-17 18:44 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johan Hovold, git, Jeff King, Kevin Daudt, Larry Finger Junio C Hamano <gitster@pobox.com> writes: > Johan Hovold <johan@kernel.org> writes: > >> That's precisely what the patch I posted earlier in the thread did. > > That's good. I didn't see any patch yet It's here: http://public-inbox.org/git/20170217110642.GD2625@localhost/ but as I explained, this removes a feature suported since several major releases and we have no idea how many users may use the "mupliple emails in one field". The approach I proposed does not suffer from this. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: body-CC-comment regression 2017-02-17 18:44 ` Matthieu Moy @ 2017-02-17 20:05 ` Junio C Hamano 2017-02-17 20:20 ` Matthieu Moy 0 siblings, 1 reply; 22+ messages in thread From: Junio C Hamano @ 2017-02-17 20:05 UTC (permalink / raw) To: Matthieu Moy; +Cc: Johan Hovold, git, Jeff King, Kevin Daudt, Larry Finger Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Johan Hovold <johan@kernel.org> writes: >> >>> That's precisely what the patch I posted earlier in the thread did. >> >> That's good. I didn't see any patch yet > > It's here: > > http://public-inbox.org/git/20170217110642.GD2625@localhost/ > > but as I explained, this removes a feature suported since several major > releases and we have no idea how many users may use the "mupliple emails > in one field". The approach I proposed does not suffer from this. OK, so you are aiming higher to please both parties, i.e. those who place a single address but cruft at the end would get the cruft stripped when we grab a usable address from the field, and those who write two or more addresses would get all of these addresses? That approach may still constrain what those in the former camp can write in the "cruft" part, like they cannot write comma or semicolon as part of the "cruft", no? If that does not pose a practical problem, then I can imagine it would work well for people in both camps. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: body-CC-comment regression 2017-02-17 20:05 ` Junio C Hamano @ 2017-02-17 20:20 ` Matthieu Moy 2017-02-17 22:22 ` Junio C Hamano 0 siblings, 1 reply; 22+ messages in thread From: Matthieu Moy @ 2017-02-17 20:20 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johan Hovold, git, Jeff King, Kevin Daudt, Larry Finger Junio C Hamano <gitster@pobox.com> writes: > That approach may still constrain what those in the former camp can > write in the "cruft" part, like they cannot write comma or semicolon > as part of the "cruft", no? Right. Indeed, this may be a problem since the use of "#" for stable seem to include commit message, and they may contain commas. So, maybe Johan's patch is better indeed. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: body-CC-comment regression 2017-02-17 20:20 ` Matthieu Moy @ 2017-02-17 22:22 ` Junio C Hamano 2017-02-17 23:04 ` Junio C Hamano 0 siblings, 1 reply; 22+ messages in thread From: Junio C Hamano @ 2017-02-17 22:22 UTC (permalink / raw) To: Matthieu Moy; +Cc: Johan Hovold, git, Jeff King, Kevin Daudt, Larry Finger Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> That approach may still constrain what those in the former camp can >> write in the "cruft" part, like they cannot write comma or semicolon >> as part of the "cruft", no? > > Right. Indeed, this may be a problem since the use of "#" for stable > seem to include commit message, and they may contain commas. > > So, maybe Johan's patch is better indeed. OK, so I'll queue that one with your Ack for now so that we won't forget. I guess we still want a few tests? Thanks. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: body-CC-comment regression 2017-02-17 22:22 ` Junio C Hamano @ 2017-02-17 23:04 ` Junio C Hamano 2017-02-20 11:44 ` [PATCH v2] send-email: only allow one address per body tag Johan Hovold 0 siblings, 1 reply; 22+ messages in thread From: Junio C Hamano @ 2017-02-17 23:04 UTC (permalink / raw) To: Johan Hovold; +Cc: Matthieu Moy, git, Jeff King, Kevin Daudt, Larry Finger Junio C Hamano <gitster@pobox.com> writes: > Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes: > >> Junio C Hamano <gitster@pobox.com> writes: >> >>> That approach may still constrain what those in the former camp can >>> write in the "cruft" part, like they cannot write comma or semicolon >>> as part of the "cruft", no? >> >> Right. Indeed, this may be a problem since the use of "#" for stable >> seem to include commit message, and they may contain commas. >> >> So, maybe Johan's patch is better indeed. > > OK, so I'll queue that one with your Ack for now so that we won't > forget. I guess we still want a few tests? It seems that there is an expectation in one of the tests that needs to be adjusted. ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2] send-email: only allow one address per body tag 2017-02-17 23:04 ` Junio C Hamano @ 2017-02-20 11:44 ` Johan Hovold 2017-02-20 12:10 ` Matthieu Moy 0 siblings, 1 reply; 22+ messages in thread From: Johan Hovold @ 2017-02-20 11:44 UTC (permalink / raw) To: Junio C Hamano Cc: Matthieu Moy, Jeff King, Kevin Daudt, Larry Finger, git, Johan Hovold Adding comments after a tag in the body is a common practise (e.g. in the Linux kernel) and git-send-email has been supporting this for years by removing any trailing cruft after the address. After some recent changes, any trailing comment is now instead appended to the recipient name (with some random white space inserted) resulting in undesirable noise in the headers, for example: CC: "# 3 . 3 . x : 1b9508f : sched : Rate-limit newidle" <stable@vger.kernel.org> Revert to the earlier behaviour of discarding anything after the (first) address in a tag while parsing the body. Note that multiple addresses after are still allowed after a command line switch (and in a CC header field). Also note that --suppress-cc=self was never honoured when using multiple addresses in a tag. Fixes: b1c8a11c8024 ("send-email: allow multiple emails using --cc, --to and --bcc") Fixes: e3fdbcc8e164 ("parse_mailboxes: accept extra text after <...> address") Signed-off-by: Johan Hovold <johan@kernel.org> --- v2: - update the cc-trailer test - amend commit message and mention the broken --suppress-cc=self git-send-email.perl | 2 +- t/t9001-send-email.sh | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/git-send-email.perl b/git-send-email.perl index 068d60b3e698..eea0a517f71b 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -1563,7 +1563,7 @@ foreach my $t (@files) { # Now parse the message body while(<$fh>) { $message .= $_; - if (/^(Signed-off-by|Cc): (.*)$/i) { + if (/^(Signed-off-by|Cc): ([^>]*>?)/i) { chomp; my ($what, $c) = ($1, $2); chomp $c; diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 0f398dd1603d..60a80f60b268 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -148,7 +148,6 @@ cat >expected-cc <<\EOF !two@example.com! !three@example.com! !four@example.com! -!five@example.com! EOF " @@ -159,9 +158,9 @@ test_expect_success $PREREQ 'cc trailer with various syntax' ' Test Cc: trailers. Cc: one@example.com - Cc: <two@example.com> # this is part of the name - Cc: <three@example.com>, <four@example.com> # not.five@example.com - Cc: "Some # Body" <five@example.com> [part.of.name.too] + Cc: <two@example.com> # trailing comments are ignored + Cc: <three@example.com>, <not.four@example.com> one address per line + Cc: "Some # Body" <four@example.com> [ <also.a.comment> ] EOF clean_fake_sendmail && git send-email -1 --to=recipient@example.com \ -- 2.11.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2] send-email: only allow one address per body tag 2017-02-20 11:44 ` [PATCH v2] send-email: only allow one address per body tag Johan Hovold @ 2017-02-20 12:10 ` Matthieu Moy 2017-02-23 18:53 ` Junio C Hamano 0 siblings, 1 reply; 22+ messages in thread From: Matthieu Moy @ 2017-02-20 12:10 UTC (permalink / raw) To: Johan Hovold; +Cc: Junio C Hamano, Jeff King, Kevin Daudt, Larry Finger, git Johan Hovold <johan@kernel.org> writes: > --- a/git-send-email.perl > +++ b/git-send-email.perl > @@ -1563,7 +1563,7 @@ foreach my $t (@files) { > # Now parse the message body > while(<$fh>) { > $message .= $_; > - if (/^(Signed-off-by|Cc): (.*)$/i) { > + if (/^(Signed-off-by|Cc): ([^>]*>?)/i) { I think this is acceptable, but this doesn't work with trailers like Cc: "Some > Body" <Some.Body@example.com> A proper management of this kind of weird address should be doable by reusing the regexp parsing "..." in parse_mailbox: my $re_quote = qr/"(?:[^\"\\]|\\.)*"/; So the final regex would look like if (/^(Signed-off-by|Cc): (([^>]*|"(?:[^\"\\]|\\.)*")>?)/i) { I don't think that should block the patch inclusion, but it may be worth considering. Anyway, thanks for the patch! -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] send-email: only allow one address per body tag 2017-02-20 12:10 ` Matthieu Moy @ 2017-02-23 18:53 ` Junio C Hamano 2017-02-26 20:45 ` Matthieu Moy 0 siblings, 1 reply; 22+ messages in thread From: Junio C Hamano @ 2017-02-23 18:53 UTC (permalink / raw) To: Matthieu Moy; +Cc: Johan Hovold, Jeff King, Kevin Daudt, Larry Finger, git Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes: > Johan Hovold <johan@kernel.org> writes: > >> --- a/git-send-email.perl >> +++ b/git-send-email.perl >> @@ -1563,7 +1563,7 @@ foreach my $t (@files) { >> # Now parse the message body >> while(<$fh>) { >> $message .= $_; >> - if (/^(Signed-off-by|Cc): (.*)$/i) { >> + if (/^(Signed-off-by|Cc): ([^>]*>?)/i) { > > I think this is acceptable, but this doesn't work with trailers like > > Cc: "Some > Body" <Some.Body@example.com> > > A proper management of this kind of weird address should be doable by > reusing the regexp parsing "..." in parse_mailbox: > > my $re_quote = qr/"(?:[^\"\\]|\\.)*"/; > > So the final regex would look like > > if (/^(Signed-off-by|Cc): (([^>]*|"(?:[^\"\\]|\\.)*")>?)/i) { > > I don't think that should block the patch inclusion, but it may be worth > considering. > > Anyway, thanks for the patch! Somehow this fell off the radar. So your reviewed-by: and then we'll cook this in 'next' for a while? Thanks. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] send-email: only allow one address per body tag 2017-02-23 18:53 ` Junio C Hamano @ 2017-02-26 20:45 ` Matthieu Moy 0 siblings, 0 replies; 22+ messages in thread From: Matthieu Moy @ 2017-02-26 20:45 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johan Hovold, Jeff King, Kevin Daudt, Larry Finger, git Junio C Hamano <gitster@pobox.com> writes: > Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes: > >> Johan Hovold <johan@kernel.org> writes: >> >>> --- a/git-send-email.perl >>> +++ b/git-send-email.perl >>> @@ -1563,7 +1563,7 @@ foreach my $t (@files) { >>> # Now parse the message body >>> while(<$fh>) { >>> $message .= $_; >>> - if (/^(Signed-off-by|Cc): (.*)$/i) { >>> + if (/^(Signed-off-by|Cc): ([^>]*>?)/i) { >> >> I think this is acceptable, but this doesn't work with trailers like >> >> Cc: "Some > Body" <Some.Body@example.com> >> >> A proper management of this kind of weird address should be doable by >> reusing the regexp parsing "..." in parse_mailbox: >> >> my $re_quote = qr/"(?:[^\"\\]|\\.)*"/; >> >> So the final regex would look like >> >> if (/^(Signed-off-by|Cc): (([^>]*|"(?:[^\"\\]|\\.)*")>?)/i) { >> >> I don't think that should block the patch inclusion, but it may be worth >> considering. >> >> Anyway, thanks for the patch! > > Somehow this fell off the radar. So your reviewed-by: and then > we'll cook this in 'next' for a while? OK. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: body-CC-comment regression 2017-02-17 13:16 ` Matthieu Moy 2017-02-17 16:42 ` Johan Hovold @ 2017-02-17 17:38 ` Linus Torvalds 1 sibling, 0 replies; 22+ messages in thread From: Linus Torvalds @ 2017-02-17 17:38 UTC (permalink / raw) To: Matthieu Moy Cc: Johan Hovold, Git Mailing List, Jeff King, Kevin Daudt, Junio C Hamano, Larry Finger On Fri, Feb 17, 2017 at 5:16 AM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote: > > I mostly agree for the SoB, but why should a Cc tag have only one email? Because changing that clearly broke real and useful behavior. The "multiple email addresses" thing is bogus and wrong. Just don't do it. How would you even parse it sanely? Are the Cc: lines now SMTP-compliant with the whole escaping and all the usual "next line" rules? For example, in email, the rule for "next line" is that if you're in a header block, and it starts with whitespace, then it's a continuation of the last line. That's *not* how Cc: lines work in commit messages. They are all individual lines, and we have lots of tools (mainly just scripts with grepping) that simply depend on it. So this notion that the bottom of the commit message is some email header crap is WRONG. Stop it. It caused bugs. It's wrong. Don't do it. Linus ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2017-02-26 20:45 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-16 17:49 body-CC-comment regression Johan Hovold 2017-02-16 17:59 ` Junio C Hamano 2017-02-16 18:14 ` Johan Hovold 2017-02-16 18:16 ` Matthieu Moy 2017-02-17 11:06 ` Johan Hovold 2017-02-17 13:16 ` Matthieu Moy 2017-02-17 16:42 ` Johan Hovold 2017-02-17 16:58 ` Matthieu Moy 2017-02-17 17:30 ` Johan Hovold 2017-02-17 18:18 ` Junio C Hamano 2017-02-17 18:23 ` Johan Hovold 2017-02-17 18:28 ` Junio C Hamano 2017-02-17 18:44 ` Matthieu Moy 2017-02-17 20:05 ` Junio C Hamano 2017-02-17 20:20 ` Matthieu Moy 2017-02-17 22:22 ` Junio C Hamano 2017-02-17 23:04 ` Junio C Hamano 2017-02-20 11:44 ` [PATCH v2] send-email: only allow one address per body tag Johan Hovold 2017-02-20 12:10 ` Matthieu Moy 2017-02-23 18:53 ` Junio C Hamano 2017-02-26 20:45 ` Matthieu Moy 2017-02-17 17:38 ` body-CC-comment regression Linus Torvalds
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).