Skip to content

Commit 6c99628

Browse files
Phlogistiqueclaude
andauthored
Fix conflict resolutions stranding the PR when its base branch moved (#59)
In scortexio/gh-stack-mv#36 a conflict resolution followed the posted instructions but the action never resumed and the conflict label stayed stuck. The resume rides on a `synchronize` event, and GitHub creates no `pull_request` runs for a PR that conflicts with its base. The PR did conflict with its base (the merged branch, kept until the resume retargets it): autorestack itself had advanced that branch when a grandparent PR merged, so the head no longer descended from its tip, and the resolution rewrote the same lines the squash reshaped, so GitHub's textual mergeability check failed too. The old merge dance guaranteed the descent structurally; the switch to git-merge-onto (#56) dropped that guarantee. This re-vendors git-merge-onto 0.2.0 with its new `--absorbed` flag (scortexio/git-merge-onto#3) and passes it both in the action's own re-parent and in the posted resolution command. The merged branch's tip is recorded as an extra parent of the merge (on a conflict it rides along in `MERGE_HEAD`, so the user's plain `git commit` records it), so the pushed resolution descends from its base again and the resume event is guaranteed to fire. New unit test replays the incident: parent advanced after the child forked, squash-merged, conflicting re-parent, resolution via the posted command; asserts the resolution descends from the old base's tip. The e2e gets the same assertion on its real conflict scenario. Do not merge before git-merge-onto 0.2.0 is on PyPI: the posted command, which the e2e replays verbatim, runs `uvx 'git-merge-onto>=0.2'`. --- _Generated by [Claude Code](https://claude.ai/code/session_01PHUVU3Ek3U9j3LrdjPnyAy)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 066613a commit 6c99628

6 files changed

Lines changed: 244 additions & 37 deletions

File tree

.github/workflows/tests.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ jobs:
1818
bash tests/test_rebase_workflow.sh
1919
bash tests/test_mixed_workflows.sh
2020
bash tests/test_conflict_resolution_resume.sh
21+
bash tests/test_conflict_absorbed_resolution.sh
2122
bash tests/test_merge_commit_merge.sh
2223
bash tests/test_rebase_merge_skip.sh
2324

git-merge-onto

Lines changed: 49 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
# dependencies = []
55
# ///
66
#
7-
# Vendored from https://github.com/scortexio/git-merge-onto (v0.1.0): a single
7+
# Vendored from https://github.com/scortexio/git-merge-onto (v0.2.0): a single
88
# zero-dependency file so the action re-parents a branch without a network
99
# download. Do not edit here -- change it upstream, publish, and re-sync.
1010
"""git merge-onto: re-parent HEAD onto <new>, dropping <old>.
@@ -21,6 +21,11 @@ its commit (a squash-merge) -- and a plain merge re-applies <old>, often
2121
conflicting. Too high -- when the new parent transitively contains HEAD's own
2222
commit (a reorder) -- and a plain merge fast-forwards, silently dropping HEAD's
2323
change. Forcing the base to merge-base(HEAD, <old>) is correct in both.
24+
25+
With --absorbed, the merge also records <old>'s tip as a parent: an assertion
26+
that <new> already carries <old>'s changes even though <old> is not its
27+
ancestor (squash merge, rebase merge, cherry-picks), so git and GitHub treat
28+
<old> as merged instead of dropped.
2429
"""
2530

2631
from __future__ import annotations
@@ -32,7 +37,7 @@ import subprocess
3237
import sys
3338
from pathlib import Path
3439

35-
__version__ = "0.1.0" # vendored snapshot; see the header above
40+
__version__ = "0.2.0" # vendored snapshot; see the header above
3641

3742
# Point at a specific git binary (used by the test suite; "git" otherwise).
3843
GIT = os.environ.get("GIT_MERGE_ONTO_GIT", "git")
@@ -113,11 +118,8 @@ def git_dir() -> Path:
113118
return Path(git("rev-parse", "--absolute-git-dir"))
114119

115120

116-
def worktree_dirty(include_untracked: bool = True) -> bool:
117-
args = ["status", "--porcelain"]
118-
if not include_untracked:
119-
args.append("--untracked-files=no")
120-
return bool(git(*args))
121+
def worktree_dirty() -> bool:
122+
return bool(git("status", "--porcelain"))
121123

122124

123125
def in_progress_merge() -> bool:
@@ -141,43 +143,52 @@ def blocking_operation() -> str | None:
141143
return None
142144

143145

144-
def setup_merge_markers(theirs: str, message: str, head_tip: str) -> None:
146+
def setup_merge_markers(merge_parents: list[str], message: str, head_tip: str) -> None:
145147
"""Write the in-progress-merge state `git commit` reads to finalize a merge:
146-
parents come from HEAD + MERGE_HEAD, the message from MERGE_MSG."""
148+
parents come from HEAD + MERGE_HEAD (one per line), the message from MERGE_MSG."""
147149
gd = git_dir()
148-
(gd / "MERGE_HEAD").write_text(theirs + "\n")
150+
(gd / "MERGE_HEAD").write_text("".join(p + "\n" for p in merge_parents))
149151
(gd / "MERGE_MODE").write_text("")
150152
(gd / "MERGE_MSG").write_text(message + "\n")
151153
(gd / "ORIG_HEAD").write_text(head_tip + "\n")
152154

153155

154-
def merge_with_base(base: str, theirs: str, message: str) -> bool:
156+
def merge_with_base(base: str, theirs: str, message: str, extra_parent: str | None = None) -> bool:
155157
"""Merge `theirs` into HEAD as if `base` were the merge base -- a `git merge`
156158
with a caller-chosen base, the one thing git porcelain cannot do.
157159
158160
Clean -> commits with parents [HEAD, theirs] and returns True. Conflict ->
159161
leaves the merge in progress (MERGE_HEAD set, conflict markers in the worktree)
160162
and returns False, so the caller (or a human) resolves and `git commit`s normally.
163+
164+
`extra_parent` is recorded as an additional parent, on the clean path and the
165+
conflict path alike (it rides along in MERGE_HEAD, so the resolver's plain
166+
`git commit` picks it up). `git commit` drops any parent that is an ancestor
167+
of another, so a redundant extra parent is harmless.
161168
"""
162169
head_tip = git("rev-parse", "HEAD")
170+
merge_parents = [theirs] if extra_parent is None else [theirs, extra_parent]
163171
# 3-way merge into index+worktree with the merge base forced to `base`.
164172
rc = git_rc("merge-recursive", base, "--", head_tip, theirs)
165173
# merge-recursive returns 0 = clean, 1 = content conflict, >1 = it refused to run
166174
# at all (dirty index/worktree, bad arg). Only set the in-progress-merge markers
167175
# when there is a real merge to finalize; on a refusal, raise so we never fabricate
168176
# a merge commit or clobber an existing MERGE_HEAD.
169177
if rc == 0:
170-
# A re-parent normally changes the tree; if it doesn't AND `theirs` is already
171-
# an ancestor, the merge commit would add nothing (no content, no new ancestor),
172-
# so skip it. (Don't skip merely because `theirs` is an ancestor: re-parenting
173-
# onto a trunk that is already an ancestor still must drop the old parent's content.)
174-
if git("write-tree") == git("rev-parse", "HEAD^{tree}") and git_rc("merge-base", "--is-ancestor", theirs, head_tip) == 0:
178+
# A re-parent normally changes the tree; if it doesn't AND every parent to
179+
# record is already an ancestor, the merge commit would add nothing (no
180+
# content, no new ancestor), so skip it. (Don't skip merely because `theirs`
181+
# is an ancestor: re-parenting onto a trunk that is already an ancestor still
182+
# must drop the old parent's content.)
183+
if git("write-tree") == git("rev-parse", "HEAD^{tree}") and all(
184+
git_rc("merge-base", "--is-ancestor", p, head_tip) == 0 for p in merge_parents
185+
):
175186
return True
176-
setup_merge_markers(theirs, message, head_tip)
187+
setup_merge_markers(merge_parents, message, head_tip)
177188
git("commit", "--no-edit")
178189
return True
179190
if rc == 1:
180-
setup_merge_markers(theirs, message, head_tip)
191+
setup_merge_markers(merge_parents, message, head_tip)
181192
return False
182193
raise CommandError(
183194
[GIT, "merge-recursive", base, "--", head_tip, theirs],
@@ -192,10 +203,15 @@ def _resolve_commit(ref: str) -> str | None:
192203
return rev_parse(ref) or rev_parse(f"origin/{ref}")
193204

194205

195-
def merge_onto(new: str, old: str, message: str | None = None) -> bool:
206+
def merge_onto(new: str, old: str, message: str | None = None, absorbed: bool = False) -> bool:
196207
"""Re-parent HEAD onto `new`, dropping `old`. Returns True on a clean merge
197208
(committed), False on a conflict (left in progress to resolve and commit).
198-
Raises UserError on a precondition failure (dirty tree, bad ref, no ancestor)."""
209+
Raises UserError on a precondition failure (dirty tree, bad ref, no ancestor).
210+
211+
`absorbed` asserts that `new` already carries `old`'s changes without its
212+
commits (squash merge, rebase merge, cherry-picks): `old`'s tip is then
213+
recorded as an extra parent of the merge, so the result descends from it
214+
and git and GitHub treat `old` as merged instead of dropped."""
199215
# merge-recursive writes straight into the index/worktree, so refuse to run during
200216
# another git operation or on a dirty tree rather than corrupt either.
201217
op = blocking_operation()
@@ -215,11 +231,11 @@ def merge_onto(new: str, old: str, message: str | None = None) -> bool:
215231
if not base:
216232
raise UserError(f"no common ancestor between HEAD and old parent {old!r}")
217233
msg = message or f"Merge {new} into HEAD, dropping {old}"
218-
return merge_with_base(base, new_sha, msg)
234+
return merge_with_base(base, new_sha, msg, extra_parent=old_sha if absorbed else None)
219235

220236

221-
def cmd_merge_onto(new: str, old: str, message: str | None) -> int:
222-
if merge_onto(new, old, message):
237+
def cmd_merge_onto(new: str, old: str, message: str | None, absorbed: bool = False) -> int:
238+
if merge_onto(new, old, message, absorbed=absorbed):
223239
print(bold(f"git merge-onto: merged {new} into HEAD, dropping {old}."), file=sys.stderr)
224240
return 0
225241
print(
@@ -242,6 +258,15 @@ def build_parser() -> argparse.ArgumentParser:
242258
),
243259
)
244260
p.add_argument("-m", "--message", help="commit message for a clean merge")
261+
p.add_argument(
262+
"--absorbed",
263+
action="store_true",
264+
help=(
265+
"assert that <new> already carries <old>'s changes (squash merge, rebase "
266+
"merge, cherry-picks): record <old> as an extra parent of the merge, so "
267+
"git and GitHub treat <old> as merged instead of dropped"
268+
),
269+
)
245270
p.add_argument("--quiet", action="store_true", help="do not echo executed git commands")
246271
p.add_argument("--version", action="version", version=f"git-merge-onto {__version__}")
247272
p.add_argument("new", help="the new parent to merge into HEAD")
@@ -255,7 +280,7 @@ def main(argv: list[str] | None = None) -> int:
255280
if args.quiet:
256281
VERBOSE = False
257282
try:
258-
return cmd_merge_onto(args.new, args.old, args.message)
283+
return cmd_merge_onto(args.new, args.old, args.message, args.absorbed)
259284
except UserError as e:
260285
print(red(f"git merge-onto: error: {e}"), file=sys.stderr)
261286
return 2

