diff --git a/cmd/roborev/review_test.go b/cmd/roborev/review_test.go index 9b9d5d62..58176533 100644 --- a/cmd/roborev/review_test.go +++ b/cmd/roborev/review_test.go @@ -474,6 +474,38 @@ func TestReviewBranchFlag(t *testing.T) { req := <-reqCh assert.Equal(t, mainSHA+"..HEAD", req.GitRef) }) + + t.Run("branch review uses origin counterpart when local main is stale after rebase", func(t *testing.T) { + // Regression: a feature branch rebased onto origin/main with a stale + // local main pointing at an older commit must review only the new + // branch commits — not the trunk commits that already landed on + // origin/main between the local main pointer and the rebase target. + repo, mux := setupTestEnvironment(t) + reqCh := mockEnqueue(t, mux) + + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("base.txt", "base", "initial") + staleMainSHA := repo.Run("rev-parse", "HEAD") + // Two commits land on the trunk after local main was last updated. + repo.CommitFile("trunk1.txt", "t1", "trunk advance 1") + repo.CommitFile("trunk2.txt", "t2", "trunk advance 2") + freshOriginSHA := repo.Run("rev-parse", "HEAD") + // Rewind local main to simulate a stale checkout. + repo.Run("update-ref", "refs/heads/main", staleMainSHA) + repo.Run("remote", "add", "origin", "/dev/null") + repo.Run("update-ref", "refs/remotes/origin/main", freshOriginSHA) + // Feature branch was rebased onto origin/main. + repo.Run("checkout", "-b", "feature", freshOriginSHA) + repo.CommitFile("feature.txt", "f", "feature commit") + repo.Run("config", "branch.feature.base", "main") + + _, _, err := executeReviewCmd("--repo", repo.Dir, "--branch") + require.NoError(t, err) + + req := <-reqCh + assert.Equal(t, freshOriginSHA+"..HEAD", req.GitRef, + "range must start at origin/main, not stale local main") + }) } func TestReviewFastFlag(t *testing.T) { @@ -715,6 +747,31 @@ func TestTryBranchReview(t *testing.T) { assert.False(t, ok, "local main tracking upstream/main must be treated as base branch") }) + t.Run("uses origin counterpart when local base is stale ancestor", func(t *testing.T) { + // Mirrors the rebase-onto-origin/main scenario for the post-commit + // hook path (tryBranchReview): branch..base = main with a + // stale local main must resolve to origin/main, not the rewound + // pointer, so the hook enqueues only the real branch commits. + repo := newTestGitRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("base.txt", "base", "initial") + staleMainSHA := repo.Run("rev-parse", "HEAD") + repo.CommitFile("trunk.txt", "t", "trunk advance") + freshOriginSHA := repo.Run("rev-parse", "HEAD") + repo.Run("update-ref", "refs/heads/main", staleMainSHA) + repo.Run("remote", "add", "origin", "/dev/null") + repo.Run("update-ref", "refs/remotes/origin/main", freshOriginSHA) + repo.Run("checkout", "-b", "feature", freshOriginSHA) + repo.CommitFile("feature.txt", "f", "feature commit") + repo.Run("config", "branch.feature.base", "main") + writeRoborevConfig(t, repo, `post_commit_review = "branch"`) + + ref, ok := tryBranchReview(repo.Dir, "") + require.True(t, ok, "expected branch review with stale local main") + assert.Equal(t, freshOriginSHA+"..HEAD", ref, + "range must start at origin/main, not stale local main") + }) + 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 diff --git a/internal/git/git.go b/internal/git/git.go index 7e1f97aa..e05751ec 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -1353,14 +1353,97 @@ func readUpstreamConfig(repoPath, ref string) (upstreamConfig, bool) { }, true } -// GetBranchBase returns roborev's branch..base config value for ref. -// Passing an empty ref is equivalent to HEAD. +// GetBranchBase returns the base ref derived from ref's branch..base +// config. Bare names (e.g. "main") are resolved to the remote-tracking ref +// (e.g. "origin/main") that the local branch tracks when local exists +// and is an ancestor of the remote-tracking ref — i.e., local is just +// a stale snapshot of the trunk. Without this, merge-base(local-main, HEAD) +// walks back to the stale pointer and the resulting range pulls in commits +// already merged into the remote trunk after a rebase-onto-origin/main. +// Qualified refs (containing '/') are returned as-is so users can pin to +// e.g. "upstream/main". Passing an empty ref is equivalent to HEAD. func GetBranchBase(repoPath, ref string) string { branch := branchNameForConfig(repoPath, ref) if branch == "" { return "" } - return readGitConfig(repoPath, "branch."+branch+".base") + raw := readGitConfig(repoPath, "branch."+branch+".base") + return resolveConfiguredBase(repoPath, raw) +} + +// resolveConfiguredBase translates a configured base name to the remote- +// tracking ref it should map to, or returns configValue unchanged when no +// translation applies. Values that already name a remote-tracking ref +// unambiguously are passed through; local branch names (including slash- +// containing names like "release/1.2") are subject to stale-ancestor +// translation; divergent local branches are also passed through. +func resolveConfiguredBase(repoPath, configValue string) string { + if configValue == "" { + return configValue + } + if isQualifiedRemoteRef(repoPath, configValue) { + return configValue + } + remoteTracking := preferredRemoteTracking(repoPath, configValue) + if remoteTracking == "" { + return configValue + } + if !refExists(repoPath, "refs/heads/"+configValue) { + return remoteTracking + } + isAncestor, err := IsAncestor(repoPath, configValue, remoteTracking) + if err != nil || !isAncestor { + return configValue + } + return remoteTracking +} + +// isQualifiedRemoteRef reports whether value names a remote-tracking ref +// that is not shadowed by a same-named local branch. Defers to the local +// branch when both exist so stale-ancestor translation still applies, and +// so the resolved base matches git's normal ref lookup precedence +// (refs/heads beats refs/remotes). +func isQualifiedRemoteRef(repoPath, value string) bool { + if !strings.Contains(value, "/") { + return false + } + if !refExists(repoPath, "refs/remotes/"+value) { + return false + } + if refExists(repoPath, "refs/heads/"+value) { + return false + } + return true +} + +// preferredRemoteTracking returns the remote-tracking ref short name for a +// branch name, preferring the branch's configured @{upstream} so fork +// workflows (local main → upstream/main) translate correctly, and falling +// back to origin/ only when no upstream is configured. Returns "" +// when no remote-tracking counterpart resolves locally; in particular: +// - An explicitly configured upstream that does not resolve is NOT +// silently substituted with origin/ — that would override the +// user's choice of remote in fork workflows. +// - For slash-containing names, the origin/ fallback only applies +// when a local branch by that name exists. Otherwise a remote- +// qualified value like "upstream/main" whose configured remote- +// tracking ref isn't fetched would collapse to "origin/upstream/main", +// silently switching to a different remote. +func preferredRemoteTracking(repoPath, name string) string { + if cfg, ok := readUpstreamConfig(repoPath, name); ok { + if strings.HasPrefix(cfg.qualified, "refs/remotes/") && + refExists(repoPath, cfg.qualified) { + return cfg.short + } + return "" + } + if !refExists(repoPath, "refs/remotes/origin/"+name) { + return "" + } + if strings.Contains(name, "/") && !refExists(repoPath, "refs/heads/"+name) { + return "" + } + return "origin/" + name } func branchNameForConfig(repoPath, ref string) string { diff --git a/internal/git/git_test.go b/internal/git/git_test.go index 9d3d65f1..912b5b44 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -2087,3 +2087,216 @@ func TestLooksLikeSHA(t *testing.T) { }) } } + +func TestGetBranchBase(t *testing.T) { + t.Run("returns empty when not configured", func(t *testing.T) { + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("base.txt", "base", "initial") + repo.Run("checkout", "-b", "feature") + + assert.Empty(t, GetBranchBase(repo.Dir, "HEAD")) + }) + + t.Run("returns local bare name when no remote-tracking exists", func(t *testing.T) { + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("base.txt", "base", "initial") + repo.Run("checkout", "-b", "feature") + repo.Run("config", "branch.feature.base", "main") + + assert.Equal(t, "main", GetBranchBase(repo.Dir, "HEAD")) + }) + + t.Run("resolves bare name to origin counterpart when local is stale ancestor", func(t *testing.T) { + // Reproduces the post-rebase bug: branch..base = main with + // a stale local main pulls extra origin/main commits into the merge- + // base..HEAD range. The fix translates the bare name to origin/main + // when local main is just an ancestor of origin/main. + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("base.txt", "base", "initial") + staleMainSHA := repo.HeadSHA() + repo.CommitFile("trunk.txt", "trunk", "trunk advance") + freshOriginSHA := repo.HeadSHA() + // Rewind local main so it lags one commit behind origin/main. + repo.Run("update-ref", "refs/heads/main", staleMainSHA) + repo.Run("remote", "add", "origin", "/dev/null") + repo.Run("update-ref", "refs/remotes/origin/main", freshOriginSHA) + repo.Run("checkout", "-b", "feature", freshOriginSHA) + repo.CommitFile("feature.txt", "f", "feature commit") + repo.Run("config", "branch.feature.base", "main") + + assert.Equal(t, "origin/main", GetBranchBase(repo.Dir, "HEAD")) + }) + + t.Run("keeps local name when local has diverged from origin", func(t *testing.T) { + // Divergence (local main has commits not on origin/main) means the + // user's local branch is not just stale — respect their config + // rather than silently picking a different base. + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("base.txt", "base", "initial") + baseSHA := repo.HeadSHA() + repo.CommitFile("origin.txt", "origin", "origin-only commit") + originMainSHA := repo.HeadSHA() + // Local main diverges with its own commit not present on origin/main. + repo.Run("update-ref", "refs/heads/main", baseSHA) + repo.Run("checkout", "main") + repo.CommitFile("local.txt", "local", "local-only commit") + repo.Run("remote", "add", "origin", "/dev/null") + repo.Run("update-ref", "refs/remotes/origin/main", originMainSHA) + repo.Run("checkout", "-b", "feature") + repo.Run("config", "branch.feature.base", "main") + + assert.Equal(t, "main", GetBranchBase(repo.Dir, "HEAD")) + }) + + t.Run("returns origin counterpart when local branch is missing", func(t *testing.T) { + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/scratch") + repo.CommitFile("base.txt", "base", "initial") + originSHA := repo.HeadSHA() + repo.Run("remote", "add", "origin", "/dev/null") + repo.Run("update-ref", "refs/remotes/origin/main", originSHA) + // Note: no refs/heads/main. Branch from origin/main directly. + repo.Run("checkout", "-b", "feature", originSHA) + repo.CommitFile("feature.txt", "f", "feature commit") + repo.Run("config", "branch.feature.base", "main") + + assert.Equal(t, "origin/main", GetBranchBase(repo.Dir, "HEAD")) + }) + + t.Run("returns qualified ref unchanged", func(t *testing.T) { + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("base.txt", "base", "initial") + repo.Run("checkout", "-b", "feature") + repo.Run("config", "branch.feature.base", "upstream/main") + + assert.Equal(t, "upstream/main", GetBranchBase(repo.Dir, "HEAD")) + }) + + t.Run("resolves slash-containing local branch when stale", func(t *testing.T) { + // Branch names may contain slashes (release/1.2, team/main). Such + // names must still get stale-ancestor translation — treating any + // '/' as a remote prefix would skip the fix for these cases. + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/release/1.2") + repo.CommitFile("base.txt", "base", "initial") + staleSHA := repo.HeadSHA() + repo.CommitFile("trunk.txt", "t", "trunk advance") + freshSHA := repo.HeadSHA() + repo.Run("update-ref", "refs/heads/release/1.2", staleSHA) + repo.Run("remote", "add", "origin", "/dev/null") + repo.Run("update-ref", "refs/remotes/origin/release/1.2", freshSHA) + repo.Run("checkout", "-b", "feature", freshSHA) + repo.CommitFile("feature.txt", "f", "feature commit") + repo.Run("config", "branch.feature.base", "release/1.2") + + assert.Equal(t, "origin/release/1.2", GetBranchBase(repo.Dir, "HEAD")) + }) + + t.Run("passes qualified remote-tracking ref through unchanged", func(t *testing.T) { + // An unambiguous remote-tracking ref (refs/remotes/ exists, + // no shadowing local branch) is not subject to translation. + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("base.txt", "base", "initial") + mainSHA := repo.HeadSHA() + repo.Run("remote", "add", "upstream", "/dev/null") + repo.Run("update-ref", "refs/remotes/upstream/main", mainSHA) + repo.Run("checkout", "-b", "feature") + repo.Run("config", "branch.feature.base", "upstream/main") + + assert.Equal(t, "upstream/main", GetBranchBase(repo.Dir, "HEAD")) + }) + + t.Run("does not fall back to origin when configured upstream is missing", func(t *testing.T) { + // Fork workflow: local main is configured to track upstream/main + // but upstream/main has not been fetched. origin/main (the user's + // fork) must NOT be silently substituted — that would compute the + // merge-base against a different remote than the user intended. + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("base.txt", "base", "initial") + mainSHA := repo.HeadSHA() + repo.Run("remote", "add", "origin", "/dev/null") + repo.Run("update-ref", "refs/remotes/origin/main", mainSHA) + // Configure local main to track upstream/main, but never set up + // the corresponding remote-tracking ref. + repo.Run("config", "branch.main.remote", "upstream") + repo.Run("config", "branch.main.merge", "refs/heads/main") + repo.Run("checkout", "-b", "feature") + repo.Run("config", "branch.feature.base", "main") + + assert.Equal(t, "main", GetBranchBase(repo.Dir, "HEAD"), + "missing upstream must not be substituted with origin counterpart") + }) + + t.Run("does not collapse remote-qualified value to origin counterpart", func(t *testing.T) { + // Regression: branch.feature.base = "upstream/main" with the + // upstream remote-tracking ref absent must not be rewritten to + // "origin/upstream/main" just because a stray refs/remotes/origin/ + // upstream/main happens to exist. The user explicitly remote- + // qualified the value; either it resolves or git surfaces an error. + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("base.txt", "base", "initial") + mainSHA := repo.HeadSHA() + repo.Run("remote", "add", "origin", "/dev/null") + // origin holds a stray ref shaped like a remote-qualified path — + // e.g., a past push of a local branch literally named + // "upstream/main". refs/remotes/upstream/main remains absent. + repo.Run("update-ref", "refs/remotes/origin/upstream/main", mainSHA) + repo.Run("checkout", "-b", "feature") + repo.Run("config", "branch.feature.base", "upstream/main") + + assert.Equal(t, "upstream/main", GetBranchBase(repo.Dir, "HEAD"), + "explicit remote-qualified value must not collapse to origin/") + }) + + t.Run("does not fall back to origin when configured upstream is local-only", func(t *testing.T) { + // Local-branch tracking (branch..remote = ".") explicitly + // targets another local branch. origin/ is unrelated and + // must not be silently substituted. + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/develop") + repo.CommitFile("base.txt", "base", "initial") + developSHA := repo.HeadSHA() + repo.Run("checkout", "-b", "main") + repo.Run("config", "branch.main.remote", ".") + repo.Run("config", "branch.main.merge", "refs/heads/develop") + repo.Run("remote", "add", "origin", "/dev/null") + repo.Run("update-ref", "refs/remotes/origin/main", developSHA) + repo.Run("checkout", "-b", "feature") + repo.Run("config", "branch.feature.base", "main") + + assert.Equal(t, "main", GetBranchBase(repo.Dir, "HEAD"), + "local-branch tracking must not be substituted with origin counterpart") + }) + + t.Run("prefers configured upstream over origin counterpart", func(t *testing.T) { + // Fork workflow: local main tracks upstream/main, and origin/main + // also exists (e.g., points at the user's fork). The base should + // resolve to upstream/main because that's what local main tracks. + repo := NewTestRepo(t) + repo.Run("symbolic-ref", "HEAD", "refs/heads/main") + repo.CommitFile("base.txt", "base", "initial") + mainSHA := repo.HeadSHA() + repo.CommitFile("trunk.txt", "trunk", "upstream advance") + upstreamSHA := repo.HeadSHA() + repo.Run("update-ref", "refs/heads/main", mainSHA) + repo.Run("remote", "add", "origin", "/dev/null") + repo.Run("remote", "add", "upstream", "/dev/null") + repo.Run("update-ref", "refs/remotes/origin/main", mainSHA) + repo.Run("update-ref", "refs/remotes/upstream/main", upstreamSHA) + repo.Run("config", "branch.main.remote", "upstream") + repo.Run("config", "branch.main.merge", "refs/heads/main") + repo.Run("checkout", "-b", "feature", upstreamSHA) + repo.CommitFile("feature.txt", "f", "feature commit") + repo.Run("config", "branch.feature.base", "main") + + assert.Equal(t, "upstream/main", GetBranchBase(repo.Dir, "HEAD")) + }) +}