Skip to content

Conversation

@chunqingchen
Copy link
Contributor

Fix issue #3967

Summary of the issue:

According to Jason's suggestion, replace the regex expression with wildcard.

Root cause of the issue:

This is an code improvement. The existed test cases are covering the scenario

Copy link
Contributor

Choose a reason for hiding this comment

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

fileName isn't used when wildcardPattern is null, so we should avoid the call (which copies and allocates memory) unless we'll actually use the value.

Copy link

Choose a reason for hiding this comment

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

Actually, shouldn't fileName also be used when there are no wildcards? Currently

if (filePath.IndexOf(pattern, StringComparison.OrdinalIgnoreCase) >= 0)

is searching the whole path for a match. If it is desirable to only target the file name when performing the wildcard match, shouldn't it also be appropriate here? (Actually, given that an exact match is expected — that's how GetFiles would work in the non-UNIX case for a non-wildcard pattern, if I am not mistaken — shouldn't the test be against fileName.Equals(pattern, StringComparison.OrdinalIgnoreCase) instead of using IndexOf?)

(An unrelated minor issue I believe exists with the logic of GetFiles is using break instead of continue when a non-wildcard match is found. This might limit the matches in the (rare) case in which a file name equals a wildcard search pattern, e.g. foo?, though I haven't tested how GetFileswould behave in that scenario, and it might be nitpicking if the expected directory and file names are curated.

Also, sorry for butting in, but since I wasn't sure enough about my observations I didn't want to open an issue, so I didn't know a better way to proceed.)

Copy link
Contributor

Choose a reason for hiding this comment

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

@gonhidi - thanks for the extra scrutiny - it is definitely appreciated.

It does seem like there should be consistency between wildcards and non-wildcards. Feel free to open an issue.

@chunqingchen
Copy link
Contributor Author

@lzybkr Thanks Jason, your comment has been resolved

@lzybkr lzybkr merged commit b4b4e60 into PowerShell:master Jun 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants