From c8ab66bf4f17c2f98decd1719b9d4926e65a2b95 Mon Sep 17 00:00:00 2001 From: Simon Ostendorf Date: Fri, 13 Nov 2026 22:23:06 +0100 Subject: [PATCH 1/7] fix(gitconfig): apply useConfigOnly outside every profile The key was written only into the profile files, which are reached through an includeIf, so it never applied to a repository no rule matches. That is the only repository it was meant to protect. It is written unconditionally at the top of the managed include file now. Refs #8 --- internal/gitconfig/gitconfig.go | 30 +++++++++-- internal/gitconfig/gitconfig_test.go | 66 +++++++++++++++++++++++ internal/gitconfig/testdata/render.golden | 3 ++ 3 files changed, 94 insertions(+), 5 deletions(-) diff --git a/internal/gitconfig/gitconfig.go b/internal/gitconfig/gitconfig.go index c29a333..194cd04 100644 --- a/internal/gitconfig/gitconfig.go +++ b/internal/gitconfig/gitconfig.go @@ -118,15 +118,35 @@ func RenderProfile(p config.Profile, useConfigOnly bool) (string, error) { return b.String(), nil } -// renderIncludes renders the include file: one plain "[include]" for -// cfg.DefaultProfile, then one "[includeIf]" block per URL form and one -// "[includeIf \"gitdir/i:...\"]" block, per rule, in ascending specificity -// order, so a later, more specific block always wins over an earlier, less -// specific one, regardless of the configuration file's own rule order. +// renderIncludes renders the include file: an unconditional +// "user.useConfigOnly" when cfg.UseConfigOnly is set, then one plain +// "[include]" for cfg.DefaultProfile, then one "[includeIf]" block per URL +// form and one "[includeIf \"gitdir/i:...\"]" block, per rule, in ascending +// specificity order, so a later, more specific block always wins over an +// earlier, less specific one, regardless of the configuration file's own +// rule order. +// +// The unconditional block is load bearing, not decorative: every other +// "useConfigOnly" this package writes sits inside a profile file, reached +// only through an includeIf condition, so it only ever applies to a +// repository some rule, or the default profile, already matches. A +// repository no rule matches, and no default profile covers, reaches +// neither, so without this unconditional copy it carries no +// "useConfigOnly" at all, and git falls back to guessing an identity from +// the account's username and hostname, silently, exactly the outcome a +// repository with an undetermined identity is supposed to refuse. +// Verified directly against git 2.54.0: with this block removed, a +// repository under neither a rule nor a default profile committed +// successfully under a guessed identity despite every profile file +// itself setting "useConfigOnly = true". func renderIncludes(cfg config.Config, home string) string { var b strings.Builder b.WriteString(generatedHeader) + if cfg.UseConfigOnly { + b.WriteString("\n[user]\n\tuseConfigOnly = true\n") + } + if cfg.DefaultProfile != "" { fmt.Fprintf(&b, "\n[include]\n\tpath = %s\n", quoteValue(ProfilePath(home, cfg.DefaultProfile))) } diff --git a/internal/gitconfig/gitconfig_test.go b/internal/gitconfig/gitconfig_test.go index 0423927..fa70115 100644 --- a/internal/gitconfig/gitconfig_test.go +++ b/internal/gitconfig/gitconfig_test.go @@ -297,6 +297,72 @@ func TestRender_HasconfigFormsPerRule(t *testing.T) { } } +// TestRender_UnconditionalUseConfigOnly guards the one block in the +// include file that applies to a repository no rule matches and no +// default profile covers: every other "useConfigOnly" this package +// writes sits inside a profile file, reached only through an includeIf +// condition that such a repository never satisfies. +func TestRender_UnconditionalUseConfigOnly(t *testing.T) { + t.Parallel() + + t.Run("present before every includeIf block when useConfigOnly is on", func(t *testing.T) { + t.Parallel() + cfg := config.Config{ + Root: "/home/simon/Code", + Default: "{host}/{owner}/{repo}", + UseConfigOnly: true, + Profiles: map[string]config.Profile{"private": {"user.email": "simon@example.test"}}, + Rules: []config.Rule{{Host: "github.com", Profile: "private"}}, + } + rendered, err := gitconfig.Render(cfg, "/home/simon") + if err != nil { + t.Fatalf("Render() returned an unexpected error: %v", err) + } + includes := rendered[gitconfig.IncludePath("/home/simon")] + + unconditional := strings.Index(includes, "[user]\n\tuseConfigOnly = true") + if unconditional == -1 { + t.Fatalf("Render() include file has no unconditional useConfigOnly block:\n%s", includes) + } + if firstIncludeIf := strings.Index(includes, "[includeIf"); firstIncludeIf != -1 && firstIncludeIf < unconditional { + t.Fatalf("Render() places the unconditional useConfigOnly block after an includeIf block:\n%s", includes) + } + }) + + t.Run("absent when the config opts out", func(t *testing.T) { + t.Parallel() + cfg := config.Config{ + Root: "/home/simon/Code", + Default: "{host}/{owner}/{repo}", + UseConfigOnly: false, + Profiles: map[string]config.Profile{"private": {"user.email": "simon@example.test"}}, + Rules: []config.Rule{{Host: "github.com", Profile: "private"}}, + } + rendered, err := gitconfig.Render(cfg, "/home/simon") + if err != nil { + t.Fatalf("Render() returned an unexpected error: %v", err) + } + includes := rendered[gitconfig.IncludePath("/home/simon")] + + if strings.Contains(includes, "useConfigOnly") { + t.Fatalf("Render() include file carries useConfigOnly despite opting out:\n%s", includes) + } + }) + + t.Run("present even with no profiles and no rules at all", func(t *testing.T) { + t.Parallel() + cfg := config.Config{Root: "/home/simon/Code", Default: "{host}/{owner}/{repo}", UseConfigOnly: true} + rendered, err := gitconfig.Render(cfg, "/home/simon") + if err != nil { + t.Fatalf("Render() returned an unexpected error: %v", err) + } + includes := rendered[gitconfig.IncludePath("/home/simon")] + if !strings.Contains(includes, "[user]\n\tuseConfigOnly = true") { + t.Fatalf("Render() with no profiles and no rules = %q, want the unconditional useConfigOnly block still present", includes) + } + }) +} + func TestRender_GitdirBlock(t *testing.T) { t.Parallel() diff --git a/internal/gitconfig/testdata/render.golden b/internal/gitconfig/testdata/render.golden index 2404289..e644678 100644 --- a/internal/gitconfig/testdata/render.golden +++ b/internal/gitconfig/testdata/render.golden @@ -1,6 +1,9 @@ === /home/simon/.config/gitcd/includes.gitconfig === # Generated by "gitcd apply". Edits here are overwritten the next time it runs. +[user] + useConfigOnly = true + [include] path = "/home/simon/.config/gitcd/profiles/private.gitconfig" From db91ef8ec3284166017cfc1466e15118755d1334 Mon Sep 17 00:00:00 2001 From: Simon Ostendorf Date: Fri, 13 Nov 2026 22:23:06 +0100 Subject: [PATCH 2/7] feat(git): find and remove a global identity key Position agnostic, unlike the existing ordering check: a key set directly in the global config wins for every unmatched repository wherever it sits. Refs #8 --- internal/git/config.go | 114 ++++++++++++++++++++++++++++++++- internal/git/config_test.go | 123 ++++++++++++++++++++++++++++++++++++ 2 files changed, 235 insertions(+), 2 deletions(-) diff --git a/internal/git/config.go b/internal/git/config.go index 44c249c..dc010f0 100644 --- a/internal/git/config.go +++ b/internal/git/config.go @@ -112,6 +112,21 @@ func ConfigUnsetLocal(ctx context.Context, path, key string) error { return nil } +// ConfigUnsetGlobal runs "git config --global --unset-all key", removing +// every global value key carries. It wraps ErrConfigKeyNotSet, rather than +// failing, when key already carries no global value. +func ConfigUnsetGlobal(ctx context.Context, key string) error { + cmd := exec.CommandContext(ctx, "git", "config", "--global", "--unset-all", key) + if out, err := cmd.CombinedOutput(); err != nil { + var exitErr *exec.ExitError + if errors.As(err, &exitErr) && exitErr.ExitCode() == 5 { + return fmt.Errorf("git: unset global %s: %w", key, ErrConfigKeyNotSet) + } + return fmt.Errorf("git: unset global %s: %w: %s", key, err, strings.TrimSpace(string(out))) + } + return nil +} + // ConfigValue is one git config key's effective value together with the // file git read it from. type ConfigValue struct { @@ -187,8 +202,10 @@ func revParsePath(ctx context.Context, path, arg string) (string, error) { var configSectionHeader = regexp.MustCompile(`^\s*\[\s*([A-Za-z0-9_.-]+)(?:\s+"([^"]*)")?\s*\]`) // configKeyLine matches a git config key assignment line, capturing the -// key name. -var configKeyLine = regexp.MustCompile(`^\s*([A-Za-z][A-Za-z0-9-]*)\s*(?:=.*)?$`) +// key name, whether it carries an explicit "=" (group 2, literally "=" or +// unmatched), and, when it does, the value's raw text after it (group 3), +// not yet unquoted. +var configKeyLine = regexp.MustCompile(`^\s*([A-Za-z][A-Za-z0-9-]*)\s*(?:(=)\s*(.*))?$`) // IdentityKeyAfterInclude reports whether any of keys sits, in file order, // after the last bare "[include]" section (no subsection) that content @@ -199,6 +216,15 @@ var configKeyLine = regexp.MustCompile(`^\s*([A-Za-z][A-Za-z0-9-]*)\s*(?:=.*)?$` // silently. found is false, with no error, when content carries no bare // "[include]" section at all, or when no key in keys sits after the one it // does carry. +// +// This is a narrower, ordering only concern: a key sitting before the +// include is "safe" by this function's own definition only in the sense +// that it is never overridden by a value gitcd's own includeIf chain +// sets. It says nothing about FindGlobalIdentityValues' concern, which is +// broader and applies regardless of position: any of these keys set +// directly anywhere in this file is already "configuration" by git's own +// definition, so it is used for every repository, matched by some rule or +// not, with or without ever reaching this function's notion of "after". func IdentityKeyAfterInclude(content []byte, keys []string) (key string, found bool, err error) { lines := strings.Split(string(content), "\n") @@ -245,6 +271,90 @@ func IdentityKeyAfterInclude(content []byte, keys []string) (key string, found b return "", false, nil } +// GlobalIdentityValue is one identity key the global git configuration +// sets directly, with the value it carries. +type GlobalIdentityValue struct { + Key string + Value string +} + +// FindGlobalIdentityValues scans content, the raw text of the user's +// global git configuration file, for every key in keys that it sets +// directly, in content's own file order, regardless of position relative +// to any "[include]" or "[includeIf]" section: a key set this way is +// already "configuration" by git's own definition, so +// "user.useConfigOnly" never suppresses it, and it applies to every +// repository gitcd manages, one some rule matches or not, exactly the gap +// IdentityKeyAfterInclude's narrower, ordering only check does not cover. +// The returned slice is ordered the same as keys, one entry per key that +// is set, carrying the last value assigned to it when content sets it +// more than once, consistent with git's own "last value wins" semantics +// for a single valued key. +func FindGlobalIdentityValues(content []byte, keys []string) []GlobalIdentityValue { + wanted := make(map[string]bool, len(keys)) + for _, k := range keys { + wanted[strings.ToLower(k)] = true + } + + values := make(map[string]string, len(keys)) + section := "" + for _, line := range strings.Split(string(content), "\n") { + if m := configSectionHeader.FindStringSubmatch(line); m != nil { + section = strings.ToLower(m[1]) + continue + } + if section == "" { + continue + } + m := configKeyLine.FindStringSubmatch(line) + if m == nil { + continue + } + full := section + "." + strings.ToLower(m[1]) + if !wanted[full] { + continue + } + if m[2] != "=" { + // A bare key with no "=" at all carries the implicit boolean + // value "true", the same as "git config --bool" reports it. + values[full] = "true" + continue + } + values[full] = parseConfigValue(m[3]) + } + + var found []GlobalIdentityValue + for _, k := range keys { + if v, ok := values[strings.ToLower(k)]; ok { + found = append(found, GlobalIdentityValue{Key: k, Value: v}) + } + } + return found +} + +// parseConfigValue extracts a key assignment's value from raw, the text a +// configKeyLine match captures after its "=". A value quoted with a +// single pair of double quotes is unquoted, undoing quoteValue's own +// escaping of "\\" and "\""; git's own handling of a quoted value's +// escape sequences is otherwise reproduced only for those two characters, +// which is all gitconfig.quoteValue, the renderer on the other side of +// this value, ever produces, and all the identity keys this package cares +// about are ever expected to carry. An unquoted value is trimmed of +// surrounding whitespace only, with no attempt to strip a trailing +// comment: this package's other textual checks are equally light, and an +// identity value carrying an inline "#" or ";" comment is not a shape any +// of them are expected to see. +func parseConfigValue(raw string) string { + raw = strings.TrimSpace(raw) + if len(raw) >= 2 && raw[0] == '"' && raw[len(raw)-1] == '"' { + inner := raw[1 : len(raw)-1] + inner = strings.ReplaceAll(inner, `\"`, `"`) + inner = strings.ReplaceAll(inner, `\\`, `\`) + return inner + } + return raw +} + // sortedConfigPairs returns pairs's keys sorted, so a caller building // "-c key=value" arguments gets a deterministic order. func sortedConfigPairs(pairs map[string]string) []string { diff --git a/internal/git/config_test.go b/internal/git/config_test.go index 48f3206..6a8375e 100644 --- a/internal/git/config_test.go +++ b/internal/git/config_test.go @@ -339,3 +339,126 @@ func TestIdentityKeyAfterInclude(t *testing.T) { // --add" always inserts a new key into that last matching section, so an // identity key sitting between two include sections ends up before the // insertion point, safe, while one sitting after both does not. + +func TestFindGlobalIdentityValues(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + content string + want []git.GlobalIdentityValue + }{ + { + name: "no identity key at all", + content: "[core]\n\teditor = vim\n", + want: nil, + }, + { + name: "a key before any include, the position IdentityKeyAfterInclude calls safe", + content: "[user]\n\tname = Test User\n\temail = test@example.test\n" + "[include]\n\tpath = /other.gitconfig\n", + want: []git.GlobalIdentityValue{ + {Key: "user.name", Value: "Test User"}, + {Key: "user.email", Value: "test@example.test"}, + }, + }, + { + name: "a key after an include is found too, same as before it", + content: "[include]\n\tpath = /other.gitconfig\n" + "[user]\n\tsigningkey = ssh-ed25519 AAAA\n", + want: []git.GlobalIdentityValue{ + {Key: "user.signingkey", Value: "ssh-ed25519 AAAA"}, + }, + }, + { + name: "a quoted value is unquoted, escaped characters undone", + content: "[user]\n\tname = \"Jane \\\"The Dev\\\" Doe\"\n", + want: []git.GlobalIdentityValue{ + {Key: "user.name", Value: `Jane "The Dev" Doe`}, + }, + }, + { + name: "a bare boolean key with no equals sign carries true", + content: "[commit]\n\tgpgsign\n", + want: []git.GlobalIdentityValue{ + {Key: "commit.gpgsign", Value: "true"}, + }, + }, + { + name: "a key under an unrelated section is ignored", + content: "[alias]\n\tname = \"Test\"\n", + want: nil, + }, + { + name: "an includeIf section is scanned too, not only a bare include", + content: "[includeIf \"gitdir:/some/path/\"]\n\tsigningkey = ssh-ed25519 AAAA\n", + want: nil, + }, + { + name: "the last value wins when a key is set more than once", + content: "[user]\n\temail = first@example.test\n[user]\n\temail = second@example.test\n", + want: []git.GlobalIdentityValue{ + {Key: "user.email", Value: "second@example.test"}, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := git.FindGlobalIdentityValues([]byte(tt.content), git.IdentityKeys) + if len(got) != len(tt.want) { + t.Fatalf("FindGlobalIdentityValues() = %+v, want %+v", got, tt.want) + } + for i := range got { + if got[i] != tt.want[i] { + t.Fatalf("FindGlobalIdentityValues()[%d] = %+v, want %+v", i, got[i], tt.want[i]) + } + } + }) + } +} + +// TestFindGlobalIdentityValues_IncludeIfSectionNameRequiresNoSubsection +// documents a deliberate narrowing shared with IdentityKeyAfterInclude: +// "[includeIf \"...\"]" carries a subsection, the condition itself, so it +// is never mistaken for the section "includeif" when looking for an +// identity key, but it is also never itself scanned as a section named +// after its condition; the key lines under it belong to the subsectioned +// header's own section name, "includeif", which "user.name" and friends +// never collide with. This test exists so a future change to how +// sections are tracked has something to catch a regression against. +func TestFindGlobalIdentityValues_IncludeIfSectionNameRequiresNoSubsection(t *testing.T) { + t.Parallel() + content := "[includeIf \"gitdir:/some/path/\"]\n\tpath = /conditional.gitconfig\n[user]\n\temail = after@example.test\n" + got := git.FindGlobalIdentityValues([]byte(content), git.IdentityKeys) + want := []git.GlobalIdentityValue{{Key: "user.email", Value: "after@example.test"}} + if len(got) != 1 || got[0] != want[0] { + t.Fatalf("FindGlobalIdentityValues() = %+v, want %+v", got, want) + } +} + +func TestConfigUnsetGlobal(t *testing.T) { + setupGitEnv(t) + + t.Run("removes a value that was set", func(t *testing.T) { + if err := git.ConfigAddGlobal(context.Background(), "user.signingkey", "ssh-ed25519 AAAA"); err != nil { + t.Fatalf("ConfigAddGlobal() returned an unexpected error: %v", err) + } + + if err := git.ConfigUnsetGlobal(context.Background(), "user.signingkey"); err != nil { + t.Fatalf("ConfigUnsetGlobal() returned an unexpected error: %v", err) + } + + // A second unset of the same key now wraps ErrConfigKeyNotSet, + // which only happens once git itself reports no value left: the + // behavioural proof that the first call actually removed it. + if err := git.ConfigUnsetGlobal(context.Background(), "user.signingkey"); !errors.Is(err, git.ErrConfigKeyNotSet) { + t.Fatalf("ConfigUnsetGlobal() after removal, error = %v, want it to wrap ErrConfigKeyNotSet", err) + } + }) + + t.Run("unset a key that is not set wraps ErrConfigKeyNotSet instead of failing", func(t *testing.T) { + err := git.ConfigUnsetGlobal(context.Background(), "user.doesnotexist") + if !errors.Is(err, git.ErrConfigKeyNotSet) { + t.Fatalf("ConfigUnsetGlobal() error = %v, want it to wrap ErrConfigKeyNotSet", err) + } + }) +} From a12b789f7606e1c7b60659904abfa5cf2c55c5f3 Mon Sep 17 00:00:00 2001 From: Simon Ostendorf Date: Fri, 13 Nov 2026 22:23:17 +0100 Subject: [PATCH 3/7] feat(config): add keys to a profile without losing comments The edit goes through yaml.Node rather than a struct round trip, so a hand written configuration keeps the comments its author put there. Refs #8 --- internal/config/config.go | 19 +++++ internal/config/config_test.go | 41 ++++++++++ internal/config/edit.go | 133 +++++++++++++++++++++++++++++++++ internal/config/edit_test.go | 101 +++++++++++++++++++++++++ 4 files changed, 294 insertions(+) create mode 100644 internal/config/edit.go create mode 100644 internal/config/edit_test.go diff --git a/internal/config/config.go b/internal/config/config.go index 9ccd683..cfedcf3 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -226,6 +226,25 @@ func defaultForHome(home string) (Config, error) { return cfg, nil } +// Path returns the configuration file Load reads: the first of its +// candidate paths that exists, or its primary candidate, "~/.gitcd.yaml" +// resolved against the calling user's home directory, when neither does. +// A caller that edits the file a person maintains by hand, "init" moving +// a global identity key into a profile for instance, writes here, so an +// edit always lands on the same file Load would read back. +func Path() (string, error) { + home, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("config: find the home directory: %w", err) + } + for _, path := range candidatePaths(home) { + if _, err := os.Stat(path); err == nil { + return path, nil + } + } + return candidatePaths(home)[0], nil +} + // candidatePaths lists, in the order Load tries them, the paths a // configuration file may live at for a user whose home directory is home. func candidatePaths(home string) []string { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index c6eabbc..884f803 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -304,6 +304,47 @@ hosts: }) } +func TestPath(t *testing.T) { + t.Run("the primary candidate when neither file exists", func(t *testing.T) { + home := withHome(t) + + got, err := config.Path() + if err != nil { + t.Fatalf("Path() returned an unexpected error: %v", err) + } + if want := filepath.Join(home, ".gitcd.yaml"); got != want { + t.Fatalf("Path() = %q, want %q", got, want) + } + }) + + t.Run("the file Load would read when it already exists", func(t *testing.T) { + home := withHome(t) + writeFile(t, filepath.Join(home, ".gitcd.yaml"), "root: ~/elsewhere\n") + + got, err := config.Path() + if err != nil { + t.Fatalf("Path() returned an unexpected error: %v", err) + } + if want := filepath.Join(home, ".gitcd.yaml"); got != want { + t.Fatalf("Path() = %q, want %q", got, want) + } + }) + + t.Run("the xdg fallback when only it exists", func(t *testing.T) { + home := withHome(t) + xdgPath := filepath.Join(home, ".config", "gitcd", "config.yaml") + writeFile(t, xdgPath, "root: ~/elsewhere\n") + + got, err := config.Path() + if err != nil { + t.Fatalf("Path() returned an unexpected error: %v", err) + } + if got != xdgPath { + t.Fatalf("Path() = %q, want %q", got, xdgPath) + } + }) +} + func TestLoad_Profiles(t *testing.T) { t.Run("no file ships useConfigOnly true and no profiles", func(t *testing.T) { withHome(t) diff --git a/internal/config/edit.go b/internal/config/edit.go new file mode 100644 index 0000000..4f80f77 --- /dev/null +++ b/internal/config/edit.go @@ -0,0 +1,133 @@ +// Copyright 2026 Simon Ostendorf +// SPDX-License-Identifier: Apache-2.0 + +package config + +import ( + "errors" + "fmt" + "sort" + + "gopkg.in/yaml.v3" +) + +// AddProfileKeys adds or overwrites key/value pairs under profiles.name in +// the YAML document existing holds, creating the top level "profiles" +// map, name's own map, or the document itself, whichever is missing, and +// preserving every comment and the rest of the document's structure +// otherwise. existing may be empty, in which case a fresh document is +// built. +// +// This is the file a person edits by hand, not one gitcd owns and +// regenerates, so the edit goes through gopkg.in/yaml.v3's comment +// preserving Node tree rather than a decode into a plain struct followed +// by a full re-encode, which would silently drop every comment the file +// carried. +func AddProfileKeys(existing []byte, name string, keys map[string]string) ([]byte, error) { + doc, err := parseOrEmptyDocument(existing) + if err != nil { + return nil, err + } + + root := doc.Content[0] + if root.Kind != yaml.MappingNode { + return nil, fmt.Errorf("config: the existing configuration is not a YAML mapping at its top level") + } + + profiles := mappingChild(root, "profiles") + profile := mappingChild(profiles, name) + for _, key := range sortedStringKeys(keys) { + setScalarChild(profile, key, keys[key]) + } + + out, err := yaml.Marshal(doc) + if err != nil { + return nil, fmt.Errorf("config: render the updated configuration: %w", err) + } + return out, nil +} + +// parseOrEmptyDocument parses existing as a YAML document node, or +// returns a fresh document holding an empty mapping when existing is +// empty: a configuration file that does not exist yet, or exists but is +// blank, is not a parse error, matching LoadFrom's own treatment of a +// missing file. +func parseOrEmptyDocument(existing []byte) (*yaml.Node, error) { + if len(existing) == 0 { + return &yaml.Node{ + Kind: yaml.DocumentNode, + Content: []*yaml.Node{{Kind: yaml.MappingNode, Tag: "!!map"}}, + }, nil + } + + var doc yaml.Node + if err := yaml.Unmarshal(existing, &doc); err != nil { + return nil, fmt.Errorf("config: parse the existing configuration: %w", err) + } + if doc.Kind == 0 || len(doc.Content) == 0 { + // A document holding only comments, or nothing at all, decodes + // into a node with no content: the same empty case as a missing + // file, just already having gone through the parser. + return &yaml.Node{ + Kind: yaml.DocumentNode, + HeadComment: doc.HeadComment, + Content: []*yaml.Node{{Kind: yaml.MappingNode, Tag: "!!map"}}, + }, nil + } + return &doc, nil +} + +// errNotFound is returned internally by mappingLookup when key is not +// among parent's children; mappingChild and setScalarChild both treat it +// as "create one" rather than an error, so it never escapes this file. +var errNotFound = errors.New("config: key not found in mapping") + +// mappingChild returns parent's child mapping node keyed name, appending +// both the key and a fresh, empty mapping value when parent does not +// already carry one. +func mappingChild(parent *yaml.Node, name string) *yaml.Node { + if value, err := mappingLookup(parent, name); err == nil { + return value + } + key := &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: name} + value := &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"} + parent.Content = append(parent.Content, key, value) + return value +} + +// setScalarChild sets parent's child scalar value keyed key to value, +// updating an existing entry's value node in place so any style or +// comment that particular node already carries is kept, or appending a +// freshly built key/value pair when parent carries no such key yet. +func setScalarChild(parent *yaml.Node, key, value string) { + if existing, err := mappingLookup(parent, key); err == nil { + existing.SetString(value) + return + } + keyNode := &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: key} + valueNode := &yaml.Node{} + valueNode.SetString(value) + parent.Content = append(parent.Content, keyNode, valueNode) +} + +// mappingLookup returns parent's value node keyed key, or errNotFound +// when parent, a mapping node, carries no such key. +func mappingLookup(parent *yaml.Node, key string) (*yaml.Node, error) { + for i := 0; i+1 < len(parent.Content); i += 2 { + if parent.Content[i].Value == key { + return parent.Content[i+1], nil + } + } + return nil, errNotFound +} + +// sortedStringKeys returns m's keys sorted, so a caller writing them into +// a document a person reads gets a deterministic order. +func sortedStringKeys(m map[string]string) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} diff --git a/internal/config/edit_test.go b/internal/config/edit_test.go new file mode 100644 index 0000000..a37603c --- /dev/null +++ b/internal/config/edit_test.go @@ -0,0 +1,101 @@ +// Copyright 2026 Simon Ostendorf +// SPDX-License-Identifier: Apache-2.0 + +package config_test + +import ( + "path/filepath" + "strings" + "testing" + + "github.com/ostendorf-cloud/gitcd/internal/config" +) + +func TestAddProfileKeys(t *testing.T) { + t.Parallel() + + t.Run("builds a fresh document when none exists yet", func(t *testing.T) { + t.Parallel() + out, err := config.AddProfileKeys(nil, "global", map[string]string{"user.email": "me@example.test"}) + if err != nil { + t.Fatalf("AddProfileKeys() returned an unexpected error: %v", err) + } + + path := filepath.Join(t.TempDir(), "gitcd.yaml") + writeFile(t, path, string(out)) + cfg, err := config.LoadFrom(path) + if err != nil { + t.Fatalf("LoadFrom() the result returned an unexpected error: %v", err) + } + if got, want := cfg.Profiles["global"]["user.email"], "me@example.test"; got != want { + t.Fatalf("profiles.global.user.email = %q, want %q", got, want) + } + }) + + t.Run("adds a new profile without disturbing an existing one", func(t *testing.T) { + t.Parallel() + existing := "profiles:\n work:\n user.email: work@example.test\n" + + out, err := config.AddProfileKeys([]byte(existing), "global", map[string]string{"user.name": "Jane Doe"}) + if err != nil { + t.Fatalf("AddProfileKeys() returned an unexpected error: %v", err) + } + + path := filepath.Join(t.TempDir(), "gitcd.yaml") + writeFile(t, path, string(out)) + cfg, err := config.LoadFrom(path) + if err != nil { + t.Fatalf("LoadFrom() the result returned an unexpected error: %v", err) + } + if got, want := cfg.Profiles["work"]["user.email"], "work@example.test"; got != want { + t.Fatalf("profiles.work.user.email = %q, want %q, the existing profile must be untouched", got, want) + } + if got, want := cfg.Profiles["global"]["user.name"], "Jane Doe"; got != want { + t.Fatalf("profiles.global.user.name = %q, want %q", got, want) + } + }) + + t.Run("overwrites an existing key in the named profile rather than duplicating it", func(t *testing.T) { + t.Parallel() + existing := "profiles:\n global:\n user.email: old@example.test\n user.name: Old Name\n" + + out, err := config.AddProfileKeys([]byte(existing), "global", map[string]string{"user.email": "new@example.test"}) + if err != nil { + t.Fatalf("AddProfileKeys() returned an unexpected error: %v", err) + } + + path := filepath.Join(t.TempDir(), "gitcd.yaml") + writeFile(t, path, string(out)) + cfg, err := config.LoadFrom(path) + if err != nil { + t.Fatalf("LoadFrom() the result returned an unexpected error: %v", err) + } + if got, want := cfg.Profiles["global"]["user.email"], "new@example.test"; got != want { + t.Fatalf("profiles.global.user.email = %q, want %q", got, want) + } + if got, want := cfg.Profiles["global"]["user.name"], "Old Name"; got != want { + t.Fatalf("profiles.global.user.name = %q, want %q, an untouched key must survive", got, want) + } + }) + + t.Run("preserves a comment elsewhere in the document", func(t *testing.T) { + t.Parallel() + existing := "# a hand written note a person left for themselves\nroot: ~/Code\n" + + out, err := config.AddProfileKeys([]byte(existing), "global", map[string]string{"user.email": "me@example.test"}) + if err != nil { + t.Fatalf("AddProfileKeys() returned an unexpected error: %v", err) + } + if !strings.Contains(string(out), "a hand written note a person left for themselves") { + t.Fatalf("AddProfileKeys() dropped the document's own comment:\n%s", out) + } + }) + + t.Run("rejects a document that is not a mapping at its top level", func(t *testing.T) { + t.Parallel() + _, err := config.AddProfileKeys([]byte("- not\n- a\n- mapping\n"), "global", map[string]string{"user.email": "me@example.test"}) + if err == nil { + t.Fatal("AddProfileKeys() succeeded, want an error for a non mapping top level document") + } + }) +} From bb2200322451077fd5a99b3528dcf39905e5e4b8 Mon Sep 17 00:00:00 2001 From: Simon Ostendorf Date: Fri, 13 Nov 2026 22:23:17 +0100 Subject: [PATCH 4/7] feat(cli): report and move a leaked global identity init names every identity key the global config sets directly, says why it defeats the per remote identity, and moves them into a dedicated profile only when asked. The default never moves and never reads stdin, and both files are backed up before a write. Refs #8 --- cmd/gitcd/init.go | 220 +++++++++++++++++++++++++++--- cmd/gitcd/init_test.go | 303 +++++++++++++++++++++++++++++++++++++++++ cmd/gitcd/main.go | 2 +- 3 files changed, 507 insertions(+), 18 deletions(-) diff --git a/cmd/gitcd/init.go b/cmd/gitcd/init.go index 0e16b96..2a9e131 100644 --- a/cmd/gitcd/init.go +++ b/cmd/gitcd/init.go @@ -4,6 +4,7 @@ package main import ( + "bufio" "context" "errors" "flag" @@ -14,23 +15,50 @@ import ( "strings" "time" + "github.com/ostendorf-cloud/gitcd/internal/config" "github.com/ostendorf-cloud/gitcd/internal/git" "github.com/ostendorf-cloud/gitcd/internal/gitconfig" ) -// runInit implements "gitcd init": it adds exactly one "include.path" line -// to the user's global git configuration, pointing at the include file -// "gitcd apply" renders, so every identity profile rule actually takes -// effect. It refuses when a global identity key already sits after the -// last existing "[include]" section, since "git config --global --add" -// inserts a new key into that section rather than at the end of the file, -// and a key sitting after it would keep beating every includeIf value -// gitcd adds, silently. -func runInit(args []string, stdout, stderr io.Writer) int { +// defaultMovedProfileName is the profile "init" moves a global identity +// key into when "--profile" does not say otherwise. It is always a +// dedicated profile, never an existing one chosen by guesswork: merging +// into whichever profile happens to already exist would silently +// overwrite a key that profile set on purpose, the moment it happened to +// share a name with one of IdentityKeys. A fresh, dedicated profile is +// never ambiguous and is always safe to merge into again on a later run. +const defaultMovedProfileName = "global" + +// runInit implements "gitcd init". Beyond adding the "include.path" line +// described below, it first detects every identity key the global git +// configuration sets directly: "user.useConfigOnly" only stops git from +// guessing a default identity, it never suppresses a value that is +// already configured, so a key sitting anywhere in this file, matched by +// gitcd's own includeIf chain or not, defeats the refusal every profile's +// "useConfigOnly" is meant to provide for a repository no rule matches. +// "init" reports every such key, names the profile it would belong in, +// and, only when asked, moves it there and removes it from the global +// file, with a backup of both files it touches. +// +// "init" also adds exactly one "include.path" line to the user's global +// git configuration, pointing at the include file "gitcd apply" renders, +// so every identity profile rule actually takes effect. It refuses when a +// global identity key already sits after the last existing "[include]" +// section, since "git config --global --add" inserts a new key into that +// section rather than at the end of the file, and a key sitting after it +// would keep beating every includeIf value gitcd adds, silently. +func runInit(args []string, stdin io.Reader, stdout, stderr io.Writer) int { fset := flag.NewFlagSet("gitcd init", flag.ContinueOnError) fset.SetOutput(stderr) dryRun := fset.Bool("dry-run", false, "print what would change without writing anything") verbose := fset.Bool("verbose", false, "enable debug logging on stderr") + moveIdentity := fset.String( + "move-identity", "", + `what to do with an identity key found directly in the global git configuration: +"yes" moves it without a prompt, "ask" prompts on stderr and reads the answer from stdin, +"no", the default, only reports it`, + ) + profileFlag := fset.String("profile", "", "profile a moved identity key is written into (default \""+defaultMovedProfileName+"\")") if err := fset.Parse(args); err != nil { return 2 } @@ -39,6 +67,17 @@ func runInit(args []string, stdout, stderr io.Writer) int { fset.Usage() return 2 } + switch *moveIdentity { + case "", "no", "yes", "ask": + default: + fmt.Fprintf(stderr, "gitcd init: --move-identity must be \"yes\", \"ask\" or \"no\", got %q\n", *moveIdentity) + return 2 + } + target := defaultMovedProfileName + if *profileFlag != "" { + target = *profileFlag + } + logger := newLogger(*verbose, stderr) ctx := context.Background() @@ -62,6 +101,50 @@ func runInit(args []string, stdout, stderr io.Writer) int { return 1 } + // A single backup of globalPath as it was found, shared between the + // identity move below and the include line further down: both can + // write to globalPath in the same run, and a second, later backup + // would either collide with the first, since both resolve to the + // same once per second timestamp, or, if it did not collide, record + // an already modified file instead of the original one. + var backupPath string + backupGlobalConfig := func() error { + if backupPath != "" || len(content) == 0 { + return nil + } + backupPath = globalPath + ".gitcd-backup-" + time.Now().UTC().Format("20060102T150405Z") + if err := os.WriteFile(backupPath, content, 0o600); err != nil { + backupPath = "" + return fmt.Errorf("gitcd init: back up %s: %w", globalPath, err) + } + logger.Debug("backed up the global git configuration file", "path", backupPath) + fmt.Fprintf(stdout, "backed up %s to %s\n", globalPath, backupPath) + return nil + } + + if found := git.FindGlobalIdentityValues(content, git.IdentityKeys); len(found) > 0 { + reportGlobalIdentityKeys(stdout, globalPath, found, target) + + move := decideMove(*moveIdentity, stdin, stderr, found, target) + switch { + case !move: + fmt.Fprintln(stdout, "not moving them: pass --move-identity=yes to move them without a prompt, or --move-identity=ask to be asked") + case *dryRun: + fmt.Fprintf(stdout, "would move %s into profile %q and remove them from %s\n", describeIdentityKeys(found), target, globalPath) + default: + if err := moveGlobalIdentity(ctx, found, target, globalPath, backupGlobalConfig, stdout); err != nil { + fmt.Fprintf(stderr, "gitcd init: %v\n", err) + return 1 + } + refreshed, err := os.ReadFile(globalPath) + if err != nil { + fmt.Fprintf(stderr, "gitcd init: read %s: %v\n", globalPath, err) + return 1 + } + content = refreshed + } + } + if alreadyIncluded(content, includePath) { fmt.Fprintf(stdout, "gitcd is already initialized: %s already includes %s\n", globalPath, includePath) return 0 @@ -86,14 +169,9 @@ func runInit(args []string, stdout, stderr io.Writer) int { return 0 } - if len(content) > 0 { - backupPath := globalPath + ".gitcd-backup-" + time.Now().UTC().Format("20060102T150405Z") - if err := os.WriteFile(backupPath, content, 0o600); err != nil { - fmt.Fprintf(stderr, "gitcd init: back up %s: %v\n", globalPath, err) - return 1 - } - logger.Debug("backed up the global git configuration file", "path", backupPath) - fmt.Fprintf(stdout, "backed up %s to %s\n", globalPath, backupPath) + if err := backupGlobalConfig(); err != nil { + fmt.Fprintf(stderr, "%v\n", err) + return 1 } if err := os.MkdirAll(filepath.Dir(includePath), 0o755); err != nil { @@ -117,3 +195,111 @@ func runInit(args []string, stdout, stderr io.Writer) int { func alreadyIncluded(content []byte, includePath string) bool { return len(content) > 0 && strings.Contains(string(content), includePath) } + +// reportGlobalIdentityKeys prints every key found, its value, and the +// profile it would belong in, to stdout: the acceptance this command's +// detection side exists for is naming them, regardless of whether a move +// follows. +func reportGlobalIdentityKeys(stdout io.Writer, globalPath string, found []git.GlobalIdentityValue, target string) { + fmt.Fprintf(stdout, "%s sets %s directly, outside any profile:\n", globalPath, describeIdentityKeys(found)) + for _, v := range found { + fmt.Fprintf(stdout, " %s = %s\n", v.Key, v.Value) + } + fmt.Fprintln(stdout, `"user.useConfigOnly" only stops git from guessing a default identity; it does`) + fmt.Fprintln(stdout, "not suppress a value already configured here, so every repository gitcd") + fmt.Fprintln(stdout, "manages, including one no rule matches, still gets an identity from this file.") + fmt.Fprintf(stdout, "They belong in profile %q instead.\n", target) +} + +// describeIdentityKeys joins found's key names for a one line message. +func describeIdentityKeys(found []git.GlobalIdentityValue) string { + names := make([]string, len(found)) + for i, v := range found { + names[i] = v.Key + } + return strings.Join(names, ", ") +} + +// decideMove reports whether found should be moved, given moveIdentity, +// the "--move-identity" flag's value: "yes" moves without asking, "no" +// and the unset default never move and never read stdin, and "ask" prompts +// on stderr and reads the answer from stdin, defaulting to "no" on +// anything other than an explicit "y" or "yes", EOF included, so a closed +// or empty stdin, the shape a script or CI job's stdin commonly has, +// resolves to the safe answer without blocking. +func decideMove(moveIdentity string, stdin io.Reader, stderr io.Writer, found []git.GlobalIdentityValue, target string) bool { + switch moveIdentity { + case "yes": + return true + case "ask": + fmt.Fprintf(stderr, "move %s into profile %q and remove them from the global git configuration? [y/N] ", describeIdentityKeys(found), target) + line, _ := bufio.NewReader(stdin).ReadString('\n') + answer := strings.ToLower(strings.TrimSpace(line)) + return answer == "y" || answer == "yes" + default: + return false + } +} + +// moveGlobalIdentity moves found into profile target: it writes the +// updated profile into the configuration file config.Path resolves, +// backing it up first when it already exists, before removing each key +// from the global git configuration, backing that up through +// backupGlobalConfig first. Persisting the new copy before touching the +// original is deliberate: if the persist step fails, the global +// configuration is never touched at all, and if a later removal fails +// partway through, every value is already safely recorded in the +// profile, not lost. +func moveGlobalIdentity( + ctx context.Context, found []git.GlobalIdentityValue, target, globalPath string, backupGlobalConfig func() error, stdout io.Writer, +) error { + cfgPath, err := config.Path() + if err != nil { + return fmt.Errorf("gitcd init: %w", err) + } + + existingYAML, err := os.ReadFile(cfgPath) + if err != nil && !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("gitcd init: read %s: %w", cfgPath, err) + } + if len(existingYAML) > 0 { + cfgBackupPath := cfgPath + ".gitcd-backup-" + time.Now().UTC().Format("20060102T150405Z") + if err := os.WriteFile(cfgBackupPath, existingYAML, 0o600); err != nil { + return fmt.Errorf("gitcd init: back up %s: %w", cfgPath, err) + } + fmt.Fprintf(stdout, "backed up %s to %s\n", cfgPath, cfgBackupPath) + } + + keys := make(map[string]string, len(found)) + for _, v := range found { + keys[v.Key] = v.Value + } + updatedYAML, err := config.AddProfileKeys(existingYAML, target, keys) + if err != nil { + return fmt.Errorf("gitcd init: %w", err) + } + if err := os.MkdirAll(filepath.Dir(cfgPath), 0o755); err != nil { + return fmt.Errorf("gitcd init: create %s: %w", filepath.Dir(cfgPath), err) + } + if err := os.WriteFile(cfgPath, updatedYAML, 0o644); err != nil { + return fmt.Errorf("gitcd init: write %s: %w", cfgPath, err) + } + fmt.Fprintf(stdout, "moved %s into profile %q in %s\n", describeIdentityKeys(found), target, cfgPath) + + if err := backupGlobalConfig(); err != nil { + return err + } + for _, v := range found { + if err := git.ConfigUnsetGlobal(ctx, v.Key); err != nil { + return fmt.Errorf("gitcd init: remove %s from %s: %w", v.Key, globalPath, err) + } + } + fmt.Fprintf(stdout, "removed %s from %s\n", describeIdentityKeys(found), globalPath) + fmt.Fprintf( + stdout, + "profile %q is not applied automatically: add a rule, or set it as the defaultProfile, then run \"gitcd apply\"\n"+ + "to reverse this, restore the backups above, or set the keys shown earlier back with \"git config --global\"\n", + target, + ) + return nil +} diff --git a/cmd/gitcd/init_test.go b/cmd/gitcd/init_test.go index d03b10e..3e4aaeb 100644 --- a/cmd/gitcd/init_test.go +++ b/cmd/gitcd/init_test.go @@ -5,7 +5,10 @@ package main import ( "bytes" + "context" + "errors" "os" + "os/exec" "path/filepath" "strings" "testing" @@ -148,6 +151,306 @@ func TestRunInit_DryRunChangesNothing(t *testing.T) { } } +func TestRunInit_ReportsGlobalIdentityKeysWithoutMovingByDefault(t *testing.T) { + home := setupEnv(t) + globalPath := globalConfigPath(home) + + var stdout, stderr bytes.Buffer + code := run([]string{"init"}, &stdout, &stderr) + if code != 0 { + t.Fatalf("init = %d, want 0, stderr: %s", code, stderr.String()) + } + if !strings.Contains(stdout.String(), "user.email") { + t.Fatalf("init stdout = %q, want it to name user.email, set globally by setupEnv", stdout.String()) + } + if !strings.Contains(stdout.String(), `"global"`) { + t.Fatalf("init stdout = %q, want it to name the profile a move would use", stdout.String()) + } + + after, err := os.ReadFile(globalPath) + if err != nil { + t.Fatalf("read %s: %v", globalPath, err) + } + if !strings.Contains(string(after), "email") { + t.Fatalf("init removed the global identity key despite no --move-identity flag: %q", after) + } + // The include line is still added: reporting a leaked identity key + // must not block the rest of "init" from doing its ordinary job. + includePath := filepath.Join(home, ".config", "gitcd", "includes.gitconfig") + if !strings.Contains(string(after), includePath) { + t.Fatalf("global config after init = %q, want it to include %q", after, includePath) + } + + if _, err := os.Stat(filepath.Join(home, ".gitcd.yaml")); err == nil { + t.Fatal("init created ~/.gitcd.yaml despite no --move-identity flag, want it left untouched") + } +} + +func TestRunInit_MovesIdentityKeysWhenExplicitlyToldYes(t *testing.T) { + home := setupEnv(t) + globalPath := globalConfigPath(home) + + var stdout, stderr bytes.Buffer + code := runInit([]string{"--move-identity=yes"}, strings.NewReader(""), &stdout, &stderr) + if code != 0 { + t.Fatalf("init --move-identity=yes = %d, want 0, stderr: %s", code, stderr.String()) + } + + after, err := os.ReadFile(globalPath) + if err != nil { + t.Fatalf("read %s: %v", globalPath, err) + } + if strings.Contains(string(after), "Test User") || strings.Contains(string(after), "[email protected]") { + t.Fatalf("global config after a move = %q, want the identity keys removed", after) + } + + cfgPath := filepath.Join(home, ".gitcd.yaml") + cfgContent, err := os.ReadFile(cfgPath) + if err != nil { + t.Fatalf("init did not write %s: %v", cfgPath, err) + } + if !strings.Contains(string(cfgContent), "[email protected]") { + t.Fatalf("%s = %q, want the moved email under profiles.global", cfgPath, cfgContent) + } + + // The global git config must have a backup of its pre-move content, + // so the move is reversible. gitcd.yaml did not exist before this + // run, so no backup of it is expected here; a pre-existing + // gitcd.yaml getting backed up is covered separately below. + gitBackupEntries, err := os.ReadDir(filepath.Dir(globalPath)) + if err != nil { + t.Fatalf("read dir %s: %v", filepath.Dir(globalPath), err) + } + var sawGitBackup bool + for _, e := range gitBackupEntries { + if strings.HasPrefix(e.Name(), filepath.Base(globalPath)+".gitcd-backup-") { + sawGitBackup = true + } + } + if !sawGitBackup { + t.Fatalf("no backup of %s found, entries: %v", globalPath, gitBackupEntries) + } +} + +func TestRunInit_MoveBacksUpAnExistingGitcdYAMLBeforeEditingIt(t *testing.T) { + home := setupEnv(t) + cfgPath := filepath.Join(home, ".gitcd.yaml") + writeFile(t, cfgPath, "profiles:\n work:\n user.email: work@example.test\nrules:\n - host: github.com\n profile: work\n") + + var stdout, stderr bytes.Buffer + code := runInit([]string{"--move-identity=yes"}, strings.NewReader(""), &stdout, &stderr) + if code != 0 { + t.Fatalf("init --move-identity=yes = %d, want 0, stderr: %s", code, stderr.String()) + } + + after, err := os.ReadFile(cfgPath) + if err != nil { + t.Fatalf("read %s: %v", cfgPath, err) + } + if !strings.Contains(string(after), "work@example.test") { + t.Fatalf("%s after a move = %q, want the pre-existing profile untouched", cfgPath, after) + } + if !strings.Contains(string(after), "[email protected]") { + t.Fatalf("%s after a move = %q, want the moved email under profiles.global", cfgPath, after) + } + + entries, err := os.ReadDir(home) + if err != nil { + t.Fatalf("read dir %s: %v", home, err) + } + var backup string + for _, e := range entries { + if strings.HasPrefix(e.Name(), filepath.Base(cfgPath)+".gitcd-backup-") { + backup = filepath.Join(home, e.Name()) + } + } + if backup == "" { + t.Fatalf("no backup of the pre-existing %s found, entries: %v", cfgPath, entries) + } + backupContent, err := os.ReadFile(backup) + if err != nil { + t.Fatalf("read backup %s: %v", backup, err) + } + if !strings.Contains(string(backupContent), "work@example.test") || strings.Contains(string(backupContent), "[email protected]") { + t.Fatalf("backup content = %q, want it to hold the configuration exactly as it was before the move", backupContent) + } +} + +func TestRunInit_AsksInteractivelyAndMovesOnYesAnswer(t *testing.T) { + home := setupEnv(t) + + var stdout, stderr bytes.Buffer + code := runInit([]string{"--move-identity=ask"}, strings.NewReader("y\n"), &stdout, &stderr) + if code != 0 { + t.Fatalf("init --move-identity=ask (y) = %d, want 0, stderr: %s", code, stderr.String()) + } + if !strings.Contains(stderr.String(), "[y/N]") { + t.Fatalf("init --move-identity=ask stderr = %q, want a prompt", stderr.String()) + } + + after, err := os.ReadFile(globalConfigPath(home)) + if err != nil { + t.Fatalf("read global config: %v", err) + } + if strings.Contains(string(after), "[email protected]") { + t.Fatalf("global config after answering yes = %q, want the identity key removed", after) + } +} + +func TestRunInit_AsksInteractivelyAndSkipsOnNoAnswer(t *testing.T) { + home := setupEnv(t) + globalPath := globalConfigPath(home) + before, err := os.ReadFile(globalPath) + if err != nil { + t.Fatalf("read %s: %v", globalPath, err) + } + + var stdout, stderr bytes.Buffer + code := runInit([]string{"--move-identity=ask"}, strings.NewReader("n\n"), &stdout, &stderr) + if code != 0 { + t.Fatalf("init --move-identity=ask (n) = %d, want 0, stderr: %s", code, stderr.String()) + } + + after, err := os.ReadFile(globalPath) + if err != nil { + t.Fatalf("read %s: %v", globalPath, err) + } + // The include line is still added on a declined move; only the + // identity keys must survive untouched. + if !strings.Contains(string(after), "[email protected]") { + t.Fatalf("global config after declining = %q, want the identity key left in place", after) + } + if string(before) == string(after) { + t.Fatal("init made no change at all after declining the move, want the include line still added") + } +} + +func TestRunInit_AsksInteractivelyDefaultsToNoOnClosedStdin(t *testing.T) { + // A script or CI job commonly runs with an empty or closed stdin, + // never a terminal: "--move-identity=ask" must resolve to the safe + // "no" answer there instead of blocking forever waiting for a line + // that will never arrive. + home := setupEnv(t) + + var stdout, stderr bytes.Buffer + code := runInit([]string{"--move-identity=ask"}, strings.NewReader(""), &stdout, &stderr) + if code != 0 { + t.Fatalf("init --move-identity=ask (closed stdin) = %d, want 0, stderr: %s", code, stderr.String()) + } + + after, err := os.ReadFile(globalConfigPath(home)) + if err != nil { + t.Fatalf("read global config: %v", err) + } + if !strings.Contains(string(after), "[email protected]") { + t.Fatalf("global config after a closed stdin prompt = %q, want the identity key left in place", after) + } +} + +func TestRunInit_DryRunDescribesTheMoveWithoutWriting(t *testing.T) { + home := setupEnv(t) + globalPath := globalConfigPath(home) + before, err := os.ReadFile(globalPath) + if err != nil { + t.Fatalf("read %s: %v", globalPath, err) + } + + var stdout, stderr bytes.Buffer + code := runInit([]string{"--dry-run", "--move-identity=yes"}, strings.NewReader(""), &stdout, &stderr) + if code != 0 { + t.Fatalf("init --dry-run --move-identity=yes = %d, want 0, stderr: %s", code, stderr.String()) + } + if !strings.Contains(stdout.String(), "would move") { + t.Fatalf("init --dry-run stdout = %q, want it to describe the move", stdout.String()) + } + + after, err := os.ReadFile(globalPath) + if err != nil { + t.Fatalf("read %s: %v", globalPath, err) + } + if string(after) != string(before) { + t.Fatalf("init --dry-run changed the global config:\nbefore: %q\nafter: %q", before, after) + } + if _, err := os.Stat(filepath.Join(home, ".gitcd.yaml")); err == nil { + t.Fatal("init --dry-run created ~/.gitcd.yaml, want it left untouched") + } +} + +// TestRunInit_UnmatchedRepositoryRefusesToCommitAfterMove is the +// end-to-end acceptance proof for issue 8: reproduces the exact scenario +// a global identity defeats "useConfigOnly" for (a global user.name and +// user.email, a rule for one host only, no defaultProfile), moves the +// leaked keys out through "gitcd init --move-identity=yes", runs "gitcd +// apply", and then attempts a real commit, through the actual git +// binary, in a repository whose remote matches no rule. Before the move, +// this same sequence commits successfully under the leaked global +// identity; the assertion below is that it now fails instead. +func TestRunInit_UnmatchedRepositoryRefusesToCommitAfterMove(t *testing.T) { + home := setupEnv(t) + writeFile(t, filepath.Join(home, ".gitcd.yaml"), ` +rules: + - host: github.com + profile: work + +profiles: + work: + user.email: work@example.test +`) + + var stdout, stderr bytes.Buffer + if code := run([]string{"init", "--move-identity=yes"}, &stdout, &stderr); code != 0 { + t.Fatalf("init --move-identity=yes = %d, want 0, stderr: %s", code, stderr.String()) + } + stdout.Reset() + stderr.Reset() + if code := run([]string{"apply"}, &stdout, &stderr); code != 0 { + t.Fatalf("apply = %d, want 0, stderr: %s", code, stderr.String()) + } + + // Built by hand rather than through initRepository, which commits as + // part of its own setup: after the move, that commit would already + // be the one expected to fail, leaving nothing left for this test's + // own, deliberate commit attempt to prove. + repo := filepath.Join(home, "elsewhere", "unmatched") + if err := os.MkdirAll(repo, 0o755); err != nil { + t.Fatalf("create %s: %v", repo, err) + } + runGit(t, repo, "init", "--initial-branch=main") + runGit(t, repo, "remote", "add", "origin", "git@gitlab.com:someone/unmatched.git") + writeFile(t, filepath.Join(repo, "README.md"), "example\n") + runGit(t, repo, "add", "README.md") + + cmd := exec.CommandContext(context.Background(), "git", "-c", "core.editor=true", "commit", "-m", "fix: probe an unmatched repository") + cmd.Dir = repo + out, err := cmd.CombinedOutput() + if err == nil { + t.Fatalf("commit in an unmatched repository succeeded after moving the leaked identity, want it to fail: %s", out) + } + + effectiveEmail := runGitShowOriginAllowFailure(t, repo, "user.email") + if effectiveEmail != "" { + t.Fatalf("user.email resolved to %q in the unmatched repository, want it unset", effectiveEmail) + } +} + +// runGitShowOriginAllowFailure runs "git -C dir config --get key", +// returning its trimmed stdout, or "" when git exits 1 because the key +// carries no value at all, which is the expected outcome here, not a +// test failure. +func runGitShowOriginAllowFailure(t *testing.T, dir, key string) string { + t.Helper() + cmd := exec.CommandContext(context.Background(), "git", "-C", dir, "config", "--get", key) + out, err := cmd.Output() + if err != nil { + var exitErr *exec.ExitError + if errors.As(err, &exitErr) && exitErr.ExitCode() == 1 { + return "" + } + t.Fatalf("git -C %s config --get %s: %v", dir, key, err) + } + return strings.TrimSpace(string(out)) +} + func TestRunInit_NeverWritesToTheRealHomeDirectory(t *testing.T) { // A sanity check on this test file's own isolation, not gitcd's // behaviour: every init test above must run with os.UserHomeDir() diff --git a/cmd/gitcd/main.go b/cmd/gitcd/main.go index 8e563c5..284c967 100644 --- a/cmd/gitcd/main.go +++ b/cmd/gitcd/main.go @@ -54,7 +54,7 @@ func run(args []string, stdout, stderr io.Writer) int { case "path": return runPath(args[1:], stdout, stderr) case "init": - return runInit(args[1:], stdout, stderr) + return runInit(args[1:], os.Stdin, stdout, stderr) case "apply": return runApply(args[1:], stdout, stderr) case "profile": From 2fbe1ced463d6b27d1d2d74083a4ae4f873ec1e3 Mon Sep 17 00:00:00 2001 From: Simon Ostendorf Date: Fri, 13 Nov 2026 22:23:17 +0100 Subject: [PATCH 5/7] docs: state what useConfigOnly does and does not protect Refs #8 --- docs/concepts/gitcd.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/docs/concepts/gitcd.md b/docs/concepts/gitcd.md index d0383c1..53de138 100644 --- a/docs/concepts/gitcd.md +++ b/docs/concepts/gitcd.md @@ -49,6 +49,18 @@ identity: a generated `includeIf` config for the common case, and a pinned local config write for the explicit override, so a repository always has one clear, inspectable reason for the identity it carries. +- `user.useConfigOnly` only stops git from guessing `user.name` and `user.email` from the + account's username and hostname; it never suppresses a value that is already configured, + from any source. `apply` writes it twice for that reason: unconditionally at the top of the + generated include file, so a repository no rule matches and no `defaultProfile` covers still + refuses to commit, and again inside every profile file, redundant for a matched repository + but needed on its own wherever a profile file is read outside that include chain. Neither + copy protects against a `user.name`, `user.email`, `user.signingkey`, `gpg.format` or + `commit.gpgsign` set directly in the user's own global git configuration, outside gitcd's + generated files: that value is already "configuration" by git's own definition, and applies + to every repository, one some rule matches or not. `init` detects exactly this case, reports + every such key, and, only when asked, moves it into a profile and removes it from the global + file, with a backup. - `internal/scan` walks the configured root into repository values for `list`, `doctor` and `status` to consume, treating a `.git` file, not only a `.git` directory, as a repository boundary, so a linked worktree is never descended into by mistake. From 0f4e43340998bad3eb195b67bc1c35fe77e2e8cd Mon Sep 17 00:00:00 2001 From: Simon Ostendorf Date: Fri, 13 Nov 2026 23:06:00 +0100 Subject: [PATCH 6/7] fix(cli): refuse to move an identity into a profile in use Nothing enforced the promise that the keys never land in an existing profile or in the defaultProfile. Targeting the default made every unmatched repository inherit the leaked identity again, the exact bug the move exists to fix, and a deliberate value was overwritten in silence either way. The updated configuration is also written and validated before anything is removed from the global config, so a file that would not load no longer leaves the user with no identity at all, and a multi document file is refused by name rather than truncated. Refs #8 --- cmd/gitcd/init.go | 140 +++++++++++++++++++++++++++++----- cmd/gitcd/init_test.go | 141 +++++++++++++++++++++++++++++++++++ internal/config/edit.go | 61 ++++++++++++++- internal/config/edit_test.go | 36 +++++++++ 4 files changed, 357 insertions(+), 21 deletions(-) diff --git a/cmd/gitcd/init.go b/cmd/gitcd/init.go index 2a9e131..ee7a1b2 100644 --- a/cmd/gitcd/init.go +++ b/cmd/gitcd/init.go @@ -18,15 +18,19 @@ import ( "github.com/ostendorf-cloud/gitcd/internal/config" "github.com/ostendorf-cloud/gitcd/internal/git" "github.com/ostendorf-cloud/gitcd/internal/gitconfig" + "gopkg.in/yaml.v3" ) // defaultMovedProfileName is the profile "init" moves a global identity -// key into when "--profile" does not say otherwise. It is always a -// dedicated profile, never an existing one chosen by guesswork: merging +// key into when "--profile" does not say otherwise. targetProfileConflict +// enforces that the chosen name, this default or an explicit "--profile", +// is always a dedicated profile, never an existing one chosen by +// guesswork, and never the profile defaultProfile already names: merging // into whichever profile happens to already exist would silently // overwrite a key that profile set on purpose, the moment it happened to -// share a name with one of IdentityKeys. A fresh, dedicated profile is -// never ambiguous and is always safe to merge into again on a later run. +// share a name with one of IdentityKeys, and landing in defaultProfile +// would make the leaked identity the default identity again, every +// unmatched repository's own, exactly the bug this move exists to fix. const defaultMovedProfileName = "global" // runInit implements "gitcd init". Beyond adding the "include.path" line @@ -115,7 +119,7 @@ func runInit(args []string, stdin io.Reader, stdout, stderr io.Writer) int { backupPath = globalPath + ".gitcd-backup-" + time.Now().UTC().Format("20060102T150405Z") if err := os.WriteFile(backupPath, content, 0o600); err != nil { backupPath = "" - return fmt.Errorf("gitcd init: back up %s: %w", globalPath, err) + return fmt.Errorf("back up %s: %w", globalPath, err) } logger.Debug("backed up the global git configuration file", "path", backupPath) fmt.Fprintf(stdout, "backed up %s to %s\n", globalPath, backupPath) @@ -126,6 +130,22 @@ func runInit(args []string, stdin io.Reader, stdout, stderr io.Writer) int { reportGlobalIdentityKeys(stdout, globalPath, found, target) move := decideMove(*moveIdentity, stdin, stderr, found, target) + if move { + cfgPath, err := config.Path() + if err != nil { + fmt.Fprintf(stderr, "gitcd init: %v\n", err) + return 1 + } + conflict, err := targetProfileConflict(cfgPath, target) + if err != nil { + fmt.Fprintf(stderr, "gitcd init: %v\n", err) + return 1 + } + if conflict != "" { + fmt.Fprintf(stderr, "gitcd init: %s\n", conflict) + return 1 + } + } switch { case !move: fmt.Fprintln(stdout, "not moving them: pass --move-identity=yes to move them without a prompt, or --move-identity=ask to be asked") @@ -220,6 +240,68 @@ func describeIdentityKeys(found []git.GlobalIdentityValue) string { return strings.Join(names, ", ") } +// targetProfileConflict reports, as a message ready to print, why "init" +// must refuse to move an identity key into profile target, or "" when +// target is clear to use. It reads cfgPath itself, with its own, lenient +// parse into only the two fields this check needs, rather than going +// through config.LoadFrom: this is purely a name collision check, which +// must not block on some unrelated problem the rest of the file carries, +// a problem moveGlobalIdentity's own, stricter validation is already +// responsible for catching once there is something to write. +// +// Two collisions are refused, matching pull request 12's review: +// - target already names defaultProfile. Never allowed, with no +// override: the found keys are, by definition, a leaked identity that +// "useConfigOnly" could not suppress; landing them in defaultProfile +// would make that same leaked identity the default for every +// repository no rule matches, again, the exact bug this move exists +// to fix, just rearmed in a broader form. +// - target already names an existing profile. Never allowed either, +// with no override flag: AddProfileKeys overwrites a key by name, so +// merging into a profile that already exists would silently replace +// a value chosen on purpose the moment it happened to share a key +// name with one of git.IdentityKeys. A dedicated profile costs +// nothing to create under a name nothing else uses yet, so there is +// no legitimate reason to merge instead, and the refusal names the +// conflict so the user can rerun with a different "--profile". +func targetProfileConflict(cfgPath, target string) (string, error) { + raw, err := os.ReadFile(cfgPath) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return "", nil + } + return "", fmt.Errorf("read %s: %w", cfgPath, err) + } + + var doc struct { + Profiles map[string]map[string]any `yaml:"profiles"` + DefaultProfile string `yaml:"defaultProfile"` + } + if err := yaml.Unmarshal(raw, &doc); err != nil { + // A file this lenient a parse cannot even make sense of is left + // for moveGlobalIdentity's own, stricter validation to reject + // with a precise error once there is something to write. + return "", nil + } + + if doc.DefaultProfile != "" && doc.DefaultProfile == target { + return fmt.Sprintf( + "profile %q is the defaultProfile in %s: moving the leaked identity there would make every repository no rule\n"+ + "matches inherit it again, the exact bug this move exists to fix\n"+ + "choose a different name with --profile, or clear defaultProfile first and run \"gitcd apply\" again", + target, cfgPath, + ), nil + } + if _, ok := doc.Profiles[target]; ok { + return fmt.Sprintf( + "profile %q already exists in %s: moving into it could silently overwrite a value chosen on purpose\n"+ + "choose a different, not yet used name with --profile", + target, cfgPath, + ), nil + } + return "", nil +} + // decideMove reports whether found should be moved, given moveIdentity, // the "--move-identity" flag's value: "yes" moves without asking, "no" // and the unset default never move and never read stdin, and "ask" prompts @@ -243,29 +325,30 @@ func decideMove(moveIdentity string, stdin io.Reader, stderr io.Writer, found [] // moveGlobalIdentity moves found into profile target: it writes the // updated profile into the configuration file config.Path resolves, -// backing it up first when it already exists, before removing each key -// from the global git configuration, backing that up through -// backupGlobalConfig first. Persisting the new copy before touching the -// original is deliberate: if the persist step fails, the global -// configuration is never touched at all, and if a later removal fails -// partway through, every value is already safely recorded in the +// backing it up first when it already exists, validates that the +// updated file actually loads, before removing each key from the global +// git configuration, backing that up through backupGlobalConfig first. +// Persisting the new copy before touching the original is deliberate: if +// the persist step fails, or the persisted copy does not load, the +// global configuration is never touched at all, and if a later removal +// fails partway through, every value is already safely recorded in the // profile, not lost. func moveGlobalIdentity( ctx context.Context, found []git.GlobalIdentityValue, target, globalPath string, backupGlobalConfig func() error, stdout io.Writer, ) error { cfgPath, err := config.Path() if err != nil { - return fmt.Errorf("gitcd init: %w", err) + return err } existingYAML, err := os.ReadFile(cfgPath) if err != nil && !errors.Is(err, os.ErrNotExist) { - return fmt.Errorf("gitcd init: read %s: %w", cfgPath, err) + return fmt.Errorf("read %s: %w", cfgPath, err) } if len(existingYAML) > 0 { cfgBackupPath := cfgPath + ".gitcd-backup-" + time.Now().UTC().Format("20060102T150405Z") if err := os.WriteFile(cfgBackupPath, existingYAML, 0o600); err != nil { - return fmt.Errorf("gitcd init: back up %s: %w", cfgPath, err) + return fmt.Errorf("back up %s: %w", cfgPath, err) } fmt.Fprintf(stdout, "backed up %s to %s\n", cfgPath, cfgBackupPath) } @@ -276,13 +359,32 @@ func moveGlobalIdentity( } updatedYAML, err := config.AddProfileKeys(existingYAML, target, keys) if err != nil { - return fmt.Errorf("gitcd init: %w", err) + return err } if err := os.MkdirAll(filepath.Dir(cfgPath), 0o755); err != nil { - return fmt.Errorf("gitcd init: create %s: %w", filepath.Dir(cfgPath), err) + return fmt.Errorf("create %s: %w", filepath.Dir(cfgPath), err) + } + + // Written to a temporary file first and validated with config.LoadFrom + // before it ever reaches cfgPath: writing an updated configuration + // that then fails to load, for instance because the file already + // carried an unknown key, must never reach the point of removing the + // identity from the global git configuration below, the comment on + // this function's own name already argues for persisting before + // destroying, and persisting something unreadable does not satisfy + // that argument. Validating against a temporary file, rather than + // cfgPath itself, means a failure here leaves cfgPath exactly as it + // was, with nothing left to restore. + tmpPath := cfgPath + ".gitcd-validate-tmp" + if err := os.WriteFile(tmpPath, updatedYAML, 0o644); err != nil { + return fmt.Errorf("write %s: %w", tmpPath, err) + } + defer func() { _ = os.Remove(tmpPath) }() + if _, err := config.LoadFrom(tmpPath); err != nil { + return fmt.Errorf("the updated configuration would not load, aborting before touching %s: %w", globalPath, err) } - if err := os.WriteFile(cfgPath, updatedYAML, 0o644); err != nil { - return fmt.Errorf("gitcd init: write %s: %w", cfgPath, err) + if err := os.Rename(tmpPath, cfgPath); err != nil { + return fmt.Errorf("write %s: %w", cfgPath, err) } fmt.Fprintf(stdout, "moved %s into profile %q in %s\n", describeIdentityKeys(found), target, cfgPath) @@ -291,7 +393,7 @@ func moveGlobalIdentity( } for _, v := range found { if err := git.ConfigUnsetGlobal(ctx, v.Key); err != nil { - return fmt.Errorf("gitcd init: remove %s from %s: %w", v.Key, globalPath, err) + return fmt.Errorf("remove %s from %s: %w", v.Key, globalPath, err) } } fmt.Fprintf(stdout, "removed %s from %s\n", describeIdentityKeys(found), globalPath) diff --git a/cmd/gitcd/init_test.go b/cmd/gitcd/init_test.go index 3e4aaeb..3b552b5 100644 --- a/cmd/gitcd/init_test.go +++ b/cmd/gitcd/init_test.go @@ -451,6 +451,147 @@ func runGitShowOriginAllowFailure(t *testing.T, dir, key string) string { return strings.TrimSpace(string(out)) } +// TestRunInit_RefusesToTargetADeliberateProfileThatIsAlsoTheDefault is +// the exact reproduction pull request 12's review returned as a blocker: +// a profile that already carries deliberate values and is also +// defaultProfile, named explicitly with "--profile". Moving the leaked +// global identity there would both overwrite the deliberate values and +// make the leaked identity the default for every repository no rule +// matches, re-arming the bug this command exists to fix in a broader +// form. "init" must refuse instead, and must not change the file. +func TestRunInit_RefusesToTargetADeliberateProfileThatIsAlsoTheDefault(t *testing.T) { + home := setupEnv(t) + cfgPath := filepath.Join(home, ".gitcd.yaml") + writeFile(t, cfgPath, ` +profiles: + everyday: + user.name: Deliberate Name + user.email: deliberate@example.com +defaultProfile: everyday +`) + before, err := os.ReadFile(cfgPath) + if err != nil { + t.Fatalf("read %s: %v", cfgPath, err) + } + + var stdout, stderr bytes.Buffer + code := runInit([]string{"--move-identity=yes", "--profile=everyday"}, strings.NewReader(""), &stdout, &stderr) + if code == 0 { + t.Fatalf("init --move-identity=yes --profile=everyday = 0, want a refusal when the target is the defaultProfile, stdout: %s", stdout.String()) + } + if !strings.Contains(stderr.String(), "defaultProfile") { + t.Fatalf("init stderr = %q, want it to name the defaultProfile conflict", stderr.String()) + } + + after, err := os.ReadFile(cfgPath) + if err != nil { + t.Fatalf("read %s: %v", cfgPath, err) + } + if string(after) != string(before) { + t.Fatalf("init changed %s despite refusing:\nbefore: %q\nafter: %q", cfgPath, before, after) + } + + globalAfter, err := os.ReadFile(globalConfigPath(home)) + if err != nil { + t.Fatalf("read global config: %v", err) + } + if !strings.Contains(string(globalAfter), "Test User") || !strings.Contains(string(globalAfter), "[email protected]") { + t.Fatalf("init removed the global identity despite refusing the move: %q", globalAfter) + } +} + +// TestRunInit_RefusesToOverwriteAPreExistingProfileNamedGlobal covers the +// review's second reproduction of the same blocker: no "--profile" flag +// at all, but a profile already sits under the default target name and +// already carries deliberate values. "init" must refuse rather than +// silently overwriting them. +func TestRunInit_RefusesToOverwriteAPreExistingProfileNamedGlobal(t *testing.T) { + home := setupEnv(t) + cfgPath := filepath.Join(home, ".gitcd.yaml") + writeFile(t, cfgPath, ` +profiles: + global: + user.email: deliberate@example.test + user.name: Deliberate Name +`) + before, err := os.ReadFile(cfgPath) + if err != nil { + t.Fatalf("read %s: %v", cfgPath, err) + } + + var stdout, stderr bytes.Buffer + code := runInit([]string{"--move-identity=yes"}, strings.NewReader(""), &stdout, &stderr) + if code == 0 { + t.Fatalf("init --move-identity=yes = 0, want a refusal when profile %q already exists, stdout: %s", defaultMovedProfileName, stdout.String()) + } + if !strings.Contains(stderr.String(), "already exists") { + t.Fatalf("init stderr = %q, want it to name the existing profile conflict", stderr.String()) + } + + after, err := os.ReadFile(cfgPath) + if err != nil { + t.Fatalf("read %s: %v", cfgPath, err) + } + if string(after) != string(before) { + t.Fatalf("init changed %s despite refusing:\nbefore: %q\nafter: %q", cfgPath, before, after) + } +} + +// TestRunInit_AbortsTheMoveWhenTheUpdatedConfigWouldNotLoad covers the +// review's "should": a pre-existing ~/.gitcd.yaml that already carries an +// unknown key still carries it once AddProfileKeys merges the moved +// identity in, so the result does not load either. "init" must abort +// before removing anything from the global git configuration, rather +// than exiting 0 with the identity gone from everywhere and a gitcd +// configuration that does not load. +func TestRunInit_AbortsTheMoveWhenTheUpdatedConfigWouldNotLoad(t *testing.T) { + home := setupEnv(t) + globalPath := globalConfigPath(home) + cfgPath := filepath.Join(home, ".gitcd.yaml") + writeFile(t, cfgPath, "totallyUnknownKey: 1\n") + + beforeCfg, err := os.ReadFile(cfgPath) + if err != nil { + t.Fatalf("read %s: %v", cfgPath, err) + } + beforeGlobal, err := os.ReadFile(globalPath) + if err != nil { + t.Fatalf("read %s: %v", globalPath, err) + } + + var stdout, stderr bytes.Buffer + code := runInit([]string{"--move-identity=yes"}, strings.NewReader(""), &stdout, &stderr) + if code == 0 { + t.Fatalf("init --move-identity=yes = 0, want a failure when the updated configuration would not load, stdout: %s", stdout.String()) + } + + afterCfg, err := os.ReadFile(cfgPath) + if err != nil { + t.Fatalf("read %s: %v", cfgPath, err) + } + if string(afterCfg) != string(beforeCfg) { + t.Fatalf("init changed %s despite aborting:\nbefore: %q\nafter: %q", cfgPath, beforeCfg, afterCfg) + } + + afterGlobal, err := os.ReadFile(globalPath) + if err != nil { + t.Fatalf("read %s: %v", globalPath, err) + } + if string(afterGlobal) != string(beforeGlobal) { + t.Fatalf("init removed the global identity despite aborting:\nbefore: %q\nafter: %q", beforeGlobal, afterGlobal) + } + + entries, err := os.ReadDir(home) + if err != nil { + t.Fatalf("read dir %s: %v", home, err) + } + for _, e := range entries { + if strings.HasPrefix(e.Name(), filepath.Base(cfgPath)+".gitcd-validate-tmp") { + t.Fatalf("init left a temporary validation file behind: %s", e.Name()) + } + } +} + func TestRunInit_NeverWritesToTheRealHomeDirectory(t *testing.T) { // A sanity check on this test file's own isolation, not gitcd's // behaviour: every init test above must run with os.UserHomeDir() diff --git a/internal/config/edit.go b/internal/config/edit.go index 4f80f77..e3e317b 100644 --- a/internal/config/edit.go +++ b/internal/config/edit.go @@ -4,8 +4,10 @@ package config import ( + "bytes" "errors" "fmt" + "io" "sort" "gopkg.in/yaml.v3" @@ -47,11 +49,26 @@ func AddProfileKeys(existing []byte, name string, keys map[string]string) ([]byt return out, nil } -// parseOrEmptyDocument parses existing as a YAML document node, or +// ErrMultipleDocuments is wrapped into the error AddProfileKeys returns +// when the existing configuration holds more than one YAML document: +// gopkg.in/yaml.v3 decodes and re-encodes only the first one, which would +// silently drop every document after it from the file this command +// rewrites. +var ErrMultipleDocuments = errors.New("config: the existing configuration holds more than one YAML document") + +// parseOrEmptyDocument parses existing as a single YAML document node, or // returns a fresh document holding an empty mapping when existing is // empty: a configuration file that does not exist yet, or exists but is // blank, is not a parse error, matching LoadFrom's own treatment of a // missing file. +// +// existing holding more than one YAML document is refused with +// ErrMultipleDocuments rather than edited: this package's own +// yaml.v3.Node round trip, like config.LoadFrom, only ever sees the +// first document, so silently writing back just that one would discard +// every document after it with nothing printed to say so. A person who +// deliberately split the file this way is expected to combine it into +// one document by hand before running a command that edits it. func parseOrEmptyDocument(existing []byte) (*yaml.Node, error) { if len(existing) == 0 { return &yaml.Node{ @@ -60,10 +77,17 @@ func parseOrEmptyDocument(existing []byte) (*yaml.Node, error) { }, nil } + dec := yaml.NewDecoder(bytes.NewReader(existing)) + var doc yaml.Node - if err := yaml.Unmarshal(existing, &doc); err != nil { + if err := dec.Decode(&doc); err != nil && !errors.Is(err, io.EOF) { return nil, fmt.Errorf("config: parse the existing configuration: %w", err) } + + if err := nextDocumentExists(dec); err != nil { + return nil, err + } + if doc.Kind == 0 || len(doc.Content) == 0 { // A document holding only comments, or nothing at all, decodes // into a node with no content: the same empty case as a missing @@ -77,6 +101,39 @@ func parseOrEmptyDocument(existing []byte) (*yaml.Node, error) { return &doc, nil } +// nextDocumentExists reports, as ErrMultipleDocuments, whether dec still +// holds a second YAML document after the one parseOrEmptyDocument already +// decoded. A trailing, empty document, the shape a lone "---" end marker +// with nothing meaningful after it decodes to, is not treated as a real +// second document: only one that itself carries content counts. +func nextDocumentExists(dec *yaml.Decoder) error { + var next yaml.Node + err := dec.Decode(&next) + switch { + case errors.Is(err, io.EOF): + return nil + case err != nil: + return fmt.Errorf("config: parse the existing configuration: %w", err) + case isEmptyDocument(&next): + return nil + default: + return ErrMultipleDocuments + } +} + +// isEmptyDocument reports whether doc, a yaml.Node decoded as a whole +// document, carries no real content: either no document node at all, or +// a document node wrapping nothing but an implicit "!!null" scalar, the +// shape a bare "---" document separator with nothing meaningful after it +// decodes to. +func isEmptyDocument(doc *yaml.Node) bool { + if doc.Kind == 0 || len(doc.Content) == 0 { + return true + } + content := doc.Content[0] + return content.Kind == yaml.ScalarNode && content.Tag == "!!null" +} + // errNotFound is returned internally by mappingLookup when key is not // among parent's children; mappingChild and setScalarChild both treat it // as "create one" rather than an error, so it never escapes this file. diff --git a/internal/config/edit_test.go b/internal/config/edit_test.go index a37603c..b4088ca 100644 --- a/internal/config/edit_test.go +++ b/internal/config/edit_test.go @@ -4,6 +4,7 @@ package config_test import ( + "errors" "path/filepath" "strings" "testing" @@ -98,4 +99,39 @@ func TestAddProfileKeys(t *testing.T) { t.Fatal("AddProfileKeys() succeeded, want an error for a non mapping top level document") } }) + + t.Run("refuses a file holding more than one YAML document rather than dropping the rest", func(t *testing.T) { + t.Parallel() + existing := "profiles:\n work:\n user.email: work@example.test\n" + + "---\n" + + "profiles:\n personal:\n user.email: personal@example.test\n" + + _, err := config.AddProfileKeys([]byte(existing), "global", map[string]string{"user.email": "me@example.test"}) + if err == nil { + t.Fatal("AddProfileKeys() succeeded, want a refusal for a multi document file") + } + if !errors.Is(err, config.ErrMultipleDocuments) { + t.Fatalf("AddProfileKeys() error = %v, want it to wrap config.ErrMultipleDocuments", err) + } + }) + + t.Run("accepts a file ending in a bare document separator with nothing after it", func(t *testing.T) { + t.Parallel() + existing := "profiles:\n work:\n user.email: work@example.test\n---\n" + + out, err := config.AddProfileKeys([]byte(existing), "global", map[string]string{"user.email": "me@example.test"}) + if err != nil { + t.Fatalf("AddProfileKeys() returned an unexpected error for a trailing empty document: %v", err) + } + + path := filepath.Join(t.TempDir(), "gitcd.yaml") + writeFile(t, path, string(out)) + cfg, err := config.LoadFrom(path) + if err != nil { + t.Fatalf("LoadFrom() the result returned an unexpected error: %v", err) + } + if got, want := cfg.Profiles["work"]["user.email"], "work@example.test"; got != want { + t.Fatalf("profiles.work.user.email = %q, want %q", got, want) + } + }) } From ce2b3f41109e5670ff8511185e6296b30f09f068 Mon Sep 17 00:00:00 2001 From: Simon Ostendorf Date: Fri, 13 Nov 2026 23:52:01 +0100 Subject: [PATCH 7/7] fix(config): never claim a move that did not reach the file A bare "profiles:" key decodes to a null scalar, and appending to a scalar is dropped on marshal, so init reported success, removed the global identity and wrote nothing. The identity survived only in the backup. A null child becomes a real mapping now, and any other shape is refused rather than coerced. Beyond the cause, the rendered bytes are parsed back and checked to carry the keys before anything is removed, so a write that does not contain what it claimed can no longer be followed by a removal. A symlinked configuration is resolved and written through, keeping a dotfiles checkout intact, and the validation file no longer has a predictable name. Refs #8 --- cmd/gitcd/init.go | 111 +++++++++++-- cmd/gitcd/init_test.go | 302 +++++++++++++++++++++++++++++++++++ internal/config/edit.go | 50 ++++-- internal/config/edit_test.go | 49 ++++++ 4 files changed, 489 insertions(+), 23 deletions(-) diff --git a/cmd/gitcd/init.go b/cmd/gitcd/init.go index ee7a1b2..de8cd5d 100644 --- a/cmd/gitcd/init.go +++ b/cmd/gitcd/init.go @@ -247,7 +247,18 @@ func describeIdentityKeys(found []git.GlobalIdentityValue) string { // through config.LoadFrom: this is purely a name collision check, which // must not block on some unrelated problem the rest of the file carries, // a problem moveGlobalIdentity's own, stricter validation is already -// responsible for catching once there is something to write. +// responsible for catching once there is something to write. The decoder +// stays loose on purpose: it is never given KnownFields, so a field this +// struct does not name, "totallyUnknownKey" for instance, is simply +// ignored here rather than failing this parse, keeping that failure mode +// for the stricter check downstream, the one already responsible for it. +// +// A failure of this lenient parse itself, meaning the two fields it does +// name do not even decode, a "profiles" that is a list rather than a +// mapping for instance, is refused rather than waved through: a parse +// this forgiving failing at all means the file's shape could not be +// understood well enough to say there is no conflict, and "no conflict" +// must never be the answer to "I could not tell." // // Two collisions are refused, matching pull request 12's review: // - target already names defaultProfile. Never allowed, with no @@ -278,10 +289,11 @@ func targetProfileConflict(cfgPath, target string) (string, error) { DefaultProfile string `yaml:"defaultProfile"` } if err := yaml.Unmarshal(raw, &doc); err != nil { - // A file this lenient a parse cannot even make sense of is left - // for moveGlobalIdentity's own, stricter validation to reject - // with a precise error once there is something to write. - return "", nil + return fmt.Sprintf( + "%s does not parse well enough to check profile %q for a conflict: %v\n"+ + "refusing the move rather than risking a silent overwrite or a silently rearmed default", + cfgPath, target, err, + ), nil } if doc.DefaultProfile != "" && doc.DefaultProfile == target { @@ -359,7 +371,7 @@ func moveGlobalIdentity( } updatedYAML, err := config.AddProfileKeys(existingYAML, target, keys) if err != nil { - return err + return fmt.Errorf("%s: %w", cfgPath, err) } if err := os.MkdirAll(filepath.Dir(cfgPath), 0o755); err != nil { return fmt.Errorf("create %s: %w", filepath.Dir(cfgPath), err) @@ -375,18 +387,73 @@ func moveGlobalIdentity( // that argument. Validating against a temporary file, rather than // cfgPath itself, means a failure here leaves cfgPath exactly as it // was, with nothing left to restore. - tmpPath := cfgPath + ".gitcd-validate-tmp" - if err := os.WriteFile(tmpPath, updatedYAML, 0o644); err != nil { - return fmt.Errorf("write %s: %w", tmpPath, err) + // + // The temporary file gets an unpredictable name from os.CreateTemp, + // in the same directory as cfgPath so the later rename stays on one + // filesystem, rather than a fixed, guessable name: a fixed name is a + // symlink another process or a leftover from a previous run could + // already occupy, and os.WriteFile follows a symlink wherever it + // finds one, which would render this validation copy through that + // link instead of writing an ordinary file gitcd fully controls. + tmpFile, err := os.CreateTemp(filepath.Dir(cfgPath), filepath.Base(cfgPath)+".gitcd-validate-tmp-*") + if err != nil { + return fmt.Errorf("create a temporary file next to %s: %w", cfgPath, err) } + tmpPath := tmpFile.Name() defer func() { _ = os.Remove(tmpPath) }() - if _, err := config.LoadFrom(tmpPath); err != nil { + // os.CreateTemp creates its file 0o600; the configuration file this + // temporary copy stands in for is written 0o644, so match that + // before the rename below carries this file's own mode over to + // cfgPath. + if err := tmpFile.Chmod(0o644); err != nil { + _ = tmpFile.Close() + return fmt.Errorf("set permissions on %s: %w", tmpPath, err) + } + if _, err := tmpFile.Write(updatedYAML); err != nil { + _ = tmpFile.Close() + return fmt.Errorf("write %s: %w", tmpPath, err) + } + if err := tmpFile.Close(); err != nil { + return fmt.Errorf("write %s: %w", tmpPath, err) + } + + cfg, err := config.LoadFrom(tmpPath) + if err != nil { return fmt.Errorf("the updated configuration would not load, aborting before touching %s: %w", globalPath, err) } - if err := os.Rename(tmpPath, cfgPath); err != nil { - return fmt.Errorf("write %s: %w", cfgPath, err) + // A cheap belt and braces on top of LoadFrom succeeding: LoadFrom not + // erroring only proves the rendered file is valid YAML and a valid + // gitcd configuration, not that this particular edit actually landed + // in it, which is exactly the gap a "profiles:" key decoding to a + // "!!null" scalar opened. identityKeysLanded parses the same bytes + // that are about to replace cfgPath, through the same loader every + // other command uses, so it catches a future shape of this class + // regardless of where in the render path it originates, and it runs + // before the rename and before the unset loop below, so a failure + // here guarantees the global keys are never removed. + if err := identityKeysLanded(cfg, target, found); err != nil { + return fmt.Errorf("the write that was supposed to move %s did not, aborting before touching %s: %w", describeIdentityKeys(found), globalPath, err) } - fmt.Fprintf(stdout, "moved %s into profile %q in %s\n", describeIdentityKeys(found), target, cfgPath) + + // cfgPath may itself be a symlink, the common shape for a + // configuration tracked in a separate dotfiles repository: renaming + // a file onto a symlink's own path replaces the link with an + // ordinary file, silently detaching it from whatever it pointed at. + // Resolving it first and writing through the resolved path keeps the + // link itself, and the dotfiles copy it points at, intact. A cfgPath + // that does not exist yet, the first run on a machine with no + // configuration file at all, is not a symlink to resolve, and is + // written at cfgPath directly. + writeTarget := cfgPath + if resolved, err := filepath.EvalSymlinks(cfgPath); err == nil { + writeTarget = resolved + } else if !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("resolve %s: %w", cfgPath, err) + } + if err := os.Rename(tmpPath, writeTarget); err != nil { + return fmt.Errorf("write %s: %w", writeTarget, err) + } + fmt.Fprintf(stdout, "moved %s into profile %q in %s\n", describeIdentityKeys(found), target, writeTarget) if err := backupGlobalConfig(); err != nil { return err @@ -405,3 +472,21 @@ func moveGlobalIdentity( ) return nil } + +// identityKeysLanded reports an error naming the first of found's keys +// that profile target does not carry in cfg with exactly the value found +// recorded, or nil when every one of them does. cfg is expected to come +// from parsing the exact bytes moveGlobalIdentity is about to rename into +// place, so a non-nil result here means that write does not contain what +// it claims to, and the caller must abort before going anywhere near the +// global git configuration's own keys. +func identityKeysLanded(cfg config.Config, target string, found []git.GlobalIdentityValue) error { + profile := cfg.Profiles[target] + for _, v := range found { + got, ok := profile[v.Key] + if !ok || got != v.Value { + return fmt.Errorf("profile %q does not carry %s", target, v.Key) + } + } + return nil +} diff --git a/cmd/gitcd/init_test.go b/cmd/gitcd/init_test.go index 3b552b5..c391230 100644 --- a/cmd/gitcd/init_test.go +++ b/cmd/gitcd/init_test.go @@ -12,6 +12,9 @@ import ( "path/filepath" "strings" "testing" + + "github.com/ostendorf-cloud/gitcd/internal/config" + "github.com/ostendorf-cloud/gitcd/internal/git" ) // globalConfigPath mirrors setupEnv's own choice of global git configuration @@ -592,6 +595,305 @@ func TestRunInit_AbortsTheMoveWhenTheUpdatedConfigWouldNotLoad(t *testing.T) { } } +// TestRunInit_NeverClaimsSuccessWhileLosingAMoveIntoABareProfilesKey is +// the exact reproduction pull request 12's review returned as this +// round's blocker: a hand written ~/.gitcd.yaml that declares "profiles:" +// and leaves it empty, the shape "profiles:\n" decodes to a "!!null" +// scalar node rather than a mapping node. "init --move-identity=yes" must +// not be able to print that it moved the identity, remove it from the +// global git configuration, exit 0, and leave ~/.gitcd.yaml byte for byte +// unchanged: either the move genuinely lands in the file, or the command +// refuses and leaves everything, including the global identity, exactly +// as it was. +func TestRunInit_NeverClaimsSuccessWhileLosingAMoveIntoABareProfilesKey(t *testing.T) { + home := setupEnv(t) + globalPath := globalConfigPath(home) + cfgPath := filepath.Join(home, ".gitcd.yaml") + writeFile(t, cfgPath, "root: ~/Code\nprofiles:\n") + + beforeCfg, err := os.ReadFile(cfgPath) + if err != nil { + t.Fatalf("read %s: %v", cfgPath, err) + } + beforeGlobal, err := os.ReadFile(globalPath) + if err != nil { + t.Fatalf("read %s: %v", globalPath, err) + } + + var stdout, stderr bytes.Buffer + code := runInit([]string{"--move-identity=yes"}, strings.NewReader(""), &stdout, &stderr) + + afterCfg, err := os.ReadFile(cfgPath) + if err != nil { + t.Fatalf("read %s: %v", cfgPath, err) + } + afterGlobal, err := os.ReadFile(globalPath) + if err != nil { + t.Fatalf("read %s: %v", globalPath, err) + } + cfgCarriesMove := strings.Contains(string(afterCfg), "[email protected]") + globalKeysGone := !strings.Contains(string(afterGlobal), "Test User") && !strings.Contains(string(afterGlobal), "[email protected]") + + // The one outcome that must never happen, regardless of exit code: + // the identity gone from the global git configuration while + // ~/.gitcd.yaml does not actually carry it. + if globalKeysGone && !cfgCarriesMove { + t.Fatalf( + "init removed the global identity without %s carrying the move: exit=%d stdout=%q stderr=%q cfg after=%q", + cfgPath, code, stdout.String(), stderr.String(), afterCfg, + ) + } + + switch code { + case 0: + if !cfgCarriesMove { + t.Fatalf("init exited 0 without %s carrying the moved identity: %q", cfgPath, afterCfg) + } + if !globalKeysGone { + t.Fatalf("init exited 0 without removing the global identity: %q", afterGlobal) + } + default: + if string(afterCfg) != string(beforeCfg) { + t.Fatalf("init refused but still changed %s:\nbefore: %q\nafter: %q", cfgPath, beforeCfg, afterCfg) + } + if string(afterGlobal) != string(beforeGlobal) { + t.Fatalf("init refused but still changed the global git configuration:\nbefore: %q\nafter: %q", beforeGlobal, afterGlobal) + } + } +} + +// TestRunInit_DoesNotFollowAPreexistingSymlinkAtTheValidationTempPath +// covers the review's third "should": the validation copy used to be +// written to a fixed, predictable path next to cfgPath with os.WriteFile, +// which follows a symlink already sitting there. A symlink planted at +// that exact legacy name, pointing anywhere else, must not have its +// target overwritten by this command. +func TestRunInit_DoesNotFollowAPreexistingSymlinkAtTheValidationTempPath(t *testing.T) { + home := setupEnv(t) + cfgPath := filepath.Join(home, ".gitcd.yaml") + + canary := filepath.Join(home, "canary.txt") + writeFile(t, canary, "do not touch\n") + + legacyTmpPath := cfgPath + ".gitcd-validate-tmp" + if err := os.Symlink(canary, legacyTmpPath); err != nil { + t.Fatalf("symlink %s -> %s: %v", legacyTmpPath, canary, err) + } + + var stdout, stderr bytes.Buffer + code := runInit([]string{"--move-identity=yes"}, strings.NewReader(""), &stdout, &stderr) + if code != 0 { + t.Fatalf("init --move-identity=yes = %d, want 0, stderr: %s", code, stderr.String()) + } + + canaryAfter, err := os.ReadFile(canary) + if err != nil { + t.Fatalf("read %s: %v", canary, err) + } + if string(canaryAfter) != "do not touch\n" { + t.Fatalf("canary content = %q, want it untouched: the validation step wrote through a preexisting symlink at its old, predictable path", canaryAfter) + } + + linkTarget, err := os.Readlink(legacyTmpPath) + if err != nil { + t.Fatalf("readlink %s: %v", legacyTmpPath, err) + } + if linkTarget != canary { + t.Fatalf("legacy symlink target = %q, want it left pointing at %q", linkTarget, canary) + } + + cfgContent, err := os.ReadFile(cfgPath) + if err != nil { + t.Fatalf("init did not write %s: %v", cfgPath, err) + } + if !strings.Contains(string(cfgContent), "[email protected]") { + t.Fatalf("%s = %q, want the moved email under profiles.global", cfgPath, cfgContent) + } +} + +// TestRunInit_WritesThroughASymlinkedGitcdYAMLRatherThanReplacingIt +// covers the review's second "should": ~/.gitcd.yaml symlinked into a +// dotfiles repository, the common setup. The rename this command used to +// do landed directly on the symlink's own path, replacing the link with +// an ordinary file and leaving the dotfiles copy holding the stale +// content. The link, and the dotfiles copy it points at, must both still +// carry the move afterwards. +func TestRunInit_WritesThroughASymlinkedGitcdYAMLRatherThanReplacingIt(t *testing.T) { + home := setupEnv(t) + dotfilesCopy := filepath.Join(home, "dotfiles", "gitcd.yaml") + writeFile(t, dotfilesCopy, "profiles:\n work:\n user.email: work@example.test\n") + + cfgPath := filepath.Join(home, ".gitcd.yaml") + if err := os.Symlink(dotfilesCopy, cfgPath); err != nil { + t.Fatalf("symlink %s -> %s: %v", cfgPath, dotfilesCopy, err) + } + + var stdout, stderr bytes.Buffer + code := runInit([]string{"--move-identity=yes"}, strings.NewReader(""), &stdout, &stderr) + if code != 0 { + t.Fatalf("init --move-identity=yes = %d, want 0, stderr: %s", code, stderr.String()) + } + + info, err := os.Lstat(cfgPath) + if err != nil { + t.Fatalf("lstat %s: %v", cfgPath, err) + } + if info.Mode()&os.ModeSymlink == 0 { + t.Fatalf("%s is no longer a symlink after the move, want the dotfiles link kept intact", cfgPath) + } + linkTarget, err := os.Readlink(cfgPath) + if err != nil { + t.Fatalf("readlink %s: %v", cfgPath, err) + } + if linkTarget != dotfilesCopy { + t.Fatalf("%s now points at %q, want it still pointing at %q", cfgPath, linkTarget, dotfilesCopy) + } + + dotfilesContent, err := os.ReadFile(dotfilesCopy) + if err != nil { + t.Fatalf("read %s: %v", dotfilesCopy, err) + } + if !strings.Contains(string(dotfilesContent), "work@example.test") { + t.Fatalf("%s = %q, want the pre-existing profile untouched", dotfilesCopy, dotfilesContent) + } + if !strings.Contains(string(dotfilesContent), "[email protected]") { + t.Fatalf("%s = %q, the dotfiles copy itself must carry the moved identity", dotfilesCopy, dotfilesContent) + } + + // Reading through the symlink must agree with reading the dotfiles + // copy directly: both name the same file. + viaLink, err := os.ReadFile(cfgPath) + if err != nil { + t.Fatalf("read %s: %v", cfgPath, err) + } + if string(viaLink) != string(dotfilesContent) { + t.Fatalf("reading %s and %s disagree:\nvia link: %q\ndotfiles: %q", cfgPath, dotfilesCopy, viaLink, dotfilesContent) + } +} + +// TestRunInit_MultipleDocumentsRefusalNamesTheFile covers the nit left +// standing across both review rounds: ErrMultipleDocuments's own message +// never names the file it refused, so the printed refusal gave no path +// to look at, even though the configuration may sit at either of two +// candidate locations. +func TestRunInit_MultipleDocumentsRefusalNamesTheFile(t *testing.T) { + home := setupEnv(t) + cfgPath := filepath.Join(home, ".gitcd.yaml") + writeFile(t, cfgPath, + "profiles:\n work:\n user.email: work@example.test\n"+ + "---\n"+ + "profiles:\n personal:\n user.email: personal@example.test\n", + ) + + var stdout, stderr bytes.Buffer + code := runInit([]string{"--move-identity=yes"}, strings.NewReader(""), &stdout, &stderr) + if code == 0 { + t.Fatalf("init --move-identity=yes = 0, want a refusal for a multi document file, stdout: %s", stdout.String()) + } + if !strings.Contains(stderr.String(), "more than one YAML document") { + t.Fatalf("init stderr = %q, want it to name the multi document refusal", stderr.String()) + } + if !strings.Contains(stderr.String(), cfgPath) { + t.Fatalf("init stderr = %q, want it to name %s, the file it refused", stderr.String(), cfgPath) + } +} + +// TestTargetProfileConflict_RefusesWhenTheLenientParseItselfFails covers +// the first "should": targetProfileConflict used to treat any parse +// failure of its own, lenient decode as "no conflict", waving a move +// through for a file it could not actually make sense of. +func TestTargetProfileConflict_RefusesWhenTheLenientParseItselfFails(t *testing.T) { + home := setupEnv(t) + cfgPath := filepath.Join(home, ".gitcd.yaml") + writeFile(t, cfgPath, "profiles:\n - not\n - a\n - mapping\n") + + msg, err := targetProfileConflict(cfgPath, "global") + if err != nil { + t.Fatalf("targetProfileConflict() returned an unexpected error: %v", err) + } + if msg == "" { + t.Fatal("targetProfileConflict() = \"\" (no conflict), want a refusal when the file does not even parse") + } +} + +// TestTargetProfileConflict_AnUnrelatedUnknownKeyAloneIsNotAConflict +// confirms the fix above stays independent of config.LoadFrom's own, +// stricter unknown key rejection: a key this lenient parse does not name +// at all must not, by itself, read as a parse failure. +func TestTargetProfileConflict_AnUnrelatedUnknownKeyAloneIsNotAConflict(t *testing.T) { + home := setupEnv(t) + cfgPath := filepath.Join(home, ".gitcd.yaml") + writeFile(t, cfgPath, "totallyUnknownKey: 1\n") + + msg, err := targetProfileConflict(cfgPath, "global") + if err != nil { + t.Fatalf("targetProfileConflict() returned an unexpected error: %v", err) + } + if msg != "" { + t.Fatalf("targetProfileConflict() = %q, want no conflict for a file whose only problem is unrelated to profiles or defaultProfile", msg) + } +} + +// TestTargetProfileConflict_AParseFailureIsNotMaskedByAnUnrelatedUnknownKey +// combines both: a genuinely malformed "profiles" key must still be +// refused even alongside an unrelated unknown key, rather than either +// masking the other. +func TestTargetProfileConflict_AParseFailureIsNotMaskedByAnUnrelatedUnknownKey(t *testing.T) { + home := setupEnv(t) + cfgPath := filepath.Join(home, ".gitcd.yaml") + writeFile(t, cfgPath, "totallyUnknownKey: 1\nprofiles:\n - not\n - a\n - mapping\n") + + msg, err := targetProfileConflict(cfgPath, "global") + if err != nil { + t.Fatalf("targetProfileConflict() returned an unexpected error: %v", err) + } + if msg == "" { + t.Fatal("targetProfileConflict() = \"\" (no conflict), want the malformed profiles key refused alongside an unrelated unknown key") + } +} + +// TestIdentityKeysLanded is the unit level proof for the blocker's belt +// and braces guarantee: moveGlobalIdentity must abort, rather than go on +// to remove anything from the global git configuration, whenever the +// configuration it is about to rename into place does not actually carry +// every key it claims to have moved. +func TestIdentityKeysLanded(t *testing.T) { + t.Parallel() + found := []git.GlobalIdentityValue{{Key: "user.email", Value: "[email protected]"}} + + t.Run("reports an error when the profile does not carry the moved key", func(t *testing.T) { + t.Parallel() + cfg := config.Config{Profiles: map[string]config.Profile{"global": {}}} + if err := identityKeysLanded(cfg, "global", found); err == nil { + t.Fatal("identityKeysLanded() = nil, want an error when the profile does not carry the key") + } + }) + + t.Run("reports an error when the profile does not exist at all", func(t *testing.T) { + t.Parallel() + cfg := config.Config{} + if err := identityKeysLanded(cfg, "global", found); err == nil { + t.Fatal("identityKeysLanded() = nil, want an error when the profile does not exist at all") + } + }) + + t.Run("reports an error when the value does not match what was moved", func(t *testing.T) { + t.Parallel() + cfg := config.Config{Profiles: map[string]config.Profile{"global": {"user.email": "wrong@example.test"}}} + if err := identityKeysLanded(cfg, "global", found); err == nil { + t.Fatal("identityKeysLanded() = nil, want an error when the value does not match") + } + }) + + t.Run("accepts a profile carrying every moved key with its exact value", func(t *testing.T) { + t.Parallel() + cfg := config.Config{Profiles: map[string]config.Profile{"global": {"user.email": "[email protected]"}}} + if err := identityKeysLanded(cfg, "global", found); err != nil { + t.Fatalf("identityKeysLanded() = %v, want nil when every moved key matches", err) + } + }) +} + func TestRunInit_NeverWritesToTheRealHomeDirectory(t *testing.T) { // A sanity check on this test file's own isolation, not gitcd's // behaviour: every init test above must run with os.UserHomeDir() diff --git a/internal/config/edit.go b/internal/config/edit.go index e3e317b..7e46a99 100644 --- a/internal/config/edit.go +++ b/internal/config/edit.go @@ -36,8 +36,14 @@ func AddProfileKeys(existing []byte, name string, keys map[string]string) ([]byt return nil, fmt.Errorf("config: the existing configuration is not a YAML mapping at its top level") } - profiles := mappingChild(root, "profiles") - profile := mappingChild(profiles, name) + profiles, err := mappingChild(root, "profiles") + if err != nil { + return nil, err + } + profile, err := mappingChild(profiles, name) + if err != nil { + return nil, err + } for _, key := range sortedStringKeys(keys) { setScalarChild(profile, key, keys[key]) } @@ -142,14 +148,38 @@ var errNotFound = errors.New("config: key not found in mapping") // mappingChild returns parent's child mapping node keyed name, appending // both the key and a fresh, empty mapping value when parent does not // already carry one. -func mappingChild(parent *yaml.Node, name string) *yaml.Node { - if value, err := mappingLookup(parent, name); err == nil { - return value - } - key := &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: name} - value := &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"} - parent.Content = append(parent.Content, key, value) - return value +// +// A child that already exists but decoded as an empty "!!null" scalar is +// turned into a mapping node in place rather than returned as is: that is +// exactly the shape a bare "profiles:" or "defaultProfile:", with nothing +// after the colon, decodes to, and appending a key/value pair to a +// scalar node's Content is silently ignored by yaml.Marshal, which let +// "init" report a move that never reached the file. A child that exists +// as some other, non-mapping, non-null shape, a list or a quoted string +// for instance, is refused instead of silently coerced, since turning it +// into a mapping would discard whatever a person deliberately wrote +// there. +func mappingChild(parent *yaml.Node, name string) (*yaml.Node, error) { + value, err := mappingLookup(parent, name) + if errors.Is(err, errNotFound) { + key := &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: name} + value = &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"} + parent.Content = append(parent.Content, key, value) + return value, nil + } + switch { + case value.Kind == yaml.MappingNode: + return value, nil + case value.Kind == yaml.ScalarNode && value.Tag == "!!null": + value.Kind = yaml.MappingNode + value.Tag = "!!map" + value.Value = "" + value.Style = 0 + value.Content = nil + return value, nil + default: + return nil, fmt.Errorf("config: %q is not a YAML mapping in the existing configuration", name) + } } // setScalarChild sets parent's child scalar value keyed key to value, diff --git a/internal/config/edit_test.go b/internal/config/edit_test.go index b4088ca..fb3a712 100644 --- a/internal/config/edit_test.go +++ b/internal/config/edit_test.go @@ -92,6 +92,55 @@ func TestAddProfileKeys(t *testing.T) { } }) + t.Run("writes into a bare profiles key with no value rather than silently losing the write", func(t *testing.T) { + t.Parallel() + // pull request 12's exact reproduction: a hand written file that + // declares "profiles:" and leaves it empty. That decodes to a + // "!!null" scalar node, not a mapping node, so appending the new + // profile's key and value to it must not be silently swallowed by + // yaml.Marshal the way it was before this fix. + existing := "root: /tmp/example/Code\nprofiles:\n" + + out, err := config.AddProfileKeys([]byte(existing), "global", map[string]string{"user.email": "me@example.test"}) + if err != nil { + t.Fatalf("AddProfileKeys() returned an unexpected error for a bare \"profiles:\" key: %v", err) + } + + path := filepath.Join(t.TempDir(), "gitcd.yaml") + writeFile(t, path, string(out)) + cfg, err := config.LoadFrom(path) + if err != nil { + t.Fatalf("LoadFrom() the result returned an unexpected error: %v", err) + } + if got, want := cfg.Profiles["global"]["user.email"], "me@example.test"; got != want { + t.Fatalf("profiles.global.user.email = %q, want %q; AddProfileKeys() must not report success while losing the write", got, want) + } + }) + + t.Run("writes into a profile name already present with no value the same way", func(t *testing.T) { + t.Parallel() + // The same "!!null" scalar shape, one call deeper: "profiles" + // already exists, but the target profile name sits under it as a + // bare key, the shape a person placing a profile name before + // filling it in would plausibly write. + existing := "profiles:\n global:\n" + + out, err := config.AddProfileKeys([]byte(existing), "global", map[string]string{"user.name": "Jane Doe"}) + if err != nil { + t.Fatalf("AddProfileKeys() returned an unexpected error for a bare profile name: %v", err) + } + + path := filepath.Join(t.TempDir(), "gitcd.yaml") + writeFile(t, path, string(out)) + cfg, err := config.LoadFrom(path) + if err != nil { + t.Fatalf("LoadFrom() the result returned an unexpected error: %v", err) + } + if got, want := cfg.Profiles["global"]["user.name"], "Jane Doe"; got != want { + t.Fatalf("profiles.global.user.name = %q, want %q; AddProfileKeys() must not report success while losing the write", got, want) + } + }) + t.Run("rejects a document that is not a mapping at its top level", func(t *testing.T) { t.Parallel() _, err := config.AddProfileKeys([]byte("- not\n- a\n- mapping\n"), "global", map[string]string{"user.email": "me@example.test"})