Re: [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS

[Ujjwal Kumar]

On 14/10/20 11:16 am, Lukas Bulwahn wrote:
> 
> 
> 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.

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