Skip to content

Commit a4b5ae5

Browse files
ByronByron
authored
Merge pull request GitoxideLabs#2426 from apollocatlin/date-fix
Remove unsafe UTF-8 conversions and add named relative date parsing
2 parents e4f016b + 4ec879f commit a4b5ae5

8 files changed

Lines changed: 137 additions & 123 deletions

File tree

‎CONTRIBUTING.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
### Please disclose the use AI…
22

3-
…if it's *full agent mode* or *multi*-line completions in the PR comment.
3+
…if it's editing files for you please let us know in the PR comment _or preferably_ using
4+
`Co-authored-by: <agent-identity>` message trailers.
45

56
For everything else, please have a look at the respective section in the [README] file.
67

‎gix-date/src/parse/mod.rs‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,9 @@ impl TimeBuf {
1414
/// Represent this instance as standard string, serialized in a format compatible with
1515
/// signature fields in Git commits, also known as anything parseable as [raw format](function::parse_header()).
1616
pub fn as_str(&self) -> &str {
17-
// SAFETY: We know that serialized times are pure ASCII, a subset of UTF-8.
18-
// `buf` and `len` are written only by time-serialization code.
17+
// Time serializes as ASCII, which is a subset of UTF-8.
1918
let time_bytes = self.buf.as_slice();
20-
#[allow(unsafe_code)]
21-
unsafe {
22-
std::str::from_utf8_unchecked(time_bytes)
23-
}
19+
std::str::from_utf8(time_bytes).expect("time serializes as valid UTF-8")
2420
}
2521

2622
/// Clear the previous content.

‎gix-date/src/parse/relative.rs‎

Lines changed: 48 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -4,38 +4,65 @@ use crate::Error;
44
use gix_error::{ensure, Exn, ResultExt, ValidationError};
55
use jiff::{tz::TimeZone, Span, Timestamp, Zoned};
66

7-
fn parse_inner(input: &str) -> Option<Result<Span, Exn<Error>>> {
8-
let mut split = input.split_whitespace();
9-
let units = i64::from_str(split.next()?).ok()?;
10-
let period = split.next()?;
11-
if split.next()? != "ago" {
12-
return None;
7+
pub fn parse(input: &str, now: Option<SystemTime>) -> Option<Result<Zoned, Exn<Error>>> {
8+
// First try named dates
9+
if let Some(result) = parse_named(input, now) {
10+
return Some(result);
1311
}
14-
span(period, units)
15-
}
1612

17-
pub fn parse(input: &str, now: Option<SystemTime>) -> Option<Result<Zoned, Exn<Error>>> {
18-
parse_inner(input).map(|result| -> Result<Zoned, Exn<Error>> {
13+
// Then try numeric relative dates
14+
parse_ago(input).map(|result| -> Result<Zoned, Exn<Error>> {
1915
let span = result?;
2016
// This was an error case in a previous version of this code, where
2117
// it would fail when converting from a negative signed integer
2218
// to an unsigned integer. This preserves that failure case even
2319
// though the code below handles it okay.
2420
ensure!(!span.is_negative(), ValidationError::new(""));
25-
let now = now.ok_or(ValidationError::new("Missing current time"))?;
26-
let ts: Timestamp = Timestamp::try_from(now).or_raise(|| Error::new("Could not convert current time"))?;
27-
// N.B. This matches the behavior of this code when it was
28-
// written with `time`, but we might consider using the system
29-
// time zone here. If we did, then it would implement "1 day
30-
// ago" correctly, even when it crosses DST transitions. Since
31-
// we're in the UTC time zone here, which has no DST, 1 day is
32-
// in practice always 24 hours. ---AG
33-
let zdt = ts.to_zoned(TimeZone::UTC);
34-
zdt.checked_sub(span)
35-
.or_raise(|| Error::new(format!("Failed to subtract {zdt} from {span}")))
21+
subtract_span(now, span)
3622
})
3723
}
3824

25+
/// Parse named relative dates like "now", "today", "yesterday".
26+
fn parse_named(input: &str, now: Option<SystemTime>) -> Option<Result<Zoned, Exn<Error>>> {
27+
let input = input.trim();
28+
let span = if input.eq_ignore_ascii_case("now") {
29+
Span::new()
30+
} else if input.eq_ignore_ascii_case("today") {
31+
// "today" is treated the same as "now" (current time) for simplicity
32+
Span::new()
33+
} else if input.eq_ignore_ascii_case("yesterday") {
34+
Span::new().try_days(1).ok()?
35+
} else {
36+
return None;
37+
};
38+
39+
Some(subtract_span(now, span))
40+
}
41+
42+
fn parse_ago(input: &str) -> Option<Result<Span, Exn<Error>>> {
43+
let mut split = input.split_whitespace();
44+
let units = i64::from_str(split.next()?).ok()?;
45+
let period = split.next()?;
46+
if split.next()? != "ago" {
47+
return None;
48+
}
49+
span(period, units)
50+
}
51+
52+
fn subtract_span(now: Option<SystemTime>, span: Span) -> Result<Zoned, Exn<ValidationError>> {
53+
let now = now.ok_or(ValidationError::new("Missing current time"))?;
54+
let ts: Timestamp = Timestamp::try_from(now).or_raise(|| Error::new("Could not convert current time"))?;
55+
// N.B. This matches the behavior of this code when it was
56+
// written with `time`, but we might consider using the system
57+
// time zone here. If we did, then it would implement "1 day
58+
// ago" correctly, even when it crosses DST transitions. Since
59+
// we're in the UTC time zone here, which has no DST, 1 day is
60+
// in practice always 24 hours. ---AG
61+
let zdt = ts.to_zoned(TimeZone::UTC);
62+
zdt.checked_sub(span)
63+
.or_raise(|| Error::new(format!("Failed to subtract {zdt} from {span}")))
64+
}
65+
3966
fn span(period: &str, units: i64) -> Option<Result<Span, Exn<Error>>> {
4067
let period = period.strip_suffix('s').unwrap_or(period);
4168
let result = match period {
@@ -51,14 +78,3 @@ fn span(period: &str, units: i64) -> Option<Result<Span, Exn<Error>>> {
5178
};
5279
Some(result.or_raise(|| Error::new(format!("Couldn't parse span from '{period} {units}'"))))
5380
}
54-
55-
#[cfg(test)]
56-
mod tests {
57-
use super::*;
58-
59-
#[test]
60-
fn two_weeks_ago() {
61-
let actual = parse_inner("2 weeks ago").unwrap().unwrap();
62-
assert_eq!(actual.fieldwise(), Span::new().weeks(2));
63-
}
64-
}

‎gix-date/src/time/write.rs‎

Lines changed: 41 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,14 @@
1-
use bstr::ByteSlice;
2-
31
use crate::{SecondsSinceUnixEpoch, Time};
42

53
/// Serialize this instance as string, similar to what [`write_to()`](Self::write_to()) would do.
64
impl std::fmt::Display for Time {
75
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
86
let mut buf = Vec::with_capacity(Time::MAX.size());
97
self.write_to(&mut buf).expect("write to memory cannot fail");
10-
// SAFETY: We know time serializes as ASCII, as subset of UTF8.
11-
#[allow(unsafe_code)]
12-
let raw = unsafe { buf.to_str_unchecked() };
8+
// Time serializes as ASCII, which is a subset of UTF-8.
9+
// Use `from_utf8()` (validated) instead of `from_utf8_unchecked()` for safety,
10+
// with the option to switch to an unsafe version if 30% performance boost are needed here.
11+
let raw = std::str::from_utf8(&buf).expect("time serializes as valid UTF-8");
1312
f.write_str(raw)
1413
}
1514
}
@@ -47,84 +46,43 @@ impl Time {
4746

4847
/// Computes the number of bytes necessary to write it using [`Time::write_to()`].
4948
pub const fn size(&self) -> usize {
50-
(if self.seconds >= 1_000_000_000_000_000_000 {
51-
19
52-
} else if self.seconds >= 100_000_000_000_000_000 {
53-
18
54-
} else if self.seconds >= 10_000_000_000_000_000 {
55-
17
56-
} else if self.seconds >= 1_000_000_000_000_000 {
57-
16
58-
} else if self.seconds >= 100_000_000_000_000 {
59-
15
60-
} else if self.seconds >= 10_000_000_000_000 {
61-
14
62-
} else if self.seconds >= 1_000_000_000_000 {
63-
13
64-
} else if self.seconds >= 100_000_000_000 {
65-
12
66-
} else if self.seconds >= 10_000_000_000 {
67-
11
68-
} else if self.seconds >= 1_000_000_000 {
69-
10
70-
} else if self.seconds >= 100_000_000 {
71-
9
72-
} else if self.seconds >= 10_000_000 {
73-
8
74-
} else if self.seconds >= 1_000_000 {
75-
7
76-
} else if self.seconds >= 100_000 {
77-
6
78-
} else if self.seconds >= 10_000 {
79-
5
80-
} else if self.seconds >= 1_000 {
81-
4
82-
} else if self.seconds >= 100 {
83-
3
84-
} else if self.seconds >= 10 {
85-
2
86-
} else if self.seconds >= 0 {
87-
1
88-
// from here, it's sign + num-digits characters
89-
} else if self.seconds >= -10 {
90-
2
91-
} else if self.seconds >= -100 {
92-
3
93-
} else if self.seconds >= -1_000 {
94-
4
95-
} else if self.seconds >= -10_000 {
96-
5
97-
} else if self.seconds >= -100_000 {
98-
6
99-
} else if self.seconds >= -1_000_000 {
100-
7
101-
} else if self.seconds >= -10_000_000 {
102-
8
103-
} else if self.seconds >= -100_000_000 {
104-
9
105-
} else if self.seconds >= -1_000_000_000 {
106-
10
107-
} else if self.seconds >= -10_000_000_000 {
108-
11
109-
} else if self.seconds >= -100_000_000_000 {
110-
12
111-
} else if self.seconds >= -1_000_000_000_000 {
112-
13
113-
} else if self.seconds >= -10_000_000_000_000 {
114-
14
115-
} else if self.seconds >= -100_000_000_000_000 {
116-
15
117-
} else if self.seconds >= -1_000_000_000_000_000 {
118-
16
119-
} else if self.seconds >= -10_000_000_000_000_000 {
120-
17
121-
} else if self.seconds >= -100_000_000_000_000_000 {
122-
18
123-
} else if self.seconds >= -1_000_000_000_000_000_000 {
124-
19
125-
} else {
126-
20
127-
}) + 2 /*space + offset sign*/ + 2 /*offset hours*/ + 2 /*offset minutes*/
49+
let is_negative = self.seconds < 0;
50+
Self::count_positive_digits(self.seconds.unsigned_abs()) + is_negative as usize + 6
51+
// space + offset sign + hours (2) + minutes (2)
52+
}
53+
54+
/// Count the number of decimal digits in a positive integer.
55+
const fn count_positive_digits(n: u64) -> usize {
56+
// Powers of 10 for comparison
57+
const POW10: [u64; 20] = [
58+
1,
59+
10,
60+
100,
61+
1_000,
62+
10_000,
63+
100_000,
64+
1_000_000,
65+
10_000_000,
66+
100_000_000,
67+
1_000_000_000,
68+
10_000_000_000,
69+
100_000_000_000,
70+
1_000_000_000_000,
71+
10_000_000_000_000,
72+
100_000_000_000_000,
73+
1_000_000_000_000_000,
74+
10_000_000_000_000_000,
75+
100_000_000_000_000_000,
76+
1_000_000_000_000_000_000,
77+
10_000_000_000_000_000_000,
78+
];
79+
80+
// Binary search would be nice but not const-fn friendly, so use simple loop
81+
let mut digits = 1;
82+
while digits < 20 && n >= POW10[digits] {
83+
digits += 1;
84+
}
85+
digits
12886
}
12987

13088
/// The numerically largest possible time instance, whose [size()](Time::size) is the largest possible

‎gix-date/tests/fixtures/generate_git_date_baseline.sh‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,10 @@ baseline '1466000000 -0200' 'RAW' # from git t0006
138138
# ============================================================================
139139
# These tests use GIT_TEST_DATE_NOW=1000000000 (Sun Sep 9 01:46:40 UTC 2001)
140140

141+
# Named
142+
# 'now' and 'today' don't seem to work.
143+
baseline_relative 'yesterday' ''
144+
141145
# Seconds - from git t0006 check_relative
142146
baseline_relative '1 second ago' ''
143147
baseline_relative '2 seconds ago' ''

‎gix-date/tests/fixtures/generated-archives/generate_git_date_baseline.tar‎

0 Bytes
Binary file not shown.

‎gix-date/tests/time/baseline.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ struct Sample {
1919
/// Other relative formats like "yesterday", "last week" etc. are not included in baseline
2020
/// testing because they would require additional handling in the baseline script.
2121
fn is_relative_date(pattern: &str) -> bool {
22-
pattern.ends_with(" ago")
22+
pattern.ends_with(" ago") || pattern == "now" || pattern == "today" || pattern == "yesterday"
2323
}
2424

2525
/// The fixed "now" timestamp used for testing relative dates.

‎gix-date/tests/time/parse/relative.rs‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,3 +152,42 @@ fn various_examples() {
152152
_ = gix_date::parse(date, None).unwrap_or_else(|err| unreachable!("{date}: all examples can be parsed: {err}"));
153153
}
154154
}
155+
156+
mod named {
157+
use std::time::{Duration, SystemTime};
158+
159+
#[test]
160+
fn now() {
161+
let now = SystemTime::UNIX_EPOCH + Duration::from_secs(1_000_000);
162+
let actual = gix_date::parse("now", Some(now)).unwrap();
163+
assert_eq!(actual.seconds, 1_000_000);
164+
}
165+
166+
#[test]
167+
fn today() {
168+
let now = SystemTime::UNIX_EPOCH + Duration::from_secs(1_000_000);
169+
let actual = gix_date::parse(" today ", Some(now)).unwrap();
170+
assert_eq!(
171+
actual.seconds, 1_000_000,
172+
"the input is independent of surrounding whitespace as well"
173+
);
174+
}
175+
176+
#[test]
177+
fn yesterday() {
178+
let now = SystemTime::UNIX_EPOCH + Duration::from_secs(1_000_000);
179+
let actual = gix_date::parse("yesterday", Some(now)).unwrap();
180+
assert_eq!(
181+
actual.seconds,
182+
1_000_000 - 86400,
183+
"yesterday is 1 day (86400 seconds) before now"
184+
);
185+
}
186+
187+
#[test]
188+
fn now_case_insensitive() {
189+
let now = SystemTime::UNIX_EPOCH + Duration::from_secs(1_000_000);
190+
let actual = gix_date::parse("NOW", Some(now)).unwrap();
191+
assert_eq!(actual.seconds, 1_000_000);
192+
}
193+
}

0 commit comments

Comments
 (0)