git: support partial opt-in to more correct loading of git config via repository.ConfigScoped() #1019
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
expose a package level variable allowing clients (including unit tests) to configure the config.ActiveFS used when reading git configuration from outside a local workingtree. the previous behavior of reading global and system level configuration from the actual underlying OS filesystem is preserved by default, while it now becomes possible to swap in alternate filesystems (e.g. in-memory). the DefaultFS() method returns a billy.Filesystem which is the equivalent of using osfs.Default directly and can be used to reset the current default behavior of using the underyling OS filesystem directly.
I have discovered a bug in the go-git v5 implementation of loading git config in that it does not behave as expected according to the git config documentation, specifically in how it loads local config only from the in-memory config storer by default, in contrast to the documented behavior that by default git should load and respect a merged configuration by layering values from several convention- based file locations, including "global" config typically located at $HOME/.gitconfig and "system" config typically located at /etc/gitconfig There are likely additional bugs hiding in the v5 implementation of merging config values as the src and dest parameters to the mergo.Merge(...) function appear to be opposite from expected; based on the documentation of the mergo.Merge(...) command I expect src to get merged into dest but the usage in repository.ConfigScoped(..) looks like it is trying to merge dest into src. In discussion with project maintainers (see go-git#395) it was determined that while the go-git v5 implementation diverges from what clients would expect, fixing it constitutes a breaking change for clients who have coded to the existing v5 config behavior. This commit introduces one possible method to support v5 and v6 config implementations side-by-side in a non-breaking way, allowing clients to opt-in to v6 behavior ahead of the go-git v6 release. Applying this commit adds new options to the config.Scope enum including - V6DefaultScope - V6SystemScope - V6GlobalScope - V6LocalScope Config path resolution is implemented according to the description section of the current git 2.43 documentation for the config subcommand: When reading, the values are read from the system, global and repository local configuration files by default, and options --system, --global, --local, --worktree and --file <filename> can be used to tell the command to read from only that location (see FILES). - https://git-scm.com/docs/git-config/2.43.0#_description And the related FILES section By default, git config will read configuration options from multiple files: $(prefix)/etc/gitconfig System-wide configuration file. $XDG_CONFIG_HOME/git/config ~/.gitconfig User-specific configuration files. When the XDG_CONFIG_HOME environment variable is not set or empty, $HOME/.config/ is used as $XDG_CONFIG_HOME. These are also called "global" configuration files. If both files exist, both files are read in the order given above. $GIT_DIR/config Repository specific configuration file. $GIT_DIR/config.worktree This is optional and is only searched when extensions.worktreeConfig is present in $GIT_DIR/config. You may also provide additional configuration parameters when running any git command by using the -c option. See git[1] for details. Options will be read from all of these files that are available. If the global or the system-wide configuration files are missing or unreadable they will be ignored. If the repository configuration file is missing or unreadable, git config will exit with a non-zero error code. An error message is produced if the file is unreadable, but not if it is missing. The files are read in the order given above, with last value found taking precedence over values read earlier. When multiple values are taken then all values of a key from all files will be used. - https://git-scm.com/docs/git-config/2.43.0#FILES This commit uses the existing mergo library to load either each of these files as a sequence based on which V6 scope is requested and merge them all into a single merged config. NOTE: This commit does not handle the --worktree nor --file <filename> flags as both options require a more extensive refactoring or reimplementation of the config system to access repository configuration while resolving config values in the --worktreee case and accept and pass the <filename> parameter to the config module in the --file case. Support for these options can be reconsidered in go-git v6 planning.
- add test cases for Repository.Config and Repository.ConfigScoped these test cases demonstrate the behaviour I expect for Repository.Config and Repository.ConfigScoped, based on the semantics of the git-config docs at git 2.43.0: https://git-scm.com/docs/git-config/2.43.0 When reading, the values are read from the system, global and repository local configuration files by default, and options --system, --global, --local, --worktree and --file <filename> can be used to tell the command to read from only that location (see FILES). - the current v5 implementations of repo.Config and repo.ConfigScoped are merging those config sources differently. - this commit adds branching based on the newer V6 config.Scope options in a non-breaking, backwards-compatible way that enables clients to opt-in to semantics that behave more like the git config subcommand ahead of the release of go-git v6
davidalpert
changed the title
davidalpert
changed the title
davidalpert
changed the title
pjbgf
reviewed
Feb 4, 2024
There was a problem hiding this comment.
@davidalpert thank you for following up on this. Below are some ideas and nits:
config/config.go
Outdated
Comment on lines
76
to
77
| // IsV6PreviewScope returns true when we want to opt into processing config commands using | ||
| // the newer semantics which will become the default in go-git v6 |
There was a problem hiding this comment.
Suggested change
| // IsV6PreviewScope returns true when we want to opt into processing config commands using | |
| // the newer semantics which will become the default in go-git v6 | |
| // IsV6PreviewScope returns true when we want to opt into processing config commands using | |
| // the newer semantics which will become the default in go-git v6. |
config/config.go
Outdated
Comment on lines
63
to
73
| // V6DefaultScope is equivalent to running config commands without any explicit flags | ||
| V6DefaultScope | ||
|
|
||
| // V6SystemScope is equivalent to running config commands with the --system flag | ||
| V6SystemScope | ||
|
|
||
| // V6GlobalScope is equivalent to running config commands with the --global flag | ||
| V6GlobalScope | ||
|
|
||
| // V6LocalScope is equivalent to running config commands with the --local flag | ||
| V6LocalScope |
There was a problem hiding this comment.
What are your thoughts on adding V6 as a suffix instead of a prefix?
Also, what is the transition/migration strategy for the V6 release? Would we keep the V6 suffixed Scopes and remove the previous ones?
It would be good to add a TODO for v6 on the outstanding steps.
There was a problem hiding this comment.
Moving V6 to a suffix is a fine choice.
As for the transition/migration strategy, I would leave that to you and the maintaining team.
My recommendation from a client/consumer perspective would be to
- have go-git v5 continue to default to the existing behavior with v6 as an opt-in choice
- with the release of go-git v6 announce this as a breaking change, flip the default behavior to v6, and either remove or leave the v5 behavior as an opt-in choice for legacy compatibility. in this case, I suggest announcing that it will be maintained for backward compatibility until v7 (or some other point in the future) but is deprecated and no longer supported as the recommended support path is to update to the v6 behavior.
config/config.go
Outdated
| // ActiveFS provides a configurable entry point to the billy.Filesystem in active use for reading global and | ||
| // system level configuration; override in a test to use a fake filesystem then reset to config.DefaultFS() to | ||
| // restore the default behavior. | ||
| ActiveFS = DefaultFS() |
There was a problem hiding this comment.
If this is for testing purposes only, I would make it private instead.
From a public API perspective, we probably want to get Config to work more like the core functionality, whereby such configuration is provided via ...WithOptions when doing New/Load configs.
There was a problem hiding this comment.
While looking at this I see that a Repository stores a billy.Filesystem rooted at the worktree of the repo, which does not allow access to files outside the repo.
Line 85 in 25aadd1
The core of #395 is that by default git operations require files outside the worktree as the default behavior is to merge system and global config into local config before executing any operation.
I can look into adding a ...WithOptions parameter to pass in the root file system as well as the worktree fs though this may get confusing to consumers, passing in two different filesystems, especially as one is most often a subset of the other.
|
Thank you @pjbgf I very much appreciate the feedback (including the level of detail and specificity) and will work on those updates today. I see the test failures also so will double-check those after integrating your feedback. Also, I am experimenting (see comments in #395) with additional syntax to enable opting into a Would you prefer additional commits to this branch or rebased to fold this feedback into one or two overall commits? |
this reduces duplication and corrects a drift from the default loader introduced when config.DefaultFS was refactored.
apply code-review suggestions
- move load and paths to be functions taking a Scope receiver - remove IsV6PreviewScope as it is not publicly required and has been replaced inside Scope-based functions with simple and co-located case statements.
this requires passing the Repository.Storer into the new Scope.LoadFromConfigStorer(..) method which allows the config package to receive a storage.Storer and treat it as it's embedded config.ConfigStorer so that the caller determines which storage system gets read.
as per PR feedback
this commit introduces a config.WithOptions pattern to allow optional passing in of a billy.Filesystem via a config.WithFS(fs) options function. config.ActiveFS has moved into ain internal config.Options member and the repository tests which need to stub in a mock filesystem are now using this option to stub the underlying filesystem rather than setting a global config.ActiveFS variable.
pjbgf
requested changes
Mar 15, 2024
There was a problem hiding this comment.
@davidalpert I really like where this got to, thank you very much for working on this and apologies for the delay getting a second review.
There are a few nits and some points around backwards compatibility. Apart from that it LGTM.
| @@ -0,0 +1 @@ | |||
| golang 1.19 | |||
There was a problem hiding this comment.
We don't currently use .tool-versions, so please remove it from the PR.
| if scope == LocalScope { | ||
| return nil, fmt.Errorf("LocalScope should be read from the a ConfigStorer") | ||
| return nil, fmt.Errorf("LocalScope should be read from the a Repository.ConfigStorer") |
There was a problem hiding this comment.
Suggested change
| return nil, fmt.Errorf("LocalScope should be read from the a Repository.ConfigStorer") | |
| return nil, fmt.Errorf("LocalScope should be read from the Repository.ConfigStorer") |
Comment on lines
+92
to
+93
| // Available ConfigScopes; those with a V6 prefix are opt-ins to preview go-git v6 config behavior which | ||
| // more closely matches the `git config` subcommand documentation |
There was a problem hiding this comment.
Suggested change
| // Available ConfigScopes; those with a V6 prefix are opt-ins to preview go-git v6 config behavior which | |
| // more closely matches the `git config` subcommand documentation | |
| // Available ConfigScopes; those with a V6 suffix are opt-ins to preview go-git v6 config behavior which | |
| // more closely matches the `git config` subcommand documentation. | |
| // TODO: for v6 drop suffixes for all "V6" options. | |
| // TODO: for v6 add "V5" suffix for all legacy options and deprecated them. |
| return nil, err | ||
| } | ||
|
|
||
| _ = mergo.Merge(global, system) |
There was a problem hiding this comment.
Suggested change
| _ = mergo.Merge(global, system) | |
| // The merge errors are ignored to keep them in line with existing behaviour. | |
| _ = mergo.Merge(global, system) |
| } | ||
|
|
||
| // DefaultFS provides a billy.Filesystem abstraction over the os filesystem (via osfs.OS) scoped to the | ||
| // root directory / in order to enable access to global and system config files via absolute paths |
There was a problem hiding this comment.
Suggested change
| // root directory / in order to enable access to global and system config files via absolute paths | |
| // root directory / in order to enable access to global and system config files via absolute paths. |
Comment on lines
+99
to
+108
| // DefaultScopeV6 is equivalent to running config commands without any explicit flags | ||
| DefaultScopeV6 | ||
|
|
||
| // SystemScopeV6 is equivalent to running config commands with the --system flag | ||
| SystemScopeV6 | ||
|
|
||
| // GlobalScopeV6 is equivalent to running config commands with the --global flag | ||
| GlobalScopeV6 | ||
|
|
||
| // LocalScopeV6 is equivalent to running config commands with the --local flag |
There was a problem hiding this comment.
Suggested change
| // DefaultScopeV6 is equivalent to running config commands without any explicit flags | |
| DefaultScopeV6 | |
| // SystemScopeV6 is equivalent to running config commands with the --system flag | |
| SystemScopeV6 | |
| // GlobalScopeV6 is equivalent to running config commands with the --global flag | |
| GlobalScopeV6 | |
| // LocalScopeV6 is equivalent to running config commands with the --local flag | |
| // DefaultScopeV6 is equivalent to running config commands without any explicit flags. | |
| DefaultScopeV6 | |
| // SystemScopeV6 is equivalent to running config commands with the --system flag. | |
| SystemScopeV6 | |
| // GlobalScopeV6 is equivalent to running config commands with the --global flag. | |
| GlobalScopeV6 | |
| // LocalScopeV6 is equivalent to running config commands with the --local flag. |
| @@ -546,49 +548,25 @@ func cleanUpDir(path string, all bool) error { | |||
|
|
|||
| // Config return the repository config. In a filesystem backed repository this | |||
| // means read the `.git/config`. | |||
| func (r *Repository) Config() (*config.Config, error) { | |||
| return r.Storer.Config() | |||
| func (r *Repository) Config(o ...config.WithOptions) (*config.Config, error) { | |||
There was a problem hiding this comment.
I generally find variadic functions quite neat, and probably something we should look into using more on V6. However, it is important to note that this can break backwards compatibility on this specific case.
For example, if we were to go ahead with this change I believe we would break argocd-autopilot as they define an interface based on our previous signature.
| func (r *Repository) SetConfig(cfg *config.Config) error { | ||
| return r.Storer.SetConfig(cfg) | ||
| } | ||
|
|
||
| // ConfigScoped returns the repository config, merged with requested scope and | ||
| // lower. For example if, config.GlobalScope is given the local and global config | ||
| // are returned merged in one config value. | ||
| func (r *Repository) ConfigScoped(scope config.Scope) (*config.Config, error) { | ||
| func (r *Repository) ConfigScoped(scope config.Scope, o ...config.WithOptions) (*config.Config, error) { |
There was a problem hiding this comment.
Same here on the backwards compatibility. It is always hard to know how folks are using our public API, and changing it on a minor release would be problematic.
| @@ -37,16 +39,103 @@ var ( | |||
| ErrRemoteConfigEmptyName = errors.New("remote config: empty name") | |||
| ) | |||
|
|
|||
| // Options allows configuration of the config package | |||
There was a problem hiding this comment.
Maybe?
Suggested change
| // Options allows configuration of the config package | |
| // Options represents all configurable options for the config package. |
pjbgf
reviewed
Mar 19, 2024
| @@ -37,16 +39,103 @@ var ( | |||
| ErrRemoteConfigEmptyName = errors.New("remote config: empty name") | |||
| ) | |||
|
|
|||
| // Options allows configuration of the config package | |||
| type Options struct { | |||
There was a problem hiding this comment.
This don't really need to be externally accessible.
Suggested change
| type Options struct { | |
| type options struct { |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is an attempt to offer a non-breaking backwards-compatible opt-in approach to an improved handling of loading git configuration according to the documented behavior of the git config subcommand (e.g. see the docs for the latest version as of this PR v2.43.0)
As discussed in #395 it was determined by maintainers that, while the go-git v5 implementation diverges from what clients
would expect, fixing it constitutes a breaking change for any client who has coded to the existing v5 config behavior.
Here we introduce new options to the
config.Scopeenumeration:Using one of these options with
config.LoadConfig(scope Scope)orrepository.ConfigScoped(scope config.Scope)will load config according to the documented git behavior as described below:config.Scopevalueconfig.LoadConfig(..)repository.ConfigScoped(..)config.V6DefaultScopeconfig.LoadConfig(config.V6DefaultScope)then merges in local in-memory config from the repository's config storerconfig.V6SystemScopegit config --systemgit config --systemconfig.V6GlobalScopegit config --globalgit config --globalconfig.V6LocalScopegit config --local(from os)git config --local(from os + from config storer)NOTES:
--worktreenor--file <filename>flags as both options require a more extensive refactoring or reimplementation of the config system to access the repository configuration while resolving config values in the--worktreecase and accept and pass the<filename>parameter to the config module in the--filecase. Support for these options can be reconsidered ingo-gitv6 planning.go-gitv6 as use of v5Repository.Config()(without the ability to opt-in to more correct configuration loading behavior) is used extensively throughout thegitpackage.