diff --git a/pnpm/scripts/format-staged-rust.mjs b/pnpm/scripts/format-staged-rust.mjs index 2d4e9ada01..8bd1a170bf 100644 --- a/pnpm/scripts/format-staged-rust.mjs +++ b/pnpm/scripts/format-staged-rust.mjs @@ -31,14 +31,14 @@ export function formatStagedRust (repo, { format = pinnedRustfmt } = {}) { console.error(`pre-commit: not formatting these files, each has unstaged changes too:\n${indent(withheld)}`) } if (foreign.length > 0) { - console.error(`pre-commit: not formatting these paths, none is a regular file in the checkout:\n${indent(foreign)}`) + console.error(`pre-commit: not formatting these paths, each must name one regular file inside the checkout:\n${indent(foreign)}`) } - if (formattable.length === 0) return 0 + if (formattable.size === 0) return 0 - const status = format(formattable.map(file => path.join(root, file))) + const status = format([...formattable.values()]) if (status !== 0) return status - const reformatted = formattable.filter(file => git(repo, ['diff', '--quiet', '--', literal(file)]).status !== 0) + const reformatted = [...formattable.keys()].filter(file => git(repo, ['diff', '--quiet', '--', literal(file)]).status !== 0) if (reformatted.length === 0) return 0 checkedGit(repo, ['add', '--', ...reformatted.map(literal)]) @@ -51,29 +51,51 @@ export function formatStagedRust (repo, { format = pinnedRustfmt } = {}) { * * A file that also has unstaged changes is held back: formatting the working * tree and staging the result would commit the part of that file the author - * deliberately kept out of this commit. A path that is not a regular file is - * not a source at all — rustfmt writes through a symlink, so a staged `.rs` - * link pointing out of the checkout would have it rewrite a file the commit - * never touches, and the repository would show nothing changed. + * deliberately kept out of this commit. A path that does not lead to a regular + * file inside the checkout is not a source at all — rustfmt writes through a + * symlink, so such a path would have it rewrite a file the commit never + * touches, and the repository would show nothing changed. */ function partitionStaged (root, staged, unstaged) { const alsoUnstaged = new Set(unstaged) - const formattable = [] + const formattable = new Map() const withheld = [] const foreign = [] for (const file of staged) { - if (!isRegularFile(path.join(root, file))) foreign.push(file) + const source = checkedOutSource(root, file) + if (source == null) foreign.push(file) else if (alsoUnstaged.has(file)) withheld.push(file) - else formattable.push(file) + else formattable.set(file, source) } return { formattable, withheld, foreign } } -function isRegularFile (absolute) { +/** + * The resolved path of a staged file that the formatter may rewrite in place, + * or null when the staged path is not one. + * + * rustfmt truncates the file it is given, so every other name that reaches the + * same bytes is rewritten with it. A path qualifies only when it names one + * regular file inside the checkout: not a symbolic link, not a second name for + * an inode that something outside the repository also holds, and not a path + * that resolves beyond `root`, which must already be resolved itself. + * + * The resolved path is what the caller hands the formatter. Passing the staged + * path on instead would have the formatter walk the links again, which is a + * second chance to arrive somewhere else. + */ +export function checkedOutSource (root, file) { + const absolute = path.join(root, file) try { - return fs.lstatSync(absolute).isFile() + const stats = fs.lstatSync(absolute) + if (!stats.isFile() || stats.nlink > 1) return null + const real = fs.realpathSync(absolute) + return real.startsWith(root + path.sep) ? real : null } catch (error) { - if (error.code === 'ENOENT') return false + // The three ways a path can fail to lead anywhere: nothing of that name, + // a non-directory used as one, a cycle of links. Each is a staged path to + // reject and report, not a reason to fail the commit. + if (['ENOENT', 'ENOTDIR', 'ELOOP'].includes(error.code)) return null throw error } } diff --git a/pnpm/scripts/format-staged-rust.test.mjs b/pnpm/scripts/format-staged-rust.test.mjs index dbf6ed8e18..1c054dcb79 100644 --- a/pnpm/scripts/format-staged-rust.test.mjs +++ b/pnpm/scripts/format-staged-rust.test.mjs @@ -1,8 +1,10 @@ import assert from 'node:assert/strict' +import console from 'node:console' import fs from 'node:fs' +import os from 'node:os' import path from 'node:path' import { test } from 'node:test' -import { formatStagedRust } from './format-staged-rust.mjs' +import { checkedOutSource, formatStagedRust } from './format-staged-rust.mjs' import { git, temporaryRepo } from './git-fixture.mjs' // Names git hands back verbatim that a shell would have split or expanded, or @@ -126,3 +128,84 @@ test('refuses to format a staged symlink', { skip: process.platform === 'win32' assert.deepEqual(seen, []) assert.equal(fs.readFileSync(outside, 'utf8'), 'fn outside() {}\n') }) + +test('refuses a staged path whose directory became a link out of the checkout', { skip: process.platform === 'win32' }, (context) => { + const repo = repoWithSources(context, ['real.rs']) + fs.mkdirSync(path.join(repo, 'dir')) + fs.writeFileSync(path.join(repo, 'dir', 'nested.rs'), 'fn nested() {}\n') + git(repo, 'add', '--all') + + const outside = fs.mkdtempSync(path.join(os.tmpdir(), 'pnpm-format-outside-')) + context.after(() => fs.rmSync(outside, { recursive: true, force: true })) + fs.writeFileSync(path.join(outside, 'nested.rs'), 'fn nested() {}\n') + fs.rmSync(path.join(repo, 'dir'), { recursive: true }) + fs.symlinkSync(outside, path.join(repo, 'dir')) + + const reported = context.mock.method(console, 'error', () => {}) + const { seen, format } = reformatter() + assert.equal(formatStagedRust(repo, { format }), 0) + + assert.deepEqual(seen, []) + assert.match(reported.mock.calls[0].arguments[0], /each must name one regular file inside the checkout:\n {2}dir\/nested\.rs/) + assert.equal(fs.readFileSync(path.join(outside, 'nested.rs'), 'utf8'), 'fn nested() {}\n') +}) + +test('rejects a source path that resolves outside the checkout', { skip: process.platform === 'win32' }, (context) => { + const repo = temporaryRepo(context, 'pnpm-format-contain-') + fs.writeFileSync(path.join(repo, 'inside.rs'), 'fn inside() {}\n') + const outside = `${repo}-outside.rs` + fs.writeFileSync(outside, 'fn outside() {}\n') + context.after(() => fs.rmSync(outside, { force: true })) + + fs.mkdirSync(path.join(repo, 'real-dir')) + fs.writeFileSync(path.join(repo, 'real-dir', 'nested.rs'), 'fn nested() {}\n') + fs.symlinkSync(path.join(repo, 'real-dir'), path.join(repo, 'link-dir')) + + assert.equal(checkedOutSource(repo, 'inside.rs'), path.join(repo, 'inside.rs')) + // The path the formatter is handed is the resolved one, not the one that + // still has a link to walk. + assert.equal(checkedOutSource(repo, 'link-dir/nested.rs'), path.join(repo, 'real-dir', 'nested.rs')) + assert.equal(checkedOutSource(repo, `../${path.basename(outside)}`), null) + assert.equal(checkedOutSource(repo, 'missing.rs'), null) +}) + +test('refuses to format a staged hardlink to a file outside the checkout', { skip: process.platform === 'win32' }, (context) => { + const repo = repoWithSources(context, ['real.rs']) + const outside = `${repo}-outside.rs` + fs.writeFileSync(outside, 'fn outside() {}\n') + context.after(() => fs.rmSync(outside, { force: true })) + fs.linkSync(outside, path.join(repo, 'linked.rs')) + git(repo, 'add', '--all') + + const reported = context.mock.method(console, 'error', () => {}) + const { seen, format } = reformatter() + assert.equal(formatStagedRust(repo, { format }), 0) + + assert.deepEqual(seen, []) + assert.match(reported.mock.calls[0].arguments[0], /must name one regular file inside the checkout:\n {2}linked\.rs/) + assert.equal(fs.readFileSync(outside, 'utf8'), 'fn outside() {}\n') +}) + +// Each replacement makes `dir/nested.rs` fail to resolve with a different +// errno, and every one of them used to abort the commit. +for (const [errno, replaceDirectory] of [ + ['ENOTDIR', (dir) => fs.writeFileSync(dir, 'no longer a directory\n')], + ['ELOOP', (dir) => fs.symlinkSync(path.basename(dir), dir)], +]) { + test(`reports a staged path that stops resolving with ${errno}`, { skip: process.platform === 'win32' }, (context) => { + const repo = repoWithSources(context, ['real.rs']) + fs.mkdirSync(path.join(repo, 'dir')) + fs.writeFileSync(path.join(repo, 'dir', 'nested.rs'), 'fn nested() {}\n') + git(repo, 'add', '--all') + fs.rmSync(path.join(repo, 'dir'), { recursive: true }) + replaceDirectory(path.join(repo, 'dir')) + assert.throws(() => fs.lstatSync(path.join(repo, 'dir', 'nested.rs')), { code: errno }) + + const reported = context.mock.method(console, 'error', () => {}) + const { seen, format } = reformatter() + assert.equal(formatStagedRust(repo, { format }), 0) + + assert.deepEqual(seen, []) + assert.match(reported.mock.calls[0].arguments[0], /must name one regular file inside the checkout:\n {2}dir\/nested\.rs/) + }) +}