Repository navigation
Reordering history can accidentally "merge" an intermediate PR in --direct mode #367
Description
Haven't put a lot of time into understanding this one, was just trying to evaluate ghstack for work on LLVM. The issue was that while editing and reordering commits, one of their PRs was automatically marked "Merged", when I had not merged anything. I assume this is due to the lack of an atomic way to update base branches for several PRs at once, but unsure the exact cause.
Simplest way to reproduce turned out to be to just swap the position of two commits in a stack repeatedly:
#!/bin/bash
stack() {
ghstack --direct || return 1
# attempt to rule out racing against a slow GH or something, doesn't seem to
# matter, and I hit the bug originally while just using the tool by-hand
#sleep 10
}
swap2() {
# swap the 2nd and 3rd `pick` lines in the interactive rebase buffer
GIT_EDITOR='printf "%s\n" 2y 2d x w | ed -s' git rebase -i origin/main
}
init() {
for n in $(seq 4); do
echo $n >> $n && git add -A && git commit -m $n || return 1
done
}
whirl() {
# seems to pretty regularly break on round 3, but add plenty to be sure.
# not actually clear to me if this is deterministic or not
for n in $(seq 10); do
stack && swap2 || {
printf 'failed on round %d\n' $n
return 1
}
done
}
main() {
init && whirl
}
"$@"
Can run as foo.sh main to have it build a short stack and then start swapping and restacking until it breaks.
If there is more info I can share, let me know. I only spent enough time to be sure I wasn't just holding it wrong.
Activity
smeenai commentedon Aug 31, 2026
For whatever it's worth, I've been maintaining a tiny fork for LLVM usage (I'll open up issues to discuss what's needed), but smeenai@ecaf2b4 is the purely Claude-generated fix for this (in case there's anything in there that's worth keeping).
ezyang commentedon Sep 4, 2026
Fixed on main in a7a5bba, which is @smeenai's fix from smeenai@ecaf2b4 cherry-picked as-is (author preserved). Thanks @smeenai, and thanks @slinder1 for the repro script.
Before pushing, pull requests whose base is moving now get parked on the default branch, so no head is ever reachable from a base GitHub still has on file. The fake GitHub used in tests now models the merged-on-reachability behavior, so both the existing direct-mode reorder test and a new repeated-swap test would catch a regression. CI is green.
smeenai commentedon Sep 9, 2026
Sorry, we were having a bunch of discussions in https://discourse.llvm.org/t/is-there-any-stacked-pr-methodology-consensus/91558/20, and I should have followed up here as well. @slinder1 pointed out that the unconditional re-parking caused some noise in the PR history, and Claude generated smeenai@f7c60ba as a follow-up.
ezyang commentedon Sep 24, 2026
The follow-up from smeenai@f7c60ba landed in #368 (68d401f): in direct mode, only pull requests whose new head would become reachable from their old base are parked on the default branch now, so a PR whose base merely moves (e.g. a commit inserted below it) no longer gets two extra base-change entries in its timeline. Thanks @smeenai and @slinder1!