[Linux-kernel-mentees] checkpatch: improving comment parsing in email

[Linux-kernel-mentees] checkpatch: improving comment parsing in email

[Dwaipayan Ray]

Hi,
checkpatch seems to have problems parsing some types of
email comments. Some examples I could find were of the type:

1) address@example.com (Comment)
2) "Name (Comment) " address@example.com
3) address@example.com #comment

These comments aren't processed currently which causes
false BAD_SIGN_OFF warnings.

Examples are:

WARNING:BAD_SIGN_OFF: email address 'David.Laight@aculab.com
(big endian system concerns)' might be better as 'David.Laight@aculab.com
(big endian system concerns)'

WARNING:BAD_SIGN_OFF: email address 'stable@vger.kernel.org #4.20+'
might be better as 'stable@vger.kernel.org#4.20+'

The earlier warning is very frequent in the kernel.

I did send a patch solving (1 and 3),
https://lore.kernel.org/linux-kernel-mentees/a15a6cc0ddea068d78113f5e315eaba6f52b917a.camel@perches.com/

Joe points out that comments and multiple comments can
exist at any part of email. (perhaps RFC 5322 Appendix A.5). So
that patch didn't solve the problem at the very root.

What do you recommend be done?

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

Re: [Linux-kernel-mentees] checkpatch: improving comment parsing in email

[Lukas Bulwahn]

On Tue, 20 Oct 2020, Dwaipayan Ray wrote:

> Hi,
> checkpatch seems to have problems parsing some types of
> email comments. Some examples I could find were of the type:
> 
> 1) address@example.com (Comment)
> 2) "Name (Comment) " address@example.com
> 3) address@example.com #comment
> 
> These comments aren't processed currently which causes
> false BAD_SIGN_OFF warnings.
> 
> Examples are:
> 
> WARNING:BAD_SIGN_OFF: email address 'David.Laight@aculab.com
> (big endian system concerns)' might be better as 'David.Laight@aculab.com
> (big endian system concerns)'
> 
> WARNING:BAD_SIGN_OFF: email address 'stable@vger.kernel.org #4.20+'
> might be better as 'stable@vger.kernel.org#4.20+'
> 
> The earlier warning is very frequent in the kernel.
> 
> I did send a patch solving (1 and 3),
> https://lore.kernel.org/linux-kernel-mentees/a15a6cc0ddea068d78113f5e315eaba6f52b917a.camel@perches.com/
> 
> Joe points out that comments and multiple comments can
> exist at any part of email. (perhaps RFC 5322 Appendix A.5). So
> that patch didn't solve the problem at the very root.
> 
> What do you recommend be done?
>

I cannot say much of BIG value and insight.

As of now, I think these cases should be handled correctly and the check 
for: is it the same name or is it the same email should work properly no 
matter where and which kind of comment is used (as described by RFC 5322 
as you pointed out).

I might change my opinion when:

1. We have an evaluation on how many cases of BAD_SIGN_OFF (and related 
types) do we currently still observed among 100,000 commits and how many 
due to not handling comments properly?

2. We have a rough idea how complex this whole checking function will get. 
At the moment, I cannot say if it "just 10 lines of code addition" (even 
if they might be quite intrinsic to get right) or if we need hundreds of 
lines with thousand of special cases etc. (which I do not expect, but who 
knows which complexity might be involved, once we go into the details).

So, I suggest to evaluate and prototype and then we can better judge if it 
is worth really going forward, discussing it with Joeand adding it to mainline.


What do you think, Dwaipayan?

Other candidates for mentorship can certainly help here with those tasks. 
Anyone up to helping us here? Just reply with what you see you can do.

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] checkpatch: improving comment parsing in email

[Dwaipayan Ray]

