From c5f5383fca5617123efcbef653db9f15d13f5862 Mon Sep 17 00:00:00 2001 From: Mohamed MAACHE Date: Sat, 4 Jul 2026 20:11:26 +0200 Subject: [PATCH 1/4] fix: include underlying parse error for invalid --changed-before/-within dates Previously an invalid date like 2025-11-31 (November has 30 days) was rejected with a generic 'is not a valid date or duration' message, discarding the actual reason. Surface the inner parser error instead, e.g. 'day ... is invalid, must be in range 1..=30', so the user can tell it's an out-of-range calendar date rather than a formatting issue. Fixes #2053 --- CHANGELOG.md | 1 + src/filter/time.rs | 73 ++++++++++++++++++++++++++++++++++------------ src/main.rs | 32 +++++++++++--------- 3 files changed, 73 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 52b09d9f3..5b409505f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ - Fire the "search pattern contains a path separator" diagnostic for any pattern containing `/`, not just patterns that happen to name an existing directory. Preserves the legacy Windows behaviour that also flags native `\` separators when the pattern resolves to a real directory. See #1873. - 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). +- Include the underlying parser error in the "not a valid date or duration" message for `--changed-before`/`--changed-within`, e.g. to explain that a given day is out of range for a calendar date, see #2053. # 10.4.2 diff --git a/src/filter/time.rs b/src/filter/time.rs index 12e8f5664..066bc994f 100644 --- a/src/filter/time.rs +++ b/src/filter/time.rs @@ -26,31 +26,47 @@ fn now() -> Zoned { } impl TimeFilter { - fn from_str(s: &str) -> Option { + /// Parses a duration/timestamp/date/`@`-prefixed unix-timestamp string. + /// + /// On failure, returns the underlying parse error message from the + /// `DateTime` parser (the calendar-date format, and the most common + /// source of confusing failures, e.g. `2025-11-31` which is not a valid + /// calendar date) instead of silently discarding it, so callers can tell + /// the user *why* their input was rejected. + fn from_str(s: &str) -> Result { if let Ok(span) = s.parse::() { - let datetime = now().checked_sub(span).ok()?; - Some(datetime.into()) - } else if let Ok(timestamp) = s.parse::() { - Some(timestamp.into()) - } else if let Ok(datetime) = s.parse::() { - Some( - TimeZone::system() - .to_ambiguous_zoned(datetime) - .later() - .ok()? - .into(), - ) - } else { - let timestamp_secs: u64 = s.strip_prefix('@')?.parse().ok()?; - Some(UNIX_EPOCH + Duration::from_secs(timestamp_secs)) + let datetime = now().checked_sub(span).map_err(|e| e.to_string())?; + return Ok(datetime.into()); + } + if let Ok(timestamp) = s.parse::() { + return Ok(timestamp.into()); + } + match s.parse::() { + Ok(datetime) => TimeZone::system() + .to_ambiguous_zoned(datetime) + .later() + .map(Into::into) + .map_err(|e| e.to_string()), + Err(datetime_err) => { + if let Some(timestamp_secs) = s.strip_prefix('@') + && let Ok(timestamp_secs) = timestamp_secs.parse() + { + return Ok(UNIX_EPOCH + Duration::from_secs(timestamp_secs)); + } + // None of the supported formats matched. The `DateTime` + // parser gives the most useful reason for calendar-date- + // shaped input (the common case for this kind of mistake), + // so surface that instead of a generic message. + Err(datetime_err.to_string()) + } } } - 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 +193,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() @@ -199,4 +215,23 @@ mod tests { .applies_to(&t1s_later) ); } + + #[test] + fn invalid_calendar_date_error_includes_inner_reason() { + // November only has 30 days, so this is not a valid calendar date. + // The error message should include *why* it's invalid (e.g. mention + // "day" being out of range) instead of a generic rejection, see + // https://github.com/sharkdp/fd/issues/2053 + let err = TimeFilter::before("2025-11-31").unwrap_err(); + assert!( + err.contains("day"), + "error message should mention the invalid 'day' component, got: {err}" + ); + + let err = TimeFilter::after("2025-11-31").unwrap_err(); + assert!( + err.contains("day"), + "error message should mention the invalid 'day' component, got: {err}" + ); + } } diff --git a/src/main.rs b/src/main.rs index 609078b2b..5ed7c7062 100644 --- a/src/main.rs +++ b/src/main.rs @@ -496,23 +496,27 @@ 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!( + "'{}' is not a valid date or duration: {}. See 'fd --help'.", + t, + 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!( + "'{}' is not a valid date or duration: {}. See 'fd --help'.", + t, + e + )); + } } } Ok(time_constraints) From a88232ee4607176304ad5dca486eda9a38705191 Mon Sep 17 00:00:00 2001 From: Mohamed MAACHE Date: Sun, 5 Jul 2026 14:26:04 +0200 Subject: [PATCH 2/4] fix: use anyhow::Result instead of String for date parse errors Addresses review comments on #2054. TimeFilter::from_str/before/after now return anyhow::Result instead of Result<_, String>, matching the error convention used elsewhere in the crate (e.g. walk.rs). Call sites in main.rs already compile unchanged since anyhow::Error's Display shows the same top-level message a String did. Also relax the calendar-date test: instead of asserting the message contains "day" (coupled to jiff's exact wording), it now checks the message is non-empty and differs from the message for an unrelated malformed input, which still proves the inner reason is surfaced without breaking on a jiff wording change. --- src/filter/time.rs | 61 ++++++++++++++++++++++++++++------------------ 1 file changed, 37 insertions(+), 24 deletions(-) diff --git a/src/filter/time.rs b/src/filter/time.rs index 066bc994f..e67e20531 100644 --- a/src/filter/time.rs +++ b/src/filter/time.rs @@ -1,3 +1,4 @@ +use anyhow::Result; use jiff::{Span, Timestamp, Zoned, civil::DateTime, tz::TimeZone}; use std::time::{Duration, SystemTime, UNIX_EPOCH}; @@ -28,25 +29,24 @@ fn now() -> Zoned { impl TimeFilter { /// Parses a duration/timestamp/date/`@`-prefixed unix-timestamp string. /// - /// On failure, returns the underlying parse error message from the - /// `DateTime` parser (the calendar-date format, and the most common - /// source of confusing failures, e.g. `2025-11-31` which is not a valid - /// calendar date) instead of silently discarding it, so callers can tell - /// the user *why* their input was rejected. - fn from_str(s: &str) -> Result { + /// On failure, returns the underlying parse error from the `DateTime` + /// parser (the calendar-date format, and the most common source of + /// confusing failures, e.g. `2025-11-31` which is not a valid calendar + /// date) instead of silently discarding it, so callers can tell the + /// user *why* their input was rejected. + fn from_str(s: &str) -> Result { if let Ok(span) = s.parse::() { - let datetime = now().checked_sub(span).map_err(|e| e.to_string())?; + let datetime = now().checked_sub(span)?; return Ok(datetime.into()); } if let Ok(timestamp) = s.parse::() { return Ok(timestamp.into()); } match s.parse::() { - Ok(datetime) => TimeZone::system() + Ok(datetime) => Ok(TimeZone::system() .to_ambiguous_zoned(datetime) - .later() - .map(Into::into) - .map_err(|e| e.to_string()), + .later()? + .into()), Err(datetime_err) => { if let Some(timestamp_secs) = s.strip_prefix('@') && let Ok(timestamp_secs) = timestamp_secs.parse() @@ -57,16 +57,16 @@ impl TimeFilter { // parser gives the most useful reason for calendar-date- // shaped input (the common case for this kind of mistake), // so surface that instead of a generic message. - Err(datetime_err.to_string()) + Err(datetime_err.into()) } } } - pub fn before(s: &str) -> Result { + pub fn before(s: &str) -> Result { TimeFilter::from_str(s).map(TimeFilter::Before) } - pub fn after(s: &str) -> Result { + pub fn after(s: &str) -> Result { TimeFilter::from_str(s).map(TimeFilter::After) } @@ -219,19 +219,32 @@ mod tests { #[test] fn invalid_calendar_date_error_includes_inner_reason() { // November only has 30 days, so this is not a valid calendar date. - // The error message should include *why* it's invalid (e.g. mention - // "day" being out of range) instead of a generic rejection, see + // The error should surface the actual underlying parse failure + // instead of a generic rejection. We deliberately don't assert on + // jiff's exact wording (e.g. that it mentions "day"), since that + // would break on an unrelated jiff version bump. Instead, check + // that the message is non-empty and differs from the message for a + // differently-malformed input, which shows the inner reason is + // actually being propagated. See // https://github.com/sharkdp/fd/issues/2053 - let err = TimeFilter::before("2025-11-31").unwrap_err(); - assert!( - err.contains("day"), - "error message should mention the invalid 'day' component, got: {err}" + let day_err = TimeFilter::before("2025-11-31").unwrap_err().to_string(); + let garbage_err = TimeFilter::before("not-a-real-date") + .unwrap_err() + .to_string(); + assert!(!day_err.is_empty()); + assert_ne!( + day_err, garbage_err, + "distinct invalid inputs should surface distinct underlying reasons, not a shared generic message" ); - let err = TimeFilter::after("2025-11-31").unwrap_err(); - assert!( - err.contains("day"), - "error message should mention the invalid 'day' component, got: {err}" + let day_err = TimeFilter::after("2025-11-31").unwrap_err().to_string(); + let garbage_err = TimeFilter::after("not-a-real-date") + .unwrap_err() + .to_string(); + assert!(!day_err.is_empty()); + assert_ne!( + day_err, garbage_err, + "distinct invalid inputs should surface distinct underlying reasons, not a shared generic message" ); } } From e84b5902a284e38827da683decaf53b21779d981 Mon Sep 17 00:00:00 2001 From: Mohamed MAACHE Date: Sun, 5 Jul 2026 15:10:29 +0200 Subject: [PATCH 3/4] test: harden inner-reason regression test against echoed-input mutation The previous version only checked that two invalid inputs produce different messages, which still passes if the code drops the real parse reason but keeps echoing the raw input. Compare two dates that are invalid for the same reason (day 31 in a 30-day month) against one invalid for a different reason (month 13), and require the same-reason pair to share substantially more text. Still avoids asserting on jiff's exact wording. --- src/filter/time.rs | 102 +++++++++++++++++++++++++++++++++------------ 1 file changed, 76 insertions(+), 26 deletions(-) diff --git a/src/filter/time.rs b/src/filter/time.rs index e67e20531..9df7a77ad 100644 --- a/src/filter/time.rs +++ b/src/filter/time.rs @@ -216,35 +216,85 @@ mod tests { ); } + /// Length of the longest contiguous substring shared by `a` and `b`. + /// + /// Used to check that two error messages share substantial content + /// without hardcoding what that content actually says. + fn longest_common_substring_len(a: &str, b: &str) -> usize { + let a: Vec = a.chars().collect(); + let b: Vec = b.chars().collect(); + let mut prev = vec![0usize; b.len() + 1]; + let mut best = 0; + for i in 1..=a.len() { + let mut cur = vec![0usize; b.len() + 1]; + for j in 1..=b.len() { + if a[i - 1] == b[j - 1] { + cur[j] = prev[j - 1] + 1; + best = best.max(cur[j]); + } + } + prev = cur; + } + best + } + #[test] fn invalid_calendar_date_error_includes_inner_reason() { - // November only has 30 days, so this is not a valid calendar date. - // The error should surface the actual underlying parse failure - // instead of a generic rejection. We deliberately don't assert on - // jiff's exact wording (e.g. that it mentions "day"), since that - // would break on an unrelated jiff version bump. Instead, check - // that the message is non-empty and differs from the message for a - // differently-malformed input, which shows the inner reason is - // actually being propagated. See + // The error must surface *why* parsing failed, not merely echo the + // raw input back under a generic wrapper. We deliberately don't + // assert on jiff's exact wording (e.g. that it mentions "day"), + // since that would break on an unrelated jiff version bump. See // https://github.com/sharkdp/fd/issues/2053 - let day_err = TimeFilter::before("2025-11-31").unwrap_err().to_string(); - let garbage_err = TimeFilter::before("not-a-real-date") - .unwrap_err() - .to_string(); - assert!(!day_err.is_empty()); - assert_ne!( - day_err, garbage_err, - "distinct invalid inputs should surface distinct underlying reasons, not a shared generic message" - ); + // + // A naive check that two different invalid inputs produce two + // different messages is gameable: a regression that dropped the + // real parse reason but still echoed the input (e.g. + // `format!("could not parse '{s}' as a date")`) would still make + // the two messages differ, purely because the inputs differ, while + // never actually surfacing the reason. + // + // Instead, compare error text for inputs that are invalid for the + // *same* underlying reason against error text for an input that's + // invalid for a *different* reason: + // - "2025-11-31" and "2019-06-31" both fail because day 31 doesn't + // exist in a 30-day month (November / June), despite the raw + // strings barely overlapping (different year and month). + // - "2025-13-01" fails for an unrelated reason (month 13 doesn't + // exist). + // A message that actually carries the reason will make the first + // pair share a large chunk of text that the third message doesn't + // share. A message that's just the input echoed into a fixed + // template would make all three overlap by roughly the same + // (small) amount, since the only shared content would be the fixed + // wrapper text. + for filter in [TimeFilter::before, TimeFilter::after] { + let same_reason_a = filter("2025-11-31").unwrap_err().to_string(); + let same_reason_b = filter("2019-06-31").unwrap_err().to_string(); + let different_reason = filter("2025-13-01").unwrap_err().to_string(); - let day_err = TimeFilter::after("2025-11-31").unwrap_err().to_string(); - let garbage_err = TimeFilter::after("not-a-real-date") - .unwrap_err() - .to_string(); - assert!(!day_err.is_empty()); - assert_ne!( - day_err, garbage_err, - "distinct invalid inputs should surface distinct underlying reasons, not a shared generic message" - ); + assert!(!same_reason_a.is_empty()); + assert_ne!( + same_reason_a, different_reason, + "distinct invalid inputs should surface distinct underlying reasons, not a shared generic message" + ); + + let same_reason_overlap = longest_common_substring_len(&same_reason_a, &same_reason_b); + let different_reason_overlap = + longest_common_substring_len(&same_reason_a, &different_reason); + assert!( + same_reason_overlap >= 20, + "two dates invalid for the same reason should share substantial error text \ + (got only {same_reason_overlap} shared characters between {same_reason_a:?} \ + and {same_reason_b:?})" + ); + assert!( + same_reason_overlap > different_reason_overlap + 10, + "shared text between same-reason errors ({same_reason_overlap} chars) should \ + clearly exceed shared text between different-reason errors \ + ({different_reason_overlap} chars): {same_reason_a:?} vs {same_reason_b:?} vs \ + {different_reason:?}; a smaller gap suggests the message may just be echoing \ + the input rather than surfacing the actual reason" + ); + } } } From aa20f635fd2a915800392ee0d3e6597b30cfcc5f Mon Sep 17 00:00:00 2001 From: Mohamed MAACHE Date: Fri, 10 Jul 2026 00:22:57 +0200 Subject: [PATCH 4/4] fix: only surface the calendar-date parse reason for date-shaped input A typo'd duration like '1 huor' used to get the calendar-date parser's error, which points at the wrong format. Non-date-shaped input now gets a summary of the accepted formats instead. --- src/filter/time.rs | 59 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 51 insertions(+), 8 deletions(-) diff --git a/src/filter/time.rs b/src/filter/time.rs index 9df7a77ad..b27bcd7da 100644 --- a/src/filter/time.rs +++ b/src/filter/time.rs @@ -1,4 +1,4 @@ -use anyhow::Result; +use anyhow::{Result, anyhow}; use jiff::{Span, Timestamp, Zoned, civil::DateTime, tz::TimeZone}; use std::time::{Duration, SystemTime, UNIX_EPOCH}; @@ -26,14 +26,23 @@ fn now() -> Zoned { TESTTIME.with_borrow(|reftime| reftime.as_ref().cloned().unwrap_or_else(Zoned::now)) } +/// Whether the input starts like a calendar date (`YYYY-`). Only then does +/// the `DateTime` parser's error refer to the format the user most likely +/// meant; for span- or timestamp-shaped input it would be misleading. +fn looks_like_calendar_date(s: &str) -> bool { + let bytes = s.as_bytes(); + bytes.len() > 4 && bytes[..4].iter().all(u8::is_ascii_digit) && bytes[4] == b'-' +} + impl TimeFilter { /// Parses a duration/timestamp/date/`@`-prefixed unix-timestamp string. /// - /// On failure, returns the underlying parse error from the `DateTime` - /// parser (the calendar-date format, and the most common source of - /// confusing failures, e.g. `2025-11-31` which is not a valid calendar - /// date) instead of silently discarding it, so callers can tell the - /// user *why* their input was rejected. + /// On failure for date-shaped input, returns the underlying parse error + /// from the `DateTime` parser (the most common source of confusing + /// failures, e.g. `2025-11-31` which is not a valid calendar date) + /// instead of silently discarding it, so callers can tell the user + /// *why* their input was rejected. Input that does not look like a + /// calendar date gets a summary of the accepted formats instead. fn from_str(s: &str) -> Result { if let Ok(span) = s.parse::() { let datetime = now().checked_sub(span)?; @@ -56,8 +65,19 @@ impl TimeFilter { // None of the supported formats matched. The `DateTime` // parser gives the most useful reason for calendar-date- // shaped input (the common case for this kind of mistake), - // so surface that instead of a generic message. - Err(datetime_err.into()) + // so surface that instead of a generic message. For input + // shaped like a span (e.g. a typo'd duration) that reason + // would point at the wrong format entirely, so summarize + // the accepted formats instead. + if looks_like_calendar_date(s) { + Err(datetime_err.into()) + } else { + Err(anyhow!( + "expected a duration (e.g. '10h', '2d'), a date (e.g. '2018-10-27'), \ + a timestamp (e.g. '2018-10-27T10:00:00-05:00'), or '@' followed by \ + a unix timestamp" + )) + } } } } @@ -238,6 +258,29 @@ mod tests { best } + #[test] + fn non_date_input_gets_format_summary_not_calendar_date_reason() { + // A typo'd duration or otherwise non-date-shaped input must not + // surface the calendar-date parser's reason (which would point at + // the wrong format entirely), but a neutral summary of the + // accepted formats. + for filter in [TimeFilter::before, TimeFilter::after] { + for input in ["1 huor", "yesterday", "@notanumber"] { + let err = filter(input).unwrap_err().to_string(); + assert!( + err.contains("duration") && err.contains("unix timestamp"), + "non-date-shaped input {input:?} should get the format summary, got: {err:?}" + ); + } + // Date-shaped input keeps the underlying parse reason. + let date_err = filter("2025-11-31").unwrap_err().to_string(); + assert!( + !date_err.contains("expected a duration"), + "date-shaped input should surface the parser's reason, got: {date_err:?}" + ); + } + } + #[test] fn invalid_calendar_date_error_includes_inner_reason() { // The error must surface *why* parsing failed, not merely echo the