fix(patching): fall back to copy when hard linking fails in patch-commit (#15395)

In environments where hard linking fails (such as cross-device
boundaries or filesystems without hard link support), fall back to
copying package files when preparing the diff view.

Closes pnpm/pnpm#7295
This commit is contained in:
Zoltan Kochan authored and GitHub committed 2026-09-23 23:36:35 +02:00
1 parent aaa12cc785
commit 8fad8a309d
5 files changed
+394 -41

No files matched your search

@@ -0,0 +1,7 @@
---
"@pnpm/patching.commands": patch
"pnpm": patch
"pacquet": patch
---
`pnpm patch-commit` now falls back to copying package files when hard linking fails.
+63 -35
View File
@@ -65,11 +65,7 @@ pub enum PatchCommitError {
#[diagnostic(code(ERR_PNPM_PACKAGE_MANAGER_PATCH_COMMIT_UNSAFE_TEMP_DIR))]
UnsafeTempDir { dir: PathBuf, reason: &'static str },
#[display(
"Failed to link package file from {} to {}: {source}",
source_path.display(),
target.display()
)]
#[display("Failed to link package file from {} to {}: {source}", source_path.display(), target.display())]
#[diagnostic(code(ERR_PNPM_PACKAGE_MANAGER_PATCH_COMMIT_LINK_FILE))]
LinkFile {
source_path: PathBuf,
@@ -131,18 +127,22 @@ fn prepare_pkg_files_for_diff_with_fs(
dir: parent.to_path_buf(),
source,
})?;
fs_ops
.hard_link(&source_path, &target)
.map_err(|source| PatchCommitError::LinkFile { source_path, target, source })?;
match fs_ops.hard_link(&source_path, &target) {
Ok(()) => {}
Err(source) if is_unsupported_link_error(&source) => {
fs_ops
.copy(&source_path, &target)
.map_err(|source| PatchCommitError::LinkFile { source_path, target, source })?;
}
Err(source) => return Err(PatchCommitError::LinkFile { source_path, target, source }),
}
}
Ok(PkgFilesForDiff::Temporary(temp_dir))
}
pub fn diff_folders(folder_a: &Path, folder_b: &Path) -> Result<String, PatchCommitError> {
let folder_a_slash = slash_path(folder_a);
let folder_b_slash = slash_path(folder_b);
let stdout = DiffTempFile::new("stdout")?;
let stderr = DiffTempFile::new("stderr")?;
let (folder_a_slash, folder_b_slash) = (slash_path(folder_a), slash_path(folder_b));
let (stdout, stderr) = (DiffTempFile::new("stdout")?, DiffTempFile::new("stderr")?);
let status = git_diff_command()
.arg(&folder_a_slash)
.arg(&folder_b_slash)
@@ -220,6 +220,7 @@ trait PatchCommitFs {
fn symlink_metadata(&self, path: &Path) -> io::Result<fs::Metadata>;
fn create_dir_all(&self, path: &Path) -> io::Result<()>;
fn hard_link(&self, source: &Path, target: &Path) -> io::Result<()>;
fn copy(&self, source: &Path, target: &Path) -> io::Result<u64>;
fn remove_dir_all(&self, path: &Path) -> io::Result<()>;
}
@@ -238,18 +239,34 @@ impl PatchCommitFs for RealPatchCommitFs {
fs::hard_link(source, target)
}
fn copy(&self, source: &Path, target: &Path) -> io::Result<u64> {
let meta = self.symlink_metadata(source)?;
if meta.file_type().is_symlink() {
recreate_symlink(source, target).map(|()| 0)
} else {
fs::copy(source, target)
}
}
fn remove_dir_all(&self, path: &Path) -> io::Result<()> {
fs::remove_dir_all(path)
}
}
fn recreate_symlink(source: &Path, target: &Path) -> io::Result<()> {
let link_target = fs::read_link(source)?;
#[cfg(unix)]
return std::os::unix::fs::symlink(link_target, target);
#[cfg(windows)]
return std::os::windows::fs::symlink_file(link_target, target);
#[cfg(not(any(unix, windows)))]
return Err(io::Error::new(io::ErrorKind::Unsupported, "symlinks unsupported"));
}
fn temporary_filtered_dir(src: &Path) -> PathBuf {
let name = src
.file_name()
.map_or_else(|| String::from("patch"), |name| name.to_string_lossy().into_owned());
src.parent()
.unwrap_or_else(|| Path::new("."))
.join(format!("{name}_tmp_{}", std::process::id()))
let name = src.file_name().map_or_else(|| "patch".into(), |n| n.to_string_lossy());
let parent = src.parent().unwrap_or_else(|| Path::new("."));
parent.join(format!("{name}_tmp_{}", std::process::id()))
}
fn slash_path(path: &Path) -> String {
@@ -257,30 +274,47 @@ fn slash_path(path: &Path) -> String {
}
fn safe_package_file_path(path: &str) -> Result<PathBuf, PatchCommitError> {
let path = path_from_forward_slash(path);
if path.is_absolute()
|| path
let buf = path_from_forward_slash(path);
let escapes = buf.is_absolute()
|| buf
.components()
.any(|component| {
.any(|c| {
matches!(
component,
c,
std::path::Component::ParentDir
| std::path::Component::RootDir
| std::path::Component::Prefix(_),
)
})
{
});
if escapes {
return Err(PatchCommitError::InvalidPackageFilePath {
path: path.to_string_lossy().into_owned(),
path: buf.to_string_lossy().into_owned(),
});
}
Ok(path)
Ok(buf)
}
fn path_from_forward_slash(path: &str) -> PathBuf {
path.split('/').collect()
}
fn is_unsupported_link_error(error: &io::Error) -> bool {
if matches!(
error.kind(),
io::ErrorKind::CrossesDevices
| io::ErrorKind::Unsupported
| io::ErrorKind::PermissionDenied,
) {
return true;
}
#[cfg(unix)]
return error.raw_os_error() == Some(18);
#[cfg(windows)]
return error.raw_os_error() == Some(17);
#[cfg(not(any(unix, windows)))]
return false;
}
fn normalize_diff_output(diff: &str, folder_a: &str, folder_b: &str) -> String {
let mut out = String::with_capacity(diff.len());
let mut in_hunk = false;
@@ -323,8 +357,6 @@ fn normalize_diff_path_line(line: &str, folder_a: &str, folder_b: &str) -> Strin
out = out.replace(&format!("{prefix}/{trimmed}/"), &format!("{prefix}/"));
out = out.replace(&format!("{prefix}{folder}/"), &format!("{prefix}/"));
}
}
for folder in [folder_a, folder_b] {
out = out.replace(&format!("{folder}/"), "");
}
out
@@ -345,14 +377,10 @@ fn remove_ds_store_diff_blocks(diff: &str) -> String {
}
fn push_non_ds_store_block(output: &mut String, block: &str) {
if block.is_empty() {
return;
}
let header = block.lines().next().unwrap_or_default();
if is_ds_store_diff_header(header) {
return;
if !block.is_empty() && !is_ds_store_diff_header(header) {
output.push_str(block);
}
output.push_str(block);
}
fn is_ds_store_diff_header(header: &str) -> bool {
+201 -3
View File
@@ -168,7 +168,7 @@ fn patch_commit_prepare_pkg_files_for_diff_reports_nested_parent_create_errors()
}
#[test]
fn patch_commit_prepare_pkg_files_for_diff_reports_hard_link_errors() {
fn patch_commit_prepare_pkg_files_for_diff_falls_back_to_copy_on_hard_link_error() {
let edit_dir = tempdir().expect("edit dir");
fs::write(
edit_dir.path().join("package.json"),
@@ -178,12 +178,106 @@ fn patch_commit_prepare_pkg_files_for_diff_reports_hard_link_errors() {
fs::write(edit_dir.path().join("index.js"), "included\n").unwrap();
fs::write(edit_dir.path().join("ignore.txt"), "excluded\n").unwrap();
let err = prepare_pkg_files_for_diff_with_fs(edit_dir.path(), &HardLinkErrorFs)
.expect_err("hard link creation should fail");
let prepared = prepare_pkg_files_for_diff_with_fs(edit_dir.path(), &HardLinkErrorFs)
.expect("prepare files with copy fallback");
let PkgFilesForDiff::Temporary(path) = prepared else {
panic!("package files should be prepared in a temporary filtered dir");
};
assert_eq!(fs::read_to_string(path.join("index.js")).unwrap(), "included\n");
assert!(!path.join("ignore.txt").exists());
fs::remove_dir_all(path).unwrap();
}
#[cfg(any(unix, windows))]
#[test]
fn patch_commit_prepare_pkg_files_for_diff_falls_back_on_raw_cross_device_errno() {
let edit_dir = tempdir().expect("edit dir");
fs::write(
edit_dir.path().join("package.json"),
r#"{"name":"pkg","version":"1.0.0","files":["index.js"]}"#,
)
.unwrap();
fs::write(edit_dir.path().join("index.js"), "included\n").unwrap();
fs::write(edit_dir.path().join("ignore.txt"), "excluded\n").unwrap();
let prepared = prepare_pkg_files_for_diff_with_fs(edit_dir.path(), &RawExdevHardLinkErrorFs)
.expect("prepare files with raw exdev fallback");
let PkgFilesForDiff::Temporary(path) = prepared else {
panic!("package files should be prepared in a temporary filtered dir");
};
assert_eq!(fs::read_to_string(path.join("index.js")).unwrap(), "included\n");
fs::remove_dir_all(path).unwrap();
}
#[cfg(any(unix, windows))]
#[test]
fn patch_commit_prepare_pkg_files_for_diff_preserves_symlinks_on_copy_fallback() {
let edit_dir = tempdir().expect("edit dir");
fs::write(
edit_dir.path().join("package.json"),
r#"{"name":"pkg","version":"1.0.0","main":"link.js","files":["index.js"]}"#,
)
.unwrap();
fs::write(edit_dir.path().join("index.js"), "target content\n").unwrap();
#[cfg(unix)]
std::os::unix::fs::symlink("index.js", edit_dir.path().join("link.js")).unwrap();
#[cfg(windows)]
match std::os::windows::fs::symlink_file("index.js", edit_dir.path().join("link.js")) {
Ok(()) => {}
Err(err) if err.kind() == io::ErrorKind::PermissionDenied => return,
Err(err) => panic!("failed to create test symlink: {err}"),
}
let prepared = prepare_pkg_files_for_diff_with_fs(edit_dir.path(), &HardLinkErrorFs)
.expect("prepare files with copy fallback");
let PkgFilesForDiff::Temporary(path) = prepared else {
panic!("package files should be prepared in a temporary filtered dir");
};
let link_meta = fs::symlink_metadata(path.join("link.js")).unwrap();
assert!(link_meta.file_type().is_symlink(), "link.js must remain a symlink");
assert_eq!(fs::read_link(path.join("link.js")).unwrap(), Path::new("index.js"));
fs::remove_dir_all(path).unwrap();
}
#[test]
fn patch_commit_prepare_pkg_files_for_diff_reports_error_when_hard_link_and_copy_fail() {
let edit_dir = tempdir().expect("edit dir");
fs::write(
edit_dir.path().join("package.json"),
r#"{"name":"pkg","version":"1.0.0","files":["index.js"]}"#,
)
.unwrap();
fs::write(edit_dir.path().join("index.js"), "included\n").unwrap();
fs::write(edit_dir.path().join("ignore.txt"), "excluded\n").unwrap();
let err = prepare_pkg_files_for_diff_with_fs(edit_dir.path(), &HardLinkAndCopyErrorFs)
.expect_err("hard link and copy creation should fail");
assert!(matches!(err, PatchCommitError::LinkFile { .. }));
}
#[test]
fn patch_commit_prepare_pkg_files_for_diff_rethrows_unexpected_link_errors_without_copy() {
let edit_dir = tempdir().expect("edit dir");
fs::write(
edit_dir.path().join("package.json"),
r#"{"name":"pkg","version":"1.0.0","files":["index.js"]}"#,
)
.unwrap();
fs::write(edit_dir.path().join("index.js"), "included\n").unwrap();
fs::write(edit_dir.path().join("ignore.txt"), "excluded\n").unwrap();
let fs_ops = UnexpectedLinkErrorFs { copied: Cell::new(false) };
let err = prepare_pkg_files_for_diff_with_fs(edit_dir.path(), &fs_ops)
.expect_err("unexpected link error should fail");
assert!(matches!(err, PatchCommitError::LinkFile { .. }));
assert!(!fs_ops.copied.get(), "copy must not be attempted on unexpected link error");
}
/// `safe_package_file_path` is patch-commit's defense-in-depth guard
/// against a packlist entry that escapes the source dir. It is unit-tested
/// directly because the packlist now filters escaping `main` / `bin`
@@ -482,6 +576,10 @@ impl PatchCommitFs for CreateDirErrorFs {
fs::hard_link(source, target)
}
fn copy(&self, source: &Path, target: &Path) -> io::Result<u64> {
fs::copy(source, target)
}
fn remove_dir_all(&self, path: &Path) -> io::Result<()> {
match fs::remove_dir_all(path) {
Ok(()) => Ok(()),
@@ -502,10 +600,75 @@ impl PatchCommitFs for HardLinkErrorFs {
fs::create_dir_all(path)
}
fn hard_link(&self, _source: &Path, _target: &Path) -> io::Result<()> {
Err(io::Error::new(io::ErrorKind::CrossesDevices, "cross-device hard_link"))
}
fn copy(&self, source: &Path, target: &Path) -> io::Result<u64> {
RealPatchCommitFs.copy(source, target)
}
fn remove_dir_all(&self, path: &Path) -> io::Result<()> {
match fs::remove_dir_all(path) {
Ok(()) => Ok(()),
Err(error) if error.kind() == io::ErrorKind::NotFound => Ok(()),
Err(error) => Err(error),
}
}
}
struct RawExdevHardLinkErrorFs;
impl PatchCommitFs for RawExdevHardLinkErrorFs {
fn symlink_metadata(&self, path: &Path) -> io::Result<fs::Metadata> {
fs::symlink_metadata(path)
}
fn create_dir_all(&self, path: &Path) -> io::Result<()> {
fs::create_dir_all(path)
}
fn hard_link(&self, _source: &Path, _target: &Path) -> io::Result<()> {
#[cfg(unix)]
return Err(io::Error::from_raw_os_error(18));
#[cfg(windows)]
return Err(io::Error::from_raw_os_error(17));
#[cfg(not(any(unix, windows)))]
return Err(io::Error::new(io::ErrorKind::Other, "unsupported"));
}
fn copy(&self, source: &Path, target: &Path) -> io::Result<u64> {
RealPatchCommitFs.copy(source, target)
}
fn remove_dir_all(&self, path: &Path) -> io::Result<()> {
match fs::remove_dir_all(path) {
Ok(()) => Ok(()),
Err(error) if error.kind() == io::ErrorKind::NotFound => Ok(()),
Err(error) => Err(error),
}
}
}
struct HardLinkAndCopyErrorFs;
impl PatchCommitFs for HardLinkAndCopyErrorFs {
fn symlink_metadata(&self, path: &Path) -> io::Result<fs::Metadata> {
fs::symlink_metadata(path)
}
fn create_dir_all(&self, path: &Path) -> io::Result<()> {
fs::create_dir_all(path)
}
fn hard_link(&self, _source: &Path, _target: &Path) -> io::Result<()> {
Err(io::Error::new(io::ErrorKind::PermissionDenied, "blocked hard_link"))
}
fn copy(&self, _source: &Path, _target: &Path) -> io::Result<u64> {
Err(io::Error::new(io::ErrorKind::PermissionDenied, "blocked copy"))
}
fn remove_dir_all(&self, path: &Path) -> io::Result<()> {
match fs::remove_dir_all(path) {
Ok(()) => Ok(()),
@@ -530,7 +693,42 @@ impl PatchCommitFs for RemoveDirErrorFs {
fs::hard_link(source, target)
}
fn copy(&self, source: &Path, target: &Path) -> io::Result<u64> {
fs::copy(source, target)
}
fn remove_dir_all(&self, _path: &Path) -> io::Result<()> {
Err(io::Error::new(io::ErrorKind::PermissionDenied, "blocked remove_dir_all"))
}
}
struct UnexpectedLinkErrorFs {
copied: Cell<bool>,
}
impl PatchCommitFs for UnexpectedLinkErrorFs {
fn symlink_metadata(&self, path: &Path) -> io::Result<fs::Metadata> {
fs::symlink_metadata(path)
}
fn create_dir_all(&self, path: &Path) -> io::Result<()> {
fs::create_dir_all(path)
}
fn hard_link(&self, _source: &Path, _target: &Path) -> io::Result<()> {
Err(io::Error::new(io::ErrorKind::BrokenPipe, "unexpected link error"))
}
fn copy(&self, _source: &Path, _target: &Path) -> io::Result<u64> {
self.copied.set(true);
Ok(0)
}
fn remove_dir_all(&self, path: &Path) -> io::Result<()> {
match fs::remove_dir_all(path) {
Ok(()) => Ok(()),
Err(error) if error.kind() == io::ErrorKind::NotFound => Ok(()),
Err(error) => Err(error),
}
}
}
+27 -3
View File
@@ -1,5 +1,6 @@
import fs from 'node:fs'
import path from 'node:path'
import util from 'node:util'
import { docsUrl } from '@pnpm/cli.utils'
import { type Config, type ConfigContext, types as allTypes } from '@pnpm/config.reader'
@@ -301,8 +302,8 @@ function removeTrailingAndLeadingSlash (p: string): string {
* If all files should be included, return the original source directory without creating any links.
* This is required in order for the diff to not include files that are not part of the package.
*/
async function preparePkgFilesForDiff (src: string): Promise<string> {
const files = Array.from(new Set((await packlist(src)).map((f) => path.join(f))))
export async function preparePkgFilesForDiff (src: string, packageFiles?: string[]): Promise<string> {
const files = packageFiles ?? Array.from(new Set((await packlist(src)).map((f) => path.join(f))))
// If there are no extra files in the source directories, then there is no reason
// to copy.
if (await areAllFilesInPkg(files, src)) {
@@ -316,12 +317,35 @@ async function preparePkgFilesForDiff (src: string): Promise<string> {
const destFile = path.join(dest, file)
const destDir = path.dirname(destFile)
await fs.promises.mkdir(destDir, { recursive: true })
await fs.promises.link(srcFile, destFile)
try {
await fs.promises.link(srcFile, destFile)
} catch (err: unknown) {
if (isUnsupportedLinkError(err)) {
const stat = await fs.promises.lstat(srcFile)
if (stat.isSymbolicLink()) {
const target = await fs.promises.readlink(srcFile)
await fs.promises.symlink(target, destFile)
} else {
await fs.promises.copyFile(srcFile, destFile)
}
} else {
throw err
}
}
})
)
return dest
}
function isUnsupportedLinkError (err: unknown): boolean {
return (
util.types.isNativeError(err) &&
'code' in err &&
typeof err.code === 'string' &&
['EXDEV', 'EPERM', 'EACCES', 'ENOTSUP', 'EOPNOTSUPP'].includes(err.code)
)
}
async function areAllFilesInPkg (files: string[], basePath: string): Promise<boolean> {
const allFiles = await glob('**', {
cwd: basePath,
@@ -390,6 +390,102 @@ describe('patch and commit', () => {
expect(fs.existsSync('node_modules/is-positive/ignore.txt')).toBe(false)
})
test.each(['EXDEV', 'EACCES'])('patch and commit falls back to copy when hard linking fails with %s', async (code) => {
const output = await patch.handler(defaultPatchOption, ['is-positive@1.0.0'])
const patchDir = getPatchDirFromPatchOutput(output)
expect(fs.existsSync(patchDir)).toBe(true)
fs.writeFileSync(path.join(patchDir, 'ignore.txt'), '', 'utf8')
fs.appendFileSync(path.join(patchDir, 'index.js'), '// test fallback', 'utf8')
const linkSpy = jest.spyOn(fs.promises, 'link').mockRejectedValue(
Object.assign(new Error(`${code}: link failure`), { code })
)
try {
await patchCommit.handler({
...DEFAULT_OPTS,
cacheDir,
dir: process.cwd(),
rootProjectManifestDir: process.cwd(),
frozenLockfile: false,
fixLockfile: true,
storeDir,
}, [patchDir])
expect(linkSpy).toHaveBeenCalled()
expect(fs.existsSync('node_modules/is-positive/ignore.txt')).toBe(false)
expect(fs.readFileSync('node_modules/is-positive/index.js', 'utf8')).toContain('// test fallback')
} finally {
linkSpy.mockRestore()
}
})
test('preparePkgFilesForDiff preserves packaged symlinks when hard linking falls back to copy', async () => {
const editDir = path.join(temporaryDirectory(), 'pkg')
await fs.promises.mkdir(editDir, { recursive: true })
await fs.promises.writeFile(path.join(editDir, 'package.json'), JSON.stringify({ name: 'pkg', version: '1.0.0' }), 'utf8')
await fs.promises.writeFile(path.join(editDir, 'index.js'), 'target content\n', 'utf8')
await fs.promises.writeFile(path.join(editDir, 'ignore.txt'), 'ignore\n', 'utf8')
try {
await fs.promises.symlink('index.js', path.join(editDir, 'link.js'))
} catch (err: unknown) {
if (process.platform === 'win32' && err && typeof err === 'object' && 'code' in err && (err.code === 'EPERM' || err.code === 'EACCES')) {
return
}
throw err
}
const linkSpy = jest.spyOn(fs.promises, 'link').mockRejectedValue(
Object.assign(new Error('EXDEV: cross-device link not permitted'), { code: 'EXDEV' })
)
try {
const preparedDir = await patchCommit.preparePkgFilesForDiff(editDir, ['package.json', 'index.js', 'link.js'])
expect(linkSpy).toHaveBeenCalled()
expect(preparedDir).toBe(`${editDir}_tmp`)
const stat = await fs.promises.lstat(path.join(preparedDir, 'link.js'))
expect(stat.isSymbolicLink()).toBe(true)
const target = await fs.promises.readlink(path.join(preparedDir, 'link.js'))
expect(target).toBe('index.js')
await fs.promises.rm(preparedDir, { recursive: true, force: true })
} finally {
linkSpy.mockRestore()
await fs.promises.rm(editDir, { recursive: true, force: true })
}
})
test('patch and commit rethrows unexpected hard link errors without falling back to copy', async () => {
const output = await patch.handler(defaultPatchOption, ['is-positive@1.0.0'])
const patchDir = getPatchDirFromPatchOutput(output)
expect(fs.existsSync(patchDir)).toBe(true)
fs.writeFileSync(path.join(patchDir, 'ignore.txt'), '', 'utf8')
fs.appendFileSync(path.join(patchDir, 'index.js'), '// test failure', 'utf8')
const linkSpy = jest.spyOn(fs.promises, 'link').mockRejectedValue(
Object.assign(new Error('EIO: i/o error'), { code: 'EIO' })
)
try {
await expect(patchCommit.handler({
...DEFAULT_OPTS,
cacheDir,
dir: process.cwd(),
rootProjectManifestDir: process.cwd(),
frozenLockfile: false,
fixLockfile: true,
storeDir,
}, [patchDir])).rejects.toMatchObject({ code: 'EIO' })
expect(linkSpy).toHaveBeenCalled()
} finally {
linkSpy.mockRestore()
}
})
test('patch and commit with a custom edit dir', async () => {
const editDir = path.join(temporaryDirectory())