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": [], } ],