From 43620453c7c240bd7d2390229edd38bbdc4324bc Mon Sep 17 00:00:00 2001 From: argszero Date: Sat, 24 Oct 2026 06:25:52 +0800 Subject: [PATCH] emrg: the landing-diff warning travels with the change it warns about --- scripts/check-merge-landing-diff.py | 20 +++++++-- tests/test_check_merge_landing_diff.py | 62 +++++++++++++++++++++++++- 2 files changed, 78 insertions(+), 4 deletions(-) diff --git a/scripts/check-merge-landing-diff.py b/scripts/check-merge-landing-diff.py index e1ca8f2e..ad74d33c 100755 --- a/scripts/check-merge-landing-diff.py +++ b/scripts/check-merge-landing-diff.py @@ -818,6 +818,16 @@ def check_pr( f"deletions this PR does not make:" ) lines += [f" {status}\t{path}" for status, path in reversed_inside] + # The pointer and the change it points at are in one stream, in that order + # (#2560). They used to be split across stdout and stderr, which makes their + # relative position a property of buffering rather than of program order: a + # reader combining the streams (`2>&1`, the way a cycle reads a tool) could get + # "the landing change is printed above it" *before* the landing change. + lines.append( + " Read the landing change above, not diff(base, head): this is a reading " + "hazard, not a defect in the PR - the change above is what merges, and the " + "diff is that change plus the base's own later commits." + ) return "backwards", "\n".join(lines) lines.append( f" diff(base, head) lists {len(apparent)} path(s), and every one of them is " @@ -886,13 +896,17 @@ def main(argv: list[str] | None = None) -> int: ) return 3 if backwards: + # On stdout, like the clean verdict two lines below and unlike stderr, which is + # for what could not be measured (#2560). The two verdicts of one tool belong in + # one stream: split across two, the hazard summary is flushed first whenever the + # streams are combined, so the reader met the warning before the change it is + # about. A position is only meaningful inside a stream. print( "\n" + ", ".join(f"#{number}" for number in backwards) + " reads backwards: reading diff(master, head) as the change this PR lands " - "would review work the PR never touched. The landing change is printed above " - "it and is what merges. This is a reading hazard, not a defect in the PR.", - file=sys.stderr, + "would review work the PR never touched. Each PR's own block names the change " + "that merges. This is a reading hazard, not a defect in the PR." ) return 1 print("\nno path reads backwards: diff(master, head) is the change this lands") diff --git a/tests/test_check_merge_landing_diff.py b/tests/test_check_merge_landing_diff.py index 7dc57178..791beb56 100644 --- a/tests/test_check_merge_landing_diff.py +++ b/tests/test_check_merge_landing_diff.py @@ -450,7 +450,11 @@ def test_main_exit_codes(mod, tmp_path, monkeypatch, capsys) -> None: monkeypatch.chdir(repo) monkeypatch.setattr(mod, "_fetch_head", lambda number: head) assert mod.main(["1", "--base", base]) == 1 - assert "reads backwards" in capsys.readouterr().err + # `out`, not `err`: the hazard verdict is a verdict, and both of this tool's + # verdicts travel on stdout now (#2560). Split across two streams the hazard half + # was flushed first whenever they were combined, so its reader met the warning + # before the change it warned about. `err` keeps what could not be measured. + assert "reads backwards" in capsys.readouterr().out repo2, base2, head2 = _fresh_repo(tmp_path) monkeypatch.chdir(repo2) @@ -1551,3 +1555,59 @@ def test_the_paths_that_read_backwards_carry_no_counts( change, _, backwards = report.partition(" reads backwards:") assert "(+" in change assert "(+" not in backwards + + +# --- where the hazard warning sends its reader ---------------------------------- + + +def test_the_hazard_explanation_is_printed_after_the_change_it_warns_about( + mod, tmp_path, monkeypatch, capsys +) -> None: + """The warning must arrive in the same stream as the block, and after it (#2560). + + The warning tells its reader that the landing change "is printed above it". That + is a claim about position, and position across two streams is decided by + buffering rather than by program order: the landing change goes to stdout, the + warning went to stderr, and a reader who combines them (`2>&1`, which is how a + cycle reads a tool) can get the warning first. Measured on master `5c6b832d`: + `check-merge-landing-diff.py 2558 2>&1 | head` printed "reads backwards: … The + landing change is printed above it" and then the landing block below it. + + So the assertion is not "the pair reads well" but "the explanation is in + *stdout*, after the landing block" — a property of one string, which no + buffering can invert. Against the tool as it stood this fails, because the + explanation was only ever on stderr. + """ + repo, base, head = _behind_repo(tmp_path) + monkeypatch.chdir(repo) + monkeypatch.setattr(mod, "_fetch_head", lambda number: head) + + assert mod.main(["1", "--base", base]) == 1 + out, err = capsys.readouterr() + + landing = out.find("landing tree") + assert landing != -1, f"no landing block in stdout: {out!r}" + explanation = out.find("reading hazard") + assert explanation != -1, ( + "the explanation a reader needs is not in stdout, so its position relative to " + f"the landing change is decided by which stream flushes first: {out!r}" + ) + assert landing < explanation, ( + "the warning is printed before the change it points at, which is what makes " + f"'printed above it' false: {out!r}" + ) + assert "landing change above" in out, ( + "the pointer has to name the direction the reader can see" + ) + # Both verdicts travel on one stream, so neither can be flushed ahead of the + # reports it summarises, and stderr keeps only what could not be measured. + summary = out.find("nothing the PR") + assert summary != -1 or out.rfind("reads backwards") > explanation, ( + f"the cross-PR summary is missing from stdout: {out!r}" + ) + assert "printed above" not in out, ( + f"a positional claim about the other stream survives: {out!r}" + ) + assert err.strip() == "", ( + f"stderr carried a verdict, so its position is again decided by buffering: {err!r}" + )