date_time: localize, document and unit test conversions from and to g…

…it format

Summary:
As one can observe by the previous diff, this code used to have a bug.

What made this bug easy to introduce and hard to detect was the fact that the logic was
spread in three different crates.

* Centralize the conversions from and to gix to mononoke_types which provides mononoke::DateTime,
* Document the subtleties of our offset storage
* Add a unit test that would have caught the bug fixed in the parent

Reviewed By: RajivTS

Differential Revision: D54944773

fbshipit-source-id: 2ce2342c0aee86a612b5c53be797cc7e55d49711

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"

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",

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<Signature> {
     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(),

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"

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",

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, Error> {
-    DateTime::from_timestamp(time.seconds, -time.offset)
-}
-
 #[derive(Clone, Debug)]
 pub enum BackfillDerivation {
     AllConfiguredTypes,

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;

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"

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",

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<Self> {
+        // 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<Self> {
+        // 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);
+        }
+    }
 }