Support unc paths for command execution #784
Conversation
| QuoteSingleArgument(outFile), | ||
| QuoteSingleArgument(errFile) | ||
| ); | ||
| psi.WorkingDirectory = workingDirectory; |
There was a problem hiding this comment.
Hrm... I'm concerned about the implications of not setting the WorkingDirectory property. In particular, the working directory property has different implications depending on the value of UseShellExecute. Did you verify the edge cases here?
https://msdn.microsoft.com/en-us/library/system.diagnostics.processstartinfo.workingdirectory(v=vs.110).aspx
There was a problem hiding this comment.
I tried testing the edge cases I could think of, but this is widely used code that is also shared with common so I'm sure I missed some areas. My understanding is that pushd replaces the need to set the working directory for executing the actual command. We are only relying on the initial, default working directory to pick up cmd.exe.
Any specific edge cases you had in mind?
There was a problem hiding this comment.
yes, see the UseShellExecute stuff in the link above. In particular...
When the UseShellExecute property is false, gets or sets the working directory for the process to be started. When UseShellExecute is true, gets or sets the directory that contains the process to be started.
There was a problem hiding this comment.
I think my question is more: what testing would make you more confident in the change?
I generally understand what UseShellExecute and WorkingDirectory do, but also believe that, with this change, working directory is only going to be used directly for looking up cmd.exe. Once we actually run node ..., pushd has already changed the working directory.
I verified all the basic scenarios against this change, while also running node programs to look at process.cwd() and opening relative file paths. Any other thoughts?
| QuoteSingleArgument(filename), | ||
| GetArguments(arguments, quoteArgs)), | ||
| CreateNoWindow = !visible, | ||
| UseShellExecute = false, |
There was a problem hiding this comment.
Nvrmind, reverted the suggested fix. This has to be false because we set the Redirect* properties. The elevated example manually redirects to a file, which is why it can get away with setting this to true.
Defect
#755 Unc paths cause a problem currently when running commands .
Fix
Use pushd to set the working directory when running a command. Tested by opening a project from
\\localhost\c$.closes #755