Repository: microsoft/terminal (language: cpp)

Issue:
profiles.defaults should be default values, not override values, or make a new section for overridable defaults
The current implementation of Terminal applies the settings found in profiles.default to all profiles, regardless of whether the individual profile has its own value for any of those settings.  Unfortunately, this behavior is unexpected and confusing.  The only reasoning I can come up with is that profiles.default was meant to override settings from automatic profiles that aren't defined in settings.json at all.  For profiles defined in settings.json, the expectation for profiles.default would be that any setting not defined in an individual profile would get its value from profiles.default, but if a setting *is* provided in the individual profile the corresponding setting in profiles.default would be ignored.

Please change Terminal so profiles.default behaves in an expected way, providing default values and not override values.  If an overriding section is really needed, then create a new profiles.overrides section for this.  With the current implementation, it is not possible to define several profiles and have most of them use a common value for some settings, while one or two exception profiles do something different without copying that setting to every individual profile.  profiles.default has very little value when it forces itself onto every profile.
If profiles.default can't change for some reason, then consider creating a new section (maybe profiles.fallback) for overridable defaults or fallback values that individual profiles can override if they provide a value for that setting in the profile.


Discussion / hints:
What version are you on? Could you share a settings snippet that repros what you're seeing?

In general, settings from `profiles.defaults` should override ones from dynamic profiles (e.g. WSL distros) and fragments (like, customizations that Canonical does to their WSL distro), but not ones from the rest of your settings file (the rest of the profiles in `profiles.list`)
Agree on the above^
The thing you've described as the expected behavior _is how `profiles.defaults` works_:

<img width="593" alt="image" src="https://user-images.githubusercontent.com/189190/162022748-2b6c1bbd-54fd-4a1d-bd9c-31436ab27505.png">

Is that true for all settings?  It sure doesn't seem to be how commandLine behaves.
Settings are case-sensitive; are you overriding `commandline` with `commandLine`?

<img width="742" alt="image" src="https://user-images.githubusercontent.com/189190/162023340-ee6b116d-6642-4d56-8302-95068e3ef063.png">

![image](https://user-images.githubusercontent.com/18356694/162023304-9efedb5e-33c0-4488-9b45-f96c655e309d.png)


did you set `commandline` or `commandLine` (with a capital `L`)?
No, that was just a typo in this issue.  My settings are correct.
Now join me on the path to weirdness...
Did some experimentation with this and found that it isn't working for me because I am referencing cmd.exe and powershell.exe without a path (since they are in the system path).  If I change the commandline value to include the path, then it works.  It doesn't matter if the commandline value under defaults has a path or not.

This profile definition will use pwsh.exe for both profiles, but if you switch to the commented-out commandline values, it will work.
```
    "profiles": 
    {
        "defaults":
        {
            "commandline": "pwsh.exe"
        },
        "list": 
        [
            {
                // "commandline": "%SystemRoot%\\System32\\WindowsPowerShell\\v1.0\\powershell.exe",
                "commandline": "powershell.exe",
                "guid": "{61c54bbd-c2c6-5271-96e7-009a87ff44bf}",
                "hidden": false,
                "name": "Windows PowerShell"
            },
            {
                // "commandline": "%SystemRoot%\\System32\\cmd.exe",
                "commandline": "cmd.exe",
                "guid": "{0caa0dad-35be-5f56-a8ff-afceeeaa6101}",
                "hidden": false,
                "name": "Command Prompt"
            }
        ]
    }
```

So this is sounding more like a bug to me.
Frick. I might have been the killer -- through @zadjii-msft's hands. https://github.com/microsoft/terminal/pull/12149/files/ba37cd0259bff7585eae1b1161657750d4629de8#r783387620
oh, you were trying to be clever?  That always gets me into trouble...

Fix patch:
diff --git a/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp b/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp
index 6675901de4d..fdb160fe275 100644
--- a/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp
+++ b/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp
@@ -40,7 +40,6 @@ namespace SettingsModelLocalTests
         TEST_METHOD(LayerProfilesOnArray);
         TEST_METHOD(DuplicateProfileTest);
         TEST_METHOD(TestGenGuidsForProfiles);
-
         TEST_METHOD(TestCorrectOldDefaultShellPaths);
     };
 
