From 81a8cc3be3e387d5ffbdd5fd41a6745ca8c10afd Mon Sep 17 00:00:00 2001 From: Alexandre Ferrreira Date: Tue, 19 May 2026 11:32:23 -0300 Subject: [PATCH] fix: allow checkout with non-conflicting dirty tracked files gitm checkout was unconditionally skipping repos with any modified tracked files, even when git itself would allow the branch switch (e.g. a modified file that is identical on both branches). This made gitm stricter than native git and blocked common workflows like switching branches with a locally-modified .npmrc. Instead of pre-checking dirty state, attempt the checkout and only skip when git reports an actual conflict ("Your local changes would be overwritten"). This delegates conflict detection to git's own merge logic. Co-Authored-By: Claude Opus 4.6 --- internal/cli/checkout.go | 63 ++++----- internal/cli/checkout_run_test.go | 219 ++++++++++++++++++++++++++++++ internal/e2e/checkout_test.go | 40 +++++- 3 files changed, 282 insertions(+), 40 deletions(-) diff --git a/internal/cli/checkout.go b/internal/cli/checkout.go index b843a4f..908fd10 100644 --- a/internal/cli/checkout.go +++ b/internal/cli/checkout.go @@ -38,7 +38,7 @@ Three modes of operation: Checks out in ALL repos where it exists. Repos where the branch is not found are skipped with a warning. -Repositories with uncommitted tracked changes are always skipped. +Repositories are skipped when uncommitted changes conflict with the target branch. Use --repo to limit the operation to specific repositories by alias.`, Example: ` gitm checkout @@ -94,23 +94,10 @@ func runCheckoutDefault(repos []*db.Repository) error { fmt.Printf("Checking out default branch and pulling for %d repositories…\n\n", len(repos)) results := runner.Run(repos, func(repo *db.Repository) (string, string, error) { - dirty, err := git.IsDirtyTrackedOnly(repo.Path) - if err != nil { - return "", "", fmt.Errorf("git status failed: %w", err) - } - if dirty { - files, filesErr := git.DirtyFiles(repo.Path) - if filesErr != nil { - return "", "", fmt.Errorf("list dirty files: %w", filesErr) - } - reason := fmt.Sprintf("uncommitted changes (%d file(s))", len(files)) - if len(files) > 0 && len(files) <= 3 { - reason += ": " + strings.Join(files, ", ") - } - return "", reason, nil - } - if checkoutErr := git.Checkout(repo.Path, repo.DefaultBranch); checkoutErr != nil { + if skip, reason := checkoutConflictSkip(repo.Path, checkoutErr); skip { + return "", reason, nil + } return "", "", fmt.Errorf("checkout %s: %w", repo.DefaultBranch, checkoutErr) } @@ -173,23 +160,6 @@ func runCheckoutInteractive(repos []*db.Repository, ui ui) error { // checkoutBranchInRepo performs the dirty check, branch-existence check, // checkout, and pull for a single repo. Shared by both specific and interactive modes. func checkoutBranchInRepo(repo *db.Repository, branch string) (string, string, error) { - // Skip if tracked files are dirty. - dirty, err := git.IsDirtyTrackedOnly(repo.Path) - if err != nil { - return "", "", fmt.Errorf("git status failed: %w", err) - } - if dirty { - files, filesErr := git.DirtyFiles(repo.Path) - if filesErr != nil { - return "", "", fmt.Errorf("list dirty files: %w", filesErr) - } - reason := fmt.Sprintf("uncommitted changes (%d file(s))", len(files)) - if len(files) > 0 && len(files) <= 3 { - reason += ": " + strings.Join(files, ", ") - } - return "", reason, nil - } - // Check branch existence: local first, then remote. localExists := git.BranchExists(repo.Path, branch) remoteExists := false @@ -210,6 +180,9 @@ func checkoutBranchInRepo(repo *db.Repository, branch string) (string, string, e } if checkoutErr := git.Checkout(repo.Path, branch); checkoutErr != nil { + if skip, reason := checkoutConflictSkip(repo.Path, checkoutErr); skip { + return "", reason, nil + } return "", "", fmt.Errorf("checkout: %w", checkoutErr) } @@ -221,6 +194,28 @@ func checkoutBranchInRepo(repo *db.Repository, branch string) (string, string, e return fmt.Sprintf("on %s — %s", branch, summarisePull(out)), "", nil } +func isCheckoutConflict(err error) bool { + msg := err.Error() + return strings.Contains(msg, "Your local changes") || + strings.Contains(msg, "overwritten by checkout") || + strings.Contains(msg, "overwritten by merge") +} + +func checkoutConflictSkip(repoPath string, checkoutErr error) (bool, string) { + if !isCheckoutConflict(checkoutErr) { + return false, "" + } + files, filesErr := git.DirtyFiles(repoPath) + if filesErr != nil { + return true, "uncommitted changes conflict with target branch" + } + reason := fmt.Sprintf("uncommitted changes conflict (%d file(s))", len(files)) + if len(files) > 0 && len(files) <= 3 { + reason += ": " + strings.Join(files, ", ") + } + return true, reason +} + // summarisePull condenses git pull output into a short message. func summarisePull(out string) string { out = strings.TrimSpace(out) diff --git a/internal/cli/checkout_run_test.go b/internal/cli/checkout_run_test.go index 43827d4..34a9ac3 100644 --- a/internal/cli/checkout_run_test.go +++ b/internal/cli/checkout_run_test.go @@ -1,6 +1,7 @@ package cli import ( + "fmt" "os" "path/filepath" "strings" @@ -286,3 +287,221 @@ func TestRunCheckoutDefault_ReturnsErrorOnFailure(t *testing.T) { t.Errorf("error = %q, want to contain \"failed to checkout\"", err.Error()) } } + +func TestRunCheckoutDefault_NonConflictingDirtyFile(t *testing.T) { + database = setupTestDB(t) + dir, _, _ := initRepoWithRemote(t) + + writeFile(t, dir, "config.txt", "original\n") + mustRunGit(t, dir, "add", "config.txt") + mustRunGit(t, dir, "commit", "-m", "add config") + mustRunGit(t, dir, "push") + + mustRunGit(t, dir, "checkout", "-b", "feature") + mustRunGit(t, dir, "push", "--set-upstream", "origin", "feature") + mustRunGit(t, dir, "commit", "--allow-empty", "-m", "feature work") + + writeFile(t, dir, "config.txt", "local modification\n") + + repo := &db.Repository{ID: 1, Alias: "repo1", Path: dir, DefaultBranch: "main"} + if err := runCheckoutDefault([]*db.Repository{repo}); err != nil { + t.Fatalf("should succeed with non-conflicting dirty file: %v", err) + } + + if head := gitCurrentBranch(t, dir); head != "main" { + t.Fatalf("head = %q, want main", head) + } +} + +func TestRunCheckoutDefault_ConflictingDirtyFile(t *testing.T) { + database = setupTestDB(t) + dir := initRepo(t) + + writeFile(t, dir, "conflict.txt", "main-version\n") + mustRunGit(t, dir, "add", "conflict.txt") + mustRunGit(t, dir, "commit", "-m", "add conflict on main") + + mustRunGit(t, dir, "checkout", "-b", "feature") + writeFile(t, dir, "conflict.txt", "feature-version\n") + mustRunGit(t, dir, "add", "conflict.txt") + mustRunGit(t, dir, "commit", "-m", "change conflict on feature") + + writeFile(t, dir, "conflict.txt", "local dirty\n") + + repo := &db.Repository{ID: 1, Alias: "repo1", Path: dir, DefaultBranch: "main"} + if err := runCheckoutDefault([]*db.Repository{repo}); err != nil { + t.Fatalf("should skip (not error) for conflicting dirty file: %v", err) + } + + if head := gitCurrentBranch(t, dir); head != "feature" { + t.Fatalf("should stay on feature (skipped), got %s", head) + } +} + +func TestCheckoutBranchInRepo_NonConflictingDirtyFile(t *testing.T) { + dir, _, _ := initRepoWithRemote(t) + + writeFile(t, dir, "safe.txt", "original\n") + mustRunGit(t, dir, "add", "safe.txt") + mustRunGit(t, dir, "commit", "-m", "add safe file") + mustRunGit(t, dir, "push") + + mustRunGit(t, dir, "checkout", "-b", "target") + mustRunGit(t, dir, "push", "--set-upstream", "origin", "target") + mustRunGit(t, dir, "commit", "--allow-empty", "-m", "target commit") + mustRunGit(t, dir, "checkout", "main") + + writeFile(t, dir, "safe.txt", "dirty but safe\n") + + repo := &db.Repository{ID: 1, Alias: "repo1", Path: dir, DefaultBranch: "main"} + msg, skipReason, err := checkoutBranchInRepo(repo, "target") + if err != nil { + t.Fatalf("should succeed: %v", err) + } + if skipReason != "" { + t.Fatalf("should not skip, got: %s", skipReason) + } + if msg == "" { + t.Fatal("expected success message") + } + + if head := gitCurrentBranch(t, dir); head != "target" { + t.Fatalf("head = %q, want target", head) + } +} + +func TestCheckoutBranchInRepo_ConflictingDirtyFile(t *testing.T) { + dir := initRepo(t) + + writeFile(t, dir, "conflict.txt", "main-version\n") + mustRunGit(t, dir, "add", "conflict.txt") + mustRunGit(t, dir, "commit", "-m", "add conflict on main") + + mustRunGit(t, dir, "checkout", "-b", "target") + writeFile(t, dir, "conflict.txt", "target-version\n") + mustRunGit(t, dir, "add", "conflict.txt") + mustRunGit(t, dir, "commit", "-m", "change conflict on target") + + mustRunGit(t, dir, "checkout", "main") + writeFile(t, dir, "conflict.txt", "local dirty\n") + + repo := &db.Repository{ID: 1, Alias: "repo1", Path: dir, DefaultBranch: "main"} + _, skipReason, err := checkoutBranchInRepo(repo, "target") + if err != nil { + t.Fatalf("should skip, not error: %v", err) + } + if skipReason == "" { + t.Fatal("expected skip reason for conflicting dirty file") + } + if !strings.Contains(skipReason, "conflict") { + t.Errorf("skip reason = %q, want to contain 'conflict'", skipReason) + } + + if head := gitCurrentBranch(t, dir); head != "main" { + t.Fatalf("should stay on main (skipped), got %s", head) + } +} + +func TestCheckoutInteractive_NonConflictingDirtyFile(t *testing.T) { + database = setupTestDB(t) + dir, _, _ := initRepoWithRemote(t) + + writeFile(t, dir, "tracked.txt", "original\n") + mustRunGit(t, dir, "add", "tracked.txt") + mustRunGit(t, dir, "commit", "-m", "add tracked file") + mustRunGit(t, dir, "push") + + mustRunGit(t, dir, "checkout", "-b", "target") + mustRunGit(t, dir, "push", "--set-upstream", "origin", "target") + mustRunGit(t, dir, "commit", "--allow-empty", "-m", "target commit") + mustRunGit(t, dir, "checkout", "main") + + writeFile(t, dir, "tracked.txt", "dirty but non-conflicting\n") + + repo := &db.Repository{ID: 1, Alias: "repo1", Path: dir, DefaultBranch: "main"} + ui := fakeUI{selectRepos: []*db.Repository{repo}, branchName: "target"} + + if err := runCheckoutInteractive([]*db.Repository{repo}, ui); err != nil { + t.Fatalf("interactive checkout should succeed with non-conflicting dirty: %v", err) + } + + if head := gitCurrentBranch(t, dir); head != "target" { + t.Fatalf("head = %q, want target", head) + } +} + +func TestCheckoutDefault_MultipleReposMixedDirtyState(t *testing.T) { + database = setupTestDB(t) + + cleanDir, _, _ := initRepoWithRemote(t) + mustRunGit(t, cleanDir, "checkout", "-b", "feature") + mustRunGit(t, cleanDir, "push", "--set-upstream", "origin", "feature") + mustRunGit(t, cleanDir, "commit", "--allow-empty", "-m", "feature") + + conflictDir, _, _ := initRepoWithRemote(t) + writeFile(t, conflictDir, "conflict.txt", "v1\n") + mustRunGit(t, conflictDir, "add", "conflict.txt") + mustRunGit(t, conflictDir, "commit", "-m", "add conflict") + mustRunGit(t, conflictDir, "push") + mustRunGit(t, conflictDir, "checkout", "-b", "feature") + mustRunGit(t, conflictDir, "push", "--set-upstream", "origin", "feature") + writeFile(t, conflictDir, "conflict.txt", "v2\n") + mustRunGit(t, conflictDir, "add", "conflict.txt") + mustRunGit(t, conflictDir, "commit", "-m", "change conflict") + writeFile(t, conflictDir, "conflict.txt", "local dirty\n") + + nonConflictDir, _, _ := initRepoWithRemote(t) + writeFile(t, nonConflictDir, "safe.txt", "original\n") + mustRunGit(t, nonConflictDir, "add", "safe.txt") + mustRunGit(t, nonConflictDir, "commit", "-m", "add safe") + mustRunGit(t, nonConflictDir, "push") + mustRunGit(t, nonConflictDir, "checkout", "-b", "feature") + mustRunGit(t, nonConflictDir, "push", "--set-upstream", "origin", "feature") + mustRunGit(t, nonConflictDir, "commit", "--allow-empty", "-m", "feature") + writeFile(t, nonConflictDir, "safe.txt", "dirty but safe\n") + + repos := []*db.Repository{ + {ID: 1, Alias: "clean", Path: cleanDir, DefaultBranch: "main"}, + {ID: 2, Alias: "conflict", Path: conflictDir, DefaultBranch: "main"}, + {ID: 3, Alias: "non-conflict", Path: nonConflictDir, DefaultBranch: "main"}, + } + + if err := runCheckoutDefault(repos); err != nil { + t.Fatalf("runCheckoutDefault failed: %v", err) + } + + if b := gitCurrentBranch(t, cleanDir); b != "main" { + t.Errorf("clean repo: expected main, got %s", b) + } + if b := gitCurrentBranch(t, conflictDir); b != "feature" { + t.Errorf("conflict repo: expected to stay on feature, got %s", b) + } + if b := gitCurrentBranch(t, nonConflictDir); b != "main" { + t.Errorf("non-conflict repo: expected main (dirty carried forward), got %s", b) + } +} + +func TestIsCheckoutConflict(t *testing.T) { + tests := []struct { + msg string + want bool + }{ + {"error: Your local changes to the following files would be overwritten by checkout", true}, + {"error: Your local changes to the following files would be overwritten by merge", true}, + {"error: pathspec 'nonexistent' did not match any file(s) known to git", false}, + {"exit status 1", false}, + } + for _, tt := range tests { + got := isCheckoutConflict(fmt.Errorf("%s", tt.msg)) + if got != tt.want { + t.Errorf("isCheckoutConflict(%q) = %v, want %v", tt.msg, got, tt.want) + } + } +} + +func TestCheckoutConflictSkip_NonConflictError(t *testing.T) { + skip, _ := checkoutConflictSkip("/nonexistent", fmt.Errorf("pathspec not found")) + if skip { + t.Error("should not skip for non-conflict errors") + } +} diff --git a/internal/e2e/checkout_test.go b/internal/e2e/checkout_test.go index 237f35a..a331d0f 100644 --- a/internal/e2e/checkout_test.go +++ b/internal/e2e/checkout_test.go @@ -68,10 +68,10 @@ func TestCheckout_NonExistentBranch(t *testing.T) { } } -func TestCheckout_DirtyRepo_Skips(t *testing.T) { +func TestCheckout_DirtyRepo_NonConflicting_Succeeds(t *testing.T) { e := newTestEnv(t) - repo, _ := e.initRepoWithRemote("co-dirty") - e.runGitm("repo", "add", repo, "--alias", "co-dirty") + repo, _ := e.initRepoWithRemote("co-dirty-ok") + e.runGitm("repo", "add", repo, "--alias", "co-dirty-ok") e.mustGit(repo, "checkout", "-b", "feat/dirty-target") e.mustGit(repo, "push", "--set-upstream", "origin", "feat/dirty-target") @@ -79,14 +79,42 @@ func TestCheckout_DirtyRepo_Skips(t *testing.T) { e.writeFile(repo, "README.md", "# dirty content\n") - r := e.runGitm("checkout", "feat/dirty-target", "--repo", "co-dirty") + r := e.runGitm("checkout", "feat/dirty-target", "--repo", "co-dirty-ok") + e.assertExitCode(r, 0) + + branch := e.currentBranch(repo) + if branch != "feat/dirty-target" { + t.Errorf("non-conflicting dirty repo should switch branches, but stayed on %s", branch) + } +} + +func TestCheckout_DirtyRepo_Conflicting_Skips(t *testing.T) { + e := newTestEnv(t) + repo, _ := e.initRepoWithRemote("co-dirty-conflict") + e.runGitm("repo", "add", repo, "--alias", "co-dirty-conflict") + + e.writeFile(repo, "conflict.txt", "main-version\n") + e.mustGit(repo, "add", "conflict.txt") + e.mustGit(repo, "commit", "-m", "add conflict file on main") + e.mustGit(repo, "push") + + e.mustGit(repo, "checkout", "-b", "feat/conflict-target") + e.writeFile(repo, "conflict.txt", "feature-version\n") + e.mustGit(repo, "add", "conflict.txt") + e.mustGit(repo, "commit", "-m", "change conflict file on feature") + e.mustGit(repo, "push", "--set-upstream", "origin", "feat/conflict-target") + e.mustGit(repo, "checkout", "main") + + e.writeFile(repo, "conflict.txt", "local dirty change\n") + + r := e.runGitm("checkout", "feat/conflict-target", "--repo", "co-dirty-conflict") e.assertExitCode(r, 0) branch := e.currentBranch(repo) if branch != "main" { - t.Errorf("dirty repo should not switch branches, but now on %s", branch) + t.Errorf("conflicting dirty repo should stay on current branch, but switched to %s", branch) } - e.assertContains(r, "co-dirty") // Should mention the repo + e.assertContains(r, "co-dirty-conflict") } func TestCheckout_UntrackedFiles_ShouldNotSkip(t *testing.T) {