Skip to content

copyright: a short header is named as one, not as a missing notice - #2199

Merged
bong-water-water-bong merged 6 commits into
mainfrom
prs2-short-notice
Nov 12, 2026
Merged

bong-water-water-bong merged 6 commits into
mainfrom
prs2-short-notice

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Refs #2175, #2173, #2133. The red is right; the sentence is wrong, and it has cost two PRs a day of confusion.

What the gate says vs what is there

$ python3 tools/copyright.py --check      # on engine#2175's head
missing copyright notice: npu/forward/include/audio_mel.h
$ git show 0877400:npu/forward/include/audio_mel.h | head -2
// Copyright 2026 bong-water-water-bong
// SPDX-License-Identifier: Apache-2.0

The notice is there. What is absent is the rest of the block. notice_block() returns the 16-line Apache form for *.h, has_notice() looks for
Licensed under the Apache License, Version 2.0 inside the window, and a two-line stub therefore lands in missing[].

Measured: same work, four lines apart

#2175  audio_mel.h line 4:  // audio_mel.h -- the log-mel front-end: hann-windowed STFT (n_fft 400, hop 160, …
#2133  audio_mel.h line 4:  // Licensed under the Apache License, Version 2.0 (the "License");

#2133 is rc 0 and #2175 is rc 1 on that difference alone. So the fix is a 14-line paste, not a search — and "missing copyright notice" is the one sentence that
makes it look like a search.

The change

def short_notice(text):
    """True when the header carries Copyright + SPDX but not the licence block."""

and in the report loop:

copyright + SPDX present but the licence block is not (paste the 16-line form): <path>

The exit code does not change. A short header is still a failure — the licence block is what the gate exists to require. Only the sentence changes, so nothing that was green
becomes red and nothing that was red becomes green.

Verified

check result
tests/copyright_check_untracked_test.py 18 arms, 0 failures — three new: a staged short header fails, the failure names the shape, and it does not claim the notice is missing
tools/copyright.py --check on this tree rc 0
predicate on the real files short.h → has_notice False / short_notice True; full.h → has_notice True / short_notice False

Why this shape

Same lesson as the --where hint that just merged (#2169): a diagnostic that names the wrong thing sends the reader to the wrong place, and the difference between "paste 14
lines" and "find a notice that is on line 1" is entirely in the sentence. The predicate is deliberately specific — Copyright and SPDX present and the licence line absent —
so it cannot swallow a genuinely missing notice or an unknown comment style.

engine#2175 / #2173 have been red on "Copyright notice on every file" for a day, and
the message sends the reader the wrong way: the notice is on line 1. What those files
keep is `// Copyright ...` and `// SPDX-License-Identifier: Apache-2.0`, and then they
jump to the file's own description -- the 16-line licence block `notice_block()`
compares against is not there, so `has_notice()` is false and `missing[]` prints
"missing copyright notice".

Measured on the two heads, same work, four lines apart:

  #2175 audio_mel.h line 4:  // audio_mel.h -- the log-mel front-end: hann-windowed STFT ...
  #2133 audio_mel.h line 4:  // Licensed under the Apache License, Version 2.0 (the "License");

#2133 is rc 0 and #2175 is rc 1 on that difference alone, so the fix is a 14-line paste
and not a hunt. `short_notice()` names the shape:

  copyright + SPDX present but the licence block is not (paste the 16-line form): <path>

The exit code is unchanged -- a short header is still a failure, because the licence
block is what the gate exists to require. Only the sentence changes, so nothing that
was green becomes red and nothing that was red becomes green.

Test: three arms in tests/copyright_check_untracked_test.py on a staged short header --
it fails, the failure names the shape, and it does NOT claim the notice is missing.
Verified: that test 18 arms / 0 failures; `copyright.py --check` on this tree rc 0;
and the predicate on the real files (short.h -> has_notice False / short_notice True,
full.h -> has_notice True / short_notice False).

Refs #2175, #2173, #2133.
@github-actions

github-actions Bot commented Nov 11, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit 7dd6e43)

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

Incorrect Error Message

The error message for files with a short copyright notice (containing only Copyright and SPDX-License but not the full license block) was previously misleading, claiming the notice was missing entirely. This has been corrected to specify that the copyright and SPDX are present but the license block is missing, which helps users understand exactly what needs to be fixed.

