Fix "Invoke-Item" to accept a file path with spaces on Unix platforms #3850
Conversation
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
adityapatwardhan
approved these changes
May 24, 2017
| // If it's headless SKUs, rethrow. | ||
| if (Platform.IsNanoServer || Platform.IsIoT) { throw; } | ||
| // If it's full Windows, then try ShellExecute. | ||
| ShellExecuteHelper.Start(invokeProcess.StartInfo); | ||
| } | ||
| #else | ||
| invokeProcess.StartInfo.FileName = path; | ||
| invokeProcess.Start(); | ||
| finally { /* Nothing to do in FullCLR */ } |
There was a problem hiding this comment.
Do we need a empty finally block?
There was a problem hiding this comment.
The finally block is kept there for FullCLR section only to match the try block above, so that we don't need to enclose try { and } in if/def. I will add one more comment to make the purpose more clear.
An empty finally block will be ignored during compilation (release version), so there won't be any performance concern. See the code below:
static void Main(string[] args)
{
Process p = null;
try { p = new Process(); } finally { }
}
Corresponding IL
.method private hidebysig static void Main(string[] args) cil managed
{
.entrypoint
// Code size 7 (0x7)
.maxstack 8
IL_0000: newobj instance void [System.Diagnostics.Process]System.Diagnostics.Process::.ctor()
IL_0005: pop
IL_0006: ret
} // end of method Program::Main
|
The cmdlet seems to have the most unpredictable behavior😕 |
|
@iSazonov I think we are focusing on 2 scenarios for this cmdlet:
|
|
@daxian-dbw Thanks for clarify. Could you move the comment to the PR description for future docs? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Address #2900
Root Cause
On Unix platforms, we are currently depending on
xdg-openon Linux andopenon OSX to open the default program for a registered file type, so the file path is served as an argument toxdg-openandopen. However, we didn't handle the case when path contains spaces.Fix
Use the method
NativeCommandParameterBinder.NeedQuotes, which is used by powershell native command processor, to check if quote is needed. If yes, add quotes in the same way as our native command processor (see the code here).There is another problem with the current
Invoke-Itemon Linux and OSX -- it cannot invoke an executable properly. The fix is to first runProcess.Startdirectly with the file path, and if it fails, then tryxdg-openoropen.Additional Info
We are focusing on 2 scenarios for "Invoke-Item":