feat: surface git's color-moved highlights for moved lines#323
Open
mvanhorn wants to merge 1 commit into
Open
Conversation
modem-dev#261 asked for hunk to honor git's color.moved + color.movedBlock so moved code in a diff is highlighted distinctly from added/removed. Loads the relevant git config keys, threads a 'moved' lane through the diff renderer, and renders moved-from / moved-to lines with the configured palette. Fixes modem-dev#261
Contributor
Greptile SummaryThis PR surfaces git's
Confidence Score: 4/5Safe to merge; the core ANSI-parsing pipeline is logically sound and the two minor code-quality findings have no impact on correctness. The index-alignment strategy between collectLineMoveKinds and @pierre/diffs is correct: context lines increment both counters, additions increment only additionLineIndex, and deletions only deletionLineIndex, matching the per-hunk cursor arithmetic in pierre.ts. The only real findings are a dead-code branch in normalizeGitColorMovedMode and a redundant string allocation in collectLineMoveKinds — both harmless. src/core/loaders.ts and src/core/git.ts have the two minor nits; all UI and theme files are straightforward. Important Files Changed
Sequence DiagramsequenceDiagram
participant Config as config.ts
participant Loaders as loaders.ts
participant Git as git.ts
participant Pierre as pierre.ts
participant Render as renderRows.tsx / staticDiffPager.ts
Config->>Loaders: "resolvedOptions.colorMoved (bool | undefined)"
Loaders->>Git: resolveGitColorMovedOptions(input, cwd)
Git-->>Loaders: "GitColorMovedOptions | null"
Loaders->>Git: buildGitDiffArgs(input, excludes, colorMoved)
Note over Git: Appends --color=always<br/>--color-moved=zebra<br/>-c color.diff.*=magenta/cyan
Git-->>Loaders: string[] (git args with ANSI color flags)
Loaders->>Loaders: collectLineMoveKinds(rawAnsiPatchText)
Note over Loaders: Scans SGR params (35=magenta=moved deletion,<br/>36=cyan=moved addition) before strip
Loaders->>Loaders: stripTerminalControl → parsePatchFiles
Loaders-->>Pierre: "DiffFile { lineMoveKinds?: DiffLineMoveKinds }"
Pierre->>Pierre: buildSplitRows / buildStackRows
Note over Pierre: Passes lineMoveKinds[i] as moveKind<br/>into makeSplitCell / makeStackCell
Pierre-->>Render: "DiffRow[] { cell.moveKind? }"
Render->>Render: splitCellPalette / stackCellPalette(kind, theme, moveKind)
Note over Render: moveKind → movedAddedBg / movedRemovedBg<br/>instead of addedBg / removedBg
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/core/git.ts:425-427
`"no"` is already a member of `GIT_BOOLEAN_FALSE_VALUES`, so the `|| normalized === "no"` guard is dead code. This is harmless today, but if `GIT_BOOLEAN_FALSE_VALUES` is ever trimmed, the intent could be lost. Removing it keeps the two constant sets as the single source of truth.
```suggestion
if (GIT_BOOLEAN_FALSE_VALUES.has(normalized)) {
return null;
}
```
### Issue 2 of 2
src/core/loaders.ts:131
`collectLineMoveKinds` receives `rawPatchText`, which the caller already produced with `.replaceAll("
", "
")`. The second `.replaceAll` inside the loop is therefore a guaranteed no-op on every call. Dropping it removes a full string allocation for every diff parse, including the common path where color-moved is disabled.
```suggestion
for (const rawLine of patchText.split("\n")) {
```
Reviews (1): Last reviewed commit: "feat: surface git's color-moved highligh..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+425
to
+427
| if (GIT_BOOLEAN_FALSE_VALUES.has(normalized) || normalized === "no") { | ||
| return null; | ||
| } |
Contributor
There was a problem hiding this comment.
"no" is already a member of GIT_BOOLEAN_FALSE_VALUES, so the || normalized === "no" guard is dead code. This is harmless today, but if GIT_BOOLEAN_FALSE_VALUES is ever trimmed, the intent could be lost. Removing it keeps the two constant sets as the single source of truth.
Suggested change
| if (GIT_BOOLEAN_FALSE_VALUES.has(normalized) || normalized === "no") { | |
| return null; | |
| } | |
| if (GIT_BOOLEAN_FALSE_VALUES.has(normalized)) { | |
| return null; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/git.ts
Line: 425-427
Comment:
`"no"` is already a member of `GIT_BOOLEAN_FALSE_VALUES`, so the `|| normalized === "no"` guard is dead code. This is harmless today, but if `GIT_BOOLEAN_FALSE_VALUES` is ever trimmed, the intent could be lost. Removing it keeps the two constant sets as the single source of truth.
```suggestion
if (GIT_BOOLEAN_FALSE_VALUES.has(normalized)) {
return null;
}
```
How can I resolve this? If you propose a fix, please make it concise.| return moveKinds; | ||
| }; | ||
|
|
||
| for (const rawLine of patchText.replaceAll("\r\n", "\n").split("\n")) { |
Contributor
There was a problem hiding this comment.
collectLineMoveKinds receives rawPatchText, which the caller already produced with .replaceAll(" ", " "). The second .replaceAll inside the loop is therefore a guaranteed no-op on every call. Dropping it removes a full string allocation for every diff parse, including the common path where color-moved is disabled.
Suggested change
| for (const rawLine of patchText.replaceAll("\r\n", "\n").split("\n")) { | |
| for (const rawLine of patchText.split("\n")) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/loaders.ts
Line: 131
Comment:
`collectLineMoveKinds` receives `rawPatchText`, which the caller already produced with `.replaceAll("
", "
")`. The second `.replaceAll` inside the loop is therefore a guaranteed no-op on every call. Dropping it removes a full string allocation for every diff parse, including the common path where color-moved is disabled.
```suggestion
for (const rawLine of patchText.split("\n")) {
```
How can I resolve this? If you propose a fix, please make it concise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Honors
git config color.movedandcolor.movedBlockso moved lines render with a distinct color rather than appearing as a paired add/remove. This matches the behavior ofgit diff --color-movedand makes large refactors much easier to read in hunk.Why this matters
#261 specifically asked for this. Today, a function moved from one file to another shows up as a delete + add and the reviewer has to mentally pair the pieces. Surfacing the move highlights closes that gap.
Changes
src/core/git.ts+src/core/git.test.ts- read the new git config keys; parse moved-line metadata out ofgit diff --color-moved=zebra --no-coloroutput and emit it as amovedlane in the parsed diff.src/core/config.ts+src/core/config.test.ts+src/core/types.ts- thread thecolor_movedsetting through.src/core/loaders.ts+src/core/loaders.test.ts- apply the loader logic for the new lane.src/ui/diff/pierre.ts+src/ui/diff/pierre.test.ts+src/ui/diff/renderRows.tsx+src/ui/diff/rowStyle.ts+src/ui/staticDiffPager.ts- render the moved lane with the configured palette.src/ui/themes.ts- add default moved-from / moved-to colors that match git's zebra palette.Testing
Targeted tests at each layer + the existing diff/UI test suites pass locally.
Fixes #261