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

[Aleksandar Markovic]

On Monday, December 9, 2019, Philippe Mathieu-Daudé <address@hidden> 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/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é <address@hidden>
> ---
>  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=address@hidden master..HEAD | grep .; then
>      echo "ERROR: pull request includes commits attributed to list"
> -- 
> 2.21.0