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

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

[Ujjwal Kumar]

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.

Signed-off-by: Ujjwal Kumar <ujjwalkumar0501@gmail.com>
---
Changes in v2:
  - Spelling correction and add example to commit
    message
  - Code style changes
  - Remove unncessary function argument
  - Use non-capturing group in regexp

 scripts/checkpatch.pl | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index fab38b493cef..7ebbee9c3672 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1795,6 +1795,23 @@ sub get_stat_here {
 	return $herectx;
 }

+sub get_shebang {
+	my ($linenr) = @_;
+	my $rawline = "";
+	my $shebang = "";
+
+	$rawline = raw_line($linenr, 3);
+	if (defined($rawline) &&
+	    $rawline =~ /^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@/) {
+		if (defined($1) && $1 == 1) {
+			$shebang = raw_line($linenr, 4);
+			$shebang = substr($shebang, 1);
+		}
+	}
+
+	return $shebang;
+}
+
 sub cat_vet {
 	my ($vet) = @_;
 	my ($res, $coded);
@@ -2680,7 +2697,9 @@ sub process {
 # Check for incorrect file permissions
 		if ($line =~ /^new (file )?mode.*[7531]\d{0,2}$/) {
 			my $permhere = $here . "FILE: $realfile\n";
+			my $shebang = get_shebang($linenr);
 			if ($realfile !~ m@scripts/@ &&
+			    $shebang !~ /^#!\s*(?:\/\w)+.*/ &&
 			    $realfile !~ /\.(py|pl|awk|sh)$/) {
 				ERROR("EXECUTE_PERMISSIONS",
 				      "do not set execute permissions for source files\n" . $permhere);

base-commit: 148fdf990dee4efd23c1114811b205de9c966680
--
2.26.2

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

[Ujjwal Kumar]

On 13/10/20 5:31 pm, 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.
> 
> Signed-off-by: Ujjwal Kumar <ujjwalkumar0501@gmail.com>
> ---
> Changes in v2:
>   - Spelling correction and add example to commit
>     message
>   - Code style changes
>   - Remove unncessary function argument
>   - Use non-capturing group in regexp
> 
>  scripts/checkpatch.pl | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index fab38b493cef..7ebbee9c3672 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1795,6 +1795,23 @@ sub get_stat_here {
>  	return $herectx;
>  }
> 
> +sub get_shebang {
> +	my ($linenr) = @_;
> +	my $rawline = "";
> +	my $shebang = "";
> +
> +	$rawline = raw_line($linenr, 3);

I'm wondering if the range information can be at a
different offset from the 'new mode line'.

> +	if (defined($rawline) &&
> +	    $rawline =~ /^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@/) {
> +		if (defined($1) && $1 == 1) {
> +			$shebang = raw_line($linenr, 4);
> +			$shebang = substr($shebang, 1);
> +		}
> +	}
> +
> +	return $shebang;
> +}
> +
>  sub cat_vet {
>  	my ($vet) = @_;
>  	my ($res, $coded);
> @@ -2680,7 +2697,9 @@ sub process {
>  # Check for incorrect file permissions
>  		if ($line =~ /^new (file )?mode.*[7531]\d{0,2}$/) {
>  			my $permhere = $here . "FILE: $realfile\n";
> +			my $shebang = get_shebang($linenr);
>  			if ($realfile !~ m@scripts/@ &&
> +			    $shebang !~ /^#!\s*(?:\/\w)+.*/ &&
>  			    $realfile !~ /\.(py|pl|awk|sh)$/) {

Consider the following case:
a python script file with '.py' filename extension but without
a shebang line. Would it be meaningful to allow execute permission
on such a file?

>  				ERROR("EXECUTE_PERMISSIONS",
>  				      "do not set execute permissions for source files\n" . $permhere);
> 
> base-commit: 148fdf990dee4efd23c1114811b205de9c966680
> --
> 2.26.2
> 

Thanks
Ujjwal Kumar

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

[Joe Perches]

On Tue, 2020-10-13 at 17:43 +0530, Ujjwal Kumar wrote:
> Consider the following case:
> a python script file with '.py' filename extension but without
> a shebang line. Would it be meaningful to allow execute permission
> on such a file?

More the question I think is for a patch to
that file, how do you tell if it has one?

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

[Lukas Bulwahn]

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

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

[Joe Perches]

On Wed, 2020-10-14 at 07:46 +0200, Lukas Bulwahn wrote:
> 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.

Eek no.

Mindless use of either tool isn't a great thing.

Linux source code has generally be created with
human readability in mind by humans, not scripts.

Please don't try to replace human readable code
with mindless tools.

If there's something inappropriate in checkpatch,
please mention it.

There is a _lot_ of relatively inappropriate
output in how clang-format changes existing code
in the kernel.

Try it and look at the results.

Improving how .clang-format is created and its
mechanisms (for example: continually out of date
ForEachMacros lists) could be reasonably be done.

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

[Lukas Bulwahn]

On Tue, 13 Oct 2020, Joe Perches wrote:

> On Wed, 2020-10-14 at 07:46 +0200, Lukas Bulwahn wrote:
> > 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.
> 
> Eek no.
> 
> Mindless use of either tool isn't a great thing.
>

I did not suggest applying the tool to the source code and just changing 
the code... that is not a good idea as it is not helping anyone and just 
causing churn and distraction.
  
> Linux source code has generally be created with
> human readability in mind by humans, not scripts.
> 
> Please don't try to replace human readable code
> with mindless tools.
>

The goal is to run both tools on the code base and with the comparison see
how both tools can be improved.

We basically assume that the code is in the style it is intended to be.
What does checkpatch.pl warn about and what does clang-format still warn 
about, which is generally accepted okay as style in the kernel?
 
Then, we can improve the checkpatch and clang-format rules.

> If there's something inappropriate in checkpatch,
> please mention it.
> 
> There is a _lot_ of relatively inappropriate
> output in how clang-format changes existing code
> in the kernel.
>

Agree.
 
> Try it and look at the results.
>

Agree, that was the proposal. Nothing else.
 
> Improving how .clang-format is created and its
> mechanisms (for example: continually out of date
> ForEachMacros lists) could be reasonably be done.
> 
>

And that is something I would hope that somebody looking at the results 
would spot and start improving.

Lukas

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

[Joe Perches]

On Wed, 2020-10-14 at 08:21 +0200, Lukas Bulwahn wrote:
> What does checkpatch.pl warn about and what does clang-format still warn 
> about, which is generally accepted okay as style in the kernel?

clang-format doesn't warn at all, it just reformats.

checkpatch using the --in-place can reformat a
patch or file with an explanation of each change
it makes.

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

[Lukas Bulwahn]

On Tue, 13 Oct 2020, Joe Perches wrote:

> On Wed, 2020-10-14 at 08:21 +0200, Lukas Bulwahn wrote:
> > What does checkpatch.pl warn about and what does clang-format still warn 
> > about, which is generally accepted okay as style in the kernel?
> 
> clang-format doesn't warn at all, it just reformats.
>

You can run clang-format with --dry-run and then it would just state the 
proposed changes, right?

I am not the clang-format expert, though.

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

[Joe Perches]

On Wed, 2020-10-14 at 08:36 +0200, Lukas Bulwahn wrote:
> 
> On Tue, 13 Oct 2020, Joe Perches wrote:
> 
> > On Wed, 2020-10-14 at 08:21 +0200, Lukas Bulwahn wrote:
> > > What does checkpatch.pl warn about and what does clang-format still warn 
> > > about, which is generally accepted okay as style in the kernel?
> > 
> > clang-format doesn't warn at all, it just reformats.
> > 
> You can run clang-format with --dry-run and then it would just state the 
> proposed changes, right?

clang-format through at least version 10 does not have
a --dry-run option.

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

[Lukas Bulwahn]

On Tue, 13 Oct 2020, Joe Perches wrote:

> On Wed, 2020-10-14 at 08:36 +0200, Lukas Bulwahn wrote:
> > 
> > On Tue, 13 Oct 2020, Joe Perches wrote:
> > 
> > > On Wed, 2020-10-14 at 08:21 +0200, Lukas Bulwahn wrote:
> > > > What does checkpatch.pl warn about and what does clang-format still warn 
> > > > about, which is generally accepted okay as style in the kernel?
> > > 
> > > clang-format doesn't warn at all, it just reformats.
> > > 
> > You can run clang-format with --dry-run and then it would just state the 
> > proposed changes, right?
> 
> clang-format through at least version 10 does not have
> a --dry-run option.
> 
>

Just a quick check:

version 9 does not have the --dry-run option:

https://releases.llvm.org/9.0.0/tools/clang/docs/ClangFormat.html

version 10 does:

https://releases.llvm.org/10.0.0/tools/clang/docs/ClangFormat.html

and version 12 (under development) does keep it:

https://clang.llvm.org/docs/ClangFormat.html

I have not played around with that, though.

Lukas 

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

[Joe Perches]

On Wed, 2020-10-14 at 08:47 +0200, Lukas Bulwahn wrote:
> 
> On Tue, 13 Oct 2020, Joe Perches wrote:
> 
> > On Wed, 2020-10-14 at 08:36 +0200, Lukas Bulwahn wrote:
> > > On Tue, 13 Oct 2020, Joe Perches wrote:
> > > 
> > > > On Wed, 2020-10-14 at 08:21 +0200, Lukas Bulwahn wrote:
> > > > > What does checkpatch.pl warn about and what does clang-format still warn 
> > > > > about, which is generally accepted okay as style in the kernel?
> > > > 
> > > > clang-format doesn't warn at all, it just reformats.
> > > > 
> > > You can run clang-format with --dry-run and then it would just state the 
> > > proposed changes, right?
> > 
> > clang-format through at least version 10 does not have
> > a --dry-run option.
> > 
> > 
> 
> Just a quick check:
> 
> version 9 does not have the --dry-run option:
> 
> https://releases.llvm.org/9.0.0/tools/clang/docs/ClangFormat.html
> 
> version 10 does:
> 
> https://releases.llvm.org/10.0.0/tools/clang/docs/ClangFormat.html

Perhaps some version 10 variants do, but 10.0.0 does not.

$ which clang-format
/usr/local/bin/clang-format
$ clang-format --version
clang-format version 10.0.0 (git://github.com/llvm/llvm-project.git 305b961f64b75e73110e309341535f6d5a48ed72)
$ clang-format --dry-run
clang-format: Unknown command line argument '--dry-run'.  Try: 'clang-format --help'
clang-format: Did you mean '  --debug'?

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

[Lukas Bulwahn]

On Tue, 13 Oct 2020, Joe Perches wrote:

> On Wed, 2020-10-14 at 08:47 +0200, Lukas Bulwahn wrote:
> > 
> > On Tue, 13 Oct 2020, Joe Perches wrote:
> > 
> > > On Wed, 2020-10-14 at 08:36 +0200, Lukas Bulwahn wrote:
> > > > On Tue, 13 Oct 2020, Joe Perches wrote:
> > > > 
> > > > > On Wed, 2020-10-14 at 08:21 +0200, Lukas Bulwahn wrote:
> > > > > > What does checkpatch.pl warn about and what does clang-format still warn 
> > > > > > about, which is generally accepted okay as style in the kernel?
> > > > > 
> > > > > clang-format doesn't warn at all, it just reformats.
> > > > > 
> > > > You can run clang-format with --dry-run and then it would just state the 
> > > > proposed changes, right?
> > > 
> > > clang-format through at least version 10 does not have
> > > a --dry-run option.
> > > 
> > > 
> > 
> > Just a quick check:
> > 
> > version 9 does not have the --dry-run option:
> > 
> > https://releases.llvm.org/9.0.0/tools/clang/docs/ClangFormat.html
> > 
> > version 10 does:
> > 
> > https://releases.llvm.org/10.0.0/tools/clang/docs/ClangFormat.html
> 
> Perhaps some version 10 variants do, but 10.0.0 does not.
> 
> $ which clang-format
> /usr/local/bin/clang-format
> $ clang-format --version
> clang-format version 10.0.0 (git://github.com/llvm/llvm-project.git 305b961f64b75e73110e309341535f6d5a48ed72)
> $ clang-format --dry-run
> clang-format: Unknown command line argument '--dry-run'.  Try: 'clang-format --help'
> clang-format: Did you mean '  --debug'?
>

Hmm... either the documentation is wrong; or the clang-format version 
10.0.0 you are was an early version 10 during development before the 
release and did not have that feature yet? 

$ clang-format-10 --version
Ubuntu clang-format version 
10.0.1-++20200928083909+ef32c611aa2-1~exp1~20200928185400.194

$ clang-format-10 --help | grep 'dry-run'
  --dry-run                  - If set, do not actually make the formatting 
changes
  --ferror-limit=<uint>      - Set the maximum number of clang-format
    errors to emit before stopping (0 = no limit). Used only with --dry-run or -n
  -n                         - Alias for --dry-run


You have probably seen that clang/llvm-11 was released; I guess a good 
motivation for us to update our clang setup? :)

Lukas

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

[Joe Perches]

On Wed, 2020-10-14 at 09:17 +0200, Lukas Bulwahn wrote:
> $ clang-format-10 --version
> Ubuntu clang-format version 
> 10.0.1-++20200928083909+ef32c611aa2-1~exp1~20200928185400.194
> 
> $ clang-format-10 --help | grep 'dry-run'
>   --dry-run                  - If set, do not actually make the formatting 
> changes
>   --ferror-limit=<uint>      - Set the maximum number of clang-format
>     errors to emit before stopping (0 = no limit). Used only with --dry-run or -n
>   -n                         - Alias for --dry-run

OK, maybe so.

However I think the clang-format --dry-run output doesn't
currently contain particularly useful information.  Maybe
later versions are better than version 10.

For instance:

$ /usr/bin/clang-format-10 --version
clang-format version 10.0.0-4ubuntu1 

$ /usr/bin/clang-format-10 --dry-run drivers/net/ethernet/intel/igb/igb_main.c 2>&1 | head -25
drivers/net/ethernet/intel/igb/igb_main.c:54:40: warning: code should be clang-formatted [-Wclang-format-violations]
static const char igb_driver_string[] =
                                       ^
drivers/net/ethernet/intel/igb/igb_main.c:56:36: warning: code should be clang-formatted [-Wclang-format-violations]
static const char igb_copyright[] =
                                   ^
drivers/net/ethernet/intel/igb/igb_main.c:100:3: warning: code should be clang-formatted [-Wclang-format-violations]
        {0, }
         ^
drivers/net/ethernet/intel/igb/igb_main.c:100:5: warning: code should be clang-formatted [-Wclang-format-violations]
        {0, }
           ^
drivers/net/ethernet/intel/igb/igb_main.c:163:58: warning: code should be clang-formatted [-Wclang-format-violations]
static int igb_ndo_set_vf_vlan(struct net_device *netdev,
                                                         ^
drivers/net/ethernet/intel/igb/igb_main.c:164:28: warning: code should be clang-formatted [-Wclang-format-violations]
                               int vf, u16 vlan, u8 qos, __be16 vlan_proto);
                                                ^
drivers/net/ethernet/intel/igb/igb_main.c:189:50: warning: code should be clang-formatted [-Wclang-format-violations]
        SET_SYSTEM_SLEEP_PM_OPS(igb_suspend, igb_resume)
                                                        ^
drivers/net/ethernet/intel/igb/igb_main.c:190:21: warning: code should be clang-formatted [-Wclang-format-violations]
        SET_RUNTIME_PM_OPS(igb_runtime_suspend, igb_runtime_resume,
                           ^
drivers/net/ethernet/intel/igb/igb_main.c:190:61: warning: code should be clang-formatted [-Wclang-format-violations]

vs

$ ./scripts/checkpatch.pl -f drivers/net/ethernet/intel/igb/igb_main.c 2>&1 | head -25
WARNING: externs should be avoided in .c files
#113: FILE: drivers/net/ethernet/intel/igb/igb_main.c:113:
+int igb_open(struct net_device *);

WARNING: function definition argument 'struct net_device *' should also have an identifier name
#113: FILE: drivers/net/ethernet/intel/igb/igb_main.c:113:
+int igb_open(struct net_device *);

WARNING: externs should be avoided in .c files
#114: FILE: drivers/net/ethernet/intel/igb/igb_main.c:114:
+int igb_close(struct net_device *);

WARNING: function definition argument 'struct net_device *' should also have an identifier name
#114: FILE: drivers/net/ethernet/intel/igb/igb_main.c:114:
+int igb_close(struct net_device *);

CHECK: Alignment should match open parenthesis
#191: FILE: drivers/net/ethernet/intel/igb/igb_main.c:191:
+	SET_RUNTIME_PM_OPS(igb_runtime_suspend, igb_runtime_resume,
+			igb_runtime_idle)

CHECK: Please use a blank line after function/struct/union/enum declarations
#193: FILE: drivers/net/ethernet/intel/igb/igb_main.c:193:
+};
+static void igb_shutdown(struct pci_dev *);

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

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

[Miguel Ojeda]

On Wed, Oct 14, 2020 at 10:40 AM Joe Perches <joe@perches.com> wrote:
>
> Eek no.
>
> Mindless use of either tool isn't a great thing.

That is up to opinion. I (and others) definitely want to get to the
point the kernel sources are automatically formatted, because it has
significant advantages. The biggest is that it saves a lot of time for
everyone involved.

> Linux source code has generally be created with
> human readability in mind by humans, not scripts.

Code readability is by definition for humans, and any code formatter
(like clang-format) is written for them.

> Please don't try to replace human readable code
> with mindless tools.

Please do :-)

> There is a _lot_ of relatively inappropriate
> output in how clang-format changes existing code
> in the kernel.

Sure, but note that:

  - Code that should be specially-formatted should be in a
clang-format-off section to begin with, so it doesn't count.

  - Some people like to tweak identifiers or refactor code to make it
fit in the line limit beautifully. If you are doing that, then you
should do that for clang-format too. It is not fair to compare both
outputs when the tool is restricted on the transformations it can
perform. You can help the tool, too, the same way you can help
yourself.

  - Some style differences may be worth ignoring if the advantage is
getting automatic formatting.

Cheers,
Miguel

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

[Joe Perches]

On Wed, 2020-10-14 at 19:45 +0200, Miguel Ojeda wrote:
>   - Code that should be specially-formatted should be in a
> clang-format-off section to begin with, so it doesn't count.

clang-format is not the end-all tool.
Any 'formatting off/on' marker should be tool agnostic.

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

[Miguel Ojeda]

On Wed, Oct 14, 2020 at 8:05 PM Joe Perches <joe@perches.com> wrote:
>
> Any 'formatting off/on' marker should be tool agnostic.

Agreed, they should have used a compiler-agnostic name for the marker.

Cheers,
Miguel

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

[Joe Perches]

On Wed, 2020-10-14 at 20:32 +0200, Miguel Ojeda wrote:
> On Wed, Oct 14, 2020 at 8:05 PM Joe Perches <joe@perches.com> wrote:
> > Any 'formatting off/on' marker should be tool agnostic.
> 
> Agreed, they should have used a compiler-agnostic name for the marker.

It means to me that linux has to invent one and any
clang-format use has to use an equivalent of the sphinx
.rst macro conversion scripts pre and post format.