Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gix-date: switch from time to jiff #1474

Merged
merged 6 commits into from
Jul 29, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 29 additions & 6 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -164,14 +164,12 @@ http-client-reqwest = ["gix/blocking-http-transport-reqwest-rust-tls"]
## Use async client networking.
gitoxide-core-async-client = ["gitoxide-core/async-client", "futures-lite"]


[dependencies]
anyhow = "1.0.42"

gitoxide-core = { version = "^0.39.1", path = "gitoxide-core" }
gix-features = { version = "^0.38.2", path = "gix-features" }
gix = { version = "^0.64.0", path = "gix", default-features = false }
time = "0.3.23"

clap = { version = "4.1.1", features = ["derive", "cargo"] }
clap_complete = "4.4.3"
Expand Down
6 changes: 3 additions & 3 deletions gix-archive/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ tar = ["dep:tar", "dep:gix-path"]
tar_gz = ["tar", "dep:flate2"]

## Enable the `zip` archive format.
zip = ["dep:zip", "dep:time"]
zip = ["dep:zip"]


[dependencies]
Expand All @@ -31,8 +31,8 @@ gix-path = { version = "^0.10.9", path = "../gix-path", optional = true }
gix-date = { version = "^0.8.7", path = "../gix-date" }

flate2 = { version = "1.0.26", optional = true }
zip = { version = "2.1.0", optional = true, default-features = false, features = ["deflate", "time"] }
time = { version = "0.3.23", optional = true, default-features = false, features = ["std"] }
zip = { version = "2.1.0", optional = true, default-features = false, features = ["deflate"] }
jiff = { version = "0.1.2", default-features = false, features = ["std"] }

