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
5 changes: 5 additions & 0 deletions .changeset/diff-two-commit-args.md
Original file line number Diff line number Diff line change
@@ -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.
64 changes: 64 additions & 0 deletions src/app/cli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,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"]);

Expand Down
41 changes: 36 additions & 5 deletions src/app/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,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>",
],
Expand Down Expand Up @@ -440,6 +441,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",
Expand Down Expand Up @@ -505,6 +507,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();
Expand Down Expand Up @@ -733,16 +740,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]!,
Expand All @@ -753,7 +784,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.",
);
}

Expand Down
16 changes: 16 additions & 0 deletions src/extension-api/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -679,10 +679,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;
Expand Down
32 changes: 32 additions & 0 deletions src/extensions/default/vcs/diffRange.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import type { ExtensionVcsDiffInput } from "hunkdiff/extension";

/**
* Shared by the bundled Git, Jujutsu, and Sapling backends: each spells a
* two-commit diff in its own argument syntax, but all three quote the same
* review back to the user.
*/

/**
* 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;
}
31 changes: 31 additions & 0 deletions src/extensions/default/vcs/git/commands.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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");
Expand Down
Loading
Loading