Skip to content

fix(glob): expand braces containing path separators before walking #25789

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ceIia wants to merge 1 commit into from
Closed

fix(glob): expand braces containing path separators before walking #25789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 125 additions & 9 deletions src/glob/GlobWalker.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,11 @@ pub fn GlobWalker_(
/// not owned by this struct
pattern: []const u8 = "",

/// expanded patterns when braces contain path separators
/// e.g., "{a/b,c/d}/**" -> ["a/b/**", "c/d/**"]
/// null if no expansion was needed
expanded_patterns: ?std.ArrayListUnmanaged([]const u8) = null,

/// If the pattern contains "./" or "../"
has_relative_components: bool = false,

Expand Down Expand Up @@ -337,12 +342,10 @@ pub fn GlobWalker_(
///
/// The only type of string impl we use is ZigString since
/// all matched paths are UTF-8 (DirIterator converts them on
/// windows) and allocated on the arnea
///
/// Multiple patterns are not supported so right now this is
/// only possible when running a pattern like:
/// windows) and allocated on the arena
///
/// `foo/**/*`
/// duplicates are automatically filtered (e.g. from brace expansion
/// or patterns like `foo/**/*`)
///
/// Use `.keys()` to get the matched paths
const MatchedMap = std.ArrayHashMapUnmanaged(BunString, void, struct {
Expand Down Expand Up @@ -1056,22 +1059,38 @@ pub fn GlobWalker_(
.end_byte_of_basename_excluding_special_syntax = 0,
};

// check if pattern needs brace expansion (has braces in path segments)
const effective_pattern = blk: {
if (needsBraceExpansionForWalk(pattern)) {
log("pattern needs brace expansion: {s}", .{pattern});
const expanded = Braces.expandBracesAlloc(pattern, arena.allocator());
if (expanded.items.len > 0) {
if (expanded.items.len > 1) {
this.expanded_patterns = expanded;
}
const first_expanded = expanded.items[0];
log("first expanded pattern: {s}", .{first_expanded});
// update pattern to expanded variant (iterator uses this.pattern)
this.pattern = first_expanded;
break :blk first_expanded;
}
}
break :blk pattern;
};

try GlobWalker.buildPatternComponents(
arena,
&this.patternComponents,
pattern,
effective_pattern,
&this.has_relative_components,
&this.end_byte_of_basename_excluding_special_syntax,
&this.basename_excluding_special_syntax_component_idx,
);

// copy arena after all allocations are successful
this.arena = arena.*;

if (bun.Environment.allow_assert) {
this.debugPatternComopnents();
}

return .success;
}

Expand All @@ -1095,6 +1114,54 @@ pub fn GlobWalker_(
pub fn walk(this: *GlobWalker) !Maybe(void) {
if (this.patternComponents.items.len == 0) return .success;

// walk the first pattern (set up in init)
switch (try this.walkSinglePattern()) {
.err => |err| return .{ .err = err },
.result => {},
}

// if we have expanded patterns, walk the remaining ones
const expanded = this.expanded_patterns orelse return .success;
if (expanded.items.len <= 1) return .success;

for (expanded.items[1..]) |pat| {
log("walking expanded pattern: {s}", .{pat});

// rebuild pattern components for this pattern
this.pattern = pat;
this.patternComponents.clearRetainingCapacity();
this.has_relative_components = false;
this.end_byte_of_basename_excluding_special_syntax = 0;
this.basename_excluding_special_syntax_component_idx = 0;

try GlobWalker.buildPatternComponents(
&this.arena,
&this.patternComponents,
pat,
&this.has_relative_components,
&this.end_byte_of_basename_excluding_special_syntax,
&this.basename_excluding_special_syntax_component_idx,
);

if (bun.Environment.allow_assert) {
this.debugPatternComopnents();
}

this.workbuf.clearRetainingCapacity();

switch (try this.walkSinglePattern()) {
.err => |err| return .{ .err = err },
.result => {},
}
}

return .success;
}

/// walks a single pattern (used internally by walk)
fn walkSinglePattern(this: *GlobWalker) !Maybe(void) {
if (this.patternComponents.items.len == 0) return .success;

var iter = GlobWalker.Iterator{ .walker = this };
defer iter.deinit();
switch (try iter.init()) {
Expand Down Expand Up @@ -1511,6 +1578,54 @@ pub fn GlobWalker_(
return component;
}

/// checks if brace alternation spans multiple path components (contains `/` inside braces).
/// multi-component alternation like `{a/b,c/d}` can't be resolved during traversal since
/// the walker needs to know which directories to descend into upfront.
/// single-component alternation like `{a,b}` is handled by normal pattern matching.
fn needsBraceExpansionForWalk(pattern: []const u8) bool {
var brace_depth: u32 = 0;
var in_brackets = false;
var i: usize = 0;

while (i < pattern.len) : (i += 1) {
const c = pattern[i];
switch (c) {
'\\' => {
if (comptime !isWindows) {
// skip escaped character on non-windows
i += 1;
} else {
// on windows, backslash is a path separator
if (brace_depth > 0 and !in_brackets) return true;
}
},
'[' => {
if (!in_brackets) in_brackets = true;
},
']' => {
in_brackets = false;
},
'{' => {
if (!in_brackets) {
brace_depth += 1;
}
},
'}' => {
if (!in_brackets and brace_depth > 0) {
brace_depth -= 1;
}
},
'/' => {
// path separator inside braces means we need expansion
if (brace_depth > 0 and !in_brackets) return true;
},
else => {},
}
}
// only expand if we saw a path separator inside braces
return false;
}
Comment on lines +1581 to +1627

@coderabbitai coderabbitai Bot Jan 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Logic is correct but escape handling could be clearer.

The function correctly detects whether brace patterns contain path separators and need expansion. The escape handling is correct but could be more explicit.

📝 Optional clarification for escape handling

Consider adding a comment to clarify the escape handling logic:

                 '\\' => {
                     if (comptime !isWindows) {
-                        // skip escaped character on non-windows
+                        // skip escaped character on non-windows (i += 1 here, then continue expression adds 1)
                         i += 1;
                     } else {
                         // on windows, backslash is a path separator
                         if (brace_depth > 0 and !in_brackets) return true;
                     }
                 },

This makes it clearer that the loop's continue expression (i += 1) will skip the escaped character.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/// checks if brace alternation spans multiple path components (contains `/` inside braces).
/// multi-component alternation like `{a/b,c/d}` can't be resolved during traversal since
/// the walker needs to know which directories to descend into upfront.
/// single-component alternation like `{a,b}` is handled by normal pattern matching.
fn needsBraceExpansionForWalk(pattern: []const u8) bool {
var brace_depth: u32 = 0;
var in_brackets = false;
var i: usize = 0;
while (i < pattern.len) : (i += 1) {
const c = pattern[i];
switch (c) {
'\\' => {
if (comptime !isWindows) {
// skip escaped character on non-windows
i += 1;
} else {
// on windows, backslash is a path separator
if (brace_depth > 0 and !in_brackets) return true;
}
},
'[' => {
if (!in_brackets) in_brackets = true;
},
']' => {
in_brackets = false;
},
'{' => {
if (!in_brackets) {
brace_depth += 1;
}
},
'}' => {
if (!in_brackets and brace_depth > 0) {
brace_depth -= 1;
}
},
'/' => {
// path separator inside braces means we need expansion
if (brace_depth > 0 and !in_brackets) return true;
},
else => {},
}
}
// only expand if we saw a path separator inside braces
return false;
}
/// checks if brace alternation spans multiple path components (contains `/` inside braces).
/// multi-component alternation like `{a/b,c/d}` can't be resolved during traversal since
/// the walker needs to know which directories to descend into upfront.
/// single-component alternation like `{a,b}` is handled by normal pattern matching.
fn needsBraceExpansionForWalk(pattern: []const u8) bool {
var brace_depth: u32 = 0;
var in_brackets = false;
var i: usize = 0;
while (i < pattern.len) : (i += 1) {
const c = pattern[i];
switch (c) {
'\\' => {
if (comptime !isWindows) {
// skip escaped character on non-windows (i += 1 here, then continue expression adds 1)
i += 1;
} else {
// on windows, backslash is a path separator
if (brace_depth > 0 and !in_brackets) return true;
}
},
'[' => {
if (!in_brackets) in_brackets = true;
},
']' => {
in_brackets = false;
},
'{' => {
if (!in_brackets) {
brace_depth += 1;
}
},
'}' => {
if (!in_brackets and brace_depth > 0) {
brace_depth -= 1;
}
},
'/' => {
// path separator inside braces means we need expansion
if (brace_depth > 0 and !in_brackets) return true;
},
else => {},
}
}
// only expand if we saw a path separator inside braces
return false;
}
🤖 Prompt for AI Agents 
In src/glob/GlobWalker.zig around lines 1581 to 1627, the escape handling in the
switch is correct but unclear: add a concise comment explaining that on
non-Windows the code intends to skip the character after a backslash by relying
on the loop's post-increment `(i += 1)`, and on Windows a backslash is treated
as a path separator so encountering one inside braces should force expansion;
update the backslash case with these two short explanatory comments so future
readers understand why we manually increment `i` on non-Windows and why we
return true on Windows when inside braces.


/// Build an ad-hoc glob pattern. Useful when you don't need to traverse
/// a directory.
pub fn buildPattern(
Expand Down Expand Up @@ -1685,6 +1800,7 @@ pub fn matchWildcardLiteral(literal: []const u8, path: []const u8) bool {

const DirIterator = @import("../bun.js/node/dir_iterator.zig");
const ResolvePath = @import("../resolver/resolve_path.zig");
const Braces = @import("../shell/braces.zig");

const bun = @import("bun");
const BunString = bun.String;
Expand Down
171 changes: 171 additions & 0 deletions test/js/bun/glob/scan.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -814,3 +814,174 @@ describe("glob.scan wildcard fast path", async () => {
);
});
});

// brace alternation without path separators (handled by normal matching)
describe("brace alternation (basic)", () => {
test("braces in first segment", async () => {
const tempdir = tempDirWithFiles("glob-brace-first", {
apps: {
"a.txt": "content",
},
packages: {
"b.txt": "content",
},
other: {
"c.txt": "content",
},
});

const glob = new Glob("{apps,packages}/**");
const entries = await Array.fromAsync(glob.scan({ cwd: tempdir, onlyFiles: false }));

expect(entries.length).toBeGreaterThan(0);
// should find both apps and packages directories/files
expect(entries.some(e => e.startsWith("apps"))).toBe(true);
expect(entries.some(e => e.startsWith("packages"))).toBe(true);
// should NOT find the "other" directory
expect(entries.some(e => e.startsWith("other"))).toBe(false);
});

test("braces in first segment (sync)", () => {
const tempdir = tempDirWithFiles("glob-brace-first-sync", {
apps: {
"a.txt": "content",
},
packages: {
"b.txt": "content",
},
other: {
"c.txt": "content",
},
});

const glob = new Glob("{apps,packages}/**");
const entries = Array.from(glob.scanSync({ cwd: tempdir, onlyFiles: false }));

expect(entries.length).toBeGreaterThan(0);
expect(entries.some(e => e.startsWith("apps"))).toBe(true);
expect(entries.some(e => e.startsWith("packages"))).toBe(true);
expect(entries.some(e => e.startsWith("other"))).toBe(false);
});

test("braces with only files option", async () => {
const tempdir = tempDirWithFiles("glob-brace-onlyfiles", {
src: {
"index.ts": "content",
lib: {
"helper.ts": "content",
},
},
test: {
"index.test.ts": "content",
},
});

const glob = new Glob("{src,test}/**/*.ts");
const entries = await Array.fromAsync(glob.scan({ cwd: tempdir, onlyFiles: true }));

expect(entries.length).toBe(3);
expect(entries.sort()).toEqual(
[`src${path.sep}index.ts`, `src${path.sep}lib${path.sep}helper.ts`, `test${path.sep}index.test.ts`].sort(),
);
});

test("single alternative in braces (edge case)", async () => {
const tempdir = tempDirWithFiles("glob-brace-single", {
apps: {
"a.txt": "content",
},
packages: {
"b.txt": "content",
},
});

// single alternative should still work
const glob = new Glob("{apps}/**");
const entries = await Array.fromAsync(glob.scan({ cwd: tempdir, onlyFiles: false }));

expect(entries.length).toBeGreaterThan(0);
expect(entries.some(e => e.startsWith("apps"))).toBe(true);
expect(entries.some(e => e.startsWith("packages"))).toBe(false);
});
});
Comment on lines +818 to +906

@coderabbitai coderabbitai Bot Jan 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider making these tests concurrent.

These tests create separate temporary directories and don't share state, so they can safely run concurrently. Using test.concurrent would improve test suite performance.

🔎 Proposed refactor to use concurrent tests 
 // brace alternation without path separators (handled by normal matching)
 describe("brace alternation (basic)", () => {
-  test("braces in first segment", async () => {
+  test.concurrent("braces in first segment", async () => {
     const tempdir = tempDirWithFiles("glob-brace-first", {
       apps: {
         "a.txt": "content",
       },
       packages: {
         "b.txt": "content",
       },
       other: {
         "c.txt": "content",
       },
     });

     const glob = new Glob("{apps,packages}/**");
     const entries = await Array.fromAsync(glob.scan({ cwd: tempdir, onlyFiles: false }));

     expect(entries.length).toBeGreaterThan(0);
     // should find both apps and packages directories/files
     expect(entries.some(e => e.startsWith("apps"))).toBe(true);
     expect(entries.some(e => e.startsWith("packages"))).toBe(true);
     // should NOT find the "other" directory
     expect(entries.some(e => e.startsWith("other"))).toBe(false);
   });

-  test("braces in first segment (sync)", () => {
+  test.concurrent("braces in first segment (sync)", () => {
     const tempdir = tempDirWithFiles("glob-brace-first-sync", {
       apps: {
         "a.txt": "content",
       },
       packages: {
         "b.txt": "content",
       },
       other: {
         "c.txt": "content",
       },
     });

     const glob = new Glob("{apps,packages}/**");
     const entries = Array.from(glob.scanSync({ cwd: tempdir, onlyFiles: false }));

     expect(entries.length).toBeGreaterThan(0);
     expect(entries.some(e => e.startsWith("apps"))).toBe(true);
     expect(entries.some(e => e.startsWith("packages"))).toBe(true);
     expect(entries.some(e => e.startsWith("other"))).toBe(false);
   });

-  test("braces with only files option", async () => {
+  test.concurrent("braces with only files option", async () => {
     const tempdir = tempDirWithFiles("glob-brace-onlyfiles", {
       src: {
         "index.ts": "content",
         lib: {
           "helper.ts": "content",
         },
       },
       test: {
         "index.test.ts": "content",
       },
     });

     const glob = new Glob("{src,test}/**/*.ts");
     const entries = await Array.fromAsync(glob.scan({ cwd: tempdir, onlyFiles: true }));

     expect(entries.length).toBe(3);
     expect(entries.sort()).toEqual(
       [`src${path.sep}index.ts`, `src${path.sep}lib${path.sep}helper.ts`, `test${path.sep}index.test.ts`].sort(),
     );
   });

-  test("single alternative in braces (edge case)", async () => {
+  test.concurrent("single alternative in braces (edge case)", async () => {
     const tempdir = tempDirWithFiles("glob-brace-single", {
       apps: {
         "a.txt": "content",
       },
       packages: {
         "b.txt": "content",
       },
     });

     // single alternative should still work
     const glob = new Glob("{apps}/**");
     const entries = await Array.fromAsync(glob.scan({ cwd: tempdir, onlyFiles: false }));

     expect(entries.length).toBeGreaterThan(0);
     expect(entries.some(e => e.startsWith("apps"))).toBe(true);
     expect(entries.some(e => e.startsWith("packages"))).toBe(false);
   });
 });

Based on coding guidelines: "Prefer concurrent tests over sequential tests using test.concurrent or describe.concurrent when multiple tests spawn processes or write files."

🤖 Prompt for AI Agents 
test/js/bun/glob/scan.test.ts lines 818-906: these tests are independent (each
creates its own temp dir) and should be run in parallel to speed up the suite;
change the block to run concurrently by either converting the describe to
describe.concurrent("brace alternation (basic)", ...) or prefixing each test
with test.concurrent instead of test, and keep async tests returning promises
as-is; verify the sync test remains safe to run concurrently (it only uses its
own temp dir) before committing.


// brace alternation with path separators inside braces (requires expansion)
describe("brace alternation with path separators", () => {
test("nested braces with path separators", async () => {
const tempdir = tempDirWithFiles("glob-brace-nested", {
apps: {
api: {
"config.yaml": "content",
},
other: {
"skip.txt": "content",
},
},
packages: {
utils: {
"index.ts": "content",
},
common: {
"helpers.ts": "content",
},
core: {
"types.ts": "content",
},
unrelated: {
"skip.txt": "content",
},
},
});

const glob = new Glob("{apps/api,packages/{utils,common,core}}/**");
const entries = await Array.fromAsync(glob.scan({ cwd: tempdir, onlyFiles: false }));

expect(entries.length).toBeGreaterThan(0);
// should find files in the specified paths
expect(entries.some(e => e.startsWith(`apps${path.sep}api`))).toBe(true);
expect(entries.some(e => e.startsWith(`packages${path.sep}utils`))).toBe(true);
expect(entries.some(e => e.startsWith(`packages${path.sep}common`))).toBe(true);
expect(entries.some(e => e.startsWith(`packages${path.sep}core`))).toBe(true);
// should NOT find files in unrelated paths
expect(entries.some(e => e.startsWith(`apps${path.sep}other`))).toBe(false);
expect(entries.some(e => e.startsWith(`packages${path.sep}unrelated`))).toBe(false);
});

test("nested braces with path separators (sync)", () => {
const tempdir = tempDirWithFiles("glob-brace-nested-sync", {
apps: {
api: {
"config.yaml": "content",
},
other: {
"skip.txt": "content",
},
},
packages: {
utils: {
"index.ts": "content",
},
common: {
"helpers.ts": "content",
},
core: {
"types.ts": "content",
},
unrelated: {
"skip.txt": "content",
},
},
});

const glob = new Glob("{apps/api,packages/{utils,common,core}}/**");
const entries = Array.from(glob.scanSync({ cwd: tempdir, onlyFiles: false }));

expect(entries.length).toBeGreaterThan(0);
expect(entries.some(e => e.startsWith(`apps${path.sep}api`))).toBe(true);
expect(entries.some(e => e.startsWith(`packages${path.sep}utils`))).toBe(true);
expect(entries.some(e => e.startsWith(`packages${path.sep}common`))).toBe(true);
expect(entries.some(e => e.startsWith(`packages${path.sep}core`))).toBe(true);
expect(entries.some(e => e.startsWith(`apps${path.sep}other`))).toBe(false);
expect(entries.some(e => e.startsWith(`packages${path.sep}unrelated`))).toBe(false);
});
});
Comment on lines +909 to +987

@coderabbitai coderabbitai Bot Jan 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider making these tests concurrent.

Similar to the previous test suite, these tests create separate temporary directories and can safely run concurrently.

🔎 Proposed refactor to use concurrent tests 
 // brace alternation with path separators inside braces (requires expansion)
 describe("brace alternation with path separators", () => {
-  test("nested braces with path separators", async () => {
+  test.concurrent("nested braces with path separators", async () => {
     const tempdir = tempDirWithFiles("glob-brace-nested", {
       apps: {
         api: {
           "config.yaml": "content",
         },
         other: {
           "skip.txt": "content",
         },
       },
       packages: {
         utils: {
           "index.ts": "content",
         },
         common: {
           "helpers.ts": "content",
         },
         core: {
           "types.ts": "content",
         },
         unrelated: {
           "skip.txt": "content",
         },
       },
     });

     const glob = new Glob("{apps/api,packages/{utils,common,core}}/**");
     const entries = await Array.fromAsync(glob.scan({ cwd: tempdir, onlyFiles: false }));

     expect(entries.length).toBeGreaterThan(0);
     // should find files in the specified paths
     expect(entries.some(e => e.startsWith(`apps${path.sep}api`))).toBe(true);
     expect(entries.some(e => e.startsWith(`packages${path.sep}utils`))).toBe(true);
     expect(entries.some(e => e.startsWith(`packages${path.sep}common`))).toBe(true);
     expect(entries.some(e => e.startsWith(`packages${path.sep}core`))).toBe(true);
     // should NOT find files in unrelated paths
     expect(entries.some(e => e.startsWith(`apps${path.sep}other`))).toBe(false);
     expect(entries.some(e => e.startsWith(`packages${path.sep}unrelated`))).toBe(false);
   });

-  test("nested braces with path separators (sync)", () => {
+  test.concurrent("nested braces with path separators (sync)", () => {
     const tempdir = tempDirWithFiles("glob-brace-nested-sync", {
       apps: {
         api: {
           "config.yaml": "content",
         },
         other: {
           "skip.txt": "content",
         },
       },
       packages: {
         utils: {
           "index.ts": "content",
         },
         common: {
           "helpers.ts": "content",
         },
         core: {
           "types.ts": "content",
         },
         unrelated: {
           "skip.txt": "content",
         },
       },
     });

     const glob = new Glob("{apps/api,packages/{utils,common,core}}/**");
     const entries = Array.from(glob.scanSync({ cwd: tempdir, onlyFiles: false }));

     expect(entries.length).toBeGreaterThan(0);
     expect(entries.some(e => e.startsWith(`apps${path.sep}api`))).toBe(true);
     expect(entries.some(e => e.startsWith(`packages${path.sep}utils`))).toBe(true);
     expect(entries.some(e => e.startsWith(`packages${path.sep}common`))).toBe(true);
     expect(entries.some(e => e.startsWith(`packages${path.sep}core`))).toBe(true);
     expect(entries.some(e => e.startsWith(`apps${path.sep}other`))).toBe(false);
     expect(entries.some(e => e.startsWith(`packages${path.sep}unrelated`))).toBe(false);
   });
 });

Based on coding guidelines: "Prefer concurrent tests over sequential tests using test.concurrent or describe.concurrent when multiple tests spawn processes or write files."

🤖 Prompt for AI Agents 
In test/js/bun/glob/scan.test.ts around lines 909 to 987, the two tests creating
temp directories run sequentially but are safe to run in parallel; change them
to concurrent tests by either converting the describe to describe.concurrent or
prefixing each test with test.concurrent (e.g., test.concurrent("nested
braces...", ...) and test.concurrent("nested braces... (sync)", ...)); ensure no
shared mutable state remains between tests (they already use independent temp
directories) and run the suite to verify no race conditions.