[PATCH 0/3] get_maintainer: add patch-only keyword matching

[PATCH 0/3] get_maintainer: add patch-only keyword matching

[Justin Stitt]

This series aims to add "D:" which behaves exactly the same as "K:" but
works only on patch files.

The goal of this is to reduce noise when folks use get_maintainer on
tree files as opposed to patches. This use case should be steered away
from [1] but "D:" should help maintainers reduce noise in their inboxes
regardless, especially when matching omnipresent keywords like [2]. In
the event of [2] Kees would be to/cc'd from folks running get_maintainer
on _any_ file containing "__counted_by". The number of these files is
rising and I fear for his inbox as his goal, as I understand it, is to
simply monitor the introduction of new __counted_by annotations to
ensure accurate semantics.

See [3/3] for an illustrative example.

This series also includes a formatting pass over get_maintainer because
I personally found it difficult to parse with the human eye.

[1]: https://lore.kernel.org/all/20230726151515.1650519-1-kuba@kernel.org/
[2]: https://lore.kernel.org/all/20230925172037.work.853-kees@kernel.org/

Signed-off-by: Justin Stitt <justinstitt@google.com>
---
Justin Stitt (3):
      MAINTAINERS: add documentation for D:
      get_maintainer: run perltidy
      get_maintainer: add patch-only pattern matching type

 MAINTAINERS               |    3 +
 scripts/get_maintainer.pl | 3334 +++++++++++++++++++++++----------------------
 2 files changed, 1718 insertions(+), 1619 deletions(-)
---
base-commit: 6465e260f48790807eef06b583b38ca9789b6072
change-id: 20230926-get_maintainer_add_d-07424a814e72

Best regards,
--
Justin Stitt <justinstitt@google.com>

[PATCH 1/3] MAINTAINERS: add documentation for D:

[Justin Stitt]

Document what "D:" does.

This is more or less the same as what "K:" does but only works for patch
files.

See [3/3] for more info and an illustrative example.
---
 MAINTAINERS | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index b19995690904..de68d2c0cf29 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -59,6 +59,9 @@ Descriptions of section entries and preferred order
 	      matches patches or files that contain one or more of the words
 	      printk, pr_info or pr_err
 	   One regex pattern per line.  Multiple K: lines acceptable.
+  D: *Content regex* (perl extended) pattern match patches only.
+     Usage same as K:.
+
 
 Maintainers List
 ----------------

-- 
2.42.0.582.g8ccd20d70d-goog

Re: [PATCH 1/3] MAINTAINERS: add documentation for D:

[Joe Perches]

On Wed, 2023-09-27 at 03:19 +0000, Justin Stitt wrote:
> Document what "D:" does.
> 
> This is more or less the same as what "K:" does but only works for patch
> files.

Nack.  I'd rather just add a !$file test to K: patterns.

Re: [PATCH 1/3] MAINTAINERS: add documentation for D:

[Justin Stitt]

On Wed, Sep 27, 2023 at 12:27 PM Joe Perches <joe@perches.com> wrote:
>
> On Wed, 2023-09-27 at 03:19 +0000, Justin Stitt wrote:
> > Document what "D:" does.
> >
> > This is more or less the same as what "K:" does but only works for patch
> > files.
>
> Nack.  I'd rather just add a !$file test to K: patterns.

Are there no legitimate use cases for K:'s current behavior to warrant
keeping it around?

>

Re: [PATCH 1/3] MAINTAINERS: add documentation for D:

[Kees Cook]

On Wed, Sep 27, 2023 at 03:19:14AM +0000, Justin Stitt wrote:
> Document what "D:" does.
> 
> This is more or less the same as what "K:" does but only works for patch
> files.
> 
> See [3/3] for more info and an illustrative example.
> ---
>  MAINTAINERS | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b19995690904..de68d2c0cf29 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -59,6 +59,9 @@ Descriptions of section entries and preferred order
>  	      matches patches or files that contain one or more of the words
>  	      printk, pr_info or pr_err
>  	   One regex pattern per line.  Multiple K: lines acceptable.
> +  D: *Content regex* (perl extended) pattern match patches only.
> +     Usage same as K:.
> +

