From 6068437ba480c020a04896f9cd272d60d84eb4cf Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Thu, 13 Aug 2026 00:13:14 -0700 Subject: [PATCH 1/3] fix: bound stalled git commands with actionable diagnostics Fixes #64 --- internal/git/git.go | 69 ++++++++++++-- internal/git/git_test.go | 192 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 252 insertions(+), 9 deletions(-) diff --git a/internal/git/git.go b/internal/git/git.go index 8b876b9..537d8b0 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -1,11 +1,20 @@ package git import ( + "context" "crypto/sha256" + "errors" "fmt" + "os" "os/exec" "path/filepath" "strings" + "time" +) + +const ( + gitCommandTimeout = 2 * time.Minute + gitCommandWaitDelay = 250 * time.Millisecond ) func FindRepoRoot() (string, error) { @@ -259,16 +268,25 @@ func IsHeadMergedIntoDefault(repoRoot, worktreePath string) (bool, string, error // IsHeadMergedIntoRef reports whether worktreePath's HEAD is an ancestor of ref. func IsHeadMergedIntoRef(worktreePath, ref string) (bool, error) { - cmd := exec.Command("git", "merge-base", "--is-ancestor", "HEAD", ref) - cmd.Dir = worktreePath - out, err := cmd.CombinedOutput() + ctx, cancel := context.WithTimeout(context.Background(), gitCommandTimeout) + defer cancel() + + return isHeadMergedIntoRefContext(ctx, worktreePath, ref) +} + +func isHeadMergedIntoRefContext(ctx context.Context, worktreePath, ref string) (bool, error) { + args := []string{"merge-base", "--is-ancestor", "HEAD", ref} + out, err := gitCommandContext(ctx, worktreePath, args...).CombinedOutput() if err == nil { return true, nil } + if errors.Is(ctx.Err(), context.DeadlineExceeded) { + return false, gitTimeoutError(worktreePath, args) + } if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 1 { return false, nil } - return false, fmt.Errorf("git merge-base --is-ancestor HEAD %s: %s", ref, strings.TrimSpace(string(out))) + return false, fmt.Errorf("git %s: %s", strings.Join(args, " "), strings.TrimSpace(string(out))) } // IsDirty reports tracked or untracked changes, ignoring status.showUntrackedFiles. @@ -286,12 +304,18 @@ func ShortHash(s string) string { } func runGit(dir string, args ...string) (string, error) { - cmd := exec.Command("git", args...) - if dir != "" { - cmd.Dir = dir - } - out, err := cmd.Output() + ctx, cancel := context.WithTimeout(context.Background(), gitCommandTimeout) + defer cancel() + + return runGitContext(ctx, dir, args...) +} + +func runGitContext(ctx context.Context, dir string, args ...string) (string, error) { + out, err := gitCommandContext(ctx, dir, args...).Output() if err != nil { + if errors.Is(ctx.Err(), context.DeadlineExceeded) { + return "", gitTimeoutError(dir, args) + } if exitErr, ok := err.(*exec.ExitError); ok { return "", fmt.Errorf("git %s: %s", strings.Join(args, " "), strings.TrimSpace(string(exitErr.Stderr))) } @@ -299,3 +323,30 @@ func runGit(dir string, args ...string) (string, error) { } return strings.TrimSpace(string(out)), nil } + +func gitCommandContext(ctx context.Context, dir string, args ...string) *exec.Cmd { + cmd := exec.CommandContext(ctx, "git", args...) + cmd.WaitDelay = gitCommandWaitDelay + if dir != "" { + cmd.Dir = dir + } + return cmd +} + +func gitTimeoutError(dir string, args []string) error { + workingDir := dir + if workingDir == "" { + if currentDir, err := os.Getwd(); err == nil { + workingDir = currentDir + } else { + workingDir = "." + } + } + lockPath := filepath.Join(".git", "index.lock") + return fmt.Errorf( + "git %s timed out in %q; check for a stale %s, blocked credential prompts, or network connectivity", + strings.Join(args, " "), + workingDir, + lockPath, + ) +} diff --git a/internal/git/git_test.go b/internal/git/git_test.go index d8358f7..1f41c71 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -1,13 +1,205 @@ package git import ( + "context" "os" "os/exec" "path/filepath" "strings" "testing" + "time" ) +func TestRunGitContextPreservesNormalOutputAndExitDiagnostics(t *testing.T) { + repoDir := t.TempDir() + repoDir, err := filepath.EvalSymlinks(repoDir) + if err != nil { + t.Fatal(err) + } + mustGit(t, "", "init", "--initial-branch=main", repoDir) + + out, err := runGitContext(context.Background(), repoDir, "rev-parse", "--show-toplevel") + if err != nil { + t.Fatalf("runGitContext failed: %v", err) + } + if out != repoDir { + t.Fatalf("expected trimmed repository path %q, got %q", repoDir, out) + } + + _, err = runGitContext(context.Background(), repoDir, "rev-parse", "--verify", "missing-ref") + if err == nil { + t.Fatal("expected missing ref to fail") + } + if !strings.Contains(err.Error(), "git rev-parse --verify missing-ref:") { + t.Fatalf("expected ordinary git exit diagnostic, got %q", err) + } +} + +func TestRunGitContextReportsActionableTimeout(t *testing.T) { + repoDir := t.TempDir() + ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(-time.Second)) + defer cancel() + + _, err := runGitContext(ctx, repoDir, "checkout", "--detach") + if err == nil { + t.Fatal("expected expired context to fail") + } + + message := err.Error() + for _, want := range []string{ + "git checkout --detach timed out", + repoDir, + filepath.Join(".git", "index.lock"), + "credential", + "network", + } { + if !strings.Contains(message, want) { + t.Errorf("expected timeout diagnostic to contain %q, got %q", want, message) + } + } +} + +func TestRunGitContextDoesNotReportCancellationAsTimeout(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + _, err := runGitContext(ctx, t.TempDir(), "status") + if err == nil { + t.Fatal("expected canceled context to fail") + } + if strings.Contains(err.Error(), "timed out") { + t.Fatalf("expected cancellation to remain distinct from timeout, got %q", err) + } +} + +func TestRunGitContextBoundsDescendantHeldOutputPipe(t *testing.T) { + helperDir := t.TempDir() + helperName := "git-pipeholder" + if filepath.Ext(os.Args[0]) == ".exe" { + helperName += ".exe" + } + helperPath := filepath.Join(helperDir, helperName) + testBinary, err := os.ReadFile(os.Args[0]) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(helperPath, testBinary, 0o755); err != nil { + t.Fatal(err) + } + + readyPath := filepath.Join(t.TempDir(), "ready") + stopPath := filepath.Join(t.TempDir(), "stop") + donePath := filepath.Join(t.TempDir(), "done") + t.Setenv("TREEHOUSE_GIT_PIPE_HOLDER", "1") + t.Setenv("TREEHOUSE_GIT_PIPE_READY", readyPath) + t.Setenv("TREEHOUSE_GIT_PIPE_STOP", stopPath) + t.Setenv("TREEHOUSE_GIT_PIPE_DONE", donePath) + defer os.WriteFile(stopPath, nil, 0o644) //nolint:errcheck -- best-effort helper cleanup + + ctx, cancel := context.WithTimeout(context.Background(), 300*time.Millisecond) + defer cancel() + started := time.Now() + _, err = runGitContext(ctx, "", "--exec-path="+helperDir, "pipeholder", "-test.run=^TestGitPipeHolderHelper$") + elapsed := time.Since(started) + if writeErr := os.WriteFile(stopPath, nil, 0o644); writeErr != nil { + t.Fatal(writeErr) + } + if !waitForFile(donePath, 2*time.Second) { + t.Error("git helper did not exit after its output pipe was released") + } + if err == nil { + t.Fatal("expected pipe-holding git command to time out") + } + if !strings.Contains(err.Error(), "timed out") { + t.Fatalf("expected timeout diagnostic, got %q", err) + } + if _, statErr := os.Stat(readyPath); statErr != nil { + t.Fatalf("expected git helper and its descendant to start: %v", statErr) + } + if elapsed >= 2*time.Second { + t.Fatalf("runGitContext remained blocked by an inherited output pipe for %v", elapsed) + } +} + +func TestGitPipeHolderHelper(t *testing.T) { + if os.Getenv("TREEHOUSE_GIT_PIPE_HOLDER") != "1" { + return + } + + stopPath := os.Getenv("TREEHOUSE_GIT_PIPE_STOP") + if os.Getenv("TREEHOUSE_GIT_PIPE_DESCENDANT") == "1" { + waitForFile(stopPath, 15*time.Second) + return + } + + cmd := exec.Command(os.Args[0], "-test.run=^TestGitPipeHolderHelper$") + cmd.Env = append(os.Environ(), "TREEHOUSE_GIT_PIPE_DESCENDANT=1") + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(os.Getenv("TREEHOUSE_GIT_PIPE_READY"), nil, 0o644); err != nil { + t.Fatal(err) + } + waitForFile(stopPath, 15*time.Second) + _ = cmd.Wait() // The command's inherited output pipe is intentionally closed on timeout. + if err := os.WriteFile(os.Getenv("TREEHOUSE_GIT_PIPE_DONE"), nil, 0o644); err != nil { + t.Fatal(err) + } +} + +func waitForFile(path string, timeout time.Duration) bool { + deadline := time.Now().Add(timeout) + for time.Now().Before(deadline) { + if _, err := os.Stat(path); err == nil { + return true + } + time.Sleep(10 * time.Millisecond) + } + return false +} + +func TestIsHeadMergedIntoRefPreservesUnmergedExitCode(t *testing.T) { + repoDir := t.TempDir() + mustGit(t, "", "init", "--initial-branch=main", repoDir) + mustGit(t, repoDir, "config", "user.email", "test@test.com") + mustGit(t, repoDir, "config", "user.name", "Test") + if err := os.WriteFile(filepath.Join(repoDir, "README.md"), []byte("main\n"), 0o644); err != nil { + t.Fatal(err) + } + mustGit(t, repoDir, "add", "README.md") + mustGit(t, repoDir, "commit", "-m", "main") + mustGit(t, repoDir, "checkout", "-b", "feature") + if err := os.WriteFile(filepath.Join(repoDir, "feature.txt"), []byte("feature\n"), 0o644); err != nil { + t.Fatal(err) + } + mustGit(t, repoDir, "add", "feature.txt") + mustGit(t, repoDir, "commit", "-m", "feature") + + merged, err := IsHeadMergedIntoRef(repoDir, "refs/heads/main") + if err != nil { + t.Fatalf("IsHeadMergedIntoRef failed: %v", err) + } + if merged { + t.Fatal("expected feature HEAD not to be merged into main") + } +} + +func TestIsHeadMergedIntoRefContextReportsTimeout(t *testing.T) { + repoDir := t.TempDir() + ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(-time.Second)) + defer cancel() + + _, err := isHeadMergedIntoRefContext(ctx, repoDir, "refs/heads/main") + if err == nil { + t.Fatal("expected expired context to fail") + } + if !strings.Contains(err.Error(), "git merge-base --is-ancestor HEAD refs/heads/main timed out") { + t.Fatalf("expected merge-base timeout diagnostic, got %q", err) + } +} + func TestRepoRootFromCommonGitDirHandlesForwardSlashPath(t *testing.T) { root, ok := repoRootFromCommonGitDir("C:/Users/runner/AppData/Local/Temp/repo/.git") if !ok { From 8d4432de2d2d99e0448086775b6c82c3fbddcdd8 Mon Sep 17 00:00:00 2001 From: Matt Van Horn Date: Thu, 20 Aug 2026 08:03:41 -0700 Subject: [PATCH 2/3] fix(git): make timeout diagnostic and toplevel test Windows-correct Two Windows-only failures in test (windows-latest): - gitTimeoutError formatted the working directory with %q, so a Windows path came out double-escaped (C:\\Users\\...). That is wrong in the message a user actually reads, not just in the assertion. Quote it explicitly and format with %s so the path renders natively on every platform. - git reports rev-parse --show-toplevel with forward slashes even on Windows, so comparing it against a native t.TempDir() path never matched. Normalize with filepath.FromSlash before comparing; this is identity on Unix. --- internal/git/git.go | 2 +- internal/git/git_test.go | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/git/git.go b/internal/git/git.go index 537d8b0..6cbd8bb 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -344,7 +344,7 @@ func gitTimeoutError(dir string, args []string) error { } lockPath := filepath.Join(".git", "index.lock") return fmt.Errorf( - "git %s timed out in %q; check for a stale %s, blocked credential prompts, or network connectivity", + "git %s timed out in \"%s\"; check for a stale %s, blocked credential prompts, or network connectivity", strings.Join(args, " "), workingDir, lockPath, diff --git a/internal/git/git_test.go b/internal/git/git_test.go index 1f41c71..63e50f6 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -22,7 +22,8 @@ func TestRunGitContextPreservesNormalOutputAndExitDiagnostics(t *testing.T) { if err != nil { t.Fatalf("runGitContext failed: %v", err) } - if out != repoDir { + // git reports --show-toplevel with forward slashes even on Windows. + if filepath.FromSlash(out) != repoDir { t.Fatalf("expected trimmed repository path %q, got %q", repoDir, out) } From 0fb398c7e8d06353f19ae3dccf5d3228fe97170c Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Fri, 21 Aug 2026 05:42:46 -0700 Subject: [PATCH 3/3] fix: propagate context through merge-base and read-tree git calls --- internal/git/git.go | 26 +++++++++++++------------- internal/git/git_test.go | 16 +++++++++++++++- 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/internal/git/git.go b/internal/git/git.go index 7d81964..d5fa536 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -324,16 +324,18 @@ func isHeadMergedIntoRefContext(ctx context.Context, worktreePath, ref string) ( return false, gitTimeoutError(worktreePath, args) } if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 1 { - return isHeadContentMergedIntoRef(worktreePath, ref) + return isHeadContentMergedIntoRefContext(ctx, worktreePath, ref) } return false, fmt.Errorf("git %s: %s", strings.Join(args, " "), strings.TrimSpace(string(out))) } -func isHeadContentMergedIntoRef(worktreePath, ref string) (bool, error) { - cmd := exec.Command("git", "merge-base", "HEAD", ref) - cmd.Dir = worktreePath - out, err := cmd.CombinedOutput() +func isHeadContentMergedIntoRefContext(ctx context.Context, worktreePath, ref string) (bool, error) { + args := []string{"merge-base", "HEAD", ref} + out, err := gitCommandContext(ctx, worktreePath, args...).CombinedOutput() if err != nil { + if errors.Is(ctx.Err(), context.DeadlineExceeded) { + return false, gitTimeoutError(worktreePath, args) + } if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 1 { return false, fmt.Errorf("git merge-base HEAD %s returned no common ancestor", ref) } @@ -344,15 +346,15 @@ func isHeadContentMergedIntoRef(worktreePath, ref string) (bool, error) { return false, fmt.Errorf("git merge-base HEAD %s returned no common ancestor", ref) } - baseTree, err := readTree(worktreePath, base) + baseTree, err := readTreeContext(ctx, worktreePath, base) if err != nil { return false, err } - headTree, err := readTree(worktreePath, "HEAD") + headTree, err := readTreeContext(ctx, worktreePath, "HEAD") if err != nil { return false, err } - targetTree, err := readTree(worktreePath, ref) + targetTree, err := readTreeContext(ctx, worktreePath, ref) if err != nil { return false, err } @@ -382,8 +384,8 @@ func isHeadContentMergedIntoRef(worktreePath, ref string) (bool, error) { return true, nil } -func readTree(repoRoot, ref string) (map[string]string, error) { - out, err := runGitRaw(repoRoot, "ls-tree", "-r", "-z", "--full-tree", ref) +func readTreeContext(ctx context.Context, repoRoot, ref string) (map[string]string, error) { + out, err := runGitRawContext(ctx, repoRoot, "ls-tree", "-r", "-z", "--full-tree", ref) if err != nil { return nil, err } @@ -479,11 +481,9 @@ func gitTimeoutError(dir string, args []string) error { workingDir = "." } } - lockPath := filepath.Join(".git", "index.lock") return fmt.Errorf( - "git %s timed out in \"%s\"; check for a stale %s, blocked credential prompts, or network connectivity", + "git %s timed out in \"%s\"; check for a stale index lock (locate it with 'git rev-parse --git-path index.lock'), blocked credential prompts, or network connectivity", strings.Join(args, " "), workingDir, - lockPath, ) } diff --git a/internal/git/git_test.go b/internal/git/git_test.go index 9703471..4ce9bc3 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -50,7 +50,7 @@ func TestRunGitContextReportsActionableTimeout(t *testing.T) { for _, want := range []string{ "git checkout --detach timed out", repoDir, - filepath.Join(".git", "index.lock"), + "git rev-parse --git-path index.lock", "credential", "network", } { @@ -201,6 +201,20 @@ func TestIsHeadMergedIntoRefContextReportsTimeout(t *testing.T) { } } +func TestIsHeadContentMergedIntoRefContextReportsTimeout(t *testing.T) { + repoDir := t.TempDir() + ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(-time.Second)) + defer cancel() + + _, err := isHeadContentMergedIntoRefContext(ctx, repoDir, "refs/heads/main") + if err == nil { + t.Fatal("expected expired context to fail") + } + if !strings.Contains(err.Error(), "git merge-base HEAD refs/heads/main timed out") { + t.Fatalf("expected fallback merge-base timeout diagnostic, got %q", err) + } +} + func TestRepoRootFromCommonGitDirHandlesForwardSlashPath(t *testing.T) { root, ok := repoRootFromCommonGitDir("C:/Users/runner/AppData/Local/Temp/repo/.git") if !ok {