diff mbox series

[ovs-dev] checkpatch.py: Port checkpatch related changes from the OVS repo.

Message ID 20240116130848.1341709-1-dceara@redhat.com
State Accepted
Headers show
Series [ovs-dev] checkpatch.py: Port checkpatch related changes from the OVS repo. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_ovn-kubernetes success github build: passed
ovsrobot/github-robot-_Build_and_Test fail github build: failed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed
ovsrobot/github-robot-_Build_and_Test fail github build: failed
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

Dumitru Ceara Jan. 16, 2024, 1:08 p.m. UTC 
This picks up the following OVS changes:
  00d3d4a7d375 ("checkpatch: Avoid catastrophic backtracking.")
  d25c6bd8df37 ("checkpatch: Reorganize flagged words using a list.")
  9a50170a805a ("checkpatch: Add suggestions to the spell checker.")
  799f697e51ec ("checkpatch: Print subject field if misspelled or missing.")
  1b8fa4a66aa4 ("checkpatch: Add checks for the subject line.")
  bf843fd439b2 ("checkpatch: Don't spell check Fixes tag.")
  74bfe3701407 ("checkpatch: Add argument to skip committer signoff check.")
  21c61243fb75 ("checkpatch: Fix personal word list storage.")
  915b97971d58 ("checkpatch.py: Load codespell dictionary.")

Signed-off-by: Dumitru Ceara <dceara@redhat.com>
---
 tests/checkpatch.at     |  45 +++++++++++++++++
 utilities/checkpatch.py | 104 ++++++++++++++++++++++++++++++++++++----
 2 files changed, 140 insertions(+), 9 deletions(-)

Comments

Numan Siddique Jan. 16, 2024, 4:23 p.m. UTC | #1 
On Tue, Jan 16, 2024 at 8:09 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> This picks up the following OVS changes:
>   00d3d4a7d375 ("checkpatch: Avoid catastrophic backtracking.")
>   d25c6bd8df37 ("checkpatch: Reorganize flagged words using a list.")
>   9a50170a805a ("checkpatch: Add suggestions to the spell checker.")
>   799f697e51ec ("checkpatch: Print subject field if misspelled or missing.")
>   1b8fa4a66aa4 ("checkpatch: Add checks for the subject line.")
>   bf843fd439b2 ("checkpatch: Don't spell check Fixes tag.")
>   74bfe3701407 ("checkpatch: Add argument to skip committer signoff check.")
>   21c61243fb75 ("checkpatch: Fix personal word list storage.")
>   915b97971d58 ("checkpatch.py: Load codespell dictionary.")
>
> Signed-off-by: Dumitru Ceara <dceara@redhat.com>

Thanks for the patch.

Acked-by: Numan Siddique <numans@ovn.org>

Numan

> ---
>  tests/checkpatch.at     |  45 +++++++++++++++++
>  utilities/checkpatch.py | 104 ++++++++++++++++++++++++++++++++++++----
>  2 files changed, 140 insertions(+), 9 deletions(-)
>
> diff --git a/tests/checkpatch.at b/tests/checkpatch.at
> index 26f3197053..e7322fff48 100755
> --- a/tests/checkpatch.at
> +++ b/tests/checkpatch.at
> @@ -8,7 +8,14 @@ OVS_START_SHELL_HELPERS
>  try_checkpatch() {
>      # Take the patch to test from $1.  Remove an initial four-space indent
>      # from it and, if it is just headers with no body, add a null body.
> +    # If it does not have a 'Subject', add a valid one.
>      echo "$1" | sed 's/^    //' > test.patch
> +    if grep 'Subject\:' test.patch >/dev/null 2>&1; then :
> +    else
> +        sed -i'' -e '1i\
> +Subject: Patch this is.
> +' test.patch
> +    fi
>      if grep '---' expout >/dev/null 2>&1; then :
>      else
>          printf '\n---\n' >> test.patch
> @@ -236,6 +243,22 @@ done
>  AT_CLEANUP
>
>
> +AT_SETUP([checkpatch - catastrophic backtracking])
> +dnl Special case this rather than using the above construct because sometimes a
> +dnl warning needs to be generated for line lengths (f.e. when the 'while'
> +dnl keyword is used).
> +try_checkpatch \
> +   "COMMON_PATCH_HEADER
> +    +     if (!b_ctx_in->chassis_rec || !b_ctx_in->br_int || !b_ctx_in->ovs_idl_txn)
> +    " \
> +    "ERROR: Inappropriate bracing around statement
> +    #8 FILE: A.c:1:
> +         if (!b_ctx_in->chassis_rec || !b_ctx_in->br_int || !b_ctx_in->ovs_idl_txn)
> +"
> +
> +AT_CLEANUP
> +
> +
>  AT_SETUP([checkpatch - parenthesized constructs - for])
>  try_checkpatch \
>     "COMMON_PATCH_HEADER
> @@ -560,3 +583,25 @@ try_checkpatch \
>  "
>
>  AT_CLEANUP
> +
> +AT_SETUP([checkpatch - subject])
> +try_checkpatch \
> +   "Author: A
> +    Commit: A
> +    Subject: netdev: invalid case and dot ending
> +
> +    Signed-off-by: A" \
> +    "WARNING: The subject summary should start with a capital.
> +    WARNING: The subject summary should end with a dot.
> +    Subject: netdev: invalid case and dot ending"
> +
> +try_checkpatch \
> +   "Author: A
> +    Commit: A
> +    Subject: netdev: This is a way to long commit summary and therefor it should report a WARNING!
> +
> +    Signed-off-by: A" \
> +    "WARNING: The subject, '<area>: <summary>', is over 70 characters, i.e., 85.
> +    Subject: netdev: This is a way to long commit summary and therefor it should report a WARNING!"
> +
> +AT_CLEANUP
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 5467d604d1..52d3fa8455 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -40,6 +40,15 @@ MAX_LINE_LEN = 79
>  def open_spell_check_dict():
>      import enchant
>
> +    try:
> +        import codespell_lib
> +        codespell_dir = os.path.dirname(codespell_lib.__file__)
> +        codespell_file = os.path.join(codespell_dir, 'data', 'dictionary.txt')
> +        if not os.path.exists(codespell_file):
> +            codespell_file = ''
> +    except:
> +        codespell_file = ''
> +
>      try:
>          extra_keywords = ['ovs', 'vswitch', 'vswitchd', 'ovs-vswitchd',
>                            'netdev', 'selinux', 'ovs-ctl', 'dpctl', 'ofctl',
> @@ -92,9 +101,18 @@ def open_spell_check_dict():
>                            'syscall', 'lacp', 'ipf', 'skb', 'valgrind']
>
>          global spell_check_dict
> +
>          spell_check_dict = enchant.Dict("en_US")
> +
> +        if codespell_file:
> +            with open(codespell_file) as f:
> +                for line in f.readlines():
> +                    words = line.strip().split('>')[1].strip(', ').split(',')
> +                    for word in words:
> +                        spell_check_dict.add_to_session(word.strip())
> +
>          for kw in extra_keywords:
> -            spell_check_dict.add(kw)
> +            spell_check_dict.add_to_session(kw)
>
>          return True
>      except:
> @@ -190,6 +208,7 @@ skip_trailing_whitespace_check = False
>  skip_gerrit_change_id_check = False
>  skip_block_whitespace_check = False
>  skip_signoff_check = False
> +skip_committer_signoff_check = False
>
>  # Don't enforce character limit on files that include these characters in their
>  # name, as they may have legitimate reasons to have longer lines.
> @@ -282,9 +301,13 @@ def if_and_for_end_with_bracket_check(line):
>          if len(line) == MAX_LINE_LEN - 1 and line[-1] == ')':
>              return True
>
> -        if __regex_ends_with_bracket.search(line) is None and \
> -           __regex_if_macros.match(line) is None:
> -            return False
> +        if __regex_ends_with_bracket.search(line) is None:
> +            if line.endswith("\\") and \
> +               __regex_if_macros.match(line) is not None:
> +                return True
> +            else:
> +                return False
> +
>      if __regex_conditional_else_bracing.match(line) is not None:
>          return False
>      if __regex_conditional_else_bracing2.match(line) is not None:
> @@ -410,9 +433,15 @@ def check_spelling(line, comment):
>      if not spell_check_dict or not spellcheck:
>          return False
>
> +    if line.startswith('Fixes: '):
> +        return False
> +
>      words = filter_comments(line, True) if comment else line
>      words = words.replace(':', ' ').split(' ')
>
> +    flagged_words = []
> +    num_suggestions = 3
> +
>      for word in words:
>          skip = False
>          strword = re.subn(r'\W+', '', word)[0].replace(',', '')
> @@ -437,9 +466,15 @@ def check_spelling(line, comment):
>                  skip = True
>
>              if not skip:
> -                print_warning("Check for spelling mistakes (e.g. \"%s\")"
> -                              % strword)
> -                return True
> +                flagged_words.append(strword)
> +
> +    if len(flagged_words) > 0:
> +        for mistake in flagged_words:
> +            print_warning("Possible misspelled word: \"%s\"" % mistake)
> +            print("Did you mean: ",
> +                  spell_check_dict.suggest(mistake)[:num_suggestions])
> +
> +        return True
>
>      return False
>
> @@ -780,6 +815,36 @@ def run_file_checks(text):
>              check['check'](text)
>
>
> +def run_subject_checks(subject, spellcheck=False):
> +    warnings = False
> +
> +    if spellcheck and check_spelling(subject, False):
> +        warnings = True
> +
> +    summary = subject[subject.rindex(': ') + 2:]
> +    area_summary = subject[subject.index(': ') + 2:]
> +    area_summary_len = len(area_summary)
> +    if area_summary_len > 70:
> +        print_warning("The subject, '<area>: <summary>', is over 70 "
> +                      "characters, i.e., %u." % area_summary_len)
> +        warnings = True
> +
> +    if summary[0].isalpha() and summary[0].islower():
> +        print_warning(
> +            "The subject summary should start with a capital.")
> +        warnings = True
> +
> +    if subject[-1] not in [".", "?", "!"]:
> +        print_warning(
> +            "The subject summary should end with a dot.")
> +        warnings = True
> +
> +    if warnings:
> +        print(subject)
> +
> +    return warnings
> +
> +
>  def ovs_checkpatch_parse(text, filename, author=None, committer=None):
>      global print_file_name, total_line, checking_file, \
>          empty_return_check_state
> @@ -800,6 +865,7 @@ def ovs_checkpatch_parse(text, filename, author=None, committer=None):
>          r'^@@ ([0-9-+]+),([0-9-+]+) ([0-9-+]+),([0-9-+]+) @@')
>      is_author = re.compile(r'^(Author|From): (.*)$', re.I | re.M | re.S)
>      is_committer = re.compile(r'^(Commit: )(.*)$', re.I | re.M | re.S)
> +    is_subject = re.compile(r'^(Subject: )(.*)$', re.I | re.M | re.S)
>      is_signature = re.compile(r'^(Signed-off-by: )(.*)$',
>                                re.I | re.M | re.S)
>      is_co_author = re.compile(r'^(Co-authored-by: )(.*)$',
> @@ -874,7 +940,8 @@ def ovs_checkpatch_parse(text, filename, author=None, committer=None):
>                              break
>                      if (committer
>                          and author != committer
> -                        and committer not in signatures):
> +                        and committer not in signatures
> +                        and not skip_committer_signoff_check):
>                          print_error("Committer %s needs to sign off."
>                                      % committer)
>
> @@ -899,6 +966,8 @@ def ovs_checkpatch_parse(text, filename, author=None, committer=None):
>                  committer = is_committer.match(line).group(2)
>              elif is_author.match(line):
>                  author = is_author.match(line).group(2)
> +            elif is_subject.match(line):
> +                run_subject_checks(line, spellcheck)
>              elif is_signature.match(line):
>                  m = is_signature.match(line)
>                  signatures.append(m.group(2))
> @@ -990,7 +1059,8 @@ Check options:
>  -S|--spellcheck                Check C comments and commit-message for possible
>                                 spelling mistakes
>  -t|--skip-trailing-whitespace  Skips the trailing whitespace test
> -   --skip-gerrit-change-id     Skips the gerrit change id test"""
> +   --skip-gerrit-change-id     Skips the gerrit change id test
> +   --skip-committer-signoff    Skips the committer sign-off test"""
>            % sys.argv[0])
>
>
> @@ -1017,6 +1087,19 @@ def ovs_checkpatch_file(filename):
>      result = ovs_checkpatch_parse(part.get_payload(decode=False), filename,
>                                    mail.get('Author', mail['From']),
>                                    mail['Commit'])
> +
> +    if not mail['Subject'] or not mail['Subject'].strip():
> +        if mail['Subject']:
> +            mail.replace_header('Subject', sys.argv[-1])
> +        else:
> +            mail.add_header('Subject', sys.argv[-1])
> +
> +        print("Subject missing! Your provisional subject is",
> +              mail['Subject'])
> +
> +    if run_subject_checks('Subject: ' + mail['Subject'], spellcheck):
> +        result = True
> +
>      ovs_checkpatch_print_result()
>      return result
>
> @@ -1048,6 +1131,7 @@ if __name__ == '__main__':
>                                         "skip-signoff-lines",
>                                         "skip-trailing-whitespace",
>                                         "skip-gerrit-change-id",
> +                                       "skip-committer-signoff",
>                                         "spellcheck",
>                                         "quiet"])
>      except:
> @@ -1068,6 +1152,8 @@ if __name__ == '__main__':
>              skip_trailing_whitespace_check = True
>          elif o in ("--skip-gerrit-change-id"):
>              skip_gerrit_change_id_check = True
> +        elif o in ("--skip-committer-signoff"):
> +            skip_committer_signoff_check = True
>          elif o in ("-f", "--check-file"):
>              checking_file = True
>          elif o in ("-S", "--spellcheck"):
> --
> 2.39.3
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Dumitru Ceara Jan. 17, 2024, 10:03 a.m. UTC | #2 
On 1/16/24 14:08, Dumitru Ceara wrote:
> This picks up the following OVS changes:
>   00d3d4a7d375 ("checkpatch: Avoid catastrophic backtracking.")
>   d25c6bd8df37 ("checkpatch: Reorganize flagged words using a list.")
>   9a50170a805a ("checkpatch: Add suggestions to the spell checker.")
>   799f697e51ec ("checkpatch: Print subject field if misspelled or missing.")
>   1b8fa4a66aa4 ("checkpatch: Add checks for the subject line.")
>   bf843fd439b2 ("checkpatch: Don't spell check Fixes tag.")
>   74bfe3701407 ("checkpatch: Add argument to skip committer signoff check.")
>   21c61243fb75 ("checkpatch: Fix personal word list storage.")
>   915b97971d58 ("checkpatch.py: Load codespell dictionary.")
> 
> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> ---

