[RFC] Use of the Nacked-by tag by CI scripts

[RFC] Use of the Nacked-by tag by CI scripts

[Philippe Mathieu-Daudé]

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

Re: [RFC] Use of the Nacked-by tag by CI scripts

[Aleksandar Markovic]

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

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
>
>
>

[-- Attachment #2: Type: text/html, Size: 4614 bytes --]

Re: [RFC] Use of the Nacked-by tag by CI scripts

[Paolo Bonzini]

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.

Re: [RFC] Use of the Nacked-by tag by CI scripts

[Aleksandar Markovic]

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

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.
>
>

[-- Attachment #2: Type: text/html, Size: 2594 bytes --]

Re: [RFC] Use of the Nacked-by tag by CI scripts

[Paolo Bonzini]

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