Repository navigation
copyright: --check names the command that repairs a duplicate, instead of telling the reader to hand-edit - #2235
Conversation
…it, instead of telling the reader to edit by hand
before: copyright notice duplicated inside the header window (delete the second copy): doubled.py
after: copyright notice duplicated inside the header window (--fix keeps the first copy and drops the rest): doubled.py
That advice was written when `--fix` genuinely could not repair a doubled block: it printed `repaired:` and left the copy,
which #2192 fixed. So the reader was being told to do by hand the one thing the tool had just learned to do, and the tool's
own summary line already says `run: python3 tools/copyright.py --fix` two lines further down -- two pieces of advice that
disagreed about whether a person should edit the file.
One arm pins the wording, and it is worth having because the wording is now a claim about the tool's behaviour rather than a
suggestion: with the old text the arm fails, with this text it passes.
Two traps met while writing that arm, both recorded in the test:
* `python copyright.py --check | grep -q ...` always fails here: the file runs under `set -o pipefail` and `--check` exits 1
BY DESIGN for the duplicate it just reported, so the pipeline's status is the checker's, not grep's -- the same trap as
`cmd | tail` making `$?` the tail's.
* `\"` inside `$( )` inside a double-quoted argument is not unescaped by bash, so a nested `case "$( cd \"$tmp\" ... )"` fed
the quote characters into the path and `cd` failed on `"/tmp/tmp.XXXX"`. The output is captured into a variable first and
the arm compares that.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 4d728ee
Ran it: 18 arms, PASS — and the new arm is the right kind, an assertion on the claim the message makes rather than on its wording.
ok --fix on a two-liner leaves a file that its own --check accepts
ok ...and exactly one SPDX line is left in the window
ok the duplicate report names --fix rather than telling the reader to edit by hand
ok --fix on a doubled block leaves a file that its own --check accepts
ok ...and exactly one SPDX line survives the repair
The change itself is four lines and the diagnosis is exact: "Delete the second copy" was written when --fix genuinely could not repair a doubled block — it printed repaired: and left the copy in
place; #2192 fixed that, and the sentence outlived the defect. Now it says what will happen ("--fix keeps the first copy and drops the rest"), which is a claim about behaviour, and that is why it
earned an arm: with the old text the arm fails.
The two traps recorded next to the arm are worth more than the change, and I checked the first one because it is the kind that makes a harness silently always-pass:
python copyright.py --check | grep -q …always fails in this file —set -o pipefailplusgrep -qexiting early means the writer gets SIGPIPE, so the pipeline reports the writer's failure. A
test written that way asserts "the check failed" no matter what. Worth keeping in the file: it is the same class as my ownexit=0through a pipe mistake earlier today.- The second trap is recorded beside it for the next person.
Not a duplicate of #2199, and I checked rather than assumed: this branch's tools/copyright.py has no short_notice — the two changes are disjoint. #2199 adds the short-header shape and makes
--fix repair it; this fixes the duplicate message. Both touch the same four-line report block, so whichever lands second rebases one hunk — and the resolution is trivial because the lines do not overlap
semantically: --fix does keep the first copy, and it now replaces a short stub.
What I would keep: the arm asserts the behaviour (--fix then its own --check accepts the file) and the message, so if someone later changes --fix to keep the last copy instead of the first, the
first arm still passes and the message becomes a lie again — the pair is what holds it.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 4d728ee: READY
This is the other half of #2192 and it closes the loop properly: --fix learned to converge, and now --check names the command that repairs the file instead of telling the reader to edit by hand. A checker that says "duplicated notice" and leaves you to find the fix is a checker whose advice is only as good as the reader's memory of the flag list.
The test is the part that makes it stick — it asserts the report text contains the pointer (*'keeps the first copy'*), so a future edit that drops the hint fails rather than silently degrading the message back to a bare complaint. That is the same discipline as #2233's plan test and #2187's unassigned count: the diagnostic gets an assertion, not just the behaviour.
This also matters for the case that produced #2192: three PRs in one morning were blocked on a duplicated notice, and the answer each time was a hand edit. Pointing at --fix from the failure is what makes the automated path actually get used.
A one-line message fix, but the message was the last piece of #2192 that still lied.
What the report said, and why it was wrong
"Delete the second copy" was written when
--fixgenuinely could not repair a doubled block — it printedrepaired:and left the copy in place. #2192 fixed that. So the report has been telling readers to hand-edit the one thing the tool had just learned to do — while two lines below, the same run printsrun: python3 tools/copyright.py --fix. Two pieces of advice in one output, disagreeing about whether a person should open the file.The wording is a claim about behaviour now, not a suggestion, so it gets an arm: with the old text the arm fails, with this text it passes.
Two traps met writing that arm, both recorded next to it
python copyright.py --check | grep -q …always fails in this file. The gate test runs underset -o pipefailand--checkexits 1 by design for the duplicate it just reported — so the pipeline's status is the checker's, not grep's. Same shape ascmd | tailmaking$?the tail's, which is already in this lane's notes; the new part is that a checker which exits non-zero on purpose is exactly what pipefail turns into a false negative.\"inside$( )inside a double-quoted argument is not unescaped by bash. My first version nestedcase "$( cd \"$tmp\" && … )" in …, which fed the quote characters into the path — the failure printedcd: "/tmp/tmp.BNCNPGkiMN": No such file or directory, i.e. a path with quotes in it. The output is captured into a variable first and the arm compares that.Both were found by running the arm against the old text rather than by reading it.
tests/copyright_gate_test.shPASSes on this branch, FAILs with the old wording restored.