Feature: user config attrs #23
Conversation
|
Can you please rebase? |
ad56a0e to
b538daf
Compare
Done. |
There was a problem hiding this comment.
I believe that the Config struct should have only scalar types and not the ScopedString, this should be only for the internal package.
When we call Repository.Config should return Config with the values merged from all the scopes from system to repository. And ideally, create a new method called Repository.ConfigWithOptions where we can provide the context (ConfigOptions)
What do you think?
| @@ -53,6 +104,29 @@ type Config struct { | |||
| Window uint | |||
| } | |||
|
|
|||
| User struct { | |||
| Name ScopedString | |||
There was a problem hiding this comment.
Why we use ScopedStrings, instead of just load the config using the priority? Global, User, Project?
There was a problem hiding this comment.
The primary reason is that we need to remember the scope for when / if we write the config back out to the underlying files. We need to know where each value came from (even if someone changed it).
I think we're either going to have to overhaul the Config stuff to not allow editing scoped values in the high-level structs or keep track of this scope information somewhere else.
There was a problem hiding this comment.
I see but I rather not exposing this complex API. If we need to write back the config, we should request a clean copy of the Repository one (without any merge), and save this. It will be very complex and error-prone, allow persist a config from a different context.
There was a problem hiding this comment.
The API on the scoped attrs is .Value() and .Set(), which doesn't seem so complex. Would it help if this were consistent across all top-level config attrs? That would be a breaking change but if we made them private struct members we could then expose their names as getter methods (and Set[Attr] for setters) so the migration would be fairly simple for users (i.e. just adding () after the attr names everywhere).
Currently there's nothing preventing writing configs because it's all based around public struct members. With un-scoped attrs, you set arbitrary struct members and then call SetConfig and suddenly your local .git/config file contains the merged result of your system, global, and local configs. I think we would need some kind of breaking change to prevent that or this library is going to mess up peoples' configs in a way that will be a ticking time bomb for them (i.e. everything will seem normal until their repo hangs onto some global or system config param they change and assume applies everywhere).
So it sounds like you're proposing some kind of new top-level, merged, read-only config. And if you want to be able to make changes to it, you have to request a specific scope's config which will be written back to that scope's corresponding file. This will be a breaking change, is that OK? Am I understanding your proposal correctly?
There was a problem hiding this comment.
Here is some things that are important: Repository.Config MUST return the repository config. Since it's a method from the repository and the goal is to operate with it. Storer.SetConfig MUST not write config.Config structs with merged information.
About how to identify the config, ideally is have a MergedConfig or something but since we can't change the Config to an interface, it should be done for v6. So meanwhile we can add config.Config.Context for example.
Regarding reading the merged context, we can add config.ReadConfig(context) config.Config, as a helper for the people that wants access to this configuration. We must remember that the config merge is a very porcelain feature, so it's good to have it optional and explicit.
Internally Repository.Commit my use the new config.ReadConfig to read the config if a new flag called something like CommitOpts.UseGlobalConfig is true (by default false)
In this way, we have something that doesn't break the API or introduce unexpected behaviors. And we introduce an important feature requested by many users.
Thanks for your efforts and sorry for not being clear, but this library is a big codebase, and is hard to have it on top of the mind.
There was a problem hiding this comment.
@cap10morgan are you planning to work on this soon? if not, I will take the task, I want to release a new version and I want to include this.
There was a problem hiding this comment.
Sorry, I probably won't get to it soon. I'm no longer working on the project that needed this, so harder to prioritize spending work hours on it. Let me know if I can help with the effort in any other ways, though.
This builds on my earlier PR #20 to add top-level config attrs
User,Author, andCommitter. These work a little differently from the others by necessity because they are scoped (i.e. they can appear in local, global, and/or system config files). So this also demonstrates one way in which we could add more such scoped values to the top-level config.These new config attributes store
ScopedStrings and (in the case ofUser.UseConfigOnly) aScopedBool. These haveValue()andSet(scope, val)methods.Valuegets the effective value that git would use based on the config scope precedence andSetallows setting values w/ their scopes so we have that to keep track of.I think ultimately this and #20 point towards a bigger config overhaul for a v6+ release that is centered around interfaces rather than structs. So, for example, calling a method like
cfg.User.Name()could look up the effective name value from inside the scoped config data structures rather than having to copy values back and forth from the raw / merged backing data and the top-level config struct.