Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/core/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ describe("config resolution", () => {
[
'theme = "graphite"',
"line_numbers = false",
"color_moved = true",
"",
"[patch]",
'mode = "split"',
Expand Down Expand Up @@ -87,6 +88,7 @@ describe("config resolution", () => {
wrapLines: true,
hunkHeaders: false,
agentNotes: true,
colorMoved: true,
});
});

Expand Down
3 changes: 3 additions & 0 deletions src/core/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ function readConfigPreferences(source: Record<string, unknown>): CommonOptions {
wrapLines: normalizeBoolean(source.wrap_lines),
hunkHeaders: normalizeBoolean(source.hunk_headers),
agentNotes: normalizeBoolean(source.agent_notes),
colorMoved: normalizeBoolean(source.color_moved),
};
}

Expand All @@ -81,6 +82,7 @@ function mergeOptions(base: CommonOptions, overrides: CommonOptions): CommonOpti
wrapLines: overrides.wrapLines ?? base.wrapLines,
hunkHeaders: overrides.hunkHeaders ?? base.hunkHeaders,
agentNotes: overrides.agentNotes ?? base.agentNotes,
colorMoved: overrides.colorMoved ?? base.colorMoved,
};
}

Expand Down Expand Up @@ -194,6 +196,7 @@ export function resolveConfiguredCliInput(
wrapLines: resolvedOptions.wrapLines ?? DEFAULT_VIEW_PREFERENCES.wrapLines,
hunkHeaders: resolvedOptions.hunkHeaders ?? DEFAULT_VIEW_PREFERENCES.showHunkHeaders,
agentNotes: resolvedOptions.agentNotes ?? DEFAULT_VIEW_PREFERENCES.showAgentNotes,
colorMoved: resolvedOptions.colorMoved,
};

return {
Expand Down
21 changes: 20 additions & 1 deletion src/core/git.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,26 @@
import { describe, expect, test } from "bun:test";
import { buildGitStashShowArgs, runGitText } from "./git";
import { buildGitDiffArgs, buildGitStashShowArgs, runGitText } from "./git";

describe("git command helpers", () => {
test("enables deterministic color-moved output for patch parsing", () => {
const args = buildGitDiffArgs(
{
kind: "vcs",
staged: false,
options: { mode: "auto" },
},
[],
{ mode: "zebra", whitespaceMode: "allow-indentation-change" },
);

expect(args).toContain("--color=always");
expect(args).toContain("--color-moved=zebra");
expect(args).toContain("--color-moved-ws=allow-indentation-change");
expect(args).not.toContain("--no-color");
expect(args).toContain("color.diff.oldMoved=magenta bold");
expect(args).toContain("color.diff.newMoved=cyan bold");
});

test("disables external diff tools for stash patches", () => {
const args = buildGitStashShowArgs({
kind: "stash-show",
Expand Down
153 changes: 144 additions & 9 deletions src/core/git.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ interface RunGitCommandOptions extends RunGitTextOptions {
acceptedExitCodes?: number[];
}

export interface GitColorMovedOptions {
mode: string;
whitespaceMode?: string;
}

/** Append Git pathspec arguments only when the caller requested them. */
export function appendGitPathspecs(args: string[], pathspecs?: string[]) {
if (!pathspecs || pathspecs.length === 0) {
Expand All @@ -44,13 +49,58 @@ const DIFF_PREFIX_NORMALIZATION_ARGS = [
"diff.dstPrefix=b/",
];

const GIT_MOVED_LINE_COLOR_CONFIG = [
"-c",
"color.diff.oldMoved=magenta bold",
"-c",
"color.diff.oldMovedAlternative=magenta bold",
"-c",
"color.diff.oldMovedDimmed=magenta dim",
"-c",
"color.diff.oldMovedAlternativeDimmed=magenta dim",
"-c",
"color.diff.newMoved=cyan bold",
"-c",
"color.diff.newMovedAlternative=cyan bold",
"-c",
"color.diff.newMovedDimmed=cyan dim",
"-c",
"color.diff.newMovedAlternativeDimmed=cyan dim",
];

function withNormalizedDiffPrefixes(args: string[]) {
return [...DIFF_PREFIX_NORMALIZATION_ARGS, ...args];
}

/** Return Git color flags for patch commands, enabling ANSI only when Hunk needs move classes. */
function gitPatchColorArgs(colorMoved: GitColorMovedOptions | null) {
if (!colorMoved) {
return ["--no-color"];
}

return [
"--color=always",
`--color-moved=${colorMoved.mode}`,
...(colorMoved.whitespaceMode ? [`--color-moved-ws=${colorMoved.whitespaceMode}`] : []),
];
}

/** Add deterministic moved-line colors so the parser can classify Git's ANSI output reliably. */
function withGitMovedLineColorConfig(args: string[], colorMoved: GitColorMovedOptions | null) {
if (!colorMoved) {
return args;
}

return [...GIT_MOVED_LINE_COLOR_CONFIG, ...args];
}

/** Build the exact `git diff` arguments used for the shared working-tree and range review path. */
export function buildGitDiffArgs(input: VcsCommandInput, excludedPathspecs: string[] = []) {
const args = ["diff", "--no-ext-diff", "--find-renames", "--no-color"];
export function buildGitDiffArgs(
input: VcsCommandInput,
excludedPathspecs: string[] = [],
colorMoved: GitColorMovedOptions | null = null,
) {
const args = ["diff", "--no-ext-diff", "--find-renames", ...gitPatchColorArgs(colorMoved)];

if (input.staged) {
args.push("--staged");
Expand All @@ -70,7 +120,7 @@ export function buildGitDiffArgs(input: VcsCommandInput, excludedPathspecs: stri
appendGitPathspecs(args, input.pathspecs);
}

return withNormalizedDiffPrefixes(args);
return withNormalizedDiffPrefixes(withGitMovedLineColorConfig(args, colorMoved));
}

/** Build the cheap tracked-file stats query used to skip huge file diffs before patch output. */
Expand Down Expand Up @@ -113,26 +163,45 @@ function buildGitNewFileDiffArgs(filePath: string) {
}

/** Build the exact `git show` arguments used for commit review. */
export function buildGitShowArgs(input: ShowCommandInput) {
const args = ["show", "--format=", "--no-ext-diff", "--find-renames", "--no-color"];
export function buildGitShowArgs(
input: ShowCommandInput,
colorMoved: GitColorMovedOptions | null = null,
) {
const args = [
"show",
"--format=",
"--no-ext-diff",
"--find-renames",
...gitPatchColorArgs(colorMoved),
];

if (input.ref) {
args.push(input.ref);
}

appendGitPathspecs(args, input.pathspecs);
return withNormalizedDiffPrefixes(args);
return withNormalizedDiffPrefixes(withGitMovedLineColorConfig(args, colorMoved));
}

/** Build the exact `git stash show -p` arguments used for stash review. */
export function buildGitStashShowArgs(input: StashShowCommandInput) {
const args = ["stash", "show", "-p", "--no-ext-diff", "--find-renames", "--no-color"];
export function buildGitStashShowArgs(
input: StashShowCommandInput,
colorMoved: GitColorMovedOptions | null = null,
) {
const args = [
"stash",
"show",
"-p",
"--no-ext-diff",
"--find-renames",
...gitPatchColorArgs(colorMoved),
];

if (input.ref) {
args.push(input.ref);
}

return withNormalizedDiffPrefixes(args);
return withNormalizedDiffPrefixes(withGitMovedLineColorConfig(args, colorMoved));
}

export function formatGitCommandLabel(input: GitBackedInput) {
Expand Down Expand Up @@ -323,6 +392,72 @@ export function runGitText(options: RunGitTextOptions) {
return runGitCommand(options).stdout;
}

const GIT_BOOLEAN_TRUE_VALUES = new Set(["true", "yes", "on", "1", "always"]);
const GIT_BOOLEAN_FALSE_VALUES = new Set(["false", "no", "off", "0", "never"]);

/** Read an optional Git config value without treating an unset key as an error. */
function readOptionalGitConfig(
input: GitBackedInput,
key: string,
options: Omit<RunGitTextOptions, "input" | "args"> = {},
) {
const result = runGitCommand({
input,
args: ["config", "--get", key],
...options,
acceptedExitCodes: [0, 1],
});

if (result.exitCode !== 0) {
return undefined;
}

return result.stdout.trim() || undefined;
}

/** Normalize Git's diff.colorMoved config into the mode Hunk should request from Git. */
function normalizeGitColorMovedMode(value: string | undefined) {
if (!value) {
return undefined;
}

const normalized = value.toLowerCase();
if (GIT_BOOLEAN_FALSE_VALUES.has(normalized) || normalized === "no") {
return null;
}
Comment on lines +425 to +427
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 "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.


if (GIT_BOOLEAN_TRUE_VALUES.has(normalized)) {
return "zebra";
}

return value;
}

/** Resolve whether Hunk should ask Git to color moved lines for this patch command. */
export function resolveGitColorMovedOptions(
input: GitBackedInput,
options: Omit<RunGitTextOptions, "input" | "args"> = {},
): GitColorMovedOptions | null {
const gitMode = normalizeGitColorMovedMode(
readOptionalGitConfig(input, "diff.colorMoved", options),
);

if (gitMode === null) {
return null;
}

const mode = gitMode ?? (input.options.colorMoved ? "zebra" : undefined);
if (!mode) {
return null;
}

const whitespaceMode = readOptionalGitConfig(input, "diff.colorMovedWS", options);
return {
mode,
whitespaceMode,
};
}

/**
* Return whether one `hunk diff` input still compares against the live working tree.
*
Expand Down
56 changes: 56 additions & 0 deletions src/core/loaders.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,62 @@ describe("loadAppBootstrap", () => {
]);
});

test("tags moved lines from git diff.colorMoved output", async () => {
const dir = createTempRepo("hunk-git-color-moved-");

writeFileSync(
join(dir, "example.txt"),
[
"start anchor",
"relocated block first line has many chars",
"relocated block second line has many chars",
"relocated block third line has many chars",
"middle unchanged one has many chars",
"middle unchanged two has many chars",
"end anchor",
"",
].join("\n"),
);
git(dir, "add", "example.txt");
git(dir, "commit", "-m", "initial");
git(dir, "config", "--local", "diff.colorMoved", "zebra");

writeFileSync(
join(dir, "example.txt"),
[
"start anchor",
"middle unchanged one has many chars",
"middle unchanged two has many chars",
"relocated block first line has many chars",
"relocated block second line has many chars",
"relocated block third line has many chars",
"end anchor",
"",
].join("\n"),
);

const bootstrap = await loadFromRepo(dir, {
kind: "vcs",
staged: false,
options: { mode: "auto" },
});
const file = bootstrap.changeset.files[0];

expect(file?.path).toBe("example.txt");
expect(file?.lineMoveKinds?.additionLines.some(Boolean)).toBe(true);
expect(file?.lineMoveKinds?.deletionLines.some(Boolean)).toBe(true);

const movedAdditions = file?.metadata.additionLines.filter(
(_line, index) => file.lineMoveKinds?.additionLines[index] === "moved",
);
const movedDeletions = file?.metadata.deletionLines.filter(
(_line, index) => file.lineMoveKinds?.deletionLines[index] === "moved",
);

expect(movedAdditions).toContain("middle unchanged one has many chars\n");
expect(movedDeletions).toContain("middle unchanged one has many chars\n");
});

test("reports a friendly error when git review runs outside a repository", async () => {
const dir = mkdtempSync(join(tmpdir(), "hunk-nonrepo-"));
tempDirs.push(dir);
Expand Down
Loading