diff mbox series

[ovs-dev] checkpatch: Add argument to skip gerrit change id check

Message ID 1592150583-178826-1-git-send-email-roid@mellanox.com
State New
Headers show
Series [ovs-dev] checkpatch: Add argument to skip gerrit change id check | expand

Commit Message

Roi Dayan June 14, 2020, 4:03 p.m. UTC
This arg can be used internally by groups using gerrit for code reviews.

Signed-off-by: Roi Dayan <roid@mellanox.com>
---
 utilities/checkpatch.py | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Roi Dayan June 16, 2020, 1:14 p.m. UTC | #1
On 2020-06-14 7:03 PM, Roi Dayan wrote:
> This arg can be used internally by groups using gerrit for code reviews.
> 
> Signed-off-by: Roi Dayan <roid@mellanox.com>
> ---
>  utilities/checkpatch.py | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index fc9e20bf1b5f..78c8c9ce49c7 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -182,6 +182,7 @@ __regex_if_macros = re.compile(r'^ +(%s) \([\S]([\s\S]+[\S])*\) { +\\' %
>  
>  skip_leading_whitespace_check = False
>  skip_trailing_whitespace_check = False
> +skip_gerrit_change_id_check = False
>  skip_block_whitespace_check = False
>  skip_signoff_check = False
>  
> @@ -814,7 +815,7 @@ def ovs_checkpatch_parse(text, filename, author=None, committer=None):
>              elif is_co_author.match(line):
>                  m = is_co_author.match(line)
>                  co_authors.append(m.group(2))
> -            elif is_gerrit_change_id.match(line):
> +            elif is_gerrit_change_id.match(line) and not skip_gerrit_change_id_check:
>                  print_error(
>                      "Remove Gerrit Change-Id's before submitting upstream.")
>                  print("%d: %s\n" % (lineno, line))
> @@ -885,7 +886,8 @@ Check options:
>  -s|--skip-signoff-lines        Tolerate missing Signed-off-by line
>  -S|--spellcheck                Check C comments and commit-message for possible
>                                 spelling mistakes
> --t|--skip-trailing-whitespace  Skips the trailing whitespace test"""
> +-t|--skip-trailing-whitespace  Skips the trailing whitespace test
> +   --skip-gerrit-change-id     Skips the gerrit change id test"""
>            % sys.argv[0])
>  
>  
> @@ -942,6 +944,7 @@ if __name__ == '__main__':
>                                         "skip-leading-whitespace",
>                                         "skip-signoff-lines",
>                                         "skip-trailing-whitespace",
> +                                       "skip-gerrit-change-id",
>                                         "spellcheck",
>                                         "quiet"])
>      except:
> @@ -960,6 +963,8 @@ if __name__ == '__main__':
>              skip_signoff_check = True
>          elif o in ("-t", "--skip-trailing-whitespace"):
>              skip_trailing_whitespace_check = True
> +        elif o in ("--skip-gerrit-change-id"):
> +            skip_gerrit_change_id_check = True
>          elif o in ("-f", "--check-file"):
>              checking_file = True
>          elif o in ("-S", "--spellcheck"):
> 

Hi, 

Any comments on this?

