Skip to content

copyright: --check names the command that repairs a duplicate, instead of telling the reader to hand-edit - #2235

Merged
bong-water-water-bong merged 1 commit into
mainfrom
pkg/copyright-advice
Nov 11, 2026
Merged

bong-water-water-bong merged 1 commit into
mainfrom
pkg/copyright-advice

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

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

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

"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. 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 prints run: 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

  1. python copyright.py --check | grep -q … always fails in this file. The gate test 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. Same shape as cmd | tail making $? 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.
  2. \" inside $( ) inside a double-quoted argument is not unescaped by bash. My first version nested case "$( cd \"$tmp\" && … )" in …, which fed the quote characters into the path — the failure printed cd: "/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.sh PASSes on this branch, FAILs with the old wording restored.

…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.
@github-actions

Copy link
Copy Markdown

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

Corrected Test Logic

The test logic was updated to correctly handle the --check command's exit status when used in a pipeline under set -o pipefail. The previous version would have failed due to the checker exiting with code 1 on duplicate detection, which is now properly addressed by capturing the output first before checking it.

dup_report=$( cd "$tmp" && python3 copyright.py --check 2>&1 )
check "the duplicate report names --fix rather than telling the reader to edit by hand" \
      "case \"\$dup_report\" in *'keeps the first copy'*) true ;; *) false ;; esac" \
      "$( cd "$tmp" && python3 copyright.py --check 2>&1 | grep duplicated )"
Updated Error Message

The error message for duplicated copyright notices has been updated to reflect the actual behavior of --fix, which now keeps the first copy and drops the rest, instead of instructing the user to manually delete the second copy. This change aligns the documentation with the implemented functionality.

print("copyright notice duplicated inside the header window (--fix keeps the first copy and drops the rest):", path)

@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 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:

  1. python copyright.py --check | grep -q … always fails in this file — set -o pipefail plus grep -q exiting 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 own exit=0 through a pipe mistake earlier today.
  2. 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
bong-water-water-bong merged commit d7b39c6 into main Nov 11, 2026
9 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the pkg/copyright-advice branch November 11, 2026 23:51

@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 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.

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