[PATCH v2] scripts: get_maintainer: steer people away from using file paths

[PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Jakub Kicinski]

We repeatedly see netcomers misuse get_maintainer by running it on
the file paths rather than the patchfile. This leads to authors
of changes (quoted commits and commits under Fixes) not getting
CCed. These are usually the best reviewers!

The file option should really not be used by inexperienced developers,
unless they are just trying to find a maintainer to manually contact.

Print a warning when someone tries to use -f and remove
the "auto-guessing" of file paths.

This script may break people's "scripts on top of get_maintainer"
if they are using -f... but that's the point.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
note addressed:
 - put more info into the warning. I think it's more than fine.
v2:
 - fix the subject (Greg)
 - s/noob/inexperienced|newcomer/ (Joe)
 - put the message on a single line (Joe)
 - s/will/may/ (Joe)
 - s/filepatch/patchfile/
 - add more reasons to help
v1: https://lore.kernel.org/all/20230725155926.2775416-1-kuba@kernel.org/

CC: joe@perches.com
Cc: geert@linux-m68k.org
Cc: gregkh@linuxfoundation.org
Cc: netdev@vger.kernel.org
Cc: workflows@vger.kernel.org
Cc: mario.limonciello@amd.com
---
 scripts/get_maintainer.pl | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index ab123b498fd9..4714056ca7f1 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -51,6 +51,7 @@ my $output_roles = 0;
 my $output_rolestats = 1;
 my $output_section_maxlen = 50;
 my $scm = 0;
+my $silence_file_warning = 0;
 my $tree = 1;
 my $web = 0;
 my $subsystem = 0;
@@ -267,6 +268,7 @@ if (!GetOptions(
 		'subsystem!' => \$subsystem,
 		'status!' => \$status,
 		'scm!' => \$scm,
+		'silence-file-warning!' => \$silence_file_warning,
 		'tree!' => \$tree,
 		'web!' => \$web,
 		'letters=s' => \$letters,
@@ -544,7 +546,11 @@ foreach my $file (@ARGV) {
     if ($from_filename && (vcs_exists() && !vcs_file_exists($file))) {
 	warn "$P: file '$file' not found in version control $!\n";
     }
-    if ($from_filename || ($file ne "&STDIN" && vcs_file_exists($file))) {
+    if ($from_filename) {
+	if (!$silence_file_warning) {
+	    warn "$P: WARNING: Prefer running the script on patches as generated by git format-patch. Selecting paths is known to miss recipients!\n";
+	}
+
 	$file =~ s/^\Q${cur_path}\E//;	#strip any absolute path
 	$file =~ s/^\Q${lk_path}\E//;	#or the path to the lk tree
 	push(@files, $file);
@@ -1081,6 +1087,7 @@ version: $V
   --mailmap => use .mailmap file (default: $email_use_mailmap)
   --no-tree => run without a kernel tree
   --self-test => show potential issues with MAINTAINERS file content
+  --silence-file-warning => silence the warning about -f being used (see Notes)
   --version => show version
   --help => show this help information
 
@@ -1089,6 +1096,11 @@ version: $V
    --pattern-depth=0 --remove-duplicates --rolestats]
 
 Notes:
+  Using "-f file" is generally discouraged, running the script on a patchfile
+      (as generated by git format-patch) is usually the right thing to do.
+      It's easy to miss a file changed by a commit and the script
+      may extract additional information from the commit message
+      (keywords, Fixes tags etc.)
   Using "-f directory" may give unexpected results:
       Used with "--git", git signators for _all_ files in and below
           directory are examined as git recurses directories.
-- 
2.41.0

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Mario Limonciello]

On 7/26/23 10:15, Jakub Kicinski wrote:
> We repeatedly see netcomers misuse get_maintainer by running it on
Presumably you meant "newcomers"?  Or is this just a phrase for 
newcomers contributing to netdev? :)

> the file paths rather than the patchfile. This leads to authors
> of changes (quoted commits and commits under Fixes) not getting
> CCed. These are usually the best reviewers!
> 
> The file option should really not be used by inexperienced developers,
> unless they are just trying to find a maintainer to manually contact.
> 
> Print a warning when someone tries to use -f and remove
> the "auto-guessing" of file paths.
> 
> This script may break people's "scripts on top of get_maintainer"
> if they are using -f... but that's the point.
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> note addressed:
>   - put more info into the warning. I think it's more than fine.
> v2:
>   - fix the subject (Greg)
>   - s/noob/inexperienced|newcomer/ (Joe)
>   - put the message on a single line (Joe)
>   - s/will/may/ (Joe)
>   - s/filepatch/patchfile/
>   - add more reasons to help
> v1: https://lore.kernel.org/all/20230725155926.2775416-1-kuba@kernel.org/
> 
> CC: joe@perches.com
> Cc: geert@linux-m68k.org
> Cc: gregkh@linuxfoundation.org
> Cc: netdev@vger.kernel.org
> Cc: workflows@vger.kernel.org
> Cc: mario.limonciello@amd.com
> ---
>   scripts/get_maintainer.pl | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
> index ab123b498fd9..4714056ca7f1 100755
> --- a/scripts/get_maintainer.pl
> +++ b/scripts/get_maintainer.pl
> @@ -51,6 +51,7 @@ my $output_roles = 0;
>   my $output_rolestats = 1;
>   my $output_section_maxlen = 50;
>   my $scm = 0;
> +my $silence_file_warning = 0;
>   my $tree = 1;
>   my $web = 0;
>   my $subsystem = 0;
> @@ -267,6 +268,7 @@ if (!GetOptions(
>   		'subsystem!' => \$subsystem,
>   		'status!' => \$status,
>   		'scm!' => \$scm,
> +		'silence-file-warning!' => \$silence_file_warning,
>   		'tree!' => \$tree,
>   		'web!' => \$web,
>   		'letters=s' => \$letters,
> @@ -544,7 +546,11 @@ foreach my $file (@ARGV) {
>       if ($from_filename && (vcs_exists() && !vcs_file_exists($file))) {
>   	warn "$P: file '$file' not found in version control $!\n";
>       }
> -    if ($from_filename || ($file ne "&STDIN" && vcs_file_exists($file))) {
> +    if ($from_filename) {
> +	if (!$silence_file_warning) {
> +	    warn "$P: WARNING: Prefer running the script on patches as generated by git format-patch. Selecting paths is known to miss recipients!\n";
> +	}
> +
>   	$file =~ s/^\Q${cur_path}\E//;	#strip any absolute path
>   	$file =~ s/^\Q${lk_path}\E//;	#or the path to the lk tree
>   	push(@files, $file);
> @@ -1081,6 +1087,7 @@ version: $V
>     --mailmap => use .mailmap file (default: $email_use_mailmap)
>     --no-tree => run without a kernel tree
>     --self-test => show potential issues with MAINTAINERS file content
> +  --silence-file-warning => silence the warning about -f being used (see Notes)
>     --version => show version
>     --help => show this help information
>   
> @@ -1089,6 +1096,11 @@ version: $V
>      --pattern-depth=0 --remove-duplicates --rolestats]
>   
>   Notes:
> +  Using "-f file" is generally discouraged, running the script on a patchfile
> +      (as generated by git format-patch) is usually the right thing to do.
> +      It's easy to miss a file changed by a commit and the script
> +      may extract additional information from the commit message
> +      (keywords, Fixes tags etc.)
>     Using "-f directory" may give unexpected results:
>         Used with "--git", git signators for _all_ files in and below
>             directory are examined as git recurses directories.

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Joe Perches]

On Wed, 2023-07-26 at 08:15 -0700, Jakub Kicinski wrote:
> We repeatedly see netcomers misuse get_maintainer by running it on

newcomers

> Print a warning when someone tries to use -f and remove
> the "auto-guessing" of file paths.

Nack on that bit.
My recollection is it's Linus' preferred mechanism.

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Jakub Kicinski]

On Wed, 26 Jul 2023 08:43:30 -0700 Joe Perches wrote:
> > Print a warning when someone tries to use -f and remove
> > the "auto-guessing" of file paths.  
> 
> Nack on that bit.
> My recollection is it's Linus' preferred mechanism.

Let Linus speak for himself, hopefully he's okay with throwing
in the -f.

BTW get_maintainer is central to our ML-based development process.
I think the patches to get_maintainer and checkpatch should flow
to workflows@.

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Linus Torvalds]

On Wed, 26 Jul 2023 at 09:23, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 26 Jul 2023 08:43:30 -0700 Joe Perches wrote:
> > > Print a warning when someone tries to use -f and remove
> > > the "auto-guessing" of file paths.
> >
> > Nack on that bit.
> > My recollection is it's Linus' preferred mechanism.
>
> Let Linus speak for himself, hopefully he's okay with throwing
> in the -f.

It's not the '-f' that would be the problem - that's how the script
used to work long ago, and I still occasionally end up adding the -f
by habit.

So removing the auto-guessing of file paths wouldn't be a problem.

But the annoying warning is wrong.

I use get_maintainers all the time, and I *only* use it for file
paths. If I know the commit, I get the list of people from the commit
itself, so why should I *ever* use that script if I have a patch?

So the whole "use of get_maintainers is only for patches, and we
should warn about file paths" is insane.

No. If I get that patch, I will remove the warning. The *only* reason
for me to ever use that script is for the file path lookup.

             Linus

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Joe Perches]

On Wed, 2023-07-26 at 09:45 -0700, Linus Torvalds wrote:

> So the whole "use of get_maintainers is only for patches, and we
> should warn about file paths" is insane.

Agree.

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Jakub Kicinski]

On Wed, 26 Jul 2023 09:45:25 -0700 Linus Torvalds wrote:
> On Wed, 26 Jul 2023 at 09:23, Jakub Kicinski <kuba@kernel.org> wrote:
> > > Nack on that bit.
> > > My recollection is it's Linus' preferred mechanism.  
> >
> > Let Linus speak for himself, hopefully he's okay with throwing
> > in the -f.  
> 
> It's not the '-f' that would be the problem - that's how the script
> used to work long ago, and I still occasionally end up adding the -f
> by habit.
> 
> So removing the auto-guessing of file paths wouldn't be a problem.
> 
> But the annoying warning is wrong.
> 
> I use get_maintainers all the time, and I *only* use it for file
> paths. If I know the commit, I get the list of people from the commit
> itself, so why should I *ever* use that script if I have a patch?

You are special, you presumably use it to find who to report
regressions to, and who to pull into conversations.

This tool is primarily used by _developers_ to find _maintainers_.
I mean *thousands* of developers use it every release to send their
patches.

The tool needs to see the commit message to fish out Fixes tags.

> So the whole "use of get_maintainers is only for patches, and we
> should warn about file paths" is insane.

Hrmpf, hrmpf.

> No. If I get that patch, I will remove the warning. The *only* reason
> for me to ever use that script is for the file path lookup.
> 
>              Linus

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Linus Torvalds]