On Wed, Oct 21, 2020 at 11:01 AM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
>
>
>
> On Tue, 20 Oct 2020, Dwaipayan Ray wrote:
>
> > Hi,
> > checkpatch seems to have problems parsing some types of
> > email comments. Some examples I could find were of the type:
> >
> > 1) address@example.com (Comment)
> > 2) "Name (Comment) " address@example.com
> > 3) address@example.com #comment
> >
> > These comments aren't processed currently which causes
> > false BAD_SIGN_OFF warnings.
> >
> > Examples are:
> >
> > WARNING:BAD_SIGN_OFF: email address 'David.Laight@aculab.com
> > (big endian system concerns)' might be better as 'David.Laight@aculab.com
> > (big endian system concerns)'
> >
> > WARNING:BAD_SIGN_OFF: email address 'stable@vger.kernel.org #4.20+'
> > might be better as 'stable@vger.kernel.org#4.20+'
> >
> > The earlier warning is very frequent in the kernel.
> >
> > I did send a patch solving (1 and 3),
> > https://lore.kernel.org/linux-kernel-mentees/a15a6cc0ddea068d78113f5e315eaba6f52b917a.camel@perches.com/
> >
> > Joe points out that comments and multiple comments can
> > exist at any part of email. (perhaps RFC 5322 Appendix A.5). So
> > that patch didn't solve the problem at the very root.
> >
> > What do you recommend be done?
> >
>
> I cannot say much of BIG value and insight.
>
> As of now, I think these cases should be handled correctly and the check
> for: is it the same name or is it the same email should work properly no
> matter where and which kind of comment is used (as described by RFC 5322
> as you pointed out).
>
> I might change my opinion when:
>
> 1. We have an evaluation on how many cases of BAD_SIGN_OFF (and related
> types) do we currently still observed among 100,000 commits and how many
> due to not handling comments properly?
>
> 2. We have a rough idea how complex this whole checking function will get.
> At the moment, I cannot say if it "just 10 lines of code addition" (even
> if they might be quite intrinsic to get right) or if we need hundreds of
> lines with thousand of special cases etc. (which I do not expect, but who
> knows which complexity might be involved, once we go into the details).
>
> So, I suggest to evaluate and prototype and then we can better judge if it
> is worth really going forward, discussing it with Joeand adding it to mainline.
>
>
> What do you think, Dwaipayan?
>

Hi,
I completely agree with you. We could run the evaluation first and
see what kinds of comment patterns are used. We could also keep
the RFC 5322 guidelines as a base and modify the code to recognize
the comment blocks as per the specifications.

It could get fairly complex, because comment blocks are allowed almost
anywhere. So maybe we should just solve the most
common ones first. That would help the committers largely.

I think Joe wanted a complete solution for it. So I guess i will try
to cover as many cases which doesn't increase the code complexity
much.

I will start with the evaluation. If others can help out too it will be great.
After it, we can determine what cases need to be handled and what
can be ignored.

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

Re: [Linux-kernel-mentees] checkpatch: improving comment parsing in email

[Lukas Bulwahn]

On Wed, 21 Oct 2020, Dwaipayan Ray wrote:

