Maintainers, please add Message-Id: when merging patches

Maintainers, please add Message-Id: when merging patches

[Stefan Hajnoczi]

Around 66% of qemu.git commits since v4.1.0 include a Message-Id: tag.  Hooray!

Message-Id: references the patch email that a commit was merged from.
This information is helpful to anyone wishing to refer back to email
discussions and patch series.

Please use git-am(1) -m/--message-id or set am.messageid in your git-config(1).

If you use the patches tool (https://github.com/stefanha/patches) then
Message-Id: is added automatically.

Thanks,
Stefan

Re: Maintainers, please add Message-Id: when merging patches

[Kevin Wolf]

Am 22.01.2020 um 13:02 hat Stefan Hajnoczi geschrieben:
> Around 66% of qemu.git commits since v4.1.0 include a Message-Id: tag.  Hooray!
> 
> Message-Id: references the patch email that a commit was merged from.
> This information is helpful to anyone wishing to refer back to email
> discussions and patch series.
> 
> Please use git-am(1) -m/--message-id or set am.messageid in your git-config(1).

I've had -m in my scripts for a while (last time someone asked me to
make the change, I guess), but it wasn't effective, because my .muttrc
has 'set pipe_decode' enabled, which doesn't only decode the output, but
also throws away most headers.

I seem to remember that this was necessary at some point because
otherwise some mails just wouldn't apply. Maybe 'git am' works better
these days and can actually parse the mails that used to give me
problems. I'll give it a try and disable pipe_decode.

Kevin

Re: Maintainers, please add Message-Id: when merging patches

[Kevin Wolf]

Am 22.01.2020 um 13:28 hat Kevin Wolf geschrieben:
> Am 22.01.2020 um 13:02 hat Stefan Hajnoczi geschrieben:
> > Around 66% of qemu.git commits since v4.1.0 include a Message-Id: tag.  Hooray!
> > 
> > Message-Id: references the patch email that a commit was merged from.
> > This information is helpful to anyone wishing to refer back to email
> > discussions and patch series.
> > 
> > Please use git-am(1) -m/--message-id or set am.messageid in your git-config(1).
> 
> I've had -m in my scripts for a while (last time someone asked me to
> make the change, I guess), but it wasn't effective, because my .muttrc
> has 'set pipe_decode' enabled, which doesn't only decode the output, but
> also throws away most headers.
> 
> I seem to remember that this was necessary at some point because
> otherwise some mails just wouldn't apply. Maybe 'git am' works better
> these days and can actually parse the mails that used to give me
> problems. I'll give it a try and disable pipe_decode.

Here is the first patch for which it failed for me:

Message-ID: <20200123124357.124019-1-felipe@nutanix.com>

The problem seems to be related to line endings because the patch that
git-apply sees eventually has "\r\n" whereas the file to be patched has
only "\n".

If I understand correctly (this is a bit of guesswork after reading man
pages and trying out a few options), git-mailsplit would normally get
rid of the "\r". However, this specific patch email is base64 encoded,
so the encoded "\r" characters survive this stage.

git-mailinfo later decodes the email, but doesn't seem to do anything
about "\r" again, so it survives this one as well. This means feeding a
patch with the wrong line endings to git-apply, which just fails.

Any suggestion how to fix this? (For this patch, I just enabled
pipe_decode again, so no Message-Id tag for it.)

Kevin

Re: Maintainers, please add Message-Id: when merging patches

[Laszlo Ersek]

