Improve base branch detection#5636
Open
stefanhaller wants to merge 17 commits into
Open
Conversation
GetBaseBranch was determining its candidates purely by "this main branch contains the merge-base" — and then returning the first one without further discrimination. That equivalence class is too loose: multiple main branches can contain the merge-base even when one is clearly closer to the feature branch than another. The new test exercises that case: with main and develop both containing the branch's merge-base, the current code returns whichever git for-each-ref happens to list first, ignoring how close each candidate actually is.
…'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.
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.
Have GetBaseBranch reuse selectBaseForBranch to disambiguate the multi-candidate case, so the fast path (for-each-ref %(ahead-behind)) and the legacy path (rev-list --left-right --count) agree on the same rule for which base is "closest" and the same config-order tiebreak when ahead values are equal. The bespoke loop in GetBaseBranch goes away.
The four callers of GetBaseBranch all ultimately want a single ref, but the disambiguation prompt we're about to add needs to know when multiple main branches are genuinely tied at the closest position so it can ask the user. Change the function to return all min-ahead refs in config order and rename to GetBaseBranchCandidates; each caller now takes candidates[0] as a placeholder until the prompt wiring lands.
GUI call sites that need a base branch all share the same logic: ask GetBaseBranchCandidates, take candidates[0] as the config-order tiebreak, and surface the candidate list when the user needs to disambiguate. Extracting that into a helper keeps the upcoming prompt and rebase wiring focused on UX concerns. Not yet routed through — subsequent commits replace the direct GetBaseBranchCandidates calls with ResolveBaseBranch.
The four call sites that need a single base branch (legacy behind-base loader, view-divergence menu, rebase-onto-base action, move-commits-to-new-branch) now go through BaseBranchHelper.ResolveBaseBranch in the GUI sites, and directly take candidates[0] in the loader. They all use the same config-order tiebreak. No prompt yet — the helper still returns the config-order first for ambiguous cases; subsequent commits add the disambiguation menu. MergeAndRebaseHelper and RefsHelper take BaseBranchHelper at construction since helpers don't have access to Helpers() the way controllers do.
When ResolveBaseBranch reports a tie, callers need a way to ask the user which of the configured main branches to use as the base. ShowPicker takes the candidate list and a continuation, presents a menu of short branch names, and runs the continuation with the user's selection. Subsequent commits wire this into the three GUI actions that care (rebase-onto-base, view-divergence, move-commits).
When the checked-out branch's fork point is contained in more than one configured main branch, pressing 'b' on the rebase menu now opens a small disambiguation menu first; the selection is recorded and then the rebase runs against the chosen base. The unambiguous case is unchanged.
Pressing 'b' on the branches view's divergence menu now shows the disambiguation menu when the selected branch's fork point is reachable from more than one configured main branch. The user's selection drives the sub-commits view and gets recorded so subsequent actions on the branch skip the prompt.
The "off of <base>" item in the move-commits-to-new-branch menu now shows the disambiguation picker first when the base is ambiguous, then continues into the existing new-branch-name prompt with the chosen base. The "stacked on current branch" path doesn't use the base, so it's unaffected.
When a branch's fork point is reachable from more than one configured main branch and the candidates disagree on the behind count, the branches column was silently showing the config-order first candidate's number — confidently asserting something we don't actually know. Worse, "nothing" in the column means "up to date with the base", which may or may not be true under ambiguity. Compute behind values for every candidate (the fast path already had them; the legacy path now does too via baseBranchCandidatesAndBehinds), then classify: - all candidates agree → show that number (or nothing if 0) - some candidates 0, others not → "?" (we can't say if up to date) - all non-zero but differing → "↓?" (definitely behind, unknown amount) Two sentinel constants on the Branch model (BehindBaseAmbiguousMaybeUpToDate and BehindBaseAmbiguousDefinitelyBehind) encode these states in the existing atomic.Int32 field via negative values; the renderer switches on them.
The label currently looks identical in the unambiguous case
("Rebase onto base branch (develop)") and the ambiguous case where
develop is just the config-order tiebreak; pressing 'b' would then
surprise the user with a picker. Show "pick: main, develop" in the
parenthetical when the resolver reports the base is ambiguous so the
upcoming prompt is no longer a surprise.
The new PickBaseBranchLabel i18n string lives next to the existing
PickBaseBranchTitle/Prompt so the disambiguation UI is grouped in
one place.
Same idea as the rebase menu's label: when the resolver reports the selected branch's base is ambiguous, show "pick: main, develop" in the parenthetical instead of just the config-order tiebreak, so the user knows the prompt will appear before they press 'b'.
Same idea as the rebase and view-divergence labels: when the base is ambiguous, the menu prompt and the "New branch from base branch (...)" item both substitute "pick: main, develop" for the parenthetical so the user knows the disambiguation picker will appear before they pick the "from base" option.
ShortBranchName previously turned "refs/remotes/origin/main" into
"origin/main", leaking the resolved-ref shape into UI labels that
the user thinks of as plain "main" — the name they put in their
mainBranches config. With the ambiguous-base label now potentially
listing several branches ("pick: origin/main, origin/13"), the noise
is even more pronounced. Strip the remote name along with the
"refs/remotes/" prefix so the short name matches what the user
configured.
Rename the helper to BaseBranchDisplayName to make the constraint
explicit at every call site: dropping the remote is a sensible
choice only for base-branch display, not for refs in general. All
current callers happen to be base-branch related, so the rename is
just a scope-tightening.
Existing integration test updated to expect the bare "master" form.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lazygit's promise was that you can configure several main branches (e.g.
main,next, anddevelop) and it will always pick the right one depending on where your feature branch forked off. Unfortunately this turns out not to be true; it can sometimes happen that you start off with a feature branch forked offmain, and then rebasing it onto its base branch rebases it ontodevelop(becausemainwas merged intodevelopmeanwhile).This PR improves the situation by guessing the base branch a bit better in certain cases, and by prompting the user to pick the base branch in cases where it still can't tell.
See #5635 for lots of detail about this.
Closes #5635.