Re: [Linux-kernel-mentees] [PATCH] checkpatch: ignore files not following NETWORKING_BLOCK_COMMENT_STYLE

[Lukas Bulwahn <lukas.bulwahn@gmail.com>]

On Mon, Dec 21, 2020 at 4:29 PM Aditya <yashsri421@gmail.com> wrote:
>
> On 21/12/20 10:57 am, Lukas Bulwahn wrote:
> > On Sun, Dec 20, 2020 at 7:33 PM Aditya Srivastava <yashsri421@gmail.com> wrote:
> >>
> >> Currently checkpatch.pl gives warning for NETWORKING_BLOCK_COMMENT_STYLE
> >> for files in net/ and drivers/net which do not follow the networking
> >> comment style.
> >> But some of these files seem to follow the generic comment style instead
> >> of networking style and some rather mixed style of comment.
> >>
> >> For e.g., drivers/net/wireless/ralink/rt2x00 largely follows generic
> >> kernel comment style in spite of being inside drivers/net.
> >>
> >> Provide an ignore file(".networking_block_comment_styles.ignore"), where
> >> users can add the files they want to ignore this warning.
> >>
> >
> > I believe that is a really bad design decision here.
> >
> > Imagine that in one directory, the maintainer wants to adjust ten
> > rules for checkpatch.
> >
> > Are we going to implement this logic for those ten rules individually?
>
> No, only the files which are not following the networking comment
> style, and are inside net/ or drivers/net should be added. The files
> which are following the networking style already, need not add
> themselves anywhere.
>
> I feel, normally maintainer will want entire directory to follow the
> same style. So, they can just add their directory in the .ignore file
> and it will be ignored from the warning.
>

I understand that.

> > Is the maintainer going to add
> >
> > None of that is obviously acceptable: 1. because there are better
> > design decisions; 2. because tailoring checkpatch would simply not be
> > worth the cost of having all those individual files laying around in
> > the repository and the complication throughout the whole script.
> >
>
> We have earlier used a similar approach with ".get_maintainer.ignore",
> where the users add their signature to be ignored from being mentioned
> by "get_maintainers.pl" script.
> Link: https://lore.kernel.org/patchwork/patch/939769/

Yeah, I know that. It is known to be called the "Christoph file in the
root directory", because for years, it only contained Christoph as he
was the only person that requested that feature. It is a good example
of a terrible feature.

>
> We just need one file for our purpose (i.e., not necessarily .ignore
> file).
> Here I have used .ignore file in the root directory, so that it is
> easier for the maintainer to edit it and add their directory in it.
>

Do you know how many maintainers in this project exist?
For the kernel, this what you suggest just does not make sense...

> If not in the root directory(as the files look scattered), perhaps we
> can place it in script directory as .ignore or .txt file, or maybe
> just place these directories in the form of a hash in the script
> itself. But perhaps we'll need a list of directories to ignore and
> then we'll be able to form the hash.
>

Which problem are you trying to solve?

> > Also, you already recognized that there is already a feature to ignore
> > or select rules through command-line parameters.
> >
> > So, how about coming up with a proper .checkpatch config file format
> > and then use the already available feature to configure the checkpatch
> > run?
> >
>
> The user can add "--ignore NETWORKING_BLOCK_COMMENT_STYLE" line in
> their .checkpatch.conf file, and checkpatch will then ignore this
> class of warning.
>
> Somewhat similar to this repository here:
> https://github.com/openil/u-boot/blob/master/.checkpatch.conf
>
> But this file(".checkpatch.conf") is ignored by git and is not
> committed (as different users may require different configurations).
> So every user will have to add this line in their individual config files.
>

I do not get what you say. Isn't this just a configuration question?
You configure it to be part of the configuration.

> Also, if the user starts working on a different file, where the
> networking comment style is followed, he will again have to delete
> this configuration line from the config file, to get this warning.
>
> I'm not sure, if we can add directory/file specific configuration in
> .checkpatch.conf. But again, if it was possible, we'll be specifying
> individual files, and telling them to be avoided, similar to the
> current approach.
>

A suitable feature can be implemented.

Think about 100 people modifying the same file at the same time and
the problem this brings during merging.

Think about commits moving directories from one place to the other and
the effort of keeping this file correct.

I would suggest not to have any "checkpatch.config" at the root level
mentioning all the different directories, as you are suggesting.

I suggest instead:

Have multiple ".checkpatch.conf" files in the different directories
and let checkpatch determine the relevant rules that need to be
applied to a specific file by traverse along the file hierarchy.
Generalize the method to work for any checkpatch rule, not just the
block_comment_style rule here.

Feel free to submit your current proposal to Joe Perches and lkml if
you are convinced of your idea and willing to get more feedback
(probably just more rejection). I personally believe this will be
rejected by others as well.

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees