Fix the issue Get-Help does not support string pattern under Unix #3852

chunqingchen · 2017-05-24T08:25:39Z

Fix issue #2653

Summary of the issue:

Under Unix system, get-help would not support wildcast pattern such as about*

Root cause of the issue:

Under unix, GetFiles() is using filePath.IndexOf() to search string pattern, this api actually doesn't support pattern.

Fix:

Use regex expression to match *, ? if the search string contains such wildcast. which are the only two cases are supported.

adityapatwardhan · 2017-05-24T17:28:58Z

src/System.Management.Automation/help/MUIFileSearcher.cs

-#endif
+            // On Linux, file names are case sensitive, so we need to put both string to lower case.
+            // We can not use StringComparison.OrdinalIgnoreCase here as api supports OrdinalIgnoreCase won't support pattern
+            return Directory.GetFiles(path.ToLower(), pattern.ToLower());


ToLower() is not required for path, as GetFiles considers it as case insensitive.

adityapatwardhan · 2017-05-24T17:29:02Z

test/powershell/engine/Help/HelpSystem.Tests.ps1

+        {
+            # Create at least one help file matches "about*" pattern
+            $null = New-Item -ItemType File -Path $helpFilePath -Value "about_test" -ErrorAction SilentlyContinue
+            (Get-ChildItem $PSHOME\$helpFileWithPattern -Recurse).Count | Should Be (Get-Help about*).Count


Can you also verify that the about_testCase.help.txt is one of the files that is returned by Get-Help?

resolved by only get the specified files start with about_testcase*

adityapatwardhan · 2017-05-24T17:41:56Z

test/powershell/engine/Help/HelpSystem.Tests.ps1

+        {
+            # Create at least one help file matches "about*" pattern
+            $null = New-Item -ItemType File -Path $helpFilePath -Value "about_test" -ErrorAction SilentlyContinue
+            (Get-ChildItem $PSHOME\$helpFileWithPattern -Recurse).Count | Should Be (Get-Help about*).Count


Get-Help can get help content from other paths under PSModulePath. So if there are other about help topics not under $PSHome, the counts would not match.

daxian-dbw · 2017-05-24T17:55:47Z

@chunqingchen The PR title and body should not be a copy of the issue. Please update the PR title and body based on the contribution.md.

General Guidelines

Use a meaningful title for the PR to describe what is changed in this PR.
- Do not include issue number in the title. Put it in the description instead, like "Fix Fix help alias #5".
- Do not use the issue title as the PR title.
  An issue title is to briefly describe what went wrong, while a PR title is to briefly describe what is changed.
The PR body shall provide an additional summary of what/why went wrong and how it's fixed in the PR description.
- An example: Fix Process.Start() on Unix platforms to not leak file descriptors of redirection pipes to child processes dotnet/corefx#19988

chunqingchen · 2017-05-25T11:40:58Z

@daxian-dbw your comment is resolved
@adityapatwardhan your comments are resolved

SteveL-MSFT · 2017-05-25T16:23:02Z

src/System.Management.Automation/help/MUIFileSearcher.cs

-        private string[] GetFiles(string path, string pattern)
-        {
+		private string[] GetFiles(string path, string pattern)
+		{ 


looks like a formatting mistake here

The formatting is fixed. It shows normal under IDE, I have to delete and re tab to fix it.

SteveL-MSFT · 2017-05-25T16:33:56Z

src/System.Management.Automation/help/MUIFileSearcher.cs

                }
            }
            return (String[])result.ToArray(typeof(string));
 #else
-            return Directory.GetFiles(path, pattern);
+			return Directory.GetFiles(path, pattern);


why the formatting change?

SteveL-MSFT · 2017-05-25T16:39:07Z

src/System.Management.Automation/help/MUIFileSearcher.cs

+            string regexPattern = pattern.Replace(".", @"\.");
+            regexPattern = regexPattern.Replace("*", ".*");
+            regexPattern = regexPattern.Replace("?", ".?");
+            Regex r = new Regex(regexPattern);


Since this matching is infrequent, you should use an interpreted regex instead of a compiled one

https://msdn.microsoft.com/en-us/library/gg578045(v=vs.110).aspx

You can also use RegEx.IsMatch(filePath, regexPattern). This avoids instantiating the an RegEx object and RegEx engine caches the compiled version for reuse.

SteveL-MSFT · 2017-05-25T16:40:32Z

test/powershell/engine/Help/HelpSystem.Tests.ps1

+            # Create at least one help file matches "about*" pattern
+            $null = New-Item -ItemType File -Path $helpFilePath1 -Value "about_test1" -ErrorAction SilentlyContinue
+            $null = New-Item -ItemType File -Path $helpFilePath2 -Value "about_test2" -ErrorAction SilentlyContinue
+            (Get-Help about_testCase*).Count | Should Be 2


This test case doesn't cover the code change, you explicitly handle *, ?, and .

Seems like you should also have a test case with multiple wildcards

adityapatwardhan · 2017-05-25T20:20:46Z

src/System.Management.Automation/help/MUIFileSearcher.cs

 #if UNIX
            // On Linux, file names are case sensitive, so we need to add
            // extra logic to select the files that match the given pattern.
            ArrayList result = new ArrayList();
            string[] files = Directory.GetFiles(path);
+
+            string regexPattern = pattern.Replace(".", @"\.");


RegEx.Escape(pattern) should be used first, before any replaces.

This actually should not be used. RegEx.Escape will also escape the '*' and '?' which we want them as normal character to replace later.

adityapatwardhan · 2017-05-25T20:27:13Z

src/System.Management.Automation/help/MUIFileSearcher.cs

+            string regexPattern = pattern.Replace(".", @"\.");
+            regexPattern = regexPattern.Replace("*", ".*");
+            regexPattern = regexPattern.Replace("?", ".?");
+            Regex r = new Regex(regexPattern);


You can also use RegEx.IsMatch(filePath, regexPattern). This avoids instantiating the an RegEx object and RegEx engine caches the compiled version for reuse.

adityapatwardhan · 2017-05-25T20:29:10Z

src/System.Management.Automation/help/MUIFileSearcher.cs

+            regexPattern = regexPattern.Replace("*", ".*");
+            regexPattern = regexPattern.Replace("?", ".?");
+            Regex r = new Regex(regexPattern);
+
            foreach (string filePath in files)


Can all this code be replaced by:

return Directory.GetFiles(path).Where(f => Regex.IsMatch(f, regExPattern.ToString())).ToArray();

return Directory.GetFiles(path).Where(f => Regex.IsMatch(f, regExPattern.ToString())).ToArray(); is not implementing the logic of string which is not using a pattern

chunqingchen · 2017-05-31T12:19:35Z

@adityapatwardhan @SteveL-MSFT Thanks Aditya and Steve. Your comments are resolved.

I have spent much time try to figure out why there's time out exception on Unix test runs on my pull request. It ended up not related to my code and also the code and tests get passed on local unix os (both Ubuntu and MacOS). See my comments with the test case. I have to put a skip on the test case if it is not windows os.

adityapatwardhan · 2017-05-31T16:33:16Z

src/System.Management.Automation/help/MUIFileSearcher.cs

@@ -114,17 +115,31 @@ private void SearchForFiles()
        }

        private string[] GetFiles(string path, string pattern)
-        {
+        { 


Extra space after {

adityapatwardhan · 2017-05-31T16:35:51Z

test/powershell/engine/Help/HelpSystem.Tests.ps1

@@ -225,4 +225,38 @@ Describe "Get-Help should find help info within help files" -Tags @('CI', 'Requi
            Remove-Item $helpFilePath -Force -ErrorAction SilentlyContinue
        }
    }
+
+    # There is a bug specified to Tranvis CI that hangs the test if "get-help" is used to search pattern string. This doesn't repro locally. 


Typos:
Tranvis -> Travis
specified -> specific

adityapatwardhan · 2017-05-31T16:41:32Z

test/powershell/engine/Help/HelpSystem.Tests.ps1

+        $helpFilePath1 = Join-Path $helpFolderPath $helpFile1
+        $helpFilePath2 = Join-Path $helpFolderPath $helpFile2
+
+        if (!(Test-Path $helpFolderPath))


The if block can be replace by the following. If it already exists, it is not modified.

$null = New-Item -ItemType Directory -Path $helpFolderPath -Force

adityapatwardhan · 2017-05-31T16:46:10Z

test/powershell/engine/Help/HelpSystem.Tests.ps1

+            # Create at least one help file matches "about*" pattern
+            $null = New-Item -ItemType File -Path $helpFilePath1 -Value "about_test1" -ErrorAction SilentlyContinue
+            $null = New-Item -ItemType File -Path $helpFilePath2 -Value "about_test2" -ErrorAction SilentlyContinue           
+            Get-Help about_testCas?1 | Should Match "about_test1"


For all the Shoulds, use Be or BeExactly. Match uses regular expression comparison. It is not required here.

adityapatwardhan · 2017-05-31T16:50:23Z

test/powershell/engine/Help/HelpSystem.Tests.ps1

+            Get-Help about_testCas?1 | Should Match "about_test1"
+            Get-Help about_testCase.? | Should Match "about_test2"
+            (Get-Help about_testCase*).Count | Should Be 2
+            Get-Help about_testCas?.2* | Should Match "about_test2"


Ideally, each It should have only one Should. This ensures that if the first should fails, the rest are not ignored. Take a look at: http://www.powershellmagazine.com/2015/06/04/pester-triangulation-and-reusing-test-cases/
You can pass the values for Get-Help as testcases and values for Should as expected values. Using testcases parameter of It also allows you to give better names for each variation.

Agree with @adityapatwardhan, this is a good example to use the -TestCases parameter of Pester. Look at other test code in this repo as a sample.

resolved with triangulation format. a good way !

chunqingchen · 2017-05-31T22:25:21Z

@adityapatwardhan @SteveL-MSFT your comments are resolved

SteveL-MSFT · 2017-05-31T22:34:28Z

test/powershell/engine/Help/HelpSystem.Tests.ps1

+                    @{command = "Get-Help about_testCas?.2*"; testname = "test ?, * pattern with dot"; result = "about_test2"}
+                 )
+
+    It "Get-Help should find pattern help files - <testname>" -TestCases $testcases -skip: (-not $IsWindows){


unless the testcases are used in other tests, I would prefer to have these inline to the It statement as it makes it easier to associate the two

A good example of using test cases would be:

PowerShell/test/powershell/Modules/Microsoft.PowerShell.Management/Get-Service.Tests.ps1

Line 15 in 02b5f35

$testCases =

adityapatwardhan · 2017-05-31T22:39:08Z

test/powershell/engine/Help/HelpSystem.Tests.ps1

+                    @{command = "Get-Help about_testCas?.2*"; testname = "test ?, * pattern with dot"; result = "about_test2"}
+                 )
+
+    It "Get-Help should find pattern help files - <testname>" -TestCases $testcases -skip: (-not $IsWindows){


A good example of using test cases would be:

PowerShell/test/powershell/Modules/Microsoft.PowerShell.Management/Get-Service.Tests.ps1

Line 15 in 02b5f35

$testCases =

adityapatwardhan · 2017-05-31T22:40:00Z

test/powershell/engine/Help/HelpSystem.Tests.ps1

+        try
+        {
+            # Create at least one help file matches "about*" pattern
+            $null = New-Item -ItemType File -Path $helpFilePath1 -Value "about_test1" -ErrorAction SilentlyContinue


Can these be done in beforeall and cleanup in afterall?

adityapatwardhan · 2017-06-06T19:57:34Z

test/powershell/engine/Help/HelpSystem.Tests.ps1

+            $command,
+            $result
+        )
+        Invoke-Expression $command | Should Be $result


Don't use Invoke-Expression instead use splatting instead. Look at: 28ec9a3

In that case (Get-Help about_testCase*).Count would needs to be explicitly written in the test case one more time. If u think this is better than invoke-expression i will update later.

After talking to Jim, in general Invoke-Expression is not a best practice. An alternative approach would be to make the command as script block. So the test would look like:

$testcases = @( @{command = {Get-Help about_testCas?1}; testname = "test ? pattern"; result = "about_test1"} @{command = {Get-Help about_testCase.?}; testname = "test ? pattern with dot"; result = "about_test2"} @{command = {(Get-Help about_testCase*).Count}; testname = "test * pattern"; result = "2"} @{command = {Get-Help about_testCas?.2*}; testname = "test ?, * pattern with dot"; result = "about_test2"} )

And replace Invoke-Expression with

$command.Invoke() | Should be $result

adityapatwardhan · 2017-06-06T19:58:02Z

test/powershell/engine/Help/HelpSystem.Tests.ps1

+                  @{command = "Get-Help about_testCas?.2*"; testname = "test ?, * pattern with dot"; result = "about_test2"}
+               )
+
+    It "Get-Help should find pattern help files - <testname>" -TestCases $testcases -skip: (-not $IsWindows){


This should be marked as Pending on Non-Windows and not skipped. So we can revisit later.

adityapatwardhan · 2017-06-06T19:59:57Z

test/powershell/engine/Help/HelpSystem.Tests.ps1

+
+    # There is a bug specific to Travis CI that hangs the test if "get-help" is used to search pattern string. This doesn't repro locally. 
+    # This occurs even if Unix system just returns "Directory.GetFiles(path, pattern);" as the windows' code does.
+    # Since there's currently no way to get the vm from Tranvis CI and the test PASSES locally on both Ubuntu and MacOS, excluding pattern test under Unix system.


Typo: Tranvis -> Travis.

adityapatwardhan · 2017-06-06T20:00:44Z

test/powershell/engine/Help/HelpSystem.Tests.ps1

+  # Remove the test files
+  AfterAll {
+    Remove-Item $helpFilePath1 -Force -ErrorAction SilentlyContinue
+    Remove-Item $helpFilePath2 -Force -ErrorAction SilentlyContinue


$helpFolderPath does not seem to be cleaned up? Maybe you can just delete $helpFolderPath recursively.

$helpFolderPath is an existed folder contains other help files.

adityapatwardhan · 2017-06-06T21:50:04Z

test/powershell/engine/Help/HelpSystem.Tests.ps1

+            $command,
+            $result
+        )
+        Invoke-Expression $command | Should Be $result


After talking to Jim, in general Invoke-Expression is not a best practice. An alternative approach would be to make the command as script block. So the test would look like:

$testcases = @( @{command = {Get-Help about_testCas?1}; testname = "test ? pattern"; result = "about_test1"} @{command = {Get-Help about_testCase.?}; testname = "test ? pattern with dot"; result = "about_test2"} @{command = {(Get-Help about_testCase*).Count}; testname = "test * pattern"; result = "2"} @{command = {Get-Help about_testCas?.2*}; testname = "test ?, * pattern with dot"; result = "about_test2"} )

And replace Invoke-Expression with

$command.Invoke() | Should be $result

chunqingchen · 2017-06-06T23:08:00Z

@adityapatwardhan your comment has been resolved

lzybkr · 2017-06-07T02:33:01Z

src/System.Management.Automation/help/MUIFileSearcher.cs

+                // If the input is pattern instead of string, we need to use Regex expression.
+                if (pattern.Contains("*") || pattern.Contains("?"))
+                {             
+                    if (Regex.IsMatch(filePath, regexPattern))


Why are we using regular expression matching instead of wildcard matching?

FWIW - wildcard matching already converts the wildcard to a regex - so this code is probably doing something similar but subtly different.

I agree. We basically need to implementation Directory.GetFiles(path, pattern); in a case-insensitive way, and the code at https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs#L5961 is doing the same thing.

#4053 is opened for this.
Never mind, it's a duplicate of #3967

chunqingchen requested a review from adityapatwardhan May 24, 2017 08:25

msftclas added the cla-not-required label May 24, 2017

chunqingchen force-pushed the bugfix0 branch from 9f52265 to 5cae2f6 Compare May 24, 2017 09:35

adityapatwardhan requested changes May 24, 2017

View reviewed changes

daxian-dbw assigned mirichmo May 24, 2017

chunqingchen force-pushed the bugfix0 branch 2 times, most recently from 9881659 to 92a2b51 Compare May 25, 2017 10:01

chunqingchen changed the title ~~Get-Help does not support string pattern under Unix~~ Fix the issue Get-Help does not support string pattern under Unix May 25, 2017

SteveL-MSFT requested changes May 25, 2017

View reviewed changes

adityapatwardhan requested changes May 25, 2017

View reviewed changes

chunqingchen force-pushed the bugfix0 branch 11 times, most recently from 950584f to b82e76b Compare May 31, 2017 11:02

adityapatwardhan requested changes May 31, 2017

View reviewed changes

chunqingchen force-pushed the bugfix0 branch from b82e76b to 4329e29 Compare May 31, 2017 22:24

SteveL-MSFT requested changes May 31, 2017

View reviewed changes

adityapatwardhan requested changes May 31, 2017

View reviewed changes

chunqingchen force-pushed the bugfix0 branch from 4329e29 to a1c500a Compare June 6, 2017 19:23

adityapatwardhan requested changes Jun 6, 2017

View reviewed changes

chunqingchen force-pushed the bugfix0 branch from a1c500a to 72cecb9 Compare June 6, 2017 20:51

SteveL-MSFT approved these changes Jun 6, 2017

View reviewed changes

adityapatwardhan requested changes Jun 6, 2017

View reviewed changes

Fix the issue Get-Help does not support string pattern under Unix

dc80349

chunqingchen force-pushed the bugfix0 branch from 72cecb9 to dc80349 Compare June 6, 2017 22:25

adityapatwardhan approved these changes Jun 6, 2017

View reviewed changes

mirichmo merged commit 751dada into PowerShell:master Jun 7, 2017

lzybkr reviewed Jun 7, 2017

View reviewed changes

This was referenced Jun 7, 2017

Get-Help does not list all 'About*' topics #2653

Closed

Get-Help about_* returns nothing #1809

Closed

lzybkr mentioned this pull request Jun 7, 2017

Wildcard matching should be used in help file search #3967

Closed

daxian-dbw mentioned this pull request Jun 20, 2017

MUIFileSearcher.GetFiles(string path, string pattern) in 'MUIFileSearcher.cs' needs to be refactored #4053

Closed

This was referenced Dec 27, 2022

Refactor MUIFileSearcher.AddFiles #18825

Merged

Enable Get-Help pattern tests on Unix #18855

Merged

Fix the issue Get-Help does not support string pattern under Unix #3852

Fix the issue Get-Help does not support string pattern under Unix #3852

Conversation

chunqingchen commented May 24, 2017 • edited by daxian-dbw

Summary of the issue:

Root cause of the issue:

Fix:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw commented May 24, 2017

chunqingchen commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunqingchen commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunqingchen commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunqingchen commented Jun 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw Jun 20, 2017 • edited

Choose a reason for hiding this comment

chunqingchen commented May 24, 2017 •

edited by daxian-dbw

daxian-dbw Jun 20, 2017 •

edited