From 1544c5ffb35b9719f85e7cc94dbe1952d0ed15c9 Mon Sep 17 00:00:00 2001 From: Simon Ostendorf Date: Sat, 14 Nov 2026 00:55:54 +0100 Subject: [PATCH 1/6] feat(doctor): detect identity, path and managed file drift Detection only, no writes. A deliberate pin marked gitcd.profile is told apart from stale config, and a global identity key is reported wherever it sits, because useConfigOnly cannot suppress one. Refs #4 --- internal/doctor/doctor.go | 348 ++++++++++++++++++++++++++++++ internal/doctor/doctor_test.go | 381 +++++++++++++++++++++++++++++++++ internal/git/config.go | 19 ++ internal/git/config_test.go | 55 +++++ 4 files changed, 803 insertions(+) create mode 100644 internal/doctor/doctor.go create mode 100644 internal/doctor/doctor_test.go diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go new file mode 100644 index 0000000..2990aca --- /dev/null +++ b/internal/doctor/doctor.go @@ -0,0 +1,348 @@ +// Copyright 2026 Simon Ostendorf +// SPDX-License-Identifier: Apache-2.0 + +// Package doctor finds drift between what gitcd's configuration implies +// and what actually sits on disk and in git's own configuration. It is +// the fleet wide form of internal/profile's single repository Inspect: +// the command layer walks the configured root and calls CheckRepo once +// per repository found, then CheckManagedFiles and CheckGlobal once for +// the fleet as a whole, and reports every Finding returned. This package +// never writes anything: "gitcd doctor --fix" and "gitcd adopt", in +// cmd/gitcd, perform every repair it only detects the need for. +package doctor + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "sort" + "strings" + + "github.com/ostendorf-cloud/gitcd/internal/config" + "github.com/ostendorf-cloud/gitcd/internal/git" + "github.com/ostendorf-cloud/gitcd/internal/gitconfig" + "github.com/ostendorf-cloud/gitcd/internal/layout" + "github.com/ostendorf-cloud/gitcd/internal/profile" +) + +// Kind names one class of finding, each tracing to one check issue #4 +// asks for. +type Kind string + +const ( + // KindIdentityDrift is a repository whose effective identity differs + // from the profile its own rules imply, with no "gitcd.profile" pin + // recorded that would explain the difference as deliberate. + KindIdentityDrift Kind = "identity-drift" + // KindStalePin is a "gitcd.profile" pin naming a profile the current + // configuration no longer carries: a deliberate pin at the time it + // was written, now stale rather than deliberate. + KindStalePin Kind = "stale-pin" + // KindPathMismatch is a repository under root sitting at a path the + // configured layout would not render for its own remote, for a + // reason other than character case. + KindPathMismatch Kind = "path-mismatch" + // KindPathCase is KindPathMismatch narrowed to a difference of + // character case only: the layout renders the same path, spelled + // differently, which "doctor --fix" repairs with a rename. + KindPathCase Kind = "path-case-mismatch" + // KindRuleCaseMismatch is a configured rule whose host, owner and + // repo would all match a repository case-insensitively, but do not + // match exactly: profile.Resolve, and the "hasconfig" glob + // internal/gitconfig renders from the same literal fields, are both + // case sensitive on owner and repo, so the rule silently never + // applies as written. + KindRuleCaseMismatch Kind = "rule-case-mismatch" + // KindManagedFileMissing is a file "gitcd apply" renders that does + // not exist on disk at all. + KindManagedFileMissing Kind = "managed-file-missing" + // KindManagedFileStale is a file "gitcd apply" renders that exists + // but no longer matches what the current configuration renders, + // covering both an out of date include block and any ordering drift + // inside it, since re-rendering always reproduces the current, + // correctly ordered content. + KindManagedFileStale Kind = "managed-file-stale" + // KindGlobalIdentityKey is an identity key the global git + // configuration sets directly: "user.useConfigOnly" cannot suppress + // it, so it applies to every repository gitcd manages, one some rule + // matches or not. + KindGlobalIdentityKey Kind = "global-identity-key" + // KindGlobalIdentityOrder is a global identity key sitting after the + // gitcd include: "git config --global --add" inserts a new key into + // that section, so a key already after it keeps beating every + // includeIf value silently. + KindGlobalIdentityOrder Kind = "global-identity-order" +) + +// Finding is one problem doctor reports. +type Finding struct { + Kind Kind `json:"kind"` + // Repo is the repository name, "/" separated relative to root, this + // finding is about, or "" for a finding not tied to one repository. + Repo string `json:"repo,omitempty"` + // Path is the absolute path this finding is about, a repository's + // own path or a managed file's path, or "" when neither applies. + Path string `json:"path,omitempty"` + // Message describes the finding in a form ready to print. + Message string `json:"message"` + // Fixable reports whether "doctor --fix" repairs this finding + // mechanically. An identity key, local or global, is never one of + // these: a value doctor did not write is never one it overwrites. + Fixable bool `json:"fixable"` + // WantPath is set alongside KindPathCase: the path "doctor --fix" + // renames Path to. + WantPath string `json:"wantPath,omitempty"` +} + +// CheckRepo inspects the repository at path, named name relative to +// root, for every per repository finding: identity drift or a stale +// pin, a path the layout would not render, and a rule that almost +// matches this repository but for a case difference. +func CheckRepo(ctx context.Context, cfg config.Config, home, name, path string) ([]Finding, error) { + repo, err := profile.Inspect(ctx, path, cfg, gitconfig.Dir(home)) + if err != nil { + return nil, fmt.Errorf("doctor: inspect %s: %w", path, err) + } + + var findings []Finding + + identityFindings, err := checkIdentity(ctx, path, name, cfg, repo) + if err != nil { + return nil, err + } + findings = append(findings, identityFindings...) + + findings = append(findings, checkPath(cfg, name, path, repo)...) + findings = append(findings, checkRuleCase(cfg, name, repo)...) + + return findings, nil +} + +// checkIdentity reports KindStalePin when path carries a "gitcd.profile" +// pin naming a profile cfg no longer has, and KindIdentityDrift when, +// with no pin recorded at all, the identity git actually resolves for +// path differs from what the profile its rules imply would set. A valid +// pin, naming a profile cfg still carries, is a deliberate choice, +// reported as neither: AC6 of issue #4 asks doctor to tell the two +// apart, not to flag a deliberate pin as if it were drift. +func checkIdentity(ctx context.Context, path, name string, cfg config.Config, repo profile.Repo) ([]Finding, error) { + pinned, err := git.ConfigGetLocal(ctx, path, "gitcd.profile") + switch { + case err == nil: + if _, ok := cfg.Profiles[pinned]; !ok { + return []Finding{{ + Kind: KindStalePin, + Repo: name, + Path: path, + Message: fmt.Sprintf( + "pinned to profile %q through gitcd.profile, which the configuration no longer carries", pinned, + ), + }}, nil + } + return nil, nil + case errors.Is(err, git.ErrConfigKeyNotSet): + // No pin: fall through to compare the effective identity + // against what the rules currently imply. + default: + return nil, fmt.Errorf("doctor: read gitcd.profile of %s: %w", path, err) + } + + if repo.ExpectedProfile == "" { + return nil, nil + } + want := cfg.Profiles[repo.ExpectedProfile] + var mismatches []string + if v, ok := want["user.email"]; ok && v != repo.Email.Value { + mismatches = append(mismatches, fmt.Sprintf("user.email: have %q, profile %q expects %q", repo.Email.Value, repo.ExpectedProfile, v)) + } + if v, ok := want["user.signingkey"]; ok && v != repo.SigningKey.Value { + mismatches = append( + mismatches, fmt.Sprintf("user.signingkey: have %q, profile %q expects %q", repo.SigningKey.Value, repo.ExpectedProfile, v), + ) + } + if len(mismatches) == 0 { + return nil, nil + } + return []Finding{{ + Kind: KindIdentityDrift, + Repo: name, + Path: path, + Message: fmt.Sprintf( + "effective identity differs from the profile its rules imply, %q: %s", repo.ExpectedProfile, strings.Join(mismatches, "; "), + ), + }}, nil +} + +// checkPath reports a repository sitting at a path the configured layout +// would not render for its own remote: KindPathCase when the layout +// would render the same path spelled differently, KindPathMismatch for +// every other mismatch, including a remote the layout cannot render a +// path for at all. +func checkPath(cfg config.Config, name, path string, repo profile.Repo) []Finding { + rel, err := layout.Render(cfg.LayoutFor(repo.Remote.Host), repo.Remote) + if err != nil { + return []Finding{{ + Kind: KindPathMismatch, + Repo: name, + Path: path, + Message: fmt.Sprintf( + "the configured layout cannot render a path for this repository's own remote: %v", err, + ), + }} + } + want := filepath.Join(cfg.Root, rel) + if want == path { + return nil + } + if strings.EqualFold(want, path) { + return []Finding{{ + Kind: KindPathCase, + Repo: name, + Path: path, + Fixable: true, + WantPath: want, + Message: fmt.Sprintf( + "on disk at %s, the layout renders %s for this repository's remote: a case only difference", path, want, + ), + }} + } + return []Finding{{ + Kind: KindPathMismatch, + Repo: name, + Path: path, + Message: fmt.Sprintf( + "on disk at %s, the layout renders %s for this repository's own remote", path, want, + ), + }} +} + +// checkRuleCase reports KindRuleCaseMismatch for every rule whose host, +// owner and repo all match repo's remote case-insensitively but not +// exactly: profile.Resolve compares owner and repo with a case sensitive +// "!=", so such a rule never actually applies, silently, and the +// "hasconfig" glob internal/gitconfig renders from the same literal +// fields inherits the same gap, since git's own wildmatch there is case +// sensitive too. +func checkRuleCase(cfg config.Config, name string, repo profile.Repo) []Finding { + var findings []Finding + for _, rule := range cfg.Rules { + if rule.Host != "" && !strings.EqualFold(rule.Host, repo.Remote.Host) { + continue + } + ownerFold := rule.Owner == "" || strings.EqualFold(rule.Owner, repo.Remote.Owner) + repoFold := rule.Repo == "" || strings.EqualFold(rule.Repo, repo.Remote.Repo) + if !ownerFold || !repoFold { + continue + } + exactOwner := rule.Owner == "" || rule.Owner == repo.Remote.Owner + exactRepo := rule.Repo == "" || rule.Repo == repo.Remote.Repo + if exactOwner && exactRepo { + continue + } + findings = append(findings, Finding{ + Kind: KindRuleCaseMismatch, + Repo: name, + Message: fmt.Sprintf( + "rule host=%q owner=%q repo=%q profile=%q would match this repository but for a case difference (actual owner=%q repo=%q); the rule never applies as written", + rule.Host, rule.Owner, rule.Repo, rule.Profile, repo.Remote.Owner, repo.Remote.Repo, + ), + }) + } + return findings +} + +// CheckManagedFiles compares every file "gitcd apply" renders for cfg +// against what actually sits on disk under home, reporting +// KindManagedFileMissing for one that does not exist and +// KindManagedFileStale for one that exists with different content: an +// out of date include block, AC3 of issue #4 asks for by name, is one +// instance of this, and so is any ordering drift inside it, since +// internal/gitconfig.Render always reproduces the current, correctly +// ordered content. +func CheckManagedFiles(cfg config.Config, home string) ([]Finding, error) { + rendered, err := gitconfig.Render(cfg, home) + if err != nil { + return nil, fmt.Errorf("doctor: render the managed configuration: %w", err) + } + + paths := make([]string, 0, len(rendered)) + for path := range rendered { + paths = append(paths, path) + } + sort.Strings(paths) + + includePath := gitconfig.IncludePath(home) + + var findings []Finding + for _, path := range paths { + want := rendered[path] + got, readErr := os.ReadFile(path) + switch { + case errors.Is(readErr, os.ErrNotExist): + findings = append(findings, Finding{ + Kind: KindManagedFileMissing, + Path: path, + Fixable: true, + Message: fmt.Sprintf("%s is missing%s", path, managedFileNote(path, includePath)), + }) + case readErr != nil: + return nil, fmt.Errorf("doctor: read %s: %w", path, readErr) + case string(got) != want: + findings = append(findings, Finding{ + Kind: KindManagedFileStale, + Path: path, + Fixable: true, + Message: fmt.Sprintf("%s no longer matches the rendered configuration%s", path, managedFileNote(path, includePath)), + }) + } + } + return findings, nil +} + +// managedFileNote names path as the gitcd include file specifically, +// when it is one, so a reported finding reads as the AC3 "missing or +// stale include block" it is, rather than only naming an unfamiliar +// path. +func managedFileNote(path, includePath string) string { + if path == includePath { + return "; it is the gitcd include file every profile rule depends on" + } + return "; it is a managed profile file \"gitcd apply\" renders" +} + +// CheckGlobal reports every global identity finding: KindGlobalIdentityKey +// for each identity key content, the global git configuration's own raw +// text, sets directly, naming movedProfileTarget as the profile it would +// belong in, and KindGlobalIdentityOrder when one of them sits after the +// gitcd include, which would keep winning over every includeIf value +// silently. +func CheckGlobal(content []byte, globalPath, movedProfileTarget string) ([]Finding, error) { + var findings []Finding + + for _, v := range git.FindGlobalIdentityValues(content, git.IdentityKeys) { + findings = append(findings, Finding{ + Kind: KindGlobalIdentityKey, + Message: fmt.Sprintf( + "%s sets %s = %s directly; \"user.useConfigOnly\" cannot suppress it, so every repository gitcd manages, including one no rule matches, gets this identity; it belongs in profile %q instead", + globalPath, v.Key, v.Value, movedProfileTarget, + ), + }) + } + + key, found, err := git.IdentityKeyAfterInclude(content, git.IdentityKeys) + if err != nil { + return nil, fmt.Errorf("doctor: check %s for an identity key after the include: %w", globalPath, err) + } + if found { + findings = append(findings, Finding{ + Kind: KindGlobalIdentityOrder, + Message: fmt.Sprintf( + "%s sets %s after the gitcd include; it would keep beating every includeIf value silently", globalPath, key, + ), + }) + } + + return findings, nil +} diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go new file mode 100644 index 0000000..5c0a9fa --- /dev/null +++ b/internal/doctor/doctor_test.go @@ -0,0 +1,381 @@ +// Copyright 2026 Simon Ostendorf +// SPDX-License-Identifier: Apache-2.0 + +package doctor_test + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/ostendorf-cloud/gitcd/internal/config" + "github.com/ostendorf-cloud/gitcd/internal/doctor" + "github.com/ostendorf-cloud/gitcd/internal/gitconfig" +) + +// setupGitEnv isolates every git invocation a test makes from the real +// user's configuration and from the network, returning the home +// directory a repository and the managed directory are built under. +func setupGitEnv(t *testing.T) string { + t.Helper() + home := t.TempDir() + globalConfig := filepath.Join(home, ".gitconfig") + // A plain token, not an address shape, so a test comparing it against + // a profile's own identity value never accidentally collides with one + // through some unrelated normalisation of email-shaped text. + if err := os.WriteFile(globalConfig, []byte("[user]\n\tname = Test User\n\temail = global-fallback-identity\n"), 0o644); err != nil { + t.Fatalf("write %s: %v", globalConfig, err) + } + t.Setenv("HOME", home) + t.Setenv("GIT_CONFIG_GLOBAL", globalConfig) + t.Setenv("GIT_CONFIG_NOSYSTEM", "1") + return home +} + +func run(t *testing.T, dir string, args ...string) { + t.Helper() + cmd := exec.CommandContext(context.Background(), "git", args...) + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %s: %v: %s", strings.Join(args, " "), err, out) + } +} + +func initRepository(t *testing.T, dir string) { + t.Helper() + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatalf("create %s: %v", dir, err) + } + run(t, dir, "init", "--initial-branch=main") + if err := os.WriteFile(filepath.Join(dir, "README.md"), []byte("example\n"), 0o644); err != nil { + t.Fatalf("write README.md: %v", err) + } + run(t, dir, "add", "README.md") + run(t, dir, "commit", "-m", "add README") +} + +// findKind reports whether findings carries one of kind, and the first +// one found. +func findKind(findings []doctor.Finding, kind doctor.Kind) (doctor.Finding, bool) { + for _, f := range findings { + if f.Kind == kind { + return f, true + } + } + return doctor.Finding{}, false +} + +func TestCheckRepo(t *testing.T) { + home := setupGitEnv(t) + root := filepath.Join(home, "code") + + t.Run("no findings for a repository whose identity and path already match the rules", func(t *testing.T) { + repoDir := filepath.Join(root, "github.com", "acme", "clean") + initRepository(t, repoDir) + run(t, repoDir, "remote", "add", "origin", "https://github.com/acme/clean.git") + run(t, repoDir, "config", "--local", "user.email", "acme-work-identity") + run(t, repoDir, "config", "--local", "gitcd.profile", "work") + + cfg := config.Config{ + Root: root, + Default: "{host}/{owner}/{repo}", + Profiles: map[string]config.Profile{ + "work": {"user.email": "acme-work-identity"}, + }, + Rules: []config.Rule{{Host: "github.com", Owner: "acme", Profile: "work"}}, + } + + findings, err := doctor.CheckRepo(context.Background(), cfg, home, "github.com/acme/clean", repoDir) + if err != nil { + t.Fatalf("CheckRepo() returned an unexpected error: %v", err) + } + if len(findings) != 0 { + t.Fatalf("CheckRepo() = %+v, want no findings", findings) + } + }) + + t.Run("identity drift: no pin, effective identity differs from the expected profile", func(t *testing.T) { + repoDir := filepath.Join(root, "github.com", "acme", "drifted") + initRepository(t, repoDir) + run(t, repoDir, "remote", "add", "origin", "https://github.com/acme/drifted.git") + // No local pin and no managed include: the global fallback name + // and email set by setupGitEnv become the effective identity, + // which does not match what the "work" profile expects. + + cfg := config.Config{ + Root: root, + Default: "{host}/{owner}/{repo}", + Profiles: map[string]config.Profile{ + // Deliberately different from setupGitEnv's own global + // "global-fallback-identity", the effective identity this + // repository falls back to with no pin and no managed + // include of its own. + "work": {"user.email": "acme-work-identity"}, + }, + Rules: []config.Rule{{Host: "github.com", Owner: "acme", Profile: "work"}}, + } + + findings, err := doctor.CheckRepo(context.Background(), cfg, home, "github.com/acme/drifted", repoDir) + if err != nil { + t.Fatalf("CheckRepo() returned an unexpected error: %v", err) + } + f, ok := findKind(findings, doctor.KindIdentityDrift) + if !ok { + t.Fatalf("CheckRepo() = %+v, want a %s finding", findings, doctor.KindIdentityDrift) + } + if !strings.Contains(f.Message, "work") { + t.Fatalf("finding message = %q, want it to name the expected profile", f.Message) + } + }) + + t.Run("a deliberate pin naming a profile that still exists is never reported as drift", func(t *testing.T) { + repoDir := filepath.Join(root, "github.com", "acme", "pinned") + initRepository(t, repoDir) + run(t, repoDir, "remote", "add", "origin", "https://github.com/acme/pinned.git") + run(t, repoDir, "config", "--local", "user.email", "acme-personal-identity") + run(t, repoDir, "config", "--local", "gitcd.profile", "personal") + + cfg := config.Config{ + Root: root, + Default: "{host}/{owner}/{repo}", + Profiles: map[string]config.Profile{ + "work": {"user.email": "acme-work-identity"}, + "personal": {"user.email": "acme-personal-identity"}, + }, + Rules: []config.Rule{{Host: "github.com", Owner: "acme", Profile: "work"}}, + } + + findings, err := doctor.CheckRepo(context.Background(), cfg, home, "github.com/acme/pinned", repoDir) + if err != nil { + t.Fatalf("CheckRepo() returned an unexpected error: %v", err) + } + if _, ok := findKind(findings, doctor.KindIdentityDrift); ok { + t.Fatalf("CheckRepo() = %+v, want no %s finding for a deliberate, still valid pin", findings, doctor.KindIdentityDrift) + } + }) + + t.Run("stale pin: gitcd.profile names a profile the configuration no longer carries", func(t *testing.T) { + repoDir := filepath.Join(root, "github.com", "acme", "stale-pin") + initRepository(t, repoDir) + run(t, repoDir, "remote", "add", "origin", "https://github.com/acme/stale-pin.git") + run(t, repoDir, "config", "--local", "gitcd.profile", "retired") + + cfg := config.Config{Root: root, Default: "{host}/{owner}/{repo}"} + + findings, err := doctor.CheckRepo(context.Background(), cfg, home, "github.com/acme/stale-pin", repoDir) + if err != nil { + t.Fatalf("CheckRepo() returned an unexpected error: %v", err) + } + f, ok := findKind(findings, doctor.KindStalePin) + if !ok { + t.Fatalf("CheckRepo() = %+v, want a %s finding", findings, doctor.KindStalePin) + } + if !strings.Contains(f.Message, "retired") { + t.Fatalf("finding message = %q, want it to name the stale profile", f.Message) + } + }) + + t.Run("path mismatch: repository sits at a path the layout would not render", func(t *testing.T) { + repoDir := filepath.Join(root, "somewhere", "else") + initRepository(t, repoDir) + run(t, repoDir, "remote", "add", "origin", "https://github.com/acme/misplaced.git") + + cfg := config.Config{Root: root, Default: "{host}/{owner}/{repo}"} + + findings, err := doctor.CheckRepo(context.Background(), cfg, home, "somewhere/else", repoDir) + if err != nil { + t.Fatalf("CheckRepo() returned an unexpected error: %v", err) + } + f, ok := findKind(findings, doctor.KindPathMismatch) + if !ok { + t.Fatalf("CheckRepo() = %+v, want a %s finding", findings, doctor.KindPathMismatch) + } + want := filepath.Join(root, "github.com", "acme", "misplaced") + if !strings.Contains(f.Message, want) { + t.Fatalf("finding message = %q, want it to name the rendered path %q", f.Message, want) + } + }) + + t.Run("path case mismatch: the layout renders the same path spelled differently", func(t *testing.T) { + repoDir := filepath.Join(root, "github.com", "ACME", "cased") + initRepository(t, repoDir) + run(t, repoDir, "remote", "add", "origin", "https://github.com/acme/cased.git") + + cfg := config.Config{Root: root, Default: "{host}/{owner}/{repo}"} + + findings, err := doctor.CheckRepo(context.Background(), cfg, home, "github.com/ACME/cased", repoDir) + if err != nil { + t.Fatalf("CheckRepo() returned an unexpected error: %v", err) + } + f, ok := findKind(findings, doctor.KindPathCase) + if !ok { + t.Fatalf("CheckRepo() = %+v, want a %s finding", findings, doctor.KindPathCase) + } + if !f.Fixable { + t.Fatal("KindPathCase finding is not marked Fixable") + } + want := filepath.Join(root, "github.com", "acme", "cased") + if f.WantPath != want { + t.Fatalf("finding WantPath = %q, want %q", f.WantPath, want) + } + if _, ok := findKind(findings, doctor.KindPathMismatch); ok { + t.Fatalf("CheckRepo() = %+v, want no %s finding alongside a case only mismatch", findings, doctor.KindPathMismatch) + } + }) + + t.Run("rule case mismatch: a rule would match case-insensitively but never applies as written", func(t *testing.T) { + repoDir := filepath.Join(root, "github.com", "acme", "rule-case") + initRepository(t, repoDir) + run(t, repoDir, "remote", "add", "origin", "https://github.com/acme/rule-case.git") + + cfg := config.Config{ + Root: root, + Default: "{host}/{owner}/{repo}", + Profiles: map[string]config.Profile{"work": {"user.email": "acme-work-identity"}}, + // "Acme" differs in case from the remote's actual "acme". + Rules: []config.Rule{{Host: "github.com", Owner: "Acme", Profile: "work"}}, + } + + findings, err := doctor.CheckRepo(context.Background(), cfg, home, "github.com/acme/rule-case", repoDir) + if err != nil { + t.Fatalf("CheckRepo() returned an unexpected error: %v", err) + } + f, ok := findKind(findings, doctor.KindRuleCaseMismatch) + if !ok { + t.Fatalf("CheckRepo() = %+v, want a %s finding", findings, doctor.KindRuleCaseMismatch) + } + if !strings.Contains(f.Message, "Acme") || !strings.Contains(f.Message, "acme") { + t.Fatalf("finding message = %q, want it to name both the rule's and the remote's casing", f.Message) + } + }) + + t.Run("an exactly matching rule is never reported as a case mismatch", func(t *testing.T) { + repoDir := filepath.Join(root, "github.com", "acme", "rule-exact") + initRepository(t, repoDir) + run(t, repoDir, "remote", "add", "origin", "https://github.com/acme/rule-exact.git") + + cfg := config.Config{ + Root: root, + Default: "{host}/{owner}/{repo}", + Profiles: map[string]config.Profile{"work": {"user.email": "acme-work-identity"}}, + Rules: []config.Rule{{Host: "github.com", Owner: "acme", Profile: "work"}}, + } + + findings, err := doctor.CheckRepo(context.Background(), cfg, home, "github.com/acme/rule-exact", repoDir) + if err != nil { + t.Fatalf("CheckRepo() returned an unexpected error: %v", err) + } + if _, ok := findKind(findings, doctor.KindRuleCaseMismatch); ok { + t.Fatalf("CheckRepo() = %+v, want no %s finding for an exactly matching rule", findings, doctor.KindRuleCaseMismatch) + } + }) +} + +func TestCheckManagedFiles(t *testing.T) { + home := setupGitEnv(t) + + cfg := config.Config{ + Root: filepath.Join(home, "code"), + Profiles: map[string]config.Profile{"work": {"user.email": "acme-work-identity"}}, + } + + t.Run("missing: a rendered file does not exist on disk at all", func(t *testing.T) { + findings, err := doctor.CheckManagedFiles(cfg, home) + if err != nil { + t.Fatalf("CheckManagedFiles() returned an unexpected error: %v", err) + } + f, ok := findKind(findings, doctor.KindManagedFileMissing) + if !ok { + t.Fatalf("CheckManagedFiles() = %+v, want a %s finding", findings, doctor.KindManagedFileMissing) + } + if !f.Fixable { + t.Fatal("KindManagedFileMissing finding is not marked Fixable") + } + }) + + t.Run("stale: a rendered file exists but no longer matches the current configuration", func(t *testing.T) { + managed := filepath.Join(home, ".config", "gitcd") + if err := os.MkdirAll(filepath.Join(managed, "profiles"), 0o755); err != nil { + t.Fatalf("create %s: %v", managed, err) + } + if err := os.WriteFile(filepath.Join(managed, "profiles", "work.gitconfig"), []byte("stale content\n"), 0o644); err != nil { + t.Fatalf("write stale profile file: %v", err) + } + if err := os.WriteFile(filepath.Join(managed, "includes.gitconfig"), []byte("stale includes\n"), 0o644); err != nil { + t.Fatalf("write stale include file: %v", err) + } + + findings, err := doctor.CheckManagedFiles(cfg, home) + if err != nil { + t.Fatalf("CheckManagedFiles() returned an unexpected error: %v", err) + } + if _, ok := findKind(findings, doctor.KindManagedFileStale); !ok { + t.Fatalf("CheckManagedFiles() = %+v, want a %s finding", findings, doctor.KindManagedFileStale) + } + }) + + t.Run("no findings once every rendered file matches disk", func(t *testing.T) { + rendered, err := gitconfig.Render(cfg, home) + if err != nil { + t.Fatalf("render the managed configuration: %v", err) + } + for path, content := range rendered { + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatalf("create %s: %v", filepath.Dir(path), err) + } + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatalf("write %s: %v", path, err) + } + } + + findings, err := doctor.CheckManagedFiles(cfg, home) + if err != nil { + t.Fatalf("CheckManagedFiles() returned an unexpected error: %v", err) + } + if len(findings) != 0 { + t.Fatalf("CheckManagedFiles() = %+v, want no findings once disk matches the render", findings) + } + }) +} + +func TestCheckGlobal(t *testing.T) { + t.Run("a global identity key is reported, naming the target profile", func(t *testing.T) { + content := []byte("[user]\n\temail = [email protected]\n") + findings, err := doctor.CheckGlobal(content, "/home/user/.gitconfig", "global") + if err != nil { + t.Fatalf("CheckGlobal() returned an unexpected error: %v", err) + } + f, ok := findKind(findings, doctor.KindGlobalIdentityKey) + if !ok { + t.Fatalf("CheckGlobal() = %+v, want a %s finding", findings, doctor.KindGlobalIdentityKey) + } + if !strings.Contains(f.Message, "[email protected]") || !strings.Contains(f.Message, `"global"`) { + t.Fatalf("finding message = %q, want it to name the key's value and the target profile", f.Message) + } + }) + + t.Run("a global identity key after the include is reported as an ordering finding too", func(t *testing.T) { + content := []byte("[include]\n\tpath = /some/include.gitconfig\n[user]\n\temail = [email protected]\n") + findings, err := doctor.CheckGlobal(content, "/home/user/.gitconfig", "global") + if err != nil { + t.Fatalf("CheckGlobal() returned an unexpected error: %v", err) + } + if _, ok := findKind(findings, doctor.KindGlobalIdentityOrder); !ok { + t.Fatalf("CheckGlobal() = %+v, want a %s finding", findings, doctor.KindGlobalIdentityOrder) + } + }) + + t.Run("no findings for a global configuration with no identity key at all", func(t *testing.T) { + content := []byte("[core]\n\teditor = vim\n") + findings, err := doctor.CheckGlobal(content, "/home/user/.gitconfig", "global") + if err != nil { + t.Fatalf("CheckGlobal() returned an unexpected error: %v", err) + } + if len(findings) != 0 { + t.Fatalf("CheckGlobal() = %+v, want no findings", findings) + } + }) +} diff --git a/internal/git/config.go b/internal/git/config.go index dc010f0..20951a2 100644 --- a/internal/git/config.go +++ b/internal/git/config.go @@ -95,6 +95,25 @@ func ConfigGetLocal(ctx context.Context, path, key string) (string, error) { return strings.TrimSpace(string(out)), nil } +// ConfigGetLocalBool runs "git -C path config --local --type=bool --get +// key" and reports the boolean value git itself normalises any of its +// accepted spellings ("true", "yes", "on", "1", and their opposites) to. +// ok is false when key carries no local value at all, which is not an +// error: a repository with no local "extensions.worktreeConfig" at all is +// the common case "adopt" expects, not a failure. +func ConfigGetLocalBool(ctx context.Context, path, key string) (value bool, ok bool, err error) { + cmd := exec.CommandContext(ctx, "git", "-C", path, "config", "--local", "--type=bool", "--get", key) + out, runErr := cmd.Output() + if runErr != nil { + var exitErr *exec.ExitError + if errors.As(runErr, &exitErr) && exitErr.ExitCode() == 1 { + return false, false, nil + } + return false, false, fmt.Errorf("git: read local %s in %s: %w", key, path, runErr) + } + return strings.TrimSpace(string(out)) == "true", true, nil +} + // ConfigUnsetLocal runs "git -C path config --local --unset-all key", // removing every local value key carries. It wraps ErrConfigKeyNotSet, // rather than failing, when key already carries no local value: a caller diff --git a/internal/git/config_test.go b/internal/git/config_test.go index 6a8375e..44d7c15 100644 --- a/internal/git/config_test.go +++ b/internal/git/config_test.go @@ -191,6 +191,61 @@ func TestLocalConfig(t *testing.T) { }) } +func TestConfigGetLocalBool(t *testing.T) { + setupGitEnv(t) + + t.Run("reports true for every spelling git itself accepts", func(t *testing.T) { + for _, spelling := range []string{"true", "yes", "on", "1"} { + root := t.TempDir() + initRepository(t, root) + run(t, root, "config", "--local", "extensions.worktreeConfig", spelling) + + got, ok, err := git.ConfigGetLocalBool(context.Background(), root, "extensions.worktreeConfig") + if err != nil { + t.Fatalf("ConfigGetLocalBool() returned an unexpected error: %v", err) + } + if !ok { + t.Fatal("ConfigGetLocalBool() ok = false, want true for a key that is set") + } + if !got { + t.Fatalf("ConfigGetLocalBool() for %q = false, want true", spelling) + } + } + }) + + t.Run("reports false for every negative spelling", func(t *testing.T) { + for _, spelling := range []string{"false", "no", "off", "0"} { + root := t.TempDir() + initRepository(t, root) + run(t, root, "config", "--local", "extensions.worktreeConfig", spelling) + + got, ok, err := git.ConfigGetLocalBool(context.Background(), root, "extensions.worktreeConfig") + if err != nil { + t.Fatalf("ConfigGetLocalBool() returned an unexpected error: %v", err) + } + if !ok { + t.Fatal("ConfigGetLocalBool() ok = false, want true for a key that is set") + } + if got { + t.Fatalf("ConfigGetLocalBool() for %q = true, want false", spelling) + } + } + }) + + t.Run("ok is false for a key that is not set at all", func(t *testing.T) { + root := t.TempDir() + initRepository(t, root) + + got, ok, err := git.ConfigGetLocalBool(context.Background(), root, "extensions.worktreeConfig") + if err != nil { + t.Fatalf("ConfigGetLocalBool() returned an unexpected error: %v", err) + } + if ok { + t.Fatalf("ConfigGetLocalBool() ok = true, got = %v, want ok = false for an unset key", got) + } + }) +} + func TestShowOrigin(t *testing.T) { setupGitEnv(t) From 05d99b78ebc00a78f42fce0147ccda8cb1d515a3 Mon Sep 17 00:00:00 2001 From: Simon Ostendorf Date: Sat, 14 Nov 2026 00:56:02 +0100 Subject: [PATCH 2/6] feat(cli): add doctor and adopt doctor exits non zero while any finding stands, so a prompt or a pipeline can read it. --fix repairs only the mechanical findings and never touches an identity key it did not write. adopt refuses a repository with extensions.worktreeConfig rather than guessing which config file to write, and moves nothing without being asked. Refs #4 --- cmd/gitcd/adopt.go | 249 ++++++++++++++++++++++++++++++ cmd/gitcd/adopt_test.go | 201 +++++++++++++++++++++++++ cmd/gitcd/apply.go | 38 +++-- cmd/gitcd/doctor.go | 318 +++++++++++++++++++++++++++++++++++++++ cmd/gitcd/doctor_test.go | 214 ++++++++++++++++++++++++++ cmd/gitcd/main.go | 6 + 6 files changed, 1015 insertions(+), 11 deletions(-) create mode 100644 cmd/gitcd/adopt.go create mode 100644 cmd/gitcd/adopt_test.go create mode 100644 cmd/gitcd/doctor.go create mode 100644 cmd/gitcd/doctor_test.go diff --git a/cmd/gitcd/adopt.go b/cmd/gitcd/adopt.go new file mode 100644 index 0000000..b2fac11 --- /dev/null +++ b/cmd/gitcd/adopt.go @@ -0,0 +1,249 @@ +// Copyright 2026 Simon Ostendorf +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "bufio" + "context" + "errors" + "flag" + "fmt" + "io" + "os" + "path/filepath" + "strings" + + "github.com/ostendorf-cloud/gitcd/internal/config" + "github.com/ostendorf-cloud/gitcd/internal/git" + "github.com/ostendorf-cloud/gitcd/internal/gitconfig" + "github.com/ostendorf-cloud/gitcd/internal/layout" + "github.com/ostendorf-cloud/gitcd/internal/profile" + "github.com/ostendorf-cloud/gitcd/internal/remote" +) + +// runAdopt implements "gitcd adopt ": it moves an existing clone, +// not necessarily made by gitcd, into the path the configured layout +// would render for its own remote, fixes a "gitcd.profile" pin that no +// longer names the profile the rules currently imply, and prints the old +// and the new path. +// +// It refuses outright, before asking anything, a repository with +// "extensions.worktreeConfig" enabled: that setting lets a worktree carry +// its own copy of a config key gitcd's identity writes normally assume +// are shared, in "config.worktree" rather than the common "config" file, +// and nothing here resolves which of the two a given key would need, so +// guessing is refused rather than risking a write to the wrong one. +// +// Moving the directory asks first, on stderr, reading the answer from +// stdin: the safe default is to never move anything, so a closed or +// empty stdin, and an unrecognised answer, both decline. "--yes" skips +// the prompt for a script that already consents. Only one path is +// accepted per run; adopting a whole tree is running this in a loop, the +// same choice issue #4 makes for "migrate". +func runAdopt(args []string, stdin io.Reader, stdout, stderr io.Writer) int { + fset := flag.NewFlagSet("gitcd adopt", flag.ContinueOnError) + fset.SetOutput(stderr) + yes := fset.Bool("yes", false, "move without asking first") + dryRun := fset.Bool("dry-run", false, "print the old and new path without moving or writing anything") + verbose := fset.Bool("verbose", false, "enable debug logging on stderr") + if err := fset.Parse(args); err != nil { + return 2 + } + if fset.NArg() != 1 { + fmt.Fprintln(stderr, "gitcd adopt: exactly one path is required") + fset.Usage() + return 2 + } + logger := newLogger(*verbose, stderr) + ctx := context.Background() + + oldPath, err := filepath.Abs(fset.Arg(0)) + if err != nil { + fmt.Fprintf(stderr, "gitcd adopt: %v\n", err) + return 1 + } + if !git.IsRepository(oldPath) { + fmt.Fprintf(stderr, "gitcd adopt: %s is not a git repository\n", oldPath) + return 1 + } + + worktreeConfig, ok, err := git.ConfigGetLocalBool(ctx, oldPath, "extensions.worktreeConfig") + if err != nil { + fmt.Fprintf(stderr, "gitcd adopt: %v\n", err) + return 1 + } + if ok && worktreeConfig { + fmt.Fprintf( + stderr, + "gitcd adopt: %s has extensions.worktreeConfig enabled\n"+ + "a key gitcd would fix could belong in \"config\" or in \"config.worktree\"; adopt refuses to guess\n"+ + "disable extensions.worktreeConfig, or fix this repository's identity by hand, before adopting it\n", + oldPath, + ) + return 1 + } + + cfg, err := config.Load() + if err != nil { + fmt.Fprintf(stderr, "gitcd adopt: %v\n", err) + return 1 + } + + remoteURL, err := git.RemoteURL(ctx, oldPath) + if err != nil { + fmt.Fprintf(stderr, "gitcd adopt: read the origin remote of %s: %v\n", oldPath, err) + return 1 + } + r, err := remote.Parse(remoteURL, cfg.AliasMap()) + if err != nil { + fmt.Fprintf(stderr, "gitcd adopt: %v\n", err) + return 1 + } + + rel, err := layout.Render(cfg.LayoutFor(r.Host), r) + if err != nil { + fmt.Fprintf(stderr, "gitcd adopt: %v\n", err) + return 1 + } + newPath := filepath.Join(cfg.Root, rel) + logger.Debug("rendered the target path", "old", oldPath, "new", newPath) + + if newPath == oldPath { + fmt.Fprintln(stdout, oldPath) + fmt.Fprintln(stdout, newPath) + fmt.Fprintln(stderr, "gitcd adopt: already at the path the configured layout renders; nothing to move") + return fixAdoptedConfig(ctx, stderr, cfg, r, newPath) + } + + if conflict := adoptTargetConflict(ctx, cfg, newPath, r); conflict != "" { + fmt.Fprintf(stderr, "gitcd adopt: %s\n", conflict) + return 1 + } + + if *dryRun { + fmt.Fprintln(stdout, oldPath) + fmt.Fprintln(stdout, newPath) + return 0 + } + + if !*yes { + fmt.Fprintf(stderr, "move %s to %s? [y/N] ", oldPath, newPath) + if !readYes(stdin) { + fmt.Fprintln(stderr, "gitcd adopt: not moving: pass --yes to move without asking") + return 1 + } + } + + if err := os.MkdirAll(filepath.Dir(newPath), 0o755); err != nil { + fmt.Fprintf(stderr, "gitcd adopt: create %s: %v\n", filepath.Dir(newPath), err) + return 1 + } + if err := os.Rename(oldPath, newPath); err != nil { + fmt.Fprintf(stderr, "gitcd adopt: move %s to %s: %v\n", oldPath, newPath, err) + return 1 + } + + fmt.Fprintln(stdout, oldPath) + fmt.Fprintln(stdout, newPath) + return fixAdoptedConfig(ctx, stderr, cfg, r, newPath) +} + +// adoptTargetConflict reports why newPath must refuse to receive oldPath's +// repository, or "" when the move is clear to make: newPath already +// existing is always a conflict, named with the remote already found +// there when it can be read, since that is the detail a user deciding +// what to do next needs most. +func adoptTargetConflict(ctx context.Context, cfg config.Config, newPath string, want remote.Remote) string { + if _, err := os.Stat(newPath); err != nil { + return "" + } + if !git.IsRepository(newPath) { + return fmt.Sprintf("%s already exists and is not a git repository", newPath) + } + existingURL, err := git.RemoteURL(ctx, newPath) + if err != nil { + return fmt.Sprintf("%s already exists: %v", newPath, err) + } + existing, err := remote.Parse(existingURL, cfg.AliasMap()) + if err != nil || existing != want { + return fmt.Sprintf("%s already exists with a different remote: %s", newPath, existingURL) + } + return fmt.Sprintf("%s already exists with the same remote: it looks already adopted", newPath) +} + +// fixAdoptedConfig brings newPath's identity in line with what the +// configured rules currently imply for r, the newly adopted repository's +// own remote, then logs it: the same expected-versus-effective report +// "gitcd clone" already prints after a clone. Only a pin gitcd itself +// wrote, the "gitcd.profile" marker, is ever rewritten here, and only +// when it already names a different profile than the rules now imply; +// a repository with no pin at all is left to the includeIf chain, the +// same common case mechanism a plain "gitcd clone" relies on, so adopt +// never invents a pin a clone would not have made either. An identity +// key set directly in this repository's own local configuration, with +// no "gitcd.profile" marker to say gitcd wrote it, is reported, not +// touched: adopt never overwrites a value it did not write. +func fixAdoptedConfig(ctx context.Context, stderr io.Writer, cfg config.Config, r remote.Remote, newPath string) int { + res := profile.Resolve(r, cfg) + + pinned, err := git.ConfigGetLocal(ctx, newPath, "gitcd.profile") + switch { + case err == nil: + if res.Profile != "" && pinned != res.Profile { + prof, ok := cfg.Profiles[res.Profile] + if !ok { + fmt.Fprintf(stderr, "gitcd adopt: %s is pinned to %q, which no longer exists; left as is\n", newPath, pinned) + break + } + for _, key := range prof.Keys() { + if err := git.ConfigSetLocal(ctx, newPath, key, prof[key]); err != nil { + fmt.Fprintf(stderr, "gitcd adopt: %v\n", err) + return 1 + } + } + if err := writeProfileMarkers(ctx, newPath, res.Profile, prof); err != nil { + fmt.Fprintf(stderr, "gitcd adopt: %v\n", err) + return 1 + } + fmt.Fprintf(stderr, "gitcd adopt: updated the pin from %q to %q, the profile the rules now imply\n", pinned, res.Profile) + } + case errors.Is(err, git.ErrConfigKeyNotSet): + // No pin: the includeIf chain is left to apply the identity, the + // same mechanism an ordinary "gitcd clone" relies on without a + // "--profile" flag. + default: + fmt.Fprintf(stderr, "gitcd adopt: read gitcd.profile of %s: %v\n", newPath, err) + return 1 + } + + home, err := os.UserHomeDir() + if err != nil { + fmt.Fprintf(stderr, "gitcd adopt: %v\n", err) + return 1 + } + repo, err := profile.Inspect(ctx, newPath, cfg, gitconfig.Dir(home)) + if err != nil { + fmt.Fprintf(stderr, "gitcd adopt: read back the identity of %s: %v\n", newPath, err) + return 1 + } + if repo.Email.Origin == profile.OriginOther { + fmt.Fprintf( + stderr, "gitcd adopt: %s sets user.email directly, outside any profile gitcd manages; left as is\n", newPath, + ) + } + fmt.Fprintf(stderr, "gitcd adopt: expected profile: %s\n", describeExpected(repo)) + fmt.Fprintf(stderr, "gitcd adopt: user.email: %s\n", describeIdentity(repo.Email)) + fmt.Fprintf(stderr, "gitcd adopt: user.signingkey: %s\n", describeIdentity(repo.SigningKey)) + return 0 +} + +// readYes reads one line from stdin and reports whether it is "y" or +// "yes", case insensitively. A closed or empty stdin, the shape a script +// or CI job's stdin commonly has, reaches EOF immediately and reports +// false, the safe default, without blocking. +func readYes(stdin io.Reader) bool { + line, _ := bufio.NewReader(stdin).ReadString('\n') + answer := strings.ToLower(strings.TrimSpace(line)) + return answer == "y" || answer == "yes" +} diff --git a/cmd/gitcd/adopt_test.go b/cmd/gitcd/adopt_test.go new file mode 100644 index 0000000..57bf7bd --- /dev/null +++ b/cmd/gitcd/adopt_test.go @@ -0,0 +1,201 @@ +// Copyright 2026 Simon Ostendorf +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestRunAdopt_MovesWithConsentAndPrintsBothPaths(t *testing.T) { + home := setupEnv(t) + root := filepath.Join(home, "Code") + writeFile(t, filepath.Join(home, ".gitcd.yaml"), "root: "+root+"\n") + + oldPath := filepath.Join(home, "elsewhere", "widget") + initRepository(t, oldPath) + runGit(t, oldPath, "remote", "add", "origin", "https://github.com/acme/widget.git") + + var stdout, stderr bytes.Buffer + code := run([]string{"adopt", "--yes", oldPath}, &stdout, &stderr) + if code != 0 { + t.Fatalf("adopt --yes = %d, want 0, stderr: %s", code, stderr.String()) + } + + wantNewPath := filepath.Join(root, "github.com", "acme", "widget") + lines := strings.Split(strings.TrimRight(stdout.String(), "\n"), "\n") + if len(lines) != 2 { + t.Fatalf("adopt stdout = %q, want exactly two lines, the old and the new path", stdout.String()) + } + if lines[0] != oldPath { + t.Fatalf("adopt stdout first line = %q, want the old path %q", lines[0], oldPath) + } + if lines[1] != wantNewPath { + t.Fatalf("adopt stdout second line = %q, want the new path %q", lines[1], wantNewPath) + } + + if _, err := os.Stat(wantNewPath); err != nil { + t.Fatalf("adopt did not move the repository to %s: %v", wantNewPath, err) + } + if _, err := os.Stat(oldPath); err == nil { + t.Fatalf("adopt left the old path %s behind", oldPath) + } +} + +func TestRunAdopt_WithoutConsentNeverMoves(t *testing.T) { + home := setupEnv(t) + root := filepath.Join(home, "Code") + writeFile(t, filepath.Join(home, ".gitcd.yaml"), "root: "+root+"\n") + + oldPath := filepath.Join(home, "elsewhere", "widget") + initRepository(t, oldPath) + runGit(t, oldPath, "remote", "add", "origin", "https://github.com/acme/widget.git") + + var stdout, stderr bytes.Buffer + // No "--yes", and stdin is empty: the prompt reaches EOF immediately + // and must decline, the safe default. + code := run([]string{"adopt", oldPath}, &stdout, &stderr) + if code == 0 { + t.Fatal("adopt with no consent and empty stdin = 0, want a non-zero exit") + } + if _, err := os.Stat(oldPath); err != nil { + t.Fatalf("adopt moved the repository despite no consent: %v", err) + } + wantNewPath := filepath.Join(root, "github.com", "acme", "widget") + if _, err := os.Stat(wantNewPath); err == nil { + t.Fatalf("adopt moved the repository to %s despite no consent", wantNewPath) + } +} + +func TestRunAdopt_DryRunNeverMoves(t *testing.T) { + home := setupEnv(t) + root := filepath.Join(home, "Code") + writeFile(t, filepath.Join(home, ".gitcd.yaml"), "root: "+root+"\n") + + oldPath := filepath.Join(home, "elsewhere", "widget") + initRepository(t, oldPath) + runGit(t, oldPath, "remote", "add", "origin", "https://github.com/acme/widget.git") + + var stdout, stderr bytes.Buffer + code := run([]string{"adopt", "--dry-run", oldPath}, &stdout, &stderr) + if code != 0 { + t.Fatalf("adopt --dry-run = %d, want 0, stderr: %s", code, stderr.String()) + } + if _, err := os.Stat(oldPath); err != nil { + t.Fatalf("adopt --dry-run moved the repository: %v", err) + } + wantNewPath := filepath.Join(root, "github.com", "acme", "widget") + if _, err := os.Stat(wantNewPath); err == nil { + t.Fatalf("adopt --dry-run wrote to %s", wantNewPath) + } + if !strings.Contains(stdout.String(), oldPath) || !strings.Contains(stdout.String(), wantNewPath) { + t.Fatalf("adopt --dry-run stdout = %q, want both the old and the new path", stdout.String()) + } +} + +func TestRunAdopt_RefusesWorktreeConfig(t *testing.T) { + home := setupEnv(t) + root := filepath.Join(home, "Code") + writeFile(t, filepath.Join(home, ".gitcd.yaml"), "root: "+root+"\n") + + oldPath := filepath.Join(home, "elsewhere", "widget") + initRepository(t, oldPath) + runGit(t, oldPath, "remote", "add", "origin", "https://github.com/acme/widget.git") + runGit(t, oldPath, "config", "--local", "extensions.worktreeConfig", "true") + + var stdout, stderr bytes.Buffer + code := run([]string{"adopt", "--yes", oldPath}, &stdout, &stderr) + if code == 0 { + t.Fatal("adopt on a repository with extensions.worktreeConfig = 0, want a refusal") + } + if !strings.Contains(stderr.String(), "worktreeConfig") { + t.Fatalf("adopt stderr = %q, want it to name extensions.worktreeConfig", stderr.String()) + } + if _, err := os.Stat(oldPath); err != nil { + t.Fatalf("adopt moved the repository despite refusing: %v", err) + } +} + +func TestRunAdopt_RefusesAnExistingTargetWithADifferentRemote(t *testing.T) { + home := setupEnv(t) + root := filepath.Join(home, "Code") + writeFile(t, filepath.Join(home, ".gitcd.yaml"), "root: "+root+"\n") + + newPath := filepath.Join(root, "github.com", "acme", "widget") + initRepository(t, newPath) + runGit(t, newPath, "remote", "add", "origin", "https://github.com/acme/other.git") + + oldPath := filepath.Join(home, "elsewhere", "widget") + initRepository(t, oldPath) + runGit(t, oldPath, "remote", "add", "origin", "https://github.com/acme/widget.git") + + var stdout, stderr bytes.Buffer + code := run([]string{"adopt", "--yes", oldPath}, &stdout, &stderr) + if code == 0 { + t.Fatal("adopt onto an occupied target = 0, want a refusal") + } + // The specific, friendly wording adoptTargetConflict produces, not + // merely a path mentioned somewhere: a bare directory rename onto an + // already occupied target fails at the OS level regardless, with its + // own message naming both paths, so that alone would not prove this + // refusal runs before ever attempting the move. + if !strings.Contains(stderr.String(), "already exists with a different remote") { + t.Fatalf("adopt stderr = %q, want the specific conflict message naming %s", stderr.String(), newPath) + } + if _, err := os.Stat(oldPath); err != nil { + t.Fatalf("adopt moved the repository despite refusing: %v", err) + } +} + +func TestRunAdopt_UpdatesAStalePinToTheCurrentlyExpectedProfile(t *testing.T) { + home := setupEnv(t) + root := filepath.Join(home, "Code") + writeFile(t, filepath.Join(home, ".gitcd.yaml"), ` +root: `+root+` +profiles: + work: + user.email: acme-work-identity + personal: + user.email: acme-personal-identity +rules: + - host: github.com + owner: acme + profile: work +`) + + oldPath := filepath.Join(home, "elsewhere", "widget") + initRepository(t, oldPath) + runGit(t, oldPath, "remote", "add", "origin", "https://github.com/acme/widget.git") + runGit(t, oldPath, "config", "--local", "user.email", "acme-personal-identity") + runGit(t, oldPath, "config", "--local", "gitcd.profile", "personal") + + var stdout, stderr bytes.Buffer + code := run([]string{"adopt", "--yes", oldPath}, &stdout, &stderr) + if code != 0 { + t.Fatalf("adopt --yes = %d, want 0, stderr: %s", code, stderr.String()) + } + + newPath := filepath.Join(root, "github.com", "acme", "widget") + got := runGitOutput(t, newPath, "config", "--local", "--get", "gitcd.profile") + if got != "work" { + t.Fatalf("gitcd.profile after adopt = %q, want it updated to %q, the profile the rules now imply", got, "work") + } + gotEmail := runGitOutput(t, newPath, "config", "--local", "--get", "user.email") + if gotEmail != "acme-work-identity" { + t.Fatalf("user.email after adopt = %q, want %q", gotEmail, "acme-work-identity") + } +} + +func TestRunAdopt_ExactlyOnePathIsRequired(t *testing.T) { + setupEnv(t) + + var stdout, stderr bytes.Buffer + code := run([]string{"adopt"}, &stdout, &stderr) + if code != 2 { + t.Fatalf("adopt with no path = %d, want 2", code) + } +} diff --git a/cmd/gitcd/apply.go b/cmd/gitcd/apply.go index e12c0f6..c29c53e 100644 --- a/cmd/gitcd/apply.go +++ b/cmd/gitcd/apply.go @@ -7,6 +7,7 @@ import ( "flag" "fmt" "io" + "log/slog" "os" "path/filepath" "sort" @@ -52,30 +53,45 @@ func runApply(args []string, stdout, stderr io.Writer) int { return 1 } - paths := make([]string, 0, len(rendered)) - for path := range rendered { - paths = append(paths, path) - } - sort.Strings(paths) - if *dryRun { + paths := make([]string, 0, len(rendered)) + for path := range rendered { + paths = append(paths, path) + } + sort.Strings(paths) for _, path := range paths { fmt.Fprintf(stdout, "%s:\n%s\n", path, rendered[path]) } return 0 } + if err := writeRenderedFiles(rendered, logger, stdout); err != nil { + fmt.Fprintf(stderr, "gitcd apply: %v\n", err) + return 1 + } + return 0 +} + +// writeRenderedFiles writes every path in rendered, creating its parent +// directory first, in a deterministic order: the mechanical write both +// "gitcd apply" and "gitcd doctor --fix" perform, kept in one place so +// the two never drift into two different ways of doing the same write. +func writeRenderedFiles(rendered gitconfig.Rendered, logger *slog.Logger, stdout io.Writer) error { + paths := make([]string, 0, len(rendered)) + for path := range rendered { + paths = append(paths, path) + } + sort.Strings(paths) + for _, path := range paths { if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { - fmt.Fprintf(stderr, "gitcd apply: create %s: %v\n", filepath.Dir(path), err) - return 1 + return fmt.Errorf("create %s: %w", filepath.Dir(path), err) } if err := os.WriteFile(path, []byte(rendered[path]), 0o644); err != nil { - fmt.Fprintf(stderr, "gitcd apply: write %s: %v\n", path, err) - return 1 + return fmt.Errorf("write %s: %w", path, err) } logger.Debug("wrote a managed file", "path", path) fmt.Fprintln(stdout, path) } - return 0 + return nil } diff --git a/cmd/gitcd/doctor.go b/cmd/gitcd/doctor.go new file mode 100644 index 0000000..a3933d8 --- /dev/null +++ b/cmd/gitcd/doctor.go @@ -0,0 +1,318 @@ +// Copyright 2026 Simon Ostendorf +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "context" + "encoding/json" + "errors" + "flag" + "fmt" + "io" + "log/slog" + "os" + "path/filepath" + "strings" + + "github.com/ostendorf-cloud/gitcd/internal/config" + "github.com/ostendorf-cloud/gitcd/internal/doctor" + "github.com/ostendorf-cloud/gitcd/internal/git" + "github.com/ostendorf-cloud/gitcd/internal/gitconfig" +) + +// doctorOutput is the shape "gitcd doctor --json" encodes: both +// configuration homes AC7 of issue #4 asks doctor to always print, plus +// what "--fix" repaired and what is still outstanding. +type doctorOutput struct { + ConfigPath string `json:"configPath"` + ManagedDir string `json:"managedDir"` + Fixed []doctor.Finding `json:"fixed,omitempty"` + Findings []doctor.Finding `json:"findings,omitempty"` +} + +// runDoctor implements "gitcd doctor": every per repository check under +// the configured root, plus the managed file and global git +// configuration checks, reported as one finding per problem. The walk +// itself is discoverRepos, the same one "status" and "sync" already use: +// issue #3's internal/scan, built for "list" and "adopt" with a richer +// repository value, lands separately and the two walks are reconciled +// once both exist, per AGENTS.md. A repository reachable only through a +// "hasconfig" rule, outside root, is not found by this walk and so is +// not checked; internal/doctor's own doc comment and docs/concepts/gitcd.md's +// open question about "list" cover the same gap for the same reason. +// +// "--fix" repairs only what is mechanical: a stale or missing managed +// file (re-rendered, exactly as "gitcd apply" would), and a path that +// differs from the layout only in character case (renamed in place). +// Every other finding, identity drift, a stale pin, a structural path +// mismatch, a rule that almost matches but for a case difference, and +// every global identity key, is always reported and never touched: a +// value doctor did not write is never one it overwrites, and a drift +// that is not mechanical is skipped and reported rather than guessed at. +func runDoctor(args []string, stdout, stderr io.Writer) int { + fset := flag.NewFlagSet("gitcd doctor", flag.ContinueOnError) + fset.SetOutput(stderr) + fix := fset.Bool("fix", false, "repair every mechanical finding: a stale or missing managed file, and a case only path mismatch") + dryRun := fset.Bool("dry-run", false, "with --fix, print what would change without writing or moving anything") + jsonOutput := fset.Bool("json", false, "print structured output instead of a text report") + verbose := fset.Bool("verbose", false, "enable debug logging on stderr") + if err := fset.Parse(args); err != nil { + return 2 + } + if fset.NArg() != 0 { + fmt.Fprintln(stderr, "gitcd doctor: no arguments are accepted") + fset.Usage() + return 2 + } + logger := newLogger(*verbose, stderr) + ctx := context.Background() + + cfg, err := config.Load() + if err != nil { + fmt.Fprintf(stderr, "gitcd doctor: %v\n", err) + return 1 + } + cfgPath, err := config.Path() + if err != nil { + fmt.Fprintf(stderr, "gitcd doctor: %v\n", err) + return 1 + } + home, err := os.UserHomeDir() + if err != nil { + fmt.Fprintf(stderr, "gitcd doctor: %v\n", err) + return 1 + } + managedDir := gitconfig.Dir(home) + + findings, err := collectFindings(ctx, cfg, home, logger, stderr) + if err != nil { + fmt.Fprintf(stderr, "gitcd doctor: %v\n", err) + return 1 + } + + var fixed, outstanding []doctor.Finding + if *fix { + fixed, outstanding = applyFixes(findings, cfg, home, logger, stdout, stderr, *dryRun) + } else { + outstanding = findings + } + + if *jsonOutput { + out := doctorOutput{ConfigPath: cfgPath, ManagedDir: managedDir, Fixed: fixed, Findings: outstanding} + if err := json.NewEncoder(stdout).Encode(out); err != nil { + fmt.Fprintf(stderr, "gitcd doctor: encode output: %v\n", err) + return 1 + } + } else { + writeDoctorReport(stdout, cfgPath, managedDir, fixed, outstanding) + } + + if len(outstanding) > 0 { + return 1 + } + return 0 +} + +// collectFindings runs every check doctor performs: one pass over +// discoverRepos's walk of cfg.Root, the managed file comparison, and the +// global git configuration checks. +func collectFindings(ctx context.Context, cfg config.Config, home string, logger *slog.Logger, stderr io.Writer) ([]doctor.Finding, error) { + var findings []doctor.Finding + + paths, err := discoverRepos(cfg.Root) + if err != nil { + return nil, fmt.Errorf("search %s: %w", cfg.Root, err) + } + logger.Debug("discovered repositories", "root", cfg.Root, "count", len(paths)) + for _, path := range paths { + name := repoName(cfg.Root, path) + repoFindings, err := doctor.CheckRepo(ctx, cfg, home, name, path) + if err != nil { + // One repository's own failure, a remote gitcd cannot parse + // for instance, is reported and skipped rather than aborting + // the whole run: a fleet wide command must still report on + // every other repository. + fmt.Fprintf(stderr, "gitcd doctor: %s: %v\n", name, err) + continue + } + findings = append(findings, repoFindings...) + } + + managedFindings, err := doctor.CheckManagedFiles(cfg, home) + if err != nil { + return nil, err + } + findings = append(findings, managedFindings...) + + globalPath, err := git.GlobalConfigPath() + if err != nil { + return nil, err + } + globalContent, err := os.ReadFile(globalPath) + if err != nil && !errors.Is(err, os.ErrNotExist) { + return nil, fmt.Errorf("read %s: %w", globalPath, err) + } + globalFindings, err := doctor.CheckGlobal(globalContent, globalPath, defaultMovedProfileName) + if err != nil { + return nil, err + } + findings = append(findings, globalFindings...) + + return findings, nil +} + +// applyFixes repairs every mechanical finding in all, splitting the +// input into what was fixed and what is still outstanding. Every +// managed file finding is repaired by one re-render, since +// internal/gitconfig.Render always reproduces every managed file's +// current, correct content in one pass; a case only path mismatch is +// repaired by renaming the repository in place. +func applyFixes( + all []doctor.Finding, cfg config.Config, home string, logger *slog.Logger, stdout, stderr io.Writer, dryRun bool, +) (fixed, outstanding []doctor.Finding) { + hasManaged := false + for _, f := range all { + if f.Kind == doctor.KindManagedFileMissing || f.Kind == doctor.KindManagedFileStale { + hasManaged = true + break + } + } + + managedWritten := false + if hasManaged { + if dryRun { + fmt.Fprintf(stdout, "would rerender every managed file under %s\n", gitconfig.Dir(home)) + } else if err := rerenderManagedFiles(cfg, home, logger, stdout); err != nil { + fmt.Fprintf(stderr, "gitcd doctor: %v\n", err) + } else { + managedWritten = true + } + } + + for _, f := range all { + switch { + case (f.Kind == doctor.KindManagedFileMissing || f.Kind == doctor.KindManagedFileStale) && managedWritten: + fixed = append(fixed, f) + case f.Kind == doctor.KindPathCase && dryRun: + fmt.Fprintf(stdout, "would rename %s to %s\n", f.Path, f.WantPath) + outstanding = append(outstanding, f) + case f.Kind == doctor.KindPathCase: + if err := renameCaseOnly(f.Path, f.WantPath); err != nil { + fmt.Fprintf(stderr, "gitcd doctor: %v\n", err) + outstanding = append(outstanding, f) + continue + } + fmt.Fprintf(stdout, "renamed %s to %s\n", f.Path, f.WantPath) + fixed = append(fixed, f) + default: + outstanding = append(outstanding, f) + } + } + return fixed, outstanding +} + +// rerenderManagedFiles re-renders and writes every managed file for cfg, +// exactly what "gitcd apply" itself does for a non dry run, reused here +// rather than reimplemented so the two can never drift into two +// different ways of performing the same write. +func rerenderManagedFiles(cfg config.Config, home string, logger *slog.Logger, stdout io.Writer) error { + rendered, err := gitconfig.Render(cfg, home) + if err != nil { + return err + } + return writeRenderedFiles(rendered, logger, stdout) +} + +// renameCaseOnly moves oldPath to newPath, a path differing from it only +// in character case, possibly in a segment above the repository's own +// leaf directory, a mis-cased owner segment for instance: it renames the +// topmost segment that actually differs, carrying everything below it +// along in the same move, rather than only the leaf, which would leave +// a mis-cased ancestor untouched. The move goes through a sibling +// temporary name: a case insensitive but case preserving filesystem, the +// common case on macOS, treats a direct rename between two spellings of +// the same name as a no-op, unlike a case sensitive one, so a plain +// two-argument rename cannot be relied on to force the change on every +// filesystem alike. +// +// When the topmost differing ancestor no longer exists at its old path +// but already exists at the new one, it was already renamed by an +// earlier call in the same "doctor --fix" run, fixing a sibling +// repository under the same mis-cased ancestor: this call is then a +// no-op rather than an error. +func renameCaseOnly(oldPath, newPath string) error { + oldAncestor, newAncestor := firstDifferingSegment(oldPath, newPath) + if oldAncestor == newAncestor { + return nil + } + if _, err := os.Stat(oldAncestor); errors.Is(err, os.ErrNotExist) { + if _, err := os.Stat(newAncestor); err == nil { + return nil + } + } + + tmp := oldAncestor + ".gitcd-doctor-rename-tmp" + if err := os.Rename(oldAncestor, tmp); err != nil { + return fmt.Errorf("rename %s to a temporary name: %w", oldAncestor, err) + } + if err := os.Rename(tmp, newAncestor); err != nil { + // Best effort restore, so a failed second hop does not leave the + // repository sitting at an unexpected temporary name. + _ = os.Rename(tmp, oldAncestor) + return fmt.Errorf("rename %s to %s: %w", oldAncestor, newAncestor, err) + } + return nil +} + +// firstDifferingSegment walks oldPath and newPath component by +// component and returns the path of the first (topmost) component at +// which they differ, in each of the two inputs, so a caller can rename +// exactly that directory and carry everything below it along in one +// move. Both return values equal oldPath and newPath themselves when +// the two share no common prefix at all. +func firstDifferingSegment(oldPath, newPath string) (string, string) { + oldSegs := strings.Split(filepath.Clean(oldPath), string(filepath.Separator)) + newSegs := strings.Split(filepath.Clean(newPath), string(filepath.Separator)) + + i := 0 + for i < len(oldSegs) && i < len(newSegs) && oldSegs[i] == newSegs[i] { + i++ + } + if i >= len(oldSegs) || i >= len(newSegs) { + return oldPath, newPath + } + return strings.Join(oldSegs[:i+1], string(filepath.Separator)), strings.Join(newSegs[:i+1], string(filepath.Separator)) +} + +// writeDoctorReport renders doctor's text report: both configuration +// homes AC7 asks doctor to always print, then one line per fixed and per +// still outstanding finding. +func writeDoctorReport(stdout io.Writer, cfgPath, managedDir string, fixed, outstanding []doctor.Finding) { + fmt.Fprintf(stdout, "config file: %s\n", cfgPath) + fmt.Fprintf(stdout, "managed directory: %s\n", managedDir) + if len(fixed) == 0 && len(outstanding) == 0 { + fmt.Fprintln(stdout, "no findings") + return + } + for _, f := range fixed { + fmt.Fprintf(stdout, "fixed [%s] %s: %s\n", f.Kind, findingSubject(f), f.Message) + } + for _, f := range outstanding { + fmt.Fprintf(stdout, "[%s] %s: %s\n", f.Kind, findingSubject(f), f.Message) + } +} + +// findingSubject names what f is about, for the text report: the +// repository when there is one, the path when there is no repository but +// a path, "global" otherwise. +func findingSubject(f doctor.Finding) string { + switch { + case f.Repo != "": + return f.Repo + case f.Path != "": + return f.Path + default: + return "global" + } +} diff --git a/cmd/gitcd/doctor_test.go b/cmd/gitcd/doctor_test.go new file mode 100644 index 0000000..f666a58 --- /dev/null +++ b/cmd/gitcd/doctor_test.go @@ -0,0 +1,214 @@ +// Copyright 2026 Simon Ostendorf +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "bytes" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestRunDoctor_NoArgumentsAccepted(t *testing.T) { + setupEnv(t) + + var stdout, stderr bytes.Buffer + code := run([]string{"doctor", "extra"}, &stdout, &stderr) + if code != 2 { + t.Fatalf("doctor with an argument = %d, want 2", code) + } +} + +func TestRunDoctor_ReportsAMissingManagedFileAndExitsNonZero(t *testing.T) { + home := setupEnv(t) + writeFile(t, filepath.Join(home, ".gitcd.yaml"), ` +profiles: + work: + user.email: acme-work-identity +rules: + - host: github.com + profile: work +`) + + var stdout, stderr bytes.Buffer + code := run([]string{"doctor"}, &stdout, &stderr) + if code != 1 { + t.Fatalf("doctor = %d, want 1, stdout: %s stderr: %s", code, stdout.String(), stderr.String()) + } + + out := stdout.String() + includePath := filepath.Join(home, ".config", "gitcd", "includes.gitconfig") + if !strings.Contains(out, includePath) { + t.Fatalf("doctor stdout = %q, want it to name the missing include file %s", out, includePath) + } + cfgPath := filepath.Join(home, ".gitcd.yaml") + if !strings.Contains(out, cfgPath) { + t.Fatalf("doctor stdout = %q, want it to print the config file path %s", out, cfgPath) + } + managedDir := filepath.Join(home, ".config", "gitcd") + if !strings.Contains(out, managedDir) { + t.Fatalf("doctor stdout = %q, want it to print the managed directory %s", out, managedDir) + } +} + +func TestRunDoctor_NoRepositoriesAndNoManagedFilesNeededIsStillNonZeroOnAGlobalIdentityKey(t *testing.T) { + // setupEnv itself sets a global user.name and user.email, exactly + // the finding doctor exists to report: with no profiles configured + // at all, there is nothing to render, so the only finding left is + // the global identity key setupEnv's own baseline carries. + setupEnv(t) + + var stdout, stderr bytes.Buffer + code := run([]string{"doctor"}, &stdout, &stderr) + if code != 1 { + t.Fatalf("doctor = %d, want 1 for a global identity key finding, stdout: %s stderr: %s", code, stdout.String(), stderr.String()) + } + if !strings.Contains(stdout.String(), "global-identity-key") { + t.Fatalf("doctor stdout = %q, want a global-identity-key finding", stdout.String()) + } +} + +func TestRunDoctor_FixRerendersAMissingManagedFile(t *testing.T) { + home := setupEnv(t) + writeFile(t, filepath.Join(home, ".gitcd.yaml"), ` +profiles: + work: + user.email: acme-work-identity +rules: + - host: github.com + profile: work +`) + includePath := filepath.Join(home, ".config", "gitcd", "includes.gitconfig") + if _, err := os.Stat(includePath); err == nil { + t.Fatalf("%s already exists before --fix", includePath) + } + + var stdout, stderr bytes.Buffer + code := run([]string{"doctor", "--fix"}, &stdout, &stderr) + // The global identity key setupEnv's own baseline carries is never + // fixed, so the exit code stays non-zero even after a successful fix + // of everything mechanical. + if code != 1 { + t.Fatalf("doctor --fix = %d, want 1 (the global identity key finding is never fixed), stderr: %s", code, stderr.String()) + } + if _, err := os.Stat(includePath); err != nil { + t.Fatalf("doctor --fix did not render %s: %v", includePath, err) + } + if !strings.Contains(stdout.String(), "fixed [managed-file-missing]") { + t.Fatalf("doctor --fix stdout = %q, want it to report the managed file finding as fixed", stdout.String()) + } + for _, line := range strings.Split(stdout.String(), "\n") { + if strings.HasPrefix(line, "[managed-file-missing]") || strings.HasPrefix(line, "[managed-file-stale]") { + t.Fatalf("doctor --fix stdout carries an outstanding managed file finding: %q", line) + } + } +} + +func TestRunDoctor_FixDryRunChangesNothing(t *testing.T) { + home := setupEnv(t) + writeFile(t, filepath.Join(home, ".gitcd.yaml"), ` +profiles: + work: + user.email: acme-work-identity +rules: + - host: github.com + profile: work +`) + includePath := filepath.Join(home, ".config", "gitcd", "includes.gitconfig") + + var stdout, stderr bytes.Buffer + code := run([]string{"doctor", "--fix", "--dry-run"}, &stdout, &stderr) + if code != 1 { + t.Fatalf("doctor --fix --dry-run = %d, want 1, stderr: %s", code, stderr.String()) + } + if _, err := os.Stat(includePath); err == nil { + t.Fatalf("doctor --fix --dry-run wrote %s", includePath) + } + if !strings.Contains(stdout.String(), "would rerender") { + t.Fatalf("doctor --fix --dry-run stdout = %q, want it to describe what would change", stdout.String()) + } +} + +func TestRunDoctor_JSONOutput(t *testing.T) { + home := setupEnv(t) + + var stdout, stderr bytes.Buffer + code := run([]string{"doctor", "--json"}, &stdout, &stderr) + if code != 1 { + t.Fatalf("doctor --json = %d, want 1, stderr: %s", code, stderr.String()) + } + + var out struct { + ConfigPath string `json:"configPath"` + ManagedDir string `json:"managedDir"` + Findings []struct { + Kind string `json:"kind"` + Message string `json:"message"` + } `json:"findings"` + } + if err := json.Unmarshal(stdout.Bytes(), &out); err != nil { + t.Fatalf("doctor --json produced invalid JSON: %v, output: %s", err, stdout.String()) + } + if out.ConfigPath != filepath.Join(home, ".gitcd.yaml") { + t.Fatalf("doctor --json configPath = %q, want %q", out.ConfigPath, filepath.Join(home, ".gitcd.yaml")) + } + if len(out.Findings) == 0 { + t.Fatal("doctor --json findings is empty, want at least the global identity key finding") + } +} + +func TestRunDoctor_FixRenamesACaseOnlyPathMismatch(t *testing.T) { + home := setupEnv(t) + root := filepath.Join(home, "Code") + writeFile(t, filepath.Join(home, ".gitcd.yaml"), "root: "+root+"\n") + + // The layout renders lowercase, but this clone sits under an upper + // cased owner segment: a case only mismatch. + misCased := filepath.Join(root, "github.com", "ACME", "widget") + initRepository(t, misCased) + runGit(t, misCased, "remote", "add", "origin", "https://github.com/acme/widget.git") + + var stdout, stderr bytes.Buffer + // The global identity key from setupEnv's own baseline is never + // fixed, so the exit code is not asserted here: TestRunDoctor_FixRerendersAMissingManagedFile + // already covers that it stays non-zero. + run([]string{"doctor", "--fix"}, &stdout, &stderr) + + wantPath := filepath.Join(root, "github.com", "acme", "widget") + if _, err := os.Stat(wantPath); err != nil { + t.Fatalf("doctor --fix did not rename the repository to %s: %v", wantPath, err) + } + // os.Stat alone cannot tell the two spellings apart on a case + // insensitive, case preserving filesystem (APFS, the macOS default): + // it resolves "ACME" and "acme" to the same entry either way. Reading + // the parent directory back reports the entry's own, exact spelling. + entries, err := os.ReadDir(filepath.Join(root, "github.com")) + if err != nil { + t.Fatalf("read %s: %v", filepath.Join(root, "github.com"), err) + } + var names []string + for _, e := range entries { + names = append(names, e.Name()) + } + if !containsExact(names, "acme") { + t.Fatalf("directory entries under github.com = %v, want the exact spelling %q on disk", names, "acme") + } + if containsExact(names, "ACME") { + t.Fatalf("directory entries under github.com = %v, want the old spelling %q gone", names, "ACME") + } + if !strings.Contains(stdout.String(), "renamed") { + t.Fatalf("doctor --fix stdout = %q, want it to report the rename", stdout.String()) + } +} + +func containsExact(names []string, want string) bool { + for _, n := range names { + if n == want { + return true + } + } + return false +} diff --git a/cmd/gitcd/main.go b/cmd/gitcd/main.go index 284c967..1612099 100644 --- a/cmd/gitcd/main.go +++ b/cmd/gitcd/main.go @@ -30,6 +30,8 @@ Commands: status print branch, dirty and ahead or behind state for every repository sync fetch every repository with a bounded pool, never pulling or merging open open a repository's remote in a browser + doctor report identity and layout drift across every repository + adopt move an existing clone into the layout and fix its config shell-init print the shell integration snippet for zsh, bash or fish version print the version @@ -65,6 +67,10 @@ func run(args []string, stdout, stderr io.Writer) int { return runSync(args[1:], stdout, stderr) case "open": return runOpen(args[1:], stdout, stderr) + case "doctor": + return runDoctor(args[1:], stdout, stderr) + case "adopt": + return runAdopt(args[1:], os.Stdin, stdout, stderr) case "shell-init": return runShellInit(args[1:], stdout, stderr) case "help", "-h", "--help": From 4dbbdd2ca118fca8f0bccf5d131c81bfc25ad57a Mon Sep 17 00:00:00 2001 From: Simon Ostendorf Date: Sat, 14 Nov 2026 00:56:22 +0100 Subject: [PATCH 3/6] docs(agents): record the doctor package Refs #4 --- AGENTS.md | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 39730d7..8f8a37e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,12 +24,15 @@ not repeated here. - `internal/gitsync/`: fetches a set of repositories through a bounded pool of workers; never pulls, rebases or merges. Named apart from `sync` so no caller shadows the standard library package of that name. +- `internal/doctor/`: detects identity drift, a repository at a path the layout would not + render, a missing or stale managed file and a global identity key. Detection only; the + command layer writes. - `internal/shell/`: the per shell init snippet `shell-init` prints. -`list`, `cd`, `doctor` and `adopt` are later milestones, tracked as issues; their own packages -land with them. `cmd/gitcd/discover.go` walks the configured root for `status`, `sync` and -`open`; issue #3's `internal/scan` walks the same ground for `list`, `doctor` and `adopt` with -a richer value, and the two should be reconciled into one once both have landed. +`list` and `cd` are a later milestone, tracked as an issue; their own packages land with them. +`cmd/gitcd/discover.go` walks the configured root for `status`, `sync`, `open` and `doctor`; +issue #3's `internal/scan` walks the same ground for `list` and `cd` with a richer value, and +issue #15 reconciles the two into one once both have landed. ## Commands From 841061543ba4dec07403a774d29f274edd2df4af Mon Sep 17 00:00:00 2001 From: Simon Ostendorf Date: Sat, 14 Nov 2026 01:45:10 +0100 Subject: [PATCH 4/6] feat(git): list a repository's linked worktrees and identity keys Refs #4 --- internal/git/config.go | 55 +++++++++++++++++++++++++++++++++++++ internal/git/config_test.go | 48 ++++++++++++++++++++++++++++++++ internal/git/git.go | 21 ++++++++++++++ internal/git/git_test.go | 49 +++++++++++++++++++++++++++++++++ 4 files changed, 173 insertions(+) diff --git a/internal/git/config.go b/internal/git/config.go index 20951a2..5342ca8 100644 --- a/internal/git/config.go +++ b/internal/git/config.go @@ -290,6 +290,61 @@ func IdentityKeyAfterInclude(content []byte, keys []string) (key string, found b return "", false, nil } +// IdentityKeysAfterInclude is IdentityKeyAfterInclude, reporting every +// key in keys that sits after the last bare "[include]" section, in file +// order, rather than only the first it finds: "git config --global --add" +// would insert a new key for any rule into that same section, so a +// "global-identity-order" finding naming only one of several keys already +// at risk would understate the problem for no reason. +func IdentityKeysAfterInclude(content []byte, keys []string) ([]string, error) { + lines := strings.Split(string(content), "\n") + + lastIncludeEnd := -1 + inBareInclude := false + for i, line := range lines { + if m := configSectionHeader.FindStringSubmatch(line); m != nil { + inBareInclude = strings.EqualFold(m[1], "include") && m[2] == "" + continue + } + if inBareInclude { + lastIncludeEnd = i + } + } + if lastIncludeEnd == -1 { + return nil, nil + } + + wanted := make(map[string]string, len(keys)) + for _, k := range keys { + wanted[strings.ToLower(k)] = k + } + + var found []string + seen := make(map[string]bool, len(keys)) + section := "" + for _, line := range lines[lastIncludeEnd+1:] { + 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]) + key, ok := wanted[full] + if !ok || seen[full] { + continue + } + seen[full] = true + found = append(found, key) + } + return found, nil +} + // GlobalIdentityValue is one identity key the global git configuration // sets directly, with the value it carries. type GlobalIdentityValue struct { diff --git a/internal/git/config_test.go b/internal/git/config_test.go index 44d7c15..3f11f92 100644 --- a/internal/git/config_test.go +++ b/internal/git/config_test.go @@ -395,6 +395,54 @@ func TestIdentityKeyAfterInclude(t *testing.T) { // identity key sitting between two include sections ends up before the // insertion point, safe, while one sitting after both does not. +func TestIdentityKeysAfterInclude(t *testing.T) { + t.Parallel() + + t.Run("no include section at all reports no keys", func(t *testing.T) { + t.Parallel() + content := "[user]\n\tname = Test User\n\temail = [email protected]\n" + keys, err := git.IdentityKeysAfterInclude([]byte(content), git.IdentityKeys) + if err != nil { + t.Fatalf("IdentityKeysAfterInclude() returned an unexpected error: %v", err) + } + if len(keys) != 0 { + t.Fatalf("IdentityKeysAfterInclude() = %v, want no keys", keys) + } + }) + + t.Run("every identity key after the include is reported, not only the first", func(t *testing.T) { + t.Parallel() + content := "[include]\n\tpath = /some/other/include.gitconfig\n" + + "[user]\n\tname = After Include\n\temail = [email protected]\n\tsigningkey = ssh-ed25519 AAAATRAILING\n" + keys, err := git.IdentityKeysAfterInclude([]byte(content), git.IdentityKeys) + if err != nil { + t.Fatalf("IdentityKeysAfterInclude() returned an unexpected error: %v", err) + } + want := []string{"user.name", "user.email", "user.signingkey"} + if len(keys) != len(want) { + t.Fatalf("IdentityKeysAfterInclude() = %v, want %v", keys, want) + } + for i, k := range want { + if keys[i] != k { + t.Fatalf("IdentityKeysAfterInclude() = %v, want %v", keys, want) + } + } + }) + + t.Run("a key before the include is never reported", func(t *testing.T) { + t.Parallel() + content := "[user]\n\tname = Before Include\n" + + "[include]\n\tpath = /some/other/include.gitconfig\n" + keys, err := git.IdentityKeysAfterInclude([]byte(content), git.IdentityKeys) + if err != nil { + t.Fatalf("IdentityKeysAfterInclude() returned an unexpected error: %v", err) + } + if len(keys) != 0 { + t.Fatalf("IdentityKeysAfterInclude() = %v, want no keys", keys) + } + }) +} + func TestFindGlobalIdentityValues(t *testing.T) { t.Parallel() diff --git a/internal/git/git.go b/internal/git/git.go index f33709f..6790ee2 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -75,3 +75,24 @@ func IsRepository(path string) bool { _, err := os.Stat(filepath.Join(path, ".git")) return err == nil } + +// WorktreePaths runs "git -C path worktree list --porcelain" and returns +// the absolute path of every worktree the repository containing path has, +// the main checkout first and then every linked worktree, in the order +// git itself reports them: a caller refusing to move a repository that +// still has a linked worktree, or that is one itself, needs this to tell +// the two apart from an ordinary clone with no worktree of its own. +func WorktreePaths(ctx context.Context, path string) ([]string, error) { + cmd := exec.CommandContext(ctx, "git", "-C", path, "worktree", "list", "--porcelain") + out, err := cmd.Output() + if err != nil { + return nil, fmt.Errorf("git: list the worktrees of %s: %w", path, err) + } + var paths []string + for _, line := range strings.Split(string(out), "\n") { + if rest, ok := strings.CutPrefix(line, "worktree "); ok { + paths = append(paths, rest) + } + } + return paths, nil +} diff --git a/internal/git/git_test.go b/internal/git/git_test.go index e878f0f..7b07c69 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -246,3 +246,52 @@ func TestIsRepository(t *testing.T) { } }) } + +func TestWorktreePaths(t *testing.T) { + setupGitEnv(t) + + t.Run("an ordinary clone with no linked worktree reports exactly itself", func(t *testing.T) { + root := t.TempDir() + source := filepath.Join(root, "source") + if err := os.MkdirAll(source, 0o755); err != nil { + t.Fatalf("create %s: %v", source, err) + } + initRepository(t, source) + + paths, err := git.WorktreePaths(context.Background(), source) + if err != nil { + t.Fatalf("WorktreePaths() returned an unexpected error: %v", err) + } + if len(paths) != 1 { + t.Fatalf("WorktreePaths() = %v, want exactly one entry", paths) + } + }) + + t.Run("a main checkout with a linked worktree reports both", func(t *testing.T) { + root := t.TempDir() + source := filepath.Join(root, "source") + if err := os.MkdirAll(source, 0o755); err != nil { + t.Fatalf("create %s: %v", source, err) + } + initRepository(t, source) + + worktree := filepath.Join(root, "worktree") + run(t, source, "worktree", "add", worktree) + + paths, err := git.WorktreePaths(context.Background(), source) + if err != nil { + t.Fatalf("WorktreePaths() returned an unexpected error: %v", err) + } + if len(paths) != 2 { + t.Fatalf("WorktreePaths() = %v, want exactly two entries", paths) + } + + fromWorktree, err := git.WorktreePaths(context.Background(), worktree) + if err != nil { + t.Fatalf("WorktreePaths() from the linked worktree returned an unexpected error: %v", err) + } + if len(fromWorktree) != 2 { + t.Fatalf("WorktreePaths() from the linked worktree = %v, want exactly two entries", fromWorktree) + } + }) +} From 11fca972db4726aade537add0215e63f95696d84 Mon Sep 17 00:00:00 2001 From: Simon Ostendorf Date: Sat, 14 Nov 2026 01:45:10 +0100 Subject: [PATCH 5/6] fix(doctor): refuse an ancestor two repositories spell differently Renaming it fixed one repository and un-placed the other, so --fix flipped the directory back and forth, reporting success each time and moving everything underneath. The conflict is a finding now, naming both repositories, and --fix converges. Refs #4 --- internal/doctor/doctor.go | 149 +++++++++++++++++++++++++++++++-- internal/doctor/doctor_test.go | 101 +++++++++++++++++++++- 2 files changed, 238 insertions(+), 12 deletions(-) diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 2990aca..1dc0eea 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -74,6 +74,15 @@ const ( // that section, so a key already after it keeps beating every // includeIf value silently. KindGlobalIdentityOrder Kind = "global-identity-order" + // KindAncestorCaseConflict is an ancestor directory, shared by two or + // more repositories under root, whose repositories disagree on its + // spelling: each repository's own remote implies a different case + // for the same, physically shared directory. Renaming it to fix one + // repository would immediately un-place the other, and the reverse + // rename on a later run would un-place the first again, so this is + // never Fixable: "doctor --fix" reports it and renames nothing under + // that ancestor, rather than flipping the two forever. + KindAncestorCaseConflict Kind = "ancestor-case-conflict" ) // Finding is one problem doctor reports. @@ -218,6 +227,114 @@ func checkPath(cfg config.Config, name, path string, repo profile.Repo) []Findin }} } +// RepoPath pairs one repository's own on-disk path with the path the +// configured layout renders for its remote, so CheckAncestorCaseConflicts +// can compare every repository under root against its siblings, including +// one that is not itself mismatched: a repository already sitting at the +// path its own remote implies still needs to be told apart from the one +// renaming a shared ancestor would place it at by accident. +type RepoPath struct { + // Repo is the repository name, "/" separated relative to root. + Repo string + // Path is the repository's actual, on-disk path. + Path string + // Want is the path the configured layout renders for this + // repository's own remote, "" when it could not be rendered at all. + Want string +} + +// FirstDifferingSegment walks oldPath and newPath component by component +// and returns the path of the first (topmost) component at which they +// differ, in each of the two inputs: the ancestor directory a caller +// renaming oldPath to newPath actually needs to rename, carrying +// everything below it along in one move, rather than only the leaf. Both +// return values equal oldPath and newPath themselves when the two share +// no common prefix at all. +func FirstDifferingSegment(oldPath, newPath string) (string, string) { + oldSegs := strings.Split(filepath.Clean(oldPath), string(filepath.Separator)) + newSegs := strings.Split(filepath.Clean(newPath), string(filepath.Separator)) + + i := 0 + for i < len(oldSegs) && i < len(newSegs) && oldSegs[i] == newSegs[i] { + i++ + } + if i >= len(oldSegs) || i >= len(newSegs) { + return oldPath, newPath + } + return strings.Join(oldSegs[:i+1], string(filepath.Separator)), strings.Join(newSegs[:i+1], string(filepath.Separator)) +} + +// CheckAncestorCaseConflicts reports KindAncestorCaseConflict for every +// ancestor directory that two or more of repos would rename to a +// different spelling: for each repository whose own path is a case-only +// mismatch of its own Want, it finds every sibling repository physically +// under the same ancestor directory and asks what that sibling's own Want +// implies for the very same ancestor. When siblings disagree, case +// insensitively equal but not identical, on what that one shared, +// physical directory should be called, renaming it to fix one would +// un-place the other, so neither is fixed and this is reported instead. +// A repository whose Want already equals Path is not itself mismatched, +// but is still consulted as a sibling: it is the repository a careless +// rename would newly un-place, exactly the bug this check exists to +// catch before "doctor --fix" ever makes it. +func CheckAncestorCaseConflicts(repos []RepoPath) []Finding { + var findings []Finding + seenAncestor := map[string]bool{} + + for _, rp := range repos { + if rp.Want == "" || rp.Path == rp.Want || !strings.EqualFold(rp.Path, rp.Want) { + continue + } + oldAncestor, _ := FirstDifferingSegment(rp.Path, rp.Want) + if seenAncestor[oldAncestor] { + continue + } + seenAncestor[oldAncestor] = true + + depth := len(strings.Split(filepath.Clean(oldAncestor), string(filepath.Separator))) + wanted := map[string][]string{} + for _, sib := range repos { + if sib.Want == "" { + continue + } + if sib.Path != oldAncestor && !strings.HasPrefix(sib.Path, oldAncestor+string(filepath.Separator)) { + continue + } + wantSegs := strings.Split(filepath.Clean(sib.Want), string(filepath.Separator)) + if depth > len(wantSegs) { + continue + } + wantAncestor := strings.Join(wantSegs[:depth], string(filepath.Separator)) + wanted[wantAncestor] = append(wanted[wantAncestor], sib.Repo) + } + if len(wanted) < 2 { + continue + } + + castings := make([]string, 0, len(wanted)) + for casing := range wanted { + castings = append(castings, casing) + } + sort.Strings(castings) + parts := make([]string, 0, len(castings)) + for _, casing := range castings { + names := wanted[casing] + sort.Strings(names) + parts = append(parts, fmt.Sprintf("%q (%s)", casing, strings.Join(names, ", "))) + } + findings = append(findings, Finding{ + Kind: KindAncestorCaseConflict, + Path: oldAncestor, + Message: fmt.Sprintf( + "repositories under %s disagree on how it should be spelled: %s; doctor --fix refuses to rename it, "+ + "since fixing one of them would un-place the other", + oldAncestor, strings.Join(parts, ", "), + ), + }) + } + return findings +} + // checkRuleCase reports KindRuleCaseMismatch for every rule whose host, // owner and repo all match repo's remote case-insensitively but not // exactly: profile.Resolve compares owner and repo with a case sensitive @@ -315,31 +432,45 @@ func managedFileNote(path, includePath string) string { // CheckGlobal reports every global identity finding: KindGlobalIdentityKey // for each identity key content, the global git configuration's own raw // text, sets directly, naming movedProfileTarget as the profile it would -// belong in, and KindGlobalIdentityOrder when one of them sits after the -// gitcd include, which would keep winning over every includeIf value -// silently. -func CheckGlobal(content []byte, globalPath, movedProfileTarget string) ([]Finding, error) { +// belong in, and KindGlobalIdentityOrder when one or more of them sit +// after the gitcd include, which would keep winning over every includeIf +// value silently. +// +// movedProfileTargetExists tells the finding whether movedProfileTarget +// already names a profile the configuration actually carries: a caller +// resolves this against its own configuration, since this package never +// loads one itself. When false, the message says so rather than naming a +// profile as if the configuration already carried it, which it need not: +// "gitcd init --move-identity" accepts a "--profile" naming any profile +// at all, and nothing here can recover which one was chosen in the past +// for a key moved before this run. +func CheckGlobal(content []byte, globalPath, movedProfileTarget string, movedProfileTargetExists bool) ([]Finding, error) { var findings []Finding + belongsIn := fmt.Sprintf("profile %q", movedProfileTarget) + if !movedProfileTargetExists { + belongsIn = fmt.Sprintf("a profile such as %q, which the configuration does not yet define", movedProfileTarget) + } for _, v := range git.FindGlobalIdentityValues(content, git.IdentityKeys) { findings = append(findings, Finding{ Kind: KindGlobalIdentityKey, Message: fmt.Sprintf( - "%s sets %s = %s directly; \"user.useConfigOnly\" cannot suppress it, so every repository gitcd manages, including one no rule matches, gets this identity; it belongs in profile %q instead", - globalPath, v.Key, v.Value, movedProfileTarget, + "%s sets %s = %s directly; \"user.useConfigOnly\" cannot suppress it, so every repository gitcd manages, including one no rule matches, gets this identity; it belongs in %s instead", + globalPath, v.Key, v.Value, belongsIn, ), }) } - key, found, err := git.IdentityKeyAfterInclude(content, git.IdentityKeys) + keys, err := git.IdentityKeysAfterInclude(content, git.IdentityKeys) if err != nil { return nil, fmt.Errorf("doctor: check %s for an identity key after the include: %w", globalPath, err) } - if found { + if len(keys) > 0 { findings = append(findings, Finding{ Kind: KindGlobalIdentityOrder, Message: fmt.Sprintf( - "%s sets %s after the gitcd include; it would keep beating every includeIf value silently", globalPath, key, + "%s sets %s after the gitcd include; a key there keeps beating every includeIf value silently", + globalPath, strings.Join(keys, ", "), ), }) } diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go index 5c0a9fa..b9ea317 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -274,6 +274,69 @@ func TestCheckRepo(t *testing.T) { }) } +func TestCheckAncestorCaseConflicts(t *testing.T) { + t.Parallel() + + t.Run("two repositories under one ancestor disagree: a conflict, naming both", func(t *testing.T) { + t.Parallel() + repos := []doctor.RepoPath{ + {Repo: "github.com/Acme/widget", Path: "/code/github.com/Acme/widget", Want: "/code/github.com/acme/widget"}, + {Repo: "github.com/Acme/other", Path: "/code/github.com/Acme/other", Want: "/code/github.com/Acme/other"}, + } + findings := doctor.CheckAncestorCaseConflicts(repos) + f, ok := findKind(findings, doctor.KindAncestorCaseConflict) + if !ok { + t.Fatalf("CheckAncestorCaseConflicts() = %+v, want a %s finding", findings, doctor.KindAncestorCaseConflict) + } + if f.Path != "/code/github.com/Acme" { + t.Fatalf("finding Path = %q, want the shared ancestor %q", f.Path, "/code/github.com/Acme") + } + if !strings.Contains(f.Message, "github.com/Acme/widget") || !strings.Contains(f.Message, "github.com/Acme/other") { + t.Fatalf("finding message = %q, want it to name both repositories", f.Message) + } + }) + + t.Run("two repositories under one ancestor that agree: no conflict", func(t *testing.T) { + t.Parallel() + repos := []doctor.RepoPath{ + {Repo: "github.com/Acme/widget", Path: "/code/github.com/Acme/widget", Want: "/code/github.com/acme/widget"}, + {Repo: "github.com/Acme/gadget", Path: "/code/github.com/Acme/gadget", Want: "/code/github.com/acme/gadget"}, + } + findings := doctor.CheckAncestorCaseConflicts(repos) + if len(findings) != 0 { + t.Fatalf("CheckAncestorCaseConflicts() = %+v, want no findings for an ancestor both repositories agree on", findings) + } + }) + + t.Run("a repository with no sibling under its ancestor: no conflict", func(t *testing.T) { + t.Parallel() + repos := []doctor.RepoPath{ + {Repo: "github.com/Acme/widget", Path: "/code/github.com/Acme/widget", Want: "/code/github.com/acme/widget"}, + } + findings := doctor.CheckAncestorCaseConflicts(repos) + if len(findings) != 0 { + t.Fatalf("CheckAncestorCaseConflicts() = %+v, want no findings with no sibling repository", findings) + } + }) + + t.Run("a repository already at its own wanted path is never itself flagged", func(t *testing.T) { + t.Parallel() + repos := []doctor.RepoPath{ + {Repo: "github.com/Acme/widget", Path: "/code/github.com/Acme/widget", Want: "/code/github.com/acme/widget"}, + {Repo: "github.com/Acme/other", Path: "/code/github.com/Acme/other", Want: "/code/github.com/Acme/other"}, + } + findings := doctor.CheckAncestorCaseConflicts(repos) + if _, ok := findKind(findings, doctor.KindAncestorCaseConflict); !ok { + t.Fatalf("CheckAncestorCaseConflicts() = %+v, want a conflict finding", findings) + } + for _, f := range findings { + if f.Repo == "github.com/Acme/other" { + t.Fatalf("CheckAncestorCaseConflicts() = %+v, want no finding naming the already correct repository as its own subject", findings) + } + } + }) +} + func TestCheckManagedFiles(t *testing.T) { home := setupGitEnv(t) @@ -344,7 +407,7 @@ func TestCheckManagedFiles(t *testing.T) { func TestCheckGlobal(t *testing.T) { t.Run("a global identity key is reported, naming the target profile", func(t *testing.T) { content := []byte("[user]\n\temail = [email protected]\n") - findings, err := doctor.CheckGlobal(content, "/home/user/.gitconfig", "global") + findings, err := doctor.CheckGlobal(content, "/home/user/.gitconfig", "global", true) if err != nil { t.Fatalf("CheckGlobal() returned an unexpected error: %v", err) } @@ -357,9 +420,24 @@ func TestCheckGlobal(t *testing.T) { } }) + t.Run("a target profile that does not exist yet is named as a suggestion, not as fact", func(t *testing.T) { + content := []byte("[user]\n\temail = [email protected]\n") + findings, err := doctor.CheckGlobal(content, "/home/user/.gitconfig", "global", false) + if err != nil { + t.Fatalf("CheckGlobal() returned an unexpected error: %v", err) + } + f, ok := findKind(findings, doctor.KindGlobalIdentityKey) + if !ok { + t.Fatalf("CheckGlobal() = %+v, want a %s finding", findings, doctor.KindGlobalIdentityKey) + } + if !strings.Contains(f.Message, "does not yet define") { + t.Fatalf("finding message = %q, want it to say the profile is not yet configured", f.Message) + } + }) + t.Run("a global identity key after the include is reported as an ordering finding too", func(t *testing.T) { content := []byte("[include]\n\tpath = /some/include.gitconfig\n[user]\n\temail = [email protected]\n") - findings, err := doctor.CheckGlobal(content, "/home/user/.gitconfig", "global") + findings, err := doctor.CheckGlobal(content, "/home/user/.gitconfig", "global", true) if err != nil { t.Fatalf("CheckGlobal() returned an unexpected error: %v", err) } @@ -368,9 +446,26 @@ func TestCheckGlobal(t *testing.T) { } }) + t.Run("every identity key after the include is named, not only the first", func(t *testing.T) { + content := []byte( + "[include]\n\tpath = /some/include.gitconfig\n[user]\n\tname = Someone\n\temail = [email protected]\n", + ) + findings, err := doctor.CheckGlobal(content, "/home/user/.gitconfig", "global", true) + if err != nil { + t.Fatalf("CheckGlobal() returned an unexpected error: %v", err) + } + f, ok := findKind(findings, doctor.KindGlobalIdentityOrder) + if !ok { + t.Fatalf("CheckGlobal() = %+v, want a %s finding", findings, doctor.KindGlobalIdentityOrder) + } + if !strings.Contains(f.Message, "user.name") || !strings.Contains(f.Message, "user.email") { + t.Fatalf("finding message = %q, want it to name both keys sitting after the include", f.Message) + } + }) + t.Run("no findings for a global configuration with no identity key at all", func(t *testing.T) { content := []byte("[core]\n\teditor = vim\n") - findings, err := doctor.CheckGlobal(content, "/home/user/.gitconfig", "global") + findings, err := doctor.CheckGlobal(content, "/home/user/.gitconfig", "global", true) if err != nil { t.Fatalf("CheckGlobal() returned an unexpected error: %v", err) } From b2e544132e7525e0f300e379d405c62bf8434678 Mon Sep 17 00:00:00 2001 From: Simon Ostendorf Date: Sat, 14 Nov 2026 01:45:19 +0100 Subject: [PATCH 6/6] fix(cli): never move a repository git has linked elsewhere adopt moved a linked worktree, or a main checkout that still had one, and reported success while leaving the other end at "not a git repository". Both directions are refused now, naming the git command that does move one safely. Also: a failed move no longer leaves the directories it created behind, the rename message names the ancestor that actually moved, an unreadable target counts as a conflict rather than as free, and --json stays valid JSON while --fix reports progress. Refs #4 --- cmd/gitcd/adopt.go | 139 ++++++++++++++++++++++++++++++++- cmd/gitcd/adopt_test.go | 122 +++++++++++++++++++++++++++++ cmd/gitcd/doctor.go | 142 +++++++++++++++++++++++++--------- cmd/gitcd/doctor_test.go | 161 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 523 insertions(+), 41 deletions(-) diff --git a/cmd/gitcd/adopt.go b/cmd/gitcd/adopt.go index b2fac11..b029403 100644 --- a/cmd/gitcd/adopt.go +++ b/cmd/gitcd/adopt.go @@ -35,6 +35,18 @@ import ( // and nothing here resolves which of the two a given key would need, so // guessing is refused rather than risking a write to the wrong one. // +// It also refuses outright a linked worktree, and a main checkout that +// still has one: a linked worktree's ".git" file names an absolute path +// into its main checkout's own ".git/worktrees", and the main checkout's +// own administrative files name an absolute path back, so moving either +// end breaks the pair the moment the other is not moved along with it, +// git.IsRepository's own guard accepting the ".git" file a linked +// worktree carries just as readily as an ordinary clone's ".git" +// directory. Repairing both ends with "git worktree repair" after the +// move is possible, but adopt only moves one path per run, so it refuses +// rather than silently taking on the job of repairing a pair of +// directories it was not asked to move together. +// // Moving the directory asks first, on stderr, reading the answer from // stdin: the safe default is to never move anything, so a closed or // empty stdin, and an unrecognised answer, both decline. "--yes" skips @@ -68,6 +80,16 @@ func runAdopt(args []string, stdin io.Reader, stdout, stderr io.Writer) int { return 1 } + worktreeMsg, err := adoptWorktreeConflict(ctx, oldPath) + if err != nil { + fmt.Fprintf(stderr, "gitcd adopt: %v\n", err) + return 1 + } + if worktreeMsg != "" { + fmt.Fprintf(stderr, "gitcd adopt: %s\n", worktreeMsg) + return 1 + } + worktreeConfig, ok, err := git.ConfigGetLocalBool(ctx, oldPath, "extensions.worktreeConfig") if err != nil { fmt.Fprintf(stderr, "gitcd adopt: %v\n", err) @@ -121,6 +143,11 @@ func runAdopt(args []string, stdin io.Reader, stdout, stderr io.Writer) int { return 1 } + if rel, err := filepath.Rel(oldPath, newPath); err == nil && rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) && rel != "." { + fmt.Fprintf(stderr, "gitcd adopt: %s is inside %s; adopt refuses to move a repository into itself\n", newPath, oldPath) + return 1 + } + if *dryRun { fmt.Fprintln(stdout, oldPath) fmt.Fprintln(stdout, newPath) @@ -135,11 +162,22 @@ func runAdopt(args []string, stdin io.Reader, stdout, stderr io.Writer) int { } } - if err := os.MkdirAll(filepath.Dir(newPath), 0o755); err != nil { - fmt.Fprintf(stderr, "gitcd adopt: create %s: %v\n", filepath.Dir(newPath), err) + mkdirTarget := filepath.Dir(newPath) + createdFrom := firstMissingAncestor(mkdirTarget) + if err := os.MkdirAll(mkdirTarget, 0o755); err != nil { + fmt.Fprintf(stderr, "gitcd adopt: create %s: %v\n", mkdirTarget, err) return 1 } if err := os.Rename(oldPath, newPath); err != nil { + if createdFrom != "" { + if removeErr := os.RemoveAll(createdFrom); removeErr != nil { + fmt.Fprintf( + stderr, "gitcd adopt: move %s to %s: %v (also failed to remove %s, created for the move: %v)\n", + oldPath, newPath, err, createdFrom, removeErr, + ) + return 1 + } + } fmt.Fprintf(stderr, "gitcd adopt: move %s to %s: %v\n", oldPath, newPath, err) return 1 } @@ -149,14 +187,107 @@ func runAdopt(args []string, stdin io.Reader, stdout, stderr io.Writer) int { return fixAdoptedConfig(ctx, stderr, cfg, r, newPath) } +// adoptWorktreeConflict reports why oldPath must refuse to move at all, +// before anything else about it is even read, because it is a linked +// worktree or still has one of its own: a linked worktree's ".git" file +// names an absolute path into its main checkout's own +// ".git/worktrees", and the main checkout's own administrative files +// name an absolute path back, so moving either end without moving the +// other breaks the pair, silently, the moment a routine "git worktree +// prune" or "git gc" next runs. It returns "" when oldPath carries no +// linked worktree relationship at all. +func adoptWorktreeConflict(ctx context.Context, oldPath string) (string, error) { + gitDir, err := git.Dir(ctx, oldPath) + if err != nil { + return "", fmt.Errorf("read the git directory of %s: %w", oldPath, err) + } + commonDir, err := git.CommonDir(ctx, oldPath) + if err != nil { + return "", fmt.Errorf("read the common directory of %s: %w", oldPath, err) + } + if gitDir != commonDir { + return fmt.Sprintf( + "%s is a linked worktree; moving it would break git's own link back to its main checkout\n"+ + "move a linked worktree with \"git worktree move %s \" instead", + oldPath, oldPath, + ), nil + } + + worktrees, err := git.WorktreePaths(ctx, oldPath) + if err != nil { + return "", fmt.Errorf("list the worktrees of %s: %w", oldPath, err) + } + // git reports every worktree path resolved through any symlink, + // "git rev-parse --path-format=absolute" style; oldPath is only + // "filepath.Abs" of whatever the user typed, so the two are compared + // resolved the same way, or oldPath itself would misidentify as its + // own, separate linked worktree whenever a symlink sits on its path, + // a system temporary directory being the common case. + resolvedOld := resolveSymlinks(oldPath) + var others []string + for _, w := range worktrees { + if resolveSymlinks(w) != resolvedOld { + others = append(others, w) + } + } + if len(others) == 0 { + return "", nil + } + return fmt.Sprintf( + "%s has a linked worktree at %s; moving it would break git's own link to it\n"+ + "move the linked worktree first with \"git worktree move\", or remove it with \"git worktree remove\", then adopt this repository", + oldPath, strings.Join(others, ", "), + ), nil +} + +// resolveSymlinks resolves every symlink in path, falling back to path +// itself when it cannot, empty or not yet created being the common case. +func resolveSymlinks(path string) string { + if resolved, err := filepath.EvalSymlinks(path); err == nil { + return resolved + } + return path +} + +// firstMissingAncestor returns the topmost directory, walking up from +// dir, that does not yet exist on disk, or "" when dir already exists: +// "os.MkdirAll(dir, ...)" creates exactly this directory and everything +// below it, so "os.RemoveAll" on the same path undoes that MkdirAll +// precisely, leaving every ancestor directory that already existed +// before untouched. +func firstMissingAncestor(dir string) string { + if _, err := os.Stat(dir); err == nil { + return "" + } + missing := dir + for { + parent := filepath.Dir(missing) + if parent == missing { + return missing + } + if _, err := os.Stat(parent); err == nil { + return missing + } + missing = parent + } +} + // adoptTargetConflict reports why newPath must refuse to receive oldPath's // repository, or "" when the move is clear to make: newPath already // existing is always a conflict, named with the remote already found // there when it can be read, since that is the detail a user deciding -// what to do next needs most. +// what to do next needs most. An "os.Stat" error other than "the path +// does not exist", a permission denied or a symlink loop for instance, +// is reported as a conflict too, rather than read as "no conflict": the +// move itself would very likely fail the same way, after having already +// asked for consent, and newPath might in fact already exist behind +// whatever is blocking the check. func adoptTargetConflict(ctx context.Context, cfg config.Config, newPath string, want remote.Remote) string { if _, err := os.Stat(newPath); err != nil { - return "" + if errors.Is(err, os.ErrNotExist) { + return "" + } + return fmt.Sprintf("cannot check whether %s already exists: %v", newPath, err) } if !git.IsRepository(newPath) { return fmt.Sprintf("%s already exists and is not a git repository", newPath) diff --git a/cmd/gitcd/adopt_test.go b/cmd/gitcd/adopt_test.go index 57bf7bd..947e89d 100644 --- a/cmd/gitcd/adopt_test.go +++ b/cmd/gitcd/adopt_test.go @@ -190,6 +190,128 @@ rules: } } +func TestRunAdopt_RefusesALinkedWorktree(t *testing.T) { + home := setupEnv(t) + root := filepath.Join(home, "Code") + writeFile(t, filepath.Join(home, ".gitcd.yaml"), "root: "+root+"\n") + + mainCheckout := filepath.Join(home, "elsewhere", "widget") + initRepository(t, mainCheckout) + runGit(t, mainCheckout, "remote", "add", "origin", "https://github.com/acme/widget.git") + + worktree := filepath.Join(home, "elsewhere", "widget-feat") + runGit(t, mainCheckout, "worktree", "add", worktree, "-b", "feat") + + var stdout, stderr bytes.Buffer + code := run([]string{"adopt", "--yes", worktree}, &stdout, &stderr) + if code == 0 { + t.Fatal("adopt on a linked worktree = 0, want a refusal") + } + if !strings.Contains(stderr.String(), "linked worktree") { + t.Fatalf("adopt stderr = %q, want it to name the linked worktree", stderr.String()) + } + if _, err := os.Stat(worktree); err != nil { + t.Fatalf("adopt moved the linked worktree despite refusing: %v", err) + } + // The pair must still work as git itself understands it: a worktree + // adopt silently broke would fail this, exactly the symptom the + // review's own reproduction demonstrated. + runGit(t, worktree, "rev-parse", "--is-inside-work-tree") +} + +func TestRunAdopt_RefusesAMainCheckoutWithALinkedWorktree(t *testing.T) { + home := setupEnv(t) + root := filepath.Join(home, "Code") + writeFile(t, filepath.Join(home, ".gitcd.yaml"), "root: "+root+"\n") + + mainCheckout := filepath.Join(home, "elsewhere", "widget") + initRepository(t, mainCheckout) + runGit(t, mainCheckout, "remote", "add", "origin", "https://github.com/acme/widget.git") + + worktree := filepath.Join(home, "elsewhere", "widget-feat") + runGit(t, mainCheckout, "worktree", "add", worktree, "-b", "feat") + + var stdout, stderr bytes.Buffer + code := run([]string{"adopt", "--yes", mainCheckout}, &stdout, &stderr) + if code == 0 { + t.Fatal("adopt on a main checkout with a linked worktree = 0, want a refusal") + } + if !strings.Contains(stderr.String(), "linked worktree") { + t.Fatalf("adopt stderr = %q, want it to name the linked worktree", stderr.String()) + } + if _, err := os.Stat(mainCheckout); err != nil { + t.Fatalf("adopt moved the main checkout despite refusing: %v", err) + } + // The pair must still work as git itself understands it: a main + // checkout moved out from under its worktree without repairing the + // link would fail this, exactly the symptom the review's own + // reproduction demonstrated. + runGit(t, worktree, "rev-parse", "--is-inside-work-tree") +} + +func TestRunAdopt_RefusesMovingIntoItselfAndLeavesNoDirectoryBehind(t *testing.T) { + home := setupEnv(t) + // The repository itself becomes the configured root, so the layout + // renders a target path nested inside the very repository being + // adopted: the scenario the review's own reproduction used. + root := filepath.Join(home, "Code") + writeFile(t, filepath.Join(home, ".gitcd.yaml"), "root: "+root+"\n") + initRepository(t, root) + runGit(t, root, "remote", "add", "origin", "https://github.com/acme/gadget.git") + + var stdout, stderr bytes.Buffer + code := run([]string{"adopt", "--yes", root}, &stdout, &stderr) + if code == 0 { + t.Fatal("adopt onto a target nested inside the source = 0, want a refusal") + } + if !strings.Contains(stderr.String(), "inside") { + t.Fatalf("adopt stderr = %q, want it to say the target is inside the source", stderr.String()) + } + entries, err := os.ReadDir(root) + if err != nil { + t.Fatalf("read %s: %v", root, err) + } + for _, e := range entries { + if e.Name() == "github.com" { + t.Fatalf("adopt left %s behind inside the repository it refused to move", filepath.Join(root, "github.com")) + } + } +} + +func TestRunAdopt_TreatsAnUncheckableTargetAsAConflict(t *testing.T) { + home := setupEnv(t) + root := filepath.Join(home, "Code") + writeFile(t, filepath.Join(home, ".gitcd.yaml"), "root: "+root+"\n") + + // The target's immediate parent exists but is unreadable: "os.Stat" + // on the target itself fails with a permission error, not "the path + // does not exist". + blocked := filepath.Join(root, "github.com", "acme") + if err := os.MkdirAll(blocked, 0o755); err != nil { + t.Fatalf("create %s: %v", blocked, err) + } + if err := os.Chmod(blocked, 0o000); err != nil { + t.Fatalf("chmod %s: %v", blocked, err) + } + t.Cleanup(func() { _ = os.Chmod(blocked, 0o755) }) + + oldPath := filepath.Join(home, "elsewhere", "widget") + initRepository(t, oldPath) + runGit(t, oldPath, "remote", "add", "origin", "https://github.com/acme/widget.git") + + var stdout, stderr bytes.Buffer + code := run([]string{"adopt", "--yes", oldPath}, &stdout, &stderr) + if code == 0 { + t.Fatal("adopt onto an unreadable target = 0, want a refusal") + } + if !strings.Contains(stderr.String(), "cannot check") { + t.Fatalf("adopt stderr = %q, want it to say the target could not be checked", stderr.String()) + } + if _, err := os.Stat(oldPath); err != nil { + t.Fatalf("adopt moved the repository despite an unreadable target: %v", err) + } +} + func TestRunAdopt_ExactlyOnePathIsRequired(t *testing.T) { setupEnv(t) diff --git a/cmd/gitcd/doctor.go b/cmd/gitcd/doctor.go index a3933d8..add86d3 100644 --- a/cmd/gitcd/doctor.go +++ b/cmd/gitcd/doctor.go @@ -19,6 +19,8 @@ import ( "github.com/ostendorf-cloud/gitcd/internal/doctor" "github.com/ostendorf-cloud/gitcd/internal/git" "github.com/ostendorf-cloud/gitcd/internal/gitconfig" + "github.com/ostendorf-cloud/gitcd/internal/layout" + "github.com/ostendorf-cloud/gitcd/internal/remote" ) // doctorOutput is the shape "gitcd doctor --json" encodes: both @@ -93,7 +95,16 @@ func runDoctor(args []string, stdout, stderr io.Writer) int { var fixed, outstanding []doctor.Finding if *fix { - fixed, outstanding = applyFixes(findings, cfg, home, logger, stdout, stderr, *dryRun) + // "--fix" prints a progress line per repair it makes. With + // "--json", stdout must carry nothing but the single encoded + // object a caller pipes to "jq", so every progress line goes to + // stderr instead, exactly where every other log line already + // goes. + progress := stdout + if *jsonOutput { + progress = stderr + } + fixed, outstanding = applyFixes(findings, cfg, home, logger, progress, stderr, *dryRun) } else { outstanding = findings } @@ -115,8 +126,9 @@ func runDoctor(args []string, stdout, stderr io.Writer) int { } // collectFindings runs every check doctor performs: one pass over -// discoverRepos's walk of cfg.Root, the managed file comparison, and the -// global git configuration checks. +// discoverRepos's walk of cfg.Root, the ancestor case conflict check that +// pass also makes possible, the managed file comparison, and the global +// git configuration checks. func collectFindings(ctx context.Context, cfg config.Config, home string, logger *slog.Logger, stderr io.Writer) ([]doctor.Finding, error) { var findings []doctor.Finding @@ -125,6 +137,8 @@ func collectFindings(ctx context.Context, cfg config.Config, home string, logger return nil, fmt.Errorf("search %s: %w", cfg.Root, err) } logger.Debug("discovered repositories", "root", cfg.Root, "count", len(paths)) + + var repoPaths []doctor.RepoPath for _, path := range paths { name := repoName(cfg.Root, path) repoFindings, err := doctor.CheckRepo(ctx, cfg, home, name, path) @@ -137,6 +151,27 @@ func collectFindings(ctx context.Context, cfg config.Config, home string, logger continue } findings = append(findings, repoFindings...) + + // want is computed again here, rather than threaded out of + // CheckRepo, so every repository, matched or not, feeds + // CheckAncestorCaseConflicts below: a repository already sitting + // at the path its own remote implies carries no finding of its + // own, yet is exactly the one a careless ancestor rename would + // un-place, so it still has to be in this list. + want, err := wantPath(ctx, cfg, path) + if err != nil { + continue + } + repoPaths = append(repoPaths, doctor.RepoPath{Repo: name, Path: path, Want: want}) + } + + if conflicts := doctor.CheckAncestorCaseConflicts(repoPaths); len(conflicts) > 0 { + contested := make([]string, 0, len(conflicts)) + for _, c := range conflicts { + contested = append(contested, c.Path) + } + findings = filterContestedPathCase(findings, contested) + findings = append(findings, conflicts...) } managedFindings, err := doctor.CheckManagedFiles(cfg, home) @@ -153,7 +188,8 @@ func collectFindings(ctx context.Context, cfg config.Config, home string, logger if err != nil && !errors.Is(err, os.ErrNotExist) { return nil, fmt.Errorf("read %s: %w", globalPath, err) } - globalFindings, err := doctor.CheckGlobal(globalContent, globalPath, defaultMovedProfileName) + _, movedProfileExists := cfg.Profiles[defaultMovedProfileName] + globalFindings, err := doctor.CheckGlobal(globalContent, globalPath, defaultMovedProfileName, movedProfileExists) if err != nil { return nil, err } @@ -195,15 +231,17 @@ func applyFixes( case (f.Kind == doctor.KindManagedFileMissing || f.Kind == doctor.KindManagedFileStale) && managedWritten: fixed = append(fixed, f) case f.Kind == doctor.KindPathCase && dryRun: - fmt.Fprintf(stdout, "would rename %s to %s\n", f.Path, f.WantPath) + oldAncestor, newAncestor := doctor.FirstDifferingSegment(f.Path, f.WantPath) + fmt.Fprintf(stdout, "would rename %s to %s\n", oldAncestor, newAncestor) outstanding = append(outstanding, f) case f.Kind == doctor.KindPathCase: - if err := renameCaseOnly(f.Path, f.WantPath); err != nil { + oldAncestor, newAncestor, err := renameCaseOnly(f.Path, f.WantPath) + if err != nil { fmt.Fprintf(stderr, "gitcd doctor: %v\n", err) outstanding = append(outstanding, f) continue } - fmt.Fprintf(stdout, "renamed %s to %s\n", f.Path, f.WantPath) + fmt.Fprintf(stdout, "renamed %s to %s\n", oldAncestor, newAncestor) fixed = append(fixed, f) default: outstanding = append(outstanding, f) @@ -229,60 +267,90 @@ func rerenderManagedFiles(cfg config.Config, home string, logger *slog.Logger, s // leaf directory, a mis-cased owner segment for instance: it renames the // topmost segment that actually differs, carrying everything below it // along in the same move, rather than only the leaf, which would leave -// a mis-cased ancestor untouched. The move goes through a sibling -// temporary name: a case insensitive but case preserving filesystem, the -// common case on macOS, treats a direct rename between two spellings of -// the same name as a no-op, unlike a case sensitive one, so a plain -// two-argument rename cannot be relied on to force the change on every -// filesystem alike. +// a mis-cased ancestor untouched, and returns that ancestor pair, the +// directories actually renamed, so a caller reports what really moved +// rather than the repository's own path. The move goes through a +// sibling temporary name: a case insensitive but case preserving +// filesystem, the common case on macOS, treats a direct rename between +// two spellings of the same name as a no-op, unlike a case sensitive +// one, so a plain two-argument rename cannot be relied on to force the +// change on every filesystem alike. // // When the topmost differing ancestor no longer exists at its old path // but already exists at the new one, it was already renamed by an // earlier call in the same "doctor --fix" run, fixing a sibling // repository under the same mis-cased ancestor: this call is then a // no-op rather than an error. -func renameCaseOnly(oldPath, newPath string) error { - oldAncestor, newAncestor := firstDifferingSegment(oldPath, newPath) +func renameCaseOnly(oldPath, newPath string) (oldAncestor, newAncestor string, err error) { + oldAncestor, newAncestor = doctor.FirstDifferingSegment(oldPath, newPath) if oldAncestor == newAncestor { - return nil + return oldAncestor, newAncestor, nil } - if _, err := os.Stat(oldAncestor); errors.Is(err, os.ErrNotExist) { - if _, err := os.Stat(newAncestor); err == nil { - return nil + if _, statErr := os.Stat(oldAncestor); errors.Is(statErr, os.ErrNotExist) { + if _, statErr := os.Stat(newAncestor); statErr == nil { + return oldAncestor, newAncestor, nil } } tmp := oldAncestor + ".gitcd-doctor-rename-tmp" if err := os.Rename(oldAncestor, tmp); err != nil { - return fmt.Errorf("rename %s to a temporary name: %w", oldAncestor, err) + return oldAncestor, newAncestor, fmt.Errorf("rename %s to a temporary name: %w", oldAncestor, err) } if err := os.Rename(tmp, newAncestor); err != nil { // Best effort restore, so a failed second hop does not leave the // repository sitting at an unexpected temporary name. _ = os.Rename(tmp, oldAncestor) - return fmt.Errorf("rename %s to %s: %w", oldAncestor, newAncestor, err) + return oldAncestor, newAncestor, fmt.Errorf("rename %s to %s: %w", oldAncestor, newAncestor, err) } - return nil + return oldAncestor, newAncestor, nil } -// firstDifferingSegment walks oldPath and newPath component by -// component and returns the path of the first (topmost) component at -// which they differ, in each of the two inputs, so a caller can rename -// exactly that directory and carry everything below it along in one -// move. Both return values equal oldPath and newPath themselves when -// the two share no common prefix at all. -func firstDifferingSegment(oldPath, newPath string) (string, string) { - oldSegs := strings.Split(filepath.Clean(oldPath), string(filepath.Separator)) - newSegs := strings.Split(filepath.Clean(newPath), string(filepath.Separator)) +// wantPath renders the path the configured layout would place path's own +// repository at, read from its remote directly rather than through +// doctor.CheckRepo's own profile.Inspect, which this package's +// CheckAncestorCaseConflicts call does not need the fuller identity +// state Inspect also resolves for. +func wantPath(ctx context.Context, cfg config.Config, path string) (string, error) { + remoteURL, err := git.RemoteURL(ctx, path) + if err != nil { + return "", fmt.Errorf("read the origin remote of %s: %w", path, err) + } + r, err := remote.Parse(remoteURL, cfg.AliasMap()) + if err != nil { + return "", err + } + rel, err := layout.Render(cfg.LayoutFor(r.Host), r) + if err != nil { + return "", err + } + return filepath.Join(cfg.Root, rel), nil +} - i := 0 - for i < len(oldSegs) && i < len(newSegs) && oldSegs[i] == newSegs[i] { - i++ +// filterContestedPathCase drops every KindPathCase finding in findings +// whose own path sits under one of contested, the ancestor directories +// CheckAncestorCaseConflicts found two or more repositories disagreeing +// about: a KindAncestorCaseConflict finding replaces it instead, so +// "doctor --fix" never attempts the rename a KindPathCase finding would +// otherwise have it try. +func filterContestedPathCase(findings []doctor.Finding, contested []string) []doctor.Finding { + kept := make([]doctor.Finding, 0, len(findings)) + for _, f := range findings { + if f.Kind == doctor.KindPathCase && underAny(f.Path, contested) { + continue + } + kept = append(kept, f) } - if i >= len(oldSegs) || i >= len(newSegs) { - return oldPath, newPath + return kept +} + +// underAny reports whether path equals, or sits under, any of ancestors. +func underAny(path string, ancestors []string) bool { + for _, a := range ancestors { + if path == a || strings.HasPrefix(path, a+string(filepath.Separator)) { + return true + } } - return strings.Join(oldSegs[:i+1], string(filepath.Separator)), strings.Join(newSegs[:i+1], string(filepath.Separator)) + return false } // writeDoctorReport renders doctor's text report: both configuration diff --git a/cmd/gitcd/doctor_test.go b/cmd/gitcd/doctor_test.go index f666a58..620478c 100644 --- a/cmd/gitcd/doctor_test.go +++ b/cmd/gitcd/doctor_test.go @@ -6,6 +6,7 @@ package main import ( "bytes" "encoding/json" + "fmt" "os" "path/filepath" "strings" @@ -160,6 +161,57 @@ func TestRunDoctor_JSONOutput(t *testing.T) { } } +func TestRunDoctor_JSONFixOutputIsValidJSON(t *testing.T) { + home := setupEnv(t) + writeFile(t, filepath.Join(home, ".gitcd.yaml"), ` +profiles: + work: + user.email: acme-work-identity +rules: + - host: github.com + profile: work +`) + + var stdout, stderr bytes.Buffer + run([]string{"doctor", "--json", "--fix"}, &stdout, &stderr) + + var out struct { + ConfigPath string `json:"configPath"` + Fixed []map[string]any `json:"fixed"` + } + if err := json.Unmarshal(stdout.Bytes(), &out); err != nil { + t.Fatalf("doctor --json --fix produced invalid JSON on stdout: %v, output: %s", err, stdout.String()) + } + if out.ConfigPath != filepath.Join(home, ".gitcd.yaml") { + t.Fatalf("doctor --json --fix configPath = %q, want %q", out.ConfigPath, filepath.Join(home, ".gitcd.yaml")) + } + if len(out.Fixed) == 0 { + t.Fatal("doctor --json --fix fixed is empty, want the managed file fix reported") + } +} + +func TestRunDoctor_JSONFixDryRunOutputIsValidJSON(t *testing.T) { + home := setupEnv(t) + writeFile(t, filepath.Join(home, ".gitcd.yaml"), ` +profiles: + work: + user.email: acme-work-identity +rules: + - host: github.com + profile: work +`) + + var stdout, stderr bytes.Buffer + run([]string{"doctor", "--json", "--fix", "--dry-run"}, &stdout, &stderr) + + var out struct { + ConfigPath string `json:"configPath"` + } + if err := json.Unmarshal(stdout.Bytes(), &out); err != nil { + t.Fatalf("doctor --json --fix --dry-run produced invalid JSON on stdout: %v, output: %s", err, stdout.String()) + } +} + func TestRunDoctor_FixRenamesACaseOnlyPathMismatch(t *testing.T) { home := setupEnv(t) root := filepath.Join(home, "Code") @@ -212,3 +264,112 @@ func containsExact(names []string, want string) bool { } return false } + +func TestRunDoctor_FixRenameMessageNamesTheAncestorNotTheRepoPath(t *testing.T) { + home := setupEnv(t) + root := filepath.Join(home, "Code") + writeFile(t, filepath.Join(home, ".gitcd.yaml"), "root: "+root+"\n") + + // Two repositories share one mis-cased owner ancestor, and both + // remotes agree it should read lowercase: a legitimate, uncontested + // rename of the shared ancestor, not the per-repository leaf path. + widget := filepath.Join(root, "github.com", "ACME", "widget") + initRepository(t, widget) + runGit(t, widget, "remote", "add", "origin", "https://github.com/acme/widget.git") + gadget := filepath.Join(root, "github.com", "ACME", "gadget") + initRepository(t, gadget) + runGit(t, gadget, "remote", "add", "origin", "https://github.com/acme/gadget.git") + + var stdout, stderr bytes.Buffer + run([]string{"doctor", "--fix"}, &stdout, &stderr) + + oldAncestor := filepath.Join(root, "github.com", "ACME") + newAncestor := filepath.Join(root, "github.com", "acme") + wantLine := fmt.Sprintf("renamed %s to %s", oldAncestor, newAncestor) + if !strings.Contains(stdout.String(), wantLine) { + t.Fatalf("doctor --fix stdout = %q, want a line naming the ancestor pair %q", stdout.String(), wantLine) + } + badLine := fmt.Sprintf("renamed %s to %s", widget, filepath.Join(root, "github.com", "acme", "widget")) + if strings.Contains(stdout.String(), badLine) { + t.Fatalf("doctor --fix stdout = %q, want it to never name the repository's own leaf path as what moved", stdout.String()) + } +} + +func TestRunDoctor_FixRefusesAContestedAncestorAndConverges(t *testing.T) { + home := setupEnv(t) + root := filepath.Join(home, "Code") + writeFile(t, filepath.Join(home, ".gitcd.yaml"), "root: "+root+"\n") + + // Both repositories physically share one ancestor directory, spelled + // "Acme" on disk. "widget"'s own remote wants it lowercase, so it is + // mismatched; "other"'s remote wants exactly "Acme", so it already + // matches and carries no finding of its own. Renaming the ancestor to + // fix "widget" would immediately un-place "other". + widget := filepath.Join(root, "github.com", "Acme", "widget") + initRepository(t, widget) + runGit(t, widget, "remote", "add", "origin", "https://github.com/acme/widget.git") + other := filepath.Join(root, "github.com", "Acme", "other") + initRepository(t, other) + runGit(t, other, "remote", "add", "origin", "https://github.com/Acme/other.git") + + readEntries := func() []string { + entries, err := os.ReadDir(filepath.Join(root, "github.com")) + if err != nil { + t.Fatalf("read %s: %v", filepath.Join(root, "github.com"), err) + } + var names []string + for _, e := range entries { + names = append(names, e.Name()) + } + return names + } + before := readEntries() + + var stdout1, stderr1 bytes.Buffer + run([]string{"doctor", "--fix"}, &stdout1, &stderr1) + if strings.Contains(stdout1.String(), "renamed") { + t.Fatalf("doctor --fix (run 1) stdout = %q, want no rename of a contested ancestor", stdout1.String()) + } + if !strings.Contains(stdout1.String(), "ancestor-case-conflict") { + t.Fatalf("doctor --fix (run 1) stdout = %q, want an ancestor-case-conflict finding", stdout1.String()) + } + afterRun1 := readEntries() + if !equalStringSets(before, afterRun1) { + t.Fatalf("doctor --fix (run 1) changed the directory entries under github.com: before %v, after %v", before, afterRun1) + } + + var stdout2, stderr2 bytes.Buffer + run([]string{"doctor", "--fix"}, &stdout2, &stderr2) + if strings.Contains(stdout2.String(), "renamed") { + t.Fatalf("doctor --fix (run 2) stdout = %q, want no rename of a contested ancestor", stdout2.String()) + } + if !strings.Contains(stdout2.String(), "ancestor-case-conflict") { + t.Fatalf("doctor --fix (run 2) stdout = %q, want the conflict reported again", stdout2.String()) + } + afterRun2 := readEntries() + if !equalStringSets(afterRun1, afterRun2) { + t.Fatalf( + "doctor --fix (run 2) is not a no-op: directory entries under github.com changed, run 1 %v, run 2 %v", + afterRun1, afterRun2, + ) + } +} + +func equalStringSets(a, b []string) bool { + if len(a) != len(b) { + return false + } + counts := map[string]int{} + for _, v := range a { + counts[v]++ + } + for _, v := range b { + counts[v]-- + } + for _, c := range counts { + if c != 0 { + return false + } + } + return true +}