On Wed, 26 Jul 2023 at 11:20, Jakub Kicinski <kuba@kernel.org> wrote:
>
> You are special,

So my mother tells me.

> you presumably use it to find who to report
> regressions to, and who to pull into conversations.

Yes. So what happens is that I get cc'd on bug reports for various
issues, and particularly for oops reports I basically have a function
name to grep for (maybe a pathname if it went through the full
decoding).

I'm NOT interested in having to either remember all people off-hand,
or going through the MAINTAINERS file by hand.

> This tool is primarily used by _developers_ to find _maintainers_.

Well, maybe.

But even if that is true, I don't see why you hate the pathname thing
even for that case. I bet developers use it for that exact same
reason, ie they are modifying a file, and they go "I want to know who
the maintainer for this file is".

I do not understand why you think a patch is somehow magically more
important or relevant than a filename.

               Linus

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Limonciello, Mario]

On 7/26/2023 1:29 PM, Linus Torvalds wrote:
> On Wed, 26 Jul 2023 at 11:20, Jakub Kicinski <kuba@kernel.org> wrote:
>>
>> You are special,
> 
> So my mother tells me.
> 
>> you presumably use it to find who to report
>> regressions to, and who to pull into conversations.
> 
> Yes. So what happens is that I get cc'd on bug reports for various
> issues, and particularly for oops reports I basically have a function
> name to grep for (maybe a pathname if it went through the full
> decoding).
> 
> I'm NOT interested in having to either remember all people off-hand,
> or going through the MAINTAINERS file by hand.
> 
>> This tool is primarily used by _developers_ to find _maintainers_.
> 
> Well, maybe.
> 
> But even if that is true, I don't see why you hate the pathname thing
> even for that case. I bet developers use it for that exact same
> reason, ie they are modifying a file, and they go "I want to know who
> the maintainer for this file is".
> 
> I do not understand why you think a patch is somehow magically more
> important or relevant than a filename.
> 
>                 Linus
If the goal is to get people who are involved with a file, how about 
some variation of:

git log --pretty=format:%ae $PATH | sort | uniq

Granted this is going to be overkill, for example I can see on 
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c that it would email 198 people.

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Linus Torvalds]

On Wed, 26 Jul 2023 at 11:45, Limonciello, Mario
<mario.limonciello@amd.com> wrote:
>
> If the goal is to get people who are involved with a file, how about
> some variation of:

No, that isn't the goal.

The goal is to get the maintainer responsible for that file, so that I
can bring them in.

Maybe it's a security issue that gets reported. Maybe it's an oops.
Maybe it's a regression, but not clear what caused it.

Maybe we could have a script to help do that, and call it
'get_maintainer' or something like that?

              Linus

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Jakub Kicinski]

On Wed, 26 Jul 2023 11:29:44 -0700 Linus Torvalds wrote:
> On Wed, 26 Jul 2023 at 11:20, Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > You are special,  
> 
> So my mother tells me.
> 
> > you presumably use it to find who to report
> > regressions to, and who to pull into conversations.  
> 
> Yes. So what happens is that I get cc'd on bug reports for various
> issues, and particularly for oops reports I basically have a function
> name to grep for (maybe a pathname if it went through the full
> decoding).
> 
> I'm NOT interested in having to either remember all people off-hand,
> or going through the MAINTAINERS file by hand.
> 
> > This tool is primarily used by _developers_ to find _maintainers_.  
> 
> Well, maybe.
> 
> But even if that is true, I don't see why you hate the pathname thing
> even for that case. I bet developers use it for that exact same
> reason, ie they are modifying a file, and they go "I want to know who
> the maintainer for this file is".

I don't hate the file path, I say as much in the commit msg:

  The file option should really not be used by inexperienced developers,
  unless they are just trying to find a maintainer to manually contact.

I'd love to make it easier to use for people who know what they're
doing. Maybe check for a magic file in the tree, listed in .gitignore?
Feels dirty. Create a separate script "blame_maintainer.sh" which just
calls get_maintainer but tosses in --silence-file-warning -f ?

> I do not understand why you think a patch is somehow magically more
> important or relevant than a filename.

Judging by traffic on the ML vast majority of the submissions are
patches, not random reach outs and conversations. That's why patches
are more important.

We get at least one fix a week where author adds a Fixes tag
but somehow magically didn't CC the author of that commit.
When we ask they usually reply with "but I run get_maintainer -f,
isn't that what I'm supposed to do?".

Then there's people who only run -f on the path where most of 
the changes are, not all the paths.

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Linus Torvalds]