tests/mock_gh.sh

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,11 @@ elif [[ "$1" == "pr" && "$2" == "edit" ]]; then
2222
# Just log the edit command
2323
echo "Mock: gh pr edit $3 --base $5"
2424
elif [[ "$1" == "pr" && "$2" == "comment" ]]; then
25+
# Consume the body when it comes on stdin (-F -); keep a copy for tests
26+
# that assert on the comment's content.
27+
if [[ " $* " == *" -F "* ]]; then
28+
cat > "${MOCK_COMMENT_FILE:-/dev/null}"
29+
fi
2530
# Just log the comment command
2631
echo "Mock: gh pr comment $3"
2732
elif [[ "$1" == "api" && "$2" == repos/*/commits/*/pulls ]]; then
Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
#!/bin/bash
2+
#
3+
# Replays the incident that motivated --absorbed (scortexio/gh-stack-mv#36):
4+
#
5+
# 1. main <- feature1 <- feature2, where feature1 advanced AFTER feature2
6+
# forked (in the incident, autorestack itself advanced it when a
7+
# grandparent PR merged). feature1's tip is therefore NOT an ancestor of
8+
# feature2.
9+
# 2. feature1 is squash-merged; the action's re-parent of feature2 conflicts,
10+
# so the action posts the resolution comment and stops.
11+
# 3. The user resolves by running the posted re-parent with --absorbed.
12+
#
13+
# The resolution must descend from origin/feature1's tip: feature2's PR is
14+
# still based on feature1 at that point, and GitHub creates no pull_request
15+
# runs for a PR that conflicts with its base, so without that ancestry the
16+
# resume event may never fire and the conflict label stays stuck forever.
17+
18+
set -ueo pipefail
19+
20+
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
21+
source "$SCRIPT_DIR/../command_utils.sh"
22+
23+
simulate_push() {
24+
log_cmd git update-ref "refs/remotes/origin/$1" "$1"
25+
}
26+
27+
TEST_REPO=$(mktemp -d)
28+
cd "$TEST_REPO"
29+
echo "Created test repo at $TEST_REPO"
30+
31+
log_cmd git init -b main
32+
log_cmd git config user.email "test@example.com"
33+
log_cmd git config user.name "Test User"
34+
35+
for i in $(seq 1 10); do echo "line $i" >> file.txt; done
36+
log_cmd git add file.txt
37+
log_cmd git commit -m "Initial commit"
38+
simulate_push main
39+
40+
# feature1: modify line 2
41+
log_cmd git checkout -b feature1
42+
sed -i '2s/.*/Feature 1 version A/' file.txt
43+
log_cmd git add file.txt
44+
log_cmd git commit -m "feature1: commit A"
45+
simulate_push feature1
46+
47+
# feature2 forks here and modifies the same line 2: its resolution will rewrite
48+
# the very lines the squash reshapes, which is what made the incident's PR read
49+
# as conflicting with its base.
50+
log_cmd git checkout -b feature2
51+
sed -i '2s/.*/Feature 2 version/' file.txt
52+
log_cmd git add file.txt
53+
log_cmd git commit -m "feature2: change line 2"
54+
simulate_push feature2
55+
56+
# feature1 advances after the fork: its tip is no longer an ancestor of feature2.
57+
log_cmd git checkout feature1
58+
sed -i '2s/.*/Feature 1 version B/' file.txt
59+
log_cmd git add file.txt
60+
log_cmd git commit -m "feature1: commit B"
61+
simulate_push feature1
62+
FEATURE1_TIP=$(git rev-parse feature1)
63+
64+
if git merge-base --is-ancestor "$FEATURE1_TIP" feature2; then
65+
echo "❌ Setup broken: feature1's tip must not be an ancestor of feature2"
66+
exit 1
67+
fi
68+
69+
# Squash-merge feature1 into main
70+
log_cmd git checkout main
71+
log_cmd git merge --squash feature1
72+
log_cmd git commit -m "Squash merge feature1"
73+
SQUASH_COMMIT=$(git rev-parse HEAD)
74+
simulate_push main
75+
76+
FEATURE2_BEFORE=$(git rev-parse feature2)
77+
# Outside the test repo: an untracked file makes git-merge-onto refuse to run.
78+
COMMENT_FILE=$(mktemp)
79+
80+
# The action must hit the conflict path: comment, label, no push, branch kept.
81+
OUT=$(env \
82+
SQUASH_COMMIT="$SQUASH_COMMIT" \
83+
MERGED_BRANCH=feature1 \
84+
PR_NUMBER=1 \
85+
TARGET_BRANCH=main \
86+
GH="$SCRIPT_DIR/mock_gh.sh" \
87+
GIT="$SCRIPT_DIR/mock_git.sh" \
88+
MOCK_COMMENT_FILE="$COMMENT_FILE" \
89+
"$SCRIPT_DIR/../update-pr-stack.sh" 2>&1)
90+
echo "$OUT"
91+
92+
if ! grep -q "Mock: gh pr comment 2" <<<"$OUT"; then
93+
echo "❌ Expected a conflict comment on the child PR"
94+
exit 1
95+
fi
96+
if grep -q "push origin :feature1" <<<"$OUT"; then
97+
echo "❌ The merged branch must be kept while the child is conflicted"
98+
exit 1
99+
fi
100+
if [[ "$(git rev-parse feature2)" != "$FEATURE2_BEFORE" ]]; then
101+
echo "❌ feature2 must not move on a conflict"
102+
exit 1
103+
fi
104+
if ! grep -q -- "--absorbed" "$COMMENT_FILE"; then
105+
echo "❌ The posted resolution command must use --absorbed"
106+
cat "$COMMENT_FILE"
107+
exit 1
108+
fi
109+
echo "✅ Conflict detected: comment posted with --absorbed, stack left alone"
110+
111+
# Resolve as the comment instructs, with the vendored copy standing in for
112+
# `uvx git-merge-onto` (unit tests have no network).
113+
log_cmd git checkout feature2
114+
if python3 "$SCRIPT_DIR/../git-merge-onto" --absorbed origin/main origin/feature1; then
115+
echo "❌ The re-parent should conflict (both sides rewrote line 2)"
116+
exit 1
117+
fi
118+
sed -i '/^<<<<<<</,/^>>>>>>>/c\Feature 2 version' file.txt
119+
log_cmd git add file.txt
120+
log_cmd git commit --no-edit
121+
simulate_push feature2
122+
123+
# The regression assertion: the resolution descends from the old base's tip.
124+
if log_cmd git merge-base --is-ancestor "$FEATURE1_TIP" feature2; then
125+
echo "✅ Resolution descends from origin/feature1's tip (--absorbed recorded it)"
126+
else
127+
echo "❌ Resolution does not descend from origin/feature1's tip"
128+
log_cmd git log --graph --oneline feature2 feature1
129+
exit 1
130+
fi
131+
if ! log_cmd git merge-base --is-ancestor "$SQUASH_COMMIT" feature2; then
132+
echo "❌ Resolution must contain the squash commit (resume checks this)"
133+
exit 1
134+
fi
135+
136+
# And the content is the user's resolution on top of main.
137+
if [[ "$(sed -n '2p' file.txt)" == "Feature 2 version" ]]; then
138+
echo "✅ Resolved content kept"
139+
else
140+
echo "❌ Resolved content lost: $(sed -n '2p' file.txt)"
141+
exit 1
142+
fi
143+
144+
echo ""
145+
echo "All absorbed-resolution tests passed! 🎉"
146+
echo "Test repository remains at: $TEST_REPO for inspection"

