diff --git a/README.md b/README.md index 4f48937..834e7af 100644 --- a/README.md +++ b/README.md @@ -130,6 +130,15 @@ against the committing repository's working-directory name (e.g. `claude-src` for a repo checked out at `~/src/claude-src`) — so it holds no matter which subdirectory the commit is made from. +`[obsidian]` and `[templates]` are required; a config missing either one is +fatal. Any **other** section is ignored, with a line on stderr naming it, never +fatal. One INI file is shared by every checkout on the machine, so a section +written for a newer release must not break a binary that predates it — which is +exactly what adding `[exclude]` did to every checkout older than 4.17.0. + +The stderr line matters: a misspelt section (`[excludes]`) is ignored too, so +without it your exclusions would silently stop applying. + --- ## Roadmap & Improvements 📈 diff --git a/src/config.rs b/src/config.rs index df7ca89..54d1c4e 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,4 +1,4 @@ -use log::{error, info}; +use log::{error, info, warn}; use std::{ fs, @@ -611,6 +611,11 @@ impl GlobalVars { .get(section, key) } + /// The sections this binary understands. Adding a section to + /// `set_obsidian_vars`' dispatch means adding it here too, or the section + /// gets applied *and* reported as unrecognised. + const KNOWN_SECTIONS: [&'static str; 3] = ["obsidian", "templates", "exclude"]; + fn get_sections_from_config(&self) -> Vec { info!("[GlobalVars::get_sections_from_config()] Getting sections from config"); let sections = self.get_config().sections(); @@ -619,21 +624,39 @@ impl GlobalVars { let has_required = ["obsidian", "templates"] .iter() .all(|required| sections.iter().any(|s| s == required)); - let all_known = sections - .iter() - .all(|s| ["obsidian", "templates", "exclude"].contains(&s.as_str())); - if has_required && all_known { - sections - } else { + if !has_required { error!( // LCOV_EXCL_START "[GlobalVars::get_sections_from_config()] These are the sections found: {sections:?}" ); // LCOV_EXCL_STOP panic!( - "[GlobalVars::get_sections_from_config()] config must have [obsidian] and [templates], plus an optional [exclude]." + "[GlobalVars::get_sections_from_config()] config must have [obsidian] and [templates]." ) } + + // An unrecognised section is ignored, never fatal: the config is shared + // by every checkout on the machine, so a section added for a newer + // release must not brick a binary that predates it. Adding [exclude] + // is exactly what killed every checkout older than 4.17.0. + let unknown: Vec<&String> = sections + .iter() + .filter(|s| !Self::KNOWN_SECTIONS.contains(&s.as_str())) + .collect(); + if !unknown.is_empty() { + warn!( + "[GlobalVars::get_sections_from_config()] ignoring unrecognised config sections {unknown:?}; this binary may be older than the config" + ); + // Also on stderr: the git hook runs without RUST_LOG, where + // env_logger caps the level at Error and would swallow the warning + // entirely. A misspelt section must never be silent - that is how + // an [excludes] typo would quietly journal the repos you excluded. + eprintln!( + "rusty-commit-saver: ignoring unrecognised config sections {unknown:?}; this binary may be older than the config" + ); + } + + sections } /// Loads all configuration variables from the "obsidian" and "templates" sections. @@ -644,15 +667,19 @@ impl GlobalVars { /// /// - For the **"obsidian"** section: calls `set_obsidian_root_path_dir` and `set_obsidian_commit_path`. /// - For the **"templates"** section: calls `set_templates_commit_date_path` and `set_templates_datetime`. + /// - For the **"exclude"** section: calls `set_excluded_repos`. + /// + /// Any other section is skipped. /// /// # Panics /// - /// Panics if the INI file contains a section other than "obsidian" or "templates", as only these two sections are supported. + /// Panics if the INI file is missing `[obsidian]` or `[templates]`; both are + /// required. An unrecognised section is not fatal. /// /// # Logging /// /// - Logs an info message when applying each section. - /// - Logs an error right before panicking on unsupported sections. + /// - Logs an error right before panicking on a missing required section. /// /// # Examples /// @@ -681,8 +708,9 @@ impl GlobalVars { info!("[GlobalVars::set_obsidian_vars()] Setting 'exclude' section variables."); self.set_excluded_repos(§ion); } - // No `else`: `get_sections_from_config()` is the single validation - // point and has already rejected any unknown section. + // No `else`: an unrecognised section is deliberately skipped here. + // `get_sections_from_config()` returns it after warning about it, + // because a config written for a newer release must not be fatal. } } @@ -892,7 +920,7 @@ impl GlobalVars { fn set_obsidian_root_path_dir(&self, section: &str) { let string_path = self .get_key_from_section_from_ini(section, "root_path_dir") - .expect("Could not get commit_path from config"); + .expect("Could not get root_path_dir from config"); let fixed_home = if string_path.contains('~') { info!("[GlobalVars::set_obsidian_root_path_dir()]: Does contain ~"); @@ -1330,7 +1358,7 @@ mod global_vars_tests { } #[test] - fn test_get_sections_from_config_invalid_count() { + fn test_get_sections_from_config_rejects_a_lone_unknown_section() { let mut config = Ini::new(); config.set("only_one_section", "key", Some("value".to_string())); @@ -1340,7 +1368,10 @@ mod global_vars_tests { let result = panic::catch_unwind(AssertUnwindSafe(|| global_vars.get_sections_from_config())); - assert!(result.is_err(), "Expected panic for invalid section count"); + assert!( + result.is_err(), + "Expected panic: the required sections are missing" + ); // Verify the panic message (panic! with string literal = &str) let panic_info = result.unwrap_err(); @@ -1367,7 +1398,7 @@ mod global_vars_tests { } #[test] - fn test_get_sections_from_config_panics_with_three_sections() { + fn test_get_sections_from_config_keeps_an_extra_section() { let mut config = Ini::new(); config.set("obsidian", "root_path_dir", Some("/tmp/test".to_string())); config.set("templates", "commit_date_path", Some("%Y.md".to_string())); @@ -1379,7 +1410,11 @@ mod global_vars_tests { let result = panic::catch_unwind(AssertUnwindSafe(|| global_vars.get_sections_from_config())); - assert!(result.is_err(), "Expected panic for three sections"); + assert!( + result.is_ok(), + "An unrecognised section must be ignored, not fatal" + ); + assert_eq!(result.unwrap().len(), 3); } #[test] @@ -1617,9 +1652,10 @@ mod global_vars_tests { #[test] #[should_panic(expected = "must have [obsidian] and [templates]")] - fn test_set_obsidian_vars_invalid_section() { + fn test_set_obsidian_vars_without_the_obsidian_section() { let mut config = Ini::new(); - // Add correct number of sections (2) but with wrong name + // [templates] is present but [obsidian] is not, which is fatal. The + // unrecognised section is incidental - it is not what makes this panic. config.set("invalid_section", "key", Some("value".to_string())); config.set( "templates", @@ -1635,7 +1671,7 @@ mod global_vars_tests { let global_vars = GlobalVars::new(); global_vars.config.set(config).unwrap(); - // Should panic because "invalid_section" is not "obsidian" or "templates" + // Panics: [obsidian] is required and missing. global_vars.set_obsidian_vars(); } @@ -1849,7 +1885,7 @@ mod global_vars_tests { } #[test] - #[should_panic(expected = "Could not get")] + #[should_panic(expected = "Could not get root_path_dir")] fn test_set_obsidian_root_path_dir_missing_key() { let mut config = Ini::new(); config.set("obsidian", "commit_path", Some("commits".to_string())); @@ -2203,7 +2239,7 @@ commit_datetime=%Y-%m-%d %H:%M:%S // } #[test] - fn test_line_606_explicit_coverage() { + fn test_missing_required_sections_panic_message() { use std::panic; let mut config = Ini::new(); @@ -2520,6 +2556,36 @@ commit_datetime = %Y-%m-%d %H:%M:%S ); } + #[test] + fn test_get_sections_tolerates_an_unknown_section() { + // A config written for a newer release must not brick an older binary: + // an unrecognised section is ignored, not fatal. This is what made the + // pre-4.17.0 binaries panic once [exclude] was added to the shared ini. + let mut config = Ini::new(); + config.set("obsidian", "root_path_dir", Some("/tmp/test".to_string())); + config.set("templates", "commit_date_path", Some("%Y.md".to_string())); + config.set("from_a_future_release", "key", Some("value".to_string())); + + let global_vars = GlobalVars::new(); + global_vars.config.set(config).unwrap(); + + let sections = global_vars.get_sections_from_config(); + assert!(sections.contains(&"obsidian".to_string())); + assert!(sections.contains(&"templates".to_string())); + } + + #[test] + #[should_panic(expected = "must have [obsidian] and [templates]")] + fn test_get_sections_rejects_obsidian_without_templates() { + // The mirror of the case below: [obsidian] alone is just as fatal. + let mut config = Ini::new(); + config.set("obsidian", "root_path_dir", Some("/tmp/test".to_string())); + + let global_vars = GlobalVars::new(); + global_vars.config.set(config).unwrap(); + let _ = global_vars.get_sections_from_config(); + } + #[test] #[should_panic(expected = "must have [obsidian] and [templates]")] fn test_get_sections_rejects_missing_required_section() { diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index eeef3ed..d2a9716 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -37,3 +37,74 @@ fn test_global_vars_full_integration_workflow() { assert_eq!(date_path, "%Y-%m-%d.md"); assert_eq!(datetime, "%Y-%m-%d %H:%M"); } + +/// An unrecognised config section must be reported on stderr, not swallowed. +/// +/// The git hook runs the binary with no `RUST_LOG`, where `env_logger` caps the +/// level at Error, so `log::warn!` alone is invisible. Without a visible +/// report, a misspelt section (`[excludes]`) silently disables exclusion and +/// the repos meant to be skipped get journalled. +#[test] +fn unknown_config_section_is_reported_on_stderr() { + let dir = tempfile::tempdir().unwrap(); + let ini = dir.path().join("rusty-commit-saver.ini"); + fs::write(&ini, ini_with_section(dir.path(), "excludes")).unwrap(); + + let output = std::process::Command::new(env!("CARGO_BIN_EXE_rusty-commit-saver")) + .env("RUSTY_COMMIT_SAVER_CONFIG", &ini) + .env_remove("RUST_LOG") + .current_dir(dir.path()) + .output() + .expect("the binary should run"); + + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("ignoring unrecognised config sections"), + "the unknown section was not reported; stderr was: {stderr}" + ); + assert!( + stderr.contains("excludes"), + "the report did not name the section; stderr was: {stderr}" + ); +} + +/// The counterpart: a config with only known sections reports nothing. +#[test] +fn known_config_sections_are_reported_silently() { + let dir = tempfile::tempdir().unwrap(); + let ini = dir.path().join("rusty-commit-saver.ini"); + fs::write(&ini, ini_with_section(dir.path(), "exclude")).unwrap(); + + let output = std::process::Command::new(env!("CARGO_BIN_EXE_rusty-commit-saver")) + .env("RUSTY_COMMIT_SAVER_CONFIG", &ini) + .env_remove("RUST_LOG") + .current_dir(dir.path()) + .output() + .expect("the binary should run"); + + let stderr = String::from_utf8_lossy(&output.stderr); + // Positive control first: the run must actually reach past config loading, + // otherwise the assertion below would pass vacuously. There is no git repo + // in the temp cwd, so the run dies right after the config is read. + assert!( + stderr.contains("panicked"), + "the run did not get past config loading; stderr was: {stderr}" + ); + assert!( + !stderr.contains("ignoring unrecognised config sections"), + "a fully known config must not report anything; stderr was: {stderr}" + ); +} + +/// A minimal valid config whose vault lives inside `dir`, plus one extra +/// section under `extra` - `exclude` for the known case, anything else for the +/// unknown one. The vault path stays inside the temp dir so a run that gets +/// further than expected cannot write outside it. +fn ini_with_section(dir: &std::path::Path, extra: &str) -> String { + format!( + "[obsidian]\nroot_path_dir={}\ncommit_path=Commits\n\ + [templates]\ncommit_date_path=%Y-%m-%d.md\ncommit_datetime=%H:%M\n\ + [{extra}]\nrepos=claude-src\n", + dir.join("vault").display() + ) +}