On 01/23/20 18:18, Kevin Wolf wrote:
> Am 22.01.2020 um 13:28 hat Kevin Wolf geschrieben:
>> Am 22.01.2020 um 13:02 hat Stefan Hajnoczi geschrieben:
>>> Around 66% of qemu.git commits since v4.1.0 include a Message-Id: tag.  Hooray!
>>>
>>> Message-Id: references the patch email that a commit was merged from.
>>> This information is helpful to anyone wishing to refer back to email
>>> discussions and patch series.
>>>
>>> Please use git-am(1) -m/--message-id or set am.messageid in your git-config(1).
>>
>> I've had -m in my scripts for a while (last time someone asked me to
>> make the change, I guess), but it wasn't effective, because my .muttrc
>> has 'set pipe_decode' enabled, which doesn't only decode the output, but
>> also throws away most headers.
>>
>> I seem to remember that this was necessary at some point because
>> otherwise some mails just wouldn't apply. Maybe 'git am' works better
>> these days and can actually parse the mails that used to give me
>> problems. I'll give it a try and disable pipe_decode.
> 
> Here is the first patch for which it failed for me:
> 
> Message-ID: <20200123124357.124019-1-felipe@nutanix.com>
> 
> The problem seems to be related to line endings because the patch that
> git-apply sees eventually has "\r\n" whereas the file to be patched has
> only "\n".
> 
> If I understand correctly (this is a bit of guesswork after reading man
> pages and trying out a few options), git-mailsplit would normally get
> rid of the "\r". However, this specific patch email is base64 encoded,
> so the encoded "\r" characters survive this stage.
> 
> git-mailinfo later decodes the email, but doesn't seem to do anything
> about "\r" again, so it survives this one as well. This means feeding a
> patch with the wrong line endings to git-apply, which just fails.
> 
> Any suggestion how to fix this? (For this patch, I just enabled
> pipe_decode again, so no Message-Id tag for it.)

In my opinion, the patch you mention is malformed.

I saved it to a local file with Thunderbird, saved the base64-encoded
body to a separate file, and decoded it with a naked "base64 --decode"
invocation. The result is a file with CRLF line terminators.

When someone sends a base64-encoded patch email, that's a statement
(again: IMO) that the patch conforms to the "canonical" checkout (=
working tree) line ending convention. For QEMU, I would think that said
convention dictates LF.

Note: I'm aware that with git, the "internal" representation for
newlines, and the "external" one, are different things. Dependent on
whether one is on Windows vs. Linux, git-checkout will produce CRLF vs.
LF in the working tree, as the "external" newline representation. What
I'm saying is that, if someone sends a base64-encoded patch, that's a
statement that their *external* newline representation matches that of
the people that they expect to apply the patch. Normally, external
representations (i.e., the local working trees' newline representations)
don't have to match each other -- but if a patch is sent with base64
Content-Transfer-Encoding, then I claim that they do.

IOW, I'd simply answer the patch in question with:

"""
Please resend the patch with one of the following options:
- use LF in your local working tree, and keep the base64 C-T-E, or
- keep CRLF in your local working tree, and send with 8bit C-T-E.
"""

As a practical result, if someone develops QEMU in a Windows
environment, they should only use 8bit C-T-E when posting patches.

Strictly my personal opinion, of course.

Thanks,
Laszlo

Re: Maintainers, please add Message-Id: when merging patches

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2112 bytes --]

On Thu, Jan 23, 2020 at 06:18:57PM +0100, Kevin Wolf wrote:
> Am 22.01.2020 um 13:28 hat Kevin Wolf geschrieben:
> > Am 22.01.2020 um 13:02 hat Stefan Hajnoczi geschrieben:
> > > Around 66% of qemu.git commits since v4.1.0 include a Message-Id: tag.  Hooray!
> > > 
> > > Message-Id: references the patch email that a commit was merged from.
> > > This information is helpful to anyone wishing to refer back to email
> > > discussions and patch series.
> > > 
> > > Please use git-am(1) -m/--message-id or set am.messageid in your git-config(1).
> > 
> > I've had -m in my scripts for a while (last time someone asked me to
> > make the change, I guess), but it wasn't effective, because my .muttrc
> > has 'set pipe_decode' enabled, which doesn't only decode the output, but
> > also throws away most headers.
> > 
> > I seem to remember that this was necessary at some point because
> > otherwise some mails just wouldn't apply. Maybe 'git am' works better
> > these days and can actually parse the mails that used to give me
> > problems. I'll give it a try and disable pipe_decode.
> 
> Here is the first patch for which it failed for me:
> 
> Message-ID: <20200123124357.124019-1-felipe@nutanix.com>
> 
> The problem seems to be related to line endings because the patch that
> git-apply sees eventually has "\r\n" whereas the file to be patched has
> only "\n".
> 
> If I understand correctly (this is a bit of guesswork after reading man
> pages and trying out a few options), git-mailsplit would normally get
> rid of the "\r". However, this specific patch email is base64 encoded,
> so the encoded "\r" characters survive this stage.
> 
> git-mailinfo later decodes the email, but doesn't seem to do anything
> about "\r" again, so it survives this one as well. This means feeding a
> patch with the wrong line endings to git-apply, which just fails.
> 
> Any suggestion how to fix this? (For this patch, I just enabled
> pipe_decode again, so no Message-Id tag for it.)

