From 6e0ce33ef0927238d395169ac5cc929a7dc87a3c Mon Sep 17 00:00:00 2001 From: Alexandre Ferrreira Date: Fri, 1 May 2026 10:52:30 +0100 Subject: [PATCH 1/2] fix: resolve 4 bugs found during e2e testing 1. Checkout remote-only branches: add git fetch before checkout so git can create a local tracking branch from the remote ref (was failing silently because ls-remote doesn't update local refs). 2. Status DIRTY column: change label from 'N modified' to 'N changed' since the count includes untracked files, not just modifications. 3. Repo add alias conflict: return non-zero exit code when an alias collision occurs with a different repo path. Previously exited 0 with a red message but no error, confusing scripts/CI. 4. Update/checkout exit code: propagate failures from runner.Run so commands return non-zero when any repository operation fails. Added HasErrors/ErrorCount helpers to the runner package. --- internal/cli/checkout.go | 23 +++++- internal/cli/checkout_run_test.go | 52 ++++++++++++++ internal/cli/repo.go | 24 +++++-- internal/cli/repo_test.go | 55 +++++++++++++++ internal/cli/status.go | 2 +- internal/cli/update.go | 5 +- internal/cli/update_run_test.go | 18 +++++ internal/git/git.go | 7 ++ internal/git/git_test.go | 60 ++++++++++++++++ internal/runner/parallel.go | 21 ++++++ internal/runner/parallel_test.go | 112 ++++++++++++++++++++++++++++++ 11 files changed, 367 insertions(+), 12 deletions(-) diff --git a/internal/cli/checkout.go b/internal/cli/checkout.go index a6fa118..b843a4f 100644 --- a/internal/cli/checkout.go +++ b/internal/cli/checkout.go @@ -93,7 +93,7 @@ func runCheckoutWithUI(ui ui, args []string, repoAliases []string) error { func runCheckoutDefault(repos []*db.Repository) error { fmt.Printf("Checking out default branch and pulling for %d repositories…\n\n", len(repos)) - runner.Run(repos, func(repo *db.Repository) (string, string, error) { + 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) @@ -122,6 +122,9 @@ func runCheckoutDefault(repos []*db.Repository) error { return fmt.Sprintf("on %s — %s", repo.DefaultBranch, summarisePull(out)), "", nil }) + if runner.HasErrors(results) { + return fmt.Errorf("%d repository(ies) failed to checkout", runner.ErrorCount(results)) + } return nil } @@ -130,10 +133,13 @@ func runCheckoutDefault(repos []*db.Repository) error { func runCheckoutBranch(repos []*db.Repository, branch string) error { fmt.Printf("Checking out branch %q in %d repositories…\n\n", branch, len(repos)) - runner.Run(repos, func(repo *db.Repository) (string, string, error) { + results := runner.Run(repos, func(repo *db.Repository) (string, string, error) { return checkoutBranchInRepo(repo, branch) }) + if runner.HasErrors(results) { + return fmt.Errorf("%d repository(ies) failed to checkout", runner.ErrorCount(results)) + } return nil } @@ -154,10 +160,13 @@ func runCheckoutInteractive(repos []*db.Repository, ui ui) error { fmt.Printf("\nChecking out branch %q in %d repositories…\n\n", branch, len(chosen)) - runner.Run(chosen, func(repo *db.Repository) (string, string, error) { + results := runner.Run(chosen, func(repo *db.Repository) (string, string, error) { return checkoutBranchInRepo(repo, branch) }) + if runner.HasErrors(results) { + return fmt.Errorf("%d repository(ies) failed to checkout", runner.ErrorCount(results)) + } return nil } @@ -192,6 +201,14 @@ func checkoutBranchInRepo(repo *db.Repository, branch string) (string, string, e return "", fmt.Sprintf("branch %q not found (local or remote)", branch), nil } + // If the branch only exists on the remote, fetch it first so git checkout + // can create a local tracking branch from the remote ref. + if !localExists && remoteExists { + if fetchErr := git.FetchBranch(repo.Path, branch); fetchErr != nil { + return "", "", fmt.Errorf("fetch remote branch %q: %w", branch, fetchErr) + } + } + if checkoutErr := git.Checkout(repo.Path, branch); checkoutErr != nil { return "", "", fmt.Errorf("checkout: %w", checkoutErr) } diff --git a/internal/cli/checkout_run_test.go b/internal/cli/checkout_run_test.go index 5f1198f..5792738 100644 --- a/internal/cli/checkout_run_test.go +++ b/internal/cli/checkout_run_test.go @@ -241,3 +241,55 @@ func TestRunCheckoutWithUI_RepoFlag_EmptySlice(t *testing.T) { t.Fatalf("repo1 head = %q, want main", head) } } + +func TestRunCheckoutBranch_RemoteOnly(t *testing.T) { + // Tests Finding #1: checkout a branch that only exists on the remote. + // The fix fetches the branch before running git checkout so git can + // create a local tracking branch. + database = setupTestDB(t) + dir, originDir, _ := initRepoWithRemote(t) + repo, err := database.AddRepository("repo1", "repo1", dir, "main") + if err != nil { + t.Fatalf("AddRepository: %v", err) + } + + // Create a branch on origin via a second clone. + clone2Dir := cloneRepo(t, originDir) + mustRunGit(t, clone2Dir, "config", "user.email", "test@example.com") + mustRunGit(t, clone2Dir, "config", "user.name", "Test User") + mustRunGit(t, clone2Dir, "checkout", "-b", "feature/remote-only") + writeFile(t, clone2Dir, "remote.txt", "from remote\n") + mustRunGit(t, clone2Dir, "add", ".") + mustRunGit(t, clone2Dir, "commit", "-m", "remote commit") + mustRunGit(t, clone2Dir, "push", "--set-upstream", "origin", "feature/remote-only") + + // The branch only exists on origin, not locally in our working repo. + if err := runCheckoutBranch([]*db.Repository{repo}, "feature/remote-only"); err != nil { + t.Fatalf("runCheckoutBranch: %v", err) + } + + if head := gitCurrentBranch(t, dir); head != "feature/remote-only" { + t.Fatalf("head = %q, want %q", head, "feature/remote-only") + } +} + +func TestRunCheckoutDefault_ReturnsErrorOnFailure(t *testing.T) { + // Tests Finding #4: checkout returns non-zero when repos fail. + database = setupTestDB(t) + + // Create a repo that points to a non-existent path. + repo := &db.Repository{ + ID: 1, + Alias: "broken", + Path: "/nonexistent/path", + DefaultBranch: "main", + } + + err := runCheckoutDefault([]*db.Repository{repo}) + if err == nil { + t.Fatal("expected error when repo fails, got nil") + } + if !strings.Contains(err.Error(), "failed to checkout") { + t.Errorf("error = %q, want to contain \"failed to checkout\"", err.Error()) + } +} diff --git a/internal/cli/repo.go b/internal/cli/repo.go index 6b53d2a..5643d7b 100644 --- a/internal/cli/repo.go +++ b/internal/cli/repo.go @@ -126,15 +126,25 @@ is useful when repos are nested inside grouping folders (e.g. project/v1, projec _, err = database.AddRepository(name, displayAlias, abs, defaultBranch) if err != nil { if strings.Contains(err.Error(), "UNIQUE constraint") { - // Check alias collision first. - aliasOwner, aliasErr := database.GetRepository(displayAlias) - if aliasErr == nil && aliasOwner.Path != abs { - color.Red(" ✗ alias %q is already used by %s", displayAlias, aliasOwner.Path) - fmt.Printf(" Use --alias to give this repo a unique name, e.g.:\n") - fmt.Printf(" gitm repo add %s --alias \n", abs) - } else if existing, pathErr := database.GetRepositoryByPath(abs); pathErr == nil { + // Check if this is a path duplicate (idempotent re-add). + if existing, pathErr := database.GetRepositoryByPath(abs); pathErr == nil { // Path already registered under a (possibly different) alias. color.Yellow(" ⚠ %s: already registered as %q", abs, existing.Alias) + } else if aliasOwner, aliasErr := database.GetRepository(displayAlias); aliasErr == nil { + // Alias collision: the alias is taken by a different repo. + // Resolve the stored path to handle symlink differences. + ownerPath := aliasOwner.Path + if resolved, evalErr := filepath.EvalSymlinks(ownerPath); evalErr == nil { + ownerPath = resolved + } + if ownerPath != abs { + color.Red(" ✗ alias %q is already used by %s", displayAlias, aliasOwner.Path) + fmt.Printf(" Use --alias to give this repo a unique name, e.g.:\n") + fmt.Printf(" gitm repo add %s --alias \n", abs) + failed++ + } else { + color.Yellow(" ⚠ %s: already registered", displayAlias) + } } else { color.Yellow(" ⚠ %s: already registered", displayAlias) } diff --git a/internal/cli/repo_test.go b/internal/cli/repo_test.go index 7baccf4..d4546b2 100644 --- a/internal/cli/repo_test.go +++ b/internal/cli/repo_test.go @@ -614,3 +614,58 @@ func TestRepoAddNormalizesSymlinkedPaths(t *testing.T) { t.Fatalf("expected 1 repo (no duplicate), got %d", len(repos)) } } + +// ─── TestRepoAddAliasConflictReturnsError ─────────────────────────────────── + +// TestRepoAddAliasConflictReturnsError verifies that adding a repo with an +// alias that is already taken by a different repo returns a non-nil error +// (exit code != 0). Regression test for Finding #3. +func TestRepoAddAliasConflictReturnsError(t *testing.T) { + setupTestDB(t) + + // Create two separate git repos. + repo1Dir := initRepo(t) + repo2Dir := initRepo(t) + + // Register repo1 with alias "my-service". + cmd1 := repoAddCmd() + if err := cmd1.Flags().Set("alias", "my-service"); err != nil { + t.Fatalf("set flag: %v", err) + } + if err := cmd1.RunE(cmd1, []string{repo1Dir}); err != nil { + t.Fatalf("add repo1: %v", err) + } + + // Try to register repo2 with the SAME alias — should fail. + cmd2 := repoAddCmd() + if err := cmd2.Flags().Set("alias", "my-service"); err != nil { + t.Fatalf("set flag: %v", err) + } + err := cmd2.RunE(cmd2, []string{repo2Dir}) + if err == nil { + t.Fatal("expected error when adding repo with duplicate alias, got nil") + } + if !strings.Contains(err.Error(), "could not be added") { + t.Errorf("error = %q, want to contain \"could not be added\"", err.Error()) + } +} + +// TestRepoAddSamePathIdempotent verifies that re-adding the same repo path +// does NOT return an error (idempotent behavior). +func TestRepoAddSamePathIdempotent(t *testing.T) { + setupTestDB(t) + + repoDir := initRepo(t) + + // First add. + cmd1 := repoAddCmd() + if err := cmd1.RunE(cmd1, []string{repoDir}); err != nil { + t.Fatalf("first add: %v", err) + } + + // Second add of same path — should succeed (no error). + cmd2 := repoAddCmd() + if err := cmd2.RunE(cmd2, []string{repoDir}); err != nil { + t.Fatalf("second add of same path should be idempotent, got: %v", err) + } +} diff --git a/internal/cli/status.go b/internal/cli/status.go index f1c76c3..b457905 100644 --- a/internal/cli/status.go +++ b/internal/cli/status.go @@ -99,7 +99,7 @@ func runStatus(cmd *cobra.Command, args []string, fetchRemote bool) error { statuses[i] = s return } - s.dirty = fmt.Sprintf("%d modified", len(files)) + s.dirty = fmt.Sprintf("%d changed", len(files)) } else { s.dirty = "clean" } diff --git a/internal/cli/update.go b/internal/cli/update.go index c29031b..326d777 100644 --- a/internal/cli/update.go +++ b/internal/cli/update.go @@ -53,7 +53,7 @@ func runUpdate(repoAliases []string) error { fmt.Printf("Pulling current branch for %d repositories…\n\n", len(repos)) - runner.Run(repos, func(repo *db.Repository) (string, string, error) { + 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: %w", err) @@ -76,6 +76,9 @@ func runUpdate(repoAliases []string) error { return msg, "", nil }) + if runner.HasErrors(results) { + return fmt.Errorf("%d repository(ies) failed to update", runner.ErrorCount(results)) + } return nil } diff --git a/internal/cli/update_run_test.go b/internal/cli/update_run_test.go index 5190cab..7e892a0 100644 --- a/internal/cli/update_run_test.go +++ b/internal/cli/update_run_test.go @@ -135,6 +135,24 @@ func TestRunUpdate_RepoFlag_EmptySlice(t *testing.T) { } } +func TestRunUpdate_ReturnsErrorOnFailure(t *testing.T) { + // Tests Finding #4: runUpdate should return a non-nil error when repos fail. + database = setupTestDB(t) + + // Register a repo with a non-existent path — git operations will fail. + if _, err := database.AddRepository("broken", "broken", "/nonexistent/path/broken", "main"); err != nil { + t.Fatalf("AddRepository: %v", err) + } + + err := runUpdate(nil) + if err == nil { + t.Fatal("expected error when repo git operations fail, got nil") + } + if !strings.Contains(err.Error(), "failed to update") { + t.Errorf("error = %q, want to contain \"failed to update\"", err.Error()) + } +} + func pushRemoteChange(t *testing.T, origin, filename string) { t.Helper() clone := cloneRepo(t, origin) diff --git a/internal/git/git.go b/internal/git/git.go index e154c3a..f865462 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -256,6 +256,13 @@ func RemoteBranchExists(path, branch string) bool { return err == nil } +// FetchBranch fetches a single branch from origin so that git checkout can +// create a local tracking branch from the remote ref. +func FetchBranch(path, branch string) error { + _, err := run(path, "fetch", "origin", branch) + return err +} + // RenameBranch renames a local branch from oldName to newName. func RenameBranch(path, oldName, newName string) error { _, err := run(path, "branch", "-m", oldName, newName) diff --git a/internal/git/git_test.go b/internal/git/git_test.go index a715c9b..18250bd 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -413,3 +413,63 @@ func TestForcePush(t *testing.T) { t.Errorf("expected rewritten commit on remote, got: %q", log[0]) } } + +// ─── FetchBranch ──────────────────────────────────────────────────────────── + +func TestFetchBranch(t *testing.T) { + // Create a bare origin and a working clone. + bareDir := t.TempDir() + mustRunGit(t, bareDir, "init", "--bare", "--initial-branch=main") + + workDir := initRepo(t) + mustRunGit(t, workDir, "remote", "add", "origin", bareDir) + mustRunGit(t, workDir, "push", "--set-upstream", "origin", "main") + + // Create a feature branch on origin via a second clone. + clone2 := t.TempDir() + mustRunGit(t, clone2, "clone", bareDir, ".") + mustRunGit(t, clone2, "config", "user.email", "test@example.com") + mustRunGit(t, clone2, "config", "user.name", "Test User") + mustRunGit(t, clone2, "checkout", "-b", "feature/remote-only") + writeFile(t, clone2, "remote.txt", "from remote") + mustRunGit(t, clone2, "add", ".") + mustRunGit(t, clone2, "commit", "-m", "remote commit") + mustRunGit(t, clone2, "push", "--set-upstream", "origin", "feature/remote-only") + + // workDir does not know about feature/remote-only yet. + if git.BranchExists(workDir, "feature/remote-only") { + t.Fatal("branch should NOT exist locally before fetch") + } + + // FetchBranch should succeed and bring the ref. + if err := git.FetchBranch(workDir, "feature/remote-only"); err != nil { + t.Fatalf("FetchBranch: %v", err) + } + + // After fetch, git checkout should be able to create a tracking branch. + if err := git.Checkout(workDir, "feature/remote-only"); err != nil { + t.Fatalf("Checkout after FetchBranch: %v", err) + } + + branch, err := git.CurrentBranch(workDir) + if err != nil { + t.Fatalf("CurrentBranch: %v", err) + } + if branch != "feature/remote-only" { + t.Errorf("branch = %q, want %q", branch, "feature/remote-only") + } +} + +func TestFetchBranch_NonExistentBranch(t *testing.T) { + bareDir := t.TempDir() + mustRunGit(t, bareDir, "init", "--bare", "--initial-branch=main") + + workDir := initRepo(t) + mustRunGit(t, workDir, "remote", "add", "origin", bareDir) + mustRunGit(t, workDir, "push", "--set-upstream", "origin", "main") + + err := git.FetchBranch(workDir, "does-not-exist") + if err == nil { + t.Fatal("expected error fetching non-existent branch, got nil") + } +} diff --git a/internal/runner/parallel.go b/internal/runner/parallel.go index 5c329ab..9c2de05 100644 --- a/internal/runner/parallel.go +++ b/internal/runner/parallel.go @@ -146,3 +146,24 @@ func printSummary(results []Result) { fmt.Printf("\n%s %s\n", bold.Sprint("Done:"), strings.Join(parts, ", ")) } + +// HasErrors returns true if any result in the slice has StatusError. +func HasErrors(results []Result) bool { + for _, r := range results { + if r.Status == StatusError { + return true + } + } + return false +} + +// ErrorCount returns the number of results with StatusError. +func ErrorCount(results []Result) int { + n := 0 + for _, r := range results { + if r.Status == StatusError { + n++ + } + } + return n +} diff --git a/internal/runner/parallel_test.go b/internal/runner/parallel_test.go index 469dffc..3174508 100644 --- a/internal/runner/parallel_test.go +++ b/internal/runner/parallel_test.go @@ -337,3 +337,115 @@ func TestRunMessagePreservation(t *testing.T) { t.Errorf("Message = %q, want %q", results[0].Message, expectedMsg) } } + +// ─── TestHasErrors ────────────────────────────────────────────────────────── + +func TestHasErrors(t *testing.T) { + tests := []struct { + name string + results []runner.Result + want bool + }{ + { + name: "nil results", + results: nil, + want: false, + }, + { + name: "empty results", + results: []runner.Result{}, + want: false, + }, + { + name: "all success", + results: []runner.Result{ + {Status: runner.StatusSuccess}, + {Status: runner.StatusSuccess}, + }, + want: false, + }, + { + name: "one error", + results: []runner.Result{ + {Status: runner.StatusSuccess}, + {Status: runner.StatusError, Err: fmt.Errorf("boom")}, + }, + want: true, + }, + { + name: "skipped only", + results: []runner.Result{ + {Status: runner.StatusSkipped}, + }, + want: false, + }, + { + name: "mixed with error", + results: []runner.Result{ + {Status: runner.StatusSuccess}, + {Status: runner.StatusSkipped}, + {Status: runner.StatusError, Err: fmt.Errorf("fail")}, + }, + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := runner.HasErrors(tt.results) + if got != tt.want { + t.Errorf("HasErrors() = %v, want %v", got, tt.want) + } + }) + } +} + +// ─── TestErrorCount ───────────────────────────────────────────────────────── + +func TestErrorCount(t *testing.T) { + tests := []struct { + name string + results []runner.Result + want int + }{ + { + name: "no results", + results: nil, + want: 0, + }, + { + name: "no errors", + results: []runner.Result{ + {Status: runner.StatusSuccess}, + {Status: runner.StatusSkipped}, + }, + want: 0, + }, + { + name: "one error", + results: []runner.Result{ + {Status: runner.StatusSuccess}, + {Status: runner.StatusError}, + }, + want: 1, + }, + { + name: "all errors", + results: []runner.Result{ + {Status: runner.StatusError}, + {Status: runner.StatusError}, + {Status: runner.StatusError}, + }, + want: 3, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := runner.ErrorCount(tt.results) + if got != tt.want { + t.Errorf("ErrorCount() = %d, want %d", got, tt.want) + } + }) + } +} From 2d54523a3356f6d468918fc7c23b23915aadd067 Mon Sep 17 00:00:00 2001 From: Alexandre Ferrreira Date: Fri, 1 May 2026 10:59:39 +0100 Subject: [PATCH 2/2] fix/e2e-findings Fix add -- before the branch name in FetchBranch --- internal/git/git.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/git/git.go b/internal/git/git.go index f865462..2cd33db 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -258,8 +258,10 @@ func RemoteBranchExists(path, branch string) bool { // FetchBranch fetches a single branch from origin so that git checkout can // create a local tracking branch from the remote ref. +// The -- separator ensures the branch name is always treated as a refspec +// and never misinterpreted as a flag (e.g. if it starts with -). func FetchBranch(path, branch string) error { - _, err := run(path, "fetch", "origin", branch) + _, err := run(path, "fetch", "origin", "--", branch) return err }