Thanks,
Roi
Simon Horman June 16, 2020, 2:05 p.m. UTC | #2
On Tue, Jun 16, 2020 at 04:14:23PM +0300, Roi Dayan wrote:
> 
> 
> On 2020-06-14 7:03 PM, Roi Dayan wrote:
> > This arg can be used internally by groups using gerrit for code reviews.
> > 
> > Signed-off-by: Roi Dayan <roid@mellanox.com>
> > ---
> >  utilities/checkpatch.py | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> > index fc9e20bf1b5f..78c8c9ce49c7 100755
> > --- a/utilities/checkpatch.py
> > +++ b/utilities/checkpatch.py
> > @@ -182,6 +182,7 @@ __regex_if_macros = re.compile(r'^ +(%s) \([\S]([\s\S]+[\S])*\) { +\\' %
> >  
> >  skip_leading_whitespace_check = False
> >  skip_trailing_whitespace_check = False
> > +skip_gerrit_change_id_check = False
> >  skip_block_whitespace_check = False
> >  skip_signoff_check = False
> >  
> > @@ -814,7 +815,7 @@ def ovs_checkpatch_parse(text, filename, author=None, committer=None):
> >              elif is_co_author.match(line):
> >                  m = is_co_author.match(line)
> >                  co_authors.append(m.group(2))
> > -            elif is_gerrit_change_id.match(line):
> > +            elif is_gerrit_change_id.match(line) and not skip_gerrit_change_id_check:
> >                  print_error(
> >                      "Remove Gerrit Change-Id's before submitting upstream.")
> >                  print("%d: %s\n" % (lineno, line))
> > @@ -885,7 +886,8 @@ Check options:
> >  -s|--skip-signoff-lines        Tolerate missing Signed-off-by line
> >  -S|--spellcheck                Check C comments and commit-message for possible
> >                                 spelling mistakes
> > --t|--skip-trailing-whitespace  Skips the trailing whitespace test"""
> > +-t|--skip-trailing-whitespace  Skips the trailing whitespace test
> > +   --skip-gerrit-change-id     Skips the gerrit change id test"""
> >            % sys.argv[0])
> >  
> >  
> > @@ -942,6 +944,7 @@ if __name__ == '__main__':
> >                                         "skip-leading-whitespace",
> >                                         "skip-signoff-lines",
> >                                         "skip-trailing-whitespace",
> > +                                       "skip-gerrit-change-id",
> >                                         "spellcheck",
> >                                         "quiet"])
> >      except:
> > @@ -960,6 +963,8 @@ if __name__ == '__main__':
> >              skip_signoff_check = True
> >          elif o in ("-t", "--skip-trailing-whitespace"):
> >              skip_trailing_whitespace_check = True
> > +        elif o in ("--skip-gerrit-change-id"):
> > +            skip_gerrit_change_id_check = True
> >          elif o in ("-f", "--check-file"):
> >              checking_file = True
> >          elif o in ("-S", "--spellcheck"):
> > 
> 
> Hi, 
> 
> Any comments on this?

Reviewed-by: Simon Horman <simon.horman@netronome.com>
Roi Dayan June 18, 2020, 4:31 p.m. UTC | #3
On 2020-06-16 5:05 PM, Simon Horman wrote:
> On Tue, Jun 16, 2020 at 04:14:23PM +0300, Roi Dayan wrote:
>>
>>
>> On 2020-06-14 7:03 PM, Roi Dayan wrote:
>>> This arg can be used internally by groups using gerrit for code reviews.
>>>
>>> Signed-off-by: Roi Dayan <roid@mellanox.com>
>>> ---
>>>  utilities/checkpatch.py | 9 +++++++--
>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
>>> index fc9e20bf1b5f..78c8c9ce49c7 100755
>>> --- a/utilities/checkpatch.py
>>> +++ b/utilities/checkpatch.py
>>> @@ -182,6 +182,7 @@ __regex_if_macros = re.compile(r'^ +(%s) \([\S]([\s\S]+[\S])*\) { +\\' %
>>>  
>>>  skip_leading_whitespace_check = False
>>>  skip_trailing_whitespace_check = False
>>> +skip_gerrit_change_id_check = False
>>>  skip_block_whitespace_check = False
>>>  skip_signoff_check = False
>>>  
>>> @@ -814,7 +815,7 @@ def ovs_checkpatch_parse(text, filename, author=None, committer=None):
>>>              elif is_co_author.match(line):
>>>                  m = is_co_author.match(line)
>>>                  co_authors.append(m.group(2))
>>> -            elif is_gerrit_change_id.match(line):
>>> +            elif is_gerrit_change_id.match(line) and not skip_gerrit_change_id_check:
>>>                  print_error(
>>>                      "Remove Gerrit Change-Id's before submitting upstream.")
>>>                  print("%d: %s\n" % (lineno, line))
>>> @@ -885,7 +886,8 @@ Check options:
>>>  -s|--skip-signoff-lines        Tolerate missing Signed-off-by line
>>>  -S|--spellcheck                Check C comments and commit-message for possible
>>>                                 spelling mistakes
>>> --t|--skip-trailing-whitespace  Skips the trailing whitespace test"""
>>> +-t|--skip-trailing-whitespace  Skips the trailing whitespace test
>>> +   --skip-gerrit-change-id     Skips the gerrit change id test"""
>>>            % sys.argv[0])
>>>  
>>>  
>>> @@ -942,6 +944,7 @@ if __name__ == '__main__':
>>>                                         "skip-leading-whitespace",
>>>                                         "skip-signoff-lines",
>>>                                         "skip-trailing-whitespace",
>>> +                                       "skip-gerrit-change-id",
>>>                                         "spellcheck",
>>>                                         "quiet"])
>>>      except:
>>> @@ -960,6 +963,8 @@ if __name__ == '__main__':
>>>              skip_signoff_check = True
>>>          elif o in ("-t", "--skip-trailing-whitespace"):
>>>              skip_trailing_whitespace_check = True
>>> +        elif o in ("--skip-gerrit-change-id"):
>>> +            skip_gerrit_change_id_check = True
>>>          elif o in ("-f", "--check-file"):
>>>              checking_file = True
>>>          elif o in ("-S", "--spellcheck"):
>>>
>>
>> Hi, 
>>
>> Any comments on this?
> 
> Reviewed-by: Simon Horman <simon.horman@netronome.com>
> 

thanks. there are no other comments. can we merge this?
Flavio Leitner July 9, 2020, 3:09 p.m. UTC | #4
On Sun, Jun 14, 2020 at 07:03:03PM +0300, Roi Dayan wrote:
> This arg can be used internally by groups using gerrit for code reviews.

The patch looks good, but there is the condition line
that goes up to column 85. It's the only line beyond
column 79 in that file.

Sorry the nitpicking, but could you please fix that?

fbl

> 
> Signed-off-by: Roi Dayan <roid@mellanox.com>
> ---
>  utilities/checkpatch.py | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index fc9e20bf1b5f..78c8c9ce49c7 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -182,6 +182,7 @@ __regex_if_macros = re.compile(r'^ +(%s) \([\S]([\s\S]+[\S])*\) { +\\' %
>  
>  skip_leading_whitespace_check = False
>  skip_trailing_whitespace_check = False
> +skip_gerrit_change_id_check = False
>  skip_block_whitespace_check = False
>  skip_signoff_check = False
>  
> @@ -814,7 +815,7 @@ def ovs_checkpatch_parse(text, filename, author=None, committer=None):
>              elif is_co_author.match(line):
>                  m = is_co_author.match(line)
>                  co_authors.append(m.group(2))
> -            elif is_gerrit_change_id.match(line):
> +            elif is_gerrit_change_id.match(line) and not skip_gerrit_change_id_check:
>                  print_error(
>                      "Remove Gerrit Change-Id's before submitting upstream.")
>                  print("%d: %s\n" % (lineno, line))
> @@ -885,7 +886,8 @@ Check options:
>  -s|--skip-signoff-lines        Tolerate missing Signed-off-by line
>  -S|--spellcheck                Check C comments and commit-message for possible
>                                 spelling mistakes
> --t|--skip-trailing-whitespace  Skips the trailing whitespace test"""
> +-t|--skip-trailing-whitespace  Skips the trailing whitespace test
> +   --skip-gerrit-change-id     Skips the gerrit change id test"""
>            % sys.argv[0])
>  
>  
> @@ -942,6 +944,7 @@ if __name__ == '__main__':
>                                         "skip-leading-whitespace",
>                                         "skip-signoff-lines",
>                                         "skip-trailing-whitespace",
> +                                       "skip-gerrit-change-id",
>                                         "spellcheck",
>                                         "quiet"])
>      except:
> @@ -960,6 +963,8 @@ if __name__ == '__main__':
>              skip_signoff_check = True
>          elif o in ("-t", "--skip-trailing-whitespace"):
>              skip_trailing_whitespace_check = True
> +        elif o in ("--skip-gerrit-change-id"):
> +            skip_gerrit_change_id_check = True
>          elif o in ("-f", "--check-file"):
>              checking_file = True
>          elif o in ("-S", "--spellcheck"):
> -- 
> 2.8.4
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ilya Maximets July 9, 2020, 6:30 p.m. UTC | #5
On 7/9/20 5:09 PM, Flavio Leitner wrote:
> On Sun, Jun 14, 2020 at 07:03:03PM +0300, Roi Dayan wrote:
>> This arg can be used internally by groups using gerrit for code reviews.
> 
> The patch looks good, but there is the condition line
> that goes up to column 85. It's the only line beyond
> column 79 in that file.
> 
> Sorry the nitpicking, but could you please fix that?

It's not a nitpicking, it's a PEP8 violation that breaks the build:
https://travis-ci.org/github/ovsrobot/ovs/jobs/698255640#L2702

> 
> fbl
> 
>>
>> Signed-off-by: Roi Dayan <roid@mellanox.com>
>> ---
>>  utilities/checkpatch.py | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
>> index fc9e20bf1b5f..78c8c9ce49c7 100755
>> --- a/utilities/checkpatch.py
>> +++ b/utilities/checkpatch.py
>> @@ -182,6 +182,7 @@ __regex_if_macros = re.compile(r'^ +(%s) \([\S]([\s\S]+[\S])*\) { +\\' %
>>  
>>  skip_leading_whitespace_check = False
>>  skip_trailing_whitespace_check = False
>> +skip_gerrit_change_id_check = False
>>  skip_block_whitespace_check = False
>>  skip_signoff_check = False
>>  
>> @@ -814,7 +815,7 @@ def ovs_checkpatch_parse(text, filename, author=None, committer=None):
>>              elif is_co_author.match(line):
>>                  m = is_co_author.match(line)
>>                  co_authors.append(m.group(2))
>> -            elif is_gerrit_change_id.match(line):
>> +            elif is_gerrit_change_id.match(line) and not skip_gerrit_change_id_check:
>>                  print_error(
>>                      "Remove Gerrit Change-Id's before submitting upstream.")
>>                  print("%d: %s\n" % (lineno, line))
>> @@ -885,7 +886,8 @@ Check options:
>>  -s|--skip-signoff-lines        Tolerate missing Signed-off-by line
>>  -S|--spellcheck                Check C comments and commit-message for possible
>>                                 spelling mistakes
>> --t|--skip-trailing-whitespace  Skips the trailing whitespace test"""
>> +-t|--skip-trailing-whitespace  Skips the trailing whitespace test
>> +   --skip-gerrit-change-id     Skips the gerrit change id test"""
>>            % sys.argv[0])
>>  
>>  
>> @@ -942,6 +944,7 @@ if __name__ == '__main__':
>>                                         "skip-leading-whitespace",
>>                                         "skip-signoff-lines",
>>                                         "skip-trailing-whitespace",
>> +                                       "skip-gerrit-change-id",
>>                                         "spellcheck",
>>                                         "quiet"])
>>      except:
>> @@ -960,6 +963,8 @@ if __name__ == '__main__':
>>              skip_signoff_check = True
>>          elif o in ("-t", "--skip-trailing-whitespace"):
>>              skip_trailing_whitespace_check = True
>> +        elif o in ("--skip-gerrit-change-id"):
>> +            skip_gerrit_change_id_check = True
>>          elif o in ("-f", "--check-file"):
>>              checking_file = True
>>          elif o in ("-S", "--spellcheck"):
>> -- 
>> 2.8.4
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Roi Dayan July 14, 2020, 7:24 a.m. UTC | #6
On 2020-07-09 9:30 PM, Ilya Maximets wrote:
> On 7/9/20 5:09 PM, Flavio Leitner wrote:
>> On Sun, Jun 14, 2020 at 07:03:03PM +0300, Roi Dayan wrote:
>>> This arg can be used internally by groups using gerrit for code reviews.
>>
>> The patch looks good, but there is the condition line
>> that goes up to column 85. It's the only line beyond
>> column 79 in that file.
>>
>> Sorry the nitpicking, but could you please fix that?
> 
> It's not a nitpicking, it's a PEP8 violation that breaks the build:
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Fgithub%2Fovsrobot%2Fovs%2Fjobs%2F698255640%23L2702&amp;data=02%7C01%7Croid%40mellanox.com%7C3ad7cba04fc64baad9be08d824361c1e%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637299162109839762&amp;sdata=I0Y3LghCrqsgnNC4Efz0iNJ1MYGo8xFC4fx0U%2FEJGKI%3D&amp;reserved=0
> 

ok. i saw Ben sent v2 so i'll update that one and send v3.