The "emphasis" tags here are used when rendering:
https://docs.kernel.org/process/maintainers.html

In this case, I assume "D" is inspired by "Diff", so perhaps reword this
to get a proper emphasis hint, and add additional context:

  D: *Diff content regex* (perl extended) pattern match that applies
     only to patches and not entire files (e.g. when using the
     get_maintainers.pl script).


-- 
Kees Cook

[PATCH 3/3] get_maintainer: add patch-only pattern matching type

[Justin Stitt]

Add the "D:" type which behaves the same as "K:" but will only match
content present in a patch file.

To illustrate:

Imagine this entry in MAINTAINERS:

NEW REPUBLIC
M: Han Solo <hansolo@rebelalliance.co>
W: https://www.jointheresistance.org
D: \bstrncpy\b

Our maintainer, Han, will only be added to the recipients if a patch
file is passed to get_maintainer (like what b4 does):
$ ./scripts/get_maintainer.pl 0004-some-change.patch

If the above patch has a `strncpy` present in the subject, commit log or
diff then Han will be to/cc'd.

However, in the event of a file from the tree given like:
$ ./scripts/get_maintainer.pl ./lib/string.c

Han will not be noisily to/cc'd (like a K: type would in this
circumstance)

Note that folks really shouldn't be using get_maintainer on tree files
anyways [1].

[1]: https://lore.kernel.org/all/20230726151515.1650519-1-kuba@kernel.org/
---
 scripts/get_maintainer.pl | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index e679eac96793..f290bf0948f1 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -309,6 +309,7 @@ if ( $tree && !top_of_kernel_tree($lk_path) ) {
 
 my @typevalue = ();
 my %keyword_hash;
+my %patch_keyword_hash;
 my @mfiles         = ();
 my @self_test_info = ();
 
@@ -339,6 +340,9 @@ sub read_maintainer_file {
             elsif ( $type eq "K" ) {
                 $keyword_hash{@typevalue} = $value;
             }
+            elsif ( $type eq "D" ) {
+                $patch_keyword_hash{@typevalue} = $value;
+            }
             push( @typevalue, "$type:$value" );
         }
         elsif ( !( /^\s*$/ || /^\s*\#/ ) ) {
@@ -591,6 +595,11 @@ foreach my $file (@ARGV) {
                         push( @keyword_tvi, $line );
                     }
                 }
+                foreach my $line ( keys %patch_keyword_hash ) {
+                    if ($patch_line =~ m/${patch_prefix}$patch_keyword_hash{$line}/x ) {
+                        push( @keyword_tvi, $line );
+                    }
+                }
             }
         }
         close($patch);

-- 
2.42.0.582.g8ccd20d70d-goog

Re: [PATCH 3/3] get_maintainer: add patch-only pattern matching type

[Greg KH]

On Wed, Sep 27, 2023 at 03:19:16AM +0000, Justin Stitt wrote:
> Note that folks really shouldn't be using get_maintainer on tree files
> anyways [1].

That's not true, Linus and I use it on a daily basis this way, it's part
of our normal workflow, AND the workflow of the kernel security team.

So please don't take that valid use-case away from us.

thanks,

greg k-h

Re: [PATCH 3/3] get_maintainer: add patch-only pattern matching type

[Justin Stitt]

On Wed, Sep 27, 2023 at 3:14 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Sep 27, 2023 at 03:19:16AM +0000, Justin Stitt wrote:
> > Note that folks really shouldn't be using get_maintainer on tree files
> > anyways [1].
>
> That's not true, Linus and I use it on a daily basis this way, it's part
> of our normal workflow, AND the workflow of the kernel security team.
>
> So please don't take that valid use-case away from us.

Fair. I'm on the side of keeping the "K:'' behavior the way it is and
that's why I'm proposing adding "D:" to provide a more granular
content matching type operating strictly on patches. It's purely
opt-in.

The patch I linked mentioned steering folks away from using
tree files but not necessarily removing the behavior.

>
> thanks,
>
> greg k-h

Thanks
Justin

Re: [PATCH 3/3] get_maintainer: add patch-only pattern matching type

[Greg KH]

