From 7c00f644e3b28c4f338be92744ba3a23d0eac3e7 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Wed, 19 Aug 2026 16:06:08 +0100 Subject: [PATCH] fix(diff): run git in literal-pathspec mode Path arguments reach git straight from the VCS listing or from the user, so a tracked file whose name begins with ":(" or contains a glob character was parsed as a pathspec expression instead of selecting itself. `git diff -- ':(top)README.md'` resolved to README.md, and a file named "*.txt" matched every .txt file in the tree, which showed the reviewer another file's diff or an empty one and wrote the same wrong content into saved review history. Every git invocation now runs with the environment GitEnv builds: literal pathspec mode on, and GIT_GLOB_PATHSPECS / GIT_ICASE_PATHSPECS dropped, because git refuses to combine either of those with literal mode and would otherwise abort with "global 'literal' pathspec setting is incompatible with all other global pathspec settings" for anyone who has one of them set. The history package builds its diff command with the same environment. The untracked-rename path already set literal mode locally; that special case is gone now the shared helper covers it. No pathspec magic is used anywhere in the codebase, so nothing else changes. --- app/diff/diff.go | 59 +++++++++++++++++++------- app/diff/diff_test.go | 82 +++++++++++++++++++++++++++++++++++++ app/history/history.go | 4 ++ app/history/history_test.go | 35 ++++++++++++++++ 4 files changed, 165 insertions(+), 15 deletions(-) diff --git a/app/diff/diff.go b/app/diff/diff.go index 4612d8de..92e286de 100644 --- a/app/diff/diff.go +++ b/app/diff/diff.go @@ -10,6 +10,7 @@ import ( "os/exec" "path/filepath" "regexp" + "slices" "strconv" "strings" "time" @@ -42,8 +43,39 @@ const ( // BinaryPlaceholder is the content used for binary file placeholders. // parseUnifiedDiff returns this when git reports "Binary files ... differ". BinaryPlaceholder = "(binary file)" + + // literalPathspecs makes git treat every path argument as a literal filename. + // Without it a tracked file whose name begins with ":(" or contains a glob + // character is parsed as a pathspec expression and selects a different file, + // or no file at all. + literalPathspecs = "GIT_LITERAL_PATHSPECS=1" ) +// conflictingPathspecEnv lists the global pathspec settings git refuses to combine +// with literal mode: with either of them set truthy in the environment every +// command dies with "global 'literal' pathspec setting is incompatible with all +// other global pathspec settings". GIT_NOGLOB_PATHSPECS is absent on purpose, +// git accepts it alongside literal mode. +var conflictingPathspecEnv = []string{"GIT_GLOB_PATHSPECS=", "GIT_ICASE_PATHSPECS="} + +// GitEnv returns the environment for a child git process: the current environment +// with literal pathspec handling forced on and the settings that conflict with it +// removed. Paths reach git straight from the VCS listing or from the user, so a +// file actually named ":(top)x" or "*.go" must select itself rather than be parsed +// as a pathspec expression; nothing here relies on pathspec magic. Dropping the +// conflicting entries is harmless, since neither affects a literal path. +func GitEnv() []string { + env := os.Environ() + out := make([]string, 0, len(env)+1) + for _, kv := range env { + if slices.ContainsFunc(conflictingPathspecEnv, func(p string) bool { return strings.HasPrefix(kv, p) }) { + continue + } + out = append(out, kv) + } + return append(out, literalPathspecs) +} + // DiffLine holds parsed line info from a diff. type DiffLine struct { OldNum int // line number in old version (0 for additions) @@ -522,12 +554,11 @@ func (g *Git) tempIndexWithIntentToAdd(paths []string) (indexPath string, cleanu return tmpPath, cleanup, nil } -// renameIndexEnv builds the environment for git commands in the untracked-rename -// path: GIT_INDEX_FILE points at the throwaway index, and GIT_LITERAL_PATHSPECS -// makes git treat path arguments literally so a working-tree filename that looks -// like pathspec magic (e.g. ":(top)x") is not misinterpreted. +// renameIndexEnv builds the extra environment for git commands in the +// untracked-rename path: GIT_INDEX_FILE points at the throwaway index. Literal +// pathspec handling comes from GitEnv, which every git call goes through. func (g *Git) renameIndexEnv(indexPath string) []string { - return []string{"GIT_INDEX_FILE=" + indexPath, "GIT_LITERAL_PATHSPECS=1"} + return []string{"GIT_INDEX_FILE=" + indexPath} } // FileDiff returns the diff view for a single file. @@ -681,13 +712,13 @@ func (g *Git) diffArgs(ref string, staged bool) []string { // runGit executes a git command in the working directory and returns its output. func (g *Git) runGit(args ...string) (string, error) { - return runVCS(g.workDir, "git", args...) + return runVCSEnv(g.workDir, GitEnv(), "git", args...) } -// runGitEnv runs git with extra environment entries (e.g. GIT_INDEX_FILE) appended -// to the process environment, used by the throwaway-index rename detection path. +// runGitEnv runs git with extra environment entries (e.g. GIT_INDEX_FILE) on top of +// GitEnv, used by the throwaway-index rename detection path. func (g *Git) runGitEnv(extraEnv []string, args ...string) (string, error) { - return runVCSEnv(g.workDir, extraEnv, "git", args...) + return runVCSEnv(g.workDir, append(GitEnv(), extraEnv...), "git", args...) } // runVCS executes a VCS command in the given directory and returns its output. @@ -695,14 +726,12 @@ func runVCS(workDir, binary string, args ...string) (string, error) { return runVCSEnv(workDir, nil, binary, args...) } -// runVCSEnv executes a VCS command in the given directory with extra environment -// entries appended to os.Environ() and returns its output. -func runVCSEnv(workDir string, extraEnv []string, binary string, args ...string) (string, error) { +// runVCSEnv executes a VCS command in the given directory and returns its output. +// A nil env inherits the process environment; otherwise env replaces it wholesale. +func runVCSEnv(workDir string, env []string, binary string, args ...string) (string, error) { cmd := exec.CommandContext(context.Background(), binary, args...) //nolint:gosec // args constructed internally, not user input cmd.Dir = workDir - if len(extraEnv) > 0 { - cmd.Env = append(os.Environ(), extraEnv...) - } + cmd.Env = env out, err := cmd.Output() if err != nil { var exitErr *exec.ExitError diff --git a/app/diff/diff_test.go b/app/diff/diff_test.go index 0dcf4cfb..8d6e3944 100644 --- a/app/diff/diff_test.go +++ b/app/diff/diff_test.go @@ -5,6 +5,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strings" "testing" "time" @@ -1820,3 +1821,84 @@ func TestCountChanges(t *testing.T) { }) } } + +func TestGit_FileDiffLiteralPathspec(t *testing.T) { + tests := []struct { + name string + magic string // filename git would otherwise read as a pathspec expression + decoy string // file that expression selects instead + posixOnly bool + }{ + {name: "top magic prefix", magic: ":(top)f.txt", decoy: "f.txt", posixOnly: true}, + {name: "glob character", magic: "*.txt", decoy: "f.txt", posixOnly: true}, + {name: "character class", magic: "[x].txt", decoy: "x.txt"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.posixOnly && runtime.GOOS == "windows" { + t.Skip("filenames containing ':' or '*' are not valid on windows") + } + dir := setupTestRepo(t) + writeFile(t, dir, tt.decoy, "one\n") + writeFile(t, dir, tt.magic, "one\n") + gitCmd(t, dir, "add", "-A") + gitCmd(t, dir, "commit", "-m", "init") + + writeFile(t, dir, tt.decoy, "one\ndecoy\n") + writeFile(t, dir, tt.magic, "one\nreal\n") + + lines, err := NewGit(dir).FileDiff(FileDiffRequest{Path: tt.magic}) + require.NoError(t, err) + + added, removed := changeContents(lines) + assert.Equal(t, []string{"real"}, added, "diff must come from %q, not %q", tt.magic, tt.decoy) + assert.Empty(t, removed) + }) + } +} + +// git refuses to run with literal mode alongside either of these, so an inherited +// one has to be dropped rather than passed through +func TestGit_FileDiffWithConflictingPathspecEnv(t *testing.T) { + for _, envVar := range []string{"GIT_GLOB_PATHSPECS", "GIT_ICASE_PATHSPECS"} { + t.Run(envVar, func(t *testing.T) { + t.Setenv(envVar, "1") + dir := setupTestRepo(t) + writeFile(t, dir, "f.txt", "one\n") + gitCmd(t, dir, "add", "-A") + gitCmd(t, dir, "commit", "-m", "init") + writeFile(t, dir, "f.txt", "one\ntwo\n") + + lines, err := NewGit(dir).FileDiff(FileDiffRequest{Path: "f.txt"}) + require.NoError(t, err) + added, _ := changeContents(lines) + assert.Equal(t, []string{"two"}, added) + }) + } +} + +// the listing feeds FileDiff verbatim in the UI, so walk the whole path a +// magic-looking filename takes: list the change, then diff the listed entry +func TestGit_ChangedFileWithMagicNameRoundTrips(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("filenames containing ':' are not valid on windows") + } + dir := setupTestRepo(t) + writeFile(t, dir, "f.txt", "one\n") + writeFile(t, dir, ":(top)f.txt", "one\n") + gitCmd(t, dir, "add", "-A") + gitCmd(t, dir, "commit", "-m", "init") + writeFile(t, dir, ":(top)f.txt", "one\nreal\n") + + g := NewGit(dir) + entries, err := g.ChangedFiles("", false) + require.NoError(t, err) + require.Len(t, entries, 1) + assert.Equal(t, ":(top)f.txt", entries[0].Path) + + lines, err := g.FileDiff(FileDiffRequest{Path: entries[0].Path}) + require.NoError(t, err) + added, _ := changeContents(lines) + assert.Equal(t, []string{"real"}, added) +} diff --git a/app/history/history.go b/app/history/history.go index ac3049e9..0b20cffa 100644 --- a/app/history/history.go +++ b/app/history/history.go @@ -11,6 +11,7 @@ import ( "strings" "time" + "github.com/umputun/revdiff/app/diff" "github.com/umputun/revdiff/app/fsutil" ) @@ -134,6 +135,9 @@ func (s *Service) gitDiff(p Params) string { cmd := exec.CommandContext(context.Background(), "git", args...) cmd.Dir = p.GitRoot + // annotated file names come from the working tree, so a file actually named + // ":(top)x" must select itself instead of being parsed as a pathspec expression + cmd.Env = diff.GitEnv() out, err := cmd.Output() if err != nil { var exitErr *exec.ExitError diff --git a/app/history/history_test.go b/app/history/history_test.go index 3b60d252..0344f1a5 100644 --- a/app/history/history_test.go +++ b/app/history/history_test.go @@ -4,6 +4,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strings" "testing" @@ -503,3 +504,37 @@ func readHistoryFiles(t *testing.T, histDir string) []string { require.NoError(t, err) return contents } + +func TestSave_DiffUsesLiteralPathspec(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("filenames containing ':' are not valid on windows") + } + gitRoot := t.TempDir() + setupGitRepo(t, gitRoot) + + // ":(top)hello.txt" is parsed as "hello.txt relative to the repo root" unless + // git runs in literal-pathspec mode, so hello.txt is the decoy here + magic := ":(top)hello.txt" + err := os.WriteFile(filepath.Join(gitRoot, magic), []byte("magic\n"), 0o600) + require.NoError(t, err) + runGit(t, gitRoot, "add", "-A") + runGit(t, gitRoot, "commit", "-m", "add magic-named file") + + err = os.WriteFile(filepath.Join(gitRoot, magic), []byte("magic changed\n"), 0o600) + require.NoError(t, err) + err = os.WriteFile(filepath.Join(gitRoot, "hello.txt"), []byte("decoy changed\n"), 0o600) + require.NoError(t, err) + + histDir := t.TempDir() + New(histDir).Save(Params{ + Annotations: "## " + magic + ":1 (+)\nlook here\n", + Path: gitRoot, + GitRoot: gitRoot, + AnnotatedFiles: []string{magic}, + }) + + entries := readHistoryFiles(t, histDir) + require.Len(t, entries, 1) + assert.Contains(t, entries[0], "magic changed") + assert.NotContains(t, entries[0], "decoy changed") +}