>>
>> fbl
>>
>>>
>>> Signed-off-by: Roi Dayan <roid@mellanox.com>
>>> ---
>>>  utilities/checkpatch.py | 9 +++++++--
>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
>>> index fc9e20bf1b5f..78c8c9ce49c7 100755
>>> --- a/utilities/checkpatch.py
>>> +++ b/utilities/checkpatch.py
>>> @@ -182,6 +182,7 @@ __regex_if_macros = re.compile(r'^ +(%s) \([\S]([\s\S]+[\S])*\) { +\\' %
>>>  
>>>  skip_leading_whitespace_check = False
>>>  skip_trailing_whitespace_check = False
>>> +skip_gerrit_change_id_check = False
>>>  skip_block_whitespace_check = False
>>>  skip_signoff_check = False
>>>  
>>> @@ -814,7 +815,7 @@ def ovs_checkpatch_parse(text, filename, author=None, committer=None):
>>>              elif is_co_author.match(line):
>>>                  m = is_co_author.match(line)
>>>                  co_authors.append(m.group(2))
>>> -            elif is_gerrit_change_id.match(line):
>>> +            elif is_gerrit_change_id.match(line) and not skip_gerrit_change_id_check:
>>>                  print_error(
>>>                      "Remove Gerrit Change-Id's before submitting upstream.")
>>>                  print("%d: %s\n" % (lineno, line))
>>> @@ -885,7 +886,8 @@ Check options:
>>>  -s|--skip-signoff-lines        Tolerate missing Signed-off-by line
>>>  -S|--spellcheck                Check C comments and commit-message for possible
>>>                                 spelling mistakes
>>> --t|--skip-trailing-whitespace  Skips the trailing whitespace test"""
>>> +-t|--skip-trailing-whitespace  Skips the trailing whitespace test
>>> +   --skip-gerrit-change-id     Skips the gerrit change id test"""
>>>            % sys.argv[0])
>>>  
>>>  
>>> @@ -942,6 +944,7 @@ if __name__ == '__main__':
>>>                                         "skip-leading-whitespace",
>>>                                         "skip-signoff-lines",
>>>                                         "skip-trailing-whitespace",
>>> +                                       "skip-gerrit-change-id",
>>>                                         "spellcheck",
>>>                                         "quiet"])
>>>      except:
>>> @@ -960,6 +963,8 @@ if __name__ == '__main__':
>>>              skip_signoff_check = True
>>>          elif o in ("-t", "--skip-trailing-whitespace"):
>>>              skip_trailing_whitespace_check = True
>>> +        elif o in ("--skip-gerrit-change-id"):
>>> +            skip_gerrit_change_id_check = True
>>>          elif o in ("-f", "--check-file"):
>>>              checking_file = True
>>>          elif o in ("-S", "--spellcheck"):
>>> -- 
>>> 2.8.4
>>>
>>> _______________________________________________
>>> dev mailing list
>>> dev@openvswitch.org
>>> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&amp;data=02%7C01%7Croid%40mellanox.com%7C3ad7cba04fc64baad9be08d824361c1e%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637299162109839762&amp;sdata=vjxbsymqIz67PcJvFOTUrHPP7uKiEPW18J84emlgPao%3D&amp;reserved=0
>>
>
diff mbox series

Patch

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index fc9e20bf1b5f..78c8c9ce49c7 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -182,6 +182,7 @@  __regex_if_macros = re.compile(r'^ +(%s) \([\S]([\s\S]+[\S])*\) { +\\' %
 
 skip_leading_whitespace_check = False
 skip_trailing_whitespace_check = False
+skip_gerrit_change_id_check = False
 skip_block_whitespace_check = False
 skip_signoff_check = False
 
@@ -814,7 +815,7 @@  def ovs_checkpatch_parse(text, filename, author=None, committer=None):
             elif is_co_author.match(line):
                 m = is_co_author.match(line)
                 co_authors.append(m.group(2))
-            elif is_gerrit_change_id.match(line):
+            elif is_gerrit_change_id.match(line) and not skip_gerrit_change_id_check:
                 print_error(
                     "Remove Gerrit Change-Id's before submitting upstream.")
                 print("%d: %s\n" % (lineno, line))
@@ -885,7 +886,8 @@  Check options:
 -s|--skip-signoff-lines        Tolerate missing Signed-off-by line
 -S|--spellcheck                Check C comments and commit-message for possible
                                spelling mistakes
--t|--skip-trailing-whitespace  Skips the trailing whitespace test"""
+-t|--skip-trailing-whitespace  Skips the trailing whitespace test
+   --skip-gerrit-change-id     Skips the gerrit change id test"""
           % sys.argv[0])
 
 
@@ -942,6 +944,7 @@  if __name__ == '__main__':
                                        "skip-leading-whitespace",
                                        "skip-signoff-lines",
                                        "skip-trailing-whitespace",
+                                       "skip-gerrit-change-id",
                                        "spellcheck",
                                        "quiet"])
     except:
@@ -960,6 +963,8 @@  if __name__ == '__main__':
             skip_signoff_check = True
         elif o in ("-t", "--skip-trailing-whitespace"):
             skip_trailing_whitespace_check = True
+        elif o in ("--skip-gerrit-change-id"):
+            skip_gerrit_change_id_check = True
         elif o in ("-f", "--check-file"):
             checking_file = True
         elif o in ("-S", "--spellcheck"):