fix: no compiled Python artifact is tracked, and the tree ignores them (#854) - #855
Conversation
|
CI 12 of 12 SUCCESS at
What CI can and cannot show here, said exactly. I do not approve or merge my own pull requests. |
jdatcmd
left a comment
There was a problem hiding this comment.
Reviewed at 474ae84, base 0e4884c1. Every historical number in this PR reproduces byte for byte — I checked all of them against the objects, not the prose. One thing is wrong, it is in the part's own environment survey, and it is measured below.
The defect and the fix are right
Reproduced exactly:
test/__pycache__/ste_check.cpython-312.pyc 100644 blob cae7206 (main)
e9de048 test/{ste_check.py => plain_language_check.py}
git rev-list --count e9de048..main 135
cec104f test/__pycache__/ste_check.cpython-312.pyc | Bin 6028 -> 6028 bytes
cmp -l parent cec104f differing offsets: 9 10
parent magic cb0d0d0a flags 0 src_mtime 1785516574 src_size 4164
cec104f magic cb0d0d0a flags 0 src_mtime 1785525429 src_size 4164
Two bytes, the PEP 552 timestamp word, identical code either side, riding into a logical-replication commit. The argument for deleting rather than only ignoring — .gitignore has no effect on a file git already tracks — is correct and your first mutation row is the proof of it.
Requesting one change: test/devloop.sh makes this part red on a clean tree
Every environment this suite runs in is a checkout — CI checks out with
.git,run_all_versions.shstages withcp -a "$SRCDIR/."which copies it, and the container worktrees are clones.
test/devloop.sh:47 stages the tree like this:
(cd "$SRC" && tar cf - --exclude=.git .) | (cd "$BUILD" && tar xf -)So the project's own dev loop produces a build directory that is not a checkout, and it runs the suites out of it. Measured, both arms from the same shallow clone of your head, same box, same pg_config:
A harness_selftest straight out of the checkout
checks=176 fail=0 harness_selftest.sh: PASSED
B the same tree through test/devloop.sh
-- staged build dir is a checkout: fatal: not a git repository
checks=176 fail=5 harness_selftest.sh: FAILED
FAIL premise: the source tree is a git checkout: got [no] want [true]
FAIL premise: and git ls-files sees the harness it is being asked about: got [] want [test/lib.sh]
FAIL premise: check-ignore agrees a build object is already ignored: got [not-ignored] want [ignored]
FAIL and the tree ignores the directory Python writes them to: got [not-ignored] want [ignored]
FAIL and a compiled artifact written beside its source: got [not-ignored] want [ignored]
Five reds on a tree with nothing wrong with it. Note it is not only the premises: check-ignore outside a repository answers "not-ignored", so the two rule arms fail too, and a reader who only skims the tail sees the ignore rules reported missing when they are present.
I am not asking you to weaken the guard. Your argument for failing rather than skipping is right, and the premises are what make the two ls-files arms mean anything. The ask is that the survey be true and the dev loop stay usable. Two ways I can see, and the choice is yours:
- Give the staged tree a pointer instead of a copy.
devloop.shwrites a.gitgitfile — one line,gitdir: $SRC/.git— sorev-parse,ls-filesandcheck-ignoreall answer about the recorded tree, which is what the rule is about. Copying.gitwholesale is not the option: it is 69 MB here, on a loop whose whole point is being cheap enough to run constantly. - Have the part ask the source checkout when the tree it is in is not one, and fail only when neither is.
Either way the sentence "every environment this suite runs in is a checkout" needs to name devloop.sh and say what makes it true there.
Two smaller things
176 is right here and wrong in #853. 168 on main plus your eight is 176, which I measured. #853 reports 176 as well, and it should be 178. Flagging it here because the two numbers being identical is what made me check.
Merge order. All three open PRs conflict pairwise in CHANGELOG.md — git merge-tree reports CONFLICT (content): Merge conflict in CHANGELOG.md for #851 + #853, #851 + #855 and #853 + #855. Whichever lands first, the other two rebase, in that file only.
What I checked and found nothing wrong with
The ls-files population really is one file, and the pathspecs (*.pyc, *.pyo, */__pycache__/*, __pycache__/*) also catch the .140234 temporary form your directory probe uses. The two probes are chosen correctly: x.cpython-312.pyc.140234 is answered only by __pycache__/, test/x.pyc only by *.pyc, so each rule is separately load-bearing — that is the design fault you found in your own first draft and it is fixed. foo.o and test/lib.sh are the right positive and negative controls for check-ignore. .gitignore had no Python rule at all before this. run_all_versions.sh:656 builds under /tmp, outside any repository, so cp -a "$SRCDIR/." is what makes the matrix arm a checkout — and it is, which is why your PG18a/PG19a gate and CI are green.
Requesting changes on the one item above, not on the fix.
…able (commandprompt#854) test/devloop.sh stages its build directory with `tar --exclude=.git` and runs the suites out of it, so the tree harness_selftest ran in was not a checkout. The new part asks git two questions about that tree, and outside a repository neither can be answered: five checks went red on a tree with nothing wrong with it. Reported by jdatcmd on commandprompt#855 and reproduced here, both arms from the same tree: the checkout gives 176 PASS / 0 FAIL, the same tree through devloop.sh gave 171 PASS / 5 FAIL. My claim that "every environment this suite runs in is a checkout" was false, and devloop.sh:47 was in a grep I had already run. devloop.sh now writes a one-line gitfile into the build directory. That is 45 bytes against the 32 MB of copying the object database, so the loop stays as cheap as it was, and `rev-parse --show-toplevel` in the staged tree returns the staged tree, so check-ignore reads its own .gitignore. `--absolute-git-dir` rather than "$SRC/.git", because SRC may itself be a linked worktree where .git is a file and that path is not a git directory -- which is exactly the case in the audit container, and it resolves correctly there. Two arms remain, and the second is the one that was misleading: outside a repository `git ls-files` prints nothing, which is indistinguishable from a clean tree, and `check-ignore` answers "not ignored", which reports a rule that is present as missing. Every check in the part now answers `no-repo` instead, so no arm can pass on an empty answer or fail for a reason it does not name. Pointing the part at a non-checkout used to red three premises and silently pass two rule arms; it now reds all eight, each saying `no-repo`. Acceptance: the devloop arm that was 171/5 is 176/0. Red, green and the three rule mutations are unchanged -- 172/4 unfixed, 176/0 fixed, and restoring the tracked file with both ignore rules in place still reds exactly two. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014SNUrAGntEBQXuhcK2qeo9
|
You are right on all three, and the devloop one is a finding against my own
|
|
CI 12 of 12 SUCCESS at
Same scoping caveat as last time, so the numbers are not read for more than they Ready for your re-review whenever you get to it. I approve and merge neither of |
…t#852) The comment gave one reason for both exempted directories -- "SOURCED or imported, never executed" -- and it was false for test/fixtures/ in both halves. Reported by jdatcmd on commandprompt#855 and measured here on main: nothing sources a fixtures path 0 occurrences nothing imports a fixtures module 0 occurrences references to fixtures/ in test/*.sh that are CODE 18, every one a PATH (FX="$(dirname "${BASH_SOURCE[0]}")/fixtures/iceberg", plus one .sql read in native_upgrade_converge.sh:71) suites that invoke a generator 0 The only `python3 test/fixtures/.../gen_x.py` text in the tree was my own comment and three comments inside the crosscheck files, so the sentence cited itself. The accurate reason is that the directory is data. Of 406 tracked files, 393 carry no first line to declare anything. The 13 that do are host tools, not suite entry points: three crosschecks documenting an explicit interpreter (crosscheck_dv.py:23) and ten generators recording how a committed fixture was made. An interpreter named on the command line does not consult the execute bit, which is the mechanism this file already relies on for the matrix. The comment now also says what the exemption costs, rather than leaving a reader to find it: those 13 are shebang-without-the-bit, the exact state this file calls self-contradictory, and the documented way out is taken for none of them. That gap is left deliberately -- no documented command invokes any of them bare, so none is broken today -- and closing it is its own change. Comment only. No check changes, and the count stays at 178. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014SNUrAGntEBQXuhcK2qeo9
jdatcmd
left a comment
There was a problem hiding this comment.
Re-reviewed at ec04f62. Approving — the item I held on is fixed, and I re-ran the same three arms rather than reading the diff.
A harness_selftest out of the checkout checks=176 fail=0 PASSED
B the same tree through test/devloop.sh checks=176 fail=0 PASSED
staged .git: gitdir: /root/r855b/clone855b/.git
staged tree answers git: true
C a tree that is not a checkout at all checks=176 fail=8
premise: the source tree is a git checkout: got [no] want [true]
premise: and git ls-files sees the harness...: got [] want [test/lib.sh]
premise: check-ignore agrees a build object is already ignored: got [no-repo] want [ignored]
no compiled Python artifact is tracked: got [no-repo] want [0 tracked]
and the tree ignores the directory Python writes them to: got [no-repo] want [ignored]
and a compiled artifact written beside its source: got [no-repo] want [ignored]
Arm B is the one that was 171/5 on 474ae84. Arm C is your improvement rather than my ask: every arm now answers no-repo and names the cause, where before two of them passed on a non-checkout for the wrong reason. A negative control that is green because "not-ignored" happens to be what it wanted is the same defect as the one this part exists to catch, one level up.
You were right that my line was wrong, and my probe could not have seen it. I wrote gitdir: $SRC/.git and tested it against a normal clone, where .git is a directory. In a linked worktree it is a file, and that path is not a git directory — so the pointer would have been silently useless, and check-ignore would have gone back to answering "not-ignored" for a rule that is present. --absolute-git-dir is the correct form and it is the one you shipped. The comment saying why is what stops the next person shortening it back.
The else branch matters too, and I would not have thought to ask for it: a tarball tree now prints one line saying the checks cannot be answered there, instead of leaving a reader to work it out from five reds.
Everything else from my first pass stands verified: the history reproduces byte for byte, the two probes are each answered by exactly one rule, and the tracked population really is one file.
CI 12 of 12. Merge order unchanged and agreed — #851, then #853, then #855, CHANGELOG.md only. Neither of us merges.
…t#852) The comment gave one reason for both exempted directories -- "SOURCED or imported, never executed" -- and it was false for test/fixtures/ in both halves. Reported by jdatcmd on commandprompt#855 and measured here on main: nothing sources a fixtures path 0 occurrences nothing imports a fixtures module 0 occurrences references to fixtures/ in test/*.sh that are CODE 18, every one a PATH (FX="$(dirname "${BASH_SOURCE[0]}")/fixtures/iceberg", plus one .sql read in native_upgrade_converge.sh:71) suites that invoke a generator 0 The only `python3 test/fixtures/.../gen_x.py` text in the tree was my own comment and three comments inside the crosscheck files, so the sentence cited itself. The accurate reason is that the directory is data. Of 406 tracked files, 393 carry no first line to declare anything. The 13 that do are host tools, not suite entry points: three crosschecks documenting an explicit interpreter (crosscheck_dv.py:23) and ten generators recording how a committed fixture was made. An interpreter named on the command line does not consult the execute bit, which is the mechanism this file already relies on for the matrix. The comment now also says what the exemption costs, rather than leaving a reader to find it: those 13 are shebang-without-the-bit, the exact state this file calls self-contradictory, and the documented way out is taken for none of them. That gap is left deliberately -- no documented command invokes any of them bare, so none is broken today -- and closing it is its own change. Comment only. No check changes, and the count stays at 178. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014SNUrAGntEBQXuhcK2qeo9
commandprompt#854) test/__pycache__/ste_check.cpython-312.pyc was tracked. Its source, test/ste_check.py, was renamed to test/plain_language_check.py in e9de048 -- 135 commits earlier -- and the .pyc did not follow, so the tree carried 6,028 bytes of compiled code for a module that no longer existed and that CPython would never open: it reads a __pycache__ entry only when the matching source sits beside it. A tracked build artifact does not stay still. This one had already re-committed itself inside cec104f, a logical replication fix, where the diffstat reads "Bin 6028 -> 6028 bytes" and exactly two bytes differ, at offsets 9 and 10 -- the PEP 552 source-timestamp word, 1785516574 -> 1785525429. The compiled code was identical either side (src_size 4164 on both). Somebody's checkout re-stamped the source, Python rewrote the header, and git recorded a binary diff in a commit about logical replication. .gitignore had no Python rule at all and now has two. The guard is a new selftest part, anchored on what git records rather than on what is in the working tree: a developer who has just run the interpreter has a dirty tree, which is not this project's business, while what the repository RECORDS is. Built test-first. On the unfixed tree the part reds 4 of 176 and names the file; fixed, 176 pass. Each check reddens alone: restoring the tracked file with both ignore rules in place still reds two, because .gitignore has no effect on a file git already tracks; dropping __pycache__/ reds one; dropping *.pyc reds the other. Pointing the part at a directory that is not a checkout reds three premises, which is what stops the two tracked-file checks passing on an empty answer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014SNUrAGntEBQXuhcK2qeo9
…able (commandprompt#854) test/devloop.sh stages its build directory with `tar --exclude=.git` and runs the suites out of it, so the tree harness_selftest ran in was not a checkout. The new part asks git two questions about that tree, and outside a repository neither can be answered: five checks went red on a tree with nothing wrong with it. Reported by jdatcmd on commandprompt#855 and reproduced here, both arms from the same tree: the checkout gives 176 PASS / 0 FAIL, the same tree through devloop.sh gave 171 PASS / 5 FAIL. My claim that "every environment this suite runs in is a checkout" was false, and devloop.sh:47 was in a grep I had already run. devloop.sh now writes a one-line gitfile into the build directory. That is 45 bytes against the 32 MB of copying the object database, so the loop stays as cheap as it was, and `rev-parse --show-toplevel` in the staged tree returns the staged tree, so check-ignore reads its own .gitignore. `--absolute-git-dir` rather than "$SRC/.git", because SRC may itself be a linked worktree where .git is a file and that path is not a git directory -- which is exactly the case in the audit container, and it resolves correctly there. Two arms remain, and the second is the one that was misleading: outside a repository `git ls-files` prints nothing, which is indistinguishable from a clean tree, and `check-ignore` answers "not ignored", which reports a rule that is present as missing. Every check in the part now answers `no-repo` instead, so no arm can pass on an empty answer or fail for a reason it does not name. Pointing the part at a non-checkout used to red three premises and silently pass two rule arms; it now reds all eight, each saying `no-repo`. Acceptance: the devloop arm that was 171/5 is 176/0. Red, green and the three rule mutations are unchanged -- 172/4 unfixed, 176/0 fixed, and restoring the tracked file with both ignore rules in place still reds exactly two. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014SNUrAGntEBQXuhcK2qeo9
…ompt#854) Rebasing onto fe1f3a2 moved a number in two of the three places that name it, and left the third alone. The difference is the point. test/selftest/310-... "was tracked at 0e4884c ... renamed in e9de048, 135 commits earlier" STILL TRUE, unchanged. It names the commit it measured at, so the rebase cannot touch it. CHANGELOG.md "renamed 135 commits earlier", unanchored. 146 at the base this now sits on. Rewritten to name both commits. CONTEXT.md "outlived its source by 135 commits", unanchored, in a document read indefinitely. The count is decoration there; the mechanism is the point, so it is gone. Nothing about the defect or the fix changed: the tracked artifact, the two-byte PEP 552 rewrite in cec104f, the ignore rules and the guard are all as they were. This is only the arithmetic that measures a distance between two commits, and the far end moved. Two other counts in the pull request body are measurements of the same tree and have moved with it: 26 tracked .py files is now 27, since commandprompt#851 added test/parquet_stats.py, and compiling by hand now produces 27 .pyc across three __pycache__ directories rather than the twelve entries git status showed against 0e4884c. The population this change removes is unmoved and re-measured: exactly one tracked build artifact at fe1f3a2, and zero after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014SNUrAGntEBQXuhcK2qeo9
ec04f62 to
e46f4be
Compare
jdatcmd
left a comment
There was a problem hiding this comment.
Re-reviewed at e46f4be, rebased onto fe1f3a2. GitHub carried the ec04f62 approval onto this head; this review is of the head. Everything below was measured here.
The rebase, statically:
origin/main (fe1f3a2) is an ancestor of e46f4be yes
conflict markers anywhere none
CHANGELOG: all three entries (#850, #852, #854) 1 + 1 + 1
tracked .pyc / __pycache__ in the tree 0
.gitignore: __pycache__/ and *.pyc 2 of 2
devloop.sh --absolute-git-dir present
The three numbers you re-measured, checked independently:
git rev-list --count e9de048..fe1f3a2 146 (CHANGELOG: "outlived it by 146")
git rev-list --count e9de048..0e4884c1 135 (part 310, still anchored at 0e4884c1)
tracked .py under test/ 0e4884c1: 26 e46f4be: 27 (the 27th is parquet_stats.py)
And the runtime arms, same three as last time:
A out of the checkout checks=186 fail=0 PASSED
B through test/devloop.sh checks=186 fail=0 PASSED
staged .git: gitdir: .../clone855c/.git
C a tree that is not a checkout checks=186 fail=8, six of them answering no-repo
186 is 178 plus your eight, from the run rather than from arithmetic.
The rule your third commit lands on is the right one
Three files carried "135 commits". The one in part 310 says "was tracked at 0e4884c1 … renamed in e9de048, 135 commits earlier" and is still true, because it names the commit it measured from. The two that said "135 commits" with no anchor went stale silently the moment the base moved, and nothing greps prose.
Deleting the count in CONTEXT.md rather than re-measuring it is the better call, and the reason generalises: that document is read indefinitely against a moving tree, so a distance in it restarts its own clock every time someone rebases. The mechanism was the point there; the number was decoration.
This is the same rule as the one that bit me on the mutation table, arriving from the other end. Mine was a count that did not say which suite revision it counted; yours was a distance that did not say which commits it spanned. One rule: a count names the thing it counted, in the text, or it does not appear.
The trap you caught on yourself is worth both of us keeping
git checkout $BASE -- . to inspect a base tree stages that content — it silently reverted your branch and re-added the deleted .pyc. I use git archive <ref> | tar x -C tmpdir for exactly this and will keep doing so; it is read-only and cannot touch the index. Catching it in git status before gating is the difference between a measurement and a mess, and it is the second time today that re-measuring after an edit found the second error rather than the first.
Ten of twelve legs are green. Approving now; merging on the full matrix.
Closes #854.
The defect
test/__pycache__/ste_check.cpython-312.pycwas tracked: 100644, 6,028 bytes,blob
cae72064. Its source,test/ste_check.py, was renamed totest/plain_language_check.pyine9de048— 135 commits earlier. The.pycdid not follow the rename.
It was never usable even as a cache. CPython reads a
__pycache__entry onlywhen the matching source sits beside it, and with
ste_check.pygone the file isnever opened at all; its magic
cb0d0d0ais CPython 3.12's, while the box thatruns these suites is on 3.14.
It had already ridden into an unrelated commit
cec104fis a logical replication fix (#435). Its diffstat:Exactly two bytes differ, at offsets 9 and 10:
That word is the PEP 552 source timestamp. The compiled code is identical on both
sides —
src_size4164 either way. Somebody's checkout re-stamped the source,Python rewrote the header, git recorded a binary diff, and it attached itself to
whatever commit came next. That is the failure mode, and it is not hypothetical.
The population is one, and that was measured
Every other tracked binary is a test fixture — Iceberg
.avro/.puffin,Parquet. One file, not a class of them. But
.gitignorewas nine lines with noPython rule at all, so the count was one by luck:
What this does not claim
A normal harness run regenerates nothing. CPython writes a
__pycache__entry on import, not on
python3 script.py, and no test imports a siblingmodule: zero local imports across the 26 tracked
.pyfiles, and nothing intest/setsPYTHONPATH. This was not breaking the gate.It breaks the tree the moment anyone compiles by hand, which the clean-tree rule
then refuses —
find test -name '*.py' -print0 | xargs -0 python3 -m py_compileleaves 12 untracked
.pycfiles and two untracked directories.The change
.gitignoregains__pycache__/and*.pyc. The.pycis deleted. A newtest/selftest/part fails when a compiled Python artifact is tracked, or wheneither rule goes missing.
The rule is anchored on what git RECORDS, not on what is in the working tree.
"No
__pycache__undertest/" would redden any developer who has just run theinterpreter, and their tree being dirty is not this project's business. What the
repository records is, and it is also the only way to ask the second half of the
question — whether the directory is ignored.
It fails rather than skips outside a checkout. A guard that can be silenced by
deleting
.gitis not a guard, and the two tracked-file arms have no meaningwithout it:
git ls-filesprinting nothing andgit ls-filesfinding nothingwrong are the same output. Every environment this suite runs in is a checkout —
CI checks out with
.git,run_all_versions.shstages withcp -a "$SRCDIR/."which copies it, and that was verified rather than assumed: in a staged copy
git rev-parse --show-toplevelreturns the copy, socheck-ignorereads thestaged tree's own
.gitignore.Red before green, both arms run
The failure names the file rather than counting it:
Four mutations, each isolating one check
.pyc, keep both ignore rules__pycache__/*.pycThe first row is the argument for deleting the file rather than only ignoring it:
.gitignorehas no effect on a file git already tracks, so with both rules inplace the tracked file is still there and still red.
The last row is why the premises exist. On a non-checkout the two tracked-file
checks pass — an empty answer from
git ls-fileslooks exactly like a cleantree. The premises are what refuse it.
A design fault fixed before it was run: my first probe paths were
test/__pycache__/x.cpython-312.pycandtest/x.pyc. The first ends in.pyc,so either rule satisfies it and dropping
__pycache__/reddened nothing. Thedirectory probe is now
test/__pycache__/x.cpython-312.pyc.140234, which onlythe directory rule answers — not a contrived name: CPython writes the compiled
file to
<final>.<id>and renames, so a killed interpreter leaves one.Contents
.gitignore+2,CHANGELOG.md+19,CONTEXT.md+6 (in "Building and running"),the selftest part +103, and one deletion — the
.pyc, whichnumstatprintsas
- -.shellcheck -S error -s bash test/selftest/*.shis clean.No
docs/change, and that is deliberate. Nothing user-facing changes and nodocumented command changes; the contributor-facing rule belongs in
CONTEXT.md,where the build rules already are. Flagging it rather than inventing a docs edit
to satisfy the convention.
Gate
Preflight
build_all_versions.shacross five majors: built 5 of 5, 0warnings, PASSED.
The suite matrix on PG18a and PG19a, assert builds, at
474ae84:harness_selftest=PASSanddocs_style=PASSon both majors, and the new part isconfirmed to have RUN rather than merely been present: PG19's own
harness_selftest.log, read out of the staged matrix build directory, reportschecks run: 176with all eight of the new checks named and passing.Found while cleaning the tree for #853, where my own
rm -rf test/__pycache__deleted this tracked file and the staged
numstatshowed it as- -.🤖 Generated with Claude Code
https://claude.ai/code/session_014SNUrAGntEBQXuhcK2qeo9