From 102c7cc71269324fea625c84bbf700d36a2b0cf9 Mon Sep 17 00:00:00 2001 From: Michael Hackner Date: Mon, 3 Aug 2026 21:16:56 -0700 Subject: [PATCH 1/2] fix(cli): accept `hunk diff A B` as the two-commit range A..B Git treats `diff A B` and `diff A..B` as the same two-commit review, but Hunk only understood the dotted spelling: a second positional was always folded into pathspecs, so `hunk diff main feature` silently reviewed `main` limited to a pathspec named `feature`. Two positionals now normalize to `A..B`, except where the old reading is the intended one: a first target that already spells a range, or a second token that exists on disk, which is what `diff ` without a `--` separator relies on. Co-authored-by: Claude Opus 5 --- .changeset/diff-two-commit-args.md | 5 +++ src/core/cli.test.ts | 44 +++++++++++++++++++ src/core/cli.ts | 35 ++++++++++++--- .../src/content/docs/docs/reference/cli.md | 1 + 4 files changed, 80 insertions(+), 5 deletions(-) create mode 100644 .changeset/diff-two-commit-args.md diff --git a/.changeset/diff-two-commit-args.md b/.changeset/diff-two-commit-args.md new file mode 100644 index 000000000..72d0a63a4 --- /dev/null +++ b/.changeset/diff-two-commit-args.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": patch +--- + +Accept `hunk diff A B` as the same two-commit review as `hunk diff A..B`. diff --git a/src/core/cli.test.ts b/src/core/cli.test.ts index 0ac924770..74d89aa46 100644 --- a/src/core/cli.test.ts +++ b/src/core/cli.test.ts @@ -266,6 +266,50 @@ describe("parseCli", () => { }); }); + test("treats two revision positionals as an A..B range", async () => { + const parsed = await parseCli(["bun", "hunk", "diff", "main", "feature"]); + + expect(parsed).toMatchObject({ + kind: "vcs", + range: "main..feature", + staged: false, + }); + }); + + test("treats two revision positionals with -- pathspecs as an A..B range", async () => { + const parsed = await parseCli(["bun", "hunk", "diff", "main", "feature", "--", "src/app.ts"]); + + expect(parsed).toMatchObject({ + kind: "vcs", + range: "main..feature", + pathspecs: ["src/app.ts"], + }); + }); + + test("keeps a single revision followed by an on-disk path as a pathspec", async () => { + const dir = createTempDir("hunk-cli-rev-path-"); + const pathspec = join(dir, "src"); + mkdirSync(pathspec); + + const parsed = await parseCli(["bun", "hunk", "diff", "HEAD", pathspec]); + + expect(parsed).toMatchObject({ + kind: "vcs", + range: "HEAD", + pathspecs: [pathspec], + }); + }); + + test("keeps a spelled-out range with a trailing pathspec that is not on disk", async () => { + const parsed = await parseCli(["bun", "hunk", "diff", "main..feature", "src/missing.ts"]); + + expect(parsed).toMatchObject({ + kind: "vcs", + range: "main..feature", + pathspecs: ["src/missing.ts"], + }); + }); + test("parses show mode with optional ref and pathspecs", async () => { const parsed = await parseCli(["bun", "hunk", "show", "HEAD~1", "--", "src/app.ts"]); diff --git a/src/core/cli.ts b/src/core/cli.ts index f95b66d09..581e59388 100644 --- a/src/core/cli.ts +++ b/src/core/cli.ts @@ -117,6 +117,7 @@ export const CLI_REFERENCE_COMMANDS = { summary: "review diffs or compare two concrete files", synopsis: [ "hunk diff [target] [-- ]", + "hunk diff [-- ]", "hunk diff --staged [-- ]", "hunk diff ", ], @@ -367,6 +368,7 @@ function renderCliHelp() { "", "Commands:", " hunk diff [target] [-- ] review working tree changes or compare against a target", + " hunk diff compare two commits (same as A..B)", " hunk diff --staged [-- ] review staged changes", " hunk diff compare two concrete files", " hunk show [target] [-- ] review the last commit or a given target", @@ -429,6 +431,11 @@ function areExistingFiles(left: string, right: string) { return [left, right].every((path) => existsSync(path) && statSync(path).isFile()); } +/** Return whether a diff target already spells its own range, as in `A..B` or `A...B`. */ +function isRangeExpression(target: string) { + return target.includes(".."); +} + /** Parse one standalone command while letting us capture `--help` as plain text. */ async function parseStandaloneCommand(command: Command, tokens: string[]) { command.exitOverride(); @@ -668,16 +675,34 @@ async function parseDiffCommand(tokens: string[], argv: string[]): Promise ` relies on. + if (!isRangeExpression(left) && (normalizedPathspecs !== undefined || !existsSync(right))) { + return { + kind: "vcs", + range: `${left}..${right}`, + staged, + pathspecs: normalizedPathspecs, + options, + }; + } + } + + if (!staged && !normalizedPathspecs) { return { kind: "vcs", range: parsedTargets[0]!, @@ -688,7 +713,7 @@ async function parseDiffCommand(tokens: string[], argv: string[]): Promise ` for file comparison.", + "Use `hunk diff [target] [-- pathspec...]`, `hunk diff `, or `hunk diff ` for file comparison.", ); } diff --git a/website/src/content/docs/docs/reference/cli.md b/website/src/content/docs/docs/reference/cli.md index 4e12c77c9..e0597b63c 100644 --- a/website/src/content/docs/docs/reference/cli.md +++ b/website/src/content/docs/docs/reference/cli.md @@ -48,6 +48,7 @@ review diffs or compare two concrete files ```bash hunk diff [target] [-- ] +hunk diff [-- ] hunk diff --staged [-- ] hunk diff ``` From 0ce938521594f6cc00cb90f623e43aff6486843f Mon Sep 17 00:00:00 2001 From: Michael Hackner Date: Fri, 7 Aug 2026 22:28:56 -0700 Subject: [PATCH 2/2] fix(vcs): let each backend spell a two-commit diff itself `hunk diff A B` normalized to the Git range `A..B` while parsing, before VCS detection. jj and Sapling read `..` as a revset over the commits between the endpoints, so a diverged from-side contributed nothing and its changes never appeared as removals. Verified against jj 0.44: `jj diff -r a..b` omits a deletion that `jj diff --from a --to b` reports. The endpoints now travel unjoined as `rangeEndpoints`, and each adapter names them in its own syntax: Git `A..B`, jj `--from`/`--to`, Sapling a `-r` per side. Parsing also stopped asking the filesystem whether a token is a revision or a pathspec. That answer moved with the working directory, and it read deleted files and globs as revisions. Two positionals are two commits; a pathspec needs `--`, unless a side already spells a range or there are more positionals than a commit pair can hold. Co-authored-by: Claude Opus 5 --- .changeset/diff-two-commit-args.md | 4 +- src/core/cli.test.ts | 42 +++++++++++----- src/core/cli.ts | 18 ++++--- src/core/vcs/diffRange.ts | 26 ++++++++++ src/core/vcs/git.test.ts | 31 ++++++++++++ src/core/vcs/git.ts | 54 ++++++++++++++------- src/core/vcs/jujutsu.test.ts | 40 +++++++++++++++ src/core/vcs/jujutsu.ts | 19 +++++++- src/core/vcs/sapling.test.ts | 15 ++++++ src/core/vcs/sapling.ts | 18 ++++++- src/extension-api/types.ts | 16 ++++++ src/extensions/default/vcs/git/index.ts | 6 ++- src/extensions/default/vcs/jujutsu/index.ts | 4 +- src/extensions/default/vcs/sapling/index.ts | 4 +- 14 files changed, 253 insertions(+), 44 deletions(-) create mode 100644 src/core/vcs/diffRange.ts diff --git a/.changeset/diff-two-commit-args.md b/.changeset/diff-two-commit-args.md index 72d0a63a4..3cc931248 100644 --- a/.changeset/diff-two-commit-args.md +++ b/.changeset/diff-two-commit-args.md @@ -1,5 +1,5 @@ --- -"hunkdiff": patch +"hunkdiff": minor --- -Accept `hunk diff A B` as the same two-commit review as `hunk diff A..B`. +Accept `hunk diff A B` as a two-commit review, same as Git's `A..B`. A pathspec following a single target now needs a `--` separator. diff --git a/src/core/cli.test.ts b/src/core/cli.test.ts index 74d89aa46..f0a4a78d3 100644 --- a/src/core/cli.test.ts +++ b/src/core/cli.test.ts @@ -266,41 +266,51 @@ describe("parseCli", () => { }); }); - test("treats two revision positionals as an A..B range", async () => { + test("treats two revision positionals as the two commits to compare", async () => { const parsed = await parseCli(["bun", "hunk", "diff", "main", "feature"]); expect(parsed).toMatchObject({ kind: "vcs", - range: "main..feature", + rangeEndpoints: { from: "main", to: "feature" }, staged: false, }); + // Joining them is the backend's job: `A..B` is Git spelling, and jj and + // Sapling read it as a revset that drops the from-side changes. + expect(parsed).not.toHaveProperty("range", "main..feature"); }); - test("treats two revision positionals with -- pathspecs as an A..B range", async () => { + test("treats two revision positionals with -- pathspecs as two commits", async () => { const parsed = await parseCli(["bun", "hunk", "diff", "main", "feature", "--", "src/app.ts"]); expect(parsed).toMatchObject({ kind: "vcs", - range: "main..feature", + rangeEndpoints: { from: "main", to: "feature" }, pathspecs: ["src/app.ts"], }); }); - test("keeps a single revision followed by an on-disk path as a pathspec", async () => { + test("reads a second positional as a revision whether or not it exists on disk", async () => { const dir = createTempDir("hunk-cli-rev-path-"); - const pathspec = join(dir, "src"); - mkdirSync(pathspec); + const onDisk = join(dir, "src"); + mkdirSync(onDisk); - const parsed = await parseCli(["bun", "hunk", "diff", "HEAD", pathspec]); + // A branch and a directory can share a name, so the filesystem cannot decide + // this. Both spellings parse the same way, and `--` is how you mean a path. + for (const second of [onDisk, join(dir, "missing")]) { + expect(await parseCli(["bun", "hunk", "diff", "HEAD", second])).toMatchObject({ + kind: "vcs", + rangeEndpoints: { from: "HEAD", to: second }, + }); + } - expect(parsed).toMatchObject({ + expect(await parseCli(["bun", "hunk", "diff", "HEAD", "--", onDisk])).toMatchObject({ kind: "vcs", range: "HEAD", - pathspecs: [pathspec], + pathspecs: [onDisk], }); }); - test("keeps a spelled-out range with a trailing pathspec that is not on disk", async () => { + test("keeps a trailing pathspec after a target that already spells a range", async () => { const parsed = await parseCli(["bun", "hunk", "diff", "main..feature", "src/missing.ts"]); expect(parsed).toMatchObject({ @@ -310,6 +320,16 @@ describe("parseCli", () => { }); }); + test("keeps bare pathspecs after a target when there are too many for a commit pair", async () => { + const parsed = await parseCli(["bun", "hunk", "diff", "HEAD", "src/app.ts", "src/other.ts"]); + + expect(parsed).toMatchObject({ + kind: "vcs", + range: "HEAD", + pathspecs: ["src/app.ts", "src/other.ts"], + }); + }); + test("parses show mode with optional ref and pathspecs", async () => { const parsed = await parseCli(["bun", "hunk", "show", "HEAD~1", "--", "src/app.ts"]); diff --git a/src/core/cli.ts b/src/core/cli.ts index 581e59388..b63fddf3f 100644 --- a/src/core/cli.ts +++ b/src/core/cli.ts @@ -368,7 +368,7 @@ function renderCliHelp() { "", "Commands:", " hunk diff [target] [-- ] review working tree changes or compare against a target", - " hunk diff compare two commits (same as A..B)", + " hunk diff compare two commits, like `git diff A B`", " hunk diff --staged [-- ] review staged changes", " hunk diff compare two concrete files", " hunk show [target] [-- ] review the last commit or a given target", @@ -688,13 +688,19 @@ async function parseDiffCommand(tokens: string[], argv: string[]): Promise ` relies on. - if (!isRangeExpression(left) && (normalizedPathspecs !== undefined || !existsSync(right))) { + // Git reads `diff A B` as the two-commit review `diff A..B`, so Hunk does too. + // The endpoints stay unjoined because `A..B` is Git spelling: jj and Sapling + // read `..` as a revset over the commits between them, so each backend has to + // name these two revisions in its own syntax. + // + // Whether the second token exists on disk deliberately does not enter into + // it. That answer depends on the working directory rather than the argument, + // and it read deleted files and globs as revisions. A pathspec needs `--`, + // unless a side already spells a range and so cannot be half of a new one. + if (!isRangeExpression(left) && !isRangeExpression(right)) { return { kind: "vcs", - range: `${left}..${right}`, + rangeEndpoints: { from: left, to: right }, staged, pathspecs: normalizedPathspecs, options, diff --git a/src/core/vcs/diffRange.ts b/src/core/vcs/diffRange.ts new file mode 100644 index 000000000..6a83fd42a --- /dev/null +++ b/src/core/vcs/diffRange.ts @@ -0,0 +1,26 @@ +import type { ExtensionVcsDiffInput } from "../../extension-api/types"; + +/** + * The compact `A..B` spelling for whatever revisions a review compares. + * + * This is display text — review titles, command labels, error messages — and it + * doubles as the literal argument Git takes, since `git diff A B` and + * `git diff A..B` are the same request. Backends that read `..` differently + * (jj and Sapling treat it as a revset) must build their arguments from + * `rangeEndpoints` instead, and use this only for text a human reads. + */ +export function describeDiffRange(input: ExtensionVcsDiffInput) { + const endpoints = input.rangeEndpoints; + return endpoints ? `${endpoints.from}..${endpoints.to}` : input.range; +} + +/** + * The review target exactly as the user spelled it on the command line. + * + * Command labels quote the invocation back in error messages, so two endpoints + * stay two arguments here rather than becoming a range the user never typed. + */ +export function describeDiffTargets(input: ExtensionVcsDiffInput) { + const endpoints = input.rangeEndpoints; + return endpoints ? `${endpoints.from} ${endpoints.to}` : input.range; +} diff --git a/src/core/vcs/git.test.ts b/src/core/vcs/git.test.ts index 837b82c33..97a7b11f2 100644 --- a/src/core/vcs/git.test.ts +++ b/src/core/vcs/git.test.ts @@ -97,6 +97,14 @@ describe("git command helpers", () => { expect(buildGitDiffArgs(makeGitInput())).toContain("core.quotePath=true"); }); + test("spells two named revisions as the A..B range Git takes for them", () => { + const args = buildGitDiffArgs( + makeGitInput({ rangeEndpoints: { from: "main", to: "feature" } }), + ); + + expect(args).toContain("main..feature"); + }); + test("disables external diff tools for stash patches", () => { const args = buildGitStashShowArgs({ kind: "stash-show", @@ -357,6 +365,29 @@ describe("resolveGitDiffEndpoints", () => { }); }); + test("two named endpoints resolve to the same pair as the range they spell", () => { + const repoRoot = createTempRepo("hunk-endpoints-two-targets-"); + writeFileSync(join(repoRoot, "x.txt"), "first\n"); + git(repoRoot, "add", "x.txt"); + git(repoRoot, "commit", "-m", "first"); + const firstSha = git(repoRoot, "rev-parse", "HEAD").trim(); + + writeFileSync(join(repoRoot, "x.txt"), "second\n"); + git(repoRoot, "add", "x.txt"); + git(repoRoot, "commit", "-m", "second"); + const secondSha = git(repoRoot, "rev-parse", "HEAD").trim(); + + const endpoints = resolveGitDiffEndpoints( + makeGitInput({ rangeEndpoints: { from: firstSha, to: secondSha } }), + { cwd: repoRoot, repoRoot }, + ); + + expect(endpoints).toEqual({ + old: { kind: "git-ref", ref: firstSha }, + new: { kind: "git-ref", ref: secondSha }, + }); + }); + test("rev^! resolves to the commit's parent..commit pair", () => { const repoRoot = createTempRepo("hunk-endpoints-bang-"); writeFileSync(join(repoRoot, "x.txt"), "first\n"); diff --git a/src/core/vcs/git.ts b/src/core/vcs/git.ts index 38f2fcc88..78c6444f2 100644 --- a/src/core/vcs/git.ts +++ b/src/core/vcs/git.ts @@ -6,6 +6,7 @@ import { type ExtensionVcsShowInput, type ExtensionVcsStashShowInput, } from "../../extension-api/types"; +import { describeDiffRange, describeDiffTargets } from "./diffRange"; import { LARGE_DIFF_FILE_MAX_BYTES, LARGE_DIFF_FILE_MAX_LINES } from "./largeFile"; import { escapeUntrackedPatchPath } from "../patch/normalize"; import { normalizePathForOS } from "../../lib/osPath"; @@ -130,8 +131,9 @@ export function buildGitDiffArgs( args.push("--staged"); } - if (input.range) { - args.push(input.range); + const range = describeDiffRange(input); + if (range) { + args.push(range); } if (excludedPathspecs.length > 0) { @@ -155,8 +157,9 @@ export function buildGitDiffNumstatArgs(input: ExtensionVcsDiffInput) { args.push("--staged"); } - if (input.range) { - args.push(input.range); + const range = describeDiffRange(input); + if (range) { + args.push(range); } appendGitPathspecs(args, input.pathspecs); @@ -283,12 +286,14 @@ export function buildGitStashShowArgs( export function formatGitCommandLabel(input: GitBackedInput) { switch (input.kind) { - case "vcs": + case "vcs": { if (input.staged) { return "hunk diff --staged"; } - return input.range ? `hunk diff ${input.range}` : "hunk diff"; + const targets = describeDiffTargets(input); + return targets ? `hunk diff ${targets}` : "hunk diff"; + } case "show": return input.ref ? `hunk show ${input.ref}` : "hunk show"; case "stash-show": @@ -356,9 +361,21 @@ function createMissingRepoError(input: GitBackedInput) { function createInvalidRevisionError(input: ExtensionVcsDiffInput | ExtensionVcsShowInput) { if (input.kind === "vcs") { + const endpoints = input.rangeEndpoints; return new HunkExtensionUserError( - `\`${formatGitCommandLabel(input)}\` could not resolve Git revision or range \`${input.range}\`.`, - { suggestions: ["Check the revision or range and try again."] }, + `\`${formatGitCommandLabel(input)}\` could not resolve Git revision or range \`${describeDiffRange(input)}\`.`, + { + suggestions: [ + "Check the revision or range and try again.", + // Two positionals are read as two commits, so someone who meant the + // second one as a path needs the separator to say so. + ...(endpoints + ? [ + `To limit the review to a path, separate it: \`hunk diff ${endpoints.from} -- ${endpoints.to}\`.`, + ] + : []), + ], + }, ); } @@ -418,7 +435,7 @@ function translateGitExitFailure(input: GitBackedInput, stderr: string) { return createMissingStashError(input); } - if (input.kind === "vcs" && input.range && isUnknownRevisionMessage(stderr)) { + if (input.kind === "vcs" && describeDiffRange(input) && isUnknownRevisionMessage(stderr)) { return createInvalidRevisionError(input); } @@ -568,11 +585,12 @@ function isWorkingTreeGitDiffInput( return false; } - if (!input.range) { + const range = describeDiffRange(input); + if (!range) { return true; } - const cacheKey = `${gitExecutable}\0${repoRoot ?? cwd}\0${input.range}`; + const cacheKey = `${gitExecutable}\0${repoRoot ?? cwd}\0${range}`; const cached = workingTreeGitDiffInputCache.get(cacheKey); if (cached !== undefined) { return cached; @@ -580,7 +598,7 @@ function isWorkingTreeGitDiffInput( const revs = runGitText({ input, - args: ["rev-parse", "--revs-only", input.range], + args: ["rev-parse", "--revs-only", range], cwd, gitExecutable, preventOptionalLocks, @@ -916,8 +934,10 @@ export function resolveGitDiffEndpoints( repoRoot, }: Omit & { repoRoot?: string } = {}, ): GitDiffEndpoints | null { + const range = describeDiffRange(input); + if (input.staged) { - if (!input.range) { + if (!range) { const headRef = tryResolveGitCommitRef(input, "HEAD", { cwd: repoRoot ?? cwd, gitExecutable, @@ -929,7 +949,7 @@ export function resolveGitDiffEndpoints( }; } - const { positives, negatives } = resolveRangeRevisions(input, input.range, { + const { positives, negatives } = resolveRangeRevisions(input, range, { cwd, gitExecutable, repoRoot, @@ -942,14 +962,14 @@ export function resolveGitDiffEndpoints( return null; } - if (!input.range) { + if (!range) { return { old: { kind: "index" }, new: { kind: "worktree" } }; } // `git diff A...B` compares merge-base(A, B) against B, not HEAD or the // working tree. Resolve the merge base explicitly so expanded source rows // read from the same revisions the diff was computed from. - const symmetric = parseSymmetricDiffRange(input.range); + const symmetric = parseSymmetricDiffRange(range); if (symmetric) { const mergeBase = runGitText({ input, @@ -976,7 +996,7 @@ export function resolveGitDiffEndpoints( // Real rev-parse failures (bogus refs, missing repo) propagate to the caller // so the user sees a clear error instead of a silent working-tree fallback. - const { positives, negatives } = resolveRangeRevisions(input, input.range, { + const { positives, negatives } = resolveRangeRevisions(input, range, { cwd, gitExecutable, repoRoot, diff --git a/src/core/vcs/jujutsu.test.ts b/src/core/vcs/jujutsu.test.ts index 9028a12c0..48b8ab82a 100644 --- a/src/core/vcs/jujutsu.test.ts +++ b/src/core/vcs/jujutsu.test.ts @@ -92,6 +92,46 @@ afterEach(() => { const jjTest = Bun.which("jj") ? test : test.skip; describe("jj command helpers", () => { + test("compares two named revisions with --from/--to rather than a `..` revset", () => { + expect(buildJjDiffArgs(diffInput({ rangeEndpoints: { from: "main", to: "feature" } }))).toEqual( + ["diff", "--git", "--from", "main", "--to", "feature"], + ); + }); + + test("passes a revset the user spelled straight through to -r", () => { + expect(buildJjDiffArgs(diffInput({ range: "trunk()..@" }))).toEqual([ + "diff", + "--git", + "-r", + "trunk()..@", + ]); + }); + + jjTest("keeps the from-side removals of two diverged revisions", () => { + const dir = createTempJjRepo("hunk-jj-diverged-endpoints-"); + writeFileSync(join(dir, "base.txt"), "base\n"); + jj(dir, "commit", "-m", "base"); + jj(dir, "bookmark", "create", "base", "-r", "@-"); + + writeFileSync(join(dir, "only-on-a.txt"), "a\n"); + jj(dir, "commit", "-m", "a"); + jj(dir, "bookmark", "create", "a", "-r", "@-"); + + jj(dir, "new", "base", "-m", "b"); + writeFileSync(join(dir, "only-on-b.txt"), "b\n"); + jj(dir, "commit", "-m", "b"); + jj(dir, "bookmark", "create", "b", "-r", "@-"); + + const input = diffInput({ rangeEndpoints: { from: "a", to: "b" } }); + const patch = runJjText({ input, args: buildJjDiffArgs(input), cwd: dir }); + + // `jj diff -r a..b` would show only the b-side addition: the revset holds the + // commits reachable from b but not from a, so nothing reverses a's own work. + expect(patch).toContain("only-on-b.txt"); + expect(patch).toContain("deleted file"); + expect(patch).toContain("only-on-a.txt"); + }); + test("reports a friendly error when jj is not installed or not on PATH", () => { expect(() => runJjText({ diff --git a/src/core/vcs/jujutsu.ts b/src/core/vcs/jujutsu.ts index 145100718..42a48f1ea 100644 --- a/src/core/vcs/jujutsu.ts +++ b/src/core/vcs/jujutsu.ts @@ -4,6 +4,7 @@ import { type ExtensionVcsShowInput, } from "../../extension-api/types"; import { normalizePathForOS } from "../../lib/osPath"; +import { describeDiffTargets } from "./diffRange"; export type JjBackedInput = ExtensionVcsDiffInput | ExtensionVcsShowInput; @@ -27,7 +28,12 @@ function appendJjFilesets(args: string[], pathspecs?: string[]) { export function buildJjDiffArgs(input: ExtensionVcsDiffInput) { const args = ["diff", "--git"]; - if (input.range) { + if (input.rangeEndpoints) { + // Not `-r from..to`: that revset covers the commits reachable from `to` but + // not from `from`, so a diverged `from` side contributes nothing and its + // changes never show up as removals. `--from`/`--to` compares the two trees. + args.push("--from", input.rangeEndpoints.from, "--to", input.rangeEndpoints.to); + } else if (input.range) { args.push("-r", input.range); } @@ -49,7 +55,8 @@ export function formatJjCommandLabel(input: JjBackedInput) { return "hunk diff --staged"; } - return input.range ? `hunk diff ${input.range}` : "hunk diff"; + const targets = describeDiffTargets(input); + return targets ? `hunk diff ${targets}` : "hunk diff"; } return input.ref ? `hunk show ${input.ref}` : "hunk show"; @@ -111,6 +118,14 @@ export function createJjStagedError(input: ExtensionVcsDiffInput) { } function createInvalidRevsetError(input: JjBackedInput) { + if (input.kind === "vcs" && input.rangeEndpoints) { + const { from, to } = input.rangeEndpoints; + return new HunkExtensionUserError( + `\`${formatJjCommandLabel(input)}\` could not resolve Jujutsu revisions \`${from}\` and \`${to}\`.`, + { suggestions: ["Check both revisions and try again."] }, + ); + } + const revset = input.kind === "vcs" ? input.range : (input.ref ?? "@"); return new HunkExtensionUserError( `\`${formatJjCommandLabel(input)}\` could not resolve Jujutsu revset \`${revset}\`.`, diff --git a/src/core/vcs/sapling.test.ts b/src/core/vcs/sapling.test.ts index 57cf5ad64..4e10db229 100644 --- a/src/core/vcs/sapling.test.ts +++ b/src/core/vcs/sapling.test.ts @@ -65,6 +65,21 @@ afterEach(() => { }); describe("sl command helpers", () => { + test("compares two named revisions with a -r per endpoint rather than a `..` revset", () => { + expect(buildSlDiffArgs(diffInput({ rangeEndpoints: { from: "main", to: "feature" } }))).toEqual( + ["diff", "--git", "-r", "main", "-r", "feature"], + ); + }); + + test("passes a revset the user spelled straight through to -r", () => { + expect(buildSlDiffArgs(diffInput({ range: ".^::." }))).toEqual([ + "diff", + "--git", + "-r", + ".^::.", + ]); + }); + test("reports a friendly error when sl is not installed or not on PATH", () => { expect(() => runSlText({ diff --git a/src/core/vcs/sapling.ts b/src/core/vcs/sapling.ts index a5c26a609..81f5fdbce 100644 --- a/src/core/vcs/sapling.ts +++ b/src/core/vcs/sapling.ts @@ -6,6 +6,7 @@ import { type ExtensionVcsShowInput, } from "../../extension-api/types"; import { normalizePathForOS } from "../../lib/osPath"; +import { describeDiffTargets } from "./diffRange"; export type SlBackedInput = ExtensionVcsDiffInput | ExtensionVcsShowInput; @@ -29,7 +30,11 @@ function appendSlPathspecs(args: string[], pathspecs?: string[]) { export function buildSlDiffArgs(input: ExtensionVcsDiffInput) { const args = ["diff", "--git"]; - if (input.range) { + if (input.rangeEndpoints) { + // Not `-r from..to`: `..` is a DAG range in Sapling revsets, not the + // two-tree comparison Git means by it. A `-r` per endpoint is that request. + args.push("-r", input.rangeEndpoints.from, "-r", input.rangeEndpoints.to); + } else if (input.range) { args.push("-r", input.range); } @@ -60,7 +65,8 @@ export function formatSlCommandLabel(input: SlBackedInput) { return "hunk diff --staged"; } - return input.range ? `hunk diff ${input.range}` : "hunk diff"; + const targets = describeDiffTargets(input); + return targets ? `hunk diff ${targets}` : "hunk diff"; } return input.ref ? `hunk show ${input.ref}` : "hunk show"; @@ -123,6 +129,14 @@ export function createSlStagedError(input: ExtensionVcsDiffInput) { } function createInvalidRevsetError(input: SlBackedInput) { + if (input.kind === "vcs" && input.rangeEndpoints) { + const { from, to } = input.rangeEndpoints; + return new HunkExtensionUserError( + `\`${formatSlCommandLabel(input)}\` could not resolve Sapling revisions \`${from}\` and \`${to}\`.`, + { suggestions: ["Check both revisions and try again."] }, + ); + } + const revset = input.kind === "vcs" ? input.range : (input.ref ?? "."); return new HunkExtensionUserError( `\`${formatSlCommandLabel(input)}\` could not resolve Sapling revset \`${revset}\`.`, diff --git a/src/extension-api/types.ts b/src/extension-api/types.ts index e0bebe63e..c3db4b979 100644 --- a/src/extension-api/types.ts +++ b/src/extension-api/types.ts @@ -535,10 +535,26 @@ export interface ExtensionVcsReviewOptions { colorMoved?: boolean; } +/** + * The two commits a `hunk diff A B` review compares, left unjoined. + * + * `A..B` is Git spelling. Jujutsu and Sapling read `..` as a revset over the + * commits *between* the endpoints, which drops A-side changes once the two have + * diverged, so Hunk cannot join them before it knows the backend. Each adapter + * spells this in its own two-sided form. + */ +export interface ExtensionVcsRangeEndpoints { + from: string; + to: string; +} + /** Working-tree review request, as extension adapters receive it. */ export interface ExtensionVcsDiffInput { kind: "vcs"; + /** A revision or range expression in the backend's own language, as typed. */ range?: string; + /** Set instead of `range` when the user named both endpoints as `hunk diff A B`. */ + rangeEndpoints?: ExtensionVcsRangeEndpoints; staged: boolean; pathspecs?: string[]; options: ExtensionVcsReviewOptions; diff --git a/src/extensions/default/vcs/git/index.ts b/src/extensions/default/vcs/git/index.ts index 4ba9b63fb..1855dfc58 100644 --- a/src/extensions/default/vcs/git/index.ts +++ b/src/extensions/default/vcs/git/index.ts @@ -21,6 +21,7 @@ import { type GitBackedInput, type GitDiffEndpoints, } from "../../../../core/vcs/git"; +import { describeDiffRange } from "../../../../core/vcs/diffRange"; import { gitEndpointSourceSpec, readGitFileSource } from "../../../../core/vcs/gitSource"; import { inspectLargeUntrackedFile } from "../../../../core/vcs/largeFile"; import { @@ -319,10 +320,11 @@ export const GitVcsAdapter = { async load(input, { cwd, gitExecutable = "git" }) { const repoRoot = resolveGitRepoRoot(input, { cwd, gitExecutable }); const repoName = basename(repoRoot); + const range = describeDiffRange(input); const title = input.staged ? `${repoName} staged changes` - : input.range - ? `${repoName} ${input.range}` + : range + ? `${repoName} ${range}` : `${repoName} working tree`; // Ask for stats before the patch so files too large to render can be // excluded from the diff instead of generating output nobody reads. diff --git a/src/extensions/default/vcs/jujutsu/index.ts b/src/extensions/default/vcs/jujutsu/index.ts index dfa08fe11..d3df0ba45 100644 --- a/src/extensions/default/vcs/jujutsu/index.ts +++ b/src/extensions/default/vcs/jujutsu/index.ts @@ -1,5 +1,6 @@ import fs from "node:fs"; import { dirname, join, resolve } from "node:path"; +import { describeDiffRange } from "../../../../core/vcs/diffRange"; import { buildJjDiffArgs, buildJjShowArgs, @@ -58,10 +59,11 @@ export const JjVcsAdapter = { } const repoRoot = resolveJjRepoRoot(input, { cwd }); const repoName = basename(repoRoot); + const range = describeDiffRange(input); return { repoRoot, sourceLabel: repoRoot, - title: input.range ? `${repoName} ${input.range}` : `${repoName} working copy`, + title: range ? `${repoName} ${range}` : `${repoName} working copy`, patchText: runJjText({ input, args: buildJjDiffArgs(input), cwd }), }; }, diff --git a/src/extensions/default/vcs/sapling/index.ts b/src/extensions/default/vcs/sapling/index.ts index 9eefbe113..366f025da 100644 --- a/src/extensions/default/vcs/sapling/index.ts +++ b/src/extensions/default/vcs/sapling/index.ts @@ -1,5 +1,6 @@ import fs from "node:fs"; import { dirname, join, resolve } from "node:path"; +import { describeDiffRange } from "../../../../core/vcs/diffRange"; import { buildSlDiffArgs, buildSlShowArgs, @@ -81,10 +82,11 @@ export const SaplingVcsAdapter = { } const repoRoot = resolveSlRepoRoot(input, { cwd }); const repoName = basename(repoRoot); + const range = describeDiffRange(input); return { repoRoot, sourceLabel: repoRoot, - title: input.range ? `${repoName} ${input.range}` : `${repoName} working copy`, + title: range ? `${repoName} ${range}` : `${repoName} working copy`, patchText: runSlText({ input, args: buildSlDiffArgs(input), cwd }), untrackedPaths: listSlUntrackedFiles(input, { cwd, repoRoot }), };