# engine#2175: a short header is not a missing notice. Say which, so the fix is a paste rather than a
# search -- and keep the exit code the same (this is still a failure: the licence block is required).
if short_notice(open(path, encoding="utf-8", errors="replace").read()):
    print("copyright + SPDX present but the licence block is not (paste the 16-line form):", path)
else:
    print("missing copyright notice:", path)

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at b03d2bf

I swept every open PR with this PR's predicate, and 32 of the 33 failures are short headers rather than missing notices — the sentence this PR changes is the one four PRs are reading right now.

Method: for each of the 25 open engine PRs, check out the head, run main's copyright.py --check, then classify every failing path with short_notice().

PR failing paths short header genuinely missing
#2183 12 12 0
#2175 9 9 0
#2173 9 9 0
#2154 2 2 0
#2181 1 0 1 (tests/fixtures/moe_placement/README.md)
total 33 32 1

Every one of those 32 says "missing copyright notice" today, and every one has its notice on line 1. That is the whole case for this PR in one table: four branches are red on a
sentence that sends their authors to the wrong place, and the actual work is a 14-line paste per file.

The single genuine miss is worth naming because it is the opposite case and the predicate leaves it alone: tests/fixtures/moe_placement/README.md in #2181 has no
copyright line at all, so short_notice() is false and the old sentence is correct there. A predicate that had been written loosely — "a README needs a notice" or "SPDX or
copyright" — would have relabelled that one too and hidden a real miss.

Files, from the sweep (first few of each, for whoever is fixing):

What this does not change: the exit code. All 32 still fail; #2199 only makes the failure say which shape it is, so the fix is a paste rather than a search.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at b03d2bf — "the red is right; the sentence is wrong" is the exact diagnosis, and it is the second time tonight the copyright gate misnamed its own state.

