Stop regex commands from double entry into the history

[jingham]

Summary

At present, if you run a regex command (e.g. the b command) you will get two history entries, one for the actual command the user typed, and one for the resolved command:

(lldb) b foo.c:12
Breakpoint 1: no locations (pending).
Breakpoint set in dummy target, will get copied into future targets.
(lldb) history
   0: b foo.c:12
   1: breakpoint set --file 'foo.c' --line 12
   2: history

That seems wrong in principle, there shouldn't be double entry like this. But it also has some subtle side effects that are even less desirable. For instance, if you write a Python based command that calls "HandleCommand" of a regex command, then the resolved command gets inserted into the history list AFTER the python command the user actually ran. That in turn means that up-arrow doesn't rerun the command the user typed, but instead runs the resolved regex command.

Since regex command resolution isn't stateful - the same input string will always resolve to the same command - there's no actual reason to do this double entry, just storing the command the user typed will suffice. And there's already a way to see resolved commands, so you don't need it for this purpose either.

[jingham]

Note, the original code tries to finesse this by passing eLazyBoolCalculate, which then resolves to a check against the current command depth. But that check was wrong for SBDebugger.HandleCommand, which is considered at depth 0, and anyway, I don't think it ever makes sense to have the regex command add the resolved command to the history.

[JDevlieghere]

LGTM

[kastiglione]

thank you for fixing!

Q: why does this test have this thin test_X/do_x_test separation?

[jingham]

In D126789#3550572, @kastiglione wrote:

thank you for fixing!

Q: why does this test have this thin test_X/do_x_test separation?

We did that separation originally (and the sample test reflects that) so that you could see the list of tests in a file (with hopefully suggestive names) easily at the top, and then all the gritty details would be in the do_ methods below. It's also good to remind yourself that for unittest all the "test_" methods are special, you can't use that name for a helper function...

This format is not unique to this test, many of the tests (particularly older ones or ones that copied the sample test) do it that way. With tiny actual test bodies like these, it's unclear how much you gain by this, but the test was already mostly following that separation (except the test implementation name was just copied over from the sample test) I followed the practice of the file.

[clayborg]

lldb/source/Commands/CommandObjectRegexCommand.cpp

73–75

Is this comment out of date now? It mentions passing in "true"?

[jingham]

lldb/source/Commands/CommandObjectRegexCommand.cpp

73–75

No, that comment has to do with the Interpreter's ExecutionContext, not the history state.

[jingham]

lldb/source/Commands/CommandObjectRegexCommand.cpp

73–75

Or rather, I think it was obsoleted a while ago. What it's really explaining is why you don't need to call the version of HandleCommand that takes an override_context. I think that was controlled by a bool at some point in the past. I'll update the comment.

[jingham]

Fix a comment that hadn't been correct for a while...

lldb/source/Commands/CommandObjectRegexCommand.cpp

if (pos->regex.Execute(command, &matches)) {
           SubstituteVariables(pos->command, matches);
       if (!new_command) {
         result.SetError(new_command.takeError());
         return false;
       }
 
       // Interpret the new command and return this as the result!
       if (m_interpreter.GetExpandRegexAliases())
         result.GetOutputStream().Printf("%s\n", new_command->c_str());
-      // Pass in true for "no context switching".  The command that called us
-      // should have set up the context appropriately, we shouldn't have to
-      // redo that.
+      // We don't have to pass an override_context here, as the command that 
+      // called us should have set up the context appropriately.
       return m_interpreter.HandleCommand(new_command->c_str(),
-                                         eLazyBoolCalculate, result);
+                                         eLazyBoolNo, result);
     }
   }
   result.SetStatus(eReturnStatusFailed);
   if (!GetSyntax().empty())
     result.AppendError(GetSyntax());
   else
     result.GetErrorStream() << "Command contents '" << command
                             << "' failed to match any "

lldb/test/API/functionalities/history/TestHistoryRecall.py

 """
-Make sure the !N and !-N commands work properly.
+Test some features of "session history" and history recall.
 """
 
 
 
 import lldb
 import lldbsuite.test.lldbutil as lldbutil
 from lldbsuite.test.lldbtest import *
 
 
 class TestHistoryRecall(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
     # If your test case doesn't stress debug info, then
     # set this to true.  That way it won't be run once for
     # each debug info format.
     NO_DEBUG_INFO_TESTCASE = True
 
     def test_history_recall(self):
         """Test the !N and !-N functionality of the command interpreter."""
-        self.sample_test()
+        self.do_bang_N_test()
 
-    def sample_test(self):
+    def test_regex_history(self):
+        """Test the regex commands don't add two elements to the history"""
+        self.do_regex_history_test()
+
+    def do_regex_history_test(self):
+        interp = self.dbg.GetCommandInterpreter()
+        result = lldb.SBCommandReturnObject()
+        command = "_regexp-break foo.c:12"
+        self.runCmd(command, msg="Run the regex break command", inHistory = True)
+        interp.HandleCommand("session history", result, True)
+        self.assertTrue(result.Succeeded(), "session history ran successfully")
+        results = result.GetOutput()
+        self.assertIn(command, results, "Recorded the actual command")
+        self.assertNotIn("breakpoint set --file 'foo.c' --line 12", results,
+                         "Didn't record the resolved command")
+        
+    def do_bang_N_test(self):
         interp = self.dbg.GetCommandInterpreter()
         result = lldb.SBCommandReturnObject()
         interp.HandleCommand("session history", result, True)
         interp.HandleCommand("platform list", result, True)
 
         interp.HandleCommand("!0", result, False)
         self.assertTrue(result.Succeeded(), "!0 command did not work: %s"%(result.GetError()))
         self.assertIn("session history", result.GetOutput(), "!0 didn't rerun session history")
 
         interp.HandleCommand("!-1", result, False)
         self.assertTrue(result.Succeeded(), "!-1 command did not work: %s"%(result.GetError()))
         self.assertIn("host:", result.GetOutput(), "!-1 didn't rerun platform list.")