[Linux-kernel-mentees] [PATCH v2] checkpatch: Fix the usage of capture group ( ... )

[Linux-kernel-mentees] [PATCH v2] checkpatch: Fix the usage of capture group ( ... )

[Mrinal Pandey]

[-- Attachment #1.1: Type: text/plain, Size: 2031 bytes --]

The usage of "capture group (...)" in the immediate condition after `&&`
results in `$1` being uninitialized. This issues a warning "Use of
uninitialized value $1 in regexp compilation at ./scripts/checkpatch.pl
line 2638".

I noticed this bug while running checkpatch on the set of commits from
v5.7 to v5.8-rc1 of the kernel on the commits with a diff content in
their commit message.

This bug was introduced in the script by commit e518e9a59ec3
("checkpatch: emit an error when there's a diff in a changelog"). It has
been in the script since then.

The author intended to store the match made by capture group in variable
`$1`. This should have contained the name of the file as `[\w/]+` matched.
However, this couldn't be accomplished due to usage of capture group and
`$1` in the same regular expression.

Fix this by placing the capture group in the condition before `&&`.
Thus, `$1` can be initialized to the text that capture group matches
thereby setting it to the desired and required value.

Reviewed-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Tested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Signed-off-by: Mrinal Pandey <mrinalmni@gmail.com>
---
Changes since v1:
Add Reviewed-by and Tested-by tag

 scripts/checkpatch.pl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4c820607540b..e73e998d582a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2636,8 +2636,8 @@ sub process {
 
 # Check if the commit log has what seems like a diff which can confuse patch
 		if ($in_commit_log && !$commit_log_has_diff &&
-		    (($line =~ m@^\s+diff\b.*a/[\w/]+@ &&
-		      $line =~ m@^\s+diff\b.*a/([\w/]+)\s+b/$1\b@) ||
+		    (($line =~ m@^\s+diff\b.*a/([\w/]+)@ &&
+		      $line =~ m@^\s+diff\b.*a/[\w/]+\s+b/$1\b@) ||
 		     $line =~ m@^\s*(?:\-\-\-\s+a/|\+\+\+\s+b/)@ ||
 		     $line =~ m/^\s*\@\@ \-\d+,\d+ \+\d+,\d+ \@\@/)) {
 			ERROR("DIFF_IN_COMMIT_MSG",
-- 
2.25.1


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

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

Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: Fix the usage of capture group ( ... )

[Lukas Bulwahn]

On Tue, 14 Jul 2020, Mrinal Pandey wrote:

> The usage of "capture group (...)" in the immediate condition after `&&`
> results in `$1` being uninitialized. This issues a warning "Use of
> uninitialized value $1 in regexp compilation at ./scripts/checkpatch.pl
> line 2638".
> 
> I noticed this bug while running checkpatch on the set of commits from
> v5.7 to v5.8-rc1 of the kernel on the commits with a diff content in
> their commit message.
> 
> This bug was introduced in the script by commit e518e9a59ec3
> ("checkpatch: emit an error when there's a diff in a changelog"). It has
> been in the script since then.
> 
> The author intended to store the match made by capture group in variable
> `$1`. This should have contained the name of the file as `[\w/]+` matched.
> However, this couldn't be accomplished due to usage of capture group and
> `$1` in the same regular expression.
> 
> Fix this by placing the capture group in the condition before `&&`.
> Thus, `$1` can be initialized to the text that capture group matches
> thereby setting it to the desired and required value.
> 
> Reviewed-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> Tested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> Signed-off-by: Mrinal Pandey <mrinalmni@gmail.com>
> ---
> Changes since v1:
> Add Reviewed-by and Tested-by tag
>


Usually, the maintainers pick up those tags when they apply a patch to 
their tree, and the patch authors do not need to add those tags and resend 
the patch as a new version on the mailing list.

Mrinal, can you please go through and check the kernel documentation on 
submitting patches and find the places where it is pointed out that the 
maintainers usually apply those tags and the authors usually do not need 
to resend a new version of the patch with the tags applied?

If you cannot find such a statement in the documentation, please let us 
know where it would ideally added in the documentation. We can then create 
a patch for that together.


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

Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: Fix the usage of capture group ( ... )

[Mrinal Pandey]

[-- Attachment #1.1: Type: text/plain, Size: 2676 bytes --]

On Tue, Jul 14, 2020 at 11:18 AM Lukas Bulwahn <lukas.bulwahn@gmail.com>
wrote:

>
>
> On Tue, 14 Jul 2020, Mrinal Pandey wrote:
>
> > The usage of "capture group (...)" in the immediate condition after `&&`
> > results in `$1` being uninitialized. This issues a warning "Use of
> > uninitialized value $1 in regexp compilation at ./scripts/checkpatch.pl
> > line 2638".
> >
> > I noticed this bug while running checkpatch on the set of commits from
> > v5.7 to v5.8-rc1 of the kernel on the commits with a diff content in
> > their commit message.
> >
> > This bug was introduced in the script by commit e518e9a59ec3
> > ("checkpatch: emit an error when there's a diff in a changelog"). It has
> > been in the script since then.
> >
> > The author intended to store the match made by capture group in variable
> > `$1`. This should have contained the name of the file as `[\w/]+`
> matched.
> > However, this couldn't be accomplished due to usage of capture group and
> > `$1` in the same regular expression.
> >
> > Fix this by placing the capture group in the condition before `&&`.
> > Thus, `$1` can be initialized to the text that capture group matches
> > thereby setting it to the desired and required value.
> >
> > Reviewed-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > Tested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > Signed-off-by: Mrinal Pandey <mrinalmni@gmail.com>
> > ---
> > Changes since v1:
> > Add Reviewed-by and Tested-by tag
> >
>
>
> Usually, the maintainers pick up those tags when they apply a patch to
> their tree, and the patch authors do not need to add those tags and resend
> the patch as a new version on the mailing list.
>

Sir,

Thank you for your insight. I'll keep this in mind.


>
> Mrinal, can you please go through and check the kernel documentation on
> submitting patches and find the places where it is pointed out that the
> maintainers usually apply those tags and the authors usually do not need
> to resend a new version of the patch with the tags applied?
>

I read through Submitting patches
<https://www.kernel.org/doc/html/latest/process/submitting-patches.html>
and a few related pieces of documentation but
couldn't infer this information.


> If you cannot find such a statement in the documentation, please let us
> know where it would ideally added in the documentation. We can then create
> a patch for that together.
>

The statement might be placed under Respond to review comments
<https://www.kernel.org/doc/html/latest/process/submitting-patches.html#respond-to-review-comments>
and maybe
it could further contain some information about how to send a new version
of the patch.

Thank you.

>
>
> Lukas
>

[-- Attachment #1.2: Type: text/html, Size: 4200 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

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

Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: Fix the usage of capture group ( ... )

[Lukas Bulwahn]

On Tue, 14 Jul 2020, Mrinal Pandey wrote:

> The usage of "capture group (...)" in the immediate condition after `&&`
> results in `$1` being uninitialized. This issues a warning "Use of
> uninitialized value $1 in regexp compilation at ./scripts/checkpatch.pl
> line 2638".
> 
> I noticed this bug while running checkpatch on the set of commits from
> v5.7 to v5.8-rc1 of the kernel on the commits with a diff content in
> their commit message.
> 
> This bug was introduced in the script by commit e518e9a59ec3
> ("checkpatch: emit an error when there's a diff in a changelog"). It has
> been in the script since then.
> 
> The author intended to store the match made by capture group in variable
> `$1`. This should have contained the name of the file as `[\w/]+` matched.
> However, this couldn't be accomplished due to usage of capture group and
> `$1` in the same regular expression.
> 
> Fix this by placing the capture group in the condition before `&&`.
> Thus, `$1` can be initialized to the text that capture group matches
> thereby setting it to the desired and required value.
> 
> Reviewed-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> Tested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> Signed-off-by: Mrinal Pandey <mrinalmni@gmail.com>
> ---
> Changes since v1:
> Add Reviewed-by and Tested-by tag
>

Hi Joe,

did you see this quick fix to checkpatch.pl? Can you comment on the 
commit and can we get a quick ack on that fix, please?

General question on patches for ./scripts/checkpatch.pl:

How do they travel to Linus?

Do you pick those patches and provide them to Andrew Morton?
Or do you just ack them and we need to send them to Andrew Morton to get 
them into mainline?

(get_maintainers tells us to just send those patches to you and Andy.)


Lukas


>  scripts/checkpatch.pl | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 4c820607540b..e73e998d582a 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2636,8 +2636,8 @@ sub process {
>  
>  # Check if the commit log has what seems like a diff which can confuse patch
>  		if ($in_commit_log && !$commit_log_has_diff &&
> -		    (($line =~ m@^\s+diff\b.*a/[\w/]+@ &&
> -		      $line =~ m@^\s+diff\b.*a/([\w/]+)\s+b/$1\b@) ||
> +		    (($line =~ m@^\s+diff\b.*a/([\w/]+)@ &&
> +		      $line =~ m@^\s+diff\b.*a/[\w/]+\s+b/$1\b@) ||
>  		     $line =~ m@^\s*(?:\-\-\-\s+a/|\+\+\+\s+b/)@ ||
>  		     $line =~ m/^\s*\@\@ \-\d+,\d+ \+\d+,\d+ \@\@/)) {
>  			ERROR("DIFF_IN_COMMIT_MSG",
> -- 
> 2.25.1
> 
> 
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: Fix the usage of capture group ( ... )

[Joe Perches]

On Thu, 2020-07-30 at 07:58 +0200, Lukas Bulwahn wrote:
> Hi Joe,
> 
> did you see this quick fix to checkpatch.pl? Can you comment on the 
> commit and can we get a quick ack on that fix, please?

Yes, in a bit.

> General question on patches for ./scripts/checkpatch.pl:
> How do they travel to Linus?

Generally via Andrew Morton.

> Do you pick those patches and provide them to Andrew Morton?
> Or do you just ack them and we need to send them to Andrew Morton to get 
> them into mainline?

Generally, ack and forward.


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

Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: Fix the usage of capture group ( ... )

[Mrinal Pandey]

[-- Attachment #1.1: Type: text/plain, Size: 937 bytes --]

On 20/07/30 12:31AM, Joe Perches wrote:
> On Thu, 2020-07-30 at 07:58 +0200, Lukas Bulwahn wrote:
> > Hi Joe,
> > 
> > did you see this quick fix to checkpatch.pl? Can you comment on the 
> > commit and can we get a quick ack on that fix, please?
> 
> Yes, in a bit.
> 
> > General question on patches for ./scripts/checkpatch.pl:
> > How do they travel to Linus?
> 
> Generally via Andrew Morton.
> 
> > Do you pick those patches and provide them to Andrew Morton?
> > Or do you just ack them and we need to send them to Andrew Morton to get 
> > them into mainline?
> 
> Generally, ack and forward.
> 
> 
Respected maintainers,

Did you get the time to check this patch out? This patch is a part of my
mentorship project and your comments on it would help me to make the
best out of this learning process.
Please review it as time permits and let me know if what I propose makes
sense at all.

Thank you.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

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

Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: Fix the usage of capture group ( ... )

[Joe Perches]

On Thu, 2020-08-20 at 10:22 +0530, Mrinal Pandey wrote:
> On 20/07/30 12:31AM, Joe Perches wrote:
> > On Thu, 2020-07-30 at 07:58 +0200, Lukas Bulwahn wrote:
> > > Hi Joe,
> > > 
> > > did you see this quick fix to checkpatch.pl? Can you comment on the 
> > > commit and can we get a quick ack on that fix, please?
> > 
> > Yes, in a bit.
> > 
> > > General question on patches for ./scripts/checkpatch.pl:
> > > How do they travel to Linus?
> > 
> > Generally via Andrew Morton.
> > 
> > > Do you pick those patches and provide them to Andrew Morton?
> > > Or do you just ack them and we need to send them to Andrew Morton to get 
> > > them into mainline?
> > 
> > Generally, ack and forward.

https://lore.kernel.org/lkml/20200714032352.f476hanaj2dlmiot@mrinalpandey/

Andrew, can you pick up this patch please.

Thanks

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