wip

src/install/install.zig

@@ -10161,6 +10161,16 @@ pub const PackageManager = struct {
         }
     }
 
+    fn nodeModulesFolderForDependencyIDs(iterator: *Lockfile.Tree.Iterator, ids: []const IdPair) !?Lockfile.Tree.NodeModulesFolder {
+        while (iterator.nextNodeModulesFolder(null)) |node_modules| {
+            for (ids) |id| {
+                _ = std.mem.indexOfScalar(DependencyID, node_modules.dependencies, id[0]) orelse continue;
+                return node_modules;
+            }
+        }
+        return null;
+    }
+
     fn nodeModulesFolderForDependencyID(iterator: *Lockfile.Tree.Iterator, dependency_id: DependencyID) !?Lockfile.Tree.NodeModulesFolder {
         while (iterator.nextNodeModulesFolder(null)) |node_modules| {
             _ = std.mem.indexOfScalar(DependencyID, node_modules.dependencies, dependency_id) orelse continue;
@@ -10170,65 +10180,154 @@ pub const PackageManager = struct {
         return null;
     }
 
-    fn pkgDepIdForNameAndVersion(
+    const IdPair = struct { DependencyID, PackageID };
+
+    fn pkgInfoForNameAndVersion(
         lockfile: *Lockfile,
+        iterator: *Lockfile.Tree.Iterator,
         pkg_maybe_version_to_patch: []const u8,
         name: []const u8,
         version: ?[]const u8,
-    ) struct { PackageID, DependencyID } {
+    ) struct { PackageID, Lockfile.Tree.NodeModulesFolder } {
+        var sfb = std.heap.stackFallback(@sizeOf(IdPair) * 4, lockfile.allocator);
+        var pairs = std.ArrayList(IdPair).initCapacity(sfb.get(), 8) catch bun.outOfMemory();
+        defer pairs.deinit();
+
         const name_hash = String.Builder.stringHash(name);
 
         const strbuf = lockfile.buffers.string_bytes.items;
 
-        const dependency_id: DependencyID, const pkg_id: PackageID = brk: {
-            var buf: [1024]u8 = undefined;
-            const dependencies = lockfile.buffers.dependencies.items;
+        var buf: [1024]u8 = undefined;
+        const dependencies = lockfile.buffers.dependencies.items;
 
-            var matches_found: u32 = 0;
-            var maybe_first_match: ?struct { DependencyID, PackageID } = null;
-            for (dependencies, 0..) |dep, dep_id| {
-                if (dep.name_hash != name_hash) continue;
-                matches_found += 1;
-                const pkg_id = lockfile.buffers.resolutions.items[dep_id];
-                if (pkg_id == invalid_package_id) continue;
-                const pkg = lockfile.packages.get(pkg_id);
-                if (version) |v| {
-                    const label = std.fmt.bufPrint(buf[0..], "{}", .{pkg.resolution.fmt(strbuf, .posix)}) catch @panic("Resolution name too long");
-                    if (std.mem.eql(u8, label, v)) break :brk .{ @intCast(dep_id), pkg_id };
-                } else maybe_first_match = .{ @intCast(dep_id), pkg_id };
+        for (dependencies, 0..) |dep, dep_id| {
+            if (dep.name_hash != name_hash) continue;
+            const pkg_id = lockfile.buffers.resolutions.items[dep_id];
+            if (pkg_id == invalid_package_id) continue;
+            const pkg = lockfile.packages.get(pkg_id);
+            if (version) |v| {
+                const label = std.fmt.bufPrint(buf[0..], "{}", .{pkg.resolution.fmt(strbuf, .posix)}) catch @panic("Resolution name too long");
+                if (std.mem.eql(u8, label, v)) {
+                    pairs.append(.{ @intCast(dep_id), pkg_id }) catch bun.outOfMemory();
+                }
+            } else {
+                pairs.append(.{ @intCast(dep_id), pkg_id }) catch bun.outOfMemory();
+            }
+        }
+
+        if (pairs.items.len == 0) {
+            Output.prettyErrorln("\n<r><red>error<r>: package <b>{s}<r> not found<r>", .{pkg_maybe_version_to_patch});
+            Global.crash();
+            return;
+        }
+
+        // user supplied a version e.g. `is-even@1.0.0`
+        if (version != null) {
+            if (pairs.items.len == 1) {
+                const dep_id, const pkg_id = pairs.items[0];
+                const folder = (try nodeModulesFolderForDependencyID(iterator, dep_id)) orelse {
+                    Output.prettyError(
+                        "<r><red>error<r>: could not find the folder for <b>{s}<r> in node_modules<r>\n<r>",
+                        .{pkg_maybe_version_to_patch},
+                    );
+                    Global.crash();
+                };
+                return .{
+                    pkg_id,
+                    folder,
+                };
             }
 
-            const first_match = maybe_first_match orelse {
-                Output.prettyErrorln("\n<r><red>error<r>: package <b>{s}<r> not found<r>", .{pkg_maybe_version_to_patch});
+            // we found multiple dependents of the supplied pkg + version
+            // the final package in the node_modules might be hoisted
+            // so we are going to try looking for each dep id in node_modules
+            _, const pkg_id = pairs.items[0];
+            const folder = (try nodeModulesFolderForDependencyIDs(iterator, pairs.items)) orelse {
+                Output.prettyError(
+                    "<r><red>error<r>: could not find the folder for <b>{s}<r> in node_modules<r>\n<r>",
+                    .{pkg_maybe_version_to_patch},
+                );
                 Global.crash();
-                return;
             };
 
-            if (matches_found > 1) {
-                Output.prettyErrorln(
-                    "\n<r><red>error<r>: Found multiple versions of <b>{s}<r>, please specify a precise version from the following list:<r>\n",
-                    .{name},
+            return .{
+                pkg_id,
+                folder,
+            };
+        }
+
+        // Otherwise the user did not supply a version, just the pkg name
+
+        // Only one match, let's use it
+        if (pairs.items.len == 1) {
+            const dep_id, const pkg_id = pairs.items[0];
+            const folder = (try nodeModulesFolderForDependencyID(iterator, dep_id)) orelse {
+                Output.prettyError(
+                    "<r><red>error<r>: could not find the folder for <b>{s}<r> in node_modules<r>\n<r>",
+                    .{pkg_maybe_version_to_patch},
                 );
-                var i: usize = 0;
-                const pkg_hashes = lockfile.packages.items(.name_hash);
-                while (i < lockfile.packages.len) {
-                    if (std.mem.indexOfScalar(u64, pkg_hashes[i..], name_hash)) |idx| {
-                        defer i += idx + 1;
-                        const pkg_id = i + idx;
-                        const pkg = lockfile.packages.get(pkg_id);
-                        if (!std.mem.eql(u8, pkg.name.slice(strbuf), name)) continue;
-
-                        Output.prettyError("  {s}@<blue>{}<r>\n", .{ pkg.name.slice(strbuf), pkg.resolution.fmt(strbuf, .posix) });
-                    } else break;
-                }
                 Global.crash();
-                return;
-            }
+            };
+            return .{
+                pkg_id,
+                folder,
+            };
+        }
 
-            break :brk .{ first_match[0], first_match[1] };
+        // Otherwise we have multiple matches
+        //
+        // There are two cases:
+        // a) the multiple matches are all the same underlying package (this happens because there could be multiple dependents of the same package)
+        // b) the matches are actually different packages, we'll prompt the user to select which one
+
+        _, const pkg_id = pairs.items[0];
+        const count = count: {
+            var count: u32 = 0;
+            for (pairs.items) |pair| {
+                if (pair[1] == pkg_id) count += 1;
+            }
+            break :count count;
         };
 
-        return .{ pkg_id, dependency_id };
+        // Disambiguate case a) from b)
+        if (count == pairs.items.len) {
+            // It may be hoisted, so we'll try the first one that matches
+            const folder = (try nodeModulesFolderForDependencyIDs(iterator, pairs.items)) orelse {
+                Output.prettyError(
+                    "<r><red>error<r>: could not find the folder for <b>{s}<r> in node_modules<r>\n<r>",
+                    .{pkg_maybe_version_to_patch},
+                );
+                Global.crash();
+            };
+            return .{
+                pkg_id,
+                folder,
+            };
+        }
+
+        Output.prettyErrorln(
+            "\n<r><red>error<r>: Found multiple versions of <b>{s}<r>, please specify a precise version from the following list:<r>\n",
+            .{name},
+        );
+        var i: usize = 0;
+        while (i < pairs.items.len) : (i += 1) {
+            _, const pkgid = pairs.items[i];
+            if (pkgid == invalid_package_id)
+                continue;
+
+            const pkg = lockfile.packages.get(pkgid);
+
+            Output.prettyError("  {s}@<blue>{}<r>\n", .{ pkg.name.slice(strbuf), pkg.resolution.fmt(strbuf, .posix) });
+
+            if (i + 1 < pairs.items.len) {
+                for (pairs.items[i + 1 ..]) |*p| {
+                    if (p[1] == pkgid) {
+                        p[1] = invalid_package_id;
+                    }
+                }
+            }
+        }
+        Global.crash();
     }
 
     const PatchArgKind = enum {
@@ -10384,17 +10483,7 @@ pub const PackageManager = struct {
             .name_and_version => brk: {
                 const pkg_maybe_version_to_patch = argument;
                 const name, const version = Dependency.splitNameAndVersion(pkg_maybe_version_to_patch);
-                const result = pkgDepIdForNameAndVersion(manager.lockfile, pkg_maybe_version_to_patch, name, version);
-                const pkg_id = result[0];
-                const dependency_id = result[1];
-
-                const folder = (try nodeModulesFolderForDependencyID(&iterator, dependency_id)) orelse {
-                    Output.prettyError(
-                        "<r><red>error<r>: could not find the folder for <b>{s}<r> in node_modules<r>\n<r>",
-                        .{pkg_maybe_version_to_patch},
-                    );
-                    Global.crash();
-                };
+                const pkg_id, const folder = pkgInfoForNameAndVersion(manager.lockfile, &iterator, pkg_maybe_version_to_patch, name, version);
 
                 const pkg = manager.lockfile.packages.get(pkg_id);
                 const pkg_name = pkg.name.slice(strbuf);
@@ -10781,19 +10870,8 @@ pub const PackageManager = struct {
             },
             .name_and_version => brk: {
                 const name, const version = Dependency.splitNameAndVersion(argument);
-                const result = pkgDepIdForNameAndVersion(lockfile, argument, name, version);
-                const pkg_id: PackageID = result[0];
-                const dependency_id: DependencyID = result[1];
-                const node_modules = (try nodeModulesFolderForDependencyID(
-                    &iterator,
-                    dependency_id,
-                )) orelse {
-                    Output.prettyError(
-                        "<r><red>error<r>: could not find the folder for <b>{s}<r> in node_modules<r>\n<r>",
-                        .{argument},
-                    );
-                    Global.crash();
-                };
+                const pkg_id, const node_modules = pkgInfoForNameAndVersion(lockfile, &iterator, argument, name, version);
+
                 const changes_dir = bun.path.joinZBuf(pathbuf[0..], &[_][]const u8{
                     node_modules.relative_path,
                     name,
@@ -11070,13 +11148,7 @@ pub const PackageManager = struct {
         }
 
         @memcpy(resolution_buf[resolution_label.len .. resolution_label.len + ".patch".len], ".patch");
-        var patch_filename: []const u8 = resolution_buf[0 .. resolution_label.len + ".patch".len];
-        var deinit = false;
-        if (escapePatchFilename(manager.allocator, patch_filename)) |escaped| {
-            deinit = true;
-            patch_filename = escaped;
-        }
-        defer if (deinit) manager.allocator.free(patch_filename);
+        const patch_filename: []const u8 = resolution_buf[0 .. resolution_label.len + ".patch".len];
 
         const path_in_patches_dir = bun.path.joinZ(
             &[_][]const u8{
@@ -11088,7 +11160,7 @@ pub const PackageManager = struct {
 
         var nodefs = bun.JSC.Node.NodeFS{};
         const args = bun.JSC.Node.Arguments.Mkdir{
-            .path = .{ .string = bun.PathString.init(manager.options.patch_features.commit.patches_dir) },
+            .path = .{ .string = bun.PathString.init(bun.path.dirname(path_in_patches_dir, .posix)) },
         };
         if (nodefs.mkdirRecursive(args, .sync).asErr()) |e| {
             Output.prettyError(

test/cli/install/bun-patch.test.ts

@@ -13,6 +13,88 @@ beforeAll(() => {
 
 describe("bun patch <pkg>", async () => {
   describe("workspace interactions", async () => {
+    /**
+     * @repo/eslint-config and @repo/typescript-config both depend on @types/ws@8.5.4
+     * so it should be hoisted to the root node_modules
+     */
+    describe("inside workspace with hoisting", async () => {
+      const args = [
+        ["node_modules/@types/ws", "packages/eslint-config/node_modules/@types/ws"],
+        ["@types/ws@8.5.4", "node_modules/@repo/eslint-config/node_modules/@types/ws"],
+      ];
+      for (const [arg, path] of args) {
+        test(arg, async () => {
+          const tempdir = tempDirWithFiles("lol", {
+            "package.json": JSON.stringify({
+              "name": "my-workspace",
+              private: "true",
+              version: "0.0.1",
+              "devDependencies": {
+                "@repo/ui": "*",
+                "@repo/eslint-config": "*",
+                "@repo/typescript-config": "*",
+                "@types/ws": "7.4.7",
+              },
+              workspaces: ["packages/*"],
+            }),
+            packages: {
+              "eslint-config": {
+                "package.json": JSON.stringify({
+                  name: "@repo/eslint-config",
+                  "version": "0.0.0",
+                  dependencies: {
+                    "@types/ws": "8.5.4",
+                  },
+                  private: "true",
+                }),
+              },
+              "typescript-config": {
+                "package.json": JSON.stringify({
+                  "name": "@repo/typescript-config",
+                  "version": "0.0.0",
+                  dependencies: {
+                    "@types/ws": "8.5.4",
+                  },
+                  private: "true",
+                }),
+              },
+              "ui": {
+                "package.json": JSON.stringify({
+                  name: "@repo/ui",
+                  version: "0.0.0",
+                  private: "true",
+                  devDependencies: {
+                    "@repo/eslint-config": "*",
+                    "@repo/typescript-config": "*",
+                  },
+                }),
+              },
+            },
+          });
+
+          console.log("TEMPDIR", tempdir);
+
+          await $`${bunExe()} i`.env(bunEnv).cwd(tempdir);
+
+          let result = await $`cd packages/eslint-config; ${bunExe()} patch ${arg}`.env(bunEnv).cwd(tempdir);
+          expect(result.stderr.toString()).not.toContain("error");
+          expect(result.stdout.toString()).toContain(
+            `To patch @types/ws, edit the following folder:\n\n  ${tempdir}/${path}\n`,
+          );
+
+          await $`echo LOL > ${path}/index.js`.env(bunEnv).cwd(tempdir);
+
+          expectNoError(await $`cd packages/eslint-config; ${bunExe()} patch --commit ${arg}`.env(bunEnv).cwd(tempdir));
+
+          expect(await $`cat ${path}/index.js`.env(bunEnv).cwd(tempdir).text()).toEqual("LOL\n");
+
+          expect(
+            (await $`cat package.json`.cwd(tempdir).env(bunEnv).json()).patchedDependencies["@types/ws@8.5.4"],
+          ).toEqual("patches/@types%2Fws@8.5.4.patch");
+        });
+      }
+    });
+
     describe("inside workspace package", async () => {
       const args = [
         ["node_modules/@types/ws", "packages/eslint-config/node_modules/@types/ws"],
@@ -581,10 +663,10 @@ module.exports = function isEven() {
         "index.ts": /* ts */ `import isEven from 'is-even'; console.log(isEven(420))`,
       });
 
-      await $`${bunExe()} run index.ts`
+      await $`${bunExe()} i`
         .env(bunEnv)
-        .cwd(filedir)
-        .then(o => expect(o.stderr.toString()).toBe(""));
+        .cwd(tempdir)
+        .then(o => expect(o.stderr.toString()).not.toContain("error"));
 
       const { stdout, stderr } = await $`${bunExe()} run index.ts`.env(bunEnv).cwd(tempdir);
       expect(stderr.toString()).toBe("");
@@ -638,10 +720,10 @@ module.exports = function isOdd() {
         "index.ts": /* ts */ `import isEven from 'is-even'; console.log(isEven(420))`,
       });
 
-      await $`${bunExe()} run index.ts`
+      await $`${bunExe()} i`
         .env(bunEnv)
-        .cwd(filedir)
-        .then(o => expect(o.stderr.toString()).toBe(""));
+        .cwd(tempdir)
+        .then(o => expect(o.stderr.toString()).not.toContain("error"));
 
       const { stdout, stderr } = await $`${bunExe()} run index.ts`.env(bunEnv).cwd(tempdir);
       expect(stderr.toString()).toBe("");