From 2d686c83c4faa6525210f053a228083554d653f6 Mon Sep 17 00:00:00 2001 From: pickx Date: Sun, 15 Jun 2025 19:48:16 +0300 Subject: [PATCH 1/4] pass by value instead --- src/application.rs | 24 ++++++++++++------------ src/editor.rs | 6 +++--- src/main.rs | 2 +- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/application.rs b/src/application.rs index 7791b7d0c..4fe9b9fe9 100644 --- a/src/application.rs +++ b/src/application.rs @@ -34,7 +34,7 @@ where ModuleProvider: module::ModuleProvider + Send + 'static impl Application where ModuleProvider: module::ModuleProvider + Send + 'static { - pub(crate) fn new(args: &Args, event_provider: EventProvider, tui: Tui) -> Result + pub(crate) fn new(args: Args, event_provider: EventProvider, tui: Tui) -> Result where EventProvider: EventReaderFn, Tui: crate::display::Tui + Send + 'static, @@ -144,7 +144,7 @@ where ModuleProvider: module::ModuleProvider + Send + 'static Ok(()) } - fn filepath_from_args(args: &Args) -> Result { + fn filepath_from_args(args: Args) -> Result { args.todo_file_path().map(String::from).ok_or_else(|| { Exit::new( ExitStatus::StateError, @@ -254,7 +254,7 @@ mod tests { fn load_filepath_from_args_failure() { let event_provider = create_event_reader(|| Ok(None)); let application: Result>, Exit> = - Application::new(&args(&[]), event_provider, create_mocked_crossterm()); + Application::new(args(&[]), event_provider, create_mocked_crossterm()); let exit = application_error!(application); assert_eq!(exit.get_status(), &ExitStatus::StateError); assert!( @@ -269,7 +269,7 @@ mod tests { with_git_directory("fixtures/not-a-repository", |_| { let event_provider = create_event_reader(|| Ok(None)); let application: Result>, Exit> = - Application::new(&args(&["todofile"]), event_provider, create_mocked_crossterm()); + Application::new(args(&["todofile"]), event_provider, create_mocked_crossterm()); let exit = application_error!(application); assert_eq!(exit.get_status(), &ExitStatus::StateError); assert!(exit.get_message().unwrap().contains("Unable to load Git repository: ")); @@ -281,7 +281,7 @@ mod tests { with_git_directory("fixtures/invalid-config", |_| { let event_provider = create_event_reader(|| Ok(None)); let application: Result>, Exit> = - Application::new(&args(&["rebase-todo"]), event_provider, create_mocked_crossterm()); + Application::new(args(&["rebase-todo"]), event_provider, create_mocked_crossterm()); let exit = application_error!(application); assert_eq!(exit.get_status(), &ExitStatus::ConfigError); }); @@ -322,7 +322,7 @@ mod tests { with_git_directory("fixtures/simple", |_| { let event_provider = create_event_reader(|| Ok(None)); let application: Result>, Exit> = - Application::new(&args(&["does-not-exist"]), event_provider, create_mocked_crossterm()); + Application::new(args(&["does-not-exist"]), event_provider, create_mocked_crossterm()); let exit = application_error!(application); assert_eq!(exit.get_status(), &ExitStatus::FileReadError); }); @@ -334,7 +334,7 @@ mod tests { let rebase_todo = format!("{git_dir}/rebase-todo-noop"); let event_provider = create_event_reader(|| Ok(None)); let application: Result>, Exit> = Application::new( - &args(&[rebase_todo.as_str()]), + args(&[rebase_todo.as_str()]), event_provider, create_mocked_crossterm(), ); @@ -349,7 +349,7 @@ mod tests { let rebase_todo = format!("{git_dir}/rebase-todo-empty"); let event_provider = create_event_reader(|| Ok(None)); let application: Result>, Exit> = Application::new( - &args(&[rebase_todo.as_str()]), + args(&[rebase_todo.as_str()]), event_provider, create_mocked_crossterm(), ); @@ -382,7 +382,7 @@ mod tests { let rebase_todo = format!("{git_dir}/rebase-todo"); let event_provider = create_event_reader(|| Ok(Some(Event::Key(KeyEvent::from(KeyCode::Char('W')))))); let mut application: Application = Application::new( - &args(&[rebase_todo.as_str()]), + args(&[rebase_todo.as_str()]), event_provider, create_mocked_crossterm(), ) @@ -408,7 +408,7 @@ mod tests { let rebase_todo = format!("{git_dir}/rebase-todo"); let event_provider = create_event_reader(|| Ok(Some(Event::Key(KeyEvent::from(KeyCode::Char('W')))))); let mut application: Application = Application::new( - &args(&[rebase_todo.as_str()]), + args(&[rebase_todo.as_str()]), event_provider, create_mocked_crossterm(), ) @@ -433,7 +433,7 @@ mod tests { )))) }); let mut application: Application = Application::new( - &args(&[rebase_todo.as_str()]), + args(&[rebase_todo.as_str()]), event_provider, create_mocked_crossterm(), ) @@ -449,7 +449,7 @@ mod tests { let rebase_todo = format!("{git_dir}/rebase-todo"); let event_provider = create_event_reader(|| Ok(Some(Event::Key(KeyEvent::from(KeyCode::Char('W')))))); let mut application: Application = Application::new( - &args(&[rebase_todo.as_str()]), + args(&[rebase_todo.as_str()]), event_provider, create_mocked_crossterm(), ) diff --git a/src/editor.rs b/src/editor.rs index de87efcfa..e287f9a81 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -11,7 +11,7 @@ use crate::{ }; #[cfg(not(tarpaulin_include))] -pub(crate) fn run(args: &Args) -> Exit { +pub(crate) fn run(args: Args) -> Exit { let mut application: Application = match Application::new(args, read_event, CrossTerm::new()) { Ok(app) => app, Err(exit) => return exit, @@ -39,7 +39,7 @@ mod tests { with_git_directory("fixtures/simple", |path| { let todo_file = Path::new(path).join("rebase-todo-empty"); assert_eq!( - run(&args(&[todo_file.to_str().unwrap()])).get_status(), + run(args(&[todo_file.to_str().unwrap()])).get_status(), &ExitStatus::Good ); }); @@ -50,7 +50,7 @@ mod tests { with_git_directory("fixtures/simple", |path| { let todo_file = Path::new(path).join("does-not-exist"); assert_eq!( - run(&args(&[todo_file.to_str().unwrap()])).get_status(), + run(args(&[todo_file.to_str().unwrap()])).get_status(), &ExitStatus::FileReadError ); }); diff --git a/src/main.rs b/src/main.rs index 1a0d861b2..eee02c87f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -80,7 +80,7 @@ fn run(os_args: Vec) -> Exit { Mode::Help => help::run(), Mode::Version => version::run(), Mode::License => license::run(), - Mode::Editor => editor::run(&args), + Mode::Editor => editor::run(args), } }, } From 3c4fa04fe570cab89b7abc507c87a9b3c995f8cd Mon Sep 17 00:00:00 2001 From: pickx Date: Sun, 15 Jun 2025 20:06:30 +0300 Subject: [PATCH 2/4] nicer/more DRY parsing of `Args` --- src/application.rs | 26 ++++++++++---------------- src/arguments.rs | 30 ++++++++++++++---------------- src/editor.rs | 16 +++++++--------- src/main.rs | 2 +- src/tests.rs | 18 +++++++++--------- 5 files changed, 41 insertions(+), 51 deletions(-) diff --git a/src/application.rs b/src/application.rs index 4fe9b9fe9..28a149d4f 100644 --- a/src/application.rs +++ b/src/application.rs @@ -208,8 +208,6 @@ where ModuleProvider: module::ModuleProvider + Send + 'static #[cfg(all(unix, test))] mod tests { - use std::ffi::OsString; - use claims::assert_ok; use super::*; @@ -228,10 +226,6 @@ mod tests { }, }; - fn args(args: &[&str]) -> Args { - Args::try_from(args.iter().map(OsString::from).collect::>()).unwrap() - } - fn create_mocked_crossterm() -> mocks::CrossTerm { let mut crossterm = mocks::CrossTerm::new(); crossterm.set_size(Size::new(300, 120)); @@ -254,7 +248,7 @@ mod tests { fn load_filepath_from_args_failure() { let event_provider = create_event_reader(|| Ok(None)); let application: Result>, Exit> = - Application::new(args(&[]), event_provider, create_mocked_crossterm()); + Application::new(Args::from_os_strings(Vec::new()).unwrap(), event_provider, create_mocked_crossterm()); let exit = application_error!(application); assert_eq!(exit.get_status(), &ExitStatus::StateError); assert!( @@ -269,7 +263,7 @@ mod tests { with_git_directory("fixtures/not-a-repository", |_| { let event_provider = create_event_reader(|| Ok(None)); let application: Result>, Exit> = - Application::new(args(&["todofile"]), event_provider, create_mocked_crossterm()); + Application::new(Args::from_strs(["todofile"]).unwrap(), event_provider, create_mocked_crossterm()); let exit = application_error!(application); assert_eq!(exit.get_status(), &ExitStatus::StateError); assert!(exit.get_message().unwrap().contains("Unable to load Git repository: ")); @@ -281,7 +275,7 @@ mod tests { with_git_directory("fixtures/invalid-config", |_| { let event_provider = create_event_reader(|| Ok(None)); let application: Result>, Exit> = - Application::new(args(&["rebase-todo"]), event_provider, create_mocked_crossterm()); + Application::new(Args::from_strs(["rebase-todo"]).unwrap(), event_provider, create_mocked_crossterm()); let exit = application_error!(application); assert_eq!(exit.get_status(), &ExitStatus::ConfigError); }); @@ -322,7 +316,7 @@ mod tests { with_git_directory("fixtures/simple", |_| { let event_provider = create_event_reader(|| Ok(None)); let application: Result>, Exit> = - Application::new(args(&["does-not-exist"]), event_provider, create_mocked_crossterm()); + Application::new(Args::from_strs(["does-not-exist"]).unwrap(), event_provider, create_mocked_crossterm()); let exit = application_error!(application); assert_eq!(exit.get_status(), &ExitStatus::FileReadError); }); @@ -334,7 +328,7 @@ mod tests { let rebase_todo = format!("{git_dir}/rebase-todo-noop"); let event_provider = create_event_reader(|| Ok(None)); let application: Result>, Exit> = Application::new( - args(&[rebase_todo.as_str()]), + Args::from_strs([rebase_todo]).unwrap(), event_provider, create_mocked_crossterm(), ); @@ -349,7 +343,7 @@ mod tests { let rebase_todo = format!("{git_dir}/rebase-todo-empty"); let event_provider = create_event_reader(|| Ok(None)); let application: Result>, Exit> = Application::new( - args(&[rebase_todo.as_str()]), + Args::from_strs([rebase_todo]).unwrap(), event_provider, create_mocked_crossterm(), ); @@ -382,7 +376,7 @@ mod tests { let rebase_todo = format!("{git_dir}/rebase-todo"); let event_provider = create_event_reader(|| Ok(Some(Event::Key(KeyEvent::from(KeyCode::Char('W')))))); let mut application: Application = Application::new( - args(&[rebase_todo.as_str()]), + Args::from_strs([rebase_todo]).unwrap(), event_provider, create_mocked_crossterm(), ) @@ -408,7 +402,7 @@ mod tests { let rebase_todo = format!("{git_dir}/rebase-todo"); let event_provider = create_event_reader(|| Ok(Some(Event::Key(KeyEvent::from(KeyCode::Char('W')))))); let mut application: Application = Application::new( - args(&[rebase_todo.as_str()]), + Args::from_strs([rebase_todo]).unwrap(), event_provider, create_mocked_crossterm(), ) @@ -433,7 +427,7 @@ mod tests { )))) }); let mut application: Application = Application::new( - args(&[rebase_todo.as_str()]), + Args::from_strs([rebase_todo]).unwrap(), event_provider, create_mocked_crossterm(), ) @@ -449,7 +443,7 @@ mod tests { let rebase_todo = format!("{git_dir}/rebase-todo"); let event_provider = create_event_reader(|| Ok(Some(Event::Key(KeyEvent::from(KeyCode::Char('W')))))); let mut application: Application = Application::new( - args(&[rebase_todo.as_str()]), + Args::from_strs([rebase_todo]).unwrap(), event_provider, create_mocked_crossterm(), ) diff --git a/src/arguments.rs b/src/arguments.rs index cf847496d..be73f360f 100644 --- a/src/arguments.rs +++ b/src/arguments.rs @@ -26,12 +26,14 @@ impl Args { pub(crate) fn todo_file_path(&self) -> Option<&str> { self.todo_file_path.as_deref() } -} -impl TryFrom> for Args { - type Error = Exit; + #[cfg(test)] + pub(crate) fn from_strs(args: impl IntoIterator>) -> Result { + let args = args.into_iter().map(|it| OsString::from(it.as_ref())).collect(); + Self::from_os_strings(args) + } - fn try_from(args: Vec) -> Result { + pub(crate) fn from_os_strings(args: Vec) -> Result { let mut pargs = Arguments::from_vec(args); let mode = if pargs.contains(["-h", "--help"]) { @@ -59,21 +61,17 @@ impl TryFrom> for Args { mod tests { use super::*; - fn create_args(args: &[&str]) -> Vec { - args.iter().map(OsString::from).collect() - } - #[test] fn mode_help() { - assert_eq!(Args::try_from(create_args(&["-h"])).unwrap().mode(), &Mode::Help); - assert_eq!(Args::try_from(create_args(&["--help"])).unwrap().mode(), &Mode::Help); + assert_eq!(Args::from_strs(["-h"]).unwrap().mode(), &Mode::Help); + assert_eq!(Args::from_strs(["--help"]).unwrap().mode(), &Mode::Help); } #[test] fn mode_version() { - assert_eq!(Args::try_from(create_args(&["-v"])).unwrap().mode(), &Mode::Version); + assert_eq!(Args::from_strs(["-v"]).unwrap().mode(), &Mode::Version); assert_eq!( - Args::try_from(create_args(&["--version"])).unwrap().mode(), + Args::from_strs(["--version"]).unwrap().mode(), &Mode::Version ); } @@ -81,21 +79,21 @@ mod tests { #[test] fn mode_license() { assert_eq!( - Args::try_from(create_args(&["--license"])).unwrap().mode(), + Args::from_strs(["--license"]).unwrap().mode(), &Mode::License ); } #[test] fn todo_file_ok() { - let args = Args::try_from(create_args(&["todofile"])).unwrap(); + let args = Args::from_strs(["todofile"]).unwrap(); assert_eq!(args.mode(), &Mode::Editor); assert_eq!(args.todo_file_path(), Some("todofile")); } #[test] fn todo_file_missing() { - let args = Args::try_from(create_args(&[])).unwrap(); + let args = Args::from_os_strings(Vec::new()).unwrap(); assert_eq!(args.mode(), &Mode::Editor); assert!(args.todo_file_path().is_none()); } @@ -105,6 +103,6 @@ mod tests { #[expect(unsafe_code)] fn todo_file_invalid() { let args = unsafe { vec![OsString::from(String::from_utf8_unchecked(vec![0xC3, 0x28]))] }; - _ = Args::try_from(args).unwrap_err(); + _ = Args::from_os_strings(args).unwrap_err(); } } diff --git a/src/editor.rs b/src/editor.rs index e287f9a81..9557e3e69 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -25,21 +25,18 @@ pub(crate) fn run(args: Args) -> Exit { #[cfg(test)] mod tests { - use std::{ffi::OsString, path::Path}; + use std::path::Path; use super::*; use crate::test_helpers::with_git_directory; - fn args(args: &[&str]) -> Args { - Args::try_from(args.iter().map(OsString::from).collect::>()).unwrap() - } - #[test] fn successful_run() { with_git_directory("fixtures/simple", |path| { - let todo_file = Path::new(path).join("rebase-todo-empty"); + let todo_file = Path::new(path).join("rebase-todo-empty").into_os_string(); + let args = Args::from_os_strings(vec![todo_file]).unwrap(); assert_eq!( - run(args(&[todo_file.to_str().unwrap()])).get_status(), + run(args).get_status(), &ExitStatus::Good ); }); @@ -48,9 +45,10 @@ mod tests { #[test] fn error_on_application_create() { with_git_directory("fixtures/simple", |path| { - let todo_file = Path::new(path).join("does-not-exist"); + let todo_file = Path::new(path).join("does-not-exist").into_os_string(); + let args = Args::from_os_strings(vec![todo_file]).unwrap(); assert_eq!( - run(args(&[todo_file.to_str().unwrap()])).get_status(), + run(args).get_status(), &ExitStatus::FileReadError ); }); diff --git a/src/main.rs b/src/main.rs index eee02c87f..0cc888712 100644 --- a/src/main.rs +++ b/src/main.rs @@ -73,7 +73,7 @@ use crate::{ #[must_use] fn run(os_args: Vec) -> Exit { - match Args::try_from(os_args) { + match Args::from_os_strings(os_args) { Err(err) => err, Ok(args) => { match *args.mode() { diff --git a/src/tests.rs b/src/tests.rs index 9e51aad72..db488ed19 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -3,14 +3,11 @@ use std::path::Path; use super::*; use crate::{module::ExitStatus, test_helpers::with_git_directory}; -fn args(args: &[&str]) -> Vec { - args.iter().map(OsString::from).collect::>() -} - #[test] #[serial_test::serial] fn successful_run_help() { - let exit = run(args(&["--help"])); + let args = ["--help"].into_iter().map(OsString::from).collect(); + let exit = run(args); assert!(exit.get_message().unwrap().contains("USAGE:")); assert_eq!(exit.get_status(), &ExitStatus::Good); } @@ -18,7 +15,8 @@ fn successful_run_help() { #[test] #[serial_test::serial] fn successful_run_version() { - let exit = run(args(&["--version"])); + let args = ["--version"].into_iter().map(OsString::from).collect(); + let exit = run(args); assert!(exit.get_message().unwrap().starts_with("interactive-rebase-tool")); assert_eq!(exit.get_status(), &ExitStatus::Good); } @@ -26,7 +24,8 @@ fn successful_run_version() { #[test] #[serial_test::serial] fn successful_run_license() { - let exit = run(args(&["--license"])); + let args = ["--license"].into_iter().map(OsString::from).collect(); + let exit = run(args); assert!( exit.get_message() .unwrap() @@ -38,9 +37,10 @@ fn successful_run_license() { #[test] fn successful_run_editor() { with_git_directory("fixtures/simple", |path| { - let todo_file = Path::new(path).join("rebase-todo-empty"); + let todo_file = Path::new(path).join("rebase-todo-empty").into_os_string(); + let args = vec![todo_file]; assert_eq!( - run(args(&[todo_file.to_str().unwrap()])).get_status(), + run(args).get_status(), &ExitStatus::Good ); }); From 390713762990d06c338918b3155e845062a127ac Mon Sep 17 00:00:00 2001 From: pickx Date: Sun, 15 Jun 2025 10:25:25 +0300 Subject: [PATCH 3/4] decouple construction of `Config` from `ConfigLoader` --- src/application.rs | 12 ++++++++---- src/config.rs | 25 ++++--------------------- 2 files changed, 12 insertions(+), 25 deletions(-) diff --git a/src/application.rs b/src/application.rs index 28a149d4f..5154ccbe2 100644 --- a/src/application.rs +++ b/src/application.rs @@ -9,7 +9,7 @@ pub(crate) use crate::application::app_data::AppData; use crate::{ Args, Exit, - config::{Config, ConfigLoader, DiffIgnoreWhitespaceSetting}, + config::{Config, ConfigError, ConfigErrorCause, ConfigLoader, DiffIgnoreWhitespaceSetting}, diff::{self, CommitDiffLoader, CommitDiffLoaderOptions}, display::Display, git::open_repository_from_env, @@ -42,7 +42,8 @@ where ModuleProvider: module::ModuleProvider + Send + 'static let filepath = Self::filepath_from_args(args)?; let repository = Self::open_repository()?; let config_loader = ConfigLoader::from(repository); - let config = Self::load_config(&config_loader)?; + let config = Self::load_config(&config_loader) + .map_err(|err| Exit::new(ExitStatus::ConfigError, format!("{err:#}").as_str()))?; let todo_file = Arc::new(Mutex::new(Self::load_todo_file(filepath.as_str(), &config)?)); let display = Display::new(tui, &config.theme); @@ -162,8 +163,11 @@ where ModuleProvider: module::ModuleProvider + Send + 'static }) } - fn load_config(config_loader: &ConfigLoader) -> Result { - Config::try_from(config_loader).map_err(|err| Exit::new(ExitStatus::ConfigError, format!("{err:#}").as_str())) + fn load_config(config_loader: &ConfigLoader) -> Result { + let config = config_loader + .load_config() + .map_err(|e| ConfigError::new_read_error("", ConfigErrorCause::GitError(e)))?; + Config::new_with_config(Some(&config)) } fn todo_file_options(config: &Config) -> TodoFileOptions { diff --git a/src/config.rs b/src/config.rs index 327485f90..d92b80c18 100644 --- a/src/config.rs +++ b/src/config.rs @@ -23,14 +23,12 @@ pub(crate) use self::{ config_loader::ConfigLoader, diff_ignore_whitespace_setting::DiffIgnoreWhitespaceSetting, diff_show_whitespace_setting::DiffShowWhitespaceSetting, + errors::{ConfigError, ConfigErrorCause, InvalidColorError}, git_config::GitConfig, key_bindings::KeyBindings, theme::Theme, }; -use crate::config::{ - errors::{ConfigError, ConfigErrorCause, InvalidColorError}, - utils::get_optional_string, -}; +use crate::config::utils::get_optional_string; const DEFAULT_SPACE_SYMBOL: &str = "\u{b7}"; // · const DEFAULT_TAB_SYMBOL: &str = "\u{2192}"; // → @@ -94,22 +92,6 @@ impl Config { } } -impl TryFrom<&ConfigLoader> for Config { - type Error = ConfigError; - - /// Creates a new Config instance loading the Git Config. - /// - /// # Errors - /// - /// Will return an `Err` if there is a problem loading the configuration. - fn try_from(config_loader: &ConfigLoader) -> Result { - let config = config_loader - .load_config() - .map_err(|e| ConfigError::new_read_error("", ConfigErrorCause::GitError(e)))?; - Self::new_with_config(Some(&config)) - } -} - impl TryFrom<&crate::git::Config> for Config { type Error = ConfigError; @@ -132,7 +114,8 @@ mod tests { fn try_from_config_loader() { with_temp_bare_repository(|repository| { let loader = ConfigLoader::from(repository); - assert_ok!(Config::try_from(&loader)); + let config = assert_ok!(loader.load_config()); + assert_ok!(Config::try_from(&config)); }); } From fb7e660b09e6c010187478e667a7dd88796270af Mon Sep 17 00:00:00 2001 From: pickx Date: Sun, 15 Jun 2025 19:34:41 +0300 Subject: [PATCH 4/4] parse `GIT_CONFIG_PARAMETERS`, add to config --- src/application.rs | 52 ++++++++++++++++++++++++------------- src/config.rs | 2 +- src/config/config_loader.rs | 36 ++++++++++++++++++------- src/editor.rs | 10 ++++--- src/main.rs | 47 ++++++++++++++++++++++++++++++--- src/tests.rs | 15 +++++++---- 6 files changed, 121 insertions(+), 41 deletions(-) diff --git a/src/application.rs b/src/application.rs index 5154ccbe2..55841ff58 100644 --- a/src/application.rs +++ b/src/application.rs @@ -34,14 +34,14 @@ where ModuleProvider: module::ModuleProvider + Send + 'static impl Application where ModuleProvider: module::ModuleProvider + Send + 'static { - pub(crate) fn new(args: Args, event_provider: EventProvider, tui: Tui) -> Result + pub(crate) fn new(args: Args, git_config_parameters: Vec<(String, String)>, event_provider: EventProvider, tui: Tui) -> Result where EventProvider: EventReaderFn, Tui: crate::display::Tui + Send + 'static, { let filepath = Self::filepath_from_args(args)?; let repository = Self::open_repository()?; - let config_loader = ConfigLoader::from(repository); + let config_loader = ConfigLoader::with_overrides(repository, git_config_parameters); let config = Self::load_config(&config_loader) .map_err(|err| Exit::new(ExitStatus::ConfigError, format!("{err:#}").as_str()))?; let todo_file = Arc::new(Mutex::new(Self::load_todo_file(filepath.as_str(), &config)?)); @@ -221,12 +221,7 @@ mod tests { module::Modules, runtime::{Installer, RuntimeError}, test_helpers::{ - DefaultTestModule, - TestModuleProvider, - create_config, - create_event_reader, - mocks, - with_git_directory, + DefaultTestModule, TestModuleProvider, create_config, create_event_reader, mocks, with_git_directory, }, }; @@ -240,8 +235,7 @@ mod tests { ($app:expr) => { if let Err(e) = $app { e - } - else { + } else { panic!("Application is not in an error state"); } }; @@ -251,8 +245,12 @@ mod tests { #[serial_test::serial] fn load_filepath_from_args_failure() { let event_provider = create_event_reader(|| Ok(None)); - let application: Result>, Exit> = - Application::new(Args::from_os_strings(Vec::new()).unwrap(), event_provider, create_mocked_crossterm()); + let application: Result>, Exit> = Application::new( + Args::from_os_strings(Vec::new()).unwrap(), + Vec::new(), + event_provider, + create_mocked_crossterm(), + ); let exit = application_error!(application); assert_eq!(exit.get_status(), &ExitStatus::StateError); assert!( @@ -266,8 +264,12 @@ mod tests { fn load_repository_failure() { with_git_directory("fixtures/not-a-repository", |_| { let event_provider = create_event_reader(|| Ok(None)); - let application: Result>, Exit> = - Application::new(Args::from_strs(["todofile"]).unwrap(), event_provider, create_mocked_crossterm()); + let application: Result>, Exit> = Application::new( + Args::from_strs(["todofile"]).unwrap(), + Vec::new(), + event_provider, + create_mocked_crossterm(), + ); let exit = application_error!(application); assert_eq!(exit.get_status(), &ExitStatus::StateError); assert!(exit.get_message().unwrap().contains("Unable to load Git repository: ")); @@ -278,8 +280,12 @@ mod tests { fn load_config_failure() { with_git_directory("fixtures/invalid-config", |_| { let event_provider = create_event_reader(|| Ok(None)); - let application: Result>, Exit> = - Application::new(Args::from_strs(["rebase-todo"]).unwrap(), event_provider, create_mocked_crossterm()); + let application: Result>, Exit> = Application::new( + Args::from_strs(["rebase-todo"]).unwrap(), + Vec::new(), + event_provider, + create_mocked_crossterm(), + ); let exit = application_error!(application); assert_eq!(exit.get_status(), &ExitStatus::ConfigError); }); @@ -319,8 +325,12 @@ mod tests { fn load_todo_file_load_error() { with_git_directory("fixtures/simple", |_| { let event_provider = create_event_reader(|| Ok(None)); - let application: Result>, Exit> = - Application::new(Args::from_strs(["does-not-exist"]).unwrap(), event_provider, create_mocked_crossterm()); + let application: Result>, Exit> = Application::new( + Args::from_strs(["does-not-exist"]).unwrap(), + Vec::new(), + event_provider, + create_mocked_crossterm(), + ); let exit = application_error!(application); assert_eq!(exit.get_status(), &ExitStatus::FileReadError); }); @@ -333,6 +343,7 @@ mod tests { let event_provider = create_event_reader(|| Ok(None)); let application: Result>, Exit> = Application::new( Args::from_strs([rebase_todo]).unwrap(), + Vec::new(), event_provider, create_mocked_crossterm(), ); @@ -348,6 +359,7 @@ mod tests { let event_provider = create_event_reader(|| Ok(None)); let application: Result>, Exit> = Application::new( Args::from_strs([rebase_todo]).unwrap(), + Vec::new(), event_provider, create_mocked_crossterm(), ); @@ -381,6 +393,7 @@ mod tests { let event_provider = create_event_reader(|| Ok(Some(Event::Key(KeyEvent::from(KeyCode::Char('W')))))); let mut application: Application = Application::new( Args::from_strs([rebase_todo]).unwrap(), + Vec::new(), event_provider, create_mocked_crossterm(), ) @@ -407,6 +420,7 @@ mod tests { let event_provider = create_event_reader(|| Ok(Some(Event::Key(KeyEvent::from(KeyCode::Char('W')))))); let mut application: Application = Application::new( Args::from_strs([rebase_todo]).unwrap(), + Vec::new(), event_provider, create_mocked_crossterm(), ) @@ -432,6 +446,7 @@ mod tests { }); let mut application: Application = Application::new( Args::from_strs([rebase_todo]).unwrap(), + Vec::new(), event_provider, create_mocked_crossterm(), ) @@ -448,6 +463,7 @@ mod tests { let event_provider = create_event_reader(|| Ok(Some(Event::Key(KeyEvent::from(KeyCode::Char('W')))))); let mut application: Application = Application::new( Args::from_strs([rebase_todo]).unwrap(), + Vec::new(), event_provider, create_mocked_crossterm(), ) diff --git a/src/config.rs b/src/config.rs index d92b80c18..96e6dfa4e 100644 --- a/src/config.rs +++ b/src/config.rs @@ -113,7 +113,7 @@ mod tests { #[test] fn try_from_config_loader() { with_temp_bare_repository(|repository| { - let loader = ConfigLoader::from(repository); + let loader = ConfigLoader::new(repository); let config = assert_ok!(loader.load_config()); assert_ok!(Config::try_from(&config)); }); diff --git a/src/config/config_loader.rs b/src/config/config_loader.rs index e9c9bbc71..3dbbadb78 100644 --- a/src/config/config_loader.rs +++ b/src/config/config_loader.rs @@ -6,15 +6,37 @@ use crate::git::{Config, GitError}; pub(crate) struct ConfigLoader { repository: Repository, + overrides: Vec<(String, String)>, } impl ConfigLoader { - /// Load the git configuration for the repository. + #[cfg(test)] + pub(crate) fn new(repository: Repository) -> Self { + let overrides = Vec::new(); + Self { repository, overrides } + } + + pub(crate) fn with_overrides(repository: Repository, overrides: Vec<(String, String)>) -> Self { + Self { repository, overrides } + } + + /// Load the git configuration for the repository, + /// with any overrides taking priority over the values defined in the repositroy /// /// # Errors /// Will result in an error if the configuration is invalid. pub(crate) fn load_config(&self) -> Result { - self.repository.config().map_err(|e| GitError::ConfigLoad { cause: e }) + let into_git_error = |cause| GitError::ConfigLoad { cause }; + + let mut config = self.repository.config().map_err(into_git_error)?; + for (name, value) in &self.overrides { + if value.is_empty() { + config.set_bool(name, true).map_err(into_git_error)?; + } else { + config.set_str(name, value).map_err(into_git_error)?; + } + } + Ok(config) } pub(crate) fn eject_repository(self) -> Repository { @@ -22,12 +44,6 @@ impl ConfigLoader { } } -impl From for ConfigLoader { - fn from(repository: Repository) -> Self { - Self { repository } - } -} - impl Debug for ConfigLoader { fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), std::fmt::Error> { f.debug_struct("ConfigLoader") @@ -49,7 +65,7 @@ mod unix_tests { #[test] fn load_config() { with_temp_bare_repository(|repository| { - let config = ConfigLoader::from(repository); + let config = ConfigLoader::new(repository); assert_ok!(config.load_config()); }); } @@ -58,7 +74,7 @@ mod unix_tests { fn fmt() { with_temp_repository(|repository| { let path = repository.path().canonicalize().unwrap(); - let loader = ConfigLoader::from(repository); + let loader = ConfigLoader::new(repository); let formatted = format!("{loader:?}"); assert_eq!( formatted, diff --git a/src/editor.rs b/src/editor.rs index 9557e3e69..704d400dd 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -11,8 +11,8 @@ use crate::{ }; #[cfg(not(tarpaulin_include))] -pub(crate) fn run(args: Args) -> Exit { - let mut application: Application = match Application::new(args, read_event, CrossTerm::new()) { +pub(crate) fn run(args: Args, git_config_parameters: Vec<(String, String)>) -> Exit { + let mut application: Application = match Application::new(args, git_config_parameters, read_event, CrossTerm::new()) { Ok(app) => app, Err(exit) => return exit, }; @@ -35,8 +35,9 @@ mod tests { with_git_directory("fixtures/simple", |path| { let todo_file = Path::new(path).join("rebase-todo-empty").into_os_string(); let args = Args::from_os_strings(vec![todo_file]).unwrap(); + let git_config_parameters = Vec::new(); assert_eq!( - run(args).get_status(), + run(args, git_config_parameters).get_status(), &ExitStatus::Good ); }); @@ -47,8 +48,9 @@ mod tests { with_git_directory("fixtures/simple", |path| { let todo_file = Path::new(path).join("does-not-exist").into_os_string(); let args = Args::from_os_strings(vec![todo_file]).unwrap(); + let git_config_parameters = Vec::new(); assert_eq!( - run(args).get_status(), + run(args, git_config_parameters).get_status(), &ExitStatus::FileReadError ); }); diff --git a/src/main.rs b/src/main.rs index 0cc888712..68dd03ee9 100644 --- a/src/main.rs +++ b/src/main.rs @@ -72,7 +72,7 @@ use crate::{ }; #[must_use] -fn run(os_args: Vec) -> Exit { +fn run(os_args: Vec, git_config_parameters: Vec<(String, String)>) -> Exit { match Args::from_os_strings(os_args) { Err(err) => err, Ok(args) => { @@ -80,16 +80,57 @@ fn run(os_args: Vec) -> Exit { Mode::Help => help::run(), Mode::Version => version::run(), Mode::License => license::run(), - Mode::Editor => editor::run(args), + Mode::Editor => editor::run(args, git_config_parameters), } }, } } +/// returns a pair of name/value pairs passed as overrides to `git`. +/// +/// input here is expected to come from `git -c`, and is in the form +/// of `-c =` pairs, for example `-c interactive-rebase-tool.diffTabWidth=4`. +/// separated by a single whitespace +/// +/// notes: +/// 1. the key/value have both gone through git's shell quoting. +/// from `gix-quote`: "every single-quote `'` is escaped as `'\''`, +/// every exclamation mark `!` is escaped as `'\!'`, and the entire string +/// is enclosed in single quotes." +/// 2. if input doesn't contain a '=', a `=` is appended between +/// `key` and `value`. `value`, will be an empty string. +/// 3. if input does contain a '=' but `value` is empty, `value` +/// will also be an empty string. +/// 4. an empty string will later be interpreted as `true`, +/// but we don't do this here. +pub(crate) fn parse_git_config_parameters(env_var: OsString) -> Vec<(String, String)> { + // we expect valid UTF-8 from the shell/git, so we don't handle errors here. + let Some(env_var) = env_var.to_str() else { + return Vec::new(); + }; + + // naive implementation: assumes correctly-escaped strings, efficiency isn't a priority + fn unescape(s: &str) -> String { + let s = s.trim_matches('\''); + let mut s = s.replace("\\'", "\'"); + s = s.replace("\\!", "!"); + s + } + + env_var + .split_ascii_whitespace() + .filter_map(|pair| pair.split_once('=')) + .map(|(name, value)| (unescape(name), unescape(value))) + .collect() +} #[expect(clippy::print_stderr, reason = "Required to print error message.")] #[cfg(not(tarpaulin_include))] fn main() -> impl Termination { - let exit = run(env::args_os().skip(1).collect()); + let args = env::args_os().skip(1).collect(); + let git_config_parameters = env::var_os("GIT_CONFIG_PARAMETERS") + .map(parse_git_config_parameters) + .unwrap_or_default(); + let exit = run(args, git_config_parameters); if let Some(message) = exit.get_message() { eprintln!("{message}"); } diff --git a/src/tests.rs b/src/tests.rs index db488ed19..db3181cc5 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -7,7 +7,8 @@ use crate::{module::ExitStatus, test_helpers::with_git_directory}; #[serial_test::serial] fn successful_run_help() { let args = ["--help"].into_iter().map(OsString::from).collect(); - let exit = run(args); + let git_config_parameters = Vec::new(); + let exit = run(args, git_config_parameters); assert!(exit.get_message().unwrap().contains("USAGE:")); assert_eq!(exit.get_status(), &ExitStatus::Good); } @@ -16,7 +17,8 @@ fn successful_run_help() { #[serial_test::serial] fn successful_run_version() { let args = ["--version"].into_iter().map(OsString::from).collect(); - let exit = run(args); + let git_config_parameters = Vec::new(); + let exit = run(args, git_config_parameters); assert!(exit.get_message().unwrap().starts_with("interactive-rebase-tool")); assert_eq!(exit.get_status(), &ExitStatus::Good); } @@ -25,7 +27,8 @@ fn successful_run_version() { #[serial_test::serial] fn successful_run_license() { let args = ["--license"].into_iter().map(OsString::from).collect(); - let exit = run(args); + let git_config_parameters = Vec::new(); + let exit = run(args, git_config_parameters); assert!( exit.get_message() .unwrap() @@ -39,8 +42,9 @@ fn successful_run_editor() { with_git_directory("fixtures/simple", |path| { let todo_file = Path::new(path).join("rebase-todo-empty").into_os_string(); let args = vec![todo_file]; + let git_config_parameters = Vec::new(); assert_eq!( - run(args).get_status(), + run(args, git_config_parameters).get_status(), &ExitStatus::Good ); }); @@ -52,5 +56,6 @@ fn successful_run_editor() { #[expect(unsafe_code)] fn error() { let args = unsafe { vec![OsString::from(String::from_utf8_unchecked(vec![0xC3, 0x28]))] }; - assert_eq!(run(args).get_status(), &ExitStatus::StateError); + let git_config_parameters = Vec::new(); + assert_eq!(run(args, git_config_parameters).get_status(), &ExitStatus::StateError); }