fix(cli): accept hunk diff A B as the two-commit range A..B

.changeset/diff-two-commit-args.md

@@ -0,0 +1,5 @@
+---
+"hunkdiff": minor
+---
+
+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.

src/core/cli.test.ts

@@ -266,6 +266,70 @@ describe("parseCli", () => {
     });
   });
 
+  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",
+      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 two commits", async () => {
+    const parsed = await parseCli(["bun", "hunk", "diff", "main", "feature", "--", "src/app.ts"]);
+
+    expect(parsed).toMatchObject({
+      kind: "vcs",
+      rangeEndpoints: { from: "main", to: "feature" },
+      pathspecs: ["src/app.ts"],
+    });
+  });
+
+  test("reads a second positional as a revision whether or not it exists on disk", async () => {
+    const dir = createTempDir("hunk-cli-rev-path-");
+    const onDisk = join(dir, "src");
+    mkdirSync(onDisk);
+
+    // 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(await parseCli(["bun", "hunk", "diff", "HEAD", "--", onDisk])).toMatchObject({
+      kind: "vcs",
+      range: "HEAD",
+      pathspecs: [onDisk],
+    });
+  });
+
+  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({
+      kind: "vcs",
+      range: "main..feature",
+      pathspecs: ["src/missing.ts"],
+    });
+  });
+
+  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"]);
 

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] [-- <pathspec...>]",
+      "hunk diff <commit> <commit> [-- <pathspec...>]",
       "hunk diff --staged [-- <pathspec...>]",
       "hunk diff <left> <right>",
     ],
@@ -367,6 +368,7 @@ function renderCliHelp() {
     "",
     "Commands:",
     "  hunk diff [target] [-- <pathspec...>]   review working tree changes or compare against a target",
+    "  hunk diff <commit> <commit>             compare two commits, like `git diff A B`",
     "  hunk diff --staged [-- <pathspec...>]   review staged changes",
     "  hunk diff <left> <right>                compare two concrete files",
     "  hunk show [target] [-- <pathspec...>]   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,40 @@ async function parseDiffCommand(tokens: string[], argv: string[]): Promise<Parse
     };
   }
 
-  if (!staged && !normalizedPathspecs) {
-    if (parsedTargets.length === 2 && areExistingFiles(parsedTargets[0]!, parsedTargets[1]!)) {
+  if (!staged && parsedTargets.length === 2) {
+    const left = parsedTargets[0]!;
+    const right = parsedTargets[1]!;
+
+    if (!normalizedPathspecs && areExistingFiles(left, right)) {
       return {
         kind: "diff",
-        left: parsedTargets[0]!,
-        right: parsedTargets[1]!,
+        left,
+        right,
         options,
       };
     }
 
+    // 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",
+        rangeEndpoints: { from: left, to: right },
+        staged,
+        pathspecs: normalizedPathspecs,
+        options,
+      };
+    }
+  }
+
+  if (!staged && !normalizedPathspecs) {
     return {
       kind: "vcs",
       range: parsedTargets[0]!,
@@ -688,7 +719,7 @@ async function parseDiffCommand(tokens: string[], argv: string[]): Promise<Parse
   }
 
   throw new Error(
-    "Use `hunk diff [target] [-- pathspec...]`, `hunk diff <left> <right>` for file comparison.",
+    "Use `hunk diff [target] [-- pathspec...]`, `hunk diff <commit> <commit>`, or `hunk diff <left> <right>` for file comparison.",
   );
 }
 

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;
+}

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");

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,19 +585,20 @@ 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;
   }
 
   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<RunGitTextOptions, "input" | "args"> & { 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,