Improved config handling and bugfixes (#2716)

* [CLEANUP] Misc changes around debug logging and configs Signed-off-by: Yolan Romailler <[email protected]> * [BUGFIX] Handle uninitialized stores with gopass config Signed-off-by: Yolan Romailler <[email protected]> * [BUGFIX] Do not always commit upon config changes Fixes #2673 Signed-off-by: Yolan Romailler <[email protected]> * [n/a] Following code review comments Signed-off-by: Yolan Romailler <[email protected]> * [n/a] More code review changes Signed-off-by: Yolan Romailler <[email protected]> * [n/a] Forgot one change Signed-off-by: Yolan Romailler <[email protected]> --------- Signed-off-by: Yolan Romailler <[email protected]>
gopasspw · Nov 24, 2023 · d168602 · d168602
1 parent 1c624dd
commit d168602
Show file tree

Hide file tree

Showing 6 changed files with 112 additions and 27 deletions.
diff --git a/docs/config.md b/docs/config.md
@@ -18,8 +18,8 @@ Some configuration options are only available through setting environment variab
 | `GOPASS_DEBUG_FILES`         | `string` | Comma separated filter for console debug output (files)                                                                                                           |
 | `GOPASS_DEBUG_FUNCS`         | `string` | Comma separated filter for console debug output (functions)                                                                                                       |
 | `GOPASS_DEBUG_LOG_SECRETS`   | `bool`   | Set to any non-empty value to enable logging of credentials                                                                                                       |
-| `GOPASS_DEBUG_LOG`           | `string` | Set to a filename to enable debug logging                                                                                                                         |
-| `GOPASS_DEBUG`               | `bool`   | Set to any non-empty value to enable verbose debug output                                                                                                         |
+| `GOPASS_DEBUG_LOG`           | `string` | Set to a filename to enable debug logging (only set GOPASS_DEBUG to log to stderr)                                                                                |
+| `GOPASS_DEBUG`               | `bool`   | Set to any non-empty value to enable verbose debug output, by default on stderr, unless GOPASS_DEBUG_LOG is set                                                   |
 | `GOPASS_EXTERNAL_PWGEN`      | `string` | Use an external password generator. See [Features](features.md#using-custom-password-generators) for details                                                      |
 | `GOPASS_FORCE_CHECK`         | `string` | (internal) Force the updater to check for updates. Used for testing.                                                                                              |
 | `GOPASS_FORCE_UPDATE`        | `bool`   | Set to any non-empty value to force an update (if available)                                                                                                      |

diff --git a/internal/action/config.go b/internal/action/config.go
@@ -6,6 +6,7 @@ import (
 	"fmt"
 
 	"github.com/gopasspw/gopass/internal/action/exit"
+	"github.com/gopasspw/gopass/internal/config"
 	"github.com/gopasspw/gopass/internal/out"
 	"github.com/gopasspw/gopass/internal/set"
 	istore "github.com/gopasspw/gopass/internal/store"
@@ -34,10 +35,10 @@ func (s *Action) Config(c *cli.Context) error {
 		return exit.Error(exit.Usage, nil, "Usage: %s config key value", s.Name)
 	}
 
-	// need to initialize sub-stores so we can update their configs.
-	// special case: we can always update the root config.
-	if err := s.IsInitialized(c); err != nil && store != "" {
-		return err
+	// sub-stores need to have been initialized so we can update their local configs.
+	// special case: we can always update the global config. NB: IsInitialized initializes the store if nil.
+	if inited, err := s.Store.IsInitialized(ctx); err != nil || (store != "" && !inited) {
+		return exit.Error(exit.Unknown, err, "Store %s seems uninitialized or cannot be initialized", store)
 	}
 
 	if err := s.setConfigValue(ctx, store, c.Args().Get(0), c.Args().Get(1)); err != nil {
@@ -80,20 +81,50 @@ func contains(haystack []string, needle string) bool {
 func (s *Action) setConfigValue(ctx context.Context, store, key, value string) error {
 	debug.Log("setting %s to %s for %q", key, value, store)
 
-	if err := s.cfg.Set(store, key, value); err != nil {
+	level, err := s.cfg.SetWithLevel(store, key, value)
+	if err != nil {
 		return fmt.Errorf("failed to set config value %q: %w", key, err)
 	}
 
+	// in case of a non-local config change we don't need to track changes in the store
+	if level != config.Local && level != config.Worktree {
+		debug.Log("did not set local config (was config level %d) in store %q, skipping commit phase", level, store)
+		s.printConfigValues(ctx, store, key)
+
+		return nil
+	}
+
 	st := s.Store.Storage(ctx, store)
 	if st == nil {
 		return fmt.Errorf("storage not available")
 	}
 
-	if err := st.Add(ctx, "config"); err != nil && !errors.Is(err, istore.ErrGitNotInit) {
-		return fmt.Errorf("failed to stage config file: %w", err)
+	configFile := "config"
+	if level == config.Worktree {
+		configFile = "config.worktree"
 	}
-	if err := st.Commit(ctx, "Update config"); err != nil && !errors.Is(err, istore.ErrGitNotInit) {
-		return fmt.Errorf("failed to commit config: %w", err)
+
+	// notice that the cfg.Set above should have created the local config file.
+	if !st.Exists(ctx, configFile) {
+		return fmt.Errorf("local config file %s didn't exist in store, this is unexpected", configFile)
+	}
+
+	switch err := st.Add(ctx, configFile); {
+	case err == nil:
+		debug.Log("Added local config for commit")
+	case errors.Is(err, istore.ErrGitNotInit):
+		debug.Log("Skipping staging of local config %q: %v", configFile, err)
+	default:
+		return fmt.Errorf("failed to Add local config %q: %w", configFile, err)
+	}
+
+	switch err := st.Commit(ctx, "Update config"); {
+	case err == nil:
+		debug.Log("Committed local config")
+	case errors.Is(err, istore.ErrGitNotInit), errors.Is(err, istore.ErrGitNothingToCommit):
+		debug.Log("Skipping Commit of local config: %v", err)
+	default:
+		return fmt.Errorf("failed to commit local config: %w", err)
 	}
 
 	s.printConfigValues(ctx, store, key)

diff --git a/internal/backend/storage/fs/store.go b/internal/backend/storage/fs/store.go
@@ -151,7 +151,7 @@ func (s *Store) Exists(ctx context.Context, name string) bool {
 	}
 	path := filepath.Join(s.path, filepath.Clean(name))
 	found := fsutil.IsFile(path)
-	debug.Log("Checking if %s exists at %s: %t", name, path, found)
+	debug.Log("Checking if '%s' exists at %s: %t", name, path, found)
 
 	return found
 }

diff --git a/internal/config/config.go b/internal/config/config.go
@@ -22,6 +22,18 @@ var (
 	systemConfig = "/etc/gopass/config"
 )
 
+type Level int
+
+const (
+	None Level = iota
+	Env
+	Worktree
+	Local
+	Global
+	System
+	Preset
+)
+
 func newGitconfig() *gitconfig.Configs {
 	c := gitconfig.New()
 	c.EnvPrefix = envPrefix
@@ -182,19 +194,29 @@ func (c *Config) GetInt(key string) int {
 //   - If mount has any other value we will attempt to write the setting to the per-directory config of this mount.
 //   - If the mount point does not exist we will return nil.
 func (c *Config) Set(mount, key, value string) error {
+	_, err := c.SetWithLevel(mount, key, value)
+
+	return err
+}
+
+// SetWithLevel is the same as Set, but it also returns the level at which the config was set.
+// It currently only supports global and local configs.
+func (c *Config) SetWithLevel(mount, key, value string) (Level, error) {
 	if mount == "" {
-		return c.root.SetGlobal(key, value)
+		return Global, c.root.SetGlobal(key, value)
 	}
 
 	if mount == "<root>" {
-		return c.root.SetLocal(key, value)
+		return Local, c.root.SetLocal(key, value)
 	}
 
-	if cfg := c.cfgs[mount]; cfg != nil {
-		return cfg.SetLocal(key, value)
+	if cfg, ok := c.cfgs[mount]; !ok {
+		return None, fmt.Errorf("substore %q is not initialized or doesn't exist", mount)
+	} else if cfg != nil {
+		return Local, cfg.SetLocal(key, value)
 	}
 
-	return nil
+	return None, nil
 }
 
 // SetEnv overrides a key in the non-persistent layer.
@@ -217,7 +239,7 @@ func (c *Config) SetPath(path string) error {
 	return c.Set("", "mounts.path", path)
 }
 
-// SetMountPath is a short cut to set a mount to a path.
+// SetMountPath is a shortcut to set a mount to a path.
 func (c *Config) SetMountPath(mount, path string) error {
 	return c.Set("", mpk(mount), path)
 }

diff --git a/pkg/gitconfig/config.go b/pkg/gitconfig/config.go
@@ -24,6 +24,22 @@ type Config struct {
 	vars     map[string][]string
 }
 
+// IsEmpty returns true if the config is empty (typically a newly initialized config, but still unused).
+// Since gitconfig.New() already sets the global path to the globalConfigFile() one, we cannot rely on
+// the path being set to checki this. We need to check the  raw length to be sure it wasn't just
+// the default empty config struct.
+func (c *Config) IsEmpty() bool {
+	if c == nil || c.vars == nil {
+		return true
+	}
+
+	if c.raw.Len() > 0 {
+		return false
+	}
+
+	return true
+}
+
 // Unset deletes a key.
 func (c *Config) Unset(key string) error {
 	if c.readonly {
@@ -130,7 +146,7 @@ func (c *Config) Set(key, value string) error {
 }
 
 func (c *Config) insertValue(key, value string) error {
-	debug.Log("input (%s: %s): ---------\n%s\n-----------\n", key, value, c.raw.String())
+	debug.Log("input (%s: %s): \n--------------\n%s\n--------------\n", key, value, strings.Join(strings.Split("- "+c.raw.String(), "\n"), "\n- "))
 
 	wSection, wSubsection, wKey := splitKey(key)
 
@@ -188,7 +204,7 @@ func (c *Config) insertValue(key, value string) error {
 	c.raw.WriteString(strings.Join(lines, "\n"))
 	c.raw.WriteString("\n")
 
-	debug.Log("output: ---------\n%s\n-----------\n", c.raw.String())
+	debug.Log("output: \n--------------\n%s\n--------------\n", strings.Join(strings.Split("+ "+c.raw.String(), "\n"), "\n+ "))
 
 	return c.flushRaw()
 }
@@ -215,15 +231,15 @@ func parseSectionHeader(line string) (section, subsection string, skip bool) { /
 // rewriteRaw is used to rewrite the raw config copy. It is used for set and unset operations
 // with different callbacks each.
 func (c *Config) rewriteRaw(key, value string, cb parseFunc) error {
-	debug.Log("input (%s: %s): ---------\n%s\n-----------\n", key, value, c.raw.String())
+	debug.Log("input (%s: %s): \n--------------\n%s\n--------------\n", key, value, strings.Join(strings.Split("- "+c.raw.String(), "\n"), "\n- "))
 
 	lines := parseConfig(strings.NewReader(c.raw.String()), key, value, cb)
 
 	c.raw = strings.Builder{}
 	c.raw.WriteString(strings.Join(lines, "\n"))
 	c.raw.WriteString("\n")
 
-	debug.Log("output: ---------\n%s\n-----------\n", c.raw.String())
+	debug.Log("output: \n--------------\n%s\n--------------\n", strings.Join(strings.Split("+ "+c.raw.String(), "\n"), "\n+ "))
 
 	return c.flushRaw()
 }
@@ -239,7 +255,7 @@ func (c *Config) flushRaw() error {
 		return err
 	}
 
-	debug.Log("writing config to %s: -----------\n%s\n--------------", c.path, c.raw.String())
+	debug.Log("writing config to %s: \n--------------\n%s\n--------------", c.path, c.raw.String())
 
 	if err := os.WriteFile(c.path, []byte(c.raw.String()), 0o600); err != nil {
 		return fmt.Errorf("failed to write config to %s: %w", c.path, err)

diff --git a/pkg/gitconfig/configs.go b/pkg/gitconfig/configs.go
@@ -60,7 +60,7 @@ func (cs *Configs) Reload() {
 
 // String implements fmt.Stringer.
 func (cs *Configs) String() string {
-	return fmt.Sprintf("GitConfigs{Env: %s - System: %s - Global: %s - Local: %s - Worktree: %s}", cs.EnvPrefix, cs.SystemConfig, cs.GlobalConfig, cs.LocalConfig, cs.WorktreeConfig)
+	return fmt.Sprintf("GitConfigs{Workdir: %s - Env: %s - System: %s - Global: %s - Local: %s - Worktree: %s}", cs.workdir, cs.EnvPrefix, cs.SystemConfig, cs.GlobalConfig, cs.LocalConfig, cs.WorktreeConfig)
 }
 
 // LoadAll tries to load all known config files. Missing or invalid files are
@@ -69,7 +69,7 @@ func (cs *Configs) String() string {
 func (cs *Configs) LoadAll(workdir string) *Configs {
 	cs.workdir = workdir
 
-	debug.Log("Loading gitconfigs for %s ...", cs)
+	debug.Log("Loading gitconfigs for %s", cs)
 
 	// load the system config, if any
 	if os.Getenv(cs.EnvPrefix+"_NOSYSTEM") == "" {
@@ -115,7 +115,7 @@ func (cs *Configs) LoadAll(workdir string) *Configs {
 			// set the path just in case we want to modify / write to it later
 			cs.worktree.path = worktreeConfigPath
 		} else {
-			debug.Log("[%s] loaded local config from %s", cs.EnvPrefix, worktreeConfigPath)
+			debug.Log("[%s] loaded worktree config from %s", cs.EnvPrefix, worktreeConfigPath)
 			cs.worktree = c
 		}
 	}
@@ -145,6 +145,22 @@ func (cs *Configs) loadGlobalConfigs() string {
 		locs = append(locs, filepath.Join(appdir.UserHome(), cs.GlobalConfig))
 	}
 
+	// if we already have a global config we can just reload it instead of trying all locations
+	if !cs.global.IsEmpty() {
+		if p := cs.global.path; p != "" {
+			debug.Log("[%s] reloading existing global config from %s", cs.EnvPrefix, p)
+			cfg, err := LoadConfig(p)
+			if err != nil {
+				debug.Log("[%s] failed to reload global config from %s", cs.EnvPrefix, p)
+			} else {
+				cs.global = cfg
+
+				return p
+			}
+		}
+	}
+
+	debug.Log("[%s] trying to find global configs in %v", cs.EnvPrefix, locs)
 	for _, p := range locs {
 		// GlobalConfig might be set to an empty string to disable it
 		// and instead of the XDG_CONFIG_HOME path only.
@@ -166,7 +182,7 @@ func (cs *Configs) loadGlobalConfigs() string {
 
 	debug.Log("[%s] no global config found", cs.EnvPrefix)
 
-	// set the path in case we want to write to it (create it) later
+	// set the path to the default one in case we want to write to it (create it) later
 	cs.global = &Config{
 		path: globalConfigFile(),
 	}