Skip to content

FileSystemGlobbing: Allow rootDir paths ending with separator to match files correctly#45139

Merged
jozkee merged 3 commits intodotnet:mainfrom
jozkee:fsglobbing_paths_with_ending_separator
Mar 30, 2021
Merged

FileSystemGlobbing: Allow rootDir paths ending with separator to match files correctly#45139
jozkee merged 3 commits intodotnet:mainfrom
jozkee:fsglobbing_paths_with_ending_separator

Conversation

@jozkee
Copy link
Member

@jozkee jozkee commented Nov 24, 2020

InMemoryDirectoryInfo.IsRootDirectory method was failing to evaluate correctly when the rootDir argument was ending with a separator e.g: C:\ or /. This PR changes the condition to correctly support the aforementioned cases.

Fixes #36415, #44767

@jozkee jozkee added this to the 6.0.0 milestone Nov 24, 2020
@jozkee jozkee self-assigned this Nov 24, 2020
@ghost
Copy link

ghost commented Nov 24, 2020

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

InMemoryDirectoryInfo.IsRootDirectory method was failing to evaluate correctly when the rootDir argument was ending with a separator e.g: C:\ or /. This PR changes the condition to correctly support the aforementioned cases.

Fixes #36415 #44767

Author: Jozkee
Assignees: Jozkee
Labels:

area-Extensions-FileSystem

Milestone: 6.0.0

Comment on lines 125 to 126
Copy link
Member Author

Choose a reason for hiding this comment

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

Existing condition on line 126 was meant to avoid false positives when the path started with rootDir but it wasn't the complete name of the directory e.g:

var matcher = new Matcher();
matcher.AddInclude(@"**\*");

Console.WriteLine(matcher.Match(rootDir: @"C:\foo", file: @"C:\foo2\bar.cs").HasMatches); 
// prints false, which is correct.

However that same condition was interfering when rootDir = @"C:\foo\" since filePath.IndexOf(Path.DirectorySeparatorChar, rootDirLength) was always going to return the index of the separator of the next directory.

Therefore I added condition on line 125 in order to skip the length check when rootDir ends with a separator.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems line 126 checks one char in position rootDirLength - why do we use whole IndexOf() method instead of simple index - filePath[rootDirLength] == Path.DirectorySeparatorChar - like in line 125?

@jozkee jozkee closed this Jan 21, 2021
@jozkee jozkee reopened this Jan 21, 2021
@jozkee jozkee force-pushed the fsglobbing_paths_with_ending_separator branch from c282838 to 855b266 Compare January 21, 2021 19:51
Copy link
Contributor

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

LGTM. Left some minor comments.

@carlossanlop carlossanlop self-requested a review February 17, 2021 00:59
@carlossanlop
Copy link
Contributor

Actually, I noticed all the unit test failures are happening in Microsoft.Extensions.FileSystemGlobbing.Tests. Can you please take a look?

Base automatically changed from master to main March 1, 2021 09:07
@jozkee jozkee force-pushed the fsglobbing_paths_with_ending_separator branch from 855b266 to 21a8093 Compare March 1, 2021 20:51
@jozkee
Copy link
Member Author

jozkee commented Mar 1, 2021

@carlossanlop I excluded scenarios using Windows-like absolute paths from running on non-Windows OSes since FileSystemGlobbing is OS-dependent when dealing with paths. That should fix the issues on CI.

@jozkee jozkee merged commit 20864e7 into dotnet:main Mar 30, 2021
@jozkee jozkee deleted the fsglobbing_paths_with_ending_separator branch March 30, 2021 16:51
@ghost ghost locked as resolved and limited conversation to collaborators Apr 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

InMemoryDirectoryInfo does not work as expected

3 participants