From 6c9d73f3be8ff75e0b74630344674e8e15a21e06 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= <clement.nerma@gmail.com>
Date: Thu, 7 Dec 2023 13:07:58 +0100
Subject: [PATCH 01/18] Improve history API

---
 Cargo.lock                   | 30 +++++++++++++
 Cargo.toml                   |  4 ++
 examples/cwd_aware_hinter.rs | 23 ++++++----
 src/engine.rs                | 35 +++++++++------
 src/history/base.rs          | 55 ++++++++++++++----------
 src/history/cursor.rs        | 82 +++++++++++++++++++-----------------
 src/history/file_backed.rs   | 32 ++++++++------
 src/history/item.rs          |  6 +--
 src/history/sqlite_backed.rs | 33 +++++++++------
 9 files changed, 192 insertions(+), 108 deletions(-)

diff --git a/Cargo.lock b/Cargo.lock
index 5acc8438..0d0aa567 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -294,6 +294,17 @@ dependencies = [
  "windows-targets",
 ]
 
+[[package]]
+name = "getrandom"
+version = "0.2.10"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "be4136b2a15dd319360be1c07d9933517ccf0be8f16bf62a3bee4f0d618df427"
+dependencies = [
+ "cfg-if",
+ "libc",
+ "wasi",
+]
+
 [[package]]
 name = "glob"
 version = "0.3.1"
@@ -557,6 +568,24 @@ dependencies = [
  "proc-macro2",
 ]
 
+[[package]]
+name = "rand"
+version = "0.8.5"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "34af8d1a0e25924bc5b7c43c079c942339d8f0a8b57c39049bef581b46327404"
+dependencies = [
+ "rand_core",
+]
+
+[[package]]
+name = "rand_core"
+version = "0.6.4"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "ec0be4795e2f6a28069bec0b5ff3e2ac9bafc99e6a9a7dc3547996c5c816922c"
+dependencies = [
+ "getrandom",
+]
+
 [[package]]
 name = "redox_syscall"
 version = "0.3.5"
@@ -579,6 +608,7 @@ dependencies = [
  "itertools",
  "nu-ansi-term",
  "pretty_assertions",
+ "rand",
  "rstest",
  "rusqlite",
  "serde",
diff --git a/Cargo.toml b/Cargo.toml
index ca1bc5ad..0d3a7683 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -23,6 +23,10 @@ crossterm = { version = "0.27.0", features = ["serde"] }
 fd-lock = "3.0.3"
 itertools = "0.10.3"
 nu-ansi-term = "0.49.0"
+rand = { version = "0.8.5", default-features = false, features = [
+    "small_rng",
+    "getrandom",
+] }
 rusqlite = { version = "0.29.0", optional = true }
 serde = { version = "1.0", features = ["derive"] }
 serde_json = { version = "1.0.79", optional = true }
diff --git a/examples/cwd_aware_hinter.rs b/examples/cwd_aware_hinter.rs
index b8b9145d..c5592fb4 100644
--- a/examples/cwd_aware_hinter.rs
+++ b/examples/cwd_aware_hinter.rs
@@ -6,14 +6,21 @@
 // pressing "a" hints to abc.
 // Up/Down or Ctrl p/n, to select next/previous match
 
-use std::io;
+use std::{
+    io,
+    sync::atomic::{AtomicI64, Ordering},
+};
+
+use reedline::HistoryItemId;
+
+static COUNTER: AtomicI64 = AtomicI64::new(0);
 
 fn create_item(cwd: &str, cmd: &str, exit_status: i64) -> reedline::HistoryItem {
     use std::time::Duration;
 
     use reedline::HistoryItem;
     HistoryItem {
-        id: None,
+        id: HistoryItemId(COUNTER.fetch_add(1, Ordering::SeqCst)),
         start_timestamp: None,
         command_line: cmd.to_string(),
         session_id: None,
@@ -32,13 +39,13 @@ fn create_filled_example_history(home_dir: &str, orig_dir: &str) -> Box<dyn reed
     #[cfg(any(feature = "sqlite", feature = "sqlite-dynlib"))]
     let mut history = Box::new(reedline::SqliteBackedHistory::in_memory().unwrap());
 
-    history.save(create_item(orig_dir, "dummy", 0)).unwrap(); // add dummy item so ids start with 1
-    history.save(create_item(orig_dir, "ls /usr", 0)).unwrap();
-    history.save(create_item(orig_dir, "pwd", 0)).unwrap();
+    history.save(&create_item(orig_dir, "dummy", 0)).unwrap(); // add dummy item so ids start with 1
+    history.save(&create_item(orig_dir, "ls /usr", 0)).unwrap();
+    history.save(&create_item(orig_dir, "pwd", 0)).unwrap();
 
-    history.save(create_item(home_dir, "cat foo", 0)).unwrap();
-    history.save(create_item(home_dir, "ls bar", 0)).unwrap();
-    history.save(create_item(home_dir, "rm baz", 0)).unwrap();
+    history.save(&create_item(home_dir, "cat foo", 0)).unwrap();
+    history.save(&create_item(home_dir, "ls bar", 0)).unwrap();
+    history.save(&create_item(home_dir, "rm baz", 0)).unwrap();
 
     history
 }
diff --git a/src/engine.rs b/src/engine.rs
index e4ca9c7e..075eeda9 100644
--- a/src/engine.rs
+++ b/src/engine.rs
@@ -1730,6 +1730,7 @@ impl Reedline {
     fn submit_buffer(&mut self, prompt: &dyn Prompt) -> io::Result<EventStatus> {
         let buffer = self.editor.get_buffer().to_string();
         self.hide_hints = true;
+
         // Additional repaint to show the content without hints etc.
         if let Some(transient_prompt) = self.transient_prompt.take() {
             self.repaint(transient_prompt.as_ref())?;
@@ -1737,25 +1738,33 @@ impl Reedline {
         } else {
             self.repaint(prompt)?;
         }
-        if !buffer.is_empty() {
-            let mut entry = HistoryItem::from_command_line(&buffer);
-            entry.session_id = self.get_history_session_id();
 
-            if self
+        if !buffer.is_empty() {
+            let filtered = self
                 .history_exclusion_prefix
                 .as_ref()
-                .map(|prefix| buffer.starts_with(prefix))
-                .unwrap_or(false)
-            {
-                entry.id = Some(Self::FILTERED_ITEM_ID);
-                self.history_last_run_id = entry.id;
-                self.history_excluded_item = Some(entry);
+                .map_or(false, |prefix| buffer.starts_with(prefix));
+
+            let entry_id = if filtered {
+                Self::FILTERED_ITEM_ID
+            } else {
+                self.history.generate_id()
+            };
+
+            let mut entry = HistoryItem::from_command_line(&buffer, entry_id);
+
+            entry.session_id = self.get_history_session_id();
+
+            if filtered {
+                self.history.replace(&entry).expect("todo: error handling");
             } else {
-                entry = self.history.save(entry).expect("todo: error handling");
-                self.history_last_run_id = entry.id;
-                self.history_excluded_item = None;
+                self.history.save(&entry).expect("todo: error handling");
             }
+
+            self.history_last_run_id = Some(entry_id);
+            self.history_excluded_item = if filtered { Some(entry) } else { None };
         }
+
         self.run_edit_commands(&[EditCommand::Clear]);
         self.editor.reset_undo_stack();
 
diff --git a/src/history/base.rs b/src/history/base.rs
index 26d4da05..17c09c94 100644
--- a/src/history/base.rs
+++ b/src/history/base.rs
@@ -186,10 +186,15 @@ impl SearchQuery {
 /// Represents a history file or database
 /// Data could be stored e.g. in a plain text file, in a `JSONL` file, in a `SQLite` database
 pub trait History: Send {
-    /// save a history item to the database
-    /// if given id is None, a new id is created and set in the return value
-    /// if given id is Some, the existing entry is updated
-    fn save(&mut self, h: HistoryItem) -> Result<HistoryItem>;
+    /// generate a unique identifier for a new [`HistoryItem`]
+    fn generate_id(&mut self) -> HistoryItemId;
+
+    /// save a new history item to the database
+    fn save(&mut self, h: &HistoryItem) -> Result<()>;
+
+    /// replace an existing history item in the database
+    fn replace(&mut self, h: &HistoryItem) -> Result<()>;
+
     /// load a history item by its id
     fn load(&self, id: HistoryItemId) -> Result<HistoryItem>;
 
@@ -227,11 +232,18 @@ mod test {
     #[cfg(not(any(feature = "sqlite", feature = "sqlite-dynlib")))]
     const IS_FILE_BASED: bool = true;
 
+    use std::{
+        sync::atomic::{AtomicI64, Ordering},
+        time::Duration,
+    };
+
     use crate::HistorySessionId;
 
+    static COUNTER: AtomicI64 = AtomicI64::new(0);
+
     fn create_item(session: i64, cwd: &str, cmd: &str, exit_status: i64) -> HistoryItem {
         HistoryItem {
-            id: None,
+            id: HistoryItemId(COUNTER.fetch_add(1, Ordering::SeqCst)),
             start_timestamp: None,
             command_line: cmd.to_string(),
             session_id: Some(HistorySessionId::new(session)),
@@ -242,7 +254,6 @@ mod test {
             more_info: None,
         }
     }
-    use std::time::Duration;
 
     use super::*;
     fn create_filled_example_history() -> Result<Box<dyn History>> {
@@ -251,20 +262,20 @@ mod test {
         #[cfg(not(any(feature = "sqlite", feature = "sqlite-dynlib")))]
         let mut history = crate::FileBackedHistory::default();
         #[cfg(not(any(feature = "sqlite", feature = "sqlite-dynlib")))]
-        history.save(create_item(1, "/", "dummy", 0))?; // add dummy item so ids start with 1
-        history.save(create_item(1, "/home/me", "cd ~/Downloads", 0))?; // 1
-        history.save(create_item(1, "/home/me/Downloads", "unzp foo.zip", 1))?; // 2
-        history.save(create_item(1, "/home/me/Downloads", "unzip foo.zip", 0))?; // 3
-        history.save(create_item(1, "/home/me/Downloads", "cd foo", 0))?; // 4
-        history.save(create_item(1, "/home/me/Downloads/foo", "ls", 0))?; // 5
-        history.save(create_item(1, "/home/me/Downloads/foo", "ls -alh", 0))?; // 6
-        history.save(create_item(1, "/home/me/Downloads/foo", "cat x.txt", 0))?; // 7
-
-        history.save(create_item(1, "/home/me", "cd /etc/nginx", 0))?; // 8
-        history.save(create_item(1, "/etc/nginx", "ls -l", 0))?; // 9
-        history.save(create_item(1, "/etc/nginx", "vim nginx.conf", 0))?; // 10
-        history.save(create_item(1, "/etc/nginx", "vim htpasswd", 0))?; // 11
-        history.save(create_item(1, "/etc/nginx", "cat nginx.conf", 0))?; // 12
+        history.save(&create_item(1, "/", "dummy", 0))?; // add dummy item so ids start with 1
+        history.save(&create_item(1, "/home/me", "cd ~/Downloads", 0))?; // 1
+        history.save(&create_item(1, "/home/me/Downloads", "unzp foo.zip", 1))?; // 2
+        history.save(&create_item(1, "/home/me/Downloads", "unzip foo.zip", 0))?; // 3
+        history.save(&create_item(1, "/home/me/Downloads", "cd foo", 0))?; // 4
+        history.save(&create_item(1, "/home/me/Downloads/foo", "ls", 0))?; // 5
+        history.save(&create_item(1, "/home/me/Downloads/foo", "ls -alh", 0))?; // 6
+        history.save(&create_item(1, "/home/me/Downloads/foo", "cat x.txt", 0))?; // 7
+
+        history.save(&create_item(1, "/home/me", "cd /etc/nginx", 0))?; // 8
+        history.save(&create_item(1, "/etc/nginx", "ls -l", 0))?; // 9
+        history.save(&create_item(1, "/etc/nginx", "vim nginx.conf", 0))?; // 10
+        history.save(&create_item(1, "/etc/nginx", "vim htpasswd", 0))?; // 11
+        history.save(&create_item(1, "/etc/nginx", "cat nginx.conf", 0))?; // 12
         Ok(Box::new(history))
     }
 
@@ -409,8 +420,8 @@ mod test {
 
         // create history, add a few entries
         let mut history = open_history();
-        history.save(create_item(1, "/home/me", "cd ~/Downloads", 0))?; // 1
-        history.save(create_item(1, "/home/me/Downloads", "unzp foo.zip", 1))?; // 2
+        history.save(&create_item(1, "/home/me", "cd ~/Downloads", 0))?; // 1
+        history.save(&create_item(1, "/home/me/Downloads", "unzp foo.zip", 1))?; // 2
         assert_eq!(history.count_all()?, 2);
         drop(history);
 
diff --git a/src/history/cursor.rs b/src/history/cursor.rs
index ab41ba4f..4fc34473 100644
--- a/src/history/cursor.rs
+++ b/src/history/cursor.rs
@@ -67,7 +67,7 @@ impl HistoryCursor {
             // if searching forward but we don't have a starting point, assume we are at the end
             return Ok(());
         }
-        let start_id = self.current.as_ref().and_then(|e| e.id);
+        let start_id = self.current.as_ref().map(|e| e.id);
         let mut next = history.search(SearchQuery {
             start_id,
             end_id: None,
@@ -108,6 +108,13 @@ mod tests {
     use super::super::*;
     use super::*;
 
+    fn save_from_command_line(hist: &mut dyn History, cmd_line: impl Into<String>) -> Result<()> {
+        let id = hist.generate_id();
+        let entry = HistoryItem::from_command_line(cmd_line, id);
+
+        hist.save(&entry)
+    }
+
     fn create_history() -> (Box<dyn History>, HistoryCursor) {
         #[cfg(any(feature = "sqlite", feature = "sqlite-dynlib"))]
         let hist = Box::new(SqliteBackedHistory::in_memory().unwrap());
@@ -118,6 +125,7 @@ mod tests {
             HistoryCursor::new(HistoryNavigationQuery::Normal(LineBuffer::default()), None),
         )
     }
+
     fn create_history_at(cap: usize, path: &Path) -> (Box<dyn History>, HistoryCursor) {
         let hist = Box::new(FileBackedHistory::with_file(cap, path.to_owned()).unwrap());
         (
@@ -133,10 +141,10 @@ mod tests {
         let actual: Vec<_> = res.iter().map(|e| e.command_line.to_string()).collect();
         actual
     }
+
     fn add_text_entries(hist: &mut dyn History, entries: &[impl AsRef<str>]) {
         entries.iter().for_each(|e| {
-            hist.save(HistoryItem::from_command_line(e.as_ref()))
-                .unwrap();
+            save_from_command_line(hist, e.as_ref()).unwrap();
         });
     }
 
@@ -166,8 +174,8 @@ mod tests {
     #[test]
     fn going_backwards_bottoms_out() -> Result<()> {
         let (mut hist, mut cursor) = create_history();
-        hist.save(HistoryItem::from_command_line("command1"))?;
-        hist.save(HistoryItem::from_command_line("command2"))?;
+        save_from_command_line(hist.as_mut(), "command1")?;
+        save_from_command_line(hist.as_mut(), "command2")?;
         cursor.back(&*hist)?;
         cursor.back(&*hist)?;
         cursor.back(&*hist)?;
@@ -180,8 +188,8 @@ mod tests {
     #[test]
     fn going_forwards_bottoms_out() -> Result<()> {
         let (mut hist, mut cursor) = create_history();
-        hist.save(HistoryItem::from_command_line("command1"))?;
-        hist.save(HistoryItem::from_command_line("command2"))?;
+        save_from_command_line(hist.as_mut(), "command1")?;
+        save_from_command_line(hist.as_mut(), "command2")?;
         cursor.forward(&*hist)?;
         cursor.forward(&*hist)?;
         cursor.forward(&*hist)?;
@@ -195,10 +203,10 @@ mod tests {
     #[test]
     fn appends_only_unique() -> Result<()> {
         let (mut hist, _) = create_history();
-        hist.save(HistoryItem::from_command_line("unique_old"))?;
-        hist.save(HistoryItem::from_command_line("test"))?;
-        hist.save(HistoryItem::from_command_line("test"))?;
-        hist.save(HistoryItem::from_command_line("unique"))?;
+        save_from_command_line(hist.as_mut(), "unique_old")?;
+        save_from_command_line(hist.as_mut(), "test")?;
+        save_from_command_line(hist.as_mut(), "test")?;
+        save_from_command_line(hist.as_mut(), "unique")?;
         assert_eq!(hist.count_all()?, 3);
         Ok(())
     }
@@ -206,9 +214,9 @@ mod tests {
     #[test]
     fn prefix_search_works() -> Result<()> {
         let (mut hist, _) = create_history();
-        hist.save(HistoryItem::from_command_line("find me as well"))?;
-        hist.save(HistoryItem::from_command_line("test"))?;
-        hist.save(HistoryItem::from_command_line("find me"))?;
+        save_from_command_line(hist.as_mut(), "find me as well")?;
+        save_from_command_line(hist.as_mut(), "test")?;
+        save_from_command_line(hist.as_mut(), "find me")?;
 
         let mut cursor = HistoryCursor::new(
             HistoryNavigationQuery::PrefixSearch("find".to_string()),
@@ -228,9 +236,9 @@ mod tests {
     #[test]
     fn prefix_search_bottoms_out() -> Result<()> {
         let (mut hist, _) = create_history();
-        hist.save(HistoryItem::from_command_line("find me as well"))?;
-        hist.save(HistoryItem::from_command_line("test"))?;
-        hist.save(HistoryItem::from_command_line("find me"))?;
+        save_from_command_line(hist.as_mut(), "find me as well")?;
+        save_from_command_line(hist.as_mut(), "test")?;
+        save_from_command_line(hist.as_mut(), "find me")?;
 
         let mut cursor = HistoryCursor::new(
             HistoryNavigationQuery::PrefixSearch("find".to_string()),
@@ -256,9 +264,9 @@ mod tests {
     #[test]
     fn prefix_search_returns_to_none() -> Result<()> {
         let (mut hist, _) = create_history();
-        hist.save(HistoryItem::from_command_line("find me as well"))?;
-        hist.save(HistoryItem::from_command_line("test"))?;
-        hist.save(HistoryItem::from_command_line("find me"))?;
+        save_from_command_line(hist.as_mut(), "find me as well")?;
+        save_from_command_line(hist.as_mut(), "test")?;
+        save_from_command_line(hist.as_mut(), "find me")?;
 
         let mut cursor = HistoryCursor::new(
             HistoryNavigationQuery::PrefixSearch("find".to_string()),
@@ -283,10 +291,10 @@ mod tests {
     #[test]
     fn prefix_search_ignores_consecutive_equivalent_entries_going_backwards() -> Result<()> {
         let (mut hist, _) = create_history();
-        hist.save(HistoryItem::from_command_line("find me as well"))?;
-        hist.save(HistoryItem::from_command_line("find me once"))?;
-        hist.save(HistoryItem::from_command_line("test"))?;
-        hist.save(HistoryItem::from_command_line("find me once"))?;
+        save_from_command_line(hist.as_mut(), "find me as well")?;
+        save_from_command_line(hist.as_mut(), "find me once")?;
+        save_from_command_line(hist.as_mut(), "test")?;
+        save_from_command_line(hist.as_mut(), "find me once")?;
 
         let mut cursor = HistoryCursor::new(
             HistoryNavigationQuery::PrefixSearch("find".to_string()),
@@ -305,10 +313,10 @@ mod tests {
     #[test]
     fn prefix_search_ignores_consecutive_equivalent_entries_going_forwards() -> Result<()> {
         let (mut hist, _) = create_history();
-        hist.save(HistoryItem::from_command_line("find me once"))?;
-        hist.save(HistoryItem::from_command_line("test"))?;
-        hist.save(HistoryItem::from_command_line("find me once"))?;
-        hist.save(HistoryItem::from_command_line("find me as well"))?;
+        save_from_command_line(hist.as_mut(), "find me once")?;
+        save_from_command_line(hist.as_mut(), "test")?;
+        save_from_command_line(hist.as_mut(), "find me once")?;
+        save_from_command_line(hist.as_mut(), "find me as well")?;
 
         let mut cursor = HistoryCursor::new(
             HistoryNavigationQuery::PrefixSearch("find".to_string()),
@@ -335,11 +343,11 @@ mod tests {
     #[test]
     fn substring_search_works() -> Result<()> {
         let (mut hist, _) = create_history();
-        hist.save(HistoryItem::from_command_line("substring"))?;
-        hist.save(HistoryItem::from_command_line("don't find me either"))?;
-        hist.save(HistoryItem::from_command_line("prefix substring"))?;
-        hist.save(HistoryItem::from_command_line("don't find me"))?;
-        hist.save(HistoryItem::from_command_line("prefix substring suffix"))?;
+        save_from_command_line(hist.as_mut(), "substring")?;
+        save_from_command_line(hist.as_mut(), "don't find me either")?;
+        save_from_command_line(hist.as_mut(), "prefix substring")?;
+        save_from_command_line(hist.as_mut(), "don't find me")?;
+        save_from_command_line(hist.as_mut(), "prefix substring suffix")?;
 
         let mut cursor = HistoryCursor::new(
             HistoryNavigationQuery::SubstringSearch("substring".to_string()),
@@ -363,7 +371,7 @@ mod tests {
     #[test]
     fn substring_search_with_empty_value_returns_none() -> Result<()> {
         let (mut hist, _) = create_history();
-        hist.save(HistoryItem::from_command_line("substring"))?;
+        save_from_command_line(hist.as_mut(), "substring")?;
 
         let cursor = HistoryCursor::new(
             HistoryNavigationQuery::SubstringSearch("".to_string()),
@@ -574,11 +582,9 @@ mod tests {
                 let hfile = histfile.clone();
                 std::thread::spawn(move || {
                     let (mut hist, _) = create_history_at(cap, &hfile);
-                    hist.save(HistoryItem::from_command_line(format!("A{i}")))
-                        .unwrap();
+                    save_from_command_line(hist.as_mut(), format!("A{i}")).unwrap();
                     hist.sync().unwrap();
-                    hist.save(HistoryItem::from_command_line(format!("B{i}")))
-                        .unwrap();
+                    save_from_command_line(hist.as_mut(), format!("B{i}")).unwrap();
                 })
             })
             .collect::<Vec<_>>();
diff --git a/src/history/file_backed.rs b/src/history/file_backed.rs
index 28b437f7..d9a7de75 100644
--- a/src/history/file_backed.rs
+++ b/src/history/file_backed.rs
@@ -51,11 +51,16 @@ fn decode_entry(s: &str) -> String {
 }
 
 impl History for FileBackedHistory {
+    fn generate_id(&mut self) -> HistoryItemId {
+        HistoryItemId((self.entries.len() - 1) as i64)
+    }
+
     /// only saves a value if it's different than the last value
-    fn save(&mut self, h: HistoryItem) -> Result<HistoryItem> {
-        let entry = h.command_line;
+    fn save(&mut self, h: &HistoryItem) -> Result<()> {
+        let entry = h.command_line.clone();
+
         // Don't append if the preceding value is identical or the string empty
-        let entry_id = if self
+        if self
             .entries
             .back()
             .map_or(true, |previous| previous != &entry)
@@ -68,16 +73,19 @@ impl History for FileBackedHistory {
                 self.len_on_disk = self.len_on_disk.saturating_sub(1);
             }
             self.entries.push_back(entry.to_string());
-            Some(HistoryItemId::new((self.entries.len() - 1) as i64))
-        } else {
-            None
-        };
-        Ok(FileBackedHistory::construct_entry(entry_id, entry))
+        }
+
+        Ok(())
     }
 
-    fn load(&self, id: HistoryItemId) -> Result<super::HistoryItem> {
+    /// this history doesn't replace entries
+    fn replace(&mut self, h: &HistoryItem) -> Result<()> {
+        self.save(h)
+    }
+
+    fn load(&self, id: HistoryItemId) -> Result<HistoryItem> {
         Ok(FileBackedHistory::construct_entry(
-            Some(id),
+            id,
             self.entries
                 .get(id.0 as usize)
                 .ok_or(ReedlineError(ReedlineErrorVariants::OtherHistoryError(
@@ -153,7 +161,7 @@ impl History for FileBackedHistory {
                 }
             }
             Some(FileBackedHistory::construct_entry(
-                Some(HistoryItemId::new(idx as i64)),
+                HistoryItemId::new(idx as i64),
                 cmd.to_string(), // todo: this copy might be a perf bottleneck
             ))
         };
@@ -318,7 +326,7 @@ impl FileBackedHistory {
     }
 
     // this history doesn't store any info except command line
-    fn construct_entry(id: Option<HistoryItemId>, command_line: String) -> HistoryItem {
+    fn construct_entry(id: HistoryItemId, command_line: String) -> HistoryItem {
         HistoryItem {
             id,
             start_timestamp: None,
diff --git a/src/history/item.rs b/src/history/item.rs
index 0c36fba3..c6c3d4ca 100644
--- a/src/history/item.rs
+++ b/src/history/item.rs
@@ -81,7 +81,7 @@ impl HistoryItemExtraInfo for IgnoreAllExtraInfo {}
 #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
 pub struct HistoryItem<ExtraInfo: HistoryItemExtraInfo = IgnoreAllExtraInfo> {
     /// primary key, unique across one history
-    pub id: Option<HistoryItemId>,
+    pub id: HistoryItemId,
     /// date-time when this command was started
     pub start_timestamp: Option<chrono::DateTime<Utc>>,
     /// the full command line as text
@@ -106,9 +106,9 @@ pub struct HistoryItem<ExtraInfo: HistoryItemExtraInfo = IgnoreAllExtraInfo> {
 
 impl HistoryItem {
     /// create a history item purely from the command line with everything else set to None
-    pub fn from_command_line(cmd: impl Into<String>) -> HistoryItem {
+    pub fn from_command_line(cmd: impl Into<String>, id: HistoryItemId) -> HistoryItem {
         HistoryItem {
-            id: None,
+            id,
             start_timestamp: None,
             command_line: cmd.into(),
             session_id: None,
diff --git a/src/history/sqlite_backed.rs b/src/history/sqlite_backed.rs
index 7ab56e96..576a1437 100644
--- a/src/history/sqlite_backed.rs
+++ b/src/history/sqlite_backed.rs
@@ -7,6 +7,7 @@ use crate::{
     Result,
 };
 use chrono::{TimeZone, Utc};
+use rand::{rngs::SmallRng, RngCore, SeedableRng};
 use rusqlite::{named_params, params, Connection, ToSql};
 use std::{path::PathBuf, time::Duration};
 const SQLITE_APPLICATION_ID: i32 = 1151497937;
@@ -18,12 +19,13 @@ pub struct SqliteBackedHistory {
     db: rusqlite::Connection,
     session: Option<HistorySessionId>,
     session_timestamp: Option<chrono::DateTime<Utc>>,
+    rng: SmallRng,
 }
 
 fn deserialize_history_item(row: &rusqlite::Row) -> rusqlite::Result<HistoryItem> {
     let x: Option<String> = row.get("more_info")?;
     Ok(HistoryItem {
-        id: Some(HistoryItemId::new(row.get("id")?)),
+        id: HistoryItemId::new(row.get("id")?),
         start_timestamp: row.get::<&str, Option<i64>>("start_timestamp")?.map(|e| {
             match Utc.timestamp_millis_opt(e) {
                 chrono::LocalResult::Single(e) => e,
@@ -56,8 +58,12 @@ fn deserialize_history_item(row: &rusqlite::Row) -> rusqlite::Result<HistoryItem
 }
 
 impl History for SqliteBackedHistory {
-    fn save(&mut self, mut entry: HistoryItem) -> Result<HistoryItem> {
-        let ret: i64 = self
+    fn generate_id(&mut self) -> HistoryItemId {
+        HistoryItemId(self.rng.next_u64() as i64)
+    }
+
+    fn save(&mut self, entry: &HistoryItem) -> Result<()> {
+        self
             .db
             .prepare(
                 "insert into history
@@ -71,13 +77,12 @@ impl History for SqliteBackedHistory {
                         cwd = excluded.cwd,
                         duration_ms = excluded.duration_ms,
                         exit_status = excluded.exit_status,
-                        more_info = excluded.more_info
-                    returning id",
+                        more_info = excluded.more_info",
             )
             .map_err(map_sqlite_err)?
-            .query_row(
+            .execute(
                 named_params! {
-                    ":id": entry.id.map(|id| id.0),
+                    ":id": entry.id.0,
                     ":start_timestamp": entry.start_timestamp.map(|e| e.timestamp_millis()),
                     ":command_line": entry.command_line,
                     ":session_id": entry.session_id.map(|e| e.0),
@@ -87,11 +92,14 @@ impl History for SqliteBackedHistory {
                     ":exit_status": entry.exit_status,
                     ":more_info": entry.more_info.as_ref().map(|e| serde_json::to_string(e).unwrap())
                 },
-                |row| row.get(0),
             )
-            .map_err(map_sqlite_err)?;
-        entry.id = Some(HistoryItemId::new(ret));
-        Ok(entry)
+            .map(|_| ())
+            .map_err(map_sqlite_err)
+    }
+
+    /// this history doesn't replace entries
+    fn replace(&mut self, h: &HistoryItem) -> Result<()> {
+        self.save(h)
     }
 
     fn load(&self, id: HistoryItemId) -> Result<HistoryItem> {
@@ -137,7 +145,7 @@ impl History for SqliteBackedHistory {
     ) -> Result<()> {
         // in theory this should run in a transaction
         let item = self.load(id)?;
-        self.save(updater(item))?;
+        self.save(&updater(item))?;
         Ok(())
     }
 
@@ -268,6 +276,7 @@ impl SqliteBackedHistory {
             db,
             session,
             session_timestamp,
+            rng: SmallRng::from_entropy(),
         })
     }
 

From f2011ac987505a89f4a7f6cade97d3d2e9c65522 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= <clement.nerma@gmail.com>
Date: Thu, 7 Dec 2023 13:19:17 +0100
Subject: [PATCH 02/18] Fix: substracting with overflow

---
 src/history/file_backed.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/history/file_backed.rs b/src/history/file_backed.rs
index d9a7de75..6b11f9e3 100644
--- a/src/history/file_backed.rs
+++ b/src/history/file_backed.rs
@@ -52,7 +52,7 @@ fn decode_entry(s: &str) -> String {
 
 impl History for FileBackedHistory {
     fn generate_id(&mut self) -> HistoryItemId {
-        HistoryItemId((self.entries.len() - 1) as i64)
+        HistoryItemId((self.entries.len() + 1) as i64)
     }
 
     /// only saves a value if it's different than the last value

From 1f0dfd957d1b7efda9848817993f322510b88ea3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= <clement.nerma@gmail.com>
Date: Thu, 7 Dec 2023 14:23:15 +0100
Subject: [PATCH 03/18] Update `FileBackedHistory` for new history system

---
 Cargo.lock                 |  50 +++++++++++++--
 Cargo.toml                 |   1 +
 src/history/file_backed.rs | 127 +++++++++++++++++++++++--------------
 3 files changed, 125 insertions(+), 53 deletions(-)

diff --git a/Cargo.lock b/Cargo.lock
index 0d0aa567..a4971517 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -4,13 +4,14 @@ version = 3
 
 [[package]]
 name = "ahash"
-version = "0.8.3"
+version = "0.8.6"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "2c99f64d1e06488f620f932677e24bc6e2897582980441ae90a671415bd7ec2f"
+checksum = "91429305e9f0a25f6205c5b8e0d2db09e0708a7a6df0f42212bb56c32c8ac97a"
 dependencies = [
  "cfg-if",
  "once_cell",
  "version_check",
+ "zerocopy",
 ]
 
 [[package]]
@@ -234,6 +235,12 @@ version = "1.8.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "7fcaabb2fef8c910e7f4c7ce9f67a1283a1715879a7c230ca9d6d1ae31f16d91"
 
+[[package]]
+name = "equivalent"
+version = "1.0.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "5443807d6dff69373d433ab9ef5378ad8df50ca6298caf15de6e52e24aaf54d5"
+
 [[package]]
 name = "errno"
 version = "0.3.1"
@@ -313,9 +320,9 @@ checksum = "d2fabcfbdc87f4758337ca535fb41a6d701b65693ce38287d856d1674551ec9b"
 
 [[package]]
 name = "hashbrown"
-version = "0.14.0"
+version = "0.14.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "2c6201b9ff9fd90a5a3bac2e56a830d0caa509576f0e503818ee82c181b3437a"
+checksum = "290f1a1d9242c78d09ce40a5e87e7554ee637af1351968159f4952f028f75604"
 dependencies = [
  "ahash",
  "allocator-api2",
@@ -359,6 +366,16 @@ dependencies = [
  "cc",
 ]
 
+[[package]]
+name = "indexmap"
+version = "2.1.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "d530e1a18b1cb4c484e6e34556a0d948706958449fca0cab753d649f2bce3d1f"
+dependencies = [
+ "equivalent",
+ "hashbrown",
+]
+
 [[package]]
 name = "itertools"
 version = "0.10.5"
@@ -605,6 +622,7 @@ dependencies = [
  "crossterm",
  "fd-lock",
  "gethostname",
+ "indexmap",
  "itertools",
  "nu-ansi-term",
  "pretty_assertions",
@@ -841,9 +859,9 @@ dependencies = [
 
 [[package]]
 name = "syn"
-version = "2.0.26"
+version = "2.0.32"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "45c3457aacde3c65315de5031ec191ce46604304d2446e803d71ade03308d970"
+checksum = "239814284fd6f1a4ffe4ca893952cdd93c224b6a1571c9a9eadd670295c0c9e2"
 dependencies = [
  "proc-macro2",
  "quote",
@@ -1120,3 +1138,23 @@ name = "yansi"
 version = "0.5.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "09041cd90cf85f7f8b2df60c646f853b7f535ce68f85244eb6731cf89fa498ec"
+
+[[package]]
+name = "zerocopy"
+version = "0.7.29"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "5d075cf85bbb114e933343e087b92f2146bac0d55b534cbb8188becf0039948e"
+dependencies = [
+ "zerocopy-derive",
+]
+
+[[package]]
+name = "zerocopy-derive"
+version = "0.7.29"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "86cd5ca076997b97ef09d3ad65efe811fa68c9e874cb636ccb211223a813b0c2"
+dependencies = [
+ "proc-macro2",
+ "quote",
+ "syn",
+]
diff --git a/Cargo.toml b/Cargo.toml
index 0d3a7683..ff7633d8 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -21,6 +21,7 @@ clipboard = { version = "0.5.0", optional = true }
 crossbeam = { version = "0.8.2", optional = true }
 crossterm = { version = "0.27.0", features = ["serde"] }
 fd-lock = "3.0.3"
+indexmap = "2.1.0"
 itertools = "0.10.3"
 nu-ansi-term = "0.49.0"
 rand = { version = "0.8.5", default-features = false, features = [
diff --git a/src/history/file_backed.rs b/src/history/file_backed.rs
index 6b11f9e3..7fc73876 100644
--- a/src/history/file_backed.rs
+++ b/src/history/file_backed.rs
@@ -1,3 +1,5 @@
+use indexmap::IndexMap;
+
 use super::{
     base::CommandLineSearch, History, HistoryItem, HistoryItemId, SearchDirection, SearchQuery,
 };
@@ -7,7 +9,6 @@ use crate::{
 };
 
 use std::{
-    collections::VecDeque,
     fs::OpenOptions,
     io::{BufRead, BufReader, BufWriter, Seek, SeekFrom, Write},
     ops::{Deref, DerefMut},
@@ -27,9 +28,9 @@ pub const NEWLINE_ESCAPE: &str = "<\\n>";
 #[derive(Debug)]
 pub struct FileBackedHistory {
     capacity: usize,
-    entries: VecDeque<String>,
+    entries: IndexMap<HistoryItemId, String>,
     file: Option<PathBuf>,
-    len_on_disk: usize, // Keep track what was previously written to disk
+    last_on_disk: Option<HistoryItemId>,
     session: Option<HistorySessionId>,
 }
 
@@ -42,12 +43,19 @@ impl Default for FileBackedHistory {
     }
 }
 
-fn encode_entry(s: &str) -> String {
-    s.replace('\n', NEWLINE_ESCAPE)
+fn encode_entry(id: HistoryItemId, s: &str) -> String {
+    format!("{id}:{}", s.replace('\n', NEWLINE_ESCAPE))
 }
 
-fn decode_entry(s: &str) -> String {
-    s.replace(NEWLINE_ESCAPE, "\n")
+fn decode_entry(s: &str) -> std::result::Result<(HistoryItemId, String), &'static str> {
+    let sep = s
+        .bytes()
+        .position(|b| b == b':')
+        .ok_or("History item ID is missing")?;
+
+    let id = s[..sep].parse().map_err(|_| "Invalid history item ID")?;
+
+    Ok((HistoryItemId(id), s.replace(NEWLINE_ESCAPE, "\n")))
 }
 
 impl History for FileBackedHistory {
@@ -62,17 +70,17 @@ impl History for FileBackedHistory {
         // Don't append if the preceding value is identical or the string empty
         if self
             .entries
-            .back()
-            .map_or(true, |previous| previous != &entry)
+            .last()
+            .map_or(true, |(_, previous)| previous != &entry)
             && !entry.is_empty()
         {
             if self.entries.len() == self.capacity {
                 // History is "full", so we delete the oldest entry first,
                 // before adding a new one.
-                self.entries.pop_front();
-                self.len_on_disk = self.len_on_disk.saturating_sub(1);
+                self.entries.remove(&HistoryItemId(0));
             }
-            self.entries.push_back(entry.to_string());
+
+            self.entries.insert(h.id, entry.to_string());
         }
 
         Ok(())
@@ -87,7 +95,7 @@ impl History for FileBackedHistory {
         Ok(FileBackedHistory::construct_entry(
             id,
             self.entries
-                .get(id.0 as usize)
+                .get(&id)
                 .ok_or(ReedlineError(ReedlineErrorVariants::OtherHistoryError(
                     "Item does not exist",
                 )))?
@@ -122,31 +130,19 @@ impl History for FileBackedHistory {
                 },
             ));
         }
-        let (min_id, max_id) = {
-            let start = query.start_id.map(|e| e.0);
-            let end = query.end_id.map(|e| e.0);
+
+        let (from_id, to_id) = {
+            let start = query.start_id;
+            let end = query.end_id;
+
             if let SearchDirection::Backward = query.direction {
                 (end, start)
             } else {
                 (start, end)
             }
         };
-        // add one to make it inclusive
-        let min_id = min_id.map(|e| e + 1).unwrap_or(0);
-        // subtract one to make it inclusive
-        let max_id = max_id
-            .map(|e| e - 1)
-            .unwrap_or(self.entries.len() as i64 - 1);
-        if max_id < 0 || min_id > self.entries.len() as i64 - 1 {
-            return Ok(vec![]);
-        }
-        let intrinsic_limit = max_id - min_id + 1;
-        let limit = if let Some(given_limit) = query.limit {
-            std::cmp::min(intrinsic_limit, given_limit) as usize
-        } else {
-            intrinsic_limit as usize
-        };
-        let filter = |(idx, cmd): (usize, &String)| {
+
+        let filter = |(_, (id, cmd)): (usize, (&HistoryItemId, &String))| {
             if !match &query.filter.command_line {
                 Some(CommandLineSearch::Prefix(p)) => cmd.starts_with(p),
                 Some(CommandLineSearch::Substring(p)) => cmd.contains(p),
@@ -161,17 +157,37 @@ impl History for FileBackedHistory {
                 }
             }
             Some(FileBackedHistory::construct_entry(
-                HistoryItemId::new(idx as i64),
+                *id,
                 cmd.to_string(), // todo: this copy might be a perf bottleneck
             ))
         };
 
+        let from_index = match from_id {
+            Some(from_id) => self.entries.binary_search_keys(&from_id).expect("todo"),
+            None => 0,
+        };
+
+        let to_index = match to_id {
+            Some(to_id) => self.entries.binary_search_keys(&to_id).expect("todo"),
+            None => self.entries.len().saturating_sub(1),
+        };
+
+        if from_index >= to_index {
+            return Ok(vec![]);
+        }
+
         let iter = self
             .entries
             .iter()
             .enumerate()
-            .skip(min_id as usize)
-            .take(intrinsic_limit as usize);
+            .skip(from_index)
+            .take(to_index - from_index);
+
+        let limit = match query.limit {
+            Some(limit) => usize::try_from(limit).unwrap(),
+            None => usize::MAX,
+        };
+
         if let SearchDirection::Backward = query.direction {
             Ok(iter.rev().filter_map(filter).take(limit).collect())
         } else {
@@ -194,7 +210,7 @@ impl History for FileBackedHistory {
 
     fn clear(&mut self) -> Result<()> {
         self.entries.clear();
-        self.len_on_disk = 0;
+        self.last_on_disk = None;
 
         if let Some(file) = &self.file {
             if let Err(err) = std::fs::remove_file(file) {
@@ -220,7 +236,16 @@ impl History for FileBackedHistory {
     fn sync(&mut self) -> std::io::Result<()> {
         if let Some(fname) = &self.file {
             // The unwritten entries
-            let own_entries = self.entries.range(self.len_on_disk..);
+            let last_index_on_disk = match self.last_on_disk {
+                Some(last_id) => self.entries.binary_search_keys(&last_id).unwrap(),
+                None => 0,
+            };
+
+            if last_index_on_disk == self.entries.len() {
+                return Ok(());
+            }
+
+            let own_entries = self.entries.get_range(last_index_on_disk..).unwrap();
 
             if let Some(base_dir) = fname.parent() {
                 std::fs::create_dir_all(base_dir)?;
@@ -234,12 +259,15 @@ impl History for FileBackedHistory {
                     .open(fname)?,
             );
             let mut writer_guard = f_lock.write()?;
+
             let (mut foreign_entries, truncate) = {
                 let reader = BufReader::new(writer_guard.deref());
+
                 let mut from_file = reader
                     .lines()
-                    .map(|o| o.map(|i| decode_entry(&i)))
-                    .collect::<std::io::Result<VecDeque<_>>>()?;
+                    .map(|o| o.map(|i| decode_entry(&i).expect("todo: error handling")))
+                    .collect::<std::io::Result<IndexMap<_, _>>>()?;
+
                 if from_file.len() + own_entries.len() > self.capacity {
                     (
                         from_file.split_off(from_file.len() - (self.capacity - own_entries.len())),
@@ -252,33 +280,38 @@ impl History for FileBackedHistory {
 
             {
                 let mut writer = BufWriter::new(writer_guard.deref_mut());
+
                 if truncate {
                     writer.rewind()?;
 
-                    for line in &foreign_entries {
-                        writer.write_all(encode_entry(line).as_bytes())?;
+                    for (id, line) in &foreign_entries {
+                        writer.write_all(encode_entry(*id, line).as_bytes())?;
                         writer.write_all("\n".as_bytes())?;
                     }
                 } else {
                     writer.seek(SeekFrom::End(0))?;
                 }
-                for line in own_entries {
-                    writer.write_all(encode_entry(line).as_bytes())?;
+
+                for (id, line) in own_entries {
+                    writer.write_all(encode_entry(*id, line).as_bytes())?;
                     writer.write_all("\n".as_bytes())?;
                 }
+
                 writer.flush()?;
             }
+
             if truncate {
                 let file = writer_guard.deref_mut();
                 let file_len = file.stream_position()?;
                 file.set_len(file_len)?;
             }
 
-            let own_entries = self.entries.drain(self.len_on_disk..);
+            let own_entries = self.entries.drain(last_index_on_disk + 1..);
             foreign_entries.extend(own_entries);
             self.entries = foreign_entries;
 
-            self.len_on_disk = self.entries.len();
+            let last_entry = self.entries.last().unwrap();
+            self.last_on_disk = Some(*last_entry.0);
         }
         Ok(())
     }
@@ -300,9 +333,9 @@ impl FileBackedHistory {
         }
         FileBackedHistory {
             capacity,
-            entries: VecDeque::new(),
+            entries: IndexMap::new(),
             file: None,
-            len_on_disk: 0,
+            last_on_disk: None,
             session: None,
         }
     }

From d0d27e83c3adb7f76fd59a5ac875f2a4f6b26c7b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= <clement.nerma@gmail.com>
Date: Thu, 7 Dec 2023 15:46:28 +0100
Subject: [PATCH 04/18] Fix: ranges and truncation

---
 src/history/cursor.rs      |  10 ++
 src/history/file_backed.rs | 213 +++++++++++++++++++++----------------
 2 files changed, 130 insertions(+), 93 deletions(-)

diff --git a/src/history/cursor.rs b/src/history/cursor.rs
index 4fc34473..c6b99a0a 100644
--- a/src/history/cursor.rs
+++ b/src/history/cursor.rs
@@ -58,6 +58,7 @@ impl HistoryCursor {
             filter
         }
     }
+
     fn navigate_in_direction(
         &mut self,
         history: &dyn History,
@@ -67,7 +68,9 @@ impl HistoryCursor {
             // if searching forward but we don't have a starting point, assume we are at the end
             return Ok(());
         }
+
         let start_id = self.current.as_ref().map(|e| e.id);
+
         let mut next = history.search(SearchQuery {
             start_id,
             end_id: None,
@@ -77,12 +80,18 @@ impl HistoryCursor {
             limit: Some(1),
             filter: self.get_search_filter(),
         })?;
+
         if next.len() == 1 {
+            // NOTE: .swap_remove() does not preserve the vector's order
+            // But is *is* faster than .remove() as it completes in O(1)
+            // And given we won't use this vector of results afterwards,
+            // we can use this function without worrying here
             self.current = Some(next.swap_remove(0));
         } else if direction == SearchDirection::Forward {
             // no result and searching forward: we are at the end
             self.current = None;
         }
+
         Ok(())
     }
 
@@ -400,6 +409,7 @@ mod tests {
         }
 
         let (reading_hist, _) = create_history_at(5, &histfile);
+
         let actual = get_all_entry_texts(reading_hist.as_ref());
         assert_eq!(entries, actual);
 
diff --git a/src/history/file_backed.rs b/src/history/file_backed.rs
index 7fc73876..ee35d44c 100644
--- a/src/history/file_backed.rs
+++ b/src/history/file_backed.rs
@@ -55,7 +55,10 @@ fn decode_entry(s: &str) -> std::result::Result<(HistoryItemId, String), &'stati
 
     let id = s[..sep].parse().map_err(|_| "Invalid history item ID")?;
 
-    Ok((HistoryItemId(id), s.replace(NEWLINE_ESCAPE, "\n")))
+    Ok((
+        HistoryItemId(id),
+        s[sep + 1..].replace(NEWLINE_ESCAPE, "\n"),
+    ))
 }
 
 impl History for FileBackedHistory {
@@ -142,57 +145,61 @@ impl History for FileBackedHistory {
             }
         };
 
-        let filter = |(_, (id, cmd)): (usize, (&HistoryItemId, &String))| {
-            if !match &query.filter.command_line {
-                Some(CommandLineSearch::Prefix(p)) => cmd.starts_with(p),
-                Some(CommandLineSearch::Substring(p)) => cmd.contains(p),
-                Some(CommandLineSearch::Exact(p)) => cmd == p,
-                None => true,
-            } {
-                return None;
-            }
-            if let Some(str) = &query.filter.not_command_line {
-                if cmd == str {
-                    return None;
-                }
-            }
-            Some(FileBackedHistory::construct_entry(
-                *id,
-                cmd.to_string(), // todo: this copy might be a perf bottleneck
-            ))
-        };
-
         let from_index = match from_id {
-            Some(from_id) => self.entries.binary_search_keys(&from_id).expect("todo"),
+            Some(from_id) => self.entries.get_index_of(&from_id).expect("todo"),
             None => 0,
         };
 
         let to_index = match to_id {
-            Some(to_id) => self.entries.binary_search_keys(&to_id).expect("todo"),
+            Some(to_id) => self.entries.get_index_of(&to_id).expect("todo"),
             None => self.entries.len().saturating_sub(1),
         };
 
-        if from_index >= to_index {
+        if from_index == to_index {
             return Ok(vec![]);
         }
 
+        assert!(from_index < to_index);
+
         let iter = self
             .entries
             .iter()
-            .enumerate()
             .skip(from_index)
-            .take(to_index - from_index);
+            .take(1 + to_index - from_index);
 
         let limit = match query.limit {
             Some(limit) => usize::try_from(limit).unwrap(),
             None => usize::MAX,
         };
 
-        if let SearchDirection::Backward = query.direction {
-            Ok(iter.rev().filter_map(filter).take(limit).collect())
-        } else {
-            Ok(iter.filter_map(filter).take(limit).collect())
-        }
+        let filter = |(id, cmd): (&HistoryItemId, &String)| {
+            let str_matches = match &query.filter.command_line {
+                Some(CommandLineSearch::Prefix(p)) => cmd.starts_with(p),
+                Some(CommandLineSearch::Substring(p)) => cmd.contains(p),
+                Some(CommandLineSearch::Exact(p)) => cmd == p,
+                None => true,
+            };
+
+            if !str_matches {
+                return None;
+            }
+
+            if let Some(str) = &query.filter.not_command_line {
+                if cmd == str {
+                    return None;
+                }
+            }
+
+            Some(FileBackedHistory::construct_entry(
+                *id,
+                cmd.clone(), // todo: this cloning might be a perf bottleneck
+            ))
+        };
+
+        Ok(match query.direction {
+            SearchDirection::Backward => iter.rev().filter_map(filter).take(limit).collect(),
+            SearchDirection::Forward => iter.filter_map(filter).take(limit).collect(),
+        })
     }
 
     fn update(
@@ -234,85 +241,102 @@ impl History for FileBackedHistory {
     ///
     /// If file would exceed `capacity` truncates the oldest entries.
     fn sync(&mut self) -> std::io::Result<()> {
-        if let Some(fname) = &self.file {
-            // The unwritten entries
-            let last_index_on_disk = match self.last_on_disk {
-                Some(last_id) => self.entries.binary_search_keys(&last_id).unwrap(),
-                None => 0,
-            };
+        let Some(fname) = &self.file else {
+            return Ok(());
+        };
 
-            if last_index_on_disk == self.entries.len() {
-                return Ok(());
-            }
+        // The unwritten entries
+        let last_index_on_disk = self
+            .last_on_disk
+            .map(|id| self.entries.get_index_of(&id).unwrap());
 
-            let own_entries = self.entries.get_range(last_index_on_disk..).unwrap();
+        let range_start = match last_index_on_disk {
+            Some(index) => index + 1,
+            None => 0,
+        };
 
-            if let Some(base_dir) = fname.parent() {
-                std::fs::create_dir_all(base_dir)?;
-            }
+        let own_entries = self.entries.get_range(range_start..).unwrap();
 
-            let mut f_lock = fd_lock::RwLock::new(
-                OpenOptions::new()
-                    .create(true)
-                    .write(true)
-                    .read(true)
-                    .open(fname)?,
-            );
-            let mut writer_guard = f_lock.write()?;
-
-            let (mut foreign_entries, truncate) = {
-                let reader = BufReader::new(writer_guard.deref());
-
-                let mut from_file = reader
-                    .lines()
-                    .map(|o| o.map(|i| decode_entry(&i).expect("todo: error handling")))
-                    .collect::<std::io::Result<IndexMap<_, _>>>()?;
-
-                if from_file.len() + own_entries.len() > self.capacity {
-                    (
-                        from_file.split_off(from_file.len() - (self.capacity - own_entries.len())),
-                        true,
-                    )
-                } else {
-                    (from_file, false)
-                }
-            };
+        if let Some(base_dir) = fname.parent() {
+            std::fs::create_dir_all(base_dir)?;
+        }
 
-            {
-                let mut writer = BufWriter::new(writer_guard.deref_mut());
+        let mut f_lock = fd_lock::RwLock::new(
+            OpenOptions::new()
+                .create(true)
+                .write(true)
+                .read(true)
+                .open(fname)?,
+        );
+
+        let mut writer_guard = f_lock.write()?;
+
+        let (mut foreign_entries, truncate) = {
+            let reader = BufReader::new(writer_guard.deref());
+
+            let mut from_file = reader
+                .lines()
+                .map(|o| o.map(|i| decode_entry(&i).expect("todo: error handling")))
+                .collect::<std::io::Result<IndexMap<_, _>>>()?;
+
+            if from_file.len() + own_entries.len() > self.capacity {
+                (
+                    from_file.split_off(from_file.len() - (self.capacity - own_entries.len())),
+                    true,
+                )
+            } else {
+                (from_file, false)
+            }
+        };
 
-                if truncate {
-                    writer.rewind()?;
+        {
+            let mut writer = BufWriter::new(writer_guard.deref_mut());
 
-                    for (id, line) in &foreign_entries {
-                        writer.write_all(encode_entry(*id, line).as_bytes())?;
-                        writer.write_all("\n".as_bytes())?;
-                    }
-                } else {
-                    writer.seek(SeekFrom::End(0))?;
-                }
+            // In case of truncation, we first write every foreign entry (replacing existing content)
+            if truncate {
+                writer.rewind()?;
 
-                for (id, line) in own_entries {
+                for (id, line) in &foreign_entries {
                     writer.write_all(encode_entry(*id, line).as_bytes())?;
                     writer.write_all("\n".as_bytes())?;
                 }
-
-                writer.flush()?;
+            } else {
+                // Otherwise we directly jump at the end of the file
+                writer.seek(SeekFrom::End(0))?;
             }
 
-            if truncate {
-                let file = writer_guard.deref_mut();
-                let file_len = file.stream_position()?;
-                file.set_len(file_len)?;
+            // Then we write new entries (that haven't been synced to the file yet)
+            for (id, line) in own_entries {
+                writer.write_all(encode_entry(*id, line).as_bytes())?;
+                writer.write_all("\n".as_bytes())?;
             }
 
-            let own_entries = self.entries.drain(last_index_on_disk + 1..);
-            foreign_entries.extend(own_entries);
-            self.entries = foreign_entries;
+            writer.flush()?;
+        }
 
-            let last_entry = self.entries.last().unwrap();
-            self.last_on_disk = Some(*last_entry.0);
+        // If truncation is needed, we then remove everything after the cursor's current location
+        if truncate {
+            let file = writer_guard.deref_mut();
+            let file_len = file.stream_position()?;
+            file.set_len(file_len)?;
         }
+
+        match last_index_on_disk {
+            Some(last_index_on_disk) => {
+                if last_index_on_disk + 1 < self.entries.len() {
+                    foreign_entries.extend(self.entries.drain(last_index_on_disk + 1..));
+                }
+            }
+
+            None => {
+                foreign_entries.extend(self.entries.drain(..));
+            }
+        }
+
+        self.entries = foreign_entries;
+
+        self.last_on_disk = self.entries.last().map(|(id, _)| *id);
+
         Ok(())
     }
 
@@ -350,11 +374,14 @@ impl FileBackedHistory {
     ///
     pub fn with_file(capacity: usize, file: PathBuf) -> std::io::Result<Self> {
         let mut hist = Self::new(capacity);
+
         if let Some(base_dir) = file.parent() {
             std::fs::create_dir_all(base_dir)?;
         }
+
         hist.file = Some(file);
         hist.sync()?;
+
         Ok(hist)
     }
 

From 9cc5d909d52b09f95429f872cc3f862cac6ac75d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= <clement.nerma@gmail.com>
Date: Thu, 7 Dec 2023 17:08:42 +0100
Subject: [PATCH 05/18] Fix: ID generation

---
 src/history/cursor.rs      |  5 +++++
 src/history/file_backed.rs | 12 +++++++-----
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/src/history/cursor.rs b/src/history/cursor.rs
index c6b99a0a..43c68508 100644
--- a/src/history/cursor.rs
+++ b/src/history/cursor.rs
@@ -549,12 +549,17 @@ mod tests {
         {
             let (mut hist_a, _) = create_history_at(capacity, &histfile);
 
+            assert_eq!(get_all_entry_texts(hist_a.as_ref()), initial_entries);
+
             {
                 let (mut hist_b, _) = create_history_at(capacity, &histfile);
 
+                assert_eq!(get_all_entry_texts(hist_b.as_ref()), initial_entries);
+
                 add_text_entries(hist_b.as_mut(), &entries_b);
                 // As `hist` goes out of scope and get's dropped, its contents are flushed to disk
             }
+
             add_text_entries(hist_a.as_mut(), &entries_a);
             // As `hist` goes out of scope and get's dropped, its contents are flushed to disk
         }
diff --git a/src/history/file_backed.rs b/src/history/file_backed.rs
index ee35d44c..52bfc47b 100644
--- a/src/history/file_backed.rs
+++ b/src/history/file_backed.rs
@@ -1,4 +1,5 @@
 use indexmap::IndexMap;
+use rand::{rngs::SmallRng, RngCore, SeedableRng};
 
 use super::{
     base::CommandLineSearch, History, HistoryItem, HistoryItemId, SearchDirection, SearchQuery,
@@ -32,6 +33,7 @@ pub struct FileBackedHistory {
     file: Option<PathBuf>,
     last_on_disk: Option<HistoryItemId>,
     session: Option<HistorySessionId>,
+    rng: SmallRng,
 }
 
 impl Default for FileBackedHistory {
@@ -63,7 +65,7 @@ fn decode_entry(s: &str) -> std::result::Result<(HistoryItemId, String), &'stati
 
 impl History for FileBackedHistory {
     fn generate_id(&mut self) -> HistoryItemId {
-        HistoryItemId((self.entries.len() + 1) as i64)
+        HistoryItemId(self.rng.next_u64() as i64)
     }
 
     /// only saves a value if it's different than the last value
@@ -280,10 +282,9 @@ impl History for FileBackedHistory {
                 .collect::<std::io::Result<IndexMap<_, _>>>()?;
 
             if from_file.len() + own_entries.len() > self.capacity {
-                (
-                    from_file.split_off(from_file.len() - (self.capacity - own_entries.len())),
-                    true,
-                )
+                let start = from_file.len() + own_entries.len() - self.capacity;
+
+                (from_file.split_off(start), true)
             } else {
                 (from_file, false)
             }
@@ -361,6 +362,7 @@ impl FileBackedHistory {
             file: None,
             last_on_disk: None,
             session: None,
+            rng: SmallRng::from_entropy(),
         }
     }
 

From 6fe8ebe496520f0cadd1728051342aaf386b541a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= <clement.nerma@gmail.com>
Date: Thu, 7 Dec 2023 17:31:15 +0100
Subject: [PATCH 06/18] Fix: last problems with `FileBackedHistory`

---
 src/history/cursor.rs      | 6 ++++++
 src/history/file_backed.rs | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/history/cursor.rs b/src/history/cursor.rs
index 43c68508..8cb7e0b3 100644
--- a/src/history/cursor.rs
+++ b/src/history/cursor.rs
@@ -322,6 +322,7 @@ mod tests {
     #[test]
     fn prefix_search_ignores_consecutive_equivalent_entries_going_forwards() -> Result<()> {
         let (mut hist, _) = create_history();
+
         save_from_command_line(hist.as_mut(), "find me once")?;
         save_from_command_line(hist.as_mut(), "test")?;
         save_from_command_line(hist.as_mut(), "find me once")?;
@@ -331,21 +332,26 @@ mod tests {
             HistoryNavigationQuery::PrefixSearch("find".to_string()),
             None,
         );
+
         cursor.back(&*hist)?;
         assert_eq!(
             cursor.string_at_cursor(),
             Some("find me as well".to_string())
         );
+
         cursor.back(&*hist)?;
         cursor.back(&*hist)?;
         assert_eq!(cursor.string_at_cursor(), Some("find me once".to_string()));
+
         cursor.forward(&*hist)?;
         assert_eq!(
             cursor.string_at_cursor(),
             Some("find me as well".to_string())
         );
+
         cursor.forward(&*hist)?;
         assert_eq!(cursor.string_at_cursor(), None);
+
         Ok(())
     }
 
diff --git a/src/history/file_backed.rs b/src/history/file_backed.rs
index 52bfc47b..72a0e4c6 100644
--- a/src/history/file_backed.rs
+++ b/src/history/file_backed.rs
@@ -82,7 +82,7 @@ impl History for FileBackedHistory {
             if self.entries.len() == self.capacity {
                 // History is "full", so we delete the oldest entry first,
                 // before adding a new one.
-                self.entries.remove(&HistoryItemId(0));
+                self.entries.shift_remove(&HistoryItemId(0));
             }
 
             self.entries.insert(h.id, entry.to_string());

From 40cd831bfee1b8c8cb6a0f5773a010db3d2e2410 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= <clement.nerma@gmail.com>
Date: Fri, 8 Dec 2023 09:32:21 +0100
Subject: [PATCH 07/18] Update `SqliteBackedHistory` for new history system

---
 src/history/sqlite_backed.rs | 107 +++++++++++++++++++----------------
 1 file changed, 59 insertions(+), 48 deletions(-)

diff --git a/src/history/sqlite_backed.rs b/src/history/sqlite_backed.rs
index 576a1437..8c22fb3b 100644
--- a/src/history/sqlite_backed.rs
+++ b/src/history/sqlite_backed.rs
@@ -24,6 +24,7 @@ pub struct SqliteBackedHistory {
 
 fn deserialize_history_item(row: &rusqlite::Row) -> rusqlite::Result<HistoryItem> {
     let x: Option<String> = row.get("more_info")?;
+
     Ok(HistoryItem {
         id: HistoryItemId::new(row.get("id")?),
         start_timestamp: row.get::<&str, Option<i64>>("start_timestamp")?.map(|e| {
@@ -67,7 +68,7 @@ impl History for SqliteBackedHistory {
             .db
             .prepare(
                 "insert into history
-                               (id,  start_timestamp,  command_line,  session_id,  hostname,  cwd,  duration_ms,  exit_status,  more_info)
+                               (id, start_timestamp,  command_line,  session_id,  hostname,  cwd,  duration_ms,  exit_status,  more_info)
                         values (:id, :start_timestamp, :command_line, :session_id, :hostname, :cwd, :duration_ms, :exit_status, :more_info)
                     on conflict (history.id) do update set
                         start_timestamp = excluded.start_timestamp,
@@ -253,7 +254,8 @@ impl SqliteBackedHistory {
         db.execute_batch(
             "
         create table if not exists history (
-            id integer primary key autoincrement,
+            idx integer primary key autoincrement,
+            id integer unique not null,
             command_line text not null,
             start_timestamp integer,
             session_id integer,
@@ -263,15 +265,16 @@ impl SqliteBackedHistory {
             exit_status integer,
             more_info text
         ) strict;
+        
         create index if not exists idx_history_time on history(start_timestamp);
         create index if not exists idx_history_cwd on history(cwd); -- suboptimal for many hosts
         create index if not exists idx_history_exit_status on history(exit_status);
         create index if not exists idx_history_cmd on history(command_line);
         create index if not exists idx_history_cmd on history(session_id);
-        -- todo: better indexes
         ",
         )
         .map_err(map_sqlite_err)?;
+
         Ok(SqliteBackedHistory {
             db,
             session,
@@ -285,94 +288,102 @@ impl SqliteBackedHistory {
         query: &'a SearchQuery,
         select_expression: &str,
     ) -> (String, BoxedNamedParams<'a>) {
+        let SearchQuery {
+            direction,
+            start_time,
+            end_time,
+            start_id,
+            end_id,
+            limit,
+            filter,
+        } = query;
+
         // TODO: this whole function could be done with less allocs
-        let (is_asc, asc) = match query.direction {
+        let (is_asc, asc) = match direction {
             SearchDirection::Forward => (true, "asc"),
             SearchDirection::Backward => (false, "desc"),
         };
+
         let mut wheres = Vec::new();
         let mut params: BoxedNamedParams = Vec::new();
-        if let Some(start) = query.start_time {
-            wheres.push(if is_asc {
-                "timestamp_start > :start_time"
-            } else {
-                "timestamp_start < :start_time"
-            });
+
+        if let Some(start) = start_time {
+            let cmp_op = if is_asc { '>' } else { '<' };
+            wheres.push(format!("timestamp_start {cmp_op} :start_time"));
             params.push((":start_time", Box::new(start.timestamp_millis())));
         }
-        if let Some(end) = query.end_time {
-            wheres.push(if is_asc {
-                ":end_time >= timestamp_start"
-            } else {
-                ":end_time <= timestamp_start"
-            });
+        if let Some(end) = end_time {
+            let cmp_op = if is_asc { ">=" } else { "<=" };
+            wheres.push(format!(":end_time {cmp_op} timestamp_start"));
             params.push((":end_time", Box::new(end.timestamp_millis())));
         }
-        if let Some(start) = query.start_id {
-            wheres.push(if is_asc {
-                "id > :start_id"
-            } else {
-                "id < :start_id"
-            });
+        if let Some(start) = start_id {
+            let cmp_op = if is_asc { '>' } else { '<' };
+            wheres.push(format!(
+                "idx {cmp_op} (SELECT idx FROM history WHERE id = :start_id)"
+            ));
             params.push((":start_id", Box::new(start.0)));
         }
-        if let Some(end) = query.end_id {
-            wheres.push(if is_asc {
-                ":end_id >= id"
-            } else {
-                ":end_id <= id"
-            });
+        if let Some(end) = end_id {
+            let cmp_op = if is_asc { ">=" } else { "<=" };
+            wheres.push(format!(
+                "idx {cmp_op} (SELECT idx FROM history WHERE id = :start_id)"
+            ));
             params.push((":end_id", Box::new(end.0)));
         }
-        let limit = match query.limit {
+        let limit = match limit {
             Some(l) => {
                 params.push((":limit", Box::new(l)));
                 "limit :limit"
             }
             None => "",
         };
-        if let Some(command_line) = &query.filter.command_line {
+        if let Some(command_line) = &filter.command_line {
             // TODO: escape %
             let command_line_like = match command_line {
                 CommandLineSearch::Exact(e) => e.to_string(),
                 CommandLineSearch::Prefix(prefix) => format!("{prefix}%"),
                 CommandLineSearch::Substring(cont) => format!("%{cont}%"),
             };
-            wheres.push("command_line like :command_line");
+            wheres.push("command_line like :command_line".to_owned());
             params.push((":command_line", Box::new(command_line_like)));
         }
 
-        if let Some(str) = &query.filter.not_command_line {
-            wheres.push("command_line != :not_cmd");
+        if let Some(str) = &filter.not_command_line {
+            wheres.push("command_line != :not_cmd".to_owned());
             params.push((":not_cmd", Box::new(str)));
         }
-        if let Some(hostname) = &query.filter.hostname {
-            wheres.push("hostname = :hostname");
+
+        if let Some(hostname) = &filter.hostname {
+            wheres.push("hostname = :hostname".to_owned());
             params.push((":hostname", Box::new(hostname)));
         }
-        if let Some(cwd_exact) = &query.filter.cwd_exact {
-            wheres.push("cwd = :cwd");
+
+        if let Some(cwd_exact) = &filter.cwd_exact {
+            wheres.push("cwd = :cwd".to_owned());
             params.push((":cwd", Box::new(cwd_exact)));
         }
-        if let Some(cwd_prefix) = &query.filter.cwd_prefix {
-            wheres.push("cwd like :cwd_like");
+
+        if let Some(cwd_prefix) = &filter.cwd_prefix {
+            wheres.push("cwd like :cwd_like".to_owned());
             let cwd_like = format!("{cwd_prefix}%");
             params.push((":cwd_like", Box::new(cwd_like)));
         }
-        if let Some(exit_successful) = query.filter.exit_successful {
-            if exit_successful {
-                wheres.push("exit_status = 0");
-            } else {
-                wheres.push("exit_status != 0");
-            }
+
+        if let Some(exit_successful) = filter.exit_successful {
+            let cmp_op = if exit_successful { "=" } else { "!=" };
+            wheres.push(format!("exit_status {cmp_op} 0"));
         }
+
         if let (Some(session_id), Some(session_timestamp)) =
-            (query.filter.session, self.session_timestamp)
+            (filter.session, self.session_timestamp)
         {
             // Filter so that we get rows:
             // - that have the same session_id, or
             // - were executed before our session started
-            wheres.push("(session_id = :session_id OR start_timestamp < :session_timestamp)");
+            wheres.push(
+                "(session_id = :session_id OR start_timestamp < :session_timestamp)".to_owned(),
+            );
             params.push((":session_id", Box::new(session_id)));
             params.push((
                 ":session_timestamp",
@@ -387,7 +398,7 @@ impl SqliteBackedHistory {
             "SELECT {select_expression} \
              FROM history \
              WHERE ({wheres}) \
-             ORDER BY id {asc} \
+             ORDER BY idx {asc} \
              {limit}"
         );
         (query, params)

From 769c156649998284f73dd0dfd5ca591aabd30e9a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= <clement.nerma@gmail.com>
Date: Fri, 8 Dec 2023 10:18:41 +0100
Subject: [PATCH 08/18] Update tests

---
 src/history/base.rs          | 68 ++++++++++++++++++++++--------------
 src/history/cursor.rs        |  8 +++++
 src/history/file_backed.rs   | 22 +++++++-----
 src/history/sqlite_backed.rs | 10 ++++--
 4 files changed, 70 insertions(+), 38 deletions(-)

diff --git a/src/history/base.rs b/src/history/base.rs
index 17c09c94..e3c12cbb 100644
--- a/src/history/base.rs
+++ b/src/history/base.rs
@@ -201,9 +201,10 @@ pub trait History: Send {
     /// retrieves the next unused session id
 
     /// count the results of a query
-    fn count(&self, query: SearchQuery) -> Result<i64>;
+    fn count(&self, query: SearchQuery) -> Result<u64>;
+
     /// return the total number of history items
-    fn count_all(&self) -> Result<i64> {
+    fn count_all(&self) -> Result<u64> {
         self.count(SearchQuery::everything(SearchDirection::Forward, None))
     }
     /// return the results of a query
@@ -232,18 +233,21 @@ mod test {
     #[cfg(not(any(feature = "sqlite", feature = "sqlite-dynlib")))]
     const IS_FILE_BASED: bool = true;
 
-    use std::{
-        sync::atomic::{AtomicI64, Ordering},
-        time::Duration,
-    };
+    use std::time::Duration;
 
     use crate::HistorySessionId;
 
-    static COUNTER: AtomicI64 = AtomicI64::new(0);
+    fn create_item(
+        hist: &mut dyn History,
+        session: i64,
+        cwd: &str,
+        cmd: &str,
+        exit_status: i64,
+    ) -> Result<()> {
+        let len = hist.count_all()?;
 
-    fn create_item(session: i64, cwd: &str, cmd: &str, exit_status: i64) -> HistoryItem {
-        HistoryItem {
-            id: HistoryItemId(COUNTER.fetch_add(1, Ordering::SeqCst)),
+        let item = HistoryItem {
+            id: HistoryItemId(len as i64),
             start_timestamp: None,
             command_line: cmd.to_string(),
             session_id: Some(HistorySessionId::new(session)),
@@ -252,30 +256,40 @@ mod test {
             duration: Some(Duration::from_millis(1000)),
             exit_status: Some(exit_status),
             more_info: None,
-        }
+        };
+
+        hist.save(&item)?;
+
+        assert_eq!(hist.count_all()?, len + 1);
+
+        Ok(())
     }
 
     use super::*;
     fn create_filled_example_history() -> Result<Box<dyn History>> {
         #[cfg(any(feature = "sqlite", feature = "sqlite-dynlib"))]
         let mut history = crate::SqliteBackedHistory::in_memory()?;
+
         #[cfg(not(any(feature = "sqlite", feature = "sqlite-dynlib")))]
         let mut history = crate::FileBackedHistory::default();
+
         #[cfg(not(any(feature = "sqlite", feature = "sqlite-dynlib")))]
-        history.save(&create_item(1, "/", "dummy", 0))?; // add dummy item so ids start with 1
-        history.save(&create_item(1, "/home/me", "cd ~/Downloads", 0))?; // 1
-        history.save(&create_item(1, "/home/me/Downloads", "unzp foo.zip", 1))?; // 2
-        history.save(&create_item(1, "/home/me/Downloads", "unzip foo.zip", 0))?; // 3
-        history.save(&create_item(1, "/home/me/Downloads", "cd foo", 0))?; // 4
-        history.save(&create_item(1, "/home/me/Downloads/foo", "ls", 0))?; // 5
-        history.save(&create_item(1, "/home/me/Downloads/foo", "ls -alh", 0))?; // 6
-        history.save(&create_item(1, "/home/me/Downloads/foo", "cat x.txt", 0))?; // 7
-
-        history.save(&create_item(1, "/home/me", "cd /etc/nginx", 0))?; // 8
-        history.save(&create_item(1, "/etc/nginx", "ls -l", 0))?; // 9
-        history.save(&create_item(1, "/etc/nginx", "vim nginx.conf", 0))?; // 10
-        history.save(&create_item(1, "/etc/nginx", "vim htpasswd", 0))?; // 11
-        history.save(&create_item(1, "/etc/nginx", "cat nginx.conf", 0))?; // 12
+        create_item(&mut history, 1, "/", "dummy", 0)?; // add dummy item so ids start with 1
+
+        create_item(&mut history, 1, "/home/me", "cd ~/Downloads", 0)?; // 1
+        create_item(&mut history, 1, "/home/me/Downloads", "unzp foo.zip", 1)?; // 2
+        create_item(&mut history, 1, "/home/me/Downloads", "unzip foo.zip", 0)?; // 3
+        create_item(&mut history, 1, "/home/me/Downloads", "cd foo", 0)?; // 4
+        create_item(&mut history, 1, "/home/me/Downloads/foo", "ls", 0)?; // 5
+        create_item(&mut history, 1, "/home/me/Downloads/foo", "ls -alh", 0)?; // 6
+        create_item(&mut history, 1, "/home/me/Downloads/foo", "cat x.txt", 0)?; // 7
+
+        create_item(&mut history, 1, "/home/me", "cd /etc/nginx", 0)?; // 8
+        create_item(&mut history, 1, "/etc/nginx", "ls -l", 0)?; // 9
+        create_item(&mut history, 1, "/etc/nginx", "vim nginx.conf", 0)?; // 10
+        create_item(&mut history, 1, "/etc/nginx", "vim htpasswd", 0)?; // 11
+        create_item(&mut history, 1, "/etc/nginx", "cat nginx.conf", 0)?; // 12
+
         Ok(Box::new(history))
     }
 
@@ -420,8 +434,8 @@ mod test {
 
         // create history, add a few entries
         let mut history = open_history();
-        history.save(&create_item(1, "/home/me", "cd ~/Downloads", 0))?; // 1
-        history.save(&create_item(1, "/home/me/Downloads", "unzp foo.zip", 1))?; // 2
+        create_item(history.as_mut(), 1, "/home/me", "cd ~/Downloads", 0)?; // 1
+        create_item(history.as_mut(), 1, "/home/me/Downloads", "unzp foo.zip", 1)?; // 2
         assert_eq!(history.count_all()?, 2);
         drop(history);
 
diff --git a/src/history/cursor.rs b/src/history/cursor.rs
index 8cb7e0b3..5d8284e3 100644
--- a/src/history/cursor.rs
+++ b/src/history/cursor.rs
@@ -468,27 +468,35 @@ mod tests {
         let expected_truncated_entries = vec!["test 4", "test 5", "test 6", "test 7", "test 8"];
 
         {
+            println!("> Creating writing history...");
             let (mut writing_hist, _) = create_history_at(capacity, &histfile);
             add_text_entries(writing_hist.as_mut(), &initial_entries);
             // As `hist` goes out of scope and get's dropped, its contents are flushed to disk
+            println!("> Flushing writing history...");
         }
 
         {
+            println!("> Creating appending history...");
             let (mut appending_hist, _) = create_history_at(capacity, &histfile);
             add_text_entries(appending_hist.as_mut(), &appending_entries);
             // As `hist` goes out of scope and get's dropped, its contents are flushed to disk
             let actual: Vec<_> = get_all_entry_texts(appending_hist.as_ref());
             assert_eq!(expected_appended_entries, actual);
+            println!("> Flushing appending history...");
         }
 
         {
+            println!("> Creating truncating history...");
             let (mut truncating_hist, _) = create_history_at(capacity, &histfile);
             add_text_entries(truncating_hist.as_mut(), &truncating_entries);
             let actual: Vec<_> = get_all_entry_texts(truncating_hist.as_ref());
             assert_eq!(expected_truncated_entries, actual);
             // As `hist` goes out of scope and get's dropped, its contents are flushed to disk
+
+            println!("> Flushing truncating history...");
         }
 
+        println!("> Creating reading history...");
         let (reading_hist, _) = create_history_at(capacity, &histfile);
 
         let actual: Vec<_> = get_all_entry_texts(reading_hist.as_ref());
diff --git a/src/history/file_backed.rs b/src/history/file_backed.rs
index 72a0e4c6..5b27c6f6 100644
--- a/src/history/file_backed.rs
+++ b/src/history/file_backed.rs
@@ -1,4 +1,5 @@
 use indexmap::IndexMap;
+use itertools::Itertools;
 use rand::{rngs::SmallRng, RngCore, SeedableRng};
 
 use super::{
@@ -79,10 +80,12 @@ impl History for FileBackedHistory {
             .map_or(true, |(_, previous)| previous != &entry)
             && !entry.is_empty()
         {
-            if self.entries.len() == self.capacity {
+            if self.entries.len() >= self.capacity {
                 // History is "full", so we delete the oldest entry first,
                 // before adding a new one.
-                self.entries.shift_remove(&HistoryItemId(0));
+                let first_id = *(self.entries.first().unwrap().0);
+                let prev = self.entries.shift_remove(&first_id);
+                assert!(prev.is_some());
             }
 
             self.entries.insert(h.id, entry.to_string());
@@ -97,6 +100,8 @@ impl History for FileBackedHistory {
     }
 
     fn load(&self, id: HistoryItemId) -> Result<HistoryItem> {
+        println!("{:?}", self.entries);
+
         Ok(FileBackedHistory::construct_entry(
             id,
             self.entries
@@ -108,9 +113,9 @@ impl History for FileBackedHistory {
         ))
     }
 
-    fn count(&self, query: SearchQuery) -> Result<i64> {
+    fn count(&self, query: SearchQuery) -> Result<u64> {
         // todo: this could be done cheaper
-        Ok(self.search(query)?.len() as i64)
+        Ok(self.search(query)?.len() as u64)
     }
 
     fn search(&self, query: SearchQuery) -> Result<Vec<HistoryItem>> {
@@ -157,11 +162,7 @@ impl History for FileBackedHistory {
             None => self.entries.len().saturating_sub(1),
         };
 
-        if from_index == to_index {
-            return Ok(vec![]);
-        }
-
-        assert!(from_index < to_index);
+        assert!(from_index <= to_index);
 
         let iter = self
             .entries
@@ -336,6 +337,9 @@ impl History for FileBackedHistory {
 
         self.entries = foreign_entries;
 
+        println!("|- Result         : {}", self.entries.values().join(" ; "));
+        println!();
+
         self.last_on_disk = self.entries.last().map(|(id, _)| *id);
 
         Ok(())
diff --git a/src/history/sqlite_backed.rs b/src/history/sqlite_backed.rs
index 8c22fb3b..17e640ac 100644
--- a/src/history/sqlite_backed.rs
+++ b/src/history/sqlite_backed.rs
@@ -110,24 +110,30 @@ impl History for SqliteBackedHistory {
             .map_err(map_sqlite_err)?
             .query_row(named_params! { ":id": id.0 }, deserialize_history_item)
             .map_err(map_sqlite_err)?;
+
         Ok(entry)
     }
 
-    fn count(&self, query: SearchQuery) -> Result<i64> {
+    fn count(&self, query: SearchQuery) -> Result<u64> {
         let (query, params) = self.construct_query(&query, "coalesce(count(*), 0)");
+
         let params_borrow: Vec<(&str, &dyn ToSql)> = params.iter().map(|e| (e.0, &*e.1)).collect();
-        let result: i64 = self
+
+        let result: u64 = self
             .db
             .prepare(&query)
             .unwrap()
             .query_row(&params_borrow[..], |r| r.get(0))
             .map_err(map_sqlite_err)?;
+
         Ok(result)
     }
 
     fn search(&self, query: SearchQuery) -> Result<Vec<HistoryItem>> {
         let (query, params) = self.construct_query(&query, "*");
+
         let params_borrow: Vec<(&str, &dyn ToSql)> = params.iter().map(|e| (e.0, &*e.1)).collect();
+
         let results: Vec<HistoryItem> = self
             .db
             .prepare(&query)

From 0da1a63689a6a2979e25eda5f79ee236ae40ba44 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= <clement.nerma@gmail.com>
Date: Fri, 8 Dec 2023 10:35:46 +0100
Subject: [PATCH 09/18] Simplify IDs handling in tests

---
 src/history/base.rs | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/src/history/base.rs b/src/history/base.rs
index e3c12cbb..1f96b7de 100644
--- a/src/history/base.rs
+++ b/src/history/base.rs
@@ -228,11 +228,6 @@ pub trait History: Send {
 
 #[cfg(test)]
 mod test {
-    #[cfg(any(feature = "sqlite", feature = "sqlite-dynlib"))]
-    const IS_FILE_BASED: bool = false;
-    #[cfg(not(any(feature = "sqlite", feature = "sqlite-dynlib")))]
-    const IS_FILE_BASED: bool = true;
-
     use std::time::Duration;
 
     use crate::HistorySessionId;
@@ -244,10 +239,10 @@ mod test {
         cmd: &str,
         exit_status: i64,
     ) -> Result<()> {
-        let len = hist.count_all()?;
+        let id = hist.count_all()? + 1;
 
         let item = HistoryItem {
-            id: HistoryItemId(len as i64),
+            id: HistoryItemId(id as i64),
             start_timestamp: None,
             command_line: cmd.to_string(),
             session_id: Some(HistorySessionId::new(session)),
@@ -260,7 +255,8 @@ mod test {
 
         hist.save(&item)?;
 
-        assert_eq!(hist.count_all()?, len + 1);
+        // Ensure the item was correctly inserted
+        assert_eq!(hist.count_all()?, id);
 
         Ok(())
     }
@@ -273,9 +269,6 @@ mod test {
         #[cfg(not(any(feature = "sqlite", feature = "sqlite-dynlib")))]
         let mut history = crate::FileBackedHistory::default();
 
-        #[cfg(not(any(feature = "sqlite", feature = "sqlite-dynlib")))]
-        create_item(&mut history, 1, "/", "dummy", 0)?; // add dummy item so ids start with 1
-
         create_item(&mut history, 1, "/home/me", "cd ~/Downloads", 0)?; // 1
         create_item(&mut history, 1, "/home/me/Downloads", "unzp foo.zip", 1)?; // 2
         create_item(&mut history, 1, "/home/me/Downloads", "unzip foo.zip", 0)?; // 3
@@ -323,7 +316,9 @@ mod test {
             .iter()
             .map(|id| history.load(HistoryItemId::new(*id)))
             .collect::<Result<Vec<HistoryItem>>>()?;
+
         assert_eq!(res, wanted);
+
         Ok(())
     }
 
@@ -335,7 +330,7 @@ mod test {
             history.search(SearchQuery::everything(SearchDirection::Forward, None))
         );
 
-        assert_eq!(history.count_all()?, if IS_FILE_BASED { 13 } else { 12 });
+        assert_eq!(history.count_all()?, 12);
         Ok(())
     }
 
@@ -355,7 +350,7 @@ mod test {
             limit: Some(1),
             ..SearchQuery::everything(SearchDirection::Forward, None)
         })?;
-        search_returned(&*history, res, vec![if IS_FILE_BASED { 0 } else { 1 }])?;
+        search_returned(&*history, res, vec![1])?;
         Ok(())
     }
 

From c7ad9c00f3d23d4f693f62e690cb32b7f0e194d1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= <clement.nerma@gmail.com>
Date: Fri, 8 Dec 2023 11:58:11 +0100
Subject: [PATCH 10/18] Add migration for SQLite backend

---
 src/history/sqlite_backed.rs | 142 +++++++++++++++++++++++------------
 1 file changed, 95 insertions(+), 47 deletions(-)

diff --git a/src/history/sqlite_backed.rs b/src/history/sqlite_backed.rs
index 17e640ac..6ad206df 100644
--- a/src/history/sqlite_backed.rs
+++ b/src/history/sqlite_backed.rs
@@ -230,63 +230,111 @@ impl SqliteBackedHistory {
     }
     /// initialize a new database / migrate an existing one
     fn from_connection(
-        db: Connection,
+        mut db: Connection,
         session: Option<HistorySessionId>,
         session_timestamp: Option<chrono::DateTime<Utc>>,
     ) -> Result<Self> {
-        // https://phiresky.github.io/blog/2020/sqlite-performance-tuning/
-        db.pragma_update(None, "journal_mode", "wal")
-            .map_err(map_sqlite_err)?;
-        db.pragma_update(None, "synchronous", "normal")
-            .map_err(map_sqlite_err)?;
-        db.pragma_update(None, "mmap_size", "1000000000")
-            .map_err(map_sqlite_err)?;
-        db.pragma_update(None, "foreign_keys", "on")
-            .map_err(map_sqlite_err)?;
-        db.pragma_update(None, "application_id", SQLITE_APPLICATION_ID)
-            .map_err(map_sqlite_err)?;
-        let db_version: i32 = db
-            .query_row(
+        let inner = || -> rusqlite::Result<(i32, Self)> {
+            // https://phiresky.github.io/blog/2020/sqlite-performance-tuning/
+            db.pragma_update(None, "journal_mode", "wal")?;
+            db.pragma_update(None, "synchronous", "normal")?;
+            db.pragma_update(None, "mmap_size", "1000000000")?;
+            db.pragma_update(None, "foreign_keys", "on")?;
+            db.pragma_update(None, "application_id", SQLITE_APPLICATION_ID)?;
+
+            let db_version: i32 = db.query_row(
                 "SELECT user_version FROM pragma_user_version",
                 params![],
                 |r| r.get(0),
-            )
-            .map_err(map_sqlite_err)?;
-        if db_version != 0 {
+            )?;
+
+            if db_version == 0 {
+                let existing_history = db.query_row(
+                    "select count(*) from pragma_table_list() where name = 'history';",
+                    (),
+                    |result| Ok(result.get::<_, usize>("count(*)")? > 0),
+                )?;
+
+                let mut statements = vec![];
+
+                if existing_history {
+                    statements.push(
+                        "
+                        alter table history rename to history_old;
+
+                        drop index if exists idx_history_cwd;
+                        drop index if exists idx_history_exit_status;
+                        drop index if exists idx_history_cmd;
+                        drop index if exists idx_history_cmd;
+                        ",
+                    );
+                }
+
+                statements.push(
+                    "
+                    create table history (
+                        idx integer primary key autoincrement,
+                        id integer unique not null,
+                        command_line text not null,
+                        start_timestamp integer,
+                        session_id integer,
+                        hostname text,
+                        cwd text,
+                        duration_ms integer,
+                        exit_status integer,
+                        more_info text
+                    ) strict;
+                    
+                    create index if not exists idx_history_time on history(start_timestamp);
+                    create index if not exists idx_history_cwd on history(cwd); -- suboptimal for many hosts
+                    create index if not exists idx_history_exit_status on history(exit_status);
+                    create index if not exists idx_history_cmd on history(command_line);
+                    create index if not exists idx_history_cmd on history(session_id);
+                    ",
+                )
+                ;
+
+                if existing_history {
+                    statements.push(
+                        "
+                        insert into history (id, command_line, start_timestamp, session_id, hostname, cwd, duration_ms, exit_status, more_info)
+                        select id as idx, command_line, start_timestamp, session_id, hostname, cwd, duration_ms, exit_status, more_info
+                        from history_old;
+
+                        drop table history_old;
+                        ",
+                    );
+                }
+
+                statements.push("pragma user_version = 1;");
+
+                // TODO: update db version to 1
+
+                let transaction = db.transaction()?;
+                transaction.execute_batch(&statements.join("\n"))?;
+                transaction.commit()?;
+            }
+
+            Ok((
+                db_version,
+                SqliteBackedHistory {
+                    db,
+                    session,
+                    session_timestamp,
+                    rng: SmallRng::from_entropy(),
+                },
+            ))
+        };
+
+        let (db_version, history) = inner().map_err(map_sqlite_err)?;
+
+        if db_version > 1 {
             return Err(ReedlineError(ReedlineErrorVariants::HistoryDatabaseError(
                 format!("Unknown database version {db_version}"),
             )));
         }
-        db.execute_batch(
-            "
-        create table if not exists history (
-            idx integer primary key autoincrement,
-            id integer unique not null,
-            command_line text not null,
-            start_timestamp integer,
-            session_id integer,
-            hostname text,
-            cwd text,
-            duration_ms integer,
-            exit_status integer,
-            more_info text
-        ) strict;
-        
-        create index if not exists idx_history_time on history(start_timestamp);
-        create index if not exists idx_history_cwd on history(cwd); -- suboptimal for many hosts
-        create index if not exists idx_history_exit_status on history(exit_status);
-        create index if not exists idx_history_cmd on history(command_line);
-        create index if not exists idx_history_cmd on history(session_id);
-        ",
-        )
-        .map_err(map_sqlite_err)?;
-
-        Ok(SqliteBackedHistory {
-            db,
-            session,
-            session_timestamp,
-            rng: SmallRng::from_entropy(),
-        })
+
+        Ok(history)
     }
 
     fn construct_query<'a>(

From d40c335cc74f7b8b7472148a00c8ad4142c35f82 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= <clement.nerma@gmail.com>
Date: Fri, 8 Dec 2023 12:16:19 +0100
Subject: [PATCH 11/18] Add support for legacy file text format

---
 src/history/file_backed.rs | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/src/history/file_backed.rs b/src/history/file_backed.rs
index 5b27c6f6..5750cc92 100644
--- a/src/history/file_backed.rs
+++ b/src/history/file_backed.rs
@@ -46,22 +46,22 @@ impl Default for FileBackedHistory {
     }
 }
 
+static ID_SEP: &str = "<id>:";
+
 fn encode_entry(id: HistoryItemId, s: &str) -> String {
-    format!("{id}:{}", s.replace('\n', NEWLINE_ESCAPE))
+    format!("{id}{ID_SEP}{}", s.replace('\n', NEWLINE_ESCAPE))
 }
 
-fn decode_entry(s: &str) -> std::result::Result<(HistoryItemId, String), &'static str> {
-    let sep = s
-        .bytes()
-        .position(|b| b == b':')
-        .ok_or("History item ID is missing")?;
+fn decode_entry(s: &str, counter: &mut i64) -> (HistoryItemId, String) {
+    if let Some(sep) = s.find(ID_SEP) {
+        if let Ok(id) = s[..sep].parse() {
+            return (HistoryItemId(id), s[sep + ID_SEP.len()..].to_owned());
+        }
+    }
 
-    let id = s[..sep].parse().map_err(|_| "Invalid history item ID")?;
+    *counter += 1;
 
-    Ok((
-        HistoryItemId(id),
-        s[sep + 1..].replace(NEWLINE_ESCAPE, "\n"),
-    ))
+    (HistoryItemId(*counter - 1), s.to_owned())
 }
 
 impl History for FileBackedHistory {
@@ -277,9 +277,11 @@ impl History for FileBackedHistory {
         let (mut foreign_entries, truncate) = {
             let reader = BufReader::new(writer_guard.deref());
 
+            let mut counter = 0;
+
             let mut from_file = reader
                 .lines()
-                .map(|o| o.map(|i| decode_entry(&i).expect("todo: error handling")))
+                .map(|o| o.map(|i| decode_entry(&i, &mut counter)))
                 .collect::<std::io::Result<IndexMap<_, _>>>()?;
 
             if from_file.len() + own_entries.len() > self.capacity {

From efcbe551b8e4f14d5293740f645c2acf377bf058 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= <clement.nerma@gmail.com>
Date: Fri, 8 Dec 2023 13:49:41 +0100
Subject: [PATCH 12/18] Fix: support for legacy file text format

---
 src/history/file_backed.rs | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/src/history/file_backed.rs b/src/history/file_backed.rs
index 5750cc92..055c9c27 100644
--- a/src/history/file_backed.rs
+++ b/src/history/file_backed.rs
@@ -52,16 +52,30 @@ fn encode_entry(id: HistoryItemId, s: &str) -> String {
     format!("{id}{ID_SEP}{}", s.replace('\n', NEWLINE_ESCAPE))
 }
 
+/// Decode an entry
+///
+/// Legacy format: ls /
+/// New format   : 182535<id>:ls /
+///
+/// If a line can't be parsed using the new format, it will fallback to the legacy one.
+///
+/// This allows this function to support decoding for both legacy and new histories,
+/// as well as mixing both of them.
 fn decode_entry(s: &str, counter: &mut i64) -> (HistoryItemId, String) {
+    let mut parsed = None;
+
     if let Some(sep) = s.find(ID_SEP) {
-        if let Ok(id) = s[..sep].parse() {
-            return (HistoryItemId(id), s[sep + ID_SEP.len()..].to_owned());
+        if let Ok(parsed_id) = s[..sep].parse() {
+            parsed = Some((parsed_id, &s[sep + ID_SEP.len()..]));
         }
     }
 
-    *counter += 1;
+    let (id, content) = parsed.unwrap_or_else(|| {
+        *counter += 1;
+        (*counter - 1, s)
+    });
 
-    (HistoryItemId(*counter - 1), s.to_owned())
+    (HistoryItemId(id), content.replace(NEWLINE_ESCAPE, "\n"))
 }
 
 impl History for FileBackedHistory {

From ff08121e8cdb4b8c751447f3348beaeaf3d18a94 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= <clement.nerma@gmail.com>
Date: Fri, 8 Dec 2023 14:05:20 +0100
Subject: [PATCH 13/18] Add comments + escaping for LIKE patterns

---
 src/history/sqlite_backed.rs | 64 +++++++++++++++++++++++++++++-------
 1 file changed, 53 insertions(+), 11 deletions(-)

diff --git a/src/history/sqlite_backed.rs b/src/history/sqlite_backed.rs
index 6ad206df..5d46834f 100644
--- a/src/history/sqlite_backed.rs
+++ b/src/history/sqlite_backed.rs
@@ -242,13 +242,18 @@ impl SqliteBackedHistory {
             db.pragma_update(None, "foreign_keys", "on")?;
             db.pragma_update(None, "application_id", SQLITE_APPLICATION_ID)?;
 
+            // Get the user version
+            // By default, it is set to 0
             let db_version: i32 = db.query_row(
                 "SELECT user_version FROM pragma_user_version",
                 params![],
                 |r| r.get(0),
             )?;
 
+            // An up-to-date database should have its version set to the latest number (currently 1)
+            // 0 means the database is either uninitialized or it is using the old history format
             if db_version == 0 {
+                // Check if an history already exists
                 let existing_history = db.query_row(
                     "select count(*) from pragma_table_list() where name = 'history';",
                     (),
@@ -257,6 +262,7 @@ impl SqliteBackedHistory {
 
                 let mut statements = vec![];
 
+                // If so, rename it and delete related indexes
                 if existing_history {
                     statements.push(
                         "
@@ -270,6 +276,7 @@ impl SqliteBackedHistory {
                     );
                 }
 
+                // Create the history table using the v1 schema
                 statements.push(
                     "
                     create table history (
@@ -291,9 +298,10 @@ impl SqliteBackedHistory {
                     create index if not exists idx_history_cmd on history(command_line);
                     create index if not exists idx_history_cmd on history(session_id);
                     ",
-                )
-                ;
+                );
 
+                // If there was an history previously, migrate it to the new table
+                // Then delete it
                 if existing_history {
                     statements.push(
                         "
@@ -306,10 +314,10 @@ impl SqliteBackedHistory {
                     );
                 }
 
+                // Update the version to indicate the DB is up-to-date
                 statements.push("pragma user_version = 1;");
 
-                // TODO: update db version to 1
-
+                // We use a transaction to ensure consistency, given we're doing multiple operations
                 let transaction = db.transaction()?;
                 transaction.execute_batch(&statements.join("\n"))?;
                 transaction.commit()?;
@@ -326,9 +334,14 @@ impl SqliteBackedHistory {
             ))
         };
 
+        // Connect to the database, check it and (if required) initialize it
         let (db_version, history) = inner().map_err(map_sqlite_err)?;
 
-        if db_version > 1 {
+        // Ensure the database version is the currently supported one
+        // If this isn't the case, then something is wrong
+        // (either the previous versions migration is buggy, or the database is using a format deployed on a
+        //  later reedline version than this one)
+        if db_version != 1 {
             return Err(ReedlineError(ReedlineErrorVariants::HistoryDatabaseError(
                 format!("Unknown database version {db_version}"),
             )));
@@ -342,6 +355,8 @@ impl SqliteBackedHistory {
         query: &'a SearchQuery,
         select_expression: &str,
     ) -> (String, BoxedNamedParams<'a>) {
+        // Destructure the query - this ensures that if another element is added to this type later on,
+        // we won't forget to update this function as the destructuring will then be incomplete.
         let SearchQuery {
             direction,
             start_time,
@@ -352,12 +367,14 @@ impl SqliteBackedHistory {
             filter,
         } = query;
 
-        // TODO: this whole function could be done with less allocs
         let (is_asc, asc) = match direction {
             SearchDirection::Forward => (true, "asc"),
             SearchDirection::Backward => (false, "desc"),
         };
 
+        // TODO: find a way to avoid too many allocations
+        // Current version is an acceptable compromise given most of the performance
+        // will be eaten by SQLite anyway
         let mut wheres = Vec::new();
         let mut params: BoxedNamedParams = Vec::new();
 
@@ -366,11 +383,13 @@ impl SqliteBackedHistory {
             wheres.push(format!("timestamp_start {cmp_op} :start_time"));
             params.push((":start_time", Box::new(start.timestamp_millis())));
         }
+
         if let Some(end) = end_time {
             let cmp_op = if is_asc { ">=" } else { "<=" };
             wheres.push(format!(":end_time {cmp_op} timestamp_start"));
             params.push((":end_time", Box::new(end.timestamp_millis())));
         }
+
         if let Some(start) = start_id {
             let cmp_op = if is_asc { '>' } else { '<' };
             wheres.push(format!(
@@ -378,6 +397,7 @@ impl SqliteBackedHistory {
             ));
             params.push((":start_id", Box::new(start.0)));
         }
+
         if let Some(end) = end_id {
             let cmp_op = if is_asc { ">=" } else { "<=" };
             wheres.push(format!(
@@ -385,6 +405,7 @@ impl SqliteBackedHistory {
             ));
             params.push((":end_id", Box::new(end.0)));
         }
+
         let limit = match limit {
             Some(l) => {
                 params.push((":limit", Box::new(l)));
@@ -392,14 +413,21 @@ impl SqliteBackedHistory {
             }
             None => "",
         };
+
         if let Some(command_line) = &filter.command_line {
-            // TODO: escape %
             let command_line_like = match command_line {
-                CommandLineSearch::Exact(e) => e.to_string(),
-                CommandLineSearch::Prefix(prefix) => format!("{prefix}%"),
-                CommandLineSearch::Substring(cont) => format!("%{cont}%"),
+                CommandLineSearch::Exact(e) => escape_like_with_backslashes(e, ESCAPE_CHAR),
+                CommandLineSearch::Prefix(prefix) => {
+                    format!("{}%", escape_like_with_backslashes(prefix, ESCAPE_CHAR))
+                }
+                CommandLineSearch::Substring(cont) => {
+                    format!("%{}%", escape_like_with_backslashes(cont, ESCAPE_CHAR))
+                }
             };
-            wheres.push("command_line like :command_line".to_owned());
+
+            wheres.push(format!(
+                "command_line like :command_line escape '{ESCAPE_CHAR}'"
+            ));
             params.push((":command_line", Box::new(command_line_like)));
         }
 
@@ -458,3 +486,17 @@ impl SqliteBackedHistory {
         (query, params)
     }
 }
+
+/// Escape special symbols for an SQL LIKE clause
+/// (!) Requires LIKE clause to specify an `ESCAPE '<char>'` clause
+fn escape_like_with_backslashes(str: &str, escape_char: char) -> String {
+    let mut str = str.replace(escape_char, &format!("{escape_char}{escape_char}"));
+
+    for forbidden in ['%', '\'', '\n'] {
+        str = str.replace(forbidden, &format!("{escape_char}{forbidden}"));
+    }
+
+    str
+}
+
+static ESCAPE_CHAR: char = '\\';

From 666e4e491edb0ce249190b5b7adaa537ecb50036 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= <clement.nerma@gmail.com>
Date: Fri, 8 Dec 2023 14:06:23 +0100
Subject: [PATCH 14/18] Fix: database's 'user_version' pragma checking

---
 src/history/sqlite_backed.rs | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/history/sqlite_backed.rs b/src/history/sqlite_backed.rs
index 5d46834f..a04ca571 100644
--- a/src/history/sqlite_backed.rs
+++ b/src/history/sqlite_backed.rs
@@ -244,7 +244,7 @@ impl SqliteBackedHistory {
 
             // Get the user version
             // By default, it is set to 0
-            let db_version: i32 = db.query_row(
+            let mut db_version: i32 = db.query_row(
                 "SELECT user_version FROM pragma_user_version",
                 params![],
                 |r| r.get(0),
@@ -316,6 +316,7 @@ impl SqliteBackedHistory {
 
                 // Update the version to indicate the DB is up-to-date
                 statements.push("pragma user_version = 1;");
+                db_version = 1;
 
                 // We use a transaction to ensure consistency, given we're doing multiple operations
                 let transaction = db.transaction()?;
@@ -472,10 +473,13 @@ impl SqliteBackedHistory {
                 Box::new(session_timestamp.timestamp_millis()),
             ));
         }
+
         let mut wheres = wheres.join(" and ");
+
         if wheres.is_empty() {
             wheres = "true".to_string();
         }
+
         let query = format!(
             "SELECT {select_expression} \
              FROM history \
@@ -483,6 +487,7 @@ impl SqliteBackedHistory {
              ORDER BY idx {asc} \
              {limit}"
         );
+
         (query, params)
     }
 }

From a61720d30f866210ecd2ff5153cd6d273ff1e1be Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= <clement.nerma@gmail.com>
Date: Fri, 8 Dec 2023 15:24:13 +0100
Subject: [PATCH 15/18] Remove debug instructions

---
 src/history/file_backed.rs | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/src/history/file_backed.rs b/src/history/file_backed.rs
index 055c9c27..fac7874d 100644
--- a/src/history/file_backed.rs
+++ b/src/history/file_backed.rs
@@ -1,5 +1,4 @@
 use indexmap::IndexMap;
-use itertools::Itertools;
 use rand::{rngs::SmallRng, RngCore, SeedableRng};
 
 use super::{
@@ -353,9 +352,6 @@ impl History for FileBackedHistory {
 
         self.entries = foreign_entries;
 
-        println!("|- Result         : {}", self.entries.values().join(" ; "));
-        println!();
-
         self.last_on_disk = self.entries.last().map(|(id, _)| *id);
 
         Ok(())

From d3317ac125f91ee7bacdda7ada649dd1719dc2c1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= <clement.nerma@gmail.com>
Date: Fri, 8 Dec 2023 16:33:27 +0100
Subject: [PATCH 16/18] Use .gen() method on SmallRng instead of casting

---
 src/history/file_backed.rs   | 4 ++--
 src/history/sqlite_backed.rs | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/history/file_backed.rs b/src/history/file_backed.rs
index fac7874d..bc90e82a 100644
--- a/src/history/file_backed.rs
+++ b/src/history/file_backed.rs
@@ -1,5 +1,5 @@
 use indexmap::IndexMap;
-use rand::{rngs::SmallRng, RngCore, SeedableRng};
+use rand::{rngs::SmallRng, Rng, SeedableRng};
 
 use super::{
     base::CommandLineSearch, History, HistoryItem, HistoryItemId, SearchDirection, SearchQuery,
@@ -79,7 +79,7 @@ fn decode_entry(s: &str, counter: &mut i64) -> (HistoryItemId, String) {
 
 impl History for FileBackedHistory {
     fn generate_id(&mut self) -> HistoryItemId {
-        HistoryItemId(self.rng.next_u64() as i64)
+        HistoryItemId(self.rng.gen())
     }
 
     /// only saves a value if it's different than the last value
diff --git a/src/history/sqlite_backed.rs b/src/history/sqlite_backed.rs
index a04ca571..556404fe 100644
--- a/src/history/sqlite_backed.rs
+++ b/src/history/sqlite_backed.rs
@@ -7,7 +7,7 @@ use crate::{
     Result,
 };
 use chrono::{TimeZone, Utc};
-use rand::{rngs::SmallRng, RngCore, SeedableRng};
+use rand::{rngs::SmallRng, Rng, SeedableRng};
 use rusqlite::{named_params, params, Connection, ToSql};
 use std::{path::PathBuf, time::Duration};
 const SQLITE_APPLICATION_ID: i32 = 1151497937;
@@ -60,7 +60,7 @@ fn deserialize_history_item(row: &rusqlite::Row) -> rusqlite::Result<HistoryItem
 
 impl History for SqliteBackedHistory {
     fn generate_id(&mut self) -> HistoryItemId {
-        HistoryItemId(self.rng.next_u64() as i64)
+        HistoryItemId(self.rng.gen())
     }
 
     fn save(&mut self, entry: &HistoryItem) -> Result<()> {

From f8024dd30d7e654953e80c70c02a2b0827039fd4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= <clement.nerma@gmail.com>
Date: Fri, 8 Dec 2023 19:24:06 +0100
Subject: [PATCH 17/18] Improve search query handling + remove TODO

---
 src/history/file_backed.rs | 62 +++++++++++++++++++++++---------------
 1 file changed, 37 insertions(+), 25 deletions(-)

diff --git a/src/history/file_backed.rs b/src/history/file_backed.rs
index bc90e82a..27278768 100644
--- a/src/history/file_backed.rs
+++ b/src/history/file_backed.rs
@@ -132,7 +132,19 @@ impl History for FileBackedHistory {
     }
 
     fn search(&self, query: SearchQuery) -> Result<Vec<HistoryItem>> {
-        if query.start_time.is_some() || query.end_time.is_some() {
+        // Destructure the query - this ensures that if another element is added to this type later on,
+        // we won't forget to update this function as the destructuring will then be incomplete.
+        let SearchQuery {
+            direction,
+            start_time,
+            end_time,
+            start_id,
+            end_id,
+            limit,
+            filter,
+        } = query;
+
+        if start_time.is_some() || end_time.is_some() {
             return Err(ReedlineError(
                 ReedlineErrorVariants::HistoryFeatureUnsupported {
                     history: "FileBackedHistory",
@@ -141,10 +153,10 @@ impl History for FileBackedHistory {
             ));
         }
 
-        if query.filter.hostname.is_some()
-            || query.filter.cwd_exact.is_some()
-            || query.filter.cwd_prefix.is_some()
-            || query.filter.exit_successful.is_some()
+        if filter.hostname.is_some()
+            || filter.cwd_exact.is_some()
+            || filter.cwd_prefix.is_some()
+            || filter.exit_successful.is_some()
         {
             return Err(ReedlineError(
                 ReedlineErrorVariants::HistoryFeatureUnsupported {
@@ -154,42 +166,42 @@ impl History for FileBackedHistory {
             ));
         }
 
-        let (from_id, to_id) = {
-            let start = query.start_id;
-            let end = query.end_id;
-
-            if let SearchDirection::Backward = query.direction {
-                (end, start)
+        let (start_id, end_id) = {
+            if let SearchDirection::Backward = direction {
+                (end_id, start_id)
             } else {
-                (start, end)
+                (start_id, end_id)
             }
         };
 
-        let from_index = match from_id {
-            Some(from_id) => self.entries.get_index_of(&from_id).expect("todo"),
+        let start_idx = match start_id {
+            Some(from_id) => self.entries.get_index_of(&from_id).ok_or(ReedlineError(
+                ReedlineErrorVariants::OtherHistoryError("provided 'start_id' item was not found"),
+            ))?,
             None => 0,
         };
 
-        let to_index = match to_id {
-            Some(to_id) => self.entries.get_index_of(&to_id).expect("todo"),
+        let end_idx = match end_id {
+            Some(to_id) => self.entries.get_index_of(&to_id).ok_or(ReedlineError(
+                ReedlineErrorVariants::OtherHistoryError("provided 'end_id' item was not found"),
+            ))?,
             None => self.entries.len().saturating_sub(1),
         };
 
-        assert!(from_index <= to_index);
+        assert!(start_idx <= end_idx);
 
         let iter = self
             .entries
             .iter()
-            .skip(from_index)
-            .take(1 + to_index - from_index);
+            .skip(start_idx)
+            .take(1 + end_idx - start_idx);
 
-        let limit = match query.limit {
-            Some(limit) => usize::try_from(limit).unwrap(),
-            None => usize::MAX,
-        };
+        let limit = limit
+            .and_then(|limit| usize::try_from(limit).ok())
+            .unwrap_or(usize::MAX);
 
         let filter = |(id, cmd): (&HistoryItemId, &String)| {
-            let str_matches = match &query.filter.command_line {
+            let str_matches = match &filter.command_line {
                 Some(CommandLineSearch::Prefix(p)) => cmd.starts_with(p),
                 Some(CommandLineSearch::Substring(p)) => cmd.contains(p),
                 Some(CommandLineSearch::Exact(p)) => cmd == p,
@@ -200,7 +212,7 @@ impl History for FileBackedHistory {
                 return None;
             }
 
-            if let Some(str) = &query.filter.not_command_line {
+            if let Some(str) = &filter.not_command_line {
                 if cmd == str {
                     return None;
                 }

From eff5630da2a0ba29c3554dd89bbe34e420f6f3ba Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= <clement.nerma@gmail.com>
Date: Fri, 8 Dec 2023 19:29:09 +0100
Subject: [PATCH 18/18] Fix: missing index drop + typo

---
 src/history/sqlite_backed.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/history/sqlite_backed.rs b/src/history/sqlite_backed.rs
index 556404fe..8eea55a6 100644
--- a/src/history/sqlite_backed.rs
+++ b/src/history/sqlite_backed.rs
@@ -268,10 +268,10 @@ impl SqliteBackedHistory {
                         "
                         alter table history rename to history_old;
 
+                        drop index if exists idx_history_time;
                         drop index if exists idx_history_cwd;
                         drop index if exists idx_history_exit_status;
                         drop index if exists idx_history_cmd;
-                        drop index if exists idx_history_cmd;
                         ",
                     );
                 }
@@ -296,7 +296,7 @@ impl SqliteBackedHistory {
                     create index if not exists idx_history_cwd on history(cwd); -- suboptimal for many hosts
                     create index if not exists idx_history_exit_status on history(exit_status);
                     create index if not exists idx_history_cmd on history(command_line);
-                    create index if not exists idx_history_cmd on history(session_id);
+                    create index if not exists idx_history_session_id on history(session_id);
                     ",
                 );