[ovs-dev,v4] checkpatch: Add new check-authors-file option to checkpatch.py.

This patch adds a new option, --check-authors-file, to the checkpatch
tool to help OVS maintainers check for missing authors in the
AUTHORS.rst file.

Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
---
v3: - Also include co-authors in the check.
    - Only report the end, when all patches are checked.
    - Fixed spelling mistake.
    - Determine git root directory for AUTHORS.rst location.

v4: - Added a test case.
---
 tests/checkpatch.at     | 21 +++++++++++++
 utilities/checkpatch.py | 68 +++++++++++++++++++++++++++++++++++++++--
 2 files changed, 86 insertions(+), 3 deletions(-)

Eelco Chaudron <echaudro@redhat.com> writes:

> This patch adds a new option, --check-authors-file, to the checkpatch
> tool to help OVS maintainers check for missing authors in the
> AUTHORS.rst file.
>
> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
> ---
> v3: - Also include co-authors in the check.
>     - Only report the end, when all patches are checked.
>     - Fixed spelling mistake.
>     - Determine git root directory for AUTHORS.rst location.
>
> v4: - Added a test case.
> ---

Thanks for the quick follow up Eelco.

>  tests/checkpatch.at     | 21 +++++++++++++
>  utilities/checkpatch.py | 68 +++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 86 insertions(+), 3 deletions(-)
>
> diff --git a/tests/checkpatch.at b/tests/checkpatch.at
> index 34971c514..fa179c707 100755
> --- a/tests/checkpatch.at
> +++ b/tests/checkpatch.at
> @@ -634,3 +634,24 @@ try_checkpatch \
>      "--skip-committer-signoff"
>  
>  AT_CLEANUP
> +
> +AT_SETUP([checkpatch - AUTHORS.rst existence])
> +
> +try_checkpatch \
> +   "Author: A <a@a.com>
> +    Commit: A <a@a.com>
> +    Subject: netdev: Subject.
> +
> +    Signed-off-by: A <a@a.com>" \
> +    ""
> +
> +try_checkpatch \
> +   "Author: A <a@a.com>
> +    Commit: A <a@a.com>
> +    Subject: netdev: Subject.
> +
> +    Signed-off-by: A <a@a.com>" \
> +    "WARNING: Author 'A <a@a.com>' is not in the AUTHORS.rst file!" \
> +    "-a"
> +
> +AT_CLEANUP
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 742a0bc47..73c20f804 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -28,12 +28,14 @@ __errors = 0
>  __warnings = 0
>  empty_return_check_state = 0
>  print_file_name = None
> +check_authors_file = False
>  checking_file = False
>  total_line = 0
>  colors = False
>  spellcheck = False
>  quiet = False
>  spell_check_dict = None
> +missing_authors = []
>  
>  
>  def open_spell_check_dict():
> @@ -860,9 +862,41 @@ def run_subject_checks(subject, spellcheck=False):
>      return warnings
>  
>  
> +def get_top_directory():
> +    with os.popen('git rev-parse --show-toplevel') as pipe:
> +        path = pipe.read()
> +
> +    if path:
> +        return path.strip()
> +
> +    return "."
> +
> +
> +def do_authors_exist(authors):
> +    authors = list(set(authors))
> +    missing_authors = []
> +
> +    try:
> +        with open(get_top_directory() + "/AUTHORS.rst", "r") as file:
> +            file_content = file.read()
> +            for author in authors:
> +                m = re.search(r'<(.*?)>', author)
> +                if not m:
> +                    continue
> +                pattern = r'\b' + re.escape(m.group(1)) + r'\b'
> +                if re.search(pattern, file_content) is None:
> +                    missing_authors.append(author)
> +
> +    except FileNotFoundError:
> +        print_error("Could not open AUTHORS.rst in '%s/'!" %
> +                    get_top_directory())
> +
> +    return missing_authors
> +
> +
>  def ovs_checkpatch_parse(text, filename, author=None, committer=None):
>      global print_file_name, total_line, checking_file, \
> -        empty_return_check_state
> +        empty_return_check_state, missing_authors
>  
>      PARSE_STATE_HEADING = 0
>      PARSE_STATE_DIFF_HEADER = 1
> @@ -977,6 +1011,11 @@ def ovs_checkpatch_parse(text, filename, author=None, committer=None):
>                                        "who are not authors or co-authors or "
>                                        "committers: %s"
>                                        % ", ".join(extra_sigs))
> +
> +                if check_authors_file:
> +                    missing_authors = do_authors_exist(missing_authors +
> +                                                       co_authors + [author])
> +
>              elif is_committer.match(line):
>                  committer = is_committer.match(line).group(2)
>              elif is_author.match(line):
> @@ -1067,6 +1106,8 @@ Input options:
>  
>  Check options:
>  -h|--help                      This help message
> +-a|--check-authors-file        Check AUTHORS file for existence of the authors.
> +                               Should be used by commiters only!
>  -b|--skip-block-whitespace     Skips the if/while/for whitespace tests
>  -l|--skip-leading-whitespace   Skips the leading whitespace test
>  -q|--quiet                     Only print error and warning information
> @@ -1089,6 +1130,19 @@ def ovs_checkpatch_print_result():
>          print("Lines checked: %d, no obvious problems found\n" % (total_line))
>  
>  
> +def ovs_checkpatch_print_missing_authors():
> +    if missing_authors:
> +        if len(missing_authors) == 1:
> +            print_warning("Author '%s' is not in the AUTHORS.rst file!"
> +                          % missing_authors[0])
> +        else:
> +            print_warning("Authors '%s' are not in the AUTHORS.rst file!"
> +                          % ', '.join(missing_authors))
> +        return True
> +
> +    return False
> +
> +
>  def ovs_checkpatch_file(filename):
>      try:
>          mail = email.message_from_file(open(filename, 'r', encoding='utf8'))
> @@ -1116,6 +1170,8 @@ def ovs_checkpatch_file(filename):
>          result = True
>  
>      ovs_checkpatch_print_result()
> +    if ovs_checkpatch_print_missing_authors():
> +        result = True
>      return result
>  
>  
> @@ -1138,9 +1194,10 @@ if __name__ == '__main__':
>                                            sys.argv[1:])
>          n_patches = int(numeric_options[-1][1:]) if numeric_options else 0
>  
> -        optlist, args = getopt.getopt(args, 'bhlstfSq',
> +        optlist, args = getopt.getopt(args, 'abhlstfSq',
>                                        ["check-file",
>                                         "help",
> +                                       "check-authors-file",
>                                         "skip-block-whitespace",
>                                         "skip-leading-whitespace",
>                                         "skip-signoff-lines",
> @@ -1157,6 +1214,8 @@ if __name__ == '__main__':
>          if o in ("-h", "--help"):
>              usage()
>              sys.exit(0)
> +        elif o in ("-a", "--check-authors-file"):
> +            check_authors_file = True
>          elif o in ("-b", "--skip-block-whitespace"):
>              skip_block_whitespace_check = True
>          elif o in ("-l", "--skip-leading-whitespace"):
> @@ -1207,9 +1266,10 @@ Subject: %s
>              if not quiet:
>                  print('== Checking %s ("%s") ==' % (revision[0:12], name))
>              result = ovs_checkpatch_parse(patch, revision)
> -            ovs_checkpatch_print_result()
>              if result:
>                  status = EXIT_FAILURE
> +        if ovs_checkpatch_print_missing_authors():
> +            status = EXIT_FAILURE
>          sys.exit(status)
>  
>      if not args:
> @@ -1218,6 +1278,8 @@ Subject: %s
>              sys.exit(EXIT_FAILURE)
>          result = ovs_checkpatch_parse(sys.stdin.read(), '-')
>          ovs_checkpatch_print_result()
> +        if ovs_checkpatch_print_missing_authors():
> +            result = EXIT_FAILURE

Thanks for doing the delayed printing.  It makes it much nicer to read
at the end.

I was also thinking that the advantage to the delayed printing is we can
have something like:

  @@ -668,6 +668,11 @@ checks = [
        'check': lambda x: has_efgrep(x),
        'print':
        lambda: print_error("grep -E/-F should be used instead of egrep/fgrep")},
  +
  +    {'regex': 'AUTHORS.rst$', 'match_name': None,
  +     'check': lambda x: update_missing_authors(x),
  +     'print': None},
  +
   ]
   
   
  @@ -872,6 +877,20 @@ def get_top_directory():
       return "."
   
   
  +def update_missing_authors(diffed_line):
  +    global missing_authors
  +    for author in missing_authors:
  +        m = re.search(r'<(.*?)>', author)
  +        if not m:
  +            continue
  +        pattern = r'\b' + re.escape(m.group(1)) + r'\b'
  +        if re.search(pattern, diffed_line) is None:
  +            continue
  +        else:
  +            missing_authors.remove(author)
  +
  +    return False
  +
   def do_authors_exist(authors):
       authors = list(set(authors))
       missing_authors = []

This means that if someone makes a patch like:

  From: Foonly Foobar <foo@bar.com>
  Subject: [PATCH] feature: A new shiny feature.

  This inserts a brand new feature that I think we all want.

  Signed-off-by: Foonly Foobar <foo@bar.com>
  ---
  diff --git a/AUTHORS.rst b/AUTHORS.rst
  index xxxx..xxxx @@ ....
   NAME
   NAME
   NAME
  +Foonly Foobar                          foo@bar.com
   NAME
   NAME
   NAME
  diff --git a/lib/feature.c b/lib/feature.c
  index xxx..xxx @@ .....
  ...

Then it won't alert.  If you think it should be a follow up patch, we
can do that also.

>          sys.exit(result)
>  
>      status = 0

On 1 Oct 2024, at 15:38, Aaron Conole wrote:

> Eelco Chaudron <echaudro@redhat.com> writes:
>
>> This patch adds a new option, --check-authors-file, to the checkpatch
>> tool to help OVS maintainers check for missing authors in the
>> AUTHORS.rst file.
>>
>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>> ---
>> v3: - Also include co-authors in the check.
>>     - Only report the end, when all patches are checked.
>>     - Fixed spelling mistake.
>>     - Determine git root directory for AUTHORS.rst location.
>>
>> v4: - Added a test case.
>> ---
>
> Thanks for the quick follow up Eelco.
>
>>  tests/checkpatch.at     | 21 +++++++++++++
>>  utilities/checkpatch.py | 68 +++++++++++++++++++++++++++++++++++++++--
>>  2 files changed, 86 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/checkpatch.at b/tests/checkpatch.at
>> index 34971c514..fa179c707 100755
>> --- a/tests/checkpatch.at
>> +++ b/tests/checkpatch.at
>> @@ -634,3 +634,24 @@ try_checkpatch \
>>      "--skip-committer-signoff"
>>
>>  AT_CLEANUP
>> +
>> +AT_SETUP([checkpatch - AUTHORS.rst existence])
>> +
>> +try_checkpatch \
>> +   "Author: A <a@a.com>
>> +    Commit: A <a@a.com>
>> +    Subject: netdev: Subject.
>> +
>> +    Signed-off-by: A <a@a.com>" \
>> +    ""
>> +
>> +try_checkpatch \
>> +   "Author: A <a@a.com>
>> +    Commit: A <a@a.com>
>> +    Subject: netdev: Subject.
>> +
>> +    Signed-off-by: A <a@a.com>" \
>> +    "WARNING: Author 'A <a@a.com>' is not in the AUTHORS.rst file!" \
>> +    "-a"
>> +
>> +AT_CLEANUP
>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
>> index 742a0bc47..73c20f804 100755
>> --- a/utilities/checkpatch.py
>> +++ b/utilities/checkpatch.py
>> @@ -28,12 +28,14 @@ __errors = 0
>>  __warnings = 0
>>  empty_return_check_state = 0
>>  print_file_name = None
>> +check_authors_file = False
>>  checking_file = False
>>  total_line = 0
>>  colors = False
>>  spellcheck = False
>>  quiet = False
>>  spell_check_dict = None
>> +missing_authors = []
>>
>>
>>  def open_spell_check_dict():
>> @@ -860,9 +862,41 @@ def run_subject_checks(subject, spellcheck=False):
>>      return warnings
>>
>>
>> +def get_top_directory():
>> +    with os.popen('git rev-parse --show-toplevel') as pipe:
>> +        path = pipe.read()
>> +
>> +    if path:
>> +        return path.strip()
>> +
>> +    return "."
>> +
>> +
>> +def do_authors_exist(authors):
>> +    authors = list(set(authors))
>> +    missing_authors = []
>> +
>> +    try:
>> +        with open(get_top_directory() + "/AUTHORS.rst", "r") as file:
>> +            file_content = file.read()
>> +            for author in authors:
>> +                m = re.search(r'<(.*?)>', author)
>> +                if not m:
>> +                    continue
>> +                pattern = r'\b' + re.escape(m.group(1)) + r'\b'
>> +                if re.search(pattern, file_content) is None:
>> +                    missing_authors.append(author)
>> +
>> +    except FileNotFoundError:
>> +        print_error("Could not open AUTHORS.rst in '%s/'!" %
>> +                    get_top_directory())
>> +
>> +    return missing_authors
>> +
>> +
>>  def ovs_checkpatch_parse(text, filename, author=None, committer=None):
>>      global print_file_name, total_line, checking_file, \
>> -        empty_return_check_state
>> +        empty_return_check_state, missing_authors
>>
>>      PARSE_STATE_HEADING = 0
>>      PARSE_STATE_DIFF_HEADER = 1
>> @@ -977,6 +1011,11 @@ def ovs_checkpatch_parse(text, filename, author=None, committer=None):
>>                                        "who are not authors or co-authors or "
>>                                        "committers: %s"
>>                                        % ", ".join(extra_sigs))
>> +
>> +                if check_authors_file:
>> +                    missing_authors = do_authors_exist(missing_authors +
>> +                                                       co_authors + [author])
>> +
>>              elif is_committer.match(line):
>>                  committer = is_committer.match(line).group(2)
>>              elif is_author.match(line):
>> @@ -1067,6 +1106,8 @@ Input options:
>>
>>  Check options:
>>  -h|--help                      This help message
>> +-a|--check-authors-file        Check AUTHORS file for existence of the authors.
>> +                               Should be used by commiters only!
>>  -b|--skip-block-whitespace     Skips the if/while/for whitespace tests
>>  -l|--skip-leading-whitespace   Skips the leading whitespace test
>>  -q|--quiet                     Only print error and warning information
>> @@ -1089,6 +1130,19 @@ def ovs_checkpatch_print_result():
>>          print("Lines checked: %d, no obvious problems found\n" % (total_line))
>>
>>
>> +def ovs_checkpatch_print_missing_authors():
>> +    if missing_authors:
>> +        if len(missing_authors) == 1:
>> +            print_warning("Author '%s' is not in the AUTHORS.rst file!"
>> +                          % missing_authors[0])
>> +        else:
>> +            print_warning("Authors '%s' are not in the AUTHORS.rst file!"
>> +                          % ', '.join(missing_authors))
>> +        return True
>> +
>> +    return False
>> +
>> +
>>  def ovs_checkpatch_file(filename):
>>      try:
>>          mail = email.message_from_file(open(filename, 'r', encoding='utf8'))
>> @@ -1116,6 +1170,8 @@ def ovs_checkpatch_file(filename):
>>          result = True
>>
>>      ovs_checkpatch_print_result()
>> +    if ovs_checkpatch_print_missing_authors():
>> +        result = True
>>      return result
>>
>>
>> @@ -1138,9 +1194,10 @@ if __name__ == '__main__':
>>                                            sys.argv[1:])
>>          n_patches = int(numeric_options[-1][1:]) if numeric_options else 0
>>
>> -        optlist, args = getopt.getopt(args, 'bhlstfSq',
>> +        optlist, args = getopt.getopt(args, 'abhlstfSq',
>>                                        ["check-file",
>>                                         "help",
>> +                                       "check-authors-file",
>>                                         "skip-block-whitespace",
>>                                         "skip-leading-whitespace",
>>                                         "skip-signoff-lines",
>> @@ -1157,6 +1214,8 @@ if __name__ == '__main__':
>>          if o in ("-h", "--help"):
>>              usage()
>>              sys.exit(0)
>> +        elif o in ("-a", "--check-authors-file"):
>> +            check_authors_file = True
>>          elif o in ("-b", "--skip-block-whitespace"):
>>              skip_block_whitespace_check = True
>>          elif o in ("-l", "--skip-leading-whitespace"):
>> @@ -1207,9 +1266,10 @@ Subject: %s
>>              if not quiet:
>>                  print('== Checking %s ("%s") ==' % (revision[0:12], name))
>>              result = ovs_checkpatch_parse(patch, revision)
>> -            ovs_checkpatch_print_result()
>>              if result:
>>                  status = EXIT_FAILURE
>> +        if ovs_checkpatch_print_missing_authors():
>> +            status = EXIT_FAILURE
>>          sys.exit(status)
>>
>>      if not args:
>> @@ -1218,6 +1278,8 @@ Subject: %s
>>              sys.exit(EXIT_FAILURE)
>>          result = ovs_checkpatch_parse(sys.stdin.read(), '-')
>>          ovs_checkpatch_print_result()
>> +        if ovs_checkpatch_print_missing_authors():
>> +            result = EXIT_FAILURE
>
> Thanks for doing the delayed printing.  It makes it much nicer to read
> at the end.
>
> I was also thinking that the advantage to the delayed printing is we can
> have something like:
>
>   @@ -668,6 +668,11 @@ checks = [
>         'check': lambda x: has_efgrep(x),
>         'print':
>         lambda: print_error("grep -E/-F should be used instead of egrep/fgrep")},
>   +
>   +    {'regex': 'AUTHORS.rst$', 'match_name': None,
>   +     'check': lambda x: update_missing_authors(x),
>   +     'print': None},
>   +
>    ]
>
>
>   @@ -872,6 +877,20 @@ def get_top_directory():
>        return "."
>
>
>   +def update_missing_authors(diffed_line):
>   +    global missing_authors
>   +    for author in missing_authors:
>   +        m = re.search(r'<(.*?)>', author)
>   +        if not m:
>   +            continue
>   +        pattern = r'\b' + re.escape(m.group(1)) + r'\b'
>   +        if re.search(pattern, diffed_line) is None:
>   +            continue
>   +        else:
>   +            missing_authors.remove(author)
>   +
>   +    return False
>   +
>    def do_authors_exist(authors):
>        authors = list(set(authors))
>        missing_authors = []
>
> This means that if someone makes a patch like:
>
>   From: Foonly Foobar <foo@bar.com>
>   Subject: [PATCH] feature: A new shiny feature.
>
>   This inserts a brand new feature that I think we all want.
>
>   Signed-off-by: Foonly Foobar <foo@bar.com>
>   ---
>   diff --git a/AUTHORS.rst b/AUTHORS.rst
>   index xxxx..xxxx @@ ....
>    NAME
>    NAME
>    NAME
>   +Foonly Foobar                          foo@bar.com
>    NAME
>    NAME
>    NAME
>   diff --git a/lib/feature.c b/lib/feature.c
>   index xxx..xxx @@ .....
>   ...
>
> Then it won't alert.  If you think it should be a follow up patch, we
> can do that also.

This addition looks good to me! I’ll add it in a v5.

//Eelco

>>          sys.exit(result)
>>
>>      status = 0