[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