pack: fill every placeholder in multi-argument error messages

src/bun_core/output.rs

@@ -1797,7 +1797,7 @@ impl fmt::Display for PrettyBuf {
 /// Positional-argument bundle for runtime template substitution.
 pub trait FmtTuple {
     /// Write the `idx`-th positional into `f`. Returns `false` if `idx` is out
-    /// of range (caller emits the literal `{}` then).
+    /// of range.
     fn write_nth(&self, idx: usize, f: &mut dyn fmt::Write) -> Result<bool, fmt::Error>;
     fn len(&self) -> usize;
 }
@@ -1877,7 +1877,7 @@ impl_fmt_tuple!(0 A, 1 B, 2 C, 3 D, 4 E, 5 F, 6 G, 7 H);
 
 /// Substitute `{}` / `{s}` / `{d}` / `{any}` / `{f}` placeholders in `template`
 /// with successive entries from `args`. `{{` / `}}` are emitted as literal
-/// braces. Unrecognised specs are passed through verbatim.
+/// braces. The spec inside any other `{...}` is ignored.
 fn substitute_template(
     template: &[u8],
     args: &impl FmtTuple,
@@ -1901,7 +1901,14 @@ fn substitute_template(
             }
             if j < t.len() {
                 // consume placeholder
-                if args.write_nth(argi, f)? {
+                let filled = args.write_nth(argi, f)?;
+                debug_assert!(
+                    filled,
+                    "template has more placeholders than the {} arg(s) passed with it (a format_args! counts as one; pass a tuple): {:?}",
+                    args.len(),
+                    bstr::BStr::new(t),
+                );
+                if filled {
                     argi += 1;
                 }
                 i = j + 1;

src/runtime/cli/pack_command.rs

@@ -2713,7 +2713,7 @@ pub(crate) fn pack<const FOR_PUBLISH: bool>(
                     Output::err(
                         err,
                         "failed to stat file: \"{}\"",
-                        format_args!("{}", file.handle),
+                        format_args!("{}", bstr::BStr::new(item.path.as_bytes())),
                     );
                     Global::crash();
                 }
@@ -3000,8 +3000,7 @@ fn tarball_destination<'a>(
     if !pack_filename.is_empty() && !pack_destination.is_empty() {
         Output::err_generic(
             "cannot use both filename and destination at the same time with tarball: filename \"{}\" and destination \"{}\"",
-            format_args!(
-                "{} {}",
+            (
                 bstr::BStr::new(strings::without_trailing_slash(pack_filename)),
                 bstr::BStr::new(strings::without_trailing_slash(pack_destination)),
             ),
@@ -3045,13 +3044,12 @@ fn tarball_destination<'a>(
         if res.is_err() {
             Output::err_generic(
                 "archive destination name too long: \"{}/{}\"",
-                format_args!(
-                    "{}/{}",
+                (
                     bstr::BStr::new(strings::without_trailing_slash(&dest_buf[..dir_len_full])),
                     fmt_tarball_filename(
                         package_name,
                         package_version,
-                        TarballNameStyle::Normalize
+                        TarballNameStyle::Normalize,
                     ),
                 ),
             );
@@ -3654,8 +3652,7 @@ impl IgnorePatterns {
         Output::err(
             err,
             "failed to {} {} at: \"{}{}{}\"",
-            format_args!(
-                "{} {} {}{}{}",
+            (
                 <&str>::from(reason),
                 <&str>::from(ignore_kind),
                 bstr::BStr::new(strings::without_trailing_slash(dir_path)),

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

@@ -2,7 +2,7 @@ import { file, spawn, write } from "bun";
 import { readTarball } from "bun:internal-for-testing";
 import { beforeEach, describe, expect, test } from "bun:test";
 import { exists, mkdir, rm } from "fs/promises";
-import { bunEnv, bunExe, pack, runBunInstall, tempDir, tmpdirSync } from "harness";
+import { bunEnv, bunExe, isLinux, isWindows, pack, runBunInstall, tempDir, tmpdirSync } from "harness";
 import fs from "node:fs/promises";
 import { join } from "path";
 
@@ -331,6 +331,31 @@ describe("flags", () => {
     });
   }
 
+  // The destination directory ends `headroom` bytes short of the path buffer, so it still fits,
+  // but the longer "/<name>-<version>.tgz" appended to it does not. Windows command lines cannot
+  // carry an argument anywhere near its path buffer size.
+  test.skipIf(isWindows)("--destination with no room left for the tarball name", async () => {
+    const pathMax = isLinux ? 4096 : 1024;
+    const headroom = 100;
+    const name = `pack-dest-too-long-${Buffer.alloc(2 * headroom, "n").toString()}`;
+    await Promise.all([
+      write(
+        join(packageDir, "package.json"),
+        JSON.stringify({
+          name,
+          version: "1.0.0",
+        }),
+      ),
+      write(join(packageDir, "index.js"), "console.log('hello ./index.js')"),
+    ]);
+
+    // packageDir + "/" + filler is `headroom` bytes short of the buffer
+    const filler = Buffer.alloc(pathMax - headroom - Buffer.byteLength(packageDir) - 1, "d").toString();
+    const dest = join(packageDir, filler);
+    const { err } = await packExpectError(packageDir, bunEnv, `--destination=${dest}`);
+    expect(err).toContain(`error: archive destination name too long: "${dest}/${name}-1.0.0.tgz"\n`);
+  });
+
   const filenameTests = [
     {
       filename: "test.tgz",
@@ -394,7 +419,12 @@ describe("flags", () => {
       write(join(packageDir, "index.js"), "console.log('hello ./index.js')"),
     ]);
 
-    expect(async () => await pack(packageDir, bunEnv, "--filename=test.tgz", "--destination=packed")).toThrowError();
+    const { err } = await packExpectError(packageDir, bunEnv, "--filename=test.tgz", "--destination=packed");
+    expect(err).toContain(
+      'error: cannot use both filename and destination at the same time with tarball: filename "test.tgz" and destination "packed"',
+    );
+    expect(await exists(join(packageDir, "test.tgz"))).toBeFalse();
+    expect(await exists(join(packageDir, "packed"))).toBeFalse();
   });
 
   test("--ignore-scripts", async () => {
@@ -1405,6 +1435,26 @@ describe(".gitignore/.npmignore", () => {
     });
   }
 
+  for (const ignoreFile of [".gitignore", ".npmignore"]) {
+    test(`reports which ${ignoreFile} could not be read`, async () => {
+      await Promise.all([
+        write(
+          join(packageDir, "package.json"),
+          JSON.stringify({
+            name: "pack-ignore-unreadable",
+            version: "1.0.0",
+          }),
+        ),
+        write(join(packageDir, "subdir", "index.js"), "console.log('hello ./subdir/index.js')"),
+        // a directory where the ignore file is expected: opening it succeeds, reading it fails
+        mkdir(join(packageDir, "subdir", ignoreFile), { recursive: true }),
+      ]);
+
+      const { err } = await packExpectError(packageDir, bunEnv);
+      expect(err).toContain(`EISDIR: failed to read ${ignoreFile} at: "${join(packageDir, "subdir", ignoreFile)}"\n`);
+    });
+  }
+
   test("excludes files recursively", async () => {
     await Promise.all([
       write(