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

[Mario Limonciello <mario.limonciello@amd.com>]

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.