From 3ee2a6bed6aafa7f984c54da76f8ef9775b19405 Mon Sep 17 00:00:00 2001 From: Chess Seventh Date: Wed, 29 Jul 2026 08:39:43 +0200 Subject: [PATCH 1/3] fix(config): ignore unknown ini sections instead of panicking One ini file is shared by every checkout on the machine, so a section written for a newer release reached binaries that predate it: adding [exclude] made every checkout older than 4.17.0 panic on the section count and stop journalling its commits. Strict section validation turns any future config addition into the same outage. - keep [obsidian] and [templates] required; missing either is still fatal - ignore any other section instead of panicking - report the ignored section on stderr as well as through log::warn, because the git hook runs without RUST_LOG and env_logger would swallow the warning; an [excludes] typo must not silently journal the repos you excluded - cover that report with integration tests that run the binary: removing the stderr line fails them, which the unit tests alone did not - hold the known-section list in one const, so a section added to the dispatch cannot be applied and reported as unrecognised at the same time - correct the set_obsidian_vars rustdoc and its call-site comment, which both still promised that an unknown section was rejected - tests: flip the three-sections case, add a section from a future release and the [obsidian]-without-[templates] mirror, and rename three tests whose names encoded the pre-4.17 contract - state the forward-compatibility contract in the README config section Co-Authored-By: Vulcan --- README.md | 9 ++++ src/config.rs | 101 ++++++++++++++++++++++++++++++------- tests/integration_tests.rs | 63 +++++++++++++++++++++++ 3 files changed, 154 insertions(+), 19 deletions(-) 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..f3990c2 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. } } @@ -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())); @@ -1367,7 +1395,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 +1407,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 +1649,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 +1668,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(); } @@ -2203,7 +2236,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 +2553,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 + // 4.14.x pins 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..b60eca5 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -37,3 +37,66 @@ 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, + "[obsidian]\nroot_path_dir=/tmp/rcs-test\ncommit_path=Commits\n\ + [templates]\ncommit_date_path=%Y-%m-%d.md\ncommit_datetime=%H:%M\n\ + [excludes]\nrepos=claude-src\n", + ) + .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, + "[obsidian]\nroot_path_dir=/tmp/rcs-test\ncommit_path=Commits\n\ + [templates]\ncommit_date_path=%Y-%m-%d.md\ncommit_datetime=%H:%M\n\ + [exclude]\nrepos=claude-src\n", + ) + .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"), + "a fully known config must not report anything; stderr was: {stderr}" + ); +} From 420dc7b2f6cd3c244270b9adb9edd82336a1f91c Mon Sep 17 00:00:00 2001 From: Chess Seventh Date: Thu, 30 Jul 2026 01:13:11 +0200 Subject: [PATCH 2/3] fix(config): name the right key when root_path_dir is missing The panic on a missing root_path_dir blamed commit_path, sending anyone debugging a broken config at the wrong line of their ini. - report root_path_dir in its own expect message Co-Authored-By: Vulcan --- src/config.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config.rs b/src/config.rs index f3990c2..ff53a15 100644 --- a/src/config.rs +++ b/src/config.rs @@ -920,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 ~"); From 111ccbe18b8a4905651f075252b53aca88baddc8 Mon Sep 17 00:00:00 2001 From: Chess Seventh Date: Thu, 30 Jul 2026 01:24:35 +0200 Subject: [PATCH 3/3] test(config): make the new config guards mutation-proof The review found the new guards passed for weak reasons: the stderr test could have passed vacuously, and the corrected panic message was asserted only by its prefix, so reverting it went unnoticed. - pin the root_path_dir expect message in should_panic, not just "Could not get"; reverting the message now fails the test - assert the run reaches past config loading before asserting the silent case, so a config that never loads cannot pass it - keep the test vault inside the temp dir instead of a shared /tmp path - drop two stale wordings: an assert message about a section count that is no longer validated, and a comment naming 4.14.x where any pre-4.17.0 binary is affected Co-Authored-By: Vulcan --- src/config.rs | 9 ++++++--- tests/integration_tests.rs | 36 ++++++++++++++++++++++-------------- 2 files changed, 28 insertions(+), 17 deletions(-) diff --git a/src/config.rs b/src/config.rs index ff53a15..54d1c4e 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1368,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(); @@ -1882,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())); @@ -2557,7 +2560,7 @@ commit_datetime = %Y-%m-%d %H:%M:%S 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 - // 4.14.x pins panic once [exclude] was added to the shared ini. + // 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())); diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index b60eca5..d2a9716 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -48,13 +48,7 @@ fn test_global_vars_full_integration_workflow() { 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, - "[obsidian]\nroot_path_dir=/tmp/rcs-test\ncommit_path=Commits\n\ - [templates]\ncommit_date_path=%Y-%m-%d.md\ncommit_datetime=%H:%M\n\ - [excludes]\nrepos=claude-src\n", - ) - .unwrap(); + 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) @@ -79,13 +73,7 @@ fn unknown_config_section_is_reported_on_stderr() { fn known_config_sections_are_reported_silently() { let dir = tempfile::tempdir().unwrap(); let ini = dir.path().join("rusty-commit-saver.ini"); - fs::write( - &ini, - "[obsidian]\nroot_path_dir=/tmp/rcs-test\ncommit_path=Commits\n\ - [templates]\ncommit_date_path=%Y-%m-%d.md\ncommit_datetime=%H:%M\n\ - [exclude]\nrepos=claude-src\n", - ) - .unwrap(); + 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) @@ -95,8 +83,28 @@ fn known_config_sections_are_reported_silently() { .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() + ) +}