install: fix bun patch --commit cache path for git, github, and tarball dependencies

src/install/PackageManager/PackageManagerDirectories.rs

@@ -920,10 +920,15 @@ pub struct CacheDirAndSubpath<'a> {
 
 /// 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<u64>,
 ) -> CacheDirAndSubpath<'a> {
@@ -939,16 +944,35 @@ pub fn compute_cache_dir_and_subpath<'a>(
         }
         ResolutionTag::Git => {
             let git = resolution.git();
-            cache_dir_subpath = cached_git_folder_name(manager, git, patch_hash);
+            let resolved = match resolution_string_bytes {
+                Some(buf) => git.resolved.slice(buf),
+                None => manager.lockfile.str(&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 = match resolution_string_bytes {
+                Some(buf) => github.resolved.slice(buf),
+                None => manager.lockfile.str(&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 buf = match resolution_string_bytes {
+                Some(buf) => buf,
+                None => manager.lockfile.buffers.string_bytes.as_slice(),
+            };
             let folder = resolution.folder().slice(buf);
             // Handle when a package depends on itself via file:
             // example:
@@ -963,17 +987,30 @@ 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 tarball = resolution.local_tarball();
+            let url = match resolution_string_bytes {
+                Some(buf) => tarball.slice(buf),
+                None => manager.lockfile.str(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 tarball = resolution.remote_tarball();
+            let url = match resolution_string_bytes {
+                Some(buf) => tarball.slice(buf),
+                None => manager.lockfile.str(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 buf = match resolution_string_bytes {
+                Some(buf) => buf,
+                None => manager.lockfile.buffers.string_bytes.as_slice(),
+            };
             let folder = resolution.workspace().slice(buf);
             // Handle when a package depends on itself
             if folder.is_empty() || (folder.len() == 1 && folder[0] == b'.') {
@@ -991,10 +1028,13 @@ 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 = {
+                let buf = match resolution_string_bytes {
+                    Some(buf) => buf,
+                    None => manager.lockfile.buffers.string_bytes.as_slice(),
+                };
+                resolution.symlink().slice(buf).to_vec()
+            };
 
             if folder.is_empty() || (folder.len() == 1 && folder[0] == b'.') {
                 cache_dir_subpath = z_static(b".\0");

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<Box<[u8]>> {
         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<Box<[u8]>> {
                 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<Box<[u8]>> {
         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,
                 );

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),
         );

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<string, string | undefined>, ...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<string, string | undefined>, 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 `<owner>-<repo>-<committish>` 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");
+  });
+});