-
-
Notifications
You must be signed in to change notification settings - Fork 501
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
Improved config handling and bugfixes #2716
Conversation
Signed-off-by: Yolan Romailler <[email protected]>
Signed-off-by: Yolan Romailler <[email protected]>
Fixes #2673 Signed-off-by: Yolan Romailler <[email protected]>
Signed-off-by: Yolan Romailler <[email protected]>
348dead
to
3fbd38d
Compare
While doing this, I realized we are not properly supporting worktree level configs in gopass: Is that expected @dominikschulz ? |
My goal was to support worktree configs, but it was rather low priority and at this point I don't remember if I finished that part or not. But the documentation claims that I didn't complete it: https://pkg.go.dev/github.com/gopasspw/gopass/pkg/gitconfig#hdr-Known_limitations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! A few more remarks and questions, but I think we're almost there.
Signed-off-by: Yolan Romailler <[email protected]>
Signed-off-by: Yolan Romailler <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
This PR is doing a few changes to the way we handle the Config action, its logging and also improves a few comments.
This is notably fixing #2673 by properly handling all the different cases for a local vs global config change not necessarily leading to a
git add
andgit commit
in the store.Sadly, there is no way when using the gitconfig
Set
command to know if the option was written to the global vs local config, and so to do this really nicely we would need to change the signature of thefunc (c *Config) Set(mount, key, value string) error
function to include a more explicit return value.But we can fix the problem by properly handling the different errors than can arise during the Add and Commit phases of the config, which is what this PR does.
This PR also prevents us from setting the global config while reporting a misleading "No existing configuration found" error.
Previously, upon using
gopass config some.name true
on an uninitialized store, we would get the following:Now we get just the expected
true
, since we do support changing the global config without having a store. When trying to do it on a local config, tho, it will now fail: