fix: retrospective review fixes for merged PR #107#119
Merged
Conversation
PR #107's fixup (a341534) narrowed prunable-worktree recovery in AddOrAttach to fire only on os.Stat(dst) returning ErrNotExist, to avoid misclassifying a transient EACCES/NFS stat error on a present worktree as prunable. That narrowing over-corrected: it only detects the "whole directory is gone" shape of prunability, but `git worktree list --porcelain` also flags an entry "prunable" when the directory survives while its own `.git` link (or git's admin-side gitdir record) was removed independently — e.g. a partial teardown that clears `.git` before the rest of the tree, or a cleanup pass that strips dotfiles. Verified against real git (2.53.0): `git worktree list --porcelain` reports "prunable gitdir file points to non-existent location" for this exact shape, while os.Stat(dst) reports no error at all, so the ENOENT-only guard silently no-ops on it — reintroducing a variant of the half-materialised-worktree bug PR #107 set out to fix. Add a Prunable field to Worktree, populated by parsing the `prunable` porcelain line List() was previously discarding, and gate recovery on `wt.Prunable || ENOENT` instead of ENOENT alone. This delegates the "is this worktree actually usable" judgment to git itself (which already computes it correctly) instead of re-deriving an incomplete proxy via a single stat call, while still falling back to the ENOENT check for git versions/entries where the annotation is absent — the EACCES/NFS protection the fixup added is preserved unchanged. New test TestRunner_AddOrAttach_RecreatesPrunableWorktree_DirSurvivesGitLinkGone exercises this against a real git worktree: removes only the `.git` link (not the whole dir), confirms git flags it prunable while the dir still exists on disk, then confirms AddOrAttach recovers it. Confirmed the test fails against the pre-fix ENOENT-only guard.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Retrospective /code-review sweep of merged PR #107 (
fix: recreate a prunable worktree instead of no-op'ing on create), which shipped without pre-merge review.What
PR #107 fixed
AddOrAttachno-op'ing on a worktree whose directory was deleted out-of-band while git's admin record survived ("prunable"). Its own fixup commita3415349narrowed recovery to fire only whenos.Stat(dst)returnsErrNotExist, specifically to avoid misclassifying a transientEACCES/stale-NFS stat error on a present worktree as prunable.Why this PR
That narrowing only detects the "whole directory is gone" shape of prunability.
git worktree list --porcelainalso emits aprunable <reason>line when the directory survives on disk but its own.gitfile (or git's admin-sidegitdirrecord) was removed independently — e.g. a partial teardown that clears.gitbefore the rest of the tree, or a cleanup pass that strips dotfiles. Verified against real git 2.53.0:os.Stat(dst)reports no error at all in this shape (the directory is right there), so the ENOENT-only guard added ina3415349silently returns the old no-op — reintroducing a variant of the exact half-materialised-worktree bug PR #107 set out to fix, just triggered by a different out-of-band deletion pattern than a fullrm -rf.Fix
internal/gitwt/gitwt.go: add aPrunable boolfield toWorktree, populated by parsing theprunableporcelain line thatList()was previously discarding entirely.AddOrAttachnow gates recovery onwt.Prunable || errors.Is(statErr, os.ErrNotExist)instead of the ENOENT check alone — trusting git's own authoritative verdict (which already covers every prunable reason, not just full deletion) while still falling back to the ENOENT check for git versions/entries where the annotation is absent. Thea3415349EACCES/NFS protection is preserved unchanged: a stat failure that isn't ENOENT, on an entry git does not flag prunable, still keeps the old no-op.Test plan
go build ./...go test ./...golangci-lint run ./...TestRunner_AddOrAttach_RecreatesPrunableWorktree_DirSurvivesGitLinkGoneexercises this against a real git worktree: removes only the.gitlink (not the whole dir), confirms git flags the entry prunable while the dir still exists on disk, then confirmsAddOrAttachrecovers it. Confirmed the test fails against the pre-fix ENOENT-only guard (verified by temporarily reverting the guard locally).