Configure CLI11 to stuff all unknown positionals into the cmdli… (#4388)

This commit fixes an issue where "wt -d C: wsl -d Alpine" would be
parsed as "wt -d C: -d Alpine wsl" and rejected as invalid due to the
repeated -d. It also fixes support for the option parsing terminator,
--, in all command lines.

Fixes #4277.

doc/user-docs/UsingCommandlineArguments.md

@@ -128,29 +128,4 @@ This creates a new Windows Terminal window with one tab, and 3 panes:
   and will use `wsl.exe` as the commandline (instead of the default profile's
   `commandline`).
 
-### Running a specific linux distro in a specific distro
-
-Say you wanted to open a "Debian" WSL instance in `C:\Users`. According to the
-above reference, you might try:
-
-```
-wt -d c:\Users wsl -d Debian
-```
-
-Unfortunately, this _won't_ work, and will give you an error about "expected
-exactly 1 argument to `--startingDirectory`, got 2". This is because we'll
-erroneously try to parse the `-d Debian` parameter as a second
-`--startingDirectory` value. This is unexpected, and is a bug currently tracked
-in [#4277].
-
-As a workaround, you can try the following commandline:
-
-```
-wt -d c:\Users -- wsl -d Debian
-```
-
-In this commandline, the `--` will indicate to the Terminal that it should treat
-everything after that like a commandline.
-
 [#4023]: https://github.com/microsoft/terminal/pull/4023
-[#4277]: https://github.com/microsoft/terminal/issues/4277

src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp

@@ -44,6 +44,7 @@ namespace TerminalAppLocalTests
         TEST_METHOD(ParseSplitPaneIntoArgs);
         TEST_METHOD(ParseComboCommandlineIntoArgs);
         TEST_METHOD(ParseFocusTabArgs);
+        TEST_METHOD(ParseArgumentsWithParsingTerminators);
 
         TEST_METHOD(ParseNoCommandIsNewTab);
 
@@ -930,8 +931,7 @@ namespace TerminalAppLocalTests
     {
         Log::Comment(NoThrowString().Format(
             L"Check what happens when the user typos a subcommand. It should "
-            L"be treated as a commandline, unless other args are present that "
+            L"be treated as a commandline"));
-            L"the new-tab subcommand doesn't understand"));
 
         {
             AppCommandlineArgs appArgs{};
@@ -971,21 +971,69 @@ namespace TerminalAppLocalTests
                 L"Pass a flag that would be accepted by split-pane, but isn't accepted by new-tab"));
             AppCommandlineArgs appArgs{};
             std::vector<const wchar_t*> rawCommands{ L"wt.exe", L"slpit-pane", L"-H" };
+            _buildCommandlinesHelper(appArgs, 1u, rawCommands);
 
-            auto commandlines = AppCommandlineArgs::BuildCommands(rawCommands);
+            VERIFY_ARE_EQUAL(1u, appArgs._startupActions.size());
-            VERIFY_ARE_EQUAL(1u, commandlines.size());
-            VERIFY_ARE_EQUAL(3u, commandlines.at(0).Argc());
-            VERIFY_ARE_EQUAL("wt.exe", commandlines.at(0).Args().at(0));
 
-            for (auto& cmdBlob : commandlines)
+            auto actionAndArgs = appArgs._startupActions.at(0);
-            {
+            VERIFY_IS_NOT_NULL(actionAndArgs.Args());
-                const auto result = appArgs.ParseCommand(cmdBlob);
+            auto myArgs = actionAndArgs.Args().try_as<NewTabArgs>();
-                Log::Comment(NoThrowString().Format(
+            VERIFY_IS_NOT_NULL(myArgs);
-                    L"Exit Message:\n%hs",
+
-                    appArgs._exitMessage.c_str()));
+            VERIFY_IS_NOT_NULL(myArgs.TerminalArgs());
-                VERIFY_ARE_NOT_EQUAL(0, result);
+            VERIFY_ARE_EQUAL(L"slpit-pane -H", myArgs.TerminalArgs().Commandline());
-                VERIFY_ARE_NOT_EQUAL("", appArgs._exitMessage);
+        }
-            }
+    }
+
+    void CommandlineTest::ParseArgumentsWithParsingTerminators()
+    {
+        { // one parsing terminator, new-tab command
+            AppCommandlineArgs appArgs{};
+            std::vector<const wchar_t*> rawCommands{ L"wt.exe", L"new-tab", L"-d", L"C:\\", L"--", L"wsl", L"-d", L"Alpine" };
+            _buildCommandlinesHelper(appArgs, 1u, rawCommands);
+
+            VERIFY_ARE_EQUAL(1u, appArgs._startupActions.size());
+
+            auto actionAndArgs = appArgs._startupActions.at(0);
+            VERIFY_ARE_EQUAL(ShortcutAction::NewTab, actionAndArgs.Action());
+            VERIFY_IS_NOT_NULL(actionAndArgs.Args());
+            auto myArgs = actionAndArgs.Args().try_as<NewTabArgs>();
+            VERIFY_IS_NOT_NULL(myArgs);
+            VERIFY_IS_NOT_NULL(myArgs.TerminalArgs());
+            VERIFY_ARE_EQUAL(L"wsl -d Alpine", myArgs.TerminalArgs().Commandline());
+            VERIFY_ARE_EQUAL(L"C:\\", myArgs.TerminalArgs().StartingDirectory());
+        }
+        { // two parsing terminators, new-tab command
+            AppCommandlineArgs appArgs{};
+            std::vector<const wchar_t*> rawCommands{ L"wt.exe", L"new-tab", L"-d", L"C:\\", L"--", L"wsl", L"-d", L"Alpine", L"--", L"sleep", L"10" };
+            _buildCommandlinesHelper(appArgs, 1u, rawCommands);
+
+            VERIFY_ARE_EQUAL(1u, appArgs._startupActions.size());
+
+            auto actionAndArgs = appArgs._startupActions.at(0);
+            VERIFY_ARE_EQUAL(ShortcutAction::NewTab, actionAndArgs.Action());
+            VERIFY_IS_NOT_NULL(actionAndArgs.Args());
+            auto myArgs = actionAndArgs.Args().try_as<NewTabArgs>();
+            VERIFY_IS_NOT_NULL(myArgs);
+            VERIFY_IS_NOT_NULL(myArgs.TerminalArgs());
+            VERIFY_ARE_EQUAL(L"wsl -d Alpine -- sleep 10", myArgs.TerminalArgs().Commandline());
+            VERIFY_ARE_EQUAL(L"C:\\", myArgs.TerminalArgs().StartingDirectory());
+        }
+        { // two parsing terminators, *no* command
+            AppCommandlineArgs appArgs{};
+            std::vector<const wchar_t*> rawCommands{ L"wt.exe", L"-d", L"C:\\", L"--", L"wsl", L"-d", L"Alpine", L"--", L"sleep", L"10" };
+            _buildCommandlinesHelper(appArgs, 1u, rawCommands);
+
+            VERIFY_ARE_EQUAL(1u, appArgs._startupActions.size());
+
+            auto actionAndArgs = appArgs._startupActions.at(0);
+            VERIFY_ARE_EQUAL(ShortcutAction::NewTab, actionAndArgs.Action());
+            VERIFY_IS_NOT_NULL(actionAndArgs.Args());
+            auto myArgs = actionAndArgs.Args().try_as<NewTabArgs>();
+            VERIFY_IS_NOT_NULL(myArgs);
+            VERIFY_IS_NOT_NULL(myArgs.TerminalArgs());
+            VERIFY_ARE_EQUAL(L"wsl -d Alpine -- sleep 10", myArgs.TerminalArgs().Commandline());
+            VERIFY_ARE_EQUAL(L"C:\\", myArgs.TerminalArgs().StartingDirectory());
         }
     }
 }

