[PATCH v3] checkpatch: add fix option for MAINTAINERS_STYLE

[PATCH v3] checkpatch: add fix option for MAINTAINERS_STYLE

[Aditya Srivastava]

Checkpatch expects entries in MAINTAINERS file in a specific order and
warns if the changes made do not follow the specified order.

E.g., running checkpatch on commit b33bc2b878e0 ("nexthop: Add entry to
MAINTAINERS") reports this warning:

WARNING: Misordered MAINTAINERS entry - list file patterns in
alphabetic order
+F:	include/uapi/linux/nexthop.h
+F:	include/net/netns/nexthop.h

Provide a simple fix by swapping the unordered lines, if both the lines
are additions (start with '+')

Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
---
Changes in v2:
modified commit message

Changes in v3:
add check if both the lines are additions(ie start with '+')

 scripts/checkpatch.pl | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2749f32dffe9..7ee3f05c354d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3299,13 +3299,22 @@ sub process {
 					     "Unknown MAINTAINERS entry type: '$cur'\n" . $herecurr);
 				} else {
 					if ($previndex >= 0 && $curindex < $previndex) {
-						WARN("MAINTAINERS_STYLE",
-						     "Misordered MAINTAINERS entry - list '$cur:' before '$prev:'\n" . $hereprev);
+						if (WARN("MAINTAINERS_STYLE",
+							 "Misordered MAINTAINERS entry - list '$cur:' before '$prev:'\n" . $hereprev) &&
+						    $fix && $prevrawline =~ /^\+[A-Z]:/) {
+							# swap these lines
+							$fixed[$fixlinenr - 1] = $rawline;
+							$fixed[$fixlinenr] = $prevrawline;
+						}
 					} elsif ((($prev eq 'F' && $cur eq 'F') ||
 						  ($prev eq 'X' && $cur eq 'X')) &&
 						 ($prevval cmp $curval) > 0) {
-						WARN("MAINTAINERS_STYLE",
-						     "Misordered MAINTAINERS entry - list file patterns in alphabetic order\n" . $hereprev);
+						if (WARN("MAINTAINERS_STYLE",
+							 "Misordered MAINTAINERS entry - list file patterns in alphabetic order\n" . $hereprev) &&
+						    $fix && $prevrawline =~ /^\+[A-Z]:/) {
+							$fixed[$fixlinenr - 1] = $rawline;
+							$fixed[$fixlinenr] = $prevrawline;
+						}
 					}
 				}
 			}
-- 
2.17.1

Re: [PATCH v3] checkpatch: add fix option for MAINTAINERS_STYLE

[Joe Perches]

On Tue, 2020-11-17 at 09:35 +0530, Aditya Srivastava wrote:
> Checkpatch expects entries in MAINTAINERS file in a specific order and
> warns if the changes made do not follow the specified order.
> 
> E.g., running checkpatch on commit b33bc2b878e0 ("nexthop: Add entry to
> MAINTAINERS") reports this warning:
> 
> WARNING: Misordered MAINTAINERS entry - list file patterns in
> alphabetic order
> +F:	include/uapi/linux/nexthop.h
> +F:	include/net/netns/nexthop.h

OK, this should work.
Thanks Aditya.

Acked-by: Joe Perches <joe@perches.com>

> 
> Provide a simple fix by swapping the unordered lines, if both the lines
> are additions (start with '+')
> 
> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> ---
> Changes in v2:
> modified commit message
> 
> Changes in v3:
> add check if both the lines are additions(ie start with '+')
> 
>  scripts/checkpatch.pl | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 2749f32dffe9..7ee3f05c354d 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3299,13 +3299,22 @@ sub process {
>  					     "Unknown MAINTAINERS entry type: '$cur'\n" . $herecurr);
>  				} else {
>  					if ($previndex >= 0 && $curindex < $previndex) {
> -						WARN("MAINTAINERS_STYLE",
> -						     "Misordered MAINTAINERS entry - list '$cur:' before '$prev:'\n" . $hereprev);
> +						if (WARN("MAINTAINERS_STYLE",
> +							 "Misordered MAINTAINERS entry - list '$cur:' before '$prev:'\n" . $hereprev) &&
> +						    $fix && $prevrawline =~ /^\+[A-Z]:/) {
> +							# swap these lines
> +							$fixed[$fixlinenr - 1] = $rawline;
> +							$fixed[$fixlinenr] = $prevrawline;
> +						}
>  					} elsif ((($prev eq 'F' && $cur eq 'F') ||
>  						  ($prev eq 'X' && $cur eq 'X')) &&
>  						 ($prevval cmp $curval) > 0) {
> -						WARN("MAINTAINERS_STYLE",
> -						     "Misordered MAINTAINERS entry - list file patterns in alphabetic order\n" . $hereprev);
> +						if (WARN("MAINTAINERS_STYLE",
> +							 "Misordered MAINTAINERS entry - list file patterns in alphabetic order\n" . $hereprev) &&
> +						    $fix && $prevrawline =~ /^\+[A-Z]:/) {
> +							$fixed[$fixlinenr - 1] = $rawline;
> +							$fixed[$fixlinenr] = $prevrawline;
> +						}
>  					}
>  				}
>  			}

