Skip to content

fix: improve worktree sync reliability for refresh operations #361

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
whitmo wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: improve worktree sync reliability for refresh operations #361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion internal/daemon/daemon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -582,7 +582,7 @@ func (d *Daemon) refreshWorktrees() {

// Refresh the worktree
d.logger.Info("Refreshing worktree for %s/%s (%d commits behind)", repoName, agentName, wtState.CommitsBehind)
result := worktree.RefreshWorktree(agent.WorktreePath, remote, mainBranch)
result := worktree.RefreshWorktreePreFetched(agent.WorktreePath, remote, mainBranch)

if result.Error != nil {
if result.HasConflicts {
Expand Down
188 changes: 173 additions & 15 deletions internal/worktree/refresh_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,11 +216,7 @@ func TestRefreshWorktree_MidRebase(t *testing.T) {
}

// Simulate mid-rebase state by creating the rebase-merge directory
gitDir := filepath.Join(wtPath, ".git")
content, err := os.ReadFile(gitDir)
if err == nil && strings.HasPrefix(string(content), "gitdir:") {
gitDir = strings.TrimSpace(strings.TrimPrefix(string(content), "gitdir:"))
}
gitDir := resolveGitDir(wtPath)
rebaseDir := filepath.Join(gitDir, "rebase-merge")
if err := os.MkdirAll(rebaseDir, 0755); err != nil {
t.Fatalf("Failed to create rebase-merge dir: %v", err)
Expand Down Expand Up @@ -249,11 +245,7 @@ func TestRefreshWorktree_MidMerge(t *testing.T) {
}

// Simulate mid-merge state by creating MERGE_HEAD file
gitDir := filepath.Join(wtPath, ".git")
content, err := os.ReadFile(gitDir)
if err == nil && strings.HasPrefix(string(content), "gitdir:") {
gitDir = strings.TrimSpace(strings.TrimPrefix(string(content), "gitdir:"))
}
gitDir := resolveGitDir(wtPath)
mergeHead := filepath.Join(gitDir, "MERGE_HEAD")
if err := os.WriteFile(mergeHead, []byte("abc123"), 0644); err != nil {
t.Fatalf("Failed to create MERGE_HEAD: %v", err)
Expand Down Expand Up @@ -594,11 +586,7 @@ func TestGetWorktreeState_WithMidRebaseApply(t *testing.T) {
}

// Simulate mid-rebase-apply state
gitDir := filepath.Join(wtPath, ".git")
content, err := os.ReadFile(gitDir)
if err == nil && strings.HasPrefix(string(content), "gitdir:") {
gitDir = strings.TrimSpace(strings.TrimPrefix(string(content), "gitdir:"))
}
gitDir := resolveGitDir(wtPath)
rebaseApplyDir := filepath.Join(gitDir, "rebase-apply")
if err := os.MkdirAll(rebaseApplyDir, 0755); err != nil {
t.Fatalf("Failed to create rebase-apply dir: %v", err)
Expand All @@ -617,3 +605,173 @@ func TestGetWorktreeState_WithMidRebaseApply(t *testing.T) {
t.Errorf("Expected mid-rebase reason, got: %s", state.RefreshReason)
}
}

func TestRefreshWorktree_RebaseConflict(t *testing.T) {
repoPath, cleanup := createTestRepoWithRemote(t)
defer cleanup()

manager := NewManager(repoPath)

// Create a worktree with a feature branch
wtPath := filepath.Join(repoPath, "wt-conflict")
if err := manager.CreateNewBranch(wtPath, "feature-conflict", "main"); err != nil {
t.Fatalf("Failed to create worktree: %v", err)
}

// Make a change to README.md in the worktree (will conflict with remote)
conflictFile := filepath.Join(wtPath, "README.md")
if err := os.WriteFile(conflictFile, []byte("# Feature branch change\n"), 0644); err != nil {
t.Fatalf("Failed to write conflict file: %v", err)
}
cmd := exec.Command("git", "add", "README.md")
cmd.Dir = wtPath
if err := cmd.Run(); err != nil {
t.Fatalf("Failed to git add: %v", err)
}
cmd = exec.Command("git", "commit", "-m", "feature change to README")
cmd.Dir = wtPath
if err := cmd.Run(); err != nil {
t.Fatalf("Failed to commit: %v", err)
}

// Push a conflicting change to main via the remote
// We need to modify README.md on main too
remoteURL := ""
cmd = exec.Command("git", "remote", "get-url", "origin")
cmd.Dir = repoPath
if out, err := cmd.Output(); err == nil {
remoteURL = strings.TrimSpace(string(out))
}

tempClone, err := os.MkdirTemp("", "conflict-clone-*")
if err != nil {
t.Fatalf("Failed to create temp dir: %v", err)
}
defer os.RemoveAll(tempClone)

cmd = exec.Command("git", "clone", remoteURL, tempClone)
if err := cmd.Run(); err != nil {
t.Fatalf("Failed to clone: %v", err)
}
cmd = exec.Command("git", "config", "user.name", "Test User")
cmd.Dir = tempClone
cmd.Run()
cmd = exec.Command("git", "config", "user.email", "test@example.com")
cmd.Dir = tempClone
cmd.Run()

if err := os.WriteFile(filepath.Join(tempClone, "README.md"), []byte("# Conflicting main change\n"), 0644); err != nil {
t.Fatalf("Failed to write: %v", err)
}
cmd = exec.Command("git", "add", "README.md")
cmd.Dir = tempClone
if err := cmd.Run(); err != nil {
t.Fatalf("Failed to add: %v", err)
}
cmd = exec.Command("git", "commit", "-m", "conflicting main change")
cmd.Dir = tempClone
if err := cmd.Run(); err != nil {
t.Fatalf("Failed to commit: %v", err)
}
cmd = exec.Command("git", "push", "origin", "main")
cmd.Dir = tempClone
if err := cmd.Run(); err != nil {
t.Fatalf("Failed to push: %v", err)
}

// Now refresh the worktree - should detect conflict, abort, and return clean state
result := RefreshWorktree(wtPath, "origin", "main")

if result.Error == nil {
t.Fatal("Expected error from conflicting rebase")
}
if !result.HasConflicts {
t.Error("Expected HasConflicts to be true")
}
if len(result.ConflictFiles) == 0 {
t.Error("Expected conflict files to be reported")
}

// Verify worktree is not left in mid-rebase state
gitDir := resolveGitDir(wtPath)
if _, err := os.Stat(filepath.Join(gitDir, "rebase-merge")); err == nil {
t.Error("Worktree should not be in mid-rebase state after abort")
}
if _, err := os.Stat(filepath.Join(gitDir, "rebase-apply")); err == nil {
t.Error("Worktree should not be in mid-rebase-apply state after abort")
}

// Verify the worktree is still on the feature branch
cmd = exec.Command("git", "symbolic-ref", "--short", "HEAD")
cmd.Dir = wtPath
branchOutput, err := cmd.Output()
if err != nil {
t.Fatalf("Failed to get branch: %v", err)
}
if strings.TrimSpace(string(branchOutput)) != "feature-conflict" {
t.Errorf("Expected branch feature-conflict, got %s", strings.TrimSpace(string(branchOutput)))
}
}

func TestRefreshWorktreePreFetched(t *testing.T) {
repoPath, cleanup := createTestRepoWithRemote(t)
defer cleanup()

manager := NewManager(repoPath)

// Create a worktree
wtPath := filepath.Join(repoPath, "wt-prefetched")
if err := manager.CreateNewBranch(wtPath, "feature-prefetched", "main"); err != nil {
t.Fatalf("Failed to create worktree: %v", err)
}

// Add a commit to remote
addCommitToRemote(t, repoPath, "remote-change")

// Manually fetch first (simulating daemon behavior)
cmd := exec.Command("git", "fetch", "origin", "main")
cmd.Dir = wtPath
if err := cmd.Run(); err != nil {
t.Fatalf("Failed to fetch: %v", err)
}

// Use PreFetched variant - should succeed without fetching again
result := RefreshWorktreePreFetched(wtPath, "origin", "main")
if result.Error != nil {
t.Errorf("Unexpected error: %v", result.Error)
}
if result.Skipped {
t.Errorf("Should not have been skipped: %s", result.SkipReason)
}
}

func TestResolveGitDir(t *testing.T) {
repoPath, cleanup := createTestRepo(t)
defer cleanup()

manager := NewManager(repoPath)

// Create a worktree
wtPath := filepath.Join(repoPath, "wt-gitdir")
if err := manager.CreateNewBranch(wtPath, "feature-gitdir", "main"); err != nil {
t.Fatalf("Failed to create worktree: %v", err)
}

// resolveGitDir should return a valid path that exists
gitDir := resolveGitDir(wtPath)
if _, err := os.Stat(gitDir); os.IsNotExist(err) {
t.Errorf("resolveGitDir returned non-existent path: %s", gitDir)
}

// The resolved path should be absolute
if !filepath.IsAbs(gitDir) {
t.Errorf("resolveGitDir should return absolute path, got: %s", gitDir)
}

// For a regular repo (not a worktree), should return .git dir
mainGitDir := resolveGitDir(repoPath)
expectedMainGitDir := filepath.Join(repoPath, ".git")
if mainGitDir != expectedMainGitDir {
t.Errorf("resolveGitDir for main repo = %s, want %s", mainGitDir, expectedMainGitDir)
}
}
89 changes: 65 additions & 24 deletions internal/worktree/worktree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,27 @@ func resolvePathWithSymlinks(path string) (string, error) {
return evalPath, nil
}

// resolveGitDir resolves the actual git directory for a worktree path.
// For regular repos, .git is a directory. For worktrees, .git is a file
// containing "gitdir: <path>" pointing to the real git dir. The path may be
// relative to the worktree, so we resolve it to absolute.
func resolveGitDir(worktreePath string) string {
gitDir := filepath.Join(worktreePath, ".git")
content, err := os.ReadFile(gitDir)
if err != nil {
return gitDir
}
s := string(content)
if !strings.HasPrefix(s, "gitdir:") {
return gitDir
}
resolved := strings.TrimSpace(strings.TrimPrefix(s, "gitdir:"))
if !filepath.IsAbs(resolved) {
resolved = filepath.Join(worktreePath, resolved)
}
return filepath.Clean(resolved)
}

// Create creates a new git worktree
func (m *Manager) Create(path, branch string) error {
_, err := m.runGit("worktree", "add", path, branch)
Expand Down Expand Up @@ -643,11 +664,7 @@ func GetWorktreeState(worktreePath string, remote string, mainBranch string) (Wo
}

// Check for mid-rebase state
gitDir := filepath.Join(worktreePath, ".git")
// For worktrees, .git is a file pointing to the real git dir
if content, err := os.ReadFile(gitDir); err == nil && strings.HasPrefix(string(content), "gitdir:") {
gitDir = strings.TrimSpace(strings.TrimPrefix(string(content), "gitdir:"))
}
gitDir := resolveGitDir(worktreePath)
rebaseDir := filepath.Join(gitDir, "rebase-merge")
rebaseApplyDir := filepath.Join(gitDir, "rebase-apply")
if _, err := os.Stat(rebaseDir); err == nil {
Expand Down Expand Up @@ -740,17 +757,24 @@ type RefreshResult struct {
// It fetches from the remote, stashes any uncommitted changes, rebases onto main,
// and restores the stash. Returns detailed results about what happened.
func RefreshWorktree(worktreePath string, remote string, mainBranch string) RefreshResult {
return refreshWorktree(worktreePath, remote, mainBranch, false)
}

// RefreshWorktreePreFetched is like RefreshWorktree but skips the fetch step.
// Use this when the caller has already fetched from the remote (e.g., the daemon
// fetches once per repo before refreshing multiple worktrees).
func RefreshWorktreePreFetched(worktreePath string, remote string, mainBranch string) RefreshResult {
return refreshWorktree(worktreePath, remote, mainBranch, true)
}

func refreshWorktree(worktreePath string, remote string, mainBranch string, skipFetch bool) RefreshResult {
result := RefreshResult{
WorktreePath: worktreePath,
}

// Check for detached HEAD, mid-rebase, or mid-merge states
// These must be resolved before we can safely refresh
gitDir := filepath.Join(worktreePath, ".git")
// For worktrees, .git is a file pointing to the real git dir
if content, err := os.ReadFile(gitDir); err == nil && strings.HasPrefix(string(content), "gitdir:") {
gitDir = strings.TrimSpace(strings.TrimPrefix(string(content), "gitdir:"))
}
gitDir := resolveGitDir(worktreePath)

// Check for mid-rebase state
if _, err := os.Stat(filepath.Join(gitDir, "rebase-merge")); err == nil {
Expand Down Expand Up @@ -797,12 +821,14 @@ func RefreshWorktree(worktreePath string, remote string, mainBranch string) Refr
return result
}

// Fetch latest from remote
cmd = exec.Command("git", "fetch", remote, mainBranch)
cmd.Dir = worktreePath
if output, err := cmd.CombinedOutput(); err != nil {
result.Error = fmt.Errorf("failed to fetch from %s: %w\nOutput: %s", remote, err, output)
return result
// Fetch latest from remote (skip if caller already fetched)
if !skipFetch {
cmd = exec.Command("git", "fetch", remote, mainBranch)
cmd.Dir = worktreePath
if output, err := cmd.CombinedOutput(); err != nil {
result.Error = fmt.Errorf("failed to fetch from %s: %w\nOutput: %s", remote, err, output)
return result
}
}

// Check for uncommitted changes
Expand Down Expand Up @@ -845,14 +871,19 @@ func RefreshWorktree(worktreePath string, remote string, mainBranch string) Refr
if len(conflictFiles) > 0 && conflictFiles[0] != "" {
result.HasConflicts = true
result.ConflictFiles = conflictFiles
// Abort the rebase to leave the worktree in a clean state
abortCmd := exec.Command("git", "rebase", "--abort")
abortCmd.Dir = worktreePath
abortCmd.Run()
}

// Abort the rebase to leave the worktree in a clean state
abortCmd := exec.Command("git", "rebase", "--abort")
abortCmd.Dir = worktreePath
if abortErr := abortCmd.Run(); abortErr != nil {
result.Error = fmt.Errorf("rebase failed and abort also failed (manual cleanup needed): rebase: %w, abort: %v\nOutput: %s", rebaseErr, abortErr, rebaseOutput)
return result
}

result.Error = fmt.Errorf("rebase failed: %w\nOutput: %s", rebaseErr, rebaseOutput)

// Restore stash if we stashed
// Restore stash if we stashed (safe now that rebase is aborted)
if result.WasStashed {
popCmd := exec.Command("git", "stash", "pop")
popCmd.Dir = worktreePath
Expand All @@ -873,9 +904,19 @@ func RefreshWorktree(worktreePath string, remote string, mainBranch string) Refr
if result.WasStashed {
cmd = exec.Command("git", "stash", "pop")
cmd.Dir = worktreePath
if err := cmd.Run(); err != nil {
// Stash pop might fail if there are conflicts
result.Error = fmt.Errorf("stash pop failed (manual resolution may be needed): %w", err)
popOutput, popErr := cmd.CombinedOutput()
if popErr != nil {
// Check if stash pop caused conflicts
conflictCmd := exec.Command("git", "diff", "--name-only", "--diff-filter=U")
conflictCmd.Dir = worktreePath
if cOutput, cErr := conflictCmd.Output(); cErr == nil {
files := strings.Split(strings.TrimSpace(string(cOutput)), "\n")
if len(files) > 0 && files[0] != "" {
result.HasConflicts = true
result.ConflictFiles = files
}
}
result.Error = fmt.Errorf("stash pop failed (manual resolution may be needed): %w\nOutput: %s", popErr, popOutput)
} else {
result.StashRestored = true
}
Expand Down