Skip to content
Draft
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
9 changes: 9 additions & 0 deletions internal/git/git.go
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,15 @@ func (r *Repo) Cmd(ctx context.Context, args []string, env []string) *exec.Cmd {
return cmd
}

// runInDir runs a git command from the specified directory instead of the repo directory.
// This is used when a branch is checked out in a different worktree.
func (r *Repo) runInDir(ctx context.Context, dir string, opts *RunOpts) (*Output, error) {
saved := r.repoDir
r.repoDir = dir
defer func() { r.repoDir = saved }()
return r.Run(ctx, opts)
Comment on lines +189 to +192

Choose a reason for hiding this comment

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

high

This implementation of runInDir is not safe for concurrent use. It mutates the repoDir field of the Repo struct, which can lead to race conditions if other goroutines are using the same Repo instance. A safer approach is to create a shallow copy of the Repo object and modify its repoDir for the scope of this call, avoiding mutation of the original object.

Suggested change
saved := r.repoDir
r.repoDir = dir
defer func() { r.repoDir = saved }()
return r.Run(ctx, opts)
rCopy := *r
rCopy.repoDir = dir
return (&rCopy).Run(ctx, opts)

}

func (r *Repo) Run(ctx context.Context, opts *RunOpts) (*Output, error) {
cmd := r.Cmd(ctx, opts.Args, opts.Env)
var stdout, stderr bytes.Buffer
Expand Down
11 changes: 11 additions & 0 deletions internal/git/rebase.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,18 @@ func (r *Repo) Rebase(ctx context.Context, opts RebaseOpts) (*Output, error) {
args = append(args, "--onto", opts.Onto)
}
args = append(args, opts.Upstream)

// If the branch is checked out in another worktree, run the rebase from
// that worktree directory instead. git rebase <branch> first does a checkout
// which fails if the branch is in a different worktree.
if opts.Branch != "" {
worktreePath, err := r.WorktreeForBranch(ctx, opts.Branch)
if err != nil {
return nil, err
}
if worktreePath != "" {
return r.runInDir(ctx, worktreePath, &RunOpts{Args: args})
}
args = append(args, opts.Branch)
}

Expand Down
38 changes: 38 additions & 0 deletions internal/git/worktree.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package git

import (
"context"
"path/filepath"
"strings"
)

// WorktreeForBranch returns the worktree path where the given branch is checked out,
// or an empty string if the branch is not checked out in any worktree.
// The branch should be in short format (e.g., "my-branch").
func (r *Repo) WorktreeForBranch(ctx context.Context, branch string) (string, error) {
out, err := r.Run(ctx, &RunOpts{
Args: []string{"worktree", "list", "--porcelain"},
ExitError: true,
})
if err != nil {
return "", err
}

repoDir, _ := filepath.EvalSymlinks(r.repoDir)

Choose a reason for hiding this comment

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

medium

The error returned by filepath.EvalSymlinks is ignored. If resolving the symlink fails, repoDir will contain the original, unresolved path. This could lead to incorrect comparisons later on, especially on systems like macOS where paths like /var are symlinks to /private/var. The error should be handled to ensure correctness.

	repoDir, err := filepath.EvalSymlinks(r.repoDir)
	if err != nil {
		return "", err
	}

var currentWorktree string
for line := range strings.SplitSeq(string(out.Stdout), "\n") {
if path, ok := strings.CutPrefix(line, "worktree "); ok {
currentWorktree = path
}
if ref, ok := strings.CutPrefix(line, "branch "); ok {
shortName := strings.TrimPrefix(ref, "refs/heads/")
if shortName == branch {
resolved, _ := filepath.EvalSymlinks(currentWorktree)

Choose a reason for hiding this comment

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

medium

Similar to the earlier comment, the error from filepath.EvalSymlinks is ignored here. This can lead to incorrect behavior if the worktree path cannot be resolved. The error should be handled to ensure the comparison against the main repository directory is reliable.

				resolved, err := filepath.EvalSymlinks(currentWorktree)
				if err != nil {
					return "", err
				}

if resolved != repoDir {
return currentWorktree, nil
}
}
}
}
return "", nil
}
80 changes: 80 additions & 0 deletions internal/git/worktree_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
package git_test

import (
"path/filepath"
"strings"
"testing"

"github.com/aviator-co/av/internal/git"
"github.com/aviator-co/av/internal/git/gittest"
"github.com/stretchr/testify/require"
)

func TestWorktreeForBranch(t *testing.T) {
repo := gittest.NewTempRepo(t)
avRepo := repo.AsAvGitRepo()

// Create a feature branch with a commit.
repo.Git(t, "checkout", "-b", "feature-1")
repo.CommitFile(t, "feature1.txt", "feature 1")
repo.Git(t, "checkout", "main")

// feature-1 is not checked out in any worktree, so should return empty.
wt, err := avRepo.WorktreeForBranch(t.Context(), "feature-1")
require.NoError(t, err)
require.Empty(t, wt)

// Add a worktree for feature-1.
wtPath := t.TempDir()
repo.Git(t, "worktree", "add", wtPath, "feature-1")

// Now feature-1 should be detected in the worktree.
wt, err = avRepo.WorktreeForBranch(t.Context(), "feature-1")
require.NoError(t, err)
// Resolve symlinks for comparison (macOS /var -> /private/var).
resolvedWt, _ := filepath.EvalSymlinks(wt)
resolvedExpected, _ := filepath.EvalSymlinks(wtPath)
Comment on lines +35 to +36

Choose a reason for hiding this comment

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

medium

Errors from filepath.EvalSymlinks are being ignored. In tests, it's important to check for errors to ensure the test setup is correct and the function is being tested under the right conditions. If EvalSymlinks were to fail, this test could produce misleading results. Please handle the errors, for example by using require.NoError(t, err).

Suggested change
resolvedWt, _ := filepath.EvalSymlinks(wt)
resolvedExpected, _ := filepath.EvalSymlinks(wtPath)
resolvedWt, err := filepath.EvalSymlinks(wt)
require.NoError(t, err)
resolvedExpected, err := filepath.EvalSymlinks(wtPath)
require.NoError(t, err)

require.Equal(t, resolvedExpected, resolvedWt)

// main is checked out in the main repo, not a different worktree.
wt, err = avRepo.WorktreeForBranch(t.Context(), "main")
require.NoError(t, err)
require.Empty(t, wt)
}

func TestRebaseInWorktree(t *testing.T) {
repo := gittest.NewTempRepo(t)
avRepo := repo.AsAvGitRepo()

// Create a stack: main -> feature-1 -> feature-2
repo.Git(t, "checkout", "-b", "feature-1")
repo.CommitFile(t, "feature1.txt", "feature 1")
repo.Git(t, "checkout", "-b", "feature-2")
repo.CommitFile(t, "feature2.txt", "feature 2")
repo.Git(t, "checkout", "main")

// Add a new commit on main to create something to restack onto.
repo.CommitFile(t, "main-update.txt", "main update")

// Put feature-1 in a separate worktree.
wtPath := t.TempDir()
repo.Git(t, "worktree", "add", wtPath, "feature-1")

// Rebase feature-1 onto main (simulating restack).
// This would fail without worktree-aware rebase because feature-1
// is checked out in another worktree.
mainHash := strings.TrimSpace(repo.Git(t, "rev-parse", "main"))

// Get the merge-base for feature-1 and main (the original branch point).
mergeBase := strings.TrimSpace(repo.Git(t, "merge-base", "main", "feature-1"))

result, err := avRepo.RebaseParse(t.Context(), git.RebaseOpts{
Branch: "feature-1",
Upstream: mergeBase,
Onto: mainHash,
})
require.NoError(t, err)
require.NotNil(t, result)
// The rebase should succeed (updated or already up to date).
require.NotEqual(t, git.RebaseConflict, result.Status)
}