[ovs-dev,v2] 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>
---
v2: Fixed partial match, and long argument check.

---
 utilities/checkpatch.py | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

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>
> ---
> v2: Fixed partial match, and long argument check.

Not sure about it.  For example, is it really a problem with the patch
if the author doesn't appear?  Maybe this could be a different checking
utility?  I certainly don't think it would be an error in the patch.

> ---
>  utilities/checkpatch.py | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 742a0bc47..636634472 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -28,6 +28,7 @@ __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
> @@ -977,6 +978,21 @@ 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 or author:
> +                    m = re.search(r'<(.*?)>', author)
> +                    if m:
> +                        try:
> +                            with open("AUTHORS.rst", "r") as file:
> +                                file_content = file.read()
> +                                pattern = r'\b' + re.escape(m.group(1)) + r'\b'
> +                                if re.search(pattern, file_content) is None:
> +                                    print_error("Author '%s' is not in the "
> +                                    "AUTHORS.rst file!" % author)
> +                        except FileNotFoundError:
> +                            print_error("Can't open AUTHORS.rst file in "
> +                                        "current path!")
> +
>              elif is_committer.match(line):
>                  committer = is_committer.match(line).group(2)
>              elif is_author.match(line):
> @@ -1067,6 +1083,7 @@ Input options:
>  
>  Check options:
>  -h|--help                      This help message
> +-a|--check-authors-file        Check AUTHORS file for existense of author
>  -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
> @@ -1138,9 +1155,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 +1175,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"):

On 25 Sep 2024, at 21:02, 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>
>> ---
>> v2: Fixed partial match, and long argument check.
>
> Not sure about it.  For example, is it really a problem with the patch
> if the author doesn't appear?  Maybe this could be a different checking
> utility?  I certainly don't think it would be an error in the patch.

It’s not an error in the patch itself, but an error to be resolved before we apply it.

As we have to duplicate a lot of infra for a stand-alone tool, and this option is not being enabled by default, I feel like it’s ok to enhance checkpatch. It also simplifies commiters the workflow, as now I can just run ‘checkpatch.py -3 -S -a’.

Anyone else thoughts, feedback on this?

//Eelco

>> ---
>>  utilities/checkpatch.py | 22 +++++++++++++++++++++-
>>  1 file changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
>> index 742a0bc47..636634472 100755
>> --- a/utilities/checkpatch.py
>> +++ b/utilities/checkpatch.py
>> @@ -28,6 +28,7 @@ __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
>> @@ -977,6 +978,21 @@ 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 or author:
>> +                    m = re.search(r'<(.*?)>', author)
>> +                    if m:
>> +                        try:
>> +                            with open("AUTHORS.rst", "r") as file:
>> +                                file_content = file.read()
>> +                                pattern = r'\b' + re.escape(m.group(1)) + r'\b'
>> +                                if re.search(pattern, file_content) is None:
>> +                                    print_error("Author '%s' is not in the "
>> +                                    "AUTHORS.rst file!" % author)
>> +                        except FileNotFoundError:
>> +                            print_error("Can't open AUTHORS.rst file in "
>> +                                        "current path!")
>> +
>>              elif is_committer.match(line):
>>                  committer = is_committer.match(line).group(2)
>>              elif is_author.match(line):
>> @@ -1067,6 +1083,7 @@ Input options:
>>
>>  Check options:
>>  -h|--help                      This help message
>> +-a|--check-authors-file        Check AUTHORS file for existense of author
>>  -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
>> @@ -1138,9 +1155,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 +1175,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"):

On Wed, Sep 25, 2024 at 10:39:04PM +0200, Eelco Chaudron wrote:
> 
> 
> On 25 Sep 2024, at 21:02, 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>
> >> ---
> >> v2: Fixed partial match, and long argument check.
> >
> > Not sure about it.  For example, is it really a problem with the patch
> > if the author doesn't appear?  Maybe this could be a different checking
> > utility?  I certainly don't think it would be an error in the patch.
> 
> It’s not an error in the patch itself, but an error to be resolved before we apply it.
> 
> As we have to duplicate a lot of infra for a stand-alone tool, and this option is not being enabled by default, I feel like it’s ok to enhance checkpatch. It also simplifies commiters the workflow, as now I can just run ‘checkpatch.py -3 -S -a’.
> 
> Anyone else thoughts, feedback on this?

