Add git blame gutter toggle (B key) - #38
Conversation
umputun
left a comment
umputun
left a comment
There was a problem hiding this comment.
nice feature, the gutter helper refactoring is a good cleanup. couple things to fix:
linter issues:
diff/blame.go:35-os.Removereturn unchecked in defer, wrap with_ = os.Remove(tmpName)diff/blame.go:22- param types can be combined:ref, file stringdiff/blame.go:37-tmp.Close()error unhandled in error path
code:
4. Blamer interface in ui/model.go is missing //go:generate moq directive, project convention requires it for all consumer-side interfaces (see Renderer and SyntaxHighlighter above it)
5. ui/annotate.go:290-296 duplicates gutter width calculation that's already in m.gutterExtra(), pls reuse it
question:
6. @ icon for blame status conflicts with @ key for annotation list popup. different things, same symbol. maybe use smth else like b or similar?
|
one more thing, pls also add |
There was a problem hiding this comment.
Pull request overview
Adds an optional git blame gutter to the diff UI, toggled via B, to show per-line authorship and relative commit age while keeping blame loading asynchronous and non-blocking.
Changes:
- Introduces
ui.Blamerand adiff.Gitblame implementation with parsing + relative age formatting. - Updates diff rendering paths (normal/wrapped/collapsed) to support an additional gutter and shared gutter helpers.
- Extends keymap, status bar iconography, tests, and documentation to cover the new blame toggle.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/model.go | Adds blamer plumbing, async blame loading, toggle handling |
| ui/model_test.go | Updates/extends model tests for blame + toggle signature change |
| ui/diffview.go | Implements blame gutter rendering + shared gutter helpers |
| ui/collapsed.go | Adds blame-aware gutters and placeholder wrap-height logic |
| ui/annotate.go | Reuses placeholder visual-height helper (wrap + gutters) |
| ui/mdtoc.go | Preallocs TOC entries slice |
| ui/filetree.go | Preallocs tree entries slice |
| keymap/keymap.go | Adds ActionToggleBlame and default B binding + help text |
| keymap/keymap_test.go | Updates expected default bindings to include B |
| diff/blame.go | New: FileBlame, porcelain parser, hex validation, RelativeAge |
| diff/blame_test.go | New: tests for blame parsing, age formatting, and git behavior |
| diff/diff.go | Adjusts git command invocation (runGit) |
| diff/diff_test.go | Adjusts test helper git command invocation |
| diff/fallback.go | Returns nil for empty file sets + prealloc result slice |
| diff/directory.go | Preallocs files slice for git ls-files -z parsing |
| cmd/revdiff/main.go | Wires optional blamer into UI when git is available |
| README.md | Documents blame gutter and B toggle |
| site/index.html | Mentions blame gutter in feature copy |
| site/docs.html | Adds B to view toggles table |
| theme/theme.go | Preallocs theme names slice |
| CLAUDE.md | Documents blame gutter behavior and new status icon |
| .claude-plugin/skills/revdiff/references/usage.md | Adds B binding to plugin usage docs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -92,7 +92,7 @@ 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) { | |||
There was a problem hiding this comment.
runGit now uses exec.CommandContext without the previous //nolint:gosec suppression. Since gosec is enabled in .golangci.yml, this will typically trigger G204 (subprocess launched with variable) and fail CI. Either restore a //nolint:gosec with a brief justification that args are internally constructed (and not user-controlled), or restructure to satisfy gosec (e.g., avoid passing arbitrary slices without validation).
| func (g *Git) runGit(args ...string) (string, error) { | |
| func (g *Git) runGit(args ...string) (string, error) { | |
| //nolint:gosec // G204: "git" is fixed and args are passed as argv entries, constructed internally without shell interpretation. |
| func gitCmd(t *testing.T, dir string, args ...string) { | ||
| t.Helper() | ||
| cmd := exec.Command("git", args...) //nolint:gosec // test helper | ||
| cmd := exec.Command("git", args...) |
There was a problem hiding this comment.
Test helper gitCmd calls exec.Command("git", args...) without //nolint:gosec. With gosec enabled, this is likely to be flagged (G204) and fail lint. Add back a targeted //nolint:gosec with an explanation (test-only helper; args are controlled by the test code) or adjust the helper to avoid gosec complaints.
| cmd := exec.Command("git", args...) | |
| cmd := exec.Command("git", args...) //nolint:gosec // test-only helper; command is fixed and args are controlled by the test code |
| // hasBlameGutter returns true when the blame gutter should be rendered. | ||
| func (m Model) hasBlameGutter() bool { | ||
| return m.showBlame && len(m.blameData) > 0 | ||
| } | ||
|
|
There was a problem hiding this comment.
hasBlameGutter is gated on len(m.blameData) > 0, which means toggling blame on shows the @ status icon but renders no gutter until blame data arrives (and renders nothing at all on blame load errors/untracked files). This makes the toggle appear broken/inconsistent. Consider rendering the gutter whenever showBlame is true (using blanks while loading/unavailable) and ensure blameAuthorLen has a non-zero default width while loading.
| parts := strings.Split(ref, "..") | ||
| if len(parts) != 2 || parts[0] == "" || parts[1] == "" { | ||
| return "" | ||
| } | ||
| return parts[1] |
There was a problem hiding this comment.
blameTargetRef splits on ".." and will mis-handle the valid git range syntax A...B (triple-dot), returning a ref like ".B". This can cause git blame to fail or blame the wrong revision. Consider explicitly handling both .. and ... forms (and/or use strings.Cut/SplitN with a check for ...).
| parts := strings.Split(ref, "..") | |
| if len(parts) != 2 || parts[0] == "" || parts[1] == "" { | |
| return "" | |
| } | |
| return parts[1] | |
| if left, right, ok := strings.Cut(ref, "..."); ok { | |
| if left == "" || right == "" { | |
| return "" | |
| } | |
| return right | |
| } | |
| if left, right, ok := strings.Cut(ref, ".."); ok { | |
| if left == "" || right == "" { | |
| return "" | |
| } | |
| return right | |
| } | |
| return "" |
| // header line: <40-hex-hash> <orig_line> <final_line> [<group_lines>] | ||
| if len(line) >= 40 && isHexString(line[:40]) { | ||
| parts := strings.Fields(line) | ||
| if len(parts) >= 3 { | ||
| if n, err := strconv.Atoi(parts[2]); err == nil { | ||
| lineNum = n | ||
| } | ||
| } | ||
| author = "" | ||
| authorTime = time.Time{} | ||
| continue | ||
| } |
There was a problem hiding this comment.
In parseBlame, when a header line is detected but the final line number fails to parse (or fields are missing), lineNum is left unchanged from the previous entry. This can incorrectly attribute the next content line to the prior line number. Reset lineNum to 0 at the start of each header (and only set it when parsing succeeds) to avoid stale state.
| if _, err := tmp.WriteString(indexContent); err != nil { | ||
| tmp.Close() //nolint:gosec // best-effort close in error path | ||
| return "", fmt.Errorf("write temp blame file for %s: %w", file, err) | ||
| } | ||
| if err := tmp.Close(); err != nil { | ||
| return "", fmt.Errorf("close temp blame file for %s: %w", file, err) | ||
| } |
There was a problem hiding this comment.
writeStagedBlameFile can leak temp files on error paths: if WriteString or the initial Close() fails, the temp file is left on disk. Also tmp.Close() in the write-error path is suppressed with //nolint:gosec, but the relevant linter for ignoring close errors is errcheck (or just handle the error). Consider removing the temp file on any failure after creation and use the correct nolint (or check the close error).
| if _, err := tmp.WriteString(indexContent); err != nil { | |
| tmp.Close() //nolint:gosec // best-effort close in error path | |
| return "", fmt.Errorf("write temp blame file for %s: %w", file, err) | |
| } | |
| if err := tmp.Close(); err != nil { | |
| return "", fmt.Errorf("close temp blame file for %s: %w", file, err) | |
| } | |
| success := false | |
| defer func() { | |
| if success { | |
| return | |
| } | |
| _ = tmp.Close() | |
| _ = os.Remove(tmp.Name()) | |
| }() | |
| if _, err := tmp.WriteString(indexContent); err != nil { | |
| return "", fmt.Errorf("write temp blame file for %s: %w", file, err) | |
| } | |
| if err := tmp.Close(); err != nil { | |
| return "", fmt.Errorf("close temp blame file for %s: %w", file, err) | |
| } | |
| success = true |
Show author name and relative commit age per line in a gutter column, toggled with the B key. Blame data loads asynchronously via git blame and is keyed by new-side line numbers (blank for removed lines/dividers). - Add diff/blame.go with FileBlame parser and RelativeAge formatter - Add Blamer interface in ui/, wired via ModelConfig from cmd/main.go - Add blameGutter rendering in diffview.go alongside lineNumGutter - Extract shared gutter helpers (lineGutters, gutterExtra, gutterBlanks, applyHorizontalScroll) to reduce duplication across render paths - Add ActionToggleBlame keymap action bound to B - Update status bar mode icons to show @ when blame is active - Update README, docs, site, CLAUDE.md, and plugin references
- Extract writeStagedBlameFile() to reduce nestif complexity in blame.go - Combine param types in FileBlame signature (gocritic) - Handle errcheck/gosec on temp file cleanup paths - Extract deletePlaceholderVisualHeight() to reduce nestif in annotate.go - Remove stale //nolint:gosec directives from diff.go and diff_test.go - Pre-allocate slices in directory.go, fallback.go, theme.go, filetree.go, mdtoc.go, and model_test.go; preserve nil-vs-empty semantics in fallback.go
35e4cc5 to
e80adaf
Compare
strings.Split("A...B", "..") produced ".B" which caused git blame
to fail or blame the wrong revision. Use strings.Cut and check for
"..." before ".." to correctly extract the target ref.
|
@umputun addressed all yours and one reasonable from Copilot |
|
looks good, tested locally. one last thing, the "Fix all golangci-lint warnings" commit removed existing |
CI golangci-lint flags G204 on exec.Command with variable args. These are safe — git args are constructed internally, not from user input.
|
Restored |
umputun
left a comment
umputun
left a comment
There was a problem hiding this comment.
LGTM, thx for addressing everything
* Add git blame gutter toggle (B key)
Show author name and relative commit age per line in a gutter column,
toggled with the B key. Blame data loads asynchronously via git blame
and is keyed by new-side line numbers (blank for removed lines/dividers).
- Add diff/blame.go with FileBlame parser and RelativeAge formatter
- Add Blamer interface in ui/, wired via ModelConfig from cmd/main.go
- Add blameGutter rendering in diffview.go alongside lineNumGutter
- Extract shared gutter helpers (lineGutters, gutterExtra, gutterBlanks,
applyHorizontalScroll) to reduce duplication across render paths
- Add ActionToggleBlame keymap action bound to B
- Update status bar mode icons to show @ when blame is active
- Update README, docs, site, CLAUDE.md, and plugin references
* Fix all golangci-lint warnings
- Extract writeStagedBlameFile() to reduce nestif complexity in blame.go
- Combine param types in FileBlame signature (gocritic)
- Handle errcheck/gosec on temp file cleanup paths
- Extract deletePlaceholderVisualHeight() to reduce nestif in annotate.go
- Remove stale //nolint:gosec directives from diff.go and diff_test.go
- Pre-allocate slices in directory.go, fallback.go, theme.go, filetree.go,
mdtoc.go, and model_test.go; preserve nil-vs-empty semantics in fallback.go
* Fix blame review follow-ups and persist view modes
* Fix blameTargetRef to handle triple-dot ref syntax
strings.Split("A...B", "..") produced ".B" which caused git blame
to fail or blame the wrong revision. Use strings.Cut and check for
"..." before ".." to correctly extract the target ref.
* Restore //nolint:gosec for internal exec.Command calls
CI golangci-lint flags G204 on exec.Command with variable args.
These are safe — git args are constructed internally, not from user input.
Summary
Bkey, showing author name (truncated to 8 chars) and relative commit age (e.g.3d,2w,1y) per linegit blameand is keyed by new-side line numbers; removed lines and dividers show blank guttersBlamerinterface (ui.Blamer) withdiff.Gitimplementation; gracefully nil when git is unavailablelineGutters,gutterExtra,gutterBlanks,applyHorizontalScroll) to reduce duplication across expanded, collapsed, and wrapped render paths@icon indicates blame gutter is activeNew files
diff/blame.go—FileBlame()parser,RelativeAge()formatter,BlameLinestructdiff/blame_test.go— tests for blame parsing, age formatting, hex validationScreenshots