Exclude directories from -Path in Select-String #4829
Conversation
Select-String can search in files only so we should skip directory objects.
iSazonov
requested review from
JamesWTruher,
PaulHigin and
dantraMSFT
as code owners
| { | ||
| foreach (string filename in expandedPaths) | ||
| { | ||
| if (System.IO.Directory.Exists(filename)) |
There was a problem hiding this comment.
There is a using directive for System.IO.Directory, so any reason to use the full type name here?
| } | ||
|
|
||
| It "Select-String does not throw on subdirectory (path with wildcard)" { | ||
| { select-string -Path $pathWithWildcard "noExists" } | Should Not Throw |
There was a problem hiding this comment.
The current behavior is to throw a non-terminating error, so for this test, it will pass even if it actually failed. see [+] ee 297ms below
PS:122> describe "aa" { It "ee" { { Select-String -Path * hi } | should not throw } }
Describing aa
Select-String : The file F:\tmp\test\pp cannot be read: Access to the path 'F:\tmp\test\pp' is denied.
At line:1 char:29
+ describe "aa" { It "ee" { { Select-String -Path * hi } | should not t ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo : InvalidArgument: (:) [Select-String], ArgumentException
+ FullyQualifiedErrorId : ProcessingFile,Microsoft.PowerShell.Commands.SelectStringCommand
[+] ee 297ms
There was a problem hiding this comment.
Like the above should not throw test, you should verify the expected error (ProcessingFile,Microsoft.PowerShell.Commands.SelectStringCommand)
| } | ||
|
|
||
| It "Select-String does not throw on subdirectory (path without wildcard)" { | ||
| { select-string -Path $pathWithoutWildcard "noExists" } | Should Not Throw |
There was a problem hiding this comment.
Should not throw might not be a good test here because it doesn't throw with the current behavior -- a non-terminating error is written out.
There was a problem hiding this comment.
I think this test is fine but you should also verify the reported error (ProcessingFile,Microsoft.PowerShell.Commands.SelectStringCommand) either in the test or in another test.
There was a problem hiding this comment.
I skipped '-ErrorAction Stop' - Fixed.
| $fileNameWithDots = $fileName.FullName.Replace("\", "\.\") | ||
|
|
||
| $subDirName = Join-Path $TestDrive 'selectSubDir' | ||
| New-Item -Path $subDirName -ItemType Directory > $null |
There was a problem hiding this comment.
Suggest using -ErrorAction Stop.
There was a problem hiding this comment.
I see we usualy use -Force - I added.
Please clarify about -ErrorAction Stop - I don't found such samples in our tests.
There was a problem hiding this comment.
For me, its all about failure diagnosability. The test is dependent on the directory and if new-item fails, there's no point in the test continuing. Using -EA Stop ensures you get the error at the point of the failure instead of later on when it may be muddled by test or product code.
Its just a suggestion.
| { | ||
| foreach (string filename in expandedPaths) | ||
| { | ||
| if (Directory.Exists(filename)) |
There was a problem hiding this comment.
Hmm... I actually think we should only ignore directories if they're among the paths that result from wildcard resolution.
If you specify an effectively literal directory path - whether via -LiteralPath or via -Path and a path that happens not to contain (unescaped) wildcard characters - you should continue to get an error (though arguably a better one than currently).
This will let users know that specifying directories is not supported. With this fix as-is, If you do something like Select-String foo ., you'll get no output and no error, which could be misinterpreted as the string not having been found in any files in the directory.
I think the behavior should be analogous to the following Get-ChildItem behavior:
Get-ChildItem -Directory -LiteralPath /NoSuchDir # error
Get-ChildItem -Directory -Path /NoSuchDir # error too, because the path contains no (unescaped) wildcard chars.
Get-ChildItem -Directory -Path /NoSuchDir* # NO error, because the wildcard expression matched nothing.There was a problem hiding this comment.
Now Select-String behavior is the same as Unblock-File. There may be other examples (?)
So I believe it is another issue.
I agree that we could fix this with a "breaking change" symptom. If @mklement0 would done the analysis (what cmdlets should we fix?), I'd have solved the Issue.
5 participants
Fix #4820.
Select-String can search in files only so we should skip directory
objects.