Skip to content
Merged
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
2 changes: 1 addition & 1 deletion internal/git/gitops.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ func CurrentOps() Ops {
// --- defaultOps method implementations ---

func (d *defaultOps) GitDir() (string, error) {
return d.path("rev-parse", "--absolute-git-dir")
return d.directory("--absolute-git-dir")
}

func (d *defaultOps) RootDir() (string, error) {
Expand Down
37 changes: 37 additions & 0 deletions internal/git/gitops_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1058,6 +1058,43 @@ func TestIntegration_ForWorktreeIndependentExecutors(t *testing.T) {
assert.Equal(t, "main", gitExec(t, dir, "branch", "--show-current"))
}

func TestIntegration_WorktreeDirectoriesReuseOnlyCurrentValidation(t *testing.T) {
root, dir := setupWorktreeRepo(t)
linked, path := addTestWorktree(t, root, dir, "feature")
tracePath := filepath.Join(t.TempDir(), "trace.jsonl")
t.Setenv("GIT_TRACE2_EVENT", tracePath)
common, err := linked.CommonDir()
require.NoError(t, err)
assert.Equal(t, canonicalGitTestPath(t, filepath.Join(dir, ".git")), common)
gitDir, err := linked.GitDir()
require.NoError(t, err)
assert.NotEqual(t, common, gitDir)
requireWorktree(t, linked, path)
trace, err := os.ReadFile(tracePath)
require.NoError(t, err)
probes := 0
for _, line := range strings.Split(strings.TrimSpace(string(trace)), "\n") {
var event struct {
Event string
Argv []string
}
require.NoError(t, json.Unmarshal([]byte(line), &event))
if event.Event == "start" && slices.Contains(event.Argv, "rev-parse") {
probes++
}
}
// 8 rev-parse probes:
// - CommonDir: 2 to validate the Git and common directories
// - GitDir: 2 for the same validations
// - ForWorktree: 4 (2 source validations + 2 destination lookups)
assert.Equal(t, 8, probes, "directory queries reuse current validation; selecting a scope still validates both directories")
Comment thread
skarim marked this conversation as resolved.
require.NoError(t, os.Rename(path, path+"-moved"))
_, err = linked.CommonDir()
require.Error(t, err, "the previous validation must not be cached across calls")
_, err = linked.GitDir()
require.Error(t, err)
}

func TestIntegration_WorktreeMissingCheckoutDoesNotReportStagedChanges(t *testing.T) {
t.Parallel()
root, dir := setupWorktreeRepo(t)
Expand Down
71 changes: 57 additions & 14 deletions internal/git/worktree.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,42 +32,81 @@ func (d *defaultOps) path(args ...string) (string, error) {
}

func (d *defaultOps) CommonDir() (string, error) {
return d.path("rev-parse", "--path-format=absolute", "--git-common-dir")
return d.directory("--git-common-dir")
}

func (d *defaultOps) gitClient() (*cligit.Client, error) {
type validatedGitClient struct {
client *cligit.Client
gitDir string
commonDir string
}

func (d *defaultOps) inspectGitClient() (validatedGitClient, error) {
c := d.client
if c == nil {
c = client
}
validated := validatedGitClient{client: c}
for _, directory := range []struct {
option string
info os.FileInfo
path *string
}{
{"--absolute-git-dir", d.gitDir},
{"--git-common-dir", d.commonDir},
{"--absolute-git-dir", d.gitDir, &validated.gitDir},
{"--git-common-dir", d.commonDir, &validated.commonDir},
} {
if directory.info == nil {
continue
}
cmd, err := c.Command(context.Background(), "rev-parse", "--path-format=absolute", directory.option)
if err != nil {
return nil, err
return validatedGitClient{}, err
}
d.configureCommand(cmd)
out, err := cmd.Output()
if err != nil {
return nil, fmt.Errorf("inspecting worktree %q: %w", c.RepoDir, err)
return validatedGitClient{}, fmt.Errorf("inspecting worktree %q: %w", c.RepoDir, err)
}
info, err := os.Stat(strings.TrimSuffix(string(out), "\n"))
path := strings.TrimSuffix(string(out), "\n")
info, err := os.Stat(path)
if err != nil {
return nil, fmt.Errorf("inspecting worktree %q: %w", c.RepoDir, err)
return validatedGitClient{}, fmt.Errorf("inspecting worktree %q: %w", c.RepoDir, err)
}
if !os.SameFile(directory.info, info) {
return nil, fmt.Errorf("worktree %q no longer refers to the selected Git directory; rediscover worktrees before continuing", c.RepoDir)
return validatedGitClient{}, fmt.Errorf("worktree %q no longer refers to the selected Git directory; rediscover worktrees before continuing", c.RepoDir)
}
*directory.path = path
}
return c, nil
return validated, nil
}

func (d *defaultOps) gitClient() (*cligit.Client, error) {
validated, err := d.inspectGitClient()
return validated.client, err
}

func (d *defaultOps) directory(option string) (string, error) {
validated, err := d.inspectGitClient()
if err != nil {
return "", err
}
// Reuse only paths validated by this call, never across commands.
if option == "--absolute-git-dir" && validated.gitDir != "" {
return validated.gitDir, nil
}
if option == "--git-common-dir" && validated.commonDir != "" {
return validated.commonDir, nil
}
cmd, err := validated.client.Command(context.Background(), "rev-parse", "--path-format=absolute", option)
if err != nil {
return "", err
}
d.configureCommand(cmd)
out, err := cmd.Output()
if err != nil {
return "", err
}
return strings.TrimSuffix(string(out), "\n"), nil
}

// ForWorktree resolves relative paths against this receiver's execution
Expand All @@ -76,13 +115,17 @@ func (d *defaultOps) ForWorktree(path string) (Ops, error) {
if path == "" {
return nil, errors.New("worktree path must not be empty")
}
c, err := d.gitClient()
validated, err := d.inspectGitClient()
if err != nil {
return nil, err
}
common, err := d.CommonDir()
if err != nil {
return nil, fmt.Errorf("locating the source repository: %w", err)
c := validated.client
common := validated.commonDir
if common == "" {
common, err = d.CommonDir()
if err != nil {
return nil, fmt.Errorf("locating the source repository: %w", err)
}
}
expectedCommon, err := os.Stat(common)
if err != nil {
Expand Down
46 changes: 25 additions & 21 deletions internal/worktree/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -202,36 +202,40 @@ func CheckClean(ops git.Ops, path string) error {
func (c *Context) Preflight(branches []string) error {
checked := make(map[string]bool)
for _, branch := range branches {
ops, err := c.Ops(branch)
if err != nil {
if _, err := c.preflightBranch(branch, checked); err != nil {
return err
}
location := c.Location(branch)
if !checked[location.Path] {
if err := CheckClean(ops, location.Path); err != nil {
return err
}
checked[location.Path] = true
}
return nil
}

func (c *Context) preflightBranch(branch string, checked map[string]bool) (git.Ops, error) {
ops, err := c.Ops(branch)
if err != nil {
return nil, err
}
location := c.Location(branch)
if !checked[location.Path] {
if err := CheckClean(ops, location.Path); err != nil {
return nil, err
}
if !SamePath(location.Path, c.Origin.Path) {
current, err := ops.CurrentBranch()
if err != nil {
return fmt.Errorf("checking current branch in worktree %s: %w", location.Path, err)
}
if current != branch {
return fmt.Errorf("worktree %s no longer has branch %s checked out", location.Path, branch)
}
checked[location.Path] = true
}
if !SamePath(location.Path, c.Origin.Path) {
current, err := ops.CurrentBranch()
if err != nil {
return nil, fmt.Errorf("checking current branch in worktree %s: %w", location.Path, err)
}
if current != branch {
return nil, fmt.Errorf("worktree %s no longer has branch %s checked out", location.Path, branch)
}
}
return nil
return ops, nil
}

// Prepare may switch the initiating worktree, but never a different worktree.
func (c *Context) Prepare(branch string) (git.Ops, error) {
if err := c.Preflight([]string{branch}); err != nil {
return nil, err
}
ops, err := c.Ops(branch)
ops, err := c.preflightBranch(branch, make(map[string]bool))
if err != nil {
return nil, err
}
Expand Down
65 changes: 65 additions & 0 deletions internal/worktree/context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -248,3 +248,68 @@ func TestStartRejectsCommitsAfterSnapshot(t *testing.T) {
assert.Empty(t, ctx.Pending)
assert.Empty(t, ctx.Touched)
}

func TestPrepareReusesOnlyTheCurrentPreflightExecutor(t *testing.T) {
common := t.TempDir()
current := "main"
resolutions := 0
scoped := &git.MockOps{
CommonDirFn: func() (string, error) { return common, nil },
GitDirFn: func() (string, error) { return common, nil },
CurrentBranchFn: func() (string, error) { return current, nil },
CheckoutBranchFn: func(branch string) error {
current = branch
return nil
},
}
restore := git.SetOps(&git.MockOps{
CommonDirFn: func() (string, error) { return common, nil },
ForWorktreeFn: func(string) (git.Ops, error) {
resolutions++
return scoped, nil
},
})
defer restore()
ctx := &Context{Origin: Location{Path: common}}
for i := 1; i <= 2; i++ {
ops, err := ctx.Prepare("feature")
require.NoError(t, err)
assert.Same(t, scoped, ops)
assert.Equal(t, i, resolutions, "resolve once per preparation, not once per context")
assert.Equal(t, "feature", current)
}

scoped.HasUncommittedChangesFn = func() (bool, error) { return true, nil }
ops, err := ctx.Prepare("other")
require.ErrorContains(t, err, "uncommitted changes")
assert.Nil(t, ops)
assert.Equal(t, 3, resolutions)
assert.Equal(t, "feature", current)
}

func TestPreflightChecksSharedOwnerOncePerCall(t *testing.T) {
common := t.TempDir()
resolutions, checks := 0, 0
scoped := &git.MockOps{
CommonDirFn: func() (string, error) { return common, nil },
GitDirFn: func() (string, error) { return common, nil },
HasUncommittedChangesFn: func() (bool, error) {
checks++
return false, nil
},
}
restore := git.SetOps(&git.MockOps{
CommonDirFn: func() (string, error) { return common, nil },
ForWorktreeFn: func(string) (git.Ops, error) {
resolutions++
return scoped, nil
},
})
defer restore()
ctx := &Context{Origin: Location{Path: common}}
for i := 1; i <= 2; i++ {
require.NoError(t, ctx.Preflight([]string{"one", "two"}))
assert.Equal(t, 2*i, resolutions, "each branch still resolves its current owner")
assert.Equal(t, i, checks, "cleanliness is shared only within this preflight")
}
}
Loading