Repository navigation
fix: discover separate-git-dir checkouts on first run - #90
Conversation
First-run discovery kept only folders whose .git is a folder, taking every .git file for a linked worktree. A checkout made with --separate-git-dir has a .git file too, but its git folder is its own: it has no commondir. Count a .git file whose gitdir: target holds HEAD and no commondir. The file is read only when it is a regular file of at most 64 KiB, so a pipe or device is never opened, and a missing, malformed or non-UTF-8 file is skipped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@sfl review |
There was a problem hiding this comment.
✅ Verdict: PASS • Round 1
Confidence: 8/10
Review done by claude-opus-5-5 (same family: same-family: codex gated: codex Weekly 163% SUS 61 🔴 pause, defer)
The PR changes first-run discover so it also counts a checkout whose .git is a regular gitdir: file under 64 KiB, pointing at a git folder that has HEAD and no commondir. Linked worktrees, pipes, oversized, malformed, non-UTF-8 and missing-target files are still skipped, and new storage tests cover each case. I found no evidenced defect in the diff. The only limit on merging now is that CI is still running.
🔨 Full-Spectrum Review Summary
Effort: high
Review time: 1m 39s
Token count: Unavailable
| Pass | 🔴 Critical | 🟠 High | 🟡 Medium | ⚪ Low |
|---|---|---|---|---|
| 🔒 Security | 0 | 0 | 0 | 0 |
| ✅ Accuracy | 0 | 0 | 0 | 0 |
| 🧰 Quality & Maintainability | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 |
Key Findings
No material defects found.
Coverage
- Files reviewed:
crates/dashboard-core/src/config.rscrates/dashboard-core/tests/storage.rs
- Context read:
AGENTS.mdREADME.mdCargo.tomlCargo.lockcrates/dashboard-core/Cargo.tomlcrates/dashboard-core/src/config_atomic_73.rscrates/dashboard-core/tests/no_child_process.rsissue #88PR #90 metadata
- Verified:
- Compare
base_commit.shaisaf1436057f950b0d704a5941e7690c27fc8a210e, the last commit is36dacbbec47d1bebd93aee55669a8af006d782ac, the merge base equals the base, ahead_by 1, behind_by 0 - PR #90 head and base SHAs match the reserved SHAs
- Only two files changed:
config.rs(+32/-3) andstorage.rs(+82) is_primary_checkoutcallsmetadata(which follows symlinks) and rejects non-regular files beforeopen, so the FIFO case in the test is never read- The
MAX_GIT_FILEsize check plustake(MAX_GIT_FILE)bounds the read; a non-UTF-8 file returns an error fromread_to_stringand is skipped - The
gitdir:prefix match and joining a relative target to the checkout folder match how git reads a gitfile - A target with
commondir(linked worktree) is excluded; a target with noHEADor a missing target is excluded - Test expectation
[plain, relative, separate]is sorted and matches the rule;../../gitdirs/rel.gitresolves togitdirs/rel.gitin the scratch folder - The test's
rustixuse is allowed because the Linux target dependencyrustix =1.1.5withfsis present in thedashboard-coremanifest and lock - No
Command/Stdiouse was added tosrc, so theno_child_processcheck is unaffected - The
discoverdoc comment was updated to the new rule, as issue #88 requires - The existing first-run test's worktree case is still excluded under the new rule (its target is missing)
- Compare
- Not verified:
- Compilation, clippy and fmt: CI check runs
coreandruston both OSes were in_progress at review time rustixmknodatandModeFrom<u32>signatures in 1.1.5 were inferred from API knowledge, not read from source- Windows behavior of joining a forward-slash relative
gitdirtarget - Whether
pmd::checkout_identityreads separate-git-dir checkouts: the claim comes from issue #88, and that code was not read
- Compilation, clippy and fmt: CI check runs
- Tests run: None. Reviewer execution is disabled.
HemSoft full-spectrum review • 3 evidence-based passes • Reviewed head 36dacbbec47d1bebd93aee55669a8af006d782ac • SFL v0
Risk level: 2/10 (project-dashboard)
Worked on by: claude-opus-5-5 on Laptop, for initiating Home70828f7a
Summary
First-run discovery treated every
.gitfile as a linked worktree, so it missed a checkout made withgit clone --separate-git-dir, even though the dashboard reads that checkout's identity correctly.config::discover(crates/dashboard-core/src/config.rs) now also counts a.gitfile whosegitdir:folder holdsHEADand nocommondir. Real git 2.55 matches this rule: a separate git folder hasHEADand nocommondir, and a linked worktree's git folder has both.commondir..gitfile is read only when it is a regular file of at most 64 KiB. A pipe, device or oversized file is never opened. A missing, empty, malformed or non-UTF-8 file, or a target withoutHEAD, is skipped. There is no panic, no git process and no new dependency.Only
discoverand its tests change. Link checkout (#87) is untouched.Acceptance Criteria
.gitfile points at a git folder withoutcommondiris discovered:first_run_discovers_separate_git_dir_checkouts_but_not_worktrees_or_broken_git_filesfindsseparate(absolutegitdir:) andrelative(gitdir: ../../gitdirs/rel.gitwith CRLF), through bothconfig::discoverandConfig::load_or_create_in.plain-pr-1, whose git folder holdsHEADandcommondir, socommondiris the only difference fromseparate. It also hasgroup/deep. The existingfirst_run_discovers_checkouts_one_level_under_the_root_and_writes_themis unchanged and passes. It coversnotesandalpha.worktrees/pr-1..gitfile, one without agitdir:line, or one pointing at a missing folder is skipped without panic: the same test hasmissing,no-head,malformed,empty,not-utf8,huge(a valid first line padded past 64 KiB) and, on Linux,pipe, whose.gitis a FIFO that would block a read.discovery_of_a_missing_root_or_a_file_is_emptycovers a missing root and a root that is a file.no_production_source_starts_a_child_processpasses. The test asserts the exact sorted list[plain, relative, separate].discoverdoc comment matches the new rule: updated, andis_primary_checkoutdocuments the rule.Definition of Done
dashboard-core, with no git needed in tests: two new tests incrates/dashboard-core/tests/storage.rs. Negative control: against main'sdiscover, the new test fails, finding only[plain]instead of[plain, relative, separate].36dacbbin CI 42265803779. Local full workspace builds were omitted to preserve the shared build slot.36dacbb, no findings and zero unresolved threads.Validation
At head
36dacbbec47d1bebd93aee55669a8af006d782ac, on Mini (Linux), from agit archiveof that commit:cargo fmt --all --check: passcargo clippy --workspace --all-targets --locked -- -D warnings: passcargo test --workspace --locked: pass (dashboard-app 184 passed, 14 ignored; every dashboard-core suite passed, includingstorage12/12 with the FIFO case, andno_child_process1/1)cargo deny --locked check advisories:advisories okOn Laptop (Windows 11), at the same source:
cargo fmt --all --checkpassed, andcargo test -p dashboard-core --locked --test storage --test no_child_processgave storage 12/12 and no_child_process 1/1. The FIFO case is Linux-only, so the Linux run above exercises it.The fixtures model git's layout, checked with git 2.55:
git init --separate-git-dir <dir>/sep.git separatewritesseparate/.git=gitdir: <dir>/sep.git, and that folder hasHEADand nocommondir.git worktree add ../separate-wtwrites agitdir:pointing atsep.git/worktrees/separate-wt, which hasHEADandcommondir.No screenshots: this changes only which folders the first-run
config.tomllists. No view, layout or text in the window changes.Risks and deferred work
config.tomlis never rescanned.Closes #88
🤖 Generated with Claude Code
All five required CI jobs passed at
36dacbb, including every executed step. The separate packaging run is in_progress; it adds no required status context.