Adding support for native command globbing on UNIX #3643
Conversation
This usually indicates the encoding of the file is not utf-8. Could you please change the encoding to utf-8? |
| @@ -218,13 +218,117 @@ private void appendOneNativeArgument(ExecutionContext context, object obj, char | |||
| } | |||
| else | |||
| { | |||
| #if UNIX | |||
| // On UNIX systems, we expand arguments containing wildcard expressions against | |||
| // the file system just like bash, etc. | |||
There was a problem hiding this comment.
I thought we settled on calling the C api glob.
There was a problem hiding this comment.
I have no recollection of any such thing. Why would we do that rather than using PowerShell's intrinsic globbing capability?
There was a problem hiding this comment.
To be compatible as compatible with bash as reasonably possible. There are things glob does that we don't with character classes.
I don't recall discussing or looking closely at the inverse - if our wildcards support things that glob does not.
There was a problem hiding this comment.
PowerShell globbing supports all of the constructs described in glob(3) but is case-insensitive plus it understands powershell drives which fnmatch(3) is unaware of. Having the globbing behavior change based on the type of the command sounds like a very bad user experience.
There was a problem hiding this comment.
Are you sure? /bin/echo * should not echo files starting with ., and /bin/echo "*" should echo *, not the files in the current directory.
Maybe less interesting, but glob(7) says glob supports character classes like [:upper:] and more - we definitely don't support those. That said, it doesn't look like bash supports those.
Case-insensitive also concerns me. rm M* shouldn't remove files starting with lowercase m.
I'm not saying that glob(3) is exactly what we should use - just that there may be subtleties that we miss by not using a POSIX api.
As for PowerShell drives - I'm really curious if they'll be used on non-Windows platforms - I'm skeptical - symbolic links have served that purpose perfectly and work outside of PowerShell.
There was a problem hiding this comment.
Looking at the bash source, a lot of the subtleties around globbing are in bash itself, not the glob routine. For example ~ expansion is not handled by glob. Likewise quote processing is handled in bash itself, not by the glob routine. Given that PowerShell does it's quote removal (and addition) in a very different way than bash, there are going to be differences. (To make /bin/echo "*" work, we need to be able to be able to determine if a string value was parsed as a bareword literal at execution time.) Also note that Bash optionally supports extended globbing (See Bash Extended Globbing for an overview). Anyway, I've opened a new issue to track this investigation #3655
| /bin/rm -fr $tempdir 2> $null | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I would rather these tests be done in $TESTDRIVE which will eliminate the need for AfterAll ($TESTDRIVE and contents are automatically deleted by pester)
BeforeAll {
if (-not $IsWindows )
{
"" > "$TESTDRIVE/abc.txt"
"" > "$TESTDRIVE/bbb.txt"
"" > "$TESTDRIVE/cbb.txt"
}
}
and the tests:
# Test simple * expansion
It 'The globbing pattern should match 3 files' @PesterSkip {
(/bin/ls $TESTDRIVE/*.txt).Length | Should Be 3
}
| if (cwdinfo != null) | ||
| { | ||
| cwd = cwdinfo.FullName; | ||
| } |
There was a problem hiding this comment.
How about using Context.EngineSessionState.CurrentLocation? Then you can check if (pwd.Provider.Name == FileSystemProvider.ProviderName) to see if it's a filesystem location, and pwd.ProviderPath to get the full path.
| } | ||
| else | ||
| { | ||
| if (! (pbo is System.IO.FileInfo || pbo is System.IO.DirectoryInfo)) |
There was a problem hiding this comment.
How about if (! (pbo is System.IO.FileSystemInfo))?
| } | ||
| else | ||
| { | ||
| System.IO.DirectoryInfo di = pbo as System.IO.DirectoryInfo; |
There was a problem hiding this comment.
It's recommended to use is-expression introduced in C# 7 to replace the as operator because it can help reduce nested brackets. For example, here it can be:
if (pbo is FileInfo fi)
{
...
}
else if (pbo is DirectoryInfo di)
{
...
}
There was a problem hiding this comment.
With the changes to the code, this no longer applies.
| /// <param name="directory">The directory to base the truncation on.</param> | ||
| /// <param name="path">The path to truncate.</param> | ||
| /// <returns>The relative path that was produced.</returns> | ||
| private static string computeRelativePath(string directory, string path) |
There was a problem hiding this comment.
Why not use SessionState.Path.NormalizeRelativePath.NormalizeRelativePath(string path, string basePath)? We use it in Resolve-Path.
fec390e to
d9955b0
Compare
|
Since @BrucePay is OOF for a conference, I pushed a commit to update the tests to use the recommended SKIP pattern. |
|
Might anyone be able to explain
a little more, I can't get it to work in my command line. It may be that it also suppresses
results in |
|
@chrisfcarroll $database="postgres://$(othercalculatedelements)"
/bin/echo -database "$database" --% -source file://./update up |
|
@SteveL-MSFT thanks for clarification. Alas that brings back all the problems of bug #3931 which is what I'm struggling to work around. |
This change enables globbing (wildcard expansion) against the file system for native commands like /bin/ls. Expansion is only done in the file system. In non-filesystem drives expansion is not done and the pattern is returned unchanged.
Limitations:
Currently quoting is not honored so for a command like
/bin/ls "*.txt", wildcard expansion will still be done. Adding support for bare word detection will come in a future PR. Use --% to suppress wildcard expansion e.g. git add --% *This (partially) fixes #954