On 27 Sep 2024, at 14:35, Aaron Conole wrote:
> Eelco Chaudron <[email protected]> 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 <[email protected]>
>> ---
>> 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 --------
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
