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..bf38e767e --- /dev/null +++ b/internal/gitsafe/gitsafe_test.go @@ -0,0 +1,327 @@ +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)) + // 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() + 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) { + 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") + decoyUntouched("git config") + 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", }, }}