On Wed, Sep 27, 2023 at 03:46:30PM +0900, Justin Stitt wrote:
> On Wed, Sep 27, 2023 at 3:14 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Sep 27, 2023 at 03:19:16AM +0000, Justin Stitt wrote:
> > > Note that folks really shouldn't be using get_maintainer on tree files
> > > anyways [1].
> >
> > That's not true, Linus and I use it on a daily basis this way, it's part
> > of our normal workflow, AND the workflow of the kernel security team.
> >
> > So please don't take that valid use-case away from us.
> 
> Fair. I'm on the side of keeping the "K:'' behavior the way it is and
> that's why I'm proposing adding "D:" to provide a more granular
> content matching type operating strictly on patches. It's purely
> opt-in.
> 
> The patch I linked mentioned steering folks away from using
> tree files but not necessarily removing the behavior.

Please don't steer folks away from it, it is a valid use case of the
tool, and I would argue, one of the most important ones given how often
I use it that way.

Hence my objection to this verbage in the changelog, it's not correct.

thanks,

greg k-h

Re: [PATCH 3/3] get_maintainer: add patch-only pattern matching type

[Kees Cook]

On Wed, Sep 27, 2023 at 03:19:16AM +0000, Justin Stitt wrote:
> Add the "D:" type which behaves the same as "K:" but will only match
> content present in a patch file.
> 
> To illustrate:
> 
> Imagine this entry in MAINTAINERS:
> 
> NEW REPUBLIC
> M: Han Solo <hansolo@rebelalliance.co>
> W: https://www.jointheresistance.org
> D: \bstrncpy\b
> 
> Our maintainer, Han, will only be added to the recipients if a patch
> file is passed to get_maintainer (like what b4 does):
> $ ./scripts/get_maintainer.pl 0004-some-change.patch
> 
> If the above patch has a `strncpy` present in the subject, commit log or
> diff then Han will be to/cc'd.
> 
> However, in the event of a file from the tree given like:
> $ ./scripts/get_maintainer.pl ./lib/string.c
> 
> Han will not be noisily to/cc'd (like a K: type would in this
> circumstance)
> 
> Note that folks really shouldn't be using get_maintainer on tree files
> anyways [1].
> 
> [1]: https://lore.kernel.org/all/20230726151515.1650519-1-kuba@kernel.org/

As Greg suggested, please drop the above paragraph and link. Then this
looks good to me.

I would immediately want to send this patch too, so please feel free to
add this to your series (and I bet many other hints on "git grep 'K:.\\b'"
would want to switch from K: to D: too):

diff --git a/MAINTAINERS b/MAINTAINERS
index 5f18c6ba3c3c..830e10866acf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5057,7 +5057,7 @@ F:	Documentation/kbuild/llvm.rst
 F:	include/linux/compiler-clang.h
 F:	scripts/Makefile.clang
 F:	scripts/clang-tools/
-K:	\b(?i:clang|llvm)\b
+D:	\b(?i:clang|llvm)\b
 
 CLK API
 M:	Russell King <linux@armlinux.org.uk>
