From 596cc295f7444c3488eb5cc11b25c2068470a7ad Mon Sep 17 00:00:00 2001 From: cds-amal Date: Fri, 13 Feb 2026 22:37:00 -0500 Subject: [PATCH 01/16] fix(e2e): use explicit default branch in test setup NewTestEnv used bare `git init` which inherits the user's global init.defaultBranch setting. Every E2E test hardcodes "main", so the suite fails on systems defaulting to "master". Pass `-b main` to make it deterministic. --- e2e/helpers_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e/helpers_test.go b/e2e/helpers_test.go index 7930c79..723ff69 100644 --- a/e2e/helpers_test.go +++ b/e2e/helpers_test.go @@ -53,7 +53,7 @@ func NewTestEnv(t *testing.T) *TestEnv { BinaryPath: binaryPath, } - env.Git("init") + env.Git("init", "-b", "main") env.Git("config", "user.email", "test@example.com") env.Git("config", "user.name", "Test User") // Prevent git from opening an editor (for commits, rebases, etc.) From d4f0da917ce943ece609808d55fb058e06e741a9 Mon Sep 17 00:00:00 2001 From: cds-amal Date: Fri, 13 Feb 2026 22:37:17 -0500 Subject: [PATCH 02/16] feat(git): add IsAncestor and GetForkPoint methods IsAncestor wraps `git merge-base --is-ancestor`. GetForkPoint wraps `git merge-base --fork-point` for reflog-aware divergence detection; callers fall back to GetMergeBase when reflogs have expired. --- internal/git/git.go | 19 ++++++++ internal/git/git_test.go | 96 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+) diff --git a/internal/git/git.go b/internal/git/git.go index c71232e..bde80bc 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -525,6 +525,25 @@ func (g *Git) GetCommits(base, head string) ([]Commit, error) { return commits, nil } +// IsAncestor returns true if ancestor is an ancestor of descendant. +// Both arguments must be valid commit references. +func (g *Git) IsAncestor(ancestor, descendant string) (bool, error) { + err := g.runSilent("merge-base", "--is-ancestor", ancestor, descendant) + if err != nil { + // Non-zero exit means not an ancestor (assuming both commits exist). + return false, nil + } + return true, nil +} + +// GetForkPoint returns the reflog-aware fork point of branch from parent. +// This uses git merge-base --fork-point which inspects the reflog to find +// where the branch originally diverged. Returns an error when reflogs have +// expired (exit code 1), in which case the caller should fall back to GetMergeBase. +func (g *Git) GetForkPoint(parent, branch string) (string, error) { + return g.run("merge-base", "--fork-point", parent, branch) +} + // AbbrevSHA safely abbreviates a SHA to 7 characters. // Returns the full string if it's shorter than 7 characters. func AbbrevSHA(sha string) string { diff --git a/internal/git/git_test.go b/internal/git/git_test.go index 5fc45a6..013e43b 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -709,6 +709,102 @@ func TestIsRebaseInProgressWorktree(t *testing.T) { } } +// contains checks if substr is in s (simple helper to avoid importing strings in test). +func contains(s, substr string) bool { + return filepath.Base(s) == substr || len(s) >= len(substr) && searchString(s, substr) +} + +func searchString(s, substr string) bool { + for i := 0; i <= len(s)-len(substr); i++ { + if s[i:i+len(substr)] == substr { + return true + } + } + return false +} + +func TestIsAncestor(t *testing.T) { + dir := setupTestRepo(t) + g := git.New(dir) + + // Record the initial commit SHA + initial, _ := g.GetTip("HEAD") + + // Create a linear chain: initial -> A -> B + os.WriteFile(filepath.Join(dir, "a.txt"), []byte("a"), 0644) + exec.Command("git", "-C", dir, "add", ".").Run() + exec.Command("git", "-C", dir, "commit", "-m", "commit A").Run() + shaA, _ := g.GetTip("HEAD") + + os.WriteFile(filepath.Join(dir, "b.txt"), []byte("b"), 0644) + exec.Command("git", "-C", dir, "add", ".").Run() + exec.Command("git", "-C", dir, "commit", "-m", "commit B").Run() + shaB, _ := g.GetTip("HEAD") + + // initial is ancestor of B + isAnc, err := g.IsAncestor(initial, shaB) + if err != nil { + t.Fatalf("IsAncestor failed: %v", err) + } + if !isAnc { + t.Error("expected initial to be ancestor of B") + } + + // A is ancestor of B + isAnc, err = g.IsAncestor(shaA, shaB) + if err != nil { + t.Fatalf("IsAncestor failed: %v", err) + } + if !isAnc { + t.Error("expected A to be ancestor of B") + } + + // B is NOT ancestor of A + isAnc, err = g.IsAncestor(shaB, shaA) + if err != nil { + t.Fatalf("IsAncestor failed: %v", err) + } + if isAnc { + t.Error("expected B to NOT be ancestor of A") + } + + // A commit is always its own ancestor + isAnc, err = g.IsAncestor(shaA, shaA) + if err != nil { + t.Fatalf("IsAncestor failed: %v", err) + } + if !isAnc { + t.Error("expected A to be ancestor of itself") + } +} + +func TestGetForkPoint(t *testing.T) { + dir := setupTestRepo(t) + g := git.New(dir) + + trunk, _ := g.CurrentBranch() + + // Record the divergence point + diverge, _ := g.GetTip(trunk) + + // Create child branch from trunk + g.CreateAndCheckout("child") + + // Add commits on child + os.WriteFile(filepath.Join(dir, "child.txt"), []byte("child"), 0644) + exec.Command("git", "-C", dir, "add", ".").Run() + exec.Command("git", "-C", dir, "commit", "-m", "child commit").Run() + + // GetForkPoint should return the divergence point + fp, err := g.GetForkPoint(trunk, "child") + if err != nil { + t.Fatalf("GetForkPoint failed: %v", err) + } + if fp != diverge { + t.Errorf("expected fork point %s, got %s", diverge, fp) + } +} + func TestRemoteBranchExists(t *testing.T) { // Create main repo dir := setupTestRepo(t) From 04436c03a3dd0e4bc359ea5cc5efd63031a07b80 Mon Sep 17 00:00:00 2001 From: cds-amal Date: Fri, 13 Feb 2026 22:37:28 -0500 Subject: [PATCH 03/16] feat: add `gh stack doctor` command Diagnoses and optionally repairs stale fork points caused by manual rebases or resets. Walks tracked branches directly (skipping tree.Build so missing-parent cases are caught) and checks six conditions from fatal to cosmetic. --fix updates the stored fork point and writes a provenance comment into .git/config preserving the old value. --- cmd/doctor.go | 285 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 285 insertions(+) create mode 100644 cmd/doctor.go diff --git a/cmd/doctor.go b/cmd/doctor.go new file mode 100644 index 0000000..6e0dc60 --- /dev/null +++ b/cmd/doctor.go @@ -0,0 +1,285 @@ +// cmd/doctor.go +package cmd + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "time" + + "github.com/boneskull/gh-stack/internal/config" + "github.com/boneskull/gh-stack/internal/git" + "github.com/boneskull/gh-stack/internal/style" + "github.com/spf13/cobra" +) + +var doctorCmd = &cobra.Command{ + Use: "doctor", + Short: "Check stack health and detect stale fork points", + Long: `Diagnose (and optionally repair) stale fork points caused by manual rebases or resets.`, + RunE: runDoctor, +} + +var doctorFixFlag bool + +func init() { + doctorCmd.Flags().BoolVar(&doctorFixFlag, "fix", false, "repair detected issues") + rootCmd.AddCommand(doctorCmd) +} + +// branchResult tracks the outcome of checking a single branch. +type branchResult struct { + name string + healthy bool + issues []string + fixed bool + fixMsg string +} + +func runDoctor(cmd *cobra.Command, args []string) error { + cwd, err := os.Getwd() + if err != nil { + return err + } + + cfg, err := config.Load(cwd) + if err != nil { + return err + } + + if _, err := cfg.GetTrunk(); err != nil { + return err + } + + g := git.New(cwd) + s := style.New() + + branches, err := cfg.ListTrackedBranches() + if err != nil { + return err + } + + if len(branches) == 0 { + fmt.Println("No tracked branches found.") + return nil + } + + fmt.Println(s.Bold("Stack Health Report")) + fmt.Println() + + var healthy, issues int + for _, branch := range branches { + result := checkBranch(g, cfg, s, branch, doctorFixFlag) + if result.healthy { + fmt.Printf("%s %s %s\n", s.SuccessIcon(), s.Branch(result.name), s.Muted("(healthy)")) + healthy++ + } else if result.fixed { + fmt.Printf("%s %s: %s\n", s.SuccessIcon(), s.Branch(result.name), result.fixMsg) + healthy++ + } else { + fmt.Printf("%s %s\n", s.FailureIcon(), s.Branch(result.name)) + for _, issue := range result.issues { + fmt.Printf(" %s\n", issue) + } + issues++ + } + } + + fmt.Println() + if issues > 0 { + noun := "issue" + if issues > 1 { + noun = "issues" + } + fmt.Printf("%d %s found.", issues, noun) + if !doctorFixFlag { + fmt.Printf(" Run 'gh stack doctor --fix' to repair.") + } + fmt.Println() + } else { + fmt.Println(s.SuccessMessage("All branches healthy.")) + } + + return nil +} + +func checkBranch(g *git.Git, cfg *config.Config, s *style.Style, branch string, fix bool) branchResult { + result := branchResult{name: branch} + + // Check 1: branch exists locally + if !g.BranchExists(branch) { + result.issues = append(result.issues, s.Error("Branch does not exist locally")) + return result + } + + // Check 2: parent exists locally + parent, err := cfg.GetParent(branch) + if err != nil { + result.issues = append(result.issues, s.Error("No parent configured")) + return result + } + + trunk, _ := cfg.GetTrunk() //nolint:errcheck // already validated in runDoctor + if parent != trunk && !g.BranchExists(parent) { + result.issues = append(result.issues, s.Errorf("Parent branch %s does not exist locally", parent)) + return result + } + + // Check 3: fork point is stored + storedFP, err := cfg.GetForkPoint(branch) + if err != nil { + if fix { + newFP, fixErr := computeForkPoint(g, parent, branch) + if fixErr != nil { + result.issues = append(result.issues, fmt.Sprintf("No fork point stored and could not compute one: %v", fixErr)) + return result + } + if setErr := cfg.SetForkPoint(branch, newFP); setErr != nil { + result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", setErr)) + return result + } + result.fixed = true + result.fixMsg = fmt.Sprintf("set fork point to %s", git.AbbrevSHA(newFP)) + return result + } + result.issues = append(result.issues, "No fork point stored") + return result + } + + // Check 4: fork point commit exists + if !g.CommitExists(storedFP) { + if fix { + newFP, fixErr := computeForkPoint(g, parent, branch) + if fixErr != nil { + result.issues = append(result.issues, fmt.Sprintf("Stored fork point %s does not exist and could not compute replacement: %v", git.AbbrevSHA(storedFP), fixErr)) + return result + } + if setErr := setForkPointWithComment(g, cfg, branch, storedFP, newFP); setErr != nil { + result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", setErr)) + return result + } + result.fixed = true + result.fixMsg = fmt.Sprintf("updated fork point %s \u2192 %s", git.AbbrevSHA(storedFP), git.AbbrevSHA(newFP)) + return result + } + result.issues = append(result.issues, fmt.Sprintf("Stored fork point %s does not exist", git.AbbrevSHA(storedFP))) + return result + } + + // Check 5: fork point is ancestor of branch + isAnc, _ := g.IsAncestor(storedFP, branch) + if !isAnc { + if fix { + newFP, fixErr := computeForkPoint(g, parent, branch) + if fixErr != nil { + result.issues = append(result.issues, fmt.Sprintf("Stored fork point %s is not an ancestor of %s and could not compute replacement: %v", git.AbbrevSHA(storedFP), branch, fixErr)) + return result + } + if setErr := setForkPointWithComment(g, cfg, branch, storedFP, newFP); setErr != nil { + result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", setErr)) + return result + } + result.fixed = true + result.fixMsg = fmt.Sprintf("updated fork point %s \u2192 %s", git.AbbrevSHA(storedFP), git.AbbrevSHA(newFP)) + return result + } + result.issues = append(result.issues, fmt.Sprintf("Stored fork point %s is not an ancestor of %s", git.AbbrevSHA(storedFP), branch)) + return result + } + + // Check 6: drift detection + mergeBase, err := g.GetMergeBase(parent, branch) + if err != nil { + // Can't compute merge-base; skip drift check + result.healthy = true + return result + } + + if storedFP != mergeBase { + if fix { + if setErr := setForkPointWithComment(g, cfg, branch, storedFP, mergeBase); setErr != nil { + result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", setErr)) + return result + } + result.fixed = true + result.fixMsg = fmt.Sprintf("updated fork point %s \u2192 %s", git.AbbrevSHA(storedFP), git.AbbrevSHA(mergeBase)) + return result + } + result.issues = append(result.issues, + fmt.Sprintf("Stored parent: %s", parent), + fmt.Sprintf("Stored fork: %s", git.AbbrevSHA(storedFP)), + fmt.Sprintf("Actual fork: %s", git.AbbrevSHA(mergeBase)), + "Drift detected: stored fork point does not match merge-base", + ) + return result + } + + result.healthy = true + return result +} + +// computeForkPoint tries the reflog-aware fork-point first, falling back to merge-base. +func computeForkPoint(g *git.Git, parent, branch string) (string, error) { + fp, err := g.GetForkPoint(parent, branch) + if err == nil { + return fp, nil + } + return g.GetMergeBase(parent, branch) +} + +// setForkPointWithComment updates the fork point and inserts a comment preserving the old value. +// The comment is best-effort; if it fails, the fork point is still updated. +func setForkPointWithComment(g *git.Git, cfg *config.Config, branch, oldSHA, newSHA string) error { + if err := cfg.SetForkPoint(branch, newSHA); err != nil { + return err + } + commentForkPointChange(g.GetGitDir(), branch, oldSHA) + return nil +} + +// commentForkPointChange inserts a comment above the stackForkPoint line in .git/config +// recording the previous value and a timestamp. This preserves provenance so old fork +// points can be recovered if a fix goes wrong. Best-effort: errors are silently ignored. +func commentForkPointChange(gitDir, branch, oldSHA string) { + configPath := filepath.Join(gitDir, "config") + data, err := os.ReadFile(configPath) + if err != nil { + return + } + + lines := strings.Split(string(data), "\n") + sectionHeader := fmt.Sprintf("[branch %q]", branch) + inSection := false + var result []string + + for _, line := range lines { + trimmed := strings.TrimSpace(line) + + if trimmed == sectionHeader { + inSection = true + result = append(result, line) + continue + } + + // New section starts; we've left the target section + if inSection && strings.HasPrefix(trimmed, "[") { + inSection = false + } + + // Insert comment before the stackForkPoint line + if inSection && strings.HasPrefix(strings.ToLower(trimmed), "stackforkpoint") { + indent := line[:len(line)-len(strings.TrimLeft(line, " \t"))] + timestamp := time.Now().UTC().Format(time.RFC3339) + result = append(result, + fmt.Sprintf("%s# doctor fix %s replaces previous value of:", indent, timestamp), + fmt.Sprintf("%s# %s", indent, oldSHA), + ) + } + + result = append(result, line) + } + + //nolint:errcheck // best-effort provenance + _ = os.WriteFile(configPath, []byte(strings.Join(result, "\n")), 0644) +} From 8fa48705149ffb27fb4a6f134a0d299cb499809d Mon Sep 17 00:00:00 2001 From: cds-amal Date: Fri, 13 Feb 2026 22:37:40 -0500 Subject: [PATCH 04/16] test(e2e): add doctor command tests Covers healthy stack, drift detection + fix, missing branch, missing parent, config update verification with provenance comment check, and not-initialized error. --- e2e/doctor_test.go | 163 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 163 insertions(+) create mode 100644 e2e/doctor_test.go diff --git a/e2e/doctor_test.go b/e2e/doctor_test.go new file mode 100644 index 0000000..c9699c5 --- /dev/null +++ b/e2e/doctor_test.go @@ -0,0 +1,163 @@ +// e2e/doctor_test.go +package e2e_test + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestDoctorHealthyStack(t *testing.T) { + env := NewTestEnv(t) + + env.MustRun("init") + env.MustRun("create", "feature-a") + env.CreateCommit("work on a") + env.MustRun("create", "feature-b") + env.CreateCommit("work on b") + + result := env.MustRun("doctor") + if !result.ContainsStdout("healthy") { + t.Errorf("expected healthy output, got: %s", result.Stdout) + } + if !result.ContainsStdout("All branches healthy") { + t.Errorf("expected 'All branches healthy', got: %s", result.Stdout) + } +} + +func TestDoctorDetectsDrift(t *testing.T) { + env := NewTestEnv(t) + + env.MustRun("init") + env.MustRun("create", "feature-a") + env.CreateCommit("work on a") + env.MustRun("create", "feature-b") + env.CreateCommit("work on b") + + // Move main forward and rebase feature-a to cause stored fork point drift + env.Git("checkout", "feature-a") + env.CreateCommit("extra work on a") + env.Git("checkout", "main") + env.CreateCommit("main moves forward") + env.Git("checkout", "feature-a") + env.Git("rebase", "main") + + // Now the stored fork point is stale + result := env.Run("doctor") + if !result.ContainsStdout("Drift detected") && !result.ContainsStdout("not an ancestor") { + t.Errorf("expected drift or ancestor issue, got: %s", result.Stdout) + } + + // Fix it + result = env.MustRun("doctor", "--fix") + if !result.ContainsStdout("updated fork point") && !result.ContainsStdout("set fork point") { + t.Errorf("expected fix message, got: %s", result.Stdout) + } + + // Should be healthy now + result = env.MustRun("doctor") + if !result.ContainsStdout("All branches healthy") { + t.Errorf("expected all healthy after fix, got: %s", result.Stdout) + } +} + +func TestDoctorMissingBranch(t *testing.T) { + env := NewTestEnv(t) + + env.MustRun("init") + env.MustRun("create", "feature-a") + env.CreateCommit("work on a") + + // Remove the branch ref but keep the stack config entries + env.Git("checkout", "main") + env.Git("update-ref", "-d", "refs/heads/feature-a") + + result := env.Run("doctor") + if !result.ContainsStdout("does not exist locally") { + t.Errorf("expected 'does not exist locally', got: %s", result.Stdout) + } +} + +func TestDoctorMissingParent(t *testing.T) { + env := NewTestEnv(t) + + env.MustRun("init") + env.MustRun("create", "feature-a") + env.CreateCommit("work on a") + env.MustRun("create", "feature-b") + env.CreateCommit("work on b") + + // Delete the parent branch (feature-a) + env.Git("checkout", "main") + env.Git("branch", "-D", "feature-a") + + result := env.Run("doctor") + // feature-b should report its parent missing + if !result.ContainsStdout("does not exist locally") { + t.Errorf("expected parent missing report, got: %s", result.Stdout) + } +} + +func TestDoctorFixUpdatesConfig(t *testing.T) { + env := NewTestEnv(t) + + env.MustRun("init") + env.MustRun("create", "feature-a") + env.CreateCommit("work on a") + + // Record the original fork point + originalFP := strings.TrimSpace(env.Git("config", "--get", "branch.feature-a.stackForkPoint")) + + // Move main forward and rebase feature-a to cause drift + env.Git("checkout", "main") + env.CreateCommit("main moves") + env.Git("checkout", "feature-a") + env.Git("rebase", "main") + + // The stored fork point should still be the old one + storedFP := strings.TrimSpace(env.Git("config", "--get", "branch.feature-a.stackForkPoint")) + if storedFP != originalFP { + t.Fatalf("expected stored fork point to still be %s, got %s", originalFP, storedFP) + } + + // Run doctor --fix + env.MustRun("doctor", "--fix") + + // Read the config directly to verify it changed + newFP := strings.TrimSpace(env.Git("config", "--get", "branch.feature-a.stackForkPoint")) + if newFP == originalFP { + t.Errorf("expected fork point to be updated, but it's still %s", originalFP) + } + + // Verify the new fork point matches the current merge-base + mergeBase := strings.TrimSpace(env.Git("merge-base", "main", "feature-a")) + if newFP != mergeBase { + t.Errorf("expected new fork point %s to match merge-base %s", newFP, mergeBase) + } + + // Verify provenance comment was written to the config file + configData, err := os.ReadFile(filepath.Join(env.WorkDir, ".git", "config")) + if err != nil { + t.Fatalf("failed to read .git/config: %v", err) + } + configStr := string(configData) + if !strings.Contains(configStr, "# doctor fix") { + t.Error("expected provenance comment '# doctor fix ...' in .git/config") + } + if !strings.Contains(configStr, "# "+originalFP) { + t.Errorf("expected old fork point %s preserved in config comment", originalFP) + } +} + +func TestDoctorNotInitialized(t *testing.T) { + env := NewTestEnv(t) + + result := env.Run("doctor") + if result.Success() { + t.Error("expected doctor to fail before init") + } + if !result.ContainsStderr("not initialized") { + t.Errorf("expected 'not initialized' error, got stderr: %s", result.Stderr) + } +} From 3e9f97a56684fc66925cdd9f7ce129afdcf36d88 Mon Sep 17 00:00:00 2001 From: cds-amal Date: Sat, 14 Feb 2026 14:07:13 -0500 Subject: [PATCH 05/16] fix(git): distinguish exit code 1 from errors in IsAncestor IsAncestor was treating all non-zero exits from `git merge-base --is-ancestor` as "not ancestor". Exit code 1 means "not ancestor", but other codes (e.g., 128) indicate real failures like invalid refs or repo corruption. Now only exit code 1 returns (false, nil); other failures propagate as errors. --- internal/git/git.go | 17 +++++++++++++---- internal/git/git_test.go | 11 +++++++++++ 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/internal/git/git.go b/internal/git/git.go index bde80bc..c66656b 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -529,11 +529,20 @@ func (g *Git) GetCommits(base, head string) ([]Commit, error) { // Both arguments must be valid commit references. func (g *Git) IsAncestor(ancestor, descendant string) (bool, error) { err := g.runSilent("merge-base", "--is-ancestor", ancestor, descendant) - if err != nil { - // Non-zero exit means not an ancestor (assuming both commits exist). - return false, nil + if err == nil { + return true, nil } - return true, nil + + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + // Exit status 1 means "not ancestor" when both commits exist. + if exitErr.ExitCode() == 1 { + return false, nil + } + } + + // Propagate other failures (e.g., invalid refs, repo errors). + return false, err } // GetForkPoint returns the reflog-aware fork point of branch from parent. diff --git a/internal/git/git_test.go b/internal/git/git_test.go index 013e43b..f5e5d02 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -776,6 +776,17 @@ func TestIsAncestor(t *testing.T) { if !isAnc { t.Error("expected A to be ancestor of itself") } + + // Invalid ref should return an error, not (false, nil) + _, err = g.IsAncestor("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef", shaA) + if err == nil { + t.Error("expected error for invalid ancestor ref, got nil") + } + + _, err = g.IsAncestor(shaA, "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef") + if err == nil { + t.Error("expected error for invalid descendant ref, got nil") + } } func TestGetForkPoint(t *testing.T) { From 62bd7f10530289e5fc3f63cefb24f92d4ddba0f0 Mon Sep 17 00:00:00 2001 From: cds-amal Date: Sat, 14 Feb 2026 13:58:46 -0500 Subject: [PATCH 06/16] docs(git): fix misleading GetForkPoint docstring The comment claimed the error was specifically about expired reflogs (exit code 1), but the wrapper doesn't inspect exit codes; it just returns whatever error g.run() gives back. --- internal/git/git.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/git/git.go b/internal/git/git.go index c66656b..25b4681 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -546,9 +546,9 @@ func (g *Git) IsAncestor(ancestor, descendant string) (bool, error) { } // GetForkPoint returns the reflog-aware fork point of branch from parent. -// This uses git merge-base --fork-point which inspects the reflog to find -// where the branch originally diverged. Returns an error when reflogs have -// expired (exit code 1), in which case the caller should fall back to GetMergeBase. +// This uses `git merge-base --fork-point`, which inspects the reflog to find +// where the branch originally diverged. If the underlying Git command fails +// for any reason, an error is returned and callers may fall back to GetMergeBase. func (g *Git) GetForkPoint(parent, branch string) (string, error) { return g.run("merge-base", "--fork-point", parent, branch) } From 5997c336823c8e501a65066a82f17641b740d63e Mon Sep 17 00:00:00 2001 From: cds-amal Date: Sat, 14 Feb 2026 13:51:02 -0500 Subject: [PATCH 07/16] fix(doctor): handle IsAncestor errors instead of discarding them If `git merge-base --is-ancestor` fails for reasons other than "not ancestor" (invalid refs, repo corruption), doctor now reports it as an issue instead of misinterpreting it as a fork-point problem that --fix might incorrectly "repair". --- cmd/doctor.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cmd/doctor.go b/cmd/doctor.go index 6e0dc60..41b5ddc 100644 --- a/cmd/doctor.go +++ b/cmd/doctor.go @@ -168,7 +168,11 @@ func checkBranch(g *git.Git, cfg *config.Config, s *style.Style, branch string, } // Check 5: fork point is ancestor of branch - isAnc, _ := g.IsAncestor(storedFP, branch) + isAnc, err := g.IsAncestor(storedFP, branch) + if err != nil { + result.issues = append(result.issues, fmt.Sprintf("Failed to check if stored fork point %s is an ancestor of %s: %v", git.AbbrevSHA(storedFP), branch, err)) + return result + } if !isAnc { if fix { newFP, fixErr := computeForkPoint(g, parent, branch) From 801ca3d10f5ee1da9a6cad487489cefd58e68fd1 Mon Sep 17 00:00:00 2001 From: cds-amal Date: Sat, 14 Feb 2026 14:02:02 -0500 Subject: [PATCH 08/16] fix(doctor): report GetMergeBase failures instead of marking healthy When GetMergeBase fails (unrelated histories, bad refs), doctor was silently marking the branch healthy and skipping drift detection. Now it reports the failure as an issue. --- cmd/doctor.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/doctor.go b/cmd/doctor.go index 41b5ddc..222b744 100644 --- a/cmd/doctor.go +++ b/cmd/doctor.go @@ -195,8 +195,8 @@ func checkBranch(g *git.Git, cfg *config.Config, s *style.Style, branch string, // Check 6: drift detection mergeBase, err := g.GetMergeBase(parent, branch) if err != nil { - // Can't compute merge-base; skip drift check - result.healthy = true + result.issues = append(result.issues, + fmt.Sprintf("Failed to compute merge-base for %s and %s: %v", parent, branch, err), return result } From 019376d991c13306d1b301af0468a159d12b4d4b Mon Sep 17 00:00:00 2001 From: cds-amal Date: Sat, 14 Feb 2026 14:06:10 -0500 Subject: [PATCH 09/16] fix(doctor): resolve config path for linked worktrees commentForkPointChange assumed .git/config at a hardcoded path, which breaks in linked worktrees where .git is a file. Now uses git rev-parse --git-path config via a new GetConfigPath method. --- cmd/doctor.go | 13 ++++++++----- internal/git/git.go | 7 +++++++ 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/cmd/doctor.go b/cmd/doctor.go index 222b744..05bef4a 100644 --- a/cmd/doctor.go +++ b/cmd/doctor.go @@ -4,7 +4,6 @@ package cmd import ( "fmt" "os" - "path/filepath" "strings" "time" @@ -197,6 +196,7 @@ func checkBranch(g *git.Git, cfg *config.Config, s *style.Style, branch string, if err != nil { result.issues = append(result.issues, fmt.Sprintf("Failed to compute merge-base for %s and %s: %v", parent, branch, err), + ) return result } @@ -238,15 +238,18 @@ func setForkPointWithComment(g *git.Git, cfg *config.Config, branch, oldSHA, new if err := cfg.SetForkPoint(branch, newSHA); err != nil { return err } - commentForkPointChange(g.GetGitDir(), branch, oldSHA) + commentForkPointChange(g, branch, oldSHA) return nil } -// commentForkPointChange inserts a comment above the stackForkPoint line in .git/config +// commentForkPointChange inserts a comment above the stackForkPoint line in the git config // recording the previous value and a timestamp. This preserves provenance so old fork // points can be recovered if a fix goes wrong. Best-effort: errors are silently ignored. -func commentForkPointChange(gitDir, branch, oldSHA string) { - configPath := filepath.Join(gitDir, "config") +func commentForkPointChange(g *git.Git, branch, oldSHA string) { + configPath, err := g.GetConfigPath() + if err != nil { + return + } data, err := os.ReadFile(configPath) if err != nil { return diff --git a/internal/git/git.go b/internal/git/git.go index 25b4681..eada670 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -363,6 +363,13 @@ func (g *Git) GetGitDir() string { return filepath.Join(g.repoPath, ".git") } +// GetConfigPath returns the path to the repo's git config file, +// resolved via `git rev-parse --git-path config` so it works +// correctly in linked worktrees and submodules. +func (g *Git) GetConfigPath() (string, error) { + return g.run("rev-parse", "--git-path", "config") +} + // Fetch fetches from origin. func (g *Git) Fetch() error { return g.runInteractive("fetch", "origin") From f4855a5e876568202a18306a73059d8c5eb55466 Mon Sep 17 00:00:00 2001 From: cds-amal Date: Sat, 14 Feb 2026 14:15:50 -0500 Subject: [PATCH 10/16] fix(doctor): preserve config file permissions and skip no-op writes commentForkPointChange was writing .git/config with hardcoded 0644, which could loosen permissions if the file was e.g. 0600. Now preserves the existing file mode via os.Stat. Also skips the write entirely when no provenance comment was inserted. --- cmd/doctor.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/cmd/doctor.go b/cmd/doctor.go index 05bef4a..30bb22c 100644 --- a/cmd/doctor.go +++ b/cmd/doctor.go @@ -250,6 +250,10 @@ func commentForkPointChange(g *git.Git, branch, oldSHA string) { if err != nil { return } + info, err := os.Stat(configPath) + if err != nil { + return + } data, err := os.ReadFile(configPath) if err != nil { return @@ -258,6 +262,7 @@ func commentForkPointChange(g *git.Git, branch, oldSHA string) { lines := strings.Split(string(data), "\n") sectionHeader := fmt.Sprintf("[branch %q]", branch) inSection := false + modified := false var result []string for _, line := range lines { @@ -282,11 +287,15 @@ func commentForkPointChange(g *git.Git, branch, oldSHA string) { fmt.Sprintf("%s# doctor fix %s replaces previous value of:", indent, timestamp), fmt.Sprintf("%s# %s", indent, oldSHA), ) + modified = true } result = append(result, line) } + if !modified { + return + } //nolint:errcheck // best-effort provenance - _ = os.WriteFile(configPath, []byte(strings.Join(result, "\n")), 0644) + _ = os.WriteFile(configPath, []byte(strings.Join(result, "\n")), info.Mode()) } From 16745d5e7501ab5bc49381c4e46a8e5c72e92ffa Mon Sep 17 00:00:00 2001 From: cds-amal Date: Sat, 14 Feb 2026 14:28:56 -0500 Subject: [PATCH 11/16] fix(doctor): detect missing trunk branch The parent existence check was skipping trunk, so a deleted or renamed trunk branch wouldn't be reported. Now checks g.BranchExists(trunk) when the parent is trunk. --- cmd/doctor.go | 7 ++++++- e2e/doctor_test.go | 16 ++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/cmd/doctor.go b/cmd/doctor.go index 30bb22c..f3059b1 100644 --- a/cmd/doctor.go +++ b/cmd/doctor.go @@ -120,7 +120,12 @@ func checkBranch(g *git.Git, cfg *config.Config, s *style.Style, branch string, } trunk, _ := cfg.GetTrunk() //nolint:errcheck // already validated in runDoctor - if parent != trunk && !g.BranchExists(parent) { + if parent == trunk { + if !g.BranchExists(trunk) { + result.issues = append(result.issues, s.Errorf("Trunk branch %s does not exist locally", trunk)) + return result + } + } else if !g.BranchExists(parent) { result.issues = append(result.issues, s.Errorf("Parent branch %s does not exist locally", parent)) return result } diff --git a/e2e/doctor_test.go b/e2e/doctor_test.go index c9699c5..625c918 100644 --- a/e2e/doctor_test.go +++ b/e2e/doctor_test.go @@ -150,6 +150,22 @@ func TestDoctorFixUpdatesConfig(t *testing.T) { } } +func TestDoctorMissingTrunk(t *testing.T) { + env := NewTestEnv(t) + + env.MustRun("init") + env.MustRun("create", "feature-a") + env.CreateCommit("work on a") + + // Delete the trunk branch (main) while on feature-a + env.Git("branch", "-D", "main") + + result := env.Run("doctor") + if !result.ContainsStdout("Trunk branch") || !result.ContainsStdout("does not exist locally") { + t.Errorf("expected missing trunk report, got: %s", result.Stdout) + } +} + func TestDoctorNotInitialized(t *testing.T) { env := NewTestEnv(t) From 9d7018cb81f729ec329047deb1c729f4ac0f139f Mon Sep 17 00:00:00 2001 From: cds-amal Date: Sat, 14 Feb 2026 14:30:53 -0500 Subject: [PATCH 12/16] style(doctor): use s.Error for "No fork point stored" message Consistent with other simple issue messages in checkBranch that use the style package for formatting. --- cmd/doctor.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/doctor.go b/cmd/doctor.go index f3059b1..963393b 100644 --- a/cmd/doctor.go +++ b/cmd/doctor.go @@ -147,7 +147,7 @@ func checkBranch(g *git.Git, cfg *config.Config, s *style.Style, branch string, result.fixMsg = fmt.Sprintf("set fork point to %s", git.AbbrevSHA(newFP)) return result } - result.issues = append(result.issues, "No fork point stored") + result.issues = append(result.issues, s.Error("No fork point stored")) return result } From 58bdd62dc0696795fc9a00f9135ccc6a5a8dc7ec Mon Sep 17 00:00:00 2001 From: cds-amal Date: Sat, 14 Feb 2026 17:24:50 -0500 Subject: [PATCH 13/16] refactor(doctor): extract reusable branch health validation into internal/health Move the pure validation logic out of doctor's checkBranch into a new health.CheckBranch API that returns structured []Issue values (with Kind, Message, Fixable fields) instead of styled strings. This makes the health checks composable so future commands (cascade, sync) can run them as pre-flight validation before rebasing without pulling in style or fix logic. Doctor becomes a thin wrapper: call health.CheckBranch for diagnosis, then layer styling and repair logic on top based on issue kind. --- cmd/doctor.go | 166 ++++++++---------- internal/health/health.go | 103 +++++++++++ internal/health/health_test.go | 305 +++++++++++++++++++++++++++++++++ 3 files changed, 478 insertions(+), 96 deletions(-) create mode 100644 internal/health/health.go create mode 100644 internal/health/health_test.go diff --git a/cmd/doctor.go b/cmd/doctor.go index 963393b..7dd77a4 100644 --- a/cmd/doctor.go +++ b/cmd/doctor.go @@ -9,6 +9,7 @@ import ( "github.com/boneskull/gh-stack/internal/config" "github.com/boneskull/gh-stack/internal/git" + "github.com/boneskull/gh-stack/internal/health" "github.com/boneskull/gh-stack/internal/style" "github.com/spf13/cobra" ) @@ -106,125 +107,98 @@ func runDoctor(cmd *cobra.Command, args []string) error { func checkBranch(g *git.Git, cfg *config.Config, s *style.Style, branch string, fix bool) branchResult { result := branchResult{name: branch} - // Check 1: branch exists locally - if !g.BranchExists(branch) { - result.issues = append(result.issues, s.Error("Branch does not exist locally")) + issues := health.CheckBranch(g, cfg, branch) + if len(issues) == 0 { + result.healthy = true return result } - // Check 2: parent exists locally - parent, err := cfg.GetParent(branch) - if err != nil { - result.issues = append(result.issues, s.Error("No parent configured")) + // If not fixing, format issues with styling and return. + if !fix { + for _, iss := range issues { + switch iss.Kind { + case health.KindBranchMissing, health.KindParentMissing, health.KindNoForkPoint: + result.issues = append(result.issues, s.Error(iss.Message)) + default: + result.issues = append(result.issues, iss.Message) + } + } return result } - trunk, _ := cfg.GetTrunk() //nolint:errcheck // already validated in runDoctor - if parent == trunk { - if !g.BranchExists(trunk) { - result.issues = append(result.issues, s.Errorf("Trunk branch %s does not exist locally", trunk)) - return result + // Attempt to fix: dispatch based on the issue kind. + kind := issues[0].Kind + if !issues[0].Fixable && kind != health.KindDrift { + // Unfixable issues (missing branch, missing parent, check failures) + for _, iss := range issues { + result.issues = append(result.issues, s.Error(iss.Message)) } - } else if !g.BranchExists(parent) { - result.issues = append(result.issues, s.Errorf("Parent branch %s does not exist locally", parent)) return result } - // Check 3: fork point is stored - storedFP, err := cfg.GetForkPoint(branch) - if err != nil { - if fix { - newFP, fixErr := computeForkPoint(g, parent, branch) - if fixErr != nil { - result.issues = append(result.issues, fmt.Sprintf("No fork point stored and could not compute one: %v", fixErr)) - return result - } - if setErr := cfg.SetForkPoint(branch, newFP); setErr != nil { - result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", setErr)) - return result - } - result.fixed = true - result.fixMsg = fmt.Sprintf("set fork point to %s", git.AbbrevSHA(newFP)) + parent, _ := cfg.GetParent(branch) //nolint:errcheck // health check validated parent exists + storedFP, _ := cfg.GetForkPoint(branch) //nolint:errcheck // may be empty for KindNoForkPoint + + switch kind { + case health.KindNoForkPoint: + newFP, fixErr := computeForkPoint(g, parent, branch) + if fixErr != nil { + result.issues = append(result.issues, fmt.Sprintf("No fork point stored and could not compute one: %v", fixErr)) return result } - result.issues = append(result.issues, s.Error("No fork point stored")) - return result - } - - // Check 4: fork point commit exists - if !g.CommitExists(storedFP) { - if fix { - newFP, fixErr := computeForkPoint(g, parent, branch) - if fixErr != nil { - result.issues = append(result.issues, fmt.Sprintf("Stored fork point %s does not exist and could not compute replacement: %v", git.AbbrevSHA(storedFP), fixErr)) - return result - } - if setErr := setForkPointWithComment(g, cfg, branch, storedFP, newFP); setErr != nil { - result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", setErr)) - return result - } - result.fixed = true - result.fixMsg = fmt.Sprintf("updated fork point %s \u2192 %s", git.AbbrevSHA(storedFP), git.AbbrevSHA(newFP)) + if setErr := cfg.SetForkPoint(branch, newFP); setErr != nil { + result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", setErr)) return result } - result.issues = append(result.issues, fmt.Sprintf("Stored fork point %s does not exist", git.AbbrevSHA(storedFP))) - return result - } + result.fixed = true + result.fixMsg = fmt.Sprintf("set fork point to %s", git.AbbrevSHA(newFP)) - // Check 5: fork point is ancestor of branch - isAnc, err := g.IsAncestor(storedFP, branch) - if err != nil { - result.issues = append(result.issues, fmt.Sprintf("Failed to check if stored fork point %s is an ancestor of %s: %v", git.AbbrevSHA(storedFP), branch, err)) - return result - } - if !isAnc { - if fix { - newFP, fixErr := computeForkPoint(g, parent, branch) - if fixErr != nil { - result.issues = append(result.issues, fmt.Sprintf("Stored fork point %s is not an ancestor of %s and could not compute replacement: %v", git.AbbrevSHA(storedFP), branch, fixErr)) - return result - } - if setErr := setForkPointWithComment(g, cfg, branch, storedFP, newFP); setErr != nil { - result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", setErr)) - return result - } - result.fixed = true - result.fixMsg = fmt.Sprintf("updated fork point %s \u2192 %s", git.AbbrevSHA(storedFP), git.AbbrevSHA(newFP)) + case health.KindForkPointMissing: + newFP, fixErr := computeForkPoint(g, parent, branch) + if fixErr != nil { + result.issues = append(result.issues, fmt.Sprintf("Stored fork point %s does not exist and could not compute replacement: %v", git.AbbrevSHA(storedFP), fixErr)) return result } - result.issues = append(result.issues, fmt.Sprintf("Stored fork point %s is not an ancestor of %s", git.AbbrevSHA(storedFP), branch)) - return result - } + if setErr := setForkPointWithComment(g, cfg, branch, storedFP, newFP); setErr != nil { + result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", setErr)) + return result + } + result.fixed = true + result.fixMsg = fmt.Sprintf("updated fork point %s \u2192 %s", git.AbbrevSHA(storedFP), git.AbbrevSHA(newFP)) - // Check 6: drift detection - mergeBase, err := g.GetMergeBase(parent, branch) - if err != nil { - result.issues = append(result.issues, - fmt.Sprintf("Failed to compute merge-base for %s and %s: %v", parent, branch, err), - ) - return result - } + case health.KindForkPointNotAncestor: + newFP, fixErr := computeForkPoint(g, parent, branch) + if fixErr != nil { + result.issues = append(result.issues, fmt.Sprintf("Stored fork point %s is not an ancestor of %s and could not compute replacement: %v", git.AbbrevSHA(storedFP), branch, fixErr)) + return result + } + if setErr := setForkPointWithComment(g, cfg, branch, storedFP, newFP); setErr != nil { + result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", setErr)) + return result + } + result.fixed = true + result.fixMsg = fmt.Sprintf("updated fork point %s \u2192 %s", git.AbbrevSHA(storedFP), git.AbbrevSHA(newFP)) - if storedFP != mergeBase { - if fix { - if setErr := setForkPointWithComment(g, cfg, branch, storedFP, mergeBase); setErr != nil { - result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", setErr)) - return result - } - result.fixed = true - result.fixMsg = fmt.Sprintf("updated fork point %s \u2192 %s", git.AbbrevSHA(storedFP), git.AbbrevSHA(mergeBase)) + case health.KindDrift: + mergeBase, err := g.GetMergeBase(parent, branch) + if err != nil { + result.issues = append(result.issues, fmt.Sprintf("Failed to compute merge-base for fix: %v", err)) return result } - result.issues = append(result.issues, - fmt.Sprintf("Stored parent: %s", parent), - fmt.Sprintf("Stored fork: %s", git.AbbrevSHA(storedFP)), - fmt.Sprintf("Actual fork: %s", git.AbbrevSHA(mergeBase)), - "Drift detected: stored fork point does not match merge-base", - ) - return result + if setErr := setForkPointWithComment(g, cfg, branch, storedFP, mergeBase); setErr != nil { + result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", setErr)) + return result + } + result.fixed = true + result.fixMsg = fmt.Sprintf("updated fork point %s \u2192 %s", git.AbbrevSHA(storedFP), git.AbbrevSHA(mergeBase)) + + default: + // Shouldn't happen, but surface issues if it does + for _, iss := range issues { + result.issues = append(result.issues, iss.Message) + } } - result.healthy = true return result } diff --git a/internal/health/health.go b/internal/health/health.go new file mode 100644 index 0000000..d6703f2 --- /dev/null +++ b/internal/health/health.go @@ -0,0 +1,103 @@ +// internal/health/health.go +package health + +import ( + "fmt" + + "github.com/boneskull/gh-stack/internal/config" + "github.com/boneskull/gh-stack/internal/git" +) + +// IssueKind identifies which health check produced an issue. +type IssueKind int + +const ( + // KindBranchMissing means the branch does not exist locally. + KindBranchMissing IssueKind = iota + 1 + // KindParentMissing means the parent branch does not exist locally. + KindParentMissing + // KindNoForkPoint means no fork point is stored in config. + KindNoForkPoint + // KindForkPointMissing means the stored fork point commit doesn't exist. + KindForkPointMissing + // KindForkPointNotAncestor means the stored fork point is not an ancestor of the branch. + KindForkPointNotAncestor + // KindDrift means the stored fork point doesn't match the computed merge-base. + KindDrift + // KindCheckFailed means a check itself errored (e.g., merge-base computation failed). + KindCheckFailed +) + +// Issue describes a branch health problem. +type Issue struct { + Kind IssueKind // which check produced this issue + Message string // plain text, no ANSI styling + Fixable bool // can `doctor --fix` repair this? +} + +// CheckBranch runs lightweight health checks on a tracked branch. +// Returns nil if healthy. Checks run in order and bail on first blocker: +// 1. Branch exists locally +// 2. Parent exists locally (including trunk) +// 3. Fork point is stored in config +// 4. Fork point commit exists in repo +// 5. Fork point is ancestor of branch +// 6. Stored fork point matches merge-base (drift detection) +func CheckBranch(g *git.Git, cfg *config.Config, branch string) []Issue { + // Check 1: branch exists locally + if !g.BranchExists(branch) { + return []Issue{{Kind: KindBranchMissing, Message: "Branch does not exist locally", Fixable: false}} + } + + // Check 2: parent exists locally + parent, err := cfg.GetParent(branch) + if err != nil { + return []Issue{{Kind: KindParentMissing, Message: "No parent configured", Fixable: false}} + } + + trunk, _ := cfg.GetTrunk() //nolint:errcheck // caller should validate trunk exists + if parent == trunk { + if !g.BranchExists(trunk) { + return []Issue{{Kind: KindParentMissing, Message: fmt.Sprintf("Trunk branch %s does not exist locally", trunk), Fixable: false}} + } + } else if !g.BranchExists(parent) { + return []Issue{{Kind: KindParentMissing, Message: fmt.Sprintf("Parent branch %s does not exist locally", parent), Fixable: false}} + } + + // Check 3: fork point is stored + storedFP, err := cfg.GetForkPoint(branch) + if err != nil { + return []Issue{{Kind: KindNoForkPoint, Message: "No fork point stored", Fixable: true}} + } + + // Check 4: fork point commit exists + if !g.CommitExists(storedFP) { + return []Issue{{Kind: KindForkPointMissing, Message: fmt.Sprintf("Stored fork point %s does not exist", git.AbbrevSHA(storedFP)), Fixable: true}} + } + + // Check 5: fork point is ancestor of branch + isAnc, err := g.IsAncestor(storedFP, branch) + if err != nil { + return []Issue{{Kind: KindCheckFailed, Message: fmt.Sprintf("Failed to check if stored fork point %s is an ancestor of %s: %v", git.AbbrevSHA(storedFP), branch, err), Fixable: false}} + } + if !isAnc { + return []Issue{{Kind: KindForkPointNotAncestor, Message: fmt.Sprintf("Stored fork point %s is not an ancestor of %s", git.AbbrevSHA(storedFP), branch), Fixable: true}} + } + + // Check 6: drift detection + mergeBase, err := g.GetMergeBase(parent, branch) + if err != nil { + return []Issue{{Kind: KindCheckFailed, Message: fmt.Sprintf("Failed to compute merge-base for %s and %s: %v", parent, branch, err), Fixable: false}} + } + + if storedFP != mergeBase { + return []Issue{ + {Kind: KindDrift, Message: fmt.Sprintf("Stored parent: %s", parent), Fixable: false}, + {Kind: KindDrift, Message: fmt.Sprintf("Stored fork: %s", git.AbbrevSHA(storedFP)), Fixable: false}, + {Kind: KindDrift, Message: fmt.Sprintf("Actual fork: %s", git.AbbrevSHA(mergeBase)), Fixable: false}, + {Kind: KindDrift, Message: "Drift detected: stored fork point does not match merge-base", Fixable: true}, + } + } + + return nil +} diff --git a/internal/health/health_test.go b/internal/health/health_test.go new file mode 100644 index 0000000..4cc5abf --- /dev/null +++ b/internal/health/health_test.go @@ -0,0 +1,305 @@ +// internal/health/health_test.go +package health_test + +import ( + "os" + "os/exec" + "path/filepath" + "testing" + + "github.com/boneskull/gh-stack/internal/config" + "github.com/boneskull/gh-stack/internal/git" + "github.com/boneskull/gh-stack/internal/health" +) + +// setupTestRepo creates a temp git repo with an initial commit and returns +// the repo path, a *git.Git, and a *config.Config. The default branch is +// configured as the stack trunk. +func setupTestRepo(t *testing.T) (string, *git.Git, *config.Config) { + t.Helper() + dir := t.TempDir() + + run := func(args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %v failed: %v\n%s", args, err, out) + } + } + + run("init") + run("config", "user.email", "test@test.com") + run("config", "user.name", "Test") + + f := filepath.Join(dir, "README.md") + os.WriteFile(f, []byte("# Test"), 0644) + run("add", ".") + run("commit", "-m", "initial") + + g := git.New(dir) + cfg, err := config.Load(dir) + if err != nil { + t.Fatalf("config.Load failed: %v", err) + } + + trunk, err := g.CurrentBranch() + if err != nil { + t.Fatalf("CurrentBranch failed: %v", err) + } + if err := cfg.SetTrunk(trunk); err != nil { + t.Fatalf("SetTrunk failed: %v", err) + } + + return dir, g, cfg +} + +// addTrackedBranch creates a branch off parent with one commit and +// configures stack metadata (parent + fork point). +func addTrackedBranch(t *testing.T, dir string, g *git.Git, cfg *config.Config, branch, parent string) { + t.Helper() + run := func(args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %v failed: %v\n%s", args, err, out) + } + } + + // Record fork point before creating branch + parentTip, err := g.GetTip(parent) + if err != nil { + t.Fatalf("GetTip(%s) failed: %v", parent, err) + } + + run("checkout", "-b", branch, parent) + + f := filepath.Join(dir, branch+".txt") + os.WriteFile(f, []byte(branch+" content"), 0644) + run("add", ".") + run("commit", "-m", branch+" commit") + + if err := cfg.SetParent(branch, parent); err != nil { + t.Fatalf("SetParent failed: %v", err) + } + if err := cfg.SetForkPoint(branch, parentTip); err != nil { + t.Fatalf("SetForkPoint failed: %v", err) + } +} + +func TestCheckBranch_Healthy(t *testing.T) { + dir, g, cfg := setupTestRepo(t) + trunk, _ := g.CurrentBranch() + addTrackedBranch(t, dir, g, cfg, "feature", trunk) + + issues := health.CheckBranch(g, cfg, "feature") + if len(issues) != 0 { + t.Errorf("expected no issues, got %v", issues) + } +} + +func TestCheckBranch_MissingBranch(t *testing.T) { + _, g, cfg := setupTestRepo(t) + + // Track a branch that doesn't exist in git + cfg.SetParent("ghost", "main") + + issues := health.CheckBranch(g, cfg, "ghost") + if len(issues) == 0 { + t.Fatal("expected issues for missing branch") + } + if issues[0].Message != "Branch does not exist locally" { + t.Errorf("unexpected message: %s", issues[0].Message) + } + if issues[0].Fixable { + t.Error("missing branch should not be fixable") + } +} + +func TestCheckBranch_MissingParent(t *testing.T) { + dir, g, cfg := setupTestRepo(t) + trunk, _ := g.CurrentBranch() + + // Create a branch tracked against a parent that will be deleted + addTrackedBranch(t, dir, g, cfg, "child", trunk) + + // Now create "middle" as parent, point child at it, then delete it + run := func(args ...string) { + cmd := exec.Command("git", args...) + cmd.Dir = dir + cmd.Run() + } + run("checkout", trunk) + run("checkout", "-b", "middle") + run("checkout", trunk) + + cfg.SetParent("child", "middle") + + // Delete the parent branch + run("branch", "-D", "middle") + + issues := health.CheckBranch(g, cfg, "child") + if len(issues) == 0 { + t.Fatal("expected issues for missing parent") + } + if got := issues[0].Message; got != "Parent branch middle does not exist locally" { + t.Errorf("unexpected message: %s", got) + } +} + +func TestCheckBranch_MissingTrunk(t *testing.T) { + dir, g, cfg := setupTestRepo(t) + trunk, _ := g.CurrentBranch() + + addTrackedBranch(t, dir, g, cfg, "feature", trunk) + + // Point the feature's parent at trunk, then rename trunk so it disappears + run := func(args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %v failed: %v\n%s", args, err, out) + } + } + + run("checkout", "feature") + run("branch", "-m", trunk, "gone-trunk") + + // feature's parent is still trunk (which no longer exists) + issues := health.CheckBranch(g, cfg, "feature") + if len(issues) == 0 { + t.Fatal("expected issues for missing trunk") + } + found := false + for _, iss := range issues { + if iss.Message == "Trunk branch "+trunk+" does not exist locally" { + found = true + } + } + if !found { + t.Errorf("expected trunk-missing message, got %v", issues) + } +} + +func TestCheckBranch_NoForkPoint(t *testing.T) { + dir, g, cfg := setupTestRepo(t) + trunk, _ := g.CurrentBranch() + + addTrackedBranch(t, dir, g, cfg, "feature", trunk) + + // Remove the fork point from config + cfg.RemoveForkPoint("feature") + + issues := health.CheckBranch(g, cfg, "feature") + if len(issues) == 0 { + t.Fatal("expected issues for missing fork point") + } + if issues[0].Message != "No fork point stored" { + t.Errorf("unexpected message: %s", issues[0].Message) + } + if !issues[0].Fixable { + t.Error("missing fork point should be fixable") + } +} + +func TestCheckBranch_NonexistentForkPointCommit(t *testing.T) { + dir, g, cfg := setupTestRepo(t) + trunk, _ := g.CurrentBranch() + + addTrackedBranch(t, dir, g, cfg, "feature", trunk) + + // Set fork point to a SHA that doesn't exist + fakeSHA := "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef" + cfg.SetForkPoint("feature", fakeSHA) + + issues := health.CheckBranch(g, cfg, "feature") + if len(issues) == 0 { + t.Fatal("expected issues for nonexistent fork point commit") + } + if !issues[0].Fixable { + t.Error("nonexistent fork point should be fixable") + } +} + +func TestCheckBranch_ForkPointNotAncestor(t *testing.T) { + dir, g, cfg := setupTestRepo(t) + trunk, _ := g.CurrentBranch() + + addTrackedBranch(t, dir, g, cfg, "feature", trunk) + + // Create an orphan commit that exists but isn't an ancestor of feature + run := func(args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %v failed: %v\n%s", args, err, out) + } + } + + run("checkout", trunk) + os.WriteFile(filepath.Join(dir, "diverged.txt"), []byte("diverged"), 0644) + run("add", ".") + run("commit", "-m", "diverged commit") + divergedSHA, _ := g.GetTip("HEAD") + + // Set fork point to this diverged commit (exists, but not ancestor of feature) + cfg.SetForkPoint("feature", divergedSHA) + + issues := health.CheckBranch(g, cfg, "feature") + if len(issues) == 0 { + t.Fatal("expected issues for fork point not ancestor") + } + if !issues[0].Fixable { + t.Error("fork point not-ancestor should be fixable") + } +} + +func TestCheckBranch_Drift(t *testing.T) { + dir, g, cfg := setupTestRepo(t) + trunk, _ := g.CurrentBranch() + + addTrackedBranch(t, dir, g, cfg, "feature", trunk) + + run := func(args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %v failed: %v\n%s", args, err, out) + } + } + + // Rebase feature onto a new commit on trunk, which moves the actual + // merge-base forward while the stored fork point stays at the old value. + run("checkout", trunk) + os.WriteFile(filepath.Join(dir, "trunk2.txt"), []byte("new trunk"), 0644) + run("add", ".") + run("commit", "-m", "trunk advance") + + run("checkout", "feature") + run("rebase", trunk) + + // Now the stored fork point is the old trunk tip, but merge-base is + // the new trunk tip. That's drift. + issues := health.CheckBranch(g, cfg, "feature") + if len(issues) == 0 { + t.Fatal("expected drift issues") + } + + // The last issue in a drift result should mention "Drift detected" + last := issues[len(issues)-1] + if last.Message != "Drift detected: stored fork point does not match merge-base" { + t.Errorf("expected drift message, got: %s", last.Message) + } + if !last.Fixable { + t.Error("drift should be fixable") + } +} From c5d8fabf3f440aa70265d4c872e6d7b19b8d5a09 Mon Sep 17 00:00:00 2001 From: cds-amal Date: Sat, 14 Feb 2026 18:09:22 -0500 Subject: [PATCH 14/16] refactor(doctor): replace hand-rolled INI parsing with git config --comment Use git config --comment (Git 2.45+) to write fork-point metadata with an inline note (extending our provenance trail so fixes are auditable), instead of manually rewriting .git/config. If --comment isn't supported, fall back to a plain git config set and emit a one-time warning. Also reduces cyclomatic complexity across all of doctor.go by extracting printResult, printSummary, fixBranch, fixMissingForkPoint, fixStaleForkPoint, and fixDrift helpers. Peak complexity drops from 21 to 8. --- cmd/doctor.go | 292 ++++++++++++++++----------------- e2e/doctor_test.go | 13 +- internal/config/config.go | 7 + internal/config/config_test.go | 35 ++++ 4 files changed, 191 insertions(+), 156 deletions(-) diff --git a/cmd/doctor.go b/cmd/doctor.go index 7dd77a4..3378643 100644 --- a/cmd/doctor.go +++ b/cmd/doctor.go @@ -4,7 +4,6 @@ package cmd import ( "fmt" "os" - "strings" "time" "github.com/boneskull/gh-stack/internal/config" @@ -68,42 +67,55 @@ func runDoctor(cmd *cobra.Command, args []string) error { fmt.Println(s.Bold("Stack Health Report")) fmt.Println() - var healthy, issues int + var issueCount int for _, branch := range branches { result := checkBranch(g, cfg, s, branch, doctorFixFlag) - if result.healthy { - fmt.Printf("%s %s %s\n", s.SuccessIcon(), s.Branch(result.name), s.Muted("(healthy)")) - healthy++ - } else if result.fixed { - fmt.Printf("%s %s: %s\n", s.SuccessIcon(), s.Branch(result.name), result.fixMsg) - healthy++ - } else { - fmt.Printf("%s %s\n", s.FailureIcon(), s.Branch(result.name)) - for _, issue := range result.issues { - fmt.Printf(" %s\n", issue) - } - issues++ + if !printResult(s, result) { + issueCount++ } } fmt.Println() - if issues > 0 { - noun := "issue" - if issues > 1 { - noun = "issues" - } - fmt.Printf("%d %s found.", issues, noun) - if !doctorFixFlag { - fmt.Printf(" Run 'gh stack doctor --fix' to repair.") - } - fmt.Println() - } else { - fmt.Println(s.SuccessMessage("All branches healthy.")) - } + printSummary(s, issueCount, doctorFixFlag) return nil } +// printResult prints the outcome of a single branch check. +// Returns true if the branch is healthy (or was fixed), false if issues remain. +func printResult(s *style.Style, r branchResult) bool { + if r.healthy { + fmt.Printf("%s %s %s\n", s.SuccessIcon(), s.Branch(r.name), s.Muted("(healthy)")) + return true + } + if r.fixed { + fmt.Printf("%s %s: %s\n", s.SuccessIcon(), s.Branch(r.name), r.fixMsg) + return true + } + fmt.Printf("%s %s\n", s.FailureIcon(), s.Branch(r.name)) + for _, issue := range r.issues { + fmt.Printf(" %s\n", issue) + } + return false +} + +// printSummary prints the final summary line after all branches have been checked. +func printSummary(s *style.Style, issueCount int, fix bool) { + if issueCount == 0 { + fmt.Println(s.SuccessMessage("All branches healthy.")) + return + } + noun := "issue" + if issueCount > 1 { + noun = "issues" + } + fmt.Printf("%d %s found.", issueCount, noun) + if !fix { + fmt.Printf(" Run 'gh stack doctor --fix' to repair.") + } + fmt.Println() +} + func checkBranch(g *git.Git, cfg *config.Config, s *style.Style, branch string, fix bool) branchResult { result := branchResult{name: branch} @@ -113,26 +125,35 @@ func checkBranch(g *git.Git, cfg *config.Config, s *style.Style, branch string, return result } - // If not fixing, format issues with styling and return. if !fix { - for _, iss := range issues { - switch iss.Kind { - case health.KindBranchMissing, health.KindParentMissing, health.KindNoForkPoint: - result.issues = append(result.issues, s.Error(iss.Message)) - default: - result.issues = append(result.issues, iss.Message) - } - } + result.issues = formatIssues(s, issues) return result } - // Attempt to fix: dispatch based on the issue kind. + return fixBranch(g, cfg, s, branch, issues) +} + +// formatIssues converts health issues into styled display strings. +func formatIssues(s *style.Style, issues []health.Issue) []string { + out := make([]string, 0, len(issues)) + for _, iss := range issues { + switch iss.Kind { + case health.KindBranchMissing, health.KindParentMissing, health.KindNoForkPoint: + out = append(out, s.Error(iss.Message)) + default: + out = append(out, iss.Message) + } + } + return out +} + +// fixBranch attempts to repair the first issue found by health.CheckBranch. +func fixBranch(g *git.Git, cfg *config.Config, s *style.Style, branch string, issues []health.Issue) branchResult { + result := branchResult{name: branch} + kind := issues[0].Kind if !issues[0].Fixable && kind != health.KindDrift { - // Unfixable issues (missing branch, missing parent, check failures) - for _, iss := range issues { - result.issues = append(result.issues, s.Error(iss.Message)) - } + result.issues = formatIssues(s, issues) return result } @@ -141,64 +162,67 @@ func checkBranch(g *git.Git, cfg *config.Config, s *style.Style, branch string, switch kind { case health.KindNoForkPoint: - newFP, fixErr := computeForkPoint(g, parent, branch) - if fixErr != nil { - result.issues = append(result.issues, fmt.Sprintf("No fork point stored and could not compute one: %v", fixErr)) - return result - } - if setErr := cfg.SetForkPoint(branch, newFP); setErr != nil { - result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", setErr)) - return result - } - result.fixed = true - result.fixMsg = fmt.Sprintf("set fork point to %s", git.AbbrevSHA(newFP)) - - case health.KindForkPointMissing: - newFP, fixErr := computeForkPoint(g, parent, branch) - if fixErr != nil { - result.issues = append(result.issues, fmt.Sprintf("Stored fork point %s does not exist and could not compute replacement: %v", git.AbbrevSHA(storedFP), fixErr)) - return result - } - if setErr := setForkPointWithComment(g, cfg, branch, storedFP, newFP); setErr != nil { - result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", setErr)) - return result - } - result.fixed = true - result.fixMsg = fmt.Sprintf("updated fork point %s \u2192 %s", git.AbbrevSHA(storedFP), git.AbbrevSHA(newFP)) - - case health.KindForkPointNotAncestor: - newFP, fixErr := computeForkPoint(g, parent, branch) - if fixErr != nil { - result.issues = append(result.issues, fmt.Sprintf("Stored fork point %s is not an ancestor of %s and could not compute replacement: %v", git.AbbrevSHA(storedFP), branch, fixErr)) - return result - } - if setErr := setForkPointWithComment(g, cfg, branch, storedFP, newFP); setErr != nil { - result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", setErr)) - return result - } - result.fixed = true - result.fixMsg = fmt.Sprintf("updated fork point %s \u2192 %s", git.AbbrevSHA(storedFP), git.AbbrevSHA(newFP)) - + return fixMissingForkPoint(g, cfg, branch, parent) + case health.KindForkPointMissing, health.KindForkPointNotAncestor: + return fixStaleForkPoint(g, cfg, s, branch, parent, storedFP, issues[0].Message) case health.KindDrift: - mergeBase, err := g.GetMergeBase(parent, branch) - if err != nil { - result.issues = append(result.issues, fmt.Sprintf("Failed to compute merge-base for fix: %v", err)) - return result - } - if setErr := setForkPointWithComment(g, cfg, branch, storedFP, mergeBase); setErr != nil { - result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", setErr)) - return result - } - result.fixed = true - result.fixMsg = fmt.Sprintf("updated fork point %s \u2192 %s", git.AbbrevSHA(storedFP), git.AbbrevSHA(mergeBase)) - + return fixDrift(g, cfg, s, branch, parent, storedFP) default: - // Shouldn't happen, but surface issues if it does for _, iss := range issues { result.issues = append(result.issues, iss.Message) } + return result + } +} + +// fixMissingForkPoint computes and sets a fork point when none is stored. +func fixMissingForkPoint(g *git.Git, cfg *config.Config, branch, parent string) branchResult { + result := branchResult{name: branch} + newFP, err := computeForkPoint(g, parent, branch) + if err != nil { + result.issues = append(result.issues, fmt.Sprintf("No fork point stored and could not compute one: %v", err)) + return result + } + if err := cfg.SetForkPoint(branch, newFP); err != nil { + result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", err)) + return result } + result.fixed = true + result.fixMsg = fmt.Sprintf("set fork point to %s", git.AbbrevSHA(newFP)) + return result +} +// fixStaleForkPoint recomputes and replaces a fork point that is missing or not an ancestor. +func fixStaleForkPoint(g *git.Git, cfg *config.Config, s *style.Style, branch, parent, storedFP, context string) branchResult { + result := branchResult{name: branch} + newFP, err := computeForkPoint(g, parent, branch) + if err != nil { + result.issues = append(result.issues, fmt.Sprintf("%s and could not compute replacement: %v", context, err)) + return result + } + if err := setForkPointWithComment(s, cfg, branch, storedFP, newFP); err != nil { + result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", err)) + return result + } + result.fixed = true + result.fixMsg = fmt.Sprintf("updated fork point %s \u2192 %s", git.AbbrevSHA(storedFP), git.AbbrevSHA(newFP)) + return result +} + +// fixDrift replaces a stored fork point that doesn't match the computed merge-base. +func fixDrift(g *git.Git, cfg *config.Config, s *style.Style, branch, parent, storedFP string) branchResult { + result := branchResult{name: branch} + mergeBase, err := g.GetMergeBase(parent, branch) + if err != nil { + result.issues = append(result.issues, fmt.Sprintf("Failed to compute merge-base for fix: %v", err)) + return result + } + if err := setForkPointWithComment(s, cfg, branch, storedFP, mergeBase); err != nil { + result.issues = append(result.issues, fmt.Sprintf("Failed to set fork point: %v", err)) + return result + } + result.fixed = true + result.fixMsg = fmt.Sprintf("updated fork point %s \u2192 %s", git.AbbrevSHA(storedFP), git.AbbrevSHA(mergeBase)) return result } @@ -211,70 +235,36 @@ func computeForkPoint(g *git.Git, parent, branch string) (string, error) { return g.GetMergeBase(parent, branch) } -// setForkPointWithComment updates the fork point and inserts a comment preserving the old value. -// The comment is best-effort; if it fails, the fork point is still updated. -func setForkPointWithComment(g *git.Git, cfg *config.Config, branch, oldSHA, newSHA string) error { - if err := cfg.SetForkPoint(branch, newSHA); err != nil { - return err +// setForkPointWithComment updates the fork point and records the old value +// as an inline git config comment for provenance. Falls back to a plain +// SetForkPoint if the git version doesn't support --comment (requires 2.45+). +func setForkPointWithComment(s *style.Style, cfg *config.Config, branch, oldSHA, newSHA string) error { + comment := fmt.Sprintf("replaces %s (%s)", + git.AbbrevSHA(oldSHA), + time.Now().UTC().Format(time.RFC3339), + ) + err := cfg.SetForkPointWithComment(branch, newSHA, comment) + if err == nil { + return nil } - commentForkPointChange(g, branch, oldSHA) - return nil -} -// commentForkPointChange inserts a comment above the stackForkPoint line in the git config -// recording the previous value and a timestamp. This preserves provenance so old fork -// points can be recovered if a fix goes wrong. Best-effort: errors are silently ignored. -func commentForkPointChange(g *git.Git, branch, oldSHA string) { - configPath, err := g.GetConfigPath() - if err != nil { - return - } - info, err := os.Stat(configPath) - if err != nil { - return - } - data, err := os.ReadFile(configPath) - if err != nil { - return + // --comment likely unsupported; fall back to plain set and warn once. + if setErr := cfg.SetForkPoint(branch, newSHA); setErr != nil { + return setErr } + warnOldGit(s) + return nil +} - lines := strings.Split(string(data), "\n") - sectionHeader := fmt.Sprintf("[branch %q]", branch) - inSection := false - modified := false - var result []string - - for _, line := range lines { - trimmed := strings.TrimSpace(line) - - if trimmed == sectionHeader { - inSection = true - result = append(result, line) - continue - } - - // New section starts; we've left the target section - if inSection && strings.HasPrefix(trimmed, "[") { - inSection = false - } - - // Insert comment before the stackForkPoint line - if inSection && strings.HasPrefix(strings.ToLower(trimmed), "stackforkpoint") { - indent := line[:len(line)-len(strings.TrimLeft(line, " \t"))] - timestamp := time.Now().UTC().Format(time.RFC3339) - result = append(result, - fmt.Sprintf("%s# doctor fix %s replaces previous value of:", indent, timestamp), - fmt.Sprintf("%s# %s", indent, oldSHA), - ) - modified = true - } - - result = append(result, line) - } +var oldGitWarned bool - if !modified { +// warnOldGit prints a one-time warning that the user's git version is too old +// for inline config comments. +func warnOldGit(s *style.Style) { + if oldGitWarned { return } - //nolint:errcheck // best-effort provenance - _ = os.WriteFile(configPath, []byte(strings.Join(result, "\n")), info.Mode()) + oldGitWarned = true + fmt.Printf("%s git config --comment not supported; fix provenance will be missing.\n", s.WarningIcon()) + fmt.Println(s.Muted(" gh-stack works best with git 2.45 or newer.")) } diff --git a/e2e/doctor_test.go b/e2e/doctor_test.go index 625c918..c123922 100644 --- a/e2e/doctor_test.go +++ b/e2e/doctor_test.go @@ -136,17 +136,20 @@ func TestDoctorFixUpdatesConfig(t *testing.T) { t.Errorf("expected new fork point %s to match merge-base %s", newFP, mergeBase) } - // Verify provenance comment was written to the config file + // Verify inline provenance comment was written to the config value line. + // git config --comment (Git 2.45+) appends "# replaces ()" + // on the same line as the stackForkPoint value. configData, err := os.ReadFile(filepath.Join(env.WorkDir, ".git", "config")) if err != nil { t.Fatalf("failed to read .git/config: %v", err) } configStr := string(configData) - if !strings.Contains(configStr, "# doctor fix") { - t.Error("expected provenance comment '# doctor fix ...' in .git/config") + if !strings.Contains(configStr, "# replaces") { + t.Error("expected inline provenance comment '# replaces ...' in .git/config") } - if !strings.Contains(configStr, "# "+originalFP) { - t.Errorf("expected old fork point %s preserved in config comment", originalFP) + abbrevOld := originalFP[:7] + if !strings.Contains(configStr, abbrevOld) { + t.Errorf("expected abbreviated old fork point %s in config comment", abbrevOld) } } diff --git a/internal/config/config.go b/internal/config/config.go index 4c93686..ab24176 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -116,6 +116,13 @@ func (c *Config) SetForkPoint(branch, sha string) error { return exec.Command("git", "-C", c.repoPath, "config", key, sha).Run() } +// SetForkPointWithComment stores the fork point SHA with an inline comment. +// Requires Git 2.45+. The comment appears after the value on the same line. +func (c *Config) SetForkPointWithComment(branch, sha, comment string) error { + key := "branch." + branch + ".stackForkPoint" + return exec.Command("git", "-C", c.repoPath, "config", "--comment", comment, key, sha).Run() +} + // RemoveForkPoint removes the stored fork point for a branch. func (c *Config) RemoveForkPoint(branch string) error { key := "branch." + branch + ".stackForkPoint" diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 9b395ca..6b1f5a7 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -3,7 +3,10 @@ package config_test import ( "errors" + "os" "os/exec" + "path/filepath" + "strings" "testing" "github.com/boneskull/gh-stack/internal/config" @@ -191,3 +194,35 @@ func TestForkPoint(t *testing.T) { t.Errorf("after remove, GetForkPoint = %v, want ErrNoForkPoint", err) } } + +func TestSetForkPointWithComment(t *testing.T) { + dir := setupTestRepo(t) + cfg, _ := config.Load(dir) + + sha := "abc123def456" + comment := "replaces deadbee (2026-02-14T00:00:00Z)" + + if err := cfg.SetForkPointWithComment("feature", sha, comment); err != nil { + t.Fatalf("SetForkPointWithComment failed: %v", err) + } + + // Value should be readable via GetForkPoint (git config ignores inline comments) + got, err := cfg.GetForkPoint("feature") + if err != nil { + t.Fatalf("GetForkPoint after SetForkPointWithComment failed: %v", err) + } + if got != sha { + t.Errorf("GetForkPoint = %q, want %q", got, sha) + } + + // The raw config file should contain the inline comment + configPath := filepath.Join(dir, ".git", "config") + data, err := os.ReadFile(configPath) + if err != nil { + t.Fatalf("failed to read .git/config: %v", err) + } + configStr := string(data) + if !strings.Contains(configStr, "# "+comment) { + t.Errorf("expected inline comment in config, got:\n%s", configStr) + } +} From cf051a3f07402d32e12b23f1d6ba9024caf38b37 Mon Sep 17 00:00:00 2001 From: cds-amal Date: Sat, 14 Feb 2026 19:03:47 -0500 Subject: [PATCH 15/16] fix(doctor): address PR feedback - Check errors in test setup helpers instead of silently discarding them (health_test.go, git_test.go) - Skip provenance-comment assertions on git < 2.45 instead of failing (config_test.go, e2e/doctor_test.go) - Discriminate "unknown option" from real errors in setForkPointWithComment so permissions/locking failures propagate instead of triggering the old-git warning (cmd/doctor.go, internal/config/config.go) - Delete unused GetConfigPath (dead code left over from the commentForkPointChange removal) Note: PR.md also updated to clarify abbreviated SHA in provenance comments (not included in this commit; will be added manually). --- cmd/doctor.go | 13 +++++++++- e2e/doctor_test.go | 14 ++++++----- internal/config/config.go | 13 +++++++++- internal/config/config_test.go | 3 ++- internal/git/git.go | 7 ------ internal/git/git_test.go | 46 ++++++++++++++++++++++++++-------- internal/health/health_test.go | 6 ++++- 7 files changed, 74 insertions(+), 28 deletions(-) diff --git a/cmd/doctor.go b/cmd/doctor.go index 3378643..062a2bf 100644 --- a/cmd/doctor.go +++ b/cmd/doctor.go @@ -4,6 +4,7 @@ package cmd import ( "fmt" "os" + "strings" "time" "github.com/boneskull/gh-stack/internal/config" @@ -248,7 +249,10 @@ func setForkPointWithComment(s *style.Style, cfg *config.Config, branch, oldSHA, return nil } - // --comment likely unsupported; fall back to plain set and warn once. + // Only fall back for unsupported --comment flag; propagate real errors. + if !isUnsupportedCommentErr(err) { + return err + } if setErr := cfg.SetForkPoint(branch, newSHA); setErr != nil { return setErr } @@ -256,6 +260,13 @@ func setForkPointWithComment(s *style.Style, cfg *config.Config, branch, oldSHA, return nil } +// isUnsupportedCommentErr returns true if the error indicates that +// git config --comment is not recognized (git < 2.45). +func isUnsupportedCommentErr(err error) bool { + msg := err.Error() + return strings.Contains(msg, "unknown option") +} + var oldGitWarned bool // warnOldGit prints a one-time warning that the user's git version is too old diff --git a/e2e/doctor_test.go b/e2e/doctor_test.go index c123922..87a59d2 100644 --- a/e2e/doctor_test.go +++ b/e2e/doctor_test.go @@ -139,17 +139,19 @@ func TestDoctorFixUpdatesConfig(t *testing.T) { // Verify inline provenance comment was written to the config value line. // git config --comment (Git 2.45+) appends "# replaces ()" // on the same line as the stackForkPoint value. + // On older git the comment won't be present; that's fine (doctor falls back gracefully). configData, err := os.ReadFile(filepath.Join(env.WorkDir, ".git", "config")) if err != nil { t.Fatalf("failed to read .git/config: %v", err) } configStr := string(configData) - if !strings.Contains(configStr, "# replaces") { - t.Error("expected inline provenance comment '# replaces ...' in .git/config") - } - abbrevOld := originalFP[:7] - if !strings.Contains(configStr, abbrevOld) { - t.Errorf("expected abbreviated old fork point %s in config comment", abbrevOld) + if strings.Contains(configStr, "# replaces") { + abbrevOld := originalFP[:7] + if !strings.Contains(configStr, abbrevOld) { + t.Errorf("expected abbreviated old fork point %s in config comment", abbrevOld) + } + } else { + t.Log("git config --comment not supported; skipping provenance comment assertion") } } diff --git a/internal/config/config.go b/internal/config/config.go index ab24176..d5484b2 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -118,9 +118,20 @@ func (c *Config) SetForkPoint(branch, sha string) error { // SetForkPointWithComment stores the fork point SHA with an inline comment. // Requires Git 2.45+. The comment appears after the value on the same line. +// Returns an error with stderr content on failure, so callers can distinguish +// "unknown option" (old git) from other failures. func (c *Config) SetForkPointWithComment(branch, sha, comment string) error { key := "branch." + branch + ".stackForkPoint" - return exec.Command("git", "-C", c.repoPath, "config", "--comment", comment, key, sha).Run() + cmd := exec.Command("git", "-C", c.repoPath, "config", "--comment", comment, key, sha) + var stderr bytes.Buffer + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + if stderr.Len() > 0 { + return errors.New(strings.TrimSpace(stderr.String())) + } + return err + } + return nil } // RemoveForkPoint removes the stored fork point for a branch. diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 6b1f5a7..baff9f5 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -203,7 +203,8 @@ func TestSetForkPointWithComment(t *testing.T) { comment := "replaces deadbee (2026-02-14T00:00:00Z)" if err := cfg.SetForkPointWithComment("feature", sha, comment); err != nil { - t.Fatalf("SetForkPointWithComment failed: %v", err) + // git config --comment requires Git 2.45+; skip on older versions. + t.Skipf("SetForkPointWithComment not supported (git too old?): %v", err) } // Value should be readable via GetForkPoint (git config ignores inline comments) diff --git a/internal/git/git.go b/internal/git/git.go index eada670..25b4681 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -363,13 +363,6 @@ func (g *Git) GetGitDir() string { return filepath.Join(g.repoPath, ".git") } -// GetConfigPath returns the path to the repo's git config file, -// resolved via `git rev-parse --git-path config` so it works -// correctly in linked worktrees and submodules. -func (g *Git) GetConfigPath() (string, error) { - return g.run("rev-parse", "--git-path", "config") -} - // Fetch fetches from origin. func (g *Git) Fetch() error { return g.runInteractive("fetch", "origin") diff --git a/internal/git/git_test.go b/internal/git/git_test.go index f5e5d02..2603be8 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -731,15 +731,33 @@ func TestIsAncestor(t *testing.T) { initial, _ := g.GetTip("HEAD") // Create a linear chain: initial -> A -> B - os.WriteFile(filepath.Join(dir, "a.txt"), []byte("a"), 0644) - exec.Command("git", "-C", dir, "add", ".").Run() - exec.Command("git", "-C", dir, "commit", "-m", "commit A").Run() - shaA, _ := g.GetTip("HEAD") + if err := os.WriteFile(filepath.Join(dir, "a.txt"), []byte("a"), 0644); err != nil { + t.Fatalf("WriteFile failed: %v", err) + } + if out, err := exec.Command("git", "-C", dir, "add", ".").CombinedOutput(); err != nil { + t.Fatalf("git add failed: %v\n%s", err, out) + } + if out, err := exec.Command("git", "-C", dir, "commit", "-m", "commit A").CombinedOutput(); err != nil { + t.Fatalf("git commit failed: %v\n%s", err, out) + } + shaA, err := g.GetTip("HEAD") + if err != nil { + t.Fatalf("GetTip failed: %v", err) + } - os.WriteFile(filepath.Join(dir, "b.txt"), []byte("b"), 0644) - exec.Command("git", "-C", dir, "add", ".").Run() - exec.Command("git", "-C", dir, "commit", "-m", "commit B").Run() - shaB, _ := g.GetTip("HEAD") + if err := os.WriteFile(filepath.Join(dir, "b.txt"), []byte("b"), 0644); err != nil { + t.Fatalf("WriteFile failed: %v", err) + } + if out, err := exec.Command("git", "-C", dir, "add", ".").CombinedOutput(); err != nil { + t.Fatalf("git add failed: %v\n%s", err, out) + } + if out, err := exec.Command("git", "-C", dir, "commit", "-m", "commit B").CombinedOutput(); err != nil { + t.Fatalf("git commit failed: %v\n%s", err, out) + } + shaB, err := g.GetTip("HEAD") + if err != nil { + t.Fatalf("GetTip failed: %v", err) + } // initial is ancestor of B isAnc, err := g.IsAncestor(initial, shaB) @@ -802,9 +820,15 @@ func TestGetForkPoint(t *testing.T) { g.CreateAndCheckout("child") // Add commits on child - os.WriteFile(filepath.Join(dir, "child.txt"), []byte("child"), 0644) - exec.Command("git", "-C", dir, "add", ".").Run() - exec.Command("git", "-C", dir, "commit", "-m", "child commit").Run() + if err := os.WriteFile(filepath.Join(dir, "child.txt"), []byte("child"), 0644); err != nil { + t.Fatalf("WriteFile failed: %v", err) + } + if out, err := exec.Command("git", "-C", dir, "add", ".").CombinedOutput(); err != nil { + t.Fatalf("git add failed: %v\n%s", err, out) + } + if out, err := exec.Command("git", "-C", dir, "commit", "-m", "child commit").CombinedOutput(); err != nil { + t.Fatalf("git commit failed: %v\n%s", err, out) + } // GetForkPoint should return the divergence point fp, err := g.GetForkPoint(trunk, "child") diff --git a/internal/health/health_test.go b/internal/health/health_test.go index 4cc5abf..bd3acbe 100644 --- a/internal/health/health_test.go +++ b/internal/health/health_test.go @@ -128,9 +128,13 @@ func TestCheckBranch_MissingParent(t *testing.T) { // Now create "middle" as parent, point child at it, then delete it run := func(args ...string) { + t.Helper() cmd := exec.Command("git", args...) cmd.Dir = dir - cmd.Run() + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %v failed: %v\n%s", args, err, out) + } } run("checkout", trunk) run("checkout", "-b", "middle") From cf48383075d6c27a7bd9a9e809a94c62150f414e Mon Sep 17 00:00:00 2001 From: cds-amal Date: Sun, 15 Feb 2026 01:52:59 -0500 Subject: [PATCH 16/16] refactor(config): route all git calls through internal/git.Git config.Config previously shelled out to git directly via exec.Command, bypassing the safeexec.LookPath resolution that internal/git provides. This adds ConfigGet/ConfigSet/ConfigUnset/ConfigSetWithComment/ConfigGetRegexp methods to git.Git, rewires Config to accept a *git.Git dependency (Load(path) becomes New(g)), and flips the construction order in all cmd/ call sites so git.New runs first. --- cmd/adopt.go | 6 ++-- cmd/adopt_test.go | 10 +++--- cmd/cascade.go | 6 ++-- cmd/continue.go | 2 +- cmd/create.go | 6 ++-- cmd/create_test.go | 15 ++++----- cmd/doctor.go | 6 ++-- cmd/init.go | 6 ++-- cmd/init_test.go | 6 ++-- cmd/link.go | 6 ++-- cmd/link_test.go | 6 ++-- cmd/log.go | 6 ++-- cmd/log_test.go | 10 ++++-- cmd/orphan.go | 6 ++-- cmd/orphan_test.go | 10 +++--- cmd/submit.go | 6 ++-- cmd/sync.go | 6 ++-- cmd/undo.go | 2 +- cmd/unlink.go | 6 ++-- cmd/unlink_test.go | 4 +-- internal/config/config.go | 60 ++++++++++++++-------------------- internal/config/config_test.go | 47 ++++++++++++++------------ internal/git/git.go | 29 ++++++++++++++++ internal/health/health_test.go | 4 +-- internal/tree/tree_test.go | 4 ++- 25 files changed, 153 insertions(+), 122 deletions(-) diff --git a/cmd/adopt.go b/cmd/adopt.go index da8c49d..69f00ce 100644 --- a/cmd/adopt.go +++ b/cmd/adopt.go @@ -36,13 +36,13 @@ func runAdopt(cmd *cobra.Command, args []string) error { return err } - cfg, err := config.Load(cwd) + g := git.New(cwd) + + cfg, err := config.New(g) if err != nil { return err } - g := git.New(cwd) - // Parent is the required positional argument parent := args[0] diff --git a/cmd/adopt_test.go b/cmd/adopt_test.go index 9f71a80..9781c5e 100644 --- a/cmd/adopt_test.go +++ b/cmd/adopt_test.go @@ -12,8 +12,8 @@ import ( func TestAdoptBranch(t *testing.T) { dir := setupTestRepo(t) - cfg, _ := config.Load(dir) g := git.New(dir) + cfg, _ := config.New(g) trunk, _ := g.CurrentBranch() cfg.SetTrunk(trunk) @@ -40,8 +40,8 @@ func TestAdoptBranch(t *testing.T) { func TestAdoptRejectsAlreadyTracked(t *testing.T) { dir := setupTestRepo(t) - cfg, _ := config.Load(dir) g := git.New(dir) + cfg, _ := config.New(g) trunk, _ := g.CurrentBranch() cfg.SetTrunk(trunk) @@ -60,8 +60,8 @@ func TestAdoptRejectsAlreadyTracked(t *testing.T) { func TestAdoptRejectsUntrackedParent(t *testing.T) { dir := setupTestRepo(t) - cfg, _ := config.Load(dir) g := git.New(dir) + cfg, _ := config.New(g) trunk, _ := g.CurrentBranch() cfg.SetTrunk(trunk) @@ -81,8 +81,8 @@ func TestAdoptRejectsUntrackedParent(t *testing.T) { func TestAdoptDetectsCycle(t *testing.T) { dir := setupTestRepo(t) - cfg, _ := config.Load(dir) g := git.New(dir) + cfg, _ := config.New(g) trunk, _ := g.CurrentBranch() cfg.SetTrunk(trunk) @@ -117,8 +117,8 @@ func TestAdoptDetectsCycle(t *testing.T) { func TestAdoptStoresForkPoint(t *testing.T) { dir := setupTestRepo(t) - cfg, _ := config.Load(dir) g := git.New(dir) + cfg, _ := config.New(g) trunk, _ := g.CurrentBranch() cfg.SetTrunk(trunk) diff --git a/cmd/cascade.go b/cmd/cascade.go index e43a139..b19d0c5 100644 --- a/cmd/cascade.go +++ b/cmd/cascade.go @@ -47,13 +47,13 @@ func runCascade(cmd *cobra.Command, args []string) error { return err } - cfg, err := config.Load(cwd) + g := git.New(cwd) + + cfg, err := config.New(g) if err != nil { return err } - g := git.New(cwd) - // Check if cascade already in progress if state.Exists(g.GetGitDir()) { return errors.New("operation already in progress; use 'gh stack continue' or 'gh stack abort'") diff --git a/cmd/continue.go b/cmd/continue.go index d1bf035..d98640a 100644 --- a/cmd/continue.go +++ b/cmd/continue.go @@ -63,7 +63,7 @@ func runContinue(cmd *cobra.Command, args []string) error { fmt.Printf("%s Completed %s\n", s.SuccessIcon(), s.Branch(st.Current)) - cfg, err := config.Load(cwd) + cfg, err := config.New(g) if err != nil { return err } diff --git a/cmd/create.go b/cmd/create.go index fa2aa58..08bcdf0 100644 --- a/cmd/create.go +++ b/cmd/create.go @@ -39,13 +39,13 @@ func runCreate(cmd *cobra.Command, args []string) error { return err } - cfg, err := config.Load(cwd) + g := git.New(cwd) + + cfg, err := config.New(g) if err != nil { return err } - g := git.New(cwd) - // Get current branch currentBranch, err := g.CurrentBranch() if err != nil { diff --git a/cmd/create_test.go b/cmd/create_test.go index ba23992..ead448c 100644 --- a/cmd/create_test.go +++ b/cmd/create_test.go @@ -14,10 +14,9 @@ import ( func TestCreateFromTrunk(t *testing.T) { dir := setupTestRepo(t) - cfg, _ := config.Load(dir) - cfg.SetTrunk("main") - g := git.New(dir) + cfg, _ := config.New(g) + cfg.SetTrunk("main") // Simulate what create command does currentBranch, _ := g.CurrentBranch() @@ -57,8 +56,8 @@ func TestCreateFromTrunk(t *testing.T) { func TestCreateFromTrackedBranch(t *testing.T) { dir := setupTestRepo(t) - cfg, _ := config.Load(dir) g := git.New(dir) + cfg, _ := config.New(g) currentBranch, _ := g.CurrentBranch() cfg.SetTrunk(currentBranch) @@ -86,8 +85,8 @@ func TestCreateFromTrackedBranch(t *testing.T) { func TestCreateRejectsUntrackedBranch(t *testing.T) { dir := setupTestRepo(t) - cfg, _ := config.Load(dir) g := git.New(dir) + cfg, _ := config.New(g) trunk, _ := g.CurrentBranch() cfg.SetTrunk(trunk) @@ -119,8 +118,8 @@ func TestCreateRejectsUntrackedBranch(t *testing.T) { func TestCreateWithStagedChanges(t *testing.T) { dir := setupTestRepo(t) - cfg, _ := config.Load(dir) g := git.New(dir) + cfg, _ := config.New(g) trunk, _ := g.CurrentBranch() cfg.SetTrunk(trunk) @@ -151,8 +150,8 @@ func TestCreateWithStagedChanges(t *testing.T) { func TestCreateEmptyWithStagedChanges(t *testing.T) { dir := setupTestRepo(t) - cfg, _ := config.Load(dir) g := git.New(dir) + cfg, _ := config.New(g) trunk, _ := g.CurrentBranch() cfg.SetTrunk(trunk) @@ -197,8 +196,8 @@ func TestBranchAlreadyExists(t *testing.T) { func TestCreateStoresForkPoint(t *testing.T) { dir := setupTestRepo(t) - cfg, _ := config.Load(dir) g := git.New(dir) + cfg, _ := config.New(g) trunk, _ := g.CurrentBranch() cfg.SetTrunk(trunk) diff --git a/cmd/doctor.go b/cmd/doctor.go index 062a2bf..f6a09f5 100644 --- a/cmd/doctor.go +++ b/cmd/doctor.go @@ -43,7 +43,9 @@ func runDoctor(cmd *cobra.Command, args []string) error { return err } - cfg, err := config.Load(cwd) + g := git.New(cwd) + + cfg, err := config.New(g) if err != nil { return err } @@ -51,8 +53,6 @@ func runDoctor(cmd *cobra.Command, args []string) error { if _, err := cfg.GetTrunk(); err != nil { return err } - - g := git.New(cwd) s := style.New() branches, err := cfg.ListTrackedBranches() diff --git a/cmd/init.go b/cmd/init.go index 6953430..e766b46 100644 --- a/cmd/init.go +++ b/cmd/init.go @@ -32,13 +32,13 @@ func runInit(cmd *cobra.Command, args []string) error { return err } - cfg, err := config.Load(cwd) + g := git.New(cwd) + + cfg, err := config.New(g) if err != nil { return err } - g := git.New(cwd) - // Determine trunk branch trunk := trunkFlag if trunk == "" { diff --git a/cmd/init_test.go b/cmd/init_test.go index 6f2b2ff..403fbd2 100644 --- a/cmd/init_test.go +++ b/cmd/init_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/boneskull/gh-stack/internal/config" + "github.com/boneskull/gh-stack/internal/git" ) func setupTestRepo(t *testing.T) string { @@ -37,9 +38,10 @@ func TestInitCommand(t *testing.T) { dir := setupTestRepo(t) // Verify the config package works for init - cfg, err := config.Load(dir) + g := git.New(dir) + cfg, err := config.New(g) if err != nil { - t.Fatalf("Load failed: %v", err) + t.Fatalf("New failed: %v", err) } err = cfg.SetTrunk("main") diff --git a/cmd/link.go b/cmd/link.go index 2d1228e..1728e8f 100644 --- a/cmd/link.go +++ b/cmd/link.go @@ -35,12 +35,12 @@ func runLink(cmd *cobra.Command, args []string) error { return err } - cfg, err := config.Load(cwd) + g := git.New(cwd) + + cfg, err := config.New(g) if err != nil { return err } - - g := git.New(cwd) branch, err := g.CurrentBranch() if err != nil { return err diff --git a/cmd/link_test.go b/cmd/link_test.go index af1ffe1..e2c5ea2 100644 --- a/cmd/link_test.go +++ b/cmd/link_test.go @@ -11,8 +11,8 @@ import ( func TestLinkPR(t *testing.T) { dir := setupTestRepo(t) - cfg, _ := config.Load(dir) g := git.New(dir) + cfg, _ := config.New(g) trunk, _ := g.CurrentBranch() cfg.SetTrunk(trunk) @@ -40,8 +40,8 @@ func TestLinkPR(t *testing.T) { func TestLinkRejectsUntrackedBranch(t *testing.T) { dir := setupTestRepo(t) - cfg, _ := config.Load(dir) g := git.New(dir) + cfg, _ := config.New(g) trunk, _ := g.CurrentBranch() cfg.SetTrunk(trunk) @@ -59,8 +59,8 @@ func TestLinkRejectsUntrackedBranch(t *testing.T) { func TestLinkOverwritesPR(t *testing.T) { dir := setupTestRepo(t) - cfg, _ := config.Load(dir) g := git.New(dir) + cfg, _ := config.New(g) trunk, _ := g.CurrentBranch() cfg.SetTrunk(trunk) diff --git a/cmd/log.go b/cmd/log.go index d53c163..2c248e1 100644 --- a/cmd/log.go +++ b/cmd/log.go @@ -40,7 +40,9 @@ func runLog(cmd *cobra.Command, args []string) error { return err } - cfg, err := config.Load(cwd) + g := git.New(cwd) + + cfg, err := config.New(g) if err != nil { return err } @@ -49,8 +51,6 @@ func runLog(cmd *cobra.Command, args []string) error { if err != nil { return err } - - g := git.New(cwd) currentBranch, _ := g.CurrentBranch() //nolint:errcheck // empty string is fine for display // Try to get GitHub client for PR URLs (optional - may fail if not in a GitHub repo) diff --git a/cmd/log_test.go b/cmd/log_test.go index b39ea73..e1575a3 100644 --- a/cmd/log_test.go +++ b/cmd/log_test.go @@ -5,13 +5,15 @@ import ( "testing" "github.com/boneskull/gh-stack/internal/config" + "github.com/boneskull/gh-stack/internal/git" "github.com/boneskull/gh-stack/internal/tree" ) func TestLogBuildTree(t *testing.T) { dir := setupTestRepo(t) - cfg, _ := config.Load(dir) + g := git.New(dir) + cfg, _ := config.New(g) cfg.SetTrunk("main") cfg.SetParent("feature-a", "main") cfg.SetParent("feature-b", "feature-a") @@ -50,7 +52,8 @@ func TestLogBuildTree(t *testing.T) { func TestLogEmptyStack(t *testing.T) { dir := setupTestRepo(t) - cfg, _ := config.Load(dir) + g := git.New(dir) + cfg, _ := config.New(g) cfg.SetTrunk("main") // No branches tracked, just trunk @@ -70,7 +73,8 @@ func TestLogEmptyStack(t *testing.T) { func TestLogMultipleBranches(t *testing.T) { dir := setupTestRepo(t) - cfg, _ := config.Load(dir) + g := git.New(dir) + cfg, _ := config.New(g) cfg.SetTrunk("main") // Two branches off main cfg.SetParent("feature-a", "main") diff --git a/cmd/orphan.go b/cmd/orphan.go index 66d5aa9..cf4654d 100644 --- a/cmd/orphan.go +++ b/cmd/orphan.go @@ -33,13 +33,13 @@ func runOrphan(cmd *cobra.Command, args []string) error { return err } - cfg, err := config.Load(cwd) + g := git.New(cwd) + + cfg, err := config.New(g) if err != nil { return err } - g := git.New(cwd) - // Determine branch to orphan var branchName string if len(args) > 0 { diff --git a/cmd/orphan_test.go b/cmd/orphan_test.go index 576359b..218a20b 100644 --- a/cmd/orphan_test.go +++ b/cmd/orphan_test.go @@ -12,8 +12,8 @@ import ( func TestOrphanBranch(t *testing.T) { dir := setupTestRepo(t) - cfg, _ := config.Load(dir) g := git.New(dir) + cfg, _ := config.New(g) trunk, _ := g.CurrentBranch() cfg.SetTrunk(trunk) @@ -41,8 +41,8 @@ func TestOrphanBranch(t *testing.T) { func TestOrphanRejectsWithChildren(t *testing.T) { dir := setupTestRepo(t) - cfg, _ := config.Load(dir) g := git.New(dir) + cfg, _ := config.New(g) trunk, _ := g.CurrentBranch() cfg.SetTrunk(trunk) @@ -66,8 +66,8 @@ func TestOrphanRejectsWithChildren(t *testing.T) { func TestOrphanForceWithDescendants(t *testing.T) { dir := setupTestRepo(t) - cfg, _ := config.Load(dir) g := git.New(dir) + cfg, _ := config.New(g) trunk, _ := g.CurrentBranch() cfg.SetTrunk(trunk) @@ -114,8 +114,8 @@ func TestOrphanForceWithDescendants(t *testing.T) { func TestOrphanRemovesPR(t *testing.T) { dir := setupTestRepo(t) - cfg, _ := config.Load(dir) g := git.New(dir) + cfg, _ := config.New(g) trunk, _ := g.CurrentBranch() cfg.SetTrunk(trunk) @@ -145,8 +145,8 @@ func TestOrphanRemovesPR(t *testing.T) { func TestOrphanRemovesForkPoint(t *testing.T) { dir := setupTestRepo(t) - cfg, _ := config.Load(dir) g := git.New(dir) + cfg, _ := config.New(g) trunk, _ := g.CurrentBranch() cfg.SetTrunk(trunk) diff --git a/cmd/submit.go b/cmd/submit.go index 843e7f1..f147a51 100644 --- a/cmd/submit.go +++ b/cmd/submit.go @@ -78,13 +78,13 @@ func runSubmit(cmd *cobra.Command, args []string) error { return err } - cfg, err := config.Load(cwd) + g := git.New(cwd) + + cfg, err := config.New(g) if err != nil { return err } - g := git.New(cwd) - // Check if operation already in progress if state.Exists(g.GetGitDir()) { return errors.New("operation already in progress; use 'gh stack continue' or 'gh stack abort'") diff --git a/cmd/sync.go b/cmd/sync.go index 85b2cc3..a8fd23c 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -96,7 +96,9 @@ func runSync(cmd *cobra.Command, args []string) error { return err } - cfg, err := config.Load(cwd) + g := git.New(cwd) + + cfg, err := config.New(g) if err != nil { return err } @@ -106,8 +108,6 @@ func runSync(cmd *cobra.Command, args []string) error { return err } - g := git.New(cwd) - trunk, err := cfg.GetTrunk() if err != nil { return err diff --git a/cmd/undo.go b/cmd/undo.go index 76ac651..7fd50c2 100644 --- a/cmd/undo.go +++ b/cmd/undo.go @@ -124,7 +124,7 @@ func runUndo(cmd *cobra.Command, args []string) error { } // Load config for restoring stack metadata - cfg, err := config.Load(cwd) + cfg, err := config.New(g) if err != nil { return fmt.Errorf("failed to load config: %w", err) } diff --git a/cmd/unlink.go b/cmd/unlink.go index e5e6194..1ed18e6 100644 --- a/cmd/unlink.go +++ b/cmd/unlink.go @@ -28,12 +28,12 @@ func runUnlink(cmd *cobra.Command, args []string) error { return err } - cfg, err := config.Load(cwd) + g := git.New(cwd) + + cfg, err := config.New(g) if err != nil { return err } - - g := git.New(cwd) branch, err := g.CurrentBranch() if err != nil { return err diff --git a/cmd/unlink_test.go b/cmd/unlink_test.go index c3d23a9..1bad1be 100644 --- a/cmd/unlink_test.go +++ b/cmd/unlink_test.go @@ -11,8 +11,8 @@ import ( func TestUnlinkPR(t *testing.T) { dir := setupTestRepo(t) - cfg, _ := config.Load(dir) g := git.New(dir) + cfg, _ := config.New(g) trunk, _ := g.CurrentBranch() cfg.SetTrunk(trunk) @@ -41,8 +41,8 @@ func TestUnlinkPR(t *testing.T) { func TestUnlinkIdempotent(t *testing.T) { dir := setupTestRepo(t) - cfg, _ := config.Load(dir) g := git.New(dir) + cfg, _ := config.New(g) trunk, _ := g.CurrentBranch() cfg.SetTrunk(trunk) diff --git a/internal/config/config.go b/internal/config/config.go index d5484b2..ac06d0f 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -3,12 +3,12 @@ package config import ( "bufio" - "bytes" "errors" - "os/exec" "regexp" "strconv" "strings" + + "github.com/boneskull/gh-stack/internal/git" ) // ErrNotInitialized is returned when stack tracking is not initialized. @@ -25,77 +25,76 @@ var ErrNoForkPoint = errors.New("no fork point stored for branch") // Config provides access to stack metadata stored in .git/config. type Config struct { - repoPath string + g *git.Git } -// Load creates a Config for the repository at the given path. -func Load(repoPath string) (*Config, error) { - if _, err := exec.Command("git", "-C", repoPath, "rev-parse", "--git-dir").Output(); err != nil { +// New creates a Config that routes all git-config calls through the given Git instance. +func New(g *git.Git) (*Config, error) { + if _, err := g.GetResolvedGitDir(); err != nil { return nil, errors.New("not a git repository") } - return &Config{repoPath: repoPath}, nil + return &Config{g: g}, nil } // GetTrunk returns the configured trunk branch name. func (c *Config) GetTrunk() (string, error) { - out, err := exec.Command("git", "-C", c.repoPath, "config", "--get", "stack.trunk").Output() + out, err := c.g.ConfigGet("stack.trunk") if err != nil { return "", ErrNotInitialized } - return strings.TrimSpace(string(out)), nil + return out, nil } // SetTrunk sets the trunk branch name. func (c *Config) SetTrunk(branch string) error { - cmd := exec.Command("git", "-C", c.repoPath, "config", "stack.trunk", branch) - return cmd.Run() + return c.g.ConfigSet("stack.trunk", branch) } // GetParent returns the parent branch for the given branch. func (c *Config) GetParent(branch string) (string, error) { key := "branch." + branch + ".stackParent" - out, err := exec.Command("git", "-C", c.repoPath, "config", "--get", key).Output() + out, err := c.g.ConfigGet(key) if err != nil { return "", ErrBranchNotTracked } - return strings.TrimSpace(string(out)), nil + return out, nil } // SetParent sets the parent branch for the given branch. func (c *Config) SetParent(branch, parent string) error { key := "branch." + branch + ".stackParent" - return exec.Command("git", "-C", c.repoPath, "config", key, parent).Run() + return c.g.ConfigSet(key, parent) } // RemoveParent removes the parent tracking for a branch. func (c *Config) RemoveParent(branch string) error { key := "branch." + branch + ".stackParent" // --unset returns error if key doesn't exist, which is fine - _ = exec.Command("git", "-C", c.repoPath, "config", "--unset", key).Run() //nolint:errcheck // unset returns error if key missing + _ = c.g.ConfigUnset(key) //nolint:errcheck // unset returns error if key missing return nil } // GetPR returns the PR number for the given branch. func (c *Config) GetPR(branch string) (int, error) { key := "branch." + branch + ".stackPR" - out, err := exec.Command("git", "-C", c.repoPath, "config", "--get", key).Output() + out, err := c.g.ConfigGet(key) if err != nil { return 0, ErrNoPR } - return strconv.Atoi(strings.TrimSpace(string(out))) + return strconv.Atoi(out) } // SetPR sets the PR number for the given branch. func (c *Config) SetPR(branch string, pr int) error { key := "branch." + branch + ".stackPR" - return exec.Command("git", "-C", c.repoPath, "config", key, strconv.Itoa(pr)).Run() + return c.g.ConfigSet(key, strconv.Itoa(pr)) } // RemovePR removes the PR association for a branch. func (c *Config) RemovePR(branch string) error { key := "branch." + branch + ".stackPR" // --unset returns error if key doesn't exist, which is fine - _ = exec.Command("git", "-C", c.repoPath, "config", "--unset", key).Run() //nolint:errcheck // unset returns error if key missing + _ = c.g.ConfigUnset(key) //nolint:errcheck // unset returns error if key missing return nil } @@ -103,17 +102,17 @@ func (c *Config) RemovePR(branch string) error { // The fork point is where the branch originally diverged from its parent. func (c *Config) GetForkPoint(branch string) (string, error) { key := "branch." + branch + ".stackForkPoint" - out, err := exec.Command("git", "-C", c.repoPath, "config", "--get", key).Output() + out, err := c.g.ConfigGet(key) if err != nil { return "", ErrNoForkPoint } - return strings.TrimSpace(string(out)), nil + return out, nil } // SetForkPoint stores the fork point SHA for a branch. func (c *Config) SetForkPoint(branch, sha string) error { key := "branch." + branch + ".stackForkPoint" - return exec.Command("git", "-C", c.repoPath, "config", key, sha).Run() + return c.g.ConfigSet(key, sha) } // SetForkPointWithComment stores the fork point SHA with an inline comment. @@ -122,29 +121,20 @@ func (c *Config) SetForkPoint(branch, sha string) error { // "unknown option" (old git) from other failures. func (c *Config) SetForkPointWithComment(branch, sha, comment string) error { key := "branch." + branch + ".stackForkPoint" - cmd := exec.Command("git", "-C", c.repoPath, "config", "--comment", comment, key, sha) - var stderr bytes.Buffer - cmd.Stderr = &stderr - if err := cmd.Run(); err != nil { - if stderr.Len() > 0 { - return errors.New(strings.TrimSpace(stderr.String())) - } - return err - } - return nil + return c.g.ConfigSetWithComment(key, sha, comment) } // RemoveForkPoint removes the stored fork point for a branch. func (c *Config) RemoveForkPoint(branch string) error { key := "branch." + branch + ".stackForkPoint" - _ = exec.Command("git", "-C", c.repoPath, "config", "--unset", key).Run() //nolint:errcheck // unset returns error if key missing + _ = c.g.ConfigUnset(key) //nolint:errcheck // unset returns error if key missing return nil } // ListTrackedBranches returns all branches that have a stackParent set. func (c *Config) ListTrackedBranches() ([]string, error) { // Note: git normalizes config keys to lowercase, so stackParent becomes stackparent - out, err := exec.Command("git", "-C", c.repoPath, "config", "--get-regexp", "^branch\\..*\\.stackparent$").Output() + out, err := c.g.ConfigGetRegexp(`^branch\..*\.stackparent$`) if err != nil { // No matches is not an error — git config exits non-zero when no keys match return []string{}, nil //nolint:nilerr // non-zero exit = no matching config keys @@ -152,7 +142,7 @@ func (c *Config) ListTrackedBranches() ([]string, error) { var branches []string re := regexp.MustCompile(`^branch\.(.+)\.stackparent\s+`) - scanner := bufio.NewScanner(bytes.NewReader(out)) + scanner := bufio.NewScanner(strings.NewReader(out)) for scanner.Scan() { line := scanner.Text() if matches := re.FindStringSubmatch(line); len(matches) > 1 { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index baff9f5..7adf813 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -10,9 +10,10 @@ import ( "testing" "github.com/boneskull/gh-stack/internal/config" + "github.com/boneskull/gh-stack/internal/git" ) -func setupTestRepo(t *testing.T) string { +func setupTestRepo(t *testing.T) *git.Git { t.Helper() dir := t.TempDir() @@ -27,15 +28,15 @@ func setupTestRepo(t *testing.T) string { exec.Command("git", "-C", dir, "config", "user.email", "test@test.com").Run() exec.Command("git", "-C", dir, "config", "user.name", "Test").Run() - return dir + return git.New(dir) } func TestGetTrunk_NotInitialized(t *testing.T) { - dir := setupTestRepo(t) + g := setupTestRepo(t) - cfg, err := config.Load(dir) + cfg, err := config.New(g) if err != nil { - t.Fatalf("Load failed: %v", err) + t.Fatalf("New failed: %v", err) } _, err = cfg.GetTrunk() @@ -45,11 +46,11 @@ func TestGetTrunk_NotInitialized(t *testing.T) { } func TestSetTrunk(t *testing.T) { - dir := setupTestRepo(t) + g := setupTestRepo(t) - cfg, err := config.Load(dir) + cfg, err := config.New(g) if err != nil { - t.Fatalf("Load failed: %v", err) + t.Fatalf("New failed: %v", err) } if setErr := cfg.SetTrunk("main"); setErr != nil { @@ -66,9 +67,9 @@ func TestSetTrunk(t *testing.T) { } func TestGetParent_NotTracked(t *testing.T) { - dir := setupTestRepo(t) + g := setupTestRepo(t) - cfg, _ := config.Load(dir) + cfg, _ := config.New(g) _, err := cfg.GetParent("feature-a") if !errors.Is(err, config.ErrBranchNotTracked) { @@ -77,9 +78,9 @@ func TestGetParent_NotTracked(t *testing.T) { } func TestSetAndGetParent(t *testing.T) { - dir := setupTestRepo(t) + g := setupTestRepo(t) - cfg, _ := config.Load(dir) + cfg, _ := config.New(g) cfg.SetTrunk("main") if err := cfg.SetParent("feature-a", "main"); err != nil { @@ -96,9 +97,9 @@ func TestSetAndGetParent(t *testing.T) { } func TestPRNumber(t *testing.T) { - dir := setupTestRepo(t) + g := setupTestRepo(t) - cfg, _ := config.Load(dir) + cfg, _ := config.New(g) // No PR set initially _, err := cfg.GetPR("feature-a") @@ -132,9 +133,9 @@ func TestPRNumber(t *testing.T) { } func TestListTrackedBranches(t *testing.T) { - dir := setupTestRepo(t) + g := setupTestRepo(t) - cfg, _ := config.Load(dir) + cfg, _ := config.New(g) cfg.SetTrunk("main") cfg.SetParent("feature-a", "main") cfg.SetParent("feature-b", "feature-a") @@ -159,8 +160,8 @@ func TestListTrackedBranches(t *testing.T) { } func TestForkPoint(t *testing.T) { - dir := setupTestRepo(t) - cfg, _ := config.Load(dir) + g := setupTestRepo(t) + cfg, _ := config.New(g) // Initially no fork point _, err := cfg.GetForkPoint("feature") @@ -196,8 +197,8 @@ func TestForkPoint(t *testing.T) { } func TestSetForkPointWithComment(t *testing.T) { - dir := setupTestRepo(t) - cfg, _ := config.Load(dir) + g := setupTestRepo(t) + cfg, _ := config.New(g) sha := "abc123def456" comment := "replaces deadbee (2026-02-14T00:00:00Z)" @@ -217,7 +218,11 @@ func TestSetForkPointWithComment(t *testing.T) { } // The raw config file should contain the inline comment - configPath := filepath.Join(dir, ".git", "config") + gitDir, err := g.GetResolvedGitDir() + if err != nil { + t.Fatalf("failed to resolve git dir: %v", err) + } + configPath := filepath.Join(gitDir, "config") data, err := os.ReadFile(configPath) if err != nil { t.Fatalf("failed to read .git/config: %v", err) diff --git a/internal/git/git.go b/internal/git/git.go index 25b4681..c3c8388 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -553,6 +553,35 @@ func (g *Git) GetForkPoint(parent, branch string) (string, error) { return g.run("merge-base", "--fork-point", parent, branch) } +// ConfigGet returns the value of a git config key. +// Returns an error if the key does not exist. +func (g *Git) ConfigGet(key string) (string, error) { + return g.run("config", "--get", key) +} + +// ConfigSet sets a git config key to a value. +func (g *Git) ConfigSet(key, value string) error { + return g.runSilent("config", key, value) +} + +// ConfigSetWithComment sets a git config key to a value with an inline comment. +// Requires Git 2.45+. Returns an error with stderr content on failure, so +// callers can distinguish "unknown option" (old git) from other failures. +func (g *Git) ConfigSetWithComment(key, value, comment string) error { + return g.runSilent("config", "--comment", comment, key, value) +} + +// ConfigUnset removes a git config key. Returns an error if the key does not exist. +func (g *Git) ConfigUnset(key string) error { + return g.runSilent("config", "--unset", key) +} + +// ConfigGetRegexp returns the raw output of `git config --get-regexp`. +// Returns an error if no keys match. +func (g *Git) ConfigGetRegexp(pattern string) (string, error) { + return g.run("config", "--get-regexp", pattern) +} + // AbbrevSHA safely abbreviates a SHA to 7 characters. // Returns the full string if it's shorter than 7 characters. func AbbrevSHA(sha string) string { diff --git a/internal/health/health_test.go b/internal/health/health_test.go index bd3acbe..2189eba 100644 --- a/internal/health/health_test.go +++ b/internal/health/health_test.go @@ -39,9 +39,9 @@ func setupTestRepo(t *testing.T) (string, *git.Git, *config.Config) { run("commit", "-m", "initial") g := git.New(dir) - cfg, err := config.Load(dir) + cfg, err := config.New(g) if err != nil { - t.Fatalf("config.Load failed: %v", err) + t.Fatalf("config.New failed: %v", err) } trunk, err := g.CurrentBranch() diff --git a/internal/tree/tree_test.go b/internal/tree/tree_test.go index 2c9be2a..7c2c6f1 100644 --- a/internal/tree/tree_test.go +++ b/internal/tree/tree_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/boneskull/gh-stack/internal/config" + "github.com/boneskull/gh-stack/internal/git" "github.com/boneskull/gh-stack/internal/tree" ) @@ -17,7 +18,8 @@ func setupTestRepo(t *testing.T) (*config.Config, string) { exec.Command("git", "-C", dir, "config", "user.email", "test@test.com").Run() exec.Command("git", "-C", dir, "config", "user.name", "Test").Run() - cfg, _ := config.Load(dir) + g := git.New(dir) + cfg, _ := config.New(g) return cfg, dir }