diff --git a/extensions/git-read/src/args.ts b/extensions/git-read/src/args.ts index e0715c17..aca960c4 100644 --- a/extensions/git-read/src/args.ts +++ b/extensions/git-read/src/args.ts @@ -4,9 +4,9 @@ * Everything here is synchronous and side-effect free so the exact argv * passed to the child process can be asserted in tests. Revision and path * inputs are validated against strict shapes before they are ever placed - * in an argument list, and every user-controlled value goes after a `--` - * separator so it can never be parsed as a flag. Only argv subcommands that - * cannot write are reachable at all. + * in an argument list. Validated revisions go before an explicit `--` + * separator and paths go after it, so Git never guesses a revision is a + * path. Only argv subcommands that cannot write are reachable at all. */ export const GIT_TIMEOUT_MS = 10_000; @@ -84,10 +84,11 @@ export function buildShowArgs(params: GitShowParams): string[] { "--no-textconv", "--format=fuller", params.revision.trim(), + "--", ]; if (params.path !== undefined) { if (!isSafeRepoPath(params.path)) throw new InvalidPathError(params.path); - args.push("--", params.path); + args.push(params.path); } return args; } @@ -126,17 +127,16 @@ export function buildDiffArgs(params: GitDiffParams): string[] { if (params.stat) args.push("--stat"); if (from !== undefined && to !== undefined) { - // An explicit `--` separator is unnecessary for the range form; a - // validated revision can never start with `-` anyway. args.push(`${from}...${to}`); } else if (from !== undefined) { args.push(from); } // No revisions: worktree vs index (or HEAD with --cached). + args.push("--"); if (params.path !== undefined) { if (!isSafeRepoPath(params.path)) throw new InvalidPathError(params.path); - args.push("--", params.path); + args.push(params.path); } return args; } @@ -163,9 +163,10 @@ export function buildLogArgs(params: GitLogParams): string[] { } args.push(params.revision.trim()); } + args.push("--"); if (params.file !== undefined) { if (!isSafeRepoPath(params.file)) throw new InvalidPathError(params.file); - args.push("--", params.file); + args.push(params.file); } return args; } diff --git a/tests/extensions/git-read/args.test.ts b/tests/extensions/git-read/args.test.ts index 0f8fa20c..58cbe710 100644 --- a/tests/extensions/git-read/args.test.ts +++ b/tests/extensions/git-read/args.test.ts @@ -18,6 +18,7 @@ test("buildShowArgs validates revisions and paths", () => { "--no-textconv", "--format=fuller", "HEAD", + "--", ]); assert.deepEqual(buildShowArgs({ revision: " abc123 " }), [ "show", @@ -26,6 +27,7 @@ test("buildShowArgs validates revisions and paths", () => { "--no-textconv", "--format=fuller", "abc123", + "--", ]); assert.deepEqual(buildShowArgs({ revision: "HEAD~2", path: "src/a.ts" }), [ "show", @@ -76,6 +78,7 @@ test("buildDiffArgs composes only read-only diff forms", () => { "--no-color", "--no-ext-diff", "--no-textconv", + "--", ]); assert.deepEqual(buildDiffArgs({ staged: true }), [ "diff", @@ -83,6 +86,7 @@ test("buildDiffArgs composes only read-only diff forms", () => { "--no-ext-diff", "--no-textconv", "--cached", + "--", ]); assert.deepEqual(buildDiffArgs({ from: "main", to: "feat" }), [ "diff", @@ -90,6 +94,7 @@ test("buildDiffArgs composes only read-only diff forms", () => { "--no-ext-diff", "--no-textconv", "main...feat", + "--", ]); assert.deepEqual(buildDiffArgs({ from: "HEAD", stat: true, path: "src" }), [ "diff", @@ -125,6 +130,7 @@ test("buildLogArgs clamps the limit and validates inputs", () => { "--oneline", "-n", String(GIT_LOG_DEFAULT_LIMIT), + "--", ]); assert.deepEqual(buildLogArgs({ limit: 10_000, revision: "main" }), [ "log", @@ -134,6 +140,7 @@ test("buildLogArgs clamps the limit and validates inputs", () => { "-n", "1000", "main", + "--", ]); assert.deepEqual( buildLogArgs({ limit: 0, file: "src/a.ts", oneline: false }), diff --git a/tests/extensions/git-read/index.test.ts b/tests/extensions/git-read/index.test.ts index ccd0a7e5..6ac1eb7c 100644 --- a/tests/extensions/git-read/index.test.ts +++ b/tests/extensions/git-read/index.test.ts @@ -46,6 +46,7 @@ function makeRepo(): string { writeFileSync(join(dir, "b.txt"), "new\n"); git(dir, ["add", "."]); git(dir, ["commit", "--quiet", "-m", "second"]); + git(dir, ["branch", "a.txt", "HEAD"]); writeFileSync(join(dir, "a.txt"), "one\ntwo\nthree\n"); return dir; } @@ -105,6 +106,35 @@ test("git show with path limits the commit patch to that path", async () => { } }); +for (const [name, args, expected] of [ + ["show", buildShowArgs({ revision: "a.txt" }), /second/], + ["log", buildLogArgs({ revision: "a.txt" }), /first/], + ["diff", buildDiffArgs({ from: "a.txt" }), /\+three/], +] as const) { + test(`git ${name} accepts a revision that also names a worktree file`, async () => { + const exit = await Effect.runPromiseExit(runGit(args, repo)); + assert.ok(Exit.isSuccess(exit)); + if (Exit.isSuccess(exit)) { + assert.match(exit.value.output.preview, expected); + } + }); +} + +for (const [name, args] of [ + ["show", buildShowArgs({ revision: "b.txt" })], + ["log", buildLogArgs({ revision: "b.txt" })], + ["diff", buildDiffArgs({ from: "b.txt" })], +] as const) { + test(`git ${name} rejects a file-only name in the revision field`, async () => { + const exit = await Effect.runPromiseExit(runGit(args, repo)); + assert.ok(Exit.isFailure(exit)); + if (Exit.isFailure(exit)) { + const error = Cause.squash(exit.cause) as { message?: string }; + assert.match(error.message ?? "", /bad revision.*b\.txt/i); + } + }); +} + test("structured git diff and show never invoke diff.external", async () => { const { command, helper, marker } = markerCommand(repo, "external-diff"); try {