This might be a good question for the git mailing list
<git@vger.kernel.org>.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Maintainers, please add Message-Id: when merging patches

[Alex Bennée]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> Around 66% of qemu.git commits since v4.1.0 include a Message-Id: tag.  Hooray!
>
> Message-Id: references the patch email that a commit was merged from.
> This information is helpful to anyone wishing to refer back to email
> discussions and patch series.

So I guess the ones that don't are maintainer originated patches unless
you actively rebuild your trees from a posted series?

>
> Please use git-am(1) -m/--message-id or set am.messageid in your git-config(1).
>
> If you use the patches tool (https://github.com/stefanha/patches) then
> Message-Id: is added automatically.
>
> Thanks,
> Stefan


-- 
Alex Bennée

Re: Maintainers, please add Message-Id: when merging patches

[Cornelia Huck]

On Wed, 22 Jan 2020 12:30:03 +0000
Alex Bennée <alex.bennee@linaro.org> wrote:

> Stefan Hajnoczi <stefanha@gmail.com> writes:
> 
> > Around 66% of qemu.git commits since v4.1.0 include a Message-Id: tag.  Hooray!
> >
> > Message-Id: references the patch email that a commit was merged from.
> > This information is helpful to anyone wishing to refer back to email
> > discussions and patch series.  
> 
> So I guess the ones that don't are maintainer originated patches unless
> you actively rebuild your trees from a posted series?

I usually paste in the message id by hand in that case (unless I
forget), which is a bit of a PITA; but so would be re-applying the sent
out patches...

One thing that probably won't have a message id is s390-ccw bios
rebuild patches, because sending that out for extra review just does
not make sense.

> 
> >
> > Please use git-am(1) -m/--message-id or set am.messageid in your git-config(1).
> >
> > If you use the patches tool (https://github.com/stefanha/patches) then
> > Message-Id: is added automatically.
> >
> > Thanks,
> > Stefan  
> 
> 

Re: Maintainers, please add Message-Id: when merging patches

[Laszlo Ersek]

On 01/22/20 13:30, Alex Bennée wrote:
> 
> Stefan Hajnoczi <stefanha@gmail.com> writes:
> 
>> Around 66% of qemu.git commits since v4.1.0 include a Message-Id: tag.  Hooray!
>>
>> Message-Id: references the patch email that a commit was merged from.
>> This information is helpful to anyone wishing to refer back to email
>> discussions and patch series.
> 
> So I guess the ones that don't are maintainer originated patches unless
> you actively rebuild your trees from a posted series?

I *think* this should not be a huge problem process wise:

Assuming that a maintainer does not include their own patches in a PULL
request for Peter until the same patches receive R-b/A-b/T-b feedback
from other list subscribers, the maintainer will want to rebase the
patches at least once anyway, in order to pick up those lines.

And, in the process, the maintainer might as well add in their own
Message-Id's from the list.

... I realize though, that could be more burden in practice than just
running git-am against the same (known) base commit... One could always
run git-range-diff in the end, to compare the "re-pick" versus the
original local branch.

Thanks
Laszlo

Re: Maintainers, please add Message-Id: when merging patches

[Alex Bennée]

Laszlo Ersek <lersek@redhat.com> writes:

> On 01/22/20 13:30, Alex Bennée wrote:
>> 
>> Stefan Hajnoczi <stefanha@gmail.com> writes:
>> 
>>> Around 66% of qemu.git commits since v4.1.0 include a Message-Id: tag.  Hooray!
>>>
>>> Message-Id: references the patch email that a commit was merged from.
>>> This information is helpful to anyone wishing to refer back to email
>>> discussions and patch series.
>> 
>> So I guess the ones that don't are maintainer originated patches unless
>> you actively rebuild your trees from a posted series?
>
> I *think* this should not be a huge problem process wise:
>
> Assuming that a maintainer does not include their own patches in a PULL
> request for Peter until the same patches receive R-b/A-b/T-b feedback
> from other list subscribers, the maintainer will want to rebase the
> patches at least once anyway, in order to pick up those lines.

Oh I always do a re-base as I apply the r-b/t-b tags. But that is
working off my tree and a bunch of references to the emails with the
appropriate tags in them.

So which Message-Id should I use. The first time the patch was posted to
the list or the last time it was?

> And, in the process, the maintainer might as well add in their own
> Message-Id's from the list.
>
> ... I realize though, that could be more burden in practice than just
> running git-am against the same (known) base commit... One could always
> run git-range-diff in the end, to compare the "re-pick" versus the
> original local branch.

I'm obviously missing out by not using patches but my own Emacs based
tooling. I guess I shall have to see if I can extend it.

>
> Thanks
> Laszlo


-- 
Alex Bennée

Re: Maintainers, please add Message-Id: when merging patches

[Cornelia Huck]

On Wed, 22 Jan 2020 18:56:47 +0000
Alex Bennée <alex.bennee@linaro.org> wrote:

> Laszlo Ersek <lersek@redhat.com> writes:
> 
> > On 01/22/20 13:30, Alex Bennée wrote:  
> >> 
> >> Stefan Hajnoczi <stefanha@gmail.com> writes:
> >>   
> >>> Around 66% of qemu.git commits since v4.1.0 include a Message-Id: tag.  Hooray!
> >>>
> >>> Message-Id: references the patch email that a commit was merged from.
> >>> This information is helpful to anyone wishing to refer back to email
> >>> discussions and patch series.  
> >> 
> >> So I guess the ones that don't are maintainer originated patches unless
> >> you actively rebuild your trees from a posted series?  
> >
> > I *think* this should not be a huge problem process wise:
> >
> > Assuming that a maintainer does not include their own patches in a PULL
> > request for Peter until the same patches receive R-b/A-b/T-b feedback
> > from other list subscribers, the maintainer will want to rebase the
> > patches at least once anyway, in order to pick up those lines.  
> 
> Oh I always do a re-base as I apply the r-b/t-b tags. But that is
> working off my tree and a bunch of references to the emails with the
> appropriate tags in them.
> 
> So which Message-Id should I use. The first time the patch was posted to
> the list or the last time it was?

From the last one? I mean, I'll pick the last incarnation if I apply
someone else's patches, as well?

[I just add the id right before I send my 'queued' email.]

Re: Maintainers, please add Message-Id: when merging patches

[Laszlo Ersek]

On 01/22/20 20:07, Cornelia Huck wrote:
> On Wed, 22 Jan 2020 18:56:47 +0000
> Alex Bennée <alex.bennee@linaro.org> wrote:
> 
>> Laszlo Ersek <lersek@redhat.com> writes:
>>
>>> On 01/22/20 13:30, Alex Bennée wrote:  
>>>>
>>>> Stefan Hajnoczi <stefanha@gmail.com> writes:
>>>>   
>>>>> Around 66% of qemu.git commits since v4.1.0 include a Message-Id: tag.  Hooray!
>>>>>
>>>>> Message-Id: references the patch email that a commit was merged from.
>>>>> This information is helpful to anyone wishing to refer back to email
>>>>> discussions and patch series.  
>>>>
>>>> So I guess the ones that don't are maintainer originated patches unless
>>>> you actively rebuild your trees from a posted series?  
>>>
>>> I *think* this should not be a huge problem process wise:
>>>
>>> Assuming that a maintainer does not include their own patches in a PULL
>>> request for Peter until the same patches receive R-b/A-b/T-b feedback
>>> from other list subscribers, the maintainer will want to rebase the
>>> patches at least once anyway, in order to pick up those lines.  
>>
>> Oh I always do a re-base as I apply the r-b/t-b tags. But that is
>> working off my tree and a bunch of references to the emails with the
>> appropriate tags in them.
>>
>> So which Message-Id should I use. The first time the patch was posted to
>> the list or the last time it was?
> 
> From the last one? I mean, I'll pick the last incarnation if I apply
> someone else's patches, as well?

I think so as well -- pick the IDs from those messages of yours that
another maintainer would apply with git-am.

(BTW I've had another thought -- git-send-email prints the message IDs
it generates while sending the emails, so one could pick those up with a
git-rebase/reword right after posting, too.)

Thanks,
Laszlo

> 
> [I just add the id right before I send my 'queued' email.]
> 

Re: Maintainers, please add Message-Id: when merging patches

[Gerd Hoffmann]

On Wed, Jan 22, 2020 at 12:30:03PM +0000, Alex Bennée wrote:
> 
> Stefan Hajnoczi <stefanha@gmail.com> writes:
> 
> > Around 66% of qemu.git commits since v4.1.0 include a Message-Id: tag.  Hooray!
> >
> > Message-Id: references the patch email that a commit was merged from.
> > This information is helpful to anyone wishing to refer back to email
> > discussions and patch series.
> 
> So I guess the ones that don't are maintainer originated patches unless
> you actively rebuild your trees from a posted series?

This is what I usually do, using Stefan's "patches" utility.  That'll
pick up both message-id and any tested/reviewed/acked-by tags in
replies.

cheers,
  Gerd

Re: Maintainers, please add Message-Id: when merging patches

[Markus Armbruster]

Alex Bennée <alex.bennee@linaro.org> writes:

> Stefan Hajnoczi <stefanha@gmail.com> writes:
>
>> Around 66% of qemu.git commits since v4.1.0 include a Message-Id: tag.  Hooray!
>>
>> Message-Id: references the patch email that a commit was merged from.
>> This information is helpful to anyone wishing to refer back to email
>> discussions and patch series.
>
> So I guess the ones that don't are maintainer originated patches unless
> you actively rebuild your trees from a posted series?

I recommend using the exact same workflow for constructing pull requests
whether you wrote the patches yourself or not.

Re: Maintainers, please add Message-Id: when merging patches

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 944 bytes --]

On Thu, Jan 23, 2020 at 09:27:54AM +0100, Markus Armbruster wrote:
> Alex Bennée <alex.bennee@linaro.org> writes:
> 
> > Stefan Hajnoczi <stefanha@gmail.com> writes:
> >
> >> Around 66% of qemu.git commits since v4.1.0 include a Message-Id: tag.  Hooray!
> >>
> >> Message-Id: references the patch email that a commit was merged from.
> >> This information is helpful to anyone wishing to refer back to email
> >> discussions and patch series.
> >
> > So I guess the ones that don't are maintainer originated patches unless
> > you actively rebuild your trees from a posted series?
> 
> I recommend using the exact same workflow for constructing pull requests
> whether you wrote the patches yourself or not.

Yes, that's the approach I follow too.  I use the 'patches' tool to
apply patch series, including my own.

It's necessary to get the Revewied-by:, Tested-by:, etc tags squashed in
automatically anyway.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]