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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ccbe3ab2-7fb0-3ce3-6054-77e0dc365b84@amd.com>
Date: Wed, 26 Jul 2023 10:20:16 -0500
From: Mario Limonciello <mario.limonciello@....com>
To: Jakub Kicinski <kuba@...nel.org>, joe@...ches.com
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
 geert@...ux-m68k.org, gregkh@...uxfoundation.org, netdev@...r.kernel.org,
 workflows@...r.kernel.org
Subject: Re: [PATCH v2] scripts: get_maintainer: steer people away from using
 file paths

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@...aro.org>
> Signed-off-by: Jakub Kicinski <kuba@...nel.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@...ches.com
> Cc: geert@...ux-m68k.org
> Cc: gregkh@...uxfoundation.org
> Cc: netdev@...r.kernel.org
> Cc: workflows@...r.kernel.org
> Cc: mario.limonciello@....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.