[Mesa-dev] [PATCH mesa 3/5] bin: rewrite get_reviewers as opt-in only
Dylan Baker
dylan at pnwbakers.com
Thu May 31 17:04:25 UTC 2018
Quoting Eric Engestrom (2018-05-31 07:04:33)
> Some people have mentioned they don't like the current get_reviewers.pl
> script (the one from the kernel) because it is way too greedy in its
> search for reviewers.
>
> I tried to modify it to remove the git blame functionality, but I had
> a way too hard time figuring out what all this perl black magic does,
> and I figured I wouldn't be the only one, so I rewrote it in python
> instead, so that more people would be able to maintain it.
>
> I kept only the basic functionality of parsing the REVIEWERS file, and
> only supporting the R: and F: tags for now (those were the only ones
> used in Mesa anyway). Patches can be passed in as arguments or via
> stdin, and `-f` can be used to simply find reviewers for the files
> given.
>
> This should be a drop-in replacement for the old script, meant to be
> used with git; you can use it automatically by setting:
> $ git config sendemail.cccmd bin/get_reviewer.py
>
> To reiterate, this script is now opt-in only, as it only returns
> reviewers added in REVIEWERS.
>
> Signed-off-by: Eric Engestrom <eric.engestrom at intel.com>
> ---
> As a quirk that I decided to leave in, "F: /dev/null" can now be used to
> subscribe to files being added or deleted, which I'm thinking of adding
> to the build systems groups, so that build system reviewers can make
> sure their build system was updated accordingly. Thoughts?
> ---
> bin/get_reviewer.py | 177 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 177 insertions(+)
> create mode 100755 bin/get_reviewer.py
>
> diff --git a/bin/get_reviewer.py b/bin/get_reviewer.py
> new file mode 100755
> index 00000000000000000000..543086ba21505b81ea0c
> --- /dev/null
> +++ b/bin/get_reviewer.py
> @@ -0,0 +1,177 @@
> +#!/usr/bin/env python3
> +"""
> +Parser for REVIEWERS, designed for use with git.
> +
> +Use it automatically by configuring git like this:
> + $ git config sendemail.cccmd bin/get_reviewer.py
> +
> +"""
> +
> +def parse_options(args):
> + """
> + Takes care of parsing the options and returning a simple dict with
> + one member per option.
> + """
> + import argparse
Please don't put the imports in the function bodies, this looks like you're
doing dirty tricks to avoid circular imports.
> + parser = argparse.ArgumentParser(description='''
> + Get the list of reviewers for your changes.
> + If no file is given, reads stdin.
> + ''')
> + parser.add_argument('-f',
> + action='store_true',
> + help='''find the reviewers for the files given,
> + instead of extracting the files from the patches''')
> + parser.add_argument('file',
> + nargs='*',
> + default=[],
> + help='patch, or plain file (see `-f`)')
> + return parser.parse_args(args)
> +
> +def parse_diff(diff):
> + """
> + Takes a unified diff and returns a list of all the files it
> + modifies.
> + """
> + files = []
> +
> + def diff_filename(line):
> + """
> + Extracts the filename (full path) from a diff header
> + """
> + filename = line.split()[1]
> +
> + # If the file is being created or deleted, don't strip anything
> + if filename == '/dev/null':
> + return filename
> +
> + # Strip a/... or b/... prefix
> + return '/'.join(filename.split('/')[1:])
> +
> + for line in diff.splitlines():
> + if line == '---':
> + # Not part of the diff itself; ignore
> + pass
> + # Keep both old and new filenames, in case of files being added
> + # or deleted
> + elif line.startswith('---') or line.startswith('+++'):
> + files.append(diff_filename(line))
> +
> + # Deduplicate the list
> + return set(files)
> +
> +def canonicalize_globs(dirty_globs):
> + """
> + Canonicalize glob pattern from REVIEWERS to make them ready for
> + python's glob() consumption
> + """
> + import os.path
> + cleaned_globs = []
> + for path in dirty_globs:
> + if '*' not in path:
> + # Make sure dirs end with /* to match all it contents
> + if os.path.isdir(path):
> + if path.endswith('/'):
> + path += '*'
> + else:
> + path += '/*'
> + else:
> + # Any full path must exist (ie. dir or file)
> + assert os.path.exists(path)
> + cleaned_globs.append(path)
> + return cleaned_globs
> +
> +def parse_reviewers():
> + """
> + Parse the REVIEWERS file and return a dict of, per group:
> + - name (first line of the group)
> + - reviewers (Full Name <email at address.tld>)
> + - files (glob-style)
> + """
> + import re
> + reviewers = []
> + with open('REVIEWERS', 'r') as file:
> + lines = file.readlines()
> + header_over = False
> + useful_lines = []
> + for line in lines:
> + line = line.strip()
> + if header_over:
> + useful_lines.append(line)
> + elif re.match(r'^----+$', line):
> + header_over = True
> + groups = '\n'.join(useful_lines).split('\n\n')
> +
> + for group in groups:
> + _name = None
> + _reviewers = []
> + _files = []
> + for line in group.splitlines():
> + if not line or line.isspace():
> + continue
> + elif _name is None:
> + _name = line
> + continue
> +
> + tag = line[0]
> + value = ' '.join(line.split()[1:])
> + if tag == 'R':
> + _reviewers.append(value)
> + elif tag == 'F':
> + _files.append(value)
> + else:
> + assert False
> +
> + _files = canonicalize_globs(_files)
> +
> + reviewers.append({
> + 'name': _name,
> + 'files': _files,
> + 'reviewers': _reviewers,
> + })
> + return reviewers
> +
> +def main(args, stdin):
> + """
> + Entrypoint when invoked as a script. Will print a list (one per
> + line) of registered reviewers for the files given, or for the files
> + touched by the patch piped in.
> + """
> + import fnmatch
> + import os
> +
> + options = parse_options(args)
> +
> + # With `-f`, any files given are used as is.
> + # Otherwise, files are considered to be patches and parsed as such.
> + # If no argument is given, then stdin is read as a patch.
> + if options.file:
> + assert all(os.path.exists(file) for file in options.file)
> + if options.f:
> + changed_files = options.file
> + else:
> + for filename in options.file:
> + with open(filename, 'r') as file:
> + changed_files = parse_diff(file.read())
> + else:
> + changed_files = parse_diff(stdin.read())
> +
> + review_groups = parse_reviewers()
> +
> + # Filter down to the reviewers who care about the files changed
> + reviewers = []
> + for changed_file in changed_files:
> + for group in review_groups:
> + for group_file in group['files']:
> + if fnmatch.fnmatch(changed_file, group_file):
> + for reviewer in group['reviewers']:
> + reviewers.append(reviewer)
> +
> + #TODO: add `-i` interactive mode
> +
> + # Deduplicate list
> + for reviewer in set(reviewers):
> + print(reviewer)
> +
> +if __name__ == '__main__':
> + import sys
> + main(sys.argv[1:], sys.stdin)
There shouldn't be any need to strip the first element of sys.argv for argparse,
it knows how to handle that 'get_reveiwers.py' is the first argument. There
shouldn't be a reason to pass it to argparse at all, since we passing all of the
arguments and argparse will just use sys.argv if it's passed no arguments.
There isn't really a reason to pass sys.stdin here either, since it's th eonly
option anyway.
Dylan
> --
> Cheers,
> Eric
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180531/4038c1fb/attachment.sig>
More information about the mesa-dev
mailing list