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@xxxxxxxxxx>
Signed-off-by: Jakub Kicinski <kuba@xxxxxxxxxx>
---
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@xxxxxxxxxx/
CC: joe@xxxxxxxxxxx
Cc: geert@xxxxxxxxxxxxxx
Cc: gregkh@xxxxxxxxxxxxxxxxxxx
Cc: netdev@xxxxxxxxxxxxxxx
Cc: workflows@xxxxxxxxxxxxxxx
Cc: mario.limonciello@xxxxxxx
---
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.
