From 00df4bc6d64e9768d3e92746ea599d397d95f538 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Wed, 31 Jan 2024 11:02:59 +0100 Subject: [PATCH 01/29] update documentation for `HistoryItemId` --- src/history/item.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/history/item.rs b/src/history/item.rs index d94af11b..ec1daae5 100644 --- a/src/history/item.rs +++ b/src/history/item.rs @@ -4,7 +4,10 @@ use rusqlite::ToSql; use serde::{de::DeserializeOwned, Deserialize, Serialize}; use std::{fmt::Display, time::Duration}; -/// Unique ID for the [`HistoryItem`]. More recent items have higher ids than older ones. +/// Unique ID for the [`HistoryItem`]. +/// These are generated pseudo-randomly. +/// Note that the ID of a given item may change between program executions. +/// These IDs only aim to uniquely identify a single item *for the program's execution*. #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)] pub struct HistoryItemId(pub i64); impl HistoryItemId { From b8d522beb984eb42896710ec83d8aad5382b1f86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Wed, 31 Jan 2024 11:03:38 +0100 Subject: [PATCH 02/29] allow to create `HistorySessionId` from the outside --- src/history/item.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/history/item.rs b/src/history/item.rs index ec1daae5..29c7d823 100644 --- a/src/history/item.rs +++ b/src/history/item.rs @@ -25,9 +25,10 @@ impl Display for HistoryItemId { /// Unique ID for the session in which reedline was run to disambiguate different sessions #[derive(Debug, Copy, Clone, PartialEq, Eq, Serialize, Deserialize)] -pub struct HistorySessionId(pub(crate) i64); +pub struct HistorySessionId(pub i64); impl HistorySessionId { - pub(crate) const fn new(i: i64) -> HistorySessionId { + /// Create a new `HistorySessionId` + pub const fn new(i: i64) -> HistorySessionId { HistorySessionId(i) } } From a23202bb6e69303d06519f008063664a85db7b58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Thu, 7 Dec 2023 13:07:58 +0100 Subject: [PATCH 03/29] Improve history API --- 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 +++++++++------ 7 files changed, 158 insertions(+), 108 deletions(-) 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 io::Result { 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())?; @@ -1774,25 +1775,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 4c07417b..774e1bfa 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; + /// 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; @@ -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> { @@ -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) -> Result<()> { + let id = hist.generate_id(); + let entry = HistoryItem::from_command_line(cmd_line, id); + + hist.save(&entry) + } + fn create_history() -> (Box, 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, 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]) { 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::>(); diff --git a/src/history/file_backed.rs b/src/history/file_backed.rs index ffff840b..cb5617b8 100644 --- a/src/history/file_backed.rs +++ b/src/history/file_backed.rs @@ -58,11 +58,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 { - 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) @@ -76,16 +81,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 { + /// this history doesn't replace entries + fn replace(&mut self, h: &HistoryItem) -> Result<()> { + self.save(h) + } + + fn load(&self, id: HistoryItemId) -> Result { Ok(FileBackedHistory::construct_entry( - Some(id), + id, self.entries .get(id.0 as usize) .ok_or(ReedlineError(ReedlineErrorVariants::OtherHistoryError( @@ -161,7 +169,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 )) }; @@ -328,7 +336,7 @@ impl FileBackedHistory { } // this history doesn't store any info except command line - fn construct_entry(id: Option, 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 29c7d823..559e29bb 100644 --- a/src/history/item.rs +++ b/src/history/item.rs @@ -85,7 +85,7 @@ impl HistoryItemExtraInfo for IgnoreAllExtraInfo {} #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] pub struct HistoryItem { /// primary key, unique across one history - pub id: Option, + pub id: HistoryItemId, /// date-time when this command was started pub start_timestamp: Option>, /// the full command line as text @@ -111,9 +111,9 @@ pub struct HistoryItem { 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) -> HistoryItem { + pub fn from_command_line(cmd: impl Into, 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 ec3b021a..1620a965 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; @@ -21,12 +22,13 @@ pub struct SqliteBackedHistory { db: rusqlite::Connection, session: Option, session_timestamp: Option>, + rng: SmallRng, } fn deserialize_history_item(row: &rusqlite::Row) -> rusqlite::Result { let x: Option = 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>("start_timestamp")?.map(|e| { match Utc.timestamp_millis_opt(e) { chrono::LocalResult::Single(e) => e, @@ -59,8 +61,12 @@ fn deserialize_history_item(row: &rusqlite::Row) -> rusqlite::Result Result { - 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 @@ -74,13 +80,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), @@ -90,11 +95,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 { @@ -140,7 +148,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(()) } @@ -271,6 +279,7 @@ impl SqliteBackedHistory { db, session, session_timestamp, + rng: SmallRng::from_entropy(), }) } From ae2c007ddbd0e9c0128a01cf8d710c4ae1653af0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Thu, 7 Dec 2023 13:19:17 +0100 Subject: [PATCH 04/29] 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 cb5617b8..fda27f63 100644 --- a/src/history/file_backed.rs +++ b/src/history/file_backed.rs @@ -59,7 +59,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 6e9e3ecf303efd558c6ded6c3fd091112c9006ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Thu, 7 Dec 2023 14:23:15 +0100 Subject: [PATCH 05/29] Update `FileBackedHistory` for new history system --- src/history/file_backed.rs | 127 +++++++++++++++++++++++-------------- 1 file changed, 80 insertions(+), 47 deletions(-) diff --git a/src/history/file_backed.rs b/src/history/file_backed.rs index fda27f63..8f71f159 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, + entries: IndexMap, file: Option, - len_on_disk: usize, // Keep track what was previously written to disk + last_on_disk: Option, session: Option, } @@ -49,12 +50,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 { @@ -69,18 +77,18 @@ 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() && self.capacity > 0 { 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(()) @@ -95,7 +103,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", )))? @@ -130,31 +138,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), @@ -169,17 +165,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 { @@ -202,7 +218,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) { @@ -228,7 +244,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)?; @@ -242,12 +267,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::>>()?; + .map(|o| o.map(|i| decode_entry(&i).expect("todo: error handling"))) + .collect::>>()?; + if from_file.len() + own_entries.len() > self.capacity { ( from_file.split_off( @@ -262,33 +290,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(()) } @@ -310,9 +343,9 @@ impl FileBackedHistory { Ok(FileBackedHistory { capacity, - entries: VecDeque::new(), + entries: IndexMap::new(), file: None, - len_on_disk: 0, + last_on_disk: None, session: None, }) } From b957895664618bf00169b4f8d316e646a226756d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Thu, 7 Dec 2023 15:46:28 +0100 Subject: [PATCH 06/29] Fix: ranges and truncation --- src/history/cursor.rs | 10 ++ src/history/file_backed.rs | 220 +++++++++++++++++++++---------------- 2 files changed, 135 insertions(+), 95 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 8f71f159..d0c70b86 100644 --- a/src/history/file_backed.rs +++ b/src/history/file_backed.rs @@ -62,7 +62,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 { @@ -150,57 +153,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( @@ -242,87 +249,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::>>()?; - - if from_file.len() + own_entries.len() > self.capacity { - ( - from_file.split_off( - from_file.len() - (self.capacity.saturating_sub(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::>>()?; + + 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(()) } @@ -358,13 +380,21 @@ impl FileBackedHistory { /// /// **Side effects:** creates all nested directories to the file /// +<<<<<<< HEAD pub fn with_file(capacity: usize, file: PathBuf) -> Result { let mut hist = Self::new(capacity)?; +======= + pub fn with_file(capacity: usize, file: PathBuf) -> std::io::Result { + let mut hist = Self::new(capacity); + +>>>>>>> d0d27e8 (Fix: ranges and truncation) if let Some(base_dir) = file.parent() { std::fs::create_dir_all(base_dir)?; } + hist.file = Some(file); hist.sync()?; + Ok(hist) } From bb4bd2d0747246a4628562f827b5ba6da0f9afce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Thu, 7 Dec 2023 17:08:42 +0100 Subject: [PATCH 07/29] Fix: ID generation --- src/history/cursor.rs | 5 +++++ src/history/file_backed.rs | 19 ++++++++----------- 2 files changed, 13 insertions(+), 11 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 d0c70b86..008aa904 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, last_on_disk: Option, session: Option, + rng: SmallRng, } impl Default for FileBackedHistory { @@ -70,7 +72,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 @@ -288,10 +290,9 @@ impl History for FileBackedHistory { .collect::>>()?; 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) } @@ -369,7 +370,8 @@ impl FileBackedHistory { file: None, last_on_disk: None, session: None, - }) + rng: SmallRng::from_entropy(), + } } /// Creates a new history with an associated history file. @@ -380,14 +382,9 @@ impl FileBackedHistory { /// /// **Side effects:** creates all nested directories to the file /// -<<<<<<< HEAD - pub fn with_file(capacity: usize, file: PathBuf) -> Result { - let mut hist = Self::new(capacity)?; -======= pub fn with_file(capacity: usize, file: PathBuf) -> std::io::Result { let mut hist = Self::new(capacity); ->>>>>>> d0d27e8 (Fix: ranges and truncation) if let Some(base_dir) = file.parent() { std::fs::create_dir_all(base_dir)?; } From 295f5b6358fd7119c181a16bac0615b689dbfd7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Thu, 7 Dec 2023 17:31:15 +0100 Subject: [PATCH 08/29] 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 008aa904..3bbf7f72 100644 --- a/src/history/file_backed.rs +++ b/src/history/file_backed.rs @@ -90,7 +90,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 b1ee57529298fbc84a62f54c870d0fcc28fe463e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Fri, 8 Dec 2023 09:32:21 +0100 Subject: [PATCH 09/29] 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 1620a965..15d7e435 100644 --- a/src/history/sqlite_backed.rs +++ b/src/history/sqlite_backed.rs @@ -27,6 +27,7 @@ pub struct SqliteBackedHistory { fn deserialize_history_item(row: &rusqlite::Row) -> rusqlite::Result { let x: Option = row.get("more_info")?; + Ok(HistoryItem { id: HistoryItemId::new(row.get("id")?), start_timestamp: row.get::<&str, Option>("start_timestamp")?.map(|e| { @@ -70,7 +71,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, @@ -256,7 +257,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, @@ -266,15 +268,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, @@ -288,94 +291,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", @@ -390,7 +401,7 @@ impl SqliteBackedHistory { "SELECT {select_expression} \ FROM history \ WHERE ({wheres}) \ - ORDER BY id {asc} \ + ORDER BY idx {asc} \ {limit}" ); (query, params) From d966d5adb1aabf267b6f26d8c17da1d25006ac42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Fri, 8 Dec 2023 10:18:41 +0100 Subject: [PATCH 10/29] 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 774e1bfa..1d62513d 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; + fn count(&self, query: SearchQuery) -> Result; + /// return the total number of history items - fn count_all(&self) -> Result { + fn count_all(&self) -> Result { 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> { #[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 3bbf7f72..8a7f1840 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::{ @@ -87,10 +88,12 @@ impl History for FileBackedHistory { && !entry.is_empty() && self.capacity > 0 { - 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()); @@ -105,6 +108,8 @@ impl History for FileBackedHistory { } fn load(&self, id: HistoryItemId) -> Result { + println!("{:?}", self.entries); + Ok(FileBackedHistory::construct_entry( id, self.entries @@ -116,9 +121,9 @@ impl History for FileBackedHistory { )) } - fn count(&self, query: SearchQuery) -> Result { + fn count(&self, query: SearchQuery) -> Result { // 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> { @@ -165,11 +170,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 @@ -344,6 +345,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 15d7e435..18945128 100644 --- a/src/history/sqlite_backed.rs +++ b/src/history/sqlite_backed.rs @@ -113,24 +113,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 { + fn count(&self, query: SearchQuery) -> Result { 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(¶ms_borrow[..], |r| r.get(0)) .map_err(map_sqlite_err)?; + Ok(result) } fn search(&self, query: SearchQuery) -> Result> { 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 = self .db .prepare(&query) From 78200a102dddd95346697ef0d684e8a07984633c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Fri, 8 Dec 2023 10:35:46 +0100 Subject: [PATCH 11/29] 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 1d62513d..a84ae926 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::>>()?; + 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 73ceef4902284584b5e340129b52d6c649eb423e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Fri, 8 Dec 2023 11:58:11 +0100 Subject: [PATCH 12/29] 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 18945128..d6636d9a 100644 --- a/src/history/sqlite_backed.rs +++ b/src/history/sqlite_backed.rs @@ -233,63 +233,111 @@ impl SqliteBackedHistory { } /// initialize a new database / migrate an existing one fn from_connection( - db: Connection, + mut db: Connection, session: Option, session_timestamp: Option>, ) -> Result { - // 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 a02207969e4cfe323e206cd62e09853d78d28421 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Fri, 8 Dec 2023 12:16:19 +0100 Subject: [PATCH 13/29] 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 8a7f1840..4dae5c7a 100644 --- a/src/history/file_backed.rs +++ b/src/history/file_backed.rs @@ -53,22 +53,22 @@ impl Default for FileBackedHistory { } } +static ID_SEP: &str = ":"; + 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 { @@ -285,9 +285,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::>>()?; if from_file.len() + own_entries.len() > self.capacity { From 1ff51dcf2fe027fbaf0732aa2f960d8c53455e42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Fri, 8 Dec 2023 13:49:41 +0100 Subject: [PATCH 14/29] 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 4dae5c7a..869a90af 100644 --- a/src/history/file_backed.rs +++ b/src/history/file_backed.rs @@ -59,16 +59,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: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 564f606265549728673ee01d00c6edeb07614bc8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Fri, 8 Dec 2023 14:05:20 +0100 Subject: [PATCH 15/29] 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 d6636d9a..aceafd43 100644 --- a/src/history/sqlite_backed.rs +++ b/src/history/sqlite_backed.rs @@ -245,13 +245,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';", (), @@ -260,6 +265,7 @@ impl SqliteBackedHistory { let mut statements = vec![]; + // If so, rename it and delete related indexes if existing_history { statements.push( " @@ -273,6 +279,7 @@ impl SqliteBackedHistory { ); } + // Create the history table using the v1 schema statements.push( " create table history ( @@ -294,9 +301,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( " @@ -309,10 +317,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()?; @@ -329,9 +337,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}"), ))); @@ -345,6 +358,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, @@ -355,12 +370,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(); @@ -369,11 +386,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!( @@ -381,6 +400,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!( @@ -388,6 +408,7 @@ impl SqliteBackedHistory { )); params.push((":end_id", Box::new(end.0))); } + let limit = match limit { Some(l) => { params.push((":limit", Box::new(l))); @@ -395,14 +416,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))); } @@ -461,3 +489,17 @@ impl SqliteBackedHistory { (query, params) } } + +/// Escape special symbols for an SQL LIKE clause +/// (!) Requires LIKE clause to specify an `ESCAPE ''` 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 6e1ad73fba4a598986a6c16197c057ca913f7ec4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Fri, 8 Dec 2023 14:06:23 +0100 Subject: [PATCH 16/29] 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 aceafd43..ac2297cc 100644 --- a/src/history/sqlite_backed.rs +++ b/src/history/sqlite_backed.rs @@ -247,7 +247,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), @@ -319,6 +319,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()?; @@ -475,10 +476,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 \ @@ -486,6 +490,7 @@ impl SqliteBackedHistory { ORDER BY idx {asc} \ {limit}" ); + (query, params) } } From 8a32b0c4d4fb7d1d3b7e2dd9f7b083c45332e279 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Fri, 8 Dec 2023 15:24:13 +0100 Subject: [PATCH 17/29] 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 869a90af..b812ecb9 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::{ @@ -361,9 +360,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 2e6a16cd5788cf649523b5a6bd9af42995c531e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Fri, 8 Dec 2023 16:33:27 +0100 Subject: [PATCH 18/29] 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 b812ecb9..d215b35f 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, @@ -86,7 +86,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 ac2297cc..b07213c0 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; @@ -63,7 +63,7 @@ fn deserialize_history_item(row: &rusqlite::Row) -> rusqlite::Result HistoryItemId { - HistoryItemId(self.rng.next_u64() as i64) + HistoryItemId(self.rng.gen()) } fn save(&mut self, entry: &HistoryItem) -> Result<()> { From 4e6a974d9a5a1128449acb65313336eb5d63379c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Fri, 8 Dec 2023 19:24:06 +0100 Subject: [PATCH 19/29] 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 d215b35f..b7301f93 100644 --- a/src/history/file_backed.rs +++ b/src/history/file_backed.rs @@ -140,7 +140,19 @@ impl History for FileBackedHistory { } fn search(&self, query: SearchQuery) -> Result> { - 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", @@ -149,10 +161,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 { @@ -162,42 +174,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, @@ -208,7 +220,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 22f5e785352e2656f02eee705db3da8e8e722e52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Fri, 8 Dec 2023 19:29:09 +0100 Subject: [PATCH 20/29] 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 b07213c0..cc99eb98 100644 --- a/src/history/sqlite_backed.rs +++ b/src/history/sqlite_backed.rs @@ -271,10 +271,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; ", ); } @@ -299,7 +299,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); ", ); From 1211d3dd84a33bfee649a3d4c9eeabad6bb20047 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Tue, 30 Jan 2024 20:33:11 +0100 Subject: [PATCH 21/29] Complete rebase --- Cargo.lock | 35 +++++++++++++++++++++++++++++-- Cargo.toml | 5 +++++ src/history/base.rs | 6 +++--- src/history/file_backed.rs | 42 ++++++++++++++++---------------------- 4 files changed, 59 insertions(+), 29 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8e21b60c..e966960b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -353,6 +353,17 @@ dependencies = [ "windows-targets 0.48.5", ] +[[package]] +name = "getrandom" +version = "0.2.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "190092ea657667030ac6a35e305e62fc4dd69fd98ac98631e5d3a2b1575a12b5" +dependencies = [ + "cfg-if", + "libc", + "wasi", +] + [[package]] name = "glob" version = "0.3.1" @@ -409,9 +420,9 @@ dependencies = [ [[package]] name = "indexmap" -version = "2.1.0" +version = "2.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d530e1a18b1cb4c484e6e34556a0d948706958449fca0cab753d649f2bce3d1f" +checksum = "433de089bd45971eecf4668ee0ee8f4cec17db4f8bd8f7bc3197a6ce37aa7d9b" dependencies = [ "equivalent", "hashbrown", @@ -706,6 +717,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.4.1" @@ -725,9 +754,11 @@ dependencies = [ "crossterm", "fd-lock", "gethostname 0.4.3", + "indexmap", "itertools", "nu-ansi-term", "pretty_assertions", + "rand", "rstest", "rusqlite", "serde", diff --git a/Cargo.toml b/Cargo.toml index 1b392cad..0ceda60b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,6 +32,11 @@ strum_macros = "0.25" thiserror = "1.0.31" unicode-segmentation = "1.9.0" unicode-width = "0.1.9" +rand = { version = "0.8.5", default-features = false, features = [ + "small_rng", + "getrandom", +] } +indexmap = "2.2.1" [dev-dependencies] gethostname = "0.4.0" diff --git a/src/history/base.rs b/src/history/base.rs index a84ae926..b0bcf459 100644 --- a/src/history/base.rs +++ b/src/history/base.rs @@ -255,8 +255,8 @@ mod test { hist.save(&item)?; - // Ensure the item was correctly inserted - assert_eq!(hist.count_all()?, id); + // // Ensure the item was correctly inserted + // assert_eq!(hist.count_all()?, id); Ok(()) } @@ -452,7 +452,7 @@ mod test { #[test] fn history_size_zero() -> Result<()> { let mut history = crate::FileBackedHistory::new(0)?; - history.save(create_item(1, "/home/me", "cd ~/Downloads", 0))?; + create_item(&mut history, 1, "/home/me", "cd ~/Downloads", 0)?; assert_eq!(history.count_all()?, 0); let _ = history.sync(); history.clear()?; diff --git a/src/history/file_backed.rs b/src/history/file_backed.rs index b7301f93..07652578 100644 --- a/src/history/file_backed.rs +++ b/src/history/file_backed.rs @@ -11,6 +11,7 @@ use crate::{ use std::{ fs::OpenOptions, + hash::{DefaultHasher, Hash, Hasher}, io::{BufRead, BufReader, BufWriter, Seek, SeekFrom, Write}, ops::{Deref, DerefMut}, path::PathBuf, @@ -52,10 +53,8 @@ impl Default for FileBackedHistory { } } -static ID_SEP: &str = ":"; - -fn encode_entry(id: HistoryItemId, s: &str) -> String { - format!("{id}{ID_SEP}{}", s.replace('\n', NEWLINE_ESCAPE)) +fn encode_entry(s: &str) -> String { + s.replace('\n', NEWLINE_ESCAPE) } /// Decode an entry @@ -68,20 +67,13 @@ fn encode_entry(id: HistoryItemId, s: &str) -> String { /// 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(parsed_id) = s[..sep].parse() { - parsed = Some((parsed_id, &s[sep + ID_SEP.len()..])); - } - } + let mut hasher = DefaultHasher::new(); + counter.hash(&mut hasher); + s.hash(&mut hasher); - let (id, content) = parsed.unwrap_or_else(|| { - *counter += 1; - (*counter - 1, s) - }); + let id = hasher.finish() as i64; - (HistoryItemId(id), content.replace(NEWLINE_ESCAPE, "\n")) + (HistoryItemId(id), s.replace(NEWLINE_ESCAPE, "\n")) } impl History for FileBackedHistory { @@ -333,8 +325,8 @@ impl History for FileBackedHistory { if truncate { writer.rewind()?; - for (id, line) in &foreign_entries { - writer.write_all(encode_entry(*id, line).as_bytes())?; + for line in foreign_entries.values() { + writer.write_all(encode_entry(line).as_bytes())?; writer.write_all("\n".as_bytes())?; } } else { @@ -343,8 +335,8 @@ impl History for FileBackedHistory { } // 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())?; + for line in own_entries.values() { + writer.write_all(encode_entry(line).as_bytes())?; writer.write_all("\n".as_bytes())?; } @@ -399,7 +391,7 @@ impl FileBackedHistory { last_on_disk: None, session: None, rng: SmallRng::from_entropy(), - } + }) } /// Creates a new history with an associated history file. @@ -410,11 +402,13 @@ impl FileBackedHistory { /// /// **Side effects:** creates all nested directories to the file /// - pub fn with_file(capacity: usize, file: PathBuf) -> std::io::Result { - let mut hist = Self::new(capacity); + pub fn with_file(capacity: usize, file: PathBuf) -> Result { + let mut hist = Self::new(capacity)?; if let Some(base_dir) = file.parent() { - std::fs::create_dir_all(base_dir)?; + std::fs::create_dir_all(base_dir) + .map_err(ReedlineErrorVariants::IOError) + .map_err(ReedlineError)?; } hist.file = Some(file); From c27edb83897c35d22c7d2998c5cc850732aefced Mon Sep 17 00:00:00 2001 From: maxomatic458 <104733404+maxomatic458@users.noreply.github.com> Date: Sat, 3 Feb 2024 00:59:12 +0100 Subject: [PATCH 22/29] fix description line not truncating in columnar menu (#739) * add builder functions * fix description wrap * fmt * fix --- src/menu/columnar_menu.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/menu/columnar_menu.rs b/src/menu/columnar_menu.rs index dea2ab22..6eac27ab 100644 --- a/src/menu/columnar_menu.rs +++ b/src/menu/columnar_menu.rs @@ -6,6 +6,7 @@ use crate::{ Completer, Suggestion, }; use nu_ansi_term::ansi::RESET; +use unicode_width::UnicodeWidthStr; /// Default values used as reference for the menu. These values are set during /// the initial declaration of the menu and are always kept as reference for the @@ -308,12 +309,16 @@ impl ColumnarMenu { .unwrap_or(self.settings.color.text_style) .prefix(); + let left_text_size = self.longest_suggestion + self.default_details.col_padding; + let right_text_size = self.get_width().saturating_sub(left_text_size); + + let max_remaining = left_text_size.saturating_sub(match_str.width()); + let max_match = max_remaining.saturating_sub(remaining_str.width()); + if index == self.index() { if let Some(description) = &suggestion.description { - let left_text_size = self.longest_suggestion + self.default_details.col_padding; - let right_text_size = self.get_width().saturating_sub(left_text_size); format!( - "{}{}{}{}{}{}{:max$}{}{}{}{}{}{}", + "{}{}{}{}{}{:max_match$}{:max_remaining$}{}{}{}{}{}{}", suggestion_style_prefix, self.settings.color.selected_match_style.prefix(), match_str, @@ -331,7 +336,6 @@ impl ColumnarMenu { .replace('\n', " "), RESET, self.end_of_line(column), - max = left_text_size, ) } else { format!( @@ -350,10 +354,8 @@ impl ColumnarMenu { ) } } else if let Some(description) = &suggestion.description { - let left_text_size = self.longest_suggestion + self.default_details.col_padding; - let right_text_size = self.get_width().saturating_sub(left_text_size); format!( - "{}{}{}{}{}{:max$}{}{}{}{}{}", + "{}{}{}{}{:max_match$}{:max_remaining$}{}{}{}{}{}", suggestion_style_prefix, self.settings.color.match_style.prefix(), match_str, @@ -369,7 +371,6 @@ impl ColumnarMenu { .replace('\n', " "), RESET, self.end_of_line(column), - max = left_text_size, ) } else { format!( From 20453ac3259ccd91288712e120c4313a9f94dc21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20=C5=BD=C3=A1dn=C3=ADk?= Date: Tue, 6 Feb 2024 20:44:16 +0200 Subject: [PATCH 23/29] Bump to version 0.29 (#742) --- Cargo.lock | 2 +- Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e966960b..c49fce8d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -746,7 +746,7 @@ dependencies = [ [[package]] name = "reedline" -version = "0.28.0" +version = "0.29.0" dependencies = [ "arboard", "chrono", diff --git a/Cargo.toml b/Cargo.toml index 0ceda60b..49189f66 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -6,7 +6,7 @@ license = "MIT" name = "reedline" repository = "https://github.com/nushell/reedline" rust-version = "1.62.1" -version = "0.28.0" +version = "0.29.0" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [lib] From 858b3b5ce222b3fd7dbaa0c7196feb778a1ca7ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Wed, 7 Feb 2024 00:05:48 +0100 Subject: [PATCH 24/29] spacing --- src/history/base.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/history/base.rs b/src/history/base.rs index b0bcf459..38191905 100644 --- a/src/history/base.rs +++ b/src/history/base.rs @@ -198,8 +198,6 @@ pub trait History: Send { /// load a history item by its id fn load(&self, id: HistoryItemId) -> Result; - /// retrieves the next unused session id - /// count the results of a query fn count(&self, query: SearchQuery) -> Result; @@ -207,6 +205,7 @@ pub trait History: Send { fn count_all(&self) -> Result { self.count(SearchQuery::everything(SearchDirection::Forward, None)) } + /// return the results of a query fn search(&self, query: SearchQuery) -> Result>; @@ -216,12 +215,16 @@ pub trait History: Send { id: HistoryItemId, updater: &dyn Fn(HistoryItem) -> HistoryItem, ) -> Result<()>; + /// delete all history items fn clear(&mut self) -> Result<()>; + /// remove an item from this history fn delete(&mut self, h: HistoryItemId) -> Result<()>; + /// ensure that this history is written to disk fn sync(&mut self) -> std::io::Result<()>; + /// get the history session id fn session(&self) -> Option; } From 0f94e4b61520e490319f9f5f88bef4a465406cb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Fri, 9 Feb 2024 16:31:58 +0100 Subject: [PATCH 25/29] Fix missing reference --- src/completion/history.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/completion/history.rs b/src/completion/history.rs index fda3384f..cc9b1c2a 100644 --- a/src/completion/history.rs +++ b/src/completion/history.rs @@ -105,7 +105,7 @@ mod tests { let expected_history_size = command_line_history.len(); let mut history = FileBackedHistory::new(command_line_history.len())?; for command_line in command_line_history { - history.save(new_history_item(command_line))?; + history.save(&new_history_item(command_line))?; } let input = "git s"; let mut sut = HistoryCompleter::new(&history); @@ -144,7 +144,7 @@ mod tests { ) -> Result<()> { let mut history = FileBackedHistory::new(history_items.len())?; for history_item in history_items { - history.save(new_history_item(history_item))?; + history.save(&new_history_item(history_item))?; } let mut sut = HistoryCompleter::new(&history); let actual: Vec = sut From 40950a2a497e9c53fb5ec73c70a15f01a8864e79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Fri, 9 Feb 2024 16:35:06 +0100 Subject: [PATCH 26/29] Update tests --- src/completion/history.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/completion/history.rs b/src/completion/history.rs index cc9b1c2a..feb41dfd 100644 --- a/src/completion/history.rs +++ b/src/completion/history.rs @@ -71,14 +71,18 @@ impl<'menu> HistoryCompleter<'menu> { #[cfg(test)] mod tests { + use std::sync::atomic::{AtomicI64, Ordering}; + use rstest::rstest; use super::*; use crate::*; + static COUNTER: AtomicI64 = AtomicI64::new(0); + fn new_history_item(command_line: &str) -> HistoryItem { HistoryItem { - id: None, + id: HistoryItemId(COUNTER.fetch_add(1, Ordering::SeqCst)), start_timestamp: None, command_line: command_line.to_string(), session_id: None, From 0fef4073435eb7c91667dbf305d37e693accd8fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Fri, 9 Feb 2024 16:36:11 +0100 Subject: [PATCH 27/29] Explicit truncation behaviour --- src/history/file_backed.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/history/file_backed.rs b/src/history/file_backed.rs index 07652578..09c13bfa 100644 --- a/src/history/file_backed.rs +++ b/src/history/file_backed.rs @@ -292,6 +292,7 @@ impl History for FileBackedHistory { let mut f_lock = fd_lock::RwLock::new( OpenOptions::new() .create(true) + .truncate(false) .write(true) .read(true) .open(fname)?, From 5fff6fa4e18fd634004ab743e41522f124dae3b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Fri, 9 Feb 2024 18:59:26 +0100 Subject: [PATCH 28/29] Make a field public --- src/history/base.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/history/base.rs b/src/history/base.rs index 38191905..852ad927 100644 --- a/src/history/base.rs +++ b/src/history/base.rs @@ -42,7 +42,7 @@ pub struct SearchFilter { /// Query for the command line content pub command_line: Option, /// Considered implementation detail for now - pub(crate) not_command_line: Option, // to skip the currently shown value in up-arrow navigation + pub not_command_line: Option, // to skip the currently shown value in up-arrow navigation /// Filter based on the executing systems hostname pub hostname: Option, /// Exact filter for the working directory From 73936382eb38fcf07a9fc3c69b5a999b90243dd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Nerma?= Date: Thu, 21 Mar 2024 10:29:14 +0100 Subject: [PATCH 29/29] Update lockfile --- Cargo.lock | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 56bf6fec..3e9f545c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -733,6 +733,8 @@ dependencies = [ "crossbeam", "crossterm", "fd-lock", + "gethostname", + "indexmap", "itertools", "nu-ansi-term", "pretty_assertions",