[PATCH] checkpatch: warn for p0 patch only if prefix is not b

[PATCH] checkpatch: warn for p0 patch only if prefix is not b

[Dafna Hirschfeld]

It might be that file 'b' happens to exit. In that
case, if the prefix is also 'b' (which is the
common case) we get the falsely warning:

patch prefix 'b' exists, appears to be a -p0 patch

So warn only if prefix is not 'b'

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 1784921c645d..72263b142e39 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2821,7 +2821,7 @@ sub process {
 			$in_commit_log = 0;
 
 			$p1_prefix = $1;
-			if (!$file && $tree && $p1_prefix ne '' &&
+			if (!$file && $tree && $p1_prefix ne '' && $p1_prefix ne 'b' &&
 			    -e "$root/$p1_prefix") {
 				WARN("PATCH_PREFIX",
 				     "patch prefix '$p1_prefix' exists, appears to be a -p0 patch\n");
-- 
2.17.1

Re: [PATCH] checkpatch: warn for p0 patch only if prefix is not b

[Lukas Bulwahn]

Dafna,

On Mon, Jan 10, 2022 at 9:06 PM Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> wrote:
>
> It might be that file 'b' happens to exit. In that
> case, if the prefix is also 'b' (which is the
> common case) we get the falsely warning:
>
> patch prefix 'b' exists, appears to be a -p0 patch
>
> So warn only if prefix is not 'b'
>

The checkpatch script that is maintained here is really only intended
for its use in the kernel development. You may use checkpatch anywhere
else, but any changes that increase complexity for those other use
cases is really difficult to argue for inclusion in the kernel
repository. The checkpatch script currently is already large and
complex enough and all rules need to be understood as rough
heuristics, not as strict rules.

So, can you point to a kernel repository where there is actually a
file 'b' included? On a quick scan, I could not find a file 'b' in the
current trees of the repositories on my machine.

I am just letting you know about what I have observed; I do not decide
on the inclusion of this patch, though.

Lukas

> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  scripts/checkpatch.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 1784921c645d..72263b142e39 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2821,7 +2821,7 @@ sub process {
>                         $in_commit_log = 0;
>
>                         $p1_prefix = $1;
> -                       if (!$file && $tree && $p1_prefix ne '' &&
> +                       if (!$file && $tree && $p1_prefix ne '' && $p1_prefix ne 'b' &&
>                             -e "$root/$p1_prefix") {
>                                 WARN("PATCH_PREFIX",
>                                      "patch prefix '$p1_prefix' exists, appears to be a -p0 patch\n");

> --
> 2.17.1
>

Re: [PATCH] checkpatch: warn for p0 patch only if prefix is not b

[Dafna Hirschfeld]

On 11.01.22 12:18, Lukas Bulwahn wrote:
> Dafna,
> 
> On Mon, Jan 10, 2022 at 9:06 PM Dafna Hirschfeld
> <dafna.hirschfeld@collabora.com> wrote:
>>
>> It might be that file 'b' happens to exit. In that
>> case, if the prefix is also 'b' (which is the
>> common case) we get the falsely warning:
>>
>> patch prefix 'b' exists, appears to be a -p0 patch
>>
>> So warn only if prefix is not 'b'
>>
> 
> The checkpatch script that is maintained here is really only intended
> for its use in the kernel development. You may use checkpatch anywhere
> else, but any changes that increase complexity for those other use
> cases is really difficult to argue for inclusion in the kernel
> repository. The checkpatch script currently is already large and
> complex enough and all rules need to be understood as rough
> heuristics, not as strict rules.
> 
> So, can you point to a kernel repository where there is actually a
> file 'b' included? On a quick scan, I could not find a file 'b' in the
> current trees of the repositories on my machine.
> 
> I am just letting you know about what I have observed; I do not decide
> on the inclusion of this patch, though.

Hi, a 'b' file might make it to the source folder as an untracked file.
This actually happened to me since I was too lazy to give it a meaningful name.
Then I got this warning and it took me some time to figure out what is the problem.

Thanks,
Dafna

> 
> Lukas
> 
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>>   scripts/checkpatch.pl | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index 1784921c645d..72263b142e39 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -2821,7 +2821,7 @@ sub process {
>>                          $in_commit_log = 0;
>>
>>                          $p1_prefix = $1;
>> -                       if (!$file && $tree && $p1_prefix ne '' &&
>> +                       if (!$file && $tree && $p1_prefix ne '' && $p1_prefix ne 'b' &&
>>                              -e "$root/$p1_prefix") {
>>                                  WARN("PATCH_PREFIX",
>>                                       "patch prefix '$p1_prefix' exists, appears to be a -p0 patch\n");
> 
>> --
>> 2.17.1
>>

Re: [PATCH] checkpatch: warn for p0 patch only if prefix is not b

[Lukas Bulwahn]

On Tue, Jan 11, 2022 at 11:47 AM Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> wrote:
>
>
>
> On 11.01.22 12:18, Lukas Bulwahn wrote:
> > Dafna,
> >
> > On Mon, Jan 10, 2022 at 9:06 PM Dafna Hirschfeld
> > <dafna.hirschfeld@collabora.com> wrote:
> >>
> >> It might be that file 'b' happens to exit. In that
> >> case, if the prefix is also 'b' (which is the
> >> common case) we get the falsely warning:
> >>
> >> patch prefix 'b' exists, appears to be a -p0 patch
> >>
> >> So warn only if prefix is not 'b'
> >>
> >
> > The checkpatch script that is maintained here is really only intended
> > for its use in the kernel development. You may use checkpatch anywhere
> > else, but any changes that increase complexity for those other use
> > cases is really difficult to argue for inclusion in the kernel
> > repository. The checkpatch script currently is already large and
> > complex enough and all rules need to be understood as rough
> > heuristics, not as strict rules.
> >
> > So, can you point to a kernel repository where there is actually a
> > file 'b' included? On a quick scan, I could not find a file 'b' in the
> > current trees of the repositories on my machine.
> >
> > I am just letting you know about what I have observed; I do not decide
> > on the inclusion of this patch, though.
>
> Hi, a 'b' file might make it to the source folder as an untracked file.
> This actually happened to me since I was too lazy to give it a meaningful name.
> Then I got this warning and it took me some time to figure out what is the problem.
>

Well, but you run checkpatch.pl on a patch, right? So, you need to add
the file explicitly with git (where you notice adding a file called b,
which probably is really not a good name), you create a git commit
(where that is pointed out again), then create a patch from that
(which you may manually look at again) and then run checkpatch.pl
before you submit it (again, submitting a patch with a file 'b' is
probably a good reason to rethink your submission).

If it helps, you can add some documentation on the PATCH_PREFIX rule
in the checkpatch documentation at
./Documentation/dev-tools/checkpatch.rst. Especially, you can note the
situation you encountered there, e.g., that adding files with explicit
name 'a' or 'b' may make this rule trigger. If that documentation of
the rule is helpful, I will ack that documentation patch and request
inclusion of it.

Lukas

> Thanks,
> Dafna
>
> >
> > Lukas
> >
> >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> >> ---
> >>   scripts/checkpatch.pl | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> >> index 1784921c645d..72263b142e39 100755
> >> --- a/scripts/checkpatch.pl
> >> +++ b/scripts/checkpatch.pl
> >> @@ -2821,7 +2821,7 @@ sub process {
> >>                          $in_commit_log = 0;
> >>
> >>                          $p1_prefix = $1;
> >> -                       if (!$file && $tree && $p1_prefix ne '' &&
> >> +                       if (!$file && $tree && $p1_prefix ne '' && $p1_prefix ne 'b' &&
> >>                              -e "$root/$p1_prefix") {
> >>                                  WARN("PATCH_PREFIX",
> >>                                       "patch prefix '$p1_prefix' exists, appears to be a -p0 patch\n");
> >
> >> --
> >> 2.17.1
> >>

Re: [PATCH] checkpatch: warn for p0 patch only if prefix is not b

[Dafna Hirschfeld]

On 11.01.22 13:27, Lukas Bulwahn wrote:
> On Tue, Jan 11, 2022 at 11:47 AM Dafna Hirschfeld
> <dafna.hirschfeld@collabora.com> wrote:
>>
>>
>>
>> On 11.01.22 12:18, Lukas Bulwahn wrote:
>>> Dafna,
>>>
>>> On Mon, Jan 10, 2022 at 9:06 PM Dafna Hirschfeld
>>> <dafna.hirschfeld@collabora.com> wrote:
>>>>
>>>> It might be that file 'b' happens to exit. In that
>>>> case, if the prefix is also 'b' (which is the
>>>> common case) we get the falsely warning:
>>>>
>>>> patch prefix 'b' exists, appears to be a -p0 patch
>>>>
>>>> So warn only if prefix is not 'b'
>>>>
>>>
>>> The checkpatch script that is maintained here is really only intended
>>> for its use in the kernel development. You may use checkpatch anywhere
>>> else, but any changes that increase complexity for those other use
>>> cases is really difficult to argue for inclusion in the kernel
>>> repository. The checkpatch script currently is already large and
>>> complex enough and all rules need to be understood as rough
>>> heuristics, not as strict rules.
>>>
>>> So, can you point to a kernel repository where there is actually a
>>> file 'b' included? On a quick scan, I could not find a file 'b' in the
>>> current trees of the repositories on my machine.
>>>
>>> I am just letting you know about what I have observed; I do not decide
>>> on the inclusion of this patch, though.
>>
>> Hi, a 'b' file might make it to the source folder as an untracked file.
>> This actually happened to me since I was too lazy to give it a meaningful name.
>> Then I got this warning and it took me some time to figure out what is the problem.
>>
> 
> Well, but you run checkpatch.pl on a patch, right? So, you need to add
> the file explicitly with git (where you notice adding a file called b,
> which probably is really not a good name), you create a git commit
> (where that is pointed out again), then create a patch from that
> (which you may manually look at again) and then run checkpatch.pl
> before you submit it (again, submitting a patch with a file 'b' is
> probably a good reason to rethink your submission).

Hi , no, the 'b' file is left untracked.
It is easily reproducible, inside a kernel source repo do:

touch b

# These two commands will both trigger the warning.
./scripts/checkpatch.pl --strict -g HEAD
./scripts/checkpatch.pl --strict 0001-some-random-patch.patch

> 
> If it helps, you can add some documentation on the PATCH_PREFIX rule
> in the checkpatch documentation at
> ./Documentation/dev-tools/checkpatch.rst. Especially, you can note the
> situation you encountered there, e.g., that adding files with explicit
> name 'a' or 'b' may make this rule trigger. If that documentation of
> the rule is helpful, I will ack that documentation patch and request
> inclusion of it.

I can do that as well if you think it is better.

Thanks,
Dafna

> 
> Lukas
> 
>> Thanks,
>> Dafna
>>
>>>
>>> Lukas
>>>
>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>>> ---
>>>>    scripts/checkpatch.pl | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>>> index 1784921c645d..72263b142e39 100755
>>>> --- a/scripts/checkpatch.pl
>>>> +++ b/scripts/checkpatch.pl
>>>> @@ -2821,7 +2821,7 @@ sub process {
>>>>                           $in_commit_log = 0;
>>>>
>>>>                           $p1_prefix = $1;
>>>> -                       if (!$file && $tree && $p1_prefix ne '' &&
>>>> +                       if (!$file && $tree && $p1_prefix ne '' && $p1_prefix ne 'b' &&
>>>>                               -e "$root/$p1_prefix") {
>>>>                                   WARN("PATCH_PREFIX",
>>>>                                        "patch prefix '$p1_prefix' exists, appears to be a -p0 patch\n");
>>>
>>>> --
>>>> 2.17.1
>>>>