make-pullreq | 11 +++++++++++ 1 file changed, 11 insertions(+)
Hi,
The Nacked-by tag can be used to manually hold a patch for further
review, or by automatic CI because of failing test.
We often miss travis-ci and shippable failures. These CI provide a easy
way to send email on failure, we can integrate the Nacked-by use there.
We can easily have patchew script send a Nacked-by tag.
If there is a consensus about using this tag, the following patch can be
added to Peter's management scripts:
https://git.linaro.org/people/pmaydell/misc-scripts.git/
If we move to another workflow, having this uniform tag can help future
merging scripts to avoid patch on hold to get automatically merged.
-- >8 --
Subject: make-pullreq: Do not automatically merge NAcked commits
The 'Nacked-by' tag is a polite way of holding a patch for
further review. Reviewers might share their disapproval with
it (see [1]).
CI scripts might NAck a patch if it breaks testing.
QEMU already thought about using this tag for CI by the past
(see [2]).
The patchwork tool already collects this tag (see [3]).
Also, there was a discussion at the last Open Source Summit
about standardizing it ([4]).
Maintainers might miss a such Nacked-by tag. Help them by
providing a last resort check before merging pull requests.
[1]
https://www.x.org/wiki/Development/Documentation/SubmittingPatches/#index1h1
[2] https://lists.gnu.org/archive/html/qemu-devel/2013-01/msg00196.html
[3]
http://git.ozlabs.org/?p=patchwork;a=blobdiff;f=apps/patchwork/models.py;h=fa213dc03e;hp=8871df0259e;hb=487b53576f;hpb=a59ebf107d84b
[4]
https://lore.kernel.org/workflows/CACT4Y+bxPxQ64HEO2uGRkbk9vJSeg64y10Lak4c2K54J7GyFFA@mail.gmail.com/
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
make-pullreq | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/make-pullreq b/make-pullreq
index 61c0f1d..fff0b2d 100755
--- a/make-pullreq
+++ b/make-pullreq
@@ -108,6 +108,17 @@ if [ "$bad" = "yes" ]; then
exit 1
fi
+# Check no commit contains a nacked-by tag
+for rev in $(git rev-list master..HEAD); do
+ if git log ${rev}^! | grep -iq "Nacked-by:"; then
+ echo "Error: commit ${rev} nacked"
+ bad=yes
+ fi
+done
+if [ "$bad" = "yes" ]; then
+ exit 1
+fi
+
# Check whether any authors needs to be corrected after SPF rewrites
if git shortlog --author=qemu-devel@nongnu.org master..HEAD | grep .; then
echo "ERROR: pull request includes commits attributed to list"
--
2.21.0
On Monday, December 9, 2019, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > Hi, > > The Nacked-by tag can be used to manually hold a patch for further review, > or by automatic CI because of failing test. > > We often miss travis-ci and shippable failures. These CI provide a easy > way to send email on failure, we can integrate the Nacked-by use there. > > We can easily have patchew script send a Nacked-by tag. > > If there is a consensus about using this tag, the following patch can be > added to Peter's management scripts: > https://git.linaro.org/people/pmaydell/misc-scripts.git/ > I always assumed that pull requests by sub-maintainers should contain "ready for merging" code (justified, reviewed, tested, ...). Why would ever a sub-maintainer send something that doesn't comply to these conditions? I think, in general, this tag would do more harm than good, allowing frivolous blocking of patches, and fixing a process that already works, without any need. Not acknowledged by me. Sincerely, Aleksandar > If we move to another workflow, having this uniform tag can help future > merging scripts to avoid patch on hold to get automatically merged. > > -- >8 -- > Subject: make-pullreq: Do not automatically merge NAcked commits > > The 'Nacked-by' tag is a polite way of holding a patch for > further review. Reviewers might share their disapproval with > it (see [1]). > > CI scripts might NAck a patch if it breaks testing. > QEMU already thought about using this tag for CI by the past > (see [2]). > > The patchwork tool already collects this tag (see [3]). > > Also, there was a discussion at the last Open Source Summit > about standardizing it ([4]). > > Maintainers might miss a such Nacked-by tag. Help them by > providing a last resort check before merging pull requests. > > [1] https://www.x.org/wiki/Development/Documentation/SubmittingP > atches/#index1h1 > [2] https://lists.gnu.org/archive/html/qemu-devel/2013-01/msg00196.html > [3] http://git.ozlabs.org/?p=patchwork;a=blobdiff;f=apps/patchwo > rk/models.py;h=fa213dc03e;hp=8871df0259e;hb=487b53576f;hpb=a59ebf107d84b > [4] https://lore.kernel.org/workflows/CACT4Y+bxPxQ64HEO2uGRkbk9v > JSeg64y10Lak4c2K54J7GyFFA@mail.gmail.com/ > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > make-pullreq | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/make-pullreq b/make-pullreq > index 61c0f1d..fff0b2d 100755 > --- a/make-pullreq > +++ b/make-pullreq > @@ -108,6 +108,17 @@ if [ "$bad" = "yes" ]; then > exit 1 > fi > > +# Check no commit contains a nacked-by tag > +for rev in $(git rev-list master..HEAD); do > + if git log ${rev}^! | grep -iq "Nacked-by:"; then > + echo "Error: commit ${rev} nacked" > + bad=yes > + fi > +done > +if [ "$bad" = "yes" ]; then > + exit 1 > +fi > + > # Check whether any authors needs to be corrected after SPF rewrites > if git shortlog --author=qemu-devel@nongnu.org master..HEAD | grep .; > then > echo "ERROR: pull request includes commits attributed to list" > -- > 2.21.0 > > >
On 09/12/19 10:28, Aleksandar Markovic wrote: > If there is a consensus about using this tag, the following patch > can be added to Peter's management scripts: > https://git.linaro.org/people/pmaydell/misc-scripts.git/ > <https://git.linaro.org/people/pmaydell/misc-scripts.git/> > > I always assumed that pull requests by sub-maintainers should contain > "ready for merging" code (justified, reviewed, tested, ...). Why would > ever a sub-maintainer send something that doesn't comply to these > conditions? Because things can and do go wrong, perhaps someone was on vacation while the original patch was posted, perhaps somebody is giving a negative review outside his maintenance area, perhaps there would be conflicts with a tree-wide series being discussed elsewhere... It's rare and I don't think it would be misused, but I think it's a good idea to have a machine-readable way to block patches. However, I'm not sure why the commits would contain a tag. Instead, we could use the patchew REST API (https://patchew.org/api/v1/projects/1/series/MESSAGE-ID/) and search for nacked-by tags in there. Paolo > I think, in general, this tag would do more harm than good, allowing > frivolous blocking of patches, and fixing a process that already works, > without any need.
On Monday, December 9, 2019, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 09/12/19 10:28, Aleksandar Markovic wrote: > > If there is a consensus about using this tag, the following patch > > can be added to Peter's management scripts: > > https://git.linaro.org/people/pmaydell/misc-scripts.git/ > > <https://git.linaro.org/people/pmaydell/misc-scripts.git/> > > > > I always assumed that pull requests by sub-maintainers should contain > > "ready for merging" code (justified, reviewed, tested, ...). Why would > > ever a sub-maintainer send something that doesn't comply to these > > conditions? > > Because things can and do go wrong, perhaps someone was on vacation > while the original patch was posted, perhaps somebody is giving a > negative review outside his maintenance area, perhaps there would be > conflicts with a tree-wide series being discussed elsewhere... It's > rare and I don't think it would be misused, but I think it's a good idea > to have a machine-readable way to block patches. > > I'm afraid this would be opening a Pandora's box. For such rare cases, a message from a person: "Please hold on this patch until I am back from vacation.", "Please wait until I merge my series acting on the same files", or similar, would perfectly do the job, as it did in the past. We are fixing something that is not broken. > However, I'm not sure why the commits would contain a tag. Instead, we > could use the patchew REST API > (https://patchew.org/api/v1/projects/1/series/MESSAGE-ID/) and search > for nacked-by tags in there. > > Paolo > > > I think, in general, this tag would do more harm than good, allowing > > frivolous blocking of patches, and fixing a process that already works, > > without any need. > >
On 09/12/19 11:12, Aleksandar Markovic wrote: > I'm afraid this would be opening a Pandora's box. For such rare cases, a > message from a person: "Please hold on this patch until I am back from > vacation.", "Please wait until I merge my series acting on the same > files", or similar, would perfectly do the job, as it did in the past. The point is when you don't see the patch until it gets in a pull request. It's not particularly common so I'm okay with no change (and I agree that Philippe's patch, as is, doesn't solve *this* particular case) but I also don't see this as causing problems in the future. Paolo
© 2016 - 2024 Red Hat, Inc.