From 4017e466e49817720a2d43ae45a0bc99ec33764b Mon Sep 17 00:00:00 2001 From: Eric Curtin Date: Tue, 28 Apr 2026 13:01:42 +0100 Subject: [PATCH 01/10] add model-cli config command with INI file support Introduce 'model-cli config' as a new top-level command with an interface and file format inspired by, but not referencing, 'git config'. - New cmd/cli/iniconfig package: parses and writes INI-style config files (section headers, subsections, boolean keys, inline comments, backslash escapes, quoted values, UTF-8 BOM). Writes are atomic via .lock + rename. - New 'config' command with subcommands: get, set, unset, list, edit. All subcommands accept --global (default per XDG_CONFIG_HOME or ~/.config/model-runner/config), --system (/etc/model-runner/config), and --file/-f flags. - Remove the 'config' alias from 'configure' to avoid a name collision; 'configure' remains hidden and undocumented for existing callers. - 'config' requires no running model-runner instance (pure local file I/O) and is registered outside the withStandaloneRunner group. - Parser: handle trailing comments on section headers ([core] # comment), raise a clear error on lines exceeding 1 MiB, preserve existing file permissions on write (default 0600 for new files). - Editor: split VISUAL/EDITOR on whitespace to support values like 'code --wait'. - Regenerate CLI reference docs. --- cmd/cli/commands/config.go | 351 +++++++++++ cmd/cli/commands/configure.go | 7 +- cmd/cli/commands/root.go | 1 + cmd/cli/docs/reference/docker_model.yaml | 2 + .../docs/reference/docker_model_config.yaml | 44 ++ .../reference/docker_model_config_edit.yaml | 48 ++ .../reference/docker_model_config_get.yaml | 79 +++ .../reference/docker_model_config_list.yaml | 55 ++ .../reference/docker_model_config_set.yaml | 47 ++ .../reference/docker_model_config_unset.yaml | 44 ++ .../reference/docker_model_configure.yaml | 1 - cmd/cli/docs/reference/model.md | 1 + cmd/cli/docs/reference/model_config.md | 38 ++ cmd/cli/docs/reference/model_config_edit.md | 19 + cmd/cli/docs/reference/model_config_get.md | 24 + cmd/cli/docs/reference/model_config_list.md | 21 + cmd/cli/docs/reference/model_config_set.md | 18 + cmd/cli/docs/reference/model_config_unset.md | 16 + cmd/cli/iniconfig/iniconfig.go | 565 ++++++++++++++++++ cmd/cli/iniconfig/iniconfig_test.go | 322 ++++++++++ cmd/cli/iniconfig/testmain_test.go | 12 + 21 files changed, 1710 insertions(+), 5 deletions(-) create mode 100644 cmd/cli/commands/config.go create mode 100644 cmd/cli/docs/reference/docker_model_config.yaml create mode 100644 cmd/cli/docs/reference/docker_model_config_edit.yaml create mode 100644 cmd/cli/docs/reference/docker_model_config_get.yaml create mode 100644 cmd/cli/docs/reference/docker_model_config_list.yaml create mode 100644 cmd/cli/docs/reference/docker_model_config_set.yaml create mode 100644 cmd/cli/docs/reference/docker_model_config_unset.yaml create mode 100644 cmd/cli/docs/reference/model_config.md create mode 100644 cmd/cli/docs/reference/model_config_edit.md create mode 100644 cmd/cli/docs/reference/model_config_get.md create mode 100644 cmd/cli/docs/reference/model_config_list.md create mode 100644 cmd/cli/docs/reference/model_config_set.md create mode 100644 cmd/cli/docs/reference/model_config_unset.md create mode 100644 cmd/cli/iniconfig/iniconfig.go create mode 100644 cmd/cli/iniconfig/iniconfig_test.go create mode 100644 cmd/cli/iniconfig/testmain_test.go diff --git a/cmd/cli/commands/config.go b/cmd/cli/commands/config.go new file mode 100644 index 000000000..f7a9a775b --- /dev/null +++ b/cmd/cli/commands/config.go @@ -0,0 +1,351 @@ +package commands + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + + "github.com/docker/model-runner/cmd/cli/iniconfig" + "github.com/spf13/cobra" +) + +// defaultConfigPath returns the default (global/user-level) config file path. +// It honours XDG_CONFIG_HOME when set: +// +// $XDG_CONFIG_HOME/model-runner/config +// ~/.config/model-runner/config (fallback) +func defaultConfigPath() string { + if xdg := os.Getenv("XDG_CONFIG_HOME"); xdg != "" { + return filepath.Join(xdg, "model-runner", "config") + } + home, err := os.UserHomeDir() + if err != nil { + return filepath.Join(".config", "model-runner", "config") + } + return filepath.Join(home, ".config", "model-runner", "config") +} + +// systemConfigPath returns the system-wide config file path. +func systemConfigPath() string { + if runtime.GOOS == "windows" { + if pd := os.Getenv("ProgramData"); pd != "" { + return filepath.Join(pd, "model-runner", "config") + } + return `C:\ProgramData\model-runner\config` + } + return "/etc/model-runner/config" +} + +// resolveConfigPath picks the config file to operate on, given the flags. +// Exactly one of global, system, or file may be set. +func resolveConfigPath(global, system bool, file string) (string, error) { + count := 0 + if global { + count++ + } + if system { + count++ + } + if file != "" { + count++ + } + if count > 1 { + return "", fmt.Errorf("only one of --global, --system, or --file may be specified") + } + switch { + case system: + return systemConfigPath(), nil + case file != "": + return file, nil + default: + // --global is the default + return defaultConfigPath(), nil + } +} + +// addLocationFlags adds the standard --global/--system/--file flags to a command. +func addLocationFlags(cmd *cobra.Command, global, system *bool, file *string) { + cmd.Flags().BoolVar(global, "global", false, "use the global (user-level) config file") + cmd.Flags().BoolVar(system, "system", false, "use the system-wide config file") + cmd.Flags().StringVarP(file, "file", "f", "", "use a specific config file") +} + +// newConfigCmd returns the top-level "config" command. +func newConfigCmd() *cobra.Command { + c := &cobra.Command{ + Use: "config", + Short: "Read and write model-runner config file values", + Long: `Read and write model-runner config file values. + +The config file uses an INI format with sections and key=value pairs: + + [section] + key = value + [section "subsection"] + key = value + +Keys are specified in dot notation: section.key or section.subsection.key. + +The default file is $XDG_CONFIG_HOME/model-runner/config, falling back to +~/.config/model-runner/config when XDG_CONFIG_HOME is not set. + +Examples: + model-cli config set user.name "Alice" + model-cli config get user.name + model-cli config list + model-cli config unset user.name + model-cli config edit`, + // Do not run a PersistentPreRunE that requires a running model-runner; + // config is pure local-file work. + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { + return nil + }, + } + + c.AddCommand( + newConfigGetCmd(), + newConfigSetCmd(), + newConfigUnsetCmd(), + newConfigListCmd(), + newConfigEditCmd(), + ) + return c +} + +// newConfigGetCmd implements "model-cli config get ". +func newConfigGetCmd() *cobra.Command { + var ( + global bool + system bool + file string + defaultVal string + hasDefault bool + showAll bool + showOrigin bool + ) + + c := &cobra.Command{ + Use: "get ", + Short: "Get the value of a config key", + Long: `Get the value of a config key. + +Prints the value of the given key to stdout. If the key appears multiple times +(multi-valued), the last value is printed. Use --all to print all values. + +Exit status is 1 if the key is not found (unless --default is given).`, + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + path, err := resolveConfigPath(global, system, file) + if err != nil { + return err + } + f, err := iniconfig.Load(path) + if err != nil { + return err + } + + key := args[0] + + if showAll { + vals := f.GetAll(key) + if len(vals) == 0 { + if hasDefault { + cmd.Println(defaultVal) + return nil + } + return fmt.Errorf("key not found: %s", key) + } + for _, v := range vals { + if showOrigin { + cmd.Printf("file:%s\t%s\n", path, v) + } else { + cmd.Println(v) + } + } + return nil + } + + v, ok := f.Get(key) + if !ok { + if hasDefault { + cmd.Println(defaultVal) + return nil + } + return fmt.Errorf("key not found: %s", key) + } + if showOrigin { + cmd.Printf("file:%s\t%s\n", path, v) + } else { + cmd.Println(v) + } + return nil + }, + } + + addLocationFlags(c, &global, &system, &file) + c.Flags().StringVar(&defaultVal, "default", "", "value to emit if the key is not set") + c.Flags().BoolVar(&showAll, "all", false, "print all values for multi-valued keys") + c.Flags().BoolVar(&showOrigin, "show-origin", false, "show the origin (file path) of each value") + // Track whether --default was explicitly provided. + c.PreRunE = func(cmd *cobra.Command, args []string) error { + hasDefault = cmd.Flags().Changed("default") + return nil + } + + return c +} + +// newConfigSetCmd implements "model-cli config set ". +func newConfigSetCmd() *cobra.Command { + var global, system bool + var file string + + c := &cobra.Command{ + Use: "set ", + Short: "Set a config key to a value", + Long: `Set a config key to a value. + +If the key already exists its value is replaced. The file is written atomically.`, + Args: cobra.ExactArgs(2), + RunE: func(cmd *cobra.Command, args []string) error { + path, err := resolveConfigPath(global, system, file) + if err != nil { + return err + } + f, err := iniconfig.Load(path) + if err != nil { + return err + } + return f.Set(args[0], args[1]) + }, + } + + addLocationFlags(c, &global, &system, &file) + return c +} + +// newConfigUnsetCmd implements "model-cli config unset ". +func newConfigUnsetCmd() *cobra.Command { + var global, system bool + var file string + + c := &cobra.Command{ + Use: "unset ", + Short: "Remove a config key", + Long: `Remove a config key (and all its values) from the file.`, + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + path, err := resolveConfigPath(global, system, file) + if err != nil { + return err + } + f, err := iniconfig.Load(path) + if err != nil { + return err + } + return f.Unset(args[0]) + }, + } + + addLocationFlags(c, &global, &system, &file) + return c +} + +// newConfigListCmd implements "model-cli config list". +func newConfigListCmd() *cobra.Command { + var global, system bool + var file string + var showOrigin bool + + c := &cobra.Command{ + Use: "list", + Aliases: []string{"ls"}, + Short: "List all config key/value pairs", + Long: `List all key=value pairs from the config file, one per line.`, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + path, err := resolveConfigPath(global, system, file) + if err != nil { + return err + } + f, err := iniconfig.Load(path) + if err != nil { + return err + } + if showOrigin { + for _, e := range f.Entries() { + cmd.Printf("file:%s\t%s=%s\n", path, e.Key, e.Value) + } + return nil + } + return f.List(cmd.OutOrStdout()) + }, + } + + addLocationFlags(c, &global, &system, &file) + c.Flags().BoolVar(&showOrigin, "show-origin", false, "show the origin (file path) of each value") + return c +} + +// newConfigEditCmd implements "model-cli config edit". +func newConfigEditCmd() *cobra.Command { + var global, system bool + var file string + + c := &cobra.Command{ + Use: "edit", + Short: "Open the config file in your editor", + Long: `Open the config file in the default editor. + +The editor is determined by the VISUAL or EDITOR environment variables, +falling back to vi on Unix and notepad on Windows.`, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + path, err := resolveConfigPath(global, system, file) + if err != nil { + return err + } + // Ensure the file (and its parent directory) exist so the editor + // has something to open. + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + return err + } + if _, err := os.Stat(path); os.IsNotExist(err) { + // Create with 0600 — config files may hold sensitive values. + f, err2 := os.OpenFile(path, os.O_CREATE|os.O_WRONLY, 0o600) + if err2 != nil { + return err2 + } + _ = f.Close() + } + + editorStr := os.Getenv("VISUAL") + if editorStr == "" { + editorStr = os.Getenv("EDITOR") + } + if editorStr == "" { + if runtime.GOOS == "windows" { + editorStr = "notepad" + } else { + editorStr = "vi" + } + } + + // VISUAL/EDITOR may contain arguments (e.g. "code --wait"). + parts := strings.Fields(editorStr) + editorArgs := append(parts[1:], path) + //nolint:gosec // editor is a user-controlled input, which is intentional + editorCmd := exec.CommandContext(cmd.Context(), parts[0], editorArgs...) + editorCmd.Stdin = os.Stdin + editorCmd.Stdout = os.Stdout + editorCmd.Stderr = os.Stderr + return editorCmd.Run() + }, + } + + addLocationFlags(c, &global, &system, &file) + return c +} diff --git a/cmd/cli/commands/configure.go b/cmd/cli/commands/configure.go index 90fe8d0cd..c6ee97ee2 100644 --- a/cmd/cli/commands/configure.go +++ b/cmd/cli/commands/configure.go @@ -11,10 +11,9 @@ func newConfigureCmd() *cobra.Command { var flags ConfigureFlags c := &cobra.Command{ - Use: "configure [--context-size=] [--speculative-draft-model=] [--hf_overrides=] [--gpu-memory-utilization=] [--mode=] [--think] [--keep-alive=] MODEL [-- ]", - Aliases: []string{"config"}, - Short: "Manage model runtime configurations", - Hidden: true, + Use: "configure [--context-size=] [--speculative-draft-model=] [--hf_overrides=] [--gpu-memory-utilization=] [--mode=] [--think] [--keep-alive=] MODEL [-- ]", + Short: "Manage model runtime configurations", + Hidden: true, Args: func(cmd *cobra.Command, args []string) error { argsBeforeDash := cmd.ArgsLenAtDash() if argsBeforeDash == -1 { diff --git a/cmd/cli/commands/root.go b/cmd/cli/commands/root.go index 358e04e2f..a4d59ebd3 100644 --- a/cmd/cli/commands/root.go +++ b/cmd/cli/commands/root.go @@ -105,6 +105,7 @@ func NewRootCmd(cli *command.DockerCli) *cobra.Command { newReinstallRunner(), newSearchCmd(), newSkillsCmd(), + newConfigCmd(), ) rootCmd.AddCommand(newGatewayCmd()) diff --git a/cmd/cli/docs/reference/docker_model.yaml b/cmd/cli/docs/reference/docker_model.yaml index 6d1588f6f..830cd66df 100644 --- a/cmd/cli/docs/reference/docker_model.yaml +++ b/cmd/cli/docs/reference/docker_model.yaml @@ -7,6 +7,7 @@ pname: docker plink: docker.yaml cname: - docker model bench + - docker model config - docker model context - docker model df - docker model gateway @@ -37,6 +38,7 @@ cname: - docker model version clink: - docker_model_bench.yaml + - docker_model_config.yaml - docker_model_context.yaml - docker_model_df.yaml - docker_model_gateway.yaml diff --git a/cmd/cli/docs/reference/docker_model_config.yaml b/cmd/cli/docs/reference/docker_model_config.yaml new file mode 100644 index 000000000..bc4b79192 --- /dev/null +++ b/cmd/cli/docs/reference/docker_model_config.yaml @@ -0,0 +1,44 @@ +command: docker model config +short: Read and write model-runner config file values +long: |- + Read and write model-runner config file values. + + The config file uses an INI format with sections and key=value pairs: + + [section] + key = value + [section "subsection"] + key = value + + Keys are specified in dot notation: section.key or section.subsection.key. + + The default file is $XDG_CONFIG_HOME/model-runner/config, falling back to + ~/.config/model-runner/config when XDG_CONFIG_HOME is not set. + + Examples: + model-cli config set user.name "Alice" + model-cli config get user.name + model-cli config list + model-cli config unset user.name + model-cli config edit +pname: docker model +plink: docker_model.yaml +cname: + - docker model config edit + - docker model config get + - docker model config list + - docker model config set + - docker model config unset +clink: + - docker_model_config_edit.yaml + - docker_model_config_get.yaml + - docker_model_config_list.yaml + - docker_model_config_set.yaml + - docker_model_config_unset.yaml +deprecated: false +hidden: false +experimental: false +experimentalcli: false +kubernetes: false +swarm: false + diff --git a/cmd/cli/docs/reference/docker_model_config_edit.yaml b/cmd/cli/docs/reference/docker_model_config_edit.yaml new file mode 100644 index 000000000..f9421c5f6 --- /dev/null +++ b/cmd/cli/docs/reference/docker_model_config_edit.yaml @@ -0,0 +1,48 @@ +command: docker model config edit +short: Open the config file in your editor +long: |- + Open the config file in the default editor. + + The editor is determined by the VISUAL or EDITOR environment variables, + falling back to vi on Unix and notepad on Windows. +usage: docker model config edit +pname: docker model config +plink: docker_model_config.yaml +options: + - option: file + shorthand: f + value_type: string + description: use a specific config file + deprecated: false + hidden: false + experimental: false + experimentalcli: false + kubernetes: false + swarm: false + - option: global + value_type: bool + default_value: "false" + description: use the global (user-level) config file + deprecated: false + hidden: false + experimental: false + experimentalcli: false + kubernetes: false + swarm: false + - option: system + value_type: bool + default_value: "false" + description: use the system-wide config file + deprecated: false + hidden: false + experimental: false + experimentalcli: false + kubernetes: false + swarm: false +deprecated: false +hidden: false +experimental: false +experimentalcli: false +kubernetes: false +swarm: false + diff --git a/cmd/cli/docs/reference/docker_model_config_get.yaml b/cmd/cli/docs/reference/docker_model_config_get.yaml new file mode 100644 index 000000000..3baf54ce2 --- /dev/null +++ b/cmd/cli/docs/reference/docker_model_config_get.yaml @@ -0,0 +1,79 @@ +command: docker model config get +short: Get the value of a config key +long: |- + Get the value of a config key. + + Prints the value of the given key to stdout. If the key appears multiple times + (multi-valued), the last value is printed. Use --all to print all values. + + Exit status is 1 if the key is not found (unless --default is given). +usage: docker model config get +pname: docker model config +plink: docker_model_config.yaml +options: + - option: all + value_type: bool + default_value: "false" + description: print all values for multi-valued keys + deprecated: false + hidden: false + experimental: false + experimentalcli: false + kubernetes: false + swarm: false + - option: default + value_type: string + description: value to emit if the key is not set + deprecated: false + hidden: false + experimental: false + experimentalcli: false + kubernetes: false + swarm: false + - option: file + shorthand: f + value_type: string + description: use a specific config file + deprecated: false + hidden: false + experimental: false + experimentalcli: false + kubernetes: false + swarm: false + - option: global + value_type: bool + default_value: "false" + description: use the global (user-level) config file + deprecated: false + hidden: false + experimental: false + experimentalcli: false + kubernetes: false + swarm: false + - option: show-origin + value_type: bool + default_value: "false" + description: show the origin (file path) of each value + deprecated: false + hidden: false + experimental: false + experimentalcli: false + kubernetes: false + swarm: false + - option: system + value_type: bool + default_value: "false" + description: use the system-wide config file + deprecated: false + hidden: false + experimental: false + experimentalcli: false + kubernetes: false + swarm: false +deprecated: false +hidden: false +experimental: false +experimentalcli: false +kubernetes: false +swarm: false + diff --git a/cmd/cli/docs/reference/docker_model_config_list.yaml b/cmd/cli/docs/reference/docker_model_config_list.yaml new file mode 100644 index 000000000..1d0112733 --- /dev/null +++ b/cmd/cli/docs/reference/docker_model_config_list.yaml @@ -0,0 +1,55 @@ +command: docker model config list +aliases: docker model config list, docker model config ls +short: List all config key/value pairs +long: List all key=value pairs from the config file, one per line. +usage: docker model config list +pname: docker model config +plink: docker_model_config.yaml +options: + - option: file + shorthand: f + value_type: string + description: use a specific config file + deprecated: false + hidden: false + experimental: false + experimentalcli: false + kubernetes: false + swarm: false + - option: global + value_type: bool + default_value: "false" + description: use the global (user-level) config file + deprecated: false + hidden: false + experimental: false + experimentalcli: false + kubernetes: false + swarm: false + - option: show-origin + value_type: bool + default_value: "false" + description: show the origin (file path) of each value + deprecated: false + hidden: false + experimental: false + experimentalcli: false + kubernetes: false + swarm: false + - option: system + value_type: bool + default_value: "false" + description: use the system-wide config file + deprecated: false + hidden: false + experimental: false + experimentalcli: false + kubernetes: false + swarm: false +deprecated: false +hidden: false +experimental: false +experimentalcli: false +kubernetes: false +swarm: false + diff --git a/cmd/cli/docs/reference/docker_model_config_set.yaml b/cmd/cli/docs/reference/docker_model_config_set.yaml new file mode 100644 index 000000000..84bd1b97f --- /dev/null +++ b/cmd/cli/docs/reference/docker_model_config_set.yaml @@ -0,0 +1,47 @@ +command: docker model config set +short: Set a config key to a value +long: |- + Set a config key to a value. + + If the key already exists its value is replaced. The file is written atomically. +usage: docker model config set +pname: docker model config +plink: docker_model_config.yaml +options: + - option: file + shorthand: f + value_type: string + description: use a specific config file + deprecated: false + hidden: false + experimental: false + experimentalcli: false + kubernetes: false + swarm: false + - option: global + value_type: bool + default_value: "false" + description: use the global (user-level) config file + deprecated: false + hidden: false + experimental: false + experimentalcli: false + kubernetes: false + swarm: false + - option: system + value_type: bool + default_value: "false" + description: use the system-wide config file + deprecated: false + hidden: false + experimental: false + experimentalcli: false + kubernetes: false + swarm: false +deprecated: false +hidden: false +experimental: false +experimentalcli: false +kubernetes: false +swarm: false + diff --git a/cmd/cli/docs/reference/docker_model_config_unset.yaml b/cmd/cli/docs/reference/docker_model_config_unset.yaml new file mode 100644 index 000000000..778067f2b --- /dev/null +++ b/cmd/cli/docs/reference/docker_model_config_unset.yaml @@ -0,0 +1,44 @@ +command: docker model config unset +short: Remove a config key +long: Remove a config key (and all its values) from the file. +usage: docker model config unset +pname: docker model config +plink: docker_model_config.yaml +options: + - option: file + shorthand: f + value_type: string + description: use a specific config file + deprecated: false + hidden: false + experimental: false + experimentalcli: false + kubernetes: false + swarm: false + - option: global + value_type: bool + default_value: "false" + description: use the global (user-level) config file + deprecated: false + hidden: false + experimental: false + experimentalcli: false + kubernetes: false + swarm: false + - option: system + value_type: bool + default_value: "false" + description: use the system-wide config file + deprecated: false + hidden: false + experimental: false + experimentalcli: false + kubernetes: false + swarm: false +deprecated: false +hidden: false +experimental: false +experimentalcli: false +kubernetes: false +swarm: false + diff --git a/cmd/cli/docs/reference/docker_model_configure.yaml b/cmd/cli/docs/reference/docker_model_configure.yaml index 77f914fdc..9849785b9 100644 --- a/cmd/cli/docs/reference/docker_model_configure.yaml +++ b/cmd/cli/docs/reference/docker_model_configure.yaml @@ -1,5 +1,4 @@ command: docker model configure -aliases: docker model configure, docker model config short: Manage model runtime configurations long: Manage model runtime configurations usage: docker model configure [--context-size=] [--speculative-draft-model=] [--hf_overrides=] [--gpu-memory-utilization=] [--mode=] [--think] [--keep-alive=] MODEL [-- ] diff --git a/cmd/cli/docs/reference/model.md b/cmd/cli/docs/reference/model.md index e26c01924..af79fca7d 100644 --- a/cmd/cli/docs/reference/model.md +++ b/cmd/cli/docs/reference/model.md @@ -8,6 +8,7 @@ Docker Model Runner | Name | Description | |:------------------------------------------------|:-----------------------------------------------------------------------| | [`bench`](model_bench.md) | Benchmark a model's performance at different concurrency levels | +| [`config`](model_config.md) | Read and write model-runner config file values | | [`context`](model_context.md) | Manage Docker Model Runner contexts | | [`df`](model_df.md) | Show Docker Model Runner disk usage | | [`gateway`](model_gateway.md) | Run an OpenAI-compatible LLM gateway | diff --git a/cmd/cli/docs/reference/model_config.md b/cmd/cli/docs/reference/model_config.md new file mode 100644 index 000000000..31b59f577 --- /dev/null +++ b/cmd/cli/docs/reference/model_config.md @@ -0,0 +1,38 @@ +# docker model config + + +Read and write model-runner config file values. + +The config file uses an INI format with sections and key=value pairs: + + [section] + key = value + [section "subsection"] + key = value + +Keys are specified in dot notation: section.key or section.subsection.key. + +The default file is $XDG_CONFIG_HOME/model-runner/config, falling back to +~/.config/model-runner/config when XDG_CONFIG_HOME is not set. + +Examples: + model-cli config set user.name "Alice" + model-cli config get user.name + model-cli config list + model-cli config unset user.name + model-cli config edit + +### Subcommands + +| Name | Description | +|:---------------------------------|:------------------------------------| +| [`edit`](model_config_edit.md) | Open the config file in your editor | +| [`get`](model_config_get.md) | Get the value of a config key | +| [`list`](model_config_list.md) | List all config key/value pairs | +| [`set`](model_config_set.md) | Set a config key to a value | +| [`unset`](model_config_unset.md) | Remove a config key | + + + + + diff --git a/cmd/cli/docs/reference/model_config_edit.md b/cmd/cli/docs/reference/model_config_edit.md new file mode 100644 index 000000000..fe5d8a93a --- /dev/null +++ b/cmd/cli/docs/reference/model_config_edit.md @@ -0,0 +1,19 @@ +# docker model config edit + + +Open the config file in the default editor. + +The editor is determined by the VISUAL or EDITOR environment variables, +falling back to vi on Unix and notepad on Windows. + +### Options + +| Name | Type | Default | Description | +|:---------------|:---------|:--------|:----------------------------------------| +| `-f`, `--file` | `string` | | use a specific config file | +| `--global` | `bool` | | use the global (user-level) config file | +| `--system` | `bool` | | use the system-wide config file | + + + + diff --git a/cmd/cli/docs/reference/model_config_get.md b/cmd/cli/docs/reference/model_config_get.md new file mode 100644 index 000000000..f79bf7800 --- /dev/null +++ b/cmd/cli/docs/reference/model_config_get.md @@ -0,0 +1,24 @@ +# docker model config get + + +Get the value of a config key. + +Prints the value of the given key to stdout. If the key appears multiple times +(multi-valued), the last value is printed. Use --all to print all values. + +Exit status is 1 if the key is not found (unless --default is given). + +### Options + +| Name | Type | Default | Description | +|:----------------|:---------|:--------|:------------------------------------------| +| `--all` | `bool` | | print all values for multi-valued keys | +| `--default` | `string` | | value to emit if the key is not set | +| `-f`, `--file` | `string` | | use a specific config file | +| `--global` | `bool` | | use the global (user-level) config file | +| `--show-origin` | `bool` | | show the origin (file path) of each value | +| `--system` | `bool` | | use the system-wide config file | + + + + diff --git a/cmd/cli/docs/reference/model_config_list.md b/cmd/cli/docs/reference/model_config_list.md new file mode 100644 index 000000000..60bdd297d --- /dev/null +++ b/cmd/cli/docs/reference/model_config_list.md @@ -0,0 +1,21 @@ +# docker model config list + + +List all key=value pairs from the config file, one per line. + +### Aliases + +`docker model config list`, `docker model config ls` + +### Options + +| Name | Type | Default | Description | +|:----------------|:---------|:--------|:------------------------------------------| +| `-f`, `--file` | `string` | | use a specific config file | +| `--global` | `bool` | | use the global (user-level) config file | +| `--show-origin` | `bool` | | show the origin (file path) of each value | +| `--system` | `bool` | | use the system-wide config file | + + + + diff --git a/cmd/cli/docs/reference/model_config_set.md b/cmd/cli/docs/reference/model_config_set.md new file mode 100644 index 000000000..f826d8dab --- /dev/null +++ b/cmd/cli/docs/reference/model_config_set.md @@ -0,0 +1,18 @@ +# docker model config set + + +Set a config key to a value. + +If the key already exists its value is replaced. The file is written atomically. + +### Options + +| Name | Type | Default | Description | +|:---------------|:---------|:--------|:----------------------------------------| +| `-f`, `--file` | `string` | | use a specific config file | +| `--global` | `bool` | | use the global (user-level) config file | +| `--system` | `bool` | | use the system-wide config file | + + + + diff --git a/cmd/cli/docs/reference/model_config_unset.md b/cmd/cli/docs/reference/model_config_unset.md new file mode 100644 index 000000000..512e0ec21 --- /dev/null +++ b/cmd/cli/docs/reference/model_config_unset.md @@ -0,0 +1,16 @@ +# docker model config unset + + +Remove a config key (and all its values) from the file. + +### Options + +| Name | Type | Default | Description | +|:---------------|:---------|:--------|:----------------------------------------| +| `-f`, `--file` | `string` | | use a specific config file | +| `--global` | `bool` | | use the global (user-level) config file | +| `--system` | `bool` | | use the system-wide config file | + + + + diff --git a/cmd/cli/iniconfig/iniconfig.go b/cmd/cli/iniconfig/iniconfig.go new file mode 100644 index 000000000..7658ed557 --- /dev/null +++ b/cmd/cli/iniconfig/iniconfig.go @@ -0,0 +1,565 @@ +// Package iniconfig implements reading and writing of INI-style config files. +// The format uses sections, optional subsections, and key=value pairs: +// +// [section] +// key = value +// [section "subsection"] +// key = value +// +// Key names are of the form "section.key" or "section.subsection.key". +// Section names and variable names are case-insensitive; subsection names are +// case-sensitive. +package iniconfig + +import ( + "bufio" + "bytes" + "errors" + "fmt" + "io" + "os" + "path/filepath" + "strings" + "unicode" +) + +// maxConfigLineBytes is the hard cap on a single config line length (1 MiB). +// This guards against unbounded memory use on pathological inputs while still +// accommodating large values such as certificates or long tokens. +const maxConfigLineBytes = 1 << 20 + +// Entry is a single key/value pair from a config file. +type Entry struct { + // Key is the canonical dotted key: "section.variable" or + // "section.subsection.variable". Section and variable are lowercased; + // subsection preserves its original case. + Key string + Value string +} + +// File represents a parsed config file and the path it was read from. +type File struct { + path string + entries []Entry +} + +// Path returns the file path associated with this File. +func (f *File) Path() string { return f.path } + +// Entries returns all key/value pairs in file order. +func (f *File) Entries() []Entry { return f.entries } + +// ---------------------------------------------------------------------------- +// Reading +// ---------------------------------------------------------------------------- + +// Load reads the config file at path. If the file does not exist an empty File +// is returned without error. +func Load(path string) (*File, error) { + data, err := os.ReadFile(path) + if err != nil { + if os.IsNotExist(err) { + return &File{path: path}, nil + } + return nil, err + } + entries, err := parse(data) + if err != nil { + return nil, fmt.Errorf("%s: %w", path, err) + } + return &File{path: path, entries: entries}, nil +} + +// parse parses INI bytes into a slice of Entries. +func parse(data []byte) ([]Entry, error) { + // Strip UTF-8 BOM if present. + data = bytes.TrimPrefix(data, []byte{0xEF, 0xBB, 0xBF}) + + var entries []Entry + var section, subsection string + + scanner := bufio.NewScanner(bytes.NewReader(data)) + scanner.Buffer(make([]byte, 0, 64*1024), maxConfigLineBytes) + lineNum := 0 + for scanner.Scan() { + lineNum++ + line := scanner.Text() + trimmed := strings.TrimSpace(line) + + // Empty line or comment. + if trimmed == "" || trimmed[0] == '#' || trimmed[0] == ';' { + continue + } + + if trimmed[0] == '[' { + // Section header. + var err error + section, subsection, err = parseSectionHeader(trimmed) + if err != nil { + return nil, fmt.Errorf("line %d: %w", lineNum, err) + } + continue + } + + // Key-value (or boolean key). + if section == "" { + return nil, fmt.Errorf("line %d: key outside of section", lineNum) + } + key, value, err := parseKeyValue(line) + if err != nil { + return nil, fmt.Errorf("line %d: %w", lineNum, err) + } + canonical := canonicalKey(section, subsection, key) + entries = append(entries, Entry{Key: canonical, Value: value}) + } + if err := scanner.Err(); err != nil { + if errors.Is(err, bufio.ErrTooLong) { + return nil, fmt.Errorf("line %d: config line too long (max %d bytes)", lineNum+1, maxConfigLineBytes) + } + return nil, err + } + return entries, nil +} + +// parseSectionHeader parses "[section]" or `[section "subsection"]`. +// Anything after the closing ']' (e.g. inline comments) is ignored. +// section is returned lowercased; subsection preserves case. +func parseSectionHeader(line string) (section, subsection string, err error) { + // Find the closing bracket; ignore anything that follows (inline comment). + closeIdx := strings.IndexByte(line, ']') + if closeIdx < 0 { + return "", "", fmt.Errorf("invalid section header: %q", line) + } + inner := line[1:closeIdx] + + // Check for subsection: section "subsection" + if idx := strings.Index(inner, "\""); idx >= 0 { + rawSection := strings.TrimRight(inner[:idx], " \t") + rawSubsection := inner[idx:] + if !strings.HasPrefix(rawSubsection, "\"") || !strings.HasSuffix(rawSubsection, "\"") || len(rawSubsection) < 2 { + return "", "", fmt.Errorf("invalid section header: %q", line) + } + sub, err2 := unescapeSubsection(rawSubsection[1 : len(rawSubsection)-1]) + if err2 != nil { + return "", "", fmt.Errorf("invalid subsection in %q: %w", line, err2) + } + return strings.ToLower(rawSection), sub, nil + } + + return strings.ToLower(strings.TrimSpace(inner)), "", nil +} + +// unescapeSubsection handles backslash escapes inside subsection names. +// \\ and \" are unescaped; a lone \ followed by any other char is silently +// dropped (the character after it is kept). +func unescapeSubsection(s string) (string, error) { + var b strings.Builder + for i := 0; i < len(s); i++ { + if s[i] == '\\' && i+1 < len(s) { + i++ + switch s[i] { + case '\\': + b.WriteByte('\\') + case '"': + b.WriteByte('"') + default: + // Unknown escape: drop the backslash, keep the character. + b.WriteByte(s[i]) + } + continue + } + b.WriteByte(s[i]) + } + return b.String(), nil +} + +// parseKeyValue parses a line of the form " key = value # comment" or a +// boolean " key". Supports line continuation with trailing backslash. +func parseKeyValue(line string) (key, value string, err error) { + trimmed := strings.TrimLeft(line, " \t") + + eqIdx := strings.IndexByte(trimmed, '=') + if eqIdx < 0 { + // Boolean key: no "=", value is implicitly "true". + key = strings.TrimRight(trimmed, " \t") + if err2 := validateVarName(key); err2 != nil { + return "", "", err2 + } + return strings.ToLower(key), "true", nil + } + + key = strings.TrimRight(trimmed[:eqIdx], " \t") + if err2 := validateVarName(key); err2 != nil { + return "", "", err2 + } + + raw := strings.TrimLeft(trimmed[eqIdx+1:], " \t") + val, err2 := parseValue(raw) + if err2 != nil { + return "", "", err2 + } + return strings.ToLower(key), val, nil +} + +// parseValue decodes the value portion of a key-value line, handling quoting, +// escape sequences, and inline comments. +func parseValue(raw string) (string, error) { + var b strings.Builder + inQuotes := false + i := 0 + for i < len(raw) { + c := raw[i] + switch { + case !inQuotes && (c == '#' || c == ';'): + // Inline comment — stop. + goto done + case !inQuotes && c == '"': + inQuotes = true + i++ + case inQuotes && c == '"': + inQuotes = false + i++ + case c == '\\': + if i+1 >= len(raw) { + // Trailing backslash = line continuation (we don't handle + // multi-line here; treat as end of value). + goto done + } + i++ + switch raw[i] { + case 'n': + b.WriteByte('\n') + case 't': + b.WriteByte('\t') + case 'b': + b.WriteByte('\b') + case '"': + b.WriteByte('"') + case '\\': + b.WriteByte('\\') + default: + return "", fmt.Errorf("unknown escape sequence \\%c", raw[i]) + } + i++ + default: + b.WriteByte(c) + i++ + } + } +done: + if inQuotes { + return "", fmt.Errorf("unterminated quoted string") + } + result := b.String() + if !inQuotes { + result = strings.TrimRight(result, " \t") + } + return result, nil +} + +// validateVarName ensures a variable name contains only [A-Za-z0-9-] and +// starts with a letter. +func validateVarName(name string) error { + if name == "" { + return fmt.Errorf("empty variable name") + } + if !unicode.IsLetter(rune(name[0])) { + return fmt.Errorf("variable name %q must start with a letter", name) + } + for _, c := range name { + if !unicode.IsLetter(c) && !unicode.IsDigit(c) && c != '-' { + return fmt.Errorf("invalid character %q in variable name %q", c, name) + } + } + return nil +} + +// canonicalKey assembles the canonical dotted key. +func canonicalKey(section, subsection, variable string) string { + section = strings.ToLower(section) + variable = strings.ToLower(variable) + if subsection == "" { + return section + "." + variable + } + return section + "." + subsection + "." + variable +} + +// ---------------------------------------------------------------------------- +// Key parsing (for CLI inputs) +// ---------------------------------------------------------------------------- + +// ParseKey splits a dotted key "section.variable" or +// "section.subsection.variable" into its components. Section and variable are +// lowercased; subsection preserves case. The split point is the last dot. +func ParseKey(key string) (section, subsection, variable string, err error) { + // The last dot separates the variable from the section[.subsection] part. + lastDot := strings.LastIndex(key, ".") + if lastDot < 0 { + return "", "", "", fmt.Errorf("invalid key %q: must contain at least one dot", key) + } + variable = strings.ToLower(key[lastDot+1:]) + prefix := key[:lastDot] + + // The first dot (if any) separates section from subsection. + firstDot := strings.Index(prefix, ".") + if firstDot < 0 { + section = strings.ToLower(prefix) + subsection = "" + } else { + section = strings.ToLower(prefix[:firstDot]) + subsection = prefix[firstDot+1:] // subsection preserves case + } + + if section == "" { + return "", "", "", fmt.Errorf("invalid key %q: empty section", key) + } + if variable == "" { + return "", "", "", fmt.Errorf("invalid key %q: empty variable", key) + } + if err2 := validateVarName(variable); err2 != nil { + return "", "", "", fmt.Errorf("invalid key %q: %w", key, err2) + } + return section, subsection, variable, nil +} + +// ---------------------------------------------------------------------------- +// Querying +// ---------------------------------------------------------------------------- + +// Get returns the last value for the given canonical key. The second return +// value is false if the key is not present. +func (f *File) Get(key string) (string, bool) { + section, subsection, variable, err := ParseKey(key) + if err != nil { + return "", false + } + canonical := canonicalKey(section, subsection, variable) + found := false + last := "" + for _, e := range f.entries { + if e.Key == canonical { + last = e.Value + found = true + } + } + return last, found +} + +// GetAll returns all values for the given canonical key. +func (f *File) GetAll(key string) []string { + section, subsection, variable, err := ParseKey(key) + if err != nil { + return nil + } + canonical := canonicalKey(section, subsection, variable) + var vals []string + for _, e := range f.entries { + if e.Key == canonical { + vals = append(vals, e.Value) + } + } + return vals +} + +// ---------------------------------------------------------------------------- +// Writing +// ---------------------------------------------------------------------------- + +// Set writes key=value to the file, replacing the last existing occurrence or +// appending if absent. The file is written atomically via a lock file. +func (f *File) Set(key, value string) error { + section, subsection, variable, err := ParseKey(key) + if err != nil { + return err + } + canonical := canonicalKey(section, subsection, variable) + return f.writeAtomic(func(entries []Entry) []Entry { + replaced := false + for i := len(entries) - 1; i >= 0; i-- { + if entries[i].Key == canonical { + entries[i].Value = value + replaced = true + break + } + } + if !replaced { + entries = append(entries, Entry{Key: canonical, Value: value}) + } + return entries + }) +} + +// Unset removes all occurrences of key from the file. +func (f *File) Unset(key string) error { + section, subsection, variable, err := ParseKey(key) + if err != nil { + return err + } + canonical := canonicalKey(section, subsection, variable) + return f.writeAtomic(func(entries []Entry) []Entry { + out := entries[:0] + for _, e := range entries { + if e.Key != canonical { + out = append(out, e) + } + } + return out + }) +} + +// writeAtomic applies transform to the in-memory entries, serialises the +// result to disk atomically (write to .lock → rename), and updates f.entries. +func (f *File) writeAtomic(transform func([]Entry) []Entry) error { + if err := os.MkdirAll(filepath.Dir(f.path), 0o755); err != nil { + return err + } + lockPath := f.path + ".lock" + + // Preserve the existing file's permissions; default to 0600 for new files + // so that config files containing sensitive values are not world-readable. + mode := os.FileMode(0o600) + if info, err := os.Stat(f.path); err == nil { + mode = info.Mode() + } + + newEntries := transform(append([]Entry(nil), f.entries...)) + + data := serialise(newEntries) + if err := os.WriteFile(lockPath, data, mode); err != nil { + return err + } + if err := os.Rename(lockPath, f.path); err != nil { + _ = os.Remove(lockPath) + return err + } + f.entries = newEntries + return nil +} + +// serialise converts a slice of Entries to INI-format bytes. +// Sections are grouped; within each group, key-value lines are tab-indented. +func serialise(entries []Entry) []byte { + var buf bytes.Buffer + + type sectionKey struct { + section string + subsection string + } + + // Preserve insertion order of sections while grouping entries. + type sectionEntry struct { + key sectionKey + items []Entry + } + + var order []sectionKey + groups := map[sectionKey]*sectionEntry{} + + for _, e := range entries { + sec, sub, _, _ := splitCanonical(e.Key) + sk := sectionKey{sec, sub} + if _, ok := groups[sk]; !ok { + order = append(order, sk) + groups[sk] = §ionEntry{key: sk} + } + groups[sk].items = append(groups[sk].items, e) + } + + for _, sk := range order { + g := groups[sk] + buf.WriteString(formatSectionHeader(g.key.section, g.key.subsection)) + for _, e := range g.items { + _, _, variable, _ := splitCanonical(e.Key) + buf.WriteString(formatKeyValue(variable, e.Value)) + } + } + + return buf.Bytes() +} + +// splitCanonical splits a canonical key "section[.subsection].variable" into +// its three parts using the same last-dot logic as ParseKey. +func splitCanonical(canonical string) (section, subsection, variable string, err error) { + lastDot := strings.LastIndex(canonical, ".") + if lastDot < 0 { + return "", "", "", fmt.Errorf("bad canonical key %q", canonical) + } + variable = canonical[lastDot+1:] + prefix := canonical[:lastDot] + firstDot := strings.Index(prefix, ".") + if firstDot < 0 { + section = prefix + } else { + section = prefix[:firstDot] + subsection = prefix[firstDot+1:] + } + return section, subsection, variable, nil +} + +// formatSectionHeader formats a section header line. +func formatSectionHeader(section, subsection string) string { + if subsection == "" { + return fmt.Sprintf("[%s]\n", section) + } + return "[" + section + ` "` + escapeSubsection(subsection) + "\"]\n" +} + +// escapeSubsection escapes backslashes and double-quotes in a subsection name. +func escapeSubsection(s string) string { + s = strings.ReplaceAll(s, "\\", "\\\\") + s = strings.ReplaceAll(s, "\"", "\\\"") + return s +} + +// formatKeyValue formats a key = value line with proper quoting. +func formatKeyValue(variable, value string) string { + return fmt.Sprintf("\t%s = %s\n", variable, quoteValue(value)) +} + +// quoteValue wraps a value in double-quotes when it contains characters that +// would be misinterpreted by the parser (leading/trailing space, #, ;, \r). +// It also applies backslash escaping inside quoted strings. +func quoteValue(v string) string { + needsQuote := v != "" && (v[0] == ' ' || v[0] == '\t' || v[len(v)-1] == ' ' || v[len(v)-1] == '\t') + for _, c := range v { + if c == '#' || c == ';' || c == '\r' || c == '\n' || c == '\\' || c == '"' { + needsQuote = true + break + } + } + if !needsQuote { + return v + } + var b strings.Builder + b.WriteByte('"') + for _, c := range v { + switch c { + case '\\': + b.WriteString(`\\`) + case '"': + b.WriteString(`\"`) + case '\n': + b.WriteString(`\n`) + case '\t': + b.WriteString(`\t`) + default: + b.WriteRune(c) + } + } + b.WriteByte('"') + return b.String() +} + +// ---------------------------------------------------------------------------- +// Listing +// ---------------------------------------------------------------------------- + +// List writes all key=value pairs to w, one per line. +func (f *File) List(w io.Writer) error { + for _, e := range f.entries { + if _, err := fmt.Fprintf(w, "%s=%s\n", e.Key, e.Value); err != nil { + return err + } + } + return nil +} diff --git a/cmd/cli/iniconfig/iniconfig_test.go b/cmd/cli/iniconfig/iniconfig_test.go new file mode 100644 index 000000000..299b86509 --- /dev/null +++ b/cmd/cli/iniconfig/iniconfig_test.go @@ -0,0 +1,322 @@ +package iniconfig_test + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/docker/model-runner/cmd/cli/iniconfig" +) + +// roundTrip writes entries to a temp file, reads them back, and checks they +// match the expected key/value pairs. +func roundTrip(t *testing.T, content string, wantEntries []iniconfig.Entry) { + t.Helper() + dir := t.TempDir() + path := filepath.Join(dir, "config") + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + f, err := iniconfig.Load(path) + if err != nil { + t.Fatalf("Load: %v", err) + } + if len(f.Entries()) != len(wantEntries) { + t.Fatalf("got %d entries, want %d\nentries: %v", len(f.Entries()), len(wantEntries), f.Entries()) + } + for i, e := range f.Entries() { + if e.Key != wantEntries[i].Key || e.Value != wantEntries[i].Value { + t.Errorf("entry[%d]: got {%q %q}, want {%q %q}", i, e.Key, e.Value, wantEntries[i].Key, wantEntries[i].Value) + } + } +} + +func TestParse_SimpleSection(t *testing.T) { + roundTrip(t, ` +[core] + bare = false + filemode = true +`, []iniconfig.Entry{ + {Key: "core.bare", Value: "false"}, + {Key: "core.filemode", Value: "true"}, + }) +} + +func TestParse_Subsection(t *testing.T) { + roundTrip(t, ` +[branch "main"] + remote = origin + merge = refs/heads/main +`, []iniconfig.Entry{ + {Key: "branch.main.remote", Value: "origin"}, + {Key: "branch.main.merge", Value: "refs/heads/main"}, + }) +} + +func TestParse_CaseInsensitiveSection(t *testing.T) { + roundTrip(t, ` +[Core] + Bare = false +`, []iniconfig.Entry{ + {Key: "core.bare", Value: "false"}, + }) +} + +func TestParse_SubsectionCaseSensitive(t *testing.T) { + roundTrip(t, ` +[branch "Main"] + remote = origin +[branch "main"] + remote = upstream +`, []iniconfig.Entry{ + {Key: "branch.Main.remote", Value: "origin"}, + {Key: "branch.main.remote", Value: "upstream"}, + }) +} + +func TestParse_BooleanKey(t *testing.T) { + roundTrip(t, ` +[core] + bare +`, []iniconfig.Entry{ + {Key: "core.bare", Value: "true"}, + }) +} + +func TestParse_InlineComment(t *testing.T) { + roundTrip(t, ` +[core] + name = hello # world +`, []iniconfig.Entry{ + {Key: "core.name", Value: "hello"}, + }) +} + +func TestParse_QuotedValue(t *testing.T) { + roundTrip(t, ` +[core] + name = "hello world" +`, []iniconfig.Entry{ + {Key: "core.name", Value: "hello world"}, + }) +} + +func TestParse_EscapeSequences(t *testing.T) { + roundTrip(t, ` +[core] + name = "hello\nworld" +`, []iniconfig.Entry{ + {Key: "core.name", Value: "hello\nworld"}, + }) +} + +func TestParse_BOM(t *testing.T) { + content := "\xEF\xBB\xBF[core]\n\tbare = false\n" + roundTrip(t, content, []iniconfig.Entry{ + {Key: "core.bare", Value: "false"}, + }) +} + +func TestParse_Comments(t *testing.T) { + roundTrip(t, ` +# This is a comment +; This is also a comment +[core] + # inline section comment + bare = false +`, []iniconfig.Entry{ + {Key: "core.bare", Value: "false"}, + }) +} + +func TestParse_SectionHeaderTrailingComment(t *testing.T) { + roundTrip(t, ` +[core] # this is a trailing comment + bare = false +`, []iniconfig.Entry{ + {Key: "core.bare", Value: "false"}, + }) +} + +func TestParse_FilePermissionsPreserved(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config") + // Create with restrictive permissions. + if err := os.WriteFile(path, []byte("[core]\n\tbare = false\n"), 0o600); err != nil { + t.Fatal(err) + } + f, err := iniconfig.Load(path) + if err != nil { + t.Fatal(err) + } + if err := f.Set("core.filemode", "true"); err != nil { + t.Fatal(err) + } + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if info.Mode() != 0o600 { + t.Errorf("expected mode 0600, got %v", info.Mode()) + } +} + +func TestLoadMissing(t *testing.T) { + f, err := iniconfig.Load("/nonexistent/path/to/config") + if err != nil { + t.Fatalf("expected no error for missing file, got: %v", err) + } + if len(f.Entries()) != 0 { + t.Fatalf("expected empty entries for missing file, got: %v", f.Entries()) + } +} + +func TestGetAndSet(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config") + f, err := iniconfig.Load(path) + if err != nil { + t.Fatal(err) + } + + if err := f.Set("core.bare", "false"); err != nil { + t.Fatal(err) + } + if v, ok := f.Get("core.bare"); !ok || v != "false" { + t.Fatalf("Get after Set: got %q, %v; want %q, true", v, ok, "false") + } + + // Overwrite + if err := f.Set("core.bare", "true"); err != nil { + t.Fatal(err) + } + if v, ok := f.Get("core.bare"); !ok || v != "true" { + t.Fatalf("Get after overwrite: got %q, %v; want %q, true", v, ok, "true") + } +} + +func TestSetSubsection(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config") + f, _ := iniconfig.Load(path) + + if err := f.Set(`branch.main.remote`, "origin"); err != nil { + t.Fatal(err) + } + if v, ok := f.Get("branch.main.remote"); !ok || v != "origin" { + t.Fatalf("got %q, %v", v, ok) + } +} + +func TestUnset(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config") + f, _ := iniconfig.Load(path) + _ = f.Set("core.bare", "false") + _ = f.Set("core.filemode", "true") + + if err := f.Unset("core.bare"); err != nil { + t.Fatal(err) + } + if _, ok := f.Get("core.bare"); ok { + t.Fatal("expected core.bare to be removed") + } + if v, ok := f.Get("core.filemode"); !ok || v != "true" { + t.Fatalf("core.filemode should still be present, got %q, %v", v, ok) + } +} + +func TestAtomicWrite(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config") + f, _ := iniconfig.Load(path) + _ = f.Set("user.name", "Alice") + + // Reload from disk and verify. + f2, err := iniconfig.Load(path) + if err != nil { + t.Fatal(err) + } + if v, ok := f2.Get("user.name"); !ok || v != "Alice" { + t.Fatalf("reload: got %q, %v", v, ok) + } +} + +func TestParseKey(t *testing.T) { + tests := []struct { + input string + section string + subsection string + variable string + wantErr bool + }{ + {"core.bare", "core", "", "bare", false}, + {"branch.main.remote", "branch", "main", "remote", false}, + {"url.https://example.com/.insteadof", "url", "https://example.com/", "insteadof", false}, + {"nokey", "", "", "", true}, + {"section.", "", "", "", true}, + } + for _, tt := range tests { + sec, sub, vari, err := iniconfig.ParseKey(tt.input) + if (err != nil) != tt.wantErr { + t.Errorf("ParseKey(%q): err=%v, wantErr=%v", tt.input, err, tt.wantErr) + continue + } + if !tt.wantErr && (sec != tt.section || sub != tt.subsection || vari != tt.variable) { + t.Errorf("ParseKey(%q) = (%q, %q, %q), want (%q, %q, %q)", + tt.input, sec, sub, vari, tt.section, tt.subsection, tt.variable) + } + } +} + +func TestList(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config") + f, _ := iniconfig.Load(path) + _ = f.Set("user.name", "Alice") + _ = f.Set("user.email", "alice@example.com") + + var sb strings.Builder + if err := f.List(&sb); err != nil { + t.Fatal(err) + } + got := sb.String() + if !strings.Contains(got, "user.name=Alice\n") { + t.Errorf("missing user.name in list output:\n%s", got) + } + if !strings.Contains(got, "user.email=alice@example.com\n") { + t.Errorf("missing user.email in list output:\n%s", got) + } +} + +func TestSerialiseRoundTrip(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config") + f, _ := iniconfig.Load(path) + _ = f.Set("core.bare", "false") + _ = f.Set("core.filemode", "true") + _ = f.Set("branch.main.remote", "origin") + + // Reload and verify structure is preserved. + f2, err := iniconfig.Load(path) + if err != nil { + t.Fatal(err) + } + entries := f2.Entries() + if len(entries) != 3 { + t.Fatalf("expected 3 entries, got %d: %v", len(entries), entries) + } +} + +func TestQuotedValueWithSpecialChars(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config") + f, _ := iniconfig.Load(path) + _ = f.Set("url.value", "value with # hash") + + f2, _ := iniconfig.Load(path) + if v, ok := f2.Get("url.value"); !ok || v != "value with # hash" { + t.Fatalf("got %q, %v", v, ok) + } +} diff --git a/cmd/cli/iniconfig/testmain_test.go b/cmd/cli/iniconfig/testmain_test.go new file mode 100644 index 000000000..27951055e --- /dev/null +++ b/cmd/cli/iniconfig/testmain_test.go @@ -0,0 +1,12 @@ +package iniconfig_test + +import ( + "testing" + + "go.uber.org/goleak" +) + +// TestMain runs goleak after the test suite to detect goroutine leaks. +func TestMain(m *testing.M) { + goleak.VerifyTestMain(m) +} From 72fb485c6ff832df17cd4b790d3a183e3c47623d Mon Sep 17 00:00:00 2001 From: Eric Curtin Date: Tue, 29 Sep 2026 01:40:22 +0100 Subject: [PATCH 02/10] fix: address config review feedback Signed-off-by: Eric Curtin --- cmd/cli/commands/config.go | 111 +++++++++-------------- cmd/cli/commands/config_test.go | 109 ++++++++++++++++++++++ cmd/cli/iniconfig/iniconfig.go | 64 ++++++++++--- cmd/cli/iniconfig/iniconfig_test.go | 69 ++++++++++++++ cmd/cli/iniconfig/iniconfig_unix_test.go | 39 ++++++++ 5 files changed, 311 insertions(+), 81 deletions(-) create mode 100644 cmd/cli/commands/config_test.go create mode 100644 cmd/cli/iniconfig/iniconfig_unix_test.go diff --git a/cmd/cli/commands/config.go b/cmd/cli/commands/config.go index f7a9a775b..e7fa7fe54 100644 --- a/cmd/cli/commands/config.go +++ b/cmd/cli/commands/config.go @@ -43,14 +43,10 @@ func systemConfigPath() string { // Exactly one of global, system, or file may be set. func resolveConfigPath(global, system bool, file string) (string, error) { count := 0 - if global { - count++ - } - if system { - count++ - } - if file != "" { - count++ + for _, set := range []bool{global, system, file != ""} { + if set { + count++ + } } if count > 1 { return "", fmt.Errorf("only one of --global, --system, or --file may be specified") @@ -66,6 +62,15 @@ func resolveConfigPath(global, system bool, file string) (string, error) { } } +// loadConfig resolves the config path from the location flags and loads it. +func loadConfig(global, system bool, file string) (*iniconfig.File, error) { + path, err := resolveConfigPath(global, system, file) + if err != nil { + return nil, err + } + return iniconfig.Load(path) +} + // addLocationFlags adds the standard --global/--system/--file flags to a command. func addLocationFlags(cmd *cobra.Command, global, system *bool, file *string) { cmd.Flags().BoolVar(global, "global", false, "use the global (user-level) config file") @@ -138,48 +143,32 @@ Prints the value of the given key to stdout. If the key appears multiple times Exit status is 1 if the key is not found (unless --default is given).`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - path, err := resolveConfigPath(global, system, file) - if err != nil { - return err - } - f, err := iniconfig.Load(path) + f, err := loadConfig(global, system, file) if err != nil { return err } key := args[0] - + var vals []string if showAll { - vals := f.GetAll(key) - if len(vals) == 0 { - if hasDefault { - cmd.Println(defaultVal) - return nil - } - return fmt.Errorf("key not found: %s", key) - } - for _, v := range vals { - if showOrigin { - cmd.Printf("file:%s\t%s\n", path, v) - } else { - cmd.Println(v) - } - } - return nil + vals = f.GetAll(key) + } else if v, ok := f.Get(key); ok { + vals = []string{v} } - v, ok := f.Get(key) - if !ok { + if len(vals) == 0 { if hasDefault { cmd.Println(defaultVal) return nil } return fmt.Errorf("key not found: %s", key) } - if showOrigin { - cmd.Printf("file:%s\t%s\n", path, v) - } else { - cmd.Println(v) + for _, v := range vals { + if showOrigin { + cmd.Printf("file:%s\t%s\n", f.Path(), v) + } else { + cmd.Println(v) + } } return nil }, @@ -211,11 +200,7 @@ func newConfigSetCmd() *cobra.Command { If the key already exists its value is replaced. The file is written atomically.`, Args: cobra.ExactArgs(2), RunE: func(cmd *cobra.Command, args []string) error { - path, err := resolveConfigPath(global, system, file) - if err != nil { - return err - } - f, err := iniconfig.Load(path) + f, err := loadConfig(global, system, file) if err != nil { return err } @@ -238,11 +223,7 @@ func newConfigUnsetCmd() *cobra.Command { Long: `Remove a config key (and all its values) from the file.`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - path, err := resolveConfigPath(global, system, file) - if err != nil { - return err - } - f, err := iniconfig.Load(path) + f, err := loadConfig(global, system, file) if err != nil { return err } @@ -267,17 +248,13 @@ func newConfigListCmd() *cobra.Command { Long: `List all key=value pairs from the config file, one per line.`, Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { - path, err := resolveConfigPath(global, system, file) - if err != nil { - return err - } - f, err := iniconfig.Load(path) + f, err := loadConfig(global, system, file) if err != nil { return err } if showOrigin { for _, e := range f.Entries() { - cmd.Printf("file:%s\t%s=%s\n", path, e.Key, e.Value) + cmd.Printf("file:%s\t%s=%s\n", f.Path(), e.Key, e.Value) } return nil } @@ -290,6 +267,20 @@ func newConfigListCmd() *cobra.Command { return c } +// editorCommand returns the editor command and its args from VISUAL, then +// EDITOR, skipping empty values, then the platform default. +func editorCommand() []string { + for _, env := range []string{"VISUAL", "EDITOR"} { + if parts := strings.Fields(os.Getenv(env)); len(parts) > 0 { + return parts + } + } + if runtime.GOOS == "windows" { + return []string{"notepad"} + } + return []string{"vi"} +} + // newConfigEditCmd implements "model-cli config edit". func newConfigEditCmd() *cobra.Command { var global, system bool @@ -314,7 +305,7 @@ falling back to vi on Unix and notepad on Windows.`, return err } if _, err := os.Stat(path); os.IsNotExist(err) { - // Create with 0600 — config files may hold sensitive values. + // Create with 0600, config files may hold sensitive values. f, err2 := os.OpenFile(path, os.O_CREATE|os.O_WRONLY, 0o600) if err2 != nil { return err2 @@ -322,20 +313,8 @@ falling back to vi on Unix and notepad on Windows.`, _ = f.Close() } - editorStr := os.Getenv("VISUAL") - if editorStr == "" { - editorStr = os.Getenv("EDITOR") - } - if editorStr == "" { - if runtime.GOOS == "windows" { - editorStr = "notepad" - } else { - editorStr = "vi" - } - } - // VISUAL/EDITOR may contain arguments (e.g. "code --wait"). - parts := strings.Fields(editorStr) + parts := editorCommand() editorArgs := append(parts[1:], path) //nolint:gosec // editor is a user-controlled input, which is intentional editorCmd := exec.CommandContext(cmd.Context(), parts[0], editorArgs...) diff --git a/cmd/cli/commands/config_test.go b/cmd/cli/commands/config_test.go new file mode 100644 index 000000000..3d2b34e7d --- /dev/null +++ b/cmd/cli/commands/config_test.go @@ -0,0 +1,109 @@ +package commands + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" +) + +func runConfig(t *testing.T, args ...string) (string, error) { + t.Helper() + cmd := newConfigCmd() + var out bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&out) + cmd.SetArgs(args) + err := cmd.Execute() + return out.String(), err +} + +func TestResolveConfigPathExclusiveFlags(t *testing.T) { + tests := []struct { + name string + global bool + system bool + file string + wantErr bool + }{ + {"none", false, false, "", false}, + {"global", true, false, "", false}, + {"system", false, true, "", false}, + {"file", false, false, "x", false}, + {"global+system", true, true, "", true}, + {"global+file", true, false, "x", true}, + {"system+file", false, true, "x", true}, + {"all", true, true, "x", true}, + } + for _, tt := range tests { + _, err := resolveConfigPath(tt.global, tt.system, tt.file) + if (err != nil) != tt.wantErr { + t.Errorf("%s: err=%v, wantErr=%v", tt.name, err, tt.wantErr) + } + } +} + +func TestConfigGet(t *testing.T) { + path := filepath.Join(t.TempDir(), "config") + content := "[user]\n\tname = a\n\tname = b\n" + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + + tests := []struct { + name string + args []string + want string + err bool + }{ + {"last", []string{"get", "-f", path, "user.name"}, "b\n", false}, + {"all", []string{"get", "-f", path, "--all", "user.name"}, "a\nb\n", false}, + {"origin", []string{"get", "-f", path, "--show-origin", "user.name"}, "file:" + path + "\tb\n", false}, + {"all origin", []string{"get", "-f", path, "--all", "--show-origin", "user.name"}, "file:" + path + "\ta\nfile:" + path + "\tb\n", false}, + {"default", []string{"get", "-f", path, "--default", "d", "user.none"}, "d\n", false}, + {"default all", []string{"get", "-f", path, "--all", "--default", "d", "user.none"}, "d\n", false}, + {"missing", []string{"get", "-f", path, "user.none"}, "", true}, + {"missing all", []string{"get", "-f", path, "--all", "user.none"}, "", true}, + } + for _, tt := range tests { + out, err := runConfig(t, tt.args...) + if (err != nil) != tt.err { + t.Errorf("%s: err=%v, wantErr=%v", tt.name, err, tt.err) + continue + } + if !tt.err && out != tt.want { + t.Errorf("%s: got %q, want %q", tt.name, out, tt.want) + } + } +} + +func TestConfigLocationFlagsConflict(t *testing.T) { + _, err := runConfig(t, "get", "--global", "--system", "user.name") + if err == nil || !strings.Contains(err.Error(), "only one of") { + t.Fatalf("expected conflict error, got %v", err) + } +} + +func TestEditorCommand(t *testing.T) { + tests := []struct { + name, visual, editor string + want string + }{ + {"visual", "code --wait", "nano", "code --wait"}, + {"editor", "", "nano -w", "nano -w"}, + {"blank visual", " ", "nano", "nano"}, + {"blank both", " ", "\t", ""}, + } + for _, tt := range tests { + t.Setenv("VISUAL", tt.visual) + t.Setenv("EDITOR", tt.editor) + got := editorCommand() + if len(got) == 0 { + t.Fatalf("%s: empty editor command", tt.name) + } + if tt.want != "" && strings.Join(got, " ") != tt.want { + t.Errorf("%s: got %q, want %q", tt.name, got, tt.want) + } + } +} diff --git a/cmd/cli/iniconfig/iniconfig.go b/cmd/cli/iniconfig/iniconfig.go index 7658ed557..6638c9013 100644 --- a/cmd/cli/iniconfig/iniconfig.go +++ b/cmd/cli/iniconfig/iniconfig.go @@ -126,7 +126,7 @@ func parse(data []byte) ([]Entry, error) { // section is returned lowercased; subsection preserves case. func parseSectionHeader(line string) (section, subsection string, err error) { // Find the closing bracket; ignore anything that follows (inline comment). - closeIdx := strings.IndexByte(line, ']') + closeIdx := closingBracket(line) if closeIdx < 0 { return "", "", fmt.Errorf("invalid section header: %q", line) } @@ -149,6 +149,23 @@ func parseSectionHeader(line string) (section, subsection string, err error) { return strings.ToLower(strings.TrimSpace(inner)), "", nil } +// closingBracket returns the index of the ']' that ends a section header, +// skipping any ']' inside a quoted subsection, or -1 if there is none. +func closingBracket(line string) int { + inQuotes := false + for i := 1; i < len(line); i++ { + switch c := line[i]; { + case inQuotes && c == '\\': + i++ + case c == '"': + inQuotes = !inQuotes + case !inQuotes && c == ']': + return i + } + } + return -1 +} + // unescapeSubsection handles backslash escapes inside subsection names. // \\ and \" are unescaped; a lone \ followed by any other char is silently // dropped (the character after it is kept). @@ -174,11 +191,15 @@ func unescapeSubsection(s string) (string, error) { } // parseKeyValue parses a line of the form " key = value # comment" or a -// boolean " key". Supports line continuation with trailing backslash. +// boolean " key". Line continuation is not supported. func parseKeyValue(line string) (key, value string, err error) { trimmed := strings.TrimLeft(line, " \t") eqIdx := strings.IndexByte(trimmed, '=') + if c := strings.IndexAny(trimmed, "#;"); c >= 0 && (eqIdx < 0 || c < eqIdx) { + // A comment before any "=": strip it, this is a boolean key. + trimmed, eqIdx = trimmed[:c], -1 + } if eqIdx < 0 { // Boolean key: no "=", value is implicitly "true". key = strings.TrimRight(trimmed, " \t") @@ -206,12 +227,15 @@ func parseKeyValue(line string) (key, value string, err error) { func parseValue(raw string) (string, error) { var b strings.Builder inQuotes := false + // keep is the length of b that must not be right-trimmed (quoted or + // escaped content). + keep := 0 i := 0 for i < len(raw) { c := raw[i] switch { case !inQuotes && (c == '#' || c == ';'): - // Inline comment — stop. + // Inline comment, stop. goto done case !inQuotes && c == '"': inQuotes = true @@ -221,9 +245,7 @@ func parseValue(raw string) (string, error) { i++ case c == '\\': if i+1 >= len(raw) { - // Trailing backslash = line continuation (we don't handle - // multi-line here; treat as end of value). - goto done + return "", fmt.Errorf("trailing backslash (line continuation is not supported)") } i++ switch raw[i] { @@ -241,8 +263,12 @@ func parseValue(raw string) (string, error) { return "", fmt.Errorf("unknown escape sequence \\%c", raw[i]) } i++ + keep = b.Len() default: b.WriteByte(c) + if inQuotes { + keep = b.Len() + } i++ } } @@ -251,10 +277,7 @@ done: return "", fmt.Errorf("unterminated quoted string") } result := b.String() - if !inQuotes { - result = strings.TrimRight(result, " \t") - } - return result, nil + return result[:keep] + strings.TrimRight(result[keep:], " \t"), nil } // validateVarName ensures a variable name contains only [A-Za-z0-9-] and @@ -308,6 +331,12 @@ func ParseKey(key string) (section, subsection, variable string, err error) { } else { section = strings.ToLower(prefix[:firstDot]) subsection = prefix[firstDot+1:] // subsection preserves case + if subsection == "" { + return "", "", "", fmt.Errorf("invalid key %q: empty subsection", key) + } + if strings.ContainsAny(subsection, "\n\r\x00") { + return "", "", "", fmt.Errorf("invalid key %q: control character in subsection", key) + } } if section == "" { @@ -408,7 +437,7 @@ func (f *File) Unset(key string) error { } // writeAtomic applies transform to the in-memory entries, serialises the -// result to disk atomically (write to .lock → rename), and updates f.entries. +// result to disk atomically (write to .lock, then rename), and updates f.entries. func (f *File) writeAtomic(transform func([]Entry) []Entry) error { if err := os.MkdirAll(filepath.Dir(f.path), 0o755); err != nil { return err @@ -419,16 +448,21 @@ func (f *File) writeAtomic(transform func([]Entry) []Entry) error { // so that config files containing sensitive values are not world-readable. mode := os.FileMode(0o600) if info, err := os.Stat(f.path); err == nil { - mode = info.Mode() + mode = info.Mode().Perm() } newEntries := transform(append([]Entry(nil), f.entries...)) data := serialise(newEntries) - if err := os.WriteFile(lockPath, data, mode); err != nil { - return err + err := os.WriteFile(lockPath, data, mode) + if err == nil { + // WriteFile applies the umask; set the exact mode. + err = os.Chmod(lockPath, mode) + } + if err == nil { + err = os.Rename(lockPath, f.path) } - if err := os.Rename(lockPath, f.path); err != nil { + if err != nil { _ = os.Remove(lockPath) return err } diff --git a/cmd/cli/iniconfig/iniconfig_test.go b/cmd/cli/iniconfig/iniconfig_test.go index 299b86509..24ff022d5 100644 --- a/cmd/cli/iniconfig/iniconfig_test.go +++ b/cmd/cli/iniconfig/iniconfig_test.go @@ -320,3 +320,72 @@ func TestQuotedValueWithSpecialChars(t *testing.T) { t.Fatalf("got %q, %v", v, ok) } } + +func TestParse_BracketInSubsection(t *testing.T) { + roundTrip(t, "[branch \"x]y\"]\n\tremote = origin\n", []iniconfig.Entry{ + {Key: "branch.x]y.remote", Value: "origin"}, + }) +} + +func TestParse_BooleanKeyInlineComment(t *testing.T) { + roundTrip(t, "[core]\n\tbare # enable this\n", []iniconfig.Entry{ + {Key: "core.bare", Value: "true"}, + }) +} + +func TestParse_TrailingBackslashRejected(t *testing.T) { + path := filepath.Join(t.TempDir(), "config") + if err := os.WriteFile(path, []byte("[core]\n\tname = abc\\\n"), 0o600); err != nil { + t.Fatal(err) + } + if _, err := iniconfig.Load(path); err == nil { + t.Fatal("expected error for trailing backslash") + } +} + +func TestSetRoundTripSpecialValues(t *testing.T) { + path := filepath.Join(t.TempDir(), "config") + f, _ := iniconfig.Load(path) + vals := map[string]string{ + "a.trail": "value ", + "a.lead": " value", + "a.bs": `C:\dir\`, + } + for k, v := range vals { + if err := f.Set(k, v); err != nil { + t.Fatalf("Set(%q): %v", k, err) + } + } + f2, err := iniconfig.Load(path) + if err != nil { + t.Fatal(err) + } + for k, want := range vals { + if got, ok := f2.Get(k); !ok || got != want { + t.Errorf("Get(%q) = %q, %v; want %q", k, got, ok, want) + } + } +} + +func TestSetSubsectionWithBracket(t *testing.T) { + path := filepath.Join(t.TempDir(), "config") + f, _ := iniconfig.Load(path) + if err := f.Set("branch.x]y.remote", "origin"); err != nil { + t.Fatal(err) + } + f2, err := iniconfig.Load(path) + if err != nil { + t.Fatal(err) + } + if v, ok := f2.Get("branch.x]y.remote"); !ok || v != "origin" { + t.Fatalf("got %q, %v", v, ok) + } +} + +func TestParseKey_Invalid(t *testing.T) { + for _, k := range []string{"core..name", "core.a\nb.name", "core.a\rb.name", "core.a\x00b.name"} { + if _, _, _, err := iniconfig.ParseKey(k); err == nil { + t.Errorf("ParseKey(%q): expected error", k) + } + } +} diff --git a/cmd/cli/iniconfig/iniconfig_unix_test.go b/cmd/cli/iniconfig/iniconfig_unix_test.go new file mode 100644 index 000000000..3dfc67e1f --- /dev/null +++ b/cmd/cli/iniconfig/iniconfig_unix_test.go @@ -0,0 +1,39 @@ +//go:build !windows + +package iniconfig_test + +import ( + "os" + "path/filepath" + "syscall" + "testing" + + "github.com/docker/model-runner/cmd/cli/iniconfig" +) + +func TestSet_PreservesModeUnderUmask(t *testing.T) { + old := syscall.Umask(0o077) + defer syscall.Umask(old) + + path := filepath.Join(t.TempDir(), "config") + if err := os.WriteFile(path, []byte("[core]\n\tbare = false\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Chmod(path, 0o644); err != nil { + t.Fatal(err) + } + f, err := iniconfig.Load(path) + if err != nil { + t.Fatal(err) + } + if err := f.Set("core.filemode", "true"); err != nil { + t.Fatal(err) + } + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if info.Mode().Perm() != 0o644 { + t.Errorf("expected mode 0644, got %v", info.Mode().Perm()) + } +} From 988e6d2757a13e89424cceb95c2b04b119781c53 Mon Sep 17 00:00:00 2001 From: Eric Curtin Date: Tue, 29 Sep 2026 12:55:35 +0100 Subject: [PATCH 03/10] fix(cli): harden iniconfig locking, limits, section names Signed-off-by: Eric Curtin --- cmd/cli/iniconfig/iniconfig.go | 134 ++++++++++++++++------------ cmd/cli/iniconfig/iniconfig_test.go | 68 ++++++++++++-- 2 files changed, 136 insertions(+), 66 deletions(-) diff --git a/cmd/cli/iniconfig/iniconfig.go b/cmd/cli/iniconfig/iniconfig.go index 6638c9013..3a50711c9 100644 --- a/cmd/cli/iniconfig/iniconfig.go +++ b/cmd/cli/iniconfig/iniconfig.go @@ -56,10 +56,19 @@ func (f *File) Entries() []Entry { return f.entries } // Load reads the config file at path. If the file does not exist an empty File // is returned without error. func Load(path string) (*File, error) { + entries, err := readEntries(path) + if err != nil { + return nil, err + } + return &File{path: path, entries: entries}, nil +} + +// readEntries parses the file at path; a missing file yields no entries. +func readEntries(path string) ([]Entry, error) { data, err := os.ReadFile(path) if err != nil { if os.IsNotExist(err) { - return &File{path: path}, nil + return nil, nil } return nil, err } @@ -67,7 +76,7 @@ func Load(path string) (*File, error) { if err != nil { return nil, fmt.Errorf("%s: %w", path, err) } - return &File{path: path, entries: entries}, nil + return entries, nil } // parse parses INI bytes into a slice of Entries. @@ -79,11 +88,18 @@ func parse(data []byte) ([]Entry, error) { var section, subsection string scanner := bufio.NewScanner(bytes.NewReader(data)) - scanner.Buffer(make([]byte, 0, 64*1024), maxConfigLineBytes) + // Allow room for the "\r\n" terminator; the content length is checked below. + scanner.Buffer(make([]byte, 0, 64*1024), maxConfigLineBytes+2) + tooLong := func(n int) error { + return fmt.Errorf("line %d: config line too long (max %d bytes)", n, maxConfigLineBytes) + } lineNum := 0 for scanner.Scan() { lineNum++ line := scanner.Text() + if len(line) > maxConfigLineBytes { + return nil, tooLong(lineNum) + } trimmed := strings.TrimSpace(line) // Empty line or comment. @@ -114,7 +130,7 @@ func parse(data []byte) ([]Entry, error) { } if err := scanner.Err(); err != nil { if errors.Is(err, bufio.ErrTooLong) { - return nil, fmt.Errorf("line %d: config line too long (max %d bytes)", lineNum+1, maxConfigLineBytes) + return nil, tooLong(lineNum + 1) } return nil, err } @@ -280,6 +296,17 @@ done: return result[:keep] + strings.TrimRight(result[keep:], " \t"), nil } +// badNameRune returns the first rune in name that is not a letter, digit or +// '-'. +func badNameRune(name string) (rune, bool) { + for _, c := range name { + if !unicode.IsLetter(c) && !unicode.IsDigit(c) && c != '-' { + return c, true + } + } + return 0, false +} + // validateVarName ensures a variable name contains only [A-Za-z0-9-] and // starts with a letter. func validateVarName(name string) error { @@ -289,10 +316,8 @@ func validateVarName(name string) error { if !unicode.IsLetter(rune(name[0])) { return fmt.Errorf("variable name %q must start with a letter", name) } - for _, c := range name { - if !unicode.IsLetter(c) && !unicode.IsDigit(c) && c != '-' { - return fmt.Errorf("invalid character %q in variable name %q", c, name) - } + if c, bad := badNameRune(name); bad { + return fmt.Errorf("invalid character %q in variable name %q", c, name) } return nil } @@ -311,37 +336,38 @@ func canonicalKey(section, subsection, variable string) string { // Key parsing (for CLI inputs) // ---------------------------------------------------------------------------- -// ParseKey splits a dotted key "section.variable" or -// "section.subsection.variable" into its components. Section and variable are -// lowercased; subsection preserves case. The split point is the last dot. -func ParseKey(key string) (section, subsection, variable string, err error) { - // The last dot separates the variable from the section[.subsection] part. +// splitKey splits "section[.subsection].variable" on the first and last dots. +func splitKey(key string) (section, subsection, variable string, err error) { lastDot := strings.LastIndex(key, ".") if lastDot < 0 { return "", "", "", fmt.Errorf("invalid key %q: must contain at least one dot", key) } - variable = strings.ToLower(key[lastDot+1:]) - prefix := key[:lastDot] - - // The first dot (if any) separates section from subsection. - firstDot := strings.Index(prefix, ".") - if firstDot < 0 { - section = strings.ToLower(prefix) - subsection = "" - } else { - section = strings.ToLower(prefix[:firstDot]) - subsection = prefix[firstDot+1:] // subsection preserves case - if subsection == "" { - return "", "", "", fmt.Errorf("invalid key %q: empty subsection", key) - } - if strings.ContainsAny(subsection, "\n\r\x00") { - return "", "", "", fmt.Errorf("invalid key %q: control character in subsection", key) - } + variable = key[lastDot+1:] + section, subsection, hasSub := strings.Cut(key[:lastDot], ".") + if hasSub && subsection == "" { + return "", "", "", fmt.Errorf("invalid key %q: empty subsection", key) } + return section, subsection, variable, nil +} +// ParseKey splits a dotted key "section.variable" or +// "section.subsection.variable" into its components. Section and variable are +// lowercased; subsection preserves case. The split point is the last dot. +func ParseKey(key string) (section, subsection, variable string, err error) { + section, subsection, variable, err = splitKey(key) + if err != nil { + return "", "", "", err + } + section, variable = strings.ToLower(section), strings.ToLower(variable) + if strings.ContainsAny(subsection, "\n\r\x00") { + return "", "", "", fmt.Errorf("invalid key %q: control character in subsection", key) + } if section == "" { return "", "", "", fmt.Errorf("invalid key %q: empty section", key) } + if c, bad := badNameRune(section); bad { + return "", "", "", fmt.Errorf("invalid key %q: invalid character %q in section name", key, c) + } if variable == "" { return "", "", "", fmt.Errorf("invalid key %q: empty variable", key) } @@ -395,7 +421,7 @@ func (f *File) GetAll(key string) []string { // ---------------------------------------------------------------------------- // Set writes key=value to the file, replacing the last existing occurrence or -// appending if absent. The file is written atomically via a lock file. +// appending if absent. The file is written atomically under a lock file. func (f *File) Set(key, value string) error { section, subsection, variable, err := ParseKey(key) if err != nil { @@ -436,8 +462,9 @@ func (f *File) Unset(key string) error { }) } -// writeAtomic applies transform to the in-memory entries, serialises the -// result to disk atomically (write to .lock, then rename), and updates f.entries. +// writeAtomic takes an exclusive lock (O_EXCL create of path+".lock"), re-reads +// the file, applies transform, and renames the lock file over the original. +// It fails if the lock is held, so concurrent updates are never lost. func (f *File) writeAtomic(transform func([]Entry) []Entry) error { if err := os.MkdirAll(filepath.Dir(f.path), 0o755); err != nil { return err @@ -451,12 +478,22 @@ func (f *File) writeAtomic(transform func([]Entry) []Entry) error { mode = info.Mode().Perm() } - newEntries := transform(append([]Entry(nil), f.entries...)) + lock, err := os.OpenFile(lockPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, mode) + if err != nil { + return fmt.Errorf("cannot lock config (remove %s if stale): %w", lockPath, err) + } - data := serialise(newEntries) - err := os.WriteFile(lockPath, data, mode) + var newEntries []Entry + current, err := readEntries(f.path) + if err == nil { + newEntries = transform(current) + _, err = lock.Write(serialise(newEntries)) + } + if cerr := lock.Close(); err == nil { + err = cerr + } if err == nil { - // WriteFile applies the umask; set the exact mode. + // OpenFile applies the umask; set the exact mode. err = os.Chmod(lockPath, mode) } if err == nil { @@ -490,7 +527,7 @@ func serialise(entries []Entry) []byte { groups := map[sectionKey]*sectionEntry{} for _, e := range entries { - sec, sub, _, _ := splitCanonical(e.Key) + sec, sub, _, _ := splitKey(e.Key) sk := sectionKey{sec, sub} if _, ok := groups[sk]; !ok { order = append(order, sk) @@ -503,7 +540,7 @@ func serialise(entries []Entry) []byte { g := groups[sk] buf.WriteString(formatSectionHeader(g.key.section, g.key.subsection)) for _, e := range g.items { - _, _, variable, _ := splitCanonical(e.Key) + _, _, variable, _ := splitKey(e.Key) buf.WriteString(formatKeyValue(variable, e.Value)) } } @@ -511,25 +548,6 @@ func serialise(entries []Entry) []byte { return buf.Bytes() } -// splitCanonical splits a canonical key "section[.subsection].variable" into -// its three parts using the same last-dot logic as ParseKey. -func splitCanonical(canonical string) (section, subsection, variable string, err error) { - lastDot := strings.LastIndex(canonical, ".") - if lastDot < 0 { - return "", "", "", fmt.Errorf("bad canonical key %q", canonical) - } - variable = canonical[lastDot+1:] - prefix := canonical[:lastDot] - firstDot := strings.Index(prefix, ".") - if firstDot < 0 { - section = prefix - } else { - section = prefix[:firstDot] - subsection = prefix[firstDot+1:] - } - return section, subsection, variable, nil -} - // formatSectionHeader formats a section header line. func formatSectionHeader(section, subsection string) string { if subsection == "" { diff --git a/cmd/cli/iniconfig/iniconfig_test.go b/cmd/cli/iniconfig/iniconfig_test.go index 24ff022d5..cf863fb8e 100644 --- a/cmd/cli/iniconfig/iniconfig_test.go +++ b/cmd/cli/iniconfig/iniconfig_test.go @@ -3,6 +3,7 @@ package iniconfig_test import ( "os" "path/filepath" + "slices" "strings" "testing" @@ -22,13 +23,8 @@ func roundTrip(t *testing.T, content string, wantEntries []iniconfig.Entry) { if err != nil { t.Fatalf("Load: %v", err) } - if len(f.Entries()) != len(wantEntries) { - t.Fatalf("got %d entries, want %d\nentries: %v", len(f.Entries()), len(wantEntries), f.Entries()) - } - for i, e := range f.Entries() { - if e.Key != wantEntries[i].Key || e.Value != wantEntries[i].Value { - t.Errorf("entry[%d]: got {%q %q}, want {%q %q}", i, e.Key, e.Value, wantEntries[i].Key, wantEntries[i].Value) - } + if !slices.Equal(f.Entries(), wantEntries) { + t.Fatalf("got %q, want %q", f.Entries(), wantEntries) } } @@ -383,9 +379,65 @@ func TestSetSubsectionWithBracket(t *testing.T) { } func TestParseKey_Invalid(t *testing.T) { - for _, k := range []string{"core..name", "core.a\nb.name", "core.a\rb.name", "core.a\x00b.name"} { + for _, k := range []string{"core..name", "core.a\nb.name", "core.a\rb.name", "core.a\x00b.name", "core]x.name", `co"re.name`, "co re.name"} { if _, _, _, err := iniconfig.ParseKey(k); err == nil { t.Errorf("ParseKey(%q): expected error", k) } } } + +func TestSet_LockHeld(t *testing.T) { + path := filepath.Join(t.TempDir(), "config") + if err := os.WriteFile(path+".lock", nil, 0o600); err != nil { + t.Fatal(err) + } + f, _ := iniconfig.Load(path) + if err := f.Set("core.bare", "true"); err == nil { + t.Fatal("expected error while lock is held") + } + if _, err := os.Stat(path + ".lock"); err != nil { + t.Errorf("foreign lock file must not be removed: %v", err) + } +} + +func TestSet_KeepsConcurrentUpdate(t *testing.T) { + path := filepath.Join(t.TempDir(), "config") + f1, _ := iniconfig.Load(path) + f2, _ := iniconfig.Load(path) + if err := f1.Set("core.a", "1"); err != nil { + t.Fatal(err) + } + if err := f2.Set("core.b", "2"); err != nil { + t.Fatal(err) + } + got, err := iniconfig.Load(path) + if err != nil { + t.Fatal(err) + } + want := []iniconfig.Entry{{Key: "core.a", Value: "1"}, {Key: "core.b", Value: "2"}} + if !slices.Equal(got.Entries(), want) { + t.Errorf("got %q, want %q", got.Entries(), want) + } +} + +func TestLoad_LineLengthLimit(t *testing.T) { + const limit = 1 << 20 + head := "[core]\n\tk = " + for _, tt := range []struct { + name string + content string + wantErr bool + }{ + {"exact limit LF", head + strings.Repeat("a", limit-len("\tk = ")) + "\n", false}, + {"exact limit CRLF", head + strings.Repeat("a", limit-len("\tk = ")) + "\r\n", false}, + {"over limit", head + strings.Repeat("a", limit-len("\tk = ")+1) + "\n", true}, + } { + path := filepath.Join(t.TempDir(), "config") + if err := os.WriteFile(path, []byte(tt.content), 0o600); err != nil { + t.Fatal(err) + } + if _, err := iniconfig.Load(path); (err != nil) != tt.wantErr { + t.Errorf("%s: err=%v, wantErr=%v", tt.name, err, tt.wantErr) + } + } +} From 6788339a2b278b287c4ac2fc642698ca564e55d2 Mon Sep 17 00:00:00 2001 From: Eric Curtin Date: Wed, 30 Sep 2026 00:17:45 +0100 Subject: [PATCH 04/10] fix(cli): reject invalid config on write and load --- cmd/cli/iniconfig/iniconfig.go | 38 ++++++++++++++++++++++------- cmd/cli/iniconfig/iniconfig_test.go | 33 +++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 9 deletions(-) diff --git a/cmd/cli/iniconfig/iniconfig.go b/cmd/cli/iniconfig/iniconfig.go index 3a50711c9..de17bebe3 100644 --- a/cmd/cli/iniconfig/iniconfig.go +++ b/cmd/cli/iniconfig/iniconfig.go @@ -159,10 +159,15 @@ func parseSectionHeader(line string) (section, subsection string, err error) { if err2 != nil { return "", "", fmt.Errorf("invalid subsection in %q: %w", line, err2) } - return strings.ToLower(rawSection), sub, nil + section = strings.ToLower(rawSection) + subsection = sub + } else { + section = strings.ToLower(strings.TrimSpace(inner)) } - - return strings.ToLower(strings.TrimSpace(inner)), "", nil + if err := validateSectionName(section); err != nil { + return "", "", fmt.Errorf("invalid section header %q: %w", line, err) + } + return section, subsection, nil } // closingBracket returns the index of the ']' that ends a section header, @@ -307,6 +312,18 @@ func badNameRune(name string) (rune, bool) { return 0, false } +// validateSectionName ensures a section name is non-empty and contains only +// [A-Za-z0-9-]. +func validateSectionName(name string) error { + if name == "" { + return fmt.Errorf("empty section") + } + if c, bad := badNameRune(name); bad { + return fmt.Errorf("invalid character %q in section name %q", c, name) + } + return nil +} + // validateVarName ensures a variable name contains only [A-Za-z0-9-] and // starts with a letter. func validateVarName(name string) error { @@ -362,11 +379,8 @@ func ParseKey(key string) (section, subsection, variable string, err error) { if strings.ContainsAny(subsection, "\n\r\x00") { return "", "", "", fmt.Errorf("invalid key %q: control character in subsection", key) } - if section == "" { - return "", "", "", fmt.Errorf("invalid key %q: empty section", key) - } - if c, bad := badNameRune(section); bad { - return "", "", "", fmt.Errorf("invalid key %q: invalid character %q in section name", key, c) + if err2 := validateSectionName(section); err2 != nil { + return "", "", "", fmt.Errorf("invalid key %q: %w", key, err2) } if variable == "" { return "", "", "", fmt.Errorf("invalid key %q: empty variable", key) @@ -487,7 +501,13 @@ func (f *File) writeAtomic(transform func([]Entry) []Entry) error { current, err := readEntries(f.path) if err == nil { newEntries = transform(current) - _, err = lock.Write(serialise(newEntries)) + data := serialise(newEntries) + // Never write a file that Load would reject. + if _, err = parse(data); err != nil { + err = fmt.Errorf("refusing to write invalid config: %w", err) + } else { + _, err = lock.Write(data) + } } if cerr := lock.Close(); err == nil { err = cerr diff --git a/cmd/cli/iniconfig/iniconfig_test.go b/cmd/cli/iniconfig/iniconfig_test.go index cf863fb8e..b04617356 100644 --- a/cmd/cli/iniconfig/iniconfig_test.go +++ b/cmd/cli/iniconfig/iniconfig_test.go @@ -441,3 +441,36 @@ func TestLoad_LineLengthLimit(t *testing.T) { } } } + +func TestSet_RejectsOverLongLine(t *testing.T) { + path := filepath.Join(t.TempDir(), "config") + f, _ := iniconfig.Load(path) + if err := f.Set("core.a", "1"); err != nil { + t.Fatal(err) + } + if err := f.Set("core.big", strings.Repeat("a", 1<<20)); err == nil { + t.Fatal("expected error for over-long value") + } + got, err := iniconfig.Load(path) + if err != nil { + t.Fatalf("config must stay loadable: %v", err) + } + if want := []iniconfig.Entry{{Key: "core.a", Value: "1"}}; !slices.Equal(got.Entries(), want) { + t.Errorf("got %q, want %q", got.Entries(), want) + } + if _, err := os.Stat(path + ".lock"); !os.IsNotExist(err) { + t.Errorf("lock file must be removed: %v", err) + } +} + +func TestLoad_InvalidSectionName(t *testing.T) { + for _, content := range []string{"[co re]\nname = x\n", "[]\nname = x\n", "[co.re]\nname = x\n", "[co re \"sub\"]\nname = x\n"} { + path := filepath.Join(t.TempDir(), "config") + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + if _, err := iniconfig.Load(path); err == nil { + t.Errorf("Load(%q): expected error", content) + } + } +} From 4c6b3fffa450a3bfbbcf6e95b9c53aafd080b0ab Mon Sep 17 00:00:00 2001 From: Eric Curtin Date: Thu, 1 Oct 2026 02:55:22 +0100 Subject: [PATCH 05/10] config: reject empty subsections and bad get keys Signed-off-by: Eric Curtin --- cmd/cli/commands/config.go | 6 +++++- cmd/cli/commands/config_test.go | 3 +++ cmd/cli/iniconfig/iniconfig.go | 24 +++++++++++++++++++----- cmd/cli/iniconfig/iniconfig_test.go | 2 +- 4 files changed, 28 insertions(+), 7 deletions(-) diff --git a/cmd/cli/commands/config.go b/cmd/cli/commands/config.go index e7fa7fe54..072503fc6 100644 --- a/cmd/cli/commands/config.go +++ b/cmd/cli/commands/config.go @@ -143,12 +143,16 @@ Prints the value of the given key to stdout. If the key appears multiple times Exit status is 1 if the key is not found (unless --default is given).`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + // Get/GetAll treat a malformed key as missing; reject it up front. + key := args[0] + if _, _, _, err := iniconfig.ParseKey(key); err != nil { + return err + } f, err := loadConfig(global, system, file) if err != nil { return err } - key := args[0] var vals []string if showAll { vals = f.GetAll(key) diff --git a/cmd/cli/commands/config_test.go b/cmd/cli/commands/config_test.go index 3d2b34e7d..334cfadec 100644 --- a/cmd/cli/commands/config_test.go +++ b/cmd/cli/commands/config_test.go @@ -65,6 +65,9 @@ func TestConfigGet(t *testing.T) { {"default all", []string{"get", "-f", path, "--all", "--default", "d", "user.none"}, "d\n", false}, {"missing", []string{"get", "-f", path, "user.none"}, "", true}, {"missing all", []string{"get", "-f", path, "--all", "user.none"}, "", true}, + {"bad key", []string{"get", "-f", path, "bad-key"}, "", true}, + {"bad key default", []string{"get", "-f", path, "--default", "d", "bad-key"}, "", true}, + {"bad key all default", []string{"get", "-f", path, "--all", "--default", "d", "user..name"}, "", true}, } for _, tt := range tests { out, err := runConfig(t, tt.args...) diff --git a/cmd/cli/iniconfig/iniconfig.go b/cmd/cli/iniconfig/iniconfig.go index de17bebe3..06ecbfaba 100644 --- a/cmd/cli/iniconfig/iniconfig.go +++ b/cmd/cli/iniconfig/iniconfig.go @@ -159,6 +159,9 @@ func parseSectionHeader(line string) (section, subsection string, err error) { if err2 != nil { return "", "", fmt.Errorf("invalid subsection in %q: %w", line, err2) } + if err2 := validateSubsection(sub); err2 != nil { + return "", "", fmt.Errorf("invalid section header %q: %w", line, err2) + } section = strings.ToLower(rawSection) subsection = sub } else { @@ -324,6 +327,18 @@ func validateSectionName(name string) error { return nil } +// validateSubsection rejects subsections that cannot round-trip: an empty one +// (indistinguishable from no subsection) or one with control characters. +func validateSubsection(sub string) error { + if sub == "" { + return fmt.Errorf("empty subsection") + } + if strings.ContainsAny(sub, "\n\r\x00") { + return fmt.Errorf("control character in subsection") + } + return nil +} + // validateVarName ensures a variable name contains only [A-Za-z0-9-] and // starts with a letter. func validateVarName(name string) error { @@ -361,8 +376,10 @@ func splitKey(key string) (section, subsection, variable string, err error) { } variable = key[lastDot+1:] section, subsection, hasSub := strings.Cut(key[:lastDot], ".") - if hasSub && subsection == "" { - return "", "", "", fmt.Errorf("invalid key %q: empty subsection", key) + if hasSub { + if err = validateSubsection(subsection); err != nil { + return "", "", "", fmt.Errorf("invalid key %q: %w", key, err) + } } return section, subsection, variable, nil } @@ -376,9 +393,6 @@ func ParseKey(key string) (section, subsection, variable string, err error) { return "", "", "", err } section, variable = strings.ToLower(section), strings.ToLower(variable) - if strings.ContainsAny(subsection, "\n\r\x00") { - return "", "", "", fmt.Errorf("invalid key %q: control character in subsection", key) - } if err2 := validateSectionName(section); err2 != nil { return "", "", "", fmt.Errorf("invalid key %q: %w", key, err2) } diff --git a/cmd/cli/iniconfig/iniconfig_test.go b/cmd/cli/iniconfig/iniconfig_test.go index b04617356..29e862381 100644 --- a/cmd/cli/iniconfig/iniconfig_test.go +++ b/cmd/cli/iniconfig/iniconfig_test.go @@ -464,7 +464,7 @@ func TestSet_RejectsOverLongLine(t *testing.T) { } func TestLoad_InvalidSectionName(t *testing.T) { - for _, content := range []string{"[co re]\nname = x\n", "[]\nname = x\n", "[co.re]\nname = x\n", "[co re \"sub\"]\nname = x\n"} { + for _, content := range []string{"[co re]\nname = x\n", "[]\nname = x\n", "[co.re]\nname = x\n", "[co re \"sub\"]\nname = x\n", "[branch \"\"]\nremote = x\n", "[branch \"a\x00b\"]\nremote = x\n"} { path := filepath.Join(t.TempDir(), "config") if err := os.WriteFile(path, []byte(content), 0o600); err != nil { t.Fatal(err) From 38b151cab0b8d0dae6b4b50bd8905e109ef69410 Mon Sep 17 00:00:00 2001 From: Eric Curtin Date: Thu, 1 Oct 2026 13:49:47 +0100 Subject: [PATCH 06/10] config: check first rune of variable name --- cmd/cli/iniconfig/iniconfig.go | 3 ++- cmd/cli/iniconfig/iniconfig_test.go | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/cmd/cli/iniconfig/iniconfig.go b/cmd/cli/iniconfig/iniconfig.go index 06ecbfaba..f43f7c820 100644 --- a/cmd/cli/iniconfig/iniconfig.go +++ b/cmd/cli/iniconfig/iniconfig.go @@ -21,6 +21,7 @@ import ( "path/filepath" "strings" "unicode" + "unicode/utf8" ) // maxConfigLineBytes is the hard cap on a single config line length (1 MiB). @@ -345,7 +346,7 @@ func validateVarName(name string) error { if name == "" { return fmt.Errorf("empty variable name") } - if !unicode.IsLetter(rune(name[0])) { + if r, _ := utf8.DecodeRuneInString(name); !unicode.IsLetter(r) { return fmt.Errorf("variable name %q must start with a letter", name) } if c, bad := badNameRune(name); bad { diff --git a/cmd/cli/iniconfig/iniconfig_test.go b/cmd/cli/iniconfig/iniconfig_test.go index 29e862381..d7bf083b8 100644 --- a/cmd/cli/iniconfig/iniconfig_test.go +++ b/cmd/cli/iniconfig/iniconfig_test.go @@ -250,6 +250,7 @@ func TestParseKey(t *testing.T) { {"core.bare", "core", "", "bare", false}, {"branch.main.remote", "branch", "main", "remote", false}, {"url.https://example.com/.insteadof", "url", "https://example.com/", "insteadof", false}, + {"core.\u05d0b", "core", "", "\u05d0b", false}, {"nokey", "", "", "", true}, {"section.", "", "", "", true}, } @@ -379,7 +380,7 @@ func TestSetSubsectionWithBracket(t *testing.T) { } func TestParseKey_Invalid(t *testing.T) { - for _, k := range []string{"core..name", "core.a\nb.name", "core.a\rb.name", "core.a\x00b.name", "core]x.name", `co"re.name`, "co re.name"} { + for _, k := range []string{"core..name", "core.a\nb.name", "core.a\rb.name", "core.a\x00b.name", "core]x.name", `co"re.name`, "co re.name", "core.\u0663x"} { if _, _, _, err := iniconfig.ParseKey(k); err == nil { t.Errorf("ParseKey(%q): expected error", k) } From 3468553312241fae039b401eedf5a72ad4a9452a Mon Sep 17 00:00:00 2001 From: Eric Curtin Date: Fri, 2 Oct 2026 01:42:39 +0100 Subject: [PATCH 07/10] config: reject junk after section header Signed-off-by: Eric Curtin --- cmd/cli/iniconfig/iniconfig.go | 7 +++++-- cmd/cli/iniconfig/iniconfig_test.go | 20 ++++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/cmd/cli/iniconfig/iniconfig.go b/cmd/cli/iniconfig/iniconfig.go index f43f7c820..e49fa8d15 100644 --- a/cmd/cli/iniconfig/iniconfig.go +++ b/cmd/cli/iniconfig/iniconfig.go @@ -139,14 +139,17 @@ func parse(data []byte) ([]Entry, error) { } // parseSectionHeader parses "[section]" or `[section "subsection"]`. -// Anything after the closing ']' (e.g. inline comments) is ignored. +// Only whitespace and an optional #/; comment may follow the closing ']'. // section is returned lowercased; subsection preserves case. func parseSectionHeader(line string) (section, subsection string, err error) { - // Find the closing bracket; ignore anything that follows (inline comment). closeIdx := closingBracket(line) if closeIdx < 0 { return "", "", fmt.Errorf("invalid section header: %q", line) } + // Reject trailing junk that is not a comment. + if rest := strings.TrimLeft(line[closeIdx+1:], " \t"); rest != "" && rest[0] != '#' && rest[0] != ';' { + return "", "", fmt.Errorf("invalid section header %q: unexpected text after ']'", line) + } inner := line[1:closeIdx] // Check for subsection: section "subsection" diff --git a/cmd/cli/iniconfig/iniconfig_test.go b/cmd/cli/iniconfig/iniconfig_test.go index d7bf083b8..cf68a4d22 100644 --- a/cmd/cli/iniconfig/iniconfig_test.go +++ b/cmd/cli/iniconfig/iniconfig_test.go @@ -135,6 +135,26 @@ func TestParse_SectionHeaderTrailingComment(t *testing.T) { }) } +func TestParse_SectionHeaderSuffix(t *testing.T) { + for _, hdr := range []string{"[core]", "[core] ", "[core]# c", "[core];c", "[core] \t# c"} { + roundTrip(t, hdr+"\n\tbare = false\n", []iniconfig.Entry{ + {Key: "core.bare", Value: "false"}, + }) + } + roundTrip(t, "[core \"x\"] ; c\n\tbare = false\n", []iniconfig.Entry{ + {Key: "core.x.bare", Value: "false"}, + }) + for _, hdr := range []string{"[core]typo", "[core] typo", "[core \"x\"]typo", "[core]]"} { + path := filepath.Join(t.TempDir(), "config") + if err := os.WriteFile(path, []byte(hdr+"\n\tbare = false\n"), 0o600); err != nil { + t.Fatal(err) + } + if _, err := iniconfig.Load(path); err == nil { + t.Errorf("Load(%q): expected error", hdr) + } + } +} + func TestParse_FilePermissionsPreserved(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "config") From 3b895c480f1f4af0410843c82d76817aaeeb31b9 Mon Sep 17 00:00:00 2001 From: Eric Curtin Date: Sat, 3 Oct 2026 03:00:38 +0100 Subject: [PATCH 08/10] cmd/cli: escape multiline values in config list Signed-off-by: Eric Curtin --- cmd/cli/commands/config.go | 2 +- cmd/cli/commands/config_test.go | 34 ++++++++++++++ cmd/cli/iniconfig/iniconfig.go | 13 +++++- cmd/cli/iniconfig/iniconfig_test.go | 72 +++++++++++++++++++++++++++++ 4 files changed, 119 insertions(+), 2 deletions(-) diff --git a/cmd/cli/commands/config.go b/cmd/cli/commands/config.go index 072503fc6..30dc1d904 100644 --- a/cmd/cli/commands/config.go +++ b/cmd/cli/commands/config.go @@ -258,7 +258,7 @@ func newConfigListCmd() *cobra.Command { } if showOrigin { for _, e := range f.Entries() { - cmd.Printf("file:%s\t%s=%s\n", f.Path(), e.Key, e.Value) + cmd.Printf("file:%s\t%s\n", f.Path(), e.Format()) } return nil } diff --git a/cmd/cli/commands/config_test.go b/cmd/cli/commands/config_test.go index 334cfadec..a9410f104 100644 --- a/cmd/cli/commands/config_test.go +++ b/cmd/cli/commands/config_test.go @@ -81,6 +81,40 @@ func TestConfigGet(t *testing.T) { } } +func TestConfigListOneLinePerKey(t *testing.T) { + for _, v := range []string{"plain", "a\nb", "a\rb", "a\r\nb", "\n", "tail\r", `q"uote`, "a#b"} { + path := filepath.Join(t.TempDir(), "config") + for _, args := range [][]string{{"set", "-f", path, "a.v", v}, {"set", "-f", path, "a.w", "next"}} { + if _, err := runConfig(t, args...); err != nil { + t.Fatalf("%q: %v", args, err) + } + } + plain, err := runConfig(t, "list", "-f", path) + if err != nil { + t.Fatal(err) + } + origin, err := runConfig(t, "list", "-f", path, "--show-origin") + if err != nil { + t.Fatal(err) + } + for name, out := range map[string]string{"list": plain, "show-origin": origin} { + if n := strings.Count(out, "\n"); n != 2 || strings.Contains(out, "\r") { + t.Errorf("%q %s: want one line per key, got %q", v, name, out) + } + } + // --show-origin must use the same value representation as list. + var want strings.Builder + for _, line := range strings.SplitAfter(plain, "\n") { + if line != "" { + want.WriteString("file:" + path + "\t" + line) + } + } + if origin != want.String() { + t.Errorf("%q: show-origin got %q, want %q", v, origin, want.String()) + } + } +} + func TestConfigLocationFlagsConflict(t *testing.T) { _, err := runConfig(t, "get", "--global", "--system", "user.name") if err == nil || !strings.Contains(err.Error(), "only one of") { diff --git a/cmd/cli/iniconfig/iniconfig.go b/cmd/cli/iniconfig/iniconfig.go index e49fa8d15..2ffd55606 100644 --- a/cmd/cli/iniconfig/iniconfig.go +++ b/cmd/cli/iniconfig/iniconfig.go @@ -279,6 +279,8 @@ func parseValue(raw string) (string, error) { switch raw[i] { case 'n': b.WriteByte('\n') + case 'r': + b.WriteByte('\r') case 't': b.WriteByte('\t') case 'b': @@ -630,6 +632,8 @@ func quoteValue(v string) string { b.WriteString(`\"`) case '\n': b.WriteString(`\n`) + case '\r': + b.WriteString(`\r`) case '\t': b.WriteString(`\t`) default: @@ -644,10 +648,17 @@ func quoteValue(v string) string { // Listing // ---------------------------------------------------------------------------- +// Format returns the entry as a single "key=value" line (without a newline). +// The value is quoted and escaped as in the config file, so it never contains +// a raw line break. +func (e Entry) Format() string { + return e.Key + "=" + quoteValue(e.Value) +} + // List writes all key=value pairs to w, one per line. func (f *File) List(w io.Writer) error { for _, e := range f.entries { - if _, err := fmt.Fprintf(w, "%s=%s\n", e.Key, e.Value); err != nil { + if _, err := fmt.Fprintln(w, e.Format()); err != nil { return err } } diff --git a/cmd/cli/iniconfig/iniconfig_test.go b/cmd/cli/iniconfig/iniconfig_test.go index cf68a4d22..472db8c03 100644 --- a/cmd/cli/iniconfig/iniconfig_test.go +++ b/cmd/cli/iniconfig/iniconfig_test.go @@ -1,6 +1,8 @@ package iniconfig_test import ( + "bytes" + "fmt" "os" "path/filepath" "slices" @@ -384,6 +386,76 @@ func TestSetRoundTripSpecialValues(t *testing.T) { } } +// controlCharValues are values containing line-break characters. +var controlCharValues = []string{ + "a\nb", "a\rb", "a\r\nb", "\n", "\r", "tail\r", "tail\n", "\r\n\r\n", "x\ty\n#z", +} + +func TestSetRoundTripControlChars(t *testing.T) { + for _, want := range controlCharValues { + t.Run(fmt.Sprintf("%q", want), func(t *testing.T) { + path := filepath.Join(t.TempDir(), "config") + f, _ := iniconfig.Load(path) + if err := f.Set("a.v", want); err != nil { + t.Fatal(err) + } + raw, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if bytes.ContainsRune(raw, '\r') || bytes.Count(raw, []byte("\n")) != 2 { + t.Errorf("file has raw line breaks in value: %q", raw) + } + f2, err := iniconfig.Load(path) + if err != nil { + t.Fatal(err) + } + if got, ok := f2.Get("a.v"); !ok || got != want { + t.Errorf("Get = %q, %v; want %q", got, ok, want) + } + }) + } +} + +func TestList_OneLinePerKey(t *testing.T) { + for _, want := range append([]string{"plain", "a b", " pad ", `q"uote`, `C:\dir`, "a#b"}, controlCharValues...) { + t.Run(fmt.Sprintf("%q", want), func(t *testing.T) { + path := filepath.Join(t.TempDir(), "config") + f, _ := iniconfig.Load(path) + if err := f.Set("a.v", want); err != nil { + t.Fatal(err) + } + if err := f.Set("a.w", "next"); err != nil { + t.Fatal(err) + } + var sb strings.Builder + if err := f.List(&sb); err != nil { + t.Fatal(err) + } + lines := strings.Split(strings.TrimSuffix(sb.String(), "\n"), "\n") + if len(lines) != 2 || strings.Contains(sb.String(), "\r") || lines[1] != "a.w=next" { + t.Fatalf("want one line per key, got %q", sb.String()) + } + // The listed value must decode back to the original. + val, ok := strings.CutPrefix(lines[0], "a.v=") + if !ok { + t.Fatalf("unexpected line %q", lines[0]) + } + rt := filepath.Join(t.TempDir(), "config") + if err := os.WriteFile(rt, []byte("[a]\n\tv = "+val+"\n"), 0o600); err != nil { + t.Fatal(err) + } + f2, err := iniconfig.Load(rt) + if err != nil { + t.Fatal(err) + } + if got, ok := f2.Get("a.v"); !ok || got != want { + t.Errorf("listed value decodes to %q, %v; want %q", got, ok, want) + } + }) + } +} + func TestSetSubsectionWithBracket(t *testing.T) { path := filepath.Join(t.TempDir(), "config") f, _ := iniconfig.Load(path) From 12ffb43f91541f00916631dc3212f0debc86a59f Mon Sep 17 00:00:00 2001 From: Eric Curtin Date: Sat, 3 Oct 2026 12:23:57 +0100 Subject: [PATCH 09/10] config: write invalid UTF-8 values byte-wise Signed-off-by: Eric Curtin --- cmd/cli/iniconfig/iniconfig.go | 7 ++++--- cmd/cli/iniconfig/iniconfig_test.go | 3 +++ 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/cmd/cli/iniconfig/iniconfig.go b/cmd/cli/iniconfig/iniconfig.go index 2ffd55606..0458e26a9 100644 --- a/cmd/cli/iniconfig/iniconfig.go +++ b/cmd/cli/iniconfig/iniconfig.go @@ -624,8 +624,9 @@ func quoteValue(v string) string { } var b strings.Builder b.WriteByte('"') - for _, c := range v { - switch c { + // Iterate bytes, not runes, so invalid UTF-8 is written back unchanged. + for i := 0; i < len(v); i++ { + switch c := v[i]; c { case '\\': b.WriteString(`\\`) case '"': @@ -637,7 +638,7 @@ func quoteValue(v string) string { case '\t': b.WriteString(`\t`) default: - b.WriteRune(c) + b.WriteByte(c) } } b.WriteByte('"') diff --git a/cmd/cli/iniconfig/iniconfig_test.go b/cmd/cli/iniconfig/iniconfig_test.go index 472db8c03..4c95d4dd3 100644 --- a/cmd/cli/iniconfig/iniconfig_test.go +++ b/cmd/cli/iniconfig/iniconfig_test.go @@ -369,6 +369,9 @@ func TestSetRoundTripSpecialValues(t *testing.T) { "a.trail": "value ", "a.lead": " value", "a.bs": `C:\dir\`, + // Invalid UTF-8 must survive both the quoted and unquoted paths. + "a.badquoted": "\xff#\xc3", + "a.badplain": "\xfe\x80", } for k, v := range vals { if err := f.Set(k, v); err != nil { From 762c05dba856d7854c9d0bbc37b9d64ca24d525d Mon Sep 17 00:00:00 2001 From: Eric Curtin Date: Sun, 4 Oct 2026 01:45:18 +0100 Subject: [PATCH 10/10] config: print to stdout, share flag and key helpers Signed-off-by: Eric Curtin --- cmd/cli/commands/config.go | 98 +++++++++++++++++---------------- cmd/cli/commands/config_test.go | 27 +++++++++ cmd/cli/iniconfig/iniconfig.go | 34 ++++++------ 3 files changed, 95 insertions(+), 64 deletions(-) diff --git a/cmd/cli/commands/config.go b/cmd/cli/commands/config.go index 30dc1d904..769c54264 100644 --- a/cmd/cli/commands/config.go +++ b/cmd/cli/commands/config.go @@ -17,15 +17,15 @@ import ( // // $XDG_CONFIG_HOME/model-runner/config // ~/.config/model-runner/config (fallback) -func defaultConfigPath() string { +func defaultConfigPath() (string, error) { if xdg := os.Getenv("XDG_CONFIG_HOME"); xdg != "" { - return filepath.Join(xdg, "model-runner", "config") + return filepath.Join(xdg, "model-runner", "config"), nil } home, err := os.UserHomeDir() if err != nil { - return filepath.Join(".config", "model-runner", "config") + return "", fmt.Errorf("cannot find the config directory (set XDG_CONFIG_HOME or use --file): %w", err) } - return filepath.Join(home, ".config", "model-runner", "config") + return filepath.Join(home, ".config", "model-runner", "config"), nil } // systemConfigPath returns the system-wide config file path. @@ -58,24 +58,43 @@ func resolveConfigPath(global, system bool, file string) (string, error) { return file, nil default: // --global is the default - return defaultConfigPath(), nil + return defaultConfigPath() } } -// loadConfig resolves the config path from the location flags and loads it. -func loadConfig(global, system bool, file string) (*iniconfig.File, error) { - path, err := resolveConfigPath(global, system, file) +// locationFlags holds the --global/--system/--file flags shared by subcommands. +type locationFlags struct { + global, system bool + file string +} + +// add registers the flags on cmd. +func (l *locationFlags) add(cmd *cobra.Command) { + cmd.Flags().BoolVar(&l.global, "global", false, "use the global (user-level) config file") + cmd.Flags().BoolVar(&l.system, "system", false, "use the system-wide config file") + cmd.Flags().StringVarP(&l.file, "file", "f", "", "use a specific config file") +} + +// path resolves the config file selected by the flags. +func (l *locationFlags) path() (string, error) { + return resolveConfigPath(l.global, l.system, l.file) +} + +// load resolves the config path and loads it. +func (l *locationFlags) load() (*iniconfig.File, error) { + path, err := l.path() if err != nil { return nil, err } return iniconfig.Load(path) } -// addLocationFlags adds the standard --global/--system/--file flags to a command. -func addLocationFlags(cmd *cobra.Command, global, system *bool, file *string) { - cmd.Flags().BoolVar(global, "global", false, "use the global (user-level) config file") - cmd.Flags().BoolVar(system, "system", false, "use the system-wide config file") - cmd.Flags().StringVarP(file, "file", "f", "", "use a specific config file") +// printLine writes s to stdout, prefixed with the file path if showOrigin. +func printLine(cmd *cobra.Command, f *iniconfig.File, showOrigin bool, s string) { + if showOrigin { + _, _ = fmt.Fprintf(cmd.OutOrStdout(), "file:%s\t", f.Path()) + } + _, _ = fmt.Fprintln(cmd.OutOrStdout(), s) } // newConfigCmd returns the top-level "config" command. @@ -123,9 +142,7 @@ Examples: // newConfigGetCmd implements "model-cli config get ". func newConfigGetCmd() *cobra.Command { var ( - global bool - system bool - file string + loc locationFlags defaultVal string hasDefault bool showAll bool @@ -148,7 +165,7 @@ Exit status is 1 if the key is not found (unless --default is given).`, if _, _, _, err := iniconfig.ParseKey(key); err != nil { return err } - f, err := loadConfig(global, system, file) + f, err := loc.load() if err != nil { return err } @@ -162,23 +179,19 @@ Exit status is 1 if the key is not found (unless --default is given).`, if len(vals) == 0 { if hasDefault { - cmd.Println(defaultVal) + printLine(cmd, f, false, defaultVal) return nil } return fmt.Errorf("key not found: %s", key) } for _, v := range vals { - if showOrigin { - cmd.Printf("file:%s\t%s\n", f.Path(), v) - } else { - cmd.Println(v) - } + printLine(cmd, f, showOrigin, v) } return nil }, } - addLocationFlags(c, &global, &system, &file) + loc.add(c) c.Flags().StringVar(&defaultVal, "default", "", "value to emit if the key is not set") c.Flags().BoolVar(&showAll, "all", false, "print all values for multi-valued keys") c.Flags().BoolVar(&showOrigin, "show-origin", false, "show the origin (file path) of each value") @@ -193,8 +206,7 @@ Exit status is 1 if the key is not found (unless --default is given).`, // newConfigSetCmd implements "model-cli config set ". func newConfigSetCmd() *cobra.Command { - var global, system bool - var file string + var loc locationFlags c := &cobra.Command{ Use: "set ", @@ -204,7 +216,7 @@ func newConfigSetCmd() *cobra.Command { If the key already exists its value is replaced. The file is written atomically.`, Args: cobra.ExactArgs(2), RunE: func(cmd *cobra.Command, args []string) error { - f, err := loadConfig(global, system, file) + f, err := loc.load() if err != nil { return err } @@ -212,14 +224,13 @@ If the key already exists its value is replaced. The file is written atomically. }, } - addLocationFlags(c, &global, &system, &file) + loc.add(c) return c } // newConfigUnsetCmd implements "model-cli config unset ". func newConfigUnsetCmd() *cobra.Command { - var global, system bool - var file string + var loc locationFlags c := &cobra.Command{ Use: "unset ", @@ -227,7 +238,7 @@ func newConfigUnsetCmd() *cobra.Command { Long: `Remove a config key (and all its values) from the file.`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - f, err := loadConfig(global, system, file) + f, err := loc.load() if err != nil { return err } @@ -235,14 +246,13 @@ func newConfigUnsetCmd() *cobra.Command { }, } - addLocationFlags(c, &global, &system, &file) + loc.add(c) return c } // newConfigListCmd implements "model-cli config list". func newConfigListCmd() *cobra.Command { - var global, system bool - var file string + var loc locationFlags var showOrigin bool c := &cobra.Command{ @@ -252,21 +262,18 @@ func newConfigListCmd() *cobra.Command { Long: `List all key=value pairs from the config file, one per line.`, Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { - f, err := loadConfig(global, system, file) + f, err := loc.load() if err != nil { return err } - if showOrigin { - for _, e := range f.Entries() { - cmd.Printf("file:%s\t%s\n", f.Path(), e.Format()) - } - return nil + for _, e := range f.Entries() { + printLine(cmd, f, showOrigin, e.Format()) } - return f.List(cmd.OutOrStdout()) + return nil }, } - addLocationFlags(c, &global, &system, &file) + loc.add(c) c.Flags().BoolVar(&showOrigin, "show-origin", false, "show the origin (file path) of each value") return c } @@ -287,8 +294,7 @@ func editorCommand() []string { // newConfigEditCmd implements "model-cli config edit". func newConfigEditCmd() *cobra.Command { - var global, system bool - var file string + var loc locationFlags c := &cobra.Command{ Use: "edit", @@ -299,7 +305,7 @@ The editor is determined by the VISUAL or EDITOR environment variables, falling back to vi on Unix and notepad on Windows.`, Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { - path, err := resolveConfigPath(global, system, file) + path, err := loc.path() if err != nil { return err } @@ -329,6 +335,6 @@ falling back to vi on Unix and notepad on Windows.`, }, } - addLocationFlags(c, &global, &system, &file) + loc.add(c) return c } diff --git a/cmd/cli/commands/config_test.go b/cmd/cli/commands/config_test.go index a9410f104..4b5407378 100644 --- a/cmd/cli/commands/config_test.go +++ b/cmd/cli/commands/config_test.go @@ -115,6 +115,33 @@ func TestConfigListOneLinePerKey(t *testing.T) { } } +func TestConfigGetWritesToStdout(t *testing.T) { + path := filepath.Join(t.TempDir(), "config") + if err := os.WriteFile(path, []byte("[user]\n\tname = a\n"), 0o600); err != nil { + t.Fatal(err) + } + cmd := newConfigCmd() + var stdout, stderr bytes.Buffer + cmd.SetOut(&stdout) + cmd.SetErr(&stderr) + cmd.SetArgs([]string{"get", "-f", path, "user.name"}) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + if stdout.String() != "a\n" || stderr.Len() != 0 { + t.Errorf("stdout=%q stderr=%q", stdout.String(), stderr.String()) + } +} + +func TestDefaultConfigPathNoHome(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", "") + t.Setenv("HOME", "") + t.Setenv("USERPROFILE", "") + if p, err := defaultConfigPath(); err == nil { + t.Skipf("home directory still resolved: %q", p) + } +} + func TestConfigLocationFlagsConflict(t *testing.T) { _, err := runConfig(t, "get", "--global", "--system", "user.name") if err == nil || !strings.Contains(err.Error(), "only one of") { diff --git a/cmd/cli/iniconfig/iniconfig.go b/cmd/cli/iniconfig/iniconfig.go index 0458e26a9..5cf0a66fb 100644 --- a/cmd/cli/iniconfig/iniconfig.go +++ b/cmd/cli/iniconfig/iniconfig.go @@ -411,6 +411,16 @@ func ParseKey(key string) (section, subsection, variable string, err error) { return section, subsection, variable, nil } +// canonicalFromKey validates a user-supplied dotted key and returns its +// canonical form. +func canonicalFromKey(key string) (string, error) { + section, subsection, variable, err := ParseKey(key) + if err != nil { + return "", err + } + return canonicalKey(section, subsection, variable), nil +} + // ---------------------------------------------------------------------------- // Querying // ---------------------------------------------------------------------------- @@ -418,29 +428,19 @@ func ParseKey(key string) (section, subsection, variable string, err error) { // Get returns the last value for the given canonical key. The second return // value is false if the key is not present. func (f *File) Get(key string) (string, bool) { - section, subsection, variable, err := ParseKey(key) - if err != nil { + vals := f.GetAll(key) + if len(vals) == 0 { return "", false } - canonical := canonicalKey(section, subsection, variable) - found := false - last := "" - for _, e := range f.entries { - if e.Key == canonical { - last = e.Value - found = true - } - } - return last, found + return vals[len(vals)-1], true } // GetAll returns all values for the given canonical key. func (f *File) GetAll(key string) []string { - section, subsection, variable, err := ParseKey(key) + canonical, err := canonicalFromKey(key) if err != nil { return nil } - canonical := canonicalKey(section, subsection, variable) var vals []string for _, e := range f.entries { if e.Key == canonical { @@ -457,11 +457,10 @@ func (f *File) GetAll(key string) []string { // Set writes key=value to the file, replacing the last existing occurrence or // appending if absent. The file is written atomically under a lock file. func (f *File) Set(key, value string) error { - section, subsection, variable, err := ParseKey(key) + canonical, err := canonicalFromKey(key) if err != nil { return err } - canonical := canonicalKey(section, subsection, variable) return f.writeAtomic(func(entries []Entry) []Entry { replaced := false for i := len(entries) - 1; i >= 0; i-- { @@ -480,11 +479,10 @@ func (f *File) Set(key, value string) error { // Unset removes all occurrences of key from the file. func (f *File) Unset(key string) error { - section, subsection, variable, err := ParseKey(key) + canonical, err := canonicalFromKey(key) if err != nil { return err } - canonical := canonicalKey(section, subsection, variable) return f.writeAtomic(func(entries []Entry) []Entry { out := entries[:0] for _, e := range entries {