FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Exclude directories from -Path in Select-String by iSazonov · Pull Request #4829 · PowerShell/PowerShell · GitHub

Exclude directories from -Path in Select-String - #4829

Merged
Dongbo Wang (daxian-dbw) merged 2 commits into
PowerShell:masterfrom
iSazonov:select-string-dir
Sep 14, 2017
Merged

Exclude directories from -Path in Select-String#4829
Dongbo Wang (daxian-dbw) merged 2 commits into
PowerShell:masterfrom
iSazonov:select-string-dir

Conversation

Copy link
Copy Markdown
Collaborator

Fix #4820.

Select-String can search in files only so we should skip directory
objects.

Select-String can search in files only so we should skip directory
objects.
{
foreach (string filename in expandedPaths)
{
if (System.IO.Directory.Exists(filename))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

There is a using directive for System.IO.Directory, so any reason to use the full type name here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed.

}

It "Select-String does not throw on subdirectory (path with wildcard)" {
{ select-string -Path $pathWithWildcard "noExists" } | Should Not Throw

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I skipped '-ErrorAction Stop' - Fixed.

$fileNameWithDots = $fileName.FullName.Replace("\", "\.\")

$subDirName = Join-Path $TestDrive 'selectSubDir'
New-Item -Path $subDirName -ItemType Directory > $null

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Suggest using -ErrorAction Stop.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I see we usualy use -Force - I added.
Please clarify about -ErrorAction Stop - I don't found such samples in our tests.

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Dongbo Wang (daxian-dbw) merged commit a4cdb80 into PowerShell:master Sep 14, 2017
Ilya (iSazonov) deleted the select-string-dir branch September 15, 2017 03:04
{
foreach (string filename in expandedPaths)
{
if (Directory.Exists(filename))

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Good point. We can reactivate #4820 and get the behavior fixed. Ilya (@iSazonov) what do you think?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 Michael Klement (@mklement0) would done the analysis (what cmdlets should we fix?), I'd have solved the Issue.

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026
…ell#4829)

Select-String can search in files only so we should skip directories.
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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL