Pick base branch by smallest ahead instead of relying on for-each-ref's order

GetBaseBranch was treating "contains the merge-base" as the equivalence
class for "is the closest base," which is too loose — multiple main
branches can contain the merge-base when one is dramatically closer
to the feature branch than another. The candidate it returned was
then whichever ref git for-each-ref happened to list first.

For example, a branch forked off "develop" can have its combined
merge-base with [main, develop] land on a commit reachable from both
(via develop's own branch-off from main). Both main and develop end
up in the candidate set, even though by any reasonable measure of
closeness the branch differs from develop by a small ahead count and
from main by a much larger one.

Discriminate within the candidate set using ahead values: for each
configured main branch that contains the merge-base, compute the
ahead count from branch to base, and pick the candidate with the
smallest ahead — the closest base. When more than one candidate is
tied at the minimum, return that tied set unchanged so callers can
flag the case as genuinely ambiguous instead of silently collapsing
it; subsequent commits build the disambiguation prompt on top.
This commit is contained in:
Stefan Haller 2026-05-21 20:31:57 +02:00
parent a0e51da643
commit dd03884087
2 changed files with 110 additions and 15 deletions

View file

@ -343,23 +343,69 @@ func (self *BranchLoader) GetBaseBranch(branch *models.Branch, mainBranches *Mai
return "", nil
}
mainBranchRefs := mainBranches.Get()
output, err := self.cmd.New(
NewGitCmd("for-each-ref").
Arg("--contains").
Arg(mergeBase).
Arg("--format=%(refname)").
Arg(mainBranches.Get()...).
Arg(mainBranchRefs...).
ToArgv(),
).DontLog().RunWithOutput()
if err != nil {
return "", err
}
trimmedOutput := strings.TrimSpace(output)
split := strings.Split(trimmedOutput, "\n")
if len(split) == 0 || split[0] == "" {
if trimmedOutput == "" {
return "", nil
}
return split[0], nil
contained := strings.Split(trimmedOutput, "\n")
// for-each-ref sorts its output alphabetically by refname regardless of
// the order we passed the refs in. Restore the user's configured order so
// it can serve as the natural tiebreaker.
candidates := lo.Filter(mainBranchRefs, func(ref string, _ int) bool {
return lo.Contains(contained, ref)
})
if len(candidates) == 0 {
return "", nil
}
if len(candidates) == 1 {
return candidates[0], nil
}
// Multiple main branches contain the merge-base. Pick the "closest" by
// the same definition the fast path uses (smallest ahead value, i.e.
// fewest branch commits not in the base). Ties fall back to config
// order, which `candidates` already preserves.
bestIdx := 0
bestAhead := -1
for i, ref := range candidates {
revListOutput, err := self.cmd.New(
NewGitCmd("rev-list").
Arg("--left-right").
Arg("--count").
Arg(fmt.Sprintf("%s...%s", branch.FullRefName(), ref)).
ToArgv(),
).DontLog().RunWithOutput()
if err != nil {
return "", err
}
parts := strings.Fields(strings.TrimSpace(revListOutput))
if len(parts) != 2 {
continue
}
ahead, err := strconv.Atoi(parts[0])
if err != nil {
continue
}
if bestAhead < 0 || ahead < bestAhead {
bestAhead = ahead
bestIdx = i
}
}
return candidates[bestIdx], nil
}
func (self *BranchLoader) obtainBranches() []*models.Branch {

View file

@ -492,13 +492,11 @@ func TestGetBehindBaseBranchValuesForAllBranches_LegacyPath(t *testing.T) {
runner.CheckForMissingCalls()
}
// When the merge-base is contained in more than one configured main branch,
// git for-each-ref returns those refs sorted alphabetically by refname,
// regardless of the order we pass them in. The chosen base should respect
// the user's configured order ("main" first), not the alphabetical accident.
//
// Demonstrates the bug; the expected behavior is asserted in the next commit.
func TestGetBaseBranch_AmbiguousPicksAlphabeticalNotConfigOrder(t *testing.T) {
// When the branch's merge-base is contained in more than one configured main
// branch and the ahead counts are equal, the chosen base must respect the
// user's configured order rather than the alphabetical order of
// for-each-ref's output.
func TestGetBaseBranch_AmbiguousFallsBackToConfigOrder(t *testing.T) {
mainBranchRefs := []string{"refs/heads/main", "refs/heads/develop"}
branch := &models.Branch{Name: "feat-x"}
@ -511,7 +509,13 @@ func TestGetBaseBranch_AmbiguousPicksAlphabeticalNotConfigOrder(t *testing.T) {
"for-each-ref", "--contains", "abc123", "--format=%(refname)",
"refs/heads/main", "refs/heads/develop",
},
"refs/heads/develop\nrefs/heads/main\n", nil)
"refs/heads/develop\nrefs/heads/main\n", nil).
ExpectGitArgs(
[]string{"rev-list", "--left-right", "--count", "refs/heads/feat-x...refs/heads/main"},
"5\t10\n", nil).
ExpectGitArgs(
[]string{"rev-list", "--left-right", "--count", "refs/heads/feat-x...refs/heads/develop"},
"5\t8\n", nil)
gitCommon := buildGitCommon(commonDeps{runner: runner})
@ -530,9 +534,54 @@ func TestGetBaseBranch_AmbiguousPicksAlphabeticalNotConfigOrder(t *testing.T) {
baseBranch, err := loader.GetBaseBranch(branch, mainBranches)
assert.NoError(t, err)
// Want: "refs/heads/main" (first in config order among the tied candidates).
// Have: "refs/heads/develop" (alphabetical first from for-each-ref).
assert.Equal(t, "refs/heads/develop", baseBranch)
assert.Equal(t, "refs/heads/main", baseBranch)
runner.CheckForMissingCalls()
}
// When a configured main branch has a strictly smaller ahead count than any
// other (e.g. the branch was forked off `main` after main's last merge into
// `develop`, so `develop` doesn't yet contain the fork point's recent main
// history), that base wins outright regardless of config order.
func TestGetBaseBranch_UnambiguousPicksSmallestAhead(t *testing.T) {
mainBranchRefs := []string{"refs/heads/develop", "refs/heads/main"}
branch := &models.Branch{Name: "feat-x"}
runner := oscommands.NewFakeRunner(t).
ExpectGitArgs(
[]string{"merge-base", "refs/heads/feat-x", "refs/heads/develop", "refs/heads/main"},
"abc123\n", nil).
ExpectGitArgs(
[]string{
"for-each-ref", "--contains", "abc123", "--format=%(refname)",
"refs/heads/develop", "refs/heads/main",
},
"refs/heads/develop\nrefs/heads/main\n", nil).
ExpectGitArgs(
[]string{"rev-list", "--left-right", "--count", "refs/heads/feat-x...refs/heads/develop"},
"8\t3\n", nil).
ExpectGitArgs(
[]string{"rev-list", "--left-right", "--count", "refs/heads/feat-x...refs/heads/main"},
"5\t10\n", nil)
gitCommon := buildGitCommon(commonDeps{runner: runner})
loader := &BranchLoader{
Common: gitCommon.Common,
GitCommon: gitCommon,
cmd: gitCommon.cmd,
}
mainBranches := &MainBranches{
c: gitCommon.Common,
cmd: gitCommon.cmd,
existingMainBranches: mainBranchRefs,
previousMainBranches: gitCommon.Common.UserConfig().Git.MainBranches,
}
baseBranch, err := loader.GetBaseBranch(branch, mainBranches)
assert.NoError(t, err)
assert.Equal(t, "refs/heads/main", baseBranch)
runner.CheckForMissingCalls()
}