Re: [RFC PATCH 2/2] BaseTools: add script to configure local git options

["Laszlo Ersek" <lersek@redhat.com>]

On 05/30/19 17:59, Leif Lindholm wrote:
> Patch contribution and review is greatly simplified by following the
> steps described in "Laszlo's unkempt guide":
> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers
> but there are a lot of tedious manual steps in there, so here is a
> python script that configures all options I am aware of
> *for the repository the script is executed from*.
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  BaseTools/Scripts/SetupGit.py | 187 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 187 insertions(+)
>  create mode 100644 BaseTools/Scripts/SetupGit.py

looks good to me, I've got superficial comments only:

> diff --git a/BaseTools/Scripts/SetupGit.py b/BaseTools/Scripts/SetupGit.py
> new file mode 100644
> index 0000000000..3b199f08b2
> --- /dev/null
> +++ b/BaseTools/Scripts/SetupGit.py
> @@ -0,0 +1,187 @@
> +## @file
> +#  Set up the git configuration for contributing to TianoCore projects
> +#
> +#  Copyright (c) 2019, Linaro Ltd. All rights reserved.<BR>
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +
> +from __future__ import print_function
> +import argparse
> +import os.path
> +import sys
> +
> +try:
> +    import git
> +except ImportError:
> +    print('Unable to load gitpython module - please install and try again.')
> +    sys.exit(1)
> +
> +try:
> +    # Try Python 2 'ConfigParser' module first since helpful lib2to3 will
> +    # otherwise automagically load it with the name 'configparser'
> +    import ConfigParser
> +except ImportError:
> +    # Otherwise, try loading the Python 3 'configparser' uner an alias

(1) s/uner/under/

