Re: [Qemu-devel] Maintainers, please git-am -m
On 2/26/19 1:15 AM, Markus Armbruster wrote: >>> The Message-id identifies the patch e-mail. It makes finding the review >>> thread easier and more reliable. It's also a valid key on Patchew[*]. >> >> I find the tag valuable enough in later git searches that I don't mind >> feeding my own patches back through the mailing list to add it (patchew >> helps with that, of course). But for it to become mandatory, we'd need >> to enhance scripts/checkpatch.pl to enforce it. > > I'm afraid checkpatch is the wrong place. When you submit v1 patches > for review, there is no Message-id. Even for respins, we don't want > one. It should be added when patches get applied for real, so the > commit carries exactly one Message-id, and it refers back to the final > version on the list. Indeed, I wasn't thinking about how it would work. checkpatch.pl can filter for 'PULL' being in a subject line, as that will not affect normal submissions, but there is no guarantee that it will mesh with the workflows of the various maintainers. > > Two ideas: > > * Have Patchew flag pull requests lacking Message-id Yes, that works, as well as patching Peter's sanity-checking script to do likewise. > > * Admittedly vague: some kind of git pre merge hook magic to make > git-merge flag missing Message-id I'm not sure if Peter has his sanity-checker script plugged in via git hooks or some other way. https://git.linaro.org/people/peter.maydell/misc-scripts.git/ if someone wants to propose a patch to it. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] Maintainers, please git-am -m
Eric Blake writes: > On 2/8/19 1:30 AM, Markus Armbruster wrote: >> Short story: please add >> >> [am] >> messageid = true >> >> to your .gitconfig. >> >> Long story. git-am can add a Message-id: tag. Looks like this: >> > >> >> Signed-off-by: Thomas Huth >> Reviewed-by: Daniel P. Berrangé >> Reviewed-by: Philippe Mathieu-Daudé >> Tested-by: Philippe Mathieu-Daudé >> Acked-by: Alex Bennée >> --->Message-id: 1549268743-18502-1-git-send-email-th...@redhat.com >> Signed-off-by: Peter Maydell >> >> The Message-id identifies the patch e-mail. It makes finding the review >> thread easier and more reliable. It's also a valid key on Patchew[*]. > > I find the tag valuable enough in later git searches that I don't mind > feeding my own patches back through the mailing list to add it (patchew > helps with that, of course). But for it to become mandatory, we'd need > to enhance scripts/checkpatch.pl to enforce it. I'm afraid checkpatch is the wrong place. When you submit v1 patches for review, there is no Message-id. Even for respins, we don't want one. It should be added when patches get applied for real, so the commit carries exactly one Message-id, and it refers back to the final version on the list. Two ideas: * Have Patchew flag pull requests lacking Message-id * Admittedly vague: some kind of git pre merge hook magic to make git-merge flag missing Message-id
Re: [Qemu-devel] Maintainers, please git-am -m
On 2/8/19 1:30 AM, Markus Armbruster wrote: > Short story: please add > > [am] > messageid = true > > to your .gitconfig. > > Long story. git-am can add a Message-id: tag. Looks like this: > > > Signed-off-by: Thomas Huth > Reviewed-by: Daniel P. Berrangé > Reviewed-by: Philippe Mathieu-Daudé > Tested-by: Philippe Mathieu-Daudé > Acked-by: Alex Bennée > --->Message-id: 1549268743-18502-1-git-send-email-th...@redhat.com > Signed-off-by: Peter Maydell > > The Message-id identifies the patch e-mail. It makes finding the review > thread easier and more reliable. It's also a valid key on Patchew[*]. I find the tag valuable enough in later git searches that I don't mind feeding my own patches back through the mailing list to add it (patchew helps with that, of course). But for it to become mandatory, we'd need to enhance scripts/checkpatch.pl to enforce it. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] Maintainers, please git-am -m
On 08/02/19 17:07, Philippe Mathieu-Daudé wrote: > And yet this isn't a pull request but a simple patch, so the Message-Id > might not be very relevant there. Yes, I agree that Message-Id is not that relevant if you're taking a patch from someone else and then reposting it. But it's a relatively rare case when >1 people are collaborating on revisions of a series. Paolo
Re: [Qemu-devel] Maintainers, please git-am -m
Philippe Mathieu-Daudé writes:
> On 2/8/19 8:30 AM, Markus Armbruster wrote:
>> Short story: please add
>>
>> [am]
>> messageid = true
>>
>> to your .gitconfig.
>>
>> Long story. git-am can add a Message-id: tag. Looks like this:
>>
>> commit 335dbb5de1e98c4dc73590349f17bb2a4d72596c
>> Author: Thomas Huth
>> Date: Mon Feb 4 09:25:43 2019 +0100
>> Commit: Peter Maydell
>> CommitDate: Mon Feb 4 15:25:21 2019 +
>>
>> tests/docker/test-mingw and docs: Remove --with-sdlabi=2.0
>>
>> Patchew currently reports failures with the mingw docker test - this
>> is due to --with-sdlabi=2.0 configure flag which does not exist
>> anymore.
>> Remove this remainder from the docker test and the docs now.
>>
>> Signed-off-by: Thomas Huth
>> Reviewed-by: Daniel P. Berrangé
>> Reviewed-by: Philippe Mathieu-Daudé
>> Tested-by: Philippe Mathieu-Daudé
>> Acked-by: Alex Bennée
>> --->Message-id: 1549268743-18502-1-git-send-email-th...@redhat.com
>> Signed-off-by: Peter Maydell
>>
>> The Message-id identifies the patch e-mail. It makes finding the review
>> thread easier and more reliable. It's also a valid key on Patchew[*].
>
> It is sometimes confusing however, see:
> https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01992.html
>
> We have:
>
> Signed-off-by: Paolo Bonzini
> Message-Id: <20190123065618.3520-23-yang.zh...@intel.com>
> Signed-off-by: Paolo Bonzini
>
> This is probably a special case, and eventually a Patchew limitation,
> but I first wondered who wrote this patch? Since the Message-Id is from
> Yang Zhong, is the Signed-off-by tag incorrect?
> Then I went thru the previous version and understood the author is
> indeed Paolo, but his patch was first sent by Yang Zhong, then he
> applied his own patch sent by Yang.
>
> And yet this isn't a pull request but a simple patch, so the Message-Id
> might not be very relevant there.
git-am appears to add a Message-Id only if none is present.
Ideally, people don't submit patches with Message-Id, and maintainers do
use git-am -m. Given how losely we enforce our process, we're bound to
deviate from the ideal, just like for other patch submission details.
All I'm asking for is a best effort.
>> Sadly, not all of our commits don't carry it. Here's how top committers
>
> 'committers' -> 'maintainers'?
>
> (Actually committers don't know their patch Message-Id before sending
> the patch).
I used "committers" in the sense of "whoever git-log shows in the
"Commit:" line. I agree these are maintainers in our work flow.
Calling them maintainers would've been clearer.
>> have been doing recently[**]:
>>
>>with without
>> 264 21 Peter Maydell
>> 65 2 Gerd Hoffmann
>> 64 0 Laurent Vivier
>> 64 0 Eric Blake
>> 62 1 Eduardo Habkost
>> 56 0 Markus Armbruster
>> 29 72 Richard Henderson
>> 24 24 Paolo Bonzini
>> 10 31 Marcel Apfelbaum
>> 0 107 Kevin Wolf
>> 0 106 David Gibson
>> 0 93 Michael S. Tsirkin
>> 0 81 Aleksandar Markovic
>> 0 69 Samuel Thibault
>> 0 54 Alex Bennée
>> 0 50 Thomas Huth
>> 0 29 Anthony PERARD
>> 0 28 Marc-André Lureau
>>
>> Not bad, but there's room for improvement.
>>
>>
>> [*] Try
>> https://patchew.org/search?q=id%3A1549268743-18502-1-git-send-email-thuth%40redhat.com
>>
>> [**] git-log master --no-merges --pretty=fuller --since '3 months ago' |
>> gawk '/^Commit: / { if (c) h[c][m]++; c=$0; m=0 } /^Message-[Ii]d: / {
>> m=1 } END { for (c in h) if (h[c][0] + h[c][1] > 25) printf "%7d %7d %s\n",
>> h[c][1], h[c][0], substr(c,13) }' | sort -nr
>>
Re: [Qemu-devel] Maintainers, please git-am -m
On 2/8/19 8:30 AM, Markus Armbruster wrote:
> Short story: please add
>
> [am]
> messageid = true
>
> to your .gitconfig.
>
> Long story. git-am can add a Message-id: tag. Looks like this:
>
> commit 335dbb5de1e98c4dc73590349f17bb2a4d72596c
> Author: Thomas Huth
> Date: Mon Feb 4 09:25:43 2019 +0100
> Commit: Peter Maydell
> CommitDate: Mon Feb 4 15:25:21 2019 +
>
> tests/docker/test-mingw and docs: Remove --with-sdlabi=2.0
>
> Patchew currently reports failures with the mingw docker test - this
> is due to --with-sdlabi=2.0 configure flag which does not exist
> anymore.
> Remove this remainder from the docker test and the docs now.
>
> Signed-off-by: Thomas Huth
> Reviewed-by: Daniel P. Berrangé
> Reviewed-by: Philippe Mathieu-Daudé
> Tested-by: Philippe Mathieu-Daudé
> Acked-by: Alex Bennée
> --->Message-id: 1549268743-18502-1-git-send-email-th...@redhat.com
> Signed-off-by: Peter Maydell
>
> The Message-id identifies the patch e-mail. It makes finding the review
> thread easier and more reliable. It's also a valid key on Patchew[*].
It is sometimes confusing however, see:
https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01992.html
We have:
Signed-off-by: Paolo Bonzini
Message-Id: <20190123065618.3520-23-yang.zh...@intel.com>
Signed-off-by: Paolo Bonzini
This is probably a special case, and eventually a Patchew limitation,
but I first wondered who wrote this patch? Since the Message-Id is from
Yang Zhong, is the Signed-off-by tag incorrect?
Then I went thru the previous version and understood the author is
indeed Paolo, but his patch was first sent by Yang Zhong, then he
applied his own patch sent by Yang.
And yet this isn't a pull request but a simple patch, so the Message-Id
might not be very relevant there.
>
> Sadly, not all of our commits don't carry it. Here's how top committers
'committers' -> 'maintainers'?
(Actually committers don't know their patch Message-Id before sending
the patch).
> have been doing recently[**]:
>
>with without
> 264 21 Peter Maydell
> 65 2 Gerd Hoffmann
> 64 0 Laurent Vivier
> 64 0 Eric Blake
> 62 1 Eduardo Habkost
> 56 0 Markus Armbruster
> 29 72 Richard Henderson
> 24 24 Paolo Bonzini
> 10 31 Marcel Apfelbaum
> 0 107 Kevin Wolf
> 0 106 David Gibson
> 0 93 Michael S. Tsirkin
> 0 81 Aleksandar Markovic
> 0 69 Samuel Thibault
> 0 54 Alex Bennée
> 0 50 Thomas Huth
> 0 29 Anthony PERARD
> 0 28 Marc-André Lureau
>
> Not bad, but there's room for improvement.
>
>
> [*] Try
> https://patchew.org/search?q=id%3A1549268743-18502-1-git-send-email-thuth%40redhat.com
>
> [**] git-log master --no-merges --pretty=fuller --since '3 months ago' | gawk
> '/^Commit: / { if (c) h[c][m]++; c=$0; m=0 } /^Message-[Ii]d: / { m=1 }
> END { for (c in h) if (h[c][0] + h[c][1] > 25) printf "%7d %7d %s\n",
> h[c][1], h[c][0], substr(c,13) }' | sort -nr
>
Re: [Qemu-devel] Maintainers, please git-am -m
On Fri, Feb 08, 2019 at 09:57:20AM +0100, Cornelia Huck wrote: > On Fri, 08 Feb 2019 08:30:11 +0100 > Markus Armbruster wrote: > > > Short story: please add > > > > [am] > > messageid = true > > > > to your .gitconfig. > > > > Long story. git-am can add a Message-id: tag. Looks like this: > > > > commit 335dbb5de1e98c4dc73590349f17bb2a4d72596c > > Author: Thomas Huth > > Date: Mon Feb 4 09:25:43 2019 +0100 > > Commit: Peter Maydell > > CommitDate: Mon Feb 4 15:25:21 2019 + > > > > tests/docker/test-mingw and docs: Remove --with-sdlabi=2.0 > > > > Patchew currently reports failures with the mingw docker test - this > > is due to --with-sdlabi=2.0 configure flag which does not exist > > anymore. > > Remove this remainder from the docker test and the docs now. > > > > Signed-off-by: Thomas Huth > > Reviewed-by: Daniel P. Berrangé > > Reviewed-by: Philippe Mathieu-Daudé > > Tested-by: Philippe Mathieu-Daudé > > Acked-by: Alex Bennée > > --->Message-id: 1549268743-18502-1-git-send-email-th...@redhat.com > > Signed-off-by: Peter Maydell > > > > The Message-id identifies the patch e-mail. It makes finding the review > > thread easier and more reliable. It's also a valid key on Patchew[*]. > > I'm using it for anything I git am, but I'm usually not git am'ing my > own patches (I just merge from the branch I have them on). Is the > message id valuable enough to warrant the extra round-trip for patches > that are committed by the authors themselves? I typically do that, using Stefan's "patches" tool, to automatically collect Reviewed-by: lines from replies. And that adds the message id too. cheers, Gerd
Re: [Qemu-devel] Maintainers, please git-am -m
Hi, > >with without > > 65 2 Gerd Hoffmann Oops. Two slipped through:( > I'd like to, but I can't. I'm still using git 1.8 and the -m parameter > is not supported there yet. rhel-7 software collection has newer git (2.9) available. HTH, Gerd
Re: [Qemu-devel] Maintainers, please git-am -m
On Fri, 08 Feb 2019 08:30:11 +0100 Markus Armbruster wrote: > Short story: please add > > [am] > messageid = true > > to your .gitconfig. > > Long story. git-am can add a Message-id: tag. Looks like this: > > commit 335dbb5de1e98c4dc73590349f17bb2a4d72596c > Author: Thomas Huth > Date: Mon Feb 4 09:25:43 2019 +0100 > Commit: Peter Maydell > CommitDate: Mon Feb 4 15:25:21 2019 + > > tests/docker/test-mingw and docs: Remove --with-sdlabi=2.0 > > Patchew currently reports failures with the mingw docker test - this > is due to --with-sdlabi=2.0 configure flag which does not exist > anymore. > Remove this remainder from the docker test and the docs now. > > Signed-off-by: Thomas Huth > Reviewed-by: Daniel P. Berrangé > Reviewed-by: Philippe Mathieu-Daudé > Tested-by: Philippe Mathieu-Daudé > Acked-by: Alex Bennée > --->Message-id: 1549268743-18502-1-git-send-email-th...@redhat.com > Signed-off-by: Peter Maydell > > The Message-id identifies the patch e-mail. It makes finding the review > thread easier and more reliable. It's also a valid key on Patchew[*]. I'm using it for anything I git am, but I'm usually not git am'ing my own patches (I just merge from the branch I have them on). Is the message id valuable enough to warrant the extra round-trip for patches that are committed by the authors themselves? [I can't say I've used it much myself, if at all.]
Re: [Qemu-devel] Maintainers, please git-am -m
On 2019-02-08 08:30, Markus Armbruster wrote: > Short story: please add > > [am] > messageid = true > > to your .gitconfig. > > Long story. git-am can add a Message-id: tag. Looks like this: > > commit 335dbb5de1e98c4dc73590349f17bb2a4d72596c > Author: Thomas Huth > Date: Mon Feb 4 09:25:43 2019 +0100 > Commit: Peter Maydell > CommitDate: Mon Feb 4 15:25:21 2019 + > > tests/docker/test-mingw and docs: Remove --with-sdlabi=2.0 > > Patchew currently reports failures with the mingw docker test - this > is due to --with-sdlabi=2.0 configure flag which does not exist > anymore. > Remove this remainder from the docker test and the docs now. > > Signed-off-by: Thomas Huth > Reviewed-by: Daniel P. Berrangé > Reviewed-by: Philippe Mathieu-Daudé > Tested-by: Philippe Mathieu-Daudé > Acked-by: Alex Bennée > --->Message-id: 1549268743-18502-1-git-send-email-th...@redhat.com > Signed-off-by: Peter Maydell > > The Message-id identifies the patch e-mail. It makes finding the review > thread easier and more reliable. It's also a valid key on Patchew[*]. > > Sadly, not all of our commits don't carry it. Here's how top committers > have been doing recently[**]: > >with without > 264 21 Peter Maydell > 65 2 Gerd Hoffmann > 64 0 Laurent Vivier > 64 0 Eric Blake > 62 1 Eduardo Habkost > 56 0 Markus Armbruster > 29 72 Richard Henderson > 24 24 Paolo Bonzini > 10 31 Marcel Apfelbaum > 0 107 Kevin Wolf > 0 106 David Gibson > 0 93 Michael S. Tsirkin > 0 81 Aleksandar Markovic > 0 69 Samuel Thibault > 0 54 Alex Bennée > 0 50 Thomas Huth > 0 29 Anthony PERARD > 0 28 Marc-André Lureau > > Not bad, but there's room for improvement. I'd like to, but I can't. I'm still using git 1.8 and the -m parameter is not supported there yet. Thomas
[Qemu-devel] Maintainers, please git-am -m
Short story: please add
[am]
messageid = true
to your .gitconfig.
Long story. git-am can add a Message-id: tag. Looks like this:
commit 335dbb5de1e98c4dc73590349f17bb2a4d72596c
Author: Thomas Huth
Date: Mon Feb 4 09:25:43 2019 +0100
Commit: Peter Maydell
CommitDate: Mon Feb 4 15:25:21 2019 +
tests/docker/test-mingw and docs: Remove --with-sdlabi=2.0
Patchew currently reports failures with the mingw docker test - this
is due to --with-sdlabi=2.0 configure flag which does not exist anymore.
Remove this remainder from the docker test and the docs now.
Signed-off-by: Thomas Huth
Reviewed-by: Daniel P. Berrangé
Reviewed-by: Philippe Mathieu-Daudé
Tested-by: Philippe Mathieu-Daudé
Acked-by: Alex Bennée
--->Message-id: 1549268743-18502-1-git-send-email-th...@redhat.com
Signed-off-by: Peter Maydell
The Message-id identifies the patch e-mail. It makes finding the review
thread easier and more reliable. It's also a valid key on Patchew[*].
Sadly, not all of our commits don't carry it. Here's how top committers
have been doing recently[**]:
with without
264 21 Peter Maydell
65 2 Gerd Hoffmann
64 0 Laurent Vivier
64 0 Eric Blake
62 1 Eduardo Habkost
56 0 Markus Armbruster
29 72 Richard Henderson
24 24 Paolo Bonzini
10 31 Marcel Apfelbaum
0 107 Kevin Wolf
0 106 David Gibson
0 93 Michael S. Tsirkin
0 81 Aleksandar Markovic
0 69 Samuel Thibault
0 54 Alex Bennée
0 50 Thomas Huth
0 29 Anthony PERARD
0 28 Marc-André Lureau
Not bad, but there's room for improvement.
[*] Try
https://patchew.org/search?q=id%3A1549268743-18502-1-git-send-email-thuth%40redhat.com
[**] git-log master --no-merges --pretty=fuller --since '3 months ago' | gawk
'/^Commit: / { if (c) h[c][m]++; c=$0; m=0 } /^Message-[Ii]d: / { m=1 } END
{ for (c in h) if (h[c][0] + h[c][1] > 25) printf "%7d %7d %s\n", h[c][1],
h[c][0], substr(c,13) }' | sort -nr
