fix(config): tolerate unrecognised ini keys and name config faults - #75
Merged
Merged
Conversation
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 <chess7th@pm.me>
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 <chess7th@pm.me>
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 <chess7th@pm.me>
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 <chess7th@pm.me>
[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 <chess7th@pm.me>
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 <chess7th@pm.me>
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 <chess7th@pm.me>
- 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 <chess7th@pm.me>
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 <chess7th@pm.me>
- 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 <chess7th@pm.me>
- 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 <chess7th@pm.me>
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 <chess7th@pm.me>
- 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 <chess7th@pm.me>
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 <chess7th@pm.me>
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 <chess7th@pm.me>
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 <chess7th@pm.me>
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 <chess7th@pm.me>
Missing and blank values were covered; a format chrono cannot render was not, though it is fatal the same way. Co-Authored-By: Vulcan <chess7th@pm.me>
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 <chess7th@pm.me>
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 <chess7th@pm.me>
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 <chess7th@pm.me>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the other axis of the outage 4.17.3 half-fixed. That release made an
unrecognised config section non-fatal; the keys were left as they were,
so a rename would have reproduced the same every-commit-in-eight-repos failure
through a different door.
Measured on 4.17.3 by driving real commits through a real post-commit hook:
panicked at src/config.rs:862: Could not get commit_path from config[section] key, and the typo that explains itcommit_path =)%Q)In every case
git commitstill exits 0 and the commit stands — the tool runspost-commit, and git ignores that hook's exit status. That is what makes a
fatal config error safe, and it is now stated in the README.
Decisions
make a broken config indistinguishable from a quiet day; the diary would stop
for weeks unnoticed. The fix is the message, not the severity.
[templates] commit_datetimeis now honoured. It was required on pain ofa fatal error and never read — the row's TIME column was hardcoded. Wiring it
up changes no output for a config already set to
%H:%M:%S.Verification
Four adversarial review rounds; three found a real blocker, each fixed.
Tests are mutation-checked: neutering the format probe, dropping the blank-value
filter, restoring the hardcoded time format, or reverting either call site each
turns the suite red.
Gate: 151 tests, clippy
-D warnings,cargo shear, coverage 96.75% (96.24% onmaster).
tests/hook-gate.shdrives a real commit through a real hook for allsix config cases.
Note:
treefmt --fail-on-changefails on.github/workflows/docs.yml. Thatdrift pre-exists this branch and is untouched by it.