> On Wed, Oct 21, 2020 at 11:01 AM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
> >
> >
> >
> > On Tue, 20 Oct 2020, Dwaipayan Ray wrote:
> >
> > > Hi,
> > > checkpatch seems to have problems parsing some types of
> > > email comments. Some examples I could find were of the type:
> > >
> > > 1) address@example.com (Comment)
> > > 2) "Name (Comment) " address@example.com
> > > 3) address@example.com #comment
> > >
> > > These comments aren't processed currently which causes
> > > false BAD_SIGN_OFF warnings.
> > >
> > > Examples are:
> > >
> > > WARNING:BAD_SIGN_OFF: email address 'David.Laight@aculab.com
> > > (big endian system concerns)' might be better as 'David.Laight@aculab.com
> > > (big endian system concerns)'
> > >
> > > WARNING:BAD_SIGN_OFF: email address 'stable@vger.kernel.org #4.20+'
> > > might be better as 'stable@vger.kernel.org#4.20+'
> > >
> > > The earlier warning is very frequent in the kernel.
> > >
> > > I did send a patch solving (1 and 3),
> > > https://lore.kernel.org/linux-kernel-mentees/a15a6cc0ddea068d78113f5e315eaba6f52b917a.camel@perches.com/
> > >
> > > Joe points out that comments and multiple comments can
> > > exist at any part of email. (perhaps RFC 5322 Appendix A.5). So
> > > that patch didn't solve the problem at the very root.
> > >
> > > What do you recommend be done?
> > >
> >
> > I cannot say much of BIG value and insight.
> >
> > As of now, I think these cases should be handled correctly and the check
> > for: is it the same name or is it the same email should work properly no
> > matter where and which kind of comment is used (as described by RFC 5322
> > as you pointed out).
> >
> > I might change my opinion when:
> >
> > 1. We have an evaluation on how many cases of BAD_SIGN_OFF (and related
> > types) do we currently still observed among 100,000 commits and how many
> > due to not handling comments properly?
> >
> > 2. We have a rough idea how complex this whole checking function will get.
> > At the moment, I cannot say if it "just 10 lines of code addition" (even
> > if they might be quite intrinsic to get right) or if we need hundreds of
> > lines with thousand of special cases etc. (which I do not expect, but who
> > knows which complexity might be involved, once we go into the details).
> >
> > So, I suggest to evaluate and prototype and then we can better judge if it
> > is worth really going forward, discussing it with Joeand adding it to mainline.
> >
> >
> > What do you think, Dwaipayan?
> >
> 
> Hi,
> I completely agree with you. We could run the evaluation first and
> see what kinds of comment patterns are used. We could also keep
> the RFC 5322 guidelines as a base and modify the code to recognize
> the comment blocks as per the specifications.
> 
> It could get fairly complex, because comment blocks are allowed almost
> anywhere. So maybe we should just solve the most
> common ones first. That would help the committers largely.
> 
> I think Joe wanted a complete solution for it. So I guess i will try
> to cover as many cases which doesn't increase the code complexity
> much.
> 
> I will start with the evaluation. If others can help out too it will be great.
> After it, we can determine what cases need to be handled and what
> can be ignored.
>

Go for it and share your results with the group once you have something 
which others can review and help to evaluate.

The requirement for future applicants to be selected is to show that they 
can help you with their work on evaluations and their explanations.

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] checkpatch: improving comment parsing in email

[Dwaipayan Ray]

On Wed, Oct 21, 2020 at 2:26 PM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
>
>
>
> On Wed, 21 Oct 2020, Dwaipayan Ray wrote:
>
> > On Wed, Oct 21, 2020 at 11:01 AM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
> > >
> > >
> > >
> > > On Tue, 20 Oct 2020, Dwaipayan Ray wrote:
> > >
> > > > Hi,
> > > > checkpatch seems to have problems parsing some types of
> > > > email comments. Some examples I could find were of the type:
> > > >
> > > > 1) address@example.com (Comment)
> > > > 2) "Name (Comment) " address@example.com
> > > > 3) address@example.com #comment
> > > >
> > > > These comments aren't processed currently which causes
> > > > false BAD_SIGN_OFF warnings.
> > > >
> > > > Examples are:
> > > >
> > > > WARNING:BAD_SIGN_OFF: email address 'David.Laight@aculab.com
> > > > (big endian system concerns)' might be better as 'David.Laight@aculab.com
> > > > (big endian system concerns)'
> > > >
> > > > WARNING:BAD_SIGN_OFF: email address 'stable@vger.kernel.org #4.20+'
> > > > might be better as 'stable@vger.kernel.org#4.20+'
> > > >
> > > > The earlier warning is very frequent in the kernel.
> > > >
> > > > I did send a patch solving (1 and 3),
> > > > https://lore.kernel.org/linux-kernel-mentees/a15a6cc0ddea068d78113f5e315eaba6f52b917a.camel@perches.com/
> > > >
> > > > Joe points out that comments and multiple comments can
> > > > exist at any part of email. (perhaps RFC 5322 Appendix A.5). So
> > > > that patch didn't solve the problem at the very root.
> > > >
> > > > What do you recommend be done?
> > > >
> > >
> > > I cannot say much of BIG value and insight.
> > >
> > > As of now, I think these cases should be handled correctly and the check
> > > for: is it the same name or is it the same email should work properly no
> > > matter where and which kind of comment is used (as described by RFC 5322
> > > as you pointed out).
> > >
> > > I might change my opinion when:
> > >
> > > 1. We have an evaluation on how many cases of BAD_SIGN_OFF (and related
> > > types) do we currently still observed among 100,000 commits and how many
> > > due to not handling comments properly?
> > >
> > > 2. We have a rough idea how complex this whole checking function will get.
> > > At the moment, I cannot say if it "just 10 lines of code addition" (even
> > > if they might be quite intrinsic to get right) or if we need hundreds of
> > > lines with thousand of special cases etc. (which I do not expect, but who
> > > knows which complexity might be involved, once we go into the details).
> > >
> > > So, I suggest to evaluate and prototype and then we can better judge if it
> > > is worth really going forward, discussing it with Joeand adding it to mainline.
> > >
> > >
> > > What do you think, Dwaipayan?
> > >
> >
> > Hi,
> > I completely agree with you. We could run the evaluation first and
> > see what kinds of comment patterns are used. We could also keep
> > the RFC 5322 guidelines as a base and modify the code to recognize
> > the comment blocks as per the specifications.
> >
> > It could get fairly complex, because comment blocks are allowed almost
> > anywhere. So maybe we should just solve the most
> > common ones first. That would help the committers largely.
> >
> > I think Joe wanted a complete solution for it. So I guess i will try
> > to cover as many cases which doesn't increase the code complexity
> > much.
> >
> > I will start with the evaluation. If others can help out too it will be great.
> > After it, we can determine what cases need to be handled and what
> > can be ignored.
> >
>
> Go for it and share your results with the group once you have something
> which others can review and help to evaluate.
>
> The requirement for future applicants to be selected is to show that they
> can help you with their work on evaluations and their explanations.
>
> Lukas

