Fix "part N of M" numbering in CLI stacked-PR footers#14152
Open
ameyypawar wants to merge 1 commit into
Open
Conversation
The CLI's stacked-PR footer numbered "part N of M" as `stack_length - stack_index`, the inverse of the `<kbd>` stack labels it prints alongside (base = 1, top = M). Because `all_pr_numbers` is ordered base-to-top, the base PR showed "part M of M" next to its `<kbd> 1 </kbd>` label and the top PR showed "part 1 of M" next to `<kbd> M </kbd>`. Number "part N" as `stack_index + 1` so it agrees with the label. This touches only the Rust footer used by the `but` CLI; the desktop GUI uses the separate TypeScript generator (apps/desktop/src/lib/forge/shared/ prFooter.ts), which orders top-to-base and already renders correctly, so it is intentionally left unchanged. Adds "part N of M" assertions to the base/top footer tests (idea from gitbutlerapp#13363). Fixes gitbutlerapp#13362
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.
Fixes #13362
Problem
In stacked-PR description footers, the
<kbd>labels number the stack base = 1 … top = M, but the "part N of M" text used the opposite convention — so the base PR showed "part 3 of 3" next to its<kbd> 1 </kbd>label, and the top PR showed "part 1 of 3" next to<kbd> 3 </kbd>. This was reported (and reproduced by others) as CLI-only.Root cause
There are two footer generators:
crates/but-forge/src/review.rsgenerate_footerapps/desktop/src/lib/forge/shared/prFooter.tsgenerateFooterIn the Rust generator,
all_pr_numbersis ordered base→top and the<kbd>loop iterates.rev()(so labels are correct: base = 1). Butnthwas computed asstack_length - stack_index, which is the inverse of the label — hence the contradiction.The desktop GUI goes through the TypeScript generator (
ReviewCreation.svelteand the drag handlers importupdatePrDescriptionTables/updateStackPrsfromprFooter.ts), which orders the list top→base and already renders correctly. That's why this only manifested on the CLI.Fix
One line in
review.rs: numbernth = stack_index + 1so "part N" agrees with the<kbd>label at every position (base → 1, top → M). The desktopprFooter.tsgenerator is intentionally left unchanged — it is already correct, and as @Byron noted on #13363 the GUI renders as expected.Tests
test_generate_footer_large_stack, which had asserted the buggypart 6 of 10for PR Auto Update Client #5 (nowpart 5 of 10).part 1 of 3/part 3 of 3assertions to the base/top footer tests so the "part N" /<kbd>agreement is locked in (assertion idea from fix: align "part N of M" with <kbd> labels in stacked PR footers #13363).Verified locally:
cargo test -p but-forge footer→ 11/11 pass; reverting the one-line change fails exactly those 3 tests.clippyandfmtclean.Relation to #13363
#13363 targets the same issue but also rewrites the desktop
prFooter.tsgenerator (changing its numbering and reversing its loop). Since the desktop generator is already correct, this PR keeps the change scoped to the Rust/CLI path where the bug actually lives, avoiding a regression to the GUI footer.