On Wed, 26 Jul 2023 at 11:48, Jakub Kicinski <kuba@kernel.org> wrote:
>
> We get at least one fix a week where author adds a Fixes tag
> but somehow magically didn't CC the author of that commit.
> When we ask they usually reply with "but I run get_maintainer -f,
> isn't that what I'm supposed to do?".

Bah. I think you're blaming entirely the wrong people, and the wrong tool.

Your complaint seems to be "we got a fix, it even says what commit it
is fixing, and the tool that the person ran didn't add the right
people automatically".

And my reaction is "I use that tooling, I want it to do exactly what
it does right now, why are you blaming that tool"?

You're already using 'patchwork'. Why don't you instead go "Oh, *that*
tool isn't doing the right thing?"

                  Linus

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Linus Torvalds]

On Wed, 26 Jul 2023 at 11:59, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> You're already using 'patchwork'. Why don't you instead go "Oh, *that*
> tool isn't doing the right thing?"

Christ. Looking around, patchwork *ALREADY DOES THIS*.

Except it looks like it might be set up to just complain
("netdev/cc_maintainers"). Which seems to be why you're complaining.

IOW, you're complaining about *another* tool, because your own tool
use is set up to complain instead of being helpful.

I'm now even more convinced that that warning is completely bogus.

No way in hell am *I* going to bend over backwards and add some stupid
new rule to my workflow because *you* use a tool that is a whining
little complaint-machine instead of just *fixing* the problem.

Guys, tools are supposed to *help*, not just whine about
technicalities and make it harder for everybody else.

So just fix your tool. Don't complain about *my* use of another tool.

                 Linus

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Linus Torvalds]

On Wed, 26 Jul 2023 at 12:05, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Except it looks like it might be set up to just complain
> ("netdev/cc_maintainers"). Which seems to be why you're complaining.
>
> IOW, you're complaining about *another* tool, because your own tool
> use is set up to complain instead of being helpful.

The very first case I actually looked at wasn't even some
"inexperienced developer" - the kind you claim is the problem, and the
kind you claim this would help.

It was a random fix from Florian Westphal, who has been around for
more than a decade, is credited with over 1500 commits (and mentioned
in many many more), and knows what he's doing.

He has a patch that references a "Fixes:" line, and clearly didn't go
through the get_maintainer script as such, and the
netdev/cc_maintainers script complains as a result.

So Jakub, I think you are barking *entirely* up the wrong tree.

The reason you blame this on mis-use by inexperienced maintainers is
that you probably never even react to the experienced ones that do the
very same things, because you trust them and never bother to tell them
"you didn't use get_maintainers to get the precise list of people that
patchwork complains about".

So the problem is not in get_maintainers. It's in having expectations
that are simply not realistic.

You seem to think that those inexperienced developers should do something that

 (a) experienced developers don't do *EITHER*

 (b) the scripts complain about instead of just doing

and then you think that changing get_maintainers would somehow hide the issue.

You definitely shouldn't require inexperienced developers to do
something that clearly experienced people then don't do.

Now, maybe I happened to just randomly pick a patchwork entry that was
very unusual. But I doubt it.

           Linus

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Jakub Kicinski]

On Wed, 26 Jul 2023 12:37:14 -0700 Linus Torvalds wrote:
> The very first case I actually looked at wasn't even some
> "inexperienced developer" - the kind you claim is the problem, and the
> kind you claim this would help.
> 
> It was a random fix from Florian Westphal, who has been around for
> more than a decade, is credited with over 1500 commits (and mentioned
> in many many more), and knows what he's doing.
> 
> He has a patch that references a "Fixes:" line, and clearly didn't go
> through the get_maintainer script as such, and the
> netdev/cc_maintainers script complains as a result.

Florian is sending us patches from his tree which have already been
reviewed on the netfilter mailing list. It's basically a PR.
There's a handful of people who do that and I don't care enough to
silence it because ignoring the false positives is a noop.

When some noob sends a patch which actually *should* have been CCed
to more people I need to either go and CC that person in myself.
Or tell the noob to repost.

IOW solving the _actually_ missing CCs is higher priority for me.

> So Jakub, I think you are barking *entirely* up the wrong tree.
> 
> The reason you blame this on mis-use by inexperienced maintainers is
> that you probably never even react to the experienced ones that do the
> very same things, because you trust them and never bother to tell them
> "you didn't use get_maintainers to get the precise list of people that
> patchwork complains about".
> 
> So the problem is not in get_maintainers. It's in having expectations
> that are simply not realistic.
> 
> You seem to think that those inexperienced developers should do something that
> 
>  (a) experienced developers don't do *EITHER*
> 
>  (b) the scripts complain about instead of just doing
> 
> and then you think that changing get_maintainers would somehow hide the issue.
> 
> You definitely shouldn't require inexperienced developers to do
> something that clearly experienced people then don't do.
> 
> Now, maybe I happened to just randomly pick a patchwork entry that was
> very unusual. But I doubt it.

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Linus Torvalds]

On Wed, 26 Jul 2023 at 13:03, Jakub Kicinski <kuba@kernel.org> wrote:
>
> IOW solving the _actually_ missing CCs is higher priority for me.

You have the script. It's already being run. Use it.

Having scripting that complains about missing Cc's, even *lists* them,
and then requires a human to do something about it - that's stupid.

Why are you using computers and automation in the first place, if said
automation then just makes for more work?

