diff --git a/src/install/PackageManager/PackageManagerDirectories.rs b/src/install/PackageManager/PackageManagerDirectories.rs index 950f3ef74b54..f8e99e405449 100644 --- a/src/install/PackageManager/PackageManagerDirectories.rs +++ b/src/install/PackageManager/PackageManagerDirectories.rs @@ -918,12 +918,27 @@ pub struct CacheDirAndSubpath<'a> { pub(crate) cache_dir_subpath: &'a ZStr, } +/// Slice one of a `Resolution`'s strings with the buffer it actually indexes +/// into: `resolution_string_bytes` when given, else `lockfile`'s own. +fn resolution_str<'a, T: Semver::Slicable>( + lockfile: &'a Lockfile, + resolution_string_bytes: Option<&'a [u8]>, + s: &'a T, +) -> &'a [u8] { + s.slice(resolution_string_bytes.unwrap_or_else(|| lockfile.buffers.string_bytes.as_slice())) +} + /// this is copy pasted from `installPackageWithNameAndResolution()` /// it's not great to do this +/// +/// `resolution_string_bytes`: the string buffer `resolution`'s strings index +/// into when it came from a lockfile other than `manager.lockfile` (`bun +/// patch --commit` loads its own copy); `None` means `manager.lockfile`. pub fn compute_cache_dir_and_subpath<'a>( manager: &mut PackageManager, pkg_name: &[u8], resolution: &Resolution, + resolution_string_bytes: Option<&[u8]>, folder_path_buf: &'a mut PathBuffer, patch_hash: Option, ) -> CacheDirAndSubpath<'a> { @@ -938,18 +953,37 @@ pub fn compute_cache_dir_and_subpath<'a>( cache_dir = get_cache_directory(manager); } ResolutionTag::Git => { - let git = resolution.git(); - cache_dir_subpath = cached_git_folder_name(manager, git, patch_hash); + let resolved = resolution_str( + &manager.lockfile, + resolution_string_bytes, + &resolution.git().resolved, + ); + cache_dir_subpath = cached_git_folder_name_print( + cached_package_folder_name_buf(), + resolved, + patch_hash, + ); cache_dir = get_cache_directory(manager); } ResolutionTag::Github => { - let github = resolution.github(); - cache_dir_subpath = cached_github_folder_name(manager, github, patch_hash); + let resolved = resolution_str( + &manager.lockfile, + resolution_string_bytes, + &resolution.github().resolved, + ); + cache_dir_subpath = cached_github_folder_name_print( + cached_package_folder_name_buf(), + resolved, + patch_hash, + ); cache_dir = get_cache_directory(manager); } ResolutionTag::Folder => { - let buf = manager.lockfile.buffers.string_bytes.as_slice(); - let folder = resolution.folder().slice(buf); + let folder = resolution_str( + &manager.lockfile, + resolution_string_bytes, + resolution.folder(), + ); // Handle when a package depends on itself via file: // example: // "mineflayer": "file:." @@ -963,18 +997,31 @@ pub fn compute_cache_dir_and_subpath<'a>( cache_dir = Fd::cwd(); } ResolutionTag::LocalTarball => { - let tarball = *resolution.local_tarball(); - cache_dir_subpath = cached_tarball_folder_name(manager, tarball, patch_hash); + let url = resolution_str( + &manager.lockfile, + resolution_string_bytes, + resolution.local_tarball(), + ); + cache_dir_subpath = + cached_tarball_folder_name_print(cached_package_folder_name_buf(), url, patch_hash); cache_dir = get_cache_directory(manager); } ResolutionTag::RemoteTarball => { - let tarball = *resolution.remote_tarball(); - cache_dir_subpath = cached_tarball_folder_name(manager, tarball, patch_hash); + let url = resolution_str( + &manager.lockfile, + resolution_string_bytes, + resolution.remote_tarball(), + ); + cache_dir_subpath = + cached_tarball_folder_name_print(cached_package_folder_name_buf(), url, patch_hash); cache_dir = get_cache_directory(manager); } ResolutionTag::Workspace => { - let buf = manager.lockfile.buffers.string_bytes.as_slice(); - let folder = resolution.workspace().slice(buf); + let folder = resolution_str( + &manager.lockfile, + resolution_string_bytes, + resolution.workspace(), + ); // Handle when a package depends on itself if folder.is_empty() || (folder.len() == 1 && folder[0] == b'.') { cache_dir_subpath = z_static(b".\0"); @@ -991,10 +1038,12 @@ pub fn compute_cache_dir_and_subpath<'a>( // borrowck — `global_link_dir_path` below reborrows // `manager` mutably, so copy the symlink target out of the lockfile // string buffer first instead of holding a slice across that call. - let folder = resolution - .symlink() - .slice(manager.lockfile.buffers.string_bytes.as_slice()) - .to_vec(); + let folder = resolution_str( + &manager.lockfile, + resolution_string_bytes, + resolution.symlink(), + ) + .to_vec(); if folder.is_empty() || (folder.len() == 1 && folder[0] == b'.') { cache_dir_subpath = z_static(b".\0"); diff --git a/src/install/PackageManager/patchPackage.rs b/src/install/PackageManager/patchPackage.rs index a416be8d9293..cbffe691b38a 100644 --- a/src/install/PackageManager/patchPackage.rs +++ b/src/install/PackageManager/patchPackage.rs @@ -252,6 +252,7 @@ pub fn do_patch_commit( manager, &name, &resolution_clone, + Some(lockfile.buffers.string_bytes.as_slice()), &mut folder_path_buf, None, ); @@ -289,6 +290,7 @@ pub fn do_patch_commit( manager, &pkg_name_slice, &resolution_clone, + Some(lockfile.buffers.string_bytes.as_slice()), &mut folder_path_buf, None, ); @@ -665,6 +667,15 @@ fn escape_patch_filename(name: &[u8]) -> Option> { Newline, CarriageReturn, Tab, + // Reserved in Windows filenames; escaped on every OS so a committed + // patches/ dir stays checkoutable on Windows. + Colon, + Question, + Asterisk, + Quote, + LessThan, + GreaterThan, + Pipe, // Dot, Other, } @@ -678,6 +689,13 @@ fn escape_patch_filename(name: &[u8]) -> Option> { EscapeVal::Newline => Some(b"%0A"), EscapeVal::CarriageReturn => Some(b"%0D"), EscapeVal::Tab => Some(b"%09"), + EscapeVal::Colon => Some(b"%3A"), + EscapeVal::Question => Some(b"%3F"), + EscapeVal::Asterisk => Some(b"%2A"), + EscapeVal::Quote => Some(b"%22"), + EscapeVal::LessThan => Some(b"%3C"), + EscapeVal::GreaterThan => Some(b"%3E"), + EscapeVal::Pipe => Some(b"%7C"), // EscapeVal::Dot => Some(b"%2E"), EscapeVal::Other => None, } @@ -693,6 +711,13 @@ fn escape_patch_filename(name: &[u8]) -> Option> { table[b'\n' as usize] = EscapeVal::Newline; table[b'\r' as usize] = EscapeVal::CarriageReturn; table[b'\t' as usize] = EscapeVal::Tab; + table[b':' as usize] = EscapeVal::Colon; + table[b'?' as usize] = EscapeVal::Question; + table[b'*' as usize] = EscapeVal::Asterisk; + table[b'"' as usize] = EscapeVal::Quote; + table[b'<' as usize] = EscapeVal::LessThan; + table[b'>' as usize] = EscapeVal::GreaterThan; + table[b'|' as usize] = EscapeVal::Pipe; table }; let mut count: usize = 0; @@ -889,6 +914,7 @@ pub fn prepare_patch(manager: &mut PackageManager) -> Result<(), crate::Error> { manager, &name, &actual_package.resolution, + None, &mut folder_path_buf, existing_patchfile_hash, ); @@ -949,6 +975,7 @@ pub fn prepare_patch(manager: &mut PackageManager) -> Result<(), crate::Error> { manager, &pkg_name, &pkg_resolution, + None, &mut folder_path_buf, existing_patchfile_hash, ); diff --git a/src/install/patch_install.rs b/src/install/patch_install.rs index 03b09ccdd32f..1b9f303664a3 100644 --- a/src/install/patch_install.rs +++ b/src/install/patch_install.rs @@ -793,6 +793,7 @@ impl PatchTask { pkg_manager, &pkg_name_slice, &resolution_clone, + None, &mut folder_path_buf, Some(patch_hash), ); diff --git a/test/cli/install/bun-patch.test.ts b/test/cli/install/bun-patch.test.ts index 0f7e828373d1..ff3daf685a64 100644 --- a/test/cli/install/bun-patch.test.ts +++ b/test/cli/install/bun-patch.test.ts @@ -946,3 +946,180 @@ module.exports = function isOdd() { } }); }); + +// `bun patch --commit` derives the pristine copy's cache folder from the +// package's resolution. For non-registry resolutions (git, github, tarball) +// the resolution strings live in the lockfile's string buffer; resolving them +// against the wrong buffer produced paths like "@GH@@@@1" and the diff step +// failed with "Could not access". +describe.concurrent("bun patch --commit for non-registry dependencies", () => { + async function runBun(cwd: string, env: Record, ...args: string[]) { + await using proc = Bun.spawn({ + cmd: [bunExe(), ...args], + env, + cwd, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { stdout, stderr, exitCode }; + } + + async function expectPatchFlowWorks(dir: string, env: Record, commitArg: string) { + { + const { stderr, exitCode } = await runBun(dir, env, "install"); + expect(exitCode, `bun install failed: ${stderr}`).toBe(0); + } + { + const { stderr, exitCode } = await runBun(dir, env, "patch", "pkg-to-patch"); + expect(exitCode, `bun patch failed: ${stderr}`).toBe(0); + } + + await Bun.write(join(dir, "node_modules", "pkg-to-patch", "index.js"), `module.exports = "patched";\n`); + + { + const { stderr, exitCode } = await runBun(dir, env, "patch", "--commit", commitArg); + expect(stderr).not.toContain("Could not access"); + expect(exitCode, `bun patch --commit failed: ${stderr}`).toBe(0); + } + + const pkg = await Bun.file(join(dir, "package.json")).json(); + const entries = Object.entries(pkg.patchedDependencies ?? {}) as [string, string][]; + expect(entries).toHaveLength(1); + const [patchKey, patchPath] = entries[0]; + // the filename must stay valid on Windows (no NTFS-reserved characters) + expect(patchPath).not.toMatch(/[:?*"<>|]/); + const patchContents = await Bun.file(join(dir, patchPath)).text(); + expect(patchContents).toContain('-module.exports = "original";'); + expect(patchContents).toContain('+module.exports = "patched";'); + // the commit flow reinstalls with the patch applied + expect(await Bun.file(join(dir, "node_modules", "pkg-to-patch", "index.js")).text()).toBe( + `module.exports = "patched";\n`, + ); + return patchKey; + } + + test("github dependency", async () => { + await using dir = tempDir("patch-commit-github", { + "package.json": JSON.stringify({ + name: "test-patch-github", + dependencies: { "pkg-to-patch": "github:testowner/testrepo#aaaaaaa" }, + }), + // GitHub API tarballs have an `--` root folder; + // that folder name becomes the `resolved` part of the cache folder name. + "tarball-src": { + "testowner-testrepo-aaaaaaa": { + "package.json": JSON.stringify({ name: "pkg-to-patch", version: "1.0.0" }), + "index.js": `module.exports = "original";\n`, + }, + }, + }); + + await using tarProc = Bun.spawn({ + cmd: [ + "tar", + "-czf", + join(String(dir), "gh.tgz"), + "-C", + join(String(dir), "tarball-src"), + "testowner-testrepo-aaaaaaa", + ], + env: bunEnv, + stdout: "inherit", + stderr: "inherit", + }); + expect(await tarProc.exited).toBe(0); + const tgz = await Bun.file(join(String(dir), "gh.tgz")).bytes(); + + await using server = Bun.serve({ + port: 0, + fetch: () => new Response(tgz, { headers: { "content-type": "application/gzip" } }), + }); + + const env = { + ...bunEnv, + GITHUB_API_URL: `http://localhost:${server.port}`, + BUN_INSTALL_CACHE_DIR: join(String(dir), ".bun-cache"), + }; + + const patchKey = await expectPatchFlowWorks(String(dir), env, "node_modules/pkg-to-patch"); + expect(patchKey).toBe("pkg-to-patch@github:testowner/testrepo#aaaaaaa"); + }); + + test("git dependency", async () => { + await using dir = tempDir("patch-commit-git", { + "gitrepo": { + "package.json": JSON.stringify({ name: "pkg-to-patch", version: "1.0.0" }), + "index.js": `module.exports = "original";\n`, + }, + "project": {}, + }); + const repo = join(String(dir), "gitrepo"); + + // keep git away from the machine's global/system config (autocrlf, gpgsign) + const gitEnv = { ...bunEnv, GIT_CONFIG_NOSYSTEM: "1", GIT_CONFIG_GLOBAL: join(String(dir), "no-gitconfig") }; + for (const args of [ + ["init", "-q"], + ["config", "core.autocrlf", "false"], + ["add", "-A"], + ["-c", "user.email=test@test.test", "-c", "user.name=test", "commit", "-q", "-m", "init"], + // serve the repo over git's dumb HTTP protocol (plain file fetches) + ["update-server-info"], + ]) { + await using proc = Bun.spawn({ cmd: ["git", ...args], cwd: repo, env: gitEnv, stderr: "pipe" }); + const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); + expect(exitCode, `git ${args.join(" ")} failed: ${stderr}`).toBe(0); + } + + await using server = Bun.serve({ + port: 0, + async fetch(req) { + const pathname = new URL(req.url).pathname; + if (!pathname.startsWith("/repo.git/")) return new Response("not found", { status: 404 }); + const file = Bun.file(join(repo, ".git", ...pathname.slice("/repo.git/".length).split("/"))); + return (await file.exists()) ? new Response(file) : new Response("not found", { status: 404 }); + }, + }); + + const project = join(String(dir), "project"); + const depUrl = `git+http://localhost:${server.port}/repo.git`; + await Bun.write( + join(project, "package.json"), + JSON.stringify({ name: "test-patch-git", dependencies: { "pkg-to-patch": depUrl } }), + ); + + const env = { ...bunEnv, BUN_INSTALL_CACHE_DIR: join(String(dir), ".bun-cache") }; + + const patchKey = await expectPatchFlowWorks(project, env, "node_modules/pkg-to-patch"); + expect(patchKey).toStartWith(`pkg-to-patch@${depUrl}#`); + }); + + test("local tarball dependency", async () => { + await using dir = tempDir("patch-commit-tarball", { + "package.json": JSON.stringify({ + name: "test-patch-tarball", + dependencies: { "pkg-to-patch": "file:./dep.tgz" }, + }), + "tarball-src": { + "package": { + "package.json": JSON.stringify({ name: "pkg-to-patch", version: "1.0.0" }), + "index.js": `module.exports = "original";\n`, + }, + }, + }); + + await using tarProc = Bun.spawn({ + cmd: ["tar", "-czf", join(String(dir), "dep.tgz"), "-C", join(String(dir), "tarball-src"), "package"], + env: bunEnv, + stdout: "inherit", + stderr: "inherit", + }); + expect(await tarProc.exited).toBe(0); + + const env = { ...bunEnv, BUN_INSTALL_CACHE_DIR: join(String(dir), ".bun-cache") }; + + // name-only argument exercises the name-and-version lookup path + const patchKey = await expectPatchFlowWorks(String(dir), env, "pkg-to-patch"); + expect(patchKey).toBe("pkg-to-patch@./dep.tgz"); + }); +});