[ovs-dev] checkpatch: fix scissors line regex
diff mbox series

Message ID 20180626125838.3289-1-aconole@redhat.com
State Superseded
Headers show
Series
  • [ovs-dev] checkpatch: fix scissors line regex
Related show

Commit Message

Aaron Conole June 26, 2018, 12:58 p.m. UTC
The scissors line is always three dashes on a line.  The regex would
previously match on any three dashes in a row.

This means that a patch (such as [1]) would trigger the parser state
machine to advance beyond the signed-off checks.  This bounds the
check only to run on specific lines.

1: https://mail.openvswitch.org/pipermail/ovs-dev/2018-June/348625.html

Fixes: c599d5ccf316 ("checkpatch.py: A simple script for finding patch issues")
Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 utilities/checkpatch.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ben Pfaff June 26, 2018, 10:25 p.m. UTC | #1
On Tue, Jun 26, 2018 at 08:58:38AM -0400, Aaron Conole wrote:
> The scissors line is always three dashes on a line.  The regex would
> previously match on any three dashes in a row.
> 
> This means that a patch (such as [1]) would trigger the parser state
> machine to advance beyond the signed-off checks.  This bounds the
> check only to run on specific lines.
> 
> 1: https://mail.openvswitch.org/pipermail/ovs-dev/2018-June/348625.html
> 
> Fixes: c599d5ccf316 ("checkpatch.py: A simple script for finding patch issues")
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  utilities/checkpatch.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 5a6d5f5ff..80b955f6b 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -619,7 +619,7 @@ def ovs_checkpatch_parse(text, filename):
>      parse = 0
>      current_file = filename if checking_file else ''
>      previous_file = ''
> -    scissors = re.compile(r'^[\w]*---[\w]*')
> +    scissors = re.compile(r'^[\w]*---[\w]*$')
>      hunks = re.compile('^(---|\+\+\+) (\S+)')
>      hunk_differences = re.compile(
>          r'^@@ ([0-9-+]+),([0-9-+]+) ([0-9-+]+),([0-9-+]+) @@')

I'm not sure that this is what Git calls a scissors line.  git-mailinfo
says:

       --scissors
           Remove everything in body before a scissors line. A line that
           mainly consists of scissors (either ">8" or "8<") and perforation
           (dash "-") marks is called a scissors line, and is used to request
           the reader to cut the message at that line. If such a line appears
           in the body of the message before the patch, everything before it
           (including the scissors line itself) is ignored when this option is
           used.

           This is useful if you want to begin your message in a discussion
           thread with comments and suggestions on the message you are
           responding to, and to conclude it with a patch submission,
           separating the discussion and the beginning of the proposed commit
           log message with a scissors line.

           This can be enabled by default with the configuration option
           mailinfo.scissors.

I personally use the following line for scissors:

--8<--------------------------cut here-------------------------->8--
Aaron Conole June 27, 2018, 1:17 p.m. UTC | #2
Ben Pfaff <blp@ovn.org> writes:

> On Tue, Jun 26, 2018 at 08:58:38AM -0400, Aaron Conole wrote:
>> The scissors line is always three dashes on a line.  The regex would
>> previously match on any three dashes in a row.
>> 
>> This means that a patch (such as [1]) would trigger the parser state
>> machine to advance beyond the signed-off checks.  This bounds the
>> check only to run on specific lines.
>> 
>> 1: https://mail.openvswitch.org/pipermail/ovs-dev/2018-June/348625.html
>> 
>> Fixes: c599d5ccf316 ("checkpatch.py: A simple script for finding patch issues")
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>>  utilities/checkpatch.py | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
>> index 5a6d5f5ff..80b955f6b 100755
>> --- a/utilities/checkpatch.py
>> +++ b/utilities/checkpatch.py
>> @@ -619,7 +619,7 @@ def ovs_checkpatch_parse(text, filename):
>>      parse = 0
>>      current_file = filename if checking_file else ''
>>      previous_file = ''
>> -    scissors = re.compile(r'^[\w]*---[\w]*')
>> +    scissors = re.compile(r'^[\w]*---[\w]*$')
>>      hunks = re.compile('^(---|\+\+\+) (\S+)')
>>      hunk_differences = re.compile(
>>          r'^@@ ([0-9-+]+),([0-9-+]+) ([0-9-+]+),([0-9-+]+) @@')
>
> I'm not sure that this is what Git calls a scissors line.  git-mailinfo
> says:

Oups.  I've always called both a scissors line (whether I use the
scissors mark or not), but it seems I really should distinguish scissors
from perforation.

Regardless, the change is needed to ensure we don't accidentally advance
the patch parsing state machine.

I'll re-spin it with a better commit message, and likely will have
another patch to address the other flag that happened with Darrell's
patch.
Ben Pfaff June 27, 2018, 1:52 p.m. UTC | #3
On Wed, Jun 27, 2018 at 09:17:16AM -0400, Aaron Conole wrote:
> Ben Pfaff <blp@ovn.org> writes:
> 
> > On Tue, Jun 26, 2018 at 08:58:38AM -0400, Aaron Conole wrote:
> >> The scissors line is always three dashes on a line.  The regex would
> >> previously match on any three dashes in a row.
> >> 
> >> This means that a patch (such as [1]) would trigger the parser state
> >> machine to advance beyond the signed-off checks.  This bounds the
> >> check only to run on specific lines.
> >> 
> >> 1: https://mail.openvswitch.org/pipermail/ovs-dev/2018-June/348625.html
> >> 
> >> Fixes: c599d5ccf316 ("checkpatch.py: A simple script for finding patch issues")
> >> Signed-off-by: Aaron Conole <aconole@redhat.com>
> >> ---
> >>  utilities/checkpatch.py | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> >> index 5a6d5f5ff..80b955f6b 100755
> >> --- a/utilities/checkpatch.py
> >> +++ b/utilities/checkpatch.py
> >> @@ -619,7 +619,7 @@ def ovs_checkpatch_parse(text, filename):
> >>      parse = 0
> >>      current_file = filename if checking_file else ''
> >>      previous_file = ''
> >> -    scissors = re.compile(r'^[\w]*---[\w]*')
> >> +    scissors = re.compile(r'^[\w]*---[\w]*$')
> >>      hunks = re.compile('^(---|\+\+\+) (\S+)')
> >>      hunk_differences = re.compile(
> >>          r'^@@ ([0-9-+]+),([0-9-+]+) ([0-9-+]+),([0-9-+]+) @@')
> >
> > I'm not sure that this is what Git calls a scissors line.  git-mailinfo
> > says:
> 
> Oups.  I've always called both a scissors line (whether I use the
> scissors mark or not), but it seems I really should distinguish scissors
> from perforation.
> 
> Regardless, the change is needed to ensure we don't accidentally advance
> the patch parsing state machine.
> 
> I'll re-spin it with a better commit message, and likely will have
> another patch to address the other flag that happened with Darrell's
> patch.

Respinning with a better commit message sounds like a good outcome to
me.  Thanks.

Patch
diff mbox series

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 5a6d5f5ff..80b955f6b 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -619,7 +619,7 @@  def ovs_checkpatch_parse(text, filename):
     parse = 0
     current_file = filename if checking_file else ''
     previous_file = ''
-    scissors = re.compile(r'^[\w]*---[\w]*')
+    scissors = re.compile(r'^[\w]*---[\w]*$')
     hunks = re.compile('^(---|\+\+\+) (\S+)')
     hunk_differences = re.compile(
         r'^@@ ([0-9-+]+),([0-9-+]+) ([0-9-+]+),([0-9-+]+) @@')