parse package-spec from CLI correctly (#1895)

* parse package-spec from CLI correctly

fixes #861

* copy separately for `UpdateRequest.name`

fix GItHub URL reporting in results

update tests to reflect latest code changes

* better fix for GitHub URL display issue

5 files changed, 203 insertions, 87 deletions

diff --git a/src/install/install.zig b/src/install/install.zig
index 940502ce8..fb955bc70 100644
--- a/src/install/install.zig
+++ b/src/install/install.zig
@@ -5269,85 +5269,77 @@ pub const PackageManager = struct {
             // add
             // remove
             outer: for (positionals) |positional| {
-                var request = UpdateRequest{
-                    .name = positional,
-                };
-                var unscoped_name = positional;
-                request.name = unscoped_name;
-
-                // request.name = "@package..." => unscoped_name = "package..."
-                if (unscoped_name.len > 0 and unscoped_name[0] == '@') {
-                    unscoped_name = unscoped_name[1..];
-                }
-
-                // if there is a semver in package name...
-                if (std.mem.indexOfScalar(u8, unscoped_name, '@')) |i| {
-                    // unscoped_name = "package@1.0.0" => request.name = "package"
-                    request.name = unscoped_name[0..i];
-
-                    // if package was scoped, put "@" back in request.name
-                    if (unscoped_name.ptr != positional.ptr) {
-                        request.name = positional[0 .. i + 1];
-                    }
-
-                    // unscoped_name = "package@1.0.0" => request.version_buf = "1.0.0"
-                    if (unscoped_name.len > i + 1) request.version_buf = unscoped_name[i + 1 ..];
+                var value = std.mem.trim(u8, positional, " \n\r\t");
+                switch (op) {
+                    .link, .unlink => if (!strings.hasPrefixComptime(value, "link:")) {
+                        value = std.fmt.allocPrint(allocator, "link:{s}", .{value}) catch unreachable;
+                    },
+                    else => {},
                 }
 
-                if (strings.hasPrefix("http://", request.name) or
-                    strings.hasPrefix("https://", request.name))
-                {
-                    if (Output.isEmojiEnabled()) {
-                        Output.prettyErrorln("<r>😢 <red>error<r><d>:<r> bun {s} http://url is not implemented yet.", .{
-                            @tagName(op),
-                        });
-                    } else {
-                        Output.prettyErrorln("<r><red>error<r><d>:<r> bun {s} http://url is not implemented yet.", .{
-                            @tagName(op),
-                        });
-                    }
-
+                const placeholder = String.from("@@@");
+                var version = Dependency.parseWithOptionalTag(
+                    allocator,
+                    placeholder,
+                    value,
+                    null,
+                    &SlicedString.init(value, value),
+                    log,
+                ) orelse switch (op) {
+                    .link, .unlink => null,
+                    else => brk: {
+                        value = std.fmt.allocPrint(allocator, "npm:{s}", .{value}) catch unreachable;
+                        break :brk Dependency.parseWithOptionalTag(
+                            allocator,
+                            placeholder,
+                            value,
+                            null,
+                            &SlicedString.init(value, value),
+                            log,
+                        );
+                    },
+                } orelse {
+                    Output.prettyErrorln("<r><red>error<r><d>:<r> unrecognised dependency format: {s}", .{
+                        positional,
+                    });
                     Global.exit(1);
+                };
+                switch (version.tag) {
+                    .dist_tag => if (version.value.dist_tag.name.eql(placeholder, value, value)) {
+                        value = std.fmt.allocPrint(allocator, "npm:{s}", .{value}) catch unreachable;
+                        version = Dependency.parseWithOptionalTag(
+                            allocator,
+                            placeholder,
+                            value,
+                            null,
+                            &SlicedString.init(value, value),
+                            log,
+                        ) orelse {
+                            Output.prettyErrorln("<r><red>error<r><d>:<r> unrecognised dependency format: {s}", .{
+                                positional,
+                            });
+                            Global.exit(1);
+                        };
+                    },
+                    else => {},
                 }
 
-                request.name = std.mem.trim(u8, request.name, "\n\r\t");
-                if (request.name.len == 0) continue;
-
+                var request = UpdateRequest{
+                    .name = allocator.dupe(u8, switch (version.tag) {
+                        .dist_tag => version.value.dist_tag.name,
+                        .github => version.value.github.repo,
+                        .npm => version.value.npm.name,
+                        .symlink => version.value.symlink,
+                        else => version.literal,
+                    }.slice(value)) catch unreachable,
+                    .version = version,
+                    .version_buf = value,
+                };
                 request.name_hash = String.Builder.stringHash(request.name);
+
                 for (update_requests.constSlice()) |*prev| {
                     if (prev.name_hash == request.name_hash and request.name.len == prev.name.len) continue :outer;
                 }
-
-                request.version_buf = std.mem.trim(u8, request.version_buf, "\n\r\t");
-
-                // https://github.com/npm/npm-package-arg/blob/fbaf2fd0b72a0f38e7c24260fd4504f4724c9466/npa.js#L330
-                if (strings.hasPrefix("https://", request.version_buf) or
-                    strings.hasPrefix("http://", request.version_buf))
-                {
-                    if (Output.isEmojiEnabled()) {
-                        Output.prettyErrorln("<r>😢 <red>error<r><d>:<r> bun {s} http://url is not implemented yet.", .{
-                            @tagName(op),
-                        });
-                    } else {
-                        Output.prettyErrorln("<r><red>error<r><d>:<r> bun {s} http://url is not implemented yet.", .{
-                            @tagName(op),
-                        });
-                    }
-
-                    Global.exit(1);
-                }
-
-                if ((op == .link or op == .unlink) and !strings.hasPrefixComptime(request.version_buf, "link:")) {
-                    request.version_buf = std.fmt.allocPrint(allocator, "link:{s}", .{request.name}) catch unreachable;
-                }
-
-                if (request.version_buf.len == 0) {
-                    request.missing_version = true;
-                } else {
-                    const sliced = SlicedString.init(request.version_buf, request.version_buf);
-                    request.version = Dependency.parse(allocator, String.init(request.name, request.name), request.version_buf, &sliced, log) orelse Dependency.Version{};
-                }
-
                 update_requests.append(request) catch break;
             }
 
@@ -5881,14 +5873,7 @@ pub const PackageManager = struct {
             const buf = this.lockfile.buffers.string_bytes.items;
 
             const destination_dir_subpath: [:0]u8 = brk: {
-                var alias = name;
-                if (this.lockfile.alias_map.get(package_id)) |str| {
-                    alias = str.slice(buf);
-                    std.mem.copy(u8, &this.destination_dir_subpath_buf, alias);
-                    this.destination_dir_subpath_buf[alias.len] = 0;
-                    break :brk this.destination_dir_subpath_buf[0..alias.len :0];
-                }
-
+                const alias = if (this.lockfile.alias_map.get(package_id)) |str| str.slice(buf) else name;
                 std.mem.copy(u8, &this.destination_dir_subpath_buf, alias);
                 this.destination_dir_subpath_buf[alias.len] = 0;
                 break :brk this.destination_dir_subpath_buf[0..alias.len :0];
diff --git a/src/install/repository.zig b/src/install/repository.zig
index e4f775b47..d1d494059 100644
--- a/src/install/repository.zig
+++ b/src/install/repository.zig
@@ -72,7 +72,11 @@ pub const Repository = extern struct {
 
             if (!formatter.repository.resolved.isEmpty()) {
                 try writer.writeAll("#");
-                try writer.writeAll(formatter.repository.resolved.slice(formatter.buf));
+                var resolved = formatter.repository.resolved.slice(formatter.buf);
+                if (std.mem.lastIndexOfScalar(u8, resolved, '-')) |i| {
+                    resolved = resolved[i + 1 ..];
+                }
+                try writer.writeAll(resolved);
             } else if (!formatter.repository.committish.isEmpty()) {
                 try writer.writeAll("#");
                 try writer.writeAll(formatter.repository.committish.slice(formatter.buf));
diff --git a/test/bun.js/install/bun-add.test.ts b/test/bun.js/install/bun-add.test.ts
new file mode 100644
index 000000000..2061ce5ec
--- /dev/null
+++ b/test/bun.js/install/bun-add.test.ts
@@ -0,0 +1,118 @@
+import { spawn } from "bun";
+import {
+  afterEach,
+  beforeEach,
+  expect,
+  it,
+} from "bun:test";
+import { bunExe } from "bunExe";
+import { bunEnv as env } from "bunEnv";
+import { mkdtemp, rm, writeFile } from "fs/promises";
+import { basename, join, relative } from "path";
+import { tmpdir } from "os";
+
+let package_dir, add_dir;
+
+beforeEach(async () => {
+  add_dir = await mkdtemp(join(tmpdir(), "bun-add.test"));
+  package_dir = await mkdtemp(join(tmpdir(), "bun-add.pkg"));
+});
+afterEach(async () => {
+  await rm(add_dir, { force: true, recursive: true });
+  await rm(package_dir, { force: true, recursive: true });
+});
+
+it("should add existing package", async () => {
+  await writeFile(join(add_dir, "package.json"), JSON.stringify({
+    name: "foo",
+    version: "0.0.1",
+  }));
+  await writeFile(join(package_dir, "package.json"), JSON.stringify({
+    name: "bar",
+    version: "0.0.2",
+  }));
+  const add_path = relative(package_dir, add_dir);
+  const { stdout, stderr, exited } = spawn({
+    cmd: [bunExe(), "add",`file:${add_path}`],
+    cwd: package_dir,
+    stdout: null,
+    stdin: "pipe",
+    stderr: "pipe",
+    env,
+  });
+  expect(stderr).toBeDefined();
+  const err = await new Response(stderr).text();
+  expect(err.replace(/^(.*?) v[^\n]+/, "$1").split(/\r?\n/)).toEqual([
+    "bun add",
+    " Saved lockfile",
+    "",
+  ]);
+  expect(stdout).toBeDefined();
+  const out = await new Response(stdout).text();
+  expect(out.replace(/\s*\[[0-9\.]+ms\]\s*$/, "").split(/\r?\n/)).toEqual([
+    ` + foo@${add_path}`,
+    "",
+    "",
+    "",
+    " 1 packages installed",
+  ]);
+  expect(await exited).toBe(0);
+});
+
+it("should reject missing package", async () => {
+  await writeFile(join(package_dir, "package.json"), JSON.stringify({
+    name: "bar",
+    version: "0.0.2",
+  }));
+  const add_path = relative(package_dir, add_dir);
+  const { stdout, stderr, exited } = spawn({
+    cmd: [bunExe(), "add",`file:${add_path}`],
+    cwd: package_dir,
+    stdout: null,
+    stdin: "pipe",
+    stderr: "pipe",
+    env,
+  });
+  expect(stderr).toBeDefined();
+  const err = await new Response(stderr).text();
+  expect(err.replace(/^(.*?) v[^\n]+/, "$1").split(/\r?\n/)).toEqual([
+    "bun add",
+    `error: file:${add_path}@file:${add_path} failed to resolve`,
+    "",
+  ]);
+  expect(stdout).toBeDefined();
+  const out = await new Response(stdout).text();
+  expect(out).toBe("");
+  expect(await exited).toBe(1);
+});
+
+it("should reject invalid path without segfault", async () => {
+  await writeFile(join(add_dir, "package.json"), JSON.stringify({
+    name: "foo",
+    version: "0.0.1",
+  }));
+  await writeFile(join(package_dir, "package.json"), JSON.stringify({
+    name: "bar",
+    version: "0.0.2",
+  }));
+  const add_path = relative(package_dir, add_dir);
+  const { stdout, stderr, exited } = spawn({
+    cmd: [bunExe(), "add",`file://${add_path}`],
+    cwd: package_dir,
+    stdout: null,
+    stdin: "pipe",
+    stderr: "pipe",
+    env,
+  });
+  expect(stderr).toBeDefined();
+  const err = await new Response(stderr).text();
+  expect(err.replace(/^(.*?) v[^\n]+/, "$1").split(/\r?\n/)).toEqual([
+    "bun add",
+    `error: file://${add_path}@file://${add_path} failed to resolve`,
+    "",
+  ]);
+  expect(stdout).toBeDefined();
+  const out = await new Response(stdout).text();
+  expect(out).toBe("");
+  expect(await exited).toBe(1);
+});
diff --git a/test/bun.js/install/bun-install.test.ts b/test/bun.js/install/bun-install.test.ts
index cdad571fc..c183ca6bf 100644
--- a/test/bun.js/install/bun-install.test.ts
+++ b/test/bun.js/install/bun-install.test.ts
@@ -1218,8 +1218,10 @@ it("should handle GitHub URL in dependencies (user/repo)", async () => {
   const err = await new Response(stderr).text();
   expect(err).toContain("Saved lockfile");
   expect(stdout).toBeDefined();
-  const out = await new Response(stdout).text();
-  expect(out.replace(/\s*\[[0-9\.]+ms\]\s*$/, "").split(/\r?\n/)).toEqual([
+  let out = await new Response(stdout).text();
+  out = out.replace(/\s*\[[0-9\.]+ms\]\s*$/, "");
+  out = out.replace(/(github:[^#]+)#[a-f0-9]+/, "$1");
+  expect(out.split(/\r?\n/)).toEqual([
     " + uglify@github:mishoo/UglifyJS",
     "",
     " 1 packages installed",
@@ -1236,6 +1238,7 @@ it("should handle GitHub URL in dependencies (user/repo)", async () => {
     "uglifyjs",
   ]);
   expect(await readdirSorted(join(package_dir, "node_modules", "uglify"))).toEqual([
+    ".bun-tag",
     ".gitattributes",
     ".github",
     ".gitignore",
@@ -1306,6 +1309,7 @@ it("should handle GitHub URL in dependencies (user/repo#commit-id)", async () =>
     join(package_dir, "node_modules", ".cache", "@GH@mishoo-UglifyJS-e219a9a"),
   );
   expect(await readdirSorted(join(package_dir, "node_modules", "uglify"))).toEqual([
+    ".bun-tag",
     ".gitattributes",
     ".github",
     ".gitignore",
@@ -1351,7 +1355,7 @@ it("should handle GitHub URL in dependencies (user/repo#tag)", async () => {
   expect(stdout).toBeDefined();
   const out = await new Response(stdout).text();
   expect(out.replace(/\s*\[[0-9\.]+ms\]\s*$/, "").split(/\r?\n/)).toEqual([
-    " + uglify@github:mishoo/UglifyJS#v3.14.1",
+    " + uglify@github:mishoo/UglifyJS#e219a9a",
     "",
     " 1 packages installed",
   ]);
@@ -1377,6 +1381,7 @@ it("should handle GitHub URL in dependencies (user/repo#tag)", async () => {
     join(package_dir, "node_modules", ".cache", "@GH@mishoo-UglifyJS-e219a9a"),
   );
   expect(await readdirSorted(join(package_dir, "node_modules", "uglify"))).toEqual([
+    ".bun-tag",
     ".gitattributes",
     ".github",
     ".gitignore",
@@ -1422,7 +1427,7 @@ it("should handle GitHub URL in dependencies (github:user/repo#tag)", async () =
   expect(stdout).toBeDefined();
   const out = await new Response(stdout).text();
   expect(out.replace(/\s*\[[0-9\.]+ms\]\s*$/, "").split(/\r?\n/)).toEqual([
-    " + uglify@github:mishoo/UglifyJS#v3.14.1",
+    " + uglify@github:mishoo/UglifyJS#e219a9a",
     "",
     " 1 packages installed",
   ]);
@@ -1448,6 +1453,7 @@ it("should handle GitHub URL in dependencies (github:user/repo#tag)", async () =
     join(package_dir, "node_modules", ".cache", "@GH@mishoo-UglifyJS-e219a9a"),
   );
   expect(await readdirSorted(join(package_dir, "node_modules", "uglify"))).toEqual([
+    ".bun-tag",
     ".gitattributes",
     ".github",
     ".gitignore",
@@ -1491,8 +1497,10 @@ it("should handle GitHub URL in dependencies (https://github.com/user/repo.git)"
   const err = await new Response(stderr).text();
   expect(err).toContain("Saved lockfile");
   expect(stdout).toBeDefined();
-  const out = await new Response(stdout).text();
-  expect(out.replace(/\s*\[[0-9\.]+ms\]\s*$/, "").split(/\r?\n/)).toEqual([
+  let out = await new Response(stdout).text();
+  out = out.replace(/\s*\[[0-9\.]+ms\]\s*$/, "");
+  out = out.replace(/(github:[^#]+)#[a-f0-9]+/, "$1");
+  expect(out.split(/\r?\n/)).toEqual([
     " + uglify@github:mishoo/UglifyJS",
     "",
     " 1 packages installed",
@@ -1509,6 +1517,7 @@ it("should handle GitHub URL in dependencies (https://github.com/user/repo.git)"
     "uglifyjs",
   ]);
   expect(await readdirSorted(join(package_dir, "node_modules", "uglify"))).toEqual([
+    ".bun-tag",
     ".gitattributes",
     ".github",
     ".gitignore",
diff --git a/test/bun.js/install/bun-link.test.ts b/test/bun.js/install/bun-link.test.ts
index 32d7b91fc..a3ef9eeb7 100644
--- a/test/bun.js/install/bun-link.test.ts
+++ b/test/bun.js/install/bun-link.test.ts
@@ -1,4 +1,4 @@
-import { file, spawn } from "bun";
+import { spawn } from "bun";
 import {
   afterEach,
   beforeEach,
@@ -7,7 +7,7 @@ import {
 } from "bun:test";
 import { bunExe } from "bunExe";
 import { bunEnv as env } from "bunEnv";
-import { access, mkdir, mkdtemp, readdir, readlink, rm, writeFile } from "fs/promises";
+import { mkdtemp, rm, writeFile } from "fs/promises";
 import { basename, join } from "path";
 import { tmpdir } from "os";