Recheck-request: github-robot
Dumitru Ceara Jan. 17, 2024, 12:51 p.m. UTC | #3 
On 1/17/24 11:03, Dumitru Ceara wrote:
> On 1/16/24 14:08, Dumitru Ceara wrote:
>> This picks up the following OVS changes:
>>   00d3d4a7d375 ("checkpatch: Avoid catastrophic backtracking.")
>>   d25c6bd8df37 ("checkpatch: Reorganize flagged words using a list.")
>>   9a50170a805a ("checkpatch: Add suggestions to the spell checker.")
>>   799f697e51ec ("checkpatch: Print subject field if misspelled or missing.")
>>   1b8fa4a66aa4 ("checkpatch: Add checks for the subject line.")
>>   bf843fd439b2 ("checkpatch: Don't spell check Fixes tag.")
>>   74bfe3701407 ("checkpatch: Add argument to skip committer signoff check.")
>>   21c61243fb75 ("checkpatch: Fix personal word list storage.")
>>   915b97971d58 ("checkpatch.py: Load codespell dictionary.")
>>
>> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
>> ---
> 
> Recheck-request: github-robot

Recheck-request: github-robot
Mark Michelson Jan. 17, 2024, 8:40 p.m. UTC | #4 
On 1/16/24 11:23, Numan Siddique wrote:
> On Tue, Jan 16, 2024 at 8:09 AM Dumitru Ceara <dceara@redhat.com> wrote:
>>
>> This picks up the following OVS changes:
>>    00d3d4a7d375 ("checkpatch: Avoid catastrophic backtracking.")
>>    d25c6bd8df37 ("checkpatch: Reorganize flagged words using a list.")
>>    9a50170a805a ("checkpatch: Add suggestions to the spell checker.")
>>    799f697e51ec ("checkpatch: Print subject field if misspelled or missing.")
>>    1b8fa4a66aa4 ("checkpatch: Add checks for the subject line.")
>>    bf843fd439b2 ("checkpatch: Don't spell check Fixes tag.")
>>    74bfe3701407 ("checkpatch: Add argument to skip committer signoff check.")
>>    21c61243fb75 ("checkpatch: Fix personal word list storage.")
>>    915b97971d58 ("checkpatch.py: Load codespell dictionary.")
>>
>> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> 
> Thanks for the patch.
> 
> Acked-by: Numan Siddique <numans@ovn.org>
> 
> Numan

Thank you Dumitru and Numan.

I merged this to main and all OVN branches back to 22.12.

