Conversation
ExcludeFilter wraps any renderer and filters out files matching exclude prefixes from ChangedFiles results. Uses a local renderer interface in diff/ to avoid import cycle with ui/.
There was a problem hiding this comment.
Pull request overview
This PR adds an “all-files” browsing mode to revdiff (turning it into a general-purpose code browser over git-tracked files) and introduces prefix-based file exclusion that works across both diff and all-files modes.
Changes:
- Add
--all-files(-A) to list files viagit ls-filesand render them as context-only (DirectoryReader). - Add
--exclude(-X) to filter file lists via a renderer wrapper (ExcludeFilter), with config/env support. - Harden context-file reading (
readFileAsContext) for binary files, broken symlinks, non-regular files, and overly long lines; update CLI/docs/plugin references and tests.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents --all-files / --exclude behavior and examples. |
| docs/plans/completed/2026-04-05-all-files-mode.md | Marks implementation plan tasks completed. |
| diff/fallback.go | Adds safer context reading (binary/non-regular/broken symlink/too-long line handling). |
| diff/fallback_test.go | Adds test coverage for new readFileAsContext edge cases. |
| diff/exclude.go | Introduces ExcludeFilter renderer wrapper for prefix filtering. |
| diff/exclude_test.go | Adds unit tests for prefix matching and filtering behavior. |
| diff/directory.go | Adds DirectoryReader for git ls-files-backed discovery. |
| diff/directory_test.go | Adds integration-style tests for listing and reading tracked files. |
| cmd/revdiff/main.go | Wires new CLI flags, validation, renderer selection, and dumpConfig writer generalization. |
| cmd/revdiff/main_test.go | Adds parse/renderer wiring tests for --all-files and --exclude. |
| CLAUDE.md | Updates architecture notes for new renderers and flags. |
| .claude-plugin/skills/revdiff/SKILL.md | Updates skill triggers/argument handling for all-files mode. |
| .claude-plugin/skills/revdiff/references/usage.md | Adds usage docs for all-files and exclude. |
| .claude-plugin/skills/revdiff/references/config.md | Documents new flags and env var behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
diff/exclude.go
Outdated
| // normalize prefixes: strip trailing slashes so "vendor/" and "vendor" both work | ||
| normalized := make([]string, len(prefixes)) | ||
| for i, p := range prefixes { | ||
| normalized[i] = strings.TrimRight(p, "/") |
There was a problem hiding this comment.
--exclude prefixes are only normalized for trailing "/", but not trimmed for whitespace or empty values. This makes common env/config inputs like REVDIFF_EXCLUDE="vendor, mocks" (note space) fail to match, and a trailing comma produce a useless empty prefix. Consider applying strings.TrimSpace to each prefix and skipping empty results during normalization.
| // normalize prefixes: strip trailing slashes so "vendor/" and "vendor" both work | |
| normalized := make([]string, len(prefixes)) | |
| for i, p := range prefixes { | |
| normalized[i] = strings.TrimRight(p, "/") | |
| // normalize prefixes: trim whitespace, strip trailing slashes so "vendor/" and "vendor" both work, | |
| // and skip empty values from env/config inputs like trailing commas. | |
| normalized := make([]string, 0, len(prefixes)) | |
| for _, p := range prefixes { | |
| p = strings.TrimSpace(p) | |
| p = strings.TrimRight(p, "/") | |
| if p == "" { | |
| continue | |
| } | |
| normalized = append(normalized, p) |
diff/directory.go
Outdated
| if err != nil { | ||
| var exitErr *exec.ExitError | ||
| if errors.As(err, &exitErr) { | ||
| return nil, fmt.Errorf("git ls-files: %s", string(exitErr.Stderr)) |
There was a problem hiding this comment.
On git ls-files failures, the exit-error path returns string(exitErr.Stderr) without trimming and without a fallback when stderr is empty. Git typically includes a trailing newline in stderr, which can leak into TUI error rendering. Consider using strings.TrimSpace and, if the trimmed stderr is empty, fall back to wrapping the original error so the message is still informative.
| return nil, fmt.Errorf("git ls-files: %s", string(exitErr.Stderr)) | |
| stderr := strings.TrimSpace(string(exitErr.Stderr)) | |
| if stderr != "" { | |
| return nil, fmt.Errorf("git ls-files: %s", stderr) | |
| } |
Address Copilot review: TrimSpace exclude prefixes so env var input like "vendor, mocks" works correctly, skip empty values from trailing commas. Trim git ls-files stderr to avoid trailing newline in errors.
Summary
--all-files(-A) flag turns revdiff into a general-purpose code browser usinggit ls-filesfor file discovery--exclude(-X) flag filters files by path prefix, works with both--all-filesand regular diff modesDirectoryReaderlists all git-tracked files;ExcludeFilterwraps any renderer for prefix-based exclusion--stagedand--only; validated at CLI levelChanges
DirectoryReaderusinggit ls-files, skips deleted files viaos.LstatExcludeFilterwrapper with prefix matchingreadFileAsContextnow detects binary files, broken symlinks, non-regular filesmakeRenderer()--all-filesand--excludewith examples