tests/test_e2e.sh

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -276,16 +276,16 @@ get_conflict_comment() {
276276
}
277277

278278
# The conflict comment must tell the user to run the re-parent the action tried:
279-
# a single `uvx git-merge-onto origin/<target> origin/<merged branch>`.
279+
# a single `uvx git-merge-onto --absorbed origin/<target> origin/<merged branch>`.
280280
assert_conflict_comment_reparent() {
281281
local comment=$1
282282
local target=$2
283283
local merged=$3
284284

285-
if echo "$comment" | grep -qxF "uvx git-merge-onto origin/$target origin/$merged"; then
285+
if echo "$comment" | grep -qxF "uvx git-merge-onto origin/$target origin/$merged --absorbed"; then
286286
echo >&2 "✅ Verification Passed: conflict comment re-parents origin/$merged onto origin/$target."
287287
else
288-
echo >&2 "❌ Verification Failed: conflict comment lacks 'uvx git-merge-onto origin/$target origin/$merged'."
288+
echo >&2 "❌ Verification Failed: conflict comment lacks 'uvx git-merge-onto origin/$target origin/$merged --absorbed'."
289289
echo >&2 "--- Full comment ---"
290290
echo >&2 "$comment"
291291
exit 1
@@ -1035,9 +1035,11 @@ else
10351035
fi
10361036

10371037
# The re-parent is one atomic merge, so on a conflict the action commits and
1038-
# pushes nothing: origin/feature3 must still sit at its pre-conflict head. That
1039-
# unchanged head stays a descendant of its base (feature2), so the PR is mergeable
1040-
# and the synchronize event that resumes the action keeps firing.
1038+
# pushes nothing: origin/feature3 must still sit at its pre-conflict head. The
1039+
# resume is guaranteed by the resolution itself: its --absorbed re-parent records
1040+
# origin/feature2's tip as a parent, so the pushed head descends from its base
1041+
# and GitHub creates the synchronize run (it creates none for a PR that
1042+
# conflicts with its base). Asserted after the resolution below.
10411043
REMOTE_FEATURE3_SHA_BEFORE_RESOLVE=$(log_cmd git rev-parse "refs/remotes/origin/feature3")
10421044
if [[ "$REMOTE_FEATURE3_SHA_BEFORE_RESOLVE" == "$FEATURE3_CONFLICT_COMMIT_SHA" ]]; then
10431045
echo >&2 "✅ Verification Passed: action pushed nothing on the conflict; origin/feature3 is unchanged."
@@ -1050,6 +1052,10 @@ fi
10501052

10511053
# 12. Resolve the conflict by following the comment the action posted.
10521054
echo >&2 "12. Resolving conflict on feature3 by following the posted comment..."
1055+
# Record the base branch's tip now: the resume deletes feature2, and step 15
1056+
# asserts the resolution descends from this tip.
1057+
log_cmd git fetch origin
1058+
FEATURE2_TIP_AT_RESOLUTION=$(log_cmd git rev-parse "refs/remotes/origin/feature2")
10531059
# Follow the comment exactly: fetch, fast-forward to origin/feature3, run the
10541060
# re-parent (uvx git-merge-onto), resolve the conflict, and push. Following it
10551061
# must leave feature3 cleanly mergeable into its new base, or the
@@ -1119,6 +1125,17 @@ else
11191125
exit 1
11201126
fi
11211127

1128+
# Verify the --absorbed re-parent recorded the old base's tip as a parent. This
1129+
# is the structural guarantee that PR3, still based on feature2 when the
1130+
# resolution was pushed, read as mergeable so GitHub created the resume run.
1131+
if log_cmd git merge-base --is-ancestor "$FEATURE2_TIP_AT_RESOLUTION" feature3; then
1132+
echo >&2 "✅ Verification Passed: Resolved feature3 descends from feature2's tip (--absorbed)."
1133+
else
1134+
echo >&2 "❌ Verification Failed: Resolved feature3 does not descend from feature2's tip ($FEATURE2_TIP_AT_RESOLUTION)."
1135+
log_cmd git log --graph --oneline feature3
1136+
exit 1
1137+
fi
1138+
11221139
# Note: feature4 is NOT updated (indirect children are not modified).
11231140
# It will be updated when feature3 is merged (becoming a direct child at that point).
11241141
echo >&2 "✅ feature4 intentionally not updated (indirect child of resolved PR)"

0 commit comments

Comments
 (0)