Re: [Linux-kernel-mentees] [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS

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

On Tue, 13 Oct 2020, Ujjwal Kumar wrote:

> checkpatch.pl checks for invalid EXECUTE_PERMISSIONS on source
> files. The script leverages filename extensions and its path in
> the repository to decide whether to allow execute permissions on
> the file or not.
> 
> Based on current check conditions, a perl script file having
> execute permissions, without '.pl' extension in its filename
> and not belonging to 'scripts/' directory is reported as ERROR
> which is a false positive.
> 
> Adding a shebang check along with current conditions will make
> the check more generalised and improve checkpatch reports.
> To do so, without breaking the core design decision of checkpatch,
> we can fetch the first line from the patch itself and match it for
> a shebang pattern.
> 
> There can be cases where the first line is not part of the patch.
> For instance: a patch that only changes permissions without
> changing any of the file content.
> In that case there may be a false positive report but in the end we
> will have less false positives as we will be handling some of the
> unhandled cases.
>

I get the intent of your addition. However:

I would bet that you only find one or two in a million commits, that would 
actually benefit for this special check of a special case of a special 
rule...

So given the added complexity of yet another 19 lines in checkpatch with 
little benefit of lowering false positive reports, I would be against 
inclusion.

You can provide convincing arguments with an evaluation, where you show 
on how many commits this change would really make a difference...

It is probably better and simpler to just have a script checking for
execute bits on all files in the repository on linux-next (with a list of 
known intended executable files) and just report to you and then to the 
developers when a new file with unintentional execute bit appeared.

Keep up the good work. I just fear this patch is a dead end.

There is still a lot of other issues you can contribute to.

Just one bigger project example: Comparing clang-format suggestions on 
patches against checkpatch.pl suggestions are fine-tuning both of them to fit to 
the actual kernel style.

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