Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a class for interacting with git config #69

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+66 −15
Open

Add a class for interacting with git config #69

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 66 additions & 15 deletions git-imerge
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,6 +580,67 @@ class AutomaticMergeFailed(Exception):
self.commit1, self.commit2 = commit1, commit2


class GitConfigError(Exception):
def __init__(self, returncode, output):
Exception.__init__(
self, 'Git config failed with exit code %s: %s' % (returncode, output,)
)


def memo(obj):
cache = {}
@functools.wraps(obj)
def wrap(*args, **kwds):
if args not in cache:
cache[args] = obj(*args, **kwds)
return cache[args]
return wrap


@memo
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was confusing to me. Let me see if I understand correctly...

This level of memoization is so that if the code tries to create multiple instances of GitConfigStore with the same name and config_prefix arguments, that only one instance is actually created. Right?

This is distinct from the caching that occurs within the GitConfigStore class itself, which is about reading all of the configuration values in a section of the config file and keeping this cache in sync as values are changed or removed.

It seems a bit overengineered to me, especially the fact that memo effectively uses a global cache. Wouldn't it be more straightforward to store an instance of GitConfigStore(name) in the MergeState instance? Or will future changes make it necessary to access the config for a given name from outside of that class?

class GitConfigStore(object):
def __init__(self, name, config_prefix='imerge'):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand. name doesn't seem to be used, aside from its role in memoization, and I don't see how that has a useful effect. Would you please explain?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing: depending on what effect name is supposed to have, maybe it should default to None to avoid having to pass None to the constructor explicitly, below.

self.config_prefix = config_prefix
self.config = self._get_all_keys()

def _get_all_keys(self):
d = {}
try:
items_with_prefix = check_output(
['git', 'config', '--get-regex', self.config_prefix]
).rstrip().split('\n')
for row in items_with_prefix:
k, v = row.split()
d[k[len(self.config_prefix + '.'):]] = v
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This storage scheme limits the GitConfigStore class to single-valued config settings, correct? That is OK, but it might be worth mentioning this limitation in the class docstring.

return d
except CalledProcessError:
return {}

def get(self, key):
return self.config.get(key)

def set(self, key, value):
self.config[key] = value
config_key = '.'.join([self.config_prefix, key])
try:
check_call(['git', 'config', config_key, value])
except CalledProcessError as e:
raise GitConfigError(e.returncode, e.output)

def unset(self, key):
if key in self.config:
del self.config[key]
config_key = '.'.join([self.config_prefix, key])
try:
check_call(['git', 'config', '--unset', config_key])
except CalledProcessError as e:
if e.returncode == 5:
# Value was not set
pass
else:
raise GitConfigError(e.returncode, e.output)


def automerge(commit1, commit2, msg=None):
"""Attempt an automatic merge of commit1 and commit2.

Expand Down Expand Up @@ -1865,27 +1926,17 @@ class MergeState(Block):
"""Set the default merge to the specified one.

name can be None to cause the default to be cleared."""

gcs = GitConfigStore(name)
if name is None:
try:
check_call(['git', 'config', '--unset', 'imerge.default'])
except CalledProcessError as e:
if e.returncode == 5:
# Value was not set
pass
else:
raise
gcs.unset("default")
else:
check_call(['git', 'config', 'imerge.default', name])
gcs.set("default", name)

@staticmethod
def get_default_name():
"""Get the name of the default merge, or None if none is currently set."""

try:
return check_output(['git', 'config', 'imerge.default']).rstrip()
except CalledProcessError:
return None
gcs = GitConfigStore(None)
return gcs.get("default")

@staticmethod
def _check_no_merges(commits):
Expand Down