* [PATCH] scripts.send-pull-request: Avoid multiple chain headers
@ 2016-11-25 20:30 Jose Lamego
2016-11-28 16:23 ` [PATCH V2] " Jose Lamego
0 siblings, 1 reply; 9+ messages in thread
From: Jose Lamego @ 2016-11-25 20:30 UTC (permalink / raw)
To: openembedded-core
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH V2] scripts.send-pull-request: Avoid multiple chain headers
2016-11-25 20:30 [PATCH] scripts.send-pull-request: Avoid multiple chain headers Jose Lamego
@ 2016-11-28 16:23 ` Jose Lamego
2016-11-28 19:47 ` Patrick Ohly
0 siblings, 1 reply; 9+ messages in thread
From: Jose Lamego @ 2016-11-28 16:23 UTC (permalink / raw)
To: openembedded-core
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH V2] scripts.send-pull-request: Avoid multiple chain headers
2016-11-28 16:23 ` [PATCH V2] " Jose Lamego
@ 2016-11-28 19:47 ` Patrick Ohly
2016-11-28 20:28 ` Jose Lamego
0 siblings, 1 reply; 9+ messages in thread
From: Patrick Ohly @ 2016-11-28 19:47 UTC (permalink / raw)
To: Jose Lamego; +Cc: openembedded-core
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2] scripts.send-pull-request: Avoid multiple chain headers
2016-11-28 19:47 ` Patrick Ohly
@ 2016-11-28 20:28 ` Jose Lamego
2016-11-28 21:34 ` Patrick Ohly
0 siblings, 1 reply; 9+ messages in thread
From: Jose Lamego @ 2016-11-28 20:28 UTC (permalink / raw)
To: Patrick Ohly; +Cc: openembedded-core
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2] scripts.send-pull-request: Avoid multiple chain headers
2016-11-28 20:28 ` Jose Lamego
@ 2016-11-28 21:34 ` Patrick Ohly
2016-11-28 22:35 ` Jose Lamego
0 siblings, 1 reply; 9+ messages in thread
From: Patrick Ohly @ 2016-11-28 21:34 UTC (permalink / raw)
To: Jose Lamego; +Cc: openembedded-core
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2] scripts.send-pull-request: Avoid multiple chain headers
2016-11-28 21:34 ` Patrick Ohly
@ 2016-11-28 22:35 ` Jose Lamego
2016-11-29 8:51 ` Patrick Ohly
0 siblings, 1 reply; 9+ messages in thread
From: Jose Lamego @ 2016-11-28 22:35 UTC (permalink / raw)
To: Patrick Ohly; +Cc: openembedded-core
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2] scripts.send-pull-request: Avoid multiple chain headers
2016-11-28 22:35 ` Jose Lamego
@ 2016-11-29 8:51 ` Patrick Ohly
2016-11-29 8:55 ` [PATCH V3] scripts/send-pull-request: " Patrick Ohly
2016-11-29 14:51 ` [PATCH V2] scripts.send-pull-request: " Jose Lamego
0 siblings, 2 replies; 9+ messages in thread
From: Patrick Ohly @ 2016-11-29 8:51 UTC (permalink / raw)
To: Jose Lamego; +Cc: openembedded-core
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH V3] scripts/send-pull-request: Avoid multiple chain headers
2016-11-29 8:51 ` Patrick Ohly
@ 2016-11-29 8:55 ` Patrick Ohly
2016-11-29 14:51 ` [PATCH V2] scripts.send-pull-request: " Jose Lamego
1 sibling, 0 replies; 9+ messages in thread
From: Patrick Ohly @ 2016-11-29 8:55 UTC (permalink / raw)
To: openembedded-core
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH V2] scripts.send-pull-request: Avoid multiple chain headers
2016-11-29 8:51 ` Patrick Ohly
2016-11-29 8:55 ` [PATCH V3] scripts/send-pull-request: " Patrick Ohly
@ 2016-11-29 14:51 ` Jose Lamego
1 sibling, 0 replies; 9+ messages in thread
From: Jose Lamego @ 2016-11-29 14:51 UTC (permalink / raw)
To: Patrick Ohly; +Cc: openembedded-core
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-11-29 14:51 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-25 20:30 [PATCH] scripts.send-pull-request: Avoid multiple chain headers Jose Lamego
2016-11-28 16:23 ` [PATCH V2] " Jose Lamego
2016-11-28 19:47 ` Patrick Ohly
2016-11-28 20:28 ` Jose Lamego
2016-11-28 21:34 ` Patrick Ohly
2016-11-28 22:35 ` Jose Lamego
2016-11-29 8:51 ` Patrick Ohly
2016-11-29 8:55 ` [PATCH V3] scripts/send-pull-request: " Patrick Ohly
2016-11-29 14:51 ` [PATCH V2] scripts.send-pull-request: " Jose Lamego
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.