From d867a2e22d046fe406a935488d58e49fcb919be4 Mon Sep 17 00:00:00 2001 From: Pablo Garcia Date: Tue, 11 Aug 2026 22:42:43 +0200 Subject: [PATCH] Report why a --changed-before/--changed-within argument failed to parse TimeFilter::before/after now return Result<_, String> instead of Option, so the underlying jiff parse error reaches the user. An input like 2025-11-31 now says the day is out of range for that month rather than only that the value is invalid. Closes #2053 --- CHANGELOG.md | 1 + src/filter/time.rs | 69 +++++++++++++++++++++++++++++++++------------- src/main.rs | 20 ++++---------- 3 files changed, 57 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c522d6f23..81c52a4fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ - Also fire the "search pattern contains a path separator" diagnostic for `--and` patterns, not only the primary positional pattern. `--and` patterns are matched against the file name just like the primary pattern, so a path separator in them silently returned zero results. See #1873. - Fix bug where passing "-" as a directory argument didn't actually search that directory, see #849 (@Sean-Kenneth-Doherty). - Fix panic when `--changed-before`/`--changed-within` is given an out-of-range `@` Unix timestamp; the value is now rejected gracefully, see #2081 (@nikolauspschuetz). +- Report the underlying reason when `--changed-before`/`--changed-within` fails to parse its argument, e.g. `--changed-before=2025-11-31` now explains that November has only 30 days instead of just calling the input invalid, see #2053 (@MsfPablo). # 10.4.2 diff --git a/src/filter/time.rs b/src/filter/time.rs index bc3b198c8..7a985fa6b 100644 --- a/src/filter/time.rs +++ b/src/filter/time.rs @@ -26,31 +26,38 @@ fn now() -> Zoned { } impl TimeFilter { - fn from_str(s: &str) -> Option { + fn from_str(s: &str) -> Result { if let Ok(span) = s.parse::() { - let datetime = now().checked_sub(span).ok()?; - Some(datetime.into()) + now() + .checked_sub(span) + .map(Into::into) + .map_err(|e| format!("duration '{s}' is out of range: {e}")) + } else if let Some(secs) = s.strip_prefix('@') { + secs.parse::() + .ok() + .and_then(|secs| UNIX_EPOCH.checked_add(Duration::from_secs(secs))) + .ok_or_else(|| { + format!("'{s}' is not a valid unix timestamp: expected '@' followed by a number of seconds since the epoch") + }) } else if let Ok(timestamp) = s.parse::() { - Some(timestamp.into()) - } else if let Ok(datetime) = s.parse::() { - Some( - TimeZone::system() + Ok(timestamp.into()) + } else { + match s.parse::() { + Ok(datetime) => TimeZone::system() .to_ambiguous_zoned(datetime) .later() - .ok()? - .into(), - ) - } else { - let timestamp_secs: u64 = s.strip_prefix('@')?.parse().ok()?; - UNIX_EPOCH.checked_add(Duration::from_secs(timestamp_secs)) + .map(Into::into) + .map_err(|e| format!("date '{s}' does not exist in the local time zone: {e}")), + Err(e) => Err(format!("'{s}' is not a valid date or duration: {e}")), + } } } - pub fn before(s: &str) -> Option { + pub fn before(s: &str) -> Result { TimeFilter::from_str(s).map(TimeFilter::Before) } - pub fn after(s: &str) -> Option { + pub fn after(s: &str) -> Result { TimeFilter::from_str(s).map(TimeFilter::After) } @@ -177,7 +184,7 @@ mod tests { let t1m_ago = ref_time - Duration::from_secs(60); let t1s_later = ref_time + Duration::from_secs(1); // Timestamp only supported via '@' prefix - assert!(TimeFilter::before(&ref_timestamp.to_string()).is_none()); + assert!(TimeFilter::before(&ref_timestamp.to_string()).is_err()); assert!( TimeFilter::before(&format!("@{ref_timestamp}")) .unwrap() @@ -203,8 +210,32 @@ mod tests { #[test] fn out_of_range_unix_timestamp_is_rejected() { // A '@' timestamp large enough to overflow SystemTime must return - // None rather than panicking. - assert!(TimeFilter::before(&format!("@{}", u64::MAX)).is_none()); - assert!(TimeFilter::after(&format!("@{}", u64::MAX)).is_none()); + // an error rather than panicking. + assert!(TimeFilter::before(&format!("@{}", u64::MAX)).is_err()); + assert!(TimeFilter::after(&format!("@{}", u64::MAX)).is_err()); + } + + #[test] + fn error_messages_explain_why_parsing_failed() { + // A well-formatted date that does not exist in the calendar should say so, + // instead of only claiming the input is invalid. + let err = TimeFilter::before("2025-11-31").unwrap_err(); + assert!( + err.contains("2025-11-31") && err.contains("day"), + "unexpected error message: {err}" + ); + + let err = TimeFilter::after("not-a-date").unwrap_err(); + assert!( + err.contains("not-a-date") + && err.len() > "'not-a-date' is not a valid date or duration".len(), + "unexpected error message: {err}" + ); + + let err = TimeFilter::before("@nope").unwrap_err(); + assert!( + err.contains("unix timestamp"), + "unexpected error message: {err}" + ); } } diff --git a/src/main.rs b/src/main.rs index 609078b2b..2e8308044 100644 --- a/src/main.rs +++ b/src/main.rs @@ -496,23 +496,15 @@ fn determine_ls_command(colored_output: bool) -> Result> { fn extract_time_constraints(opts: &Opts) -> Result> { let mut time_constraints: Vec = Vec::new(); if let Some(ref t) = opts.changed_within { - if let Some(f) = TimeFilter::after(t) { - time_constraints.push(f); - } else { - return Err(anyhow!( - "'{}' is not a valid date or duration. See 'fd --help'.", - t - )); + match TimeFilter::after(t) { + Ok(f) => time_constraints.push(f), + Err(e) => return Err(anyhow!("{}. See 'fd --help'.", e)), } } if let Some(ref t) = opts.changed_before { - if let Some(f) = TimeFilter::before(t) { - time_constraints.push(f); - } else { - return Err(anyhow!( - "'{}' is not a valid date or duration. See 'fd --help'.", - t - )); + match TimeFilter::before(t) { + Ok(f) => time_constraints.push(f), + Err(e) => return Err(anyhow!("{}. See 'fd --help'.", e)), } } Ok(time_constraints)