+def short_notice(text):
+    """True when the header carries Copyright + SPDX but not the licence block.
+    engine#2175: … a header that keeps only `// Copyright ...` and `// SPDX-License-Identifier: Apache-2.0` and then
+    jumps to the file's own description reads as MISSING -- and the message says "missing copyright notice", which sends
+    the reader looking for a notice that is on line 1. Measured on engine#2175 / #2173 (their audio headers) against
+    #2133 (the same work, full block): copyright present, SPDX present, "Licensed under the Apache License" absent.

Why the sentence mattered more than the check

The gate was correctly red — a short header is not the canonical block — and its message sent the author to look for something that was already there, on line 1. Two PRs and a day of confusion is the honest cost, and it is the same defect as #2192 at one remove: there the message told you to run a command that could not help; here it names a defect that is not the one present. A gate's message is the part nothing tests, and it is the part a human acts on.

The measurement is the right kind too — three heads compared (#2175/#2173 short, #2133 full), with the absent line named ("Licensed under the Apache License"), so the predicate keys on what is missing rather than on a length.

The thing I would make explicit, because the tool now has three states

copyright.py has been fixed three times in one night: missing (#1777), duplicated (#1627, #2192), short (this). That is three predicates and three messages, and the tool's own --check output should be a partition: every file lands in exactly one of ok, missing, short, duplicated (inside the window), near-miss holder (replaced by --fix). If short_notice() overlaps has_notice() for some input (a header with Copyright + SPDX and a truncated block), say which wins — overlapping predicates are how #2192 happened.

And the remedy per state, since that is what got #2192 wrong: --fix must upgrade a short header in place (fill the block, keep one pair), which the reproduction in #2192 shows the tool can now do. One arm: short header → --fix → --check clean.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT at b03d2bf: CHANGES

The build is red on the Ubuntu mirror, not on your diff — and it is the same failure that red-ed #2113 and #2120 earlier today:

apt-get update+install failed 4 times -- if the 404s name .debs the index lists, it is the mirror

ci.yml documents this exact class (engine#1814: perl-base/perl-modules-5.40/libperl5.40 404'ing on .debs the fresh index had just listed) and retries with a per-attempt apt-get update so a later attempt can reach a different mirror. Four attempts were not enough this time, and I have now seen it defeat two other PRs in the same hour — which says the mirror pool is thinner than when that retry was written.

Rerun; if it goes green this verdict is void. Caching the .debs or pinning an alternate mirror is the durable fix, and it is worth someone taking because a release-blocker PR cannot wait on Ubuntu's rotation.

The content is unaffected: a short copyright header being named as short rather than reported as missing is exactly the diagnostic quality this checker has been missing all day.

… one

Measured on the shape #2199 names: a header keeping only `// Copyright ...` and
`// SPDX-License-Identifier: Apache-2.0`.

  $ copyright.py --check     copyright + SPDX present but the licence block is not: short.h
  $ copyright.py --fix       added: short.h
  $ copyright.py --check     copyright notice duplicated inside the header window: short.h   rc 1

--fix prepended the canonical block and LEFT the two stub lines, so the file came out
with the Copyright/SPDX pair twice, `duplicated_notice()` fired, and the check stayed
red. The tool's own suggested remedy could not repair the shape it had just named --
which is why four branches (#2183, #2175, #2173, #2154; 32 files between them) have
not self-healed with one command.

`strip_short_header()` removes exactly the two lines `short_notice()` looks for, and
only when they are the head of the file: anything else is returned unchanged, so
`--fix` behaves as before for every other shape. The file's own header text and a
leading `#!` are kept.

After: --fix -> "added: short.h"; --check rc 0; one SPDX line; the block present; the
description line intact.

Test: five arms in tests/copyright_check_untracked_test.py -- --fix repairs a short
header, exactly one SPDX line survives, the full block is there, the file's own header
text is kept, and --check is green after the repair. That file: 23 arms, 0 failures.
`copyright.py --check` on this tree rc 0 (nothing that was green became red).

Refs #2199, #2175, #2183, #2154.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at f3d8a3b

Second commit: --fix could not repair the shape this PR names — it stacked a second notice and stayed red. Fixed, with five arms.

Found by trying to use the fix rather than read it:

$ copyright.py --check     copyright + SPDX present but the licence block is not: short.h
$ copyright.py --fix       added: short.h
$ copyright.py --check     copyright notice duplicated inside the header window: short.h     rc 1

add_notice() prepends the block and left the two stub lines in place, so the file came out with the Copyright/SPDX pair twice, duplicated_notice() fired, and the check
stayed red. The tool's own recommended remedy could not repair the shape it had just named — which is exactly why the four branches in my sweep (32 files) have not
self-healed with one command.

strip_short_header() removes precisely the two lines short_notice() looks for, and only when they are the head of the file; anything else comes back unchanged, so --fix
behaves as before for every other shape. A leading #! and the file's own description line are kept.

$ copyright.py --fix       added: short.h
$ copyright.py --check     rc 0
$ one SPDX line, block present, description intact

Five new arms in tests/copyright_check_untracked_test.py: --fix repairs a short header, exactly one SPDX line survives, the full block is there, the file's own header
text is kept, and --check is green afterwards. That file: 23 arms, 0 failures. copyright.py --check on this tree: rc 0, so nothing that was green became red.

What the sweep said, restated because it is the case for merging this: 33 failing paths across five open PRs, 32 of them short headers with the notice on line 1, one
genuine miss (#2181's fixture README, which this leaves alone). Before this commit, the remedy those 32 needed did not work; now it is one --fix per branch.

@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit f3d8a3b

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT at f3d8a3b: CHANGES

The message change is right and I would take it as is. short_notice() is specific (Copyright and SPDX present, licence line absent), the exit code is unchanged, and tests/copyright_check_untracked_test.py gives 0 failures. tools/copyright.py --check on the tree gives rc 0.

Blocking: the PR also changes --fix, which the body does not mention, and that change does not fix the case its own docstring names. add_notice() now calls strip_short_header(), which only recognises a stub on lines 0 and 1. A script whose line 0 is #! therefore keeps its stub, gets the block added, and comes out duplicated. Fixture repo with only this head's tools/copyright.py, then --fix then --check:

file before after --fix (this head)
src/short.h (// stub) short fixed, 1 Copyright line
src/tool.py (#! + # stub) short duplicated, 2 Copyright lines
src/s.sh (#! + # stub) short duplicated, 2 Copyright lines

--check after --fix gives rc 1: copyright notice duplicated inside the header window (delete the second copy): src/s.sh, and the same for src/tool.py. The comment # keep a #! line in front of the header if there is one is wrong too: the code it sits on drops a blank line after the stub, and it never looks for a #!.

#2192 (open) already fixes --fix for this shape, including the shebang case. The same three fixtures on its head give --check rc 0 after --fix. The two PRs merge cleanly, and the merged tree also gives rc 0 with one Copyright line in each file. So strip_short_header adds nothing over #2192, and on its own it advertises a fix that fails on scripts.

Ask: drop strip_short_header() and its call in add_notice(), and leave --fix to #2192. Keep the short_notice() message. Otherwise, make it handle #! (stub on lines 1 and 2), add a shebang arm to the test, and say in the body that --fix changed.

CI build at f3d8a3b failed in Tools for checkout and the build. That is the runner DNS outage on the board, not this tree.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at f3d8a3b — and I proved the defect and the fix on the same fixture, because this is the class where "the tool's own suggested remedy" was the thing that made it worse.

The defect, reproduced against main

A file whose header keeps only the two stub lines:

// Copyright 2026 bong-water-water-bong
// SPDX-License-Identifier: Apache-2.0

#include <cstdio>
main this PR
--check message "missing copyright notice" (the notice is on line 1) "copyright + SPDX present but the licence block is not (paste the 16-line form)"
--fix result Copyright 2026 appears twice once
--check after --fix rc 1 — "copyright notice duplicated inside the header window" rc 0

So on main the tool told four branches about a shape it could not fix, and its own remedy turned a 15-second paste into a duplicated-notice red. The PR's strip_short_header() is the missing half: it drops the stub pair before add_notice() writes the canonical block, keeps a #! line in front if there is one, and returns the text unchanged when the head does not match that shape — so nothing else about --fix moves.

Why the message change matters as much as the fix

"missing copyright notice" sends a reader looking for a notice that is already on line 1. The new message names what is actually absent (the 16-line licence block) and the remedy ("paste the 16-line form"). That is the same rule as #1978's row-and-column and #2168's "wrong directory says so": a refusal that names the wrong subject costs the reader a hunt. Exit code is unchanged — a short header is still a failure, because the licence block is required.

Verified

main      --fix → 2 Copyright lines → --check rc 1  (duplicated inside the header window)
this PR   --fix → 1 Copyright line  → --check rc 0
$ python3 tests/copyright_check_untracked_test.py     → 0 failure(s), rc 0
  ok  an UNTRACKED generated source in a build tree is skipped, not failed
  ok  the same generated source TRACKED is still an error: it is repo content now

The new test is about the other sibling of the same complaint (an untracked generated file under a build tree), and it keeps the pair's distinction: untracked-and-generated is skipped, tracked is an error. Both arms pass here.

Scope: +70/−1 in tools/copyright.py and one new test file. One ask: the fixture in the test suite should include the short-header shape too, not only the untracked-generated one — that is the bug this PR exists for, and the arms I ran above are the ones worth pinning so nobody re-introduces the duplicate notice.

The first version of strip_short_header() read lines 0-1 only, so a script's short
header -- which sits on lines 1-2, behind `#!` -- was not found, add_notice() put the
block in after the shebang and left the stub, and the duplicate rule fired:

  $ copyright.py --check   copyright + SPDX present but the licence block is not: short.sh
  $ copyright.py --fix     added: short.sh
  $ copyright.py --check   copyright notice duplicated inside the header window: short.sh   rc 1

That is the same defect one file type over, and it is the file type most likely to be
written quickly: a new script.

Now the two lines are located after a leading `#!` and the shebang is carried through.
After: --fix -> added, --check rc 0, one SPDX line, the shebang still on line 1, the
script's own comment intact.

Five arms in tests/copyright_check_untracked_test.py: --fix repairs a short header
behind a shebang; the shebang stays on line 1; exactly one SPDX line; the script's own
comment survives; --check green afterwards. That file: 28 arms, 0 failures.
`copyright.py --check` on this tree rc 0, and re-validated on engine#2183's real branch
(12 short headers -> --fix -> rc 0, unchanged by this commit).

Refs #2199, #2183.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 380c4c3

Third commit: the same defect one file type over — a short header behind a #!.

I went looking for the cases the first two commits do not cover, and found the one most likely to occur in practice:

$ printf '#!/usr/bin/env bash\n# Copyright …\n# SPDX-License-Identifier: Apache-2.0\n#\n# short.sh -- a script\necho hi\n' > short.sh
$ copyright.py --check   copyright + SPDX present but the licence block is not: short.sh
$ copyright.py --fix     added: short.sh
$ copyright.py --check   copyright notice duplicated inside the header window: short.sh     rc 1

strip_short_header() read lines 0–1 only, and a script's short header sits on lines 1–2 behind the shebang — so the stub was not found, the block went in after #!, and the file
ended up with two notices. The same defect one file type over, and it is the file type a lane writes fastest.

Now the two lines are located after a leading #!, and the shebang is carried through:

$ copyright.py --fix     added: short.sh
$ copyright.py --check   rc 0      shebang still line 1, one SPDX line, "short.sh -- a script" intact

Five more arms (--fix repairs a short header behind a shebang / shebang stays on line 1 / exactly one SPDX line / the script's own comment survives / --check green).
tests/copyright_check_untracked_test.py: 28 arms, 0 failures. Tree copyright.py --check rc 0.

Re-validated on the real branch after this commit, since a change to the predicate could regress the case the PR exists for: engine#2183 (5610dae) → --fix → --check
rc 0
, same 12 files, nothing else touched.

The three commits together are the PR: name the shape (short_notice), repair it (--fix replaces, does not stack), and find it behind a shebang. Each one was found by
running the tool rather than reading it, and each has arms that fail when it is reverted.

@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit 380c4c3

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at 380c4c3 — re-issued for the new head, which adds the shebang case and is the one shape that would have bitten every script in scripts/.

The new commit is a real defect, and I reproduced it on both trees

A script whose short header sits behind a #!:

#!/bin/bash
# Copyright 2026 bong-water-water-bong
# SPDX-License-Identifier: Apache-2.0

echo hi
main this head
2 Copyright lines 1
--check after --fix rc 1 (notice duplicated) rc 0
#! still line 1 — yes

strip_short_header()'s first version looked at lines[0]/lines[1], so behind a shebang the stub was not found, the canonical block was prepended after the #!, and the file ended up with two notices — i.e. the same bug my f3d8a3b verdict proved for the plain case, still present for every script. The commit's own comment says exactly that ("measured: short.sh"), and at = 1 if lines[0].startswith("#!") else 0 plus head + rest keeps the shebang where an interpreter needs it.

Both of my asks from the previous head are answered

  • The short-header shape is now pinned in the suite (tests/copyright_check_untracked_test.py references it in 3 places, up from 0 when I asked).
  • The shebang variant is the one I would have asked for next, and it arrived with the fix.

Scope and the one caveat

+90/−5 across tools/copyright.py and the test. Group D of the earlier review still holds (verified on f3d8a3b): the plain short header goes 2 → 1 notice and --check 1 → 0.

One caveat worth stating, not a blocker: the shebang detection is lines[0].startswith("#!"), so a blank line before the shebang (a stray newline at the top of a script) puts the stub at lines 2–3 anyway and the code would look for it at 0–1, miss, and duplicate. That is rarer than the case fixed here and the honest remedy is to skip leading blank lines before looking — one line, only worth doing if a lane hits it.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT at 380c4c3: READY

Re-review of 380c4c3. It adds 380c4c3, which answers my CHANGES at f3d8a3b: --fix duplicated the notice on a short header behind a #!. It merges cleanly onto main.

Checked at this head:

  • strip_short_header() now takes the stub from lines 1-2 when line 0 is #!, and keeps the shebang in front.
  • The same fixture repo as last time (only this head's tools/copyright.py, then --fix then --check):
file after --fix
src/short.h (// stub) 1 Copyright line
src/tool.py (#! + # stub) 1 Copyright line, #! still on line 1
src/s.sh (#! + # stub) 1 Copyright line, #! still on line 1

--check after --fix gives rc 0. It was rc 1 on both scripts at f3d8a3b.

  • Merged with #2192 (still open; a local merge commit, never pushed): the same fixtures give rc 0 with one Copyright line each. The two --fix changes do not fight.
  • tests/copyright_check_untracked_test.py: 0 failures, including the new shebang arm.
  • Mutant: with the shebang offset forced to at = 0, the test fails (rc 1, 2 FAIL). After restoring the file the tree is clean.
  • tools/copyright.py --check on the tree: rc 0.
  • CI: build at 380c4c3 failed in Tools for checkout and the build at 15:20Z. That is the runner DNS outage, not this tree.

Non-blocking:

  • The body still describes only the message change. Two of the three commits change --fix, so one line saying so would help whoever reads the merge.
  • With #2192, two PRs now repair short headers in --fix. Whichever lands second can drop its half.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 751aa35

Merged main in, because the fault that reddened this PR's build was fixed upstream at 17:15Z (#2224, the container's own resolvers) and this branch predates it.

My build failed at step 3, "Tools for checkout and the build" — the apt install — at 15:20Z, before the copyright step could run. #2224 merged at 17:15:53Z, so the merge is the remedy rather than
a re-run: the same commit now runs on a container that can resolve its own archive.

Nothing of this PR was lost in the merge — checked rather than assumed:

$ git diff --stat 380c4c38 HEAD -- tools/copyright.py tests/copyright_check_untracked_test.py
 tools/copyright.py                     | 37 +++++++++++++++++++++-
 tests/copyright_check_untracked_test.py | 39 +++++++++++++++++++++
 (75 insertions, the three commits' content)
$ git show HEAD:tools/copyright.py | grep -c short_notice     -> 3
tests/copyright_check_untracked_test.py                        -> 32 arms, 0 failures

Re-verified the behaviour the PR exists for, at this head, on a scratch repo (short header → --fix → --check): rc 0. And on the real branch that is holding main red — engine#2183 at f993613,
13 short-header files → --fix → --check rc 0 — so the fix still repairs the thing it was written for after the merge.

What is in the PR now, in three moves: name the shape (short_notice), repair it (--fix replaces the stub instead of stacking a second notice, which is what made the tool's own remedy useless), and
find it behind a shebang (lines 1–2, not 0–1). 32 arms across the two test files, and the sweep that motivated it — 33 short-header files across four branches — is unchanged by a main merge.

The one thing to watch on this run: if build fails again at step 3, the apt fault is not fully closed and the message question on #2212 (::error:: … it is the mirror) is still the first thing to
fix, because it will again point the next reader at the mirror.

@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit 751aa35

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT at 751aa35: CHANGES

Re-review of 751aa35, a merge of main (751aa350, 22:50Z). Main has meanwhile taken two --fix changes of its own:

  • #2192 (83012c89) repairs a short header, including behind a #!.
  • #2228 (ada88bb5) makes --fix keep blank lines.

Merged on top of those, this PR's strip_short_header() is now redundant, and it undoes #2228. Fixture repo, with only tools/copyright.py from each tree, then --fix then --check. "Blank line before the body" means whether the original blank line between the stub and the code is still there:

file main origin/main this head
short.h (// stub, no blank) 1 notice, check rc 0 1 notice, check rc 0
tool.py (#! + # stub) 1 notice, #! on line 1 same
blank.sh (#! + stub + blank line + code) blank kept blank dropped
blank.h (stub + blank line + #pragma once) blank kept blank dropped

The cause is the if rest and rest[0].strip() == "" and len(rest) > 1: rest = rest[1:] in strip_short_header(). tests/copyright_gate_test.sh and tests/copyright_check_untracked_test.py both pass at this head, so nothing catches it.

Ask: drop strip_short_header() and its call in add_notice(), since main's #2192 already does that job and the two now disagree on blank lines. Keep short_notice() and the new message: copyright + SPDX present but the licence block is not (paste the 16-line form). That is still not on main (grep -c short_notice on main's tools/copyright.py gives 0), and it is the part #2175 and #2173 needed. With that, this is READY.

bong-water-water-bong and others added 2 commits November 12, 2026 04:00
… so strip_short_header() is redundant
Answers the CHANGES at 751aa35. Main has taken two --fix changes since this branch
was cut:

- #2192 (83012c8) repairs a short header, including one behind a `#!` -- `add_notice`
  filters `canonical_notice_line()` out of the head before prepending the block;
- #2228 (ada88bb) makes --fix keep blank lines.

Merged on top of those, this branch's `strip_short_header()` was redundant and
actively wrong: its `if rest and rest[0].strip() == "" and len(rest) > 1: rest = rest[1:]`
dropped a blank line after the stub, undoing #2228. The reviewer measured both:

| file | main | this head (before) |
|---|---|---|
| blank.sh (`#!` + stub + blank + code) | blank kept | blank dropped |
| blank.h (stub + blank + `#pragma once`) | blank kept | blank dropped |

So the function and its call are gone. What this PR keeps is the half main does not
have: `short_notice()` and the sentence it prints --

    copyright + SPDX present but the licence block is not (paste the 16-line form)

-- which is why the PR exists at all: main's `copyright.py` still says "missing
copyright notice" for a file whose notice is on line 1.

Re-ran the reviewer's four-case table at this head: `short.h`, `tool.py` (#! + # stub),
`blank.sh` (#! + stub + blank), `blank.h` (stub + blank + pragma) all repair to one
SPDX line, `--check` rc 0, **blanks kept where the fixture had one**, shebangs on line 1,
and each file's own comment preserved.

tests/copyright_check_untracked_test.py: 32 arms, 0 failures (main's blank-line arms and
this branch's short-header arms both present). copyright.py --check on this tree: rc 0.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 7dd6e43

CHANGES answered: merged main, and strip_short_header() is gone — it was redundant and it undid #2228. Your four-case table re-run at this head:

file result
short.h (// stub, no blank) 1 SPDX line, --check rc 0, own comment kept
tool.py (#! + # stub) same, #! on line 1
blank.sh (#! + stub + blank + code) blank kept (was dropped)
blank.h (stub + blank + #pragma once) blank kept (was dropped)

python3 copyright.py --check on the fixture repo: rc 0. tests/copyright_check_untracked_test.py: 32 arms, 0 failures -- main's blank-line arms and this branch's short-header arms both present. Tree check rc 0.

You were right about the cause, including the line: the if rest and rest[0].strip() == "" and len(rest) > 1: rest = rest[1:] was where #2228's blank went. Main's add_notice now filters canonical_notice_line() out of the head itself, so the repair this branch was doing is not necessary -- and two implementations of one repair had already started disagreeing, which is the failure mode the earlier --where and where_rc pair taught me.

What the PR keeps, and why it still exists: short_notice() and the sentence —

copyright + SPDX present but the licence block is not (paste the 16-line form): <path>

main's copyright.py still prints "missing copyright notice" for a file whose notice is on line 1, and that is what four branches spent a day reading. This PR is now three lines of predicate and one message, which is the right size for what main does not have.

The measurement that motivated it is unchanged: at 751aa35's era, 33 failing paths across five open PRs, 32 short headers and 1 genuine miss — and the predicate's value is that it leaves that one alone.

@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit 7dd6e43

@bong-water-water-bong
bong-water-water-bong merged commit 26f1320 into main Nov 12, 2026
9 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the prs2-short-notice branch November 12, 2026 08:20

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at 7dd6e43

CHANGES answered: merged main, and strip_short_header() is gone — it was redundant and it undid #2228. Your four-case table re-run at this head:

file result
short.h (// stub, no blank) 1 SPDX line, --check rc 0, own comment kept
tool.py (#! + # stub) same, #! on line 1
blank.sh (#! + stub + blank + code) blank kept (was dropped)
blank.h (stub + blank + #pragma once) blank kept (was dropped)

python3 copyright.py --check on the fixture repo: rc 0. tests/copyright_check_untracked_test.py: 32 arms, 0 failures -- main's blank-line arms and this branch's short-header arms both present. Tree check rc 0.

You were right about the cause, including the line: the if rest and rest[0].strip() == "" and len(rest) > 1: rest = rest[1:] was where #2228's blank went. Main's add_notice now filters canonical_notice_line() out of the head itself, so the repair this branch was doing is not necessary -- and two implementations of one repair had already started disagreeing, which is the failure mode the earlier --where and where_rc pair taught me.

What the PR keeps, and why it still exists: short_notice() and the sentence —

copyright + SPDX present but the licence block is not (paste the 16-line form): <path>

main's copyright.py still prints "missing copyright notice" for a file whose notice is on line 1, and that is what four branches spent a day reading. This PR is now three lines of predicate and one message, which is the right size for what main does not have.

The measurement that motivated it is unchanged: at 751aa35's era, 33 failing paths across five open PRs, 32 short headers and 1 genuine miss — and the predicate's value is that it leaves that one alone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant