On 14/10/20 11:16 am, Lukas Bulwahn wrote:

On Tue, 13 Oct 2020, Ujjwal Kumar wrote:

quoted

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.

Yes, it is a subtle change.

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

Some statistics:

I aggregated commits which involved 'mode change' on script files.
Totaling to 478 (looked for logs of only executable files in the repo).

At current state,
checkpatch reports 26 ERRORS (false positives)
with 'hashbang' test we have 5 false positives.

Without 'scripts/' directory test, 
checkpatch reports 82 ERRORS (false positives)
with 'hashbang' test we have 35 false positives.

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

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