From c63b6837af816b3e1374ddb18359679b6f32b09c Mon Sep 17 00:00:00 2001 From: Chess Seventh Date: Thu, 30 Jul 2026 23:04:07 +0200 Subject: [PATCH 01/21] docs(feature): brief the config key-skew fix The shared ini and the binary reading it drift apart by design, so config skew has two axes. Only sections were made tolerant in 4.17.3. - record both key faults measured through a real post-commit hook: an unrecognised key is swallowed in total silence, and a missing required key panics with a message that never names the config file - record the decision: unrecognised keys warn and continue, required keys stay fatal because a missing destination must not look like a quiet day - state the acceptance scenarios that drive the fix, including the real-commit gate at the hook boundary Co-Authored-By: Vulcan --- docs/feature/L76-config-key-skew/brief.md | 104 ++++++++++++++++++++++ 1 file changed, 104 insertions(+) create mode 100644 docs/feature/L76-config-key-skew/brief.md diff --git a/docs/feature/L76-config-key-skew/brief.md b/docs/feature/L76-config-key-skew/brief.md new file mode 100644 index 0000000..0fe4e66 --- /dev/null +++ b/docs/feature/L76-config-key-skew/brief.md @@ -0,0 +1,104 @@ +# Bug-fix brief - L76 config key skew + +> Lean nWave motion, standard-rigor gate. Second axis of the outage L66 closed: +> 4.17.3 made an unrecognised config SECTION non-fatal, but the KEYS were left +> as they were. Root cause known up front; this brief records the defect, the +> decision, the gate, and the acceptance scenarios that drive +> RED -> GREEN -> COMMIT. + +## Defect + +One INI file at `~/.config/rusty-commit-saver/rusty-commit-saver.ini` is shared +by every checkout on the machine, while the binary reading it is pinned +per-repo and per-home-manager-generation. Config and binary therefore drift +apart by design, and the config parser has two axes of skew: sections and keys. +Only sections were made tolerant. + +Measured on 4.17.3, by driving real commits through a real post-commit hook +(`git init` repo, `core.hooksPath` pointing at a hook that execs the binary): + +| config fault | what the user sees | journalled | +|---|---|---| +| unrecognised section | one stderr line naming the section | yes | +| unrecognised key | **nothing at all** | yes | +| missing required key | `thread 'main' panicked at src/config.rs:862:14: Could not get commit_path from config` + backtrace note | no | + +Both key rows are wrong, and they compound: + +1. **An unrecognised key is swallowed in total silence.** `configparser` simply + never returns a key nobody asks for, and there is no key-level equivalent of + `KNOWN_SECTIONS` (`src/config.rs:617`). A `commit_datetimes` typo, or a key + renamed by a newer release, applies nothing and says nothing. +2. **A required key that is absent panics with a message that does not name the + config file.** Four sites hard-`.expect()` on the `Option`: + `src/config.rs:738-747` (`commit_datetime`), `819-830` (`commit_date_path`), + `859-885` (`commit_path`), `920-950` (`root_path_dir`). The panic text names + the key but not the file, and `retrieve_config_file_path()` + (`src/config.rs:1050`) returns the file *contents*, so the resolved path is + not retained anywhere for an error message to use. + +Together they make a rename the same outage as L66 through a different door: +the new key is ignored silently (1), the old key is missing, so the run panics +(2) - and the message points at a key rather than at the file to edit. + +## Decision: fatal, but say why + +A post-commit hook cannot abort a commit - git ignores its exit status, which +the gate below verifies (`git commit` exits 0 and the commit exists in every +fault case). So the real cost of a config fault is stderr noise plus a +silently missing diary entry, never a lost commit. + +Given that, the policy is: + +- **Unrecognised key -> warn and continue.** Same reasoning as the section fix: + a config written for a newer release must not brick an older binary. +- **Missing required key -> still fatal**, because without it there is no + destination to write to. Degrading to "journal nothing, exit 0" would make a + broken config indistinguishable from a quiet day, and the diary would stop + for weeks unnoticed. The fix is the message, not the severity: name the + resolved config file and the exact `[section] key`. +- Both warnings go to **stderr as well as the log**, because the hook runs + without `RUST_LOG` and `env_logger` caps the level at Error there - a + log-only warning is invisible in practice. + +Decided by Franci, 2026-07-30, over "never fatal, always degrade" and "fatal +only when there is no destination". + +## Fix + +- New `KNOWN_KEYS` table beside `KNOWN_SECTIONS` (`src/config.rs`), listing what + each known section understands. +- New `unrecognised_keys()` returning the sorted `[section] key` list, and + `report_unrecognised_keys()` warning it to log + stderr, called from + `set_obsidian_vars()`. +- `GlobalVars` gains `config_path`, set in `set_all()` from the resolved path, + so an error can name the file. `get_ini_file_at()` / `read_config_file()` + split out for that; `get_ini_file()` and `retrieve_config_file_path()` keep + their signatures and behaviour. +- New `require_key()` replaces the four `.expect()` calls with one fatal path + whose message names the file and the key. + +## Gate (acceptance scenarios) + +1. A key the binary does not know, in a section it does -> named on stderr, + run continues, commit still journalled. *(the silent half)* +2. A required key absent -> run aborts with a message naming the resolved + config file and `[section] key`, no bare "Could not get X from config". + *(the unhelpful half)* +3. An unrecognised **section** -> still warns and continues (4.17.3 behaviour + must not regress). +4. A good config -> no warning at all on stderr, entry journalled as before. + +Verified by unit tests in `src/config.rs`, plus - and this is the point of the +lane - the real-hook gate: a genuine `git commit` in a throwaway repo whose +`core.hooksPath` runs the built binary against each config above, asserting the +stderr text, the commit's exit status, and whether a diary file appeared. +Script: `tests/hook-gate.sh`. Full gate: `devenv shell -- pre-check`. + +## Deploy (Franci) + +Merge to master -> the release workflow bumps + tags from the conventional +`fix:` commit; then `up-hm` deploys the new binary. The machine's global +post-commit hook still pins 4.17.0, which is unaffected by the current config +(it knows `[obsidian]`, `[templates]` and `[exclude]`) - verified, no live +outage waiting on this deploy. From 84dda4af0fe21e1a6586f56b3f1248b6500ed8e7 Mon Sep 17 00:00:00 2001 From: Chess Seventh Date: Thu, 30 Jul 2026 23:09:35 +0200 Subject: [PATCH 02/21] fix(config): report unrecognised ini keys instead of hiding them A key this binary does not know was swallowed in total silence, so a misspelt or renamed key applied nothing and said nothing. - add a KNOWN_KEYS table beside KNOWN_SECTIONS and name every key that falls outside it, sorted so hash-map order cannot leak into the output - warn to stderr as well as the log, because the git hook runs without RUST_LOG and would swallow a log-only warning - keep it non-fatal, for the reason an unknown section is: one ini file is shared by every checkout, so a newer config must not brick an old binary - leave the keys of an unrecognised section alone; that section is already reported whole, and listing its keys would charge one mistake twice Co-Authored-By: Vulcan --- src/config.rs | 177 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 174 insertions(+), 3 deletions(-) diff --git a/src/config.rs b/src/config.rs index 54d1c4e..aed6ebf 100644 --- a/src/config.rs +++ b/src/config.rs @@ -616,6 +616,69 @@ impl GlobalVars { /// gets applied *and* reported as unrecognised. const KNOWN_SECTIONS: [&'static str; 3] = ["obsidian", "templates", "exclude"]; + /// The keys each known section understands. Adding a key to a setter means + /// adding it here too, or the key gets applied *and* reported as + /// unrecognised. + const KNOWN_KEYS: [(&'static str, &'static [&'static str]); 3] = [ + ("obsidian", &["root_path_dir", "commit_path"]), + ("templates", &["commit_date_path", "commit_datetime"]), + ("exclude", &["repos"]), + ]; + + /// Lists the keys this binary does not understand, as sorted + /// `[section] key` labels. + /// + /// Only keys in a *known* section are listed: an unrecognised section is + /// already reported whole by [`get_sections_from_config()`](Self::get_sections_from_config), + /// and listing its keys as well would charge one mistake twice. + /// + /// Sorted because the parser holds keys in a hash map, whose iteration + /// order would otherwise vary from run to run. + fn unrecognised_keys(&self) -> Vec { + let config = self.get_config(); + let map = config.get_map_ref(); + let mut unknown = Vec::new(); + + for (section, known) in Self::KNOWN_KEYS { + let Some(present) = map.get(section) else { + continue; + }; + for key in present.keys() { + if !known.contains(&key.as_str()) { + unknown.push(format!("[{section}] {key}")); + } + } + } + + unknown.sort(); + unknown + } + + /// Reports every unrecognised key, then carries on. + /// + /// An unknown key is never fatal, for the reason an unknown section is not: + /// one INI file is shared by every checkout on the machine, so a key + /// written for a newer release must not brick a binary that predates it. + /// Reporting it is what a silent skip failed to do - a misspelt + /// `commit_datetimes` used to apply nothing and say nothing. + fn report_unrecognised_keys(&self) { + let unknown = self.unrecognised_keys(); + if unknown.is_empty() { + return; + } + + let list = unknown.join(", "); + warn!( + "[GlobalVars::report_unrecognised_keys()] ignoring unrecognised config keys {list}; this binary may be older than the config" + ); + // Also on stderr, for the same reason the section warning is: the git + // hook runs without RUST_LOG, where env_logger caps the level at Error + // and would swallow the warning entirely. + eprintln!( + "rusty-commit-saver: ignoring unrecognised config keys {list}; this binary may be older than the config" + ); + } + fn get_sections_from_config(&self) -> Vec { info!("[GlobalVars::get_sections_from_config()] Getting sections from config"); let sections = self.get_config().sections(); @@ -669,12 +732,13 @@ impl GlobalVars { /// - 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. + /// Any other section is skipped. Keys the binary does not understand are + /// reported on stderr and skipped too. /// /// # Panics /// /// Panics if the INI file is missing `[obsidian]` or `[templates]`; both are - /// required. An unrecognised section is not fatal. + /// required. An unrecognised section or key is not fatal. /// /// # Logging /// @@ -695,7 +759,10 @@ impl GlobalVars { /// global_vars.set_obsidian_vars(); /// ``` pub fn set_obsidian_vars(&self) { - for section in self.get_sections_from_config() { + let sections = self.get_sections_from_config(); + self.report_unrecognised_keys(); + + for section in sections { if section == "obsidian" { info!("[GlobalVars::set_obsidian_vars()] Setting 'obsidian' section variables."); self.set_obsidian_root_path_dir(§ion); @@ -1417,6 +1484,110 @@ mod global_vars_tests { assert_eq!(result.unwrap().len(), 3); } + #[test] + fn test_unrecognised_keys_names_a_typo_in_a_known_section() { + let mut config = Ini::new(); + config.set("obsidian", "root_path_dir", Some("/tmp/test".to_string())); + config.set("obsidian", "commit_path", Some("commits".to_string())); + config.set("templates", "commit_date_path", Some("%F.md".to_string())); + // The rename/typo case: the real key is still there, so nothing breaks + // - which is exactly why this used to pass unnoticed. + config.set("templates", "commit_datetime", Some("%T".to_string())); + config.set("templates", "commit_datetimes", Some("%T".to_string())); + + let global_vars = GlobalVars::new(); + global_vars.config.set(config).unwrap(); + + assert_eq!( + global_vars.unrecognised_keys(), + vec!["[templates] commit_datetimes".to_string()], + "an unknown key must be named, not swallowed" + ); + } + + #[test] + fn test_unrecognised_keys_is_empty_for_a_known_config() { + let mut config = Ini::new(); + config.set("obsidian", "root_path_dir", Some("/tmp/test".to_string())); + config.set("obsidian", "commit_path", Some("commits".to_string())); + config.set("templates", "commit_date_path", Some("%F.md".to_string())); + config.set("templates", "commit_datetime", Some("%T".to_string())); + config.set("exclude", "repos", Some("claude-src".to_string())); + + let global_vars = GlobalVars::new(); + global_vars.config.set(config).unwrap(); + + assert!( + global_vars.unrecognised_keys().is_empty(), + "a config this binary fully understands must warn about nothing" + ); + } + + #[test] + fn test_unrecognised_keys_leaves_an_unknown_section_to_the_section_check() { + let mut config = Ini::new(); + config.set("obsidian", "root_path_dir", Some("/tmp/test".to_string())); + config.set("obsidian", "commit_path", Some("commits".to_string())); + config.set("templates", "commit_date_path", Some("%F.md".to_string())); + config.set("templates", "commit_datetime", Some("%T".to_string())); + config.set("future_release", "whatever", Some("value".to_string())); + + let global_vars = GlobalVars::new(); + global_vars.config.set(config).unwrap(); + + assert!( + global_vars.unrecognised_keys().is_empty(), + "the section is already reported whole; its keys must not double the noise" + ); + } + + #[test] + fn test_unrecognised_keys_are_sorted_and_name_every_section() { + let mut config = Ini::new(); + config.set("obsidian", "root_path_dir", Some("/tmp/test".to_string())); + config.set("obsidian", "commit_path", Some("commits".to_string())); + config.set("obsidian", "vault", Some("stale".to_string())); + config.set("templates", "commit_date_path", Some("%F.md".to_string())); + config.set("templates", "commit_datetime", Some("%T".to_string())); + config.set("templates", "author", Some("stale".to_string())); + config.set("exclude", "repos", Some("claude-src".to_string())); + config.set("exclude", "branches", Some("stale".to_string())); + + let global_vars = GlobalVars::new(); + global_vars.config.set(config).unwrap(); + + assert_eq!( + global_vars.unrecognised_keys(), + vec![ + "[exclude] branches".to_string(), + "[obsidian] vault".to_string(), + "[templates] author".to_string(), + ], + "hash-map order must not leak into the reported list" + ); + } + + #[test] + fn test_set_obsidian_vars_survives_an_unrecognised_key() { + let mut config = Ini::new(); + config.set("obsidian", "root_path_dir", Some("/tmp/test".to_string())); + config.set("obsidian", "commit_path", Some("commits".to_string())); + config.set("templates", "commit_date_path", Some("%F.md".to_string())); + config.set("templates", "commit_datetime", Some("%T".to_string())); + config.set("templates", "commit_datetimes", Some("%T".to_string())); + + let global_vars = GlobalVars::new(); + global_vars.config.set(config).unwrap(); + + let result = panic::catch_unwind(AssertUnwindSafe(|| global_vars.set_obsidian_vars())); + + assert!( + result.is_ok(), + "an unrecognised key must be reported, never fatal" + ); + assert_eq!(global_vars.get_template_commit_datetime(), "%T"); + } + #[test] fn test_get_key_from_section_from_ini_exists() { let mut config = Ini::new(); From bbf6c0ac0aeb225295d46854379d7a57c9077479 Mon Sep 17 00:00:00 2001 From: Chess Seventh Date: Thu, 30 Jul 2026 23:11:54 +0200 Subject: [PATCH 03/21] refactor(config): resolve the ini path once and retain it No behaviour change. Groundwork for naming the config file in an error: the resolved path was thrown away, because the function that looks like it returns a path returns the file contents. - split read_config_file() out of retrieve_config_file_path(), and add a get_ini_file_at() that takes the path instead of resolving it again - keep both public entry points and their behaviour exactly as they were - store the resolved path on GlobalVars, left unset when a caller hands in a config directly Co-Authored-By: Vulcan --- src/config.rs | 49 ++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 44 insertions(+), 5 deletions(-) diff --git a/src/config.rs b/src/config.rs index aed6ebf..120d0fc 100644 --- a/src/config.rs +++ b/src/config.rs @@ -132,6 +132,13 @@ pub struct GlobalVars { /// accessed from multiple threads. pub config: OnceCell, + /// Path of the INI the configuration was read from. + /// + /// Retained so a configuration error can name the file to edit. Set by + /// [`set_all()`](Self::set_all); a `GlobalVars` handed a config directly + /// (as the tests do) leaves it unset. + pub config_path: OnceCell, + /// Root directory of the Obsidian vault. /// /// The base directory where all Obsidian files are stored. @@ -299,6 +306,7 @@ impl GlobalVars { info!("[GlobalVars::new()] Creating new GlobalVars with OnceCell default values."); GlobalVars { config: OnceCell::new(), + config_path: OnceCell::new(), obsidian_root_path_dir: OnceCell::new(), obsidian_commit_path: OnceCell::new(), @@ -360,9 +368,13 @@ impl GlobalVars { /// ``` pub fn set_all(&self) -> &Self { info!("[GlobalVars::set_all()] Setting all variables for GlobalVars"); - let config = get_ini_file(); + let config_path = get_or_default_config_ini_path(); + let config = get_ini_file_at(&config_path); info!("[GlobalVars::set_all()]: Setting Config Ini file."); + self.config_path + .set(config_path) + .expect("Couldn't set config_path in GlobalVars"); self.config .set(config) .expect("Coulnd't set config in GlobalVars"); @@ -1118,9 +1130,21 @@ pub fn retrieve_config_file_path() -> String { info!( "[UserInput::retrieve_config_file_path()]: retrieving the string path from CLI or default" ); - let config_path = get_or_default_config_ini_path(); + read_config_file(&get_or_default_config_ini_path()) +} - if Path::new(&config_path).exists() { +/// Reads the configuration file at `config_path` and returns its contents. +/// +/// Split from [`retrieve_config_file_path()`] so a caller that needs the path +/// itself - to name the file in an error - resolves it once and passes it in, +/// rather than resolving it a second time behind the caller's back. +/// +/// # Panics +/// +/// Panics if the file does not exist, or cannot be read. +#[must_use] +fn read_config_file(config_path: &str) -> String { + if Path::new(config_path).exists() { info!("[UserInput::retrieve_config_file_path()]: config_path exists {config_path:}"); } else { error!( @@ -1131,7 +1155,7 @@ pub fn retrieve_config_file_path() -> String { ); } info!("[UserInput::retrieve_config_file_path()] retrieved config path: {config_path:}"); - fs::read_to_string(config_path.clone()) + fs::read_to_string(config_path) .unwrap_or_else(|_| panic!("Should have been able to read the file: {config_path:}")) } @@ -1292,8 +1316,23 @@ pub fn get_default_ini_path() -> String { /// - [`parse_ini_content()`] - Parses INI text into `Ini` struct #[must_use] pub fn get_ini_file() -> Ini { + get_ini_file_at(&get_or_default_config_ini_path()) +} + +/// Loads and parses the INI configuration file at `config_path`. +/// +/// The path-taking half of [`get_ini_file()`], for a caller that has already +/// resolved the path and wants to keep it - [`GlobalVars::set_all()`] retains +/// it so a configuration error can name the file to edit. +/// +/// # Panics +/// +/// Panics under the same conditions as [`get_ini_file()`]: the file is +/// missing, unreadable, or not valid INI. +#[must_use] +pub fn get_ini_file_at(config_path: &str) -> Ini { info!("[get_ini_file()]: Retrieving the INI File"); - let content_ini = retrieve_config_file_path(); + let content_ini = read_config_file(config_path); let mut config = Ini::new(); config .read(content_ini) From 0df0b425986d43f7db930f95fd9a79f48a6dc904 Mon Sep 17 00:00:00 2001 From: Chess Seventh Date: Thu, 30 Jul 2026 23:19:14 +0200 Subject: [PATCH 04/21] fix(config): name the file and key when a required key is missing The panic named a key and a source line, never the config file to edit, so diagnosing a rename meant reading the source. - add require_key(), replacing four scattered expects with one fatal path that names the resolved config file and the [section] key - name the unrecognised keys of that same section in the same message: a misspelt commit_paths is the usual reason commit_path is missing - treat a blank value as missing; commit_path= passed the presence check and silently journalled into the vault root at exit 0 - withdraw the empty-root_path_dir contract a test used to bless, where an empty vault root resolved to / and the run carried on Co-Authored-By: Vulcan --- src/config.rs | 211 +++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 181 insertions(+), 30 deletions(-) diff --git a/src/config.rs b/src/config.rs index 120d0fc..c6204f0 100644 --- a/src/config.rs +++ b/src/config.rs @@ -647,18 +647,11 @@ impl GlobalVars { /// Sorted because the parser holds keys in a hash map, whose iteration /// order would otherwise vary from run to run. fn unrecognised_keys(&self) -> Vec { - let config = self.get_config(); - let map = config.get_map_ref(); let mut unknown = Vec::new(); - for (section, known) in Self::KNOWN_KEYS { - let Some(present) = map.get(section) else { - continue; - }; - for key in present.keys() { - if !known.contains(&key.as_str()) { - unknown.push(format!("[{section}] {key}")); - } + for (section, _) in Self::KNOWN_KEYS { + for key in self.unrecognised_keys_in(section) { + unknown.push(format!("[{section}] {key}")); } } @@ -666,6 +659,83 @@ impl GlobalVars { unknown } + /// The unrecognised keys of one known section, sorted, without the + /// `[section]` prefix. An unknown section has none by definition: the + /// binary has no idea what it should contain. + fn unrecognised_keys_in(&self, section: &str) -> Vec { + let Some((_, known)) = Self::KNOWN_KEYS.iter().find(|(name, _)| *name == section) else { + return Vec::new(); + }; + + let config = self.get_config(); + let Some(present) = config.get_map_ref().get(section) else { + return Vec::new(); + }; + + let mut unknown: Vec = present + .keys() + .filter(|key| !known.contains(&key.as_str())) + .cloned() + .collect(); + + unknown.sort(); + unknown + } + + /// Names the configuration file for an error message. + /// + /// Falls back to a plain description rather than a guessed path when the + /// config was handed in directly instead of read from disk. + fn config_file_label(&self) -> String { + self.config_path + .get() + .cloned() + .unwrap_or_else(|| "the rusty-commit-saver config".to_string()) + } + + /// Reads a key the binary cannot work without. + /// + /// Fatal by design. Without it there is no destination to write to, and a + /// hook that quietly journals nothing is indistinguishable from a quiet + /// day - the diary would stop for weeks before anyone noticed. What the + /// fatal path owes the user is a message they can act on: it names the + /// resolved config file, the `[section] key`, and any unrecognised key in + /// that same section, because a misspelt `commit_paths` is the usual + /// reason `commit_path` is missing and naming both at once saves reading + /// the source. + /// + /// A present-but-blank value counts as missing: `commit_path =` used to + /// satisfy the old presence check and silently journal into the vault + /// root instead of the configured folder. + /// + /// # Panics + /// + /// Panics if the key is absent, or its value is empty or whitespace. + fn require_key(&self, section: &str, key: &str) -> String { + let value = self + .get_key_from_section_from_ini(section, key) + .filter(|value| !value.trim().is_empty()); + + if let Some(value) = value { + return value; + } + + let file = self.config_file_label(); + let typos = self.unrecognised_keys_in(section); + let hint = if typos.is_empty() { + String::new() + } else { + format!("; unrecognised in [{section}]: {}", typos.join(", ")) + }; + + error!( + "[GlobalVars::require_key()] {file}: missing required key '{key}' in section [{section}]{hint}" + ); + panic!( + "rusty-commit-saver: {file}: missing required key '{key}' in section [{section}]{hint}" + ) + } + /// Reports every unrecognised key, then carries on. /// /// An unknown key is never fatal, for the reason an unknown section is not: @@ -816,9 +886,7 @@ impl GlobalVars { /// ``` fn set_templates_datetime(&self, section: &str) { info!("[GlobalVars::set_templates_datetime()]: Setting the templates_datetime."); - let key = self - .get_key_from_section_from_ini(section, "commit_datetime") - .expect("Could not get the commit_datetime from INI"); + let key = self.require_key(section, "commit_datetime"); self.template_commit_datetime .set(key) @@ -899,9 +967,7 @@ impl GlobalVars { info!( "[GlobalVars::set_templates_commit_date_path()]: Setting the template_commit_date_path." ); - let key = self - .get_key_from_section_from_ini(section, "commit_date_path") - .expect("Could not get the commit_date_path from INI"); + let key = self.require_key(section, "commit_date_path"); self.template_commit_date_path .set(key) @@ -936,9 +1002,7 @@ impl GlobalVars { /// commit_path = ~/Documents/Obsidian/Diaries/Commits /// ``` fn set_obsidian_commit_path(&self, section: &str) { - let string_path = self - .get_key_from_section_from_ini(section, "commit_path") - .expect("Could not get commit_path from config"); + let string_path = self.require_key(section, "commit_path"); let fixed_home = if string_path.contains('~') { info!("[GlobalVars::set_obsidian_commit_path()]: Path does contain: '~'."); @@ -997,9 +1061,7 @@ impl GlobalVars { /// root_path_dir = ~/Documents/Obsidian /// ``` 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 root_path_dir from config"); + let string_path = self.require_key(section, "root_path_dir"); let fixed_home = if string_path.contains('~') { info!("[GlobalVars::set_obsidian_root_path_dir()]: Does contain ~"); @@ -2057,6 +2119,11 @@ mod global_vars_tests { #[test] fn test_set_obsidian_root_path_dir_empty_string() { + // This used to assert the opposite - that an empty root_path_dir still + // produced a usable PathBuf. It did: `/`. The vault root silently + // became the filesystem root, and the run carried on at exit 0. A + // blank value now counts as a missing key, which is the whole point of + // the check; the old contract is deliberately withdrawn. let mut config = Ini::new(); config.set("obsidian", "root_path_dir", Some(String::new())); config.set( @@ -2068,16 +2135,24 @@ mod global_vars_tests { let global_vars = GlobalVars::new(); global_vars.config.set(config).unwrap(); - global_vars.set_obsidian_root_path_dir("obsidian"); - let result = global_vars.get_obsidian_root_path_dir(); + let result = panic::catch_unwind(AssertUnwindSafe(|| { + global_vars.set_obsidian_root_path_dir("obsidian") + })); + + let panic_info = result.expect_err("a blank root_path_dir must be fatal"); + let msg = panic_info + .downcast_ref::() + .expect("panic message should be a formatted String"); - // Should at least create a PathBuf (even if empty or just "/") - assert!(!result.to_string_lossy().is_empty()); + assert!( + msg.contains("missing required key 'root_path_dir' in section [obsidian]"), + "a blank value must be reported as the missing key it is: {msg}" + ); } #[test] - #[should_panic(expected = "Could not get commit_path from config")] + #[should_panic(expected = "missing required key 'commit_path' in section [obsidian]")] fn test_set_obsidian_commit_path_missing_key() { let mut config = Ini::new(); config.set("obsidian", "root_path_dir", Some("/tmp/test".to_string())); @@ -2095,7 +2170,7 @@ mod global_vars_tests { } #[test] - #[should_panic(expected = "Could not get root_path_dir")] + #[should_panic(expected = "missing required key 'root_path_dir' in section [obsidian]")] fn test_set_obsidian_root_path_dir_missing_key() { let mut config = Ini::new(); config.set("obsidian", "commit_path", Some("commits".to_string())); @@ -2113,7 +2188,7 @@ mod global_vars_tests { } #[test] - #[should_panic(expected = "Could not get the commit_date_path from INI")] + #[should_panic(expected = "missing required key 'commit_date_path' in section [templates]")] fn test_set_templates_commit_date_path_missing_key() { let mut config = Ini::new(); config.set("templates", "commit_datetime", Some("%Y-%m-%d".to_string())); @@ -2127,7 +2202,7 @@ mod global_vars_tests { } #[test] - #[should_panic(expected = "Could not get the commit_datetime from INI")] + #[should_panic(expected = "missing required key 'commit_datetime' in section [templates]")] fn test_set_templates_datetime_missing_key() { let mut config = Ini::new(); config.set( @@ -2144,6 +2219,82 @@ mod global_vars_tests { global_vars.set_templates_datetime("templates"); } + #[test] + #[should_panic(expected = "missing required key 'commit_path' in section [obsidian]")] + fn test_require_key_treats_a_blank_value_as_missing() { + // `commit_path =` used to satisfy the presence check and journal into + // the vault root instead of the configured folder, at exit 0. + let mut config = Ini::new(); + config.set("obsidian", "root_path_dir", Some("/tmp/test".to_string())); + config.set("obsidian", "commit_path", Some(" ".to_string())); + + let global_vars = GlobalVars::new(); + global_vars.config.set(config).unwrap(); + + global_vars.set_obsidian_commit_path("obsidian"); + } + + #[test] + fn test_require_key_names_the_config_file_and_the_typo() { + let mut config = Ini::new(); + config.set("obsidian", "root_path_dir", Some("/tmp/test".to_string())); + // The whole point: the missing key and the reason it is missing get + // named together, so nobody has to read the source to connect them. + config.set("obsidian", "commit_paths", Some("commits".to_string())); + + let global_vars = GlobalVars::new(); + global_vars.config.set(config).unwrap(); + global_vars + .config_path + .set("/tmp/some/rusty-commit-saver.ini".to_string()) + .unwrap(); + + let result = panic::catch_unwind(AssertUnwindSafe(|| { + global_vars.set_obsidian_commit_path("obsidian") + })); + + let panic_info = result.expect_err("a missing required key must be fatal"); + let msg = panic_info + .downcast_ref::() + .expect("panic message should be a formatted String"); + + assert!( + msg.contains("/tmp/some/rusty-commit-saver.ini"), + "the message must name the file to edit: {msg}" + ); + assert!( + msg.contains("missing required key 'commit_path' in section [obsidian]"), + "the message must name the key and its section: {msg}" + ); + assert!( + msg.contains("unrecognised in [obsidian]: commit_paths"), + "the message must name the typo that explains the absence: {msg}" + ); + } + + #[test] + fn test_require_key_says_which_config_when_none_was_read() { + let mut config = Ini::new(); + config.set("obsidian", "commit_path", Some("commits".to_string())); + + let global_vars = GlobalVars::new(); + global_vars.config.set(config).unwrap(); + + let result = panic::catch_unwind(AssertUnwindSafe(|| { + global_vars.set_obsidian_root_path_dir("obsidian") + })); + + let panic_info = result.expect_err("a missing required key must be fatal"); + let msg = panic_info + .downcast_ref::() + .expect("panic message should be a formatted String"); + + assert!( + msg.contains("the rusty-commit-saver config"), + "with no file read, the message must say so rather than guess a path: {msg}" + ); + } + #[test] fn test_global_vars_set_all_method() { use std::io::Write; From 3b9fc1a1fee48741043272d8a08710d122b48634 Mon Sep 17 00:00:00 2001 From: Chess Seventh Date: Thu, 30 Jul 2026 23:47:04 +0200 Subject: [PATCH 05/21] fix(diary): honour the configured time format in the commit row [templates] commit_datetime was read from the config and required on pain of a fatal error, then never consumed: the TIME column was hardcoded to %H:%M:%S. A typo in that key could abort a run over a value nothing read. - thread the configured format from main through run_commit_saver and append_entry_to_diary to the row builder - cover it: the configured format reaches the row and the hardcoded one no longer wins No output change for the live config, whose value is already %H:%M:%S. Co-Authored-By: Vulcan --- src/lib.rs | 3 ++- src/main.rs | 67 ++++++++++++++++++++++++++++++++++++++++------- src/vim_commit.rs | 42 ++++++++++++++++++++++------- 3 files changed, 92 insertions(+), 20 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 6ee912c..9106496 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -28,10 +28,11 @@ //! let obsidian_root = global_vars.get_obsidian_root_path_dir(); //! let commit_path = global_vars.get_obsidian_commit_path(); //! let date_template = global_vars.get_template_commit_date_path(); +//! let time_template = global_vars.get_template_commit_datetime(); //! let excluded_repos = global_vars.get_excluded_repos(); //! //! // Save the commit (skips cleanly if this repo is excluded) -//! run_commit_saver(obsidian_root, &commit_path, &date_template, &excluded_repos).unwrap(); +//! run_commit_saver(obsidian_root, &commit_path, &date_template, &time_template, &excluded_repos).unwrap(); //! ``` //! //! ## Configuration diff --git a/src/main.rs b/src/main.rs index f1a585a..02e94a5 100644 --- a/src/main.rs +++ b/src/main.rs @@ -33,6 +33,7 @@ use std::path::PathBuf; /// * `obsidian_root_path_dir` - Base directory for Obsidian vault (e.g., `/home/user/Obsidian`) /// * `obsidian_commit_path` - Subdirectory for commits (e.g., `Diaries/Commits`) /// * `template_commit_date_path` - Chrono format for date hierarchy (e.g., `%Y/%m-%B/%F.md`) +/// * `template_commit_datetime` - Chrono format for the row's TIME column (e.g., `%H:%M:%S`) /// * `excluded_repos` - Repository names to skip; when the current repo's /// working-directory name matches, the run returns `Ok(())` without writing /// @@ -59,10 +60,11 @@ use std::path::PathBuf; /// let obsidian_root = PathBuf::from("/home/user/Obsidian"); /// let commit_path = PathBuf::from("Diaries/Commits"); /// let date_template = "%Y/%m-%B/%F.md"; // YYYY/MM-MonthName/YYYY-MM-DD.md +/// let time_template = "%H:%M:%S"; // the row's TIME column /// /// let excluded_repos: Vec = vec![]; // e.g. vec!["claude-src".to_string()] /// -/// match run_commit_saver(obsidian_root, &commit_path, date_template, &excluded_repos) { +/// match run_commit_saver(obsidian_root, &commit_path, date_template, time_template, &excluded_repos) { /// Ok(()) => println!("✓ Commit successfully logged!"), /// Err(e) => eprintln!("✗ Failed to log commit: {}", e), /// } @@ -102,6 +104,7 @@ pub fn run_commit_saver( obsidian_root_path_dir: PathBuf, obsidian_commit_path: &Path, template_commit_date_path: &str, + template_commit_datetime: &str, excluded_repos: &[String], ) -> Result<(), Box> { if let Some(repo_name) = current_repo_canonical_name() { @@ -143,7 +146,7 @@ pub fn run_commit_saver( } info!("[run_commit_saver()]: Writing the commit in the file."); - commit_saver_struct.append_entry_to_diary(&full_path)?; + commit_saver_struct.append_entry_to_diary(&full_path, template_commit_datetime)?; info!("[run_commit_saver]: Commit logged in "); Ok(()) @@ -161,12 +164,14 @@ fn main() { let obsidian_root_path_dir = global_vars.get_obsidian_root_path_dir(); let obsidian_commit_path = global_vars.get_obsidian_commit_path(); let template_commit_date_path = global_vars.get_template_commit_date_path(); + let template_commit_datetime = global_vars.get_template_commit_datetime(); let excluded_repos = global_vars.get_excluded_repos(); match run_commit_saver( obsidian_root_path_dir, &obsidian_commit_path, &template_commit_date_path, + &template_commit_datetime, &excluded_repos, ) { Ok(()) => (), @@ -294,7 +299,13 @@ mod main_tests { // This assumes we're in a git repo for CommitSaver::new() to work if Repository::discover("./").is_ok() { - let result = run_commit_saver(obsidian_root.clone(), &commit_path, date_template, &[]); + let result = run_commit_saver( + obsidian_root.clone(), + &commit_path, + date_template, + "%H:%M:%S", + &[], + ); // Should succeed and create diary file assert!(result.is_ok()); @@ -318,7 +329,13 @@ mod main_tests { // Only run if we're in a git repo if Repository::discover("./").is_ok() { - let result = run_commit_saver(obsidian_root.clone(), &commit_path, date_template, &[]); + let result = run_commit_saver( + obsidian_root.clone(), + &commit_path, + date_template, + "%H:%M:%S", + &[], + ); // Should succeed and create the missing directories assert!(result.is_ok()); @@ -339,10 +356,22 @@ mod main_tests { // Only run if in a git repo if Repository::discover("./").is_ok() { // First run - creates the file - run_commit_saver(obsidian_root.clone(), &commit_path, date_template, &[])?; + run_commit_saver( + obsidian_root.clone(), + &commit_path, + date_template, + "%H:%M:%S", + &[], + )?; // Second run - should append to existing file - let result = run_commit_saver(obsidian_root.clone(), &commit_path, date_template, &[]); + let result = run_commit_saver( + obsidian_root.clone(), + &commit_path, + date_template, + "%H:%M:%S", + &[], + ); assert!(result.is_ok()); // Verify file exists and has multiple entries @@ -367,7 +396,13 @@ mod main_tests { // Only run if in a git repo if Repository::discover("./").is_ok() { // Create directory structure first - let result = run_commit_saver(obsidian_root.clone(), &commit_path, date_template, &[]); + let result = run_commit_saver( + obsidian_root.clone(), + &commit_path, + date_template, + "%H:%M:%S", + &[], + ); assert!(result.is_ok()); // Now make the directory read-only to trigger write errors on second run @@ -457,8 +492,13 @@ mod main_tests { if Repository::discover("./").is_ok() { // Run three times - should be idempotent for _ in 0..3 { - let result = - run_commit_saver(obsidian_root.clone(), &commit_path, date_template, &[]); + let result = run_commit_saver( + obsidian_root.clone(), + &commit_path, + date_template, + "%H:%M:%S", + &[], + ); assert!(result.is_ok()); } } @@ -486,7 +526,13 @@ mod main_tests { let date_template = "%Y/%m-%B/%d/%F.md"; if Repository::discover("./").is_ok() { - let result = run_commit_saver(complex_root.clone(), &commit_path, date_template, &[]); + let result = run_commit_saver( + complex_root.clone(), + &commit_path, + date_template, + "%H:%M:%S", + &[], + ); assert!(result.is_ok()); // Verify deep directory structure was created @@ -515,6 +561,7 @@ mod main_tests { obsidian_root.clone(), &commit_path, date_template, + "%H:%M:%S", &excluded, ); diff --git a/src/vim_commit.rs b/src/vim_commit.rs index b36d979..f596177 100644 --- a/src/vim_commit.rs +++ b/src/vim_commit.rs @@ -279,11 +279,11 @@ impl CommitSaver { /// This is a private helper method called by [`append_entry_to_diary()`](Self::append_entry_to_diary). /// The commit message has already been formatted with escaped pipes and `
` separators /// during struct initialization. - fn prepare_commit_entry_as_string(&mut self, path: &Path) -> String { + fn prepare_commit_entry_as_string(&mut self, path: &Path, time_format: &str) -> String { format!( "| {:} | {:} | {:} | {:} | {:} | {:} |\n", path.display(), - self.commit_datetime.format("%H:%M:%S"), + self.commit_datetime.format(time_format), self.commit_msg, self.repository_url, self.commit_branch_name, @@ -496,12 +496,16 @@ impl CommitSaver { /// Err(e) => eprintln!("Failed to log commit: {}", e), /// } /// ``` - pub fn append_entry_to_diary(&mut self, wiki: &PathBuf) -> Result<(), Box> { + pub fn append_entry_to_diary( + &mut self, + wiki: &PathBuf, + time_format: &str, + ) -> Result<(), Box> { info!("[CommitSaver::append_entry_to_diary()]: Getting current directory."); let path = env::current_dir()?; info!("[CommitSaver::append_entry_to_diary()]: Preparing the commit_entry_as_string."); - let new_commit_str = self.prepare_commit_entry_as_string(&path); + let new_commit_str = self.prepare_commit_entry_as_string(&path, time_format); debug!("[CommitSaver::append_entry_to_diary()]: Commit String: {new_commit_str:}"); debug!( @@ -949,7 +953,7 @@ mod commit_saver_tests { let mut commit_saver = create_test_commit_saver(); let test_path = PathBuf::from("/test/path"); - let result = commit_saver.prepare_commit_entry_as_string(&test_path); + let result = commit_saver.prepare_commit_entry_as_string(&test_path, "%H:%M:%S"); assert!(result.contains("/test/path")); assert!(result.contains("10:30:00")); @@ -960,6 +964,26 @@ mod commit_saver_tests { assert!(result.ends_with("|\n")); } + #[test] + fn test_prepare_commit_entry_honours_the_configured_time_format() { + // `[templates] commit_datetime` was read from the config, required on + // pain of a fatal error, and then never consumed: the TIME column was + // hardcoded. The key now means what it says. + let mut commit_saver = create_test_commit_saver(); + let test_path = PathBuf::from("/test/path"); + + let result = commit_saver.prepare_commit_entry_as_string(&test_path, "%H%Mh"); + + assert!( + result.contains("1030h"), + "the configured format must reach the row: {result}" + ); + assert!( + !result.contains("10:30:00"), + "the hardcoded format must no longer win: {result}" + ); + } + #[test] fn test_prepare_commit_entry_with_pipe_escaping() { let mut commit_saver = CommitSaver { @@ -971,7 +995,7 @@ mod commit_saver_tests { }; let test_path = PathBuf::from("/test/path"); - let result = commit_saver.prepare_commit_entry_as_string(&test_path); + let result = commit_saver.prepare_commit_entry_as_string(&test_path, "%H:%M:%S"); // The commit message should have pipes escaped assert!(result.contains("Test | commit | with | pipes")); @@ -997,7 +1021,7 @@ mod commit_saver_tests { // Create the file first File::create(&file_path)?; - let result = commit_saver.append_entry_to_diary(&file_path); + let result = commit_saver.append_entry_to_diary(&file_path, "%H:%M:%S"); assert!(result.is_ok()); @@ -1014,7 +1038,7 @@ mod commit_saver_tests { let mut commit_saver = create_test_commit_saver(); let non_existent_path = PathBuf::from("/non/existent/file.md"); - let result = commit_saver.append_entry_to_diary(&non_existent_path); + let result = commit_saver.append_entry_to_diary(&non_existent_path, "%H:%M:%S"); assert!(result.is_err()); } @@ -1271,7 +1295,7 @@ mod commit_saver_tests { // append_entry_to_diary opens with append mode; file must exist. // Parent doesn't exist, so open should fail. - let result = commit_saver.append_entry_to_diary(&missing_parent_path); + let result = commit_saver.append_entry_to_diary(&missing_parent_path, "%H:%M:%S"); assert!( result.is_err(), From a8a8bd58c00651023ae2bf2bd749037aa75deec0 Mon Sep 17 00:00:00 2001 From: Chess Seventh Date: Thu, 30 Jul 2026 23:48:22 +0200 Subject: [PATCH 06/21] test(config): cover key skew at the binary boundary The section fix had a real-binary test asserting stderr; the key work had no twin, which is how a silent key stayed silent. - assert an unrecognised key is named on stderr by the actual binary, and that a fully known config still says nothing - assert a missing required key names the config file, the [section] key and the typo that explains it - key the silent-case positive control on the repo-discovery failure rather than on the word panicked, which any config fault would satisfy Co-Authored-By: Vulcan --- tests/integration_tests.rs | 109 +++++++++++++++++++++++++++++++++++++ 1 file changed, 109 insertions(+) diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index d2a9716..573ad3c 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -96,6 +96,105 @@ fn known_config_sections_are_reported_silently() { ); } +/// An unrecognised config key must be reported on stderr too. +/// +/// The key twin of `unknown_config_section_is_reported_on_stderr`. A misspelt +/// key used to apply nothing and say nothing at all: the binary only ever asks +/// for the keys it knows, so one nobody asks for is invisible. +#[test] +fn unknown_config_key_is_reported_on_stderr() { + let dir = tempfile::tempdir().unwrap(); + let ini = dir.path().join("rusty-commit-saver.ini"); + fs::write(&ini, ini_with_extra_key(dir.path(), "commit_datetimes=%T")).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 keys"), + "the unknown key was not reported; stderr was: {stderr}" + ); + assert!( + stderr.contains("[templates] commit_datetimes"), + "the report did not name the key and its section; stderr was: {stderr}" + ); +} + +/// The counterpart: a config whose keys are all known reports nothing. +#[test] +fn known_config_keys_are_reported_silently() { + let dir = tempfile::tempdir().unwrap(); + let ini = dir.path().join("rusty-commit-saver.ini"); + fs::write(&ini, ini_with_extra_key(dir.path(), "")).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: the run must have read the config and got past it, + // otherwise "reported nothing" would pass vacuously. There is no git repo + // in the temp cwd, so it dies in repo discovery - after config loading, + // and with a message no config fault produces. + assert!( + stderr.contains("failed to build CommitSaver"), + "the run did not get past config loading; stderr was: {stderr}" + ); + assert!( + !stderr.contains("ignoring unrecognised config keys"), + "a fully known config must not report anything; stderr was: {stderr}" + ); +} + +/// A missing required key must name the file to edit and the key itself. +/// +/// The message is the whole deliverable here: the old one named a key and a +/// source line, so diagnosing it meant reading the source. +#[test] +fn missing_required_key_names_the_file_and_the_key() { + let dir = tempfile::tempdir().unwrap(); + let ini = dir.path().join("rusty-commit-saver.ini"); + // The rename in full: the new key is present, the old one is gone. + fs::write( + &ini, + format!( + "[obsidian]\nroot_path_dir={}\ncommit_paths=Commits\n\ + [templates]\ncommit_date_path=%Y-%m-%d.md\ncommit_datetime=%H:%M\n", + dir.path().join("vault").display() + ), + ) + .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(&ini.display().to_string()), + "the message must name the config file to edit; stderr was: {stderr}" + ); + assert!( + stderr.contains("missing required key 'commit_path' in section [obsidian]"), + "the message must name the key and its section; stderr was: {stderr}" + ); + assert!( + stderr.contains("unrecognised in [obsidian]: commit_paths"), + "the message must name the typo that explains the absence; 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 @@ -108,3 +207,13 @@ fn ini_with_section(dir: &std::path::Path, extra: &str) -> String { dir.join("vault").display() ) } + +/// The same minimal config, with one extra line appended to `[templates]` - +/// an unrecognised key for the unknown case, empty for the known one. +fn ini_with_extra_key(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}\n", + dir.join("vault").display() + ) +} From 8844b6a619857b936e9027448232d703082a114d Mon Sep 17 00:00:00 2001 From: Chess Seventh Date: Thu, 30 Jul 2026 23:49:53 +0200 Subject: [PATCH 07/21] test(config): add the real-commit hook gate A unit test cannot show what this tool actually does wrong: every failure it has shipped was a config fault at the hook boundary, where the binary runs with no RUST_LOG and the only thing a human sees is stderr. - drive a real git commit through a real post-commit hook against each config fault, in a throwaway repo and vault with an explicit hooksPath - report the commit exit status, the stderr, and what was journalled, which is exactly the evidence the last two fixes turned on Co-Authored-By: Vulcan --- tests/hook-gate.sh | 95 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100755 tests/hook-gate.sh diff --git a/tests/hook-gate.sh b/tests/hook-gate.sh new file mode 100755 index 0000000..8c91672 --- /dev/null +++ b/tests/hook-gate.sh @@ -0,0 +1,95 @@ +#!/usr/bin/env bash +# The gate the Rust tests cannot be: a REAL git commit through a REAL +# post-commit hook running this binary. +# +# The failures this repo has actually shipped were config faults at the hook +# boundary, where the binary runs with no RUST_LOG and nobody reads the log - +# so the thing worth asserting is what a human sees on stderr, and whether the +# diary entry appeared. Everything runs in a throwaway repo with a throwaway +# vault and an explicit core.hooksPath, so a machine-wide hooks directory +# cannot interfere and nothing touches the real vault. +# +# Usage: +# tests/hook-gate.sh [config-kind] [path-to-binary] +# +# config-kind: good | unknown-key | blank-key | missing-key | unknown-section +# (default: good) +# binary: default target/debug/rusty-commit-saver +# +# Prints the git commit exit status, the hook's stderr, and what was +# journalled. Read it - the point is the human-visible output, so this +# reports rather than asserts. +set -uo pipefail + +kind=${1:-good} +bin=${2:-target/debug/rusty-commit-saver} + +[ -x "$bin" ] || { + echo "hook-gate: no binary at $bin - run: cargo build" >&2 + exit 2 +} +bin=$(realpath "$bin") + +work=$(mktemp -d) +trap 'rm -rf "$work"' EXIT + +vault="$work/vault" +cfg="$work/rusty-commit-saver.ini" +hooks="$work/hooks" +repo="$work/repo" +mkdir -p "$vault" "$hooks" "$repo" + +{ + echo '[obsidian]' + echo "root_path_dir = $vault" + case "$kind" in + missing-key) : ;; # commit_path absent entirely + blank-key) echo 'commit_path =' ;; # present but empty + *) echo 'commit_path = Diaries/Commits' ;; + esac + echo + echo '[templates]' + echo 'commit_date_path = %Y/%m-%B/%F.md' + echo 'commit_datetime = %H:%M:%S' + case "$kind" in + unknown-key) echo 'commit_datetimes = %H:%M' ;; + esac + echo + echo '[exclude]' + echo 'repos = some-other-repo' + case "$kind" in + unknown-section) + echo + echo '[future_release]' + echo 'key = value' + ;; + esac +} >"$cfg" + +printf '#!/bin/sh\nexec "%s"\n' "$bin" >"$hooks/post-commit" +chmod +x "$hooks/post-commit" + +git init -q "$repo" +git -C "$repo" config user.email gate@example.invalid +git -C "$repo" config user.name 'hook gate' +git -C "$repo" config core.hooksPath "$hooks" +echo hello >"$repo/file.txt" +git -C "$repo" add file.txt + +RUSTY_COMMIT_SAVER_CONFIG="$cfg" \ + git -C "$repo" commit -q -m 'test: drive the real post-commit hook' \ + >"$work/stdout" 2>"$work/stderr" +rc=$? + +echo "config kind : $kind" +echo "binary : $bin" +echo "git commit exit: $rc" +echo "commit created : $(git -C "$repo" rev-parse --short HEAD 2>/dev/null || echo NONE)" +echo '--- hook stderr ---' +cat "$work/stderr" +echo '--- journalled ---' +if find "$vault" -type f | grep -q .; then + find "$vault" -type f -printf '%P\n' +else + echo '(nothing written)' +fi From 146ea55e4d2a968385cbe58d9d71e1b3596e35e4 Mon Sep 17 00:00:00 2001 From: Chess Seventh Date: Thu, 30 Jul 2026 23:50:41 +0200 Subject: [PATCH 08/21] docs(readme): document how unrecognised ini keys are handled - state the key rules beside the section rules they mirror: unknown keys are named on stderr, required keys stay fatal, a blank value counts as missing - show the fatal message, so the shape of it is the documented contract - say plainly that no config fault can cost a commit, since the tool runs post-commit and git ignores that exit status - point at the hook gate script for checking the behaviour by hand Co-Authored-By: Vulcan --- README.md | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/README.md b/README.md index 834e7af..c1875fe 100644 --- a/README.md +++ b/README.md @@ -139,6 +139,42 @@ 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. +Keys work the same way, for the same reason: + +- A key this binary does not understand is **ignored and named on stderr** + (`ignoring unrecognised config keys [templates] commit_datetimes`). It used to + be ignored in complete silence, so a typo applied nothing and said nothing. +- The five keys above are **required**, and so is a non-empty value for each — + `commit_path =` counts as missing. Without them there is no destination to + write to, and a hook that quietly journals nothing looks exactly like a quiet + day, so this one stays fatal. +- The fatal message names the config file, the key and its section, plus any + unrecognised key in that same section, since a misspelt `commit_paths` is the + usual reason `commit_path` is missing: + + ```text + rusty-commit-saver: /home/you/.config/rusty-commit-saver/rusty-commit-saver.ini: + missing required key 'commit_path' in section [obsidian]; + unrecognised in [obsidian]: commit_paths + ``` + +None of this can cost you a commit: the tool runs as a **post-commit** hook, and +git ignores that hook's exit status. A config fault costs you the diary entry +and prints on stderr; the commit itself always stands. + +### Checking hook behaviour by hand + +`tests/hook-gate.sh` drives a real commit through a real post-commit hook, in a +throwaway repo and vault, and prints what a human would see: + +```bash +cargo build +./tests/hook-gate.sh good # journals, says nothing +./tests/hook-gate.sh unknown-key # journals, names the key on stderr +./tests/hook-gate.sh missing-key # journals nothing, names file + key +./tests/hook-gate.sh unknown-section # journals, names the section +``` + --- ## Roadmap & Improvements 📈 From a7ae31e308822312fb95069e8a6c78e23a058104 Mon Sep 17 00:00:00 2001 From: Chess Seventh Date: Thu, 30 Jul 2026 23:51:26 +0200 Subject: [PATCH 09/21] docs(readme): correct how an excluded repo is matched The prose still described the behaviour from before 4.17.0: matching on the working-directory name. It has matched the canonical name from the origin remote since, which is what makes one entry cover every worktree. Co-Authored-By: Vulcan --- README.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index c1875fe..15ab284 100644 --- a/README.md +++ b/README.md @@ -126,9 +126,11 @@ repos = claude-src ``` The `[exclude]` section is optional. Each entry is matched, case-sensitively, -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. +against the committing repository's **canonical name** — taken from its `origin` +remote URL (`…/claude-src.git` → `claude-src`), falling back to the +working-directory name for a repo with no usable `origin`. Because the origin is +the same from every checkout, one entry covers the main clone and every git +worktree of that repo, from any subdirectory. `[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 From 49d689b4423de8b7048eb2eb66c4092725688eec Mon Sep 17 00:00:00 2001 From: Chess Seventh Date: Thu, 30 Jul 2026 23:55:33 +0200 Subject: [PATCH 10/21] test(config): cover the paths the key work left untested - pin both empty cases of the per-section key lookup: an unknown section, and a known section the config does not carry - exercise get_ini_file() through the env var, which nothing covered once set_all() stopped calling it Co-Authored-By: Vulcan --- src/config.rs | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/src/config.rs b/src/config.rs index c6204f0..51dc62c 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1642,6 +1642,36 @@ mod global_vars_tests { ); } + #[test] + fn test_unrecognised_keys_in_an_unknown_section_is_empty() { + let mut config = Ini::new(); + config.set("future_release", "whatever", Some("value".to_string())); + + let global_vars = GlobalVars::new(); + global_vars.config.set(config).unwrap(); + + assert!( + global_vars + .unrecognised_keys_in("future_release") + .is_empty(), + "the binary cannot know what an unknown section should contain" + ); + } + + #[test] + fn test_unrecognised_keys_in_a_section_the_config_lacks_is_empty() { + 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(); + + assert!( + global_vars.unrecognised_keys_in("exclude").is_empty(), + "a section that is not in the config has no keys to report" + ); + } + #[test] fn test_unrecognised_keys_are_sorted_and_name_every_section() { let mut config = Ini::new(); @@ -2803,6 +2833,34 @@ commit_datetime = %Y-%m-%d %H:%M:%S fs::set_permissions(&file_path, fs::Permissions::from_mode(0o644)).unwrap(); } + #[test] + fn test_get_ini_file_reads_the_configured_path() { + use std::io::Write; + use tempfile::NamedTempFile; + + let mut temp_file = NamedTempFile::new().unwrap(); + writeln!(temp_file, "[obsidian]").unwrap(); + writeln!(temp_file, "root_path_dir=/tmp/test").unwrap(); + writeln!(temp_file, "commit_path=commits").unwrap(); + writeln!(temp_file, "[templates]").unwrap(); + writeln!(temp_file, "commit_date_path=%Y-%m-%d.md").unwrap(); + writeln!(temp_file, "commit_datetime=%H:%M:%S").unwrap(); + temp_file.flush().unwrap(); + + std::env::set_var("RUSTY_COMMIT_SAVER_CONFIG", temp_file.path()); + + // The convenience wrapper must resolve the path the same way + // set_all() does, now that set_all() resolves it itself. + let config = get_ini_file(); + + std::env::remove_var("RUSTY_COMMIT_SAVER_CONFIG"); + + assert_eq!( + config.get("obsidian", "commit_path"), + Some("commits".to_string()) + ); + } + #[test] fn test_parse_exclude_repos_basic() { assert_eq!( From 545748de85e0321448bd83325426d11dcf48d736 Mon Sep 17 00:00:00 2001 From: Chess Seventh Date: Thu, 30 Jul 2026 23:55:34 +0200 Subject: [PATCH 11/21] docs(feature): record the two faults the root-cause pass added - a blank value defeated the presence check: commit_path= exited 0 and journalled into the vault root - commit_datetime was required and never read, so a typo could abort a run over a value nothing consumed; wiring it up was Franci call - add both to the acceptance scenarios Co-Authored-By: Vulcan --- docs/feature/L76-config-key-skew/brief.md | 25 ++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/docs/feature/L76-config-key-skew/brief.md b/docs/feature/L76-config-key-skew/brief.md index 0fe4e66..8c37638 100644 --- a/docs/feature/L76-config-key-skew/brief.md +++ b/docs/feature/L76-config-key-skew/brief.md @@ -64,6 +64,21 @@ Given that, the policy is: Decided by Franci, 2026-07-30, over "never fatal, always degrade" and "fatal only when there is no destination". +Two findings from the root-cause pass widened that decision, both reproduced +before acting on them: + +- **A blank value defeats a presence check.** `commit_path =` satisfied + `Option::is_some`, exited 0, and journalled into the vault *root* instead of + the configured folder - a wrong-location write with no error at all. An + empty or whitespace value therefore counts as missing. A test that blessed + the same hole for `root_path_dir` (empty vault root resolving to `/`) is + withdrawn deliberately rather than worked around. +- **`commit_datetime` was required and never read.** The TIME column was + hardcoded to `%H:%M:%S`, so a typo in that key could abort a run over a + value nothing consumed - fatal-and-ignored, which no policy can defend. + Franci's call: wire it up, so the key means what it says. The live config + already carries `%H:%M:%S`, so no diary output changes. + ## Fix - New `KNOWN_KEYS` table beside `KNOWN_SECTIONS` (`src/config.rs`), listing what @@ -76,7 +91,11 @@ only when there is no destination". split out for that; `get_ini_file()` and `retrieve_config_file_path()` keep their signatures and behaviour. - New `require_key()` replaces the four `.expect()` calls with one fatal path - whose message names the file and the key. + whose message names the file, the key, and the unrecognised keys of that same + section; it rejects a blank value as missing. +- `[templates] commit_datetime` is threaded from `main()` through + `run_commit_saver()` and `append_entry_to_diary()` to the row builder in + `src/vim_commit.rs`, which had the format hardcoded. ## Gate (acceptance scenarios) @@ -88,6 +107,10 @@ only when there is no destination". 3. An unrecognised **section** -> still warns and continues (4.17.3 behaviour must not regress). 4. A good config -> no warning at all on stderr, entry journalled as before. +5. A required key present but **blank** -> treated as missing, same message, + nothing journalled. Previously: exit 0 and a diary in the wrong directory. +6. The configured time format reaches the diary row, instead of the hardcoded + one. Verified by unit tests in `src/config.rs`, plus - and this is the point of the lane - the real-hook gate: a genuine `git commit` in a throwaway repo whose From 1b5ff82400228736c84df63d88afb4a59662b15b Mon Sep 17 00:00:00 2001 From: Chess Seventh Date: Fri, 31 Jul 2026 00:08:26 +0200 Subject: [PATCH 12/21] docs(readme): correct which hook stage runs the saver Usage said the pre-commit hook invokes the saver. It has always been the post-commit stage, and the difference matters: a post-commit hook cannot affect the commit, which is what makes a fatal config error safe. Co-Authored-By: Vulcan --- README.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 15ab284..89b70b9 100644 --- a/README.md +++ b/README.md @@ -79,10 +79,11 @@ and appends it to a dated diary entry in your Wiki directory. ## Usage 🛞 -Simply commit as usual. The pre-commit hook will: +Simply commit as usual. The hooks will: -1. Run linters (`clippy`, `rustfmt`, etc.) inside the Nix shell -2. Invoke Rusty Commit Saver to log the commit +1. Run linters (`clippy`, `rustfmt`, etc.) inside the Nix shell — **pre-commit** +2. Invoke Rusty Commit Saver to log the commit — **post-commit**, once the + commit exists, which is why nothing this tool does can cost you a commit If you prefer manual invocation: From 5ec4fa51758d752602898d32fdbdead3b76a9fda Mon Sep 17 00:00:00 2001 From: Chess Seventh Date: Fri, 31 Jul 2026 00:09:10 +0200 Subject: [PATCH 13/21] docs(readme): fix the required-key count and two stale lines - four keys are required, not five: [exclude] repos is optional, like its own section says two paragraphs above - the example comment still described exclusion by working-directory name, which the prose beneath it had already corrected - list the blank-value case the hook gate supports Co-Authored-By: Vulcan --- README.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 89b70b9..f649360 100644 --- a/README.md +++ b/README.md @@ -120,7 +120,7 @@ commit_path = Diaries/Commits commit_date_path = %Y/%m-%B/%F.md commit_datetime = %H:%M:%S -# Optional: repositories to skip, by working-directory name (comma-separated). +# Optional: repositories to skip, by canonical repo name (comma-separated). # A commit made in one of these repos writes nothing to the diary. [exclude] repos = claude-src @@ -147,10 +147,11 @@ Keys work the same way, for the same reason: - A key this binary does not understand is **ignored and named on stderr** (`ignoring unrecognised config keys [templates] commit_datetimes`). It used to be ignored in complete silence, so a typo applied nothing and said nothing. -- The five keys above are **required**, and so is a non-empty value for each — - `commit_path =` counts as missing. Without them there is no destination to - write to, and a hook that quietly journals nothing looks exactly like a quiet - day, so this one stays fatal. +- The four keys in `[obsidian]` and `[templates]` are **required**, and so is a + non-empty value for each — `commit_path =` counts as missing. Without them + there is no destination to write to, and a hook that quietly journals nothing + looks exactly like a quiet day, so this one stays fatal. (`[exclude] repos` is + optional, like its section.) - The fatal message names the config file, the key and its section, plus any unrecognised key in that same section, since a misspelt `commit_paths` is the usual reason `commit_path` is missing: @@ -175,6 +176,7 @@ cargo build ./tests/hook-gate.sh good # journals, says nothing ./tests/hook-gate.sh unknown-key # journals, names the key on stderr ./tests/hook-gate.sh missing-key # journals nothing, names file + key +./tests/hook-gate.sh blank-key # same, for a key with an empty value ./tests/hook-gate.sh unknown-section # journals, names the section ``` From df42520c3cdfdcbb8eb2f05e0e5486d472eae3ca Mon Sep 17 00:00:00 2001 From: Chess Seventh Date: Fri, 31 Jul 2026 00:16:08 +0200 Subject: [PATCH 14/21] fix(config): reject a time format chrono cannot render Honouring commit_datetime made a bad value reachable, and it surfaced from inside the writer as "a formatting trait implementation returned an error", naming neither the file nor the key - after an empty diary file had already been created. commit_date_path had the same hole before this lane. - check both format keys where the rest of the config is checked, with the same message shape: file, [section] key, and the value that fails - the run now stops before writing anything at all Co-Authored-By: Vulcan --- src/config.rs | 95 +++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 93 insertions(+), 2 deletions(-) diff --git a/src/config.rs b/src/config.rs index 51dc62c..c3b33bc 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,3 +1,4 @@ +use chrono::DateTime; use log::{error, info, warn}; use std::{ @@ -711,6 +712,36 @@ impl GlobalVars { /// # Panics /// /// Panics if the key is absent, or its value is empty or whitespace. + /// Reads a required key whose value must be a `chrono` format string. + /// + /// A format `chrono` cannot render is config skew like any other, but it + /// used to surface from deep inside the writer as `a formatting trait + /// implementation returned an error when the underlying stream did not`, + /// naming neither the file nor the key - and only after an empty diary + /// file had already been created. Checking it where the rest of the config + /// is checked keeps the message the same shape as every other config + /// fault, and stops the run before it writes anything. + /// + /// # Panics + /// + /// Panics if the key is missing (see [`require_key()`](Self::require_key)), + /// or if `chrono` cannot render its value. + fn require_time_format(&self, section: &str, key: &str) -> String { + let format = self.require_key(section, key); + + if is_renderable_time_format(&format) { + return format; + } + + let file = self.config_file_label(); + error!( + "[GlobalVars::require_time_format()] {file}: key '{key}' in section [{section}] is not a time format chrono can render: '{format}'" + ); + panic!( + "rusty-commit-saver: {file}: key '{key}' in section [{section}] is not a time format chrono can render: '{format}'" + ) + } + fn require_key(&self, section: &str, key: &str) -> String { let value = self .get_key_from_section_from_ini(section, key) @@ -886,7 +917,7 @@ impl GlobalVars { /// ``` fn set_templates_datetime(&self, section: &str) { info!("[GlobalVars::set_templates_datetime()]: Setting the templates_datetime."); - let key = self.require_key(section, "commit_datetime"); + let key = self.require_time_format(section, "commit_datetime"); self.template_commit_datetime .set(key) @@ -967,7 +998,7 @@ impl GlobalVars { info!( "[GlobalVars::set_templates_commit_date_path()]: Setting the template_commit_date_path." ); - let key = self.require_key(section, "commit_date_path"); + let key = self.require_time_format(section, "commit_date_path"); self.template_commit_date_path .set(key) @@ -1455,6 +1486,20 @@ fn set_proper_home_dir(cfg_str: &str) -> String { cfg_str.replace('~', &home_dir) } +/// Whether `chrono` can render this format string. +/// +/// `DateTime::format()` defers the work, and `to_string()` turns an invalid +/// specifier into a panic; writing into a `String` returns the error instead, +/// which is what makes the format checkable at all. +fn is_renderable_time_format(format: &str) -> bool { + use std::fmt::Write; + + let probe = DateTime::from_timestamp(0, 0).expect("the epoch is a valid timestamp"); + let mut rendered = String::new(); + + write!(rendered, "{}", probe.format(format)).is_ok() +} + /// Parses a comma-separated list of repository names into a clean vector. /// /// Each entry is trimmed of surrounding whitespace and empty entries are @@ -2302,6 +2347,52 @@ mod global_vars_tests { ); } + #[test] + fn test_require_time_format_rejects_what_chrono_cannot_render() { + let mut config = Ini::new(); + config.set("templates", "commit_datetime", Some("%Q".to_string())); + + let global_vars = GlobalVars::new(); + global_vars.config.set(config).unwrap(); + global_vars + .config_path + .set("/tmp/some/rusty-commit-saver.ini".to_string()) + .unwrap(); + + let result = panic::catch_unwind(AssertUnwindSafe(|| { + global_vars.set_templates_datetime("templates") + })); + + let panic_info = result.expect_err("a format chrono cannot render must be fatal"); + let msg = panic_info + .downcast_ref::() + .expect("panic message should be a formatted String"); + + assert!( + msg.contains("/tmp/some/rusty-commit-saver.ini"), + "the message must name the file to edit: {msg}" + ); + assert!( + msg.contains("key 'commit_datetime' in section [templates]"), + "the message must name the key and its section: {msg}" + ); + assert!( + msg.contains("'%Q'"), + "the message must quote the value that cannot be rendered: {msg}" + ); + } + + #[test] + fn test_is_renderable_time_format_accepts_the_shipped_defaults() { + assert!(is_renderable_time_format("%H:%M:%S")); + assert!(is_renderable_time_format("%Y/%m-%B/%F.md")); + assert!( + is_renderable_time_format("Commits"), + "a format with no specifier at all is still renderable" + ); + assert!(!is_renderable_time_format("%Q")); + } + #[test] fn test_require_key_says_which_config_when_none_was_read() { let mut config = Ini::new(); From a19d1b93d1d24b5e547a6d192f846ea4f9a2c22e Mon Sep 17 00:00:00 2001 From: Chess Seventh Date: Fri, 31 Jul 2026 00:16:33 +0200 Subject: [PATCH 15/21] docs(config): note that a blank or unrenderable value is fatal too The four setters still documented only a missing key as fatal, while their shared reader also rejects a blank value and an unrenderable format. Co-Authored-By: Vulcan --- src/config.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/config.rs b/src/config.rs index c3b33bc..38702f0 100644 --- a/src/config.rs +++ b/src/config.rs @@ -906,7 +906,8 @@ impl GlobalVars { /// # Panics /// /// Panics if: - /// - The `commit_datetime` key is missing from the INI section + /// - The `commit_datetime` key is missing, or its value is blank, or it is + /// not a format `chrono` can render /// - The `OnceCell` has already been set (called multiple times) /// /// # Expected INI Key @@ -985,7 +986,8 @@ impl GlobalVars { /// # Panics /// /// Panics if: - /// - The `commit_date_path` key is missing from the INI section + /// - The `commit_date_path` key is missing, or its value is blank, or it is + /// not a format `chrono` can render /// - The `OnceCell` has already been set (called multiple times) /// /// # Expected INI Key @@ -1022,7 +1024,7 @@ impl GlobalVars { /// # Panics /// /// Panics if: - /// - The `commit_path` key is missing from the INI section + /// - The `commit_path` key is missing, or its value is blank /// - Home directory cannot be determined (when `~` is used) /// - The `OnceCell` has already been set /// @@ -1081,7 +1083,7 @@ impl GlobalVars { /// # Panics /// /// Panics if: - /// - The `root_path_dir` key is missing from the INI section + /// - The `root_path_dir` key is missing, or its value is blank /// - Home directory cannot be determined (when `~` is used) /// - The `OnceCell` has already been set /// From 05173bfa59753d499919eae572d8a6faadc557ad Mon Sep 17 00:00:00 2001 From: Chess Seventh Date: Fri, 31 Jul 2026 00:27:26 +0200 Subject: [PATCH 16/21] fix(config): restore the doc comment the format check displaced The new format check landed between require_key and its documentation, so the most safety-critical function in the config path ended up undocumented while the new one inherited two Panics sections, the first describing the wrong function. - move the function below require_key, where its own docs belong - cover the second call site: reverting the date-path guard alone left the whole suite green - say format rather than time format, since one of the two keys is a path template Co-Authored-By: Vulcan --- src/config.rs | 83 ++++++++++++++++++++++++++++++++++----------------- 1 file changed, 56 insertions(+), 27 deletions(-) diff --git a/src/config.rs b/src/config.rs index 38702f0..c838c5e 100644 --- a/src/config.rs +++ b/src/config.rs @@ -712,6 +712,31 @@ impl GlobalVars { /// # Panics /// /// Panics if the key is absent, or its value is empty or whitespace. + fn require_key(&self, section: &str, key: &str) -> String { + let value = self + .get_key_from_section_from_ini(section, key) + .filter(|value| !value.trim().is_empty()); + + if let Some(value) = value { + return value; + } + + let file = self.config_file_label(); + let typos = self.unrecognised_keys_in(section); + let hint = if typos.is_empty() { + String::new() + } else { + format!("; unrecognised in [{section}]: {}", typos.join(", ")) + }; + + error!( + "[GlobalVars::require_key()] {file}: missing required key '{key}' in section [{section}]{hint}" + ); + panic!( + "rusty-commit-saver: {file}: missing required key '{key}' in section [{section}]{hint}" + ) + } + /// Reads a required key whose value must be a `chrono` format string. /// /// A format `chrono` cannot render is config skew like any other, but it @@ -735,35 +760,10 @@ impl GlobalVars { let file = self.config_file_label(); error!( - "[GlobalVars::require_time_format()] {file}: key '{key}' in section [{section}] is not a time format chrono can render: '{format}'" + "[GlobalVars::require_time_format()] {file}: key '{key}' in section [{section}] is not a format chrono can render: '{format}'" ); panic!( - "rusty-commit-saver: {file}: key '{key}' in section [{section}] is not a time format chrono can render: '{format}'" - ) - } - - fn require_key(&self, section: &str, key: &str) -> String { - let value = self - .get_key_from_section_from_ini(section, key) - .filter(|value| !value.trim().is_empty()); - - if let Some(value) = value { - return value; - } - - let file = self.config_file_label(); - let typos = self.unrecognised_keys_in(section); - let hint = if typos.is_empty() { - String::new() - } else { - format!("; unrecognised in [{section}]: {}", typos.join(", ")) - }; - - error!( - "[GlobalVars::require_key()] {file}: missing required key '{key}' in section [{section}]{hint}" - ); - panic!( - "rusty-commit-saver: {file}: missing required key '{key}' in section [{section}]{hint}" + "rusty-commit-saver: {file}: key '{key}' in section [{section}] is not a format chrono can render: '{format}'" ) } @@ -2384,6 +2384,35 @@ mod global_vars_tests { ); } + #[test] + fn test_require_time_format_guards_the_date_path_too() { + // Both format keys go through the same check. Without this, reverting + // the date-path call site alone would leave every test green. + let mut config = Ini::new(); + config.set( + "templates", + "commit_date_path", + Some("%Y/%Q.md".to_string()), + ); + + let global_vars = GlobalVars::new(); + global_vars.config.set(config).unwrap(); + + let result = panic::catch_unwind(AssertUnwindSafe(|| { + global_vars.set_templates_commit_date_path("templates") + })); + + let panic_info = result.expect_err("a format chrono cannot render must be fatal"); + let msg = panic_info + .downcast_ref::() + .expect("panic message should be a formatted String"); + + assert!( + msg.contains("key 'commit_date_path' in section [templates]"), + "the message must name the key and its section: {msg}" + ); + } + #[test] fn test_is_renderable_time_format_accepts_the_shipped_defaults() { assert!(is_renderable_time_format("%H:%M:%S")); From e7782896bc66624a48efe8af55c8da6d305d30db Mon Sep 17 00:00:00 2001 From: Chess Seventh Date: Fri, 31 Jul 2026 00:28:48 +0200 Subject: [PATCH 17/21] test(config): drive the unrenderable format through the real hook The lane gate is a real commit through a real hook; the format check had only unit tests behind it. - add a bad-format case to the hook gate script - assert at the binary boundary that the message names file, key and value, and that the vault is never created Co-Authored-By: Vulcan --- tests/hook-gate.sh | 9 +++++--- tests/integration_tests.rs | 45 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/tests/hook-gate.sh b/tests/hook-gate.sh index 8c91672..aafbb57 100755 --- a/tests/hook-gate.sh +++ b/tests/hook-gate.sh @@ -12,8 +12,8 @@ # Usage: # tests/hook-gate.sh [config-kind] [path-to-binary] # -# config-kind: good | unknown-key | blank-key | missing-key | unknown-section -# (default: good) +# config-kind: good | unknown-key | blank-key | missing-key | bad-format | +# unknown-section (default: good) # binary: default target/debug/rusty-commit-saver # # Prints the git commit exit status, the hook's stderr, and what was @@ -50,7 +50,10 @@ mkdir -p "$vault" "$hooks" "$repo" echo echo '[templates]' echo 'commit_date_path = %Y/%m-%B/%F.md' - echo 'commit_datetime = %H:%M:%S' + case "$kind" in + bad-format) echo 'commit_datetime = %Q' ;; # not a specifier chrono knows + *) echo 'commit_datetime = %H:%M:%S' ;; + esac case "$kind" in unknown-key) echo 'commit_datetimes = %H:%M' ;; esac diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index 573ad3c..c9eabf9 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -195,6 +195,51 @@ fn missing_required_key_names_the_file_and_the_key() { ); } +/// A format `chrono` cannot render must fail the same way, and before writing. +/// +/// This one used to surface from inside the writer as `a formatting trait +/// implementation returned an error`, naming neither file nor key - and only +/// after an empty diary file had been created. +#[test] +fn unrenderable_time_format_names_the_file_and_the_key() { + let dir = tempfile::tempdir().unwrap(); + let ini = dir.path().join("rusty-commit-saver.ini"); + fs::write( + &ini, + format!( + "[obsidian]\nroot_path_dir={}\ncommit_path=Commits\n\ + [templates]\ncommit_date_path=%Y-%m-%d.md\ncommit_datetime=%Q\n", + dir.path().join("vault").display() + ), + ) + .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(&ini.display().to_string()), + "the message must name the config file to edit; stderr was: {stderr}" + ); + assert!( + stderr.contains("key 'commit_datetime' in section [templates]"), + "the message must name the key and its section; stderr was: {stderr}" + ); + assert!( + stderr.contains("'%Q'"), + "the message must quote the value that fails; stderr was: {stderr}" + ); + assert!( + !dir.path().join("vault").exists(), + "the run must stop before writing anything" + ); +} + /// 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 From 4936e5275745a74a072db10a79bb5f9278df4fa9 Mon Sep 17 00:00:00 2001 From: Chess Seventh Date: Fri, 31 Jul 2026 00:28:49 +0200 Subject: [PATCH 18/21] docs(readme): document the third fatal config path Missing and blank values were covered; a format chrono cannot render was not, though it is fatal the same way. Co-Authored-By: Vulcan --- README.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/README.md b/README.md index f649360..2d90add 100644 --- a/README.md +++ b/README.md @@ -152,6 +152,10 @@ Keys work the same way, for the same reason: there is no destination to write to, and a hook that quietly journals nothing looks exactly like a quiet day, so this one stays fatal. (`[exclude] repos` is optional, like its section.) +- The two `[templates]` values must be formats `chrono` can actually render, + and that is checked when the config is read. A bad specifier used to surface + from inside the writer as `a formatting trait implementation returned an + error`, naming nothing, after an empty diary file had already been created. - The fatal message names the config file, the key and its section, plus any unrecognised key in that same section, since a misspelt `commit_paths` is the usual reason `commit_path` is missing: @@ -177,6 +181,7 @@ cargo build ./tests/hook-gate.sh unknown-key # journals, names the key on stderr ./tests/hook-gate.sh missing-key # journals nothing, names file + key ./tests/hook-gate.sh blank-key # same, for a key with an empty value +./tests/hook-gate.sh bad-format # same, for a format chrono cannot render ./tests/hook-gate.sh unknown-section # journals, names the section ``` From c905d3556cc3b17f6f1514fcb19537bc254470c3 Mon Sep 17 00:00:00 2001 From: Chess Seventh Date: Fri, 31 Jul 2026 00:38:28 +0200 Subject: [PATCH 19/21] test(config): make the wrote-nothing assertion actually bite It ran in a bare temp directory, so the binary always died in repository discovery before it could write - the assertion passed even with the format check deleted, while reading like a guard. - run it inside a real git repository, so the run reaches the point where it would create the empty diary file the check exists to prevent - add the control it needed: a renderable format in the same setup must journal, otherwise wrote-nothing proves nothing Verified by mutation: neutering the check now fails this test. Co-Authored-By: Vulcan --- tests/integration_tests.rs | 64 +++++++++++++++++++++++++++++++++++--- 1 file changed, 60 insertions(+), 4 deletions(-) diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index c9eabf9..a20733e 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -200,19 +200,27 @@ fn missing_required_key_names_the_file_and_the_key() { /// This one used to surface from inside the writer as `a formatting trait /// implementation returned an error`, naming neither file nor key - and only /// after an empty diary file had been created. +/// +/// Runs inside a **real git repository**, unlike its neighbours here. The +/// others can die in repo discovery and still prove their point; this one +/// cannot, because "wrote nothing" is only meaningful if the run could have +/// got far enough to write. In a bare temp directory that assertion passes +/// even with the config check deleted. #[test] -fn unrenderable_time_format_names_the_file_and_the_key() { +fn unrenderable_time_format_names_the_key_and_writes_nothing() { let dir = tempfile::tempdir().unwrap(); + let vault = dir.path().join("vault"); let ini = dir.path().join("rusty-commit-saver.ini"); fs::write( &ini, format!( "[obsidian]\nroot_path_dir={}\ncommit_path=Commits\n\ [templates]\ncommit_date_path=%Y-%m-%d.md\ncommit_datetime=%Q\n", - dir.path().join("vault").display() + vault.display() ), ) .unwrap(); + commit_once_in(dir.path()); let output = std::process::Command::new(env!("CARGO_BIN_EXE_rusty-commit-saver")) .env("RUSTY_COMMIT_SAVER_CONFIG", &ini) @@ -235,11 +243,59 @@ fn unrenderable_time_format_names_the_file_and_the_key() { "the message must quote the value that fails; stderr was: {stderr}" ); assert!( - !dir.path().join("vault").exists(), - "the run must stop before writing anything" + !vault.exists(), + "the run must stop before creating anything; the empty diary file this \ + used to leave behind is half the reason the check exists" ); } +/// The control for the test above: the same run, with a format `chrono` can +/// render, must reach the vault and write. Without this, "wrote nothing" could +/// pass for a reason that has nothing to do with the config check. +#[test] +fn a_renderable_time_format_reaches_the_vault() { + let dir = tempfile::tempdir().unwrap(); + let vault = dir.path().join("vault"); + let ini = dir.path().join("rusty-commit-saver.ini"); + fs::write( + &ini, + format!( + "[obsidian]\nroot_path_dir={}\ncommit_path=Commits\n\ + [templates]\ncommit_date_path=%Y-%m-%d.md\ncommit_datetime=%H:%M\n", + vault.display() + ), + ) + .unwrap(); + commit_once_in(dir.path()); + + 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"); + + assert!( + vault.exists(), + "a good config must journal; stderr was: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +/// Initialises a git repository at `path` with one commit, so a run started +/// there gets past repository discovery. +fn commit_once_in(path: &std::path::Path) { + use git2::{Repository, Signature}; + + let repo = Repository::init(path).unwrap(); + let sig = Signature::now("Test User", "test@example.com").unwrap(); + let tree_id = repo.index().unwrap().write_tree().unwrap(); + let tree = repo.find_tree(tree_id).unwrap(); + + repo.commit(Some("HEAD"), &sig, &sig, "Initial commit", &tree, &[]) + .unwrap(); +} + /// 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 From 5423cea4ac750eb75a09e725a307efcdc83f963e Mon Sep 17 00:00:00 2001 From: Chess Seventh Date: Fri, 31 Jul 2026 00:54:14 +0200 Subject: [PATCH 20/21] test(config): serialise the tests that share the config env var Five tests in this module write RUSTY_COMMIT_SAVER_CONFIG and two read it back. Under cargo test they share one process and race: a reader can see the value another test just set, and the run fails on a path it never wrote. Nextest, which the gate uses, gives each test its own process and hides it. - take one lock around every test that touches the variable - ignore poisoning: several of these panic deliberately, which is no reason to fail the rest Measured: 100 threaded runs, 0 failures, against roughly 3 in 100 before. Co-Authored-By: Vulcan --- src/config.rs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/config.rs b/src/config.rs index c838c5e..20ee011 100644 --- a/src/config.rs +++ b/src/config.rs @@ -2530,6 +2530,20 @@ mod global_vars_tests { mod user_input_tests { use super::*; use clap::Parser; + use std::sync::{Mutex, MutexGuard, PoisonError}; + + /// `RUSTY_COMMIT_SAVER_CONFIG` is process-global, and several tests here + /// both write and read it. Under `cargo test` they share one process and + /// race: a test reading the var can see the value another test just set. + /// (`cargo nextest`, which the gate uses, runs each test in its own + /// process and never sees this.) + static CONFIG_ENV: Mutex<()> = Mutex::new(()); + + /// Takes the lock above, ignoring poisoning - several of these tests panic + /// deliberately, and a poisoned lock is not a reason to fail the rest. + fn lock_config_env() -> MutexGuard<'static, ()> { + CONFIG_ENV.lock().unwrap_or_else(PoisonError::into_inner) + } #[test] fn test_user_input_parse_with_config() { @@ -2783,6 +2797,7 @@ commit_datetime = %Y-%m-%d %H:%M:%S "; fs::write(temp_file.path(), config_content).expect("Failed to write temp config"); + let _guard = lock_config_env(); env::set_var( "RUSTY_COMMIT_SAVER_CONFIG", temp_file.path().to_str().unwrap(), @@ -2803,6 +2818,7 @@ commit_datetime = %Y-%m-%d %H:%M:%S #[test] #[should_panic(expected = "config_path DOES NOT exists")] fn test_retrieve_config_file_path_panics_on_missing_file() { + let _guard = lock_config_env(); std::env::set_var("RUSTY_COMMIT_SAVER_CONFIG", "/nonexistent/path/config.ini"); let _ = retrieve_config_file_path(); } @@ -2811,6 +2827,7 @@ commit_datetime = %Y-%m-%d %H:%M:%S fn test_get_or_default_config_ini_path_env_var_with_tilde() { use std::env; + let _guard = lock_config_env(); let var_name = "RUSTY_COMMIT_SAVER_CONFIG"; let original = env::var(var_name).ok(); @@ -2946,6 +2963,7 @@ commit_datetime = %Y-%m-%d %H:%M:%S File::create(&file_path).unwrap(); fs::set_permissions(&file_path, fs::Permissions::from_mode(0o000)).unwrap(); + let _guard = lock_config_env(); std::env::set_var("RUSTY_COMMIT_SAVER_CONFIG", file_path.to_str().unwrap()); // This should panic because file exists but can't be read @@ -2969,6 +2987,7 @@ commit_datetime = %Y-%m-%d %H:%M:%S writeln!(temp_file, "commit_datetime=%H:%M:%S").unwrap(); temp_file.flush().unwrap(); + let _guard = lock_config_env(); std::env::set_var("RUSTY_COMMIT_SAVER_CONFIG", temp_file.path()); // The convenience wrapper must resolve the path the same way From 3a37b1a19e9dc722fecfa8872433a96ddc82397c Mon Sep 17 00:00:00 2001 From: Chess Seventh Date: Fri, 31 Jul 2026 00:55:11 +0200 Subject: [PATCH 21/21] docs(config): stop the control test claiming more than it checks It asserts the run reaches the point of creating the vault, not that a row lands in the file; the row is covered in vim_commit. Co-Authored-By: Vulcan --- tests/integration_tests.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index a20733e..833e7a7 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -250,8 +250,10 @@ fn unrenderable_time_format_names_the_key_and_writes_nothing() { } /// The control for the test above: the same run, with a format `chrono` can -/// render, must reach the vault and write. Without this, "wrote nothing" could -/// pass for a reason that has nothing to do with the config check. +/// render, must get as far as creating the vault. Without this, "wrote +/// nothing" could pass for a reason that has nothing to do with the config +/// check. What lands *in* the file is covered by the row tests in +/// `src/vim_commit.rs`. #[test] fn a_renderable_time_format_reaches_the_vault() { let dir = tempfile::tempdir().unwrap();