Repository: microsoft/terminal (language: cpp)

Issue:
Command History should discard empty histories first before discarding LRU non-empty history
# Description of the new feature/enhancement

Scenario:
1) Run one of the dbgeng console debuggers (cdb.exe) to debug a program in a build window.
2) Exit the debugger.
3) Run a build (I'm using the Windows build system).
4) Relaunch the debugger and try to use the command history.

Behavior: No history for the debugger is preserved, since the executables in the build have flushed out the relevant history.

Desired Behavior: The executables that were part of the build had no input commands, while the console debugger had some input, so we should leave the console debugger's history in the list.

# Proposed technical implementation details (optional)

There are a couple implementations we could consider to avoid discarding useful history:
1) When picking a history in CommandHistory::s_Allocate, scan first for history buffers that aren't !CLE_ALLOCATED and have zero commands, and only discard a non-empty history if no candidate was found with zero commands.
2) Alternatively, lazily call s_Allocate when the first command is added to a history buffer.

Option 1 is a simpler implementation, but option 2 is nice in that we can use the full NumberOfHistoryBuffers for useful histories, rather than losing one buffer for empty commands cycling through.


Discussion / hints:
_Huh_. That's a good point.
now this is a commandline change I can get behind and can't POSSIBLY imagine that an oil rig is depending on. Thanks for the suggestion!

Fix patch:
diff --git a/src/host/history.cpp b/src/host/history.cpp
index 9e875b14a36..58ae5eba21c 100644
--- a/src/host/history.cpp
+++ b/src/host/history.cpp
@@ -343,7 +343,7 @@ CommandHistory* CommandHistory::s_Allocate(const std::wstring_view appName, cons
     {
         if (WI_IsFlagClear(it->Flags, CLE_ALLOCATED))
         {
-            // use LRU history buffer with same app name
+            // use MRU history buffer with same app name
             if (it->IsAppNameMatch(appName))
             {
                 BestCandidate = *it;
@@ -367,18 +367,28 @@ CommandHistory* CommandHistory::s_Allocate(const std::wstring_view appName, cons
         History._processHandle = processHandle;
         return &s_historyLists.emplace_front(History);
     }
-    else if (!BestCandidate.has_value() && s_historyLists.size() > 0)
+
+    // If we have no candidate already and we need one,
+    // take the LRU (which is the back/last one) which isn't allocated
+    // and if possible the one with empty commands list.
+    if (!BestCandidate.has_value())
     {
-        // If we have no candidate already and we need one, take the LRU (which is the back/last one) which isn't allocated.
-        for (auto it = s_historyLists.crbegin(); it != s_historyLists.crend(); it++)
+        auto BestCandidateIt = s_historyLists.cend();
+        for (auto it = s_historyLists.cbegin(); it != s_historyLists.cend(); it++)
         {
             if (WI_IsFlagClear(it->Flags, CLE_ALLOCATED))
             {
-                BestCandidate = *it;
-                s_historyLists.erase(std::next(it).base()); // trickery to turn reverse iterator into forward iterator for erase.
-                break;
+                if (it->_commands.empty() || BestCandidateIt == s_historyLists.cend() || !BestCandidateIt->_commands.empty())
+                {
+                    BestCandidateIt = it;
+                }
             }
         }
+        if (BestCandidateIt != s_historyLists.cend())
+        {
+            BestCandidate = *BestCandidateIt;
+            s_historyLists.erase(BestCandidateIt);
+        }
     }
 
     // If the app name doesn't match, copy in the new app name and free the old commands.


Test patch: