From 0eea91b86aeef8260a01e9ed15bf04ed196bd138 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Wed, 21 Oct 2026 22:28:19 -0400 Subject: [PATCH 1/2] Reuse validated worktree metadata within operations --- internal/git/gitops.go | 2 +- internal/git/gitops_test.go | 33 ++++++++++++++ internal/git/worktree.go | 71 +++++++++++++++++++++++++------ internal/worktree/context.go | 46 +++++++++++--------- internal/worktree/context_test.go | 65 ++++++++++++++++++++++++++++ 5 files changed, 181 insertions(+), 36 deletions(-) diff --git a/internal/git/gitops.go b/internal/git/gitops.go index 1f37c16f..df95848b 100644 --- a/internal/git/gitops.go +++ b/internal/git/gitops.go @@ -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) { diff --git a/internal/git/gitops_test.go b/internal/git/gitops_test.go index a6a055fc..888fab55 100644 --- a/internal/git/gitops_test.go +++ b/internal/git/gitops_test.go @@ -1058,6 +1058,39 @@ 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++ + } + } + assert.Equal(t, 8, probes, "directory queries reuse current validation; selecting a scope still validates both directories") + 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) diff --git a/internal/git/worktree.go b/internal/git/worktree.go index c8464144..24a55e76 100644 --- a/internal/git/worktree.go +++ b/internal/git/worktree.go @@ -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 @@ -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 { diff --git a/internal/worktree/context.go b/internal/worktree/context.go index 0db11ca8..f1d054e2 100644 --- a/internal/worktree/context.go +++ b/internal/worktree/context.go @@ -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 } diff --git a/internal/worktree/context_test.go b/internal/worktree/context_test.go index 292e40bb..77e6ac40 100644 --- a/internal/worktree/context_test.go +++ b/internal/worktree/context_test.go @@ -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") + } +} From 84d5c479bd047fad33fae65878b55e6f1ad3a20a Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Thu, 22 Oct 2026 16:08:17 -0400 Subject: [PATCH 2/2] Add comment on where assertion count comes from --- internal/git/gitops_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/git/gitops_test.go b/internal/git/gitops_test.go index 888fab55..512b8d64 100644 --- a/internal/git/gitops_test.go +++ b/internal/git/gitops_test.go @@ -1083,6 +1083,10 @@ func TestIntegration_WorktreeDirectoriesReuseOnlyCurrentValidation(t *testing.T) 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") require.NoError(t, os.Rename(path, path+"-moved")) _, err = linked.CommonDir()