Re: [PATCH v3] checkpatch: add fix option for MAINTAINERS_STYLE

[Joe Perches]

On Tue, 2020-11-17 at 09:35 +0530, Aditya Srivastava wrote:
> Checkpatch expects entries in MAINTAINERS file in a specific order and
> warns if the changes made do not follow the specified order.
> 
> E.g., running checkpatch on commit b33bc2b878e0 ("nexthop: Add entry to
> MAINTAINERS") reports this warning:
> 
> WARNING: Misordered MAINTAINERS entry - list file patterns in
> alphabetic order
> +F:	include/uapi/linux/nexthop.h
> +F:	include/net/netns/nexthop.h
> 
> Provide a simple fix by swapping the unordered lines, if both the lines
> are additions (start with '+')

On second thought, nak.

This fails when there are 3 consecutive misordered lines.

SECTION
F:	c
F:	b
F:	a

Re: [PATCH v3] checkpatch: add fix option for MAINTAINERS_STYLE

[Joe Perches]

On Mon, 2020-11-16 at 20:26 -0800, Joe Perches wrote:
> On Tue, 2020-11-17 at 09:35 +0530, Aditya Srivastava wrote:
> > Checkpatch expects entries in MAINTAINERS file in a specific order and
> > warns if the changes made do not follow the specified order.
> > 
> > E.g., running checkpatch on commit b33bc2b878e0 ("nexthop: Add entry to
> > MAINTAINERS") reports this warning:
> > 
> > WARNING: Misordered MAINTAINERS entry - list file patterns in
> > alphabetic order
> > +F:	include/uapi/linux/nexthop.h
> > +F:	include/net/netns/nexthop.h
> > 
> > Provide a simple fix by swapping the unordered lines, if both the lines
> > are additions (start with '+')
> 
> On second thought, nak.
> 
> This fails when there are 3 consecutive misordered lines.
> 
> SECTION
> F:	c
> F:	b
> F:	a
> 

btw:

scripts/parse-maintainers.pl already does this reordering properly so
this particular --fix addition isn't all that useful.

Re: [PATCH v3] checkpatch: add fix option for MAINTAINERS_STYLE

[Lukas Bulwahn]

On Tue, Nov 17, 2020 at 5:29 AM Joe Perches <joe@perches.com> wrote:
>
> On Mon, 2020-11-16 at 20:26 -0800, Joe Perches wrote:
> > On Tue, 2020-11-17 at 09:35 +0530, Aditya Srivastava wrote:
> > > Checkpatch expects entries in MAINTAINERS file in a specific order and
> > > warns if the changes made do not follow the specified order.
> > >
> > > E.g., running checkpatch on commit b33bc2b878e0 ("nexthop: Add entry to
> > > MAINTAINERS") reports this warning:
> > >
> > > WARNING: Misordered MAINTAINERS entry - list file patterns in
> > > alphabetic order
> > > +F: include/uapi/linux/nexthop.h
> > > +F: include/net/netns/nexthop.h
> > >
> > > Provide a simple fix by swapping the unordered lines, if both the lines
> > > are additions (start with '+')
> >
> > On second thought, nak.
> >
> > This fails when there are 3 consecutive misordered lines.
> >
> > SECTION
> > F:    c
> > F:    b
> > F:    a
> >
>
> btw:
>
> scripts/parse-maintainers.pl already does this reordering properly so
> this particular --fix addition isn't all that useful.
>

I think the real fix is to provide some more documentation on
scripts/parse-maintainers.pl that explains how to run this script when
an author hits the warning type in checkpatch.pl.

I see these follow-up patches:

  - some documentation on scripts/parse-maintainers.pl
  - a patch to checkpatch.pl that points out this documentation when
this warning occurs.
  - maybe: improve of scripts/parse-maintainers.pl to handle exactly
this use case of a patch author (assuming that the patch was just
created with git format-patch -1) and how to get the corrected diff
for this patch.
  - maybe: a patch to checkpatch.pl that can create the command for
scripts/parse-maintainers.pl for a specific patch and which then can
be added with git commit --amend or git commit && git rebase and
squashing that in.

I think once that is done and better understood, you can much better
judge if there is really a need for a fix in checkpatch.pl.

Lukas

Re: [PATCH v3] checkpatch: add fix option for MAINTAINERS_STYLE

[Aditya]

On 17/11/20 3:51 pm, Lukas Bulwahn wrote:
> On Tue, Nov 17, 2020 at 5:29 AM Joe Perches <joe@perches.com> wrote:
>>
>> On Mon, 2020-11-16 at 20:26 -0800, Joe Perches wrote:
>>> On Tue, 2020-11-17 at 09:35 +0530, Aditya Srivastava wrote:
>>>> Checkpatch expects entries in MAINTAINERS file in a specific order and
>>>> warns if the changes made do not follow the specified order.
>>>>
>>>> E.g., running checkpatch on commit b33bc2b878e0 ("nexthop: Add entry to
>>>> MAINTAINERS") reports this warning:
>>>>
>>>> WARNING: Misordered MAINTAINERS entry - list file patterns in
>>>> alphabetic order
>>>> +F: include/uapi/linux/nexthop.h
>>>> +F: include/net/netns/nexthop.h
>>>>
>>>> Provide a simple fix by swapping the unordered lines, if both the lines
>>>> are additions (start with '+')
>>>
>>> On second thought, nak.
>>>
>>> This fails when there are 3 consecutive misordered lines.
>>>
>>> SECTION
>>> F:    c
>>> F:    b
>>> F:    a
>>>
>>
>> btw:
>>
>> scripts/parse-maintainers.pl already does this reordering properly so
>> this particular --fix addition isn't all that useful.
>>
> 
> I think the real fix is to provide some more documentation on
> scripts/parse-maintainers.pl that explains how to run this script when
> an author hits the warning type in checkpatch.pl.
> 
> I see these follow-up patches:
> 
>   - some documentation on scripts/parse-maintainers.pl
>   - a patch to checkpatch.pl that points out this documentation when
> this warning occurs.
>   - maybe: improve of scripts/parse-maintainers.pl to handle exactly
> this use case of a patch author (assuming that the patch was just
> created with git format-patch -1) and how to get the corrected diff
> for this patch.
>   - maybe: a patch to checkpatch.pl that can create the command for
> scripts/parse-maintainers.pl for a specific patch and which then can
> be added with git commit --amend or git commit && git rebase and
> squashing that in.
> 
> I think once that is done and better understood, you can much better
> judge if there is really a need for a fix in checkpatch.pl.
> 

Okay thanks. Will check these out.

Thanks
Aditya

> Lukas
>