main: fail gracefully when --config-file is missing or malformed - #1235
Open
VXNCXNX wants to merge 1 commit into
Open
main: fail gracefully when --config-file is missing or malformed#1235VXNCXNX wants to merge 1 commit into
VXNCXNX wants to merge 1 commit into
Conversation
Config::from_file already prints its own diagnostic and returns None by design; .expect() turned that into a panic with a backtrace note. The XDG config path keeps falling back silently.
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
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.
No issue is filed for this one, I found it while looking at something else.
What's broken
--config-filepanics instead of failing cleanly:For a missing file nothing at all is printed before the panic, because
from_file's error branch only callsprint_error!when the io error is notNotFound. So the user gets a raw Rust panic and a backtrace prompt with no diagnostic.A malformed file prints the diagnostic and then panics anyway:
The same malformed file at
$XDG_CONFIG_HOME/lsd/config.yamldoes not panic. It prints the error and falls back to defaults, so--config-fileis the odd one out.The cause
src/main.rs:122,Config::from_file(path).expect("Provided file path is invalid").from_filereturnsOptionand handles its own diagnostics, returningNoneby design. The.expect()turns that designedNoneinto a panic. It came in with #1173.The fix
Print an
lsd:diagnostic and exitMajorIssue, matching the existing "cannot access file" exit status.I put the message in
main.rsrather than infrom_file's error branch on purpose.from_fileis shared with the XDG path, where a missing file is normal and must stay silent. One unconditional message at the explicit--config-filecall site covers both failure causes in three lines and changes nothing else.An explicitly passed path that is wrong should not fall back to defaults: someone who typos a path deserves to be told.
After:
The malformed case now prints the specific parse error first, then the verdict.
Verification
cargo fmt --all -- --checkclean,cargo build --releaseclean, unit tests 391 passed.Two integration tests added, asserting non-zero exit, an
lsd:prefixed stderr, and nopanicked at. Reverting the source with the tests kept makes exactly those two fail.Note for CI, neither caused by this change and both confirmed on a clean tree:
test_date_custom_format_supports_nanos_with_lengthfails under a locale that formats decimals with a comma, andcargo clippy --tests -- -D warningsreports 7 errors insrc/color.rs,src/display.rsandsrc/meta/name.rsfrom lints newer than the pinned toolchain.TODO
cargo fmt(cargo fmt --all -- --checkclean)tests/integration.rs)doc/samples(if applicable) (not applicable: no config key is added or changed)doc/samples(if applicable) (not applicable: no icon change)doc/samples(if applicable) (not applicable: no color change)--config-filebut not its failure behaviour)Disclosure: written with AI assistance (Claude Code). I reproduced the issue, ran the change and the verification myself.