Relative time ambiguity across DST adjustments

[EliahKagan]

Current behavior 😯

When gix-date::date time::parse::relative::various is run in an environment that uses local time in a time zone that has daylight savings clock adjustments, it fails in the two weeks after daylight saving time changes. All other gix-date tests pass.

For example, my time zone is Eastern Time, which had a DST clock adjustment from EDT (UTC-4) to EST (UTC-5) on 3 November 2024. Since then, and until two weeks later, the test failed when I ran it locally. Here's a run from 9 November 2024:
 

        FAIL [   0.310s] gix-date::date time::parse::relative::various

--- STDOUT:              gix-date::date time::parse::relative::various ---

running 1 test
test time::parse::relative::various ... FAILED

failures:

failures:
    time::parse::relative::various

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 27 filtered out; finished in 0.31s


--- STDERR:              gix-date::date time::parse::relative::various ---
thread 'time::parse::relative::various' panicked at gix-date/tests/time/parse.rs:195:9:
assertion `left == right` failed: relative times differ
  left: 2024-10-26T20:22:51Z
 right: 2024-10-26T19:22:51Z
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

------------
     Summary [   0.325s] 29 tests run: 28 passed, 1 failed, 0 skipped
        FAIL [   0.310s] gix-date::date time::parse::relative::various
error: test run failed

Under those conditions, the time is off by one hour, due to ambiguity of phrases like 2 weeks ago. If there was a recent daylight savings adjustment to local time, then was 2 weeks ago the current time minus the usual 336 hours? Or was 2 weeks ago the time, fourteen calendar days prior, at which local clocks showed the time that local clocks show now? Often these two interpretations of relative times coincide, but they do not always.

The test failed on GNU/Linux, macOS, and Windows, and at least on GNU/Linux and macOS, it went away when TZ=UTC is set. With no change to the code, the test started passing again, on all systems, on 17 November 2024, but the bug is still present.

This bug has not been detected automatically on CI, because the environment on GitHub Actions runners uses UTC rather than local time.

