From 2e0da9a8550700683c4dc2c373446a88ec7f8ee8 Mon Sep 17 00:00:00 2001 From: Muskan Paliwal Date: Tue, 25 Aug 2026 13:31:13 +0530 Subject: [PATCH 1/8] gogit: Git-compatible config get/set/unset with format-preserving writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses go-git/cli#12, which asked for CLI-level config introspection. Its suggested `config --set user.name` spelling is stale: Git rejects --set (exit 129), and current upstream (v2.55.GIT) documents `git config get|set|unset` subcommands. Both spellings are supported here — the modern subcommands and the legacy flags — so existing callers keep working. The previous configCmd corrupted any repository whose key had a subsection. `gogit config remote.origin.url ` wrote `origin.url` as an option under [remote] while leaving [remote "origin"] intact, producing a file neither Git nor go-git can parse: git config remote.origin.url -> fatal: bad config line 9 (exit 128) gogit config user.name -> illegal character U+002E '.' Fixing that means representing keys the way Git does — splitting at the first and last dot, so remote.team.one.url addresses [remote "team.one"] url — and it means not round-tripping the file through go-git's raw decoder/encoder. That pair is lossy in two ways that silently damage a user's config: it drops every comment, and its decoder folds [user ""] into [user] (decoder.go calls AddOption with an empty subsection, which Config.AddOption routes to the section), merging two distinct keys. So config file access moves to internal/plumbing/format/config, a line-oriented parser that keeps the original bytes and splices individual variables, leaving comments, blank lines, indentation and ordering untouched. It covers quoted values, escapes, backslash-newline continuations, trailing comments, CRLF, same-line options, legacy [a.sub] headers and empty subsections; anything it cannot parse becomes an error rather than a guess. Writes go through a temp file and rename, with the Close error checked, so a failed write cannot truncate the original. Also in this change: - Exit statuses match Git: 1 for a missing key, 5 for unsetting an absent key or collapsing a multivalued one, 128 for a malformed file. A missing key is now distinct from an explicitly empty value, which still prints one blank line and exits 0. - Reads consult system, XDG, global, local and -c overrides in Git's precedence order, honouring GIT_CONFIG_GLOBAL/SYSTEM/NOSYSTEM. Writes still default to the repository config. - --file, --local, --global, --system, --all and --path, with --path doing Git's path canonicalisation (leading ~), not file selection. - --add appends instead of replacing, and set refuses to overwrite a multivalued key without --all, as Git does. - Repository discovery handles .git files, linked worktrees (config resolves through commondir), bare repositories and GIT_DIR. Diagnostic paths are reported the way Git spells them rather than absolutised. Verified by 147 Go tests plus a differential harness that runs each invocation under Git 2.50.0 and gogit in twin repositories: 76/76 match on stdout, exit status and resulting file contents, and 17/18 match on stderr. The outstanding case is `--file ` inside a bare repo, where Git's cwd-prefix rewriting says ./config; reproducing that needs gogit to chdir to the top level, which is out of scope here. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Pp48YmGBdCMfRjvqEHRpe --- cmd/gogit/config-cmd.go | 339 ++++++-- cmd/gogit/config-scope.go | 262 ++++++ cmd/gogit/config.go | 21 + cmd/gogit/config_cmd_test.go | 843 +++++++++++++++++++ cmd/gogit/count-objects.go | 6 +- cmd/gogit/gitdir.go | 110 +++ cmd/gogit/main.go | 12 + cmd/gogit/main_test.go | 2 +- internal/plumbing/format/config/file.go | 391 +++++++++ internal/plumbing/format/config/file_test.go | 410 +++++++++ internal/plumbing/format/config/key.go | 144 ++++ internal/plumbing/format/config/key_test.go | 151 ++++ internal/plumbing/format/config/parser.go | 377 +++++++++ internal/plumbing/format/config/write.go | 82 ++ 14 files changed, 3085 insertions(+), 65 deletions(-) create mode 100644 cmd/gogit/config-scope.go create mode 100644 cmd/gogit/config_cmd_test.go create mode 100644 cmd/gogit/gitdir.go create mode 100644 internal/plumbing/format/config/file.go create mode 100644 internal/plumbing/format/config/file_test.go create mode 100644 internal/plumbing/format/config/key.go create mode 100644 internal/plumbing/format/config/key_test.go create mode 100644 internal/plumbing/format/config/parser.go create mode 100644 internal/plumbing/format/config/write.go diff --git a/cmd/gogit/config-cmd.go b/cmd/gogit/config-cmd.go index 818aeec..3069598 100644 --- a/cmd/gogit/config-cmd.go +++ b/cmd/gogit/config-cmd.go @@ -7,114 +7,331 @@ import ( "path/filepath" "strings" - formatcfg "github.com/go-git/go-git/v6/plumbing/format/config" + gitconfig "github.com/go-git/cli/internal/plumbing/format/config" "github.com/spf13/cobra" ) +// Git's documented exit statuses for git-config. +const ( + exitNotFound = 1 // the key was not found + exitInvalidKey = 1 // the section or key is invalid + exitUnsetMissing = 5 // unset of a key that does not exist + exitCannotReplace = 5 // single value cannot replace several + exitFatal = 128 // the config file could not be read +) + +// configOpts holds the flags shared by `config`, `config get`, `config set` +// and `config unset`. Each command owns its own instance so the modern and +// legacy spellings stay independently parseable. +type configOpts struct { + file string + local bool + global bool + system bool + + all bool + path bool +} + var ( - configUnsetAll bool - configAdd bool + legacyOpts configOpts + getOpts configOpts + setOpts configOpts + unsetOpts configOpts + + legacyGet bool + legacyGetAll bool + legacyAdd bool + legacyUnset bool + legacyUnsetAll bool + legacyReplaceAll bool ) +// registerLocation adds the flags that choose which configuration file to act +// on. They are mutually exclusive. +func (o *configOpts) registerLocation(cmd *cobra.Command) { + cmd.Flags().StringVarP(&o.file, "file", "f", "", "Use the given config file instead of the repository config") + cmd.Flags().BoolVar(&o.local, "local", false, "Use the repository config file") + cmd.Flags().BoolVar(&o.global, "global", false, "Use the global (per-user) config file") + cmd.Flags().BoolVar(&o.system, "system", false, "Use the system-wide config file") + cmd.MarkFlagsMutuallyExclusive("file", "local", "global", "system") +} + func init() { - configCmd.Flags().BoolVar(&configUnsetAll, "unset-all", false, "Remove all occurrences of the key") - configCmd.Flags().BoolVar(&configAdd, "add", false, - "Add a new value without altering existing ones (treated as a plain set for v1)") + legacyOpts.registerLocation(configCmd) + configCmd.Flags().BoolVar(&legacyOpts.path, "path", false, + "Canonicalize the value as a path, expanding a leading ~") + configCmd.Flags().BoolVar(&legacyGet, "get", false, "Get the value for the given key") + configCmd.Flags().BoolVar(&legacyGetAll, "get-all", false, "Get all values for the given key") + configCmd.Flags().BoolVar(&legacyAdd, "add", false, "Add a new value without altering existing ones") + configCmd.Flags().BoolVar(&legacyUnset, "unset", false, "Remove the value for the given key") + configCmd.Flags().BoolVar(&legacyUnsetAll, "unset-all", false, "Remove all occurrences of the key") + configCmd.Flags().BoolVar(&legacyReplaceAll, "replace-all", false, "Replace all values for the given key") + configCmd.MarkFlagsMutuallyExclusive("get", "get-all", "add", "unset", "unset-all", "replace-all") + + getOpts.registerLocation(configGetCmd) + configGetCmd.Flags().BoolVar(&getOpts.all, "all", false, "Show all values for the key") + configGetCmd.Flags().BoolVar(&getOpts.path, "path", false, + "Canonicalize the value as a path, expanding a leading ~") + + setOpts.registerLocation(configSetCmd) + configSetCmd.Flags().BoolVar(&setOpts.all, "all", false, "Replace all values for the key") + + unsetOpts.registerLocation(configUnsetCmd) + configUnsetCmd.Flags().BoolVar(&unsetOpts.all, "all", false, "Remove all values for the key") + + configCmd.AddCommand(configGetCmd, configSetCmd, configUnsetCmd) rootCmd.AddCommand(configCmd) } var configCmd = &cobra.Command{ - Use: "config []", + Use: "config [] []", Short: "Get or set repository configuration", - Args: cobra.RangeArgs(1, 2), + Long: "Get or set configuration values.\n\n" + + "The modern forms are `config get `, `config set ` and\n" + + "`config unset `. The legacy flag spellings (--get, --add, --unset-all\n" + + "and a bare `config []`) are also accepted.", + Args: cobra.RangeArgs(1, 2), + RunE: runConfigLegacy, + DisableFlagsInUseLine: true, + SilenceUsage: true, + SilenceErrors: true, +} + +var configGetCmd = &cobra.Command{ + Use: "get [] ", + Short: "Print the value of a configuration key", + Args: cobra.ExactArgs(1), + RunE: func(_ *cobra.Command, args []string) error { return runConfigGet(&getOpts, args[0]) }, + DisableFlagsInUseLine: true, + SilenceUsage: true, + SilenceErrors: true, +} + +var configSetCmd = &cobra.Command{ + Use: "set [] ", + Short: "Set the value of a configuration key", + Args: cobra.ExactArgs(2), + RunE: func(_ *cobra.Command, args []string) error { + return runConfigWrite(&setOpts, args[0], args[1], writeSet) + }, + DisableFlagsInUseLine: true, + SilenceUsage: true, + SilenceErrors: true, +} + +var configUnsetCmd = &cobra.Command{ + Use: "unset [] ", + Short: "Remove a configuration key", + Args: cobra.ExactArgs(1), RunE: func(_ *cobra.Command, args []string) error { - gitDir, err := findGitDir() - if err != nil { - return err + return runConfigWrite(&unsetOpts, args[0], "", writeUnset) + }, + DisableFlagsInUseLine: true, + SilenceUsage: true, + SilenceErrors: true, +} + +// runConfigLegacy dispatches the pre-subcommand spellings of git config. +func runConfigLegacy(_ *cobra.Command, args []string) error { + switch { + case legacyGet, legacyGetAll: + if len(args) != 1 { + return usageError("--get takes exactly one key") } - cfgPath := filepath.Join(gitDir, "config") + legacyOpts.all = legacyGetAll - raw := formatcfg.New() + return runConfigGet(&legacyOpts, args[0]) - if data, rerr := os.ReadFile(cfgPath); rerr == nil { - if err := formatcfg.NewDecoder(strings.NewReader(string(data))).Decode(raw); err != nil { - return fmt.Errorf("parse config: %w", err) - } + case legacyAdd: + if len(args) != 2 { + return usageError("--add takes a key and a value") } - section, key, err := splitConfigKey(args[0]) - if err != nil { - return err + return runConfigWrite(&legacyOpts, args[0], args[1], writeAdd) + + case legacyReplaceAll: + if len(args) != 2 { + return usageError("--replace-all takes a key and a value") } - if configUnsetAll { - if !raw.Section(section).HasOption(key) { - // git exits 5 when the key is not found; test_unconfig treats 5 as ok. - os.Exit(5) //nolint:gocritic // intentional non-error exit for git compat - } + legacyOpts.all = true - raw.Section(section).RemoveOption(key) + return runConfigWrite(&legacyOpts, args[0], args[1], writeSet) - return writeConfigFile(cfgPath, raw) + case legacyUnset, legacyUnsetAll: + if len(args) != 1 { + return usageError("--unset takes exactly one key") } - if len(args) == 1 { - fmt.Println(raw.Section(section).Option(key)) + legacyOpts.all = legacyUnsetAll - return nil - } + return runConfigWrite(&legacyOpts, args[0], "", writeUnset) - raw.Section(section).SetOption(key, args[1]) + case len(args) == 2: + return runConfigWrite(&legacyOpts, args[0], args[1], writeSet) - return writeConfigFile(cfgPath, raw) - }, - DisableFlagsInUseLine: true, - SilenceUsage: true, - SilenceErrors: true, + default: + return runConfigGet(&legacyOpts, args[0]) + } } -func writeConfigFile(cfgPath string, raw *formatcfg.Config) error { - f, err := os.Create(cfgPath) +func runConfigGet(o *configOpts, rawKey string) error { + key, err := parseConfigKey(rawKey) if err != nil { - return fmt.Errorf("open config for write: %w", err) + return err } - defer f.Close() + sources, err := readSources(o) + if err != nil { + return err + } + + var values []string - return formatcfg.NewEncoder(f).Encode(raw) + for _, src := range sources { + values = append(values, src.values(key)...) + } + + if len(values) == 0 { + // git prints nothing and exits 1 for a key that is not set. This is + // distinct from a key whose value is the empty string, which still + // prints one empty line and exits 0. + return &gitExitError{code: exitNotFound} + } + + if !o.all { + values = values[len(values)-1:] + } + + for _, v := range values { + if o.path { + if v, err = expandPath(v); err != nil { + return err + } + } + + fmt.Println(v) + } + + return nil } -// splitConfigKey splits "section.key" into (section, key, nil). -func splitConfigKey(key string) (string, string, error) { - parts := strings.SplitN(key, ".", 2) - if len(parts) != 2 || parts[0] == "" || parts[1] == "" { - return "", "", fmt.Errorf("invalid config key %q: want
.", key) +type writeMode int + +const ( + writeSet writeMode = iota + writeAdd + writeUnset +) + +func runConfigWrite(o *configOpts, rawKey, value string, mode writeMode) error { + key, err := parseConfigKey(rawKey) + if err != nil { + return err } - return parts[0], parts[1], nil + target, err := writeTarget(o) + if err != nil { + return err + } + + f, err := gitconfig.ReadFile(target.path) + if err != nil { + return configReadError(target, err) + } + + switch mode { + case writeAdd: + err = f.Add(key, value) + + case writeUnset: + if !o.all && len(f.Values(key)) > 1 { + fmt.Fprintf(os.Stderr, "warning: %s has multiple values\n", rawKey) + + return &gitExitError{ + code: exitCannotReplace, + msg: fmt.Sprintf("error: cannot unset multiple values for %s; use --all", rawKey), + } + } + + var n int + + if n, err = f.UnsetAll(key); err == nil && n == 0 { + // git exits 5 without a diagnostic; test_unconfig relies on it. + return &gitExitError{code: exitUnsetMissing} + } + + case writeSet: + if o.all { + err = f.ReplaceAll(key, value) + } else if err = f.Set(key, value); errors.Is(err, gitconfig.ErrMultipleValues) { + fmt.Fprintf(os.Stderr, "warning: %s has multiple values\n", rawKey) + + return &gitExitError{ + code: exitCannotReplace, + msg: fmt.Sprintf("error: cannot overwrite multiple values with a single value\n"+ + " Use --add or --all to change %s.", rawKey), + } + } + } + + if err != nil { + return err + } + + return gitconfig.WriteFile(target.path, f, gitconfig.FileMode(target.path)) } -// findGitDir locates the .git directory starting from the current directory. -func findGitDir() (string, error) { - dir, err := os.Getwd() +// parseConfigKey converts Git's key diagnostics into exit-1 errors. +func parseConfigKey(raw string) (gitconfig.Key, error) { + key, err := gitconfig.ParseKey(raw) if err != nil { - return "", err + return gitconfig.Key{}, &gitExitError{code: exitInvalidKey, msg: "error: " + err.Error()} } - for { - gitDir := filepath.Join(dir, ".git") - if info, err := os.Stat(gitDir); err == nil && info.IsDir() { - return gitDir, nil + return key, nil +} + +func configReadError(cf configFile, err error) error { + var perr *gitconfig.ParseError + if errors.As(err, &perr) { + return &gitExitError{ + code: exitFatal, + msg: fmt.Sprintf("fatal: bad config line %d in file %s", perr.Line, cf.display), } + } + + return err +} - parent := filepath.Dir(dir) - if parent == dir { - break +func usageError(msg string) error { + return &gitExitError{code: exitInvalidKey, msg: "error: " + msg} +} + +// expandPath applies --path canonicalization: a leading ~ becomes the user's +// home directory. Any other value is returned unchanged. +func expandPath(v string) (string, error) { + if v != "~" && !strings.HasPrefix(v, "~/") { + if strings.HasPrefix(v, "~") { + return "", &gitExitError{ + code: exitFatal, + msg: fmt.Sprintf("fatal: failed to expand user dir in: '%s'", v), + } } - dir = parent + return v, nil + } + + home, err := os.UserHomeDir() + if err != nil { + return "", err + } + + if v == "~" { + return home, nil } - return "", errors.New("not a git repository") + return filepath.Join(home, v[2:]), nil } diff --git a/cmd/gogit/config-scope.go b/cmd/gogit/config-scope.go new file mode 100644 index 0000000..13343c3 --- /dev/null +++ b/cmd/gogit/config-scope.go @@ -0,0 +1,262 @@ +package main + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strings" + + gitconfig "github.com/go-git/cli/internal/plumbing/format/config" +) + +// gitExitError carries a git-compatible exit status out of a command. msg, when +// non-empty, is the single stderr line to print; main must not print the +// error itself, because git stays silent for some non-zero statuses (a +// missing key exits 1, an unset of a missing key exits 5, both without +// diagnostics). +type gitExitError struct { + code int + msg string +} + +func (e *gitExitError) Error() string { + if e.msg != "" { + return e.msg + } + + return fmt.Sprintf("exit status %d", e.code) +} + +// configFile names a configuration file twice: the path used for all I/O, and +// the spelling git would use for it in a diagnostic. They differ for a +// repository discovered from the working tree, which git reports relative to +// the top level. +type configFile struct { + path string + display string +} + +// configSource is one configuration file consulted for a read, or the set of +// -c command-line overrides. +type configSource struct { + file *gitconfig.File + overrides []configOverride +} + +func (s configSource) values(key gitconfig.Key) []string { + if s.file != nil { + return s.file.Values(key) + } + + var out []string + + for _, o := range s.overrides { + if o.key == key { + out = append(out, o.value) + } + } + + return out +} + +// readSources returns the files to consult, lowest precedence first. +// +// A location flag selects exactly one source. Otherwise git's default order +// applies: system, then the XDG and per-user global files, then the +// repository, then -c overrides. +func readSources(o *configOpts) ([]configSource, error) { + if file, ok, err := explicitLocation(o); err != nil { + return nil, err + } else if ok { + src, err := loadSource(file) + if err != nil { + return nil, err + } + + return []configSource{src}, nil + } + + var files []configFile + + if p, ok := systemConfigPath(); ok { + files = append(files, absoluteFile(p)) + } + + for _, p := range globalConfigPaths() { + files = append(files, absoluteFile(p)) + } + + // Being outside a repository is not an error for a default read: -c + // overrides and the global files still apply, as they do in git. + if f, err := localConfigFile(); err == nil { + files = append(files, f) + } + + sources := make([]configSource, 0, len(files)+1) + + for _, f := range files { + src, err := loadSource(f) + if err != nil { + return nil, err + } + + sources = append(sources, src) + } + + return append(sources, configSource{overrides: configOverrideList}), nil +} + +// absoluteFile names a file that git reports by its full path, which is how +// it reports every file it did not discover by walking up from the cwd. +func absoluteFile(path string) configFile { + return configFile{path: path, display: path} +} + +// writeTarget returns the single file a mutation applies to. Writes default +// to the repository config, never to the merged view. +func writeTarget(o *configOpts) (configFile, error) { + if file, ok, err := explicitLocation(o); err != nil { + return configFile{}, err + } else if ok { + return file, nil + } + + return localConfigFile() +} + +// explicitLocation resolves --file/--local/--global/--system. For --global it +// picks the file git would write to: the XDG file when it already exists, +// otherwise ~/.gitconfig. +func explicitLocation(o *configOpts) (configFile, bool, error) { + switch { + case o.file != "": + // git reports --file exactly as it was spelled on the command line. + return absoluteFile(o.file), true, nil + + case o.local: + f, err := localConfigFile() + + return f, true, err + + case o.global: + paths := globalConfigPaths() + for _, p := range paths { + if _, err := os.Stat(p); err == nil { + return absoluteFile(p), true, nil + } + } + + if len(paths) == 0 { + return configFile{}, true, errors.New("no global config file available") + } + + return absoluteFile(paths[len(paths)-1]), true, nil + + case o.system: + p, ok := systemConfigPath() + if !ok { + return configFile{}, true, errors.New("system config is disabled by GIT_CONFIG_NOSYSTEM") + } + + return absoluteFile(p), true, nil + } + + return configFile{}, false, nil +} + +func loadSource(cf configFile) (configSource, error) { + f, err := gitconfig.ReadFile(cf.path) + if err != nil { + return configSource{}, configReadError(cf, err) + } + + return configSource{file: f}, nil +} + +// globalConfigPaths returns the per-user config files in ascending precedence +// order, so ~/.gitconfig wins over the XDG file as it does in git. +func globalConfigPaths() []string { + if p, ok := os.LookupEnv("GIT_CONFIG_GLOBAL"); ok { + if p == "" || p == os.DevNull { + return nil + } + + return []string{p} + } + + var paths []string + + if xdg := os.Getenv("XDG_CONFIG_HOME"); xdg != "" { + paths = append(paths, filepath.Join(xdg, "git", "config")) + } else if home, err := os.UserHomeDir(); err == nil { + paths = append(paths, filepath.Join(home, ".config", "git", "config")) + } + + if home, err := os.UserHomeDir(); err == nil { + paths = append(paths, filepath.Join(home, ".gitconfig")) + } + + return paths +} + +// systemConfigPath reports the system config file, and whether the system +// scope is enabled at all. +func systemConfigPath() (string, bool) { + if v := os.Getenv("GIT_CONFIG_NOSYSTEM"); v != "" && v != "0" { + return "", false + } + + if p, ok := os.LookupEnv("GIT_CONFIG_SYSTEM"); ok { + if p == "" || p == os.DevNull { + return "", false + } + + return p, true + } + + return "/etc/gitconfig", true +} + +// localConfigFile returns the repository's config file. For a linked worktree +// this is the common directory's config, not the worktree's own git dir. +func localConfigFile() (configFile, error) { + gitDir, display, err := discoverGitDir() + if err != nil { + return configFile{}, err + } + + common := commonGitDir(gitDir) + if common != gitDir { + // Resolving through commondir yields an absolute path, and that is + // what git reports for a linked worktree. + display = common + } + + return configFile{ + path: filepath.Join(common, "config"), + // Concatenated rather than joined: git names a bare repository's + // config "./config", which filepath.Join would clean to "config". + display: display + "/config", + }, nil +} + +// commonGitDir follows a linked worktree's `commondir` pointer back to the +// main git directory, which is where shared state such as config lives. +func commonGitDir(gitDir string) string { + data, err := os.ReadFile(filepath.Join(gitDir, "commondir")) + if err != nil { + return gitDir + } + + common := strings.TrimSpace(string(data)) + if common == "" { + return gitDir + } + + if !filepath.IsAbs(common) { + common = filepath.Join(gitDir, common) + } + + return filepath.Clean(common) +} diff --git a/cmd/gogit/config.go b/cmd/gogit/config.go index 5eeb8a3..15194f6 100644 --- a/cmd/gogit/config.go +++ b/cmd/gogit/config.go @@ -5,6 +5,7 @@ import ( "strings" "sync" + gitconfig "github.com/go-git/cli/internal/plumbing/format/config" "github.com/go-git/go-git/v6/config" ) @@ -12,8 +13,20 @@ var ( configOverridesRaw []string configOverrides = map[string]string{} configOverrideMu sync.Mutex + + // configOverrideList keeps the -c overrides in the order they were given + // and with their keys normalised, which the config command needs to + // report repeated values and to match subsection spellings. The map above + // stays keyed by the raw string for the existing callers. + configOverrideList []configOverride ) +// configOverride is a single -c key=value pair with its key parsed. +type configOverride struct { + key gitconfig.Key + value string +} + // splitKV splits "=" into (key, value, true). Invalid input // (no '=' or empty key) returns ("", "", false). Empty value is allowed. func splitKV(s string) (string, string, bool) { @@ -38,6 +51,7 @@ func resetConfigOverrides() { configOverrides = map[string]string{} configOverridesRaw = nil + configOverrideList = nil } // applyConfigOverridesFromFlags parses raw `-c k=v` values previously captured @@ -49,6 +63,13 @@ func applyConfigOverridesFromFlags() error { return fmt.Errorf("invalid -c value %q (want key=value)", raw) } + key, kerr := gitconfig.ParseKey(k) + if kerr != nil { + return fmt.Errorf("invalid -c value %q: %w", raw, kerr) + } + + configOverrideList = append(configOverrideList, configOverride{key: key, value: v}) + applyConfigOverride(k, v) } diff --git a/cmd/gogit/config_cmd_test.go b/cmd/gogit/config_cmd_test.go new file mode 100644 index 0000000..6bdf381 --- /dev/null +++ b/cmd/gogit/config_cmd_test.go @@ -0,0 +1,843 @@ +package main + +import ( + "errors" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// configEnv isolates a test from the developer's real configuration: HOME +// points at a scratch directory and the system config is switched off, the +// same way upstream's test-lib.sh does it. +func configEnv(home string) []string { + return []string{"HOME=" + home, "GIT_CONFIG_NOSYSTEM=1", "XDG_CONFIG_HOME="} +} + +// runConfig runs gogit in dir and returns stdout, stderr and the exit status. +func runConfig(t *testing.T, dir, home string, args ...string) (string, string, int) { + t.Helper() + + stdout, stderr, err := runGogitEnv(t, dir, configEnv(home), args...) + + code := 0 + + if err != nil { + var ee *exec.ExitError + if !errors.As(err, &ee) { + t.Fatalf("gogit %v: %v", args, err) + } + + code = ee.ExitCode() + } + + return stdout, stderr, code +} + +// newConfigRepo creates a repository with the given extra config content and +// returns the work tree and the isolated HOME. +func newConfigRepo(t *testing.T, extra string) (string, string) { + t.Helper() + + base := t.TempDir() + repo := filepath.Join(base, "repo") + home := filepath.Join(base, "home") + + mkdirAll(t, filepath.Join(repo, ".git")) + mkdirAll(t, home) + + // A config file plus HEAD/objects/refs is enough for the config command, + // and avoids depending on another gogit subcommand to set the test up. + mkdirAll(t, filepath.Join(repo, ".git", "objects")) + mkdirAll(t, filepath.Join(repo, ".git", "refs")) + + if err := os.WriteFile(filepath.Join(repo, ".git", "HEAD"), []byte("ref: refs/heads/main\n"), 0o644); err != nil { + t.Fatal(err) + } + + writeConfig(t, filepath.Join(repo, ".git", "config"), extra) + + return repo, home +} + +func mkdirAll(t *testing.T, path string) { + t.Helper() + + if err := os.MkdirAll(path, 0o755); err != nil { + t.Fatal(err) + } +} + +func writeConfig(t *testing.T, path, content string) { + t.Helper() + + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } +} + +func readFileString(t *testing.T, path string) string { + t.Helper() + + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + + return string(data) +} + +// Argument and key literals shared by the config tables. +const ( + cmdConfig = "config" + subGet = "get" + subSet = "set" + subUnset = "unset" + + flagGet = "--get" + flagAdd = "--add" + flagAll = "--all" + flagUnsetAll = "--unset-all" + flagPath = "--path" + flagFile = "--file" + flagLocal = "--local" + flagGlobal = "--global" + + keyUserName = "user.name" + keyFooBar = "foo.bar" + keyMissing = "no.such" + keyOriginURL = "remote.origin.url" + keyPr = "pr.k" + + valAuthor = "A U Thor\n" + valThree = "three" + keyPathDir = "p.dir" + valNewName = "New Name" + overridePr = "pr.k=CMD" +) + +const baseConfig = `[core] + repositoryformatversion = 0 +[user] + name = A U Thor +[remote "origin"] + url = https://example.com/x.git +[remote "team.one"] + url = https://t.example/o.git +[foo] + bar = one + bar = two +[e] + empty = +` + +func TestConfigGet(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + args []string + want string + wantCode int + }{ + {name: "implicit get", args: []string{cmdConfig, keyUserName}, want: valAuthor}, + {name: "legacy --get", args: []string{cmdConfig, flagGet, keyUserName}, want: valAuthor}, + {name: "modern get", args: []string{cmdConfig, subGet, keyUserName}, want: valAuthor}, + { + name: "subsection", args: []string{cmdConfig, subGet, keyOriginURL}, + want: "https://example.com/x.git\n", + }, + { + name: "subsection containing dots", args: []string{cmdConfig, subGet, "remote.team.one.url"}, + want: "https://t.example/o.git\n", + }, + {name: "key is case-insensitive", args: []string{cmdConfig, subGet, "USER.NAME"}, want: valAuthor}, + {name: "multivalue reports the last", args: []string{cmdConfig, subGet, keyFooBar}, want: "two\n"}, + {name: "get --all", args: []string{cmdConfig, subGet, flagAll, keyFooBar}, want: "one\ntwo\n"}, + {name: "legacy --get-all", args: []string{cmdConfig, "--get-all", keyFooBar}, want: "one\ntwo\n"}, + + // An explicitly empty value is not the same as a missing one. + {name: "empty value prints a blank line", args: []string{cmdConfig, subGet, "e.empty"}, want: "\n"}, + {name: "missing key is silent", args: []string{cmdConfig, subGet, keyMissing}, want: "", wantCode: 1}, + {name: "missing key legacy form", args: []string{cmdConfig, keyMissing}, want: "", wantCode: 1}, + { + name: "missing subsection", args: []string{cmdConfig, subGet, "remote.other.url"}, + want: "", wantCode: 1, + }, + + {name: "key without a section", args: []string{cmdConfig, subGet, "user"}, wantCode: 1}, + {name: "key without a variable", args: []string{cmdConfig, subGet, "user."}, wantCode: 1}, + {name: "invalid variable name", args: []string{cmdConfig, subGet, "a.1b"}, wantCode: 1}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + repo, home := newConfigRepo(t, baseConfig) + + stdout, _, code := runConfig(t, repo, home, tc.args...) + if code != tc.wantCode { + t.Fatalf("gogit %v: exit %d, want %d (stdout %q)", tc.args, code, tc.wantCode, stdout) + } + + if stdout != tc.want { + t.Fatalf("gogit %v: stdout %q, want %q", tc.args, stdout, tc.want) + } + }) + } +} + +func TestConfigWrite(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + args []string + wantCode int + // wantConfig is the expected config file, or "" to skip the check. + wantConfig string + // unchanged asserts the file was not touched at all. + unchanged bool + }{ + { + name: "implicit set replaces in place", + args: []string{cmdConfig, keyUserName, valNewName}, + wantConfig: strings.Replace(baseConfig, + "name = A U Thor", "name = New Name", 1), + }, + { + name: "modern set replaces in place", + args: []string{cmdConfig, subSet, keyUserName, valNewName}, + wantConfig: strings.Replace(baseConfig, + "name = A U Thor", "name = New Name", 1), + }, + { + name: "set writes into the right subsection", + args: []string{cmdConfig, subSet, keyOriginURL, "https://new/z.git"}, + wantConfig: strings.Replace(baseConfig, + "url = https://example.com/x.git", "url = https://new/z.git", 1), + }, + { + name: "set on a dotted subsection", + args: []string{cmdConfig, subSet, "remote.team.one.url", "https://new/o.git"}, + wantConfig: strings.Replace(baseConfig, + "url = https://t.example/o.git", "url = https://new/o.git", 1), + }, + { + name: "a new section is appended", + args: []string{cmdConfig, subSet, "new.key", "v"}, + wantConfig: baseConfig + "[new]\n\tkey = v\n", + }, + { + name: "a new subsection is appended", + args: []string{cmdConfig, subSet, "remote.other.url", "https://o/p.git"}, + wantConfig: baseConfig + + "[remote \"other\"]\n\turl = https://o/p.git\n", + }, + { + name: "--add appends without replacing", + args: []string{cmdConfig, flagAdd, keyFooBar, valThree}, + wantConfig: strings.Replace(baseConfig, + "\tbar = two\n", "\tbar = two\n\tbar = three\n", 1), + }, + { + name: "set refuses to collapse multiple values", + args: []string{cmdConfig, subSet, keyFooBar, valThree}, + wantCode: 5, + unchanged: true, + }, + { + name: "implicit set refuses to collapse multiple values", + args: []string{cmdConfig, keyFooBar, valThree}, + wantCode: 5, + unchanged: true, + }, + { + name: "set --all collapses them deliberately", + args: []string{cmdConfig, subSet, flagAll, keyFooBar, valThree}, + wantConfig: strings.Replace(baseConfig, + "\tbar = one\n\tbar = two\n", "\tbar = three\n", 1), + }, + { + name: "--replace-all collapses them deliberately", + args: []string{cmdConfig, "--replace-all", keyFooBar, valThree}, + wantConfig: strings.Replace(baseConfig, + "\tbar = one\n\tbar = two\n", "\tbar = three\n", 1), + }, + { + name: "unset removes one line", + args: []string{cmdConfig, subUnset, keyUserName}, + wantConfig: strings.Replace(baseConfig, + "[user]\n\tname = A U Thor\n", "", 1), + }, + { + name: "legacy --unset-all removes every occurrence", + args: []string{cmdConfig, flagUnsetAll, keyFooBar}, + wantConfig: strings.Replace(baseConfig, + "[foo]\n\tbar = one\n\tbar = two\n", "", 1), + }, + { + name: "unset on a subsection", + args: []string{cmdConfig, subUnset, keyOriginURL}, + wantConfig: strings.Replace(baseConfig, + "[remote \"origin\"]\n\turl = https://example.com/x.git\n", "", 1), + }, + { + name: "unset of a missing key exits 5", + args: []string{cmdConfig, subUnset, keyMissing}, + wantCode: 5, + unchanged: true, + }, + { + name: "legacy --unset-all of a missing key exits 5", + args: []string{cmdConfig, flagUnsetAll, keyMissing}, + wantCode: 5, + unchanged: true, + }, + { + name: "unset refuses a multivalued key without --all", + args: []string{cmdConfig, subUnset, keyFooBar}, + wantCode: 5, + unchanged: true, + }, + { + name: "an invalid key never reaches the file", + args: []string{cmdConfig, subSet, "a_x.b", "v"}, + wantCode: 1, + unchanged: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + repo, home := newConfigRepo(t, baseConfig) + path := filepath.Join(repo, ".git", cmdConfig) + + _, stderr, code := runConfig(t, repo, home, tc.args...) + if code != tc.wantCode { + t.Fatalf("gogit %v: exit %d, want %d (stderr %q)", tc.args, code, tc.wantCode, stderr) + } + + got := readFileString(t, path) + + switch { + case tc.unchanged: + if got != baseConfig { + t.Fatalf("gogit %v modified the config:\n%s", tc.args, got) + } + case tc.wantConfig != "": + if got != tc.wantConfig { + t.Fatalf("gogit %v:\n--- got ---\n%s\n--- want ---\n%s", tc.args, got, tc.wantConfig) + } + } + }) + } +} + +// TestConfigSetPreservesCommentsAndLayout is the guarantee that made a +// format-preserving writer necessary: a canonical re-encode would delete +// every comment and blank line in the file. +func TestConfigSetPreservesCommentsAndLayout(t *testing.T) { + t.Parallel() + + const src = `# a comment worth keeping +[user] + ; and an inline note + name = Old Name + email = a@b.c + +[remote "origin"] + url = https://x/y.git +` + + repo, home := newConfigRepo(t, src) + path := filepath.Join(repo, ".git", cmdConfig) + + if _, stderr, code := runConfig(t, repo, home, cmdConfig, subSet, keyUserName, valNewName); code != 0 { + t.Fatalf("set failed: exit %d, stderr %q", code, stderr) + } + + want := strings.Replace(src, "name = Old Name", "name = New Name", 1) + if got := readFileString(t, path); got != want { + t.Fatalf("--- got ---\n%s\n--- want ---\n%s", got, want) + } +} + +// TestConfigParseFailureNeverRewritesFile proves a read or parse failure +// cannot truncate or canonicalise the config. +func TestConfigParseFailureNeverRewritesFile(t *testing.T) { + t.Parallel() + + const src = `# keep me +[user] + name = Old Name + +bogus line here +` + + repo, home := newConfigRepo(t, src) + path := filepath.Join(repo, ".git", cmdConfig) + + for _, args := range [][]string{ + {cmdConfig, subGet, keyUserName}, + {cmdConfig, subSet, keyUserName, valNewName}, + {cmdConfig, subUnset, keyUserName}, + {cmdConfig, flagAdd, keyUserName, "Another"}, + } { + _, stderr, code := runConfig(t, repo, home, args...) + if code != 128 { + t.Errorf("gogit %v: exit %d, want 128 (stderr %q)", args, code, stderr) + } + + // git names the repository config relative to the top level. + if want := "fatal: bad config line 5 in file .git/config\n"; stderr != want { + t.Errorf("gogit %v: stderr %q, want %q", args, stderr, want) + } + + if got := readFileString(t, path); got != src { + t.Fatalf("gogit %v rewrote a malformed config:\n%s", args, got) + } + } +} + +func TestConfigScopePrecedence(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + args []string + want string + wantCode int + }{ + {name: "local wins over global", args: []string{cmdConfig, subGet, keyPr}, want: "LOCAL\n"}, + { + name: "-c wins over local", + args: []string{"-c", overridePr, cmdConfig, subGet, keyPr}, want: "CMD\n", + }, + { + name: "--local ignores global and -c", + args: []string{"-c", overridePr, cmdConfig, subGet, flagLocal, keyPr}, want: "LOCAL\n", + }, + {name: "--global selects the global file", args: []string{cmdConfig, flagGlobal, keyPr}, want: "GLOBAL\n"}, + {name: "global-only key is visible by default", args: []string{cmdConfig, subGet, "g.only"}, want: "FROMGLOBAL\n"}, + { + name: "--local does not see a global-only key", + args: []string{cmdConfig, subGet, flagLocal, "g.only"}, wantCode: 1, + }, + { + name: "-c with a subsection", + args: []string{"-c", "remote.origin.url=CMD", cmdConfig, subGet, keyOriginURL}, want: "CMD\n", + }, + { + name: "-c contributes to --all in precedence order", + args: []string{"-c", overridePr, cmdConfig, subGet, flagAll, keyPr}, want: "GLOBAL\nLOCAL\nCMD\n", + }, + { + name: "-c can set an empty value", + args: []string{"-c", "pr.k=", cmdConfig, subGet, keyPr}, want: "\n", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + repo, home := newConfigRepo(t, "[pr]\n\tk = LOCAL\n") + writeConfig(t, filepath.Join(home, ".gitconfig"), "[pr]\n\tk = GLOBAL\n[g]\n\tonly = FROMGLOBAL\n") + + stdout, stderr, code := runConfig(t, repo, home, tc.args...) + if code != tc.wantCode { + t.Fatalf("gogit %v: exit %d, want %d (stderr %q)", tc.args, code, tc.wantCode, stderr) + } + + if stdout != tc.want { + t.Fatalf("gogit %v: stdout %q, want %q", tc.args, stdout, tc.want) + } + }) + } +} + +func TestConfigWritesDefaultToLocalScope(t *testing.T) { + t.Parallel() + + repo, home := newConfigRepo(t, "[pr]\n\tk = LOCAL\n") + global := filepath.Join(home, ".gitconfig") + writeConfig(t, global, "[pr]\n\tk = GLOBAL\n") + + if _, stderr, code := runConfig(t, repo, home, cmdConfig, subSet, keyPr, "CHANGED"); code != 0 { + t.Fatalf("set failed: exit %d, stderr %q", code, stderr) + } + + if got, want := readFileString(t, filepath.Join(repo, ".git", cmdConfig)), "[pr]\n\tk = CHANGED\n"; got != want { + t.Fatalf("local config = %q, want %q", got, want) + } + + if got, want := readFileString(t, global), "[pr]\n\tk = GLOBAL\n"; got != want { + t.Fatalf("a default write touched the global config: %q", got) + } +} + +func TestConfigGlobalWrite(t *testing.T) { + t.Parallel() + + repo, home := newConfigRepo(t, "[pr]\n\tk = LOCAL\n") + global := filepath.Join(home, ".gitconfig") + writeConfig(t, global, "[pr]\n\tk = GLOBAL\n") + + if _, stderr, code := runConfig(t, repo, home, cmdConfig, subSet, flagGlobal, keyPr, "CHANGED"); code != 0 { + t.Fatalf("set --global failed: exit %d, stderr %q", code, stderr) + } + + if got, want := readFileString(t, global), "[pr]\n\tk = CHANGED\n"; got != want { + t.Fatalf("global config = %q, want %q", got, want) + } + + if got, want := readFileString(t, filepath.Join(repo, ".git", cmdConfig)), "[pr]\n\tk = LOCAL\n"; got != want { + t.Fatalf("--global write touched the local config: %q", got) + } +} + +func TestConfigFile(t *testing.T) { + t.Parallel() + + repo, home := newConfigRepo(t, baseConfig) + external := filepath.Join(t.TempDir(), "external.cfg") + writeConfig(t, external, "[a]\n\tb = c\n") + + stdout, stderr, code := runConfig(t, repo, home, cmdConfig, subGet, flagFile, external, "a.b") + if code != 0 || stdout != "c\n" { + t.Fatalf("--file read: exit %d, stdout %q, stderr %q", code, stdout, stderr) + } + + // --file must not fall back to the repository. + if _, _, code := runConfig(t, repo, home, cmdConfig, subGet, flagFile, external, keyUserName); code != 1 { + t.Fatalf("--file leaked repository values: exit %d", code) + } + + if _, stderr, code := runConfig(t, repo, home, cmdConfig, subSet, flagFile, external, "a.b", "d"); code != 0 { + t.Fatalf("--file write: exit %d, stderr %q", code, stderr) + } + + if got, want := readFileString(t, external), "[a]\n\tb = d\n"; got != want { + t.Fatalf("external file = %q, want %q", got, want) + } + + if got := readFileString(t, filepath.Join(repo, ".git", cmdConfig)); got != baseConfig { + t.Fatalf("--file write touched the repository config:\n%s", got) + } + + // A --file write outside any repository still works. + fresh := filepath.Join(t.TempDir(), "fresh.cfg") + if _, stderr, code := runConfig(t, t.TempDir(), home, cmdConfig, subSet, flagFile, fresh, "x.y", "z"); code != 0 { + t.Fatalf("--file write outside a repo: exit %d, stderr %q", code, stderr) + } + + if got, want := readFileString(t, fresh), "[x]\n\ty = z\n"; got != want { + t.Fatalf("new file = %q, want %q", got, want) + } +} + +func TestConfigPath(t *testing.T) { + t.Parallel() + + repo, home := newConfigRepo(t, "[p]\n\tdir = ~/sub\n\tabs = /etc\n\trel = x/y\n\tuser = ~someone/z\n") + + tests := []struct { + name string + args []string + want string + wantCode int + }{ + { + name: "tilde expands", + args: []string{cmdConfig, subGet, flagPath, keyPathDir}, + want: filepath.Join(home, "sub") + "\n", + }, + {name: "legacy --path", args: []string{cmdConfig, flagPath, keyPathDir}, want: filepath.Join(home, "sub") + "\n"}, + {name: "without --path the value is literal", args: []string{cmdConfig, subGet, keyPathDir}, want: "~/sub\n"}, + {name: "absolute path is unchanged", args: []string{cmdConfig, subGet, flagPath, "p.abs"}, want: "/etc\n"}, + {name: "relative path is unchanged", args: []string{cmdConfig, subGet, flagPath, "p.rel"}, want: "x/y\n"}, + {name: "~user is rejected", args: []string{cmdConfig, subGet, flagPath, "p.user"}, wantCode: 128}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + stdout, stderr, code := runConfig(t, repo, home, tc.args...) + if code != tc.wantCode { + t.Fatalf("gogit %v: exit %d, want %d (stderr %q)", tc.args, code, tc.wantCode, stderr) + } + + if stdout != tc.want { + t.Fatalf("gogit %v: stdout %q, want %q", tc.args, stdout, tc.want) + } + }) + } +} + +func TestConfigInvalidCombinations(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + args []string + }{ + {name: "--get with a value", args: []string{cmdConfig, flagGet, keyUserName, "extra"}}, + {name: "--add without a value", args: []string{cmdConfig, flagAdd, keyUserName}}, + {name: "--unset-all with a value", args: []string{cmdConfig, flagUnsetAll, keyUserName, "extra"}}, + {name: "--get and --unset-all together", args: []string{cmdConfig, flagGet, flagUnsetAll, keyUserName}}, + {name: "--local and --global together", args: []string{cmdConfig, subGet, flagLocal, flagGlobal, keyUserName}}, + {name: "--file and --global together", args: []string{cmdConfig, subGet, flagFile, "x", flagGlobal, keyUserName}}, + {name: "get without a key", args: []string{cmdConfig, subGet}}, + {name: "set without a value", args: []string{cmdConfig, subSet, keyUserName}}, + {name: "no arguments at all", args: []string{cmdConfig}}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + repo, home := newConfigRepo(t, baseConfig) + + _, _, code := runConfig(t, repo, home, tc.args...) + if code == 0 { + t.Fatalf("gogit %v unexpectedly succeeded", tc.args) + } + + if got := readFileString(t, filepath.Join(repo, ".git", cmdConfig)); got != baseConfig { + t.Fatalf("gogit %v modified the config:\n%s", tc.args, got) + } + }) + } +} + +func TestConfigLinkedWorktree(t *testing.T) { + t.Parallel() + + base := t.TempDir() + home := filepath.Join(base, "home") + main := filepath.Join(base, "main") + wt := filepath.Join(base, "wt") + wtGitDir := filepath.Join(main, ".git", "worktrees", "wt") + + mkdirAll(t, home) + mkdirAll(t, wt) + mkdirAll(t, wtGitDir) + mkdirAll(t, filepath.Join(main, ".git", "objects")) + mkdirAll(t, filepath.Join(main, ".git", "refs")) + + writeConfig(t, filepath.Join(main, ".git", "HEAD"), "ref: refs/heads/main\n") + writeConfig(t, filepath.Join(main, ".git", cmdConfig), "[user]\n\tname = MAIN\n") + writeConfig(t, filepath.Join(wt, ".git"), "gitdir: "+wtGitDir+"\n") + writeConfig(t, filepath.Join(wtGitDir, "HEAD"), "ref: refs/heads/other\n") + writeConfig(t, filepath.Join(wtGitDir, "commondir"), "../..\n") + + stdout, stderr, code := runConfig(t, wt, home, cmdConfig, subGet, keyUserName) + if code != 0 || stdout != "MAIN\n" { + t.Fatalf("worktree read: exit %d, stdout %q, stderr %q", code, stdout, stderr) + } + + // A write from a linked worktree belongs in the common directory. + if _, stderr, code := runConfig(t, wt, home, cmdConfig, subSet, keyUserName, "CHANGED"); code != 0 { + t.Fatalf("worktree write: exit %d, stderr %q", code, stderr) + } + + if got, want := readFileString(t, filepath.Join(main, ".git", cmdConfig)), "[user]\n\tname = CHANGED\n"; got != want { + t.Fatalf("common config = %q, want %q", got, want) + } + + if _, err := os.Stat(filepath.Join(wtGitDir, cmdConfig)); !os.IsNotExist(err) { + t.Fatal("a worktree write created a per-worktree config file") + } +} + +func TestConfigBareRepository(t *testing.T) { + t.Parallel() + + base := t.TempDir() + home := filepath.Join(base, "home") + bare := filepath.Join(base, "bare.git") + + mkdirAll(t, home) + mkdirAll(t, filepath.Join(bare, "objects")) + mkdirAll(t, filepath.Join(bare, "refs")) + + writeConfig(t, filepath.Join(bare, "HEAD"), "ref: refs/heads/main\n") + writeConfig(t, filepath.Join(bare, cmdConfig), "[user]\n\tname = BARE\n") + + stdout, stderr, code := runConfig(t, bare, home, cmdConfig, subGet, keyUserName) + if code != 0 || stdout != "BARE\n" { + t.Fatalf("bare read: exit %d, stdout %q, stderr %q", code, stdout, stderr) + } + + if _, stderr, code := runConfig(t, bare, home, cmdConfig, subSet, keyUserName, "CHANGED"); code != 0 { + t.Fatalf("bare write: exit %d, stderr %q", code, stderr) + } + + if got, want := readFileString(t, filepath.Join(bare, cmdConfig)), "[user]\n\tname = CHANGED\n"; got != want { + t.Fatalf("bare config = %q, want %q", got, want) + } +} + +func TestConfigOutsideRepository(t *testing.T) { + t.Parallel() + + base := t.TempDir() + home := filepath.Join(base, "home") + + mkdirAll(t, home) + + dir := filepath.Join(base, "plain") + mkdirAll(t, dir) + + // -c overrides and global values still apply with no repository present. + stdout, stderr, code := runConfig(t, dir, home, "-c", "only.cmd=C", cmdConfig, subGet, "only.cmd") + if code != 0 || stdout != "C\n" { + t.Fatalf("-c outside a repo: exit %d, stdout %q, stderr %q", code, stdout, stderr) + } + + if _, _, code := runConfig(t, dir, home, cmdConfig, subGet, keyUserName); code != 1 { + t.Fatalf("missing key outside a repo: exit %d, want 1", code) + } + + // A local write outside a repository must fail rather than invent a file. + if _, _, code := runConfig(t, dir, home, cmdConfig, subSet, keyUserName, "X"); code == 0 { + t.Fatal("a local write outside a repository unexpectedly succeeded") + } +} + +// TestConfigDiagnosticPaths pins how the config file is named in a +// diagnostic. git reports the path exactly as it resolved it rather than +// absolutising it, so the spelling depends on how the repository was found. +func TestConfigDiagnosticPaths(t *testing.T) { + t.Parallel() + + const malformed = "[user]\n\tname = x\nbogus line\n" + + t.Run("discovered from the working tree", func(t *testing.T) { + t.Parallel() + + repo, home := newConfigRepo(t, malformed) + + _, stderr, code := runConfig(t, repo, home, cmdConfig, subGet, keyUserName) + assertDiagnostic(t, stderr, code, "fatal: bad config line 3 in file .git/config\n") + }) + + t.Run("discovered from a subdirectory", func(t *testing.T) { + t.Parallel() + + repo, home := newConfigRepo(t, malformed) + sub := filepath.Join(repo, "deep", "nested") + mkdirAll(t, sub) + + // git chdirs to the top level before reporting, so the path stays + // ".git/config" however deep the caller is. + _, stderr, code := runConfig(t, sub, home, cmdConfig, subGet, keyUserName) + assertDiagnostic(t, stderr, code, "fatal: bad config line 3 in file .git/config\n") + }) + + t.Run("bare repository", func(t *testing.T) { + t.Parallel() + + base := t.TempDir() + home := filepath.Join(base, "home") + bare := filepath.Join(base, "bare.git") + + mkdirAll(t, home) + mkdirAll(t, filepath.Join(bare, "objects")) + mkdirAll(t, filepath.Join(bare, "refs")) + writeConfig(t, filepath.Join(bare, "HEAD"), "ref: refs/heads/main\n") + writeConfig(t, filepath.Join(bare, "config"), malformed) + + // A bare repository is its own git dir, which git names ".". + _, stderr, code := runConfig(t, bare, home, cmdConfig, subGet, keyUserName) + assertDiagnostic(t, stderr, code, "fatal: bad config line 3 in file ./config\n") + }) + + t.Run("linked worktree reports the common dir", func(t *testing.T) { + t.Parallel() + + base := t.TempDir() + home := filepath.Join(base, "home") + main := filepath.Join(base, "main") + wt := filepath.Join(base, "wt") + wtGitDir := filepath.Join(main, ".git", "worktrees", "wt") + + mkdirAll(t, home) + mkdirAll(t, wt) + mkdirAll(t, wtGitDir) + mkdirAll(t, filepath.Join(main, ".git", "objects")) + mkdirAll(t, filepath.Join(main, ".git", "refs")) + writeConfig(t, filepath.Join(main, ".git", "HEAD"), "ref: refs/heads/main\n") + writeConfig(t, filepath.Join(main, ".git", "config"), malformed) + writeConfig(t, filepath.Join(wt, ".git"), "gitdir: "+wtGitDir+"\n") + writeConfig(t, filepath.Join(wtGitDir, "commondir"), "../..\n") + + // Resolving through commondir yields an absolute path, and that is + // what git reports. + want := "fatal: bad config line 3 in file " + filepath.Join(main, ".git", "config") + "\n" + + _, stderr, code := runConfig(t, wt, home, cmdConfig, subGet, keyUserName) + assertDiagnostic(t, stderr, code, want) + }) + + t.Run("GIT_DIR is reported as given", func(t *testing.T) { + t.Parallel() + + repo, home := newConfigRepo(t, malformed) + + stdout, stderr, err := runGogitEnv(t, repo, + append(configEnv(home), "GIT_DIR=.git"), cmdConfig, subGet, keyUserName) + if stdout != "" { + t.Fatalf("stdout = %q, want empty", stdout) + } + + if err == nil { + t.Fatal("expected a non-zero exit") + } + + if want := "fatal: bad config line 3 in file .git/config\n"; stderr != want { + t.Fatalf("stderr = %q, want %q", stderr, want) + } + }) + + t.Run("--file is reported as given", func(t *testing.T) { + t.Parallel() + + repo, home := newConfigRepo(t, baseConfig) + external := filepath.Join(t.TempDir(), "external.cfg") + writeConfig(t, external, malformed) + + _, stderr, code := runConfig(t, repo, home, cmdConfig, subGet, flagFile, external, keyUserName) + assertDiagnostic(t, stderr, code, "fatal: bad config line 3 in file "+external+"\n") + }) + + t.Run("a malformed global file is reported by full path", func(t *testing.T) { + t.Parallel() + + repo, home := newConfigRepo(t, "[a]\n\tb = c\n") + global := filepath.Join(home, ".gitconfig") + writeConfig(t, global, malformed) + + _, stderr, code := runConfig(t, repo, home, cmdConfig, subGet, keyUserName) + assertDiagnostic(t, stderr, code, "fatal: bad config line 3 in file "+global+"\n") + }) +} + +func assertDiagnostic(t *testing.T, stderr string, code int, want string) { + t.Helper() + + if code != 128 { + t.Fatalf("exit %d, want 128 (stderr %q)", code, stderr) + } + + if stderr != want { + t.Fatalf("stderr = %q, want %q", stderr, want) + } +} diff --git a/cmd/gogit/count-objects.go b/cmd/gogit/count-objects.go index 8fb5571..1f7a1be 100644 --- a/cmd/gogit/count-objects.go +++ b/cmd/gogit/count-objects.go @@ -52,15 +52,15 @@ func repoGitDir(r *git.Repository) string { wd, err := os.Getwd() if err != nil { - return ".git" + return gitDirName } - candidate := filepath.Join(wd, ".git") + candidate := filepath.Join(wd, gitDirName) if _, err := os.Stat(candidate); err == nil { return candidate } - return ".git" + return gitDirName } // walkLooseObjects sums loose object count and bytes under /objects. diff --git a/cmd/gogit/gitdir.go b/cmd/gogit/gitdir.go new file mode 100644 index 0000000..85f0d35 --- /dev/null +++ b/cmd/gogit/gitdir.go @@ -0,0 +1,110 @@ +package main + +import ( + "errors" + "os" + "path/filepath" + "strings" +) + +// gitDirName is the name of a repository's git directory inside a work tree. +const gitDirName = ".git" + +// findGitDir locates the repository's git directory. +func findGitDir() (string, error) { + path, _, err := discoverGitDir() + + return path, err +} + +// discoverGitDir locates the repository's git directory and returns both the +// path to use for I/O and the spelling git would use to name it in a +// diagnostic. +// +// It handles the three shapes git supports: a .git directory in the working +// tree or an ancestor, a .git *file* pointing at a linked worktree's git dir, +// and a bare repository whose working directory is the git dir itself. +// +// The two results differ because git chdirs to the top level of the working +// tree before doing anything, which leaves its GIT_DIR relative; gogit does +// not chdir, so it needs the absolute path to open the file and the relative +// one to report it. +func discoverGitDir() (string, string, error) { + if d := os.Getenv("GIT_DIR"); d != "" { + // An explicit GIT_DIR is reported exactly as it was given. + return d, d, nil + } + + dir, err := os.Getwd() + if err != nil { + return "", "", err + } + + for { + gitPath := filepath.Join(dir, gitDirName) + + info, err := os.Stat(gitPath) + switch { + case err == nil && info.IsDir(): + return gitPath, gitDirName, nil + case err == nil && info.Mode().IsRegular(): + // A linked worktree resolves to an absolute path, and git reports + // it that way. + resolved, err := readGitFile(gitPath) + + return resolved, resolved, err + } + + if isGitDir(dir) { + // A bare repository is its own git dir, which git names ".". + return dir, ".", nil + } + + parent := filepath.Dir(dir) + if parent == dir { + return "", "", errors.New("not a git repository") + } + + dir = parent + } +} + +// readGitFile resolves a ".git" file, whose contents are "gitdir: ". +func readGitFile(path string) (string, error) { + data, err := os.ReadFile(path) + if err != nil { + return "", err + } + + target, ok := strings.CutPrefix(strings.TrimSpace(string(data)), "gitdir:") + if !ok { + return "", errors.New("not a git repository") + } + + target = strings.TrimSpace(target) + if target == "" { + return "", errors.New("not a git repository") + } + + if !filepath.IsAbs(target) { + target = filepath.Join(filepath.Dir(path), target) + } + + return filepath.Clean(target), nil +} + +// isGitDir reports whether dir is itself a git directory, which is how a bare +// repository presents itself. +func isGitDir(dir string) bool { + if _, err := os.Stat(filepath.Join(dir, "HEAD")); err != nil { + return false + } + + return isDir(filepath.Join(dir, "objects")) && isDir(filepath.Join(dir, "refs")) +} + +func isDir(path string) bool { + info, err := os.Stat(path) + + return err == nil && info.IsDir() +} diff --git a/cmd/gogit/main.go b/cmd/gogit/main.go index 7dfc1b2..7c0e3d4 100644 --- a/cmd/gogit/main.go +++ b/cmd/gogit/main.go @@ -82,6 +82,18 @@ func main() { err := rootCmd.Execute() if err != nil { + // Some commands need git's own exit statuses, and git stays + // silent for several of them, so gitExitError carries both the + // code and whether anything is printed. + var gerr *gitExitError + if errors.As(err, &gerr) { + if gerr.msg != "" { + fmt.Fprintln(os.Stderr, gerr.msg) + } + + os.Exit(gerr.code) + } + var rerr *transport.RemoteError if errors.As(err, &rerr) { fmt.Fprintln(os.Stderr, rerr) diff --git a/cmd/gogit/main_test.go b/cmd/gogit/main_test.go index 9d8f9b2..0290367 100644 --- a/cmd/gogit/main_test.go +++ b/cmd/gogit/main_test.go @@ -50,7 +50,7 @@ func runGogit(t *testing.T, dir string, args ...string) (string, string, error) return stdout.String(), stderr.String(), err } -func runGogitEnv(t *testing.T, dir string, env []string, args ...string) (string, string, error) { //nolint:unparam +func runGogitEnv(t *testing.T, dir string, env []string, args ...string) (string, string, error) { t.Helper() cmd := exec.Command(gogitBin, args...) diff --git a/internal/plumbing/format/config/file.go b/internal/plumbing/format/config/file.go new file mode 100644 index 0000000..749c106 --- /dev/null +++ b/internal/plumbing/format/config/file.go @@ -0,0 +1,391 @@ +package config + +import ( + "errors" + "fmt" + "slices" + "strings" +) + +// ParseError reports a line Git would reject with +// "fatal: bad config line N in file ". +type ParseError struct{ Line int } + +func (e *ParseError) Error() string { + return fmt.Sprintf("bad config line %d", e.Line) +} + +// ErrMultipleValues is returned by Set when the key already has more than one +// value, mirroring git's refusal to collapse them into one. +var ErrMultipleValues = errors.New("cannot overwrite multiple values with a single value") + +// File is a parsed configuration file that retains its original bytes, so +// mutations can splice individual variables without disturbing comments, +// blank lines, indentation or the ordering of anything else. +type File struct { + data []byte + sections []*sectionRec + options []*optionRec +} + +// sectionRec is one occurrence of a section header in the file. The same +// section may be opened more than once. +type sectionRec struct { + key Key // only Section, Subsection and HasSubsection are meaningful + + // entryEnd is where a new variable belonging to this header occurrence + // should be inserted: just past the header line initially, then just + // past the last variable line parsed under it. + entryEnd int + + lineStart int // start of the header's physical line + headerEnd int // just past the header's closing ']' + lineEnd int // just past the header line's terminator + + // plain reports a header line carrying nothing but the header itself. + // git keeps a header that has a trailing comment even once its last + // variable is removed. + plain bool + + // regionEnd is where the next header begins, or end of file. + regionEnd int +} + +// optionRec is one occurrence of a variable in the file, with the byte ranges +// needed to rewrite or delete it in place. +type optionRec struct { + key Key + value string + + // valueless marks a bare "name" with no '=', which Git treats as a + // boolean true but renders as an empty string without --type=bool. + valueless bool + + secIdx int // index into File.sections of the governing header + lineStart int // start of the first physical line this variable occupies + lineEnd int // just past the terminator of its last physical line + nameEnd int // just past the variable name + + // valueStart..logicalEnd is what Set replaces. It runs to the end of the + // last physical line, so a trailing comment is dropped on rewrite, as + // git does. + valueStart int + logicalEnd int + + // alone reports that no section header shares the variable's first line, + // so Unset can delete whole lines rather than a byte range. + alone bool +} + +// Parse reads a configuration file. It returns a *ParseError for any +// construct Git itself would reject, so a malformed file is never partially +// understood and then rewritten. +func Parse(data []byte) (*File, error) { + p := &parser{data: data, line: 1, f: &File{data: data}} + if err := p.run(); err != nil { + return nil, err + } + + return p.f, nil +} + +// Bytes returns the current file contents. +func (f *File) Bytes() []byte { + return f.data +} + +// Values returns every value recorded for key, in file order. A nil result +// means the key is absent, which callers must distinguish from a key whose +// single value is the empty string. +func (f *File) Values(key Key) []string { + var out []string + + for _, o := range f.options { + if o.key.matches(key) { + out = append(out, o.value) + } + } + + return out +} + +// Get returns the last value for key, which is the one Git reports for a +// plain lookup, and whether the key is present at all. +func (f *File) Get(key Key) (string, bool) { + vals := f.Values(key) + if len(vals) == 0 { + return "", false + } + + return vals[len(vals)-1], true +} + +// Set replaces the value of key. It refuses a key with several values, as git +// does, returning ErrMultipleValues; use Add or ReplaceAll instead. +func (f *File) Set(key Key, value string) error { + var found []*optionRec + + for _, o := range f.options { + if o.key.matches(key) { + found = append(found, o) + } + } + + if len(found) > 1 { + return ErrMultipleValues + } + + if len(found) == 0 { + return f.insert(key, value) + } + + return f.rewrite(found[0], value) +} + +// ReplaceAll collapses every value of key into a single value. +func (f *File) ReplaceAll(key Key, value string) error { + var found []*optionRec + + for _, o := range f.options { + if o.key.matches(key) { + found = append(found, o) + } + } + + switch len(found) { + case 0: + return f.insert(key, value) + case 1: + return f.rewrite(found[0], value) + } + + // Keep the first occurrence in place and drop the rest, so the value + // stays where the file already had it. + edits := []edit{{start: found[0].valueStart, end: found[0].logicalEnd, text: encodeValue(value)}} + for _, o := range found[1:] { + edits = append(edits, deleteEdit(o)) + } + + return f.apply(edits) +} + +// Add appends a new value for key without touching existing ones. +func (f *File) Add(key Key, value string) error { + return f.insert(key, value) +} + +// UnsetAll removes every occurrence of key and reports how many were removed. +func (f *File) UnsetAll(key Key) (int, error) { + var ( + edits []edit + n int + ) + + for _, o := range f.options { + if o.key.matches(key) { + edits = append(edits, deleteEdit(o)) + n++ + } + } + + if n == 0 { + return 0, nil + } + + return n, f.apply(append(edits, f.emptySectionEdits(edits)...)) +} + +// emptySectionEdits returns the header lines that become empty once edits are +// applied. git drops a section header whose last variable is removed, but only +// when nothing else — not even a comment — remains under it, and only when the +// header line itself carries nothing but the header. +func (f *File) emptySectionEdits(deletions []edit) []edit { + touched := map[int]bool{} + + for _, o := range f.options { + for _, d := range deletions { + if o.lineStart >= d.start && o.lineEnd <= d.end { + touched[o.secIdx] = true + } + } + } + + var out []edit + + for idx := range touched { + s := f.sections[idx] + if !s.plain { + continue + } + + if remainderIsHeaderOnly(f.data, s, deletions) { + out = append(out, edit{start: s.lineStart, end: s.lineEnd}) + } + } + + return out +} + +// remainderIsHeaderOnly reports whether the section's region would contain +// nothing but its own header line once deletions are applied. +func remainderIsHeaderOnly(data []byte, s *sectionRec, deletions []edit) bool { + for i := s.lineEnd; i < s.regionEnd && i < len(data); i++ { + deleted := false + + for _, d := range deletions { + if i >= d.start && i < d.end { + deleted = true + + break + } + } + + if !deleted && !isSpace(data[i]) { + return false + } + } + + return true +} + +func isSpace(c byte) bool { + return c == ' ' || c == '\t' || c == '\r' || c == '\n' +} + +func (f *File) rewrite(o *optionRec, value string) error { + if !o.alone { + // The variable shares its line with its section header. git moves it + // onto a line of its own rather than rewriting in place. + sec := f.sections[o.secIdx] + text := "\n\t" + f.rawName(o) + " = " + encodeValue(value) + + return f.apply([]edit{{start: sec.headerEnd, end: o.logicalEnd, text: text}}) + } + + text := encodeValue(value) + if o.valueless { + // A bare "name" gains its separator along with the value. + text = " = " + text + } + + return f.apply([]edit{{start: o.valueStart, end: o.logicalEnd, text: text}}) +} + +// rawName returns the variable name as spelled in the file, so rewriting a +// value never silently changes the name's capitalisation. +func (f *File) rawName(o *optionRec) string { + return string(f.data[o.nameEnd-len(o.key.Name) : o.nameEnd]) +} + +// insert places a new variable after the last variable of the section it +// belongs to, appending a new section at end of file when there is none. +func (f *File) insert(key Key, value string) error { + line := "\t" + key.Name + " = " + encodeValue(value) + "\n" + + for _, s := range slices.Backward(f.sections) { + if s.key.Section == key.Section && + s.key.HasSubsection == key.HasSubsection && + s.key.Subsection == key.Subsection { + text := line + if s.entryEnd > 0 && f.data[s.entryEnd-1] != '\n' { + // The section is the last line and lacks a terminator. + text = "\n" + text + } + + return f.apply([]edit{{start: s.entryEnd, end: s.entryEnd, text: text}}) + } + } + + var b strings.Builder + + if len(f.data) > 0 && f.data[len(f.data)-1] != '\n' { + b.WriteByte('\n') + } + + b.WriteString(encodeHeader(key)) + b.WriteString(line) + + return f.apply([]edit{{start: len(f.data), end: len(f.data), text: b.String()}}) +} + +type edit struct { + start, end int + text string +} + +func deleteEdit(o *optionRec) edit { + if o.alone { + return edit{start: o.lineStart, end: o.lineEnd} + } + + // The variable shares its line with a section header; drop just the + // variable text and leave the header standing. + return edit{start: o.nameEnd - len(o.key.Name), end: o.logicalEnd} +} + +// apply splices edits into the document and re-parses, so recorded offsets +// always describe the current bytes. +func (f *File) apply(edits []edit) error { + // Descending order keeps earlier offsets valid as later ones are spliced. + slices.SortFunc(edits, func(a, b edit) int { return b.start - a.start }) + + data := f.data + for _, e := range edits { + out := make([]byte, 0, len(data)-(e.end-e.start)+len(e.text)) + out = append(out, data[:e.start]...) + out = append(out, e.text...) + out = append(out, data[e.end:]...) + data = out + } + + next, err := Parse(data) + if err != nil { + return err + } + + *f = *next + + return nil +} + +func encodeHeader(key Key) string { + if !key.HasSubsection { + return "[" + key.Section + "]\n" + } + + r := strings.NewReplacer(`\`, `\\`, `"`, `\"`) + + return "[" + key.Section + ` "` + r.Replace(key.Subsection) + "\"]\n" +} + +// encodeValue renders a value the way git does: control characters and quotes +// are always escaped, and the result is wrapped in quotes only when leading or +// trailing spaces or a comment character would otherwise change its meaning. +func encodeValue(v string) string { + var b strings.Builder + + for i := range len(v) { + switch v[i] { + case '\n': + b.WriteString(`\n`) + case '\t': + b.WriteString(`\t`) + case '\b': + b.WriteString(`\b`) + case '\\': + b.WriteString(`\\`) + case '"': + b.WriteString(`\"`) + default: + b.WriteByte(v[i]) + } + } + + out := b.String() + if strings.HasPrefix(v, " ") || strings.HasSuffix(v, " ") || + strings.ContainsAny(v, "#;") { + return `"` + out + `"` + } + + return out +} diff --git a/internal/plumbing/format/config/file_test.go b/internal/plumbing/format/config/file_test.go new file mode 100644 index 0000000..9c5ffee --- /dev/null +++ b/internal/plumbing/format/config/file_test.go @@ -0,0 +1,410 @@ +package config_test + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" + + config "github.com/go-git/cli/internal/plumbing/format/config" +) + +func mustKey(t *testing.T, s string) config.Key { + t.Helper() + + k, err := config.ParseKey(s) + if err != nil { + t.Fatalf("ParseKey(%q): %v", s, err) + } + + return k +} + +func mustParse(t *testing.T, src string) *config.File { + t.Helper() + + f, err := config.Parse([]byte(src)) + if err != nil { + t.Fatalf("Parse(%q): %v", src, err) + } + + return f +} + +func TestValues(t *testing.T) { + t.Parallel() + + const src = `# leading comment +[user] + ; why + name = A U Thor + email = a@b.c +[remote "origin"] + url = https://example.com/x.git +[remote "team.one"] + url = https://t.example/o.git +[user ""] + name = EMPTYSUB +[foo] + bar = one + bar = two +[e] + empty = +[b] + flag +[crlf]` + "\r\n\tk = v\r\n" + `[same] inline = yes +[a.SUB] + dotted = legacy +[q] + quoted = "x y" + esc = "tab\there" + trail = value # comment +` + + tests := []struct { + name string + key string + want []string + }{ + {name: "simple", key: keyUserName, want: []string{"A U Thor"}}, + {name: "case-insensitive key", key: "USER.NAME", want: []string{"A U Thor"}}, + {name: "subsection", key: keyOriginURL, want: []string{"https://example.com/x.git"}}, + {name: "subsection with dots", key: keyDottedSub, want: []string{"https://t.example/o.git"}}, + {name: "empty subsection", key: keyEmptySub, want: []string{"EMPTYSUB"}}, + {name: "multivalue in file order", key: "foo.bar", want: []string{"one", "two"}}, + {name: "explicitly empty value", key: "e.empty", want: []string{""}}, + {name: "valueless variable", key: "b.flag", want: []string{""}}, + {name: "crlf line endings", key: "crlf.k", want: []string{"v"}}, + {name: "option on the header line", key: "same.inline", want: []string{"yes"}}, + {name: "legacy dotted section", key: "a.sub.dotted", want: []string{"legacy"}}, + {name: "legacy dotted section is case-folded", key: "a.SUB.dotted", want: nil}, + {name: "quoted value keeps inner spaces", key: "q.quoted", want: []string{"x y"}}, + {name: "escape sequences decoded", key: "q.esc", want: []string{"tab\there"}}, + {name: "trailing comment excluded", key: "q.trail", want: []string{"value"}}, + {name: "absent key", key: "no.such", want: nil}, + {name: "absent subsection", key: "remote.other.url", want: nil}, + } + + f := mustParse(t, src) + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + got := f.Values(mustKey(t, tc.key)) + if len(got) != len(tc.want) { + t.Fatalf("Values(%s) = %q, want %q", tc.key, got, tc.want) + } + + for i := range got { + if got[i] != tc.want[i] { + t.Fatalf("Values(%s) = %q, want %q", tc.key, got, tc.want) + } + } + }) + } +} + +// TestGetDistinguishesMissingFromEmpty pins the difference the command layer +// turns into "exit 1 with no output" versus "exit 0 with one blank line". +func TestGetDistinguishesMissingFromEmpty(t *testing.T) { + t.Parallel() + + f := mustParse(t, "[e]\n\tempty =\n") + + if v, ok := f.Get(mustKey(t, "e.empty")); !ok || v != "" { + t.Fatalf("Get(e.empty) = (%q, %v), want (\"\", true)", v, ok) + } + + if v, ok := f.Get(mustKey(t, "e.missing")); ok { + t.Fatalf("Get(e.missing) = (%q, %v), want (\"\", false)", v, ok) + } +} + +func TestParseRejectsMalformed(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + src string + wantLine int + }{ + {name: "bare words", src: "[a]\nbogus line here\n", wantLine: 2}, + {name: "variable before any section", src: "b = c\n", wantLine: 1}, + {name: "unterminated section", src: "[a\n", wantLine: 1}, + {name: "unterminated subsection quote", src: "[a \"sub\n", wantLine: 1}, + {name: "unterminated value quote", src: "[a]\n\tb = \"oops\n", wantLine: 2}, + {name: "invalid section character", src: "[a_b]\n\tc = d\n", wantLine: 1}, + {name: "variable starting with a digit", src: "[a]\n\t1b = c\n", wantLine: 2}, + {name: "junk after a value", src: "[a]\n\tb = c\n\td e f\n", wantLine: 3}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + _, err := config.Parse([]byte(tc.src)) + if err == nil { + t.Fatalf("Parse(%q) succeeded, want a parse error", tc.src) + } + + var perr *config.ParseError + if !errors.As(err, &perr) { + t.Fatalf("Parse(%q) error = %v (%T), want *ParseError", tc.src, err, err) + } + + if perr.Line != tc.wantLine { + t.Fatalf("Parse(%q) reported line %d, want %d", tc.src, perr.Line, tc.wantLine) + } + }) + } +} + +func TestMutationsPreserveFormatting(t *testing.T) { + t.Parallel() + + const src = `# a comment worth keeping +[user] + ; and an inline note + name = Old Name + email = a@b.c + +[remote "origin"] + url = https://x/y.git +[foo] + bar = one + bar = two +` + + tests := []struct { + name string + do func(*testing.T, *config.File) + want string + }{ + { + name: "set rewrites only the value", + do: func(t *testing.T, f *config.File) { + t.Helper() + + if err := f.Set(mustKey(t, keyUserName), "New Name"); err != nil { + t.Fatal(err) + } + }, + want: strings.Replace(src, "name = Old Name", "name = New Name", 1), + }, + { + name: "set on a subsection targets the subsection", + do: func(t *testing.T, f *config.File) { + t.Helper() + + if err := f.Set(mustKey(t, keyOriginURL), "https://new/z.git"); err != nil { + t.Fatal(err) + } + }, + want: strings.Replace(src, "url = https://x/y.git", "url = https://new/z.git", 1), + }, + { + name: "add appends after the section's last variable", + do: func(t *testing.T, f *config.File) { + t.Helper() + + if err := f.Add(mustKey(t, keyUserName), "Second"); err != nil { + t.Fatal(err) + } + }, + want: strings.Replace(src, "\temail = a@b.c\n", "\temail = a@b.c\n\tname = Second\n", 1), + }, + { + name: "a new section is appended at end of file", + do: func(t *testing.T, f *config.File) { + t.Helper() + + if err := f.Set(mustKey(t, "new.key"), "v"); err != nil { + t.Fatal(err) + } + }, + want: src + "[new]\n\tkey = v\n", + }, + { + name: "unset removes the line and nothing else", + do: func(t *testing.T, f *config.File) { + t.Helper() + + if _, err := f.UnsetAll(mustKey(t, keyUserName)); err != nil { + t.Fatal(err) + } + }, + want: strings.Replace(src, "\tname = Old Name\n", "", 1), + }, + { + name: "unset --all removes every occurrence and the empty section", + do: func(t *testing.T, f *config.File) { + t.Helper() + + if _, err := f.UnsetAll(mustKey(t, "foo.bar")); err != nil { + t.Fatal(err) + } + }, + want: strings.Replace(src, "[foo]\n\tbar = one\n\tbar = two\n", "", 1), + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + f := mustParse(t, src) + tc.do(t, f) + + if got := string(f.Bytes()); got != tc.want { + t.Fatalf("result mismatch\n--- got ---\n%s\n--- want ---\n%s", got, tc.want) + } + }) + } +} + +func TestSetRefusesMultipleValues(t *testing.T) { + t.Parallel() + + f := mustParse(t, "[foo]\n\tbar = one\n\tbar = two\n") + + err := f.Set(mustKey(t, "foo.bar"), "three") + if !errors.Is(err, config.ErrMultipleValues) { + t.Fatalf("Set on a multivalued key = %v, want ErrMultipleValues", err) + } + + if got := string(f.Bytes()); got != "[foo]\n\tbar = one\n\tbar = two\n" { + t.Fatalf("refused Set modified the file:\n%s", got) + } + + if err := f.ReplaceAll(mustKey(t, "foo.bar"), "three"); err != nil { + t.Fatalf("ReplaceAll: %v", err) + } + + if got, want := string(f.Bytes()), "[foo]\n\tbar = three\n"; got != want { + t.Fatalf("ReplaceAll = %q, want %q", got, want) + } +} + +func TestUnsetAllReportsMissingKey(t *testing.T) { + t.Parallel() + + f := mustParse(t, "[a]\n\tb = c\n") + + n, err := f.UnsetAll(mustKey(t, "a.missing")) + if err != nil || n != 0 { + t.Fatalf("UnsetAll(a.missing) = (%d, %v), want (0, nil)", n, err) + } + + if got := string(f.Bytes()); got != "[a]\n\tb = c\n" { + t.Fatalf("no-op UnsetAll modified the file: %q", got) + } +} + +// TestSetQuotesValuesLikeGit exercises value encoding through the public API: +// escapes are always applied, and quotes are added only when leading or +// trailing spaces or a comment character would otherwise change the meaning. +func TestSetQuotesValuesLikeGit(t *testing.T) { + t.Parallel() + + tests := []struct{ name, in, want string }{ + {name: valPlain, in: valPlain, want: valPlain}, + {name: "empty", in: "", want: ""}, + {name: "inner space", in: "a b", want: "a b"}, + {name: "leading space", in: " lead", want: `" lead"`}, + {name: "trailing space", in: "trail ", want: `"trail "`}, + {name: "hash", in: "has # hash", want: `"has # hash"`}, + {name: "semicolon", in: "has ; semi", want: `"has ; semi"`}, + {name: "quote", in: `has "quote"`, want: `has \"quote\"`}, + {name: "backslash", in: `has \back`, want: `has \\back`}, + {name: "tab", in: "has\ttab", want: `has\ttab`}, + {name: "newline", in: "two\nlines", want: `two\nlines`}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + f := mustParse(t, "[a]\n\tb = old\n") + if err := f.Set(mustKey(t, "a.b"), tc.in); err != nil { + t.Fatal(err) + } + + want := "[a]\n\tb = " + tc.want + "\n" + if got := string(f.Bytes()); got != want { + t.Fatalf("Set(%q) wrote %q, want %q", tc.in, got, want) + } + + // The encoding must survive a round trip unchanged. + again := mustParse(t, string(f.Bytes())) + if got, _ := again.Get(mustKey(t, "a.b")); got != tc.in { + t.Fatalf("round trip of %q produced %q", tc.in, got) + } + }) + } +} + +// TestReadFileErrorsAreNotEmptyConfigs guards against a read failure being +// mistaken for an absent file, which would let a later write truncate a +// config that is merely unreadable right now. +func TestReadFileErrorsAreNotEmptyConfigs(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + + if f, err := config.ReadFile(filepath.Join(dir, "does-not-exist")); err != nil { + t.Fatalf("missing file should parse as empty, got %v", err) + } else if len(f.Bytes()) != 0 { + t.Fatalf("missing file parsed to %q, want empty", f.Bytes()) + } + + // A directory stands in for any read error that is not "not exist". + if _, err := config.ReadFile(dir); err == nil { + t.Fatal("reading a directory should fail, not yield an empty config") + } +} + +func TestWriteFileIsAtomicAndKeepsMode(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + path := filepath.Join(dir, "config") + + if err := os.WriteFile(path, []byte("[a]\n\tb = c\n"), 0o640); err != nil { + t.Fatal(err) + } + + f := mustParse(t, "[a]\n\tb = d\n") + + if err := config.WriteFile(path, f, config.FileMode(path)); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + st, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + + if st.Mode().Perm() != 0o640 { + t.Fatalf("mode = %v, want 0640", st.Mode().Perm()) + } + + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + + if string(got) != "[a]\n\tb = d\n" { + t.Fatalf("contents = %q", got) + } + + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + + if len(entries) != 1 { + t.Fatalf("WriteFile left temp files behind: %v", entries) + } +} diff --git a/internal/plumbing/format/config/key.go b/internal/plumbing/format/config/key.go new file mode 100644 index 0000000..13e185d --- /dev/null +++ b/internal/plumbing/format/config/key.go @@ -0,0 +1,144 @@ +// Package config parses and edits Git configuration files while preserving +// their original formatting. +// +// The go-git raw config decoder/encoder pair round-trips through a canonical +// representation: it drops every comment and cannot represent an empty +// subsection name ("[user \"\"]" is folded into "[user]"). Both are lossy in +// ways that silently corrupt a user's file, so config file mutation here is +// done by splicing bytes in the original document instead. +package config + +import "strings" + +// Key is a parsed configuration key such as "remote.origin.url". +type Key struct { + // Section is the section name, lower-cased. Section names are + // case-insensitive in Git. + Section string + // Subsection is the subsection name, preserved verbatim. Subsection + // names are case-sensitive in Git. + Subsection string + // HasSubsection distinguishes "user..name", which addresses the empty + // subsection [user ""], from "user.name", which addresses [user]. + HasSubsection bool + // Name is the variable name, lower-cased. Variable names are + // case-insensitive in Git. + Name string +} + +// KeyNoSectionError reports a key with no '.' separator, matching git's +// "key does not contain a section" diagnostic. +type KeyNoSectionError struct{ Key string } + +func (e *KeyNoSectionError) Error() string { + return "key does not contain a section: " + e.Key +} + +// KeyNoVariableError reports a key that ends at the section separator, matching +// git's "key does not contain variable name" diagnostic. +type KeyNoVariableError struct{ Key string } + +func (e *KeyNoVariableError) Error() string { + return "key does not contain variable name: " + e.Key +} + +// KeyInvalidError reports a key whose section or variable name uses characters +// Git does not accept. +type KeyInvalidError struct{ Key string } + +func (e *KeyInvalidError) Error() string { + return "invalid key: " + e.Key +} + +// ParseKey splits a fully qualified configuration key. +// +// Git splits at the first and the last '.': everything between them is the +// subsection, which may itself contain dots ("remote.team.one.url" addresses +// [remote "team.one"] url). A key with exactly two components has no +// subsection; "a..b" has an empty one. +func ParseKey(key string) (Key, error) { + first := strings.IndexByte(key, '.') + if first < 0 { + return Key{}, &KeyNoSectionError{Key: key} + } + + last := strings.LastIndexByte(key, '.') + if last == len(key)-1 { + return Key{}, &KeyNoVariableError{Key: key} + } + + k := Key{ + Section: strings.ToLower(key[:first]), + Name: strings.ToLower(key[last+1:]), + } + + if first != last { + k.Subsection = key[first+1 : last] + k.HasSubsection = true + } + + if !validSectionName(k.Section) || !validVariableName(k.Name) { + return Key{}, &KeyInvalidError{Key: key} + } + + return k, nil +} + +// String renders the key in the form ParseKey accepts. +func (k Key) String() string { + if k.HasSubsection { + return k.Section + "." + k.Subsection + "." + k.Name + } + + return k.Section + "." + k.Name +} + +// matches reports whether k addresses the same variable as other. Section and +// variable names compare case-insensitively (both are stored lower-cased); +// subsection names compare byte-for-byte. +func (k Key) matches(other Key) bool { + return k.Section == other.Section && + k.Name == other.Name && + k.HasSubsection == other.HasSubsection && + k.Subsection == other.Subsection +} + +// validSectionName accepts alphanumerics and '-'. Unlike variable names, a +// section name may start with a digit. +func validSectionName(s string) bool { + if s == "" { + return false + } + + for i := range len(s) { + if !isAlnum(s[i]) && s[i] != '-' { + return false + } + } + + return true +} + +// validVariableName accepts alphanumerics and '-', and requires an alphabetic +// first character. +func validVariableName(s string) bool { + if s == "" || !isAlpha(s[0]) { + return false + } + + for i := range len(s) { + if !isAlnum(s[i]) && s[i] != '-' { + return false + } + } + + return true +} + +func isAlpha(c byte) bool { + return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') +} + +func isAlnum(c byte) bool { + return isAlpha(c) || (c >= '0' && c <= '9') +} diff --git a/internal/plumbing/format/config/key_test.go b/internal/plumbing/format/config/key_test.go new file mode 100644 index 0000000..8ec4b7e --- /dev/null +++ b/internal/plumbing/format/config/key_test.go @@ -0,0 +1,151 @@ +package config_test + +import ( + "errors" + "testing" + + config "github.com/go-git/cli/internal/plumbing/format/config" +) + +// Literals shared by the tests in this package. +const ( + secUser = "user" + secRemote = "remote" + varName = "name" + varURL = "url" + + keyUserName = "user.name" + keyEmptySub = "user..name" + keyOriginURL = "remote.origin.url" + keyDottedSub = "remote.team.one.url" + + valPlain = "plain" +) + +func TestParseKey(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + in string + want config.Key + // wantErr matches the expected error type; nil means success. + wantErr func(error) bool + }{ + { + name: "two components", + in: keyUserName, + want: config.Key{Section: secUser, Name: varName}, + }, + { + name: "subsection", + in: keyOriginURL, + want: config.Key{Section: secRemote, Subsection: "origin", HasSubsection: true, Name: varURL}, + }, + { + name: "subsection containing dots splits at first and last dot", + in: keyDottedSub, + want: config.Key{Section: secRemote, Subsection: "team.one", HasSubsection: true, Name: varURL}, + }, + { + name: "empty subsection is distinct from no subsection", + in: keyEmptySub, + want: config.Key{Section: secUser, Subsection: "", HasSubsection: true, Name: varName}, + }, + { + name: "section and variable are lower-cased", + in: "USER.NAME", + want: config.Key{Section: secUser, Name: varName}, + }, + { + name: "subsection keeps its case", + in: "remote.Origin.url", + want: config.Key{Section: secRemote, Subsection: "Origin", HasSubsection: true, Name: varURL}, + }, + { + name: "section may start with a digit", + in: "0section.name", + want: config.Key{Section: "0section", Name: varName}, + }, + { + name: "hyphens are allowed", + in: "a-b.c-d", + want: config.Key{Section: "a-b", Name: "c-d"}, + }, + {name: "no separator", in: secUser, wantErr: isNoSection}, + {name: "empty", in: "", wantErr: isNoSection}, + {name: "trailing separator", in: "user.", wantErr: isNoVariable}, + {name: "variable starting with a digit", in: "a.1b", wantErr: isInvalidKey}, + {name: "variable starting with a hyphen", in: "a.-b", wantErr: isInvalidKey}, + {name: "underscore in section", in: "a_x.b", wantErr: isInvalidKey}, + {name: "underscore in variable", in: "a.b_y", wantErr: isInvalidKey}, + {name: "space in variable", in: "a.b c", wantErr: isInvalidKey}, + {name: "space in section", in: "a b.c", wantErr: isInvalidKey}, + {name: "leading separator", in: ".b", wantErr: isInvalidKey}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + got, err := config.ParseKey(tc.in) + + if tc.wantErr != nil { + if err == nil { + t.Fatalf("ParseKey(%q) = %+v, want an error", tc.in, got) + } + + if !tc.wantErr(err) { + t.Fatalf("ParseKey(%q) returned the wrong error type: %v (%T)", tc.in, err, err) + } + + return + } + + if err != nil { + t.Fatalf("ParseKey(%q) failed: %v", tc.in, err) + } + + if got != tc.want { + t.Fatalf("ParseKey(%q) = %+v, want %+v", tc.in, got, tc.want) + } + }) + } +} + +func isNoSection(err error) bool { + var target *config.KeyNoSectionError + + return errors.As(err, &target) +} + +func isNoVariable(err error) bool { + var target *config.KeyNoVariableError + + return errors.As(err, &target) +} + +func isInvalidKey(err error) bool { + var target *config.KeyInvalidError + + return errors.As(err, &target) +} + +func TestKeyString(t *testing.T) { + t.Parallel() + + for _, in := range []string{keyUserName, keyOriginURL, keyDottedSub, keyEmptySub} { + t.Run(in, func(t *testing.T) { + t.Parallel() + + k, err := config.ParseKey(in) + if err != nil { + t.Fatalf("ParseKey(%q): %v", in, err) + } + + if got := k.String(); got != in { + t.Fatalf("Key.String() = %q, want %q", got, in) + } + }) + } +} diff --git a/internal/plumbing/format/config/parser.go b/internal/plumbing/format/config/parser.go new file mode 100644 index 0000000..cef4542 --- /dev/null +++ b/internal/plumbing/format/config/parser.go @@ -0,0 +1,377 @@ +package config + +import "strings" + +// parser walks a configuration file byte by byte, recording the position of +// every section header and variable so they can later be spliced in place. +type parser struct { + data []byte + pos int + line int + + f *File + curSec *sectionRec +} + +func (p *parser) run() error { + defer func() { + if n := len(p.f.sections); n > 0 { + p.f.sections[n-1].regionEnd = len(p.data) + } + }() + + for p.pos < len(p.data) { + lineStart := p.pos + p.skipBlank() + + if p.atLineEnd() { + p.consumeLineEnd() + + continue + } + + if p.data[p.pos] == '#' || p.data[p.pos] == ';' { + p.skipToLineEnd() + p.consumeLineEnd() + + continue + } + + sawHeader := false + + if p.data[p.pos] == '[' { + if err := p.parseHeader(lineStart); err != nil { + return err + } + + sawHeader = true + + p.skipBlank() + + if p.atLineEnd() || p.data[p.pos] == '#' || p.data[p.pos] == ';' { + p.curSec.plain = p.atLineEnd() + p.skipToLineEnd() + p.consumeLineEnd() + p.curSec.lineEnd = p.pos + p.curSec.entryEnd = p.pos + + continue + } + // Git allows a variable to follow the header on the same line. + } + + if err := p.parseVariable(lineStart, !sawHeader); err != nil { + return err + } + } + + return nil +} + +func (p *parser) parseHeader(lineStart int) error { + p.pos++ // consume '[' + + start := p.pos + for p.pos < len(p.data) && (isAlnum(p.data[p.pos]) || p.data[p.pos] == '-' || p.data[p.pos] == '.') { + p.pos++ + } + + name := string(p.data[start:p.pos]) + if name == "" { + return p.err() + } + + var key Key + + p.skipBlank() + + switch { + case p.pos < len(p.data) && p.data[p.pos] == '"': + sub, err := p.parseSubsection() + if err != nil { + return err + } + + if !validSectionName(name) { + return p.err() + } + + key = Key{Section: strings.ToLower(name), Subsection: sub, HasSubsection: true} + case strings.Contains(name, "."): + // Deprecated "[section.subsection]" form. Git lower-cases the whole + // header, so the subsection is case-insensitive here only. + sec, sub, _ := strings.Cut(name, ".") + if !validSectionName(sec) { + return p.err() + } + + key = Key{ + Section: strings.ToLower(sec), + Subsection: strings.ToLower(sub), + HasSubsection: true, + } + default: + if !validSectionName(name) { + return p.err() + } + + key = Key{Section: strings.ToLower(name)} + } + + p.skipBlank() + + if p.pos >= len(p.data) || p.data[p.pos] != ']' { + return p.err() + } + + p.pos++ + + sec := §ionRec{key: key, lineStart: lineStart, headerEnd: p.pos} + if n := len(p.f.sections); n > 0 { + // A section occurrence owns the file up to the next header. + p.f.sections[n-1].regionEnd = lineStart + } + + p.f.sections = append(p.f.sections, sec) + p.curSec = sec + + return nil +} + +func (p *parser) parseSubsection() (string, error) { + p.pos++ // consume '"' + + var b strings.Builder + + for { + if p.pos >= len(p.data) || p.data[p.pos] == '\n' { + return "", p.err() + } + + c := p.data[p.pos] + if c == '"' { + p.pos++ + + return b.String(), nil + } + + if c == '\\' { + p.pos++ + + if p.pos >= len(p.data) || p.data[p.pos] == '\n' { + return "", p.err() + } + + b.WriteByte(p.data[p.pos]) + p.pos++ + + continue + } + + b.WriteByte(c) + + p.pos++ + } +} + +func (p *parser) parseVariable(lineStart int, alone bool) error { + if p.curSec == nil { + // A variable before any section header has no key Git could name. + return p.err() + } + + nameStart := p.pos + for p.pos < len(p.data) && (isAlnum(p.data[p.pos]) || p.data[p.pos] == '-') { + p.pos++ + } + + name := strings.ToLower(string(p.data[nameStart:p.pos])) + if !validVariableName(name) { + return p.err() + } + + o := &optionRec{ + key: Key{ + Section: p.curSec.key.Section, + Subsection: p.curSec.key.Subsection, + HasSubsection: p.curSec.key.HasSubsection, + Name: name, + }, + lineStart: lineStart, + nameEnd: p.pos, + alone: alone, + } + + p.skipBlank() + + if p.pos < len(p.data) && p.data[p.pos] == '=' { + p.pos++ + p.skipBlank() + + o.valueStart = p.pos + + value, err := p.parseValue() + if err != nil { + return err + } + + o.value = value + } else { + o.valueless = true + o.valueStart = o.nameEnd + + // Only a comment may follow a bare variable name. Anything else is + // a malformed line, which git rejects rather than guessing at. + if !p.atLineEnd() && p.data[p.pos] != '#' && p.data[p.pos] != ';' { + return p.err() + } + } + + p.skipToLineEnd() + + o.logicalEnd = p.pos + if p.pos < len(p.data) && p.data[p.pos] == '\r' { + // git replaces the CR along with the value, so a rewritten line ends + // up with a bare LF even in a CRLF file. + o.logicalEnd++ + } + + p.consumeLineEnd() + o.lineEnd = p.pos + + o.secIdx = len(p.f.sections) - 1 + p.f.options = append(p.f.options, o) + p.curSec.entryEnd = p.pos + + return nil +} + +// parseValue reads a variable's value, honouring quoted runs, backslash +// escapes and backslash-newline continuations, and trimming unquoted trailing +// whitespace. +func (p *parser) parseValue() (string, error) { + var ( + b strings.Builder + inQuote bool + lastKeep int + ) + + for p.pos < len(p.data) { + c := p.data[p.pos] + + switch { + case c == '\n' || (c == '\r' && p.peekIsLF()): + if inQuote { + return "", p.err() + } + + return b.String()[:lastKeep], nil + + case c == '"': + inQuote = !inQuote + p.pos++ + + case c == '\\': + p.pos++ + + if p.pos >= len(p.data) { + return "", p.err() + } + + e := p.data[p.pos] + if e == '\r' && p.peekIsLF() { + p.pos++ + e = '\n' + } + + if e == '\n' { + // Line continuation: the value carries on below. + p.pos++ + p.line++ + + continue + } + + decoded, ok := unescape(e) + if !ok { + return "", p.err() + } + + b.WriteByte(decoded) + + p.pos++ + + lastKeep = b.Len() + + case !inQuote && (c == '#' || c == ';'): + return b.String()[:lastKeep], nil + + default: + b.WriteByte(c) + + p.pos++ + + if inQuote || (c != ' ' && c != '\t') { + lastKeep = b.Len() + } + } + } + + if inQuote { + return "", p.err() + } + + return b.String()[:lastKeep], nil +} + +func unescape(c byte) (byte, bool) { + switch c { + case 'n': + return '\n', true + case 't': + return '\t', true + case 'b': + return '\b', true + case '\\': + return '\\', true + case '"': + return '"', true + } + + return 0, false +} + +func (p *parser) skipBlank() { + for p.pos < len(p.data) && (p.data[p.pos] == ' ' || p.data[p.pos] == '\t') { + p.pos++ + } +} + +func (p *parser) atLineEnd() bool { + return p.pos >= len(p.data) || p.data[p.pos] == '\n' || (p.data[p.pos] == '\r' && p.peekIsLF()) +} + +func (p *parser) peekIsLF() bool { + return p.pos+1 < len(p.data) && p.data[p.pos+1] == '\n' +} + +func (p *parser) skipToLineEnd() { + for !p.atLineEnd() { + p.pos++ + } +} + +func (p *parser) consumeLineEnd() { + if p.pos < len(p.data) && p.data[p.pos] == '\r' { + p.pos++ + } + + if p.pos < len(p.data) && p.data[p.pos] == '\n' { + p.pos++ + p.line++ + } +} + +func (p *parser) err() error { + return &ParseError{Line: p.line} +} diff --git a/internal/plumbing/format/config/write.go b/internal/plumbing/format/config/write.go new file mode 100644 index 0000000..981d642 --- /dev/null +++ b/internal/plumbing/format/config/write.go @@ -0,0 +1,82 @@ +package config + +import ( + "fmt" + "os" + "path/filepath" +) + +// ReadFile parses the configuration file at path. A missing file parses as an +// empty configuration, which is what Git does; any other read error is +// reported so a transient failure can never be mistaken for an empty file and +// then written back over the original. +func ReadFile(path string) (*File, error) { + data, err := os.ReadFile(path) + if err != nil { + if os.IsNotExist(err) { + return Parse(nil) + } + + return nil, err + } + + return Parse(data) +} + +// WriteFile replaces path with f's contents atomically, so an interrupted or +// failing write leaves the original file untouched rather than truncated. +func WriteFile(path string, f *File, perm os.FileMode) error { + tmp, err := os.CreateTemp(filepath.Dir(path), filepath.Base(path)+".gogit-*") + if err != nil { + return fmt.Errorf("create temp config: %w", err) + } + + tmpName := tmp.Name() + renamed := false + + // Any path out of this function other than a completed rename leaves the + // original file untouched and removes the partial temp file. + defer func() { + if !renamed { + _ = tmp.Close() + _ = os.Remove(tmpName) + } + }() + + if err := tmp.Chmod(perm); err != nil { + return fmt.Errorf("chmod temp config: %w", err) + } + + if _, err := tmp.Write(f.Bytes()); err != nil { + return fmt.Errorf("write temp config: %w", err) + } + + if err := tmp.Sync(); err != nil { + return fmt.Errorf("sync temp config: %w", err) + } + + // Close before rename, and report the error: a deferred Close would hide + // write failures that only surface on flush. + if err := tmp.Close(); err != nil { + return fmt.Errorf("close temp config: %w", err) + } + + if err := os.Rename(tmpName, path); err != nil { + return fmt.Errorf("replace config: %w", err) + } + + renamed = true + + return nil +} + +// FileMode returns the permissions to give a rewritten config file: the +// existing file's mode, or 0o666 for a new one, so an existing file's +// permissions survive the rename. +func FileMode(path string) os.FileMode { + if st, err := os.Stat(path); err == nil { + return st.Mode().Perm() + } + + return 0o666 +} From 3d080af6ca8335c1ccc9514b0761f3086ca224c4 Mon Sep 17 00:00:00 2001 From: Muskan Paliwal Date: Wed, 26 Aug 2026 12:14:15 +0530 Subject: [PATCH 2/8] gogit: Write config keys with the spelling the caller gave Git writes a variable using the spelling from the command line rather than folding it, and creates a section header the same way. gogit lower-cased both, so it wrote bytes Git would not: config Core.MyVar Value -> git: [Core] MyVar = Value gogit: [core] myvar = Value Upstream gates on this. t1300-config.sh case 17, "mixed case", appends Section.Movie into an existing [section] and expects the variable spelled Movie; running it against gogit halted there. The rule is broader than creation. Git rewrites the whole "name = value" pair, so setting an existing variable through a differently cased key re-spells it too: [section] Movie = old, set via Section.MOVIE, becomes MOVIE = new. The previous code went out of its way to preserve the file's spelling on rewrite, which was wrong in the other direction. So Key now carries the spelling it was given rather than a folded form, and case-insensitivity moves from the data to the comparison: Matches folds section and variable names, leaves subsection names byte-exact, and keeps the empty subsection distinct from no subsection. Key must no longer be compared with ==, which the -c override lookup was doing. The parser likewise records each section and variable as the file spells it. The one place folding stays is the deprecated [section.subsection] header form, where Git folds the whole header and the subsection really is case-insensitive. Verified against Git 2.50.0 across thirteen paired invocations covering creation, append into an existing section of either case, rewrite through a differently cased key, --add, --replace-all, unset, valueless variables, options sharing a line with their header, and subsection case-sensitivity; all thirteen agree on file contents, stdout and exit status. t1300-config.sh now reaches case 20, up from 17; the new blocker is value-pattern matching on --replace-all, which remains out of scope. The earlier differential suites are unchanged at 43/43, 33/33 and 17/18, and make conformance still passes 42/42. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Pp48YmGBdCMfRjvqEHRpe --- cmd/gogit/config-scope.go | 2 +- cmd/gogit/config_cmd_test.go | 93 ++++++++++++ internal/plumbing/format/config/file.go | 50 +++--- internal/plumbing/format/config/file_test.go | 152 ++++++++++++++++++- internal/plumbing/format/config/key.go | 37 +++-- internal/plumbing/format/config/key_test.go | 58 ++++++- internal/plumbing/format/config/parser.go | 10 +- 7 files changed, 350 insertions(+), 52 deletions(-) diff --git a/cmd/gogit/config-scope.go b/cmd/gogit/config-scope.go index 13343c3..e07bf5d 100644 --- a/cmd/gogit/config-scope.go +++ b/cmd/gogit/config-scope.go @@ -52,7 +52,7 @@ func (s configSource) values(key gitconfig.Key) []string { var out []string for _, o := range s.overrides { - if o.key == key { + if o.key.Matches(key) { out = append(out, o.value) } } diff --git a/cmd/gogit/config_cmd_test.go b/cmd/gogit/config_cmd_test.go index 6bdf381..40fb959 100644 --- a/cmd/gogit/config_cmd_test.go +++ b/cmd/gogit/config_cmd_test.go @@ -114,6 +114,7 @@ const ( valAuthor = "A U Thor\n" valThree = "three" keyPathDir = "p.dir" + keyMixed = "Section.Movie" valNewName = "New Name" overridePr = "pr.k=CMD" ) @@ -841,3 +842,95 @@ func assertDiagnostic(t *testing.T, stderr string, code int, want string) { t.Fatalf("stderr = %q, want %q", stderr, want) } } + +// TestConfigPreservesKeySpelling covers the command level of git's rule that a +// variable is written with the spelling given on the command line. Upstream's +// t1300-config.sh gates on this at its "mixed case" case. +func TestConfigPreservesKeySpelling(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + src string + args []string + want string + }{ + { + name: "new variable in an existing section", + src: "[section]\n\tpenguin = little blue\n", + args: []string{cmdConfig, subSet, keyMixed, "BadPhysics"}, + want: "[section]\n\tpenguin = little blue\n\tMovie = BadPhysics\n", + }, + { + name: "legacy implicit set", + src: "[section]\n\tpenguin = little blue\n", + args: []string{cmdConfig, keyMixed, "BadPhysics"}, + want: "[section]\n\tpenguin = little blue\n\tMovie = BadPhysics\n", + }, + { + name: "rewriting replaces the old spelling", + src: "[section]\n\tMovie = old\n", + args: []string{cmdConfig, subSet, "Section.MOVIE", "new"}, + want: "[section]\n\tMOVIE = new\n", + }, + { + name: "a new section takes the command-line spelling", + src: "", + args: []string{cmdConfig, subSet, "Core.MyVar", "V"}, + want: "[Core]\n\tMyVar = V\n", + }, + { + name: "--add keeps the command-line spelling", + src: "[core]\n\tx = 1\n", + args: []string{cmdConfig, flagAdd, "Core.MyVar", "V"}, + want: "[core]\n\tx = 1\n\tMyVar = V\n", + }, + { + name: "a new subsection keeps every part's spelling", + src: "", + args: []string{cmdConfig, subSet, "Remote.Origin.URL", "u"}, + want: "[Remote \"Origin\"]\n\tURL = u\n", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + repo, home := newConfigRepo(t, tc.src) + path := filepath.Join(repo, ".git", "config") + + if _, stderr, code := runConfig(t, repo, home, tc.args...); code != 0 { + t.Fatalf("gogit %v: exit %d, stderr %q", tc.args, code, stderr) + } + + if got := readFileString(t, path); got != tc.want { + t.Fatalf("gogit %v:\n--- got ---\n%s\n--- want ---\n%s", tc.args, got, tc.want) + } + }) + } +} + +// TestConfigLookupIgnoresCase confirms reads still fold section and variable +// names while keeping subsection names case-sensitive. +func TestConfigLookupIgnoresCase(t *testing.T) { + t.Parallel() + + repo, home := newConfigRepo(t, "[Section]\n\tMovie = BadPhysics\n[remote \"Origin\"]\n\tURL = u\n") + + for _, key := range []string{"section.movie", "SECTION.MOVIE", keyMixed} { + stdout, _, code := runConfig(t, repo, home, cmdConfig, subGet, key) + if code != 0 || stdout != "BadPhysics\n" { + t.Errorf("get %s: exit %d, stdout %q", key, code, stdout) + } + } + + if stdout, _, code := runConfig(t, repo, home, cmdConfig, subGet, "REMOTE.Origin.URL"); code != 0 || stdout != "u\n" { + t.Errorf("get REMOTE.Origin.URL: exit %d, stdout %q", code, stdout) + } + + // The subsection is the one part that stays case-sensitive. + if _, _, code := runConfig(t, repo, home, cmdConfig, subGet, "remote.origin.url"); code != 1 { + t.Errorf("subsection lookup should be case-sensitive: exit %d, want 1", code) + } +} diff --git a/internal/plumbing/format/config/file.go b/internal/plumbing/format/config/file.go index 749c106..2b5a2e8 100644 --- a/internal/plumbing/format/config/file.go +++ b/internal/plumbing/format/config/file.go @@ -101,7 +101,7 @@ func (f *File) Values(key Key) []string { var out []string for _, o := range f.options { - if o.key.matches(key) { + if o.key.Matches(key) { out = append(out, o.value) } } @@ -126,7 +126,7 @@ func (f *File) Set(key Key, value string) error { var found []*optionRec for _, o := range f.options { - if o.key.matches(key) { + if o.key.Matches(key) { found = append(found, o) } } @@ -139,7 +139,7 @@ func (f *File) Set(key Key, value string) error { return f.insert(key, value) } - return f.rewrite(found[0], value) + return f.rewrite(found[0], key, value) } // ReplaceAll collapses every value of key into a single value. @@ -147,7 +147,7 @@ func (f *File) ReplaceAll(key Key, value string) error { var found []*optionRec for _, o := range f.options { - if o.key.matches(key) { + if o.key.Matches(key) { found = append(found, o) } } @@ -156,12 +156,12 @@ func (f *File) ReplaceAll(key Key, value string) error { case 0: return f.insert(key, value) case 1: - return f.rewrite(found[0], value) + return f.rewrite(found[0], key, value) } // Keep the first occurrence in place and drop the rest, so the value // stays where the file already had it. - edits := []edit{{start: found[0].valueStart, end: found[0].logicalEnd, text: encodeValue(value)}} + edits := []edit{f.writeEdit(found[0], key, value)} for _, o := range found[1:] { edits = append(edits, deleteEdit(o)) } @@ -182,7 +182,7 @@ func (f *File) UnsetAll(key Key) (int, error) { ) for _, o := range f.options { - if o.key.matches(key) { + if o.key.Matches(key) { edits = append(edits, deleteEdit(o)) n++ } @@ -252,29 +252,25 @@ func isSpace(c byte) bool { return c == ' ' || c == '\t' || c == '\r' || c == '\n' } -func (f *File) rewrite(o *optionRec, value string) error { +func (f *File) rewrite(o *optionRec, key Key, value string) error { + return f.apply([]edit{f.writeEdit(o, key, value)}) +} + +// writeEdit returns the splice that makes o hold value. git rewrites the whole +// "name = value" pair using the spelling from the command line, so a variable +// already in the file can come back differently capitalised. +func (f *File) writeEdit(o *optionRec, key Key, value string) edit { + text := key.Name + " = " + encodeValue(value) + if !o.alone { // The variable shares its line with its section header. git moves it // onto a line of its own rather than rewriting in place. - sec := f.sections[o.secIdx] - text := "\n\t" + f.rawName(o) + " = " + encodeValue(value) - - return f.apply([]edit{{start: sec.headerEnd, end: o.logicalEnd, text: text}}) + return edit{start: f.sections[o.secIdx].headerEnd, end: o.logicalEnd, text: "\n\t" + text} } - text := encodeValue(value) - if o.valueless { - // A bare "name" gains its separator along with the value. - text = " = " + text - } - - return f.apply([]edit{{start: o.valueStart, end: o.logicalEnd, text: text}}) -} - -// rawName returns the variable name as spelled in the file, so rewriting a -// value never silently changes the name's capitalisation. -func (f *File) rawName(o *optionRec) string { - return string(f.data[o.nameEnd-len(o.key.Name) : o.nameEnd]) + // Start at the name rather than the value, so the leading indentation is + // kept but the old spelling is not. + return edit{start: o.nameEnd - len(o.key.Name), end: o.logicalEnd, text: text} } // insert places a new variable after the last variable of the section it @@ -283,9 +279,7 @@ func (f *File) insert(key Key, value string) error { line := "\t" + key.Name + " = " + encodeValue(value) + "\n" for _, s := range slices.Backward(f.sections) { - if s.key.Section == key.Section && - s.key.HasSubsection == key.HasSubsection && - s.key.Subsection == key.Subsection { + if s.key.sameSection(key) { text := line if s.entryEnd > 0 && f.data[s.entryEnd-1] != '\n' { // The section is the last line and lacks a terminator. diff --git a/internal/plumbing/format/config/file_test.go b/internal/plumbing/format/config/file_test.go index 9c5ffee..5eb051b 100644 --- a/internal/plumbing/format/config/file_test.go +++ b/internal/plumbing/format/config/file_test.go @@ -68,7 +68,7 @@ func TestValues(t *testing.T) { want []string }{ {name: "simple", key: keyUserName, want: []string{"A U Thor"}}, - {name: "case-insensitive key", key: "USER.NAME", want: []string{"A U Thor"}}, + {name: "case-insensitive key", key: keyUpperName, want: []string{"A U Thor"}}, {name: "subsection", key: keyOriginURL, want: []string{"https://example.com/x.git"}}, {name: "subsection with dots", key: keyDottedSub, want: []string{"https://t.example/o.git"}}, {name: "empty subsection", key: keyEmptySub, want: []string{"EMPTYSUB"}}, @@ -408,3 +408,153 @@ func TestWriteFileIsAtomicAndKeepsMode(t *testing.T) { t.Fatalf("WriteFile left temp files behind: %v", entries) } } + +// TestWritesUseTheGivenSpelling pins git's rule that a variable is written +// with the spelling supplied by the caller, not folded and not taken from the +// file. Existing section headers keep their own spelling; a newly created one +// takes the caller's. +func TestWritesUseTheGivenSpelling(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + src string + do func(*testing.T, *config.File) + want string + }{ + { + name: "new variable in an existing section", + src: "[section]\n\tpenguin = little blue\n", + do: func(t *testing.T, f *config.File) { + t.Helper() + + if err := f.Set(mustKey(t, "Section.Movie"), "BadPhysics"); err != nil { + t.Fatal(err) + } + }, + want: "[section]\n\tpenguin = little blue\n\tMovie = BadPhysics\n", + }, + { + name: "an existing header is never re-cased", + src: "[Section]\n\tpenguin = x\n", + do: func(t *testing.T, f *config.File) { + t.Helper() + + if err := f.Set(mustKey(t, "section.movie"), "Y"); err != nil { + t.Fatal(err) + } + }, + want: "[Section]\n\tpenguin = x\n\tmovie = Y\n", + }, + { + name: "rewriting replaces the old spelling", + src: "[section]\n\tMovie = old\n", + do: func(t *testing.T, f *config.File) { + t.Helper() + + if err := f.Set(mustKey(t, "Section.MOVIE"), "new"); err != nil { + t.Fatal(err) + } + }, + want: "[section]\n\tMOVIE = new\n", + }, + { + name: "a new section takes the caller's spelling", + src: "", + do: func(t *testing.T, f *config.File) { + t.Helper() + + if err := f.Set(mustKey(t, "Core.MyVar"), "V"); err != nil { + t.Fatal(err) + } + }, + want: "[Core]\n\tMyVar = V\n", + }, + { + name: "a new subsection keeps every part's spelling", + src: "", + do: func(t *testing.T, f *config.File) { + t.Helper() + + if err := f.Set(mustKey(t, "Remote.Origin.URL"), "u"); err != nil { + t.Fatal(err) + } + }, + want: "[Remote \"Origin\"]\n\tURL = u\n", + }, + { + name: "add uses the caller's spelling", + src: "[core]\n\tx = 1\n", + do: func(t *testing.T, f *config.File) { + t.Helper() + + if err := f.Add(mustKey(t, "Core.MyVar"), "V"); err != nil { + t.Fatal(err) + } + }, + want: "[core]\n\tx = 1\n\tMyVar = V\n", + }, + { + name: "replace-all uses the caller's spelling", + src: "[core]\n\tmyvar = 1\n\tmyvar = 2\n", + do: func(t *testing.T, f *config.File) { + t.Helper() + + if err := f.ReplaceAll(mustKey(t, "Core.MyVar"), "V"); err != nil { + t.Fatal(err) + } + }, + want: "[core]\n\tMyVar = V\n", + }, + { + name: "a valueless variable gains a value and the new spelling", + src: "[a]\n\tFlag\n", + do: func(t *testing.T, f *config.File) { + t.Helper() + + if err := f.Set(mustKey(t, "A.FLAG"), "yes"); err != nil { + t.Fatal(err) + } + }, + want: "[a]\n\tFLAG = yes\n", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + f := mustParse(t, tc.src) + tc.do(t, f) + + if got := string(f.Bytes()); got != tc.want { + t.Fatalf("--- got ---\n%s\n--- want ---\n%s", got, tc.want) + } + }) + } +} + +// TestLookupIgnoresCase confirms that changing the model to carry spelling did +// not make lookups case-sensitive. +func TestLookupIgnoresCase(t *testing.T) { + t.Parallel() + + f := mustParse(t, "[Section]\n\tMovie = BadPhysics\n[remote \"Origin\"]\n\tURL = u\n") + + for _, key := range []string{"section.movie", "SECTION.MOVIE", "Section.Movie"} { + if got, ok := f.Get(mustKey(t, key)); !ok || got != "BadPhysics" { + t.Errorf("Get(%q) = (%q, %v), want (BadPhysics, true)", key, got, ok) + } + } + + for _, key := range []string{keyMixedSub, "REMOTE.Origin.URL"} { + if got, ok := f.Get(mustKey(t, key)); !ok || got != "u" { + t.Errorf("Get(%q) = (%q, %v), want (u, true)", key, got, ok) + } + } + + // The subsection is the one part that stays case-sensitive. + if _, ok := f.Get(mustKey(t, "remote.origin.url")); ok { + t.Error("subsection lookup should be case-sensitive") + } +} diff --git a/internal/plumbing/format/config/key.go b/internal/plumbing/format/config/key.go index 13e185d..e5411b9 100644 --- a/internal/plumbing/format/config/key.go +++ b/internal/plumbing/format/config/key.go @@ -11,18 +11,23 @@ package config import "strings" // Key is a parsed configuration key such as "remote.origin.url". +// +// Every field holds the spelling it was given, because git writes a variable +// using the spelling from the command line rather than folding it. Matching, +// by contrast, ignores case for section and variable names, so compare keys +// with Matches and never with ==. type Key struct { - // Section is the section name, lower-cased. Section names are - // case-insensitive in Git. + // Section is the section name as spelled. Section names match + // case-insensitively in Git. Section string - // Subsection is the subsection name, preserved verbatim. Subsection - // names are case-sensitive in Git. + // Subsection is the subsection name. Subsection names match + // case-sensitively in Git. Subsection string // HasSubsection distinguishes "user..name", which addresses the empty // subsection [user ""], from "user.name", which addresses [user]. HasSubsection bool - // Name is the variable name, lower-cased. Variable names are - // case-insensitive in Git. + // Name is the variable name as spelled. Variable names match + // case-insensitively in Git. Name string } @@ -68,8 +73,8 @@ func ParseKey(key string) (Key, error) { } k := Key{ - Section: strings.ToLower(key[:first]), - Name: strings.ToLower(key[last+1:]), + Section: key[:first], + Name: key[last+1:], } if first != last { @@ -93,12 +98,16 @@ func (k Key) String() string { return k.Section + "." + k.Name } -// matches reports whether k addresses the same variable as other. Section and -// variable names compare case-insensitively (both are stored lower-cased); -// subsection names compare byte-for-byte. -func (k Key) matches(other Key) bool { - return k.Section == other.Section && - k.Name == other.Name && +// Matches reports whether k addresses the same variable as other. Section and +// variable names compare case-insensitively; subsection names compare +// byte-for-byte, and an empty subsection is distinct from none at all. +func (k Key) Matches(other Key) bool { + return k.sameSection(other) && strings.EqualFold(k.Name, other.Name) +} + +// sameSection reports whether both keys address the same section header. +func (k Key) sameSection(other Key) bool { + return strings.EqualFold(k.Section, other.Section) && k.HasSubsection == other.HasSubsection && k.Subsection == other.Subsection } diff --git a/internal/plumbing/format/config/key_test.go b/internal/plumbing/format/config/key_test.go index 8ec4b7e..a66c6f4 100644 --- a/internal/plumbing/format/config/key_test.go +++ b/internal/plumbing/format/config/key_test.go @@ -19,6 +19,9 @@ const ( keyOriginURL = "remote.origin.url" keyDottedSub = "remote.team.one.url" + keyUpperName = "USER.NAME" + keyMixedSub = "remote.Origin.url" + valPlain = "plain" ) @@ -53,13 +56,13 @@ func TestParseKey(t *testing.T) { want: config.Key{Section: secUser, Subsection: "", HasSubsection: true, Name: varName}, }, { - name: "section and variable are lower-cased", - in: "USER.NAME", - want: config.Key{Section: secUser, Name: varName}, + name: "spelling is preserved, not folded", + in: keyUpperName, + want: config.Key{Section: "USER", Name: "NAME"}, }, { name: "subsection keeps its case", - in: "remote.Origin.url", + in: keyMixedSub, want: config.Key{Section: secRemote, Subsection: "Origin", HasSubsection: true, Name: varURL}, }, { @@ -149,3 +152,50 @@ func TestKeyString(t *testing.T) { }) } } + +// TestKeyMatches pins the comparison rules: section and variable names fold, +// subsection names do not, and an empty subsection is its own thing. Keys must +// never be compared with ==, which would make these all unequal. +func TestKeyMatches(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + a, b string + want bool + }{ + {name: "identical", a: keyUserName, b: keyUserName, want: true}, + {name: "section folds", a: "USER.name", b: keyUserName, want: true}, + {name: "variable folds", a: "user.NAME", b: keyUserName, want: true}, + {name: "both fold", a: keyUpperName, b: keyUserName, want: true}, + {name: "subsection does not fold", a: keyMixedSub, b: keyOriginURL, want: false}, + {name: "subsection section folds", a: "REMOTE.origin.url", b: keyOriginURL, want: true}, + {name: "empty subsection is not no subsection", a: keyEmptySub, b: keyUserName, want: false}, + {name: "different variable", a: "user.email", b: keyUserName, want: false}, + {name: "different section", a: "core.name", b: keyUserName, want: false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + a, err := config.ParseKey(tc.a) + if err != nil { + t.Fatalf("ParseKey(%q): %v", tc.a, err) + } + + b, err := config.ParseKey(tc.b) + if err != nil { + t.Fatalf("ParseKey(%q): %v", tc.b, err) + } + + if got := a.Matches(b); got != tc.want { + t.Fatalf("%q.Matches(%q) = %v, want %v", tc.a, tc.b, got, tc.want) + } + + if got := b.Matches(a); got != tc.want { + t.Fatalf("Matches is not symmetric for %q and %q", tc.a, tc.b) + } + }) + } +} diff --git a/internal/plumbing/format/config/parser.go b/internal/plumbing/format/config/parser.go index cef4542..cd5607f 100644 --- a/internal/plumbing/format/config/parser.go +++ b/internal/plumbing/format/config/parser.go @@ -96,7 +96,7 @@ func (p *parser) parseHeader(lineStart int) error { return p.err() } - key = Key{Section: strings.ToLower(name), Subsection: sub, HasSubsection: true} + key = Key{Section: name, Subsection: sub, HasSubsection: true} case strings.Contains(name, "."): // Deprecated "[section.subsection]" form. Git lower-cases the whole // header, so the subsection is case-insensitive here only. @@ -106,7 +106,9 @@ func (p *parser) parseHeader(lineStart int) error { } key = Key{ - Section: strings.ToLower(sec), + Section: sec, + // Git folds the whole "[section.subsection]" header, so only in + // this deprecated form is the subsection case-insensitive. Subsection: strings.ToLower(sub), HasSubsection: true, } @@ -115,7 +117,7 @@ func (p *parser) parseHeader(lineStart int) error { return p.err() } - key = Key{Section: strings.ToLower(name)} + key = Key{Section: name} } p.skipBlank() @@ -185,7 +187,7 @@ func (p *parser) parseVariable(lineStart int, alone bool) error { p.pos++ } - name := strings.ToLower(string(p.data[nameStart:p.pos])) + name := string(p.data[nameStart:p.pos]) if !validVariableName(name) { return p.err() } From d04072145af70758faa855aeb45760e6979b5e7e Mon Sep 17 00:00:00 2001 From: Muskan Paliwal Date: Wed, 26 Aug 2026 21:43:59 +0530 Subject: [PATCH 3/8] fix(config): preserve safe config edits --- cmd/gogit/config-scope.go | 42 ++++++++++++++---- cmd/gogit/config_cmd_test.go | 46 ++++++++++++++++++++ internal/plumbing/format/config/file.go | 6 +-- internal/plumbing/format/config/file_test.go | 2 + internal/plumbing/format/config/key.go | 8 ++++ internal/plumbing/format/config/key_test.go | 2 + internal/plumbing/format/config/parser.go | 9 +++- 7 files changed, 102 insertions(+), 13 deletions(-) diff --git a/cmd/gogit/config-scope.go b/cmd/gogit/config-scope.go index e07bf5d..564984a 100644 --- a/cmd/gogit/config-scope.go +++ b/cmd/gogit/config-scope.go @@ -77,25 +77,33 @@ func readSources(o *configOpts) ([]configSource, error) { return []configSource{src}, nil } - var files []configFile + sources := make([]configSource, 0) if p, ok := systemConfigPath(); ok { - files = append(files, absoluteFile(p)) + src, loaded, err := loadOptionalSource(absoluteFile(p)) + if err != nil { + return nil, err + } + + if loaded { + sources = append(sources, src) + } } for _, p := range globalConfigPaths() { - files = append(files, absoluteFile(p)) + src, loaded, err := loadOptionalSource(absoluteFile(p)) + if err != nil { + return nil, err + } + + if loaded { + sources = append(sources, src) + } } // Being outside a repository is not an error for a default read: -c // overrides and the global files still apply, as they do in git. if f, err := localConfigFile(); err == nil { - files = append(files, f) - } - - sources := make([]configSource, 0, len(files)+1) - - for _, f := range files { src, err := loadSource(f) if err != nil { return nil, err @@ -174,6 +182,22 @@ func loadSource(cf configFile) (configSource, error) { return configSource{file: f}, nil } +// loadOptionalSource ignores missing and unreadable system/global files, as +// git does, but still reports malformed files instead of hiding corruption. +func loadOptionalSource(cf configFile) (configSource, bool, error) { + f, err := gitconfig.ReadFile(cf.path) + if err != nil { + var perr *gitconfig.ParseError + if errors.As(err, &perr) { + return configSource{}, false, configReadError(cf, err) + } + + return configSource{}, false, nil + } + + return configSource{file: f}, true, nil +} + // globalConfigPaths returns the per-user config files in ascending precedence // order, so ~/.gitconfig wins over the XDG file as it does in git. func globalConfigPaths() []string { diff --git a/cmd/gogit/config_cmd_test.go b/cmd/gogit/config_cmd_test.go index 40fb959..0f31735 100644 --- a/cmd/gogit/config_cmd_test.go +++ b/cmd/gogit/config_cmd_test.go @@ -543,6 +543,52 @@ func TestConfigFile(t *testing.T) { } } +func TestConfigUnsetRemovesSameLineSection(t *testing.T) { + t.Parallel() + + base := t.TempDir() + home := filepath.Join(base, "home") + file := filepath.Join(base, "config") + + mkdirAll(t, home) + writeConfig(t, file, "[a] value = old # remove with the value\n[b]\n\tother = kept\n") + + _, stderr, err := runGogitEnv(t, base, configEnv(home), cmdConfig, subUnset, flagFile, file, "a.value") + if err != nil { + t.Fatalf("unset failed: %v (stderr %q)", err, stderr) + } + + if got, want := readFileString(t, file), "[b]\n\tother = kept\n"; got != want { + t.Fatalf("config after unset = %q, want %q", got, want) + } +} + +func TestConfigIgnoresUnreadableOptionalGlobal(t *testing.T) { + t.Parallel() + + base := t.TempDir() + home := filepath.Join(base, "home") + global := filepath.Join(base, "unreadable-global") + + mkdirAll(t, home) + + if err := os.Mkdir(global, 0o755); err != nil { + t.Fatal(err) + } + + env := append(configEnv(home), "GIT_CONFIG_GLOBAL="+global) + + stdout, stderr, err := runGogitEnv(t, base, env, cmdConfig, subGet, "missing.key") + if stdout != "" || stderr != "" { + t.Fatalf("unreadable optional global produced stdout=%q stderr=%q", stdout, stderr) + } + + var ee *exec.ExitError + if !errors.As(err, &ee) || ee.ExitCode() != 1 { + t.Fatalf("exit error = %v, want status 1", err) + } +} + func TestConfigPath(t *testing.T) { t.Parallel() diff --git a/internal/plumbing/format/config/file.go b/internal/plumbing/format/config/file.go index 2b5a2e8..f5d8279 100644 --- a/internal/plumbing/format/config/file.go +++ b/internal/plumbing/format/config/file.go @@ -312,9 +312,9 @@ func deleteEdit(o *optionRec) edit { return edit{start: o.lineStart, end: o.lineEnd} } - // The variable shares its line with a section header; drop just the - // variable text and leave the header standing. - return edit{start: o.nameEnd - len(o.key.Name), end: o.logicalEnd} + // A section header and its only same-line variable are one logical entry to + // git's unset operation, so remove the whole physical line. + return edit{start: o.lineStart, end: o.lineEnd} } // apply splices edits into the document and re-parses, so recorded offsets diff --git a/internal/plumbing/format/config/file_test.go b/internal/plumbing/format/config/file_test.go index 5eb051b..7726c30 100644 --- a/internal/plumbing/format/config/file_test.go +++ b/internal/plumbing/format/config/file_test.go @@ -138,6 +138,8 @@ func TestParseRejectsMalformed(t *testing.T) { {name: "invalid section character", src: "[a_b]\n\tc = d\n", wantLine: 1}, {name: "variable starting with a digit", src: "[a]\n\t1b = c\n", wantLine: 2}, {name: "junk after a value", src: "[a]\n\tb = c\n\td e f\n", wantLine: 3}, + {name: "NUL in a value", src: "[a]\n\tb = one\x00two\n", wantLine: 2}, + {name: "NUL in a subsection", src: "[a \"sub\x00section\"]\n\tb = c\n", wantLine: 1}, } for _, tc := range tests { diff --git a/internal/plumbing/format/config/key.go b/internal/plumbing/format/config/key.go index e5411b9..e6c3837 100644 --- a/internal/plumbing/format/config/key.go +++ b/internal/plumbing/format/config/key.go @@ -80,6 +80,10 @@ func ParseKey(key string) (Key, error) { if first != last { k.Subsection = key[first+1 : last] k.HasSubsection = true + + if !validSubsectionName(k.Subsection) { + return Key{}, &KeyInvalidError{Key: key} + } } if !validSectionName(k.Section) || !validVariableName(k.Name) { @@ -144,6 +148,10 @@ func validVariableName(s string) bool { return true } +func validSubsectionName(s string) bool { + return !strings.ContainsAny(s, "\x00\n") +} + func isAlpha(c byte) bool { return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') } diff --git a/internal/plumbing/format/config/key_test.go b/internal/plumbing/format/config/key_test.go index a66c6f4..d4719af 100644 --- a/internal/plumbing/format/config/key_test.go +++ b/internal/plumbing/format/config/key_test.go @@ -85,6 +85,8 @@ func TestParseKey(t *testing.T) { {name: "space in variable", in: "a.b c", wantErr: isInvalidKey}, {name: "space in section", in: "a b.c", wantErr: isInvalidKey}, {name: "leading separator", in: ".b", wantErr: isInvalidKey}, + {name: "newline in subsection", in: "a.foo\nbar.b", wantErr: isInvalidKey}, + {name: "NUL in subsection", in: "a.foo\x00bar.b", wantErr: isInvalidKey}, } for _, tc := range tests { diff --git a/internal/plumbing/format/config/parser.go b/internal/plumbing/format/config/parser.go index cd5607f..73b5798 100644 --- a/internal/plumbing/format/config/parser.go +++ b/internal/plumbing/format/config/parser.go @@ -151,6 +151,10 @@ func (p *parser) parseSubsection() (string, error) { } c := p.data[p.pos] + if c == 0 { + return "", p.err() + } + if c == '"' { p.pos++ @@ -251,7 +255,7 @@ func (p *parser) parseVariable(lineStart int, alone bool) error { // parseValue reads a variable's value, honouring quoted runs, backslash // escapes and backslash-newline continuations, and trimming unquoted trailing // whitespace. -func (p *parser) parseValue() (string, error) { +func (p *parser) parseValue() (string, error) { //nolint:gocognit // parser state machine var ( b strings.Builder inQuote bool @@ -260,6 +264,9 @@ func (p *parser) parseValue() (string, error) { for p.pos < len(p.data) { c := p.data[p.pos] + if c == 0 { + return "", p.err() + } switch { case c == '\n' || (c == '\r' && p.peekIsLF()): From 1692576ce1e97f97496e43994860e774f42b5a57 Mon Sep 17 00:00:00 2001 From: Muskan Paliwal Date: Wed, 26 Aug 2026 23:53:13 +0530 Subject: [PATCH 4/8] fix(config): preserve repeated section edits --- cmd/gogit/config.go | 2 +- internal/plumbing/format/config/file.go | 97 +++++++++++++++----- internal/plumbing/format/config/file_test.go | 39 ++++++++ 3 files changed, 114 insertions(+), 24 deletions(-) diff --git a/cmd/gogit/config.go b/cmd/gogit/config.go index 15194f6..32fa480 100644 --- a/cmd/gogit/config.go +++ b/cmd/gogit/config.go @@ -89,7 +89,7 @@ func hasConfigOverride(key string) bool { // Lookup order: -c override > defaultVal. Empty-string override means false. // repoCfg is accepted for future expansion but not consulted in v1. // -//nolint:unparam // key/repoCfg used by future callers (Task 7+). +//nolint:unparam // repoCfg is part of the shared config helper contract. func configBool(key string, repoCfg *config.Config, defaultVal bool) bool { configOverrideMu.Lock() v, ok := configOverrides[key] diff --git a/internal/plumbing/format/config/file.go b/internal/plumbing/format/config/file.go index f5d8279..e4686d1 100644 --- a/internal/plumbing/format/config/file.go +++ b/internal/plumbing/format/config/file.go @@ -159,10 +159,10 @@ func (f *File) ReplaceAll(key Key, value string) error { return f.rewrite(found[0], key, value) } - // Keep the first occurrence in place and drop the rest, so the value - // stays where the file already had it. - edits := []edit{f.writeEdit(found[0], key, value)} - for _, o := range found[1:] { + // Git keeps the last matching occurrence in place and drops the earlier + // ones, preserving the section ordering around the surviving value. + edits := []edit{f.writeEdit(found[len(found)-1], key, value)} + for _, o := range found[:len(found)-1] { edits = append(edits, deleteEdit(o)) } @@ -203,22 +203,50 @@ func (f *File) emptySectionEdits(deletions []edit) []edit { touched := map[int]bool{} for _, o := range f.options { - for _, d := range deletions { - if o.lineStart >= d.start && o.lineEnd <= d.end { - touched[o.secIdx] = true - } + if isDeletedOption(o, deletions) { + touched[o.secIdx] = true } } var out []edit + processed := map[int]bool{} + for idx := range touched { - s := f.sections[idx] - if !s.plain { + if processed[idx] { + continue + } + + var group []int + + for candidate, s := range f.sections { + if s.key.sameSection(f.sections[idx].key) { + group = append(group, candidate) + processed[candidate] = true + } + } + + if groupHasRemainingOption(f.options, group, deletions) || + groupHasContent(f.data, f.sections, group, deletions) { + continue + } + + allPlain := true + + for _, candidate := range group { + if !f.sections[candidate].plain { + allPlain = false + + break + } + } + + if !allPlain { continue } - if remainderIsHeaderOnly(f.data, s, deletions) { + for _, candidate := range group { + s := f.sections[candidate] out = append(out, edit{start: s.lineStart, end: s.lineEnd}) } } @@ -226,26 +254,49 @@ func (f *File) emptySectionEdits(deletions []edit) []edit { return out } -// remainderIsHeaderOnly reports whether the section's region would contain -// nothing but its own header line once deletions are applied. -func remainderIsHeaderOnly(data []byte, s *sectionRec, deletions []edit) bool { - for i := s.lineEnd; i < s.regionEnd && i < len(data); i++ { - deleted := false +func isDeletedOption(o *optionRec, deletions []edit) bool { + for _, d := range deletions { + if o.lineStart >= d.start && o.lineEnd <= d.end { + return true + } + } - for _, d := range deletions { - if i >= d.start && i < d.end { - deleted = true + return false +} - break +func groupHasRemainingOption(options []*optionRec, group []int, deletions []edit) bool { + for _, o := range options { + for _, idx := range group { + if o.secIdx == idx && !isDeletedOption(o, deletions) { + return true } } + } - if !deleted && !isSpace(data[i]) { - return false + return false +} + +func groupHasContent(data []byte, sections []*sectionRec, group []int, deletions []edit) bool { + for _, idx := range group { + s := sections[idx] + for i := s.lineEnd; i < s.regionEnd && i < len(data); i++ { + deleted := false + + for _, d := range deletions { + if i >= d.start && i < d.end { + deleted = true + + break + } + } + + if !deleted && !isSpace(data[i]) { + return true + } } } - return true + return false } func isSpace(c byte) bool { diff --git a/internal/plumbing/format/config/file_test.go b/internal/plumbing/format/config/file_test.go index 7726c30..186c03d 100644 --- a/internal/plumbing/format/config/file_test.go +++ b/internal/plumbing/format/config/file_test.go @@ -289,6 +289,45 @@ func TestSetRefusesMultipleValues(t *testing.T) { } } +func TestReplaceAllUsesLastRepeatedSection(t *testing.T) { + t.Parallel() + + f := mustParse(t, "[a]\n\tv = one\n[a]\n\tv = two\n") + if err := f.ReplaceAll(mustKey(t, "a.v"), "three"); err != nil { + t.Fatal(err) + } + + if got, want := string(f.Bytes()), "[a]\n[a]\n\tv = three\n"; got != want { + t.Fatalf("ReplaceAll = %q, want %q", got, want) + } +} + +func TestUnsetKeepsRepeatedSectionHeadersWhenGroupHasValues(t *testing.T) { + t.Parallel() + + f := mustParse(t, "[a]\n\tv = one\n[a]\n\tother = two\n") + if _, err := f.UnsetAll(mustKey(t, "a.v")); err != nil { + t.Fatal(err) + } + + if got, want := string(f.Bytes()), "[a]\n[a]\n\tother = two\n"; got != want { + t.Fatalf("UnsetAll = %q, want %q", got, want) + } +} + +func TestUnsetRemovesAllEmptyRepeatedSections(t *testing.T) { + t.Parallel() + + f := mustParse(t, "[a]\n\tv = one\n[a]\n\tv = two\n") + if _, err := f.UnsetAll(mustKey(t, "a.v")); err != nil { + t.Fatal(err) + } + + if got := string(f.Bytes()); got != "" { + t.Fatalf("UnsetAll = %q, want empty file", got) + } +} + func TestUnsetAllReportsMissingKey(t *testing.T) { t.Parallel() From 844616082fc2129ca8ca1a7da9b3fc0fa6381893 Mon Sep 17 00:00:00 2001 From: Muskan Paliwal Date: Thu, 27 Aug 2026 22:10:37 +0530 Subject: [PATCH 5/8] fix(config): preserve symlink targets --- internal/plumbing/format/config/file_test.go | 66 ++++++++++++++++++++ internal/plumbing/format/config/write.go | 51 ++++++++++++++- 2 files changed, 115 insertions(+), 2 deletions(-) diff --git a/internal/plumbing/format/config/file_test.go b/internal/plumbing/format/config/file_test.go index 186c03d..9ffbb1d 100644 --- a/internal/plumbing/format/config/file_test.go +++ b/internal/plumbing/format/config/file_test.go @@ -450,6 +450,72 @@ func TestWriteFileIsAtomicAndKeepsMode(t *testing.T) { } } +func TestWriteFileFollowsConfigSymlink(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + f := mustParse(t, "[a]\n\tb = updated\n") + + t.Run("existing target", func(t *testing.T) { + t.Parallel() + + realPath := filepath.Join(dir, "real") + linkPath := filepath.Join(dir, "link") + + if err := os.WriteFile(realPath, []byte("[a]\n\tb = old\n"), 0o640); err != nil { + t.Fatal(err) + } + + if err := os.Symlink("real", linkPath); err != nil { + t.Skipf("symbolic links unavailable: %v", err) + } + + if err := config.WriteFile(linkPath, f, config.FileMode(linkPath)); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + if target, err := os.Readlink(linkPath); err != nil || target != "real" { + t.Fatalf("link target = %q, %v; want real", target, err) + } + + got, err := os.ReadFile(realPath) + if err != nil { + t.Fatal(err) + } + + if string(got) != string(f.Bytes()) { + t.Fatalf("target contents = %q, want %q", got, f.Bytes()) + } + }) + + t.Run("missing target", func(t *testing.T) { + t.Parallel() + + linkPath := filepath.Join(dir, "dangling") + + if err := os.Symlink("created", linkPath); err != nil { + t.Skipf("symbolic links unavailable: %v", err) + } + + if err := config.WriteFile(linkPath, f, config.FileMode(linkPath)); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + if target, err := os.Readlink(linkPath); err != nil || target != "created" { + t.Fatalf("link target = %q, %v; want created", target, err) + } + + got, err := os.ReadFile(filepath.Join(dir, "created")) + if err != nil { + t.Fatal(err) + } + + if string(got) != string(f.Bytes()) { + t.Fatalf("target contents = %q, want %q", got, f.Bytes()) + } + }) +} + // TestWritesUseTheGivenSpelling pins git's rule that a variable is written // with the spelling supplied by the caller, not folded and not taken from the // file. Existing section headers keep their own spelling; a newly created one diff --git a/internal/plumbing/format/config/write.go b/internal/plumbing/format/config/write.go index 981d642..f8792f7 100644 --- a/internal/plumbing/format/config/write.go +++ b/internal/plumbing/format/config/write.go @@ -26,7 +26,12 @@ func ReadFile(path string) (*File, error) { // WriteFile replaces path with f's contents atomically, so an interrupted or // failing write leaves the original file untouched rather than truncated. func WriteFile(path string, f *File, perm os.FileMode) error { - tmp, err := os.CreateTemp(filepath.Dir(path), filepath.Base(path)+".gogit-*") + target, err := resolveWritePath(path) + if err != nil { + return fmt.Errorf("resolve config path: %w", err) + } + + tmp, err := os.CreateTemp(filepath.Dir(target), filepath.Base(target)+".gogit-*") if err != nil { return fmt.Errorf("create temp config: %w", err) } @@ -61,7 +66,7 @@ func WriteFile(path string, f *File, perm os.FileMode) error { return fmt.Errorf("close temp config: %w", err) } - if err := os.Rename(tmpName, path); err != nil { + if err := os.Rename(tmpName, target); err != nil { return fmt.Errorf("replace config: %w", err) } @@ -70,6 +75,48 @@ func WriteFile(path string, f *File, perm os.FileMode) error { return nil } +func resolveWritePath(path string) (string, error) { + current, err := filepath.Abs(path) + if err != nil { + return "", err + } + + seen := map[string]bool{} + + for { + current = filepath.Clean(current) + if seen[current] { + return "", fmt.Errorf("symbolic link loop at %s", current) + } + + seen[current] = true + + info, err := os.Lstat(current) + if err != nil { + if os.IsNotExist(err) { + return current, nil + } + + return "", err + } + + if info.Mode()&os.ModeSymlink == 0 { + return current, nil + } + + target, err := os.Readlink(current) + if err != nil { + return "", err + } + + if !filepath.IsAbs(target) { + target = filepath.Join(filepath.Dir(current), target) + } + + current = target + } +} + // FileMode returns the permissions to give a rewritten config file: the // existing file's mode, or 0o666 for a new one, so an existing file's // permissions survive the rename. From bb2fe4d0820ecd361000da2a559e04b3a731ff7b Mon Sep 17 00:00:00 2001 From: Muskan Paliwal Date: Mon, 31 Aug 2026 12:49:32 +0530 Subject: [PATCH 6/8] fix(config): complete Git-compatible config behavior --- cmd/gogit/config-cmd.go | 102 +++-- cmd/gogit/config-include.go | 409 +++++++++++++++++++ cmd/gogit/config-scope.go | 160 ++++++-- cmd/gogit/config_cmd_test.go | 147 ++++++- internal/plumbing/format/config/file.go | 17 + internal/plumbing/format/config/file_test.go | 130 +++++- internal/plumbing/format/config/write.go | 70 ++-- 7 files changed, 898 insertions(+), 137 deletions(-) create mode 100644 cmd/gogit/config-include.go diff --git a/cmd/gogit/config-cmd.go b/cmd/gogit/config-cmd.go index 3069598..745f7bd 100644 --- a/cmd/gogit/config-cmd.go +++ b/cmd/gogit/config-cmd.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "os" + "os/user" "path/filepath" "strings" @@ -188,10 +189,9 @@ func runConfigGet(o *configOpts, rawKey string) error { return err } - var values []string - - for _, src := range sources { - values = append(values, src.values(key)...) + values, err := effectiveConfigValues(sources, key) + if err != nil { + return err } if len(values) == 0 { @@ -237,36 +237,38 @@ func runConfigWrite(o *configOpts, rawKey, value string, mode writeMode) error { return err } - f, err := gitconfig.ReadFile(target.path) - if err != nil { - return configReadError(target, err) - } + err = gitconfig.UpdateFile(target.path, func(f *gitconfig.File) error { + switch mode { + case writeAdd: + return f.Add(key, value) - switch mode { - case writeAdd: - err = f.Add(key, value) + case writeUnset: + if !o.all && len(f.Values(key)) > 1 { + fmt.Fprintf(os.Stderr, "warning: %s has multiple values\n", rawKey) - case writeUnset: - if !o.all && len(f.Values(key)) > 1 { - fmt.Fprintf(os.Stderr, "warning: %s has multiple values\n", rawKey) + return &gitExitError{ + code: exitCannotReplace, + msg: fmt.Sprintf("error: cannot unset multiple values for %s; use --all", rawKey), + } + } - return &gitExitError{ - code: exitCannotReplace, - msg: fmt.Sprintf("error: cannot unset multiple values for %s; use --all", rawKey), + n, err := f.UnsetAll(key) + if err == nil && n == 0 { + return &gitExitError{code: exitUnsetMissing} } - } - var n int + return err - if n, err = f.UnsetAll(key); err == nil && n == 0 { - // git exits 5 without a diagnostic; test_unconfig relies on it. - return &gitExitError{code: exitUnsetMissing} - } + case writeSet: + if o.all { + return f.ReplaceAll(key, value) + } + + err := f.Set(key, value) + if !errors.Is(err, gitconfig.ErrMultipleValues) { + return err + } - case writeSet: - if o.all { - err = f.ReplaceAll(key, value) - } else if err = f.Set(key, value); errors.Is(err, gitconfig.ErrMultipleValues) { fmt.Fprintf(os.Stderr, "warning: %s has multiple values\n", rawKey) return &gitExitError{ @@ -275,13 +277,14 @@ func runConfigWrite(o *configOpts, rawKey, value string, mode writeMode) error { " Use --add or --all to change %s.", rawKey), } } - } + return nil + }) if err != nil { - return err + return configReadError(target, err) } - return gitconfig.WriteFile(target.path, f, gitconfig.FileMode(target.path)) + return nil } // parseConfigKey converts Git's key diagnostics into exit-1 errors. @@ -313,25 +316,36 @@ func usageError(msg string) error { // expandPath applies --path canonicalization: a leading ~ becomes the user's // home directory. Any other value is returned unchanged. func expandPath(v string) (string, error) { - if v != "~" && !strings.HasPrefix(v, "~/") { - if strings.HasPrefix(v, "~") { - return "", &gitExitError{ - code: exitFatal, - msg: fmt.Sprintf("fatal: failed to expand user dir in: '%s'", v), - } - } - + if !strings.HasPrefix(v, "~") { return v, nil } - home, err := os.UserHomeDir() - if err != nil { - return "", err + if v == "~" || strings.HasPrefix(v, "~/") { + home, err := os.UserHomeDir() + if err != nil { + return "", err + } + + if v == "~" { + return home, nil + } + + return filepath.Join(home, v[2:]), nil } - if v == "~" { - return home, nil + name, suffix, _ := strings.Cut(v[1:], "/") + + account, err := user.Lookup(name) + if err == nil && account.HomeDir != "" { + if suffix == "" { + return account.HomeDir, nil + } + + return filepath.Join(account.HomeDir, suffix), nil } - return filepath.Join(home, v[2:]), nil + return "", &gitExitError{ + code: exitFatal, + msg: fmt.Sprintf("fatal: failed to expand user dir in: '%s'", v), + } } diff --git a/cmd/gogit/config-include.go b/cmd/gogit/config-include.go new file mode 100644 index 0000000..2e27526 --- /dev/null +++ b/cmd/gogit/config-include.go @@ -0,0 +1,409 @@ +package main + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "regexp" + "strings" + + gitconfig "github.com/go-git/cli/internal/plumbing/format/config" +) + +const maxConfigIncludeDepth = 10 + +type configIncludeContext struct { + gitDirs []string + branch string + remoteURLs []string +} + +func effectiveConfigValues(sources []configSource, key gitconfig.Key) ([]string, error) { + ctx := configContext() + + urls, err := collectRemoteURLs(sources, &ctx) + if err != nil { + return nil, err + } + + ctx.remoteURLs = urls + + var values []string + + for _, source := range sources { + found, err := sourceValues(source, key, &ctx, map[string]bool{}, 0) + if err != nil { + return nil, err + } + + values = append(values, found...) + } + + return values, nil +} + +func sourceValues( + source configSource, + key gitconfig.Key, + ctx *configIncludeContext, + stack map[string]bool, + depth int, +) ([]string, error) { + if source.file == nil { + var values []string + + for _, override := range source.overrides { + if override.key.Matches(key) { + values = append(values, override.value) + } + } + + return values, nil + } + + var values []string + + for _, entry := range source.file.Entries() { + if entry.Key.Matches(key) { + values = append(values, entry.Value) + } + + if !source.includes { + continue + } + + included, ok, err := includedSource(source, entry, ctx, stack, depth) + if err != nil { + return nil, err + } + + if !ok { + continue + } + + found, err := sourceValues(included, key, ctx, stack, depth+1) + delete(stack, included.location.path) + + if err != nil { + return nil, err + } + + values = append(values, found...) + } + + return values, nil +} + +func includedSource( + parent configSource, + entry gitconfig.Entry, + ctx *configIncludeContext, + stack map[string]bool, + depth int, +) (configSource, bool, error) { + condition, include := includeCondition(entry.Key) + if !include || !conditionMatches(condition, parent.location.path, ctx) { + return configSource{}, false, nil + } + + if depth >= maxConfigIncludeDepth { + return configSource{}, false, errors.New("maximum config include depth exceeded") + } + + path, err := resolveIncludePath(entry.Value, parent.location.path) + if err != nil { + return configSource{}, false, err + } + + if stack[path] { + return configSource{}, false, fmt.Errorf("config include cycle at %s", path) + } + + stack[path] = true + + source, err := loadSource(absoluteFile(path)) + if err != nil { + delete(stack, path) + + return configSource{}, false, err + } + + source.includes = true + + return source, true, nil +} + +func includeCondition(key gitconfig.Key) (string, bool) { + if !strings.EqualFold(key.Name, "path") { + return "", false + } + + switch { + case strings.EqualFold(key.Section, "include") && !key.HasSubsection: + return "", true + case strings.EqualFold(key.Section, "includeIf") && key.HasSubsection: + return key.Subsection, true + default: + return "", false + } +} + +func conditionMatches(condition, sourcePath string, ctx *configIncludeContext) bool { + if condition == "" { + return true + } + + if pattern, ok := strings.CutPrefix(condition, "gitdir:"); ok { + return matchGitDirs(pattern, sourcePath, ctx.gitDirs, false) + } + + if pattern, ok := strings.CutPrefix(condition, "gitdir/i:"); ok { + return matchGitDirs(pattern, sourcePath, ctx.gitDirs, true) + } + + if pattern, ok := strings.CutPrefix(condition, "onbranch:"); ok { + if strings.HasSuffix(pattern, "/") { + pattern += "**" + } + + return gitWildMatch(pattern, ctx.branch, false) + } + + const remoteCondition = "hasconfig:remote.*.url:" + if pattern, ok := strings.CutPrefix(condition, remoteCondition); ok { + for _, remoteURL := range ctx.remoteURLs { + if gitWildMatch(pattern, remoteURL, false) { + return true + } + } + } + + return false +} + +func matchGitDirs(pattern, sourcePath string, gitDirs []string, insensitive bool) bool { + switch { + case strings.HasPrefix(pattern, "~/"): + if home := os.Getenv("HOME"); home != "" { + pattern = filepath.Join(home, pattern[2:]) + } + case strings.HasPrefix(pattern, "./"): + pattern = filepath.Join(filepath.Dir(sourcePath), pattern[2:]) + case !filepath.IsAbs(pattern): + pattern = "**/" + pattern + } + + if strings.HasSuffix(pattern, "/") { + pattern += "**" + } + + patterns := []string{filepath.ToSlash(pattern)} + if !strings.ContainsAny(pattern, "*?[") { + if resolved, err := filepath.EvalSymlinks(pattern); err == nil && resolved != pattern { + patterns = append(patterns, filepath.ToSlash(resolved)) + } + } + + for _, candidatePattern := range patterns { + for _, gitDir := range gitDirs { + candidate := filepath.ToSlash(gitDir) + if gitWildMatch(candidatePattern, candidate, insensitive) || + gitWildMatch(candidatePattern, candidate+"/", insensitive) { + return true + } + } + } + + return false +} + +func gitWildMatch(pattern, value string, insensitive bool) bool { + var expression strings.Builder + expression.WriteByte('^') + + for i := 0; i < len(pattern); { + switch pattern[i] { + case '*': + if i+1 < len(pattern) && pattern[i+1] == '*' { + i += 2 + if i < len(pattern) && pattern[i] == '/' { + expression.WriteString("(?:.*/)?") + + i++ + } else { + expression.WriteString(".*") + } + } else { + expression.WriteString("[^/]*") + + i++ + } + case '?': + expression.WriteString("[^/]") + + i++ + case '[': + end := strings.IndexByte(pattern[i+1:], ']') + if end < 0 { + expression.WriteString(`\[`) + + i++ + + continue + } + + end += i + 1 + class := pattern[i+1 : end] + + expression.WriteByte('[') + + if strings.HasPrefix(class, "!") { + expression.WriteByte('^') + + class = class[1:] + } + + expression.WriteString(strings.ReplaceAll(class, `\`, `\\`)) + expression.WriteByte(']') + + i = end + 1 + default: + expression.WriteString(regexp.QuoteMeta(pattern[i : i+1])) + i++ + } + } + + expression.WriteByte('$') + + patternExpression := expression.String() + if insensitive { + patternExpression = "(?i:" + patternExpression + ")" + } + + compiled, err := regexp.Compile(patternExpression) + + return err == nil && compiled.MatchString(value) +} + +func resolveIncludePath(path, sourcePath string) (string, error) { + expanded, err := expandPath(path) + if err != nil { + return "", err + } + + if !filepath.IsAbs(expanded) { + expanded = filepath.Join(filepath.Dir(sourcePath), expanded) + } + + return filepath.Clean(expanded), nil +} + +func configContext() configIncludeContext { + gitDir, _, err := discoverGitDir() + if err != nil { + return configIncludeContext{} + } + + abs, err := filepath.Abs(gitDir) + if err != nil { + abs = filepath.Clean(gitDir) + } + + gitDirs := []string{abs} + if resolved, err := filepath.EvalSymlinks(abs); err == nil && resolved != abs { + gitDirs = append(gitDirs, resolved) + } + + return configIncludeContext{ + gitDirs: gitDirs, + branch: currentBranch(gitDir), + } +} + +func currentBranch(gitDir string) string { + data, err := os.ReadFile(filepath.Join(gitDir, "HEAD")) + if err != nil { + return "" + } + + ref, ok := strings.CutPrefix(strings.TrimSpace(string(data)), "ref: refs/heads/") + if !ok { + return "" + } + + return ref +} + +func collectRemoteURLs(sources []configSource, ctx *configIncludeContext) ([]string, error) { + var urls []string + + for _, source := range sources { + found, err := sourceRemoteURLs(source, ctx, map[string]bool{}, 0) + if err != nil { + return nil, err + } + + urls = append(urls, found...) + } + + return urls, nil +} + +func sourceRemoteURLs( + source configSource, + ctx *configIncludeContext, + stack map[string]bool, + depth int, +) ([]string, error) { + if source.file == nil { + var urls []string + + for _, override := range source.overrides { + if isRemoteURL(override.key) { + urls = append(urls, override.value) + } + } + + return urls, nil + } + + var urls []string + + for _, entry := range source.file.Entries() { + if isRemoteURL(entry.Key) { + urls = append(urls, entry.Value) + } + + condition, include := includeCondition(entry.Key) + if !source.includes || !include || strings.HasPrefix(condition, "hasconfig:") || + !conditionMatches(condition, source.location.path, ctx) { + continue + } + + included, ok, err := includedSource(source, entry, ctx, stack, depth) + if err != nil { + return nil, err + } + + if !ok { + continue + } + + found, err := sourceRemoteURLs(included, ctx, stack, depth+1) + delete(stack, included.location.path) + + if err != nil { + return nil, err + } + + urls = append(urls, found...) + } + + return urls, nil +} + +func isRemoteURL(key gitconfig.Key) bool { + return strings.EqualFold(key.Section, "remote") && key.HasSubsection && + strings.EqualFold(key.Name, "url") +} diff --git a/cmd/gogit/config-scope.go b/cmd/gogit/config-scope.go index 564984a..5ad95aa 100644 --- a/cmd/gogit/config-scope.go +++ b/cmd/gogit/config-scope.go @@ -5,6 +5,8 @@ import ( "fmt" "os" "path/filepath" + "slices" + "strconv" "strings" gitconfig "github.com/go-git/cli/internal/plumbing/format/config" @@ -40,41 +42,32 @@ type configFile struct { // configSource is one configuration file consulted for a read, or the set of // -c command-line overrides. type configSource struct { - file *gitconfig.File - overrides []configOverride -} - -func (s configSource) values(key gitconfig.Key) []string { - if s.file != nil { - return s.file.Values(key) - } - - var out []string - - for _, o := range s.overrides { - if o.key.Matches(key) { - out = append(out, o.value) - } - } + location configFile + file *gitconfig.File - return out + overrides []configOverride + includes bool } // readSources returns the files to consult, lowest precedence first. // -// A location flag selects exactly one source. Otherwise git's default order -// applies: system, then the XDG and per-user global files, then the -// repository, then -c overrides. +// A location flag limits the sources to one scope. Otherwise git's default +// precedence applies. func readSources(o *configOpts) ([]configSource, error) { - if file, ok, err := explicitLocation(o); err != nil { + if files, ok, err := explicitReadFiles(o); err != nil { return nil, err } else if ok { - src, err := loadSource(file) - if err != nil { - return nil, err + sources := make([]configSource, 0, len(files)) + for _, file := range files { + src, err := loadSource(file) + if err != nil { + return nil, err + } + + sources = append(sources, src) } - return []configSource{src}, nil + return sources, nil } sources := make([]configSource, 0) @@ -86,6 +79,7 @@ func readSources(o *configOpts) ([]configSource, error) { } if loaded { + src.includes = true sources = append(sources, src) } } @@ -97,6 +91,7 @@ func readSources(o *configOpts) ([]configSource, error) { } if loaded { + src.includes = true sources = append(sources, src) } } @@ -109,7 +104,20 @@ func readSources(o *configOpts) ([]configSource, error) { return nil, err } + src.includes = true sources = append(sources, src) + + if worktree, enabled, err := worktreeConfigFile(src.file); err != nil { + return nil, err + } else if enabled { + worktreeSource, err := loadSource(worktree) + if err != nil { + return nil, err + } + + worktreeSource.includes = true + sources = append(sources, worktreeSource) + } } return append(sources, configSource{overrides: configOverrideList}), nil @@ -124,7 +132,7 @@ func absoluteFile(path string) configFile { // writeTarget returns the single file a mutation applies to. Writes default // to the repository config, never to the merged view. func writeTarget(o *configOpts) (configFile, error) { - if file, ok, err := explicitLocation(o); err != nil { + if file, ok, err := explicitWriteLocation(o); err != nil { return configFile{}, err } else if ok { return file, nil @@ -133,13 +141,42 @@ func writeTarget(o *configOpts) (configFile, error) { return localConfigFile() } -// explicitLocation resolves --file/--local/--global/--system. For --global it -// picks the file git would write to: the XDG file when it already exists, -// otherwise ~/.gitconfig. -func explicitLocation(o *configOpts) (configFile, bool, error) { +func explicitReadFiles(o *configOpts) ([]configFile, bool, error) { + switch { + case o.file != "": + return []configFile{absoluteFile(o.file)}, true, nil + + case o.local: + f, err := localConfigFile() + + return []configFile{f}, true, err + + case o.global: + paths := globalConfigPaths() + + files := make([]configFile, 0, len(paths)) + for _, path := range paths { + files = append(files, absoluteFile(path)) + } + + return files, true, nil + + case o.system: + p, ok := systemConfigPath() + if !ok { + return nil, true, errors.New("system config is disabled by GIT_CONFIG_NOSYSTEM") + } + + return []configFile{absoluteFile(p)}, true, nil + } + + return nil, false, nil +} + +// explicitWriteLocation resolves the single file changed by an explicit scope. +func explicitWriteLocation(o *configOpts) (configFile, bool, error) { switch { case o.file != "": - // git reports --file exactly as it was spelled on the command line. return absoluteFile(o.file), true, nil case o.local: @@ -149,9 +186,9 @@ func explicitLocation(o *configOpts) (configFile, bool, error) { case o.global: paths := globalConfigPaths() - for _, p := range paths { - if _, err := os.Stat(p); err == nil { - return absoluteFile(p), true, nil + for _, path := range slices.Backward(paths) { + if _, err := os.Stat(path); err == nil { + return absoluteFile(path), true, nil } } @@ -179,7 +216,7 @@ func loadSource(cf configFile) (configSource, error) { return configSource{}, configReadError(cf, err) } - return configSource{file: f}, nil + return configSource{location: cf, file: f}, nil } // loadOptionalSource ignores missing and unreadable system/global files, as @@ -195,7 +232,7 @@ func loadOptionalSource(cf configFile) (configSource, bool, error) { return configSource{}, false, nil } - return configSource{file: f}, true, nil + return configSource{location: cf, file: f}, true, nil } // globalConfigPaths returns the per-user config files in ascending precedence @@ -227,7 +264,7 @@ func globalConfigPaths() []string { // systemConfigPath reports the system config file, and whether the system // scope is enabled at all. func systemConfigPath() (string, bool) { - if v := os.Getenv("GIT_CONFIG_NOSYSTEM"); v != "" && v != "0" { + if gitEnvBool(os.Getenv("GIT_CONFIG_NOSYSTEM")) { return "", false } @@ -242,6 +279,15 @@ func systemConfigPath() (string, bool) { return "/etc/gitconfig", true } +func gitEnvBool(value string) bool { + switch strings.ToLower(value) { + case "", "0", "false", "no", "off": + return false + default: + return true + } +} + // localConfigFile returns the repository's config file. For a linked worktree // this is the common directory's config, not the worktree's own git dir. func localConfigFile() (configFile, error) { @@ -265,6 +311,46 @@ func localConfigFile() (configFile, error) { }, nil } +func worktreeConfigFile(local *gitconfig.File) (configFile, bool, error) { + enabled := false + + key := gitconfig.Key{Section: "extensions", Name: "worktreeConfig"} + for _, entry := range local.Entries() { + if entry.Key.Matches(key) { + enabled = gitBool(entry.Value, entry.Implicit) + } + } + + if !enabled { + return configFile{}, false, nil + } + + gitDir, display, err := discoverGitDir() + if err != nil { + return configFile{}, false, err + } + + return configFile{ + path: filepath.Join(gitDir, "config.worktree"), + display: display + "/config.worktree", + }, true, nil +} + +func gitBool(value string, implicit bool) bool { + if implicit { + return true + } + + switch strings.ToLower(value) { + case "true", "yes", "on", "1": + return true + } + + n, err := strconv.ParseInt(value, 10, 64) + + return err == nil && n != 0 +} + // commonGitDir follows a linked worktree's `commondir` pointer back to the // main git directory, which is where shared state such as config lives. func commonGitDir(gitDir string) string { diff --git a/cmd/gogit/config_cmd_test.go b/cmd/gogit/config_cmd_test.go index 0f31735..df854a8 100644 --- a/cmd/gogit/config_cmd_test.go +++ b/cmd/gogit/config_cmd_test.go @@ -2,8 +2,10 @@ package main import ( "errors" + "fmt" "os" "os/exec" + "os/user" "path/filepath" "strings" "testing" @@ -117,6 +119,7 @@ const ( keyMixed = "Section.Movie" valNewName = "New Name" overridePr = "pr.k=CMD" + changedPr = "[pr]\n\tk = CHANGED\n" ) const baseConfig = `[core] @@ -463,6 +466,69 @@ func TestConfigScopePrecedence(t *testing.T) { } } +func TestConfigIncludes(t *testing.T) { + t.Parallel() + + repo, home := newConfigRepo(t, "") + included := filepath.Join(repo, ".git", "included.cfg") + writeConfig(t, included, "[order]\n\tvalue = INCLUDED\n") + + gitDirPattern := filepath.ToSlash(filepath.Join(repo, ".git")) + writeConfig(t, filepath.Join(repo, ".git", "config"), fmt.Sprintf(`[order] + value = BEFORE +[include] + path = included.cfg +[includeIf "gitdir:%s"] + path = included.cfg +[includeIf "onbranch:main"] + path = included.cfg +[includeIf "hasconfig:remote.*.url:https://example.com/**"] + path = included.cfg +[remote "origin"] + url = https://example.com/repo +[order] + value = AFTER +`, gitDirPattern)) + + stdout, stderr, code := runConfig(t, repo, home, cmdConfig, subGet, flagAll, "order.value") + if code != 0 { + t.Fatalf("included read: exit %d, stderr %q", code, stderr) + } + + if want := "BEFORE\n" + strings.Repeat("INCLUDED\n", 4) + "AFTER\n"; stdout != want { + t.Fatalf("included values = %q, want %q", stdout, want) + } + + stdout, _, code = runConfig(t, repo, home, cmdConfig, subGet, flagFile, + filepath.Join(repo, ".git", "config"), flagAll, "order.value") + if code != 0 || stdout != "BEFORE\nAFTER\n" { + t.Fatalf("--file should not follow includes by default: exit %d, stdout %q", code, stdout) + } +} + +func TestGitWildMatch(t *testing.T) { + t.Parallel() + + tests := []struct { + pattern string + value string + insensitive bool + want bool + }{ + {pattern: "**/group/**", value: "/tmp/group/repo/.git", want: true}, + {pattern: "feature/**", value: "feature/team/topic", want: true}, + {pattern: "release/[0-9]?", value: "release/12", want: true}, + {pattern: "repo", value: "REPO", insensitive: true, want: true}, + {pattern: "repo", value: "REPO", want: false}, + } + + for _, test := range tests { + if got := gitWildMatch(test.pattern, test.value, test.insensitive); got != test.want { + t.Errorf("gitWildMatch(%q, %q) = %v, want %v", test.pattern, test.value, got, test.want) + } + } +} + func TestConfigWritesDefaultToLocalScope(t *testing.T) { t.Parallel() @@ -474,7 +540,7 @@ func TestConfigWritesDefaultToLocalScope(t *testing.T) { t.Fatalf("set failed: exit %d, stderr %q", code, stderr) } - if got, want := readFileString(t, filepath.Join(repo, ".git", cmdConfig)), "[pr]\n\tk = CHANGED\n"; got != want { + if got, want := readFileString(t, filepath.Join(repo, ".git", cmdConfig)), changedPr; got != want { t.Fatalf("local config = %q, want %q", got, want) } @@ -494,7 +560,7 @@ func TestConfigGlobalWrite(t *testing.T) { t.Fatalf("set --global failed: exit %d, stderr %q", code, stderr) } - if got, want := readFileString(t, global), "[pr]\n\tk = CHANGED\n"; got != want { + if got, want := readFileString(t, global), changedPr; got != want { t.Fatalf("global config = %q, want %q", got, want) } @@ -503,6 +569,30 @@ func TestConfigGlobalWrite(t *testing.T) { } } +func TestConfigGlobalPrefersHomeFileOverXDG(t *testing.T) { + t.Parallel() + + repo, home := newConfigRepo(t, "") + xdg := filepath.Join(home, ".config", "git", "config") + global := filepath.Join(home, ".gitconfig") + + mkdirAll(t, filepath.Dir(xdg)) + writeConfig(t, xdg, "[pr]\n\tk = XDG\n") + writeConfig(t, global, "[pr]\n\tk = HOME\n") + + if _, stderr, code := runConfig(t, repo, home, cmdConfig, subSet, flagGlobal, keyPr, "CHANGED"); code != 0 { + t.Fatalf("set --global failed: exit %d, stderr %q", code, stderr) + } + + if got, want := readFileString(t, global), changedPr; got != want { + t.Fatalf("home config = %q, want %q", got, want) + } + + if got, want := readFileString(t, xdg), "[pr]\n\tk = XDG\n"; got != want { + t.Fatalf("XDG config was modified: got %q, want %q", got, want) + } +} + func TestConfigFile(t *testing.T) { t.Parallel() @@ -628,6 +718,50 @@ func TestConfigPath(t *testing.T) { } } +func TestConfigPathExpandsExistingUser(t *testing.T) { + t.Parallel() + + account, err := user.Current() + if err != nil || account.Username == "" || account.HomeDir == "" || strings.Contains(account.Username, "/") { + t.Skipf("current user is unavailable for ~user expansion: %v", err) + } + + repo, home := newConfigRepo(t, "[p]\n\tdir = ~"+account.Username+"/sub\n") + + stdout, stderr, code := runConfig(t, repo, home, cmdConfig, subGet, flagPath, keyPathDir) + if code != 0 { + t.Fatalf("--path failed: exit %d, stderr %q", code, stderr) + } + + if want := filepath.Join(account.HomeDir, "sub") + "\n"; stdout != want { + t.Fatalf("expanded path = %q, want %q", stdout, want) + } +} + +func TestConfigNoSystemFalseKeepsSystemScopeEnabled(t *testing.T) { + t.Parallel() + + repo, home := newConfigRepo(t, "") + system := filepath.Join(t.TempDir(), "system.cfg") + writeConfig(t, system, "[scope]\n\tvalue = SYSTEM\n") + + env := []string{ + "HOME=" + home, + "XDG_CONFIG_HOME=", + "GIT_CONFIG_NOSYSTEM=false", + "GIT_CONFIG_SYSTEM=" + system, + } + + stdout, stderr, err := runGogitEnv(t, repo, env, cmdConfig, subGet, "scope.value") + if err != nil { + t.Fatalf("system read failed: %v (stderr %q)", err, stderr) + } + + if stdout != "SYSTEM\n" { + t.Fatalf("system value = %q, want SYSTEM", stdout) + } +} + func TestConfigInvalidCombinations(t *testing.T) { t.Parallel() @@ -680,13 +814,15 @@ func TestConfigLinkedWorktree(t *testing.T) { mkdirAll(t, filepath.Join(main, ".git", "refs")) writeConfig(t, filepath.Join(main, ".git", "HEAD"), "ref: refs/heads/main\n") - writeConfig(t, filepath.Join(main, ".git", cmdConfig), "[user]\n\tname = MAIN\n") + writeConfig(t, filepath.Join(main, ".git", cmdConfig), + "[extensions]\n\tworktreeConfig = true\n[user]\n\tname = MAIN\n") writeConfig(t, filepath.Join(wt, ".git"), "gitdir: "+wtGitDir+"\n") writeConfig(t, filepath.Join(wtGitDir, "HEAD"), "ref: refs/heads/other\n") writeConfig(t, filepath.Join(wtGitDir, "commondir"), "../..\n") + writeConfig(t, filepath.Join(wtGitDir, "config.worktree"), "[user]\n\tname = WORKTREE\n") stdout, stderr, code := runConfig(t, wt, home, cmdConfig, subGet, keyUserName) - if code != 0 || stdout != "MAIN\n" { + if code != 0 || stdout != "WORKTREE\n" { t.Fatalf("worktree read: exit %d, stdout %q, stderr %q", code, stdout, stderr) } @@ -695,7 +831,8 @@ func TestConfigLinkedWorktree(t *testing.T) { t.Fatalf("worktree write: exit %d, stderr %q", code, stderr) } - if got, want := readFileString(t, filepath.Join(main, ".git", cmdConfig)), "[user]\n\tname = CHANGED\n"; got != want { + if got, want := readFileString(t, filepath.Join(main, ".git", cmdConfig)), + "[extensions]\n\tworktreeConfig = true\n[user]\n\tname = CHANGED\n"; got != want { t.Fatalf("common config = %q, want %q", got, want) } diff --git a/internal/plumbing/format/config/file.go b/internal/plumbing/format/config/file.go index e4686d1..8336e55 100644 --- a/internal/plumbing/format/config/file.go +++ b/internal/plumbing/format/config/file.go @@ -28,6 +28,13 @@ type File struct { options []*optionRec } +// Entry is one configuration variable in file order. +type Entry struct { + Key Key + Value string + Implicit bool +} + // sectionRec is one occurrence of a section header in the file. The same // section may be opened more than once. type sectionRec struct { @@ -94,6 +101,16 @@ func (f *File) Bytes() []byte { return f.data } +// Entries returns the variables in their original file order. +func (f *File) Entries() []Entry { + entries := make([]Entry, 0, len(f.options)) + for _, o := range f.options { + entries = append(entries, Entry{Key: o.key, Value: o.value, Implicit: o.valueless}) + } + + return entries +} + // Values returns every value recorded for key, in file order. A nil result // means the key is absent, which callers must distinguish from a key whose // single value is the empty string. diff --git a/internal/plumbing/format/config/file_test.go b/internal/plumbing/format/config/file_test.go index 9ffbb1d..1aed709 100644 --- a/internal/plumbing/format/config/file_test.go +++ b/internal/plumbing/format/config/file_test.go @@ -2,9 +2,12 @@ package config_test import ( "errors" + "fmt" "os" "path/filepath" "strings" + "sync" + "sync/atomic" "testing" config "github.com/go-git/cli/internal/plumbing/format/config" @@ -406,7 +409,7 @@ func TestReadFileErrorsAreNotEmptyConfigs(t *testing.T) { } } -func TestWriteFileIsAtomicAndKeepsMode(t *testing.T) { +func TestUpdateFileIsAtomicAndKeepsMode(t *testing.T) { t.Parallel() dir := t.TempDir() @@ -416,10 +419,11 @@ func TestWriteFileIsAtomicAndKeepsMode(t *testing.T) { t.Fatal(err) } - f := mustParse(t, "[a]\n\tb = d\n") - - if err := config.WriteFile(path, f, config.FileMode(path)); err != nil { - t.Fatalf("WriteFile: %v", err) + key := mustKey(t, "a.b") + if err := config.UpdateFile(path, func(f *config.File) error { + return f.Set(key, "d") + }); err != nil { + t.Fatalf("UpdateFile: %v", err) } st, err := os.Stat(path) @@ -446,15 +450,105 @@ func TestWriteFileIsAtomicAndKeepsMode(t *testing.T) { } if len(entries) != 1 { - t.Fatalf("WriteFile left temp files behind: %v", entries) + t.Fatalf("UpdateFile left lock files behind: %v", entries) + } +} + +func TestUpdateFileNewFileHonorsCreationMode(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + control := filepath.Join(dir, "control") + + controlFile, err := os.OpenFile(control, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o666) + if err != nil { + t.Fatal(err) + } + + if err := controlFile.Close(); err != nil { + t.Fatal(err) + } + + path := filepath.Join(dir, "config") + + key := mustKey(t, "secret.token") + if err := config.UpdateFile(path, func(f *config.File) error { + return f.Set(key, "value") + }); err != nil { + t.Fatalf("UpdateFile: %v", err) + } + + controlInfo, err := os.Stat(control) + if err != nil { + t.Fatal(err) + } + + configInfo, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + + if got, want := configInfo.Mode().Perm(), controlInfo.Mode().Perm(); got != want { + t.Fatalf("new config mode = %v, want creation mode %v", got, want) + } +} + +func TestUpdateFileConcurrentWritersNeverLoseSuccessfulChanges(t *testing.T) { + t.Parallel() + + const writers = 32 + + path := filepath.Join(t.TempDir(), "config") + start := make(chan struct{}) + + keys := make([]config.Key, writers) + for i := range writers { + keys[i] = mustKey(t, fmt.Sprintf("batch.k%d", i)) + } + + var ( + wg sync.WaitGroup + succeeded atomic.Int32 + ) + + for i := range writers { + wg.Go(func() { + <-start + + if err := config.UpdateFile(path, func(f *config.File) error { + return f.Set(keys[i], "value") + }); err == nil { + succeeded.Add(1) + } + }) + } + + close(start) + wg.Wait() + + f, err := config.ReadFile(path) + if err != nil { + t.Fatal(err) + } + + var stored int + + for _, entry := range f.Entries() { + if strings.EqualFold(entry.Key.Section, "batch") { + stored++ + } + } + + if got, want := stored, int(succeeded.Load()); got != want { + t.Fatalf("stored %d successful writes, want %d", got, want) } } -func TestWriteFileFollowsConfigSymlink(t *testing.T) { +func TestUpdateFileFollowsConfigSymlink(t *testing.T) { t.Parallel() dir := t.TempDir() - f := mustParse(t, "[a]\n\tb = updated\n") + key := mustKey(t, "a.b") t.Run("existing target", func(t *testing.T) { t.Parallel() @@ -470,8 +564,10 @@ func TestWriteFileFollowsConfigSymlink(t *testing.T) { t.Skipf("symbolic links unavailable: %v", err) } - if err := config.WriteFile(linkPath, f, config.FileMode(linkPath)); err != nil { - t.Fatalf("WriteFile: %v", err) + if err := config.UpdateFile(linkPath, func(f *config.File) error { + return f.Set(key, "updated") + }); err != nil { + t.Fatalf("UpdateFile: %v", err) } if target, err := os.Readlink(linkPath); err != nil || target != "real" { @@ -483,8 +579,8 @@ func TestWriteFileFollowsConfigSymlink(t *testing.T) { t.Fatal(err) } - if string(got) != string(f.Bytes()) { - t.Fatalf("target contents = %q, want %q", got, f.Bytes()) + if want := "[a]\n\tb = updated\n"; string(got) != want { + t.Fatalf("target contents = %q, want %q", got, want) } }) @@ -497,8 +593,10 @@ func TestWriteFileFollowsConfigSymlink(t *testing.T) { t.Skipf("symbolic links unavailable: %v", err) } - if err := config.WriteFile(linkPath, f, config.FileMode(linkPath)); err != nil { - t.Fatalf("WriteFile: %v", err) + if err := config.UpdateFile(linkPath, func(f *config.File) error { + return f.Set(key, "updated") + }); err != nil { + t.Fatalf("UpdateFile: %v", err) } if target, err := os.Readlink(linkPath); err != nil || target != "created" { @@ -510,8 +608,8 @@ func TestWriteFileFollowsConfigSymlink(t *testing.T) { t.Fatal(err) } - if string(got) != string(f.Bytes()) { - t.Fatalf("target contents = %q, want %q", got, f.Bytes()) + if want := "[a]\n\tb = updated\n"; string(got) != want { + t.Fatalf("target contents = %q, want %q", got, want) } }) } diff --git a/internal/plumbing/format/config/write.go b/internal/plumbing/format/config/write.go index f8792f7..b91cc81 100644 --- a/internal/plumbing/format/config/write.go +++ b/internal/plumbing/format/config/write.go @@ -23,54 +23,65 @@ func ReadFile(path string) (*File, error) { return Parse(data) } -// WriteFile replaces path with f's contents atomically, so an interrupted or -// failing write leaves the original file untouched rather than truncated. -func WriteFile(path string, f *File, perm os.FileMode) error { +// UpdateFile applies mutate while holding the config lock and atomically +// replaces path. The lock covers the read as well as the write, preventing two +// writers from successfully committing stale snapshots over one another. +func UpdateFile(path string, mutate func(*File) error) error { target, err := resolveWritePath(path) if err != nil { return fmt.Errorf("resolve config path: %w", err) } - tmp, err := os.CreateTemp(filepath.Dir(target), filepath.Base(target)+".gogit-*") + lockPath := target + ".lock" + + lock, err := os.OpenFile(lockPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o666) if err != nil { - return fmt.Errorf("create temp config: %w", err) + return fmt.Errorf("lock config file %s: %w", path, err) } - tmpName := tmp.Name() - renamed := false + committed := false - // Any path out of this function other than a completed rename leaves the - // original file untouched and removes the partial temp file. defer func() { - if !renamed { - _ = tmp.Close() - _ = os.Remove(tmpName) + if !committed { + _ = lock.Close() + _ = os.Remove(lockPath) } }() - if err := tmp.Chmod(perm); err != nil { - return fmt.Errorf("chmod temp config: %w", err) + f, err := ReadFile(target) + if err != nil { + return err + } + + if err := mutate(f); err != nil { + return err } - if _, err := tmp.Write(f.Bytes()); err != nil { - return fmt.Errorf("write temp config: %w", err) + if st, err := os.Stat(target); err == nil { + if err := lock.Chmod(st.Mode().Perm()); err != nil { + return fmt.Errorf("chmod config lock: %w", err) + } + } else if !os.IsNotExist(err) { + return err } - if err := tmp.Sync(); err != nil { - return fmt.Errorf("sync temp config: %w", err) + if _, err := lock.Write(f.Bytes()); err != nil { + return fmt.Errorf("write config lock: %w", err) } - // Close before rename, and report the error: a deferred Close would hide - // write failures that only surface on flush. - if err := tmp.Close(); err != nil { - return fmt.Errorf("close temp config: %w", err) + if err := lock.Sync(); err != nil { + return fmt.Errorf("sync config lock: %w", err) } - if err := os.Rename(tmpName, target); err != nil { + if err := lock.Close(); err != nil { + return fmt.Errorf("close config lock: %w", err) + } + + if err := os.Rename(lockPath, target); err != nil { return fmt.Errorf("replace config: %w", err) } - renamed = true + committed = true return nil } @@ -116,14 +127,3 @@ func resolveWritePath(path string) (string, error) { current = target } } - -// FileMode returns the permissions to give a rewritten config file: the -// existing file's mode, or 0o666 for a new one, so an existing file's -// permissions survive the rename. -func FileMode(path string) os.FileMode { - if st, err := os.Stat(path); err == nil { - return st.Mode().Perm() - } - - return 0o666 -} From c409dabfac6c81d8b16264085a53ad6e594b72b5 Mon Sep 17 00:00:00 2001 From: Muskan Paliwal Date: Mon, 31 Aug 2026 16:59:32 +0530 Subject: [PATCH 7/8] fix(config): address Git compatibility gaps --- cmd/gogit/config-cmd.go | 25 ++++- cmd/gogit/config-include.go | 52 +++++++--- cmd/gogit/config-scope.go | 159 ++++++++++++++++++++++++++---- cmd/gogit/config_cmd_test.go | 185 ++++++++++++++++++++++++++++++++++- cmd/gogit/main_test.go | 14 +++ 5 files changed, 393 insertions(+), 42 deletions(-) diff --git a/cmd/gogit/config-cmd.go b/cmd/gogit/config-cmd.go index 745f7bd..3845591 100644 --- a/cmd/gogit/config-cmd.go +++ b/cmd/gogit/config-cmd.go @@ -19,6 +19,7 @@ const ( exitUnsetMissing = 5 // unset of a key that does not exist exitCannotReplace = 5 // single value cannot replace several exitFatal = 128 // the config file could not be read + exitUsage = 129 // the command invocation is malformed ) // configOpts holds the flags shared by `config`, `config get`, `config set` @@ -92,7 +93,7 @@ var configCmd = &cobra.Command{ "The modern forms are `config get `, `config set ` and\n" + "`config unset `. The legacy flag spellings (--get, --add, --unset-all\n" + "and a bare `config []`) are also accepted.", - Args: cobra.RangeArgs(1, 2), + Args: configArgs(cobra.RangeArgs(1, 2)), RunE: runConfigLegacy, DisableFlagsInUseLine: true, SilenceUsage: true, @@ -102,7 +103,7 @@ var configCmd = &cobra.Command{ var configGetCmd = &cobra.Command{ Use: "get [] ", Short: "Print the value of a configuration key", - Args: cobra.ExactArgs(1), + Args: configArgs(cobra.ExactArgs(1)), RunE: func(_ *cobra.Command, args []string) error { return runConfigGet(&getOpts, args[0]) }, DisableFlagsInUseLine: true, SilenceUsage: true, @@ -112,7 +113,7 @@ var configGetCmd = &cobra.Command{ var configSetCmd = &cobra.Command{ Use: "set [] ", Short: "Set the value of a configuration key", - Args: cobra.ExactArgs(2), + Args: configArgs(cobra.ExactArgs(2)), RunE: func(_ *cobra.Command, args []string) error { return runConfigWrite(&setOpts, args[0], args[1], writeSet) }, @@ -124,7 +125,7 @@ var configSetCmd = &cobra.Command{ var configUnsetCmd = &cobra.Command{ Use: "unset [] ", Short: "Remove a configuration key", - Args: cobra.ExactArgs(1), + Args: configArgs(cobra.ExactArgs(1)), RunE: func(_ *cobra.Command, args []string) error { return runConfigWrite(&unsetOpts, args[0], "", writeUnset) }, @@ -310,7 +311,21 @@ func configReadError(cf configFile, err error) error { } func usageError(msg string) error { - return &gitExitError{code: exitInvalidKey, msg: "error: " + msg} + return &gitExitError{code: exitUsage, msg: "error: " + msg} +} + +func configArgs(validate cobra.PositionalArgs) cobra.PositionalArgs { + return func(cmd *cobra.Command, args []string) error { + if err := validate(cmd, args); err != nil { + return usageError(err.Error()) + } + + if err := cmd.ValidateFlagGroups(); err != nil { + return usageError(err.Error()) + } + + return nil + } } // expandPath applies --path canonicalization: a leading ~ becomes the user's diff --git a/cmd/gogit/config-include.go b/cmd/gogit/config-include.go index 2e27526..2534dbb 100644 --- a/cmd/gogit/config-include.go +++ b/cmd/gogit/config-include.go @@ -13,6 +13,8 @@ import ( const maxConfigIncludeDepth = 10 +const hasConfigRemoteURLCondition = "hasconfig:remote.*.url:" + type configIncludeContext struct { gitDirs []string branch string @@ -32,7 +34,7 @@ func effectiveConfigValues(sources []configSource, key gitconfig.Key) ([]string, var values []string for _, source := range sources { - found, err := sourceValues(source, key, &ctx, map[string]bool{}, 0) + found, err := sourceValues(source, key, &ctx, map[string]bool{}, 0, false) if err != nil { return nil, err } @@ -49,6 +51,7 @@ func sourceValues( ctx *configIncludeContext, stack map[string]bool, depth int, + includedByHasConfig bool, ) ([]string, error) { if source.file == nil { var values []string @@ -65,6 +68,14 @@ func sourceValues( var values []string for _, entry := range source.file.Entries() { + if includedByHasConfig && isRemoteURL(entry.Key) { + return nil, &gitExitError{ + code: exitFatal, + msg: "fatal: remote URLs cannot be configured in file directly or indirectly " + + "included by includeIf.hasconfig:remote.*.url", + } + } + if entry.Key.Matches(key) { values = append(values, entry.Value) } @@ -82,7 +93,15 @@ func sourceValues( continue } - found, err := sourceValues(included, key, ctx, stack, depth+1) + condition, _ := includeCondition(entry.Key) + found, err := sourceValues( + included, + key, + ctx, + stack, + depth+1, + includedByHasConfig || strings.HasPrefix(condition, hasConfigRemoteURLCondition), + ) delete(stack, included.location.path) if err != nil { @@ -170,8 +189,7 @@ func conditionMatches(condition, sourcePath string, ctx *configIncludeContext) b return gitWildMatch(pattern, ctx.branch, false) } - const remoteCondition = "hasconfig:remote.*.url:" - if pattern, ok := strings.CutPrefix(condition, remoteCondition); ok { + if pattern, ok := strings.CutPrefix(condition, hasConfigRemoteURLCondition); ok { for _, remoteURL := range ctx.remoteURLs { if gitWildMatch(pattern, remoteURL, false) { return true @@ -225,19 +243,25 @@ func gitWildMatch(pattern, value string, insensitive bool) bool { for i := 0; i < len(pattern); { switch pattern[i] { case '*': - if i+1 < len(pattern) && pattern[i+1] == '*' { - i += 2 - if i < len(pattern) && pattern[i] == '/' { - expression.WriteString("(?:.*/)?") - - i++ - } else { - expression.WriteString(".*") - } - } else { + start := i + for i < len(pattern) && pattern[i] == '*' { + i++ + } + + doubleStar := i-start >= 2 && (start == 0 || pattern[start-1] == '/') && + (i == len(pattern) || pattern[i] == '/') + if !doubleStar { expression.WriteString("[^/]*") + continue + } + + if i < len(pattern) { + expression.WriteString("(?:.*/)?") + i++ + } else { + expression.WriteString(".*") } case '?': expression.WriteString("[^/]") diff --git a/cmd/gogit/config-scope.go b/cmd/gogit/config-scope.go index 5ad95aa..551b218 100644 --- a/cmd/gogit/config-scope.go +++ b/cmd/gogit/config-scope.go @@ -3,8 +3,11 @@ package main import ( "errors" "fmt" + "io" "os" + "os/exec" "path/filepath" + "runtime" "slices" "strconv" "strings" @@ -12,6 +15,8 @@ import ( gitconfig "github.com/go-git/cli/internal/plumbing/format/config" ) +const defaultSystemConfigFile = "/etc/gitconfig" + // gitExitError carries a git-compatible exit status out of a command. msg, when // non-empty, is the single stderr line to print; main must not print the // error itself, because git stays silent for some non-zero statuses (a @@ -72,7 +77,9 @@ func readSources(o *configOpts) ([]configSource, error) { sources := make([]configSource, 0) - if p, ok := systemConfigPath(); ok { + if p, ok, err := systemConfigPath(); err != nil { + return nil, err + } else if ok { src, loaded, err := loadOptionalSource(absoluteFile(p)) if err != nil { return nil, err @@ -144,6 +151,10 @@ func writeTarget(o *configOpts) (configFile, error) { func explicitReadFiles(o *configOpts) ([]configFile, bool, error) { switch { case o.file != "": + if o.file == "-" { + return []configFile{{path: "-", display: "standard input"}}, true, nil + } + return []configFile{absoluteFile(o.file)}, true, nil case o.local: @@ -162,7 +173,11 @@ func explicitReadFiles(o *configOpts) ([]configFile, bool, error) { return files, true, nil case o.system: - p, ok := systemConfigPath() + p, ok, err := systemConfigPath() + if err != nil { + return nil, true, err + } + if !ok { return nil, true, errors.New("system config is disabled by GIT_CONFIG_NOSYSTEM") } @@ -177,6 +192,13 @@ func explicitReadFiles(o *configOpts) ([]configFile, bool, error) { func explicitWriteLocation(o *configOpts) (configFile, bool, error) { switch { case o.file != "": + if o.file == "-" { + return configFile{}, true, &gitExitError{ + code: exitFatal, + msg: "fatal: writing to stdin is not supported", + } + } + return absoluteFile(o.file), true, nil case o.local: @@ -199,7 +221,11 @@ func explicitWriteLocation(o *configOpts) (configFile, bool, error) { return absoluteFile(paths[len(paths)-1]), true, nil case o.system: - p, ok := systemConfigPath() + p, ok, err := systemConfigPath() + if err != nil { + return configFile{}, true, err + } + if !ok { return configFile{}, true, errors.New("system config is disabled by GIT_CONFIG_NOSYSTEM") } @@ -211,6 +237,20 @@ func explicitWriteLocation(o *configOpts) (configFile, bool, error) { } func loadSource(cf configFile) (configSource, error) { + if cf.path == "-" { + data, err := io.ReadAll(os.Stdin) + if err != nil { + return configSource{}, err + } + + f, err := gitconfig.Parse(data) + if err != nil { + return configSource{}, configReadError(cf, err) + } + + return configSource{location: cf, file: f}, nil + } + f, err := gitconfig.ReadFile(cf.path) if err != nil { return configSource{}, configReadError(cf, err) @@ -263,29 +303,85 @@ func globalConfigPaths() []string { // systemConfigPath reports the system config file, and whether the system // scope is enabled at all. -func systemConfigPath() (string, bool) { - if gitEnvBool(os.Getenv("GIT_CONFIG_NOSYSTEM")) { - return "", false +func systemConfigPath() (string, bool, error) { + noSystem, valid := gitBool(os.Getenv("GIT_CONFIG_NOSYSTEM"), false) + if !valid { + return "", false, &gitExitError{ + code: exitFatal, + msg: fmt.Sprintf( + "fatal: bad boolean environment value '%s' for 'GIT_CONFIG_NOSYSTEM'", + os.Getenv("GIT_CONFIG_NOSYSTEM"), + ), + } + } + + if noSystem { + return "", false, nil } if p, ok := os.LookupEnv("GIT_CONFIG_SYSTEM"); ok { if p == "" || p == os.DevNull { - return "", false + return "", false, nil } - return p, true + return p, true, nil } - return "/etc/gitconfig", true + return defaultSystemConfigPath(), true, nil } -func gitEnvBool(value string) bool { - switch strings.ToLower(value) { - case "", "0", "false", "no", "off": +func defaultSystemConfigPath() string { + gitPath, err := exec.LookPath("git") + if err == nil && !sameExecutable(gitPath) { + if path := systemConfigPathForExecutable(runtime.GOOS, gitPath); path != "" { + return path + } + } + + if runtime.GOOS == "windows" { + if programFiles := os.Getenv("PROGRAMFILES"); programFiles != "" { + return filepath.Join(programFiles, "Git", "etc", "gitconfig") + } + } + + return defaultSystemConfigFile +} + +func sameExecutable(path string) bool { + executable, err := os.Executable() + if err != nil { return false + } + + want, err := os.Stat(executable) + if err != nil { + return false + } + + got, err := os.Stat(path) + + return err == nil && os.SameFile(want, got) +} + +func systemConfigPathForExecutable(goos, executable string) string { + dir := filepath.Dir(executable) + + var prefix string + + switch filepath.Base(dir) { + case "bin", "cmd": + prefix = filepath.Dir(dir) + case "git-core": + prefix = filepath.Dir(filepath.Dir(dir)) default: - return true + return "" + } + + if goos != "windows" && prefix == "/usr" { + return defaultSystemConfigFile } + + return filepath.Join(prefix, "etc", "gitconfig") } // localConfigFile returns the repository's config file. For a linked worktree @@ -312,12 +408,33 @@ func localConfigFile() (configFile, error) { } func worktreeConfigFile(local *gitconfig.File) (configFile, bool, error) { - enabled := false - key := gitconfig.Key{Section: "extensions", Name: "worktreeConfig"} + + var ( + value string + implicit, found bool + ) + for _, entry := range local.Entries() { if entry.Key.Matches(key) { - enabled = gitBool(entry.Value, entry.Implicit) + value, implicit, found = entry.Value, entry.Implicit, true + } + } + + enabled := false + + if found { + var valid bool + + enabled, valid = gitBool(value, implicit) + if !valid { + return configFile{}, false, &gitExitError{ + code: exitFatal, + msg: fmt.Sprintf( + "fatal: bad boolean config value '%s' for 'extensions.worktreeconfig'", + value, + ), + } } } @@ -336,19 +453,21 @@ func worktreeConfigFile(local *gitconfig.File) (configFile, bool, error) { }, true, nil } -func gitBool(value string, implicit bool) bool { +func gitBool(value string, implicit bool) (bool, bool) { if implicit { - return true + return true, true } switch strings.ToLower(value) { case "true", "yes", "on", "1": - return true + return true, true + case "", "false", "no", "off", "0": + return false, true } n, err := strconv.ParseInt(value, 10, 64) - return err == nil && n != 0 + return n != 0, err == nil } // commonGitDir follows a linked worktree's `commondir` pointer back to the diff --git a/cmd/gogit/config_cmd_test.go b/cmd/gogit/config_cmd_test.go index df854a8..fdc7535 100644 --- a/cmd/gogit/config_cmd_test.go +++ b/cmd/gogit/config_cmd_test.go @@ -506,6 +506,54 @@ func TestConfigIncludes(t *testing.T) { } } +func TestConfigRejectsRemoteURLFromHasConfigInclude(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + conditional string + nested string + }{ + { + name: "direct", + conditional: "[remote \"other\"]\n\turl = https://other/repo\n[x]\n\ty = included\n", + }, + { + name: "nested", + conditional: "[include]\n\tpath = nested.cfg\n[x]\n\ty = included\n", + nested: "[remote \"other\"]\n\turl = https://other/repo\n", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + repo, home := newConfigRepo(t, `[remote "origin"] + url = https://example/repo +[includeIf "hasconfig:remote.*.url:https://example/**"] + path = conditional.cfg +`) + writeConfig(t, filepath.Join(repo, ".git", "conditional.cfg"), tc.conditional) + + if tc.nested != "" { + writeConfig(t, filepath.Join(repo, ".git", "nested.cfg"), tc.nested) + } + + stdout, stderr, code := runConfig(t, repo, home, cmdConfig, subGet, "x.y") + if code != 128 || stdout != "" { + t.Fatalf("hasconfig include: exit %d, stdout %q, stderr %q", code, stdout, stderr) + } + + want := "fatal: remote URLs cannot be configured in file directly or indirectly " + + "included by includeIf.hasconfig:remote.*.url\n" + if stderr != want { + t.Fatalf("stderr = %q, want %q", stderr, want) + } + }) + } +} + func TestGitWildMatch(t *testing.T) { t.Parallel() @@ -517,6 +565,10 @@ func TestGitWildMatch(t *testing.T) { }{ {pattern: "**/group/**", value: "/tmp/group/repo/.git", want: true}, {pattern: "feature/**", value: "feature/team/topic", want: true}, + {pattern: "a/**/c", value: "a/c", want: true}, + {pattern: "a/**/c", value: "a/b/d/c", want: true}, + {pattern: "a**c", value: "ab/c", want: false}, + {pattern: "a**c", value: "abbc", want: true}, {pattern: "release/[0-9]?", value: "release/12", want: true}, {pattern: "repo", value: "REPO", insensitive: true, want: true}, {pattern: "repo", value: "REPO", want: false}, @@ -633,6 +685,53 @@ func TestConfigFile(t *testing.T) { } } +func TestConfigFileDashUsesStdin(t *testing.T) { + t.Parallel() + + base := t.TempDir() + home := filepath.Join(base, "home") + mkdirAll(t, home) + + stdout, stderr, err := runGogitEnvStdin( + t, + base, + configEnv(home), + "[user]\n\tname = Alice\n", + cmdConfig, + subGet, + flagFile, + "-", + keyUserName, + ) + if err != nil || stdout != "Alice\n" || stderr != "" { + t.Fatalf("stdin read: stdout %q, stderr %q, err %v", stdout, stderr, err) + } + + stdout, stderr, err = runGogitEnvStdin( + t, + base, + configEnv(home), + "", + cmdConfig, + subSet, + flagFile, + "-", + keyUserName, + "Alice", + ) + + var exitErr *exec.ExitError + + if !errors.As(err, &exitErr) || exitErr.ExitCode() != 128 || stdout != "" || + stderr != "fatal: writing to stdin is not supported\n" { + t.Fatalf("stdin write: stdout %q, stderr %q, err %v", stdout, stderr, err) + } + + if _, err := os.Stat(filepath.Join(base, "-")); !os.IsNotExist(err) { + t.Fatalf("stdin write created a literal '-' file: %v", err) + } +} + func TestConfigUnsetRemovesSameLineSection(t *testing.T) { t.Parallel() @@ -762,6 +861,86 @@ func TestConfigNoSystemFalseKeepsSystemScopeEnabled(t *testing.T) { } } +func TestConfigRejectsInvalidBooleans(t *testing.T) { + t.Parallel() + + t.Run("environment", func(t *testing.T) { + t.Parallel() + + repo, home := newConfigRepo(t, "[user]\n\tname = Alice\n") + env := append(configEnv(home), "GIT_CONFIG_NOSYSTEM=maybe") + + stdout, stderr, err := runGogitEnv(t, repo, env, cmdConfig, subGet, keyUserName) + + var exitErr *exec.ExitError + + if !errors.As(err, &exitErr) || exitErr.ExitCode() != 128 || stdout != "" { + t.Fatalf("invalid environment bool: stdout %q, stderr %q, err %v", stdout, stderr, err) + } + + want := "fatal: bad boolean environment value 'maybe' for 'GIT_CONFIG_NOSYSTEM'\n" + if stderr != want { + t.Fatalf("stderr = %q, want %q", stderr, want) + } + }) + + t.Run("worktree extension", func(t *testing.T) { + t.Parallel() + + repo, home := newConfigRepo(t, "[extensions]\n\tworktreeConfig = maybe\n") + + stdout, stderr, code := runConfig(t, repo, home, cmdConfig, subGet, keyUserName) + if code != 128 || stdout != "" { + t.Fatalf("invalid config bool: exit %d, stdout %q, stderr %q", code, stdout, stderr) + } + + want := "fatal: bad boolean config value 'maybe' for 'extensions.worktreeconfig'\n" + if stderr != want { + t.Fatalf("stderr = %q, want %q", stderr, want) + } + }) +} + +func TestSystemConfigPathForGitInstallation(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + goos string + executable string + want string + }{ + { + name: "Homebrew", + goos: "darwin", + executable: "/opt/homebrew/bin/git", + want: "/opt/homebrew/etc/gitconfig", + }, + { + name: "Unix system", + goos: "linux", + executable: "/usr/bin/git", + want: defaultSystemConfigFile, + }, + { + name: "libexec", + goos: "darwin", + executable: "/opt/local/libexec/git-core/git", + want: "/opt/local/etc/gitconfig", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + if got := systemConfigPathForExecutable(tc.goos, tc.executable); got != tc.want { + t.Fatalf("system path = %q, want %q", got, tc.want) + } + }) + } +} + func TestConfigInvalidCombinations(t *testing.T) { t.Parallel() @@ -786,9 +965,9 @@ func TestConfigInvalidCombinations(t *testing.T) { repo, home := newConfigRepo(t, baseConfig) - _, _, code := runConfig(t, repo, home, tc.args...) - if code == 0 { - t.Fatalf("gogit %v unexpectedly succeeded", tc.args) + _, stderr, code := runConfig(t, repo, home, tc.args...) + if code != 129 { + t.Fatalf("gogit %v: exit %d, want 129 (stderr %q)", tc.args, code, stderr) } if got := readFileString(t, filepath.Join(repo, ".git", cmdConfig)); got != baseConfig { diff --git a/cmd/gogit/main_test.go b/cmd/gogit/main_test.go index 0290367..f328b18 100644 --- a/cmd/gogit/main_test.go +++ b/cmd/gogit/main_test.go @@ -70,8 +70,22 @@ func runGogitEnv(t *testing.T, dir string, env []string, args ...string) (string func runGogitStdin(t *testing.T, dir string, stdin string, args ...string) (string, string, error) { t.Helper() + return runGogitEnvStdin(t, dir, nil, stdin, args...) +} + +func runGogitEnvStdin( + t *testing.T, + dir string, + env []string, + stdin string, + args ...string, +) (string, string, error) { + t.Helper() + cmd := exec.Command(gogitBin, args...) cmd.Dir = dir + + cmd.Env = append(os.Environ(), env...) cmd.Stdin = strings.NewReader(stdin) var stdout, stderr bytes.Buffer From 862151d6d7675267b33c75be00510dc1090b8348 Mon Sep 17 00:00:00 2001 From: Muskan Paliwal Date: Mon, 31 Aug 2026 20:47:42 +0530 Subject: [PATCH 8/8] fix(config): match Git config edge cases Entire-Checkpoint: 0825631a1f33 --- cmd/gogit/config-cmd.go | 6 + cmd/gogit/config-include.go | 261 +++++++++++++------ cmd/gogit/config-scope.go | 127 ++++++--- cmd/gogit/config.go | 32 ++- cmd/gogit/config_cmd_test.go | 144 +++++++++- cmd/gogit/gitdir.go | 34 ++- internal/plumbing/format/config/file.go | 39 ++- internal/plumbing/format/config/file_test.go | 26 ++ internal/plumbing/format/config/parser.go | 4 + 9 files changed, 547 insertions(+), 126 deletions(-) diff --git a/cmd/gogit/config-cmd.go b/cmd/gogit/config-cmd.go index 3845591..801cff5 100644 --- a/cmd/gogit/config-cmd.go +++ b/cmd/gogit/config-cmd.go @@ -60,6 +60,12 @@ func (o *configOpts) registerLocation(cmd *cobra.Command) { } func init() { + for _, cmd := range []*cobra.Command{configCmd, configGetCmd, configSetCmd, configUnsetCmd} { + cmd.SetFlagErrorFunc(func(_ *cobra.Command, err error) error { + return usageError(err.Error()) + }) + } + legacyOpts.registerLocation(configCmd) configCmd.Flags().BoolVar(&legacyOpts.path, "path", false, "Canonicalize the value as a path, expanding a leading ~") diff --git a/cmd/gogit/config-include.go b/cmd/gogit/config-include.go index 2534dbb..b812b9d 100644 --- a/cmd/gogit/config-include.go +++ b/cmd/gogit/config-include.go @@ -5,7 +5,6 @@ import ( "fmt" "os" "path/filepath" - "regexp" "strings" gitconfig "github.com/go-git/cli/internal/plumbing/format/config" @@ -53,21 +52,9 @@ func sourceValues( depth int, includedByHasConfig bool, ) ([]string, error) { - if source.file == nil { - var values []string - - for _, override := range source.overrides { - if override.key.Matches(key) { - values = append(values, override.value) - } - } - - return values, nil - } - var values []string - for _, entry := range source.file.Entries() { + for _, entry := range sourceEntries(source) { if includedByHasConfig && isRemoteURL(entry.Key) { return nil, &gitExitError{ code: exitFatal, @@ -114,6 +101,21 @@ func sourceValues( return values, nil } +func sourceEntries(source configSource) []gitconfig.Entry { + if source.file != nil { + return source.file.Entries() + } + + entries := make([]gitconfig.Entry, 0, len(source.overrides)) + for _, override := range source.overrides { + entries = append(entries, gitconfig.Entry{ + Key: override.key, Value: override.value, Implicit: override.implicit, + }) + } + + return entries +} + func includedSource( parent configSource, entry gitconfig.Entry, @@ -237,77 +239,194 @@ func matchGitDirs(pattern, sourcePath string, gitDirs []string, insensitive bool } func gitWildMatch(pattern, value string, insensitive bool) bool { - var expression strings.Builder - expression.WriteByte('^') - - for i := 0; i < len(pattern); { - switch pattern[i] { - case '*': - start := i - for i < len(pattern) && pattern[i] == '*' { - i++ - } + matcher := wildMatcher{ + pattern: pattern, + value: value, + insensitive: insensitive, + memo: map[wildPosition]bool{}, + } - doubleStar := i-start >= 2 && (start == 0 || pattern[start-1] == '/') && - (i == len(pattern) || pattern[i] == '/') - if !doubleStar { - expression.WriteString("[^/]*") + return matcher.match(0, 0) +} - continue - } +type wildPosition struct{ pattern, value int } - if i < len(pattern) { - expression.WriteString("(?:.*/)?") +type wildMatcher struct { + pattern string + value string + insensitive bool + memo map[wildPosition]bool +} - i++ - } else { - expression.WriteString(".*") - } - case '?': - expression.WriteString("[^/]") +func (m *wildMatcher) match(patternPos, valuePos int) bool { + state := wildPosition{patternPos, valuePos} + if matched, ok := m.memo[state]; ok { + return matched + } - i++ - case '[': - end := strings.IndexByte(pattern[i+1:], ']') - if end < 0 { - expression.WriteString(`\[`) + matched := m.matchPosition(patternPos, valuePos) + m.memo[state] = matched - i++ + return matched +} - continue - } +func (m *wildMatcher) matchPosition(patternPos, valuePos int) bool { + if patternPos == len(m.pattern) { + return valuePos == len(m.value) + } + + switch m.pattern[patternPos] { + case '*': + return m.matchStar(patternPos, valuePos) + case '?': + return valuePos < len(m.value) && m.value[valuePos] != '/' && + m.match(patternPos+1, valuePos+1) + case '[': + end, classMatched, ok := matchWildClass( + m.pattern, patternPos, m.value, valuePos, m.insensitive, + ) + if ok { + return classMatched && m.match(end, valuePos+1) + } + } - end += i + 1 - class := pattern[i+1 : end] + return valuePos < len(m.value) && + equalWildByte(m.pattern[patternPos], m.value[valuePos], m.insensitive) && + m.match(patternPos+1, valuePos+1) +} - expression.WriteByte('[') +func (m *wildMatcher) matchStar(patternPos, valuePos int) bool { + end := patternPos + for end < len(m.pattern) && m.pattern[end] == '*' { + end++ + } - if strings.HasPrefix(class, "!") { - expression.WriteByte('^') + doubleStar := end-patternPos >= 2 && (patternPos == 0 || m.pattern[patternPos-1] == '/') && + (end == len(m.pattern) || m.pattern[end] == '/') + if doubleStar { + return m.matchDoubleStar(end, valuePos) + } - class = class[1:] - } + for i := valuePos; ; i++ { + if m.match(end, i) { + return true + } + + if i == len(m.value) || m.value[i] == '/' { + return false + } + } +} + +func (m *wildMatcher) matchDoubleStar(patternEnd, valuePos int) bool { + if patternEnd == len(m.pattern) { + return true + } + + if m.match(patternEnd+1, valuePos) { + return true + } + + for i := valuePos; i < len(m.value); i++ { + if m.value[i] == '/' && m.match(patternEnd+1, i+1) { + return true + } + } + + return false +} + +func matchWildClass( + pattern string, + patternPos int, + value string, + valuePos int, + insensitive bool, +) (int, bool, bool) { + if valuePos >= len(value) || value[valuePos] == '/' { + return 0, false, false + } + + i := patternPos + 1 + negated := false + + if i < len(pattern) && (pattern[i] == '!' || pattern[i] == '^') { + negated = true + i++ + } - expression.WriteString(strings.ReplaceAll(class, `\`, `\\`)) - expression.WriteByte(']') + classStart := i + if i < len(pattern) && pattern[i] == ']' { + i++ + } - i = end + 1 - default: - expression.WriteString(regexp.QuoteMeta(pattern[i : i+1])) + for i < len(pattern) && pattern[i] != ']' { + if pattern[i] == '\\' && i+1 < len(pattern) { + i += 2 + } else { i++ } } - expression.WriteByte('$') + if i == len(pattern) { + return 0, false, false + } + + matched := wildClassContains(pattern[classStart:i], value[valuePos], insensitive) + if negated { + matched = !matched + } + + return i + 1, matched, true +} + +func wildClassContains(class string, value byte, insensitive bool) bool { + for i := 0; i < len(class); { + start := class[i] + if start == '\\' && i+1 < len(class) { + i++ + start = class[i] + } + + i++ + + if i+1 < len(class) && class[i] == '-' { + i++ + + end := class[i] + if end == '\\' && i+1 < len(class) { + i++ + end = class[i] + } + + i++ + + candidate := foldWildByte(value, insensitive) + if candidate >= foldWildByte(start, insensitive) && candidate <= foldWildByte(end, insensitive) { + return true + } + + continue + } - patternExpression := expression.String() - if insensitive { - patternExpression = "(?i:" + patternExpression + ")" + if equalWildByte(start, value, insensitive) { + return true + } } - compiled, err := regexp.Compile(patternExpression) + return false +} + +func equalWildByte(left, right byte, insensitive bool) bool { + return foldWildByte(left, insensitive) == foldWildByte(right, insensitive) +} - return err == nil && compiled.MatchString(value) +func foldWildByte(value byte, insensitive bool) byte { + if insensitive && value >= 'A' && value <= 'Z' { + return value + ('a' - 'A') + } + + return value } func resolveIncludePath(path, sourcePath string) (string, error) { @@ -380,21 +499,9 @@ func sourceRemoteURLs( stack map[string]bool, depth int, ) ([]string, error) { - if source.file == nil { - var urls []string - - for _, override := range source.overrides { - if isRemoteURL(override.key) { - urls = append(urls, override.value) - } - } - - return urls, nil - } - var urls []string - for _, entry := range source.file.Entries() { + for _, entry := range sourceEntries(source) { if isRemoteURL(entry.Key) { urls = append(urls, entry.Value) } diff --git a/cmd/gogit/config-scope.go b/cmd/gogit/config-scope.go index 551b218..0a2cb48 100644 --- a/cmd/gogit/config-scope.go +++ b/cmd/gogit/config-scope.go @@ -59,8 +59,12 @@ type configSource struct { // A location flag limits the sources to one scope. Otherwise git's default // precedence applies. func readSources(o *configOpts) ([]configSource, error) { - if files, ok, err := explicitReadFiles(o); err != nil { + if err := validateNoSystem(); err != nil { return nil, err + } + + if files, ok, err := explicitReadFiles(o); err != nil { + return nil, configRepositoryError(err) } else if ok { sources := make([]configSource, 0, len(files)) for _, file := range files { @@ -103,31 +107,60 @@ func readSources(o *configOpts) ([]configSource, error) { } } - // Being outside a repository is not an error for a default read: -c - // overrides and the global files still apply, as they do in git. - if f, err := localConfigFile(); err == nil { - src, err := loadSource(f) + sources, err := appendLocalConfigSources(sources) + if err != nil { + return nil, err + } + + if len(configOverrideList) > 0 { + cwd, err := os.Getwd() if err != nil { return nil, err } - src.includes = true - sources = append(sources, src) + sources = append(sources, configSource{ + location: configFile{path: filepath.Join(cwd, ".gitconfig-command-line")}, + overrides: configOverrideList, + includes: true, + }) + } - if worktree, enabled, err := worktreeConfigFile(src.file); err != nil { - return nil, err - } else if enabled { - worktreeSource, err := loadSource(worktree) - if err != nil { - return nil, err - } + return sources, nil +} - worktreeSource.includes = true - sources = append(sources, worktreeSource) - } +func appendLocalConfigSources(sources []configSource) ([]configSource, error) { + // Being outside a repository is not an error for a default read: -c + // overrides and the global files still apply, as they do in git. + file, err := localConfigFile() + if errors.Is(err, errNoRepository) { + return sources, nil + } + + if err != nil { + return nil, configRepositoryError(err) + } + + source, err := loadSource(file) + if err != nil { + return nil, err + } + + source.includes = true + sources = append(sources, source) + + worktree, enabled, err := worktreeConfigFile(source.file) + if err != nil || !enabled { + return sources, err + } + + worktreeSource, err := loadSource(worktree) + if err != nil { + return nil, err } - return append(sources, configSource{overrides: configOverrideList}), nil + worktreeSource.includes = true + + return append(sources, worktreeSource), nil } // absoluteFile names a file that git reports by its full path, which is how @@ -139,13 +172,32 @@ func absoluteFile(path string) configFile { // writeTarget returns the single file a mutation applies to. Writes default // to the repository config, never to the merged view. func writeTarget(o *configOpts) (configFile, error) { - if file, ok, err := explicitWriteLocation(o); err != nil { + if err := validateNoSystem(); err != nil { return configFile{}, err + } + + if file, ok, err := explicitWriteLocation(o); err != nil { + return configFile{}, configRepositoryError(err) } else if ok { return file, nil } - return localConfigFile() + file, err := localConfigFile() + + return file, configRepositoryError(err) +} + +func configRepositoryError(err error) error { + if err == nil { + return nil + } + + var exitErr *gitExitError + if errors.As(err, &exitErr) { + return err + } + + return &gitExitError{code: exitFatal, msg: "fatal: " + err.Error()} } func explicitReadFiles(o *configOpts) ([]configFile, bool, error) { @@ -304,15 +356,9 @@ func globalConfigPaths() []string { // systemConfigPath reports the system config file, and whether the system // scope is enabled at all. func systemConfigPath() (string, bool, error) { - noSystem, valid := gitBool(os.Getenv("GIT_CONFIG_NOSYSTEM"), false) - if !valid { - return "", false, &gitExitError{ - code: exitFatal, - msg: fmt.Sprintf( - "fatal: bad boolean environment value '%s' for 'GIT_CONFIG_NOSYSTEM'", - os.Getenv("GIT_CONFIG_NOSYSTEM"), - ), - } + noSystem, err := noSystemConfig() + if err != nil { + return "", false, err } if noSystem { @@ -330,6 +376,29 @@ func systemConfigPath() (string, bool, error) { return defaultSystemConfigPath(), true, nil } +func validateNoSystem() error { + _, err := noSystemConfig() + + return err +} + +func noSystemConfig() (bool, error) { + value := os.Getenv("GIT_CONFIG_NOSYSTEM") + noSystem, valid := gitBool(value, false) + + if !valid { + return false, &gitExitError{ + code: exitFatal, + msg: fmt.Sprintf( + "fatal: bad boolean environment value '%s' for 'GIT_CONFIG_NOSYSTEM'", + value, + ), + } + } + + return noSystem, nil +} + func defaultSystemConfigPath() string { gitPath, err := exec.LookPath("git") if err == nil && !sameExecutable(gitPath) { diff --git a/cmd/gogit/config.go b/cmd/gogit/config.go index 32fa480..cfb6802 100644 --- a/cmd/gogit/config.go +++ b/cmd/gogit/config.go @@ -12,6 +12,7 @@ import ( var ( configOverridesRaw []string configOverrides = map[string]string{} + configImplicit = map[string]bool{} configOverrideMu sync.Mutex // configOverrideList keeps the -c overrides in the order they were given @@ -23,8 +24,9 @@ var ( // configOverride is a single -c key=value pair with its key parsed. type configOverride struct { - key gitconfig.Key - value string + key gitconfig.Key + value string + implicit bool } // splitKV splits "=" into (key, value, true). Invalid input @@ -39,10 +41,15 @@ func splitKV(s string) (string, string, bool) { } func applyConfigOverride(key, value string) { + applyConfigOverrideValue(key, value, false) +} + +func applyConfigOverrideValue(key, value string, implicit bool) { configOverrideMu.Lock() defer configOverrideMu.Unlock() configOverrides[key] = value + configImplicit[key] = implicit } func resetConfigOverrides() { @@ -50,6 +57,7 @@ func resetConfigOverrides() { defer configOverrideMu.Unlock() configOverrides = map[string]string{} + configImplicit = map[string]bool{} configOverridesRaw = nil configOverrideList = nil } @@ -59,8 +67,15 @@ func resetConfigOverrides() { func applyConfigOverridesFromFlags() error { for _, raw := range configOverridesRaw { k, v, ok := splitKV(raw) + implicit := !ok + if !ok { - return fmt.Errorf("invalid -c value %q (want key=value)", raw) + k = raw + v = "" + + if k == "" { + return fmt.Errorf("invalid -c value %q", raw) + } } key, kerr := gitconfig.ParseKey(k) @@ -68,9 +83,11 @@ func applyConfigOverridesFromFlags() error { return fmt.Errorf("invalid -c value %q: %w", raw, kerr) } - configOverrideList = append(configOverrideList, configOverride{key: key, value: v}) + configOverrideList = append(configOverrideList, configOverride{ + key: key, value: v, implicit: implicit, + }) - applyConfigOverride(k, v) + applyConfigOverrideValue(k, v, implicit) } return nil @@ -93,11 +110,16 @@ func hasConfigOverride(key string) bool { func configBool(key string, repoCfg *config.Config, defaultVal bool) bool { configOverrideMu.Lock() v, ok := configOverrides[key] + implicit := configImplicit[key] configOverrideMu.Unlock() _ = repoCfg if ok { + if implicit { + return true + } + return strings.EqualFold(v, "true") } diff --git a/cmd/gogit/config_cmd_test.go b/cmd/gogit/config_cmd_test.go index fdc7535..1391752 100644 --- a/cmd/gogit/config_cmd_test.go +++ b/cmd/gogit/config_cmd_test.go @@ -11,11 +11,13 @@ import ( "testing" ) +const emptyXDGConfigHome = "XDG_CONFIG_HOME=" + // configEnv isolates a test from the developer's real configuration: HOME // points at a scratch directory and the system config is switched off, the // same way upstream's test-lib.sh does it. func configEnv(home string) []string { - return []string{"HOME=" + home, "GIT_CONFIG_NOSYSTEM=1", "XDG_CONFIG_HOME="} + return []string{"HOME=" + home, "GIT_CONFIG_NOSYSTEM=1", emptyXDGConfigHome} } // runConfig runs gogit in dir and returns stdout, stderr and the exit status. @@ -445,6 +447,10 @@ func TestConfigScopePrecedence(t *testing.T) { name: "-c can set an empty value", args: []string{"-c", "pr.k=", cmdConfig, subGet, keyPr}, want: "\n", }, + { + name: "-c accepts an implicit boolean", + args: []string{"-c", "feature.enabled", cmdConfig, subGet, "feature.enabled"}, want: "\n", + }, } for _, tc := range tests { @@ -506,6 +512,44 @@ func TestConfigIncludes(t *testing.T) { } } +func TestConfigCommandLineIncludes(t *testing.T) { + t.Parallel() + + repo, home := newConfigRepo(t, "") + included := filepath.Join(repo, "command-line.cfg") + writeConfig(t, included, "[x]\n\ty = from-include\n") + + tests := []struct { + name string + override string + }{ + {name: "unconditional", override: "include.path=" + included}, + {name: "relative to working directory", override: "include.path=command-line.cfg"}, + {name: "conditional", override: "includeIf.onbranch:main.path=" + included}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + stdout, stderr, code := runConfig(t, repo, home, + "-c", tc.override, cmdConfig, subGet, "x.y") + if code != 0 || stdout != "from-include\n" { + t.Fatalf("command-line include: exit %d, stdout %q, stderr %q", code, stdout, stderr) + } + }) + } + + stdout, stderr, code := runConfig(t, repo, home, + "-c", "x.y=before", + "-c", "include.path="+included, + "-c", "x.y=after", + cmdConfig, subGet, flagAll, "x.y") + if code != 0 || stdout != "before\nfrom-include\nafter\n" { + t.Fatalf("ordered command-line include: exit %d, stdout %q, stderr %q", code, stdout, stderr) + } +} + func TestConfigRejectsRemoteURLFromHasConfigInclude(t *testing.T) { t.Parallel() @@ -570,6 +614,9 @@ func TestGitWildMatch(t *testing.T) { {pattern: "a**c", value: "ab/c", want: false}, {pattern: "a**c", value: "abbc", want: true}, {pattern: "release/[0-9]?", value: "release/12", want: true}, + {pattern: "a[!x]b", value: "a/b", want: false}, + {pattern: "a[!x]b", value: "ayb", want: true}, + {pattern: "a[]]b", value: "a]b", want: true}, {pattern: "repo", value: "REPO", insensitive: true, want: true}, {pattern: "repo", value: "REPO", want: false}, } @@ -846,7 +893,7 @@ func TestConfigNoSystemFalseKeepsSystemScopeEnabled(t *testing.T) { env := []string{ "HOME=" + home, - "XDG_CONFIG_HOME=", + emptyXDGConfigHome, "GIT_CONFIG_NOSYSTEM=false", "GIT_CONFIG_SYSTEM=" + system, } @@ -901,6 +948,33 @@ func TestConfigRejectsInvalidBooleans(t *testing.T) { }) } +func TestConfigRejectsInvalidNoSystemBeforeExplicitWrite(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + home := filepath.Join(dir, "home") + mkdirAll(t, home) + + target := filepath.Join(dir, "config") + env := []string{"HOME=" + home, emptyXDGConfigHome, "GIT_CONFIG_NOSYSTEM=maybe"} + + stdout, stderr, err := runGogitEnv(t, dir, env, + cmdConfig, subSet, flagFile, target, "x.y", "value") + + var exitErr *exec.ExitError + if !errors.As(err, &exitErr) || exitErr.ExitCode() != 128 || stdout != "" { + t.Fatalf("invalid environment bool: stdout %q, stderr %q, err %v", stdout, stderr, err) + } + + if want := "fatal: bad boolean environment value 'maybe' for 'GIT_CONFIG_NOSYSTEM'\n"; stderr != want { + t.Fatalf("stderr = %q, want %q", stderr, want) + } + + if _, err := os.Stat(target); !os.IsNotExist(err) { + t.Fatalf("explicit config file was created: %v", err) + } +} + func TestSystemConfigPathForGitInstallation(t *testing.T) { t.Parallel() @@ -977,6 +1051,22 @@ func TestConfigInvalidCombinations(t *testing.T) { } } +func TestConfigFlagErrorsUseUsageStatus(t *testing.T) { + t.Parallel() + + repo, home := newConfigRepo(t, baseConfig) + + for _, args := range [][]string{ + {cmdConfig, "--definitely-invalid"}, + {cmdConfig, subGet, "--definitely-invalid", keyUserName}, + } { + _, stderr, code := runConfig(t, repo, home, args...) + if code != 129 { + t.Errorf("gogit %v: exit %d, want 129 (stderr %q)", args, code, stderr) + } + } +} + func TestConfigLinkedWorktree(t *testing.T) { t.Parallel() @@ -1075,6 +1165,56 @@ func TestConfigOutsideRepository(t *testing.T) { } } +func TestConfigRejectsMalformedGitFile(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + gitfile string + want func(string) string + }{ + { + name: "invalid format", + gitfile: "not a gitfile\n", + want: func(repo string) string { + return "fatal: invalid gitfile format: " + filepath.Join(repo, ".git") + "\n" + }, + }, + { + name: "missing target", + gitfile: "gitdir: missing\n", + want: func(repo string) string { + return "fatal: not a git repository: " + filepath.Join(repo, "missing") + "\n" + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + base := t.TempDir() + repo := filepath.Join(base, "repo") + home := filepath.Join(base, "home") + + mkdirAll(t, repo) + mkdirAll(t, home) + writeConfig(t, filepath.Join(repo, ".git"), tc.gitfile) + writeConfig(t, filepath.Join(home, ".gitconfig"), "[user]\n\tname = GLOBAL\n") + + resolvedRepo, err := filepath.EvalSymlinks(repo) + if err != nil { + t.Fatal(err) + } + + stdout, stderr, code := runConfig(t, repo, home, cmdConfig, subGet, keyUserName) + if code != 128 || stdout != "" || stderr != tc.want(resolvedRepo) { + t.Fatalf("malformed gitfile: exit %d, stdout %q, stderr %q", code, stdout, stderr) + } + }) + } +} + // TestConfigDiagnosticPaths pins how the config file is named in a // diagnostic. git reports the path exactly as it resolved it rather than // absolutising it, so the spelling depends on how the repository was found. diff --git a/cmd/gogit/gitdir.go b/cmd/gogit/gitdir.go index 85f0d35..7aa75e4 100644 --- a/cmd/gogit/gitdir.go +++ b/cmd/gogit/gitdir.go @@ -2,6 +2,7 @@ package main import ( "errors" + "fmt" "os" "path/filepath" "strings" @@ -10,6 +11,8 @@ import ( // gitDirName is the name of a repository's git directory inside a work tree. const gitDirName = ".git" +var errNoRepository = errors.New("not a git repository") + // findGitDir locates the repository's git directory. func findGitDir() (string, error) { path, _, err := discoverGitDir() @@ -62,7 +65,7 @@ func discoverGitDir() (string, string, error) { parent := filepath.Dir(dir) if parent == dir { - return "", "", errors.New("not a git repository") + return "", "", errNoRepository } dir = parent @@ -78,19 +81,42 @@ func readGitFile(path string) (string, error) { target, ok := strings.CutPrefix(strings.TrimSpace(string(data)), "gitdir:") if !ok { - return "", errors.New("not a git repository") + return "", fmt.Errorf("invalid gitfile format: %s", path) } target = strings.TrimSpace(target) if target == "" { - return "", errors.New("not a git repository") + return "", fmt.Errorf("invalid gitfile format: %s", path) } if !filepath.IsAbs(target) { target = filepath.Join(filepath.Dir(path), target) } - return filepath.Clean(target), nil + target = filepath.Clean(target) + if !isGitDir(target) && !isLinkedWorktreeGitDir(target) { + return "", fmt.Errorf("not a git repository: %s", target) + } + + return target, nil +} + +func isLinkedWorktreeGitDir(dir string) bool { + data, err := os.ReadFile(filepath.Join(dir, "commondir")) + if err != nil { + return false + } + + common := strings.TrimSpace(string(data)) + if common == "" { + return false + } + + if !filepath.IsAbs(common) { + common = filepath.Join(dir, common) + } + + return isGitDir(filepath.Clean(common)) } // isGitDir reports whether dir is itself a git directory, which is how a bare diff --git a/internal/plumbing/format/config/file.go b/internal/plumbing/format/config/file.go index 8336e55..0fe4cb8 100644 --- a/internal/plumbing/format/config/file.go +++ b/internal/plumbing/format/config/file.go @@ -180,7 +180,7 @@ func (f *File) ReplaceAll(key Key, value string) error { // ones, preserving the section ordering around the surviving value. edits := []edit{f.writeEdit(found[len(found)-1], key, value)} for _, o := range found[:len(found)-1] { - edits = append(edits, deleteEdit(o)) + edits = append(edits, f.deleteEdit(o)) } return f.apply(edits) @@ -200,7 +200,7 @@ func (f *File) UnsetAll(key Key) (int, error) { for _, o := range f.options { if o.key.Matches(key) { - edits = append(edits, deleteEdit(o)) + edits = append(edits, f.deleteEdit(o)) n++ } } @@ -209,7 +209,14 @@ func (f *File) UnsetAll(key Key) (int, error) { return 0, nil } - return n, f.apply(append(edits, f.emptySectionEdits(edits)...)) + sectionEdits := f.emptySectionEdits(edits) + for _, sectionEdit := range sectionEdits { + edits = slices.DeleteFunc(edits, func(e edit) bool { + return e.start >= sectionEdit.start && e.end <= sectionEdit.end + }) + } + + return n, f.apply(append(edits, sectionEdits...)) } // emptySectionEdits returns the header lines that become empty once edits are @@ -251,7 +258,7 @@ func (f *File) emptySectionEdits(deletions []edit) []edit { allPlain := true for _, candidate := range group { - if !f.sections[candidate].plain { + if !f.sectionPlainAfterDeletions(candidate, deletions) { allPlain = false break @@ -271,9 +278,25 @@ func (f *File) emptySectionEdits(deletions []edit) []edit { return out } +func (f *File) sectionPlainAfterDeletions(section int, deletions []edit) bool { + if f.sections[section].plain { + return true + } + + for _, o := range f.options { + if o.secIdx == section && !o.alone { + return isDeletedOption(o, deletions) + } + } + + return false +} + func isDeletedOption(o *optionRec, deletions []edit) bool { + nameStart := o.nameEnd - len(o.key.Name) + for _, d := range deletions { - if o.lineStart >= d.start && o.lineEnd <= d.end { + if nameStart >= d.start && o.logicalEnd <= d.end { return true } } @@ -375,14 +398,12 @@ type edit struct { text string } -func deleteEdit(o *optionRec) edit { +func (f *File) deleteEdit(o *optionRec) edit { if o.alone { return edit{start: o.lineStart, end: o.lineEnd} } - // A section header and its only same-line variable are one logical entry to - // git's unset operation, so remove the whole physical line. - return edit{start: o.lineStart, end: o.lineEnd} + return edit{start: f.sections[o.secIdx].headerEnd, end: o.logicalEnd} } // apply splices edits into the document and re-parses, so recorded offsets diff --git a/internal/plumbing/format/config/file_test.go b/internal/plumbing/format/config/file_test.go index 1aed709..71e055c 100644 --- a/internal/plumbing/format/config/file_test.go +++ b/internal/plumbing/format/config/file_test.go @@ -318,6 +318,32 @@ func TestUnsetKeepsRepeatedSectionHeadersWhenGroupHasValues(t *testing.T) { } } +func TestUnsetKeepsHeaderSharedWithRemovedOption(t *testing.T) { + t.Parallel() + + f := mustParse(t, "[section] one = 1\n\ttwo = 2\n") + if _, err := f.UnsetAll(mustKey(t, "section.one")); err != nil { + t.Fatal(err) + } + + if got, want := string(f.Bytes()), "[section]\n\ttwo = 2\n"; got != want { + t.Fatalf("UnsetAll = %q, want %q", got, want) + } +} + +func TestUnsetRemovesHeaderSharedWithOnlyOption(t *testing.T) { + t.Parallel() + + f := mustParse(t, "[section] one = 1\n") + if _, err := f.UnsetAll(mustKey(t, "section.one")); err != nil { + t.Fatal(err) + } + + if got := string(f.Bytes()); got != "" { + t.Fatalf("UnsetAll = %q, want empty file", got) + } +} + func TestUnsetRemovesAllEmptyRepeatedSections(t *testing.T) { t.Parallel() diff --git a/internal/plumbing/format/config/parser.go b/internal/plumbing/format/config/parser.go index 73b5798..e9bad9d 100644 --- a/internal/plumbing/format/config/parser.go +++ b/internal/plumbing/format/config/parser.go @@ -245,6 +245,10 @@ func (p *parser) parseVariable(lineStart int, alone bool) error { p.consumeLineEnd() o.lineEnd = p.pos + if !alone { + p.curSec.lineEnd = p.pos + } + o.secIdx = len(p.f.sections) - 1 p.f.options = append(p.f.options, o) p.curSec.entryEnd = p.pos