refactor: unify the paused-rebase classifier (RestackBranch + Onto)#91
Merged
Conversation
… Onto Both RestackBranch and Onto open with the same "rebase failed -> is it a paused conflict, a state-probe failure, or an outright failure?" logic. Extract it into rebaseFailure(g, rebaseErr, action, branch, onto), so the definition of "did the rebase pause on a conflict?" lives in one place and the two cannot drift. Each caller keeps its own bookkeeping around the shared classification: Onto still records the PendingReparent (and aborts on a save failure); RestackBranch still restores HEAD on a non-conflict failure. Onto's behaviour is byte-for-byte unchanged. The only behavioural delta: in RestackBranch, when the rebase-state probe itself fails AND the subsequent HEAD restore also fails, the wrapped error is now the (more informative) "checking rebase state ... failed" message rather than the bare rebase error. This corner requires git's rebase-state probe to fail, which no test can reach with the fake git; the new error is strictly more complete. Verified: engine + model/invariant (random op sequences incl. conflicts) + e2e + a manual restack/onto-conflict smoke test (exit 2, st continue promotes the reparent). make ci green, coverage 86.4%.
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.
What
RestackBranch(restack.go) andOnto(engine.go) each contained their own copy of the "aRebaseOntofailed — is it a paused conflict, a rebase-state-probe failure, or an outright failure?" logic. This is the most safety-critical error path in the engine, so two drifting copies are exactly what you don't want.Extracts the classification into one helper:
Each caller keeps its own bookkeeping around it:
PendingReparentand aborts the rebase if the save fails — byte-for-byte unchanged.Review notes — the one behavioural delta
In
RestackBranch, when git's rebase-state probe itself fails and the subsequent HEAD restore also fails, the wrapped error is now the more-informative"checking rebase state … failed (original error: …)"instead of the bare rebase error. Reaching this corner requiresRebaseInProgress()to return an error, which the fake git never does, so no test exercises it; the new message is strictly more complete. Every other path (including all conflict/continue/abort flows) is identical.Verification
Engine unit tests, the model/invariant test (thousands of random op sequences including injected conflicts), e2e, and a manual real-git smoke test of both a restack conflict and an onto conflict (both exit 2;
st continuepromotes the pending reparent correctly).make cigreen — lint 0 issues, coverage 86.4%.