> +    try:
> +        import configparser as ConfigParser
> +    except ImportError:
> +        print("Unable to load configparser/ConfigParser module - please install and try again!")
> +        sys.exit(1)
> +
> +
> +# Assumptions: Script is in edk2/BaseTools/Scripts,
> +#              templates in edk2/BaseTools/Conf
> +CONFDIR = os.path.join(os.path.dirname(os.path.dirname(os.path.realpath(__file__))),
> +                       'Conf')
> +
> +UPSTREAMS = [
> +    {'name': 'edk2',
> +     'repo': 'https://github.com/tianocore/edk2.git',
> +     'list': 'devel@edk2.groups.io'},
> +    {'name': 'edk2-platforms',
> +     'repo': 'https://github.com/tianocore/edk2-platforms.git',
> +     'list': 'devel@edk2.groups.io', 'prefix': 'edk2-platforms'},
> +    {'name': 'edk2-non-osi',
> +     'repo': 'https://github.com/tianocore/edk2-non-osi.git',
> +     'list': 'devel@edk2.groups.io', 'prefix': 'edk2-non-osi'}
> +    ]
> +
> +# The minimum version required for all of the below options to work
> +MIN_GIT_VERSION = (1, 9, 0)
> +
> +# Set of options to be set identically for all repositories
> +OPTIONS = [
> +    {'section': 'am',          'option': 'keepcr',         'value': True},
> +    {'section': 'am',          'option': 'signoff',        'value': True},
> +    {'section': 'cherry-pick', 'option': 'signoff',        'value': True},
> +    {'section': 'color',       'option': 'diff',           'value': True},
> +    {'section': 'color',       'option': 'grep',           'value': 'auto'},
> +    {'section': 'commit',      'option': 'signoff',        'value': True},
> +    {'section': 'core',        'option': 'abbrev',         'value': 12},
> +    {'section': 'core',        'option': 'attributesFile',
> +     'value': os.path.join(CONFDIR, 'gitattributes').replace('\\', '/')},

(2) this could be dropped if we modify the first patch to add .gitattributes

> +    {'section': 'core',        'option': 'whitespace',     'value': 'cr-at-eol'},
> +    {'section': 'diff',        'option': 'algorithm',      'value': 'patience'},
> +    {'section': 'diff',        'option': 'orderFile',
> +     'value': os.path.join(CONFDIR, 'diff.order').replace('\\', '/')},
> +    {'section': 'diff',        'option': 'renames',        'value': 'copies'},
> +    {'section': 'diff',        'option': 'statGraphWidth', 'value': '20'},

(3) Can we also add --stat=1000 with a permanent setting like this?

> +    {'section': 'diff "ini"',    'option': 'xfuncname',
> +     'value': '^\\\\[[A-Za-z0-9_., ]+]'}, # or 'diff "ini"'?

(4) Is the comment stale?

With these investigated (and updated as you see fit):

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo


> +    {'section': 'format',      'option': 'coverLetter',    'value': True},
> +    {'section': 'format',      'option': 'numbered',       'value': True},
> +    {'section': 'format',      'option': 'signoff',        'value': False},
> +    {'section': 'notes',       'option': 'rewriteRef',     'value': 'refs/notes/commits'},
> +    {'section': 'sendemail',   'option': 'chainreplyto',   'value': False},
> +    {'section': 'sendemail',   'option': 'thread',         'value': True},
> +    ]
> +
> +
> +def locate_repo():
> +    """Opens a Repo object for the current tree, searching upwards in the directory hierarchy."""
> +    try:
> +        repo = git.Repo(path='.', search_parent_directories=True)
> +    except (git.InvalidGitRepositoryError, git.NoSuchPathError):
> +        print("It doesn't look like we're inside a git repository - aborting.")
> +        sys.exit(2)
> +    return repo
> +
> +
> +def get_upstream(url):
> +    """Extracts the dict for the current repo origin."""
> +    for upstream in UPSTREAMS:
> +        if upstream['repo'] == url:
> +            return upstream
> +    print("Unknown upstream '%s' - aborting!" % url)
> +    sys.exit(3)
> +
> +
> +def check_versions():
> +    """Checks versions of dependencies."""
> +    version = git.cmd.Git().version_info
> +
> +    if version < MIN_GIT_VERSION:
> +        print('Need git version %d.%d or later!' % (version[0], version[1]))
> +        sys.exit(4)
> +
> +
> +def write_config_value(repo, section, option, data):
> +    """."""
> +    with repo.config_writer(config_level='repository') as configwriter:
> +        configwriter.set_value(section, option, data)
> +
> +
> +if __name__ == '__main__':
> +    check_versions()
> +
> +    PARSER = argparse.ArgumentParser(
> +        description='Sets up a git repository according to TianoCore rules.')
> +    PARSER.add_argument('-c', '--check',
> +                        help='check current config only, printing what would be changed',
> +                        action='store_true',
> +                        required=False)
> +    PARSER.add_argument('-f', '--force',
> +                        help='overwrite existing settings conflicting with program defaults',
> +                        action='store_true',
> +                        required=False)
> +    PARSER.add_argument('-v', '--verbose',
> +                        help='enable more detailed output',
> +                        action='store_true',
> +                        required=False)
> +    ARGS = PARSER.parse_args()
> +
> +    REPO = locate_repo()
> +    if REPO.bare:
> +        print('Bare repo - please check out an upstream one!')
> +        sys.exit(6)
> +
> +    URL = REPO.remotes.origin.url
> +
> +    UPSTREAM = get_upstream(URL)
> +    if not UPSTREAM:
> +        print("Upstream '%s' unknown, aborting!" % URL)
> +        sys.exit(7)
> +
> +    # Set a list email address if our upstream wants it
> +    if 'list' in UPSTREAM:
> +        OPTIONS.append({'section': 'sendemail', 'option': 'to',
> +                        'value': UPSTREAM['list']})
> +    # Append a subject prefix entry to OPTIONS if our upstream wants it
> +    if 'prefix' in UPSTREAM:
> +        OPTIONS.append({'section': 'format', 'option': 'subjectPrefix',
> +                        'value': "PATCH " + UPSTREAM['prefix']})
> +
> +    CONFIG = REPO.config_reader(config_level='repository')
> +
> +    for entry in OPTIONS:
> +        exists = False
> +        try:
> +            # Make sure to read boolean/int settings as real type rather than strings
> +            if isinstance(entry['value'], bool):
> +                value = CONFIG.getboolean(entry['section'], entry['option'])
> +            elif isinstance(entry['value'], int):
> +                value = CONFIG.getint(entry['section'], entry['option'])
> +            else:
> +                value = CONFIG.get(entry['section'], entry['option'])
> +
> +            exists = True
> +        # Don't bail out from options not already being set
> +        except (ConfigParser.NoSectionError, ConfigParser.NoOptionError):
> +            pass
> +
> +        if exists:
> +            if value == entry['value']:
> +                if ARGS.verbose:
> +                    print("%s.%s already set (to '%s')" % (entry['section'], entry['option'], value))
> +            else:
> +                if ARGS.force:
> +                    write_config_value(REPO, entry['section'], entry['option'], entry['value'])
> +                else:
> +                    print("Not overwriting existing %s.%s value:" % (entry['section'], entry['option']))
> +                    print("  '%s' != '%s'" % (value, entry['value']))
> +                    print("  add '-f' to command line to force overwriting existing settings")
> +        else:
> +            print("%s.%s => '%s'" % (entry['section'], entry['option'], entry['value']))
> +            if not ARGS.check:
> +                write_config_value(REPO, entry['section'], entry['option'], entry['value'])
>