test(workspaceindex): pin Scan's fallback when gitsafe refuses the repository

A repository whose own config defines a filter or diff command is refused by
gitsafe, and Scan then takes the non-git path. Scan reaches gitsafe.Check
through a package variable now, so a test can stand in for such a repository
without configuring one that names a command, and
TestScanFallsBackWhenGitsafeRefusesTheRepository checks both sides: with git
consulted, .gitignore decides and the fixed list does not apply; refused, the
fixed list applies and .gitignore is not read.

internal/gitsafe/callers_test.go

@@ -12,11 +12,13 @@ import (
 )
 
 // The packages whose git calls Zero makes on its own. The CLI runs git for the
-// scratch-file warning and the TUI for its files sweep and pull request
-// diffstat; each call has to come through Command.
+// scratch-file warning, the TUI for its files sweep and pull request diffstat,
+// and workspaceindex for the ignored paths Scan skips on every turn; each call
+// has to come through Command or a Checked.
 var automaticGitPackages = []string{
 	filepath.Join("..", "cli"),
 	filepath.Join("..", "tui"),
+	filepath.Join("..", "workspaceindex"),
 }
 
 // parsePackage parses the non-test Go files in dir.
@@ -112,14 +114,21 @@ func TestAutomaticGitCallsDoNotExecGitDirectly(t *testing.T) {
 	}
 }
 
-// gitArguments returns the git arguments of call when it is gitsafe.Command or a
-// runner called with the program "git" (the TUI's runner(ctx, cwd, "git", ...)),
-// as far as they are string literals.
+// gitArguments returns the git arguments of call when it is gitsafe.Command, a
+// Command on a Checked in a file that imports gitsafe (checked.Command(ctx, ...)),
+// or a runner called with the program "git" (the TUI's runner(ctx, cwd, "git",
+// ...)), as far as they are string literals.
 func gitArguments(call *ast.CallExpr, gitsafeName string) ([]string, bool) {
 	start := -1
 	if selector, ok := call.Fun.(*ast.SelectorExpr); ok && gitsafeName != "" && selector.Sel.Name == "Command" {
-		if pkg, ok := selector.X.(*ast.Ident); ok && pkg.Name == gitsafeName {
-			start = 2
+		if receiver, ok := selector.X.(*ast.Ident); ok {
+			switch receiver.Name {
+			case gitsafeName:
+				start = 2
+			case "exec":
+			default:
+				start = 1
+			}
 		}
 	}
 	if start < 0 {
@@ -174,10 +183,10 @@ func TestAutomaticStatusAndDiffSkipSubmoduleWorkTrees(t *testing.T) {
 			})
 		}
 	}
-	// The scratch-file status, the files sweep's status and diff, and the pull
-	// request diffstat.
-	if checked != 4 {
-		t.Fatalf("checked %d automatic status or diff calls, want 4; update this test when they change", checked)
+	// The scratch-file status, the files sweep's status and diff, the pull request
+	// diffstat, and Scan's ignored-path status.
+	if checked != 5 {
+		t.Fatalf("checked %d automatic status or diff calls, want 5; update this test when they change", checked)
 	}
 }
 

internal/gitsafe/gitsafe.go

@@ -26,6 +26,7 @@ import (
 	"os"
 	"os/exec"
 	"runtime"
+	"slices"
 	"strings"
 )
 
