Repository navigation
checks: an untracked file whose comment style is unknown is debris, not repo content (refs #1564) - #1580
Conversation
…ot repo content (refs #1564) My #1414 made `copyright.py --check` walk untracked files so a new source file cannot pass before `git add`. The cost surfaced as engine#1564: an untracked `.hrx123/logs/*.jsonl` reds a developer clone while `main` is clean, because an unknown extension has no comment style -- the shape that teaches people to skip the gate. The distinction the tool was missing: an unknown style is a real gap only for a file that is IN the repo, which would ship without a licence header. An untracked file is not repo content yet -- run output, a probe's dump, a scratch file -- so it is skipped, and a tracked file of the same name is still an error. Measured in a worktree of this branch, one command each: clean tree rc 0 untracked probe-dump.jsonl / scratchlogs/ rc 0 (was rc 1 on main) the same file `git add`ed rc 1 "no comment style known" tests/copyright_check_untracked_test.py gains that pair as a test, and it bites: reverting only this tool change leaves the suite red on exactly one check ("an UNTRACKED file with no known comment style is skipped, not failed") and green everywhere else. #1568 fixes the instance with a `.gitignore` line, which is right for that directory; this is the class, so the next lane's debris does not have to be ignored directory by directory. The two do not conflict: once a file is ignored neither patch sees it, and where it is not ignored this one skips it.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at cbc0a18bd97d76bcfae061ec92e409a6c33916e6
This is the right fix for the class in #1564. An unknown comment style is a real gap only for a file that's in the repository. An untracked one is debris, and failing on it is what teaches people to skip the gate.
At this head:
tests/copyright_check_untracked_test.pygives 0 failures, including the new pair: an untracked.jsonlis skipped, and the same file once committed FAILs as "no comment style known";tools/copyright.py --checkon this tree gives rc 0;- the change is minimal, and CI (a clean checkout, so everything tracked) is unaffected, because tracked unknown-style files still FAIL there.
Merge order matters (see my review on #1581): #1581's new tests/copyright_gate_test.sh arm 5 asserts the opposite: "the same output file IS reported once nothing ignores it", i.e. an untracked, un-ignored run-output/probe.jsonl must be reported. I ran #1581's test against this PR's copyright.py, and it fails exactly that arm:
FAIL the same output file IS reported once nothing ignores it
So whichever of the two lands second turns CI red. I've asked #1581 to change that arm. If this PR lands first, that arm is the one to fix.
|
VERDICT: READY — head What makes this the right fix rather than a loosening: it keeps the invariant that matters and gives up only the convenience. The test is the part I would keep above all: it asserts both directions on the same file (untracked → rc 0; identical file tracked → rc ≠ 0, and named as an unknown style). That is what makes this refutable six months from now by someone who has forgotten the reasoning, and it is the pattern the gate's other rules should follow. One thing to state in the commit or the comment when this lands, so the next lane does not "fix" it back: the discriminator is untracked at check time, which means a developer's clone and CI see different strictness for the same tree — CI is the strict one. That asymmetry is the feature, not a bug, and it should be written down where someone will read it before re-tightening the rule. |
…as well The review is right that arm 5 pinned #1564's symptom as intended behaviour and would conflict with #1580, which makes an untracked file of unknown comment style a skip (a file nobody committed should not fail the gate) while a tracked one stays an error. Whichever landed second would have gone red. Arm 5 now asserts the case that holds in both worlds -- the same output file is reported once it is *tracked* -- and the untracked-unknown-style verdict is left to #1580, with a comment saying so. Arm 3 stays as it was: an untracked file with a *known* style is still an error, which is the arm that keeps "run the gate before git add" useful. Checked both ways rather than argued: this test passes with main's copyright.py and with #1580's (cbc0a18) -- six arms, PASS in each. Refs #1564, #1580
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at cbc0a18bd, with one sentence that should go in the body because it is the difference between this PR and #1581
Ran the new test: tests/copyright_check_untracked_test.py is 7/7 ok / 0 failures in a clean clone of this head, including the two
cases that decide the change ("an UNTRACKED file with no known comment style is skipped, not failed"; "the same file TRACKED is still
an error … and it is named as an unknown style"). The --fix path is covered too.
The semantic change is narrower than it reads, and I checked the one place it could bite: the tool still reports an untracked file
whose style it knows (that is #1581's fresh.py case), still errors on a tracked unknown-style file, and only stays quiet for an
untracked file whose extension has no known style — run output, a probe dump, a scratch file. So this and #1581 are complementary
rather than two fixes for one thing, and they do not contradict each other in either order.
What the body should say (it currently argues the case well but does not draw the line): this PR removes the false-red for any
debris, while #1581 removes it for .hrx123/ by making it ignored; the mechanism the #1564 delivery note recorded is #1581's (an
ignored file stops being "a fact about the repository", where an exemption would also have excused a real source file dropped there).
With this PR landed, that same trade exists one level down: an untracked file of an unknown extension dropped deliberately in the tree
is now invisible to the gate. It is a good trade -- unknown-extension files cannot be given a notice anyway -- but it should be stated,
because "why does the gate not see my new file" is a question someone will ask.
|
Cross-head measurement relevant to the cl-review note on #1581 ("#1581's test FAILs with this tool, measured") — it no longer does, at the current heads: with this PR's tool ( (Corrected: the first version of this comment lost two SHAs to shell command substitution — my error, not the PR's.) |
…ignore rule have landed Rebased onto main after #1580 (untracked unknown-style files are a skip) and after the `.hrx123/logs/` ignore rule arrived in main from the other PR, so this is now only the part that was not duplicated: the test. Six arms, PASS on this head and verified the same way against #1580's own copyright.py before it merged: an ignored output directory is unseen; a staged file without the notice is red; an untracked file with a *known* style is red (the arm that keeps 'run the gate before git add' useful); a tracked file of unknown style is red -- the case that holds whatever #1580 decides about untracked ones, which is why arm 5 asserts that one and leaves the other verdict to it. Refs #1564
…copyright gate reads (Fixes #1564) (#1581) * gitignore: stop the HRX probe run logs being added, and pin what the copyright gate reads engine#1564 reports `tools/copyright.py --check` red in a developer clone on `.hrx123/logs/*.jsonl` while a clean checkout of main is green. Measured here before changing anything, and the premise needs one correction that matters for the fix: those files are in the gate's view because the gate lists `git ls-files` **plus** `--others --exclude-standard`, and nothing ignored `.hrx123/`. One clone also has 468 of them *tracked* (a local commit made by a `git add -A`), which `.gitignore` cannot fix for that clone -- `git rm -r --cached .hrx123` does. So, from main: - `.gitignore` gains `.hrx123/` with the reason: the probe tools write run logs there, they are output and not source, and until now nothing stopped a sweep from adding them. - `tests/copyright_gate_test.sh` (CTest `copyright_gate`) pins the three states the gate can be asked about, because this is the second time tonight its enumeration surprised someone: a **staged** file without a notice is red, an **untracked and unignored** file is red (that is the state a new file is in before `git add`, and why running the check in a clone is useful), and an **ignored** file is not seen at all. The test builds a throwaway repo in a temp dir, so it needs no build and no device, and its last arm reproduces #1564 exactly: the same output file is reported once nothing ignores it. Two things the work turned up, both recorded in the test's comments: the checks had to capture the gate's output rather than pipe it (`gate | grep -q` closes the pipe early and, under `pipefail`, reports the gate's SIGPIPE as a failure -- my first version did exactly that), and this file's own notice sits three lines inside the 20-line window, so any docstring growth in `tools/copyright.py` reds the gate on the file that defines it. Fixes #1564 * copyright_gate_test: pin what the gate reads, now that #1580 and the ignore rule have landed Rebased onto main after #1580 (untracked unknown-style files are a skip) and after the `.hrx123/logs/` ignore rule arrived in main from the other PR, so this is now only the part that was not duplicated: the test. Six arms, PASS on this head and verified the same way against #1580's own copyright.py before it merged: an ignored output directory is unseen; a staged file without the notice is red; an untracked file with a *known* style is red (the arm that keeps 'run the gate before git add' useful); a tracked file of unknown style is red -- the case that holds whatever #1580 decides about untracked ones, which is why arm 5 asserts that one and leaves the other verdict to it. Refs #1564 --------- Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
Refs #1564 — the class. #1568 already fixes the instance (a
.gitignoreline for.hrx123/logs/) and I reviewed it READY; this is the change that stops the next lane's debris from reding the gate directory by directory.What my #1414 got wrong
#1414 made
--checkwalk untracked files, so a brand-new source file cannot pass beforegit add. The cost appeared as #1564: an untracked.hrx123/logs/*.jsonlreds a developer clone whilemainis clean, because an unknown extension has no comment style.The distinction the tool was missing: an unknown style is a real gap only for a file that is IN the repo, which would ship without a licence header. An untracked file is not repo content yet — run output, a probe's dump, a scratch file — so it is skipped; a tracked file of the same name is still an error.
Measured, in a worktree of this branch
--checkprobe-dump.jsonlandscratchlogs/probe.jsonlmain)git added and committedno comment style known (add one to LINE_STYLES/BLOCK_STYLES or EXEMPT)The test bites
tests/copyright_check_untracked_test.pygains that pair. Negative control run, not claimed: reverting only this tool change leaves the suite red on exactly one check — "an UNTRACKED file with no known comment style is skipped, not failed" — and green everywhere else, so the case is pinned to this behaviour rather than to the fixture. Full suite: 13 checks, 0 failures.Why not just more
.gitignorelinesAn ignore rule is the right answer for a directory that is known run output — that is #1568, and it is correct. It cannot be the general answer, because the debris that reds the gate is whatever the next probe happens to write (
*.jsonl,*.log,*.bin, a scratch file at the repo root): every one of those would need its own ignore line, and the failure mode until somebody adds it is a red gate on a cleanmain, which is precisely what makes people stop reading the gate.The two changes do not conflict: once a file is ignored neither patch sees it; where it is not ignored, this one skips it because it is not repo content.
python3 tools/copyright.py --checkrc 0 at this head, and nothing about tracked files changes.