thiserror = "1.0.26"
bstr = { version = "1.5.0", default-features = false }
Expand Down
18 changes: 15 additions & 3 deletions gix-archive/src/write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,10 +134,22 @@ where
{
let mut ar = zip::write::ZipWriter::new(out);
let mut buf = Vec::new();
let mtime = time::OffsetDateTime::from_unix_timestamp(opts.modification_time)
let zdt = jiff::Timestamp::from_second(opts.modification_time)
.map_err(|err| Error::InvalidModificationTime(Box::new(err)))?
.try_into()
.map_err(|err| Error::InvalidModificationTime(Box::new(err)))?;
.to_zoned(jiff::tz::TimeZone::UTC);
let mtime = zip::DateTime::from_date_and_time(
zdt.year()
.try_into()
.map_err(|err| Error::InvalidModificationTime(Box::new(err)))?,
// These are all OK because month, day, hour, minute and second
// are always positive.
zdt.month().try_into().expect("non-negative"),
zdt.day().try_into().expect("non-negative"),
zdt.hour().try_into().expect("non-negative"),
zdt.minute().try_into().expect("non-negative"),
zdt.second().try_into().expect("non-negative"),
)
.map_err(|err| Error::InvalidModificationTime(Box::new(err)))?;
while let Some(entry) = next_entry(stream)? {
append_zip_entry(
&mut ar,
Expand Down
2 changes: 1 addition & 1 deletion gix-date/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ serde= ["dep:serde", "bstr/serde"]
bstr = { version = "1.3.0", default-features = false, features = ["std"]}
serde = { version = "1.0.114", optional = true, default-features = false, features = ["derive"]}
itoa = "1.0.1"
time = { version = "0.3.23", default-features = false, features = ["local-offset", "formatting", "macros", "parsing"] }
jiff = "0.1.1"
thiserror = "1.0.32"

document-features = { version = "0.2.0", optional = true }
Expand Down
103 changes: 65 additions & 38 deletions gix-date/src/parse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ pub enum Error {
pub(crate) mod function {
use std::{str::FromStr, time::SystemTime};

use time::{format_description::well_known, Date, OffsetDateTime};
use jiff::{civil::Date, fmt::rfc2822, tz::TimeZone, Zoned};

use crate::{
parse::{relative, Error},
Expand All @@ -32,27 +32,27 @@ pub(crate) mod function {
return Ok(Time::new(42, 1800));
}

Ok(if let Ok(val) = Date::parse(input, SHORT.0) {
let val = val.with_hms(0, 0, 0).expect("date is in range").assume_utc();
Time::new(val.unix_timestamp(), val.offset().whole_seconds())
} else if let Ok(val) = OffsetDateTime::parse(input, &well_known::Rfc2822) {
Time::new(val.unix_timestamp(), val.offset().whole_seconds())
} else if let Ok(val) = OffsetDateTime::parse(input, ISO8601.0) {
Time::new(val.unix_timestamp(), val.offset().whole_seconds())
} else if let Ok(val) = OffsetDateTime::parse(input, ISO8601_STRICT.0) {
Time::new(val.unix_timestamp(), val.offset().whole_seconds())
} else if let Ok(val) = OffsetDateTime::parse(input, GITOXIDE.0) {
Time::new(val.unix_timestamp(), val.offset().whole_seconds())
} else if let Ok(val) = OffsetDateTime::parse(input, DEFAULT.0) {
Time::new(val.unix_timestamp(), val.offset().whole_seconds())
Ok(if let Ok(val) = Date::strptime(SHORT.0, input) {
let val = val.to_zoned(TimeZone::UTC).expect("date is in range");
Time::new(val.timestamp().as_second(), val.offset().seconds())
} else if let Ok(val) = rfc2822_relaxed(input) {
Time::new(val.timestamp().as_second(), val.offset().seconds())
} else if let Ok(val) = strptime_relaxed(ISO8601.0, input) {
Time::new(val.timestamp().as_second(), val.offset().seconds())
} else if let Ok(val) = strptime_relaxed(ISO8601_STRICT.0, input) {
Time::new(val.timestamp().as_second(), val.offset().seconds())
} else if let Ok(val) = strptime_relaxed(GITOXIDE.0, input) {
Time::new(val.timestamp().as_second(), val.offset().seconds())
} else if let Ok(val) = strptime_relaxed(DEFAULT.0, input) {
Time::new(val.timestamp().as_second(), val.offset().seconds())
} else if let Ok(val) = SecondsSinceUnixEpoch::from_str(input) {
// Format::Unix
Time::new(val, 0)
} else if let Some(val) = parse_raw(input) {
// Format::Raw
val
} else if let Some(time) = relative::parse(input, now).transpose()? {
Time::new(time.unix_timestamp(), time.offset().whole_seconds())
} else if let Some(val) = relative::parse(input, now).transpose()? {
Time::new(val.timestamp().as_second(), val.offset().seconds())
} else {
return Err(Error::InvalidDateString { input: input.into() });
})
Expand Down Expand Up @@ -83,52 +83,79 @@ pub(crate) mod function {
};
Some(time)
}

/// This is just like `Zoned::strptime`, but it allows parsing datetimes
/// whose weekdays are inconsistent with the date. While the day-of-week
/// still must be parsed, it is otherwise ignored. This seems to be
/// consistent with how `git` behaves.
fn strptime_relaxed(fmt: &str, input: &str) -> Result<Zoned, jiff::Error> {
let mut tm = jiff::fmt::strtime::parse(fmt, input)?;
tm.set_weekday(None);
tm.to_zoned()
}

/// This is just like strptime_relaxed, except for RFC 2822 parsing.
/// Namely, it permits the weekday to be inconsistent with the date.
fn rfc2822_relaxed(input: &str) -> Result<Zoned, jiff::Error> {
static P: rfc2822::DateTimeParser = rfc2822::DateTimeParser::new().relaxed_weekday(true);
P.parse_zoned(input)
}
}

mod relative {
use std::{str::FromStr, time::SystemTime};

use time::{Duration, OffsetDateTime};
use jiff::{tz::TimeZone, Span, Timestamp, Zoned};

use crate::parse::Error;

fn parse_inner(input: &str) -> Option<Duration> {
fn parse_inner(input: &str) -> Option<Result<Span, Error>> {
let mut split = input.split_whitespace();
let multiplier = i64::from_str(split.next()?).ok()?;
let units = i64::from_str(split.next()?).ok()?;
let period = split.next()?;
if split.next()? != "ago" {
return None;
}
duration(period, multiplier)
span(period, units)
}

pub(crate) fn parse(input: &str, now: Option<SystemTime>) -> Option<Result<OffsetDateTime, Error>> {
parse_inner(input).map(|offset| {
let offset = std::time::Duration::from_secs(offset.whole_seconds().try_into()?);
pub(crate) fn parse(input: &str, now: Option<SystemTime>) -> Option<Result<Zoned, Error>> {
parse_inner(input).map(|result| {
let span = result?;
// This was an error case in a previous version of this code, where
// it would fail when converting from a negative signed integer
// to an unsigned integer. This preserves that failure case even
// though the code below handles it okay.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember that some tests were indeed quite time dependent, while some code also tried to workaround panics in time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I noticed that because I was intensely curious as to why there was a catch_unwind here deep inside datetime parsing. And yeah indeed, I believe time's panic comes from its From<SystemTime> impl, which in turn calls OffsetDateTime's Add impl that specifically panics. The Add impl panicking makes sense, since that's consistent with what std does too. But the From impl should probably be a TryFrom impl. Since it's a From impl, time's behavior here is pretty much locked into its API. With that said, I think you probably could have avoided the catch_unwind here by first converting now to a time::OffsetDateTime and then doing your arithmetic, since time exposes checked operations on time::OffsetDateTime. With Jiff, you can't mess this up because it doesn't provide a From<SystemTime> impl. Only TryFrom<SystemTime>.

if span.is_negative() {
return Err(Error::RelativeTimeConversion);
}
now.ok_or(Error::MissingCurrentTime).and_then(|now| {
std::panic::catch_unwind(|| {
now.checked_sub(offset)
.expect("BUG: values can't be large enough to cause underflow")
.into()
})
.map_err(|_| Error::RelativeTimeConversion)
let ts = Timestamp::try_from(now).map_err(|_| Error::RelativeTimeConversion)?;
// N.B. This matches the behavior of this code when it was
// written with `time`, but we might consider using the system
// time zone here. If we did, then it would implement "1 day
// ago" correctly, even when it crosses DST transitions. Since
// we're in the UTC time zone here, which has no DST, 1 day is
// in practice always 24 hours. ---AG
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to see this becoming more correct, and likely even more correct than Git itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I definitely don't have the context to be fully confident in such a change here. It depends on your test coverage I think. But the idea here is that you'd do ts.to_zoned(TimeZone::system()), and then zdt.checked_sub(span) would become DST aware according to the end user's configured time zone.

let zdt = ts.to_zoned(TimeZone::UTC);
zdt.checked_sub(span).map_err(|_| Error::RelativeTimeConversion)
})
})
}

fn duration(period: &str, multiplier: i64) -> Option<Duration> {
fn span(period: &str, units: i64) -> Option<Result<Span, Error>> {
let period = period.strip_suffix('s').unwrap_or(period);
let seconds: i64 = match period {
"second" => 1,
"minute" => 60,
"hour" => 60 * 60,
"day" => 24 * 60 * 60,
"week" => 7 * 24 * 60 * 60,
let result = match period {
"second" => Span::new().try_seconds(units),
"minute" => Span::new().try_minutes(units),
"hour" => Span::new().try_hours(units),
"day" => Span::new().try_days(units),
"week" => Span::new().try_weeks(units),
// TODO months & years? YES
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something that would easily be supported now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. It would just be Span::new().try_months and then Span::new().try_years. Then everything else should just work.

// Ignore values you don't know, assume seconds then (so does git)
_ => return None,
};
seconds.checked_mul(multiplier).map(Duration::seconds)
Some(result.map_err(|_| Error::RelativeTimeConversion))
}

#[cfg(test)]
Expand All @@ -137,7 +164,7 @@ mod relative {

#[test]
fn two_weeks_ago() {
assert_eq!(parse_inner("2 weeks ago"), Some(Duration::weeks(2)));
assert_eq!(parse_inner("2 weeks ago").unwrap().unwrap(), Span::new().weeks(2));
}
}
}
Loading
Loading