Then requiring inexperienced developers to do those extra things,
knowing - and not caring - that the experienced ones won't even
bother, that goes from stupid to actively malicious.

And then asking me to change my workflow because I use a different
script that does exactly what I want - that takes "stupid and
malicious" to something where I will just ignore you.

In other words: those changes to get_maintainer are simply not going to happen.

Fix your own scripts, and fix your bad workflows.

Your bad workflow not only makes it harder for new people to get
involved, they apparently waste your *own* time so much that you are
upset about it all.

Don't shoot yourself in the foot - and if you insist on doing so,
don't ask *others* to join you in your self-destructive tendencies.

               Linus

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Jakub Kicinski]

On Wed, 26 Jul 2023 13:13:11 -0700 Linus Torvalds wrote:
> On Wed, 26 Jul 2023 at 13:03, Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > IOW solving the _actually_ missing CCs is higher priority for me.  
> 
> You have the script. It's already being run. Use it.
> 
> Having scripting that complains about missing Cc's, even *lists* them,
> and then requires a human to do something about it - that's stupid.

Just so I fully understand what you're saying - what do you expect me
to do? Send the developer a notifications saying "please repost" with
this CC list? How is that preferable to making them do it right the
first time?!

The script in patchwork *just runs get_maintainer on the patch*:

https://github.com/kuba-moo/nipa/blob/master/tests/patch/cc_maintainers/test.py#L58

And developers also *already* *run* get_maintainer, they just need to 
be nudged to prefer running it on the patch rather than on the path.

And no, Joe's position that this is "just a documentation problem"
does not survive crash with reality because we already documented:

Documentation/process/submitting-patches.rst:

  scripts/get_maintainer.pl can be very useful at this step (pass paths
  to your patches as arguments to scripts/get_maintainer.pl).

Documentation/process/3.Early-stage.rst:

 If passed a patch on the command line, it will list the maintainers
 who should probably receive copies of the patch.  This is the
 preferred way (unlike "-f" option) to get the list of people to Cc for
 your patches.

> Why are you using computers and automation in the first place, if said
> automation then just makes for more work?

Writing and maintaining that automation is also damn work. We complain
nobody wants to be a maintainer and then refuse to make maintainers'
life's easier :|

> Then requiring inexperienced developers to do those extra things,
> knowing - and not caring - that the experienced ones won't even
> bother, that goes from stupid to actively malicious.
> 
> And then asking me to change my workflow because I use a different
> script that does exactly what I want - that takes "stupid and
> malicious" to something where I will just ignore you.
> 
> In other words: those changes to get_maintainer are simply not going to happen.
> 
> Fix your own scripts, and fix your bad workflows.
> 
> Your bad workflow not only makes it harder for new people to get
> involved, they apparently waste your *own* time so much that you are
> upset about it all.
> 
> Don't shoot yourself in the foot - and if you insist on doing so,
> don't ask *others* to join you in your self-destructive tendencies.

No idea what you mean by "my workflow". But yeah, I kind of expected
that this patch would be a waste of time. Certain problems only become
clear with sufficient volume of patches, and I'm clearly incapable
of explaining shit.

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Linus Torvalds]

On Wed, 26 Jul 2023 at 13:36, Jakub Kicinski <kuba@kernel.org> wrote:
>
> Just so I fully understand what you're saying - what do you expect me
> to do? Send the developer a notifications saying "please repost" with
> this CC list? How is that preferable to making them do it right the
> first time?!

Not at all.

The whole point is that you already end up relying on scripting to
notice that some people should be cc'd, so just add them
automatically.

Why would you

 (a) waste your own time asking the original developer to re-do his submission

 (b) ask the original developer to do something that clearly long-time
developers don't do

 (c) waste *everybody's* time re-submitting a change that was detected
automatically and could just have been done automatically in the first
place?

just make patchwork add the cc's automatically to the patch - and send
out emails to the people it added.

Patchwork already sends out emails for other things. Guess how I know?
Because I get the patchwork-bot emails all the time for things I have
been cc'd on.  Including, very much, the netdevbpf ones.

And people who don't want to be notified can already register with
patchwork to not be notified. It's right there in that

   Deet-doot-dot, I am a bot.
   https://korg.docs.kernel.org/patchwork/pwbot.html

footer.

So I would literally suggest you just stop asking people to do things
that automation CAN DO BETTER.

The patchwork notification could be just a small note (the same way
the pull request notes are) that point to the submission, and say
"your name has been added to the Cc for this patch because it claims
to fix something you authored or acked".

See what I'm saying? Why are you wasting your time on this? Why are
you making new developers do pointless stuff that is better done by a
script, since you're just asking the developer to run a script in the
first place?

You are just wasting literally EVERYBODY'S time with your workflow
rules. For no actual advantage, since the whole - and only - point of
this all was that it was scriptable, and is in fact already being
scripted, which is how you even notice the issue in the first place.

You seem to be just overly attached to having people waste their time
on running a script that you run automatically *anyway*, and make that
some "required thing for inexperienced developers".

And it can't even be the right thing to do, when experienced
developers don't do it.

That, to me, seems completely crazy.

                   Linus

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Jakub Kicinski]

