Skip object database lookups for the null id when diffing - #15746
Conversation
Worktree-side resources reach the diff platform with the null id. With core.autocrlf=input (or text=auto), the end-of-line filter looks that id up to compare against the index version, the lookup misses, and gix rescans the pack directory on every miss. In a repository with hundreds of packs this made `but status` spend seconds in stat() for a couple of hundred changed files. The repository is now wrapped so null-id lookups answer immediately.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped to object lookup behavior for null IDs, matches the described root cause, and does not appear to alter diff output semantics for non-null objects.
Pull request overview
This PR improves but status performance by avoiding expensive object database refresh work triggered by lookups of the null object id during worktree diff generation in but-core.
Changes:
- Wraps
gix::Repositorywith a smallFind/FindHeadershim that immediately returns “not found” for null OIDs. - Routes diff-platform object lookups through that shim when setting diff resources, preventing ODB rescans on guaranteed misses.
File summaries
| File | Description |
|---|---|
crates/but-core/src/unified_diff.rs |
Adds a NullIdIsMissing Find/FindHeader wrapper and uses it in UnifiedPatch::compute_with_filter() to short-circuit null-id lookups during diff preparation. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Byron
left a comment
Byron
left a comment
There was a problem hiding this comment.
That's quite a footgun, particularly because ultimately I ran into it I suppose. Regarding NullIds being looked up, I will take a look at that in gix as my understanding is that these already serve as marker to say such resources should be looked up in the worktree.
GitoxideLabs/gitoxide#2853 contains many improvements, and one of them is that after the first pack refresh, it will have a cooldown during which it won't trigger a refresh again, effectively neutering this foot gun.
Let's merge this and see if I can ship a better fix with #15676 as well, after all I am still puzzled why this lookup on null happens at all.
|
Holding off on merging this until I've got my performance benchmarks up and runng. I think I've got a working repro of the performance characteristics of this PR in a controlled environment. Will try to get the benchies running today so we can merge probably tomorrow. |
|
This is made obsolete by #15676 |
The performance story
but statustook 5.4 seconds in a sandbox clone of this repository with ~240 changed files, whilegit statustook 0.09s in the same checkout. Sampling the process showed 85% of wall time insidegix_odb::Store::consolidate_with_disk_state: readdir plus astat()of every file in.git/objects/pack, repeated once per changed file. That checkout has 555 packs, so each rescan touched ~1900 files.The chain that gets there:
filter_from_stateonly setsnew_rootwhen the id is null).core.autocrlf=input(or atext=autoattribute), gix's end-of-line filter runs in auto-text mode and asks the object database for the index version of the file, using that id, to decide whether CRLF conversion is safe.RefreshMode::AfterAllIndicesLoadedmakes gix rescan the pack directory in case a new pack appeared.So the cost is roughly
changed files × pack filesinstat()calls, all system time, and it scales with how fragmented the object store is.core.autocrlf=inputis a common global setting on macOS and Linux, so this is not specific to the sandbox.The fix
compute_with_filternow wraps the repository in a smallFind/FindHeadershim that answers null ids with "not found" without consulting the object database. Every other lookup passes through unchanged. The desktop app's worktree diffs share this code path, so it benefits as well.The proper long-term fix belongs in gix-diff's
convert_to_diffable, which should skip the lookup when the id is null; this shim is harmless to keep either way.Benchmark
Sandbox: gitbutler clone, 555 packs, 1852 refs, 238 uncommitted files,
core.autocrlf=input. Debug build, warm cache, best of three.but status --jsonOutput is byte-identical before and after. What remains is graph traversal over the sandbox's 36k commits and ordinary diffing.