@@ -367,31 +366,43 @@ namespace SettingsModelLocalTests
             ]
         })" };
         static constexpr std::string_view userProfiles{ R"({
-            "profiles": [
-                {
-                    "name" : "powershell 1",
-                    "commandline": "powershell.exe",
-                    "guid" : "{61c54bbd-c2c6-5271-96e7-009a87ff44bf}"
-                },
-                {
-                    "name" : "powershell 2",
-                    "commandline": "powershell.exe",
-                    "guid" : "{61c54bbd-0000-5271-96e7-009a87ff44bf}"
-                },
+            "profiles": {
+                "defaults":
                 {
-                    "name" : "cmd 1",
-                    "commandline": "cmd.exe",
-                    "guid" : "{0caa0dad-35be-5f56-a8ff-afceeeaa6101}"
+                    "commandline": "pwsh.exe"
                 },
-                {
-                    "name" : "cmd 2",
-                    "commandline": "cmd.exe",
-                    "guid" : "{0caa0dad-0000-5f56-a8ff-afceeeaa6101}"
-                }
-            ]
+                "list":
+                [
+                    {
+                        "name" : "powershell 1",
+                        "commandline": "powershell.exe",
+                        "guid" : "{61c54bbd-c2c6-5271-96e7-009a87ff44bf}"
+                    },
+                    {
+                        "name" : "powershell 2",
+                        "commandline": "powershell.exe",
+                        "guid" : "{61c54bbd-0000-5271-96e7-009a87ff44bf}"
+                    },
+                    {
+                        "name" : "cmd 1",
+                        "commandline": "cmd.exe",
+                        "guid" : "{0caa0dad-35be-5f56-a8ff-afceeeaa6101}"
+                    },
+                    {
+                        "name" : "cmd 2",
+                        "commandline": "cmd.exe",
+                        "guid" : "{0caa0dad-0000-5f56-a8ff-afceeeaa6101}"
+                    }
+                ]
+            }
         })" };
 
-        const auto settings = winrt::make_self<implementation::CascadiaSettings>(userProfiles, inboxProfiles);
+        implementation::SettingsLoader loader{ userProfiles, inboxProfiles };
+        loader.MergeInboxIntoUserSettings();
+        loader.FinalizeLayering();
+        loader.FixupUserSettings();
+
+        const auto settings = winrt::make_self<implementation::CascadiaSettings>(std::move(loader));
         const auto allProfiles = settings->AllProfiles();
         VERIFY_ARE_EQUAL(4u, allProfiles.Size());
         VERIFY_ARE_EQUAL(L"powershell 1", allProfiles.GetAt(0).Name());
diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
index 55456867f91..375899ee01d 100644
--- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
+++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
@@ -404,7 +404,6 @@ winrt::com_ptr<Profile> CascadiaSettings::_createNewProfile(const std::wstring_v
 // - <none>
 void CascadiaSettings::_validateSettings()
 {
-    _validateCorrectDefaultShellPaths();
     _validateAllSchemesExist();
     _validateMediaResources();
     _validateKeybindings();
diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h
index 381a5148ce6..daa724a0f4c 100644
--- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h
+++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h
@@ -60,6 +60,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         void MergeFragmentIntoUserSettings(const winrt::hstring& source, const std::string_view& content);
         void FinalizeLayering();
         bool DisableDeletedProfiles();
+        bool FixupUserSettings();
 
         ParsedSettings inboxSettings;
         ParsedSettings userSettings;
@@ -155,7 +156,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         void _validateMediaResources();
         void _validateKeybindings() const;
         void _validateColorSchemesInCommands() const;
-        void _validateCorrectDefaultShellPaths() const;
         bool _hasInvalidColorScheme(const Model::Command& command) const;
 
         // user settings
diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
index 6d50e3990b9..7cce19fe736 100644
--- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
+++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
@@ -334,6 +334,9 @@ void SettingsLoader::FinalizeLayering()
 // This section of code recognizes if a profile was seen before and marks it as
 // `"hidden": true` by default and thus ensures the behavior the user expects:
 // Profiles won't show up again after they've been removed from settings.json.
+//
+// Returns true if something got changed and
+// the settings need to be saved to disk.
 bool SettingsLoader::DisableDeletedProfiles()
 {
     const auto& state = winrt::get_self<ApplicationState>(ApplicationState::SharedInstance());
@@ -361,6 +364,56 @@ bool SettingsLoader::DisableDeletedProfiles()
     return newGeneratedProfiles;
 }
 
+// Runs migrations and fixups on user settings.
+// Returns true if something got changed and
+// the settings need to be saved to disk.
+bool SettingsLoader::FixupUserSettings()
+{
+    struct CommandlinePatch
+    {
+        winrt::guid guid;
+        std::wstring_view before;
+        std::wstring_view after;
+    };
+
+    static constexpr std::array commandlinePatches{
+        CommandlinePatch{ DEFAULT_COMMAND_PROMPT_GUID, L"cmd.exe", L"%SystemRoot%\\System32\\cmd.exe" },
+        CommandlinePatch{ DEFAULT_WINDOWS_POWERSHELL_GUID, L"powershell.exe", L"%SystemRoot%\\System32\\WindowsPowerShell\\v1.0\\powershell.exe" },
+    };
+
+    bool fixedUp = false;
+
+    for (const auto& profile : userSettings.profiles)
+    {
+        if (!profile->HasCommandline())
+        {
+            continue;
+        }
+
+        for (const auto& patch : commandlinePatches)
+        {
+            if (profile->Guid() == patch.guid && til::equals_insensitive_ascii(profile->Commandline(), patch.before))
+            {
+                profile->ClearCommandline();
+
+                // GH#12842:
+                // With the commandline field on the user profile gone, it's actually unknown what
+                // commandline it'll inherit, since a user profile can have multiple parents. We have to
+                // make sure we restore the correct commandline in case we don't inherit the expected one.
+                if (profile->Commandline() != patch.after)
+                {
+                    profile->Commandline(winrt::hstring{ patch.after });
+                }
+
+                fixedUp = true;
+                break;
+            }
+        }
+    }
+
+    return fixedUp;
+}
+
 // Give a string of length N and a position of [0,N) this function returns
 // the line/column within the string, similar to how text editors do it.
 // Newlines are considered part of the current line (as per POSIX).
@@ -750,9 +803,9 @@ try
     loader.FinalizeLayering();
 
     // DisableDeletedProfiles returns true whenever we encountered any new generated/dynamic profiles.
-    // Coincidentally this is also the time we should write the new settings.json
-    // to disk (so that it contains the new profiles for manual editing by the user).
+    // Similarly FixupUserSettings returns true, when it encountered settings that were patched up.
     mustWriteToDisk |= loader.DisableDeletedProfiles();
+    mustWriteToDisk |= loader.FixupUserSettings();
 
     // If this throws, the app will catch it and use the default settings.
     const auto settings = winrt::make_self<CascadiaSettings>(std::move(loader));
@@ -1024,20 +1077,3 @@ void CascadiaSettings::_resolveDefaultProfile() const
     // Use the first profile as the new default.
     GlobalSettings().DefaultProfile(_allProfiles.GetAt(0).Guid());
 }
-
-void CascadiaSettings::_validateCorrectDefaultShellPaths() const
-{
-    for (auto profile : _allProfiles)
-    {
-        if (profile.Guid() == DEFAULT_COMMAND_PROMPT_GUID &&
-            profile.Commandline() == L"cmd.exe")
-        {
-            profile.ClearCommandline();
-        }
-        else if (profile.Guid() == DEFAULT_WINDOWS_POWERSHELL_GUID &&
-                 profile.Commandline() == L"powershell.exe")
-        {
-            profile.ClearCommandline();
-        }
-    }
-}


Test patch: