Open
Conversation
FrameData is a struct, so FrameData frame = Frame; (line 38 in Linear, line 36 in Ragged) shallow-copies value fields but copies the _stemItems list reference. Both the old and new frame then share the same List<string>, so .Add() mutates it for all frames.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes incorrect stem accumulation in Microsoft.Extensions.FileSystemGlobbing caused by struct frame copies sharing the same _stemItems list reference, ensuring each frame has isolated stem state and adding a regression test for sibling wildcard directories.
Changes:
- Clone
FrameDataonPushDirectoryin both linear and ragged pattern contexts to avoid sharing stem list state across frames. - Remove
PatternContextRagged.PopDirectorystem-item mutation workaround (no longer needed with isolated frame state). - Add a functional regression test covering multiple sibling wildcard directories to validate correct stem output.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/Internal/PatternContexts/PatternContextLinear.cs | Uses Frame.Clone() to prevent shared stem list state across stack frames. |
| src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/Internal/PatternContexts/PatternContextRagged.cs | Switches to frame cloning and removes pop-time stem mutation to fix stem correctness. |
| src/libraries/Microsoft.Extensions.FileSystemGlobbing/tests/FunctionalTests.cs | Adds regression test asserting stems don’t accumulate across sibling wildcard directories. |
src/libraries/Microsoft.Extensions.FileSystemGlobbing/tests/FunctionalTests.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Extensions.FileSystemGlobbing/src/Internal/PatternContexts/PatternContextLinear.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Extensions.FileSystemGlobbing/src/Internal/PatternContexts/PatternContextRagged.cs
Outdated
Show resolved
Hide resolved
mrek-msft
approved these changes
Mar 26, 2026
svick
reviewed
Mar 26, 2026
| } | ||
|
|
||
| [Fact] | ||
| public void StemIsCorrectForMultipleWildcardSiblingDirectories() |
Member
There was a problem hiding this comment.
AFAICT, this tests PatternContextLinear. Shouldn't there be also a test for PatternContextRagged ?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #97333.