9
\$\begingroup\$

A couple of months ago I posted a bash script to manage multiple Git identities as a solution on Stack Overflow but soon found the hook isn't flexible enough.

Thus I decided to rewrite the hook in Python. Basically it does what it should but I'd really like to have someone review the code in order to eliminate bad practices, to remove unnecessary complexity and how it could be improved in regards to style, readability and performance.

Another question is if you see any point where it would be better to introduce a class.

For the rewrite I created a git repository at github.com: git-passport - A Git command and hook written in Python to manage multiple Git accounts / user identities.

#!/usr/bin/env python3
# -*- coding: utf-8 -*-


""" git-passport is a Git pre-commit hook written in Python to manage
    multiple Git user identities.
"""


# ..................................................................... Imports
import configparser
import os.path
import subprocess
import sys
import textwrap
import time
import urllib.parse


# ............................................................ Config functions
def config_create(filename):
    """ Create a configuration file containing sample data inside the home
        directory if none exists yet.

        Args:
            filename (str): The complete `filepath` of the configuration file
    """
    if not os.path.exists(filename):
        preset = configparser.ConfigParser()

        preset["General"] = {}
        preset["General"]["enable_hook"] = "True"
        preset["General"]["sleep_duration"] = "0.75"

        preset["Passport 0"] = {}
        preset["Passport 0"]["email"] = "email_0@example.com"
        preset["Passport 0"]["name"] = "name_0"
        preset["Passport 0"]["service"] = "github.com"

        preset["Passport 1"] = {}
        preset["Passport 1"]["email"] = "email_1@example.com"
        preset["Passport 1"]["name"] = "name_1"
        preset["Passport 1"]["service"] = "gitlab.com"

        try:
            msg = """
                No configuration file found.
                Generating a sample configuration file.
            """

            print(textwrap.dedent(msg).strip())
            with open(filename, "w") as configfile:
                preset.write(configfile)
            sys.exit("\n~Done~")

        except Exception as error:
            print(error)
            raise sys.exit("\n~Quitting~")


def config_read(filename):
    """ Read a provided configuration file and «import» allowed sections and
        their keys/values into a dictionary.

        Args:
            filename (str): The complete `filepath` of the configuration file

        Returns:
            config (dict): Contains all allowed configuration sections
    """
    raw_config = configparser.ConfigParser()
    raw_config.read(filename)

    # Match an arbitrary number of sections starting with pattern
    pattern = "Passport"

    # A generator to filter matching sections:
    # Let's see if user defined config sections match a pattern
    def generate_matches():
        for section in raw_config.items():
            if pattern in section[0]:
                yield dict(section[1])

    # Construct a custom dict containing allowed sections
    config = dict(raw_config.items("General"))
    config["git_local_ids"] = dict(enumerate(generate_matches()))

    return config


def config_validate(config):
    """ Validate and convert certain keys and values of a given dictionary
        containing a set of configuration options. If unexpected values are
        found we quit the script and notify the user what went wrong.

        Since ``ConfigParser`` only accepts strings when setting up a default
        config it is necessary to convert some values to numbers and boolean.

        Args:
            config (dict): Contains all allowed configuration sections

        Returns:
            config (dict): Contains valid and converted configuration options
    """
    for key, value in config.items():
        if key == "enable_hook":
            if value == "True":
                config[key] = True
            elif value == "False":
                config[key] = False
            else:
                msg = "E > Settings > {}: Expecting True or False."
                raise sys.exit(msg).format(key)

        elif key == "sleep_duration":
            try:
                config[key] = float(value)
            except ValueError:
                msg = "E > Settings > {}: Expecting float or number."
                raise sys.exit(msg).format(key)

        # Here the values could really be anything...
        elif key == "git_local_ids":
            pass

        else:
            msg = "E > Settings > {}: Section/key unknown."
            raise sys.exit(msg).format(key)

    return config


# ............................................................... Git functions
def git_get_id(config, scope, property):
    """ Get the email address or username of the global or local Git ID.

        Args:
            config (dict): Contains validated configuration options
            scope (str): Search inside a `global` or `local` scope
            property (str): Type of `email` or `name`

        Returns:
            git_id (str): A name or email address
            error (str): Exception
    """
    try:
        git_process = subprocess.Popen([
            "git",
            "config",
            "--get",
            "--" + scope,
            "user." + property
        ], stdout=subprocess.PIPE)

        git_id = git_process.communicate()[0].decode("utf-8")
        return git_id.replace("\n", "")

    except Exception as error:
        raise error


def git_get_url():
    """ Get the local remote.origin.url of a Git repository.

        Returns:
            git_url (str): The local and active remote.origin.url
            error (str): Exception
    """
    try:
        git_process = subprocess.Popen([
            "git",
            "config",
            "--get",
            "--local",
            "remote.origin.url"
        ], stdout=subprocess.PIPE)

        git_url = git_process.communicate()[0].decode("utf-8")
        return git_url.replace("\n", "")

    except Exception as error:
        raise error


def git_set_id(config, value, property):
    """ Set the email address or username as a local Git ID for a repository.

        Args:
            config (dict): Contains validated configuration options
            value (str): A name or email address
            property (str): Type of `email` or `name`

        Returns:
            error (str): Exception
    """
    try:
        subprocess.Popen([
            "git",
            "config",
            "--local",
            "user." + property,
            value
        ], stdout=subprocess.PIPE)

    except Exception as error:
        raise error


# ............................................................ Helper functions
def get_user_input(pool):
    """ Prompt a user to select a number from a list of numbers representing
        available Git IDs. Optionally the user can choose `q` to quit the
        selection process.

        Args:
            pool (list): A list of numbers representing available Git IDs

        Returns:
            selection (int): A number representing a Git ID chosen by a user
    """
    while True:
        # https://stackoverflow.com/questions/7437261/how-is-it-possible-to-use-raw-input-in-a-python-git-hook
        sys.stdin = open("/dev/tty")
        selection = input("» Select a passport [ID] or «(q)uit»: ")

        try:
            selection = int(selection)
        except ValueError:
            if selection == "q" or selection == "quit":
                sys.exit("\n~Quitting~\n")
            continue

        if selection not in pool:
            continue
        break
    return selection


def print_choice(choice):
    """ Before showing the actual prompt by calling `get_user_input()` print a
        list of available Git IDs containing properties ID, «scope», name,
        email and service.

        Args:
            choice (dict): Contains a list of preselected Git ID candidates
    """
    for key, value in choice.items():
        if value.get("flag") == "global":
            msg = """
                ~:Global ID: {}
                    . User:   {}
                    . E-Mail: {}
            """
            print(textwrap.dedent(msg).lstrip().format(
                key,
                value["name"],
                value["email"])
            )
        else:
            msg = """
                ~Passport ID: {}
                    . User:    {}
                    . E-Mail:  {}
                    . Service: {}
            """
            print(textwrap.dedent(msg).lstrip().format(
                key,
                value["name"],
                value["email"],
                value["service"])
            )


def add_global_id(config, target):
    """ Adds the global Git ID to a dictionary containing potential preselected
        candidates.

        Args:
            config (dict): Contains validated configuration options
            target (dict): Contains preselected local Git IDs
    """
    global_email = git_get_id(config, "global", "email")
    global_name = git_get_id(config, "global", "name")
    local_ids = config["git_local_ids"]

    if global_email and global_name:
        position = len(local_ids)
        target[position] = {}
        target[position]["email"] = global_email
        target[position]["name"] = global_name
        target[position]["flag"] = "global"


# .............................................................. Implementation
def identity_exists(config, email, name, url):
    """ Prints an existing ID of a local gitconfig.

        Args:
            config (dict): Contains validated configuration options
            email (str): An email address
            name (str): A name
            url (str): A remote.origin.url
    """
    duration = config["sleep_duration"]

    if not url:
        url = "«remote.origin.url» is not set."

    msg = """
        ~Intermission~

        ~Active Passport:
            . User:   {}
            . E-Mail: {}
            . Remote: {}
    """

    print(textwrap.dedent(msg).lstrip().format(name, email, url))
    sys.exit(time.sleep(duration))


def url_exists(config, url):
    """ If a local gitconfig contains a remote.origin.url add all user defined
        Git IDs matching remote.origin.url as a candidate. However if there is
        not a single match then add all available user defined Git IDs and the
        global Git ID as candidates.

        Args:
            config (dict): Contains validated configuration options
            url (str): A remote.origin.url

        Returns:
            candidates (dict): Contains preselected Git ID candidates
    """
    local_ids = config["git_local_ids"]
    netloc = urllib.parse.urlparse(url)[1]

    # A generator to filter matching sections:
    # Let's see if user defined IDs match remote.origin.url
    def generate_candidates():
        for key, value in local_ids.items():
            if value.get("service") == netloc:
                yield (key, value)

    candidates = dict(generate_candidates())

    if len(candidates) >= 1:
        msg = """
            ~Intermission~
                One or more identities match your current git provider.
                remote.origin.url: {}
        """
        print(textwrap.dedent(msg).lstrip().format(url))
    else:
        candidates = local_ids
        msg = """
            ~Intermission~
                Zero passports matching - listing all passports.
                remote.origin.url: {}
        """

        print(textwrap.dedent(msg).lstrip().format(url))
        add_global_id(config, candidates)

    print_choice(candidates)
    return candidates


def no_url_exists(config, url):
    """ If a local gitconfig does not contain a remote.origin.url add
        all available user defined Git IDs and the global Git ID as
        candidates.

        Args:
            config (dict): Contains validated configuration options
            url (str): A remote.origin.url

        Returns:
            candidates (dict): Contains preselected Git ID candidates
    """
    candidates = config["git_local_ids"]
    msg = """
        ~Intermission~
            «remote.origin.url» is not set, listing all IDs:
    """

    add_global_id(config, candidates)
    print(textwrap.dedent(msg).lstrip())
    print_choice(candidates)
    return candidates