On Wed, 26 Jul 2023 14:07:28 -0700 Linus Torvalds wrote:
> On Wed, 26 Jul 2023 at 13:36, Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > Just so I fully understand what you're saying - what do you expect me
> > to do? Send the developer a notifications saying "please repost" with
> > this CC list? How is that preferable to making them do it right the
> > first time?!  
> 
> Not at all.
> 
> The whole point is that you already end up relying on scripting to
> notice that some people should be cc'd, so just add them
> automatically.
> 
> Why would you
> 
>  (a) waste your own time asking the original developer to re-do his submission
> 
>  (b) ask the original developer to do something that clearly long-time
> developers don't do
> 
>  (c) waste *everybody's* time re-submitting a change that was detected
> automatically and could just have been done automatically in the first
> place?
> 
> just make patchwork add the cc's automatically to the patch - and send
> out emails to the people it added.
> 
> Patchwork already sends out emails for other things. Guess how I know?
> Because I get the patchwork-bot emails all the time for things I have
> been cc'd on.  Including, very much, the netdevbpf ones.
> 
> And people who don't want to be notified can already register with
> patchwork to not be notified. It's right there in that
> 
>    Deet-doot-dot, I am a bot.
>    https://korg.docs.kernel.org/patchwork/pwbot.html
> 
> footer.
> 
> So I would literally suggest you just stop asking people to do things
> that automation CAN DO BETTER.
> 
> The patchwork notification could be just a small note (the same way
> the pull request notes are) that point to the submission, and say
> "your name has been added to the Cc for this patch because it claims
> to fix something you authored or acked".

Lots of those will be false positives, and also I do not want 
to sign up to maintain a bot which actively bothers people.
And have every other subsystem replicate something of that nature.

Sidebar, but IMO we should work on lore to create a way to *subscribe*
to patches based on paths without running any local agents. But if I
can't explain how get_maintainers is misused I'm sure I'll have a lot
of luck explaining that one :D

> See what I'm saying? Why are you wasting your time on this? Why are
> you making new developers do pointless stuff that is better done by a
> script, since you're just asking the developer to run a script in the
> first place?

For the last time, most people already run get_maintainer, they just 
choose the wrong "mode" of running it for the use case.
I am not trying to make anyone do anything they aren't already doing.

> You are just wasting literally EVERYBODY'S time with your workflow
> rules. For no actual advantage, since the whole - and only - point of
> this all was that it was scriptable, and is in fact already being
> scripted, which is how you even notice the issue in the first place.

And it has nothing to do with *my* workflow. Unless you're arguing 
that asking for authors of patches which Fixes points to is part of
"my" workflow and nobody else's.

> You seem to be just overly attached to having people waste their time
> on running a script that you run automatically *anyway*, and make that
> some "required thing for inexperienced developers".

I said "for the last time" so I won't repeat...

> And it can't even be the right thing to do, when experienced
> developers don't do it.

I explained to you already that Florian's posting is a PR.

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Linus Torvalds]

On Wed, 26 Jul 2023 at 14:57, Jakub Kicinski <kuba@kernel.org> wrote:
>
> > And it can't even be the right thing to do, when experienced
> > developers don't do it.
>
> I explained to you already that Florian's posting is a PR.

.. and your point is?

The fact is, experienced developers don't add cc'd when they commit
their own patches, and nobody bats an eye on it.

So you are *literally* saying that inexperienced people should do this
pointless thing that can be - and is - automated, even though it's
clearly not the thing that is required in general.

And you're saying that I should have to change my workflow to make it
MORE INCONVENIENT to do the thing that you claim inexperienced people
do now.

Yup. We're done here.

               Linus

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Linus Torvalds]

On Wed, 26 Jul 2023 at 14:57, Jakub Kicinski <kuba@kernel.org> wrote:
>
> Lots of those will be false positives, and also I do not want
> to sign up to maintain a bot which actively bothers people.

I don't really see the difference between creating a bot that
"actively bothers people" and asking _people_ to run a script and then
re-send the patch to actively bother the exact same people.

They'd get bothered either way.

At least with the bot, they can opt out of the automation.

> Sidebar, but IMO we should work on lore to create a way to *subscribe*
> to patches based on paths without running any local agents.

I already contacted Konstantin to see how hard it would be to change
patchwork-bot scripting, and he's trawling this thread.

Maybe he will just go "the code already does all the patch detection
and looks up pathnames in them anyway, and we already maintain a list
of people who do not want to be bothered, it would be easy to add the
'please bother me for these paths' script too".

             Linus

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Konstantin Ryabitsev]

On Wed, Jul 26, 2023 at 02:57:21PM -0700, Jakub Kicinski wrote:
> > The patchwork notification could be just a small note (the same way
> > the pull request notes are) that point to the submission, and say
> > "your name has been added to the Cc for this patch because it claims
> > to fix something you authored or acked".
> 
> Lots of those will be false positives, and also I do not want 
> to sign up to maintain a bot which actively bothers people.

I feel seen.

> And have every other subsystem replicate something of that nature.
> 
> Sidebar, but IMO we should work on lore to create a way to *subscribe*
> to patches based on paths without running any local agents. But if I
> can't explain how get_maintainers is misused I'm sure I'll have a lot
> of luck explaining that one :D

I just need to get off my ass and implement this. We should be able to offer
the following:

- subsystem maintainers come up with query language for what they want
  to monitor (basically, whatever the query box of lore.kernel.org takes)
- we maintain a bot that runs these queries and populates a public-inbox feed
- this feed is available via read-only pop/imap/nntp (pull subscription)
- it is also fed to a mailing list service (push subscription)

The goal is to turn the tables -- instead of patch submitters needing to
figure out where the patch needs to go (via get_maintainer or similar
scripts), they just send everything to lkml or patches@lists.linux.dev and let
the system figure out who needs to look at them.

That's for the part that I was already planning to do. In addition, coming
back to the topic of this thread, we could also look at individual patches
hitting the feed, pass them through any desired configuration of
get_maintainer.pl, and send them off any recipients not already cc'd by the
patch author. I believe this is what you want to have in place, right, Jakub?

-K

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Jakub Kicinski]

On Wed, 26 Jul 2023 19:47:31 -0400 Konstantin Ryabitsev wrote:
> > And have every other subsystem replicate something of that nature.
> > 
> > Sidebar, but IMO we should work on lore to create a way to *subscribe*
> > to patches based on paths without running any local agents. But if I
> > can't explain how get_maintainers is misused I'm sure I'll have a lot
> > of luck explaining that one :D  
> 
> I just need to get off my ass and implement this. We should be able to offer
> the following:
> 
> - subsystem maintainers come up with query language for what they want
>   to monitor (basically, whatever the query box of lore.kernel.org takes)
> - we maintain a bot that runs these queries and populates a public-inbox feed
> - this feed is available via read-only pop/imap/nntp (pull subscription)
> - it is also fed to a mailing list service (push subscription)

*Nod*

> The goal is to turn the tables -- instead of patch submitters needing to
> figure out where the patch needs to go (via get_maintainer or similar
> scripts), they just send everything to lkml or patches@lists.linux.dev and let
> the system figure out who needs to look at them.

My initial motivation for this was to let people (who are *not*
maintainers) subscribe to parts of netdev. During previous cycles we
saw ~246 emails a day. If someone is only interested in e.g. IP routing
fishing out the one routing patch a week from all the driver noise is
almost impossible.

> That's for the part that I was already planning to do. In addition, coming
> back to the topic of this thread, we could also look at individual patches
> hitting the feed, pass them through any desired configuration of
> get_maintainer.pl, and send them off any recipients not already cc'd by the
> patch author. I believe this is what you want to have in place, right, Jakub?

Hm, hm. I wasn't thrilled by the idea of sending people a notification
that "you weren't CCed on this patch, here's a link". But depending on
your definition of "hitting the feed" it sounds like we may be able to
insert the CC into the actual email before it hits lore? That'd be
very cool! At least for the lists already migrated from vger to korg?

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Konstantin Ryabitsev]

On Wed, Jul 26, 2023 at 05:11:23PM -0700, Jakub Kicinski wrote:
> Hm, hm. I wasn't thrilled by the idea of sending people a notification
> that "you weren't CCed on this patch, here's a link". But depending on
> your definition of "hitting the feed" it sounds like we may be able to
> insert the CC into the actual email before it hits lore? That'd be
> very cool! At least for the lists already migrated from vger to korg?

No, inserting their addresses into message headers would invalidate DKIM,
which is not what we want to do. However, the idea is that they would receive
the actual patches in the same way they would receive them if they were
subscribed to a mailing list.

Think as if instead of being Cc'd on patches, they got Bcc'd on them.

-K

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Jakub Kicinski]

On Wed, 26 Jul 2023 20:24:06 -0400 Konstantin Ryabitsev wrote:
> On Wed, Jul 26, 2023 at 05:11:23PM -0700, Jakub Kicinski wrote:
> > Hm, hm. I wasn't thrilled by the idea of sending people a notification
> > that "you weren't CCed on this patch, here's a link". But depending on
> > your definition of "hitting the feed" it sounds like we may be able to
> > insert the CC into the actual email before it hits lore? That'd be
> > very cool! At least for the lists already migrated from vger to korg?  
> 
> No, inserting their addresses into message headers would invalidate DKIM,
> which is not what we want to do. However, the idea is that they would receive
> the actual patches in the same way they would receive them if they were
> subscribed to a mailing list.

Ugh, right :S

> Think as if instead of being Cc'd on patches, they got Bcc'd on them.

I was being crafty 'cause if the CC is present in the lore archive
our patchwork check would see it :] 
But cant just trust the automation and kill the check, then.

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Konstantin Ryabitsev]

On Wed, Jul 26, 2023 at 05:27:58PM -0700, Jakub Kicinski wrote:
> > Think as if instead of being Cc'd on patches, they got Bcc'd on them.
> 
> I was being crafty 'cause if the CC is present in the lore archive
> our patchwork check would see it :] 
> But cant just trust the automation and kill the check, then.

If we can get it working reliably and to everyone's satisfaction, then I think
the need for this check goes away. The submitter can then just worry about
getting *everything else* right about the patch and not worry about who to add
into Cc's.

-K

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Joe Perches]

On Wed, 2023-07-26 at 20:33 -0400, Konstantin Ryabitsev wrote:
> On Wed, Jul 26, 2023 at 05:27:58PM -0700, Jakub Kicinski wrote:
> > > Think as if instead of being Cc'd on patches, they got Bcc'd on them.
> > 
> > I was being crafty 'cause if the CC is present in the lore archive
> > our patchwork check would see it :] 
> > But cant just trust the automation and kill the check, then.
> 
> If we can get it working reliably and to everyone's satisfaction,

big if.  great concept though.

arch wide changes are different.
API changes are different.
staging patches are different.

It'll take a bit of settling and some special case handling
but I believe it'd work out quite well in long run to have a
patches@vger.kernel.org address front end that is a scripted
exploder to the appropriate maintainers, reviewers, mailing
lists etc.

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Andrew Lunn]

> Think as if instead of being Cc'd on patches, they got Bcc'd on them.

And how does reply work? I assume it would only go to those in To: or
Cc: ? Is there enough context in the headers in a reply for the system
to figure out who to Bcc: the reply to?

   Andrew

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Konstantin Ryabitsev]

On Thu, Jul 27, 2023 at 01:00:15PM +0200, Andrew Lunn wrote:
> > Think as if instead of being Cc'd on patches, they got Bcc'd on them.
> 
> And how does reply work? I assume it would only go to those in To: or
> Cc: ? Is there enough context in the headers in a reply for the system
> to figure out who to Bcc: the reply to?

I have actually solved a similar problem already as part of a different
project (bugbot). We associate a set of additional addresses with a thread and
can send any thread updates to those addresses.

It would require a bit more effort to adapt it so we properly handle bounces,
but effectively this does what you're asking about -- replies sent to a thread
will be sent out to all addresses we've associated with that thread (via
get_maintainer.pl). In a sense, this will create a miniature pseudo-mailing
list per each thread with its own set of subscribers.

I just need to make sure this doesn't fall over once we are hitting
LKML-levels of activity.

-K

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Jakub Kicinski]

On Fri, 28 Jul 2023 16:29:13 -0400 Konstantin Ryabitsev wrote:
> I have actually solved a similar problem already as part of a different
> project (bugbot). We associate a set of additional addresses with a thread and
> can send any thread updates to those addresses.
> 
> It would require a bit more effort to adapt it so we properly handle bounces,
> but effectively this does what you're asking about -- replies sent to a thread
> will be sent out to all addresses we've associated with that thread (via
> get_maintainer.pl). In a sense, this will create a miniature pseudo-mailing
> list per each thread with its own set of subscribers.
> 
> I just need to make sure this doesn't fall over once we are hitting
> LKML-levels of activity.

How does that square with the "subscribe by path / keyword" concept?
If we can do deep magic of this sort can we also use it to SMTP to
people what they wanted to subscribe to rather than expose it as
POP/IMAP/NNTP?

Could be easier for people to do dedup and alike if subscriptions were
flowing into their usual inbox.

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Konstantin Ryabitsev]

On Fri, Jul 28, 2023 at 01:38:01PM -0700, Jakub Kicinski wrote:
> > It would require a bit more effort to adapt it so we properly handle bounces,
> > but effectively this does what you're asking about -- replies sent to a thread
> > will be sent out to all addresses we've associated with that thread (via
> > get_maintainer.pl). In a sense, this will create a miniature pseudo-mailing
> > list per each thread with its own set of subscribers.
> > 
> > I just need to make sure this doesn't fall over once we are hitting
> > LKML-levels of activity.
> 
> How does that square with the "subscribe by path / keyword" concept?
> If we can do deep magic of this sort can we also use it to SMTP to
> people what they wanted to subscribe to rather than expose it as
> POP/IMAP/NNTP?

Well, people can subscribe without needing to become an M: or R: entry in
MAINTAINERS. I guess that would be the main difference.

-K

Re: [PATCH v2] scripts: get_maintainer: steer people away from using file paths

[Joe Perches]

On Fri, 2023-07-28 at 16:29 -0400, Konstantin Ryabitsev wrote:
> On Thu, Jul 27, 2023 at 01:00:15PM +0200, Andrew Lunn wrote:
> > > Think as if instead of being Cc'd on patches, they got Bcc'd on them.
> > 
> > And how does reply work? I assume it would only go to those in To: or
> > Cc: ? Is there enough context in the headers in a reply for the system
> > to figure out who to Bcc: the reply to?
> 
> I have actually solved a similar problem already as part of a different
> project (bugbot). We associate a set of additional addresses with a thread and
> can send any thread updates to those addresses.
> 
> It would require a bit more effort to adapt it so we properly handle bounces,
> but effectively this does what you're asking about -- replies sent to a thread
> will be sent out to all addresses we've associated with that thread (via
> get_maintainer.pl). In a sense, this will create a miniature pseudo-mailing
> list per each thread with its own set of subscribers.
> 
> I just need to make sure this doesn't fall over once we are hitting
> LKML-levels of activity.
> 

How about whenever a single mailing list like
	linux-patches@vger.kernel.org
gets new 0/n without an in-reply-to header and m/n patches with
only the single in-reply-to header of an 0/n patch or simply a
single patch without an in-reply-to header, the cc list is
automatically generated from a tool like get_maintainer and a
From: <sender> line is added if necessary to the message body
and the email forwarded to all cc's and linux-patches is removed
from the email?

I believe that would help solve most correctness of recipient
list issues and then the linux-patches list would not need
further involvement.