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.

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

Conversation

dleen
Copy link
Contributor

@dleen dleen commented Jun 18, 2014

This commit adds the GitConfigStore class. This allows us to easily get and set keys and values in git config.

The purpose of this class is to enable commands to use git config in a unified manner. The functions to get and set the default imerge name now use this class.

The class is memoized to avoid repeated calls to git config.

Tests pass.

This commit adds the GitConfigStore class. This allows us to easily
get and set keys and values in git config.

Tests pass.
@dleen
Copy link
Contributor Author

dleen commented Jun 26, 2014

Did you get a chance to look at this?

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?

@mhagger
Copy link
Owner

mhagger commented Jul 17, 2014

If you continue to do memoization of the class constructor, it might make sense to split that into a separate commit to make each change more self-contained. But see my comment above where I express skepticism about whether this memoization is the correct approach. Another unfortunate side-effect of your memoization approach is that GitConfigStore cannot be used as a normal class; for example, if there were a reason to add static methods to the class, it would be impossible to access them.

In summary: I like the idea of having a class for interfacing with git config, but I think that the level of complication could be dialed back a notch or two unless/until we need it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants