Quoting args doesn't work as expected on Windows

[Tyriar]

Downstream issue: microsoft/vscode#19078

argvToCommandLine makes the following transformation:

cmdline: cmd.exe,/c,dir "a b"
cmdlineFlat: cmd.exe /c "dir \"a b\""

[Tyriar]

@the-ress any feedback on 28be849 would be appreciated 😃

[the-ress]

The problem is that every application can parse the command line differently. The current implementation works well with applications that use CommandLineToArgvW or compatible implementation (node.js and C# applications among others). cmd.exe or batch files handle arguments differently.

So 28be849 breaks escaping for the first group of applications. I would solve the problem in microsoft/vscode#19078 by using ["dir", "test folder"] because that works too, but I can't think think of any universal solution.

[Tyriar]

The issue with ["dir", "test folder"] is it's 2 args though so it might behave differently. If the command required escaping within an arg could you not just specify args as ["dir \\"test folder\\""]?

Do you have an example command/args where the change would fail?

[TSlivede]

@Tyriar
I think 28be849 should definitely be reverted:

When calling an executable with an array of arguments, almost every CLI-App splits this Commandline according to these rules (the only exception I currently know of, is old legacy cmd.exe). As node-pty presents an Unix-like interface to conlole applications, it should also provide a unix-like argv[] calling mechanism instead of windows-like escape-Commandline-yourself way. Therefore the Commandline should be build in a way, that matches those rules.

Many modern Commandshells on windows do this:

• https://www.hamiltonlabs.com/Cshell.htm (not documented)
• Cygwin (see linebuf::fromargv)
• Python (see list2cmdline)
• Tcl (see BuildCommandLine, Comment: "N backslashes followed a quote -> insert N * 2 + 1 backslashes then a quote.")

To solve the problem with cmd, one could allow passing a string as args instead of an array. Or one could explicitly test if cmd.exe is called and then do different processing.

28be849 is very inconsistent (backslashes are escaped, quotes not?). If it is really desired, to let the user quote arguments himself, argvToCommandLine should be completely removed, so the user must enquote and escape the arguments by himself, like in the "Note" box on MSDN: _exec, but this seems somewhat messy...

Examples where 28be849 fails: Starting any other Commandlineapplication than cmd.exe with complicated arguments.

[Tyriar]

Thanks for the detailed explanation @TSlivede, makes sense. I was holding off on release until I got someone who knows more about the area to confirm 😄 I'll probably allow args as a string as you suggest:

export function fork(file?: string, args?: string | string[], opt?: IPtyForkOptions): ITerminal

[TSlivede]

@Tyriar Very nice!! 👍 👍 👍
Althought I'd call it ArgsOrCmdline or similar instead of ArgsOrArgv.
Both args and argv are names for Argument-arrays. 😃
CommandLine or CmdLine is the Microsoft terminology for the single argument-string.

[Tyriar]

Sounds good 👍

[TSlivede]

Just another note: To solve the problem in microsoft/vscode#19078, one could simply replace cmd.exe,/c,dir "a b" with powershell.exe,/c,dir "a b" and it should work.
Btw: This (with powershell) is one case that breaks with 28be849 😄

One more Note:
In unixTerminal.ts maybe instead of
throw new Error('args as a string is not supported on unix.');
better
throw new Error('args as type string is not supported on unix, please use a string array.');
to tell people what they did wrong?

[Tyriar]

Fixed in #47