Python-based Git pre-commit hook to manage multiple users/Git identities

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"

t

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.

StackExchange Code Review Q#76935, answer score: 6