diff --git a/internal/gitsafe/callers_test.go b/internal/gitsafe/callers_test.go index c7978cf59..d5d6e0854 100644 --- a/internal/gitsafe/callers_test.go +++ b/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) } } diff --git a/internal/gitsafe/gitsafe.go b/internal/gitsafe/gitsafe.go index cb1c20110..b21509f00 100644 --- a/internal/gitsafe/gitsafe.go +++ b/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 diff --git a/internal/gitsafe/gitsafe_test.go b/internal/gitsafe/gitsafe_test.go index bf38e767e..41e5ec184 100644 --- a/internal/gitsafe/gitsafe_test.go +++ b/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) { diff --git a/internal/workspaceindex/gitignore.go b/internal/workspaceindex/gitignore.go index 462ae0a20..2b4dc41b7 100644 --- a/internal/workspaceindex/gitignore.go +++ b/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=` 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 /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 /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 -} diff --git a/internal/workspaceindex/gitignore_test.go b/internal/workspaceindex/gitignore_test.go index 367d94365..e1802b406 100644 --- a/internal/workspaceindex/gitignore_test.go +++ b/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) {