@@ -8199,7 +8199,7 @@ F:	lib/strcat_kunit.c
 F:	lib/strscpy_kunit.c
 F:	lib/test_fortify/*
 F:	scripts/test_fortify.sh
-K:	\b__NO_FORTIFY\b
+D:	\b__NO_FORTIFY\b
 
 FPGA DFL DRIVERS
 M:	Wu Hao <hao.wu@intel.com>
@@ -11457,9 +11457,9 @@ F:	include/linux/overflow.h
 F:	include/linux/randomize_kstack.h
 F:	kernel/configs/hardening.config
 F:	mm/usercopy.c
-K:	\b(add|choose)_random_kstack_offset\b
-K:	\b__check_(object_size|heap_object)\b
-K:	\b__counted_by\b
+D:	\b(add|choose)_random_kstack_offset\b
+D:	\b__check_(object_size|heap_object)\b
+D:	\b__counted_by\b
 
 KERNEL JANITORS
 L:	kernel-janitors@vger.kernel.org
@@ -17354,7 +17354,7 @@ F:	drivers/acpi/apei/erst.c
 F:	drivers/firmware/efi/efi-pstore.c
 F:	fs/pstore/
 F:	include/linux/pstore*
-K:	\b(pstore|ramoops)
+D:	\b(pstore|ramoops)
 
 PTP HARDWARE CLOCK SUPPORT
 M:	Richard Cochran <richardcochran@gmail.com>
@@ -19302,8 +19302,8 @@ F:	include/uapi/linux/seccomp.h
 F:	kernel/seccomp.c
 F:	tools/testing/selftests/kselftest_harness.h
 F:	tools/testing/selftests/seccomp/*
-K:	\bsecure_computing
-K:	\bTIF_SECCOMP\b
+D:	\bsecure_computing
+D:	\bTIF_SECCOMP\b
 
 SECURE DIGITAL HOST CONTROLLER INTERFACE (SDHCI) Broadcom BRCMSTB DRIVER
 M:	Kamal Dasu <kamal.dasu@broadcom.com>

-- 
Kees Cook

Re: [PATCH 3/3] get_maintainer: add patch-only pattern matching type

[Joe Perches]

On Wed, 2023-09-27 at 09:15 -0700, Kees Cook wrote:
> On Wed, Sep 27, 2023 at 03:19:16AM +0000, Justin Stitt wrote:
> > Add the "D:" type which behaves the same as "K:" but will only match
> > content present in a patch file.
> > 
> > To illustrate:
> > 
> > Imagine this entry in MAINTAINERS:
> > 
> > NEW REPUBLIC
> > M: Han Solo <hansolo@rebelalliance.co>
> > W: https://www.jointheresistance.org
> > D: \bstrncpy\b
> > 
> > Our maintainer, Han, will only be added to the recipients if a patch
> > file is passed to get_maintainer (like what b4 does):
> > $ ./scripts/get_maintainer.pl 0004-some-change.patch
> > 
> > If the above patch has a `strncpy` present in the subject, commit log or
> > diff then Han will be to/cc'd.
> > 
> > However, in the event of a file from the tree given like:
> > $ ./scripts/get_maintainer.pl ./lib/string.c
> > 
> > Han will not be noisily to/cc'd (like a K: type would in this
> > circumstance)
> > 
> > Note that folks really shouldn't be using get_maintainer on tree files
> > anyways [1].
> > 
> > [1]: https://lore.kernel.org/all/20230726151515.1650519-1-kuba@kernel.org/
> 
> As Greg suggested, please drop the above paragraph and link. Then this
> looks good to me.
> 
> I would immediately want to send this patch too, so please feel free to
> add this to your series (and I bet many other hints on "git grep 'K:.\\b'"
> would want to switch from K: to D: too):
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
[]
> @@ -5057,7 +5057,7 @@ F:	Documentation/kbuild/llvm.rst
>  F:	include/linux/compiler-clang.h
>  F:	scripts/Makefile.clang
>  F:	scripts/clang-tools/
> -K:	\b(?i:clang|llvm)\b
> +D:	\b(?i:clang|llvm)\b

etc...

My assumption is that the K: --file use is just unnecessary
and it'd be better to only use the K: lookup on patches.

(and I've somehow stuffed up the receiving side of my
 email configuration so please ignore any emails to me
 that bounce for a while)

Re: [PATCH 3/3] get_maintainer: add patch-only pattern matching type

[Joe Perches]

On Wed, 2023-09-27 at 03:19 +0000, Justin Stitt wrote:
> Add the "D:" type which behaves the same as "K:" but will only match
> content present in a patch file.

Likely it'd be less aggravating just to document
that K: is only for patches and add a !$file test.

Re: [PATCH 2/3] get_maintainer: run perltidy

[Joe Perches]

On Wed, 2023-09-27 at 03:19 +0000, Justin Stitt wrote:
> I'm a first time contributor to get_maintainer.pl and the formatting is
> suspicious. I am not sure if there is a particular reason it is the way
> it is but I let my editor format it and submitted the diff here in this
> patch.

Capital NACK.  Completely unnecessary and adds no value.

Re: [PATCH 0/3] get_maintainer: add patch-only keyword matching

[Nick Desaulniers]

On Tue, Sep 26, 2023 at 8:19 PM Justin Stitt <justinstitt@google.com> wrote:
>
> This series aims to add "D:" which behaves exactly the same as "K:" but
> works only on patch files.
>
> The goal of this is to reduce noise when folks use get_maintainer on
> tree files as opposed to patches. This use case should be steered away
> from [1] but "D:" should help maintainers reduce noise in their inboxes
> regardless, especially when matching omnipresent keywords like [2]. In
> the event of [2] Kees would be to/cc'd from folks running get_maintainer
> on _any_ file containing "__counted_by". The number of these files is
> rising and I fear for his inbox as his goal, as I understand it, is to
> simply monitor the introduction of new __counted_by annotations to
> ensure accurate semantics.

Something like this (whether this series or a different approach)
would be helpful to me as well; we use K: to get cc'ed on patches
mentioning clang or llvm, but our ML also then ends up getting cc'ed
on every follow up patch to most files.

This is causing excessive posts on our ML. As a result, it's a
struggle to get folks to cc themselves to the ML, which puts the code
review burden on fewer people.

Whether it's a new D: or refinement to the behavior of K:, I applaud
the effort.  Hopefully we can find an approach that works for
everyone.

And may God have mercy on your soul for having to touch that much perl. :-P

>
> See [3/3] for an illustrative example.
>
> This series also includes a formatting pass over get_maintainer because
> I personally found it difficult to parse with the human eye.
>
> [1]: https://lore.kernel.org/all/20230726151515.1650519-1-kuba@kernel.org/
> [2]: https://lore.kernel.org/all/20230925172037.work.853-kees@kernel.org/
>
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
> Justin Stitt (3):
>       MAINTAINERS: add documentation for D:
>       get_maintainer: run perltidy
>       get_maintainer: add patch-only pattern matching type
>
>  MAINTAINERS               |    3 +
>  scripts/get_maintainer.pl | 3334 +++++++++++++++++++++++----------------------
>  2 files changed, 1718 insertions(+), 1619 deletions(-)
> ---
> base-commit: 6465e260f48790807eef06b583b38ca9789b6072
> change-id: 20230926-get_maintainer_add_d-07424a814e72
>
> Best regards,
> --
> Justin Stitt <justinstitt@google.com>
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 0/3] get_maintainer: add patch-only keyword matching

[Kees Cook]

On Wed, Sep 27, 2023 at 08:24:58AM -0700, Nick Desaulniers wrote:
> On Tue, Sep 26, 2023 at 8:19 PM Justin Stitt <justinstitt@google.com> wrote:
> >
> > This series aims to add "D:" which behaves exactly the same as "K:" but
> > works only on patch files.
> >
> > The goal of this is to reduce noise when folks use get_maintainer on
> > tree files as opposed to patches. This use case should be steered away
> > from [1] but "D:" should help maintainers reduce noise in their inboxes
> > regardless, especially when matching omnipresent keywords like [2]. In
> > the event of [2] Kees would be to/cc'd from folks running get_maintainer
> > on _any_ file containing "__counted_by". The number of these files is
> > rising and I fear for his inbox as his goal, as I understand it, is to
> > simply monitor the introduction of new __counted_by annotations to
> > ensure accurate semantics.
> 
> Something like this (whether this series or a different approach)
> would be helpful to me as well; we use K: to get cc'ed on patches
> mentioning clang or llvm, but our ML also then ends up getting cc'ed
> on every follow up patch to most files.
> 
> This is causing excessive posts on our ML. As a result, it's a
> struggle to get folks to cc themselves to the ML, which puts the code
> review burden on fewer people.
> 
> Whether it's a new D: or refinement to the behavior of K:, I applaud
> the effort.  Hopefully we can find an approach that works for
> everyone.

Yes, please! I would use this immediately -- there are a bunch of places
where pstore, strings, hardening, etc all want review if certain
functions or structures are changed in a patch, but we're not
maintainers of the files they appear in.

> > Justin Stitt (3):
> >       MAINTAINERS: add documentation for D:
> >       get_maintainer: add patch-only pattern matching type

Can we squash these two changes together, and then likely add some
patches for moving things out of K: ?

-- 
Kees Cook