The approach taken here seems reasonable to me.
Those who have a use for the feature can use checkpatch with
the option enabled, while those who don't won't be effected.
And the implementation can make use of existing infrastructure
in checkpatch.py.

Simon Horman <horms@ovn.org> writes:

> On Wed, Sep 25, 2024 at 10:39:04PM +0200, Eelco Chaudron wrote:
>> 
>> 
>> On 25 Sep 2024, at 21:02, 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>
>> >> ---
>> >> v2: Fixed partial match, and long argument check.
>> >
>> > Not sure about it.  For example, is it really a problem with the patch
>> > if the author doesn't appear?  Maybe this could be a different checking
>> > utility?  I certainly don't think it would be an error in the patch.
>> 
>> It’s not an error in the patch itself, but an error to be resolved
>> before we apply it.
>> 
>> As we have to duplicate a lot of infra for a stand-alone tool, and
>> this option is not being enabled by default, I feel like it’s ok to
>> enhance checkpatch. It also simplifies commiters the workflow, as
>> now I can just run ‘checkpatch.py -3 -S -a’.
>> 
>> Anyone else thoughts, feedback on this?
>
> The approach taken here seems reasonable to me.
> Those who have a use for the feature can use checkpatch with
> the option enabled, while those who don't won't be effected.
> And the implementation can make use of existing infrastructure
> in checkpatch.py.

Okay, that makes sense.  I'll reply to the original patch with one nit.

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>
> ---
> v2: Fixed partial match, and long argument check.

Hi Eelco,

During the review I had some other thoughts about the way this feature
would behave, and I think it would be good to add a test to the
checkpatch tests for it to cover the behavior.

> ---
>  utilities/checkpatch.py | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 742a0bc47..636634472 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -28,6 +28,7 @@ __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
> @@ -977,6 +978,21 @@ 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 or author:

Should this be 'and' instead of 'or'?  All patches we receive will have
an author so from what I can tell this check is always running.

Also, we probably want to do the below for all the co_authors as well.

> +                    m = re.search(r'<(.*?)>', author)
> +                    if m:
> +                        try:
> +                            with open("AUTHORS.rst", "r") as file:
> +                                file_content = file.read()
> +                                pattern = r'\b' + re.escape(m.group(1)) + r'\b'
> +                                if re.search(pattern, file_content) is None:
> +                                    print_error("Author '%s' is not in the "
> +                                    "AUTHORS.rst file!" % author)

Maybe print_warn instead.  I can imagine someone running this and adding
a patch where they add themselves to the AUTHORS.rst file in another
commit.

> +                        except FileNotFoundError:
> +                            print_error("Can't open AUTHORS.rst file in "
> +                                        "current path!")
> +

This one I don't know - maybe we should make it so that we can use git
to find the AUTHORS.rst file.  See the code snippet I give at the end
(it's not fully tested).

Also, maybe we want to delay printing these until the end of the patch.
For example, if someone does run this and adds themselves to AUTHORS.rst
as part of the patch the scan may not pick it up.  None of the other
checks behave so differently between patch scan and file scan mode,
iiuc.  If we delay scanning until the end we can avoid printing this in
the case that the AUTHORS.rst file gets modified in a later hunk.

WDYT?

>              elif is_committer.match(line):
>                  committer = is_committer.match(line).group(2)
>              elif is_author.match(line):
> @@ -1067,6 +1083,7 @@ Input options:
>  
>  Check options:
>  -h|--help                      This help message
> +-a|--check-authors-file        Check AUTHORS file for existense of author

s/existense/existence/

Maybe we should also flag somehow that this check should generally be
run by a committer.

>  -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
> @@ -1138,9 +1155,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 +1175,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"):


------- 8< --------
def get_git_topdir(start_dir='.'):
    """Find the .git directory in the upper path"""
    current_dir = os.path.abspath(start_dir)
    while True:
        git_dir = os.path.join(current_dir, '.git')
        if os.path.isdir(git_dir):
            return current_dir

        parent_dir = os.path.dirname(current_dir)
        if parent_dir == current_dir:
            return None
        current_dir = parent_dir

def git_source_find(filename):
    """Try to open a path file in the current tree."""
    if os.path.exists(filename):
        return os.path.abspath(filename)

    topdir = get_git_topdir()
    if topdir is not None and \
       os.path.exists(os.path.join(topdir, filename)):
        return os.path.join(topdir, filename)

    return None

-------- >8 --------

On 27 Sep 2024, at 14:35, 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>
>> ---
>> v2: Fixed partial match, and long argument check.
>
> Hi Eelco,
>
> During the review I had some other thoughts about the way this feature
> would behave, and I think it would be good to add a test to the
> checkpatch tests for it to cover the behavior.

Oops!! I sent out a v3 addressing all your comments, but I missed the
test case part. Will send out a v4 with a test case.

//Eelco

>> ---
>>  utilities/checkpatch.py | 22 +++++++++++++++++++++-
>>  1 file changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
>> index 742a0bc47..636634472 100755
>> --- a/utilities/checkpatch.py
>> +++ b/utilities/checkpatch.py
>> @@ -28,6 +28,7 @@ __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
>> @@ -977,6 +978,21 @@ 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 or author:
>
> Should this be 'and' instead of 'or'?  All patches we receive will have
> an author so from what I can tell this check is always running.
>
> Also, we probably want to do the below for all the co_authors as well.
>
>> +                    m = re.search(r'<(.*?)>', author)
>> +                    if m:
>> +                        try:
>> +                            with open("AUTHORS.rst", "r") as file:
>> +                                file_content = file.read()
>> +                                pattern = r'\b' + re.escape(m.group(1)) + r'\b'
>> +                                if re.search(pattern, file_content) is None:
>> +                                    print_error("Author '%s' is not in the "
>> +                                    "AUTHORS.rst file!" % author)
>
> Maybe print_warn instead.  I can imagine someone running this and adding
> a patch where they add themselves to the AUTHORS.rst file in another
> commit.
>
>> +                        except FileNotFoundError:
>> +                            print_error("Can't open AUTHORS.rst file in "
>> +                                        "current path!")
>> +
>
> This one I don't know - maybe we should make it so that we can use git
> to find the AUTHORS.rst file.  See the code snippet I give at the end
> (it's not fully tested).
>
> Also, maybe we want to delay printing these until the end of the patch.
> For example, if someone does run this and adds themselves to AUTHORS.rst
> as part of the patch the scan may not pick it up.  None of the other
> checks behave so differently between patch scan and file scan mode,
> iiuc.  If we delay scanning until the end we can avoid printing this in
> the case that the AUTHORS.rst file gets modified in a later hunk.
>
> WDYT?
>
>>              elif is_committer.match(line):
>>                  committer = is_committer.match(line).group(2)
>>              elif is_author.match(line):
>> @@ -1067,6 +1083,7 @@ Input options:
>>
>>  Check options:
>>  -h|--help                      This help message
>> +-a|--check-authors-file        Check AUTHORS file for existense of author
>
> s/existense/existence/
>
> Maybe we should also flag somehow that this check should generally be
> run by a committer.
>
>>  -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
>> @@ -1138,9 +1155,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 +1175,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"):
>
>
> ------- 8< --------
> def get_git_topdir(start_dir='.'):
>     """Find the .git directory in the upper path"""
>     current_dir = os.path.abspath(start_dir)
>     while True:
>         git_dir = os.path.join(current_dir, '.git')
>         if os.path.isdir(git_dir):
>             return current_dir
>
>         parent_dir = os.path.dirname(current_dir)
>         if parent_dir == current_dir:
>             return None
>         current_dir = parent_dir
>
> def git_source_find(filename):
>     """Try to open a path file in the current tree."""
>     if os.path.exists(filename):
>         return os.path.abspath(filename)
>
>     topdir = get_git_topdir()
>     if topdir is not None and \
>        os.path.exists(os.path.join(topdir, filename)):
>         return os.path.join(topdir, filename)
>
>     return None
>
> -------- >8 --------