fix(list): keep the conflict probe's objects out of the real database - #3884
fix(list): keep the conflict probe's objects out of the real database#3884srobroek wants to merge 7 commits into
Conversation
`WorkingTreeConflictsTask` snapshots each dirty worktree into a temporary index and runs `git write-tree`. The temporary index keeps the user's staging state intact, but `write-tree` materialised its tree — and a blob per non-gitignored untracked file — in the real object database, where nothing ever referenced it. Every invocation whose working tree changed since the last one therefore left unreachable objects behind, so a repo carrying large untracked artifacts grew by their full size per probe until a `git gc --prune`. Anything calling `wt list` on a timer or per prompt (`wt list statusline`) accumulates continuously. max-sixty#3535 already built the mechanism: `GIT_OBJECT_DIRECTORY` pointed at a temporary store with the real database as a read-only alternate, with a docstring noting these probe objects "are never referenced, so writing them to a throwaway store is harmless". It was gated on the object database being read-only. Ungate it for the two observational entry points (`collect()` and `populate_item()`), which is where that reasoning already applied, and rename it `redirect_objects_for_observation` since writability is no longer the trigger. Mutating commands keep the persistent path, so the safety property that scoped the redirect is unchanged. The tradeoff is losing cross-run reuse of these objects: a probe no longer finds a tree an earlier invocation wrote, so identical content is re-hashed. The hash was computed either way — only the object-file write was saved — against growth that was otherwise unbounded. Four tests cover it: staging state is preserved, an unchanged worktree is object-neutral, a changing worktree adds no objects and leaves no unreachable trees, and untracked content never reaches the real database. The last asserts bytes rather than object count, since the count barely moves while the volume climbs.
worktrunk-bot
left a comment
worktrunk-bot
left a comment
There was a problem hiding this comment.
The diagnosis and the mechanism are right, and the tests are good — test_list_keeps_untracked_content_out_of_the_object_database asserting bytes rather than object count is exactly the right assertion for this bug. One measured concern on the tradeoff, and some doc/inventory follow-ups.
The tradeoff is larger than the docstring says, on the hottest path
redirect_objects_for_observation's closing paragraph says cross-run reuse "costs the write either way — the saving was only in the object file". That isn't what git does: write_object_file calls freshen_packed_object/freshen_loose_object first, and check_and_freshen_nonlocal walks the alternates, so when the id already resolves git skips the deflate and the write entirely. Reuse was saving the whole compress-and-write, not an inode.
That matters because it splits the workload in two, and the PR's benchmark only covers one half:
| 20 MB non-gitignored untracked file | before | after |
|---|---|---|
| content changes between runs (the PR's benchmark) | ~600 ms/probe, leaks 20 MB/run | ~600 ms/probe, leaks nothing ✅ |
| content stable between runs | ~600 ms first run, then ~60 ms, leaks 20 MB once | ~600 ms every run, leaks nothing |
The second row is the regression: the temp store starts empty in every process, so a stable untracked artifact is re-deflated and re-written on every invocation instead of resolving through the alternate. Measured below with the probe's own two commands (git add -A --sparse -- . then git write-tree) on git 2.55.0.
The reason I'd want this weighed rather than waved through: statusline_options builds all_columns() with show_full: true, so ColumnKind::Status::required_tasks() pulls in WorkingTreeConflicts — the probe runs on every wt list statusline render, i.e. every prompt redraw. A repo with a large stable untracked file (a dataset, a checkpoint, an asset someone hasn't gitignored) goes from a bounded one-time 20 MB in .git to a recurring half-second of deflate per redraw, per dirty worktree. That's the same repo shape #3883 is about, so it's not a hypothetical corner.
One direction that might get both properties, if it appeals: point the redirect at a persistent wt-owned store (.git/wt/probe-objects/) rather than a fresh TempDir, with the real database as the alternate as now. Probe objects still never enter the real database, but they're reusable across runs, and the existing .git/wt/cache LRU pattern already has the machinery to bound it. Not prescribing it — the current shape may well be the right call once the cost is explicit — but the choice reads differently with the second row on the table.
Measurement
Two fresh repos, one commit, a 20 MB /dev/urandom untracked file that is not rewritten between runs. Each run copies .git/index to a temp index and runs the probe's two commands against it.
--- redirect (this PR), artifact never in the real DB ---
run 1: 661 ms (temp store: 20M)
run 2: 605 ms (temp store: 20M)
run 3: 609 ms (temp store: 20M)
run 4: 607 ms (temp store: 20M)
--- real DB (main) ---
run 1: 607 ms (.git/objects: 20M)
run 2: 61 ms (.git/objects: 20M)
run 3: 62 ms (.git/objects: 20M)
run 4: 61 ms (.git/objects: 20M)
The PR's own table (3 runs, 20 MB untracked artifact: 0 → 61 MB) is ~3 copies, so that artifact changed each run — the top row, where the redirect is free. The bottom row is untested.
Secondary, same mechanism: the probe's blobs now land in $TMPDIR, which on many Linux setups is tmpfs, so untracked content is written to RAM rather than to the repo's disk — and a SIGKILLed render (a statusline timeout) leaves worktrunk-list-objects-* behind with that content still in it.
$TMPDIR/worktrunk-list-objects-* isn't in the file inventory
CLAUDE.md's Data Safety section says "Full inventory: FAQ [What files does Worktrunk create?] … Review new code that changes this surface against those sections." FAQ §5 lists $TMPDIR/worktrunk-temp-index-* and closes with "No files outside .git/, config directories, worktree directories, or the system temporary directory". Before this PR the object store was only created in a read-only checkout; now every wt list and every statusline render creates one, which is squarely a change to that surface. Happy to push the FAQ entry if you want it in this PR.
Two comments the rename left behind
Both outside the diff, so no inline suggestion — say the word and I'll push them:
object_database_path's docstring still describes it as "the store a redirected repository probes for writability and names as its read-only alternate". The writability probe is the code this PR deletes.- In
has_merge_conflicts_by_tree_with_base_sha, the comment on thecommit-treecall reads "The commit is unreferenced and will be GC'd." That assumption is the one #3883 disproves and this PR replaces — the commit now vanishes at process exit instead.
Everything else I checked came back clean: the rename is complete (no stale redirect_objects_if_read_only references anywhere, docs included); no probe-created object id escapes the writing process, since sha_cache stores booleans/LineDiff/MergeProbeResult and the only consumer of MergeTreeOutcome::Clean { tree } string-compares it against commit_to_tree_sha; the shared Arc<RepoCache> between the redirected clone and the original is therefore safe; and the Arc<TemporaryObjectDirectory> keeps the store alive for any pool task outliving collect().
I'm leaving this as a comment rather than an approval on my own judgment — the docstring argues a tradeoff that the measurement above contradicts in one direction, and which way to go on the hot path is your call, which is also what you asked for in the PR description.
Co-authored-by: Worktrunk Bot <w@worktrunk.dev>
worktrunk-bot
left a comment
worktrunk-bot
left a comment
There was a problem hiding this comment.
Ready-for-review pass. My earlier comment on this same commit stands unchanged — the hot-path tradeoff and the FAQ inventory gap are still open and I'm not restating them here. What follows is what draft mode skipped: the overlapping-PR check (no other open PR touches the probe path) and the duplication scan. One finding from that: a test that can't fail.
test_list_does_not_stage_working_tree_changes asserts on a worktree the probe never touches
The test creates untracked.txt inside the feature worktree, then reads the primary worktree's status. Three facts make the assertion vacuous:
TestRepo::add_worktreeplaces the worktree at{temp_dir}/repo.{branch}— a sibling of the repo root, not a subdirectory — so the primary'sgit statuscan never see that file.TestRepo::git_outputruns git withcurrent_dirset toself.root(seeTestRepo::git_command), i.e. always the primary worktree.TestRepo::commitdoesgit add .plus a commit, so the primary is clean — andWorkingTreeConflictsTaskreturns early on!is_dirty, before the temp-indexgit add -Apath. The primary is never probed at all.
So before and after are both the empty string, and assert_eq!(before, after) holds whether or not the probe stages into feature's real index. The half of the mechanism the docstring says this test covers is unasserted. The inline suggestion points it at the probed worktree and pins the dirty precondition so it can't silently go vacuous again.
Reproduction of the layout
Plain git, same shape the fixture builds (commit, then git worktree add ../repo.feature, then write the untracked file into it):
--- git status --porcelain in the PRIMARY worktree (what repo.git_output does) ---
(empty)
--- git status --porcelain in the FEATURE worktree (the one the probe snapshots) ---
?? untracked.txt
The two loose-object walks disagree on failure handling
loose_object_count returns 0 when read_dir fails; the byte-summing walk in test_list_keeps_untracked_content_out_of_the_object_database unwraps instead. The silent 0 is the risky half — assert_eq!(loose_object_count(&git_dir), baseline) in both count-based tests would pass as 0 == 0 if the object database ever became unreadable, which is the shape tests/CLAUDE.md is describing under "A test's setup is part of its proof". The inline suggestion unwraps it, matching the sibling walk.
The two walks also duplicate the hex-fanout filter verbatim. A loose_object_bytes helper beside loose_object_count would remove ~15 lines from the fourth test — say the word and I'll push it, along with the docstring fixes below.
One more for that same doc-fix list (outside the diff, so no inline suggestion): with_temporary_object_directory's docstring still says "the policy — whether to redirect at all — lives in redirect_objects_for_observation". After this change that function holds no policy; it's a straight passthrough, and the policy now lives entirely in which callers are allowed to call it.
The rest of this pass came back clean. No other open PR touches redirect_objects_for_observation, collect(), or the probe path, so there's no duplicate to close. The duplication scan turned up no reinvented internal API — loose_object_count has no existing equivalent in tests/ or src/testing/, and the diff adds no new production function, only the rename. And the redirected clone stays local to both entry points: collect() and populate_item() each shadow repo with the redirected value and neither returns it or stores it in ListData, so no mutating command can inherit a repository whose object writes vanish at exit.
Co-authored-by: Worktrunk Bot <w@worktrunk.dev>
Co-authored-by: Worktrunk Bot <w@worktrunk.dev>
The redirect landed probe objects in a fresh `TempDir` per process, which kept them out of the real database but gave up cross-run reuse. Git skips an object write entirely when the id already resolves, including through an alternate, so an empty-per-run store re-deflates and rewrites unchanged content on every probe. Measured on a stable 20 MB untracked artifact: the temp store pays ~600 ms every run, where the real database paid ~600 ms once and ~60 ms after. Point the redirect at `.git/wt/cache/probe-objects` instead, keeping the real database as the read-only alternate. Probe objects stay out of the store `git gc` and `git fsck` account for, and a repeat probe resolves through the store rather than rewriting: ~330 ms per run against ~390 ms for the leaking baseline, with the real database unchanged at 0 KB. A git dir that cannot be written to falls back to a temporary store. The store sits under `.git/wt/cache/`, so `wt config state cache clear` sweeps it as part of the git commands cache category, which keeps the `state get` ↔ `state clear` parity the module documents. It holds only probe output, so removing it costs nothing but a re-derive. Two tests cover the new behaviour: the store outlives its invocation and an unchanged worktree adds nothing to it, and the store never appears in `git status`. The FAQ file inventory gains the path.
|
The hot-path finding holds. I reproduced it before changing anything. My docstring claim that reuse "cost the write either way" was wrong: git's Measured here on git 2.54.0, stable 20 MB untracked artifact, the probe's own two commands:
So I took the direction you suggested. The redirect now points at Repeat-probe cost is back at baseline with nothing leaked. A git dir that cannot be written to falls back to a temporary store. I put it under Also applied, all of them yours:
Two new tests: the store outlives its invocation and an unchanged worktree adds nothing to it, and the store never shows in
One thing I did not do: bound the store's size. It grows with distinct working-tree states, and Ready for another look. |
worktrunk-bot
left a comment
worktrunk-bot
left a comment
There was a problem hiding this comment.
The persistent store is the right shape, and the three findings from last round are genuinely fixed — the vacuous test now asserts against the probed worktree with a dirty precondition, loose_object_files deduplicates both walks and unwraps, and the FAQ inventory gained the path. Thanks for the measurement table; it settles the tradeoff cleanly.
Three things came out of this pass, one of them a regression on the read-only checkout this mechanism originally existed for. Reproductions below are against a cargo build'd f9e4e09 in a scratch repo, not inference.
A pre-existing probe store defeats the read-only fallback
persistent_probe_object_store uses create_dir_all(...).ok()? as its writability signal, but create_dir_all returns Ok(()) for a directory that already exists whatever its mode. So once .git/wt/cache/probe-objects/{info,pack} exists, the persistent branch is taken unconditionally — including when .git has since become read-only, where the temporary fallback is the whole point. That's the exact guard the deleted tempfile_in(&objects) probe provided, and nothing re-establishes it.
The trigger is the ordinary sandbox sequence rather than a corner: the user runs wt list on their repo while it's writable (creating the store), then the repo is mounted read-only. Same repo, same command, A/B on the store's presence:
.git read-only |
probe store pre-exists | result |
|---|---|---|
| yes | yes | error: insufficient permission for adding an object to repository database …/probe-objects, main_state degrades to null |
| yes | no | clean, main_state: "diverged" |
The failing side is precisely what test_list_full_survives_read_only_object_database asserts can't happen — it just can't see it, because ReadOnlyObjectDirectory freezes only .git/objects and leaves .git/wt writable. I confirmed that test now runs entirely through the persistent branch (7 objects land in .git/wt/cache/probe-objects during it), so ProbeObjectStore::Temporary has no end-to-end coverage at all, and that test's docstring — updated in this PR — still describes the temp store as what the redirect uses.
The inline suggestion probes the store itself the way the old code probed objects. A ReadOnlyObjectDirectory variant that also freezes .git/wt would pin the fallback.
Reproduction
Diverged topology, feature dirty, so both the merge-tree and temp-index write-tree probes run. Warm run while writable populates the store; then every directory and file under .git loses its write bits.
=== CASE A: stale probe store present, .git read-only ===
error: insufficient permission for adding an object to repository database /tmp/ro2/repo/.git/wt/cache/probe-objects
error: insufficient permission for adding an object to repository database /tmp/ro2/repo/.git/wt/cache/probe-objects
{"main_state":null,"main":{"ahead":1,"behind":1,"diff":{"added":1,"deleted":0}}}
=== CASE B: same repo, probe store removed first, .git read-only ===
write errors: 0
{"main_state":"diverged"}
And the shape of the create_dir_all result the branch turns on, on an existing directory with every write bit cleared:
create_dir_all(info) on existing read-only dir -> Ok(())
create_dir_all(pack) on existing read-only dir -> Ok(())
write into store -> Err(Os { code: 13, kind: PermissionDenied })
state get ↔ state clear parity is broken, not intact
clear_probe_objects's docstring says reporting under the git commands cache "keeps the state get ↔ state clear parity intact", but only the clear side was wired up. render_git_commands_section and the git_commands_cache JSON field still compute sha_cache::count_all + preview_cache::count_all, so the store is removed by a category that never counted it:
$ wt config state cache get --format=json | jq '{git_commands_cache}'
{ "git_commands_cache": 14 }
$ wt config state cache clear
✓ Cleared 15 git commands cache entries
That's the MUST at the top of the same file — "A user should never be able to run state clear and have something disappear that state get never mentioned." The store held 7 objects at that point, so neither number describes it either.
Two smaller things in the same function. clear_probe_objects returns 1 whenever the directory exists, not "when it held anything" as the docstring says — an empty store reports ✓ Cleared 1 git commands cache entry. And since redirect_objects_for_observation creates the directory unconditionally on every wt list, even one that writes nothing into it, that 1 is present in essentially every repo where wt list has run. The get side is outside the diff, so no inline suggestion — say the word and I'll push a count_probe_objects beside the clear, counting loose objects on both sides so the two numbers agree.
Two docstrings merged into one
with_probe_object_store has no doc comment; both its intended block ("Build a clone whose object writes are redirected…", plus the mechanism-vs-policy paragraph) and persistent_probe_object_store's ("The persistent probe object store at .git/wt/cache/probe-objects…") are stacked above persistent_probe_object_store. Inline suggestion moves the first block onto the function it describes.
Two notes, neither blocking.
The store's reclaim path changed, and the docstring reads more reassuring than the PR comment. "It grows with distinct working-tree states rather than without bound" — distinct working-tree states over a repo's life aren't bounded, which your own comment concedes ("A long-lived repo with a large churning artifact still accumulates"). Worth saying plainly there, because what also changed is who reclaims: unreachable objects in .git/objects were eventually collected by git's own auto-gc after the two-week prune grace, whereas nothing reclaims .git/wt/cache/probe-objects except an explicit wt config state cache clear. For the repo shape #3883 is about, the per-probe disk growth is unchanged in the steady state; it moves somewhere git gc can't see. That's still a clear improvement (the database git fsck and git gc account for stays clean, and repeat probes are cheap again), but the bound reads like a follow-up that matters rather than an optional one. Note cache::sweep_lru won't drop in as-is — it counts top-level .json files, and this is a two-level fanout.
Naming rot. TemporaryObjectDirectory, the temporary_object_directory field, and the |temporary| binding in object_store_environment now all name the fallback rather than the normal path — the one thing this PR changed. Happy to push a rename to ProbeObjectRedirect / probe_object_store along with the doc fixes above.
On the data-safety surface: this diff adds std::fs::remove_dir_all, but everything under its target is regenerable probe output, so it doesn't reach the "what a user can't get back" class and I didn't treat it as a hold. Flagging the assessment in case you read it differently.
I'm leaving this as a comment rather than an approval on my own judgment — the read-only regression is reproducible and reverts a behavior this code path was added for.
…olds Three fixes from review. **A pre-existing store defeated the read-only fallback.** `persistent_probe_object_store` used `create_dir_all(...).ok()?` as its writability signal, but `create_dir_all` returns `Ok(())` for a directory that already exists whatever its mode. Once the store existed, the persistent branch was taken unconditionally, including after the repo became read-only, where every probe then failed with `insufficient permission for adding an object`. That is the ordinary sandbox sequence: run `wt list` while writable, then mount read-only. Restore the write probe the ungating deleted, against the store rather than the object database, so it answers the question the branch actually turns on. **`state get` and `state clear` disagreed.** Only the clear side counted the store, so it was removed by a category that never counted it, which is the MUST at the top of `commands/config/state.rs`. Add `count_probe_objects` and feed it into the JSON field and the rendered section as well. Both sides now count loose objects rather than the directory's existence, since `wt list` creates the directory even when no probe writes into it; an empty store previously reported one cleared entry in essentially every repo. **Two docstrings had merged into one.** `with_probe_object_store` had no doc comment: both its block and `persistent_probe_object_store`'s were stacked above the latter. Split them onto the functions they describe. Also rename `TemporaryObjectDirectory` to `ProbeObjectRedirect` and its field to `probe_object_redirect`, since "temporary" now names the fallback rather than the normal path, and say plainly in the docstring that the store is not bounded and that nothing except `wt config state cache clear` reclaims it — git's auto-gc used to collect the equivalent objects. Three new tests: the redirect prefers the persistent store, a read-only git dir falls back, and a pre-existing read-only store still falls back. The last fails without the write probe. Two more cover the clear path for a populated and an absent store.
worktrunk-bot
left a comment
worktrunk-bot
left a comment
There was a problem hiding this comment.
The read-only regression from last round is genuinely fixed, and the fix is the right shape: create_dir_all can't answer "can git write here?", so probing the store the way git's own writers do is the only thing that can. a_pre_existing_probe_store_still_falls_back_when_read_only pins exactly the sequence that broke, and the parity is now real on both sides rather than only on the clear side. Nothing from the previous review is still open.
Four things came out of this pass, all on the new commit. None is a shipped-behavior bug — the top two are about whether the invariant this commit establishes survives the next change to it.
The probe store's path is now hard-coded in two production modules, with nothing tying them together
persistent_probe_object_store builds self.wt_dir().join("cache").join("probe-objects"); state.rs's new probe_object_store builds repo.wt_dir().join("cache").join("probe-objects"). They are the same three components typed twice, in modules that don't reference each other.
That is load-bearing in a way the old code wasn't. Before this commit clear_probe_objects was the only consumer, and a divergence would have shown up as "clear stops clearing". Now count_probe_objects feeds state get as well, so a rename on the Repository side leaves both sides of the parity pointing at a directory that no longer exists: state get reports 0, state clear removes nothing and reports nothing, and the store grows forever with no user-visible way to reach it. The failure is silent in both directions, and it defeats the same MUST — "a user should never be able to run state clear and have something disappear that state get never mentioned" — from the other end. No test would catch it either: test_state_cache_clear_removes_probe_objects and test_probe_store_persists_between_invocations each spell the path out a third and fourth time, so they'd follow whichever side the writer remembered.
A pub fn probe_object_store(&self) -> PathBuf on Repository, called by persistent_probe_object_store and by state.rs, collapses it to one definition. Happy to push that if you want it here.
The number this commit exists to make agree is never asserted
test_state_cache_clear_removes_probe_objects asserts the directory is gone. That was already true before this commit. Nothing asserts the count on either side, so both halves of the change are unpinned: restoring Ok(1) in clear_probe_objects, or deleting + count_probe_objects(repo) from all three git_commands_cache call sites, leaves every test in this PR green. The nearest existing coverage is the git_commands_cache: 1 in the state get snapshot at config_state.rs, and that 1 is a seeded merge-tree-conflicts entry — the probe store isn't in that fixture at all.
The inline suggestion adds the get-side assertion to the test that already seeds a store, which is the cheap place to put it.
Four comments still describe the redirect as writing to a temporary object database
The type and field renamed, but the prose at the call sites didn't. All four now describe the fallback as if it were the normal path, which is the one thing the persistent-store commit changed:
collect()'s comment — "Redirect them into a temporary object database", and itsA None (no writable temp dir)clause, which now means neither store could be createdpopulate_item()'s mirror commentTempIndex::object_store_environment's doc — "into the temporary store", withNone on the normal persistent pathnow reading against the grain, since the persistent store is the normal redirect targettest_list_full_survives_read_only_object_database's docstring — "reroutes into a temporary object database"
That last one is wrong twice over: ReadOnlyObjectDirectory freezes .git/objects and leaves .git/wt writable, so the test runs entirely through the persistent branch. Its inline comment further down — "the integration probe wrote to the temporary store" — says the same thing and is outside the diff, so no suggestion; say the word and I'll push it with the rest.
The new permission tests skip the repo's root guard
redirect_falls_back_to_a_temporary_store_when_the_git_dir_is_read_only and a_pre_existing_probe_store_still_falls_back_when_read_only both turn on set_mode(0o500) actually denying a write. Under root it doesn't, so persistent_probe_object_store's new probe succeeds, the persistent branch is taken, and both assertions fail — a false red rather than a skip. The repo already treats this as a real environment: tests/integration_tests/remove.rs and approval_save.rs both write a probe file first and return with "Skipping - running with elevated privileges" when it lands. Worth mirroring, particularly since dev/codex.sh targets a container. GitHub's runners are non-root, so this won't turn CI red as things stand.
Everything else checked out. count_probe_objects's two-hex fanout filter excludes the new .worktrunk-write-probe-* file and the info/pack directories, so the probe doesn't inflate its own count; the probe NamedTempFile is dropped at the end of its statement, so the file doesn't survive the call; clear_probe_objects still short-circuits on a missing store, so the count is only taken when there's something to count; and the empty-but-existing store now reports nothing while still being removed, which is consistent with state get reporting 0 for it. The rename is complete — no temporary_object_directory or TemporaryObjectDirectory references remain. No other open PR touches the probe path.
On the data-safety surface: unchanged from last round — remove_dir_all's target holds only regenerable probe output, so I didn't treat it as a hold. Flagging the assessment again in case you read it differently.
I'm leaving this as a comment rather than an approval on my own judgment, and it's a weaker hold than last time: the correctness finding is closed and what's left is that the parity invariant rests on two independently typed copies of one path with no test on the resulting number. Both are cheap to close, and I'd rather not approve the mechanism in a state where the next rename breaks it quietly.
|
All four applied. The second was the most useful. My test asserted the directory disappeared, which was already true, so nothing pinned either half of the parity fix. I checked the new assertion by unwiring
One thing that suggestion led me into: I copied the skip pattern from
Two flakes worth naming, both load-sensitive and unrelated to this diff. Each passes alone and fails only in a fully parallel run:
I mention them because I cannot claim five clean runs, not because I think this diff caused them. If it is useful, I can file them separately. Still open by choice: the store is unbounded. The docstring says so plainly, along with the fact that only |
c770695 to
1edbc93
Compare
|
Correction on the previous comment: the Fixed and force-pushed. Verified this time with |
|
You've already got the diagnosis and the fix, so just the part that outlives it: the thing that steered you into Both forms are correct, in the same repo, for different crates.
On the flakes: worth filing separately. I searched issues in all states for both test names and found nothing, so neither is tracked. One data point for the issue: neither surfaced in CI on this branch — The unbounded store reads fine as a follow-up from here: the docstring states the growth and names the only reclaim path, and your finding that The review pass over |
worktrunk-bot
left a comment
worktrunk-bot
left a comment
There was a problem hiding this comment.
All four findings from last round landed, and the second one landed the right way — I unwired count_probe_objects from the three git_commands_cache call sites and test_state_cache_clear_removes_probe_objects now fails with the number in the message, which is exactly what the old directory-existence assertion couldn't do. The guards mirror remove.rs, the four comments now describe the persistent store, and probe_object_store_path gives the path one definition.
You force-pushed the crate::styling:: fix while this pass was running, so the E0433 that had lint / test (linux) / test (macos) red on c770695 is closed — I'd reproduced it and confirmed crate:: was the resolution, and 1edbc93 is what I re-reviewed. One repo-level thing came out of it that is worth fixing separately, and I'll offer that rather than fold it in: check_stderr_macros_come_from_styling's failure message says "Import the macro from worktrunk::styling, qualify the call as worktrunk::styling::eprintln!(…)". That form is right for src/commands/, src/cli/ and src/main.rs — the wt binary, where worktrunk is a real dependency — and can't compile in any of the files the same scan covers under src/git/, src/config/, src/shell*/, src/trace/, which are the lib crate and have to say crate::styling::. The guard named a fix that doesn't build in the file it fired on. Say the word and I'll push a PR widening that message.
codecov/patch is red at 91.72%, and none of the 12 misses is reachable
Handing over the arithmetic since CLAUDE.md → Coverage makes this a merge gate. Patch is 145 lines, 133 hit, 12 missed against a 98.12% target — passing would need at most 2 misses (98.62%), so effectively all 12 would have to be covered. Every one is in src/git/repository/integration.rs, and each is unreachable by construction rather than untested:
| lines | what | why it can't be hit |
|---|---|---|
| 1357–1360, 1425–1430 | the two root-guard early-return bodies | the guard exists because the branch is only taken under elevated privileges; GitHub's runners are non-root, so the skip never fires |
| 1323, 1377 | directory.display() / store.display() |
assert! failure-message arguments, evaluated only when the assertion fails |
The structural reason the guards cost anything here: codecov measures src/ only — no tests/ file appears in the compare at all — so the identical pattern in tests/integration_tests/remove.rs and approval_save.rs is free, while a #[cfg(test)] module inside src/ puts it on the patch. Relocating these two tests isn't free either, since object_store_environment is pub(super). So I don't think there's a version of the guard that is both correct and green, and I'd rather see the guard than the 6.4 points.
That's a maintainer call rather than something I'd resolve in review. Flagging it as the arithmetic, not as a request to chase the number.
Two reuse points, neither blocking
probe_object_store_path is the right shape and closes the finding; the definition inside it is still the third hand-rolled copy of .git/wt/cache/<kind>. cache::cache_dir(repo, kind) is literally repo.wt_dir().join("cache").join(kind), and owning that join is the stated job of src/cache.rs — "this module only owns the filesystem mechanics so those rules have one implementation instead of three". This is the substance of the earlier thread on state.rs, which the move made outdated rather than answered; raising it once here because the line is new, not to re-argue it. crate::cache::cache_dir(self, "probe-objects") compiles from repository/mod.rs and leaves your docstring's guarantee intact, with the cache module as the single spelling instead of a second one.
strip_ansi reimplements anstream::adapter::strip_str. anstream is a direct dependency and the repo already strips this way in src/trace/profile.rs (in a test, same use as here) and src/commands/list/progressive_table.rs, whose module docs call it "the exact transform an anstream printer applies". The hand-rolled version also stops at the first ASCII-alphabetic byte after ESC, which is right for CSI and wrong for OSC (ESC ] … BEL), so it's a narrower tool than the one already in the tree. Swapping the call site and deleting the helper drops 18 lines; the test passes unchanged with it.
Nothing else on this pass. The get-side assertion can't false-pass — Cleared 11 … doesn't contain Cleared 1 git commands cache entry, so a wrong count fails rather than substring-matches. Both root guards restore permissions before returning, and the second one's move of originals is confined to the early-return branch. probe_object_store_path needs pub rather than pub(crate) because state.rs is in the wt binary crate. Data-safety surface unchanged from last round: remove_dir_all's target holds only regenerable probe output, so I didn't treat it as a hold.
Holding the approval only on codecov/patch being red — CLAUDE.md is explicit that merging over it needs your say-so, and I'd rather hand you the line list than approve past it. The two suggestions below are yours to take or leave; neither changes my read of the mechanism, which I think is right.
1edbc93 to
fcbc629
Compare
|
Pushed a test for the It asserts the store path is not the persistent one and that the real database is still the read-only alternate, and it carries the same root guard as the sibling permission tests.
|
…ount Four fixes from review. **The store path was spelled twice in production.** `persistent_probe_object_store` and `state.rs` each built `wt_dir()/cache/probe-objects` independently, in modules that do not reference each other. That became load-bearing once `count_probe_objects` fed `state get`: a rename on the `Repository` side would leave both sides of the parity pointing at a directory that no longer exists, so `state get` reports 0, `state clear` removes nothing, and the store grows with no user-visible way to reach it — the same MUST defeated from the other end. Add `Repository::probe_object_store_path` as the one definition. **The number this change exists to make agree was never asserted.** `test_state_cache_clear_removes_probe_objects` only checked that the directory disappeared, which was true before the change too. Restoring `Ok(1)` in `clear_probe_objects`, or dropping `count_probe_objects` from the three `git_commands_cache` call sites, left every test green. Assert the get-side count and the clear-side report in the test that already seeds a store; both now fail if either half is reverted. **Four comments still described the redirect as writing to a temporary object database**, which after the persistent-store commit describes the fallback rather than the normal path: both `collect()` and `populate_item()` call sites, `TempIndex`'s object-store doc, and `test_list_full_survives_read_only_object_database`. That last was wrong twice, since `ReadOnlyObjectDirectory` leaves `.git/wt` writable and the test runs entirely through the persistent branch. **The two permission tests skipped the repo's root guard.** Under root `set_mode(0o500)` does not deny the write, so the persistent branch is taken and both assertions fail — a false red rather than a skip. Probe first and return with "Skipping - running with elevated privileges", as `tests/integration_tests/remove.rs` does.
|
So the new test adds a third copy of that guard — The one thing it asserts that the existing fallback test doesn't is the alternates equality. It doesn't turn the gate green either way. That part is unchanged from last round: every remaining miss is a root guard or an assert-message argument, and whether that's worth merging over is @max-sixty's call, not a number to chase. MeasurementCodecov compare API, both heads against the PR base Missed patch lines at
API=https://api.codecov.io/api/v2/github/max-sixty/repos/worktrunk
curl -sL "$API/compare/?base=ad62f2ad81a68d738ec05ad2093aaf5fee6d0118&head=fcbc62962078548da9d42bfdec1f616d64056182"
|
fcbc629 to
9775aec
Compare
|
Both reuse points applied. The coverage arithmetic changed what I did rather than confirming it: my previous push added a
On the patch number, I took two of the twelve without touching the guards. That leaves the six guard-body lines. I reach your conclusion for two reasons:
I would rather keep the guard than the points, so I am flagging this for the maintainer rather than chasing it further. Two follow-ups I have not done, both yours to take or leave:
|
|
One correction to your count: it's ten guard-body lines, not six. The two survivors aren't the same length — the leftover-store test restores a map, so its guard runs six lines to the fallback test's four. The full remaining miss listAll ten, all in
Per-file patch totals: API=https://api.codecov.io/api/v2/github/max-sixty/repos/worktrunk
curl -sL "$API/compare/?base=ad62f2ad81a68d738ec05ad2093aaf5fee6d0118&head=9775aecf521778c6ce1accaee0e723291636a325"
So the gate is red on nothing but the two skip paths, which is @max-sixty's call as you say — I don't think there's a version of this that both keeps the guards and clears 98%. On the guard message — took it, as #3888. Sent separately rather than folded into #3878: that one is green and merge-ready, and a push would have restarted its whole matrix to carry a string change. They edit different hunks of the same file, so whichever merges second may want a trivial rebase. It widens both guards, not just the one that caught you — |
worktrunk-bot
left a comment
worktrunk-bot
left a comment
There was a problem hiding this comment.
All four of last round's points landed, and the two I'd raised as reuse are now single-definition: probe_object_store_path is crate::cache::cache_dir(self, "probe-objects") and state.rs calls that method rather than spelling the join again, so cache_dir is the one place .git/wt/cache/<kind> exists. Merging a_temporary_probe_store_reports_its_own_path into the fallback test was the right call over keeping it — the alternates assertion is the only thing it added, and it now sits in the test that already builds the read-only fixture.
I re-reviewed the whole diff at 9775aec rather than the increment, since the force-push rewrote the commit I'd read. Nothing new came out of it. The redirect still escapes neither entry point (collect() and populate_item() both shadow repo and neither stores it; the picker's collect::collect(&bg_repo, …) passes by reference, so do_removal keeps the real database), worktree_at borrows rather than reconstructing so TempIndex still inherits the store, and the fallback covers the cases that reach it — a store created by another user under core.sharedRepository, a probe-objects path occupied by a file, and the read-only sequence the last round's finding was about.
codecov/patch at 93.15%, and the remaining misses are now only the guards
test (linux), test (macos), test (windows), lint, code-coverage and codecov/project are all green at this head — the E0433 and the two flakes you named are behind it. codecov/patch is the one red, and the arithmetic moved in your favour without reaching the target:
| patch lines | hits | misses | patch | |
|---|---|---|---|---|
1edbc93 (last round) |
145 | 133 | 12 | 91.72% |
9775aec |
146 | 136 | 10 | 93.15% (target 98.12%) |
The {directory:?} change closed both .display() lines and merging the duplicate test dropped one guard, so the ten that remain are exactly the two root-guard bodies and nothing else — integration.rs 1356–1359 and 1428–1433, both remove_file / set_permissions / eprintln! / return. There is no third category left to trim, and 98.12% would need ≤ 2 misses, so closing the gap means deleting a guard rather than covering a line. I reach the same conclusion you did: keep the guard.
@max-sixty — that makes this a decision rather than a number to chase. CLAUDE.md → Coverage is explicit that merging over a red codecov/patch needs your say-so, so I'm holding the approval on that alone; the mechanism reads correct to me and every platform test is green.
Measurement
API=https://api.codecov.io/api/v2/github/max-sixty/repos/worktrunk
curl -sL "$API/compare/?base=6aad9e17d9b53a0e2ea5a74596cc76c6b94521a1&head=9775aecf521778c6ce1accaee0e723291636a325"Every missed patch line at 9775aec, summed across the five files with diff coverage (state.rs 30/30, collect/mod.rs 2/2, repository/mod.rs 33/33, integration.rs 71/81):
1356 let _ = std::fs::remove_file(git_dir.join(".write-probe"));
1357 std::fs::set_permissions(&git_dir, original).unwrap();
1358 crate::styling::eprintln!("Skipping - running with elevated privileges");
1359 return;
1428 let _ = std::fs::remove_file(store.join(".write-probe"));
1429 for (path, permissions) in originals {
1430 std::fs::set_permissions(&path, permissions).unwrap();
1431 }
1432 crate::styling::eprintln!("Skipping - running with elevated privileges");
1433 return;
2 participants
Fixes #3883.
Problem
WorkingTreeConflictsTasksnapshots each dirty worktree into a temp index and runsgit write-tree. The temp index keeps the user's staging state intact. Butwrite-treematerialises its tree, plus a blob per non-gitignored untracked file, in the real object database. Nothing references them, so every invocation over a worktree that changed since the last one leaves unreachable objects behind.Each probe copies every non-gitignored untracked file into the database, so a 20 MB untracked artifact adds 20 MB per invocation.
gc.autodoes not bound it: nowtsubcommand callsgc --auto, and a later auto-gc packs the unreachable objects rather than dropping them.Approach
#3535 added
GIT_OBJECT_DIRECTORYredirection: a temp store with the real database as a read-only alternate. A check for a read-only database gates it. Its docstring already covers this case: these probe objects "are never referenced, so writing them to a throwaway store is harmless".This ungates it for the two observational entry points,
collect()andpopulate_item(), and renames itredirect_objects_for_observation, since writability does not decide it. Mutating commands keep the persistent path, which preserves the property that scoped the redirect: a redirected commit would vanish at exit.writable_object_database_is_not_redirectedasserted the old policy, so this inverts it. That inversion is the behavior change, and making it explicit seemed better than leaving the test green on a technicality.Tradeoff: probes lose cross-run object reuse, so git re-hashes identical content. It computes that hash either way, so the redirect costs only the object write.
Testing
New
tests/integration_tests/list_object_churn.rs, 4 tests:The last two fail on
mainand pass here. I checked by reverting onlysrc/and re-running.cargo test --test integration: 2005 passed, 0 failed, 1 ignoredcargo test --lib: 1516 passed, plus 11 in the second binarycargo clippy --all-targets,cargo fmt --check: cleanEnd to end against a repo with 3 dirty worktrees, using a binary built from this branch:
wt listoutput does not change. Built with Rust 1.97.1; the crate requires 1.96.The policy change is yours to accept. An agent wrote this.
Reported and fixed with Claude Code.