Hi,
Sorry I took a bit of time for this. It was awfully slow and I had to cancel
it several times.

Nevertheless, here is my report.
I ran checkpatch over 50,000 commits from v5.4.

There were:
1516 BAD_SIGN_OFF warnings.

out of which:
798 BAD_SIGN_OFF: email address <email1> might be better as <email2>
warnings.

Since this was the potential error point, I dug deeper, and there were
293 BAD_SIGN_OFF: email address '<stable@vger.kernel.org> # .*' might
be better as warnings.

There are 68 other potential warnings, some of which are are of type:

email address '"Kai Mäkisara (Kolumbus)" <kai.makisara@kolumbus.fi>' might be
 better as '"Kai Mäkisara"(Kolumbus) <kai.makisara@kolumbus.fi>'

The name comment wasn't parsed properly due to the quotes handling.

Recursive parantheses types like (john (dean)) were not found.

So I think for now, taking care of the correct name comment parsing
for quoted names and proper comment parsing after the email address
should fix most of the BAD_SIGN_OFF false positives.

What do you think?

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

Re: [Linux-kernel-mentees] checkpatch: improving comment parsing in email

[Lukas Bulwahn]

[-- Attachment #1: Type: text/plain, Size: 5438 bytes --]



On Sun, 25 Oct 2020, Dwaipayan Ray wrote:

> On Wed, Oct 21, 2020 at 2:26 PM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
> >
> >
> >
> > On Wed, 21 Oct 2020, Dwaipayan Ray wrote:
> >
> > > On Wed, Oct 21, 2020 at 11:01 AM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
> > > >
> > > >
> > > >
> > > > On Tue, 20 Oct 2020, Dwaipayan Ray wrote:
> > > >
> > > > > Hi,
> > > > > checkpatch seems to have problems parsing some types of
> > > > > email comments. Some examples I could find were of the type:
> > > > >
> > > > > 1) address@example.com (Comment)
> > > > > 2) "Name (Comment) " address@example.com
> > > > > 3) address@example.com #comment
> > > > >
> > > > > These comments aren't processed currently which causes
> > > > > false BAD_SIGN_OFF warnings.
> > > > >
> > > > > Examples are:
> > > > >
> > > > > WARNING:BAD_SIGN_OFF: email address 'David.Laight@aculab.com
> > > > > (big endian system concerns)' might be better as 'David.Laight@aculab.com
> > > > > (big endian system concerns)'
> > > > >
> > > > > WARNING:BAD_SIGN_OFF: email address 'stable@vger.kernel.org #4.20+'
> > > > > might be better as 'stable@vger.kernel.org#4.20+'
> > > > >
> > > > > The earlier warning is very frequent in the kernel.
> > > > >
> > > > > I did send a patch solving (1 and 3),
> > > > > https://lore.kernel.org/linux-kernel-mentees/a15a6cc0ddea068d78113f5e315eaba6f52b917a.camel@perches.com/
> > > > >
> > > > > Joe points out that comments and multiple comments can
> > > > > exist at any part of email. (perhaps RFC 5322 Appendix A.5). So
> > > > > that patch didn't solve the problem at the very root.
> > > > >
> > > > > What do you recommend be done?
> > > > >
> > > >
> > > > I cannot say much of BIG value and insight.
> > > >
> > > > As of now, I think these cases should be handled correctly and the check
> > > > for: is it the same name or is it the same email should work properly no
> > > > matter where and which kind of comment is used (as described by RFC 5322
> > > > as you pointed out).
> > > >
> > > > I might change my opinion when:
> > > >
> > > > 1. We have an evaluation on how many cases of BAD_SIGN_OFF (and related
> > > > types) do we currently still observed among 100,000 commits and how many
> > > > due to not handling comments properly?
> > > >
> > > > 2. We have a rough idea how complex this whole checking function will get.
> > > > At the moment, I cannot say if it "just 10 lines of code addition" (even
> > > > if they might be quite intrinsic to get right) or if we need hundreds of
> > > > lines with thousand of special cases etc. (which I do not expect, but who
> > > > knows which complexity might be involved, once we go into the details).
> > > >
> > > > So, I suggest to evaluate and prototype and then we can better judge if it
> > > > is worth really going forward, discussing it with Joeand adding it to mainline.
> > > >
> > > >
> > > > What do you think, Dwaipayan?
> > > >
> > >
> > > Hi,
> > > I completely agree with you. We could run the evaluation first and
> > > see what kinds of comment patterns are used. We could also keep
> > > the RFC 5322 guidelines as a base and modify the code to recognize
> > > the comment blocks as per the specifications.
> > >
> > > It could get fairly complex, because comment blocks are allowed almost
> > > anywhere. So maybe we should just solve the most
> > > common ones first. That would help the committers largely.
> > >
> > > I think Joe wanted a complete solution for it. So I guess i will try
> > > to cover as many cases which doesn't increase the code complexity
> > > much.
> > >
> > > I will start with the evaluation. If others can help out too it will be great.
> > > After it, we can determine what cases need to be handled and what
> > > can be ignored.
> > >
> >
> > Go for it and share your results with the group once you have something
> > which others can review and help to evaluate.
> >
> > The requirement for future applicants to be selected is to show that they
> > can help you with their work on evaluations and their explanations.
> >
> > Lukas
> 
> Hi,
> Sorry I took a bit of time for this. It was awfully slow and I had to cancel
> it several times.
> 
> Nevertheless, here is my report.
> I ran checkpatch over 50,000 commits from v5.4.
> 
> There were:
> 1516 BAD_SIGN_OFF warnings.
> 
> out of which:
> 798 BAD_SIGN_OFF: email address <email1> might be better as <email2>
> warnings.
> 
> Since this was the potential error point, I dug deeper, and there were
> 293 BAD_SIGN_OFF: email address '<stable@vger.kernel.org> # .*' might
> be better as warnings.
>

Yeah, that is a default pattern for stable; that should be handled.
 
> There are 68 other potential warnings, some of which are are of type:
> 
> email address '"Kai Mäkisara (Kolumbus)" <kai.makisara@kolumbus.fi>' might be
>  better as '"Kai Mäkisara"(Kolumbus) <kai.makisara@kolumbus.fi>'
> 
> The name comment wasn't parsed properly due to the quotes handling.
>

Maybe you can address that as well.
 
> Recursive parantheses types like (john (dean)) were not found.
> 
> So I think for now, taking care of the correct name comment parsing
> for quoted names and proper comment parsing after the email address
> should fix most of the BAD_SIGN_OFF false positives.
> 
> What do you think?
>

I think both are worth the effort to add proper support to the script.

Once you have those patches, I can run another evaluation as well.

Lukas

[-- 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