Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 45 additions & 8 deletions internal/git/mock_runner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,25 @@ func TestMoveWorktreeNoForce(t *testing.T) {
}
}

func TestMoveWorktreeInsertsDashDash(t *testing.T) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium — TestMoveWorktreeNoForce is now fully subsumed by this test

TestMoveWorktreeNoForce (line 335) calls MoveWorktree(r, "/old/path", "/new/path", false) and asserts !slices.Contains(captured, flagForce). This new test makes the same call and asserts slices.Equal(captured, want) — which already guarantees no --force (since want excludes it). The older test adds zero additional coverage.

Suggested: delete TestMoveWorktreeNoForce (or collapse both no-force tests into a table-driven test alongside TestMoveWorktreeForce to keep all three cases in one place).

var captured []string
r := &mockRunner{
run: func(args ...string) (string, error) {
captured = args
return "", nil
},
}

if err := MoveWorktree(r, "/old/path", "/new/path", false); err != nil {
t.Fatalf("MoveWorktree: %v", err)
}

want := []string{cmdWorktree, "move", "--", "/old/path", "/new/path"}
if !slices.Equal(captured, want) {
t.Errorf("args = %v, want %v", captured, want)
}
}

func TestMoveWorktreeForce(t *testing.T) {
var captured []string
r := &mockRunner{
Expand All @@ -362,14 +381,9 @@ func TestMoveWorktreeForce(t *testing.T) {
t.Fatalf("MoveWorktree: %v", err)
}
// git worktree move requires --force twice to move locked worktrees
forceCount := 0
for _, a := range captured {
if a == flagForce {
forceCount++
}
}
if forceCount != 2 {
t.Errorf("expected 2 %s flags, got %d in args %v", flagForce, forceCount, captured)
want := []string{cmdWorktree, "move", flagForce, flagForce, "--", "/old/path", "/new/path"}
if !slices.Equal(captured, want) {
t.Errorf("args = %v, want %v", captured, want)
}
}

Expand Down Expand Up @@ -983,6 +997,29 @@ func TestCheckoutInsertsDashDash(t *testing.T) {
}
}

func TestAddWorktreeInsertsDashDash(t *testing.T) {
const (
branch = "feature/some-task"
source = "-default-source"
)
var capturedArgs []string
r := &mockRunner{
run: func(args ...string) (string, error) {
capturedArgs = args
return "", nil
},
}

if err := AddWorktree(r, fakePath, branch, source); err != nil {
t.Fatalf("AddWorktree: %v", err)
}

want := []string{cmdWorktree, "add", "-b", branch, "--", fakePath, source}
if !slices.Equal(capturedArgs, want) {
t.Errorf("args = %v, want %v", capturedArgs, want)
}
}

func TestAddWorktreeFromBranchInsertsDashDash(t *testing.T) {
const branch = "feature/some-task"
var capturedArgs []string
Expand Down
5 changes: 3 additions & 2 deletions internal/git/worktree.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ type WorktreeEntry struct {

// AddWorktree creates a new worktree at the given path with a new branch from source.
func AddWorktree(r Runner, path, branch, source string) error {
_, err := r.Run(cmdWorktree, "add", "-b", branch, path, source)
_, err := r.Run(cmdWorktree, "add", "-b", branch, "--", path, source)
return err
}

Expand All @@ -50,10 +50,11 @@ func RemoveWorktree(r Runner, path string, force bool) error {
// MoveWorktree moves the worktree from oldPath to newPath.
// When force is true, --force is passed twice so that even locked worktrees can be moved.
func MoveWorktree(r Runner, oldPath, newPath string, force bool) error {
args := []string{cmdWorktree, "move", oldPath, newPath}
args := []string{cmdWorktree, "move"}
if force {
args = append(args, flagForce, flagForce)
}
args = append(args, "--", oldPath, newPath)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

High — git worktree move may not support -- as end-of-options separator

git worktree add explicitly lists -- in its synopsis; git worktree move does not. The generated invocation is now:

git worktree move --force --force -- /old /new

If move does not recognise -- as end-of-options, git reads -- as <worktree> and /old as <new-path>, silently moving the wrong worktree (or erroring). Verify empirically in a temp repo:

git worktree move -- /some/path /other/path

If git rejects it, the -- must be omitted for move (or conditioned on the operands actually starting with -). A targeted integration test would catch this definitively.

_, err := r.Run(args...)
return err
}
Expand Down
Loading