[bong-water-water-bong]

tools/copyright.py --fix deletes blank lines from the files it repairs. I found it while reviewing #2183: build there is red on --check for 13 two-liner headers, and the obvious fix, --fix, changed those files by +143 −68 when it should have only added lines.

Cause

_CANON_LINES is built from every line of NOTICE. The licence block's blank comment lines (#, //) reduce to "", so the set contained the empty string, and canonical_notice_line() returned true for every blank or bare-comment line.

• add_notice() drops every line in the 20-line header window that it treats as a notice line. That removed the separator after a file's description, the blank lines between #include groups, and the one after namespace {.
• repair_notice()'s doubled-block dedupe keeps the first copy of each canonical line. With "" in the set, it dropped every blank line after the first in the whole file, past the header window too: 0 of 36 kept in the test case below.

Fix

A line with no words in it cannot be a claim. The set and the predicate now exclude the empty key (+8 −2 in tools/copyright.py, most of it the comment saying why).

Measured

	main	this PR
--fix on #2183's 13 files at 7297f03	+143 −68, then --check rc 0	+156 −0 (12 licence lines each), then --check rc 0
new case: two-liner header, add path	FAIL, lost 5 blank lines	ok
new case: near-miss holder, repair path	FAIL, lost 7 blank lines	ok
tests/copyright_check_untracked_test.py	2 failures of 19	0 failures of 19
tests/copyright_gate_test.sh	PASS	PASS

Both new cases compare the written file with the exact expected text, so a single lost line fails them.

Overlap

#2199 also edits tools/copyright.py and the same test file. It adds the short-header detection and message (short_notice, strip_short_header), not _CANON_LINES or canonical_notice_line(). The hunks are disjoint, but whichever lands second will need a trivial rebase of the test file's tail.

Refs #2183 (review at 7297f03).

🤖 Generated with Claude Code

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Blank Line Handling Bug The fix addresses a bug where blank comment lines in the licence block were being treated as canonical notice lines, causing --fix to incorrectly delete blank lines from files. This occurred because the _CANON_LINES set included the empty string, which resulted in canonical_notice_line() returning true for all blank or bare-comment lines. The fix ensures only lines with actual content are considered canonical. # The licence block's own blank comment lines (`#`, `//`) reduce to "", so a set built from every NOTICE line held the # empty string, and canonical_notice_line() said yes to every blank or bare-comment line in a file. add_notice() then # deleted each one in the header window (the separator after the description, the blank lines between #include groups # and after `namespace {`), and repair_notice()'s doubled-block dedupe deleted every blank line after the first in the # whole file. Only lines with words in them can be a claim, so only those are canonical. _CANON_LINES = {k for k in (re.sub(r"^\W+", "", n).strip() for n in NOTICE) if k} def canonical_notice_line(line): """True when the line IS one of the canonical notice lines, spelled exactly. engine#2129, met while fixing a new file's header: an SPDX two-liner without the licence block is "missing" to has_notice(), so --fix prepends the block -- and the two-liner it just merged into that block stayed behind, so the file --fix had just written failed --check with "duplicated inside the header window". near_miss_holder() cannot cover it by construction: it returns False for an exact holder, because an exact holder next to the canonical block is the *repaired* state. The block already contains both lines, so a copy inside the window is not a second claim. """ key = re.sub(r"^\W+", "", line).strip() return bool(key) and key in _CANON_LINES

[bong-water-water-bong]

VERDICT: READY at f08a74c — and I have to own the cause: this defect was introduced by the fix I READY'd in #2192.

The chain, exactly

# #2192 (reviewed READY by me):
_CANON_LINES = {re.sub(r"^\W+", "", n).strip() for n in NOTICE}
canonical_notice_line = lambda line: re.sub(r"^\W+", "", line).strip() in _CANON_LINES
-    kept = [l for l in head if not near_miss_holder(l)]
+    kept = [l for l in head if not near_miss_holder(l) and not canonical_notice_line(l)]

NOTICE contains blank separator lines (between the SPDX line and the licence paragraph, between paragraphs). Each reduces to "", so "" ∈ _CANON_LINES and canonical_notice_line("") is True for every blank line. The filter then drops every blank line in the header window, and repair_notice()'s doubled-block dedupe drops every blank line after the first in the whole file — the +143 −68 measured here.

I reviewed that diff, called the reasoning "correct rather than convenient", and did not ask the one question the code begs: what is in _CANON_LINES besides the lines with words in them? A set built from a multi-line string with blank lines in it has "" in it, and a predicate that says "this line is the canonical notice" then says yes to nothing at all.

The fix states the right principle

"Only lines with words in them can be a claim, so only those are canonical."

That is the invariant, and it is the reason this is a fix rather than a patch. Two asks:

1. make it unrepresentable — build the set with if n.strip(), or assert "" not in _CANON_LINES next to it. The comment explains the hazard; the code should make the hazard impossible, because the next person to extend NOTICE will not read a comment;
2. an arm for this defect: tests/copyright_check_untracked_test.py covers a different bug — "--check must see a file that is not staged yet" — so nothing in this PR fails if blank lines start being deleted again. A file with a # separator inside its header, --fix, then assert the separator survives, is the arm that pins the regression I caused.

And the diffstat trap, which I nearly fell into twice

git diff --stat origin/main...HEAD showed 16 files, +743/−40 — most of it main's own movement (tools/hrx_f16_narrowing_check.py is already on main). Measured against the merge base, this PR is 2 files, +47/−2. Same trap as #2035 (+23,692 that was main's commits); the only way to read a diffstat is against the merge base.