# ........................................................................ Glue
def main():
    config_file = os.path.expanduser("~/.git_passport")
    config_create(config_file)

    config = config_validate(config_read(config_file))

    if config["enable_hook"]:
        local_email = git_get_id(config, "local", "email")
        local_name = git_get_id(config, "local", "name")
        local_url = git_get_url()

        if local_email and local_name:
            identity_exists(config, local_email, local_name, local_url)
        elif local_url:
            candidates = url_exists(config, local_url)
        else:
            candidates = no_url_exists(config, local_url)

        selected_id = get_user_input(candidates.keys())
        git_set_id(config, candidates[selected_id]["email"], "email")
        git_set_id(config, candidates[selected_id]["name"], "name")
        print("\n~Done~\n")


if __name__ == "__main__":
    main()

EDIT: Since there has been posted an answer I'll leave the question as it is / freeze it in order to avoid any discrepancies. Development continues at the repository.

\$\endgroup\$
5
\$\begingroup\$

Architecture / general thoughts

Overall looks good. You have lots of mostly useful comments which help to understand the program, the code is clear if a little verbose, so to me this was easy to read.

That said I'm going to list a few things which could improve the program and then continue with in detail code issues below.

Hope that helps and good luck with the program, it looks like a useful tool e.g. if you work both on personal projects and e.g. for a company. Could you post a link to your repository? It would be great if you could package and release this. A quick search also mentions the need to use different Github user names and SSH keys, so there are definitely more use cases (and options) to consider in the future.

  • I'd get rid of the config_validate function and do that in config_read instead, using a separate config_read_passport or so, which then uses ConfigParser.getboolean and the other get... functions with a very strict schema, so you make sure that everything coming out of the config object is properly parsed. That way you have to write less verification code (as the ConfigParser object takes care of that for you) and you can spend that on making sure that the individual passports have the correct format before handing them off to the rest of the application.
  • I personally don't use origin as a remote very often. It would be great if that could be configurable.
  • A script like this is probably fine without using classes. I can see how using it for the configuration would help structuring it better, but it's by no means necessary if this works for you.
  • I kind of find the output with lots of irregular characters unexpected, but of course that's your choice.

Code

  • The docstrings say Returns: error (str): Exception, but the functions don't return exceptions, they raise them, so the docstring should say Throws or Raises instead and mention what kind of exception it uses. If you can't know that, e.g. in most of the git_* functions, leave it out, or refer to the specific function which might cause problems, i.e. subprocess.Popen.
  • Just as example, in config_create you can remove one level of indentation if you return early, i.e. if not os.path.exists(filename): return;.
  • This is a style choice, but the creation of the preset dictionary can be shorter if you'd just use the literal dictionary syntax, i.e. preset = {"General": {...}, "Passport 0": {...}, ...} instead of repeating the keys all the time.
  • textwrap.dedent(foo).strip() is nice, I'm copying that; since you use it very often I think that separate functions, like dedented or so, are in order; something short and simple. Same for lstrip.
  • sys.exit already exits the process, no raise necessary, unless I'm completely missing the idiom here. And then maybe don't catch the exception in the first place, just let it propagate. The result will be the same if you already print the exception.
  • Also, I would use less sys.exits in general. It is helpful if you can just import the script for testing purposes and it's jarring if using e.g. config_create suddenly kills the interpreter. Same for proper testing later.
  • generate_matches could very well be a regular function and accept the two arguments pattern and raw_config instead.
  • I'd wrap getting values from git in a separate function, maybe git_config_get, which would (for now) still use the same subprocess.Popen method with passed in arguments and then does the communicate/decode handling. And reraising the exception isn't necessary. Whether you create a separate git_config_set (instead of using the same mechanism as git config) is kind of a trade-off. Performance-wise there's always the option to not call a separate program and use something like a libgit binding, e.g. pygit2 instead.
  • get_user_input should reset sys.stdin to its previous value I think. Again, think of reusing this; same with the sys.exit.
  • I'd structure the loop more like while True: read selection; if in pool: return selection. That's way less confusing than if not in pool: continue; (else) break; return.
  • For performance, you can always use iteritems on dictionaries if you don't need the intermediate list.
  • add_global_id does nothing if either global_email or global_name doesn't exist. Shouldn't it rather mention that problem to the user?
  • sys.exit(time.sleep(..)) implies that the return value of time.sleep is somehow significant. time.sleep(); sys.exit() is clearer. I'd also consider waiting for a keypress (or newline) from the user before exiting instead of using a timeout.
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for your feedback I'll work through your review step by step during the next week. I updated the question with a link to the repository. \$\endgroup\$ – Saucier Jan 11 '15 at 13:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.