[PATCH] scripts.send-pull-request: Avoid multiple chain headers

[PATCH] scripts.send-pull-request: Avoid multiple chain headers

[Jose Lamego]

When creating a patch set with cover letter using the
send-pull-request script, both the "In-Reply-To" and "References"
headers are appended twice in patch 2 and subsequent.

This change appends only one header pointing to very first patch
in series or to cover letter if available.

[Yocto #10718]

Signed-off-by: Jose Lamego <jose.a.lamego@linux.intel.com>
---
 scripts/send-pull-request | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/send-pull-request b/scripts/send-pull-request
index 575549d..a660c37 100755
--- a/scripts/send-pull-request
+++ b/scripts/send-pull-request
@@ -162,7 +162,7 @@ PATCHES=$(echo $PDIR/*.patch)
 if [ $AUTO_CL -eq 1 ]; then
 	# Send the cover letter to every recipient, both specified as well as
 	# harvested. Then remove it from the patches list.
-	eval "git send-email $GIT_TO $GIT_CC $GIT_EXTRA_CC --confirm=always --no-chain-reply-to --suppress-cc=all $CL"
+	eval "git send-email $GIT_TO $GIT_CC $GIT_EXTRA_CC --confirm=always --no-thread --suppress-cc=all $CL"
 	if [ $? -eq 1 ]; then
 		echo "ERROR: failed to send cover-letter with automatic recipients."
 		exit 1
-- 
1.9.1

[PATCH V2] scripts.send-pull-request: Avoid multiple chain headers

[Jose Lamego]

When creating a patch set with cover letter using the
send-pull-request script, both the "In-Reply-To" and "References"
headers are appended twice in patch 2 and subsequent.

This change appends only one header pointing to very first patch
in series or to cover letter if available.

[YOCTO #10718]

Signed-off-by: Jose Lamego <jose.a.lamego@linux.intel.com>
---
 scripts/send-pull-request | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/send-pull-request b/scripts/send-pull-request
index 575549d..a660c37 100755
--- a/scripts/send-pull-request
+++ b/scripts/send-pull-request
@@ -162,7 +162,7 @@ PATCHES=$(echo $PDIR/*.patch)
 if [ $AUTO_CL -eq 1 ]; then
 	# Send the cover letter to every recipient, both specified as well as
 	# harvested. Then remove it from the patches list.
-	eval "git send-email $GIT_TO $GIT_CC $GIT_EXTRA_CC --confirm=always --no-chain-reply-to --suppress-cc=all $CL"
+	eval "git send-email $GIT_TO $GIT_CC $GIT_EXTRA_CC --confirm=always --no-thread --suppress-cc=all $CL"
 	if [ $? -eq 1 ]; then
 		echo "ERROR: failed to send cover-letter with automatic recipients."
 		exit 1
-- 
1.9.1

Re: [PATCH V2] scripts.send-pull-request: Avoid multiple chain headers

[Patrick Ohly]

On Mon, 2016-11-28 at 10:23 -0600, Jose Lamego wrote:
> When creating a patch set with cover letter using the
> send-pull-request script, both the "In-Reply-To" and "References"
> headers are appended twice in patch 2 and subsequent.

The "why" part is missing in the commit header. "Why" is appending those
twice a problem? Is it a bug in the script (because it violates some
RFC) or is it merely a workaround for a problem in other software (mail
programs or Patchwork)?

I know that this change is related to the issues that Patchwork has with
identifying a patch series, but even with that background knowledge it
is not clear why this fix is the right solution.

> This change appends only one header pointing to very first patch
> in series or to cover letter if available.
>
> [YOCTO #10718]
> 
> Signed-off-by: Jose Lamego <jose.a.lamego@linux.intel.com>
> ---
>  scripts/send-pull-request | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/send-pull-request b/scripts/send-pull-request
> index 575549d..a660c37 100755
> --- a/scripts/send-pull-request
> +++ b/scripts/send-pull-request
> @@ -162,7 +162,7 @@ PATCHES=$(echo $PDIR/*.patch)
>  if [ $AUTO_CL -eq 1 ]; then
>  	# Send the cover letter to every recipient, both specified as well as
>  	# harvested. Then remove it from the patches list.
> -	eval "git send-email $GIT_TO $GIT_CC $GIT_EXTRA_CC --confirm=always --no-chain-reply-to --suppress-cc=all $CL"
> +	eval "git send-email $GIT_TO $GIT_CC $GIT_EXTRA_CC --confirm=always --no-thread --suppress-cc=all $CL"
>  	if [ $? -eq 1 ]; then
>  		echo "ERROR: failed to send cover-letter with automatic recipients."
>  		exit 1


And I don't understand why this proposed change has the described
effect. Does changing the threading parameters change the output of "git
send-email" and thus indirectly the mail headers of the following
patches?

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

Re: [PATCH V2] scripts.send-pull-request: Avoid multiple chain headers

[Jose Lamego]

[-- Attachment #1.1: Type: text/plain, Size: 2512 bytes --]

Agree. Please provide feedback about below comments and I will submit a
v3 patch.

On 11/28/2016 01:47 PM, Patrick Ohly wrote:
> On Mon, 2016-11-28 at 10:23 -0600, Jose Lamego wrote:
More than 1 "In-Reply-To" and "References" message headers are in
violation of rfc2822 [1] and may cause that some email-related
applications do not point to the appropriate root message in a
conversation/series.

>> When creating a patch set with cover letter using the
>> send-pull-request script, both the "In-Reply-To" and "References"
>> headers are appended twice in patch 2 and subsequent.
> 
> The "why" part is missing in the commit header. "Why" is appending those
> twice a problem? Is it a bug in the script (because it violates some
> RFC) or is it merely a workaround for a problem in other software (mail
> programs or Patchwork)?
> 
> I know that this change is related to the issues that Patchwork has with
> identifying a patch series, but even with that background knowledge it
> is not clear why this fix is the right solution.
> 
This change appends only one header pointing to very first patch
in series (patch #1) or to cover letter if available, which results in an
appropriate message-chain.

[1] https://tools.ietf.org/html/rfc2822#section-3.6
>>
>> [YOCTO #10718]
>>
>> Signed-off-by: Jose Lamego <jose.a.lamego@linux.intel.com>
>> ---
>>  scripts/send-pull-request | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/scripts/send-pull-request b/scripts/send-pull-request
>> index 575549d..a660c37 100755
>> --- a/scripts/send-pull-request
>> +++ b/scripts/send-pull-request
>> @@ -162,7 +162,7 @@ PATCHES=$(echo $PDIR/*.patch)
>>  if [ $AUTO_CL -eq 1 ]; then
>>  	# Send the cover letter to every recipient, both specified as well as
>>  	# harvested. Then remove it from the patches list.
>> -	eval "git send-email $GIT_TO $GIT_CC $GIT_EXTRA_CC --confirm=always --no-chain-reply-to --suppress-cc=all $CL"
>> +	eval "git send-email $GIT_TO $GIT_CC $GIT_EXTRA_CC --confirm=always --no-thread --suppress-cc=all $CL"
>>  	if [ $? -eq 1 ]; then
>>  		echo "ERROR: failed to send cover-letter with automatic recipients."
>>  		exit 1
> 
> 
> And I don't understand why this proposed change has the described
> effect. Does changing the threading parameters change the output of "git
> send-email" and thus indirectly the mail headers of the following
> patches?
> 

-- 
Jose Lamego | OTC Embedded Platforms & Tools | GDC


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 501 bytes --]

Re: [PATCH V2] scripts.send-pull-request: Avoid multiple chain headers

[Patrick Ohly]

On Mon, 2016-11-28 at 14:28 -0600, Jose Lamego wrote:
> Agree. Please provide feedback about below comments and I will submit a
> v3 patch.
> 
> On 11/28/2016 01:47 PM, Patrick Ohly wrote:
> > On Mon, 2016-11-28 at 10:23 -0600, Jose Lamego wrote:
> More than 1 "In-Reply-To" and "References" message headers are in
> violation of rfc2822 [1] and may cause that some email-related
> applications do not point to the appropriate root message in a
> conversation/series.

Fixing that makes sense. Just add it as reason and the "why" part is
covered.

> > And I don't understand why this proposed change has the described
> > effect. Does changing the threading parameters change the output of "git
> > send-email" and thus indirectly the mail headers of the following
> > patches?

The "how" part still isn't clear to me. Perhaps I'm just dumb, but would
you bear with me and explain a bit more how changing the sending of the
cover letter affects sending of the patches?

As it isn't obvious, perhaps even add a comment to the script explaining
it.

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

Re: [PATCH V2] scripts.send-pull-request: Avoid multiple chain headers

[Jose Lamego]

[-- Attachment #1.1: Type: text/plain, Size: 2285 bytes --]



On 11/28/2016 03:34 PM, Patrick Ohly wrote:
> On Mon, 2016-11-28 at 14:28 -0600, Jose Lamego wrote:
>> Agree. Please provide feedback about below comments and I will submit a
>> v3 patch.
>>
>> On 11/28/2016 01:47 PM, Patrick Ohly wrote:
>>> On Mon, 2016-11-28 at 10:23 -0600, Jose Lamego wrote:
>> More than 1 "In-Reply-To" and "References" message headers are in
>> violation of rfc2822 [1] and may cause that some email-related
>> applications do not point to the appropriate root message in a
>> conversation/series.
> 
> Fixing that makes sense. Just add it as reason and the "why" part is
> covered.
> 
>>> And I don't understand why this proposed change has the described
>>> effect. Does changing the threading parameters change the output of "git
>>> send-email" and thus indirectly the mail headers of the following
>>> patches?
> 
> The "how" part still isn't clear to me. Perhaps I'm just dumb, but would
> you bear with me and explain a bit more how changing the sending of the
> cover letter affects sending of the patches?
> 
The script is duplicating the headers because it contains two individual
calls to git-send-email, one for the cover letter (when available) and
one for the rest of the series patches, both using the --no-chain-reply
option that includes a reference to the first message. What I'm doing
here is to include no reference to any root message at the first call,
then including a reference at the second call to the very first message
in the chain, which is either the cover letter or the patch #1.
This change is currently implemented/tested at [2] and complements a
change in patchwork [3] that handles messages including repeated headers
(created before this change gets implemented).

So the comment I would add to patch is:
    This change appends only one header pointing to very first patch
    in series or to cover letter if available by calling send-email
    with thread history option only once, instead of the original twice.

[2] patchwork-staging.openembedded.org
[3] https://lists.yoctoproject.org/pipermail/yocto/2016-November/033200.html
> As it isn't obvious, perhaps even add a comment to the script explaining
> it.
> 

-- 
Jose Lamego | OTC Embedded Platforms & Tools | GDC


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 501 bytes --]

Re: [PATCH V2] scripts.send-pull-request: Avoid multiple chain headers

[Patrick Ohly]

On Mon, 2016-11-28 at 16:35 -0600, Jose Lamego wrote:
> 
> On 11/28/2016 03:34 PM, Patrick Ohly wrote:
> > On Mon, 2016-11-28 at 14:28 -0600, Jose Lamego wrote:
> >> Agree. Please provide feedback about below comments and I will submit a
> >> v3 patch.
> >>
> >> On 11/28/2016 01:47 PM, Patrick Ohly wrote:
> >>> On Mon, 2016-11-28 at 10:23 -0600, Jose Lamego wrote:
> >> More than 1 "In-Reply-To" and "References" message headers are in
> >> violation of rfc2822 [1] and may cause that some email-related
> >> applications do not point to the appropriate root message in a
> >> conversation/series.
> > 
> > Fixing that makes sense. Just add it as reason and the "why" part is
> > covered.
> > 
> >>> And I don't understand why this proposed change has the described
> >>> effect. Does changing the threading parameters change the output of "git
> >>> send-email" and thus indirectly the mail headers of the following
> >>> patches?
> > 
> > The "how" part still isn't clear to me. Perhaps I'm just dumb, but would
> > you bear with me and explain a bit more how changing the sending of the
> > cover letter affects sending of the patches?

I've tried out your proposed change with
  bash -x ../poky/scripts/send-pull-request --to=patrick.ohly@gmx.de -p pull-11827
where pull-11827 is my recent bitbake submission.

The resulting emails are still broken because that one line that you
modify isn't event used. It's under "if [ $AUTO_CL -eq 1 ]" and I am not
using the -a option that enables that behavior.

Even when I use -a, the result is still broken.

The root cause of the problem is that both create-pull-request and
send-pull-request allow git to insert In-Reply-To headers.

"git send-email --help" explicitly warns about that:

        It is up to the user to ensure that no In-Reply-To header already exists when git send-email is asked
        to add it (especially note that git format-patch can be configured to do the threading itself). Failure
        to do so may not produce the expected result in the recipient’s MUA.

>  What I'm doing
> here is to include no reference to any root message at the first call,
> then including a reference at the second call to the very first message
> in the chain, which is either the cover letter or the patch #1.

No, that doesn't work. Whether the first call uses --no-thread or
--no-chain-reply-to has no effect whatsoever, because when "git
send-email" only sends a single email, it doesn't add headers, and the
second call was left unmodified in your patch.

The right fix (tested successfully here) is to use --no-thread in the
second call which sends the sequence of patches. I'll send my  change
for review separately.

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

[PATCH V3] scripts/send-pull-request: Avoid multiple chain headers

[Patrick Ohly]

When creating a patch set with cover letter using the
send-pull-request script, both the "In-Reply-To" and "References"
headers are appended twice in patch 2 and subsequent.

That's because git-format-patch already inserted them and then
git-send-email repeats that. Suppressing mail threading in
git-send-email with --no-thread avoids the problem and is the
right solution because it works regardless whether git-send-email is
called once or twicee.

Repeating these headers is a violation of RFC 2822 and can confuse
mail programs. For example, Patchwork does not detect a patch series
problem when there are these extra headers.

[YOCTO #10718]

Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
---
 scripts/send-pull-request | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/scripts/send-pull-request b/scripts/send-pull-request
index 575549d..883deac 100755
--- a/scripts/send-pull-request
+++ b/scripts/send-pull-request
@@ -158,11 +158,16 @@ GIT_EXTRA_CC=$(for R in $EXTRA_CC; do echo -n "--cc='$R' "; done)
 unset IFS
 
 # Handoff to git-send-email. It will perform the send confirmation.
+# Mail threading was already handled by git-format-patch in
+# create-pull-request, so we must not allow git-send-email to
+# add In-Reply-To and References headers again.
 PATCHES=$(echo $PDIR/*.patch)
 if [ $AUTO_CL -eq 1 ]; then
 	# Send the cover letter to every recipient, both specified as well as
 	# harvested. Then remove it from the patches list.
-	eval "git send-email $GIT_TO $GIT_CC $GIT_EXTRA_CC --confirm=always --no-chain-reply-to --suppress-cc=all $CL"
+	# --no-thread is redundant here (only sending a single message) and
+	# merely added for the sake of consistency.
+	eval "git send-email $GIT_TO $GIT_CC $GIT_EXTRA_CC --confirm=always --no-thread --suppress-cc=all $CL"
 	if [ $? -eq 1 ]; then
 		echo "ERROR: failed to send cover-letter with automatic recipients."
 		exit 1
@@ -172,7 +177,7 @@ fi
 
 # Send the patch to the specified recipients and, if -c was specified, those git
 # finds in this specific patch.
-eval "git send-email $GIT_TO $GIT_EXTRA_CC --confirm=always --no-chain-reply-to $GITSOBCC $PATCHES"
+eval "git send-email $GIT_TO $GIT_EXTRA_CC --confirm=always --no-thread $GITSOBCC $PATCHES"
 if [ $? -eq 1 ]; then
 	echo "ERROR: failed to send patches."
 	exit 1
-- 
2.1.4

Re: [PATCH V2] scripts.send-pull-request: Avoid multiple chain headers

[Jose Lamego]

[-- Attachment #1.1: Type: text/plain, Size: 3193 bytes --]



On 11/29/2016 02:51 AM, Patrick Ohly wrote:
> On Mon, 2016-11-28 at 16:35 -0600, Jose Lamego wrote:
>>
>> On 11/28/2016 03:34 PM, Patrick Ohly wrote:
>>> On Mon, 2016-11-28 at 14:28 -0600, Jose Lamego wrote:
>>>> Agree. Please provide feedback about below comments and I will submit a
>>>> v3 patch.
>>>>
>>>> On 11/28/2016 01:47 PM, Patrick Ohly wrote:
>>>>> On Mon, 2016-11-28 at 10:23 -0600, Jose Lamego wrote:
>>>> More than 1 "In-Reply-To" and "References" message headers are in
>>>> violation of rfc2822 [1] and may cause that some email-related
>>>> applications do not point to the appropriate root message in a
>>>> conversation/series.
>>>
>>> Fixing that makes sense. Just add it as reason and the "why" part is
>>> covered.
>>>
>>>>> And I don't understand why this proposed change has the described
>>>>> effect. Does changing the threading parameters change the output of "git
>>>>> send-email" and thus indirectly the mail headers of the following
>>>>> patches?
>>>
>>> The "how" part still isn't clear to me. Perhaps I'm just dumb, but would
>>> you bear with me and explain a bit more how changing the sending of the
>>> cover letter affects sending of the patches?
> 
> I've tried out your proposed change with
>   bash -x ../poky/scripts/send-pull-request --to=patrick.ohly@gmx.de -p pull-11827
> where pull-11827 is my recent bitbake submission.
> 
> The resulting emails are still broken because that one line that you
> modify isn't event used. It's under "if [ $AUTO_CL -eq 1 ]" and I am not
> using the -a option that enables that behavior.
> 
> Even when I use -a, the result is still broken.
> 
> The root cause of the problem is that both create-pull-request and
> send-pull-request allow git to insert In-Reply-To headers.
> 
> "git send-email --help" explicitly warns about that:
> 
>         It is up to the user to ensure that no In-Reply-To header already exists when git send-email is asked
>         to add it (especially note that git format-patch can be configured to do the threading itself). Failure
>         to do so may not produce the expected result in the recipient’s MUA.
> 
>>  What I'm doing
>> here is to include no reference to any root message at the first call,
>> then including a reference at the second call to the very first message
>> in the chain, which is either the cover letter or the patch #1.
> 
> No, that doesn't work. Whether the first call uses --no-thread or
> --no-chain-reply-to has no effect whatsoever, because when "git
> send-email" only sends a single email, it doesn't add headers, and the
> second call was left unmodified in your patch.
> 
You are right, I wrongly tested using patches created with
git-format-patch command, and then send-pull-request which produced a
correctly created chain with only the first change, but didn't tested
creating a pull request in the first place, which is the appropriate.

> The right fix (tested successfully here) is to use --no-thread in the
> second call which sends the sequence of patches. I'll send my  change
> for review separately.
> 

-- 
Jose Lamego | OTC Embedded Platforms & Tools | GDC


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 501 bytes --]