diff --git a/eden/mononoke/git/git_types/Cargo.toml b/eden/mononoke/git/git_types/Cargo.toml index 20456bebb28f8..1b3a8fa6e4221 100644 --- a/eden/mononoke/git/git_types/Cargo.toml +++ b/eden/mononoke/git/git_types/Cargo.toml @@ -26,7 +26,6 @@ futures = { version = "0.3.28", features = ["async-await", "compat"] } futures-util = "0.3.7" git_types_thrift = { version = "0.1.0", path = "if" } gix-actor = "0.24" -gix-date = "0.7" gix-diff = "0.33" gix-hash = "0.11" gix-object = "0.33" diff --git a/eden/mononoke/git/git_types/TARGETS b/eden/mononoke/git/git_types/TARGETS index beaace498787c..d1c0d1af456b9 100644 --- a/eden/mononoke/git/git_types/TARGETS +++ b/eden/mononoke/git/git_types/TARGETS @@ -38,7 +38,6 @@ rust_library( "fbsource//third-party/rust:futures", "fbsource//third-party/rust:futures-util", "fbsource//third-party/rust:gix-actor", - "fbsource//third-party/rust:gix-date", "fbsource//third-party/rust:gix-diff", "fbsource//third-party/rust:gix-hash", "fbsource//third-party/rust:gix-object", diff --git a/eden/mononoke/git/git_types/src/derive_commit.rs b/eden/mononoke/git/git_types/src/derive_commit.rs index d23975cb7329f..1357ae0214eed 100644 --- a/eden/mononoke/git/git_types/src/derive_commit.rs +++ b/eden/mononoke/git/git_types/src/derive_commit.rs @@ -36,7 +36,7 @@ use crate::TreeHandle; fn get_signature(id_str: &str, time: &DateTime) -> Result { let (name, email) = get_name_and_email(id_str)?; - let signature_time = gix_date::Time::new(time.timestamp_secs(), -time.tz_offset_secs()); + let signature_time = time.into_gix(); Ok(Signature { name: name.into(), email: email.into(), diff --git a/eden/mononoke/git/import_tools/Cargo.toml b/eden/mononoke/git/import_tools/Cargo.toml index 21859794ef1d8..60fc5d4aea257 100644 --- a/eden/mononoke/git/import_tools/Cargo.toml +++ b/eden/mononoke/git/import_tools/Cargo.toml @@ -20,7 +20,6 @@ filestore = { version = "0.1.0", path = "../../filestore" } futures = { version = "0.3.28", features = ["async-await", "compat"] } git_symbolic_refs = { version = "0.1.0", path = "../../git_symbolic_refs" } gix-actor = "0.24" -gix-date = "0.7" gix-hash = "0.11" gix-object = "0.33" http = "0.2" diff --git a/eden/mononoke/git/import_tools/TARGETS b/eden/mononoke/git/import_tools/TARGETS index 7ce187abb5d10..a3007ecd231db 100644 --- a/eden/mononoke/git/import_tools/TARGETS +++ b/eden/mononoke/git/import_tools/TARGETS @@ -12,7 +12,6 @@ rust_library( "fbsource//third-party/rust:encoding_rs", "fbsource//third-party/rust:futures", "fbsource//third-party/rust:gix-actor", - "fbsource//third-party/rust:gix-date", "fbsource//third-party/rust:gix-hash", "fbsource//third-party/rust:gix-object", "fbsource//third-party/rust:http", diff --git a/eden/mononoke/git/import_tools/src/gitimport_objects.rs b/eden/mononoke/git/import_tools/src/gitimport_objects.rs index 2f6926ca38ea8..3e52b0b10c5c1 100644 --- a/eden/mononoke/git/import_tools/src/gitimport_objects.rs +++ b/eden/mononoke/git/import_tools/src/gitimport_objects.rs @@ -349,7 +349,7 @@ impl TagMetadata { let author_date = tagger .take() - .map(|tagger| convert_time_to_datetime(&tagger.time)) + .map(|tagger| DateTime::from_gix(tagger.time)) .transpose()?; let author = tagger .take() @@ -499,8 +499,8 @@ impl ExtractedCommit { trees }; - let author_date = convert_time_to_datetime(&author.time)?; - let committer_date = convert_time_to_datetime(&committer.time)?; + let author_date = DateTime::from_gix(author.time)?; + let committer_date = DateTime::from_gix(committer.time)?; let author = format_signature(author.to_ref()); let committer = format_signature(committer.to_ref()); let message = decode_message(&message, &encoding, ctx.logger())?; @@ -620,10 +620,6 @@ impl ExtractedCommit { } } -pub fn convert_time_to_datetime(time: &gix_date::Time) -> Result { - DateTime::from_timestamp(time.seconds, -time.offset) -} - #[derive(Clone, Debug)] pub enum BackfillDerivation { AllConfiguredTypes, diff --git a/eden/mononoke/git/import_tools/src/lib.rs b/eden/mononoke/git/import_tools/src/lib.rs index 58a7fc549d7f0..65eb993497b0f 100644 --- a/eden/mononoke/git/import_tools/src/lib.rs +++ b/eden/mononoke/git/import_tools/src/lib.rs @@ -46,7 +46,6 @@ use tokio::process::Command; use tokio::task; pub use crate::git_reader::GitRepoReader; -pub use crate::gitimport_objects::convert_time_to_datetime; pub use crate::gitimport_objects::oid_to_sha1; use crate::gitimport_objects::read_raw_object; pub use crate::gitimport_objects::BackfillDerivation; diff --git a/eden/mononoke/mononoke_types/Cargo.toml b/eden/mononoke/mononoke_types/Cargo.toml index fcd719f66cc45..298503623aa17 100644 --- a/eden/mononoke/mononoke_types/Cargo.toml +++ b/eden/mononoke/mononoke_types/Cargo.toml @@ -31,6 +31,7 @@ edenapi_types = { version = "0.1.0", path = "../../scm/lib/edenapi/types" } faster-hex = "0.6.1" fbthrift = { version = "0.0.1+unstable", git = "https://github.com/facebook/fbthrift.git", branch = "main" } futures = { version = "0.3.28", features = ["async-await", "compat"] } +gix-date = "0.7" gix-hash = "0.11" itertools = "0.11.0" lazy_static = "1.4" diff --git a/eden/mononoke/mononoke_types/TARGETS b/eden/mononoke/mononoke_types/TARGETS index 9c2733c4a8f84..8de826115ebeb 100644 --- a/eden/mononoke/mononoke_types/TARGETS +++ b/eden/mononoke/mononoke_types/TARGETS @@ -35,6 +35,7 @@ rust_library( "fbsource//third-party/rust:derivative", "fbsource//third-party/rust:faster-hex", "fbsource//third-party/rust:futures", + "fbsource//third-party/rust:gix-date", "fbsource//third-party/rust:gix-hash", "fbsource//third-party/rust:itertools", "fbsource//third-party/rust:lazy_static", diff --git a/eden/mononoke/mononoke_types/src/datetime.rs b/eden/mononoke/mononoke_types/src/datetime.rs index 854c65b2b0928..fab5124570b63 100644 --- a/eden/mononoke/mononoke_types/src/datetime.rs +++ b/eden/mononoke/mononoke_types/src/datetime.rs @@ -57,7 +57,14 @@ impl DateTime { DateTime(now.with_timezone(now.offset())) } + /// Note: the way we store timezone offsets here is unconventional. + /// We store the timezone with `FixedOffset::west_opt`. + /// Typically, a negative timezone offset would indicate that the timezone is west (for + /// instance, if thinking of New York as UTC-5, thinking of the timezone offset as -5 is the + /// eastern convention that would be achieved with `FixedOffset::east_opt`). + /// We picked the western convention for Mononoke, but that contrasts with common practice. pub fn from_timestamp(secs: i64, tz_offset_secs: i32) -> Result { + // https://docs.rs/chrono/latest/chrono/struct.FixedOffset.html#method.west_opt let tz = FixedOffset::west_opt(tz_offset_secs).ok_or_else(|| { MononokeTypeError::InvalidDateTime(format!( "timezone offset out of range: {}", @@ -74,6 +81,19 @@ impl DateTime { Ok(Self::new(dt)) } + pub fn from_gix(time: gix_date::Time) -> Result { + // As you can see in from_timestamp, we store the timezone offset with + // `FixedOffset::west_opt` (offset would be 5 for UTC-5) + // gix uses the eastern convension (offset would be -5 for UTC-5) + Self::from_timestamp(time.seconds, -time.offset) + } + + pub fn into_gix(&self) -> gix_date::Time { + // As you can see in tz_offset_secs, that offset is representes as UTC - local (offset would be 5 for UTC-5) + // gix needs the opposite (offset would be -5 for UTC-5) + gix_date::Time::new(self.timestamp_secs(), -self.tz_offset_secs()) + } + /// Construct a new `DateTime` from an RFC3339 string. /// /// RFC3339 is a standardized way to represent a specific moment in time. See @@ -335,4 +355,26 @@ mod test { let ts1 = Timestamp::from_timestamp_secs(1); assert_eq!(ts0, ts1); } + + #[test] + fn gix_round_trip() { + // Let's use ISO8601_STRICT (subset of rfc 3339) as our human readable reference for a timestamp to ensure we don't + // only round trip between gix and our DateTime, but also that we are not doing a mistake + // and cancelling it out during the round trip + for human_readable in [ + "2018-01-01T00:00:00+00:00", + "2018-01-01T00:00:00+04:00", + "2018-01-01T00:00:00-04:00", + "2018-01-01T01:02:03+05:45", + ] { + let original_date_time = DateTime::from_rfc3339(human_readable).unwrap(); + let gix_time = original_date_time.into_gix(); + assert_eq!( + human_readable.to_string(), + gix_time.format(gix_date::time::format::ISO8601_STRICT) + ); + let roundtripped_date_time = DateTime::from_gix(gix_time).unwrap(); + assert_eq!(original_date_time, roundtripped_date_time); + } + } }