10000 plumbing: format, config.Merged to allow access to local and global config by cap10morgan · Pull Request #20 · go-git/go-git · GitHub
[go: up one dir, main page]
Skip to content

plumbing: format, config.Merged to allow access to local and global config #20

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

Jump to bottom
Merged
merged 6 commits into from
Apr 7, 2020
Merged

plumbing: format, config.Merged to allow access to local and global config #20

merged 6 commits into from
Apr 7, 2020

Conversation

cap10morgan
Copy link
Contributor
@cap10morgan cap10morgan commented Apr 6, 2020 •
edited
Loading

...for reading and writing global (~/.gitconfig) and reading system (/etc/gitconfig) configs in addition to local repo config.

This is heavily based on / inspired by @djgilcrease's PR on the original src-d repo: src-d/go-git#1243. However, I ran into an issue using that code where if you ran SetConfig it would write out the merged system, global, and local config params all to your local ./.git/config file, which was hard to fix under that architecture.

So, I tried to take a simpler starting approach with this PR but still allow it to accomplish my 2 goals:

  1. Show a merged view of system (/etc/gitconfig), global (~/.gitconfig), and local (./.git/config) configs with the same precedence ordering that git itself uses (local overrides global overrides system).
  2. Allow setting / changing options in any of these files (with the exception of setting system options b/c we typically won't have permission to write to that file).

The way I did this was basically to leave the higher-level config stuff (e.g. .Remotes, .Core.IsBare) alone and add a new .Merged param that works similarly to .Raw but brings in these other configs. So now you can see the merged view with things like .Merged.Section("foo").Option("bar") (which is all read-only) or set options with things like .Merged.GlobalConfig().AddOption(...). .Raw now delegates to .Merged.LocalConfig().

I added a config example to demonstrate some of the usage and a couple of tests of marshalling and unmarshalling the different configs.

@cap10morgan
Add Merged config
ecb64c0 
...for reading and writing global (~/.git/config) and reading system (/etc/gitconfig) configs in addition to local repo config
@cap10morgan
Add funcs to set global & local config in Merged
231a188
@mcuadros mcuadros self-requested a review April 6, 2020 19:21
@mcuadros
Copy link
Member
mcuadros commented Apr 6, 2020

Can you fix the "Test" suite? Something is missing on the PR

@djgilcrease
Copy link

Thank you for picking this up @cap10morgan

@cap10morgan
Copy link
Contributor Author

Thank you for picking this up @cap10morgan

Thank you for the shoulders to stand on!

@cap10morgan
Copy link
Contributor Author

Can you fix the "Test" suite? Something is missing on the PR

Yep, sorry about that.

@mcuadros
Copy link
Member
mcuadros commented Apr 7, 2020

Ok, the code for the merge thing looks complex but fair. My only concern is about the example and promulgating the usage of the Config.Merged, this should be an advanced pattern (last resort) to access data not defined on the Config struct. So I suggest included a big note in the example explaining that is an advanced use case.

Also, if we are planning to use the author and committer info from the config, we should add the fields to it, as we have Core.IsBare and all other values. This can be done in this PR or in the following PRs.

mcuadros
mcuadros reviewed Apr 7, 2020
View reviewed changes
plumbing/format/config/merged.go
8000

type Scope int

const (
Copy link
Member

Choose a reason for hiding this comment

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

Documentation, in general, will be good, at least in the structs and consts.

@mcuadros mcuadros mentioned this pull request Apr 7, 2020
Closed
@cap10morgan
Copy link
Contributor Author

Ok, the code for the merge thing looks complex but fair. My only concern is about the example and promulgating the usage of the Config.Merged, this should be an advanced pattern (last resort) to access data not defined on the Config struct. So I suggest in 8000 cluded a big note in the example explaining that is an advanced use case.

Done in 4d4e2b1.

Also, if we are planning to use the author and committer info from the config, we should add the fields to it, as we have Core.IsBare and all other values. This can be done in this PR or in the following PRs.

OK. I'm working on some code to do that, but will post a separate PR for it.

@cap10morgan cap10morgan mentioned this pull request Apr 7, 2020
Closed
@mcuadros mcuadros changed the title Add Merged config plumbing: format, config.Merged to allow access to local and global config Apr 7, 2020
@mcuadros mcuadros merged commit 3127ad9 into go-git:master Apr 7, 2020
@b4nst
Copy link
b4nst commented May 17, 2020

Thanks for the PR ! Any idea of when the next release with this feature will be published ?

mcuadros added a commit to mcuadros/go-git that referenced this pull request May 24, 2020
@mcuadros
Revert "Merge pull request go-git#20 from quorumcontrol/feature/other…
c809794 
…-configs"

This reverts commit 3127ad9, reversing
changes made to 73c52ed.
@mcuadros mcuadros mentioned this pull request May 24, 2020
Merged
@b4nst b4nst mentioned this pull request May 25, 2020
Closed
@davidalpert davidalpert mentioned this pull request Aug 27, 2022
Open
traidare pushed a commit to traidare/go-git that referenced this pull request Oct 26, 2024
@mcuadros
Merge pull request go-git#20 from quorumcontrol/feature/other-configs
b90bf06 
plumbing: format, config.Merged to allow access to local and global config
traidare pushed a commit to traidare/go-git that referenced this pull request Oct 26, 2024
@mcuadros
Revert "Merge pull request go-git#20 from quorumcontrol/feature/other…
3820ed1 
…-configs"

This reverts commit b90bf06, reversing
changes made to 7bb4be4.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcuadros mcuadros mcuadros left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cap10morgan @mcuadros @djgilcrease @b4nst
0