diff --git a/internal/cli/clean.go b/internal/cli/clean.go index ea5e77e..3e30b9d 100644 --- a/internal/cli/clean.go +++ b/internal/cli/clean.go @@ -33,6 +33,28 @@ func init() { cleanCmd.Flags().BoolVarP(&cleanDry, "dry-run", "n", false, "Show what would be removed without removing") } +// selectMergedWorktrees returns the worktrees eligible for cleanup: non-primary +// worktrees whose branch is in the merged set. The default branch is never a +// candidate (MergedBranches no longer self-filters it now that it measures +// against a fully-qualified ref). +func selectMergedWorktrees(worktrees []*worktree.Worktree, mergedBranches []string, defaultBranch string) []*worktree.Worktree { + mergedSet := make(map[string]bool, len(mergedBranches)) + for _, b := range mergedBranches { + mergedSet[b] = true + } + + var toRemove []*worktree.Worktree + for _, wt := range worktrees { + if wt.IsPrimary || wt.Branch == defaultBranch { + continue + } + if mergedSet[wt.Branch] { + toRemove = append(toRemove, wt) + } + } + return toRemove +} + func runClean(cmd *cobra.Command, args []string) error { cwd, err := os.Getwd() if err != nil { @@ -57,27 +79,16 @@ func runClean(cmd *cobra.Command, args []string) error { return err } - // Find merged branches - mergedBranches, err := wm.MergedBranches(defaultBranch) + // Find merged branches, measured against the resolved default tip + // (origin/ when local is stale) so branches merged upstream are + // detected consistently with how `ygg new` bases new worktrees. + mergedBranches, err := wm.MergedBranches(wm.BaseRef(defaultBranch)) if err != nil { errorMsg("Failed to get merged branches: %v", err) return err } - mergedSet := make(map[string]bool) - for _, b := range mergedBranches { - mergedSet[b] = true - } - - var toRemove []*worktree.Worktree - for _, wt := range worktrees { - if wt.IsPrimary { - continue - } - if mergedSet[wt.Branch] { - toRemove = append(toRemove, wt) - } - } + toRemove := selectMergedWorktrees(worktrees, mergedBranches, defaultBranch) if len(toRemove) == 0 { info("No merged worktrees to clean up") diff --git a/internal/cli/clean_test.go b/internal/cli/clean_test.go new file mode 100644 index 0000000..d048448 --- /dev/null +++ b/internal/cli/clean_test.go @@ -0,0 +1,31 @@ +package cli + +import ( + "testing" + + "github.com/joch/ygg/internal/worktree" +) + +func TestSelectMergedWorktreesExcludesDefaultBranch(t *testing.T) { + worktrees := []*worktree.Worktree{ + {Name: "main", Branch: "main", IsPrimary: true}, + {Name: "mainwt", Branch: "main", IsPrimary: false}, // non-primary, on default branch + {Name: "feature", Branch: "feature", IsPrimary: false}, + {Name: "wip", Branch: "wip", IsPrimary: false}, + } + // MergedBranches now measures against a fully-qualified ref, so the default + // branch itself appears in the merged list (it no longer self-filters). + merged := []string{"main", "feature"} + + got := selectMergedWorktrees(worktrees, merged, "main") + + // Only "feature" qualifies: primary is skipped, the non-primary default-branch + // worktree must be skipped, and "wip" is not merged. + var names []string + for _, wt := range got { + names = append(names, wt.Name) + } + if len(names) != 1 || names[0] != "feature" { + t.Fatalf("selected %v, want exactly [feature]", names) + } +} diff --git a/internal/cli/new.go b/internal/cli/new.go index c614b97..13a570d 100644 --- a/internal/cli/new.go +++ b/internal/cli/new.go @@ -16,8 +16,10 @@ var newCmd = &cobra.Command{ Long: `Create a new git worktree with the specified name. This will: -1. Fetch and pull the default branch (main/master) -2. Create a new worktree with a branch named +1. Fetch the latest changes from origin +2. Create a new worktree with a branch named , based on the latest + origin/ (falling back to the local default branch when + there is no remote) 3. Enter a subshell in the new worktree directory Exit the subshell to return to your original directory.`, @@ -50,14 +52,15 @@ func runNew(cmd *cobra.Command, args []string) error { info("Could not fetch (offline?)") } - // Pull default branch in main repo + // Detect the default branch (main/master); the worktree is based on the + // freshly fetched origin/ tip, so no local pull is required. defaultBranch, err := wm.DefaultBranch() if err != nil { errorMsg("Could not detect default branch: %v", err) return err } - info("Creating worktree: %s (based on %s)", name, defaultBranch) + info("Creating worktree: %s (default branch %s)", name, defaultBranch) wt, err := wm.Create(name) if err != nil { @@ -65,7 +68,11 @@ func runNew(cmd *cobra.Command, args []string) error { return err } - success("Created worktree at %s", wt.Path) + if wt.Base != "" { + success("Created worktree at %s (based on %s)", wt.Path, wt.Base) + } else { + success("Created worktree at %s", wt.Path) + } // Warn if .worktrees is not in .gitignore if !wm.IsWorktreeDirIgnored() { diff --git a/internal/worktree/worktree.go b/internal/worktree/worktree.go index cdc8e5b..1bd157c 100644 --- a/internal/worktree/worktree.go +++ b/internal/worktree/worktree.go @@ -12,6 +12,7 @@ type Worktree struct { Name string Path string Branch string + Base string // Ref the new branch was based on (empty when checking out an existing branch) IsBare bool IsLocked bool IsPrimary bool @@ -90,6 +91,65 @@ func (m *Manager) DefaultBranch() (string, error) { return "", fmt.Errorf("could not detect default branch") } +// BaseRef returns the effective tip of the default branch — the ref new +// worktrees are based on and that merge detection is measured against. It +// prefers the freshly fetched origin/ remote-tracking ref so it +// reflects the latest upstream even when the local default branch is stale +// (the common "forgot to pull" case) — but only when origin is ahead of, or +// equal to, the local branch. When the local branch has commits origin lacks +// (committed directly to it, or fetch failed offline), or the two have +// diverged, the local branch wins so those commits are never silently dropped. +// Falls back to the local branch when no remote-tracking ref exists. +// +// The returned ref is fully qualified (refs/remotes/... or refs/heads/...) so it +// stays unambiguous when passed to git even in repos that have a local branch +// named e.g. "origin/main". Use DisplayRef for user-facing output. +func (m *Manager) BaseRef(defaultBranch string) string { + remoteRef := "refs/remotes/origin/" + defaultBranch + localRef := "refs/heads/" + defaultBranch + hasRemote := m.refExists(remoteRef) + hasLocal := m.refExists(localRef) + + switch { + case hasRemote && hasLocal: + // Use origin only when the local branch is an ancestor of it + // (origin is ahead or identical); otherwise keep local. + if m.isAncestor(localRef, remoteRef) { + return remoteRef + } + return localRef + case hasRemote: + return remoteRef + default: + return localRef + } +} + +// DisplayRef shortens a fully-qualified ref for user-facing output, e.g. +// refs/remotes/origin/main -> origin/main and refs/heads/main -> main. +func DisplayRef(ref string) string { + for _, prefix := range []string{"refs/remotes/", "refs/heads/"} { + if s := strings.TrimPrefix(ref, prefix); s != ref { + return s + } + } + return ref +} + +// refExists reports whether the given fully-qualified ref resolves in the repo. +func (m *Manager) refExists(ref string) bool { + cmd := exec.Command("git", "rev-parse", "--verify", "--quiet", ref) + cmd.Dir = m.repoPath + return cmd.Run() == nil +} + +// isAncestor reports whether ref a is an ancestor of (or identical to) ref b. +func (m *Manager) isAncestor(a, b string) bool { + cmd := exec.Command("git", "merge-base", "--is-ancestor", a, b) + cmd.Dir = m.repoPath + return cmd.Run() == nil +} + // Fetch fetches from origin. func (m *Manager) Fetch() error { cmd := exec.Command("git", "fetch", "origin") @@ -101,17 +161,6 @@ func (m *Manager) Fetch() error { return nil } -// Pull pulls the current branch from origin. -func (m *Manager) Pull() error { - cmd := exec.Command("git", "pull") - cmd.Dir = m.repoPath - output, err := cmd.CombinedOutput() - if err != nil { - return fmt.Errorf("git pull failed: %w\n%s", err, output) - } - return nil -} - // HasUncommittedChanges returns true if there are uncommitted changes. func (m *Manager) HasUncommittedChanges(path string) bool { cmd := exec.Command("git", "status", "--porcelain") @@ -140,22 +189,24 @@ func (m *Manager) Create(name string) (*Worktree, error) { return nil, fmt.Errorf("failed to detect default branch: %w", err) } - // Create worktree with new branch based on default branch - cmd := exec.Command("git", "worktree", "add", "-b", name, worktreePath, defaultBranch) + var base string + var cmd *exec.Cmd + if m.refExists("refs/heads/" + name) { + // A branch with this name already exists — check it out into the new + // worktree as-is, without re-pointing it at a base ref. + cmd = exec.Command("git", "worktree", "add", worktreePath, name) + } else { + // Base the new branch on the freshly fetched origin/ tip when + // it is ahead of the local branch (the "forgot to pull" case), + // otherwise on the local default branch — see BaseRef. --no-track keeps + // the new branch from adopting origin/ as its upstream. + base = m.BaseRef(defaultBranch) + cmd = exec.Command("git", "worktree", "add", "--no-track", "-b", name, worktreePath, base) + } cmd.Dir = m.repoPath - output, err := cmd.CombinedOutput() - if err != nil { - // Branch might already exist, try without -b - if strings.Contains(string(output), "already exists") { - cmd = exec.Command("git", "worktree", "add", worktreePath, name) - cmd.Dir = m.repoPath - output, err = cmd.CombinedOutput() - } - - if err != nil { - return nil, fmt.Errorf("failed to create worktree: %w\n%s", err, output) - } + if output, err := cmd.CombinedOutput(); err != nil { + return nil, fmt.Errorf("failed to create worktree: %w\n%s", err, output) } // Copy untracked/ignored files from main worktree @@ -165,6 +216,7 @@ func (m *Manager) Create(name string) (*Worktree, error) { Name: name, Path: worktreePath, Branch: name, + Base: DisplayRef(base), CopiedFiles: copied, CopyError: copyErr, }, nil @@ -261,7 +313,9 @@ func (m *Manager) IsBranchMerged(branch string) (bool, error) { return true, nil } - merged, err := m.MergedBranches(defaultBranch) + // Measure against the resolved default tip (origin/ when local is + // stale) so a branch merged upstream is recognized even before a local pull. + merged, err := m.MergedBranches(m.BaseRef(defaultBranch)) if err != nil { return false, err } diff --git a/internal/worktree/worktree_test.go b/internal/worktree/worktree_test.go new file mode 100644 index 0000000..a670550 --- /dev/null +++ b/internal/worktree/worktree_test.go @@ -0,0 +1,340 @@ +package worktree + +import ( + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// gitOut runs a git command and returns trimmed stdout, failing the test on +// error. It uses Output() (not CombinedOutput) so stderr can never pollute the +// returned value, which callers compare against commit SHAs. +func gitOut(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + out, err := cmd.Output() + if err != nil { + t.Fatalf("git %v failed: %v", args, err) + } + return strings.TrimSpace(string(out)) +} + +// setupRepoWithStaleMain builds a local clone whose origin/main is one commit +// ahead of the local main branch (simulating "forgot to pull main"). It returns +// the local repo path, the stale local main SHA (A), and the fresh origin SHA (B). +func setupRepoWithStaleMain(t *testing.T) (localDir, staleA, freshB string) { + t.Helper() + tmp := t.TempDir() + + originDir := filepath.Join(tmp, "origin.git") + localDir = filepath.Join(tmp, "local") + otherDir := filepath.Join(tmp, "other") + + // Bare origin + runGit(t, tmp, "init", "-q", "--bare", "--initial-branch=main", originDir) + + // Local clone: commit A on main, push, record origin HEAD + runGit(t, tmp, "clone", "-q", originDir, localDir) + runGit(t, localDir, "config", "user.email", "test@test.com") + runGit(t, localDir, "config", "user.name", "Test") + runGit(t, localDir, "checkout", "-q", "-b", "main") + runGit(t, localDir, "commit", "-q", "--allow-empty", "-m", "commit A") + runGit(t, localDir, "push", "-q", "-u", "origin", "main") + runGit(t, localDir, "remote", "set-head", "origin", "-a") + staleA = gitOut(t, localDir, "rev-parse", "HEAD") + + // Teammate pushes commit B to origin from a separate clone + runGit(t, tmp, "clone", "-q", originDir, otherDir) + runGit(t, otherDir, "config", "user.email", "other@test.com") + runGit(t, otherDir, "config", "user.name", "Other") + runGit(t, otherDir, "commit", "-q", "--allow-empty", "-m", "commit B") + runGit(t, otherDir, "push", "-q", "origin", "main") + freshB = gitOut(t, otherDir, "rev-parse", "HEAD") + + if staleA == freshB { + t.Fatal("setup error: stale and fresh SHAs are identical") + } + return localDir, staleA, freshB +} + +// TestCreateBasesOnFreshOrigin verifies that a new worktree is based on the +// freshly fetched origin/ tip, not the stale local default branch. +func TestCreateBasesOnFreshOrigin(t *testing.T) { + localDir, staleA, freshB := setupRepoWithStaleMain(t) + + wm, err := NewManager(localDir) + if err != nil { + t.Fatalf("NewManager failed: %v", err) + } + + // Mirror "ygg new": fetch origin (updates origin/main, not local main). + if err := wm.Fetch(); err != nil { + t.Fatalf("Fetch failed: %v", err) + } + + // Sanity: local main is still stale, origin/main is fresh. + if got := gitOut(t, localDir, "rev-parse", "main"); got != staleA { + t.Fatalf("precondition: local main = %s, want stale %s", got, staleA) + } + if got := gitOut(t, localDir, "rev-parse", "origin/main"); got != freshB { + t.Fatalf("precondition: origin/main = %s, want fresh %s", got, freshB) + } + + wt, err := wm.Create("feature") + if err != nil { + t.Fatalf("Create failed: %v", err) + } + + head := gitOut(t, wt.Path, "rev-parse", "HEAD") + if head != freshB { + t.Errorf("worktree HEAD = %s, want fresh origin/main %s (stale local main was %s)", + head, freshB, staleA) + } + if wt.Base != "origin/main" { + t.Errorf("wt.Base = %q, want %q", wt.Base, "origin/main") + } +} + +// TestBaseRefQualifiesRemoteRef verifies BaseRef returns a fully-qualified +// remote-tracking ref so it stays unambiguous even when a local branch named +// "origin/" exists — otherwise the short name "origin/main" is +// ambiguous and git commands fail with "ambiguous object name". +func TestBaseRefQualifiesRemoteRef(t *testing.T) { + localDir, staleA, freshB := setupRepoWithStaleMain(t) + + wm, err := NewManager(localDir) + if err != nil { + t.Fatalf("NewManager failed: %v", err) + } + if err := wm.Fetch(); err != nil { + t.Fatalf("Fetch failed: %v", err) + } + + // Pathological but valid: a local branch literally named origin/main, at the + // stale commit. This makes the short name "origin/main" ambiguous. + runGit(t, localDir, "branch", "origin/main", staleA) + + if base := wm.BaseRef("main"); base != "refs/remotes/origin/main" { + t.Errorf("BaseRef = %q, want fully-qualified refs/remotes/origin/main", base) + } + + wt, err := wm.Create("feature") + if err != nil { + t.Fatalf("Create failed (ambiguous ref?): %v", err) + } + if head := gitOut(t, wt.Path, "rev-parse", "HEAD"); head != freshB { + t.Errorf("worktree HEAD = %s, want fresh origin/main %s (not stale local origin/main %s)", + head, freshB, staleA) + } +} + +// setupRepoWithUpstreamMerge builds a local clone where origin/ is +// ahead of a stale local default branch and contains a merge of a "feature" +// branch (which also exists locally). It returns the local repo path. This is +// the "merged a PR but forgot to pull main" scenario. +func setupRepoWithUpstreamMerge(t *testing.T) string { + t.Helper() + tmp := t.TempDir() + originDir := filepath.Join(tmp, "origin.git") + localDir := filepath.Join(tmp, "local") + otherDir := filepath.Join(tmp, "other") + + runGit(t, tmp, "init", "-q", "--bare", "--initial-branch=main", originDir) + + runGit(t, tmp, "clone", "-q", originDir, localDir) + runGit(t, localDir, "config", "user.email", "test@test.com") + runGit(t, localDir, "config", "user.name", "Test") + runGit(t, localDir, "checkout", "-q", "-b", "main") + runGit(t, localDir, "commit", "-q", "--allow-empty", "-m", "commit A") + runGit(t, localDir, "push", "-q", "-u", "origin", "main") + runGit(t, localDir, "remote", "set-head", "origin", "-a") + + // In a separate clone, merge a feature branch into main and push both, so + // origin/main advances past the local main with feature merged in. + runGit(t, tmp, "clone", "-q", originDir, otherDir) + runGit(t, otherDir, "config", "user.email", "other@test.com") + runGit(t, otherDir, "config", "user.name", "Other") + runGit(t, otherDir, "checkout", "-q", "-b", "feature") + runGit(t, otherDir, "commit", "-q", "--allow-empty", "-m", "feature work") + runGit(t, otherDir, "checkout", "-q", "main") + runGit(t, otherDir, "merge", "-q", "--no-ff", "-m", "merge feature", "feature") + runGit(t, otherDir, "push", "-q", "origin", "main") + runGit(t, otherDir, "push", "-q", "origin", "feature") + + // Locally: fetch (origin/main now ahead, local main stays stale) and create + // a local feature branch tracking the fetched feature. + runGit(t, localDir, "fetch", "-q", "origin") + runGit(t, localDir, "branch", "feature", "origin/feature") + return localDir +} + +// TestIsBranchMergedUsesResolvedBaseForStaleLocalMain verifies that merge +// detection measures against origin/ (the resolved base) so a branch +// merged upstream is recognized as merged even when local main is stale. This +// keeps `ygg clean` / `ygg remove` consistent with `ygg new` basing on origin. +func TestIsBranchMergedUsesResolvedBaseForStaleLocalMain(t *testing.T) { + localDir := setupRepoWithUpstreamMerge(t) + + wm, err := NewManager(localDir) + if err != nil { + t.Fatalf("NewManager failed: %v", err) + } + + // The clean.go path: the base must resolve to the fetched origin tip here. + if base := wm.BaseRef("main"); base != "refs/remotes/origin/main" { + t.Fatalf("BaseRef(main) = %q, want refs/remotes/origin/main", base) + } + + // The remove.go path: feature is merged into origin/main, so despite the + // stale local main it must be reported as merged. + merged, err := wm.IsBranchMerged("feature") + if err != nil { + t.Fatalf("IsBranchMerged failed: %v", err) + } + if !merged { + t.Errorf("IsBranchMerged(feature) = false, want true (merged into origin/main)") + } + + // Document the bug being fixed: against the stale local main it is missed. + stale, err := wm.MergedBranches("main") + if err != nil { + t.Fatalf("MergedBranches failed: %v", err) + } + for _, b := range stale { + if b == "feature" { + t.Fatal("precondition broken: stale local main already sees feature as merged") + } + } +} + +// TestCreateUsesLocalWhenAheadOfOrigin verifies that when the local default +// branch has commits not yet on origin (committed directly without pushing, or +// fetch failed offline), the new worktree includes them rather than silently +// basing on an older origin/ tip. +func TestCreateUsesLocalWhenAheadOfOrigin(t *testing.T) { + tmp := t.TempDir() + originDir := filepath.Join(tmp, "origin.git") + localDir := filepath.Join(tmp, "local") + + runGit(t, tmp, "init", "-q", "--bare", "--initial-branch=main", originDir) + runGit(t, tmp, "clone", "-q", originDir, localDir) + runGit(t, localDir, "config", "user.email", "test@test.com") + runGit(t, localDir, "config", "user.name", "Test") + runGit(t, localDir, "checkout", "-q", "-b", "main") + runGit(t, localDir, "commit", "-q", "--allow-empty", "-m", "commit A") + runGit(t, localDir, "push", "-q", "-u", "origin", "main") + runGit(t, localDir, "remote", "set-head", "origin", "-a") + + // Commit B locally on main without pushing: local main ahead of origin/main. + runGit(t, localDir, "commit", "-q", "--allow-empty", "-m", "commit B (unpushed)") + localAhead := gitOut(t, localDir, "rev-parse", "main") + + wm, err := NewManager(localDir) + if err != nil { + t.Fatalf("NewManager failed: %v", err) + } + if err := wm.Fetch(); err != nil { + t.Fatalf("Fetch failed: %v", err) + } + + wt, err := wm.Create("feature") + if err != nil { + t.Fatalf("Create failed: %v", err) + } + if head := gitOut(t, wt.Path, "rev-parse", "HEAD"); head != localAhead { + t.Errorf("worktree HEAD = %s, want local main %s (must not drop unpushed commits)", head, localAhead) + } + if wt.Base != "main" { + t.Errorf("wt.Base = %q, want %q", wt.Base, "main") + } +} + +// TestCreateDoesNotTrackOrigin verifies the new branch is not set up to track +// origin/ as its upstream when based on the remote-tracking ref. +func TestCreateDoesNotTrackOrigin(t *testing.T) { + localDir, _, _ := setupRepoWithStaleMain(t) + + wm, err := NewManager(localDir) + if err != nil { + t.Fatalf("NewManager failed: %v", err) + } + if err := wm.Fetch(); err != nil { + t.Fatalf("Fetch failed: %v", err) + } + if _, err := wm.Create("feature"); err != nil { + t.Fatalf("Create failed: %v", err) + } + + // No upstream configured -> rev-parse for the upstream fails (exit non-zero). + cmd := exec.Command("git", "rev-parse", "--abbrev-ref", "feature@{upstream}") + cmd.Dir = localDir + if out, err := cmd.CombinedOutput(); err == nil { + t.Errorf("feature branch unexpectedly tracks %q; want no upstream", strings.TrimSpace(string(out))) + } +} + +// TestCreateWithoutRemoteUsesLocalBranch verifies that in a repo with no remote, +// Create falls back to the local default branch and still succeeds. +func TestCreateWithoutRemoteUsesLocalBranch(t *testing.T) { + tmp := t.TempDir() + repo := filepath.Join(tmp, "repo") + + runGit(t, tmp, "init", "-q", "--initial-branch=main", repo) + runGit(t, repo, "config", "user.email", "test@test.com") + runGit(t, repo, "config", "user.name", "Test") + runGit(t, repo, "commit", "-q", "--allow-empty", "-m", "commit A") + localMain := gitOut(t, repo, "rev-parse", "HEAD") + + wm, err := NewManager(repo) + if err != nil { + t.Fatalf("NewManager failed: %v", err) + } + + wt, err := wm.Create("feature") + if err != nil { + t.Fatalf("Create failed: %v", err) + } + + if head := gitOut(t, wt.Path, "rev-parse", "HEAD"); head != localMain { + t.Errorf("worktree HEAD = %s, want local main %s", head, localMain) + } +} + +// TestCreateChecksOutExistingBranch verifies that calling Create with a name +// that already exists as a local branch checks that branch out into the new +// worktree (at its own tip), rather than failing or re-pointing it. +func TestCreateChecksOutExistingBranch(t *testing.T) { + tmp := t.TempDir() + repo := filepath.Join(tmp, "repo") + + runGit(t, tmp, "init", "-q", "--initial-branch=main", repo) + runGit(t, repo, "config", "user.email", "test@test.com") + runGit(t, repo, "config", "user.name", "Test") + runGit(t, repo, "commit", "-q", "--allow-empty", "-m", "commit A") + + // Pre-create branch "feature" pointing at its own commit. + runGit(t, repo, "branch", "feature") + runGit(t, repo, "checkout", "-q", "feature") + runGit(t, repo, "commit", "-q", "--allow-empty", "-m", "feature commit") + featureTip := gitOut(t, repo, "rev-parse", "feature") + runGit(t, repo, "checkout", "-q", "main") + + wm, err := NewManager(repo) + if err != nil { + t.Fatalf("NewManager failed: %v", err) + } + + wt, err := wm.Create("feature") + if err != nil { + t.Fatalf("Create failed: %v", err) + } + + if head := gitOut(t, wt.Path, "rev-parse", "HEAD"); head != featureTip { + t.Errorf("worktree HEAD = %s, want existing feature tip %s", head, featureTip) + } + if wt.Base != "" { + t.Errorf("wt.Base = %q, want empty for existing-branch checkout", wt.Base) + } +}