Make glob work for windows (#8382)

* Fix some related to paths and string encoding * Fix relative paths with glob * Fix scan tests * Fix glob scan test * [autofix.ci] apply automated fixes * Fix leak test * clean up post merge * [autofix.ci] apply automated fixes * clean up glob getcwd * remove old struct * fix open on posix * feat: Add win32 path.toNamespacedPath and align rest of node:path with Node (#8469) * restore zls file change * [autofix.ci] apply automated fixes * switch to using fs.top_level_dir in glob --------- Co-authored-by: Jarred Sumner <[email protected]> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Georgijs Vilums <[email protected]> Co-authored-by: Georgijs Vilums <[email protected]> Co-authored-by: John-David Dalton <[email protected]> Co-authored-by: Georgijs <[email protected]>
oven-sh · Feb 13, 2024 · aca9365 · aca9365
1 parent 6ba146c
commit aca9365
Show file tree

Hide file tree

Showing 6 changed files with 48 additions and 105 deletions.
diff --git a/src/bun.js/api/glob.zig b/src/bun.js/api/glob.zig
@@ -40,40 +40,7 @@ const ScanOpts = struct {
     error_on_broken_symlinks: bool,
 
     fn parseCWD(globalThis: *JSGlobalObject, allocator: std.mem.Allocator, cwdVal: JSC.JSValue, absolute: bool, comptime fnName: string) ?[]const u8 {
-        const cwd_str_raw = cwd_str_raw: {
-            // Windows wants utf-16
-            if (comptime bun.Environment.isWindows) {
-                const cwd_zig_str = cwdVal.getZigString(globalThis);
-                // Dupe if already utf-16
-                if (cwd_zig_str.is16Bit()) {
-                    const duped = allocator.dupe(u8, cwd_zig_str.slice()) catch {
-                        globalThis.throwOutOfMemory();
-                        return null;
-                    };
-
-                    break :cwd_str_raw ZigString.Slice.init(allocator, duped);
-                }
-
-                // Convert to utf-16
-                const utf16 = bun.strings.toUTF16AllocForReal(
-                    allocator,
-                    cwd_zig_str.slice(),
-                    // Let windows APIs handle errors with invalid surrogate pairs, etc.
-                    false,
-                    false,
-                ) catch {
-                    globalThis.throwOutOfMemory();
-                    return null;
-                };
-
-                const ptr: [*]u8 = @ptrCast(utf16.ptr);
-                break :cwd_str_raw ZigString.Slice.init(allocator, ptr[0 .. utf16.len * 2]);
-            }
-
-            // `.toSlice()` internally converts to WTF-8
-            break :cwd_str_raw cwdVal.toSlice(globalThis, allocator);
-        };
-
+        const cwd_str_raw = cwdVal.toSlice(globalThis, allocator);
         if (cwd_str_raw.len == 0) return "";
 
         const cwd_str = cwd_str: {

diff --git a/src/glob.zig b/src/glob.zig
@@ -111,6 +111,8 @@ const CursorState = struct {
     }
 };
 
+const log = bun.Output.scoped(.glob, false);
+
 pub const BunGlobWalker = GlobWalker_(null, false);
 
 fn dummyFilterTrue(val: []const u8) bool {
@@ -153,12 +155,13 @@ pub fn GlobWalker_(
 
         dot: bool = false,
         absolute: bool = false,
+
         cwd: []const u8 = "",
         follow_symlinks: bool = false,
         error_on_broken_symlinks: bool = false,
         only_files: bool = true,
 
-        pathBuf: [bun.MAX_PATH_BYTES]u8 = undefined,
+        pathBuf: bun.PathBuffer = undefined,
         // iteration state
         workbuf: ArrayList(WorkItem) = ArrayList(WorkItem){},
 
@@ -197,12 +200,13 @@ pub fn GlobWalker_(
             fds_open: if (bun.Environment.allow_assert) usize else u0 = 0,
 
             pub fn init(this: *Iterator) !Maybe(void) {
-                var path_buf: *[bun.MAX_PATH_BYTES]u8 = &this.walker.pathBuf;
+                const path_buf: *[bun.MAX_PATH_BYTES]u8 = &this.walker.pathBuf;
                 const root_path = this.walker.cwd;
                 @memcpy(path_buf[0..root_path.len], root_path[0..root_path.len]);
                 path_buf[root_path.len] = 0;
-                const cwd_fd = switch (Syscall.open(@ptrCast(path_buf[0 .. root_path.len + 1]), std.os.O.DIRECTORY | std.os.O.RDONLY, 0)) {
-                    .err => |err| return .{ .err = this.walker.handleSysErrWithPath(err, @ptrCast(path_buf[0 .. root_path.len + 1])) },
+                const root_path_z = path_buf[0..root_path.len :0];
+                const cwd_fd = switch (Syscall.open(root_path_z, std.os.O.DIRECTORY | std.os.O.RDONLY, 0)) {
+                    .err => |err| return .{ .err = this.walker.handleSysErrWithPath(err, root_path_z) },
                     .result => |fd| fd,
                 };
 
@@ -250,7 +254,7 @@ pub fn GlobWalker_(
             }
 
             pub fn closeDisallowingCwd(this: *Iterator, fd: bun.FileDescriptor) void {
-                if (fd == this.cwd_fd) return;
+                if (fd == this.cwd_fd or fd == bun.invalid_fd) return;
                 _ = Syscall.close(fd);
                 if (bun.Environment.allow_assert) this.fds_open -= 1;
             }
@@ -268,6 +272,7 @@ pub fn GlobWalker_(
                 work_item: WorkItem,
                 comptime root: bool,
             ) !Maybe(void) {
+                log("transition => {s}", .{work_item.path});
                 this.iter_state = .{ .directory = .{
                     .fd = .zero,
                     .iter = undefined,
@@ -303,6 +308,7 @@ pub fn GlobWalker_(
                 this.iter_state.directory.next_pattern = if (component_idx + 1 < this.walker.patternComponents.items.len) &this.walker.patternComponents.items[component_idx + 1] else null;
                 this.iter_state.directory.is_last = component_idx == this.walker.patternComponents.items.len - 1;
                 this.iter_state.directory.at_cwd = false;
+                this.iter_state.directory.fd = bun.invalid_fd;
 
                 const fd: bun.FileDescriptor = fd: {
                     if (work_item.fd) |fd| break :fd fd;
@@ -332,6 +338,8 @@ pub fn GlobWalker_(
                     };
                 };
 
+                // std.fs.cwd().iterate();
+
                 this.iter_state.directory.fd = fd;
                 const iterator = DirIterator.iterate(fd.asDir(), .u8);
                 this.iter_state.directory.iter = iterator;
@@ -450,6 +458,7 @@ pub fn GlobWalker_(
                                 this.iter_state = .get_next;
                                 continue;
                             };
+                            log("dir: {s} entry: {s}", .{ dir.dir_path, entry.name.slice() });
 
                             const dir_iter_state: *const IterState.Directory = &this.iter_state.directory;
 
@@ -614,22 +623,10 @@ pub fn GlobWalker_(
             only_files: bool,
         ) !Maybe(void) {
             errdefer arena.deinit();
-            var cwd: []const u8 = undefined;
-            switch (Syscall.getcwd(&this.pathBuf)) {
-                .err => |err| {
-                    return .{ .err = err };
-                },
-                .result => |result| {
-                    const copiedCwd = try arena.allocator().alloc(u8, result.len);
-                    @memcpy(copiedCwd, result);
-                    cwd = copiedCwd;
-                },
-            }
-
             return try this.initWithCwd(
                 arena,
                 pattern,
-                cwd,
+                bun.fs.FileSystem.instance.top_level_dir,
                 dot,
                 absolute,
                 follow_symlinks,
@@ -709,6 +706,7 @@ pub fn GlobWalker_(
                 .err => |err| return .{ .err = err },
                 .result => |matched_path| matched_path,
             }) |path| {
+                log("walker: matched path: {s}", .{path});
                 try this.matchedPaths.append(this.arena.allocator(), BunString.fromBytes(path));
             }
 
@@ -904,6 +902,7 @@ pub fn GlobWalker_(
             pattern_component: *Component,
             filepath: []const u8,
         ) bool {
+            log("matchPatternImpl: {s}", .{filepath});
             if (!this.dot and GlobWalker.startsWithDot(filepath)) return false;
             if (is_ignored(filepath)) return false;
 
@@ -1008,34 +1007,7 @@ pub fn GlobWalker_(
         }
 
         inline fn startsWithDot(filepath: []const u8) bool {
-            if (comptime isWindows) {
-                return filepath.len > 1 and filepath[1] == '.';
-            } else {
-                return filepath.len > 0 and filepath[0] == '.';
-            }
-        }
-
-        fn hasLeadingDot(filepath: []const u8, comptime allow_non_utf8: bool) bool {
-            if (comptime bun.Environment.isWindows and allow_non_utf8) {
-                // utf-16
-                if (filepath.len >= 4 and filepath[1] == '.' and filepath[3] == '/')
-                    return true;
-            } else {
-                if (filepath.len >= 2 and filepath[0] == '.' and filepath[1] == '/')
-                    return true;
-            }
-
-            return false;
-        }
-
-        /// NOTE This doesn't check that there is leading dot, use `hasLeadingDot()` to do that
-        fn removeLeadingDot(filepath: []const u8, comptime allow_non_utf8: bool) []const u8 {
-            if (comptime bun.Environment.allow_assert) std.debug.assert(hasLeadingDot(filepath, allow_non_utf8));
-            if (comptime bun.Environment.isWindows and allow_non_utf8) {
-                return filepath[4..];
-            } else {
-                return filepath[2..];
-            }
+            return filepath.len > 0 and filepath[0] == '.';
         }
 
         fn checkSpecialSyntax(pattern: []const u8) bool {

diff --git a/src/sys.zig b/src/sys.zig
@@ -1551,7 +1551,7 @@ pub fn getFdPath(fd: bun.FileDescriptor, out_buffer: *[MAX_PATH_BYTES]u8) Maybe(
     switch (comptime builtin.os.tag) {
         .windows => {
             var wide_buf: [windows.PATH_MAX_WIDE]u16 = undefined;
-            const wide_slice = std.os.windows.GetFinalPathNameByHandle(bun.fdcast(fd), .{}, wide_buf[0..]) catch {
+            const wide_slice = std.os.windows.GetFinalPathNameByHandle(fd.cast(), .{}, wide_buf[0..]) catch {
                 return Maybe([]u8){ .err = .{ .errno = @intFromEnum(bun.C.SystemErrno.EBADF), .syscall = .GetFinalPathNameByHandle } };
             };
 

diff --git a/test/js/bun/glob/leak.test.ts b/test/js/bun/glob/leak.test.ts
@@ -15,7 +15,7 @@ describe("leaks", () => {
         Bun.gc(true);
         (function () {
           const glob = new Bun.Glob("**/*");
-          Array.from(glob.scanSync({ cwd: '${cwd}' }));
+          Array.from(glob.scanSync({ cwd: '${escapeCwd(cwd)}' }));
         })();
         Bun.gc(true);
         const val = process.memoryUsage.rss();
@@ -41,7 +41,7 @@ describe("leaks", () => {
         Bun.gc(true);
         await (async function () {
           const glob = new Bun.Glob("**/*");
-          await Array.fromAsync(glob.scan({ cwd: '${cwd}' }));
+          await Array.fromAsync(glob.scan({ cwd: '${escapeCwd(cwd)}' }));
         })();
         Bun.gc(true);
         const val = process.memoryUsage.rss();
@@ -60,3 +60,8 @@ describe("leaks", () => {
     expect(exitCode).toBe(0);
   });
 });
+
+function escapeCwd(cwd: string): string {
+  if (process.platform == "win32") return cwd.replaceAll("\\", "\\\\");
+  return cwd;
+}
diff --git a/test/js/bun/glob/scan.test.ts b/test/js/bun/glob/scan.test.ts
@@ -25,7 +25,7 @@ import { Glob, GlobScanOptions } from "bun";
 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
 import fg from "fast-glob";
 import * as path from "path";
-import { tempFixturesDir, createTempDirectoryWithBrokenSymlinks } from "./util";
+import { tempFixturesDir, createTempDirectoryWithBrokenSymlinks, prepareEntries } from "./util";
 
 let origAggressiveGC = Bun.unsafe.gcAggressionLevel();
 let tempBrokenSymlinksDir: string;
@@ -63,7 +63,7 @@ describe("glob.match", async () => {
       async () => {
         const pattern = "**/node_modules/**/*.js";
         const glob = new Glob(pattern);
-        const filepaths = await Array.fromAsync(glob.scan(bunGlobOpts));
+        const filepaths = prepareEntries(await Array.fromAsync(glob.scan(bunGlobOpts)));
         const fgFilepths = await fg.glob(pattern, fgOpts);
 
         // console.error(filepaths);
@@ -83,7 +83,7 @@ describe("glob.match", async () => {
       async () => {
         const pattern = "**/*.js";
         const glob = new Glob(pattern);
-        const filepaths = await Array.fromAsync(glob.scan(bunGlobOpts));
+        const filepaths = prepareEntries(await Array.fromAsync(glob.scan(bunGlobOpts)));
         const fgFilepths = await fg.glob(pattern, fgOpts);
 
         expect(filepaths.length).toEqual(fgFilepths.length);
@@ -102,7 +102,7 @@ describe("glob.match", async () => {
       async () => {
         const pattern = "**/*.ts";
         const glob = new Glob(pattern);
-        const filepaths = await Array.fromAsync(glob.scan(bunGlobOpts));
+        const filepaths = prepareEntries(await Array.fromAsync(glob.scan(bunGlobOpts)));
         const fgFilepths = await fg.glob(pattern, fgOpts);
 
         expect(filepaths.length).toEqual(fgFilepths.length);
@@ -142,7 +142,7 @@ describe("glob.match", async () => {
     const cwd = import.meta.dir;
 
     const glob = new Glob(pattern);
-    const entries = await Array.fromAsync(glob.scan({ cwd }));
+    const entries = prepareEntries(await Array.fromAsync(glob.scan({ cwd })));
 
     expect(entries.sort()).toEqual(
       [
@@ -226,30 +226,25 @@ const regular = {
     { pattern: "*", cwd: "fixtures" },
     { pattern: "**", cwd: "fixtures" },
     { pattern: "**/*", cwd: "fixtures" },
-
     { pattern: "*/nested", cwd: "fixtures" },
     { pattern: "*/nested/*", cwd: "fixtures" },
     { pattern: "*/nested/**", cwd: "fixtures" },
     { pattern: "*/nested/**/*", cwd: "fixtures" },
     { pattern: "**/nested/*", cwd: "fixtures" },
     { pattern: "**/nested/**", cwd: "fixtures" },
     { pattern: "**/nested/**/*", cwd: "fixtures" },
-
     { pattern: "{first,second}", cwd: "fixtures" },
     { pattern: "{first,second}/*", cwd: "fixtures" },
     { pattern: "{first,second}/**", cwd: "fixtures" },
     { pattern: "{first,second}/**/*", cwd: "fixtures" },
-
     { pattern: "*/{first,second}/*", cwd: "fixtures" },
     { pattern: "*/{first,second}/*/{nested,file.md}", cwd: "fixtures" },
     { pattern: "**/{first,second}/**", cwd: "fixtures" },
     { pattern: "**/{first,second}/{nested,file.md}", cwd: "fixtures" },
     { pattern: "**/{first,second}/**/{nested,file.md}", cwd: "fixtures" },
-
     { pattern: "{first,second}/{nested,file.md}", cwd: "fixtures" },
     { pattern: "{first,second}/*/nested/*", cwd: "fixtures" },
     { pattern: "{first,second}/**/nested/**", cwd: "fixtures" },
-
     { pattern: "*/{nested,file.md}/*", cwd: "fixtures" },
     { pattern: "**/{nested,file.md}/*", cwd: "fixtures" },
   ],
@@ -258,11 +253,9 @@ const regular = {
     { pattern: "./*", cwd: "fixtures" },
     { pattern: "./**", cwd: "fixtures" },
     { pattern: "./**/*", cwd: "fixtures" },
-
     { pattern: "../*", cwd: "fixtures/first" },
     { pattern: "../**", cwd: "fixtures/first", issue: 47 },
     { pattern: "../../*", cwd: "fixtures/first/nested" },
-
     { pattern: "../{first,second}", cwd: "fixtures/first" },
     { pattern: "./../*", cwd: "fixtures/first" },
   ],
@@ -321,12 +314,12 @@ beforeAll(() => {
 describe("fast-glob e2e tests", async () => {
   const absoluteCwd = process.cwd();
   const cwd = import.meta.dir;
+  console.log("CWD IS", cwd);
 
   regular.regular.forEach(pattern =>
     test(`patterns regular ${pattern}`, () => {
       // let entries = fg.globSync(pattern, { cwd });
-      let entries = Array.from(new Glob(pattern).scanSync({ cwd, followSymlinks: true }));
-      entries = entries.sort();
+      const entries = prepareEntries(Array.from(new Glob(pattern).scanSync({ cwd, followSymlinks: true })));
       expect(entries).toMatchSnapshot(pattern);
     }),
   );
@@ -335,8 +328,7 @@ describe("fast-glob e2e tests", async () => {
     test(`patterns regular cwd ${pattern}`, () => {
       const testCwd = path.join(cwd, secondHalf);
       // let entries = fg.globSync(pattern, { cwd: testCwd });
-      let entries = Array.from(new Glob(pattern).scanSync({ cwd: testCwd, followSymlinks: true }));
-      entries = entries.sort();
+      let entries = prepareEntries(Array.from(new Glob(pattern).scanSync({ cwd: testCwd, followSymlinks: true })));
       expect(entries).toMatchSnapshot(pattern);
     }),
   );
@@ -345,8 +337,7 @@ describe("fast-glob e2e tests", async () => {
     test(`patterns regular relative cwd ${pattern}`, () => {
       const testCwd = secondHalf ? path.join(cwd, secondHalf) : cwd;
       // let entries = fg.globSync(pattern, { cwd: testCwd });
-      let entries = Array.from(new Glob(pattern).scanSync({ cwd: testCwd, followSymlinks: true }));
-      entries = entries.sort();
+      let entries = prepareEntries(Array.from(new Glob(pattern).scanSync({ cwd: testCwd, followSymlinks: true })));
       expect(entries).toMatchSnapshot(pattern);
     }),
   );
@@ -357,15 +348,17 @@ describe("fast-glob e2e tests", async () => {
       // let entries = fg.globSync(pattern, { cwd: testCwd, absolute: true });
       let entries = Array.from(new Glob(pattern).scanSync({ cwd: testCwd, followSymlinks: true, absolute: true }));
       entries = entries.sort().map(entry => entry.slice(absoluteCwd.length + 1));
+      entries = prepareEntries(entries);
       expect(entries).toMatchSnapshot(pattern);
     }),
   );
 
   onlyFilesPatterns.regular.forEach(pattern =>
     test(`only files ${pattern}`, () => {
       // let entries = fg.globSync(pattern, { cwd, absolute: false, onlyFiles: true });
-      let entries = Array.from(new Glob(pattern).scanSync({ cwd, followSymlinks: true, onlyFiles: true }));
-      entries = entries.sort();
+      let entries = prepareEntries(
+        Array.from(new Glob(pattern).scanSync({ cwd, followSymlinks: true, onlyFiles: true })),
+      );
       expect(entries).toMatchSnapshot(pattern);
     }),
   );
@@ -374,8 +367,9 @@ describe("fast-glob e2e tests", async () => {
     test(`only files (cwd) ${pattern}`, () => {
       const testCwd = secondHalf ? path.join(cwd, secondHalf) : cwd;
       // let entries = fg.globSync(pattern, { cwd: testCwd, absolute: false, onlyFiles: true });
-      let entries = Array.from(new Glob(pattern).scanSync({ cwd: testCwd, followSymlinks: true, onlyFiles: true }));
-      entries = entries.sort();
+      let entries = prepareEntries(
+        Array.from(new Glob(pattern).scanSync({ cwd: testCwd, followSymlinks: true, onlyFiles: true })),
+      );
       expect(entries).toMatchSnapshot(pattern);
     }),
   );