I'm curious about something. Since we have the OVS submodule, and there 
are no OVN-specific additions to checkpatch (that I'm aware of anyway), 
would it make sense to remove the checkpatch from OVN and just rely on 
the OVS submodule's checkpatch? This way, whenever we do a submodule 
bump, we'd automatically pull in the checkpatch modifications.

What do you think?

> 
>> ---
>>   tests/checkpatch.at     |  45 +++++++++++++++++
>>   utilities/checkpatch.py | 104 ++++++++++++++++++++++++++++++++++++----
>>   2 files changed, 140 insertions(+), 9 deletions(-)
>>
>> diff --git a/tests/checkpatch.at b/tests/checkpatch.at
>> index 26f3197053..e7322fff48 100755
>> --- a/tests/checkpatch.at
>> +++ b/tests/checkpatch.at
>> @@ -8,7 +8,14 @@ OVS_START_SHELL_HELPERS
>>   try_checkpatch() {
>>       # Take the patch to test from $1.  Remove an initial four-space indent
>>       # from it and, if it is just headers with no body, add a null body.
>> +    # If it does not have a 'Subject', add a valid one.
>>       echo "$1" | sed 's/^    //' > test.patch
>> +    if grep 'Subject\:' test.patch >/dev/null 2>&1; then :
>> +    else
>> +        sed -i'' -e '1i\
>> +Subject: Patch this is.
>> +' test.patch
>> +    fi
>>       if grep '---' expout >/dev/null 2>&1; then :
>>       else
>>           printf '\n---\n' >> test.patch
>> @@ -236,6 +243,22 @@ done
>>   AT_CLEANUP
>>
>>
>> +AT_SETUP([checkpatch - catastrophic backtracking])
>> +dnl Special case this rather than using the above construct because sometimes a
>> +dnl warning needs to be generated for line lengths (f.e. when the 'while'
>> +dnl keyword is used).
>> +try_checkpatch \
>> +   "COMMON_PATCH_HEADER
>> +    +     if (!b_ctx_in->chassis_rec || !b_ctx_in->br_int || !b_ctx_in->ovs_idl_txn)
>> +    " \
>> +    "ERROR: Inappropriate bracing around statement
>> +    #8 FILE: A.c:1:
>> +         if (!b_ctx_in->chassis_rec || !b_ctx_in->br_int || !b_ctx_in->ovs_idl_txn)
>> +"
>> +
>> +AT_CLEANUP
>> +
>> +
>>   AT_SETUP([checkpatch - parenthesized constructs - for])
>>   try_checkpatch \
>>      "COMMON_PATCH_HEADER
>> @@ -560,3 +583,25 @@ try_checkpatch \
>>   "
>>
>>   AT_CLEANUP
>> +
>> +AT_SETUP([checkpatch - subject])
>> +try_checkpatch \
>> +   "Author: A
>> +    Commit: A
>> +    Subject: netdev: invalid case and dot ending
>> +
>> +    Signed-off-by: A" \
>> +    "WARNING: The subject summary should start with a capital.
>> +    WARNING: The subject summary should end with a dot.
>> +    Subject: netdev: invalid case and dot ending"
>> +
>> +try_checkpatch \
>> +   "Author: A
>> +    Commit: A
>> +    Subject: netdev: This is a way to long commit summary and therefor it should report a WARNING!
>> +
>> +    Signed-off-by: A" \
>> +    "WARNING: The subject, '<area>: <summary>', is over 70 characters, i.e., 85.
>> +    Subject: netdev: This is a way to long commit summary and therefor it should report a WARNING!"
>> +
>> +AT_CLEANUP
>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
>> index 5467d604d1..52d3fa8455 100755
>> --- a/utilities/checkpatch.py
>> +++ b/utilities/checkpatch.py
>> @@ -40,6 +40,15 @@ MAX_LINE_LEN = 79
>>   def open_spell_check_dict():
>>       import enchant
>>
>> +    try:
>> +        import codespell_lib
>> +        codespell_dir = os.path.dirname(codespell_lib.__file__)
>> +        codespell_file = os.path.join(codespell_dir, 'data', 'dictionary.txt')
>> +        if not os.path.exists(codespell_file):
>> +            codespell_file = ''
>> +    except:
>> +        codespell_file = ''
>> +
>>       try:
>>           extra_keywords = ['ovs', 'vswitch', 'vswitchd', 'ovs-vswitchd',
>>                             'netdev', 'selinux', 'ovs-ctl', 'dpctl', 'ofctl',
>> @@ -92,9 +101,18 @@ def open_spell_check_dict():
>>                             'syscall', 'lacp', 'ipf', 'skb', 'valgrind']
>>
>>           global spell_check_dict
>> +
>>           spell_check_dict = enchant.Dict("en_US")
>> +
>> +        if codespell_file:
>> +            with open(codespell_file) as f:
>> +                for line in f.readlines():
>> +                    words = line.strip().split('>')[1].strip(', ').split(',')
>> +                    for word in words:
>> +                        spell_check_dict.add_to_session(word.strip())
>> +
>>           for kw in extra_keywords:
>> -            spell_check_dict.add(kw)
>> +            spell_check_dict.add_to_session(kw)
>>
>>           return True
>>       except:
>> @@ -190,6 +208,7 @@ skip_trailing_whitespace_check = False
>>   skip_gerrit_change_id_check = False
>>   skip_block_whitespace_check = False
>>   skip_signoff_check = False
>> +skip_committer_signoff_check = False
>>
>>   # Don't enforce character limit on files that include these characters in their
>>   # name, as they may have legitimate reasons to have longer lines.
>> @@ -282,9 +301,13 @@ def if_and_for_end_with_bracket_check(line):
>>           if len(line) == MAX_LINE_LEN - 1 and line[-1] == ')':
>>               return True
>>
>> -        if __regex_ends_with_bracket.search(line) is None and \
>> -           __regex_if_macros.match(line) is None:
>> -            return False
>> +        if __regex_ends_with_bracket.search(line) is None:
>> +            if line.endswith("\\") and \
>> +               __regex_if_macros.match(line) is not None:
>> +                return True
>> +            else:
>> +                return False
>> +
>>       if __regex_conditional_else_bracing.match(line) is not None:
>>           return False
>>       if __regex_conditional_else_bracing2.match(line) is not None:
>> @@ -410,9 +433,15 @@ def check_spelling(line, comment):
>>       if not spell_check_dict or not spellcheck:
>>           return False
>>
>> +    if line.startswith('Fixes: '):
>> +        return False
>> +
>>       words = filter_comments(line, True) if comment else line
>>       words = words.replace(':', ' ').split(' ')
>>
>> +    flagged_words = []
>> +    num_suggestions = 3
>> +
>>       for word in words:
>>           skip = False
>>           strword = re.subn(r'\W+', '', word)[0].replace(',', '')
>> @@ -437,9 +466,15 @@ def check_spelling(line, comment):
>>                   skip = True
>>
>>               if not skip:
>> -                print_warning("Check for spelling mistakes (e.g. \"%s\")"
>> -                              % strword)
>> -                return True
>> +                flagged_words.append(strword)
>> +
>> +    if len(flagged_words) > 0:
>> +        for mistake in flagged_words:
>> +            print_warning("Possible misspelled word: \"%s\"" % mistake)
>> +            print("Did you mean: ",
>> +                  spell_check_dict.suggest(mistake)[:num_suggestions])
>> +
>> +        return True
>>
>>       return False
>>
>> @@ -780,6 +815,36 @@ def run_file_checks(text):
>>               check['check'](text)
>>
>>
>> +def run_subject_checks(subject, spellcheck=False):
>> +    warnings = False
>> +
>> +    if spellcheck and check_spelling(subject, False):
>> +        warnings = True
>> +
>> +    summary = subject[subject.rindex(': ') + 2:]
>> +    area_summary = subject[subject.index(': ') + 2:]
>> +    area_summary_len = len(area_summary)
>> +    if area_summary_len > 70:
>> +        print_warning("The subject, '<area>: <summary>', is over 70 "
>> +                      "characters, i.e., %u." % area_summary_len)
>> +        warnings = True
>> +
>> +    if summary[0].isalpha() and summary[0].islower():
>> +        print_warning(
>> +            "The subject summary should start with a capital.")
>> +        warnings = True
>> +
>> +    if subject[-1] not in [".", "?", "!"]:
>> +        print_warning(
>> +            "The subject summary should end with a dot.")
>> +        warnings = True
>> +
>> +    if warnings:
>> +        print(subject)
>> +
>> +    return warnings
>> +
>> +
>>   def ovs_checkpatch_parse(text, filename, author=None, committer=None):
>>       global print_file_name, total_line, checking_file, \
>>           empty_return_check_state
>> @@ -800,6 +865,7 @@ def ovs_checkpatch_parse(text, filename, author=None, committer=None):
>>           r'^@@ ([0-9-+]+),([0-9-+]+) ([0-9-+]+),([0-9-+]+) @@')
>>       is_author = re.compile(r'^(Author|From): (.*)$', re.I | re.M | re.S)
>>       is_committer = re.compile(r'^(Commit: )(.*)$', re.I | re.M | re.S)
>> +    is_subject = re.compile(r'^(Subject: )(.*)$', re.I | re.M | re.S)
>>       is_signature = re.compile(r'^(Signed-off-by: )(.*)$',
>>                                 re.I | re.M | re.S)
>>       is_co_author = re.compile(r'^(Co-authored-by: )(.*)$',
>> @@ -874,7 +940,8 @@ def ovs_checkpatch_parse(text, filename, author=None, committer=None):
>>                               break
>>                       if (committer
>>                           and author != committer
>> -                        and committer not in signatures):
>> +                        and committer not in signatures
>> +                        and not skip_committer_signoff_check):
>>                           print_error("Committer %s needs to sign off."
>>                                       % committer)
>>
>> @@ -899,6 +966,8 @@ def ovs_checkpatch_parse(text, filename, author=None, committer=None):
>>                   committer = is_committer.match(line).group(2)
>>               elif is_author.match(line):
>>                   author = is_author.match(line).group(2)
>> +            elif is_subject.match(line):
>> +                run_subject_checks(line, spellcheck)
>>               elif is_signature.match(line):
>>                   m = is_signature.match(line)
>>                   signatures.append(m.group(2))
>> @@ -990,7 +1059,8 @@ Check options:
>>   -S|--spellcheck                Check C comments and commit-message for possible
>>                                  spelling mistakes
>>   -t|--skip-trailing-whitespace  Skips the trailing whitespace test
>> -   --skip-gerrit-change-id     Skips the gerrit change id test"""
>> +   --skip-gerrit-change-id     Skips the gerrit change id test
>> +   --skip-committer-signoff    Skips the committer sign-off test"""
>>             % sys.argv[0])
>>
>>
>> @@ -1017,6 +1087,19 @@ def ovs_checkpatch_file(filename):
>>       result = ovs_checkpatch_parse(part.get_payload(decode=False), filename,
>>                                     mail.get('Author', mail['From']),
>>                                     mail['Commit'])
>> +
>> +    if not mail['Subject'] or not mail['Subject'].strip():
>> +        if mail['Subject']:
>> +            mail.replace_header('Subject', sys.argv[-1])
>> +        else:
>> +            mail.add_header('Subject', sys.argv[-1])
>> +
>> +        print("Subject missing! Your provisional subject is",
>> +              mail['Subject'])
>> +
>> +    if run_subject_checks('Subject: ' + mail['Subject'], spellcheck):
>> +        result = True
>> +
>>       ovs_checkpatch_print_result()
>>       return result
>>
>> @@ -1048,6 +1131,7 @@ if __name__ == '__main__':
>>                                          "skip-signoff-lines",
>>                                          "skip-trailing-whitespace",
>>                                          "skip-gerrit-change-id",
>> +                                       "skip-committer-signoff",
>>                                          "spellcheck",
>>                                          "quiet"])
>>       except:
>> @@ -1068,6 +1152,8 @@ if __name__ == '__main__':
>>               skip_trailing_whitespace_check = True
>>           elif o in ("--skip-gerrit-change-id"):
>>               skip_gerrit_change_id_check = True
>> +        elif o in ("--skip-committer-signoff"):
>> +            skip_committer_signoff_check = True
>>           elif o in ("-f", "--check-file"):
>>               checking_file = True
>>           elif o in ("-S", "--spellcheck"):
>> --
>> 2.39.3
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Dumitru Ceara Jan. 17, 2024, 9:33 p.m. UTC | #5 
On 1/17/24 21:40, Mark Michelson wrote:
> On 1/16/24 11:23, Numan Siddique wrote:
>> On Tue, Jan 16, 2024 at 8:09 AM Dumitru Ceara <dceara@redhat.com> wrote:
>>>
>>> This picks up the following OVS changes:
>>>    00d3d4a7d375 ("checkpatch: Avoid catastrophic backtracking.")
>>>    d25c6bd8df37 ("checkpatch: Reorganize flagged words using a list.")
>>>    9a50170a805a ("checkpatch: Add suggestions to the spell checker.")
>>>    799f697e51ec ("checkpatch: Print subject field if misspelled or
>>> missing.")
>>>    1b8fa4a66aa4 ("checkpatch: Add checks for the subject line.")
>>>    bf843fd439b2 ("checkpatch: Don't spell check Fixes tag.")
>>>    74bfe3701407 ("checkpatch: Add argument to skip committer signoff
>>> check.")
>>>    21c61243fb75 ("checkpatch: Fix personal word list storage.")
>>>    915b97971d58 ("checkpatch.py: Load codespell dictionary.")
>>>
>>> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
>>
>> Thanks for the patch.
>>
>> Acked-by: Numan Siddique <numans@ovn.org>
>>
>> Numan
> 
> Thank you Dumitru and Numan.
> 
> I merged this to main and all OVN branches back to 22.12.
> 

Thanks, Numan and Mark!

> I'm curious about something. Since we have the OVS submodule, and there
> are no OVN-specific additions to checkpatch (that I'm aware of anyway),
> would it make sense to remove the checkpatch from OVN and just rely on
> the OVS submodule's checkpatch? This way, whenever we do a submodule
> bump, we'd automatically pull in the checkpatch modifications.
> 
> What do you think?
> 

+1 I think we do have one OVN-specific change to checkpatch but I'm sure
we can figure out how to have it ported to the OVS one if needed:

https://github.com/ovn-org/ovn/commit/04a5527c6a8657b682450f71f7ee681512557b36

On second thought maybe we don't even need this one ported.  I think OVS
has its own mitigation in place.

Another thing that took me by surprise is that 0-day bot always uses the
OVS checkpatch script (that's why it didn't choke on Numan's I-P patch
like OVN checkpatch did for me locally) - cc-ing Aaron.

Regards,
Dumitru
Numan Siddique Jan. 17, 2024, 9:48 p.m. UTC | #6 
On Wed, Jan 17, 2024 at 4:33 PM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 1/17/24 21:40, Mark Michelson wrote:
> > On 1/16/24 11:23, Numan Siddique wrote:
> >> On Tue, Jan 16, 2024 at 8:09 AM Dumitru Ceara <dceara@redhat.com> wrote:
> >>>
> >>> This picks up the following OVS changes:
> >>>    00d3d4a7d375 ("checkpatch: Avoid catastrophic backtracking.")
> >>>    d25c6bd8df37 ("checkpatch: Reorganize flagged words using a list.")
> >>>    9a50170a805a ("checkpatch: Add suggestions to the spell checker.")
> >>>    799f697e51ec ("checkpatch: Print subject field if misspelled or
> >>> missing.")
> >>>    1b8fa4a66aa4 ("checkpatch: Add checks for the subject line.")
> >>>    bf843fd439b2 ("checkpatch: Don't spell check Fixes tag.")
> >>>    74bfe3701407 ("checkpatch: Add argument to skip committer signoff
> >>> check.")
> >>>    21c61243fb75 ("checkpatch: Fix personal word list storage.")
> >>>    915b97971d58 ("checkpatch.py: Load codespell dictionary.")
> >>>
> >>> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> >>
> >> Thanks for the patch.
> >>
> >> Acked-by: Numan Siddique <numans@ovn.org>
> >>
> >> Numan
> >
> > Thank you Dumitru and Numan.
> >
> > I merged this to main and all OVN branches back to 22.12.
> >
>
> Thanks, Numan and Mark!
>
> > I'm curious about something. Since we have the OVS submodule, and there
> > are no OVN-specific additions to checkpatch (that I'm aware of anyway),
> > would it make sense to remove the checkpatch from OVN and just rely on
> > the OVS submodule's checkpatch? This way, whenever we do a submodule
> > bump, we'd automatically pull in the checkpatch modifications.
> >
> > What do you think?
> >
>
> +1 I think we do have one OVN-specific change to checkpatch but I'm sure
> we can figure out how to have it ported to the OVS one if needed:

+1.  Makes sense.  Can we have a symbolic link in OVN
utilities/checkpatch.py which points to ovs/utilities/checkpatch.py ?

Numan

>
> https://github.com/ovn-org/ovn/commit/04a5527c6a8657b682450f71f7ee681512557b36
>
> On second thought maybe we don't even need this one ported.  I think OVS
> has its own mitigation in place.
>
> Another thing that took me by surprise is that 0-day bot always uses the
> OVS checkpatch script (that's why it didn't choke on Numan's I-P patch
> like OVN checkpatch did for me locally) - cc-ing Aaron.
>
> Regards,
> Dumitru
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Mark Michelson Jan. 18, 2024, 5:13 p.m. UTC | #7 
On 1/17/24 16:48, Numan Siddique wrote:
> On Wed, Jan 17, 2024 at 4:33 PM Dumitru Ceara <dceara@redhat.com> wrote:
>>
>> On 1/17/24 21:40, Mark Michelson wrote:
>>> On 1/16/24 11:23, Numan Siddique wrote:
>>>> On Tue, Jan 16, 2024 at 8:09 AM Dumitru Ceara <dceara@redhat.com> wrote:
>>>>>
>>>>> This picks up the following OVS changes:
>>>>>     00d3d4a7d375 ("checkpatch: Avoid catastrophic backtracking.")
>>>>>     d25c6bd8df37 ("checkpatch: Reorganize flagged words using a list.")
>>>>>     9a50170a805a ("checkpatch: Add suggestions to the spell checker.")
>>>>>     799f697e51ec ("checkpatch: Print subject field if misspelled or
>>>>> missing.")
>>>>>     1b8fa4a66aa4 ("checkpatch: Add checks for the subject line.")
>>>>>     bf843fd439b2 ("checkpatch: Don't spell check Fixes tag.")
>>>>>     74bfe3701407 ("checkpatch: Add argument to skip committer signoff
>>>>> check.")
>>>>>     21c61243fb75 ("checkpatch: Fix personal word list storage.")
>>>>>     915b97971d58 ("checkpatch.py: Load codespell dictionary.")
>>>>>
>>>>> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
>>>>
>>>> Thanks for the patch.
>>>>
>>>> Acked-by: Numan Siddique <numans@ovn.org>
>>>>
>>>> Numan
>>>
>>> Thank you Dumitru and Numan.
>>>
>>> I merged this to main and all OVN branches back to 22.12.
>>>
>>
>> Thanks, Numan and Mark!
>>
>>> I'm curious about something. Since we have the OVS submodule, and there
>>> are no OVN-specific additions to checkpatch (that I'm aware of anyway),
>>> would it make sense to remove the checkpatch from OVN and just rely on
>>> the OVS submodule's checkpatch? This way, whenever we do a submodule
>>> bump, we'd automatically pull in the checkpatch modifications.
>>>
>>> What do you think?
>>>
>>
>> +1 I think we do have one OVN-specific change to checkpatch but I'm sure
>> we can figure out how to have it ported to the OVS one if needed:
> 
> +1.  Makes sense.  Can we have a symbolic link in OVN
> utilities/checkpatch.py which points to ovs/utilities/checkpatch.py ?
> 
> Numan

My only concern with this is that I'm unsure about the portability of 
symbolic links. Would a Windows machine handle the symlink properly?

> 
>>
>> https://github.com/ovn-org/ovn/commit/04a5527c6a8657b682450f71f7ee681512557b36
>>
>> On second thought maybe we don't even need this one ported.  I think OVS
>> has its own mitigation in place.
>>
>> Another thing that took me by surprise is that 0-day bot always uses the
>> OVS checkpatch script (that's why it didn't choke on Numan's I-P patch
>> like OVN checkpatch did for me locally) - cc-ing Aaron.
>>
>> Regards,
>> Dumitru
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Aaron Conole Jan. 19, 2024, 3:02 p.m. UTC | #8 
Dumitru Ceara <dceara@redhat.com> writes:

> On 1/17/24 21:40, Mark Michelson wrote:
>> On 1/16/24 11:23, Numan Siddique wrote:
>>> On Tue, Jan 16, 2024 at 8:09 AM Dumitru Ceara <dceara@redhat.com> wrote:
>>>>
>>>> This picks up the following OVS changes:
>>>>    00d3d4a7d375 ("checkpatch: Avoid catastrophic backtracking.")
>>>>    d25c6bd8df37 ("checkpatch: Reorganize flagged words using a list.")
>>>>    9a50170a805a ("checkpatch: Add suggestions to the spell checker.")
>>>>    799f697e51ec ("checkpatch: Print subject field if misspelled or
>>>> missing.")
>>>>    1b8fa4a66aa4 ("checkpatch: Add checks for the subject line.")
>>>>    bf843fd439b2 ("checkpatch: Don't spell check Fixes tag.")
>>>>    74bfe3701407 ("checkpatch: Add argument to skip committer signoff
>>>> check.")
>>>>    21c61243fb75 ("checkpatch: Fix personal word list storage.")
>>>>    915b97971d58 ("checkpatch.py: Load codespell dictionary.")
>>>>
>>>> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
>>>
>>> Thanks for the patch.
>>>
>>> Acked-by: Numan Siddique <numans@ovn.org>
>>>
>>> Numan
>> 
>> Thank you Dumitru and Numan.
>> 
>> I merged this to main and all OVN branches back to 22.12.
>> 
>
> Thanks, Numan and Mark!
>
>> I'm curious about something. Since we have the OVS submodule, and there
>> are no OVN-specific additions to checkpatch (that I'm aware of anyway),
>> would it make sense to remove the checkpatch from OVN and just rely on
>> the OVS submodule's checkpatch? This way, whenever we do a submodule
>> bump, we'd automatically pull in the checkpatch modifications.
>> 
>> What do you think?
>> 
>
> +1 I think we do have one OVN-specific change to checkpatch but I'm sure
> we can figure out how to have it ported to the OVS one if needed:
>
> https://github.com/ovn-org/ovn/commit/04a5527c6a8657b682450f71f7ee681512557b36
>
> On second thought maybe we don't even need this one ported.  I think OVS
> has its own mitigation in place.

Yep - exactly.

> Another thing that took me by surprise is that 0-day bot always uses the
> OVS checkpatch script (that's why it didn't choke on Numan's I-P patch
> like OVN checkpatch did for me locally) - cc-ing Aaron.

It does?  I thought it should be using the checkpatch that is included in
the OVN repo.  That sounds like a bug.  But if you decide to switch to
using the OVS one then maybe it becomes a feature. :)

> Regards,
> Dumitru
diff mbox series

Patch

diff --git a/tests/checkpatch.at b/tests/checkpatch.at
index 26f3197053..e7322fff48 100755
--- a/tests/checkpatch.at
+++ b/tests/checkpatch.at
@@ -8,7 +8,14 @@  OVS_START_SHELL_HELPERS
 try_checkpatch() {
     # Take the patch to test from $1.  Remove an initial four-space indent
     # from it and, if it is just headers with no body, add a null body.
+    # If it does not have a 'Subject', add a valid one.
     echo "$1" | sed 's/^    //' > test.patch
+    if grep 'Subject\:' test.patch >/dev/null 2>&1; then :
+    else
+        sed -i'' -e '1i\
+Subject: Patch this is.
+' test.patch
+    fi
     if grep '---' expout >/dev/null 2>&1; then :
     else
         printf '\n---\n' >> test.patch
@@ -236,6 +243,22 @@  done
 AT_CLEANUP
 
 
+AT_SETUP([checkpatch - catastrophic backtracking])
+dnl Special case this rather than using the above construct because sometimes a
+dnl warning needs to be generated for line lengths (f.e. when the 'while'
+dnl keyword is used).
+try_checkpatch \
+   "COMMON_PATCH_HEADER
+    +     if (!b_ctx_in->chassis_rec || !b_ctx_in->br_int || !b_ctx_in->ovs_idl_txn)
+    " \
+    "ERROR: Inappropriate bracing around statement
+    #8 FILE: A.c:1:
+         if (!b_ctx_in->chassis_rec || !b_ctx_in->br_int || !b_ctx_in->ovs_idl_txn)
+"
+
+AT_CLEANUP
+
+
 AT_SETUP([checkpatch - parenthesized constructs - for])
 try_checkpatch \
    "COMMON_PATCH_HEADER
@@ -560,3 +583,25 @@  try_checkpatch \
 "
 
 AT_CLEANUP
+
+AT_SETUP([checkpatch - subject])
+try_checkpatch \
+   "Author: A
+    Commit: A
+    Subject: netdev: invalid case and dot ending
+
+    Signed-off-by: A" \
+    "WARNING: The subject summary should start with a capital.
+    WARNING: The subject summary should end with a dot.
+    Subject: netdev: invalid case and dot ending"
+
+try_checkpatch \
+   "Author: A
+    Commit: A
+    Subject: netdev: This is a way to long commit summary and therefor it should report a WARNING!
+
+    Signed-off-by: A" \
+    "WARNING: The subject, '<area>: <summary>', is over 70 characters, i.e., 85.
+    Subject: netdev: This is a way to long commit summary and therefor it should report a WARNING!"
+
+AT_CLEANUP
diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 5467d604d1..52d3fa8455 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -40,6 +40,15 @@  MAX_LINE_LEN = 79
 def open_spell_check_dict():
     import enchant
 
+    try:
+        import codespell_lib
+        codespell_dir = os.path.dirname(codespell_lib.__file__)
+        codespell_file = os.path.join(codespell_dir, 'data', 'dictionary.txt')
+        if not os.path.exists(codespell_file):
+            codespell_file = ''
+    except:
+        codespell_file = ''
+
     try:
         extra_keywords = ['ovs', 'vswitch', 'vswitchd', 'ovs-vswitchd',
                           'netdev', 'selinux', 'ovs-ctl', 'dpctl', 'ofctl',
@@ -92,9 +101,18 @@  def open_spell_check_dict():
                           'syscall', 'lacp', 'ipf', 'skb', 'valgrind']
 
         global spell_check_dict
+
         spell_check_dict = enchant.Dict("en_US")
+
+        if codespell_file:
+            with open(codespell_file) as f:
+                for line in f.readlines():
+                    words = line.strip().split('>')[1].strip(', ').split(',')
+                    for word in words:
+                        spell_check_dict.add_to_session(word.strip())
+
         for kw in extra_keywords:
-            spell_check_dict.add(kw)
+            spell_check_dict.add_to_session(kw)
 
         return True
     except:
@@ -190,6 +208,7 @@  skip_trailing_whitespace_check = False
 skip_gerrit_change_id_check = False
 skip_block_whitespace_check = False
 skip_signoff_check = False
+skip_committer_signoff_check = False
 
 # Don't enforce character limit on files that include these characters in their
 # name, as they may have legitimate reasons to have longer lines.
@@ -282,9 +301,13 @@  def if_and_for_end_with_bracket_check(line):
         if len(line) == MAX_LINE_LEN - 1 and line[-1] == ')':
             return True
 
-        if __regex_ends_with_bracket.search(line) is None and \
-           __regex_if_macros.match(line) is None:
-            return False
+        if __regex_ends_with_bracket.search(line) is None:
+            if line.endswith("\\") and \
+               __regex_if_macros.match(line) is not None:
+                return True
+            else:
+                return False
+
     if __regex_conditional_else_bracing.match(line) is not None:
         return False
     if __regex_conditional_else_bracing2.match(line) is not None:
@@ -410,9 +433,15 @@  def check_spelling(line, comment):
     if not spell_check_dict or not spellcheck:
         return False
 
+    if line.startswith('Fixes: '):
+        return False
+
     words = filter_comments(line, True) if comment else line
     words = words.replace(':', ' ').split(' ')
 
+    flagged_words = []
+    num_suggestions = 3
+
     for word in words:
         skip = False
         strword = re.subn(r'\W+', '', word)[0].replace(',', '')
@@ -437,9 +466,15 @@  def check_spelling(line, comment):
                 skip = True
 
             if not skip:
-                print_warning("Check for spelling mistakes (e.g. \"%s\")"
-                              % strword)
-                return True
+                flagged_words.append(strword)
+
+    if len(flagged_words) > 0:
+        for mistake in flagged_words:
+            print_warning("Possible misspelled word: \"%s\"" % mistake)
+            print("Did you mean: ",
+                  spell_check_dict.suggest(mistake)[:num_suggestions])
+
+        return True
 
     return False
 
@@ -780,6 +815,36 @@  def run_file_checks(text):
             check['check'](text)
 
 
+def run_subject_checks(subject, spellcheck=False):
+    warnings = False
+
+    if spellcheck and check_spelling(subject, False):
+        warnings = True
+
+    summary = subject[subject.rindex(': ') + 2:]
+    area_summary = subject[subject.index(': ') + 2:]
+    area_summary_len = len(area_summary)
+    if area_summary_len > 70:
+        print_warning("The subject, '<area>: <summary>', is over 70 "
+                      "characters, i.e., %u." % area_summary_len)
+        warnings = True
+
+    if summary[0].isalpha() and summary[0].islower():
+        print_warning(
+            "The subject summary should start with a capital.")
+        warnings = True
+
+    if subject[-1] not in [".", "?", "!"]:
+        print_warning(
+            "The subject summary should end with a dot.")
+        warnings = True
+
+    if warnings:
+        print(subject)
+
+    return warnings
+
+
 def ovs_checkpatch_parse(text, filename, author=None, committer=None):
     global print_file_name, total_line, checking_file, \
         empty_return_check_state
@@ -800,6 +865,7 @@  def ovs_checkpatch_parse(text, filename, author=None, committer=None):
         r'^@@ ([0-9-+]+),([0-9-+]+) ([0-9-+]+),([0-9-+]+) @@')
     is_author = re.compile(r'^(Author|From): (.*)$', re.I | re.M | re.S)
     is_committer = re.compile(r'^(Commit: )(.*)$', re.I | re.M | re.S)
+    is_subject = re.compile(r'^(Subject: )(.*)$', re.I | re.M | re.S)
     is_signature = re.compile(r'^(Signed-off-by: )(.*)$',
                               re.I | re.M | re.S)
     is_co_author = re.compile(r'^(Co-authored-by: )(.*)$',
@@ -874,7 +940,8 @@  def ovs_checkpatch_parse(text, filename, author=None, committer=None):
                             break
                     if (committer
                         and author != committer
-                        and committer not in signatures):
+                        and committer not in signatures
+                        and not skip_committer_signoff_check):
                         print_error("Committer %s needs to sign off."
                                     % committer)
 
@@ -899,6 +966,8 @@  def ovs_checkpatch_parse(text, filename, author=None, committer=None):
                 committer = is_committer.match(line).group(2)
             elif is_author.match(line):
                 author = is_author.match(line).group(2)
+            elif is_subject.match(line):
+                run_subject_checks(line, spellcheck)
             elif is_signature.match(line):
                 m = is_signature.match(line)
                 signatures.append(m.group(2))
@@ -990,7 +1059,8 @@  Check options:
 -S|--spellcheck                Check C comments and commit-message for possible
                                spelling mistakes
 -t|--skip-trailing-whitespace  Skips the trailing whitespace test
-   --skip-gerrit-change-id     Skips the gerrit change id test"""
+   --skip-gerrit-change-id     Skips the gerrit change id test
+   --skip-committer-signoff    Skips the committer sign-off test"""
           % sys.argv[0])
 
 
@@ -1017,6 +1087,19 @@  def ovs_checkpatch_file(filename):
     result = ovs_checkpatch_parse(part.get_payload(decode=False), filename,
                                   mail.get('Author', mail['From']),
                                   mail['Commit'])
+
+    if not mail['Subject'] or not mail['Subject'].strip():
+        if mail['Subject']:
+            mail.replace_header('Subject', sys.argv[-1])
+        else:
+            mail.add_header('Subject', sys.argv[-1])
+
+        print("Subject missing! Your provisional subject is",
+              mail['Subject'])
+
+    if run_subject_checks('Subject: ' + mail['Subject'], spellcheck):
+        result = True
+
     ovs_checkpatch_print_result()
     return result
 
@@ -1048,6 +1131,7 @@  if __name__ == '__main__':
                                        "skip-signoff-lines",
                                        "skip-trailing-whitespace",
                                        "skip-gerrit-change-id",
+                                       "skip-committer-signoff",
                                        "spellcheck",
                                        "quiet"])
     except:
@@ -1068,6 +1152,8 @@  if __name__ == '__main__':
             skip_trailing_whitespace_check = True
         elif o in ("--skip-gerrit-change-id"):
             skip_gerrit_change_id_check = True
+        elif o in ("--skip-committer-signoff"):
+            skip_committer_signoff_check = True
         elif o in ("-f", "--check-file"):
             checking_file = True
         elif o in ("-S", "--spellcheck"):