Skip to content

gix-date: accept a timezone offset after a short compact ISO8601 time - #2985

Merged
Sebastian Thiel (Byron) merged 2 commits into
GitoxideLabs:mainfrom
youdie006:fix-short-time-offset-split
Sep 9, 2026
Merged

gix-date: accept a timezone offset after a short compact ISO8601 time#2985
Sebastian Thiel (Byron) merged 2 commits into
GitoxideLabs:mainfrom
youdie006:fix-short-time-offset-split

Conversation

@youdie006

Copy link
Copy Markdown
Contributor

Disclosure

This change was prepared and verified by an AI agent (Claude, operating through Claude Code) running under my account, per CONTRIBUTING.md "Prevent agent impersonation". Everything below - the diff, the tests, the mutation table and the differential probe - was produced and run by the agent on my machine. I will answer review questions myself.

The bug

parse_compact_iso8601 documents four accepted time shapes, each "With optional timezone" (gix-date/src/parse/git.rs:41-47):

/// Parse compact ISO8601 formats:
/// - `20080214T203045` (compact time)
/// - `20080214T20:30:45` (normal time)
/// - `20080214T2030` (hours and minutes only)
/// - `20080214T20` (hours only)
/// - With optional subsecond precision (ignored)
/// - With optional timezone

The last two are false today. split_time_and_offset only recognises a sign as the start of an offset when it sits at index 5 or later (gix-date/src/parse/git.rs:165-170):

    // Look for + or - that indicates timezone (not part of time)
    // Time format is HH:MM:SS or HHMMSS, so offset starts after that
    // Find the last + or - that's after position 5 (minimum for HH:MM)
    let mut offset_start = None;
    for (i, c) in input.char_indices().rev() {
        if (c == '+' || c == '-') && i >= 5 {

HH:MM is five characters, so the sign lands at index 5 and passes. But HHMM puts the sign at index 4, and HH puts it at index 2. In both cases no offset is found, the sign and the offset digits stay glued to the time, parse_time_component cannot parse the result, and gix_date::parse returns an error:

input at main with this change
20080214T20:30-04:00 Ok Ok (unchanged)
20080214T2030-04:00 Err Ok(1203035400, -14400)
20080214T2030-0400 Err Ok(1203035400, -14400)
20080214T20-0400 Err Ok(1203033600, -14400)

Why 2

The comment above the loop derives the bound from HH:MM, but the function it feeds accepts a shorter time than that. parse_time_component already handles a bare two-character hour (gix-date/src/parse/git.rs:197-219):

/// Parse time component: HH:MM:SS, HHMMSS, HH:MM, HHMM, or HH
...
        // Compact: HHMMSS, HHMM, or HH
        match time.len() {
            2 => {
                let hour: u32 = time.parse().ok()?;
                Some((hour, 0, 0))
            }

So the splitter's bound assumes a time of at least five characters while the consumer it feeds accepts times of length 2 and 4. Two characters is the shortest time this function can produce, so it is the earliest index at which an offset can legitimately begin.

The index-based bound also makes the result depend on whitespace rather than on the value. At main:

20080214T2030 -0400  ->  Ok(Time { seconds: 1203035400, offset: -14400 })
20080214T2030-0400   ->  Err

Same time, same offset; the space is what pushes the sign to index 5. With this change both return Ok(1203035400, -14400).

Who this reaches

gix::repository::identity::Personas::from_config_and_env parses gitoxide.commit.authorDate and gitoxide.commit.committerDate and falls back on any parse error (gix/src/repository/identity.rs:153-155):

                        .and_then(|date| gix_date::parse(date, Some(gix_date::Zoned::now())).ok())
                })
                .or_else(|| Some(gix_date::Time::now_local_or_utc()))

So gitoxide.commit.authorDate = 20080214T2030-0400 does not produce an error - it silently records the current time on the commit instead of the requested one. The same string is also reachable through config::tree::keys::Time::try_into_time (gix/src/config/tree/keys.rs:495) and config::tree::keys::validate::Time::validate (gix/src/config/tree/keys.rs:589), where it surfaces as a spurious validation failure.

The change

     // Look for + or - that indicates timezone (not part of time)
-    // Time format is HH:MM:SS or HHMMSS, so offset starts after that
-    // Find the last + or - that's after position 5 (minimum for HH:MM)
+    // The time never contains a sign, so an offset starts no earlier than after `HH`.
     let mut offset_start = None;
     for (i, c) in input.char_indices().rev() {
-        if (c == '+' || c == '-') && i >= 5 {
+        if (c == '+' || c == '-') && i >= 2 {

Four tests were added to the existing gix-date/tests/time/parse/compact_iso8601.rs.

Verification

cargo test -p gix-date -p gix-hash -F gix-hash/sha1, with the source file touched before every build so nothing is served from cache. src_md5 is the md5 of gix-date/src/parse/git.rs as compiled.

run bound src_md5 exit failing tests
main + new tests (red) i >= 5 f19749aa734948882d3a89c377efec83 101 all four new tests (56 passed / 4 failed)
this PR (green) i >= 2 7d529b30acdc39b7c61e613419cfc149 0 none (60 passed)
revert the fix i >= 5 c2d8505154ef860afb20a81ead9a1a37 101 all four (56 / 4)
over-correct by one i >= 3 eac36a8dc539ebfedb116e2cd6c7bc32 101 hour_only_with_timezone only (59 / 1)
under-correct by one i >= 1 35fa2ee27c56962414881ee44f159234 0 none - survives
under-correct by two i >= 0 0488b713344c809439c4b890de0b51ca 0 none - survives

The two surviving mutants deserve an explanation rather than a shrug, so I checked them against a generated corpus instead of guessing. A temporary integration test dumped gix_date::parse(input, None) for 9,108 inputs (8,592 distinct) built from the cross product of four date prefixes (20080214T, 2008-02-14T, 2008-02-14 , 2008.02.14T), 23 time shapes (empty, 2, 20, 203, 2030, 20304, 203045, the colon forms, subsecond forms, and a few malformed ones), nine sign forms and eleven offset forms. The file was deleted afterwards and is not part of this PR.

  • i >= 1 produces byte-identical output to this PR on all 9,108 inputs (dd46a7b3eb1eb06e13172f3fa8c8fe52 for both). It is an equivalent mutant: a sign at index 1 leaves a one-character time, which neither branch of parse_time_component can parse, so both bounds return None. The empty failing set is expected here, not a coverage gap.
  • i >= 0 is not equivalent - it changes 45 of the 9,108 outputs. Every one of them has no time component at all (20080214T+04:00, 2008-02-14 +4, and so on), which main currently reads as the time +04:00 and answers 04:00:00 at offset 0. Turning those into None may well be an improvement, but it is a separate behaviour change and I deliberately left it out of scope. That is the concrete reason the bound is 2 and not 0.
  • For reference, the fix itself changes 231 of the 9,108 outputs relative to main, and i >= 3 changes 63.

Gates, all from a forced rebuild:

  • cargo +nightly fmt --all --check - exit 0
  • cargo clippy -p gix-date --lib -- -D warnings -A unknown-lints - exit 0
  • cargo test -p gix-date -p gix-hash -F gix-hash/sha1 - exit 0, 60 + 58 + 2 + 1 + 1 passed

Two notes on what I could not check: just is not installed here, so I ran the crate's tests directly rather than through just test; and a full-workspace clippy --all-targets fails on gix-tempfile (collapsible_if) both with and without this change, so I scoped the lint run to the crate I touched.

One divergence worth knowing about

With this change 20080214T20-04:00 (hour-only time, colon offset) returns (1203033600, -14400), which is the sane reading. Real git 2.43.0 returns 1208617200 for that string, i.e. it does not agree. The four tests I added deliberately use only forms where git and this parser agree, so nothing here pins gitoxide to the divergent case. Happy to restrict the bound further if matching git byte-for-byte on that input matters more than the documented behaviour.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-09T07:54:57.891017Z 7c5ae10 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7c5ae10483

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".

Comment thread gix-date/tests/time/parse/compact_iso8601.rs Outdated
KBS (youdie006) and others added 2 commits September 9, 2026 17:25
parse_compact_iso8601 documents `20080214T2030` and `20080214T20` as
supported "With optional timezone", but split_time_and_offset only treats
a sign as the start of an offset from index 5 onwards, which assumes a
time of at least `HH:MM`. parse_time_component accepts times of length 2
and 4 as well, so `20080214T2030-0400` and `20080214T20-0400` failed to
parse while `20080214T20:30-04:00` succeeded.

Lower the bound to 2, the length of the shortest time component this
function can produce.
@Byron

Copy link
Copy Markdown
Member

Good catch!

@Byron
Sebastian Thiel (Byron) merged commit 4ec7c08 into GitoxideLabs:main Sep 9, 2026
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants