* Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload [not found] ` <c690776ce6fd247c2b2aeb805744d5779b6293ab.camel@perches.com> @ 2023-07-25 1:04 ` Jakub Kicinski 2023-07-25 3:53 ` Joe Perches 0 siblings, 1 reply; 16+ messages in thread From: Jakub Kicinski @ 2023-07-25 1:04 UTC (permalink / raw) To: Joe Perches; +Cc: Krzysztof Kozlowski, netdev, workflows, linux-kernel [clean up the CC list] On Fri, 21 Jul 2023 20:32:44 -0700 Joe Perches wrote: > On Fri, 2023-07-21 at 18:55 -0700, Jakub Kicinski wrote: > > On Fri, 21 Jul 2023 18:21:32 +0200 Krzysztof Kozlowski wrote: > > > That's not how you run it. get_maintainers.pl should be run on patches > > > or on all files, not just some selection. > > > > Adding Joe for visibility (I proposed to print a warning when people > > do this and IIRC he wasn't on board). > > What's the issue here? Other than _how_ the script was used, > I don't see an actual problem with the script itself. I just CCed you on another case. If people keep using get_maintainers wrong, it *is* an issue with the script. > I use the scripts below to send patch series where a patch series > are the only files in individual directories. The fact that most people end up wrapping get_maintainers in another script is also a pretty strong indication that the user experience is inadequate. To be clear - I'm happy to put in the work to make the changes. It's just that from past experience you seem to have strong opinions which run counter to maintainers' needs, and I don't really enjoy writing Perl for the sake of it. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload 2023-07-25 1:04 ` [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload Jakub Kicinski @ 2023-07-25 3:53 ` Joe Perches 2023-07-25 7:33 ` Geert Uytterhoeven 0 siblings, 1 reply; 16+ messages in thread From: Joe Perches @ 2023-07-25 3:53 UTC (permalink / raw) To: Jakub Kicinski Cc: Krzysztof Kozlowski, netdev, workflows, linux-kernel, MarioLimonciello On Mon, 2023-07-24 at 18:04 -0700, Jakub Kicinski wrote: > [clean up the CC list] > > On Fri, 21 Jul 2023 20:32:44 -0700 Joe Perches wrote: > > On Fri, 2023-07-21 at 18:55 -0700, Jakub Kicinski wrote: > > > On Fri, 21 Jul 2023 18:21:32 +0200 Krzysztof Kozlowski wrote: > > > > That's not how you run it. get_maintainers.pl should be run on patches > > > > or on all files, not just some selection. > > > > > > Adding Joe for visibility (I proposed to print a warning when people > > > do this and IIRC he wasn't on board). > > > > What's the issue here? Other than _how_ the script was used, > > I don't see an actual problem with the script itself. > > I just CCed you on another case. If people keep using get_maintainers > wrong, it *is* an issue with the script. Silly. No it's not. > > I use the scripts below to send patch series where a patch series > > are the only files in individual directories. > > The fact that most people end up wrapping get_maintainers in another > script is also a pretty strong indication that the user experience is > inadequate. Not a useful argument. Process and documentation are required. > To be clear - I'm happy to put in the work to make the changes. > It's just that from past experience you seem to have strong opinions > which run counter to maintainers' needs, and I don't really enjoy > writing Perl for the sake of it. Does anyone? Knock yourself out doing whatever you like. I do suggest you instead write wrapper scripts to get the output you want rather than updating the defaults for the script and update the process documentation to let other people know what do to as well. Something akin to Mario Limonciello's suggestion back in 2022: https://lore.kernel.org/lkml/20220617183215.25917-1-mario.limonciello@amd.com/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload 2023-07-25 3:53 ` Joe Perches @ 2023-07-25 7:33 ` Geert Uytterhoeven 2023-07-25 13:19 ` Mario Limonciello 0 siblings, 1 reply; 16+ messages in thread From: Geert Uytterhoeven @ 2023-07-25 7:33 UTC (permalink / raw) To: Joe Perches Cc: Jakub Kicinski, Krzysztof Kozlowski, netdev, workflows, linux-kernel, MarioLimonciello Hi Joe, On Tue, Jul 25, 2023 at 6:22 AM Joe Perches <joe@perches.com> wrote: > I do suggest you instead write wrapper scripts to get > the output you want rather than updating the defaults > for the script and update the process documentation > to let other people know what do to as well. > > Something akin to Mario Limonciello's suggestion back in 2022: > > https://lore.kernel.org/lkml/20220617183215.25917-1-mario.limonciello@amd.com/ FTR, this is more or less what I am using to generate a script to send out patches: OUT=... echo git send-email \\ > $OUT # Add -cc # Wrap comment inside $(: ...) # Replace (...) in comment by [...] # Replace ] at EOL by ) again # Add continuation to EOL scripts/get_maintainer.pl $* | \ tr -d \" | \ sed -e 's/^/--cc "/' \ -e 's/ (/" $(: /' \ -e 's/ (/ [/' -e 's/)/]/' \ -e 's/]$/)/' \ -e 's/$/ \\/' | \ tee -a $OUT echo "*[0-9][0-9][0-9][0-9]-*.*" >> $OUT After generation, I edit the script to - Replace some --cc by --to, - Add/remove some people, and run "source $OUT" to send the patches... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload 2023-07-25 7:33 ` Geert Uytterhoeven @ 2023-07-25 13:19 ` Mario Limonciello 2023-07-25 13:43 ` Joe Perches 0 siblings, 1 reply; 16+ messages in thread From: Mario Limonciello @ 2023-07-25 13:19 UTC (permalink / raw) To: Geert Uytterhoeven, Joe Perches Cc: Jakub Kicinski, Krzysztof Kozlowski, netdev, workflows, linux-kernel On 7/25/23 02:33, Geert Uytterhoeven wrote: > Hi Joe, > > On Tue, Jul 25, 2023 at 6:22 AM Joe Perches <joe@perches.com> wrote: >> I do suggest you instead write wrapper scripts to get >> the output you want rather than updating the defaults >> for the script and update the process documentation >> to let other people know what do to as well. >> >> Something akin to Mario Limonciello's suggestion back in 2022: >> >> https://lore.kernel.org/lkml/20220617183215.25917-1-mario.limonciello@amd.com/ > > FTR, this is more or less what I am using to generate a script > to send out patches: > > OUT=... > echo git send-email \\ > $OUT > # Add -cc > # Wrap comment inside $(: ...) > # Replace (...) in comment by [...] > # Replace ] at EOL by ) again > # Add continuation to EOL > scripts/get_maintainer.pl $* | \ > tr -d \" | \ > sed -e 's/^/--cc "/' \ > -e 's/ (/" $(: /' \ > -e 's/ (/ [/' -e 's/)/]/' \ > -e 's/]$/)/' \ > -e 's/$/ \\/' | \ > tee -a $OUT > echo "*[0-9][0-9][0-9][0-9]-*.*" >> $OUT > > After generation, I edit the script to > - Replace some --cc by --to, > - Add/remove some people, > and run "source $OUT" to send the patches... > > Gr{oetje,eeting}s, My script is great for single subsystem patches as it gets all the right people but I've found problems whenever it crosses multiple subsystems. Many subsystem owners want to see the whole series of patches to understand how they interact. So the group of patches needs to be treated together which would need the wrapper to look at all patches instead. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload 2023-07-25 13:19 ` Mario Limonciello @ 2023-07-25 13:43 ` Joe Perches 2023-07-25 14:37 ` Krzysztof Kozlowski 0 siblings, 1 reply; 16+ messages in thread From: Joe Perches @ 2023-07-25 13:43 UTC (permalink / raw) To: Mario Limonciello, Geert Uytterhoeven Cc: Jakub Kicinski, Krzysztof Kozlowski, netdev, workflows, linux-kernel On Tue, 2023-07-25 at 08:19 -0500, Mario Limonciello wrote: > On 7/25/23 02:33, Geert Uytterhoeven wrote: > > Hi Joe, > > > > On Tue, Jul 25, 2023 at 6:22 AM Joe Perches <joe@perches.com> wrote: > > > I do suggest you instead write wrapper scripts to get > > > the output you want rather than updating the defaults > > > for the script and update the process documentation > > > to let other people know what do to as well. > > > > > > Something akin to Mario Limonciello's suggestion back in 2022: > > > > > > https://lore.kernel.org/lkml/20220617183215.25917-1-mario.limonciello@amd.com/ > > > > FTR, this is more or less what I am using to generate a script > > to send out patches: > > > > OUT=... > > echo git send-email \\ > $OUT > > # Add -cc > > # Wrap comment inside $(: ...) > > # Replace (...) in comment by [...] > > # Replace ] at EOL by ) again > > # Add continuation to EOL > > scripts/get_maintainer.pl $* | \ > > tr -d \" | \ > > sed -e 's/^/--cc "/' \ > > -e 's/ (/" $(: /' \ > > -e 's/ (/ [/' -e 's/)/]/' \ > > -e 's/]$/)/' \ > > -e 's/$/ \\/' | \ > > tee -a $OUT > > echo "*[0-9][0-9][0-9][0-9]-*.*" >> $OUT > > > > After generation, I edit the script to > > - Replace some --cc by --to, > > - Add/remove some people, > > and run "source $OUT" to send the patches... > > > > Gr{oetje,eeting}s, > > My script is great for single subsystem patches as it gets all the right > people but I've found problems whenever it crosses multiple subsystems. > > Many subsystem owners want to see the whole series of patches to > understand how they interact. So the group of patches needs to be > treated together which would need the wrapper to look at all patches > instead. Which can't really work all the time as vger has a recipient limit and subsystem spanning patches frequently exceed that limit. bcc's don't work well either as the reply-to chain is broken. No great solution to that. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload 2023-07-25 13:43 ` Joe Perches @ 2023-07-25 14:37 ` Krzysztof Kozlowski 2023-07-25 15:59 ` [PATCH] scripts: checkpatch: steer people away from using file paths Jakub Kicinski 0 siblings, 1 reply; 16+ messages in thread From: Krzysztof Kozlowski @ 2023-07-25 14:37 UTC (permalink / raw) To: Joe Perches, Mario Limonciello, Geert Uytterhoeven Cc: Jakub Kicinski, netdev, workflows, linux-kernel On 25/07/2023 15:43, Joe Perches wrote: > On Tue, 2023-07-25 at 08:19 -0500, Mario Limonciello wrote: >> On 7/25/23 02:33, Geert Uytterhoeven wrote: >>> Hi Joe, >>> >>> On Tue, Jul 25, 2023 at 6:22 AM Joe Perches <joe@perches.com> wrote: >>>> I do suggest you instead write wrapper scripts to get >>>> the output you want rather than updating the defaults >>>> for the script and update the process documentation >>>> to let other people know what do to as well. >>>> >>>> Something akin to Mario Limonciello's suggestion back in 2022: >>>> >>>> https://lore.kernel.org/lkml/20220617183215.25917-1-mario.limonciello@amd.com/ >>> >>> FTR, this is more or less what I am using to generate a script >>> to send out patches: >>> >>> OUT=... >>> echo git send-email \\ > $OUT >>> # Add -cc >>> # Wrap comment inside $(: ...) >>> # Replace (...) in comment by [...] >>> # Replace ] at EOL by ) again >>> # Add continuation to EOL >>> scripts/get_maintainer.pl $* | \ >>> tr -d \" | \ >>> sed -e 's/^/--cc "/' \ >>> -e 's/ (/" $(: /' \ >>> -e 's/ (/ [/' -e 's/)/]/' \ >>> -e 's/]$/)/' \ >>> -e 's/$/ \\/' | \ >>> tee -a $OUT >>> echo "*[0-9][0-9][0-9][0-9]-*.*" >> $OUT >>> >>> After generation, I edit the script to >>> - Replace some --cc by --to, >>> - Add/remove some people, >>> and run "source $OUT" to send the patches... >>> >>> Gr{oetje,eeting}s, >> >> My script is great for single subsystem patches as it gets all the right >> people but I've found problems whenever it crosses multiple subsystems. >> >> Many subsystem owners want to see the whole series of patches to >> understand how they interact. So the group of patches needs to be >> treated together which would need the wrapper to look at all patches >> instead. > > Which can't really work all the time as vger has a recipient limit > and subsystem spanning patches frequently exceed that limit. > > bcc's don't work well either as the reply-to chain is broken. > > No great solution to that. > For small patchsets (and recipients list) I recommend: https://github.com/krzk/tools/blob/master/linux/.bash_aliases_linux#L91 For bigger patchsets - Rob's sendemail identity could work: https://lore.kernel.org/all/CAL_JsqLubWBr2W3xZPsuPLOGav7CFgBdH=aCfT22F_m0_cx3cQ@mail.gmail.com/ but cover letter has to be treated separately. Anyway, it is not the case here. This is small patchset and the submitter should run get_maintainers.pl on *the patchset*, not on one chosen file. Running it one one file, ignoring maintainers of all other patches, does not make sense. There is nothing to fix in get_maintainers.pl. I believe our docs are also correct here. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] scripts: checkpatch: steer people away from using file paths 2023-07-25 14:37 ` Krzysztof Kozlowski @ 2023-07-25 15:59 ` Jakub Kicinski 2023-07-25 16:53 ` Greg KH ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Jakub Kicinski @ 2023-07-25 15:59 UTC (permalink / raw) To: krzk; +Cc: Jakub Kicinski, joe, geert, netdev, workflows, mario.limonciello 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(-) 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. -- 2.41.0 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] scripts: checkpatch: steer people away from using file paths 2023-07-25 15:59 ` [PATCH] scripts: checkpatch: steer people away from using file paths Jakub Kicinski @ 2023-07-25 16:53 ` Greg KH 2023-07-25 17:10 ` Jakub Kicinski 2023-07-25 16:57 ` Krzysztof Kozlowski 2023-07-25 21:18 ` Joe Perches 2 siblings, 1 reply; 16+ messages in thread From: Greg KH @ 2023-07-25 16:53 UTC (permalink / raw) To: Jakub Kicinski; +Cc: krzk, joe, geert, netdev, workflows, mario.limonciello 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] scripts: checkpatch: steer people away from using file paths 2023-07-25 16:53 ` Greg KH @ 2023-07-25 17:10 ` Jakub Kicinski 2023-07-25 17:25 ` Greg KH 0 siblings, 1 reply; 16+ messages in thread From: Jakub Kicinski @ 2023-07-25 17:10 UTC (permalink / raw) To: Greg KH; +Cc: krzk, joe, geert, netdev, workflows, mario.limonciello 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 :) ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] scripts: checkpatch: steer people away from using file paths 2023-07-25 17:10 ` Jakub Kicinski @ 2023-07-25 17:25 ` Greg KH 2023-07-25 19:52 ` Jakub Kicinski 0 siblings, 1 reply; 16+ messages in thread From: Greg KH @ 2023-07-25 17:25 UTC (permalink / raw) To: Jakub Kicinski; +Cc: krzk, joe, geert, netdev, workflows, mario.limonciello 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] scripts: checkpatch: steer people away from using file paths 2023-07-25 17:25 ` Greg KH @ 2023-07-25 19:52 ` Jakub Kicinski 2023-07-25 21:01 ` Joe Perches 0 siblings, 1 reply; 16+ messages in thread From: Jakub Kicinski @ 2023-07-25 19:52 UTC (permalink / raw) To: Greg KH; +Cc: krzk, joe, geert, netdev, workflows, mario.limonciello 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... ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] scripts: checkpatch: steer people away from using file paths 2023-07-25 19:52 ` Jakub Kicinski @ 2023-07-25 21:01 ` Joe Perches 0 siblings, 0 replies; 16+ messages in thread From: Joe Perches @ 2023-07-25 21:01 UTC (permalink / raw) To: Jakub Kicinski, Greg KH; +Cc: krzk, geert, netdev, workflows, mario.limonciello 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] scripts: checkpatch: steer people away from using file paths 2023-07-25 15:59 ` [PATCH] scripts: checkpatch: steer people away from using file paths Jakub Kicinski 2023-07-25 16:53 ` Greg KH @ 2023-07-25 16:57 ` Krzysztof Kozlowski 2023-07-25 21:18 ` Joe Perches 2 siblings, 0 replies; 16+ messages in thread From: Krzysztof Kozlowski @ 2023-07-25 16:57 UTC (permalink / raw) To: Jakub Kicinski; +Cc: joe, geert, netdev, workflows, mario.limonciello 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] scripts: checkpatch: steer people away from using file paths 2023-07-25 15:59 ` [PATCH] scripts: checkpatch: steer people away from using file paths Jakub Kicinski 2023-07-25 16:53 ` Greg KH 2023-07-25 16:57 ` Krzysztof Kozlowski @ 2023-07-25 21:18 ` Joe Perches 2023-07-25 22:15 ` Jakub Kicinski 2 siblings, 1 reply; 16+ messages in thread From: Joe Perches @ 2023-07-25 21:18 UTC (permalink / raw) To: Jakub Kicinski, krzk; +Cc: geert, netdev, workflows, mario.limonciello 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] scripts: checkpatch: steer people away from using file paths 2023-07-25 21:18 ` Joe Perches @ 2023-07-25 22:15 ` Jakub Kicinski 2023-07-26 6:28 ` Joe Perches 0 siblings, 1 reply; 16+ messages in thread From: Jakub Kicinski @ 2023-07-25 22:15 UTC (permalink / raw) To: Joe Perches; +Cc: krzk, geert, netdev, workflows, mario.limonciello 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] scripts: checkpatch: steer people away from using file paths 2023-07-25 22:15 ` Jakub Kicinski @ 2023-07-26 6:28 ` Joe Perches 0 siblings, 0 replies; 16+ messages in thread From: Joe Perches @ 2023-07-26 6:28 UTC (permalink / raw) To: Jakub Kicinski; +Cc: krzk, geert, netdev, workflows, mario.limonciello 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2023-07-26 6:28 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20230721062617.9810-1-boon.khai.ng@intel.com>
[not found] ` <20230721062617.9810-2-boon.khai.ng@intel.com>
[not found] ` <e552cea3-abbb-93e3-4167-aebe979aac6b@kernel.org>
[not found] ` <DM8PR11MB5751EAB220E28AECF6153522C13FA@DM8PR11MB5751.namprd11.prod.outlook.com>
[not found] ` <8e2f9c5f-6249-4325-58b2-a14549eb105d@kernel.org>
[not found] ` <20230721185557.199fb5b8@kernel.org>
[not found] ` <c690776ce6fd247c2b2aeb805744d5779b6293ab.camel@perches.com>
2023-07-25 1:04 ` [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload Jakub Kicinski
2023-07-25 3:53 ` Joe Perches
2023-07-25 7:33 ` Geert Uytterhoeven
2023-07-25 13:19 ` Mario Limonciello
2023-07-25 13:43 ` Joe Perches
2023-07-25 14:37 ` Krzysztof Kozlowski
2023-07-25 15:59 ` [PATCH] scripts: checkpatch: steer people away from using file paths Jakub Kicinski
2023-07-25 16:53 ` Greg KH
2023-07-25 17:10 ` Jakub Kicinski
2023-07-25 17:25 ` Greg KH
2023-07-25 19:52 ` Jakub Kicinski
2023-07-25 21:01 ` Joe Perches
2023-07-25 16:57 ` Krzysztof Kozlowski
2023-07-25 21:18 ` Joe Perches
2023-07-25 22:15 ` Jakub Kicinski
2023-07-26 6:28 ` Joe Perches
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox