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.
> ---
> 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