src/cascadia/TerminalApp/AppCommandlineArgs.cpp

@@ -292,9 +292,16 @@ void AppCommandlineArgs::_addNewTerminalArgs(AppCommandlineArgs::NewTerminalSubc
     subcommand.startingDirectoryOption = subcommand.subcommand->add_option("-d,--startingDirectory",
                                                                            _startingDirectory,
                                                                            NEEDS_LOC("Open in the given directory instead of the profile's set startingDirectory"));
-    subcommand.commandlineOption = subcommand.subcommand->add_option("cmdline",
+
-                                                                     _commandline,
+    // Using positionals_at_end allows us to support "wt new-tab -d wsl -d Ubuntu"
-                                                                     NEEDS_LOC("Commandline to run in the given profile"));
+    // without CLI11 thinking that we've specified -d twice.
+    // There's an alternate construction where we make all subcommands "prefix commands",
+    // which lets us get all remaining non-option args provided at the end, but that
+    // doesn't support "wt new-tab -- wsl -d Ubuntu -- sleep 10" because the first
+    // -- breaks out of the subcommand (instead of the subcommand options).
+    // See https://github.com/CLIUtils/CLI11/issues/417 for more info.
+    subcommand.commandlineOption = subcommand.subcommand->add_option("command", _commandline, NEEDS_LOC("An optional command, with arguments, to be spawned in the new tab or pane"));
+    subcommand.subcommand->positionals_at_end(true);
 }
 
 // Method Description:
@@ -307,75 +314,29 @@ NewTerminalArgs AppCommandlineArgs::_getNewTerminalArgs(AppCommandlineArgs::NewT
 {
     auto args = winrt::make_self<implementation::NewTerminalArgs>();
 
-    // If a commandline was provided, we need to get a little tricky parsing
+    if (!_commandline.empty())
-    // here. The behavior we want is that once we see a string that we don't
-    // recognize, we want to treat the rest of the command as the commandline to
-    // pass to the NewTerminalArgs. However, if that commandline contains any
-    // options that _we_ understand, CLI11 will try to first grab those options
-    // from the provided string to parse our own args, and _not_ include them in
-    // the _commandline here. Consider for example, the following command:
-    //
-    //     wt.exe new-tab wsl.exe -d Alpine
-    //
-    // We want the commandline here to be "wsl.exe -d Alpine zsh". However, by
-    // default, that'll be parsed by CLI11 as a _startingDirectory of "Alpine",
-    // with a commandline of "wsl.exe zsh".
-    //
-    // So, instead, when we see that there's a commandlineOption that's been
-    // parsed, we'll use that as our cue to handle the commandline ourselves.
-    //  * We'll start by going through all the options that were parsed by
-    //    CLI11, and clearing all of the ones after the first commandline
-    //    option. This will prevent use from acting on their values later.
-    //  * Then, we'll grab all the strings starting with the first commandline
-    //    string, and add them to the commandline for the NewTerminalArgs.
-    if (*subcommand.commandlineOption)
     {
-        const std::vector<CLI::Option*>& opts = subcommand.subcommand->parse_order();
+        std::ostringstream cmdlineBuffer;
-        auto foundCommandlineStart = false;
-        for (auto opt : opts)
-        {
-            if (opt == subcommand.commandlineOption)
-            {
-                foundCommandlineStart = true;
-            }
-            else if (foundCommandlineStart)
-            {
-                opt->clear();
-            }
-            // otherwise, this option preceeded the start of the commandline
-        }
 
-        // Concatenate all the strings starting with the first arg in the
+        for (const auto& arg : _commandline)
-        // _commandline as the _real_ commandline.
-        const auto& firstCmdlineArg = _commandline.at(0);
-        auto foundFirstArg = false;
-        std::string fullCommandlineBuffer;
-        for (const auto& arg : _currentCommandline->Args())
         {
-            if (arg == firstCmdlineArg)
+            if (cmdlineBuffer.tellp() != 0)
             {
-                foundFirstArg = true;
+                // If there's already something in here, prepend a space
+                cmdlineBuffer << ' ';
             }
-            if (foundFirstArg)
+
+            if (arg.find(" ") != std::string::npos)
             {
-                if (arg.find(" ") != std::string::npos)
+                cmdlineBuffer << '"' << arg << '"';
-                {
+            }
-                    fullCommandlineBuffer += "\"";
+            else
-                    fullCommandlineBuffer += arg;
+            {
-                    fullCommandlineBuffer += "\"";
+                cmdlineBuffer << arg;
-                }
-                else
-                {
-                    fullCommandlineBuffer += arg;
-                }
-                fullCommandlineBuffer += " ";
             }
         }
 
-        // Discard the space from the last arg we appended.
+        args->Commandline(winrt::to_hstring(cmdlineBuffer.str()));
-        std::string_view realCommandline = fullCommandlineBuffer;
-        realCommandline = realCommandline.substr(0, static_cast<size_t>(fullCommandlineBuffer.size() - 1));
-        args->Commandline(winrt::to_hstring(realCommandline));
     }
 
     if (*subcommand.profileNameOption)

src/cascadia/TerminalApp/AppCommandlineArgs.h

@@ -64,8 +64,9 @@ private:
     std::string _profileName;
     std::string _startingDirectory;
 
-    // _commandline will receive the commandline as it's parsed by CLI11
+    // _commandline will contain the command line with which we'll be spawning a new terminal
     std::vector<std::string> _commandline;
+
     const Commandline* _currentCommandline{ nullptr };
 
     bool _splitVertical{ false };