| Message ID | 20230725155926.2775416-1-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scripts: checkpatch: steer people away from using file paths | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Tue, Jul 25, 2023 at 08:59:26AM -0700, Jakub Kicinski wrote: > We repeatedly see noobs 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 noobs, 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. Ok, I'll go fix up my local scripts, but you should change your subject line to say "get_maintainer", not "checkpatch" :) thanks, greg k-h
On 25/07/2023 17:59, Jakub Kicinski wrote: > We repeatedly see noobs 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 noobs, 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. > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > This is what I had in mind. "Anything that can go wrong will go wrong." https://en.wikipedia.org/wiki/Murphy%27s_law Therefore: Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On Tue, 25 Jul 2023 18:53:48 +0200 Greg KH wrote: > > This script may break people's "scripts on top of get_maintainer" > > if they are using -f... but that's the point. > > Ok, I'll go fix up my local scripts, Which one? I spotted this in your repo but it already seems to use patches: https://github.com/gregkh/gregkh-linux/blob/master/scripts/generate_cc_list How do you use this, BTW? > but you should change your subject > line to say "get_maintainer", not "checkpatch" :) Ugh, will do :)
On Tue, Jul 25, 2023 at 10:10:51AM -0700, Jakub Kicinski wrote: > On Tue, 25 Jul 2023 18:53:48 +0200 Greg KH wrote: > > > This script may break people's "scripts on top of get_maintainer" > > > if they are using -f... but that's the point. > > > > Ok, I'll go fix up my local scripts, > > Which one? I spotted this in your repo but it already seems > to use patches: > > https://github.com/gregkh/gregkh-linux/blob/master/scripts/generate_cc_list Oh yeah, it does work on patches. Nevermind, I think I just use the -f version manually when trying to figure out who to blame for a bug report in a specific file :) > How do you use this, BTW? I do: - git format-patch to generate the patch series. - run the generate_cc_list script which creates XXXX.info files (the XXXX being the patch number) that contain the people/lists to cc: on the patch - git rebase -i on the patch series and edit the changelog description and paste in the XXXX.info file for that specific patch. Yeah, it's a lot of manual steps, I should use b4 for it, one of these days... thanks, greg k-h
On Tue, 25 Jul 2023 19:25:09 +0200 Greg KH wrote: > I do: > - git format-patch to generate the patch series. > - run the generate_cc_list script which creates XXXX.info files > (the XXXX being the patch number) that contain the > people/lists to cc: on the patch > - git rebase -i on the patch series and edit the changelog > description and paste in the XXXX.info file for that specific > patch. > > Yeah, it's a lot of manual steps, I should use b4 for it, one of these > days... Oh, neat! I do a similar thing. Modulo the number of steps: git rebase --exec './ccer.py --inline' I was wondering if I was the only one who pastes the Cc: person into the patch, because I'd love to teach get_maintainer to do the --inline thing, instead of carrying my own wrapper...
On Tue, 2023-07-25 at 12:52 -0700, Jakub Kicinski wrote: > On Tue, 25 Jul 2023 19:25:09 +0200 Greg KH wrote: > > I do: > > - git format-patch to generate the patch series. > > - run the generate_cc_list script which creates XXXX.info files > > (the XXXX being the patch number) that contain the > > people/lists to cc: on the patch > > - git rebase -i on the patch series and edit the changelog > > description and paste in the XXXX.info file for that specific > > patch. > > > > Yeah, it's a lot of manual steps, I should use b4 for it, one of these > > days... > > Oh, neat! I do a similar thing. Modulo the number of steps: > > git rebase --exec './ccer.py --inline' > > I was wondering if I was the only one who pastes the Cc: person > into the patch, because I'd love to teach get_maintainer to do > the --inline thing, instead of carrying my own wrapper... Now for that, you'd want checkpatch or yet another script to do it.
On Tue, 2023-07-25 at 08:59 -0700, Jakub Kicinski wrote: > We repeatedly see noobs 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 noobs, unless > they are just trying to find a maintainer to manually contact. noobs is not an appropriate use IMO for a commit message. > This is what I had in mind. <shrug> I think it's unnecessary and this content should be better described in some process doc. > diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl [] > @@ -544,7 +546,13 @@ 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"; Don't separate a single output message into multiple lines. Coalesce the string elements. Also, this should show some reason why this isn't appropriate as a patch to a single file would not have this issue. e.g.: When a patch series touches multiple files, showing all maintainers is useful. see: <some process doc> > @@ -1089,6 +1098,10 @@ version: $V > --pattern-depth=0 --remove-duplicates --rolestats] > > Notes: > + Using "-f file" is generally discouraged, running the script on a filepatch > + (as generated by git format-patch) is usually the right thing to do. > + Commit message is an integral part of the change and $P > + will extract additional information from it (keywords, Fixes tags etc.) -f <file> "filepatch" doesn't appear in the kernel at all. Use "patch file". grammar: The commit message is... may instead of will
On Tue, 25 Jul 2023 14:18:15 -0700 Joe Perches wrote: > > @@ -544,7 +546,13 @@ 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"; > > Don't separate a single output message into multiple lines. > Coalesce the string elements. > > Also, this should show some reason why this isn't appropriate > as a patch to a single file would not have this issue. > > e.g.: When a patch series touches multiple files, showing all maintainers is useful. see: <some process doc> I tried to do that in --help. Added the "multiple files" one there, too. > > @@ -1089,6 +1098,10 @@ version: $V > > --pattern-depth=0 --remove-duplicates --rolestats] > > > > Notes: > > + Using "-f file" is generally discouraged, running the script on a filepatch > > + (as generated by git format-patch) is usually the right thing to do. > > + Commit message is an integral part of the change and $P > > + will extract additional information from it (keywords, Fixes tags etc.) > > "filepatch" doesn't appear in the kernel at all. Use "patch file". I got it the wrong way around. I'll use patchfile (no space) for v2 since that's what's what get_maintainer uses in two other places.
On Tue, 2023-07-25 at 15:15 -0700, Jakub Kicinski wrote: > On Tue, 25 Jul 2023 14:18:15 -0700 Joe Perches wrote: > > > @@ -544,7 +546,13 @@ 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"; > > > > Don't separate a single output message into multiple lines. > > Coalesce the string elements. > > > > Also, this should show some reason why this isn't appropriate > > as a patch to a single file would not have this issue. > > > > e.g.: When a patch series touches multiple files, showing all maintainers is useful. see: <some process doc> > > I tried to do that in --help. Added the "multiple files" one there, too. It'd be more useful in the warning message. Hardly anyone reads help. > > > > @@ -1089,6 +1098,10 @@ version: $V > > > --pattern-depth=0 --remove-duplicates --rolestats] > > > > > > Notes: > > > + Using "-f file" is generally discouraged, running the script on a filepatch > > > + (as generated by git format-patch) is usually the right thing to do. > > > + Commit message is an integral part of the change and $P > > > + will extract additional information from it (keywords, Fixes tags etc.) > > > > "filepatch" doesn't appear in the kernel at all. Use "patch file". > > I got it the wrong way around. I'll use patchfile (no space) for v2 > since that's what's what get_maintainer uses in two other places.
diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl index ab123b498fd9..3635b3d40ebe 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,13 @@ 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 +1089,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 +1098,10 @@ version: $V --pattern-depth=0 --remove-duplicates --rolestats] Notes: + Using "-f file" is generally discouraged, running the script on a filepatch + (as generated by git format-patch) is usually the right thing to do. + Commit message is an integral part of the change and $P + will extract additional information from it (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.
We repeatedly see noobs 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 noobs, 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. Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- This is what I had in mind. CC: joe@perches.com Cc: geert@linux-m68k.org Cc: krzk@kernel.org Cc: netdev@vger.kernel.org Cc: workflows@vger.kernel.org Cc: mario.limonciello@amd.com --- scripts/get_maintainer.pl | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)