Skip to content

Commit 3bd0b91

Browse files
authored
[fix] Repair orphaned worktree before removal so clean/remove fully clear it (#407)
* [fix] Repair orphaned worktree before removal so clean/remove fully clear it (#405) removeWorktreeEntry made a one-shot decision from the list-time prunable snapshot, so an orphaned worktree (directory present, .git deleted out-of-band) either died with exit 128 (no fallback) or got pruned while leaving the directory on disk. Heal via `git worktree repair` before removal so clean/remove/merge fully clear it instead. git worktree repair's own exit status is unreliable (it can report failure while having already rewritten the .git linkfile), so success is judged by whether the follow-up remove works, not repair's reported status. * [fix] Drop ticket refs and shorten comments added in this PR No functional change — trims comments this PR introduced to be self-explanatory without pointing at issue numbers, per review feedback. * [fix] Respect --force through the orphan-heal path healAndRemoveOrphan hardcoded force=true, silently discarding real uncommitted content in an orphaned worktree regardless of the caller's --force flag. Thread the real force value through instead, and only fall back to prune when the .git linkfile is still missing after the retry — a genuinely dirty (but now-healed) worktree surfaces its real git error rather than being force-deleted or misreported as pruned. Also wraps the retried remove's error alongside prune's on total failure, and renames the operations-layer output fields (RemoveResult/ CleanedItem/MergeResult) from Prunable to LeftOnDisk to decouple them from the input-classification meaning of Prunable elsewhere. The public --json field names are unaffected.
1 parent 15d7aa0 commit 3bd0b91

18 files changed

Lines changed: 552 additions & 140 deletions

cmd/clean.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -299,7 +299,7 @@ func runCleanJSON(ctx context.Context, cmd *cobra.Command, r git.Runner, sp *spi
299299
cleaned := make([]output.CleanedItemJSON, 0, len(items))
300300
for _, it := range items {
301301
cleaned = append(cleaned, output.CleanedItemJSON{
302-
Branch: it.Branch, Path: it.Path, Prunable: it.Prunable,
302+
Branch: it.Branch, Path: it.Path, Prunable: it.LeftOnDisk,
303303
WorktreeRemoved: it.WorktreeRemoved, BranchDeleted: it.BranchDeleted,
304304
RemoteDeleted: it.RemoteDeleted, RemoteError: errStr(it.RemoteError),
305305
Error: errStr(it.Error),
@@ -494,13 +494,13 @@ func printCleanedItems(cmd *cobra.Command, items []operations.CleanedItem) {
494494
for _, item := range items {
495495
if !item.WorktreeRemoved {
496496
hint := "git worktree remove --force -- " + item.Path
497-
if item.Prunable {
497+
if item.LeftOnDisk {
498498
hint = "git worktree prune"
499499
}
500500
fmt.Fprintf(cmd.OutOrStdout(), "Failed to remove worktree %s\nTo remove manually: %s\n", item.Branch, hint)
501501
continue
502502
}
503-
if item.Prunable {
503+
if item.LeftOnDisk {
504504
fmt.Fprintf(cmd.OutOrStdout(), "Cleared stale worktree registration: %s (directory left on disk — remove manually if unneeded)\n", item.Path)
505505
} else {
506506
fmt.Fprintf(cmd.OutOrStdout(), "Removed worktree: %s\n", item.Path)

cmd/clean_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -868,9 +868,9 @@ func TestPrintCleanedItemsAllBranches(t *testing.T) {
868868
// Failure: worktree removal failed
869869
{Branch: "feature/fail", Path: "/wt/fail", WorktreeRemoved: false, BranchDeleted: false},
870870
// Failure: prunable worktree, removal via prune failed
871-
{Branch: "feature/prunable-fail", Path: "/wt/prunable-fail", Prunable: true, WorktreeRemoved: false, BranchDeleted: false},
871+
{Branch: "feature/prunable-fail", Path: "/wt/prunable-fail", LeftOnDisk: true, WorktreeRemoved: false, BranchDeleted: false},
872872
// Success: prunable worktree recovered via git worktree prune (directory left on disk)
873-
{Branch: "feature/prunable-ok", Path: "/wt/prunable-ok", Prunable: true, WorktreeRemoved: true, BranchDeleted: true},
873+
{Branch: "feature/prunable-ok", Path: "/wt/prunable-ok", LeftOnDisk: true, WorktreeRemoved: true, BranchDeleted: true},
874874
}
875875
printCleanedItems(cmd, items)
876876
out := buf.String()
@@ -1133,7 +1133,7 @@ func TestCleanMergedJSONForceAllFailedCountStaysZero(t *testing.T) {
11331133
r := cleanMergedTestRunner(t, " "+branchDone, worktreeOut)
11341134
baseRun := r.run
11351135
r.run = func(args ...string) (string, error) {
1136-
if len(args) >= 2 && args[0] == cmdWorktreeTest && args[1] == cmdRemove {
1136+
if len(args) >= 2 && args[0] == cmdWorktreeTest && (args[1] == cmdRemove || args[1] == cmdPrune) {
11371137
return "", errGitFailed
11381138
}
11391139
return baseRun(args...)

cmd/merge.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ var mergeCmd = &cobra.Command{
131131
MergingToMain: result.MergingToMain,
132132
SourceRemoved: result.SourceRemoved,
133133
WorktreeRemoved: result.WorktreeRemoved,
134-
SourcePrunable: result.SourcePrunable,
134+
SourcePrunable: result.SourceLeftOnDisk,
135135
RemoveError: errStr(result.RemoveError),
136136
RemoteDeleted: result.RemoteDeleted,
137137
RemoteError: errStr(result.RemoteError),
@@ -151,7 +151,7 @@ var mergeCmd = &cobra.Command{
151151
fmt.Fprintln(cmd.OutOrStdout(), result.RemoveError)
152152
} else if result.RemoveError != nil {
153153
hint := "git worktree remove --force -- " + result.SourcePath
154-
if result.SourcePrunable {
154+
if result.SourceLeftOnDisk {
155155
hint = "git worktree prune"
156156
}
157157
fmt.Fprintf(cmd.OutOrStdout(), "Merged successfully but failed to remove worktree: %v\nTo remove manually: %s\n", result.RemoveError, hint)
@@ -172,7 +172,7 @@ var mergeCmd = &cobra.Command{
172172
// a prunable recovery (git worktree prune — directory left on disk) from a
173173
// real worktree removal, matching cmd/clean.go and cmd/remove.go.
174174
func printMergeWorktreeRemoved(cmd *cobra.Command, result operations.MergeResult) {
175-
if result.SourcePrunable {
175+
if result.SourceLeftOnDisk {
176176
fmt.Fprintf(cmd.OutOrStdout(), "Cleared stale worktree registration: %s (directory left on disk — remove manually if unneeded)\n", result.SourcePath)
177177
return
178178
}

cmd/merge_test.go

Lines changed: 72 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -274,6 +274,15 @@ func TestMergeSourceNotFound(t *testing.T) {
274274
}
275275

276276
func TestMergeRemoveWorktreeFails(t *testing.T) {
277+
// A real .git file makes this a genuine (non-orphaned) failure, so it
278+
// short-circuits instead of routing through the heal-and-retry path.
279+
sourceDir := t.TempDir()
280+
if err := os.WriteFile(filepath.Join(sourceDir, ".git"), []byte("gitdir: /somewhere/.git/worktrees/login\n"), 0o644); err != nil {
281+
t.Fatalf("failed to create .git fixture: %v", err)
282+
}
283+
worktreeOut := "worktree " + repoPath + "\nHEAD abc123\nbranch refs/heads/main\n\n" +
284+
"worktree " + sourceDir + "\nHEAD def456\nbranch refs/heads/feature/login\n"
285+
277286
cfg := &config.Config{DefaultSource: branchMain, WorktreeDir: defaultRelativeWtDir}
278287
r := &mockRunner{
279288
run: func(args ...string) (string, error) {
@@ -283,7 +292,7 @@ func TestMergeRemoveWorktreeFails(t *testing.T) {
283292
if len(args) >= 2 && args[0] == cmdWorktreeTest && args[1] == cmdRemove {
284293
return "", errors.New("locked")
285294
}
286-
return mergeWorktreeOut, nil
295+
return worktreeOut, nil
287296
},
288297
runInDir: noopRunInDir,
289298
}
@@ -305,7 +314,7 @@ func TestMergeRemoveWorktreeFails(t *testing.T) {
305314
if !strings.Contains(out, "failed to remove worktree") {
306315
t.Errorf("output = %q, want 'failed to remove worktree'", out)
307316
}
308-
if !strings.Contains(out, "To remove manually: git worktree remove --force -- /wt/feature-login") {
317+
if !strings.Contains(out, "To remove manually: git worktree remove --force -- "+sourceDir) {
309318
t.Errorf("output = %q, want a path-based 'git worktree remove --force --' hint", out)
310319
}
311320
}
@@ -324,7 +333,7 @@ func TestMergeRemoveWorktreePrunablePruneFails(t *testing.T) {
324333
if len(args) >= 2 && args[1] == cmdShowToplevel {
325334
return repoPath, nil
326335
}
327-
if len(args) >= 2 && args[0] == cmdWorktreeTest && args[1] == cmdPrune {
336+
if len(args) >= 2 && args[0] == cmdWorktreeTest && (args[1] == cmdRemove || args[1] == cmdPrune) {
328337
return "", errors.New("prune failed")
329338
}
330339
return prunableWorktreeOut, nil
@@ -354,13 +363,9 @@ func TestMergeRemoveWorktreePrunablePruneFails(t *testing.T) {
354363
}
355364
}
356365

357-
// TestMergePrunableSourceSuccessMessage guards a follow-up to #374: once a
358-
// prunable source's dirty check is skipped, a merge with auto-cleanup can
359-
// actually succeed and reach the success-path print — which must use the
360-
// same "Cleared stale worktree registration" wording as clean/remove, not
361-
// the misleading "Removed worktree" (git worktree prune leaves the
362-
// directory on disk).
363-
func TestMergePrunableSourceSuccessMessage(t *testing.T) {
366+
// TestMergePrunableSourceHealsAndRemoves: repair+remove now heal a prunable
367+
// source fully, so the message must say "Removed worktree", not "Cleared stale worktree registration".
368+
func TestMergePrunableSourceHealsAndRemoves(t *testing.T) {
364369
prunableWorktreeOut := "worktree /repo\nHEAD abc123\nbranch refs/heads/main\n\n" +
365370
"worktree /wt/feature-login\nHEAD def456\nbranch refs/heads/feature/login\nprunable gitdir file points to non-existent location\n"
366371

@@ -389,6 +394,53 @@ func TestMergePrunableSourceSuccessMessage(t *testing.T) {
389394
cmd.Flags().Bool(flagDelete, false, "")
390395
cmd.SetContext(config.WithConfig(context.Background(), cfg))
391396

397+
err := mergeCmd.RunE(cmd, []string{"login"})
398+
if err != nil {
399+
t.Fatalf(fatalMergeRunE, err)
400+
}
401+
out := buf.String()
402+
if !strings.Contains(out, "Removed worktree: /wt/feature-login") {
403+
t.Errorf("output = %q, want 'Removed worktree' — repair+remove fully cleared the directory", out)
404+
}
405+
if strings.Contains(out, "Cleared stale worktree registration") {
406+
t.Errorf("output = %q, want NOT 'Cleared stale worktree registration' once repair+remove succeed", out)
407+
}
408+
}
409+
410+
// TestMergePrunableSourceFallbackMessage: when repair+remove both still fail,
411+
// the prune-only fallback leaves the dir on disk, so "Cleared stale worktree registration" still applies.
412+
func TestMergePrunableSourceFallbackMessage(t *testing.T) {
413+
prunableWorktreeOut := "worktree /repo\nHEAD abc123\nbranch refs/heads/main\n\n" +
414+
"worktree /wt/feature-login\nHEAD def456\nbranch refs/heads/feature/login\nprunable gitdir file points to non-existent location\n"
415+
416+
cfg := &config.Config{DefaultSource: branchMain, WorktreeDir: defaultRelativeWtDir}
417+
r := &mockRunner{
418+
run: func(args ...string) (string, error) {
419+
if len(args) >= 2 && args[1] == cmdShowToplevel {
420+
return repoPath, nil
421+
}
422+
if len(args) >= 2 && args[0] == cmdWorktreeTest && args[1] == cmdRemove {
423+
return "", errors.New("remove failed")
424+
}
425+
return prunableWorktreeOut, nil
426+
},
427+
runInDir: func(_ string, args ...string) (string, error) {
428+
if len(args) >= 1 && (args[0] == cmdStatus || args[0] == flagSyncMerge) {
429+
return "", nil
430+
}
431+
return "", nil
432+
},
433+
}
434+
restore := overrideNewRunner(r)
435+
defer restore()
436+
437+
cmd, buf := newTestCmd()
438+
cmd.Flags().String(flagInto, "", "")
439+
cmd.Flags().Bool(flagNoFF, false, "")
440+
cmd.Flags().Bool(flagKeep, false, "")
441+
cmd.Flags().Bool(flagDelete, false, "")
442+
cmd.SetContext(config.WithConfig(context.Background(), cfg))
443+
392444
err := mergeCmd.RunE(cmd, []string{"login"})
393445
if err != nil {
394446
t.Fatalf(fatalMergeRunE, err)
@@ -857,6 +909,15 @@ func TestMergeDryRunJSON(t *testing.T) {
857909
}
858910

859911
func TestMergeRemoveErrorJSON(t *testing.T) {
912+
// A real .git file makes this a genuine (non-orphaned) failure, so it
913+
// short-circuits instead of routing through the heal-and-retry path.
914+
sourceDir := t.TempDir()
915+
if err := os.WriteFile(filepath.Join(sourceDir, ".git"), []byte("gitdir: /somewhere/.git/worktrees/login\n"), 0o644); err != nil {
916+
t.Fatalf("failed to create .git fixture: %v", err)
917+
}
918+
worktreeOut := "worktree " + repoPath + "\nHEAD abc123\nbranch refs/heads/main\n\n" +
919+
"worktree " + sourceDir + "\nHEAD def456\nbranch refs/heads/feature/login\n"
920+
860921
cfg := &config.Config{DefaultSource: branchMain, WorktreeDir: defaultRelativeWtDir}
861922
r := &mockRunner{
862923
run: func(args ...string) (string, error) {
@@ -866,7 +927,7 @@ func TestMergeRemoveErrorJSON(t *testing.T) {
866927
if len(args) >= 2 && args[0] == cmdWorktreeTest && args[1] == cmdRemove {
867928
return "", errors.New("locked")
868929
}
869-
return mergeWorktreeOut, nil
930+
return worktreeOut, nil
870931
},
871932
runInDir: noopRunInDir,
872933
}

cmd/remove.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ var removeCmd = &cobra.Command{
9898
Task: result.Task,
9999
Branch: result.Branch,
100100
Path: result.Path,
101-
Prunable: result.Prunable,
101+
Prunable: result.LeftOnDisk,
102102
WorktreeRemoved: result.WorktreeRemoved,
103103
BranchDeleted: result.BranchDeleted,
104104
KeepBranch: keepBranch,
@@ -107,7 +107,7 @@ var removeCmd = &cobra.Command{
107107
})
108108
}
109109

110-
if result.Prunable {
110+
if result.LeftOnDisk {
111111
fmt.Fprintf(cmd.OutOrStdout(), "Cleared stale worktree registration: %s (directory left on disk — remove manually if unneeded)\n", result.Path)
112112
} else {
113113
fmt.Fprintf(cmd.OutOrStdout(), "Removed worktree: %s\n", result.Path)

cmd/remove_test.go

Lines changed: 62 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ package cmd
33
import (
44
"context"
55
"errors"
6+
"os"
7+
"path/filepath"
68
"strings"
79
"testing"
810

@@ -41,21 +43,23 @@ func TestRemoveSuccess(t *testing.T) {
4143
}
4244
}
4345

44-
func TestRemovePrunablePathRunsPrune(t *testing.T) {
46+
func TestRemovePrunablePathHealsAndRemoves(t *testing.T) {
4547
prunableWorktreeOut := "worktree /repo\nHEAD abc123\nbranch refs/heads/main\n\n" +
4648
"worktree /wt/feature-login\nHEAD def456\nbranch refs/heads/feature/login\nprunable gitdir file points to non-existent location\n"
4749

48-
var pruneInvoked, removeInvoked bool
50+
var repairInvoked, removeInvoked, pruneInvoked bool
4951
r := &mockRunner{
5052
run: func(args ...string) (string, error) {
5153
cmd := strings.Join(args, " ")
5254
switch {
53-
case strings.Contains(cmd, "worktree prune"):
54-
pruneInvoked = true
55+
case strings.Contains(cmd, "worktree repair"):
56+
repairInvoked = true
5557
case strings.Contains(cmd, "worktree remove"):
5658
removeInvoked = true
59+
case strings.Contains(cmd, "worktree prune"):
60+
pruneInvoked = true
5761
}
58-
if len(args) > 0 && args[0] == "worktree" && args[1] == "list" {
62+
if len(args) > 0 && args[0] == cmdWorktreeTest && args[1] == cmdList {
5963
return prunableWorktreeOut, nil
6064
}
6165
return "", nil
@@ -74,11 +78,51 @@ func TestRemovePrunablePathRunsPrune(t *testing.T) {
7478
if err != nil {
7579
t.Fatalf("removeCmd.RunE: %v", err)
7680
}
77-
if !pruneInvoked {
78-
t.Error("expected 'git worktree prune' to be invoked for a prunable worktree")
81+
if !repairInvoked {
82+
t.Error("expected 'git worktree repair' to be invoked for a prunable worktree")
83+
}
84+
if !removeInvoked {
85+
t.Error("expected 'git worktree remove' to be invoked after repair")
86+
}
87+
if pruneInvoked {
88+
t.Error("expected 'git worktree prune' NOT to be invoked when repair+remove succeed")
89+
}
90+
out := buf.String()
91+
if !strings.Contains(out, "Removed worktree") {
92+
t.Errorf("output = %q, want 'Removed worktree' — repair+remove fully cleared the directory", out)
93+
}
94+
if strings.Contains(out, "Cleared stale worktree registration") {
95+
t.Errorf("output = %q, want NOT 'Cleared stale worktree registration' once repair+remove succeed", out)
96+
}
97+
}
98+
99+
func TestRemovePrunablePathFallbackMessage(t *testing.T) {
100+
prunableWorktreeOut := "worktree /repo\nHEAD abc123\nbranch refs/heads/main\n\n" +
101+
"worktree /wt/feature-login\nHEAD def456\nbranch refs/heads/feature/login\nprunable gitdir file points to non-existent location\n"
102+
103+
r := &mockRunner{
104+
run: func(args ...string) (string, error) {
105+
if len(args) > 0 && args[0] == cmdWorktreeTest && args[1] == cmdList {
106+
return prunableWorktreeOut, nil
107+
}
108+
if len(args) > 1 && args[0] == "worktree" && args[1] == "remove" {
109+
return "", errors.New("remove failed")
110+
}
111+
return "", nil
112+
},
113+
runInDir: noopRunInDir,
79114
}
80-
if removeInvoked {
81-
t.Error("expected 'git worktree remove' NOT to be invoked for a prunable worktree")
115+
restore := overrideNewRunner(r)
116+
defer restore()
117+
118+
cmd, buf := newTestCmd()
119+
cmd.SetContext(config.WithConfig(context.Background(), &config.Config{}))
120+
cmd.Flags().Bool(flagKeepBranch, false, "")
121+
cmd.Flags().Bool(flagForce, false, "")
122+
123+
err := removeCmd.RunE(cmd, []string{"login"})
124+
if err != nil {
125+
t.Fatalf("removeCmd.RunE: %v", err)
82126
}
83127
out := buf.String()
84128
if !strings.Contains(out, "Cleared stale worktree registration") {
@@ -195,12 +239,20 @@ func TestRemoveWorktreeNotFound(t *testing.T) {
195239
}
196240

197241
func TestRemoveWorktreeFails(t *testing.T) {
242+
// A real .git file makes this a genuine (non-orphaned) failure, so it
243+
// short-circuits instead of routing through the heal-and-retry path.
244+
wtPath := t.TempDir()
245+
if err := os.WriteFile(filepath.Join(wtPath, ".git"), []byte("gitdir: /somewhere/.git/worktrees/login\n"), 0o644); err != nil {
246+
t.Fatalf("failed to create .git fixture: %v", err)
247+
}
248+
worktreeOut := "worktree /repo\nHEAD abc123\nbranch refs/heads/main\n\nworktree " + wtPath + "\nHEAD def456\nbranch refs/heads/feature/login\n"
249+
198250
r := &mockRunner{
199251
run: func(args ...string) (string, error) {
200252
if len(args) >= 2 && args[0] == cmdWorktreeTest && args[1] == "remove" {
201253
return "", errors.New("locked")
202254
}
203-
return removeWorktreeOut, nil
255+
return worktreeOut, nil
204256
},
205257
runInDir: noopRunInDir,
206258
}

internal/git/worktree.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,13 @@ func MoveWorktree(ctx context.Context, r Runner, oldPath, newPath string, force
6363
return err
6464
}
6565

66+
// RepairWorktree recreates a missing .git linkfile from the still-present
67+
// admin back-pointer, so a subsequent RemoveWorktree can fully remove it.
68+
func RepairWorktree(ctx context.Context, r Runner, path string) error {
69+
_, err := r.Run(ctx, cmdWorktree, "repair", "--", path)
70+
return err
71+
}
72+
6673
// ListWorktrees returns all worktrees by parsing `git worktree list --porcelain`.
6774
func ListWorktrees(ctx context.Context, r Runner) ([]WorktreeEntry, error) {
6875
out, err := r.Run(ctx, cmdWorktree, "list", "--porcelain")

internal/git/worktree_test.go

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,34 @@ func TestRemoveWorktreeLeadingDashPath(t *testing.T) {
129129
}
130130
}
131131

132+
func TestRepairWorktree(t *testing.T) {
133+
if testing.Short() {
134+
t.Skip(skipIntegration)
135+
}
136+
137+
repo := testutil.NewTestRepo(t)
138+
r := &git.ExecRunner{Dir: repo}
139+
140+
wtPath := filepath.Join(filepath.Dir(repo), "wt-to-repair")
141+
if err := git.AddWorktree(context.Background(), r, wtPath, "feat/repair-me", "main"); err != nil {
142+
t.Fatalf(fatalAddWorktree, err)
143+
}
144+
145+
gitFile := filepath.Join(wtPath, ".git")
146+
if err := os.Remove(gitFile); err != nil {
147+
t.Fatalf("remove .git file: %v", err)
148+
}
149+
150+
// RepairWorktree's own error is unreliable (git can exit non-zero here
151+
// while still rewriting the .git linkfile), so assert on the actual
152+
// effect rather than the returned error.
153+
_ = git.RepairWorktree(context.Background(), r, wtPath)
154+
155+
if _, err := os.Stat(gitFile); err != nil {
156+
t.Errorf("expected .git file to be recreated after repair, stat err: %v", err)
157+
}
158+
}
159+
132160
func TestListWorktreesPrunableEntry(t *testing.T) {
133161
if testing.Short() {
134162
t.Skip(skipIntegration)

0 commit comments

Comments
 (0)