From 786735655d9323dd85dc07668e1bb00c55968495 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Wed, 14 Oct 2026 16:27:51 +0530 Subject: [PATCH 1/2] fix(tui,cli): stop a repository's git config from running programs in Zero's own git calls Zero runs git status and git diff on its own: the TUI's files sweep at startup and after each command, its pull request diffstat, and the scratch-file warning after a run. A repository's own config can name programs those commands run (core.fsmonitor on every status, hooks when the index is rewritten, clean filters and diff drivers assigned through .gitattributes), so a workspace unpacked from an archive ran its own code on the first turn, although Zero refuses the same workspace's project hooks and plugins until the user trusts it. internal/gitsafe builds those commands with core.fsmonitor and core.hooksPath turned off and GIT_OPTIONAL_LOCKS=0, and drops the repository-location variables git exports to hooks, using the same environment for its config check and for the command. A repository whose own config, or per-worktree config when extensions.worktreeConfig is on, defines a filter or diff driver command is refused, and the callers already treat that like a directory that is not a repository. The status and diff calls also pass --ignore-submodules=dirty, because a submodule's own config is not checked. --- internal/cli/scratch_files.go | 10 +- internal/gitsafe/callers_test.go | 218 +++++++++++++++++++++ internal/gitsafe/gitsafe.go | 192 +++++++++++++++++++ internal/gitsafe/gitsafe_test.go | 312 +++++++++++++++++++++++++++++++ internal/tui/files_git_sweep.go | 6 +- internal/tui/pr_status.go | 22 ++- internal/tui/pr_status_test.go | 6 +- 7 files changed, 756 insertions(+), 10 deletions(-) create mode 100644 internal/gitsafe/callers_test.go create mode 100644 internal/gitsafe/gitsafe.go create mode 100644 internal/gitsafe/gitsafe_test.go diff --git a/internal/cli/scratch_files.go b/internal/cli/scratch_files.go index 9365ce447..9f7238744 100644 --- a/internal/cli/scratch_files.go +++ b/internal/cli/scratch_files.go @@ -7,6 +7,8 @@ import ( "strconv" "strings" "time" + + "github.com/Gitlawb/zero/internal/gitsafe" ) type scratchFileBaseline map[string]bool @@ -78,7 +80,13 @@ func gitUntrackedFiles(workspaceRoot string) ([]string, bool) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - output, err := exec.CommandContext(ctx, "git", "-C", workspaceRoot, "status", "--porcelain", "-z", "--untracked-files=all").Output() + // gitsafe keeps the workspace's own git config from running a program, and + // --ignore-submodules=dirty keeps git out of submodule work trees. + command, err := gitsafe.Command(ctx, workspaceRoot, "status", "--porcelain", "-z", "--untracked-files=all", "--ignore-submodules=dirty") + if err != nil { + return nil, false + } + output, err := command.Output() if err != nil { // Not a git repo (or git failed for some other reason) — nothing // reliable to report against, so stay silent rather than guess. diff --git a/internal/gitsafe/callers_test.go b/internal/gitsafe/callers_test.go new file mode 100644 index 000000000..c7978cf59 --- /dev/null +++ b/internal/gitsafe/callers_test.go @@ -0,0 +1,218 @@ +package gitsafe + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "strconv" + "strings" + "testing" +) + +// 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. +var automaticGitPackages = []string{ + filepath.Join("..", "cli"), + filepath.Join("..", "tui"), +} + +// parsePackage parses the non-test Go files in dir. +func parsePackage(t *testing.T, dir string) (*token.FileSet, []*ast.File) { + t.Helper() + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("read %s: %v", dir, err) + } + fset := token.NewFileSet() + var files []*ast.File + for _, entry := range entries { + name := entry.Name() + if entry.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + file, err := parser.ParseFile(fset, filepath.Join(dir, name), nil, 0) + if err != nil { + t.Fatalf("parse %s: %v", name, err) + } + files = append(files, file) + } + if len(files) == 0 { + t.Fatalf("no Go files in %s", dir) + } + return fset, files +} + +// importName is the name file uses for the package at path, or "" when the +// file does not import it. +func importName(file *ast.File, path string) string { + for _, spec := range file.Imports { + if value, err := strconv.Unquote(spec.Path.Value); err != nil || value != path { + continue + } + if spec.Name != nil { + return spec.Name.Name + } + return filepath.Base(path) + } + return "" +} + +// directGitExecs lists the exec.Command and exec.CommandContext calls in files +// whose program is the literal "git". +func directGitExecs(fset *token.FileSet, files []*ast.File) []string { + var found []string + for _, file := range files { + execName := importName(file, "os/exec") + if execName == "" { + continue + } + ast.Inspect(file, func(node ast.Node) bool { + call, ok := node.(*ast.CallExpr) + if !ok { + return true + } + selector, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + if pkg, ok := selector.X.(*ast.Ident); !ok || pkg.Name != execName { + return true + } + program := 0 + switch selector.Sel.Name { + case "Command": + case "CommandContext": + program = 1 + default: + return true + } + if len(call.Args) <= program { + return true + } + if literal, ok := call.Args[program].(*ast.BasicLit); ok && literal.Kind == token.STRING { + if value, err := strconv.Unquote(literal.Value); err == nil && value == "git" { + found = append(found, fset.Position(call.Pos()).String()) + } + } + return true + }) + } + return found +} + +func TestAutomaticGitCallsDoNotExecGitDirectly(t *testing.T) { + for _, dir := range automaticGitPackages { + fset, files := parsePackage(t, dir) + for _, position := range directGitExecs(fset, files) { + t.Errorf("%s runs git directly; go through gitsafe.Command so the repository's config cannot make it run a program", position) + } + } +} + +// 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. +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 start < 0 { + for index, argument := range call.Args { + if literal, ok := argument.(*ast.BasicLit); ok && literal.Kind == token.STRING && literal.Value == `"git"` { + start = index + 1 + break + } + } + } + if start < 0 || start > len(call.Args) { + return nil, false + } + var arguments []string + for _, argument := range call.Args[start:] { + if literal, ok := argument.(*ast.BasicLit); ok && literal.Kind == token.STRING { + if value, err := strconv.Unquote(literal.Value); err == nil { + arguments = append(arguments, value) + continue + } + } + arguments = append(arguments, "") + } + return arguments, true +} + +// A submodule has its own config, which Command does not check, so the +// automatic status and diff calls stay out of submodule work trees. +func TestAutomaticStatusAndDiffSkipSubmoduleWorkTrees(t *testing.T) { + checked := 0 + for _, dir := range automaticGitPackages { + fset, files := parsePackage(t, dir) + for _, file := range files { + gitsafeName := importName(file, "github.com/Gitlawb/zero/internal/gitsafe") + ast.Inspect(file, func(node ast.Node) bool { + call, ok := node.(*ast.CallExpr) + if !ok { + return true + } + arguments, ok := gitArguments(call, gitsafeName) + if !ok || len(arguments) == 0 || (arguments[0] != "status" && arguments[0] != "diff") { + return true + } + checked++ + for _, argument := range arguments { + if argument == "--ignore-submodules=dirty" { + return true + } + } + t.Errorf("%s runs git %s without --ignore-submodules=dirty", fset.Position(call.Pos()), arguments[0]) + return true + }) + } + } + // 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 TUI runs every git command through one runner, whose program name is a +// parameter, so the check above cannot see it. The runner has to hand git to +// Command itself. +func TestTUICommandRunnerSendsGitThroughCommand(t *testing.T) { + _, files := parsePackage(t, filepath.Join("..", "tui")) + var runner *ast.FuncDecl + var runnerFile *ast.File + for _, file := range files { + for _, declaration := range file.Decls { + if function, ok := declaration.(*ast.FuncDecl); ok && function.Recv == nil && function.Name.Name == "defaultPRCommandRunner" { + runner, runnerFile = function, file + } + } + } + if runner == nil { + t.Fatal("defaultPRCommandRunner not found in internal/tui") + } + gitsafeName := importName(runnerFile, "github.com/Gitlawb/zero/internal/gitsafe") + calls := false + if gitsafeName != "" { + ast.Inspect(runner.Body, func(node ast.Node) bool { + if call, ok := node.(*ast.CallExpr); ok { + if selector, ok := call.Fun.(*ast.SelectorExpr); ok && selector.Sel.Name == "Command" { + if pkg, ok := selector.X.(*ast.Ident); ok && pkg.Name == gitsafeName { + calls = true + } + } + } + return !calls + }) + } + if !calls { + t.Fatal("defaultPRCommandRunner does not call gitsafe.Command, so the TUI's git calls skip it") + } +} diff --git a/internal/gitsafe/gitsafe.go b/internal/gitsafe/gitsafe.go new file mode 100644 index 000000000..cb1c20110 --- /dev/null +++ b/internal/gitsafe/gitsafe.go @@ -0,0 +1,192 @@ +// Package gitsafe builds the read-only git commands Zero runs on its own, as +// opposed to a git command the user or the model asked for: the status and diff +// calls behind the TUI's changed-files sweep, its pull request diffstat and the +// scratch-file warning, which run without anyone asking for git. +// +// A REPOSITORY CAN MAKE THOSE COMMANDS RUN A PROGRAM. git status and git diff read +// the repository's own .git/config, and several keys there name programs git then +// runs: core.fsmonitor on every status, a clean filter or diff driver assigned +// through .gitattributes whenever a file's content has to be read, and hooks such +// as post-index-change when the index is rewritten. A repository unpacked from an +// archive or handed over as a directory carries that config, and Zero would run it +// on the first turn, although it refuses the same workspace's project hooks and +// plugins until the user trusts it. +// +// The single-key cases are switched off on the command line. Filter and diff +// driver commands are named per driver, so they cannot be switched off without +// knowing the names; a repository whose own config defines any is skipped instead. +// Commands defined in the user's own global config, such as git-lfs, are the +// user's choice and are not affected. +package gitsafe + +import ( + "bytes" + "context" + "errors" + "os" + "os/exec" + "runtime" + "strings" +) + +// ErrRepositoryDefinesCommands is returned for a repository whose own config +// defines a filter or diff driver command. The caller goes without the git +// information rather than run it. +var ErrRepositoryDefinesCommands = errors.New("the repository's own git config defines a filter or diff command, so Zero does not run git there on its own") + +// commandKeys matches the config keys whose value is a program git can run while +// computing status or a diff, and which are named per driver. Git prints section +// and variable names in lower case, so the patterns do not need to fold case. +const commandKeys = `filter\..+\.(clean|smudge|process)|diff\..+\.(command|textconv)|diff\.external` + +// worktreeConfigKey makes git also read per-worktree config (config.worktree), +// which a --local read leaves out. Without it git ignores that file. +const worktreeConfigKey = "extensions.worktreeconfig" + +const ( + localConfigPattern = `^(` + commandKeys + `|extensions\.worktreeconfig)$` + worktreeConfigPattern = `^(` + commandKeys + `)$` +) + +// Command returns git with args, run in dir, with the programs a repository can +// configure turned off. It fails with ErrRepositoryDefinesCommands when the +// repository's own config defines a filter or diff driver command, and with git's +// own error when dir is not inside a repository. +// +// Callers that inspect a work tree that may contain submodules should also pass +// --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) { + env := environment() + defines, err := repositoryDefinesCommands(ctx, dir, env) + if err != nil { + return nil, err + } + if defines { + return nil, ErrRepositoryDefinesCommands + } + command := exec.CommandContext(ctx, "git", append(disabledPrograms(), args...)...) + command.Dir = 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 +} + +// disabledPrograms turns off the single-key ways a repository runs a program +// during status or diff. Git ignores a key it does not know, so an older git +// without hooksPath (2.9) or fsmonitor (2.16) accepts these unchanged. +func disabledPrograms() []string { + return []string{"-c", "core.fsmonitor=false", "-c", "core.hooksPath=" + os.DevNull} +} + +// repositoryLocationVariables make git use a given repository, work tree or +// index instead of discovering them from the working directory. Git exports +// them to hooks, so a Zero started from a hook inherits the hook's repository. +var repositoryLocationVariables = []string{ + "GIT_DIR", "GIT_WORK_TREE", "GIT_INDEX_FILE", "GIT_COMMON_DIR", + "GIT_OBJECT_DIRECTORY", "GIT_ALTERNATE_OBJECT_DIRECTORIES", "GIT_NAMESPACE", "GIT_PREFIX", +} + +// environment is Zero's environment without the repository-location variables, +// so git finds the repository from dir. The config check and the command share +// it: checking one repository's config and then running git in another would +// let the second one's commands through. +func environment() []string { + parent := os.Environ() + env := make([]string, 0, len(parent)+1) + for _, entry := range parent { + name, _, _ := strings.Cut(entry, "=") + if !isRepositoryLocationVariable(name) { + env = append(env, entry) + } + } + return env +} + +func isRepositoryLocationVariable(name string) bool { + for _, variable := range repositoryLocationVariables { + // Windows environment names are case-insensitive. + if name == variable || (runtime.GOOS == "windows" && strings.EqualFold(name, variable)) { + return true + } + } + return false +} + +// repositoryDefinesCommands reports whether the repository's own config, with +// includes followed, defines a filter or diff driver command. The local config is +// read once, together with the key that turns on per-worktree config, and that +// file is read only when it is on, so most repositories cost one git process. +// Reading config runs nothing. +func repositoryDefinesCommands(ctx context.Context, dir string, env []string) (bool, error) { + entries, err := readConfig(ctx, dir, env, "--local", localConfigPattern) + if err != nil { + return false, err + } + worktreeConfig := false + for _, entry := range entries { + if entry.key != worktreeConfigKey { + return true, nil + } + worktreeConfig = !configFalse(entry.value) + } + if !worktreeConfig { + return false, nil + } + // The extension needs git 2.20, which also has --worktree, so a failure here + // is real and the caller goes without git. + entries, err = readConfig(ctx, dir, env, "--worktree", worktreeConfigPattern) + if err != nil { + return false, err + } + return len(entries) > 0, nil +} + +// configFalse reports whether a config value is one of git's spellings of false. +// Anything else, including a key with no value, counts as on, so an unexpected +// spelling costs one more read rather than skipping the file. +func configFalse(value string) bool { + switch strings.ToLower(strings.TrimSpace(value)) { + case "false", "no", "off", "0": + return true + } + return false +} + +type configEntry struct { + key string + value string +} + +// readConfig returns the entries in one config scope whose key matches pattern. +// It is a variable so tests can answer for git. +var readConfig = func(ctx context.Context, dir string, env []string, scope string, pattern string) ([]configEntry, error) { + command := exec.CommandContext(ctx, "git", "config", "-z", scope, "--includes", "--get-regexp", pattern) + command.Dir = dir + command.Env = env + output, err := command.Output() + if err != nil { + var exit *exec.ExitError + if errors.As(err, &exit) && exit.ExitCode() == 1 { + return nil, nil // no key matched + } + return nil, err + } + return parseConfigEntries(output), nil +} + +// parseConfigEntries splits git config -z output: each entry ends with a NUL, +// and a newline separates the key from its value when there is one. +func parseConfigEntries(output []byte) []configEntry { + var entries []configEntry + for _, record := range bytes.Split(output, []byte{0}) { + if len(record) == 0 { + continue + } + key, value, _ := bytes.Cut(record, []byte{'\n'}) + entries = append(entries, configEntry{key: string(key), value: string(value)}) + } + return entries +} diff --git a/internal/gitsafe/gitsafe_test.go b/internal/gitsafe/gitsafe_test.go new file mode 100644 index 000000000..226753691 --- /dev/null +++ b/internal/gitsafe/gitsafe_test.go @@ -0,0 +1,312 @@ +package gitsafe + +import ( + "context" + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "reflect" + "regexp" + "runtime" + "strings" + "testing" +) + +type configCall struct { + scope string + env []string +} + +// answerConfig stands in for git's config read and records each call. +func answerConfig(t *testing.T, answer func(scope string) ([]configEntry, error)) *[]configCall { + t.Helper() + var calls []configCall + previous := readConfig + readConfig = func(_ context.Context, _ string, env []string, scope string, _ string) ([]configEntry, error) { + calls = append(calls, configCall{scope: scope, env: env}) + return answer(scope) + } + t.Cleanup(func() { readConfig = previous }) + return &calls +} + +func noEntries(string) ([]configEntry, error) { return nil, nil } + +func scopes(calls *[]configCall) []string { + var read []string + for _, call := range *calls { + read = append(read, call.scope) + } + return read +} + +// effectiveEnv returns the value a child process gets for name: os/exec keeps +// the last entry for a name, compared without case on Windows. +func effectiveEnv(env []string, name string) (string, bool) { + value, found := "", false + for _, entry := range env { + key, rest, ok := strings.Cut(entry, "=") + if ok && (key == name || (runtime.GOOS == "windows" && strings.EqualFold(key, name))) { + value, found = rest, true + } + } + return value, found +} + +func TestCommandTurnsOffRepositoryPrograms(t *testing.T) { + answerConfig(t, noEntries) + t.Setenv("GIT_OPTIONAL_LOCKS", "1") + dir := t.TempDir() + + command, err := Command(context.Background(), dir, "status", "--porcelain") + if err != nil { + t.Fatalf("Command: %v", err) + } + want := []string{"git", "-c", "core.fsmonitor=false", "-c", "core.hooksPath=" + os.DevNull, "status", "--porcelain"} + if !reflect.DeepEqual(command.Args, want) { + t.Fatalf("args = %q, want %q", command.Args, want) + } + if command.Dir != dir { + t.Fatalf("dir = %q, want %q", command.Dir, dir) + } + // An inherited GIT_OPTIONAL_LOCKS must not turn the index refresh back on. + if value, _ := effectiveEnv(command.Env, "GIT_OPTIONAL_LOCKS"); value != "0" { + t.Fatalf("effective GIT_OPTIONAL_LOCKS = %q, want 0", value) + } +} + +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) { + if scope == "--local" { + return []configEntry{{key: worktreeConfigKey, value: "true"}}, nil + } + return nil, nil + }) + elsewhere := filepath.Join(t.TempDir(), "elsewhere") + for _, name := range repositoryLocationVariables { + t.Setenv(name, elsewhere) + } + t.Setenv("ZERO_GITSAFE_TEST_KEPT", "kept") + + command, err := Command(context.Background(), t.TempDir(), "status") + if err != nil { + t.Fatalf("Command: %v", err) + } + for _, name := range repositoryLocationVariables { + if value, found := effectiveEnv(command.Env, name); found { + t.Fatalf("%s=%q reached git", name, value) + } + } + if value, _ := effectiveEnv(command.Env, "ZERO_GITSAFE_TEST_KEPT"); value != "kept" { + t.Fatalf("an unrelated variable was dropped: %q", value) + } + // The config check has to look at the repository the command runs in, so it + // gets the same environment, minus only the lock variable added afterwards. + if got := scopes(calls); !reflect.DeepEqual(got, []string{"--local", "--worktree"}) { + t.Fatalf("config reads = %q, want --local then --worktree", got) + } + for _, call := range *calls { + if !reflect.DeepEqual(call.env, command.Env[:len(command.Env)-1]) { + t.Fatalf("the %s config read ran with a different environment than the command", call.scope) + } + } +} + +func TestLocationVariableNamesFoldCaseOnlyOnWindows(t *testing.T) { + if !isRepositoryLocationVariable("GIT_DIR") { + t.Fatal("GIT_DIR not recognised") + } + if got, want := isRepositoryLocationVariable("Git_Dir"), runtime.GOOS == "windows"; got != want { + t.Fatalf("Git_Dir recognised = %v, want %v on %s", got, want, runtime.GOOS) + } + for _, name := range []string{"GIT_DIRECTORY", "MY_GIT_DIR", "GIT_CONFIG_GLOBAL", ""} { + if isRepositoryLocationVariable(name) { + t.Fatalf("%q treated as a repository-location variable", name) + } + } +} + +func TestCommandRefusesRepositoryThatDefinesCommands(t *testing.T) { + enabled := configEntry{key: worktreeConfigKey, value: "true"} + command := configEntry{key: "filter.x.clean", value: "x"} + cases := map[string]map[string][]configEntry{ + "local": {"--local": {command}}, + "local after the extension": {"--local": {enabled, command}}, + "per-worktree": {"--local": {enabled}, "--worktree": {{key: "diff.x.textconv", value: "x"}}}, + } + for name, answers := range cases { + t.Run(name, func(t *testing.T) { + answerConfig(t, func(scope string) ([]configEntry, error) { return answers[scope], nil }) + got, err := Command(context.Background(), t.TempDir(), "status") + if !errors.Is(err, ErrRepositoryDefinesCommands) { + t.Fatalf("err = %v, want ErrRepositoryDefinesCommands", err) + } + if got != nil { + t.Fatal("a command came back for a repository that defines commands") + } + }) + } +} + +func TestCommandReadsPerWorktreeConfigOnlyWhenItIsOn(t *testing.T) { + cases := map[string]struct { + local []configEntry + want []string + }{ + "no extension": {nil, []string{"--local"}}, + "extension off": {[]configEntry{{key: worktreeConfigKey, value: "false"}}, []string{"--local"}}, + "extension off as no": {[]configEntry{{key: worktreeConfigKey, value: "No"}}, []string{"--local"}}, + "extension on": {[]configEntry{{key: worktreeConfigKey, value: "true"}}, []string{"--local", "--worktree"}}, + "extension with no value": {[]configEntry{{key: worktreeConfigKey}}, []string{"--local", "--worktree"}}, + "turned off again": {[]configEntry{{key: worktreeConfigKey, value: "true"}, {key: worktreeConfigKey, value: "off"}}, []string{"--local"}}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + calls := answerConfig(t, func(scope string) ([]configEntry, error) { + if scope == "--local" { + return tc.local, nil + } + return nil, nil + }) + if _, err := Command(context.Background(), t.TempDir(), "status"); err != nil { + t.Fatalf("Command: %v", err) + } + if got := scopes(calls); !reflect.DeepEqual(got, tc.want) { + t.Fatalf("config reads = %q, want %q", got, tc.want) + } + }) + } +} + +func TestCommandFailsWhenConfigCannotBeRead(t *testing.T) { + unreadable := errors.New("not a git repository") + for _, failing := range []string{"--local", "--worktree"} { + t.Run(failing, func(t *testing.T) { + answerConfig(t, func(scope string) ([]configEntry, error) { + if scope == failing { + return nil, unreadable + } + return []configEntry{{key: worktreeConfigKey, value: "true"}}, nil + }) + if _, err := Command(context.Background(), t.TempDir(), "status"); !errors.Is(err, unreadable) { + t.Fatalf("err = %v, want the config read error", err) + } + }) + } +} + +// Go's regexp stands in for git's extended regex here; the patterns use nothing +// the two treat differently. +func TestConfigPatterns(t *testing.T) { + local := regexp.MustCompile(localConfigPattern) + worktree := regexp.MustCompile(worktreeConfigPattern) + for _, key := range []string{ + "filter.lfs.clean", "filter.lfs.smudge", "filter.lfs.process", "filter.a.b.clean", + "diff.word.textconv", "diff.tool.command", "diff.external", + } { + if !local.MatchString(key) || !worktree.MatchString(key) { + t.Errorf("%s is not matched in both scopes", key) + } + } + if !local.MatchString(worktreeConfigKey) || worktree.MatchString(worktreeConfigKey) { + t.Errorf("%s belongs to the local read only", worktreeConfigKey) + } + for _, key := range []string{ + "filter.lfs.required", "diff.word.cachetextconv", "diff.word.binary", + "core.fsmonitor", "diff.external.x", "diff.renames", "extensions.worktreeconfigx", + } { + if local.MatchString(key) || worktree.MatchString(key) { + t.Errorf("%s is matched", key) + } + } +} + +func TestParseConfigEntries(t *testing.T) { + got := parseConfigEntries([]byte("filter.x.clean\nrun this\nand that\x00extensions.worktreeconfig\x00")) + want := []configEntry{ + {key: "filter.x.clean", value: "run this\nand that"}, + {key: "extensions.worktreeconfig"}, + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("entries = %+v, want %+v", got, want) + } +} + +// Real git: a fresh repository with nothing configured, the same repository with +// per-worktree config turned on, and a directory that is not a repository. This +// pins the output format and exit codes readConfig relies on; the command keys +// themselves are covered above. +func TestCommandAgainstRealGit(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, ".config")) + t.Setenv("GIT_CONFIG_GLOBAL", os.DevNull) + t.Setenv("GIT_CONFIG_NOSYSTEM", "1") + repo := t.TempDir() + outside := t.TempDir() + // Both share a parent; stop git from finding an enclosing repository there. + t.Setenv("GIT_CEILING_DIRECTORIES", filepath.Dir(repo)) + git := func(args ...string) { + t.Helper() + if output, err := exec.Command("git", append([]string{"-C", repo}, args...)...).CombinedOutput(); err != nil { + t.Fatalf("git %v: %v: %s", args, err, output) + } + } + git("init", "-q") + gitRead := readConfig + var read []string + readConfig = func(ctx context.Context, dir string, env []string, scope string, pattern string) ([]configEntry, error) { + read = append(read, scope) + return gitRead(ctx, dir, env, scope, pattern) + } + t.Cleanup(func() { readConfig = gitRead }) + + command, err := Command(context.Background(), repo, "status", "--porcelain") + if err != nil { + t.Fatalf("Command in a plain repository: %v", err) + } + if output, err := command.CombinedOutput(); err != nil { + t.Fatalf("git status: %v: %s", err, output) + } + if !reflect.DeepEqual(read, []string{"--local"}) { + t.Fatalf("config reads in a plain repository = %q, want --local only", read) + } + + if !gitAtLeast(t, 2, 20) { + t.Log("skipping the per-worktree case: it needs git 2.20 for extensions.worktreeConfig") + } else { + git("config", "extensions.worktreeConfig", "true") + read = nil + if _, err := Command(context.Background(), repo, "status"); err != nil { + t.Fatalf("Command with per-worktree config on: %v", err) + } + if !reflect.DeepEqual(read, []string{"--local", "--worktree"}) { + t.Fatalf("config reads with per-worktree config on = %q, want --local then --worktree", read) + } + } + + if _, err := Command(context.Background(), outside, "status"); err == nil { + t.Fatal("Command outside a repository succeeded") + } +} + +// gitAtLeast reports whether the git on PATH is at least major.minor. +func gitAtLeast(t *testing.T, major, minor int) bool { + t.Helper() + output, err := exec.Command("git", "version").Output() + if err != nil { + t.Fatalf("git version: %v", err) + } + var gotMajor, gotMinor int + if _, err := fmt.Sscanf(strings.TrimPrefix(strings.TrimSpace(string(output)), "git version "), "%d.%d", &gotMajor, &gotMinor); err != nil { + t.Fatalf("parse %q: %v", output, err) + } + return gotMajor > major || (gotMajor == major && gotMinor >= minor) +} diff --git a/internal/tui/files_git_sweep.go b/internal/tui/files_git_sweep.go index 8c6678dfb..57319afbb 100644 --- a/internal/tui/files_git_sweep.go +++ b/internal/tui/files_git_sweep.go @@ -54,7 +54,9 @@ func gitSweepCmd(parent context.Context, cwd string, baseline bool) tea.Cmd { defer cancel() // --untracked-files=all enumerates files inside new directories; plain // porcelain collapses them to "dir/", which is useless as a FILES row. - status, err := defaultPRCommandRunner(ctx, cwd, "git", "status", "--porcelain", "--untracked-files=all") + // --ignore-submodules=dirty keeps git out of submodule work trees, whose + // own config gitsafe does not check. + status, err := defaultPRCommandRunner(ctx, cwd, "git", "status", "--porcelain", "--untracked-files=all", "--ignore-submodules=dirty") if err != nil { return gitSweepMsg{baseline: baseline, ok: false} } @@ -66,7 +68,7 @@ func gitSweepCmd(parent context.Context, cwd string, baseline bool) tea.Cmd { // would never match a porcelain path). Best-effort: a failure (e.g. unborn // HEAD in a fresh repo) keeps the file list with zero counts rather than // dropping the sweep. - if numstat, err := defaultPRCommandRunner(ctx, cwd, "git", "diff", "HEAD", "--numstat", "-z"); err == nil { + if numstat, err := defaultPRCommandRunner(ctx, cwd, "git", "diff", "HEAD", "--numstat", "-z", "--ignore-submodules=dirty"); err == nil { stats := parseGitNumstat(numstat) for i := range files { if counts, ok := stats[files[i].path]; ok { diff --git a/internal/tui/pr_status.go b/internal/tui/pr_status.go index 6f661a7f8..4922598cf 100644 --- a/internal/tui/pr_status.go +++ b/internal/tui/pr_status.go @@ -13,6 +13,8 @@ import ( "time" "charm.land/lipgloss/v2" + + "github.com/Gitlawb/zero/internal/gitsafe" ) type PrStatus string @@ -243,7 +245,7 @@ func getLocalDiffStats(ctx context.Context, cwd string, baseBranch string, run p if err != nil { mergeBase = originBase } - output, err := run(ctx, cwd, "git", "diff", "--numstat", strings.TrimSpace(mergeBase)) + output, err := run(ctx, cwd, "git", "diff", "--numstat", "--ignore-submodules=dirty", strings.TrimSpace(mergeBase)) if err != nil { return 0, 0, err } @@ -290,9 +292,21 @@ func jsonScalarString(raw json.RawMessage) string { } func defaultPRCommandRunner(ctx context.Context, cwd string, name string, args ...string) (string, error) { - command := exec.CommandContext(ctx, name, args...) - if strings.TrimSpace(cwd) != "" { - command.Dir = cwd + var command *exec.Cmd + if name == "git" { + // Every git call the TUI makes on its own comes through here (the files + // sweep and the pull request diffstat), so the repository being shown + // cannot make them run a program. + safe, err := gitsafe.Command(ctx, cwd, args...) + if err != nil { + return "", fmt.Errorf("%s: %w", name, err) + } + command = safe + } else { + command = exec.CommandContext(ctx, name, args...) + if strings.TrimSpace(cwd) != "" { + command.Dir = cwd + } } var stdout bytes.Buffer var stderr bytes.Buffer diff --git a/internal/tui/pr_status_test.go b/internal/tui/pr_status_test.go index 4511e0b0b..f1c4cf7ad 100644 --- a/internal/tui/pr_status_test.go +++ b/internal/tui/pr_status_test.go @@ -15,7 +15,7 @@ func TestPrServiceDetectsGitHubPRAndDiffStats(t *testing.T) { "git merge-base origin/main HEAD": { output: "abc123\n", }, - "git diff --numstat abc123": { + "git diff --numstat --ignore-submodules=dirty abc123": { output: "42\t7\tinternal/tui/view.go\n", }, }} @@ -49,7 +49,7 @@ func TestPrServiceFallsBackToGitLabMR(t *testing.T) { "git merge-base origin/develop HEAD": { output: "def456\n", }, - "git diff --numstat def456": { + "git diff --numstat --ignore-submodules=dirty def456": { output: "4\t5\ta.go\n5\t6\tb.go\n", }, }} @@ -71,7 +71,7 @@ func TestGetLocalDiffStatsFallsBackToOriginBase(t *testing.T) { "git merge-base origin/main HEAD": { err: errors.New("unknown revision"), }, - "git diff --numstat origin/main": { + "git diff --numstat --ignore-submodules=dirty origin/main": { output: "3\t0\tREADME.md\n", }, }} From 142282a1d90fae9d778efd52ce0f25b5bcfa77fa Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Thu, 15 Oct 2026 09:51:22 +0530 Subject: [PATCH 2/2] test(gitsafe): keep the real-git setup off an inherited GIT_DIR The setup helper in TestCommandAgainstRealGit ran git with the inherited environment, so from a hook, or a shell that exports GIT_DIR or GIT_WORK_TREE, git init and git config would act on the repository those name instead of the temporary one. It now uses the same environment as Command, and the test sets both variables to a decoy repository and checks after each setup step that the decoy was not touched. --- internal/gitsafe/gitsafe_test.go | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/internal/gitsafe/gitsafe_test.go b/internal/gitsafe/gitsafe_test.go index 226753691..bf38e767e 100644 --- a/internal/gitsafe/gitsafe_test.go +++ b/internal/gitsafe/gitsafe_test.go @@ -253,13 +253,27 @@ func TestCommandAgainstRealGit(t *testing.T) { outside := t.TempDir() // Both share a parent; stop git from finding an enclosing repository there. t.Setenv("GIT_CEILING_DIRECTORIES", filepath.Dir(repo)) + // Run as if from a hook in another repository. The setup below has to reach + // repo, as Command does, and never the repository these name. + decoy := t.TempDir() + t.Setenv("GIT_DIR", filepath.Join(decoy, ".git")) + t.Setenv("GIT_WORK_TREE", decoy) + decoyUntouched := func(step string) { + t.Helper() + if _, err := os.Stat(filepath.Join(decoy, ".git")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("%s reached the repository GIT_DIR names instead of repo: %v", step, err) + } + } git := func(args ...string) { t.Helper() - if output, err := exec.Command("git", append([]string{"-C", repo}, args...)...).CombinedOutput(); err != nil { + command := exec.Command("git", append([]string{"-C", repo}, args...)...) + command.Env = environment() + if output, err := command.CombinedOutput(); err != nil { t.Fatalf("git %v: %v: %s", args, err, output) } } git("init", "-q") + decoyUntouched("git init") gitRead := readConfig var read []string readConfig = func(ctx context.Context, dir string, env []string, scope string, pattern string) ([]configEntry, error) { @@ -283,6 +297,7 @@ func TestCommandAgainstRealGit(t *testing.T) { t.Log("skipping the per-worktree case: it needs git 2.20 for extensions.worktreeConfig") } else { git("config", "extensions.worktreeConfig", "true") + decoyUntouched("git config") read = nil if _, err := Command(context.Background(), repo, "status"); err != nil { t.Fatalf("Command with per-worktree config on: %v", err)