Improve history isolation

[Hofer-Julian]

After this change (with enabled history isolation), we will get rows:

• that have the same session_id, or
• have a lower id than the first command with our session_id, or
• if there's no command with our session_id, take the whole history.

See nushell/nushell#10104 (comment) for an example of the new behavior

[amtoine]

thanks @Hofer-Julian 🙏

that's the first time, so how would you test that in Nushell?
making sure the behaviour in this comment of mine is truely there with these changes 😋

[Hofer-Julian]

@amtoine I assume it wouldn't be too hard to test it in nushell.

But for now, I think it's fine to just run the reedline demo.

• Modify this line to true:

  reedline/examples/demo.rs

  Line 71 in b2b8f3f

  let history_per_session = false;

• Then run cargo run --example demo --features sqlite

[amtoine]

ooh this is very interesting 👌

[amtoine]

here are the steps i followed
firs, apply

diff --git a/examples/demo.rs b/examples/demo.rs
index af98c0b..2f40008 100644
--- a/examples/demo.rs
+++ b/examples/demo.rs
@@ -68,7 +68,7 @@ fn main() -> std::io::Result<()> {
 
     // Setting history_per_session to true will allow the history to be isolated to the current session
     // Setting history_per_session to false will allow the history to be shared across all sessions
-    let history_per_session = false;
+    let history_per_session = true;
     let mut history_session_id = if history_per_session {
         Reedline::create_history_session_id()
     } else {

1. open an instance A
2. run commandA in it
3. open two new instances B and C
4. ✔️ they both have access to commandA
5. run commandB and commandC in B and C respectively
6. ✔️ B does not have access to commandC
7. ❌ C has access to commandB
8. open a last instance D
9. ✔️ D has access to commandA, commandB and commandC

this is almost correct, right?

[Hofer-Julian]

Oh nice, thanks for testing.

Will see if I can reproduce and fix it.

[amtoine]

Will see if I can reproduce and fix it.

yeah at least if you can reproduce 😋

[fdncred]

I added this stuff in the original PR in order to help test the history session stuff. I'm not sure if it's still helpful or not. It's like adding 3 commands.

reedline/examples/demo.rs

Lines 169 to 191 in b2b8f3f

	// Get the history only pertinent to the current session
	if buffer.trim() == "history session" {
	line_editor.print_history_session()?;
	continue;
	}
	// Get this history session identifier
	if buffer.trim() == "history sessionid" {
	line_editor.print_history_session_id()?;
	continue;
	}
	// Toggle between the full history and the history pertinent to the current session
	if buffer.trim() == "toggle history_session" {
	let hist_session_id = if history_session_id.is_none() {
	// If we never created a history session ID, create one now
	let sesh = Reedline::create_history_session_id();
	history_session_id = sesh;
	sesh
	} else {
	history_session_id
	};
	line_editor.toggle_history_session_matching(hist_session_id)?;
	continue;
	}

[Hofer-Julian]

@amtoine I can reproduce the behavior you found, nice find! 😄

It only happens if you open two terminals at the same time. If you then start typing in both of them, the second one has access to the very first command that was typed in the first terminal. It is not the first one that is suggested, and all the other commands are still isolated.

I understand why it happens. However, with the current data in history.sqlite3, I cannot think of a way to avoid that. But because of how minimal the impact is, my gut feeling is that it's not worth introducing logic and maybe even a separate database for it.

[Hofer-Julian]

@fdncred I think your commands continue to be as useful as they were before. Feel free to remove some of them if you question their usefulness now.

[sholderbach]

With the one command I am wondering if this pathology is down to the hold one entry back behavior for the "ignore commands with leading space" feature?

[Hofer-Julian]

With the one command I am wondering if this pathology is down to the hold one entry back behavior for the "ignore commands with leading space" feature?

I think the problem is in the way I check for “old history”. I do that by finding commands that were executed before the first command with my session id. That is mostly, but not exactly the wanted behaviour as described above. The difference is that the first run command counts, not the time when the session was created.

We could workaround that I think by adding e.g. adding an empty history entry when the session is started. Not sure if that is necessary though.

[Hofer-Julian]

I've now stored the session_timestamp in SqliteBackedHistory, this should avoid the discrepancy: c8a163d

I've also noticed that it was relatively easy to enforce history_isolation for hints as well, so I've added that too: badf4d3

@amtoine maybe you can try it again and see if you can find more bugs :)

As a sidenote, I've noticed two things that should be cleaned up I think:

• the session_id can be set/toggled after the fact, but I don't think this is done thoroughly. I also wonder how useful that is. My feeling is that this functionality should be removed
• nit, but the session id differs in naming from session, to session_id and history_session_id. I think that should be unified

That is outside the scope of this PR though.

[fdncred]

I've now stored the session_timestamp in SqliteBackedHistory, this should avoid the discrepancy: c8a163d

Does this always mean that there is a valid session id, or can it still be stored as none in the db?

I've also noticed that it was relatively easy to enforce history_isolation for hints as well, so I've added that too: badf4d3

Good job! at the end of my original changes, I was so sick of messing with it, that I was like, "land it and let someone else improve it". Thanks for doing that.

the session_id can be set/toggled after the fact, but I don't think this is done thoroughly. I also wonder how useful that is. My feeling is that this functionality should be removed

I did this mostly for testing. But it also comes into play when history.isolation is a configuration point in nushell and you reconfigure and source your config without restarting the shell. e.g. I have this keybinding in nushell for F5

  {
    name: reload_config
    modifier: none
    keycode: f5
    mode: [ emacs vi_insert vi_normal ]
    event: [
      { edit: clear }
      { send: executehostcommand cmd: $"source ($nu.env-path); source ($nu.config-path)" }
      # {
      #   edit: insertString
      #   value: $"source ($nu.env-path); source ($nu.config-path)"
      # }
      # { send: Enter }
    ]
  }

nit, but the session id differs in naming from session, to session_id and history_session_id. I think that should be unified

I'm guessing this is my fault too. I think originally just named things whatever and then at the end went for the verbose history_session_id so there was no confusion. I'm fine with them all being named the verbose way if they're indeed all the same thing.

[Hofer-Julian]

Does this always mean that there is a valid session id, or can it still be stored as none in the db?

It can still be stored as none. History isolation is now only present if a session_timestamp is provided as well.

I did this mostly for testing. But it also comes into play when history.isolation is a configuration point in nushell and you reconfigure and source your config without restarting the shell. e.g. I have this keybinding in nushell for F5

That's useful to know

[amtoine]

@Hofer-Julian
i think the steps above work now 👌

at least i do not see commandB in instance C now i think 😋

[Hofer-Julian]

Thanks for checking @amtoine!

From my side this PR is ready for review then.

[Hofer-Julian]

Friendly reminder to check this PR :)
Not sure whether @sholderbach or @fdncred is typically the one to review this part of the code.

[fdncred]

We're two days from the 0.85.0 release. The question is do we think it's stable enough and working the way it needs to in order to land a PR this close to the release? I always approach releases with an abundance of caution. I could be convinced to land it though.

[Hofer-Julian]

We're two days from the 0.85.0 release. The question is do we think it's stable enough and working the way it needs to in order to land a PR this close to the release? I always approach releases with an abundance of caution. I could be convinced to land it though.

It will need (minor) adaptions for nushell as well. I am happy to provide a PR for that as soon as it is merged.
Tbh, I wouldn't be too worried about breakage. Sqlite is not the default and neither is history isolation.

Merging it would allow us to test for one release cycle (or more if needed) to see whether it is good enough to be the default.

[fdncred]

I'm ok with landing. What do you think @sholderbach?

[sholderbach]

I am OK with both landing and waiting here. The changes look fine in the code and @amtoine's testing seems to have succeeded. As you @Hofer-Julian have the patch for nushell ready we could go ahead. My risk tolerance is a bit higher for the features that are currently broken anyways.

At the same time both the nushell REPL and the edge cases of the SqliteHistory are undertested when it comes to any CI watching.

But the Nushell change doesn't risk to introduce any panic if the timestamp is weird etc. The only folks affected would be those that explicitly use the feature. So a green light from my side.

[fdncred]

Thanks @Hofer-Julian.