From 28cfcaced2abb471c0824b2994d40dd2ab81c7d9 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 21 May 2026 20:36:01 +0200 Subject: [PATCH] Reshape selection to expose the winning base ref MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fast path's selectBehindForBranch returned only the behind value of the closest base, throwing away which base actually won. We need that information so subsequent commits can present a disambiguation prompt when more than one main branch is the closest base. Rename to selectBaseForBranch and have it return (winner, behind, candidates), where candidates is the full set of refs tied at the minimum ahead (in config order) so callers can recognise ambiguity via len(candidates) > 1. To make that possible, parseAheadBehindForEachRefOutput now preserves malformed entries with a valid=false flag instead of silently dropping them — without this the slice would drift out of alignment with mainRefs and the index→ref mapping would be unreliable. --- pkg/commands/git_commands/branch_loader.go | 57 ++++++--- .../git_commands/branch_loader_test.go | 108 ++++++++++++------ 2 files changed, 112 insertions(+), 53 deletions(-) diff --git a/pkg/commands/git_commands/branch_loader.go b/pkg/commands/git_commands/branch_loader.go index 321141c69..f5f96f1d3 100644 --- a/pkg/commands/git_commands/branch_loader.go +++ b/pkg/commands/git_commands/branch_loader.go @@ -206,9 +206,13 @@ func (self *BranchLoader) getBehindBaseBranchValuesLegacy( return err } -// Holds parsed values from a single %(ahead-behind:) field. +// Holds parsed values from a single %(ahead-behind:) field. `valid` +// is false when the field failed to parse (e.g. the base was unreachable +// from this ref); the entry is preserved so that the slice stays index- +// aligned with the configured main branches. type aheadBehind struct { ahead, behind int + valid bool } type branchAheadBehind struct { @@ -222,7 +226,7 @@ type branchAheadBehind struct { // // Lines whose NUL-split column count doesn't match (1 + numBases) are dropped. // Blank lines are ignored. -// Individual malformed ahead-behind fields produce {valid: false} entries +// Individual malformed ahead-behind fields produce {valid: false} entries. func parseAheadBehindForEachRefOutput( output string, numBases int, // number of %(ahead-behind:...) tokens @@ -238,7 +242,7 @@ func parseAheadBehindForEachRefOutput( continue } refName := cols[0] - aheadBehinds := lo.FilterMap(cols[1:], func(col string, _ int) (aheadBehind, bool) { + aheadBehinds := lo.Map(cols[1:], func(col string, _ int) aheadBehind { return parseAheadBehindField(col) }) entry := branchAheadBehind{ @@ -250,27 +254,48 @@ func parseAheadBehindForEachRefOutput( return result } -func parseAheadBehindField(s string) (aheadBehind, bool) { +func parseAheadBehindField(s string) aheadBehind { parts := strings.Fields(s) if len(parts) != 2 { - return aheadBehind{}, false + return aheadBehind{} } ahead, err1 := strconv.Atoi(parts[0]) behind, err2 := strconv.Atoi(parts[1]) if err1 != nil || err2 != nil { - return aheadBehind{}, false + return aheadBehind{} } - return aheadBehind{ahead: ahead, behind: behind}, true + return aheadBehind{ahead: ahead, behind: behind, valid: true} } -// Picks the "closest" base by smallest ahead value (commits the branch -// has that the base doesn't = roughly "since fork point") and returns -// its behind value. -// Ties are broken by index order -func selectBehindForBranch(aheadBehinds []aheadBehind) int { - return lo.MinBy(aheadBehinds, func(a, b aheadBehind) bool { - return a.ahead < b.ahead - }).behind +// selectBaseForBranch picks the closest base for a branch given (ahead, +// behind) measurements against each configured main branch. "Closest" = +// smallest ahead value (fewest branch commits not in the base). Ties are +// broken by the order of mainRefs (i.e. config order). +// +// aheadBehinds must be index-aligned with mainRefs; invalid entries are +// skipped. Returns the winning ref, its behind value, and the full set +// of refs tied at the minimum ahead (in config order). The caller can +// detect ambiguity via `len(candidates) > 1`. With no valid entry the +// return is ("", 0, nil). +func selectBaseForBranch( + aheadBehinds []aheadBehind, mainRefs []string, +) (winner string, behind int, candidates []string) { + bestAhead := -1 + for i, ab := range aheadBehinds { + if !ab.valid { + continue + } + switch { + case bestAhead < 0 || ab.ahead < bestAhead: + bestAhead = ab.ahead + winner = mainRefs[i] + behind = ab.behind + candidates = []string{mainRefs[i]} + case ab.ahead == bestAhead: + candidates = append(candidates, mainRefs[i]) + } + } + return winner, behind, candidates } // The output format is: @@ -313,7 +338,7 @@ func (self *BranchLoader) getBehindBaseBranchValuesFast( for _, p := range parsed { if branch, ok := branchByRef[p.refName]; ok { - behind := selectBehindForBranch(p.aheadBehinds) + _, behind, _ := selectBaseForBranch(p.aheadBehinds, mainBranchRefs) branch.BehindBaseBranch.Store(int32(behind)) delete(branchByRef, p.refName) } diff --git a/pkg/commands/git_commands/branch_loader_test.go b/pkg/commands/git_commands/branch_loader_test.go index 2a0da414e..fe004adc9 100644 --- a/pkg/commands/git_commands/branch_loader_test.go +++ b/pkg/commands/git_commands/branch_loader_test.go @@ -142,7 +142,7 @@ func TestParseAheadBehindForEachRefOutput(t *testing.T) { expected: []branchAheadBehind{ { refName: "refs/heads/feat", - aheadBehinds: []aheadBehind{{ahead: 2, behind: 5}}, + aheadBehinds: []aheadBehind{{ahead: 2, behind: 5, valid: true}}, }, }, }, @@ -155,15 +155,15 @@ func TestParseAheadBehindForEachRefOutput(t *testing.T) { { refName: "refs/heads/feat", aheadBehinds: []aheadBehind{ - {ahead: 2, behind: 5}, - {ahead: 10, behind: 1}, + {ahead: 2, behind: 5, valid: true}, + {ahead: 10, behind: 1, valid: true}, }, }, { refName: "refs/heads/main", aheadBehinds: []aheadBehind{ - {ahead: 0, behind: 0}, - {ahead: 0, behind: 0}, + {ahead: 0, behind: 0, valid: true}, + {ahead: 0, behind: 0, valid: true}, }, }, }, @@ -176,7 +176,8 @@ func TestParseAheadBehindForEachRefOutput(t *testing.T) { { refName: "refs/heads/feat", aheadBehinds: []aheadBehind{ - {ahead: 2, behind: 5}, + {valid: false}, + {ahead: 2, behind: 5, valid: true}, }, }, }, @@ -188,7 +189,7 @@ func TestParseAheadBehindForEachRefOutput(t *testing.T) { expected: []branchAheadBehind{ { refName: "refs/heads/feat/foo-bar", - aheadBehinds: []aheadBehind{{ahead: 1, behind: 2}}, + aheadBehinds: []aheadBehind{{ahead: 1, behind: 2, valid: true}}, }, }, }, @@ -199,7 +200,7 @@ func TestParseAheadBehindForEachRefOutput(t *testing.T) { expected: []branchAheadBehind{ { refName: "refs/heads/feat", - aheadBehinds: []aheadBehind{{ahead: 1, behind: 2}}, + aheadBehinds: []aheadBehind{{ahead: 1, behind: 2, valid: true}}, }, }, }, @@ -212,11 +213,11 @@ func TestParseAheadBehindForEachRefOutput(t *testing.T) { expected: []branchAheadBehind{ { refName: "refs/heads/good", - aheadBehinds: []aheadBehind{{ahead: 1, behind: 2}}, + aheadBehinds: []aheadBehind{{ahead: 1, behind: 2, valid: true}}, }, { refName: "refs/heads/also_good", - aheadBehinds: []aheadBehind{{ahead: 3, behind: 4}}, + aheadBehinds: []aheadBehind{{ahead: 3, behind: 4, valid: true}}, }, }, }, @@ -227,7 +228,7 @@ func TestParseAheadBehindForEachRefOutput(t *testing.T) { expected: []branchAheadBehind{ { refName: "refs/heads/feat", - aheadBehinds: []aheadBehind{}, + aheadBehinds: []aheadBehind{{valid: false}}, }, }, }, @@ -247,26 +248,35 @@ func TestParseAheadBehindForEachRefOutput(t *testing.T) { } } -func TestSelectBehindForBranch(t *testing.T) { +func TestSelectBaseForBranch(t *testing.T) { type scenario struct { - testName string - aheadBehinds []aheadBehind - expected int + testName string + aheadBehinds []aheadBehind + mainRefs []string + expectedWinner string + expectedBehind int + expectedCandidates []string } scenarios := []scenario{ { - testName: "single base, valid value", - aheadBehinds: []aheadBehind{{ahead: 3, behind: 7}}, - expected: 7, + testName: "single base, valid value", + aheadBehinds: []aheadBehind{{ahead: 3, behind: 7, valid: true}}, + mainRefs: []string{"refs/heads/master"}, + expectedWinner: "refs/heads/master", + expectedBehind: 7, + expectedCandidates: []string{"refs/heads/master"}, }, { testName: "multi-base, clear winner by ahead", aheadBehinds: []aheadBehind{ - {ahead: 50, behind: 10}, // master - {ahead: 5, behind: 2}, // develop ← smallest ahead + {ahead: 50, behind: 10, valid: true}, // master + {ahead: 5, behind: 2, valid: true}, // develop ← smallest ahead }, - expected: 2, + mainRefs: []string{"refs/heads/master", "refs/heads/develop"}, + expectedWinner: "refs/heads/develop", + expectedBehind: 2, + expectedCandidates: []string{"refs/heads/develop"}, }, { testName: "develop forked from master case (ancestor-of-each-other)", @@ -275,42 +285,66 @@ func TestSelectBehindForBranch(t *testing.T) { // ahead vs master = 5 + 50 = 55; behind vs master = 0 // ahead vs develop = 5; behind vs develop = 5 aheadBehinds: []aheadBehind{ - {ahead: 55, behind: 0}, // master - {ahead: 5, behind: 5}, // develop ← smallest ahead + {ahead: 55, behind: 0, valid: true}, // master + {ahead: 5, behind: 5, valid: true}, // develop ← smallest ahead }, - expected: 5, + mainRefs: []string{"refs/heads/master", "refs/heads/develop"}, + expectedWinner: "refs/heads/develop", + expectedBehind: 5, + expectedCandidates: []string{"refs/heads/develop"}, }, { testName: "tie on ahead - first base wins (config order)", aheadBehinds: []aheadBehind{ - {ahead: 5, behind: 10}, // first - {ahead: 5, behind: 99}, // second, same ahead + {ahead: 5, behind: 10, valid: true}, // first + {ahead: 5, behind: 99, valid: true}, // second, same ahead + }, + mainRefs: []string{"refs/heads/main", "refs/heads/develop"}, + expectedWinner: "refs/heads/main", + expectedBehind: 10, + expectedCandidates: []string{ + "refs/heads/main", + "refs/heads/develop", }, - expected: 10, }, { testName: "first base invalid, second valid", aheadBehinds: []aheadBehind{ - {ahead: 3, behind: 8}, + {valid: false}, + {ahead: 3, behind: 8, valid: true}, }, - expected: 8, + mainRefs: []string{"refs/heads/master", "refs/heads/develop"}, + expectedWinner: "refs/heads/develop", + expectedBehind: 8, + expectedCandidates: []string{"refs/heads/develop"}, }, { - testName: "all invalid - returns 0", - aheadBehinds: []aheadBehind{}, - expected: 0, + testName: "all invalid - returns empty", + aheadBehinds: []aheadBehind{ + {valid: false}, + {valid: false}, + }, + mainRefs: []string{"refs/heads/master", "refs/heads/develop"}, + expectedWinner: "", + expectedBehind: 0, + expectedCandidates: nil, }, { - testName: "empty - returns 0", - aheadBehinds: nil, - expected: 0, + testName: "empty - returns empty", + aheadBehinds: nil, + mainRefs: nil, + expectedWinner: "", + expectedBehind: 0, + expectedCandidates: nil, }, } for _, s := range scenarios { t.Run(s.testName, func(t *testing.T) { - result := selectBehindForBranch(s.aheadBehinds) - assert.Equal(t, s.expected, result) + winner, behind, candidates := selectBaseForBranch(s.aheadBehinds, s.mainRefs) + assert.Equal(t, s.expectedWinner, winner) + assert.Equal(t, s.expectedBehind, behind) + assert.Equal(t, s.expectedCandidates, candidates) }) } }