diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 95b4ad9..fb6f879 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -18,6 +18,7 @@ jobs: bash tests/test_rebase_workflow.sh bash tests/test_mixed_workflows.sh bash tests/test_conflict_resolution_resume.sh + bash tests/test_conflict_absorbed_resolution.sh bash tests/test_merge_commit_merge.sh bash tests/test_rebase_merge_skip.sh diff --git a/git-merge-onto b/git-merge-onto index 2d1f8d7..ba8d643 100755 --- a/git-merge-onto +++ b/git-merge-onto @@ -4,7 +4,7 @@ # dependencies = [] # /// # -# Vendored from https://github.com/scortexio/git-merge-onto (v0.1.0): a single +# Vendored from https://github.com/scortexio/git-merge-onto (v0.2.0): a single # zero-dependency file so the action re-parents a branch without a network # download. Do not edit here -- change it upstream, publish, and re-sync. """git merge-onto: re-parent HEAD onto , dropping . @@ -21,6 +21,11 @@ its commit (a squash-merge) -- and a plain merge re-applies , often conflicting. Too high -- when the new parent transitively contains HEAD's own commit (a reorder) -- and a plain merge fast-forwards, silently dropping HEAD's change. Forcing the base to merge-base(HEAD, ) is correct in both. + +With --absorbed, the merge also records 's tip as a parent: an assertion +that already carries 's changes even though is not its +ancestor (squash merge, rebase merge, cherry-picks), so git and GitHub treat + as merged instead of dropped. """ from __future__ import annotations @@ -32,7 +37,7 @@ import subprocess import sys from pathlib import Path -__version__ = "0.1.0" # vendored snapshot; see the header above +__version__ = "0.2.0" # vendored snapshot; see the header above # Point at a specific git binary (used by the test suite; "git" otherwise). GIT = os.environ.get("GIT_MERGE_ONTO_GIT", "git") @@ -113,11 +118,8 @@ def git_dir() -> Path: return Path(git("rev-parse", "--absolute-git-dir")) -def worktree_dirty(include_untracked: bool = True) -> bool: - args = ["status", "--porcelain"] - if not include_untracked: - args.append("--untracked-files=no") - return bool(git(*args)) +def worktree_dirty() -> bool: + return bool(git("status", "--porcelain")) def in_progress_merge() -> bool: @@ -141,25 +143,31 @@ def blocking_operation() -> str | None: return None -def setup_merge_markers(theirs: str, message: str, head_tip: str) -> None: +def setup_merge_markers(merge_parents: list[str], message: str, head_tip: str) -> None: """Write the in-progress-merge state `git commit` reads to finalize a merge: - parents come from HEAD + MERGE_HEAD, the message from MERGE_MSG.""" + parents come from HEAD + MERGE_HEAD (one per line), the message from MERGE_MSG.""" gd = git_dir() - (gd / "MERGE_HEAD").write_text(theirs + "\n") + (gd / "MERGE_HEAD").write_text("".join(p + "\n" for p in merge_parents)) (gd / "MERGE_MODE").write_text("") (gd / "MERGE_MSG").write_text(message + "\n") (gd / "ORIG_HEAD").write_text(head_tip + "\n") -def merge_with_base(base: str, theirs: str, message: str) -> bool: +def merge_with_base(base: str, theirs: str, message: str, extra_parent: str | None = None) -> bool: """Merge `theirs` into HEAD as if `base` were the merge base -- a `git merge` with a caller-chosen base, the one thing git porcelain cannot do. Clean -> commits with parents [HEAD, theirs] and returns True. Conflict -> leaves the merge in progress (MERGE_HEAD set, conflict markers in the worktree) and returns False, so the caller (or a human) resolves and `git commit`s normally. + + `extra_parent` is recorded as an additional parent, on the clean path and the + conflict path alike (it rides along in MERGE_HEAD, so the resolver's plain + `git commit` picks it up). `git commit` drops any parent that is an ancestor + of another, so a redundant extra parent is harmless. """ head_tip = git("rev-parse", "HEAD") + merge_parents = [theirs] if extra_parent is None else [theirs, extra_parent] # 3-way merge into index+worktree with the merge base forced to `base`. rc = git_rc("merge-recursive", base, "--", head_tip, theirs) # merge-recursive returns 0 = clean, 1 = content conflict, >1 = it refused to run @@ -167,17 +175,20 @@ def merge_with_base(base: str, theirs: str, message: str) -> bool: # when there is a real merge to finalize; on a refusal, raise so we never fabricate # a merge commit or clobber an existing MERGE_HEAD. if rc == 0: - # A re-parent normally changes the tree; if it doesn't AND `theirs` is already - # an ancestor, the merge commit would add nothing (no content, no new ancestor), - # so skip it. (Don't skip merely because `theirs` is an ancestor: re-parenting - # onto a trunk that is already an ancestor still must drop the old parent's content.) - if git("write-tree") == git("rev-parse", "HEAD^{tree}") and git_rc("merge-base", "--is-ancestor", theirs, head_tip) == 0: + # A re-parent normally changes the tree; if it doesn't AND every parent to + # record is already an ancestor, the merge commit would add nothing (no + # content, no new ancestor), so skip it. (Don't skip merely because `theirs` + # is an ancestor: re-parenting onto a trunk that is already an ancestor still + # must drop the old parent's content.) + if git("write-tree") == git("rev-parse", "HEAD^{tree}") and all( + git_rc("merge-base", "--is-ancestor", p, head_tip) == 0 for p in merge_parents + ): return True - setup_merge_markers(theirs, message, head_tip) + setup_merge_markers(merge_parents, message, head_tip) git("commit", "--no-edit") return True if rc == 1: - setup_merge_markers(theirs, message, head_tip) + setup_merge_markers(merge_parents, message, head_tip) return False raise CommandError( [GIT, "merge-recursive", base, "--", head_tip, theirs], @@ -192,10 +203,15 @@ def _resolve_commit(ref: str) -> str | None: return rev_parse(ref) or rev_parse(f"origin/{ref}") -def merge_onto(new: str, old: str, message: str | None = None) -> bool: +def merge_onto(new: str, old: str, message: str | None = None, absorbed: bool = False) -> bool: """Re-parent HEAD onto `new`, dropping `old`. Returns True on a clean merge (committed), False on a conflict (left in progress to resolve and commit). - Raises UserError on a precondition failure (dirty tree, bad ref, no ancestor).""" + Raises UserError on a precondition failure (dirty tree, bad ref, no ancestor). + + `absorbed` asserts that `new` already carries `old`'s changes without its + commits (squash merge, rebase merge, cherry-picks): `old`'s tip is then + recorded as an extra parent of the merge, so the result descends from it + and git and GitHub treat `old` as merged instead of dropped.""" # merge-recursive writes straight into the index/worktree, so refuse to run during # another git operation or on a dirty tree rather than corrupt either. op = blocking_operation() @@ -215,11 +231,11 @@ def merge_onto(new: str, old: str, message: str | None = None) -> bool: if not base: raise UserError(f"no common ancestor between HEAD and old parent {old!r}") msg = message or f"Merge {new} into HEAD, dropping {old}" - return merge_with_base(base, new_sha, msg) + return merge_with_base(base, new_sha, msg, extra_parent=old_sha if absorbed else None) -def cmd_merge_onto(new: str, old: str, message: str | None) -> int: - if merge_onto(new, old, message): +def cmd_merge_onto(new: str, old: str, message: str | None, absorbed: bool = False) -> int: + if merge_onto(new, old, message, absorbed=absorbed): print(bold(f"git merge-onto: merged {new} into HEAD, dropping {old}."), file=sys.stderr) return 0 print( @@ -242,6 +258,15 @@ def build_parser() -> argparse.ArgumentParser: ), ) p.add_argument("-m", "--message", help="commit message for a clean merge") + p.add_argument( + "--absorbed", + action="store_true", + help=( + "assert that already carries 's changes (squash merge, rebase " + "merge, cherry-picks): record as an extra parent of the merge, so " + "git and GitHub treat as merged instead of dropped" + ), + ) p.add_argument("--quiet", action="store_true", help="do not echo executed git commands") p.add_argument("--version", action="version", version=f"git-merge-onto {__version__}") p.add_argument("new", help="the new parent to merge into HEAD") @@ -255,7 +280,7 @@ def main(argv: list[str] | None = None) -> int: if args.quiet: VERBOSE = False try: - return cmd_merge_onto(args.new, args.old, args.message) + return cmd_merge_onto(args.new, args.old, args.message, args.absorbed) except UserError as e: print(red(f"git merge-onto: error: {e}"), file=sys.stderr) return 2 diff --git a/tests/mock_gh.sh b/tests/mock_gh.sh index cfeafb9..e2ff8ff 100755 --- a/tests/mock_gh.sh +++ b/tests/mock_gh.sh @@ -22,6 +22,11 @@ elif [[ "$1" == "pr" && "$2" == "edit" ]]; then # Just log the edit command echo "Mock: gh pr edit $3 --base $5" elif [[ "$1" == "pr" && "$2" == "comment" ]]; then + # Consume the body when it comes on stdin (-F -); keep a copy for tests + # that assert on the comment's content. + if [[ " $* " == *" -F "* ]]; then + cat > "${MOCK_COMMENT_FILE:-/dev/null}" + fi # Just log the comment command echo "Mock: gh pr comment $3" elif [[ "$1" == "api" && "$2" == repos/*/commits/*/pulls ]]; then diff --git a/tests/test_conflict_absorbed_resolution.sh b/tests/test_conflict_absorbed_resolution.sh new file mode 100644 index 0000000..a289654 --- /dev/null +++ b/tests/test_conflict_absorbed_resolution.sh @@ -0,0 +1,146 @@ +#!/bin/bash +# +# Replays the incident that motivated --absorbed (scortexio/gh-stack-mv#36): +# +# 1. main <- feature1 <- feature2, where feature1 advanced AFTER feature2 +# forked (in the incident, autorestack itself advanced it when a +# grandparent PR merged). feature1's tip is therefore NOT an ancestor of +# feature2. +# 2. feature1 is squash-merged; the action's re-parent of feature2 conflicts, +# so the action posts the resolution comment and stops. +# 3. The user resolves by running the posted re-parent with --absorbed. +# +# The resolution must descend from origin/feature1's tip: feature2's PR is +# still based on feature1 at that point, and GitHub creates no pull_request +# runs for a PR that conflicts with its base, so without that ancestry the +# resume event may never fire and the conflict label stays stuck forever. + +set -ueo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +source "$SCRIPT_DIR/../command_utils.sh" + +simulate_push() { + log_cmd git update-ref "refs/remotes/origin/$1" "$1" +} + +TEST_REPO=$(mktemp -d) +cd "$TEST_REPO" +echo "Created test repo at $TEST_REPO" + +log_cmd git init -b main +log_cmd git config user.email "test@example.com" +log_cmd git config user.name "Test User" + +for i in $(seq 1 10); do echo "line $i" >> file.txt; done +log_cmd git add file.txt +log_cmd git commit -m "Initial commit" +simulate_push main + +# feature1: modify line 2 +log_cmd git checkout -b feature1 +sed -i '2s/.*/Feature 1 version A/' file.txt +log_cmd git add file.txt +log_cmd git commit -m "feature1: commit A" +simulate_push feature1 + +# feature2 forks here and modifies the same line 2: its resolution will rewrite +# the very lines the squash reshapes, which is what made the incident's PR read +# as conflicting with its base. +log_cmd git checkout -b feature2 +sed -i '2s/.*/Feature 2 version/' file.txt +log_cmd git add file.txt +log_cmd git commit -m "feature2: change line 2" +simulate_push feature2 + +# feature1 advances after the fork: its tip is no longer an ancestor of feature2. +log_cmd git checkout feature1 +sed -i '2s/.*/Feature 1 version B/' file.txt +log_cmd git add file.txt +log_cmd git commit -m "feature1: commit B" +simulate_push feature1 +FEATURE1_TIP=$(git rev-parse feature1) + +if git merge-base --is-ancestor "$FEATURE1_TIP" feature2; then + echo "❌ Setup broken: feature1's tip must not be an ancestor of feature2" + exit 1 +fi + +# Squash-merge feature1 into main +log_cmd git checkout main +log_cmd git merge --squash feature1 +log_cmd git commit -m "Squash merge feature1" +SQUASH_COMMIT=$(git rev-parse HEAD) +simulate_push main + +FEATURE2_BEFORE=$(git rev-parse feature2) +# Outside the test repo: an untracked file makes git-merge-onto refuse to run. +COMMENT_FILE=$(mktemp) + +# The action must hit the conflict path: comment, label, no push, branch kept. +OUT=$(env \ + SQUASH_COMMIT="$SQUASH_COMMIT" \ + MERGED_BRANCH=feature1 \ + PR_NUMBER=1 \ + TARGET_BRANCH=main \ + GH="$SCRIPT_DIR/mock_gh.sh" \ + GIT="$SCRIPT_DIR/mock_git.sh" \ + MOCK_COMMENT_FILE="$COMMENT_FILE" \ + "$SCRIPT_DIR/../update-pr-stack.sh" 2>&1) +echo "$OUT" + +if ! grep -q "Mock: gh pr comment 2" <<<"$OUT"; then + echo "❌ Expected a conflict comment on the child PR" + exit 1 +fi +if grep -q "push origin :feature1" <<<"$OUT"; then + echo "❌ The merged branch must be kept while the child is conflicted" + exit 1 +fi +if [[ "$(git rev-parse feature2)" != "$FEATURE2_BEFORE" ]]; then + echo "❌ feature2 must not move on a conflict" + exit 1 +fi +if ! grep -q -- "--absorbed" "$COMMENT_FILE"; then + echo "❌ The posted resolution command must use --absorbed" + cat "$COMMENT_FILE" + exit 1 +fi +echo "✅ Conflict detected: comment posted with --absorbed, stack left alone" + +# Resolve as the comment instructs, with the vendored copy standing in for +# `uvx git-merge-onto` (unit tests have no network). +log_cmd git checkout feature2 +if python3 "$SCRIPT_DIR/../git-merge-onto" --absorbed origin/main origin/feature1; then + echo "❌ The re-parent should conflict (both sides rewrote line 2)" + exit 1 +fi +sed -i '/^<<<<<<>>>>>>/c\Feature 2 version' file.txt +log_cmd git add file.txt +log_cmd git commit --no-edit +simulate_push feature2 + +# The regression assertion: the resolution descends from the old base's tip. +if log_cmd git merge-base --is-ancestor "$FEATURE1_TIP" feature2; then + echo "✅ Resolution descends from origin/feature1's tip (--absorbed recorded it)" +else + echo "❌ Resolution does not descend from origin/feature1's tip" + log_cmd git log --graph --oneline feature2 feature1 + exit 1 +fi +if ! log_cmd git merge-base --is-ancestor "$SQUASH_COMMIT" feature2; then + echo "❌ Resolution must contain the squash commit (resume checks this)" + exit 1 +fi + +# And the content is the user's resolution on top of main. +if [[ "$(sed -n '2p' file.txt)" == "Feature 2 version" ]]; then + echo "✅ Resolved content kept" +else + echo "❌ Resolved content lost: $(sed -n '2p' file.txt)" + exit 1 +fi + +echo "" +echo "All absorbed-resolution tests passed! 🎉" +echo "Test repository remains at: $TEST_REPO for inspection" diff --git a/tests/test_e2e.sh b/tests/test_e2e.sh index 78b2d7a..358056f 100755 --- a/tests/test_e2e.sh +++ b/tests/test_e2e.sh @@ -276,16 +276,16 @@ get_conflict_comment() { } # The conflict comment must tell the user to run the re-parent the action tried: -# a single `uvx git-merge-onto origin/ origin/`. +# a single `uvx git-merge-onto --absorbed origin/ origin/`. assert_conflict_comment_reparent() { local comment=$1 local target=$2 local merged=$3 - if echo "$comment" | grep -qxF "uvx git-merge-onto origin/$target origin/$merged"; then + if echo "$comment" | grep -qxF "uvx git-merge-onto origin/$target origin/$merged --absorbed"; then echo >&2 "✅ Verification Passed: conflict comment re-parents origin/$merged onto origin/$target." else - echo >&2 "❌ Verification Failed: conflict comment lacks 'uvx git-merge-onto origin/$target origin/$merged'." + echo >&2 "❌ Verification Failed: conflict comment lacks 'uvx git-merge-onto origin/$target origin/$merged --absorbed'." echo >&2 "--- Full comment ---" echo >&2 "$comment" exit 1 @@ -1035,9 +1035,11 @@ else fi # The re-parent is one atomic merge, so on a conflict the action commits and -# pushes nothing: origin/feature3 must still sit at its pre-conflict head. That -# unchanged head stays a descendant of its base (feature2), so the PR is mergeable -# and the synchronize event that resumes the action keeps firing. +# pushes nothing: origin/feature3 must still sit at its pre-conflict head. The +# resume is guaranteed by the resolution itself: its --absorbed re-parent records +# origin/feature2's tip as a parent, so the pushed head descends from its base +# and GitHub creates the synchronize run (it creates none for a PR that +# conflicts with its base). Asserted after the resolution below. REMOTE_FEATURE3_SHA_BEFORE_RESOLVE=$(log_cmd git rev-parse "refs/remotes/origin/feature3") if [[ "$REMOTE_FEATURE3_SHA_BEFORE_RESOLVE" == "$FEATURE3_CONFLICT_COMMIT_SHA" ]]; then echo >&2 "✅ Verification Passed: action pushed nothing on the conflict; origin/feature3 is unchanged." @@ -1050,6 +1052,10 @@ fi # 12. Resolve the conflict by following the comment the action posted. echo >&2 "12. Resolving conflict on feature3 by following the posted comment..." +# Record the base branch's tip now: the resume deletes feature2, and step 15 +# asserts the resolution descends from this tip. +log_cmd git fetch origin +FEATURE2_TIP_AT_RESOLUTION=$(log_cmd git rev-parse "refs/remotes/origin/feature2") # Follow the comment exactly: fetch, fast-forward to origin/feature3, run the # re-parent (uvx git-merge-onto), resolve the conflict, and push. Following it # must leave feature3 cleanly mergeable into its new base, or the @@ -1119,6 +1125,17 @@ else exit 1 fi +# Verify the --absorbed re-parent recorded the old base's tip as a parent. This +# is the structural guarantee that PR3, still based on feature2 when the +# resolution was pushed, read as mergeable so GitHub created the resume run. +if log_cmd git merge-base --is-ancestor "$FEATURE2_TIP_AT_RESOLUTION" feature3; then + echo >&2 "✅ Verification Passed: Resolved feature3 descends from feature2's tip (--absorbed)." +else + echo >&2 "❌ Verification Failed: Resolved feature3 does not descend from feature2's tip ($FEATURE2_TIP_AT_RESOLUTION)." + log_cmd git log --graph --oneline feature3 + exit 1 +fi + # Note: feature4 is NOT updated (indirect children are not modified). # It will be updated when feature3 is merged (becoming a direct child at that point). echo >&2 "✅ feature4 intentionally not updated (indirect child of resolved PR)" diff --git a/update-pr-stack.sh b/update-pr-stack.sh index ee20077..f93de17 100755 --- a/update-pr-stack.sh +++ b/update-pr-stack.sh @@ -200,9 +200,12 @@ See $GITHUB_SERVER_URL/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID" # commit with the base forced to merge-base(HEAD, origin/$MERGED_BRANCH). That # drops the merged branch's content (now carried by the target via the squash) # while keeping the child's own changes -- the merge equivalent of - # `git rebase --onto`, done by the vendored git-merge-onto. + # `git rebase --onto`, done by the vendored git-merge-onto. --absorbed records + # the merged branch's tip as an extra parent: the head then still descends + # from its current base (the merged branch) even when that base moved after + # the head forked, so the PR reads as mergeable until it is retargeted. local RC=0 - try python3 "$SCRIPT_DIR/git-merge-onto" -m "$MERGE_MSG" SQUASH_COMMIT "origin/$MERGED_BRANCH" || RC=$? + try python3 "$SCRIPT_DIR/git-merge-onto" --absorbed -m "$MERGE_MSG" SQUASH_COMMIT "origin/$MERGED_BRANCH" || RC=$? if [[ "$RC" -eq 0 ]]; then return 0 fi @@ -211,10 +214,20 @@ See $GITHUB_SERVER_URL/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID" fi # Conflict (exit 1): git-merge-onto committed nothing and left the merge in - # progress, so the head is unchanged and still a descendant of its base -- the - # PR stays mergeable and the synchronize event that resumes this action keeps - # firing. Clean the runner's tree, ask the user to resolve, and record the state - # so the next push can resume. The label comes last: it is what re-triggers us. + # progress. Clean the runner's tree, ask the user to resolve, and record the + # state so the next push can resume. The label comes last: it is what + # re-triggers us. + # + # The resume rides on a synchronize event, and GitHub creates no pull_request + # runs for a PR that conflicts with its base. This PR's base is the merged + # branch (kept until the resume retargets it), and the head does not always + # descend from its tip: when an earlier run updated that branch after this + # head forked (an ancestor PR merged first), GitHub falls back to a textual + # merge to decide mergeability, which fails exactly when the resolution + # rewrote the same lines. That would strand the PR: no run, label stuck. + # --absorbed in the posted command is what prevents this: it records the + # merged branch's tip as a parent of the resolution, so the pushed head + # descends from its base again and the resume event is guaranteed to fire. abort_merge_if_in_progress local SQUASH_HASH_FOR_MARKER SQUASH_HASH_FOR_MARKER=$(git rev-parse SQUASH_COMMIT) || die "cannot resolve SQUASH_COMMIT" @@ -226,7 +239,7 @@ See $GITHUB_SERVER_URL/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID" echo "git fetch origin" echo "git switch $BRANCH" echo "git merge --ff-only origin/$BRANCH" - echo "uvx git-merge-onto origin/$BASE_BRANCH origin/$MERGED_BRANCH" + echo "uvx git-merge-onto origin/$BASE_BRANCH origin/$MERGED_BRANCH --absorbed" echo '```' echo echo 'Fix the conflicts (for instance with `git mergetool`), then run `git add -A && git commit` to finish the merge.'