fix: reject an existing bare repository as an init destination - #29
Conversation
`initialize_repository` guarded against an existing repository two ways, and a bare one slipped between them: it has no `.git` child, and `rev-parse --is-inside-work-tree` answers `false` inside it. The destination is non-empty, so the user got the "folder is not empty" prompt rather than "already a Git repository" — and on confirming, `git init` ran with the bare repo as its working directory. Verified against real git: it prints "Initialized empty Git repository in .../bare.git/.git/" and creates a nested repository inside the bare one. The bare repo's own HEAD, refs and core.bare survive untouched — `diff -rq` against a backup shows only the added `.git` — so this is a wrong state rather than data loss, but it is exactly what the sibling guard exists to prevent. Probe the destination itself with `rev-parse --resolve-git-dir`. It answers for the path GIVEN and does not walk up to a parent, so a plain folder that merely sits inside a repository is still a valid destination — that case belongs to is_inside_worktree, and a test now pins it. The `.git` check is kept alongside rather than replaced, so nothing that was refused before is accepted now. Reported by Codex review on #26; the code is not part of that PR — it came from 15e51c3 (2026-07-17) and git_ops.rs is untouched there — so it is fixed here. cargo test full workspace green (git-core 185); npm run check 524 files 0 errors; npm test 330 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
💡 Codex Reviewgit-it/crates/git-core/src/git_ops.rs Lines 118 to 120 in a6673e8 When the selected parent is itself a bare repository or a normal repository's Line 40 in a6673e8 The upgrade to Vite 8 raises the runtime requirement to Node ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Two findings from Codex review on #29. **Git directory as parent.** The same blind spot this PR fixed for the destination, one level up. When the destination does not exist yet the nesting probe falls back to the PARENT, and `--is-inside-work-tree` answers false inside a Git directory just as it does inside a bare repo. Verified: selecting `some-repo/.git` produced `some-repo/.git/proj`, a whole repository inside another repository's metadata, and returned initialized: true. A bare repo as parent did the same. `is_git_dir(&parent)` now rejects both before any filesystem mutation, with a message that names the actual problem. **Node floor.** Vite 8 requires `^20.19.0 || >=22.12.0` (recorded in package-lock.json) but package.json declared no `engines` and the build-from-source instructions named no version, so `npm install` on Node 18 or early 20 warns and then fails later as a confusing build error. Declared in `engines` and stated in the README next to the commands it governs. CI already builds on 24, so this documents the existing requirement rather than changing it. Unrelated to the rest of this PR, but fixing it here keeps the review on one branch. cargo test 186 + 7 passed; clippy clean; npm run check 524 files 0 errors; npm test 330 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both addressed in Git directory as parent — confirmed, and it is the same blind spot this PR fixes, one level up. I guarded the destination; the parent has the identical hole. Probed both candidates: With the destination not yet existing the nesting probe falls back to the parent, which answers — a whole repository inside another repository's metadata. Node floor — confirmed. That second one is unrelated to bare-repo detection; fixed here anyway so the review converges on one branch rather than spawning another PR.
|
💡 Codex Reviewgit-it/src/routes/+page.svelte Line 298 in 4ddc2af Because this window-level listener unconditionally calls ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Correct about the behaviour, but it is not this PR — and it is already fixed.
You saw the old code because this branch was cut from const style = el && getComputedStyle(el);
if (style && (style.webkitUserSelect || style.userSelect) !== "none") return;
event.preventDefault();It reads the computed Gate after the merge: |
💡 Codex Reviewgit-it/src/lib/store.svelte.ts Lines 1684 to 1688 in 50d15f8 On an A → B switch, git-it/src/lib/updater.svelte.ts Lines 131 to 133 in 50d15f8 If an automatic beta update is queued while Settings is open, the user can switch to stable and run a manual check. When that check returns no update—or a different version—the backend pending-update slot is cleared or replaced, but this UI queue is retained because it is only cleared for the same version. Closing Settings then presents the stale beta prompt, whose Download action either fails with ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…rseded update Two findings from Codex review on #29. Both are pre-existing on `next` and in files this PR does not otherwise touch; fixed here to keep the review on one branch. **P1 — sidebar acting on the wrong repository.** A switch deliberately keeps the previous repo's refs on screen until the reload lands, because clearing them flashed the sidebar empty mid-switch. But `appState.repo` already points at the new repo, so a command fired from a stale row runs against it. With a name both repos have — `main`, `develop` — "Delete branch" deletes the wrong repo's branch, and there is no reflog prompt in front of that. Clearing the state would reintroduce the flicker the retention exists to prevent, so the rows stay and the actions go inert: checkout, delete (also the Linked Worktrees panel, which routes through the same handler), and the ref context menu all refuse while `repoLoading`, which `reloadGraph` clears in a `finally` guarded against a superseding switch. The context menu bails before it is built, so its items are unreachable rather than merely guarded. Selection and scrolling stay live — they read nothing and write nothing. Also gated the detached-HEAD menu, which the report did not mention: "Create branch here" would use the OLD repo's HEAD sha, and two clones of one project share commit ids, so that can quietly succeed in the wrong repository instead of erroring on an unknown sha. **P2 — superseded automatic update still prompting.** A manual check replaces the backend's pending-update slot, but the UI queue was only cleared on an exact version match. A manual check finding nothing, or a different version, left the old one queued; closing the overlay then presented a version the backend no longer had — Download failed with "no pending update", or fetched something other than what the dialog named. The queue is now dropped when a manual check starts. Two regression tests, both watched failing first. npm run check 530 files 0 errors; npm test 388 passed; cargo test 232 + 7; clippy clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both fixed in P1 — sidebar acting on the wrong repository ✅Confirmed structurally: Of your two suggestions I took the second. Clearing the repo-affiliated state would reintroduce the exact flicker the retention exists to prevent (documented at the retention site: clearing "flashed the header branch chip, sidebar and status bar empty mid-switch"). So the rows stay and the actions go inert while Covered: checkout, delete (including the Linked Worktrees panel, which routes through the same One you did not mention, same root cause: the detached-HEAD menu. "Create branch here" would use the old repo's HEAD sha — and two clones of one project share commit ids, so that can quietly succeed in the wrong repository rather than erroring on an unknown sha. Gated too. P2 — superseded automatic update ✅Confirmed, and both holes reproduce. Regression tests written first and watched fail: The queue is now dropped when a manual check starts, which covers both the nothing-found and different-version cases the exact-version match missed. The retry timer needs no cancelling — it re-reads the variable and stops on its own. The old same-version clear is removed as dead.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87a0114bb2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…r itself `is_git_dir` resolves an exact path, so the parent guard saw `repo/.git` and a bare repo but walked straight past `repo/.git/hooks` and `bare.git/objects` — which a folder picker reaches just as easily. `--is-inside-work-tree` is false down there too, so nothing else caught it either, and the flow created a repository inside another repository's object store. Reproduced: it returned initialized: true with a path of `host/.git/hooks/proj`. `--is-inside-git-dir` is documented as true anywhere below the repository directory, which is the question a candidate parent actually has to answer: path resolve-git-dir is-inside-git-dir normal (worktree) false false normal/.git true true normal/.git/hooks false true bare.git true true bare.git/objects false true plain (no repo) false error The working-tree row is why this stays a separate probe from the nesting one: a normal working directory must remain a valid parent. The error row is handled the way `is_inside_worktree` already does — a non-zero exit counts as false. `is_git_dir` still guards the DESTINATION, where the exact-path question is the right one. The parent test now covers both tops and both descendants. cargo test 232 + 7; clippy clean; npm run check 530 files 0 errors; npm test 388 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reported by Codex review on #26 as P2. The code is not part of that PR —
crates/git-core/src/git_ops.rsis untouched by #26, and the guard came from15e51c3(2026-07-17) — so it is fixed here on its own branch, same as #27.The gap
initialize_repositoryguards against an existing repository two ways, and a bare one slips between them:destination.join(".git").exists().gitchildis_inside_worktree(destination)rev-parse --is-inside-work-treesaysfalseinside a bare repoThe destination is non-empty, so the user gets the "folder is not empty" prompt rather than "already a Git repository". On confirming,
git initruns with the bare repository as its working directory.What actually happens
Verified against real git rather than assumed — this is the exact command
git_ops.rsissues,cwdset to a bare repo holding a real branch:So it is a wrong state, not data loss — the bare repo survives intact and the only change is the nested
.git. Worth stating plainly since "mutates a directory that should have been rejected" could read as destructive. It is still exactly what the sibling guard exists to prevent, and the app then reports success and opens the nested repo.The fix
Probe the destination itself with
rev-parse --resolve-git-dir:I characterised the probe against every shape before picking it, because the risk is over-reach — refusing a legitimate destination:
--resolve-git-dir.gitThat last row is the important one:
--resolve-git-diranswers for the path given, so a plain folder that merely sits inside a repository is still a valid destination. Nesting remainsis_inside_worktree's job, and a test now pins that boundary.The
.gitcheck is kept alongside rather than replaced, so nothing refused before is accepted now.The operand is always absolute (
parentisfs::canonicalized before the join), so it cannot be read as a flag — noted in the doc comment since the project's shell-out rule would otherwise want a--, which--resolve-git-dircannot take because it consumes the next argument.Tests (TDD)
initialize_repository_rejects_an_existing_bare_repository— watched fail first, returningOk(initialized: true, existing_entries: 7). Asserts the error and that no nested.gitwas created.initialize_repository_still_accepts_a_plain_empty_folder— the over-reach guard. Passed before the change and after.Both run with
cwdinside a repository (cargo's working dir), so the "already inside a repo" case is exercised naturally.Gates:
cargo testfull workspace green (git-core 185) ·npm run check524 files / 0 errors ·npm test330 passed.🤖 Generated with Claude Code