@@ -57,6 +58,26 @@ const (
 // --ignore-submodules=dirty: a submodule has its own config, which this does not
 // read, and git only runs commands there when it looks inside the submodule.
 func Command(ctx context.Context, dir string, args ...string) (*exec.Cmd, error) {
+	checked, err := Check(ctx, dir)
+	if err != nil {
+		return nil, err
+	}
+	return checked.Command(ctx, args...), nil
+}
+
+// Checked is a directory whose repository config Check has read, for running
+// several commands there at the cost of one config read. Reading the config
+// takes a git process of its own, and a caller that runs git more than once per
+// call, like Scan on every turn, would otherwise pay for it each time.
+type Checked struct {
+	dir string
+	env []string
+}
+
+// Check reads the repository config for dir and fails the way Command does:
+// with ErrRepositoryDefinesCommands, or with git's own error when dir is not
+// inside a repository.
+func Check(ctx context.Context, dir string) (*Checked, error) {
 	env := environment()
 	defines, err := repositoryDefinesCommands(ctx, dir, env)
 	if err != nil {
@@ -65,13 +86,20 @@ func Command(ctx context.Context, dir string, args ...string) (*exec.Cmd, error)
 	if defines {
 		return nil, ErrRepositoryDefinesCommands
 	}
+	return &Checked{dir: dir, env: env}, nil
+}
+
+// Command returns git with args, run in the checked directory, with the
+// programs a repository can configure turned off.
+func (checked *Checked) Command(ctx context.Context, args ...string) *exec.Cmd {
 	command := exec.CommandContext(ctx, "git", append(disabledPrograms(), args...)...)
-	command.Dir = dir
+	command.Dir = checked.dir
 	// No opportunistic index refresh: a read-only call never rewrites the index,
 	// so it never triggers the hooks that run when the index is written. Git
-	// before 2.15 does not know the variable and ignores it.
-	command.Env = append(env, "GIT_OPTIONAL_LOCKS=0")
-	return command, nil
+	// before 2.15 does not know the variable and ignores it. Each command gets
+	// its own copy, so two commands never share a backing array.
+	command.Env = append(slices.Clone(checked.env), "GIT_OPTIONAL_LOCKS=0")
+	return command
 }
 
 // disabledPrograms turns off the single-key ways a repository runs a program

internal/gitsafe/gitsafe_test.go

@@ -10,6 +10,7 @@ import (
 	"reflect"
 	"regexp"
 	"runtime"
+	"slices"
 	"strings"
 	"testing"
 )
@@ -77,6 +78,34 @@ func TestCommandTurnsOffRepositoryPrograms(t *testing.T) {
 	}
 }
 
+// Check reads the repository config once, and every command built from it gets
+// the same flags, the same directory and its own copy of the environment.
+func TestCheckReadsTheConfigOnceForSeveralCommands(t *testing.T) {
+	calls := answerConfig(t, noEntries)
+	dir := t.TempDir()
+	checked, err := Check(context.Background(), dir)
+	if err != nil {
+		t.Fatalf("Check: %v", err)
+	}
+	status := checked.Command(context.Background(), "status")
+	diff := checked.Command(context.Background(), "diff")
+	if got := scopes(calls); !reflect.DeepEqual(got, []string{"--local"}) {
+		t.Fatalf("config reads = %q, want one --local read for both commands", got)
+	}
+	for _, command := range []*exec.Cmd{status, diff} {
+		if !slices.Contains(command.Args, "core.fsmonitor=false") || command.Dir != dir {
+			t.Fatalf("command %q in %q lost its flags or its directory", command.Args, command.Dir)
+		}
+		if value, _ := effectiveEnv(command.Env, "GIT_OPTIONAL_LOCKS"); value != "0" {
+			t.Fatalf("effective GIT_OPTIONAL_LOCKS = %q, want 0", value)
+		}
+	}
+	status.Env[0] = "ZERO_GITSAFE_TEST_CHANGED=1"
+	if diff.Env[0] == status.Env[0] {
+		t.Fatal("two commands from one check share an environment")
+	}
+}
+
 func TestCommandDropsRepositoryLocationVariables(t *testing.T) {
 	// Turn per-worktree config on so both reads happen and both are checked.
 	calls := answerConfig(t, func(scope string) ([]configEntry, error) {

internal/workspaceindex/gitignore.go

@@ -3,11 +3,11 @@ package workspaceindex
 import (
 	"bytes"
 	"context"
-	"os"
-	"os/exec"
 	"path"
 	"strings"
 	"time"
+
+	"github.com/Gitlawb/zero/internal/gitsafe"
 )
 
 // gitIgnoredPathsTimeout bounds how long Scan waits on git before giving up and falling
@@ -15,6 +15,10 @@ import (
 // degenerate tree (network filesystem, huge monorepo) must not stall a turn.
 const gitIgnoredPathsTimeout = 5 * time.Second
 
+// checkRepository is gitsafe.Check. It is a variable so tests can stand in for a
+// repository gitsafe refuses, without configuring one that names a command.
+var checkRepository = gitsafe.Check
+
 // gitIgnoredPaths asks git which paths under root it considers ignored, honoring every
 // source git itself resolves (nested .gitignore files, .git/info/exclude,
 // core.excludesFile). Returns a set of paths relative to root: whole ignored directories
@@ -22,7 +26,8 @@ const gitIgnoredPathsTimeout = 5 * time.Second
 // trailing slash (the `rel` form Scan computes).
 //
 // Returns (nil, false) when root is not inside a git work tree, git is missing or older
-// than the floor below, or a command fails or times out. Scan then behaves as it does
+// than the floor below, the repository's own config defines a filter or diff command
+// (see gitsafe), or a command fails or times out. Scan then behaves as it does
 // outside git. A scan must never fail or hang because of this lookup.
 //
 // Git version floor: 2.16. `status --ignored=<mode>` landed in 2.16 and `--porcelain=v1`
@@ -39,11 +44,24 @@ func gitIgnoredPaths(root string) (map[string]bool, bool) {
 	ctx, cancel := context.WithTimeout(context.Background(), gitIgnoredPathsTimeout)
 	defer cancel()
 
+	// Both calls below go through one gitsafe check of the repository's config, so
+	// the repository under scan cannot make them run a program (core.fsmonitor, its
+	// hooks, or a filter or diff driver its own config defines) and an inherited
+	// GIT_DIR or GIT_WORK_TREE cannot point them at another repository. A repository
+	// whose own config defines a filter or diff command takes the non-git path.
+	// gitsafe also sets GIT_OPTIONAL_LOCKS=0, so status never takes .git/index.lock to
+	// refresh the index: Scan runs every turn, often while the user or the model runs
+	// git in the same repository, and that lock makes a concurrent `git commit` or
+	// `git add` fail with "index.lock exists".
+	checked, err := checkRepository(ctx, root)
+	if err != nil {
+		return nil, false
+	}
+
 	// status reports paths relative to the repository top level even when run from a
 	// subdirectory, while Scan's `rel` is relative to root. show-prefix is root's own
-	// location inside the repo ("" at the top level, "svc/" for <repo>/svc), and also the
-	// cheapest way to learn whether this is a git work tree at all.
-	prefixOut, err := gitCommand(ctx, root, "rev-parse", "--show-prefix").Output()
+	// location inside the repo ("" at the top level, "svc/" for <repo>/svc).
+	prefixOut, err := checked.Command(ctx, "rev-parse", "--show-prefix").Output()
 	if err != nil {
 		return nil, false
 	}
@@ -54,7 +72,9 @@ func gitIgnoredPaths(root string) (map[string]bool, bool) {
 	// The trailing "." limits the report to root when root is a repository subdirectory.
 	// --untracked-files=normal: status.showUntrackedFiles=no makes git reject
 	// --ignored=matching; without an explicit override Scan falls back to the fixed list.
-	out, err := gitCommand(ctx, root, "status", "--porcelain=v1", "-z", "--untracked-files=normal", "--ignored=matching", ".").Output()
+	// --ignore-submodules=dirty keeps git out of submodule work trees, whose own config
+	// the gitsafe check does not read.
+	out, err := checked.Command(ctx, "status", "--porcelain=v1", "-z", "--untracked-files=normal", "--ignored=matching", "--ignore-submodules=dirty", ".").Output()
 	if err != nil {
 		return nil, false
 	}
@@ -101,39 +121,3 @@ func parseIgnoredStatus(out []byte, prefix string) map[string]bool {
 	}
 	return paths
 }
-
-// gitCommand builds a git invocation with a fixed argv (no shell) rooted at dir.
-//
-// GIT_OPTIONAL_LOCKS=0 stops `git status` from taking .git/index.lock to refresh the
-// index stat cache. Scan runs on every turn, often while the user or the agent is running
-// git in the same repository; an opportunistic lock there can make a concurrent
-// `git commit` or `git add` fail with "index.lock exists".
-func gitCommand(ctx context.Context, dir string, args ...string) *exec.Cmd {
-	// #nosec G204 -- fixed argv, no shell; dir is the caller's own absolute path.
-	// core.fsmonitor in .git/config can point at a command git status runs; disable it so
-	// Scan never executes foreign hooks from the repository under scan.
-	cmd := exec.CommandContext(ctx, "git", append([]string{"-C", dir, "-c", "core.fsmonitor=false"}, args...)...)
-	cmd.Env = append(withoutRepoLocationEnv(os.Environ()), "GIT_OPTIONAL_LOCKS=0")
-	return cmd
-}
-
-// withoutRepoLocationEnv drops the repository path overrides from git(1),
-// "The Git Repository": GIT_DIR, GIT_WORK_TREE, GIT_INDEX_FILE, GIT_COMMON_DIR,
-// GIT_OBJECT_DIRECTORY, and GIT_ALTERNATE_OBJECT_DIRECTORIES. Hooks export
-// them (githooks(5)) so git commands inside the hook find that repository.
-// Inherited, -C dir still describes the hook's repository, and Scan applies
-// its ignore rules instead of the tree under dir. GIT_CEILING_DIRECTORIES is
-// not one of these; it only bounds discovery and must be inherited.
-func withoutRepoLocationEnv(env []string) []string {
-	out := make([]string, 0, len(env))
-	for _, kv := range env {
-		key, _, _ := strings.Cut(kv, "=")
-		switch key {
-		case "GIT_DIR", "GIT_WORK_TREE", "GIT_INDEX_FILE", "GIT_COMMON_DIR",
-			"GIT_OBJECT_DIRECTORY", "GIT_ALTERNATE_OBJECT_DIRECTORIES":
-			continue
-		}
-		out = append(out, kv)
-	}
-	return out
-}

internal/workspaceindex/gitignore_test.go

@@ -9,6 +9,8 @@ import (
 	"reflect"
 	"strings"
 	"testing"
+
+	"github.com/Gitlawb/zero/internal/gitsafe"
 )
 
 // isolateGitConfig keeps the git child processes away from the developer's real global and
@@ -189,6 +191,50 @@ func TestScanSkipsBuildCacheDirsOutsideGit(t *testing.T) {
 	}
 }
 
+// A repository whose own config defines a filter or diff command is refused by gitsafe,
+// and Scan then takes the non-git path, as it does when git cannot run: .gitignore is
+// not consulted and the fixed list applies. How gitsafe detects such a repository is
+// covered by its own tests; this pins what Scan does with the refusal.
+func TestScanFallsBackWhenGitsafeRefusesTheRepository(t *testing.T) {
+	root := t.TempDir()
+	initGitRepo(t, root, ".locust_env/\n")
+	writeFile(t, root, "main.go", "package main\n")
+	writeFile(t, root, ".locust_env/lib/x.py", "skipped by .gitignore, kept once git is refused")
+	writeFile(t, root, "target/debug/app.d", "kept by git, skipped by the fixed list")
+
+	// Inside a work tree git decides, so the fixed list does not apply.
+	honored, err := Scan(root, Options{MaxDepth: DefaultMaxDepth})
+	if err != nil {
+		t.Fatalf("Scan: %v", err)
+	}
+	if want := []string{".gitignore", "main.go", "target/debug/app.d"}; !reflect.DeepEqual(pathsOf(honored.Files), want) {
+		t.Fatalf("Files=%v want %v with git consulted", pathsOf(honored.Files), want)
+	}
+
+	refusals := 0
+	previous := checkRepository
+	checkRepository = func(context.Context, string) (*gitsafe.Checked, error) {
+		refusals++
+		return nil, gitsafe.ErrRepositoryDefinesCommands
+	}
+	t.Cleanup(func() { checkRepository = previous })
+
+	if paths, ok := gitIgnoredPaths(root); ok || paths != nil {
+		t.Fatalf("gitIgnoredPaths for a refused repository = (%v, %v), want (nil, false)", paths, ok)
+	}
+	before := refusals
+	got, err := Scan(root, Options{MaxDepth: DefaultMaxDepth})
+	if err != nil {
+		t.Fatalf("Scan: %v", err)
+	}
+	if refusals == before {
+		t.Fatal("Scan never asked gitsafe about the repository")
+	}
+	if want := []string{".gitignore", ".locust_env/lib/x.py", "main.go"}; !reflect.DeepEqual(pathsOf(got.Files), want) {
+		t.Fatalf("Files=%v want %v from the fixed list", pathsOf(got.Files), want)
+	}
+}
+
 // When git cannot run at all, Scan must not error and must behave exactly as outside a
 // repository, even inside one: .gitignore is not consulted, the fixed list applies.
 func TestScanFallsBackWhenGitUnavailable(t *testing.T) {
@@ -330,23 +376,6 @@ func TestScanIgnoresInheritedRepositoryLocation(t *testing.T) {
 	}
 }
 
-// The ignored-path lookup runs every turn, often next to a git command the user or agent
-// is running in the same repository. It must not take the optional index lock, and a
-// GIT_OPTIONAL_LOCKS value inherited from the environment must not re-enable it.
-func TestGitCommandDisablesOptionalLocks(t *testing.T) {
-	t.Setenv("GIT_OPTIONAL_LOCKS", "1")
-	cmd := gitCommand(context.Background(), t.TempDir(), "status")
-	value := ""
-	for _, entry := range cmd.Env {
-		if after, ok := strings.CutPrefix(entry, "GIT_OPTIONAL_LOCKS="); ok {
-			value = after
-		}
-	}
-	if value != "0" {
-		t.Fatalf("effective GIT_OPTIONAL_LOCKS=%q want \"0\"", value)
-	}
-}
-
 // A rename or copy record carries its original path in the next NUL field, with no XY
 // prefix. An original name that starts with "!! " must not be read as an ignored entry.
 func TestParseIgnoredStatusSkipsRenameAndCopyOrigPaths(t *testing.T) {