diff --git a/cmd/roborev/analyze.go b/cmd/roborev/analyze.go index 3633fa11..42c24650 100644 --- a/cmd/roborev/analyze.go +++ b/cmd/roborev/analyze.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -967,6 +968,25 @@ func getBranchFiles(cmd *cobra.Command, repoRoot string, opts analyzeOptions) (m // Determine base branch base := opts.baseBranch + if base == "" { + // Prefer the branch's upstream tracking ref only when it resolves to + // a trunk-named branch (e.g., local main tracking upstream/main in a + // fork). A branch tracking its own remote counterpart (feature + // tracking origin/feature) is not trunk — using it would skip + // already-pushed feature commits, contradicting "--branch analyzes + // all commits since trunk". + upstream, uerr := git.GetUpstream(repoRoot, targetRef) + var missing *git.UpstreamMissingError + if errors.As(uerr, &missing) { + return nil, fmt.Errorf("%w (or pass --base )", missing) + } + if uerr != nil { + return nil, fmt.Errorf("resolve upstream for %s: %w (pass --base to skip)", targetRef, uerr) + } + if upstream != "" && git.UpstreamIsTrunk(repoRoot, targetRef) { + base = upstream + } + } if base == "" { var err error base, err = git.GetDefaultBranch(repoRoot) @@ -978,8 +998,8 @@ func getBranchFiles(cmd *cobra.Command, repoRoot string, opts analyzeOptions) (m // Validate not on base branch (only when analyzing current branch) if targetRef == "HEAD" { currentBranch := git.GetCurrentBranch(repoRoot) - if currentBranch == git.LocalBranchName(base) { - return nil, fmt.Errorf("already on %s - switch to a feature branch first", git.LocalBranchName(base)) + if git.IsOnBaseBranch(repoRoot, currentBranch, base) { + return nil, fmt.Errorf("already on %s - switch to a feature branch first", currentBranch) } } diff --git a/cmd/roborev/analyze_test.go b/cmd/roborev/analyze_test.go index c82ddb71..24040153 100644 --- a/cmd/roborev/analyze_test.go +++ b/cmd/roborev/analyze_test.go @@ -570,6 +570,51 @@ func TestGetBranchFiles(t *testing.T) { require.Error(t, err, "expected error when on base branch") assert.Contains(t, err.Error(), "already on main", "unexpected error") }) + + t.Run("prefers non-origin upstream as base", func(t *testing.T) { + // Fork workflow: origin (stale fork) and upstream (real repo). + // Feature branch tracks upstream/main; merge-base must be taken + // against upstream/main, not origin/main, so commits already + // merged upstream aren't re-analyzed. + remote := newBareTestGitRepo(t) + stale := newBareTestGitRepo(t) + repo := newTestGitRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("a.go", "package main", "u1") + repo.CommitFile("b.go", "package main", "u2") + repo.Run("remote", "add", "upstream", remote.Dir) + repo.Run("push", "-u", "upstream", "main") + + // origin lags upstream by 1 commit. + repo.Run("remote", "add", "origin", stale.Dir) + repo.Run("push", "origin", "HEAD~1:refs/heads/main") + repo.Run("fetch", "origin") + repo.Run("remote", "set-head", "origin", "main") + + repo.Run("checkout", "-b", "feature", "--track", "upstream/main") + repo.CommitFile("only-new.go", "package main\nfunc New() {}", "feature work") + + files, err := getBranchFiles(cmd, repo.Dir, analyzeOptions{branch: "HEAD"}) + require.NoError(t, err) + // b.go was already merged to upstream/main; must not be analyzed. + assert.NotContains(t, files, "b.go", + "already-merged upstream commit must not be analyzed") + assert.Contains(t, files, "only-new.go", + "new feature file should be analyzed") + }) + + t.Run("blocks local main tracking non-origin upstream", func(t *testing.T) { + remote := newBareTestGitRepo(t) + repo := newTestGitRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("a.go", "package main", "initial") + repo.Run("remote", "add", "upstream", remote.Dir) + repo.Run("push", "-u", "upstream", "main") + + _, err := getBranchFiles(cmd, repo.Dir, analyzeOptions{branch: "HEAD"}) + require.Error(t, err, "expected error when on base branch") + assert.Contains(t, err.Error(), "already on main", "unexpected error") + }) } func TestAnalyzeBranchSpaceSeparated(t *testing.T) { diff --git a/cmd/roborev/helpers_test.go b/cmd/roborev/helpers_test.go index 24727c48..b2532fa6 100644 --- a/cmd/roborev/helpers_test.go +++ b/cmd/roborev/helpers_test.go @@ -43,6 +43,21 @@ func newTestGitRepo(t *testing.T) *TestGitRepo { return r } +// newBareTestGitRepo creates a bare git repository for use as a remote. +func newBareTestGitRepo(t *testing.T) *TestGitRepo { + t.Helper() + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + dir := t.TempDir() + resolved, err := filepath.EvalSymlinks(dir) + require.NoError(t, err, "Failed to resolve symlinks: %v") + + r := &TestGitRepo{Dir: resolved, t: t} + r.Run("init", "--bare") + return r +} + // chdir changes to dir and registers a t.Cleanup to restore the original directory. func chdir(t *testing.T, dir string) { t.Helper() diff --git a/cmd/roborev/refine.go b/cmd/roborev/refine.go index 489ab664..ae45765e 100644 --- a/cmd/roborev/refine.go +++ b/cmd/roborev/refine.go @@ -207,12 +207,14 @@ func (t *stepTimer) stopLive() { } // validateRefineContext validates git and branch preconditions for refine. -// Returns repoPath, currentBranch, defaultBranch, mergeBase, or an error. +// Returns repoPath, currentBranch, base, mergeBase, or an error. +// base is the ref that HEAD was diverged from — the branch's upstream +// tracking ref when configured, otherwise the repository's default branch. // If branchFlag is non-empty, validates that the user is on that branch. // This validation happens before any daemon interaction. func validateRefineContext( cwd, since, branchFlag string, -) (repoPath, currentBranch, defaultBranch, mergeBase string, err error) { +) (repoPath, currentBranch, base, mergeBase string, err error) { repoPath, err = git.GetRepoRoot(cwd) if err != nil { return "", "", "", "", @@ -235,14 +237,6 @@ func validateRefineContext( ) } - defaultBranch, err = git.GetDefaultBranch(repoPath) - if err != nil { - return "", "", "", "", - fmt.Errorf( - "cannot determine default branch: %w", err, - ) - } - currentBranch = git.GetCurrentBranch(repoPath) // --branch: validate the user is on the expected branch @@ -255,6 +249,9 @@ func validateRefineContext( } if since != "" { + // --since provides an explicit merge base, so upstream/default-branch + // resolution is unnecessary. Skip it so a misconfigured or unfetched + // upstream doesn't block an otherwise-valid --since invocation. mergeBase, err = git.ResolveSHA(repoPath, since) if err != nil { return "", "", "", "", @@ -281,27 +278,58 @@ func validateRefineContext( ) } } else { - if currentBranch == git.LocalBranchName(defaultBranch) { + // Prefer the current branch's upstream tracking ref only when it + // resolves to a trunk-named branch (e.g., local main tracking + // upstream/main in a fork). A branch tracking its own remote + // counterpart is not trunk — use GetDefaultBranch instead. + upstream, uerr := git.GetUpstream(repoPath, "HEAD") + var missing *git.UpstreamMissingError + if errors.As(uerr, &missing) { + return "", "", "", "", + fmt.Errorf( + "%w (run 'git fetch' or pass --since)", missing, + ) + } + if uerr != nil { + return "", "", "", "", + fmt.Errorf( + "resolve upstream for HEAD: %w (pass --since to skip)", uerr, + ) + } + if upstream != "" && git.UpstreamIsTrunk(repoPath, "HEAD") { + base = upstream + } + if base == "" { + base, err = git.GetDefaultBranch(repoPath) + if err != nil { + return "", "", "", "", + fmt.Errorf( + "cannot determine default branch: %w", err, + ) + } + } + + if git.IsOnBaseBranch(repoPath, currentBranch, base) { return "", "", "", "", fmt.Errorf( "refusing to refine on %s branch "+ "without --since flag", - git.LocalBranchName(defaultBranch), + currentBranch, ) } mergeBase, err = git.GetMergeBase( - repoPath, defaultBranch, "HEAD", + repoPath, base, "HEAD", ) if err != nil { return "", "", "", "", fmt.Errorf( "cannot find merge-base with %s: %w", - defaultBranch, err, + base, err, ) } } - return repoPath, currentBranch, defaultBranch, mergeBase, nil + return repoPath, currentBranch, base, mergeBase, nil } // RunContext encapsulates the runtime context for the refine command, @@ -314,7 +342,7 @@ type RunContext struct { func runRefine(ctx RunContext, opts refineOptions) error { // 1. Validate git and branch context (before touching daemon) - repoPath, currentBranch, defaultBranch, mergeBase, err := validateRefineContext( + repoPath, currentBranch, base, mergeBase, err := validateRefineContext( ctx.WorkingDir, opts.since, opts.branch, ) if err != nil { @@ -368,7 +396,7 @@ func runRefine(ctx RunContext, opts refineOptions) error { if opts.since != "" { fmt.Printf("Refining commits since %s on branch %q\n", git.ShortSHA(mergeBase), currentBranch) } else { - fmt.Printf("Refining branch %q (diverged from %s at %s)\n", currentBranch, defaultBranch, git.ShortSHA(mergeBase)) + fmt.Printf("Refining branch %q (diverged from %s at %s)\n", currentBranch, base, git.ShortSHA(mergeBase)) } allowUnsafe := resolveAllowUnsafeAgents(opts.allowUnsafeAgents, opts.unsafeFlagChanged, cfg) diff --git a/cmd/roborev/refine_test.go b/cmd/roborev/refine_test.go index 85c31f7f..ef242192 100644 --- a/cmd/roborev/refine_test.go +++ b/cmd/roborev/refine_test.go @@ -11,6 +11,7 @@ import ( "github.com/roborev-dev/roborev/internal/agent" "github.com/roborev-dev/roborev/internal/config" "github.com/roborev-dev/roborev/internal/daemon" + "github.com/roborev-dev/roborev/internal/git" "github.com/roborev-dev/roborev/internal/storage" "github.com/roborev-dev/roborev/internal/testutil" "github.com/stretchr/testify/assert" @@ -563,6 +564,54 @@ func TestValidateRefineContext_AllowsMainBranchWithSince(t *testing.T) { assert.Equal(t, mergeBase, baseSHA) } +func TestValidateRefineContext_SinceIgnoresUpstreamMissing(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + + // --since provides the merge base explicitly, so an unfetched or + // missing @{upstream} must not block a valid --since invocation. + repo := testutil.InitTestRepo(t) + baseSHA := repo.RevParse("HEAD") + repo.CommitFile("second.txt", "second", "second commit") + + // Configure tracking against an upstream that never resolves locally. + repo.Run("config", "branch.main.remote", "upstream") + repo.Run("config", "branch.main.merge", "refs/heads/main") + + chdirForTest(t, repo.Root) + + repoPath, currentBranch, _, mergeBase, err := validateRefineContext("", baseSHA, "") + require.NoError(t, err, "--since should bypass upstream resolution: %v", err) + + assert.NotEmpty(t, repoPath) + assert.Equal(t, "main", currentBranch) + assert.Equal(t, baseSHA, mergeBase) +} + +func TestValidateRefineContext_UpstreamMissingErrorsWithoutSince(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + + // Without --since, a configured-but-unfetched upstream must surface + // as UpstreamMissingError rather than silently falling back to the + // repository default branch (which could yield the wrong range). + repo := testutil.InitTestRepo(t) + repo.RunGit("checkout", "-b", "feature") + repo.CommitFile("feature.txt", "feature", "feature commit") + repo.Run("config", "branch.feature.remote", "upstream") + repo.Run("config", "branch.feature.merge", "refs/heads/main") + + chdirForTest(t, repo.Root) + + _, _, _, _, err := validateRefineContext("", "", "") + require.Error(t, err, "expected missing-upstream error without --since") + + var missing *git.UpstreamMissingError + assert.ErrorAs(t, err, &missing, "expected UpstreamMissingError, got: %v", err) +} + func TestValidateRefineContext_SinceWorksOnFeatureBranch(t *testing.T) { if _, err := exec.LookPath("git"); err != nil { t.Skip("git not available") @@ -619,6 +668,69 @@ func TestValidateRefineContext_SinceNotAncestorOfHEAD(t *testing.T) { assert.Contains(t, err.Error(), "not an ancestor of HEAD") } +func TestValidateRefineContext_PrefersNonOriginUpstream(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + + // Fork workflow: origin (stale fork) lags behind upstream (real repo). + // Feature branch tracks upstream/main. validateRefineContext must + // pick upstream/main as the base so commits already merged upstream + // are not refined. + repo := testutil.InitTestRepo(t) + repo.CommitFile("upstream_c2.go", "package main", "upstream c2") + upstreamSHA := repo.RevParse("HEAD") + + bareRemote := t.TempDir() + bareStale := t.TempDir() + repo.Run("init", "--bare", bareRemote) + repo.Run("init", "--bare", bareStale) + repo.Run("remote", "add", "upstream", bareRemote) + repo.Run("push", "-u", "upstream", "main") + + repo.Run("remote", "add", "origin", bareStale) + repo.Run("push", "origin", "HEAD~1:refs/heads/main") + repo.Run("fetch", "origin") + repo.Run("remote", "set-head", "origin", "main") + + repo.RunGit("checkout", "-b", "feature", "--track", "upstream/main") + repo.CommitFile("feature.go", "package main", "feature commit") + + chdirForTest(t, repo.Root) + + repoPath, currentBranch, base, mergeBase, err := validateRefineContext("", "", "") + require.NoError(t, err, "validation should pass on feature tracking upstream/main") + + assert.NotEmpty(t, repoPath) + assert.Equal(t, "feature", currentBranch) + assert.Equal(t, "upstream/main", base, + "base must resolve to the branch's upstream, not origin/main") + assert.Equal(t, upstreamSHA, mergeBase, + "merge-base must be upstream/main's tip so already-merged commits aren't refined") +} + +func TestValidateRefineContext_RefusesLocalMainTrackingNonOriginUpstream(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + + // Local main tracking upstream/main must still hit the "refusing to + // refine on main" guardrail even though base is "upstream/main" + // (not "origin/main"). Regression for IsOnBaseBranch's non-origin + // remote handling. + repo := testutil.InitTestRepo(t) + bareRemote := t.TempDir() + repo.Run("init", "--bare", bareRemote) + repo.Run("remote", "add", "upstream", bareRemote) + repo.Run("push", "-u", "upstream", "main") + + chdirForTest(t, repo.Root) + + _, _, _, _, err := validateRefineContext("", "", "") + require.Error(t, err, "expected refuse-on-base error for main tracking upstream/main") + assert.Contains(t, err.Error(), "refusing to refine on main") +} + func TestValidateRefineContext_FeatureBranchWithoutSinceStillWorks(t *testing.T) { if _, err := exec.LookPath("git"); err != nil { t.Skip("git not available") diff --git a/cmd/roborev/review.go b/cmd/roborev/review.go index 67620bab..39e7d7fe 100644 --- a/cmd/roborev/review.go +++ b/cmd/roborev/review.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -160,6 +161,25 @@ Examples: } base := baseBranch + if base == "" { + // Prefer the branch's upstream tracking ref only when it resolves + // to a trunk-named branch (e.g., local main tracking upstream/main + // in a fork). A branch tracking its own remote counterpart + // (e.g., feature tracking origin/feature) is not trunk — using it + // would skip already-pushed feature commits, contradicting + // "--branch reviews all commits since trunk". + upstream, uerr := git.GetUpstream(root, targetRef) + var missing *git.UpstreamMissingError + if errors.As(uerr, &missing) { + return fmt.Errorf("%w (or pass --base )", missing) + } + if uerr != nil { + return fmt.Errorf("resolve upstream for %s: %w (pass --base to skip)", targetRef, uerr) + } + if upstream != "" && git.UpstreamIsTrunk(root, targetRef) { + base = upstream + } + } if base == "" { var err error base, err = git.GetDefaultBranch(root) @@ -168,11 +188,14 @@ Examples: } } - // Validate not on base branch (only when reviewing current branch) + // Validate not on base branch (only when reviewing current branch). + // With UpstreamIsTrunk gating, `base` is either the user's --base + // override, a trunk-shaped upstream, or GetDefaultBranch — never a + // self-counterpart, so this check fires only for true trunk cases. if targetRef == "HEAD" { currentBranch := git.GetCurrentBranch(root) - if currentBranch == git.LocalBranchName(base) { - return fmt.Errorf("already on %s - create a feature branch first", git.LocalBranchName(base)) + if git.IsOnBaseBranch(root, currentBranch, base) { + return fmt.Errorf("already on %s - create a feature branch first", currentBranch) } } @@ -504,6 +527,19 @@ func tryBranchReview(root, baseBranchOverride string) (string, bool) { } base := baseBranchOverride + if base == "" { + // Prefer the branch's upstream tracking ref only when it resolves to + // trunk. Hooks must never block commits, but any GetUpstream failure + // (missing ref, corrupt config, subprocess error) means we cannot + // confidently pick a base — skip instead of falling back. + upstream, err := git.GetUpstream(root, "HEAD") + if err != nil { + return "", false + } + if upstream != "" && git.UpstreamIsTrunk(root, "HEAD") { + base = upstream + } + } if base == "" { var err error base, err = git.GetDefaultBranch(root) @@ -514,7 +550,7 @@ func tryBranchReview(root, baseBranchOverride string) (string, bool) { // Don't branch-review in detached HEAD or on the base branch current := git.GetCurrentBranch(root) - if current == "" || current == git.LocalBranchName(base) { + if current == "" || git.IsOnBaseBranch(root, current, base) { return "", false } diff --git a/cmd/roborev/review_test.go b/cmd/roborev/review_test.go index f711276e..a2864cc5 100644 --- a/cmd/roborev/review_test.go +++ b/cmd/roborev/review_test.go @@ -610,6 +610,108 @@ func TestTryBranchReview(t *testing.T) { _, ok := tryBranchReview(repo.Dir, "") assert.False(t, ok) }) + + t.Run("allows feature branch tracking its own remote counterpart", func(t *testing.T) { + // Regression: using @{upstream} as base for a feature tracking + // origin/feature would skip already-pushed feature commits, which + // contradicts "--branch reviews all commits since trunk". With + // UpstreamIsTrunk gating, origin/feature (not trunk-named) is + // rejected and the review falls back to the repository's default + // branch for the merge base. + remote := newBareTestGitRepo(t) + repo := newTestGitRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("file.txt", "content", "initial") + repo.Run("remote", "add", "origin", remote.Dir) + repo.Run("push", "-u", "origin", "main") + repo.Run("remote", "set-head", "origin", "main") + mainSHA := repo.Run("rev-parse", "origin/main") + repo.Run("checkout", "-b", "feature") + repo.CommitFile("first.txt", "first", "first feature commit") + repo.Run("push", "-u", "origin", "feature") + repo.CommitFile("second.txt", "second", "unpushed commit") + writeRoborevConfig(t, repo, `post_commit_review = "branch"`) + + ref, ok := tryBranchReview(repo.Dir, "") + require.True(t, ok, "feature tracking origin/feature with unpushed commits must still enqueue a review") + // Merge-base range runs from origin/main (trunk), covering BOTH the + // pushed and unpushed feature commits — the documented --branch + // semantic. Using @{u}=origin/feature would have skipped the pushed + // one. + assert.Equal(t, mainSHA+"..HEAD", ref) + }) + + t.Run("skips when upstream is configured but unresolvable", func(t *testing.T) { + // Regression: when @{upstream} is configured but the remote-tracking + // ref has not been fetched, tryBranchReview must skip rather than + // silently fall back to origin/main or local main — that fallback + // would enqueue a review against the wrong commit range in fork + // workflows. + repo := newTestGitRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("file.txt", "content", "initial") + repo.Run("checkout", "-b", "feature") + repo.CommitFile("feature.txt", "feature", "feature commit") + // Configure tracking against an upstream that never resolves locally. + repo.Run("config", "branch.feature.remote", "upstream") + repo.Run("config", "branch.feature.merge", "refs/heads/main") + writeRoborevConfig(t, repo, `post_commit_review = "branch"`) + + ref, ok := tryBranchReview(repo.Dir, "") + assert.False(t, ok, "must skip when upstream is configured but missing") + assert.Empty(t, ref) + }) + + t.Run("blocks local main tracking non-origin upstream", func(t *testing.T) { + // Regression: LocalBranchName only stripped "origin/", so + // current="main" vs base="upstream/main" missed the guardrail + // and produced a branch review on the base branch itself. + remote := newBareTestGitRepo(t) + repo := newTestGitRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("file.txt", "content", "initial") + repo.Run("remote", "add", "upstream", remote.Dir) + repo.Run("push", "-u", "upstream", "main") + writeRoborevConfig(t, repo, `post_commit_review = "branch"`) + + _, ok := tryBranchReview(repo.Dir, "") + assert.False(t, ok, "local main tracking upstream/main must be treated as base branch") + }) + + t.Run("prefers branch upstream over default branch", func(t *testing.T) { + // Reproduces the "single commit past upstream" bug: when the default + // branch (e.g., origin/main) lags behind the tip the branch actually + // diverged from (e.g., upstream/main), the review must use the + // upstream tracking ref, not the default branch, so already-merged + // commits are not re-reviewed. + remote := newBareTestGitRepo(t) + repo := newTestGitRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("file.txt", "u1", "upstream c1") + repo.CommitFile("file.txt", "u1\nu2", "upstream c2") + repo.CommitFile("file.txt", "u1\nu2\nu3", "upstream c3") + repo.Run("remote", "add", "upstream", remote.Dir) + repo.Run("push", "-u", "upstream", "main") + + // Simulate origin lagging behind upstream by 2 commits. + staleOrigin := newBareTestGitRepo(t) + repo.Run("remote", "add", "origin", staleOrigin.Dir) + repo.Run("push", "origin", "HEAD~2:refs/heads/main") + repo.Run("fetch", "origin") + repo.Run("remote", "set-head", "origin", "main") + + repo.Run("checkout", "-b", "feature", "--track", "upstream/main") + repo.CommitFile("feature.txt", "only-new-commit", "feature commit") + writeRoborevConfig(t, repo, `post_commit_review = "branch"`) + + ref, ok := tryBranchReview(repo.Dir, "") + require.True(t, ok, "expected branch review to run") + + upstreamSHA := repo.Run("rev-parse", "upstream/main") + want := upstreamSHA + "..HEAD" + assert.Equal(t, want, ref, + "review must compare against upstream/main, not origin/main") + }) } func TestReviewIgnoresBranchConfig(t *testing.T) { diff --git a/internal/git/git.go b/internal/git/git.go index d7dbd540..dab09019 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -9,6 +9,7 @@ import ( "os/exec" "path/filepath" "runtime" + "sort" "strings" "time" "unicode/utf8" @@ -100,6 +101,109 @@ func LocalBranchName(branch string) string { return strings.TrimPrefix(branch, "origin/") } +// IsOnBaseBranch returns true if currentBranch is equivalent to base for the +// purpose of "already on the base branch" guardrails. Bare local names +// ("main") match directly. Slash-containing bases are classified by +// namespace: an unambiguous remote-tracking ref (refs/remotes/ exists, +// refs/heads/ does not) is stripped of its configured-remote prefix +// before comparison; an unambiguous local branch (only refs/heads/ +// exists) is compared verbatim; and an ambiguous name (both exist) is +// refused so neither the pathological local "origin/main" nor a shadowed +// remote-tracking ref is treated as "already on base". +func IsOnBaseBranch(repoPath, currentBranch, base string) bool { + if currentBranch == "" { + return false + } + if !strings.Contains(base, "/") { + return currentBranch == base + } + remoteExists := refExists(repoPath, "refs/remotes/"+base) + localExists := refExists(repoPath, "refs/heads/"+base) + if !remoteExists && !localExists { + return false + } + if localExists && !remoteExists { + return currentBranch == base + } + if localExists && remoteExists { + return false + } + // remoteExists && !localExists — unambiguously remote-tracking. + stripped := stripRemotePrefix(repoPath, base) + if stripped == base { + // The prefix did not match any configured remote name; bail out + // rather than matching against an unstripped slashy name. + return false + } + return currentBranch == stripped +} + +// UpstreamIsTrunk reports whether ref's @{upstream} points to the +// repository's trunk. Only remote-tracking upstreams can be trunk: a +// local-branch upstream (configured via branch..remote = ".") is +// rejected even if its short name coincidentally matches the default +// branch. For unambiguous remote-tracking upstreams, the branch part +// after stripping the configured remote prefix must exactly match the +// branch part of GetDefaultBranch. Returns false if ref has no upstream +// configured or the default branch cannot be detected. +func UpstreamIsTrunk(repoPath, ref string) bool { + cfg, ok := readUpstreamConfig(repoPath, ref) + if !ok { + return false + } + if !strings.HasPrefix(cfg.qualified, "refs/remotes/") { + return false + } + defaultBranch, err := GetDefaultBranch(repoPath) + if err != nil { + return false + } + return stripRemotePrefix(repoPath, cfg.short) == stripRemotePrefix(repoPath, defaultBranch) +} + +// stripRemotePrefix removes the longest configured-remote prefix from ref. +// "origin/main" → "main", "company/fork/main" → "main" (when "company/fork" is +// a remote), "origin/team/main" → "team/main" (when only "origin" is a remote +// — "origin/team" is not), and bare "main" → "main". +func stripRemotePrefix(repoPath, ref string) string { + if !strings.Contains(ref, "/") { + return ref + } + remotes, err := listRemotes(repoPath) + if err != nil { + return ref + } + // Longest match first so multi-slash remote names (e.g. "company/fork") are + // tested before shorter prefixes that happen to overlap. + sort.Slice(remotes, func(i, j int) bool { return len(remotes[i]) > len(remotes[j]) }) + for _, r := range remotes { + if stripped, ok := strings.CutPrefix(ref, r+"/"); ok { + return stripped + } + } + return ref +} + +// refExists reports whether the given fully-qualified ref resolves in the repo. +func refExists(repoPath, fullRef string) bool { + cmd := exec.Command("git", "rev-parse", "--verify", "--quiet", fullRef) + cmd.Dir = repoPath + return cmd.Run() == nil +} + +// listRemotes returns the names of configured remotes (e.g. ["origin", "upstream"]). +// Uses strings.Fields so CRLF-terminated output on Windows doesn't leave stray +// "\r" on remote names (which would break prefix matching downstream). +func listRemotes(repoPath string) ([]string, error) { + cmd := exec.Command("git", "remote") + cmd.Dir = repoPath + out, err := cmd.Output() + if err != nil { + return nil, fmt.Errorf("git remote: %w", err) + } + return strings.Fields(string(out)), nil +} + // GetDiff returns the full diff for a commit, excluding generated // files like lock files. Extra exclude patterns (filenames or globs) // are appended to the built-in exclusion list. @@ -1138,6 +1242,119 @@ func GetDefaultBranch(repoPath string) (string, error) { return "", fmt.Errorf("could not detect default branch (tried origin/HEAD, main, master)") } +// UpstreamMissingError reports that a branch's @{upstream} is configured but +// the referenced ref does not resolve locally (e.g., the remote-tracking ref +// has not been fetched or was deleted). Callers should surface this to the +// user instead of silently falling back to a different base branch, which +// could select the wrong commit range in fork workflows. +type UpstreamMissingError struct { + Ref string // The branch whose upstream was resolved (e.g., "HEAD" or "feature"). + Upstream string // The configured upstream name (e.g., "upstream/main"). +} + +func (e *UpstreamMissingError) Error() string { + return fmt.Sprintf("upstream %q for %s does not resolve locally (try 'git fetch')", e.Upstream, e.Ref) +} + +// GetUpstream returns the upstream tracking branch for a ref (e.g., "upstream/main"). +// Returns ("", nil) when no @{upstream} is configured, so callers can fall back +// to a default base. Returns ("", *UpstreamMissingError) when @{upstream} is +// configured but the referenced ref does not resolve locally — callers should +// surface this instead of falling back, because the fallback target may select +// the wrong commit range. Passing an empty ref is equivalent to HEAD. +func GetUpstream(repoPath, ref string) (string, error) { + if ref == "" { + ref = "HEAD" + } + cmd := exec.Command("git", "rev-parse", "--abbrev-ref", "--symbolic-full-name", ref+"@{upstream}") + cmd.Dir = repoPath + + out, err := cmd.Output() + if err != nil { + // Exit code 128 covers both "no upstream configured" and "upstream + // configured but ref not resolvable" (git varies between versions). + // Distinguish by re-reading branch..remote/merge. + if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 128 { + if cfg, ok := readUpstreamConfig(repoPath, ref); ok { + return "", &UpstreamMissingError{Ref: ref, Upstream: cfg.short} + } + return "", nil + } + return "", fmt.Errorf("git rev-parse @{upstream}: %w", err) + } + + upstream := strings.TrimSpace(string(out)) + if upstream == "" { + return "", nil + } + // Verify the exact namespace the tracking config points to so a lookalike + // ref with the same short name (local branch, tag) can't shadow a missing + // remote-tracking ref. Fall back to an unqualified check if the config + // can't be read — e.g., for detached HEAD callers. + if cfg, ok := readUpstreamConfig(repoPath, ref); ok { + if !refExists(repoPath, cfg.qualified) { + return "", &UpstreamMissingError{Ref: ref, Upstream: upstream} + } + } else if !refExists(repoPath, upstream) { + return "", &UpstreamMissingError{Ref: ref, Upstream: upstream} + } + return upstream, nil +} + +// upstreamConfig captures the resolved short name and fully-qualified ref +// implied by branch..remote and branch..merge. +type upstreamConfig struct { + short string // e.g. "upstream/main" or "main" for local tracking + qualified string // e.g. "refs/remotes/upstream/main" or "refs/heads/main" +} + +// readUpstreamConfig returns the upstream configuration for a ref. Returns +// (cfg, true) when branch..remote and branch..merge are set, +// indicating @{upstream} is configured even if rev-parse cannot resolve the +// ref. Returns (zero, false) otherwise. +func readUpstreamConfig(repoPath, ref string) (upstreamConfig, bool) { + branch := ref + if branch == "HEAD" || branch == "" { + branch = GetCurrentBranch(repoPath) + if branch == "" { + return upstreamConfig{}, false + } + } else { + // Accept fully-qualified local branch refs (e.g., "refs/heads/feature") + // so callers who pass a ResolveSHA-style ref still resolve to the + // correct branch..* config keys. + branch = strings.TrimPrefix(branch, "refs/heads/") + } + remote := readGitConfig(repoPath, "branch."+branch+".remote") + merge := readGitConfig(repoPath, "branch."+branch+".merge") + if remote == "" || merge == "" { + return upstreamConfig{}, false + } + mergeBranch := strings.TrimPrefix(merge, "refs/heads/") + if remote == "." { + // Local-branch tracking writes the target verbatim. + return upstreamConfig{ + short: mergeBranch, + qualified: "refs/heads/" + mergeBranch, + }, true + } + return upstreamConfig{ + short: remote + "/" + mergeBranch, + qualified: "refs/remotes/" + remote + "/" + mergeBranch, + }, true +} + +// readGitConfig returns the value of a git config key, or "" if missing. +func readGitConfig(repoPath, key string) string { + cmd := exec.Command("git", "config", "--get", key) + cmd.Dir = repoPath + out, err := cmd.Output() + if err != nil { + return "" + } + return strings.TrimSpace(string(out)) +} + // GetMergeBase returns the merge-base (common ancestor) between two refs func GetMergeBase(repoPath, ref1, ref2 string) (string, error) { cmd := exec.Command("git", "merge-base", ref1, ref2) diff --git a/internal/git/git_test.go b/internal/git/git_test.go index 54ec6696..f76ab5d7 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -638,6 +638,173 @@ func TestGetCurrentBranch(t *testing.T) { }) } +func TestGetUpstream(t *testing.T) { + t.Run("returns empty when no upstream configured", func(t *testing.T) { + repo := NewTestRepoWithCommit(t) + + upstream, err := GetUpstream(repo.Dir, "HEAD") + require.NoError(t, err) + assert.Empty(t, upstream) + }) + + t.Run("returns upstream tracking branch", func(t *testing.T) { + remote := NewBareTestRepo(t) + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("file.txt", "content", "initial") + + // Name the remote "upstream" to match the user's fork-style setup. + repo.Run("remote", "add", "upstream", remote.Dir) + repo.Run("push", "-u", "upstream", "main") + repo.Run("checkout", "-b", "feature", "--track", "upstream/main") + + upstream, err := GetUpstream(repo.Dir, "HEAD") + require.NoError(t, err) + assert.Equal(t, "upstream/main", upstream) + }) + + t.Run("returns upstream for named ref", func(t *testing.T) { + remote := NewBareTestRepo(t) + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("file.txt", "content", "initial") + + repo.Run("remote", "add", "origin", remote.Dir) + repo.Run("push", "-u", "origin", "main") + repo.Run("checkout", "-b", "feature") + + // feature has no upstream, but main does. + upstream, err := GetUpstream(repo.Dir, "main") + require.NoError(t, err) + assert.Equal(t, "origin/main", upstream) + + upstream, err = GetUpstream(repo.Dir, "feature") + require.NoError(t, err) + assert.Empty(t, upstream) + }) + + t.Run("empty ref defaults to HEAD", func(t *testing.T) { + remote := NewBareTestRepo(t) + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("file.txt", "content", "initial") + + repo.Run("remote", "add", "origin", remote.Dir) + repo.Run("push", "-u", "origin", "main") + + upstream, err := GetUpstream(repo.Dir, "") + require.NoError(t, err) + assert.Equal(t, "origin/main", upstream) + }) + + t.Run("accepts local-branch upstream", func(t *testing.T) { + // `git branch -u ` sets tracking against a local ref + // under refs/heads/... (no remote involved). GetUpstream must not + // reject this as "missing" just because refs/remotes/ + // doesn't exist. + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("file.txt", "content", "initial") + repo.Run("checkout", "-b", "dev") + repo.Run("branch", "-u", "main", "dev") + + upstream, err := GetUpstream(repo.Dir, "HEAD") + require.NoError(t, err) + assert.Equal(t, "main", upstream, + "local-branch upstream should be returned, not dropped") + }) + + t.Run("errors when tracking ref is missing locally", func(t *testing.T) { + // Tracking config set but refs/remotes/ does not resolve + // (e.g., never fetched, or was manually removed). Callers must be + // able to distinguish this from "no upstream configured" so the + // user isn't silently switched to a different base branch that + // could yield the wrong commit range. + remote := NewBareTestRepo(t) + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("file.txt", "content", "initial") + repo.Run("remote", "add", "upstream", remote.Dir) + repo.Run("push", "-u", "upstream", "main") + // Tracking config now points at refs/remotes/upstream/main. Remove it. + repo.Run("update-ref", "-d", "refs/remotes/upstream/main") + + upstream, err := GetUpstream(repo.Dir, "HEAD") + assert.Empty(t, upstream) + var missing *UpstreamMissingError + require.ErrorAs(t, err, &missing, "expected UpstreamMissingError, got %T: %v", err, err) + assert.Equal(t, "upstream/main", missing.Upstream) + }) + + t.Run("errors when tracking is configured but never fetched", func(t *testing.T) { + // Fresh repo with manual tracking config against a ref that has + // never been fetched. rev-parse @{upstream} fails with exit 128, + // but branch..remote/merge are set, so callers must still + // see UpstreamMissingError, not ("", nil). + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("file.txt", "content", "initial") + repo.Run("config", "branch.main.remote", "upstream") + repo.Run("config", "branch.main.merge", "refs/heads/main") + + upstream, err := GetUpstream(repo.Dir, "HEAD") + assert.Empty(t, upstream) + var missing *UpstreamMissingError + require.ErrorAs(t, err, &missing, "expected UpstreamMissingError, got %T: %v", err, err) + assert.Equal(t, "upstream/main", missing.Upstream) + }) + + t.Run("handles branch names containing dots", func(t *testing.T) { + // Regression: git parses section/subsection/key by splitting on the + // first and last dots, so "branch.release/1.2.3.remote" correctly + // extracts subsection "release/1.2.3" and key "remote". Confirm + // GetUpstream returns UpstreamMissingError for a dotted branch name + // whose tracking ref has been removed. + remote := NewBareTestRepo(t) + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("file.txt", "content", "initial") + repo.Run("remote", "add", "upstream", remote.Dir) + repo.Run("push", "-u", "upstream", "main") + repo.Run("checkout", "-b", "release/1.2.3", "--track", "upstream/main") + repo.Run("update-ref", "-d", "refs/remotes/upstream/main") + + upstream, err := GetUpstream(repo.Dir, "HEAD") + assert.Empty(t, upstream) + var missing *UpstreamMissingError + require.ErrorAs(t, err, &missing, + "dotted branch name must not silently fall back to default") + assert.Equal(t, "upstream/main", missing.Upstream) + }) + + t.Run("errors when remote ref missing despite a colliding local ref", func(t *testing.T) { + // Regression: an unqualified refExists check can pass for a local + // branch whose short name equals the upstream short name, masking + // a missing refs/remotes//... and causing downstream + // merge-base to use the wrong ref. Verify the tracking config's + // qualified ref explicitly. + remote := NewBareTestRepo(t) + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("file.txt", "content", "initial") + repo.Run("remote", "add", "upstream", remote.Dir) + repo.Run("push", "-u", "upstream", "main") + // Remove the remote-tracking ref that @{upstream} points to… + repo.Run("update-ref", "-d", "refs/remotes/upstream/main") + // …and create a local ref with the identical short name so an + // unqualified rev-parse would succeed. + head := repo.HeadSHA() + repo.Run("update-ref", "refs/heads/upstream/main", head) + + upstream, err := GetUpstream(repo.Dir, "HEAD") + assert.Empty(t, upstream) + var missing *UpstreamMissingError + require.ErrorAs(t, err, &missing, + "lookalike local ref must not satisfy the remote upstream check") + assert.Equal(t, "upstream/main", missing.Upstream) + }) +} + func TestHasUncommittedChanges(t *testing.T) { t.Run("no changes", func(t *testing.T) { repo := NewTestRepoWithCommit(t) @@ -1125,6 +1292,275 @@ func TestResetWorkingTree(t *testing.T) { }) } +func TestUpstreamIsTrunk(t *testing.T) { + t.Run("trunk-named upstream matches", func(t *testing.T) { + remote := NewBareTestRepo(t) + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("file.txt", "content", "initial") + repo.Run("remote", "add", "origin", remote.Dir) + repo.Run("push", "-u", "origin", "main") + repo.Run("remote", "set-head", "origin", "main") + + assert.True(t, UpstreamIsTrunk(repo.Dir, "HEAD")) + }) + + t.Run("self-counterpart upstream does not match", func(t *testing.T) { + remote := NewBareTestRepo(t) + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("file.txt", "content", "initial") + repo.Run("remote", "add", "origin", remote.Dir) + repo.Run("push", "-u", "origin", "main") + repo.Run("remote", "set-head", "origin", "main") + repo.Run("checkout", "-b", "feature") + repo.Run("push", "-u", "origin", "feature") + + // feature tracks origin/feature (its own remote counterpart) — not trunk. + assert.False(t, UpstreamIsTrunk(repo.Dir, "HEAD")) + }) + + t.Run("multi-remote trunk matches", func(t *testing.T) { + // Fork workflow: local main tracks upstream/main while the default + // branch is origin/main. Both refs strip to "main" → trunk. + remote := NewBareTestRepo(t) + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("file.txt", "content", "initial") + repo.Run("remote", "add", "origin", remote.Dir) + repo.Run("push", "-u", "origin", "main") + repo.Run("remote", "set-head", "origin", "main") + upRemote := NewBareTestRepo(t) + repo.Run("remote", "add", "upstream", upRemote.Dir) + repo.Run("push", "upstream", "main") + repo.Run("branch", "-u", "upstream/main", "main") + + assert.True(t, UpstreamIsTrunk(repo.Dir, "HEAD")) + }) + + t.Run("returns false when no upstream is configured", func(t *testing.T) { + repo := NewTestRepoWithCommit(t) + assert.False(t, UpstreamIsTrunk(repo.Dir, "HEAD")) + }) + + t.Run("returns false when default branch cannot be detected", func(t *testing.T) { + // Branch tracks some upstream, but origin/HEAD and main/master are + // all missing so GetDefaultBranch fails. + remote := NewBareTestRepo(t) + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/trunk") + repo.CommitFile("file.txt", "content", "initial") + repo.Run("remote", "add", "origin", remote.Dir) + repo.Run("push", "-u", "origin", "trunk") + + assert.False(t, UpstreamIsTrunk(repo.Dir, "HEAD")) + }) + + t.Run("feature branch whose leaf matches default is not trunk", func(t *testing.T) { + // Regression: a feature branch tracking e.g. origin/team/main has + // last path segment "main" and would wrongly match the default + // leaf. UpstreamIsTrunk must compare the full branch name after + // stripping the configured remote prefix, not just the leaf. + remote := NewBareTestRepo(t) + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("file.txt", "content", "initial") + repo.Run("remote", "add", "origin", remote.Dir) + repo.Run("push", "-u", "origin", "main") + repo.Run("remote", "set-head", "origin", "main") + // Simulate a feature branch whose remote-tracking ref ends in "main" + // but isn't trunk. + head := repo.HeadSHA() + repo.Run("update-ref", "refs/remotes/origin/team/main", head) + repo.Run("checkout", "-b", "team/main") + repo.Run("config", "branch.team/main.remote", "origin") + repo.Run("config", "branch.team/main.merge", "refs/heads/team/main") + + assert.False(t, UpstreamIsTrunk(repo.Dir, "HEAD"), + "branch tracking origin/team/main is not trunk — its branch part is team/main, not main") + }) + + t.Run("accepts refs/heads/-qualified branch refs", func(t *testing.T) { + // Regression: callers that pass a fully-qualified ref (e.g., + // "refs/heads/feature" or output of ResolveSHA) must still hit + // the branch..* config keys after the prefix is stripped. + remote := NewBareTestRepo(t) + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("file.txt", "content", "initial") + repo.Run("remote", "add", "origin", remote.Dir) + repo.Run("push", "-u", "origin", "main") + repo.Run("remote", "set-head", "origin", "main") + + assert.True(t, UpstreamIsTrunk(repo.Dir, "refs/heads/main"), + "refs/heads/-qualified ref should resolve to the same branch config as 'HEAD'") + }) + + t.Run("local-branch upstream is not trunk even if its name matches default", func(t *testing.T) { + // Regression: a branch can track a local branch via + // branch..remote = ".". If the tracked local branch is + // literally named "origin/main" in a repo whose default is + // origin/main, stripRemotePrefix would normalize both to "main" + // and misclassify the local upstream as trunk. The namespace + // check (refs/heads/... vs refs/remotes/...) must reject the + // local-branch upstream. + remote := NewBareTestRepo(t) + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("file.txt", "content", "initial") + repo.Run("remote", "add", "origin", remote.Dir) + repo.Run("push", "-u", "origin", "main") + repo.Run("remote", "set-head", "origin", "main") + head := repo.HeadSHA() + // Local branch literally named "origin/main". + repo.Run("update-ref", "refs/heads/origin/main", head) + // New branch whose upstream is the LOCAL "origin/main", via remote=".". + repo.Run("checkout", "-b", "pinned", head) + repo.Run("config", "branch.pinned.remote", ".") + repo.Run("config", "branch.pinned.merge", "refs/heads/origin/main") + + assert.False(t, UpstreamIsTrunk(repo.Dir, "HEAD"), + "local-branch upstream named origin/main must not be classified as trunk") + }) +} + +func TestIsOnBaseBranch(t *testing.T) { + t.Run("matches bare local name", func(t *testing.T) { + repo := NewTestRepoWithCommit(t) + assert.True(t, IsOnBaseBranch(repo.Dir, "main", "main")) + assert.False(t, IsOnBaseBranch(repo.Dir, "feature", "main")) + }) + + t.Run("matches origin-prefixed ref", func(t *testing.T) { + remote := NewBareTestRepo(t) + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("file.txt", "content", "initial") + repo.Run("remote", "add", "origin", remote.Dir) + repo.Run("push", "-u", "origin", "main") + + assert.True(t, IsOnBaseBranch(repo.Dir, "main", "origin/main")) + assert.False(t, IsOnBaseBranch(repo.Dir, "feature", "origin/main")) + }) + + t.Run("matches non-origin remote prefix", func(t *testing.T) { + remote := NewBareTestRepo(t) + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("file.txt", "content", "initial") + repo.Run("remote", "add", "upstream", remote.Dir) + repo.Run("push", "-u", "upstream", "main") + + assert.True(t, IsOnBaseBranch(repo.Dir, "main", "upstream/main")) + assert.False(t, IsOnBaseBranch(repo.Dir, "feature", "upstream/main")) + }) + + t.Run("does not strip slash when no matching remote-tracking ref", func(t *testing.T) { + // feature/foo is a local branch, not origin/main style. Even when a + // remote named "feature" is configured, we must not treat base + // "feature/foo" as if it were a remote-tracking ref and strip the + // prefix — that would falsely match a local branch named "foo". + remote := NewBareTestRepo(t) + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("file.txt", "content", "initial") + repo.Run("remote", "add", "feature", remote.Dir) + repo.Run("checkout", "-b", "feature/foo") + repo.CommitFile("b.txt", "b", "work") + repo.Run("checkout", "-b", "foo", "main") + + // Current branch "foo" vs base "feature/foo" — refs/remotes/feature/foo + // does not exist, so the prefix must not be stripped. + assert.False(t, IsOnBaseBranch(repo.Dir, "foo", "feature/foo")) + // And the real "on-base" case for a local branch with a slash still works. + assert.True(t, IsOnBaseBranch(repo.Dir, "feature/foo", "feature/foo")) + }) + + t.Run("empty current branch does not match", func(t *testing.T) { + repo := NewTestRepoWithCommit(t) + assert.False(t, IsOnBaseBranch(repo.Dir, "", "main")) + }) + + t.Run("multi-slash remote name strips full prefix", func(t *testing.T) { + // A remote named "company/fork" produces tracking refs under + // refs/remotes/company/fork/. Stripping only the first + // slash ("company/") would leave "fork/main" and wrongly match + // a local branch of that name. The full remote prefix + // "company/fork/" must be stripped to yield "main". + remote := NewBareTestRepo(t) + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("file.txt", "content", "initial") + repo.Run("remote", "add", "company/fork", remote.Dir) + repo.Run("push", "-u", "company/fork", "main") + + assert.True(t, IsOnBaseBranch(repo.Dir, "main", "company/fork/main"), + "current=main on base=company/fork/main must strip full remote prefix") + assert.False(t, IsOnBaseBranch(repo.Dir, "fork/main", "company/fork/main"), + "current=fork/main must not falsely match after a single-slash strip") + }) + + t.Run("pathological local branch named like remote tracking does not equality-match", func(t *testing.T) { + // Regression: the raw equality fast-path would treat a local branch + // named "origin/main" as "already on base" when base was the + // remote-tracking ref of the same short name. The two are distinct + // refs in different namespaces; when both exist, the guardrail + // must refuse to match rather than assume they're the same. + remote := NewBareTestRepo(t) + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("file.txt", "content", "initial") + repo.Run("remote", "add", "origin", remote.Dir) + repo.Run("push", "-u", "origin", "main") + head := repo.HeadSHA() + // Create a local branch literally named "origin/main" so both + // refs/heads/origin/main AND refs/remotes/origin/main resolve. + repo.Run("update-ref", "refs/heads/origin/main", head) + + assert.False(t, IsOnBaseBranch(repo.Dir, "origin/main", "origin/main"), + "ambiguous name in both refs/heads and refs/remotes must not match") + }) + + t.Run("origin-prefixed local branch without remote ref is not base", func(t *testing.T) { + // Regression: the legacy LocalBranchName shortcut stripped "origin/" + // unconditionally, so a local branch literally named "origin/foo" + // (no refs/remotes/origin/foo) made currentBranch "foo" appear + // "already on base origin/foo". That would wrongly block + // review --branch / refine on a perfectly valid feature branch. + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("file.txt", "content", "initial") + // Create a local branch literally named "origin/foo" — no remote involved. + repo.Run("branch", "origin/foo") + + assert.False(t, IsOnBaseBranch(repo.Dir, "foo", "origin/foo"), + "local branch origin/foo without a remote-tracking ref must not match 'foo'") + assert.True(t, IsOnBaseBranch(repo.Dir, "origin/foo", "origin/foo"), + "exact-name match still works") + }) + + t.Run("ambiguous slash-containing base refuses to match", func(t *testing.T) { + // Pathological case: both refs/heads/feature/foo and + // refs/remotes/feature/foo exist. The caller's intent is unclear + // (local branch or remote-tracking?), so the safe response is to + // refuse to match either way. The downstream merge-base / range + // check will surface any real "nothing to review" condition. + remote := NewBareTestRepo(t) + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("file.txt", "content", "initial") + repo.Run("remote", "add", "feature", remote.Dir) + repo.Run("branch", "feature/foo") + head := repo.HeadSHA() + repo.Run("update-ref", "refs/remotes/feature/foo", head) + + assert.False(t, IsOnBaseBranch(repo.Dir, "foo", "feature/foo"), + "ambiguous ref must not be stripped") + assert.False(t, IsOnBaseBranch(repo.Dir, "feature/foo", "feature/foo"), + "ambiguous ref must not equality-match either") + }) +} + func TestLocalBranchName(t *testing.T) { tests := []struct { input string