From 3132b13abaf782898baae1f3cedec1032c105645 Mon Sep 17 00:00:00 2001 From: Razvan-Liviu Varzaru Date: Thu, 15 Oct 2026 11:28:37 +0300 Subject: [PATCH] Attribute forward-merge conflicts per file on the first-parent line Old behaviour: walked `rev-list --no-merges target..source`, so it also test-merged commits that only arrived through merges (e.g. 10.11 commits in 11.4) straight into the target, against the wrong merge base. The first failure, even one touching none of the conflicted files, was shown as the "first likely source-side commit" for all of them. That is how 23367c88 was blamed for the 11.4 -> 11.8 conflicts. New behaviour: walks only the source's first-parent line, with merges as single steps, using in-memory `git merge-tree` instead of a worktree per commit. Each conflicted file is attributed to the commit where it starts to conflict, and the report groups files by that commit. For a merge it names the merger and lists the merged-in commits that touch the files. The health fingerprint changes shape, so a blocked chain sends one "blocked result changed" notification after deploy. Co-Authored-By: Claude Opus 5.5 --- .github/forward-merge-check/README.md | 24 +- .../forward-merge-check/checker/git_ops.py | 56 +++- .../checker/merge_engine.py | 116 ++++++-- .github/forward-merge-check/checker/models.py | 18 +- .github/forward-merge-check/checker/modes.py | 21 +- .../checker/notification_text.py | 62 +++-- .github/forward-merge-check/checker/output.py | 22 +- .github/forward-merge-check/checker/state.py | 50 +++- .../tests/test_forward_mergeability.py | 250 ++++++++++++++++-- 9 files changed, 520 insertions(+), 99 deletions(-) diff --git a/.github/forward-merge-check/README.md b/.github/forward-merge-check/README.md index d1220cd..ff88d51 100644 --- a/.github/forward-merge-check/README.md +++ b/.github/forward-merge-check/README.md @@ -91,14 +91,28 @@ failed synthetic 10.11 -> 11.4 When a merge fails, the script reports: - blocked edge -- conflicted files -- first likely source-side commit that fails this target merge +- conflicted files, grouped by the source-side commit where each file starts + to conflict - candidate source-side and target-side commits that touched conflicted files + (console output only) + +To find where a file starts to conflict, the checker walks the source's own +first-parent line, newest first, and merges each commit into the target in +memory with `git merge-tree --write-tree` (git 2.38 or newer). A file's origin +is the commit where it last went from merging cleanly to conflicting. + +Merge commits such as "Merge 10.11 into 11.4" are single steps in that walk. +The 10.11 commits they brought in are not merged into 11.8 on their own, because +that would use a different merge base and skip the merge that may already have +adapted them. When the origin is a merge, the report lists the commits it +brought in that touch the files, and names the merge author as "Merged by" +rather than "Author". When the origin is the checker's own test merge of earlier +branches, the report says "unmerged changes from earlier branches" and lists +the commits from those branches that touch the files. Candidate commits come from `git log --left-right --merge` while the repository -is still in the conflicted merge state. The "first likely" commit is an -approximation. Git conflicts come from the interaction between both branches, so -this is a debugging hint, not proof. +is still in the conflicted merge state. They show who touched a conflicted file, +not whose change conflicts. ## Repository Model diff --git a/.github/forward-merge-check/checker/git_ops.py b/.github/forward-merge-check/checker/git_ops.py index bf2f102..1409303 100644 --- a/.github/forward-merge-check/checker/git_ops.py +++ b/.github/forward-merge-check/checker/git_ops.py @@ -112,13 +112,61 @@ def is_ancestor(repo: Path, maybe_ancestor: str, descendant: str) -> bool: return proc.returncode == 0 -def get_commit_info(repo: Path, sha: str) -> CommitInfo: - fmt = "%H%x00%an <%ae>%x00%s" - proc = git(repo, ["show", "-s", f"--format={fmt}", sha]) - commit_sha, author, subject = proc.stdout.rstrip("\n").split("\x00", 2) +COMMIT_INFO_FORMAT = "%H%x00%an <%ae>%x00%s" + + +def parse_commit_info(line: str) -> CommitInfo: + commit_sha, author, subject = line.split("\x00", 2) return CommitInfo(sha=commit_sha, author=author, subject=subject) +def get_commit_info(repo: Path, sha: str) -> CommitInfo: + proc = git(repo, ["show", "-s", f"--format={COMMIT_INFO_FORMAT}", sha]) + return parse_commit_info(proc.stdout.rstrip("\n")) + + +def is_merge_commit(repo: Path, sha: str) -> bool: + parents = git(repo, ["show", "-s", "--format=%P", sha]).stdout.split() + return len(parents) > 1 + + +def commits_brought_in_by_merge(repo: Path, merge_sha: str, paths: list[str]) -> list[CommitInfo]: + proc = git( + repo, + [ + "log", + "--no-merges", + f"--format={COMMIT_INFO_FORMAT}", + f"{merge_sha}^1..{merge_sha}", + "--", + *paths, + ], + ) + return [parse_commit_info(line) for line in proc.stdout.splitlines() if line] + + +def merge_tree_conflicted_files(repo: Path, ours: str, theirs: str) -> list[str]: + """ + Merge in memory with `git merge-tree --write-tree` (git 2.38+), without a + worktree or checkout, and return the conflicted paths. + """ + proc = git( + repo, + ["merge-tree", "--write-tree", "--name-only", "--no-messages", "-z", ours, theirs], + check=False, + ) + # Exit code 0 is a clean merge and 1 is a merge with conflicts. + if proc.returncode not in (0, 1): + raise RuntimeError( + f"git merge-tree failed for {ours} and {theirs}\n" + f"exit code: {proc.returncode}\n" + f"stderr:\n{proc.stderr or ''}" + ) + + # The output is the merged tree id, then one entry per conflicted path. + return [path for path in proc.stdout.split("\0")[1:] if path] + + def branch_head(repo: Path, branch: str) -> str: return git(repo, ["rev-parse", require_branch_ref(repo, branch)]).stdout.strip() diff --git a/.github/forward-merge-check/checker/merge_engine.py b/.github/forward-merge-check/checker/merge_engine.py index 18d7157..10b0b40 100644 --- a/.github/forward-merge-check/checker/merge_engine.py +++ b/.github/forward-merge-check/checker/merge_engine.py @@ -3,24 +3,35 @@ from .git_ops import ( add_worktree, + commits_brought_in_by_merge, ensure_ref, get_commit_info, get_conflicted_files, git, is_ancestor, + is_merge_commit, + merge_tree_conflicted_files, remove_worktree, require_branch_ref, ) -from .models import CommitInfo, ConflictCommit, MergeResult +from .models import ConflictCommit, ConflictOrigin, MergeResult def source_commits(repo: Path, target_ref: str, source_ref: str) -> list[str]: + """ + Commits on the source's own first-parent line that the target does not + have yet, newest first. + + Merge commits stay in the walk because "Merge 10.11 into 11.4" is a step on + 11.4's line. Commits that only arrived through such a merge are left out: + merging one of them on its own into the target uses a different merge base + and skips the merge that may already have adapted it. + """ proc = git( repo, [ "rev-list", - "--reverse", - "--no-merges", + "--first-parent", f"{target_ref}..{source_ref}", ], ) @@ -118,33 +129,82 @@ def try_merge( remove_worktree(repo, worktree) -def find_first_conflicting_commit( +def conflict_start_commits( repo: Path, - source_ref: str, - target: str, - scratch_root: Path, -) -> Optional[CommitInfo]: + target_ref: str, + commits: list[str], + conflicted_files: list[str], +) -> dict[str, Optional[str]]: """ - Approximation. + Map each conflicted file to the source-side commit where it last went from + merging cleanly to conflicting. - Each source-side non-merge commit is tried independently against the target. - Git conflicts are produced by both branches together, so this is not a - perfect root cause. It is a practical answer to "which source-side commit - should I inspect first?" for scheduled chain-health runs too, not only PRs. + Walks the source commits from the tip back towards the target, merging each + one into the target in memory, and stops once every file has merged + cleanly at some point. A file that conflicts at every commit started at the + oldest one, whose parent the target already has. A file that does not + conflict at the tip maps to None. """ - target_ref = require_branch_ref(repo, target) + started_at: dict[str, Optional[str]] = {} + pending = set(conflicted_files) + newer: Optional[str] = None + + for sha in commits: + if not pending: + break + + conflicted = set(merge_tree_conflicted_files(repo, target_ref, sha)) + for path in pending - conflicted: + started_at[path] = newer + pending &= conflicted + newer = sha + + for path in pending: + started_at[path] = newer + + return started_at + - for sha in source_commits(repo, target_ref, source_ref): - ok, _files, _merge_sha, _candidates = try_merge( - repo, - target_ref, - sha, - scratch_root, +def find_conflict_origins( + repo: Path, + target_ref: str, + source_ref: str, + conflicted_files: list[str], + source_is_test_merge: bool = False, +) -> list[ConflictOrigin]: + commits = source_commits(repo, target_ref, source_ref) + started_at = conflict_start_commits(repo, target_ref, commits, conflicted_files) + origins: list[ConflictOrigin] = [] + + # Oldest commit first, then any files that could not be attributed. + for sha in [*reversed(commits), None]: + files = [path for path in conflicted_files if started_at.get(path) == sha] + if not files: + continue + + if sha is None: + origins.append(ConflictOrigin(files=files)) + continue + + via_commits = ( + commits_brought_in_by_merge(repo, sha, files) + if is_merge_commit(repo, sha) + else [] ) - if not ok: - return get_commit_info(repo, sha) + if source_is_test_merge and sha == commits[0]: + origins.append( + ConflictOrigin(files=files, via_commits=via_commits, from_test_merge=True) + ) + else: + origins.append( + ConflictOrigin( + files=files, + commit=get_commit_info(repo, sha), + via_commits=via_commits, + ) + ) - return None + return origins def check_merge( @@ -153,6 +213,7 @@ def check_merge( source_ref: str, target: str, scratch_root: Path, + source_is_test_merge: bool = False, ) -> MergeResult: target_ref = require_branch_ref(repo, target) ensure_ref(repo, target_ref) @@ -185,11 +246,12 @@ def check_merge( message=f"{source_label} merges cleanly into {target}.", ) - first_conflicting_commit = find_first_conflicting_commit( + conflict_origins = find_conflict_origins( repo=repo, + target_ref=target_ref, source_ref=source_ref, - target=target, - scratch_root=scratch_root, + conflicted_files=conflicted_files, + source_is_test_merge=source_is_test_merge, ) return MergeResult( source_label=source_label, @@ -198,7 +260,7 @@ def check_merge( status="conflict", message=f"{source_label} cannot be cleanly merged into {target}.", conflicted_files=conflicted_files, - first_conflicting_commit=first_conflicting_commit, + conflict_origins=conflict_origins, candidate_commits=candidates, ) diff --git a/.github/forward-merge-check/checker/models.py b/.github/forward-merge-check/checker/models.py index 9e3727a..ed105ce 100644 --- a/.github/forward-merge-check/checker/models.py +++ b/.github/forward-merge-check/checker/models.py @@ -18,6 +18,22 @@ class ConflictCommit: subject: str +@dataclass +class ConflictOrigin: + """ + Conflicted files grouped by the source-side commit where they start to + conflict. When that commit is a merge, via_commits lists the non-merge + commits it brought in that touch the files. + """ + + files: list[str] + commit: Optional[CommitInfo] = None + via_commits: list[CommitInfo] = field(default_factory=list) + # The files start to conflict at the checker's own test merge of earlier + # branches. That commit only exists locally, so it is not reported. + from_test_merge: bool = False + + @dataclass class MergeResult: source_label: str @@ -26,7 +42,7 @@ class MergeResult: status: str message: str conflicted_files: list[str] = field(default_factory=list) - first_conflicting_commit: Optional[CommitInfo] = None + conflict_origins: list[ConflictOrigin] = field(default_factory=list) candidate_commits: list[ConflictCommit] = field(default_factory=list) diff --git a/.github/forward-merge-check/checker/modes.py b/.github/forward-merge-check/checker/modes.py index 0ec8290..658edbb 100644 --- a/.github/forward-merge-check/checker/modes.py +++ b/.github/forward-merge-check/checker/modes.py @@ -7,11 +7,12 @@ from .output import print_result -def next_source_after_result(repo: Path, result: MergeResult) -> tuple[str, str]: +def next_source_after_result(repo: Path, result: MergeResult) -> tuple[str, str, bool]: + """Return the next source label, ref and whether that ref is a local test merge.""" if result.status == "merge_ok": - return f"test merge through {result.target}", result.source_ref + return f"test merge through {result.target}", result.source_ref, True - return result.target, require_branch_ref(repo, result.target) + return result.target, require_branch_ref(repo, result.target), False def first_downstream_broken_target_index( @@ -73,16 +74,19 @@ def run_pr_mode( source_label = "PR head" source_ref = pr_ref + source_is_test_merge = False for target in targets: - result = check_merge(repo, source_label, source_ref, target, scratch_root) + result = check_merge( + repo, source_label, source_ref, target, scratch_root, source_is_test_merge + ) results.append(result) print_result(result) if result.status == "conflict": break - source_label, source_ref = next_source_after_result(repo, result) + source_label, source_ref, source_is_test_merge = next_source_after_result(repo, result) return results @@ -91,12 +95,15 @@ def run_chain_health_mode(repo: Path, branches: list[str], base_branch: str, scr results: list[MergeResult] = [] source_label = base_branch source_ref = require_branch_ref(repo, base_branch) + source_is_test_merge = False for target in branch_targets_after(branches, base_branch): - result = check_merge(repo, source_label, source_ref, target, scratch_root) + result = check_merge( + repo, source_label, source_ref, target, scratch_root, source_is_test_merge + ) results.append(result) print_result(result) - source_label, source_ref = next_source_after_result(repo, result) + source_label, source_ref, source_is_test_merge = next_source_after_result(repo, result) return results diff --git a/.github/forward-merge-check/checker/notification_text.py b/.github/forward-merge-check/checker/notification_text.py index c1172a2..ae1c967 100644 --- a/.github/forward-merge-check/checker/notification_text.py +++ b/.github/forward-merge-check/checker/notification_text.py @@ -3,6 +3,7 @@ from .models import NotificationReason +MAX_VIA_COMMITS = 5 def github_repository_label() -> str: return ( @@ -139,22 +140,51 @@ def format_notification(state: dict, reasons: list[NotificationReason], style: s for index, blocked in blocked_results: lines.append("") lines.append(f"{index}. *Edge:* `{blocked['source_label']}` -> `{blocked['target']}`") - commit = blocked.get("first_conflicting_commit") - if commit: - short_sha = commit["sha"][:12] - commit_label = format_link(github_commit_url(commit["sha"]), short_sha, style) - lines.append( - "*First likely source-side commit:* " - f"{commit_label} - {commit['subject']}" - ) - lines.append(f"*Author:* {commit['author']}") - else: - lines.append("*First likely source-side commit:* not identified") - if blocked["conflicted_files"]: - lines.append(f"*Conflicted files ({len(blocked['conflicted_files'])}):*") - lines.append("```") - lines.extend(f"- {path}" for path in blocked["conflicted_files"]) - lines.append("```") + if not blocked["conflicted_files"]: + continue + + lines.append(f"*Conflicted files ({len(blocked['conflicted_files'])}):*") + origins = blocked.get("conflict_origins") or [] + if not origins: + lines.extend(format_file_block(blocked["conflicted_files"])) + for origin in origins: + lines.extend(format_conflict_origin(origin, style)) return "\n".join(lines) + + +def format_commit(commit: dict, style: str) -> str: + label = format_link(github_commit_url(commit["sha"]), commit["sha"][:12], style) + return f"{label} - {commit['subject']}" + + +def format_file_block(files: list[str]) -> list[str]: + return ["```", *(f"- {path}" for path in files), "```"] + + +def format_conflict_origin(origin: dict, style: str) -> list[str]: + commit = origin.get("commit") + via_commits = origin.get("via_commits") or [] + + if origin.get("from_test_merge"): + lines = ["*Started conflicting at:* unmerged changes from earlier branches"] + elif commit: + lines = [f"*Started conflicting at:* {format_commit(commit, style)}"] + # A merge that brought the change in did not author it. + author_label = "Merged by" if via_commits else "Author" + lines.append(f"*{author_label}:* {commit['author']}") + else: + lines = ["*Started conflicting at:* not identified"] + + if via_commits: + lines.append("*Changes brought in that touch these files:*") + lines.extend( + f"- {format_commit(via, style)} ({via['author']})" + for via in via_commits[:MAX_VIA_COMMITS] + ) + if len(via_commits) > MAX_VIA_COMMITS: + lines.append(f"- and {len(via_commits) - MAX_VIA_COMMITS} more") + + lines.extend(format_file_block(origin["files"])) + return lines diff --git a/.github/forward-merge-check/checker/output.py b/.github/forward-merge-check/checker/output.py index e4ca08d..17c8734 100644 --- a/.github/forward-merge-check/checker/output.py +++ b/.github/forward-merge-check/checker/output.py @@ -49,13 +49,23 @@ def print_result(result: MergeResult) -> None: for path in result.conflicted_files: print(f" - {path}") - if result.first_conflicting_commit: - c = result.first_conflicting_commit + if result.conflict_origins: print() - print("First likely source-side commit that fails this target merge:") - print(f" - {c.sha}") - print(f" Author: {c.author}") - print(f" Subject: {c.subject}") + print("Source-side commits where the conflicted files start to conflict:") + for origin in result.conflict_origins: + if origin.from_test_merge: + print(" - unmerged changes from earlier branches") + elif origin.commit: + c = origin.commit + print(f" - {c.sha}") + print(f" Author: {c.author}") + print(f" Subject: {c.subject}") + else: + print(" - not identified") + for c in origin.via_commits: + print(f" Via: {c.sha[:12]} {c.subject} ({c.author})") + for path in origin.files: + print(f" File: {path}") if result.candidate_commits: print() diff --git a/.github/forward-merge-check/checker/state.py b/.github/forward-merge-check/checker/state.py index 92ca87e..1585923 100644 --- a/.github/forward-merge-check/checker/state.py +++ b/.github/forward-merge-check/checker/state.py @@ -5,7 +5,7 @@ from typing import Optional from .git_ops import branch_head -from .models import CommitInfo, ConflictCommit, MergeResult, NotificationReason +from .models import CommitInfo, ConflictCommit, ConflictOrigin, MergeResult, NotificationReason def commit_to_dict(commit: Optional[CommitInfo]) -> Optional[dict]: @@ -18,6 +18,15 @@ def commit_to_dict(commit: Optional[CommitInfo]) -> Optional[dict]: } +def conflict_origin_to_dict(origin: ConflictOrigin) -> dict: + return { + "files": origin.files, + "commit": commit_to_dict(origin.commit), + "via_commits": [commit_to_dict(commit) for commit in origin.via_commits], + "from_test_merge": origin.from_test_merge, + } + + def conflict_commit_to_dict(commit: ConflictCommit) -> dict: return { "side": commit.side, @@ -35,7 +44,9 @@ def result_to_dict(result: MergeResult) -> dict: "status": result.status, "message": result.message, "conflicted_files": result.conflicted_files, - "first_conflicting_commit": commit_to_dict(result.first_conflicting_commit), + "conflict_origins": [ + conflict_origin_to_dict(origin) for origin in result.conflict_origins + ], "candidate_commits": [ conflict_commit_to_dict(commit) for commit in result.candidate_commits ], @@ -63,9 +74,18 @@ def current_state(repo: Path, branches: list[str], base_branch: str, results: li "status": status, "blocked_edge": None if blocked is None else [blocked.source_label, blocked.target], "conflicted_files": [] if blocked is None else blocked.conflicted_files, - "first_conflicting_commit": None - if blocked is None or blocked.first_conflicting_commit is None - else blocked.first_conflicting_commit.sha, + # Via commits are left out: for a test merge they grow with every new + # commit on the earlier branches, which is not a health change. + "conflict_origins": [] + if blocked is None + else [ + { + "files": origin.files, + "commit": None if origin.commit is None else origin.commit.sha, + "from_test_merge": origin.from_test_merge, + } + for origin in blocked.conflict_origins + ], "results": [ { "target": result.target, @@ -147,12 +167,20 @@ def compact_state(state: dict) -> dict: "target": result["target"], "status": result["status"], "conflicted_files": result["conflicted_files"], - "first_conflicting_commit": None - if result["first_conflicting_commit"] is None - else { - "sha": result["first_conflicting_commit"]["sha"], - "subject": result["first_conflicting_commit"]["subject"], - }, + "conflict_origins": [ + { + "files": origin["files"], + "commit": None + if origin["commit"] is None + else { + "sha": origin["commit"]["sha"], + "subject": origin["commit"]["subject"], + }, + "via_commits": [commit["sha"] for commit in origin["via_commits"]], + "from_test_merge": origin["from_test_merge"], + } + for origin in result["conflict_origins"] + ], } for result in state["results"] ], diff --git a/.github/forward-merge-check/tests/test_forward_mergeability.py b/.github/forward-merge-check/tests/test_forward_mergeability.py index 548c345..831bbca 100644 --- a/.github/forward-merge-check/tests/test_forward_mergeability.py +++ b/.github/forward-merge-check/tests/test_forward_mergeability.py @@ -81,11 +81,17 @@ def write(self, relative: str, content: str) -> None: target.parent.mkdir(parents=True, exist_ok=True) target.write_text(content, encoding="utf-8") - def commit(self, message: str) -> str: + def commit(self, message: str, author: str | None = None) -> str: git(self.path, "add", ".") - git(self.path, "commit", "-m", message) + args = ["commit", "-m", message] + if author: + args.append(f"--author={author}") + git(self.path, *args) return git(self.path, "rev-parse", "HEAD") + def start_merge(self, branch: str) -> None: + git(self.path, "merge", "--no-ff", "--no-commit", branch) + def checkout(self, branch: str, start: str | None = None) -> None: args = ["checkout", "-B", branch] if start: @@ -144,6 +150,74 @@ def make_conflicting_chain(root: Path) -> tuple[GitRepo, str]: return repo, old_sha +def make_merged_from_older_branch_chain(root: Path) -> tuple[GitRepo, dict[str, str]]: + """ + Shaped like 10.11 -> 11.4 -> 11.8, with 10.11 already merged into 11.4: + + - old adapted.txt: conflicts with next on its own, but the merge into mid + dropped it, so it is not a real mid -> next conflict. + - old rpl.txt: reaches next only through the merge into mid and conflicts. + - mid harmless: touches shared.txt far from next's change. + - mid conflict: touches the same shared.txt line as next. + """ + repo = GitRepo(root / "repo") + repo.write("shared.txt", "1\n2\n3\n4\n5\n") + repo.write("adapted.txt", "base\n") + repo.write("rpl.txt", "base\n") + repo.commit("base") + + repo.checkout("next", "master") + repo.write("shared.txt", "next\n2\n3\n4\n5\n") + repo.write("adapted.txt", "next\n") + repo.write("rpl.txt", "next\n") + repo.commit("next changes") + + shas = {} + repo.checkout("old", "master") + repo.write("adapted.txt", "old\n") + shas["old_adapted"] = repo.commit("old adapted fix", "Adapted Author ") + repo.write("rpl.txt", "old\n") + shas["old_rpl"] = repo.commit("old rpl fix", "Rpl Author ") + + repo.checkout("mid", "master") + repo.write("shared.txt", "1\n2\n3\n4\nharmless\n") + shas["mid_harmless"] = repo.commit("mid harmless", "Harmless Author ") + repo.write("shared.txt", "mid\n2\n3\n4\nharmless\n") + shas["mid_conflict"] = repo.commit("mid conflict", "Conflict Author ") + repo.start_merge("old") + repo.write("adapted.txt", "base\n") + shas["merge"] = repo.commit("Merge old into mid", "Merger ") + + for branch in ["old", "mid", "next"]: + repo.expose_origin_ref(branch) + + return repo, shas + + +def make_test_merge_conflict_chain(root: Path) -> tuple[GitRepo, str]: + repo = GitRepo(root / "repo") + repo.write("file.txt", "base\n") + repo.write("mid.txt", "base\n") + repo.commit("base") + + repo.checkout("old", "master") + repo.write("file.txt", "old\n") + old_sha = repo.commit("old changes line", "Old Author ") + + repo.checkout("mid", "master") + repo.write("mid.txt", "mid\n") + repo.commit("mid changes other file") + + repo.checkout("next", "master") + repo.write("file.txt", "next\n") + repo.commit("next changes line") + + for branch in ["old", "mid", "next"]: + repo.expose_origin_ref(branch) + + return repo, old_sha + + def make_downstream_broken_chain(root: Path) -> GitRepo: repo = GitRepo(root / "repo") repo.write("old-only.txt", "base\n") @@ -276,8 +350,12 @@ def test_chain_health_conflict_reports_likely_commit_and_files(self): self.assertEqual(len(results), 2) self.assertEqual(results[0].status, "conflict") self.assertEqual(results[0].conflicted_files, ["file.txt"]) - self.assertIsNotNone(results[0].first_conflicting_commit) - self.assertEqual(results[0].first_conflicting_commit.sha, old_sha) + self.assertEqual(len(results[0].conflict_origins), 1) + origin = results[0].conflict_origins[0] + self.assertEqual(origin.files, ["file.txt"]) + self.assertEqual(origin.commit.sha, old_sha) + self.assertEqual(origin.via_commits, []) + self.assertFalse(origin.from_test_merge) candidate_subjects = { (commit.side, commit.subject) for commit in results[0].candidate_commits } @@ -287,6 +365,76 @@ def test_chain_health_conflict_reports_likely_commit_and_files(self): self.assertEqual(results[1].target, "main") self.assertEqual(results[1].status, "nothing_to_merge") + def test_conflict_origins_follow_first_parent_line_through_merges(self): + with tempfile.TemporaryDirectory() as tmp: + repo, shas = make_merged_from_older_branch_chain(Path(tmp)) + scratch = Path(tmp) / "scratch" + scratch.mkdir() + + # Merged into next on its own, the adapted old commit does conflict. + adapted_alone = git_ops.merge_tree_conflicted_files( + repo.path, "next", shas["old_adapted"] + ) + results = quietly( + check.run_chain_health_mode, + repo.path, + ["old", "mid", "next"], + "old", + scratch, + ) + + self.assertEqual(adapted_alone, ["adapted.txt"]) + self.assertEqual( + [result.status for result in results], + ["nothing_to_merge", "conflict"], + ) + result = results[1] + self.assertEqual(result.conflicted_files, ["rpl.txt", "shared.txt"]) + + origins = [ + ( + origin.files, + origin.commit.sha, + [commit.sha for commit in origin.via_commits], + ) + for origin in result.conflict_origins + ] + self.assertEqual( + origins, + [ + (["shared.txt"], shas["mid_conflict"], []), + (["rpl.txt"], shas["merge"], [shas["old_rpl"]]), + ], + ) + + def test_conflict_origin_in_test_merge_lists_earlier_branch_commits(self): + with tempfile.TemporaryDirectory() as tmp: + repo, old_sha = make_test_merge_conflict_chain(Path(tmp)) + scratch = Path(tmp) / "scratch" + scratch.mkdir() + + results = quietly( + check.run_chain_health_mode, + repo.path, + ["old", "mid", "next"], + "old", + scratch, + ) + state = check.current_state(repo.path, ["old", "mid", "next"], "old", results) + + self.assertEqual([result.status for result in results], ["merge_ok", "conflict"]) + self.assertEqual(results[1].source_label, "test merge through mid") + self.assertEqual(len(results[1].conflict_origins), 1) + origin = results[1].conflict_origins[0] + self.assertEqual(origin.files, ["file.txt"]) + self.assertTrue(origin.from_test_merge) + self.assertIsNone(origin.commit) + self.assertEqual([commit.sha for commit in origin.via_commits], [old_sha]) + self.assertNotIn( + results[0].source_ref, + json.dumps(state["results"][1]["conflict_origins"]), + ) + def test_pr_mode_stops_before_preexisting_downstream_break(self): with tempfile.TemporaryDirectory() as tmp: repo = make_downstream_broken_chain(Path(tmp)) @@ -421,11 +569,18 @@ def test_write_outputs_writes_compact_state_notification_and_github_outputs(self "status": "conflict", "message": "blocked", "conflicted_files": ["file.txt"], - "first_conflicting_commit": { - "sha": "abc", - "author": "Tester ", - "subject": "break it", - }, + "conflict_origins": [ + { + "files": ["file.txt"], + "commit": { + "sha": "abc", + "author": "Tester ", + "subject": "break it", + }, + "via_commits": [], + "from_test_merge": False, + } + ], "candidate_commits": [ { "side": "source", @@ -447,12 +602,38 @@ def test_write_outputs_writes_compact_state_notification_and_github_outputs(self "target": "main", "status": "conflict", "message": "blocked again", - "conflicted_files": ["other.txt", "src/deep/path.cc"], - "first_conflicting_commit": { - "sha": "def456789012", - "author": "Second Tester ", - "subject": "break it again", - }, + "conflicted_files": ["other.txt", "src/deep/path.cc", "unmerged.txt"], + "conflict_origins": [ + { + "files": ["other.txt", "src/deep/path.cc"], + "commit": { + "sha": "def456789012", + "author": "Second Tester ", + "subject": "Merge old into next", + }, + "via_commits": [ + { + "sha": "fed987654321", + "author": "Via Tester ", + "subject": "break it again", + } + ], + "from_test_merge": False, + }, + { + "files": ["unmerged.txt"], + "commit": None, + "via_commits": [ + { + "sha": f"{index:012d}", + "author": "Old Tester ", + "subject": f"old change {index}", + } + for index in range(7) + ], + "from_test_merge": True, + }, + ], "candidate_commits": [], } ], @@ -499,16 +680,34 @@ def test_write_outputs_writes_compact_state_notification_and_github_outputs(self "[def456789012](https://github.example/owner/repo/commit/def456789012)", zulip_text, ) - self.assertIn("*Conflicted files (1):*\n```\n- file.txt\n```", slack_text) + self.assertIn( + "*Conflicted files (1):*\n" + "*Started conflicting at:* - break it\n" + "*Author:* Tester \n" + "```\n- file.txt\n```", + slack_text, + ) self.assertNotIn("*Candidate commits touching conflicted files:*", slack_text) self.assertNotIn("abc111222333", slack_text) self.assertNotIn("def111222333", slack_text) self.assertNotIn("abc111222333", zulip_text) self.assertNotIn("def111222333", zulip_text) self.assertIn( - "*Conflicted files (2):*\n```\n- other.txt\n- src/deep/path.cc\n```", + "*Conflicted files (3):*\n" + "*Started conflicting at:* " + " - Merge old into next\n" + "*Merged by:* Second Tester \n" + "*Changes brought in that touch these files:*\n" + "- - break it again " + "(Via Tester )\n" + "```\n- other.txt\n- src/deep/path.cc\n```\n" + "*Started conflicting at:* unmerged changes from earlier branches\n" + "*Changes brought in that touch these files:*\n", slack_text, ) + self.assertIn("old change 4", slack_text) + self.assertNotIn("old change 5", slack_text) + self.assertIn("- and 2 more\n```\n- unmerged.txt\n```", slack_text) self.assertNotIn("Base branch:", slack_text) self.assertNotIn("Health fingerprint:", slack_text) self.assertNotIn("Chain fingerprint:", slack_text) @@ -540,11 +739,18 @@ def test_write_outputs_uses_target_repository_override_for_notification_links(se "status": "conflict", "message": "blocked", "conflicted_files": ["file.txt"], - "first_conflicting_commit": { - "sha": "abc123", - "author": "Tester ", - "subject": "break it", - }, + "conflict_origins": [ + { + "files": ["file.txt"], + "commit": { + "sha": "abc123", + "author": "Tester ", + "subject": "break it", + }, + "via_commits": [], + "from_test_merge": False, + } + ], "candidate_commits": [], } ],