(This is the bug that the mention of TZ=UTC in d74e919 (#1652) alludes to.)

Expected behavior 🤔

I am uncertain whether it is the code under test, or the test itself, that should be changed. But if we want to interpret relative times like "2 weeks ago" in the same way GNU/Linux date -d does, and in the way I think Git does (see below for details), then the code under test would have to be changed.

Either way, the bug is not specific to 2-week intervals. if the code is wrong, then the test's use of 2 weeks ago is merely a limitation in when the test is able to identify the code as wrong. If the test is wrong, then while the failure only occurs under limited conditions, the interpretation of relative time that it expresses is still broader.

This is to say that even if it is (only) the test that should change, I think making the interval shorter so that it fails less often would not be the right way to go about it.

Also, either way, we should probably make the test check multiple relative time strings representing intervals of widely varying length.

Git behavior

I don't know if I'm looking at the most relevant Git behavior, but the easiest to examine are the tests in t0006-date.sh. If I understand correctly, relative date strings represent exact numbers of seconds elapsed:

check_relative() {
	t=$(($GIT_TEST_DATE_NOW - $1))
	echo "$t -> $2" >expect
	test_expect_${3:-success} "relative date ($2)" "
	test-tool date relative $t >actual &&
	test_cmp expect actual
	"
}

check_relative 5 '5 seconds ago'
check_relative 300 '5 minutes ago'
check_relative 18000 '5 hours ago'
check_relative 432000 '5 days ago'
check_relative 1728000 '3 weeks ago'
check_relative 13000000 '5 months ago'
check_relative 37500000 '1 year, 2 months ago'
check_relative 55188000 '1 year, 9 months ago'
check_relative 630000000 '20 years ago'
check_relative 31449600 '12 months ago'
check_relative 62985600 '2 years ago'

Those tests pass when run today, on an Arch Linux system set to America/New York and using local time, including when the script is run directly. If git behaved analogously to the current behavior of gix-date, then I would expect the 3 weeks ago and 5 months ago cases, as well as some others, to have failed. I ran that test script at the tip of the master branch, with:

make -j10
cd t
./t0006-date.sh

So that would agree with what gix-date's test suite is asserting, while disagreeing with the current gix-date behavior. It seems to me that this may be a reason to consider our existing test in gix-date correct, and the current behavior of the gix-date being tested as incorrect.

Steps to reproduce 🕹

Although the tests don't fail in my time zone anymore, because 2 weeks ago is now after the DST adjustment, there are a few approaches that may reproduce it.

One is to use a tool like faketime. This carries some subtleties, though, since accesses to date and time information are not always guaranteed to go through the functions it patches, and since cursory experimentation with faketime and the date command suggests that locale-sensitive interpretation of strings like EST and EDT as time zone offsets does not always behave the way it did at the date and time faketime attempts to fake it to.

Another is to set the clock back. This will carry fewer subtleties but may be undesirable.

Instead, assuming one's time zone has DST adjustments, I suggest reproducing this by modifying the interval in the test from 2 weeks ago to an interval that refers to a date where local time used a different UTC offset than currently.

5 months ago would be nearly ideal, but cannot be used because we don't yet parse months or years, so that would give InvalidDateString { input: "5 months ago" }. (The absence of parsing of months or years might be considered a bug, but it is not this bug.)

A sufficient number of weeks will work, though. For example, starting in the top-level gitoxide directory, the interval can be changed from 2 to 20 weeks, which is approximately 5 months and should produce the bug at most (though not all) times of the year in most time zones where DST adjustments are made:

cd gix-date
sed -i.orig 's/two_weeks_ago/twenty_weeks_ago/; s/2\.weeks/20.weeks/; s/2 weeks ago/20 weeks ago/' tests/time/parse.rs
git --no-pager diff
cargo nextest run time::parse::relative::various

Edit: If #1697 is merged then the procedure here with sed -i will no longer be correct or needed, and the bug will automatically be observable at any time of year if it could be observed before at some time of year.

Running those commands on the test system produces the bug as intended:

ek in 🌐 catenary in gitoxide on  main is 📦 v0.38.0 via 🦀 v1.82.0
❯ cd gix-date

ek in 🌐 catenary in gitoxide/gix-date on  main is 📦 v0.9.1 via 🦀 v1.82.0
❯ sed -i.orig 's/two_weeks_ago/twenty_weeks_ago/; s/2\.weeks/20.weeks/; s/2 weeks ago/20 weeks ago/' tests/time/parse.rs

ek in 🌐 catenary in gitoxide/gix-date on  main [!?] is 📦 v0.9.1 via 🦀 v1.82.0
❯ git --no-pager diff
diff --git a/gix-date/tests/time/parse.rs b/gix-date/tests/time/parse.rs
index 545fbb7fc..8b0475c1d 100644
--- a/gix-date/tests/time/parse.rs
+++ b/gix-date/tests/time/parse.rs
@@ -179,9 +179,9 @@ mod relative {
     #[test]
     fn various() {
         let now = SystemTime::now();
-        let two_weeks_ago = gix_date::parse("2 weeks ago", Some(now)).unwrap();
-        assert_eq!(Sign::Plus, two_weeks_ago.sign);
-        assert_eq!(0, two_weeks_ago.offset);
+        let twenty_weeks_ago = gix_date::parse("20 weeks ago", Some(now)).unwrap();
+        assert_eq!(Sign::Plus, twenty_weeks_ago.sign);
+        assert_eq!(0, twenty_weeks_ago.offset);
         let expected = Zoned::try_from(now)
             .unwrap()
             // account for the loss of precision when creating `Time` with seconds
@@ -191,9 +191,9 @@ mod relative {
                     .mode(jiff::RoundMode::Trunc),
             )
             .unwrap()
-            .saturating_sub(2.weeks());
+            .saturating_sub(20.weeks());
         assert_eq!(
-            jiff::Timestamp::from_second(two_weeks_ago.seconds).unwrap(),
+            jiff::Timestamp::from_second(twenty_weeks_ago.seconds).unwrap(),
             expected.timestamp(),
             "relative times differ"
         );

ek in 🌐 catenary in gitoxide/gix-date on  main [!?] is 📦 v0.9.1 via 🦀 v1.82.0
❯ cargo nextest run time::parse::relative::various
   Compiling gix-date v0.9.1 (/home/ek/repos/gitoxide/gix-date)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.92s
------------
 Nextest run ID 55a072b6-40a8-4bd9-b177-58e4158ce1fe with nextest profile: default
    Starting 1 test across 2 binaries (28 tests skipped)
        FAIL [   0.014s] gix-date::date time::parse::relative::various

--- STDOUT:              gix-date::date time::parse::relative::various ---

running 1 test
test time::parse::relative::various ... FAILED

failures:

failures:
    time::parse::relative::various

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 27 filtered out; finished in 0.01s


--- STDERR:              gix-date::date time::parse::relative::various ---
thread 'time::parse::relative::various' panicked at gix-date/tests/time/parse.rs:195:9:
assertion `left == right` failed: relative times differ
  left: 2024-07-05T18:55:17Z
 right: 2024-07-05T17:55:17Z
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

  Cancelling due to test failure
------------
     Summary [   0.015s] 1 test run: 0 passed, 1 failed, 28 skipped
        FAIL [   0.014s] gix-date::date time::parse::relative::various
error: test run failed

Although less reproducible, it may be more intuitive to use a shorter interval. At the moment, 3 weeks is sufficient. A run changing 2 weeks to 3 (rather than 20) weeks can be seen here.

[Byron]

Thanks so much for the detailed report.

I understand that Git interprets 2 weeks ago as the amount of seconds passed, whereas gix-date thanks to jiff now considers it to be 14 calendar days passed, which correctly incorporates the local time as affected by the TZ environment variable.

The dependence on the TZ environment variable was surprising to me and usually gitoxide crates make that explicit and/or overridable, but the way it's currently implemented here (or how jiff is used) makes this implicit.

Is it correct to say that in theory, if that time computation would be using UTC always without implicitly picking up the local time, it should work as expected?
If that's true, then using a jiff API that doesn't rely on the system timezone should constitute a fix.

Let me CC @BurntSushi here just so that he is informed, and might be able to share ideas on how to deal with this as well.

On another note

5 months ago would be nearly ideal, but cannot be used because we don't yet parse months or years, so that would give InvalidDateString { input: "5 months ago" }. (The absence of parsing of months or years might be considered a bug, but it is not this bug.)

I wasn't aware actually, and am surprised this is the case as it should be trivial to support. But maybe it wasn't done yet as with time, nothing truly is trivial 😅.

[EliahKagan]

Let me CC @BurntSushi here just so that he is informed, and might be able to share ideas on how to deal with this as well.

Good idea. #1474 seems relevant, and especially this part, which I should've mentioned in the issue description, though I don't feel that I have a good handle on what ought to be done:

gitoxide/gix-date/src/parse.rs

Lines 134 to 144 in 438ee47

	now.ok_or(Error::MissingCurrentTime).and_then(|now| {
	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
	let zdt = ts.to_zoned(TimeZone::UTC);
	zdt.checked_sub(span).map_err(|_| Error::RelativeTimeConversion)
	})

This is discussed at #1474 (comment), which I hadn't noticed before.

---

Some other discussion in #1474 is relevant to:

5 months ago would be nearly ideal, but cannot be used because we don't yet parse months or years [...]

I wasn't aware actually, and am surprised this is the case as it should be trivial to support. But maybe it wasn't done yet as with time, nothing truly is trivial 😅.

Per #1474 (comment), it looks like parsing months and years just never ended up being added, but that it is expected to work since #1474.

[Byron]

Thanks for digging that out!

The code example seems to show that it already attempts to be timezone independent, but it might not be working as expected? Something there must read TZ even though it shouldn't?
It feels we are onto something here.

[BurntSushi]

Is it correct to say that in theory, if that time computation would be using UTC always without implicitly picking up the local time, it should work as expected?

Yes. In the UTC "time zone," there are zero time zone transitions. So days are always 24 hours.

The code example seems to show that it already attempts to be timezone independent, but it might not be working as expected? Something there must read TZ even though it shouldn't?

It looks like it's the test itself?

https://github.com/BurntSushi/gitoxide.git/blob/cb3149fec63dc9e366baf0399040d20161616b22/gix-date/tests/time/parse.rs#L202-L214

That Zoned::try_from will automatically use the system time zone.

I wasn't aware actually, and am surprised this is the case as it should be trivial to support. But maybe it wasn't done yet as with time, nothing truly is trivial 😅.

When I worked on the patch to add Jiff support, I didn't do months/years because that wasn't supported before. So I didn't want to add anything more than needed. But yes, Jiff should handle it fine. Jiff should make it trivial (assuming you give it a relative date, which I believe you do), but lots of things get this wrong. For example:

https://github.com/tailhook/humantime/blob/12ce6f50894a56a410b390e5608ac9db8afe2407/src/duration.rs#L305-L307

[BurntSushi]

If the goal is to match git's behavior here, then yeah, I think you need to interpret 2 weeks ago as a fixed amount of time that never changes due to time zone transitions. I would personally argue that git is wrong here, or at least, git's behavior is questionable.

[Byron]

Thanks so much for chiming in!

And thanks for the hint about the test - it seems the code itself uses UTC, so it will produce 'flat' time, but the test asserts with timezoned time. A fix should thus be in the test itself, it should use UTC, at least if it wants to match Git.

I would personally argue that git is wrong here, or at least, git's behavior is questionable.

Probably the way these relative dates are used in is so fuzzy that humans also don't have a clue if it's right or wrong, so nobody filed a bug-report for this yet.
Something I'd be afraid of is people comparing both implementations, and mismatches will then be reported here. That seems like a good reason to match Git even though one could be more correct.

When I worked on the patch to add Jiff support, I didn't do months/years because that wasn't supported before. So I didn't want to add anything more than needed.

Yes, I remember that, but don't remember who did what and why to which extend. This is to say I my note wasn't meant as criticism, just genuine surprise.

In any case, I think now we seem to know how to proceed here, which could be…

• fix the test to not rely on the system timezone
• (optionally) add the remaining conversions for months and years for completeness

And that should fix this issue if I am not missing something.

[BurntSushi]

That sounds right to me!

[EliahKagan]

• (optionally) add the remaining conversions for months and years for completeness

I've done just this part in #1702, since it seems like it's been wanted for some time and there's no strong reason to put it off. That PR does not fix this issue, however. It actually exacerbates it, in that the ambiguity described in this issue will apply not just to "days" and "weeks" but also to the newly recognized "months" and "years". However, the changes to the test that seem natural to do alongside recognizing those units (and that test that they are recognized) also make the contours of this issue clearer.