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") +}