# [PATCH 0/3] refs/reftable: fix on-disk representation of reflog timezones 16 messages from 2026-09-29 to 2026-10-01. Participants: Patrick Steinhardt, Karthik Nayak, Junio C Hamano. Thread: https://gitlist.dev/t/66419 ## Patrick Steinhardt, 2026-09-29 09:56 Subject: [PATCH 0/3] refs/reftable: fix on-disk representation of reflog timezones Message-ID: <20260929-pks-reftables-fix-timezone-format-v1-0-3df105a95ed1@pks.im> ``` Hi, it was reported [1] that the way we store reflog timezones with the reftable format has a mismatch with the reftable specification. While the spec says that reftables should be stored as a signed offset in minutes, we store them in the "[+-]HHMM" format that we typically use in commit headers, for example. This patch series fixes this bug by making our on-disk representation match the specification. This will of course make us reinterpret old reftables. But ultimately, the fallout caused by this change is somewhat limited as we only ever use reflog timezones for display purposes. So yes, we'll display a wrong timezone. But it's not used as part of any kind of computations. The series is built on top of v2.56.0. Thanks! Patrick [1]: <85f7daa8-d60b-4348-ac2f-b1a68628af7b@app.fastmail.com> --- Patrick Steinhardt (3): date: add helpers to convert between "+HHMM" timezones and minutes t/helper: fix segfault in "dump-reftable -t" refs/reftable: fix on-disk representation of reflog timezones apply.c | 3 ++- date.c | 25 +++++++++++++++++-------- date.h | 9 +++++++++ refs/reftable-backend.c | 7 ++++--- strbuf.c | 3 +-- t/helper/test-reftable.c | 13 +++++++++++-- t/t0610-reftable-basics.sh | 35 +++++++++++++++++++++++++++++++++++ 7 files changed, 79 insertions(+), 16 deletions(-) --- base-commit: a018953688f1b10bddf91bff8747068f5f4746a4 change-id: 20260929-pks-reftables-fix-timezone-format-93ecd0e7a1d1 ``` ## Patrick Steinhardt, 2026-09-29 09:56 Subject: [PATCH 1/3] date: add helpers to convert between "+HHMM" timezones and minutes Message-ID: <20260929-pks-reftables-fix-timezone-format-v1-1-3df105a95ed1@pks.im> In-Reply-To: <20260929-pks-reftables-fix-timezone-format-v1-0-3df105a95ed1@pks.im> ``` The timezones that we store in commits as part of the identity information are encoded in "[+-]HHMM", for example "-0700" for UTC-7. Internally we typically pass around this timezone either as string or as a parsed integer (-700). Some sites want to convert between this format and minutes or vice versa, and that conversion is performed ad-hoc. We're about to introduce another site though that wants to have access to this logic, and having it cluttered across our codebase is a bit awkward. Introduce two new helpers `tz_to_minutes()` and `minutes_to_tz()` that perform the conversion for us and convert call sites to use them. Note that we used to perform a dance in `gm_time_t()` where we first convert `tz` into a positive value, then calculate the minutes, and finally turn the minutes into a negative value again. This dance is performed because it is implementation-defined in C89 whether the division on negative values truncates towards zero or not [1]: If either operand is negative, whether the result of the / operator is the largest integer less than the algebraic quotient or the smallest integer greater than the algebraic quotient is implementation-defined, as is the sign of the result of the % operator. So under C89, `-130 / 100` could legitimately result in -1 or -2, and `-130 % 100` could result in either -30 or 70. For us though, the result that we want is the first one (-1 and -30), which is called truncation toward zero. This part of the C language has changed in C99, where this edge case is now well-defined to always truncate towards zero [2]: When integers are divided, the result of the / operator is the algebraic quotient with any fractional part discarded.90) If the quotient a/b is representable, the expression (a/b)*b + a%b shall equal a. 90) This is often called ''truncation toward zero''. So in theory it's unlikely that we still need this logic. In practice though it feels safer to just retain it as we don't require a fully C99-compliant compiler in Git. [1]: https://port70.net/~nsz/c/c89/c89-draft.html#3.3.5 [2]: https://port70.net/~nsz/c/c99/n1256.html#6.5.5p6 Signed-off-by: Patrick Steinhardt --- apply.c | 3 ++- date.c | 25 +++++++++++++++++-------- date.h | 9 +++++++++ strbuf.c | 3 +-- 4 files changed, 29 insertions(+), 11 deletions(-) diff --git a/apply.c b/apply.c index f00b7ba4d3..367271b8ac 100644 --- a/apply.c +++ b/apply.c @@ -14,6 +14,7 @@ #include "abspath.h" #include "base85.h" #include "config.h" +#include "date.h" #include "odb.h" #include "delta.h" #include "diff.h" @@ -851,7 +852,7 @@ static int has_epoch_timestamp(const char *nameline) if (*colon == ':') zoneoffset = zoneoffset * 60 + strtol(colon + 1, NULL, 10); else - zoneoffset = (zoneoffset / 100) * 60 + (zoneoffset % 100); + zoneoffset = tz_to_minutes(zoneoffset); if (timestamp[m[3].rm_so] == '-') zoneoffset = -zoneoffset; diff --git a/date.c b/date.c index 014065b419..63ea9dbc76 100644 --- a/date.c +++ b/date.c @@ -45,13 +45,23 @@ static const char *weekday_names[] = { "Sundays", "Mondays", "Tuesdays", "Wednesdays", "Thursdays", "Fridays", "Saturdays" }; -static time_t gm_time_t(timestamp_t time, int tz) +int tz_to_minutes(int tz) { - int minutes; + int minutes = tz < 0 ? -tz : tz; + minutes = (minutes / 100) * 60 + (minutes % 100); + return tz < 0 ? -minutes : minutes; +} - minutes = tz < 0 ? -tz : tz; - minutes = (minutes / 100)*60 + (minutes % 100); - minutes = tz < 0 ? -minutes : minutes; +int minutes_to_tz(int minutes) +{ + int tz = minutes < 0 ? -minutes : minutes; + tz = (tz / 60) * 100 + (tz % 60); + return minutes < 0 ? -tz : tz; +} + +static time_t gm_time_t(timestamp_t time, int tz) +{ + int minutes = tz_to_minutes(tz); if (minutes > 0) { if (unsigned_add_overflows(time, minutes * 60)) @@ -103,8 +113,7 @@ static int local_time_tzoffset(time_t t, struct tm *tm) offset = t_local - t; } offset /= 60; /* in minutes */ - offset = (offset % 60) + ((offset / 60) * 100); - return offset * eastwest; + return minutes_to_tz(offset * eastwest); } /* @@ -862,7 +871,7 @@ static int match_object_header_date(const char *date, timestamp_t *timestamp, in ofs = strtol(date, &end, 10); if ((*end != '\0' && (*end != '\n')) || end != date + 4) return -1; - ofs = (ofs / 100) * 60 + (ofs % 100); + ofs = tz_to_minutes(ofs); if (date[-1] == '-') ofs = -ofs; *timestamp = stamp; diff --git a/date.h b/date.h index 0747864fd7..816df5b833 100644 --- a/date.h +++ b/date.h @@ -70,4 +70,13 @@ void datestamp(struct strbuf *out); timestamp_t approxidate_careful(const char *, int *); int date_overflows(timestamp_t date); time_t tm_to_time_t(const struct tm *tm); + +/** + * Convert between the "[+-]HHMM" timezone format and minutes. This format is + * used for example as part of commit headers and reflogs. For example, the + * timezone -0100 is converted to -60 minutes. + */ +int tz_to_minutes(int tz); +int minutes_to_tz(int minutes); + #endif diff --git a/strbuf.c b/strbuf.c index 44955669e8..c3baa47b3f 100644 --- a/strbuf.c +++ b/strbuf.c @@ -1023,8 +1023,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm, else if (skip_prefix(fmt, "s", &fmt)) strbuf_addf(&munged_fmt, "%"PRItime, (timestamp_t)tm_to_time_t(tm) - - 3600 * (tz_offset / 100) - - 60 * (tz_offset % 100)); + 60 * tz_to_minutes(tz_offset)); else if (skip_prefix(fmt, "z", &fmt)) strbuf_addf(&munged_fmt, "%+05d", tz_offset); else if (suppress_tz_name && skip_prefix(fmt, "Z", &fmt)) -- 2.56.0.rc2.329.gd58861e689.dirty ``` ## Patrick Steinhardt, 2026-09-29 09:56 Subject: [PATCH 2/3] t/helper: fix segfault in "dump-reftable -t" Message-ID: <20260929-pks-reftables-fix-timezone-format-v1-2-3df105a95ed1@pks.im> In-Reply-To: <20260929-pks-reftables-fix-timezone-format-v1-0-3df105a95ed1@pks.im> ``` The `test-tool dump-reftable` command can be used to dump the on-disk contents of reftables. The "-t" subcommand specifically can be used to dump a single table from disk. When trying to use this subcommand though one will quickly realize that it is broken, as it always segfaults. The root cause of this segfault is that we try to detect the hash algorithm via the merged table's hash ID. But that hash ID is not the same as Git's understanding of a hash ID, and consequently we fail to look up the correct algorithm. This will then lead to a segfault later on when we try to dereference a NULL pointer. This breakage went undetected until now because this particular subcommand is not used anywhere in our test suite. So the obvious way to fix the bug is by just removing the code outright. But in the next commit we're about to add a user. Fix the issue by properly converting between the two hash IDs. Signed-off-by: Patrick Steinhardt --- t/helper/test-reftable.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c index fc49fafc34..57758936b0 100644 --- a/t/helper/test-reftable.c +++ b/t/helper/test-reftable.c @@ -103,7 +103,16 @@ static int dump_table(struct reftable_merged_table *mt) if (err < 0) return err; - algop = &hash_algos[hash_algo_by_id(reftable_merged_table_hash_id(mt))]; + switch (reftable_merged_table_hash_id(mt)) { + case REFTABLE_HASH_SHA1: + algop = &hash_algos[GIT_HASH_SHA1]; + break; + case REFTABLE_HASH_SHA256: + algop = &hash_algos[GIT_HASH_SHA256]; + break; + default: + die("unknown reftable hash function: %d", reftable_merged_table_hash_id(mt)); + } while (1) { err = reftable_iterator_next_ref(&it, &ref); -- 2.56.0.rc2.329.gd58861e689.dirty ``` ## Patrick Steinhardt, 2026-09-29 09:56 Subject: [PATCH 3/3] refs/reftable: fix on-disk representation of reflog timezones Message-ID: <20260929-pks-reftables-fix-timezone-format-v1-3-3df105a95ed1@pks.im> In-Reply-To: <20260929-pks-reftables-fix-timezone-format-v1-0-3df105a95ed1@pks.im> ``` When writing reflog entries to disk we also record authorship information for the reflog. Besides the author name and mail address, it also contains the date and timezone at which the record has been created. The timezone information is typically encoded in the "[+-]HHMM" format, and we often pass it around as parsed integer. For example, the timezone "-0700" would be passed around as -700. And this is also the value that we eventually store in the reftable on disk. But the specification in "Documentation/technical/reftable.adoc" notes that the timezone is a "2-byte timezone offset in minutes (signed)". So instead of storing -700 in the above example, we have to first convert that value into minutes and then store -420. We don't though, so we have a mismatch between specification and implementation. Ideally, we'd just adapt the specification to match the implementation. But that's easier said than done, because the specification is 11 years old by now and reftables have already been implemented by JGit for a long time. So if we now changed the specification, those libraries would have to make a backwards-incompatible change. Another alternative would be to bump the reftable format version, but that feels suboptimal, too. Other libraries would all have to adapt, and it wouldn't really help us to fix the discrepancy between alternative implementations and our implementation as older versions would still be misinterpreted. The only viable option seems to be that we simply treat this as a bug and fix it. This will of course make us misinterpret older reftables that already exist on disk: ┌───────┬───────────────┬─────────────────┬────────────┐ │ tz │ HHMM encoding │ correct minutes │ divergence │ ├───────┼───────────────┼─────────────────┼────────────┤ │ +1400 │ 1400 │ 840 │ 560 │ ├───────┼───────────────┼─────────────────┼────────────┤ │ -1200 │ -1200 │ -720 │ 480 │ ├───────┼───────────────┼─────────────────┼────────────┤ │ +0530 │ 530 │ 330 │ 200 │ ├───────┼───────────────┼─────────────────┼────────────┤ │ +0000 │ 0 │ 0 │ 0 │ └───────┴───────────────┴─────────────────┴────────────┘ But this divergence ultimately doesn't matter much, as Git only uses the timezone of reflog entries for display purposes anyway. We don't take the timezone into account when parsing "HEAD@{1.hour.ago}" syntax, and `should_expire_reflog_ent()` doesn't use it either to decide whether reflog entries should be pruned. In summary, the fallout from this change is quite contained. Adapt the reftable backend accordingly and simply reinterpret the timezones with the specified meaning. Add a test to verify that we properly encode the timezone as offset in minutes. Adapt the test helper accordingly to no longer zero-pad the offset with "%04d", as that can be easily misinterpreted as the "HHMM" encoding. Reported-by: Josh McKinney Signed-off-by: Patrick Steinhardt --- refs/reftable-backend.c | 7 ++++--- t/helper/test-reftable.c | 2 +- t/t0610-reftable-basics.sh | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 4 deletions(-) diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 10db03991e..d0de066355 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -2,6 +2,7 @@ #include "../abspath.h" #include "../chdir-notify.h" #include "../config.h" +#include "../date.h" #include "../dir.h" #include "../environment.h" #include "../fsck.h" @@ -317,7 +318,7 @@ static void fill_reftable_log_record(struct reftable_log_record *log, const stru tz_begin++; } - log->value.update.tz_offset = sign * atoi(tz_begin); + log->value.update.tz_offset = tz_to_minutes(sign * atoi(tz_begin)); } static int reftable_be_config(const char *var, const char *value, @@ -2186,7 +2187,7 @@ static int yield_log_record(struct reftable_ref_store *refs, full_committer = fmt_ident(log->value.update.name, log->value.update.email, WANT_COMMITTER_IDENT, NULL, IDENT_NO_DATE); return fn(log->refname, &old_oid, &new_oid, full_committer, - log->value.update.time, log->value.update.tz_offset, + log->value.update.time, minutes_to_tz(log->value.update.tz_offset), log->value.update.message, cb_data); } @@ -2690,7 +2691,7 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store, if (should_prune_fn(&old_oid, &new_oid, logs[i].value.update.email, (timestamp_t)logs[i].value.update.time, - logs[i].value.update.tz_offset, + minutes_to_tz(logs[i].value.update.tz_offset), logs[i].value.update.message, policy_cb_data)) { dest->value_type = REFTABLE_LOG_DELETION; diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c index 57758936b0..d9f2ca1d0e 100644 --- a/t/helper/test-reftable.c +++ b/t/helper/test-reftable.c @@ -163,7 +163,7 @@ static int dump_table(struct reftable_merged_table *mt) log.update_index); break; case REFTABLE_LOG_UPDATE: - printf("log{%s(%" PRIu64 ") %s <%s> %" PRIu64 " %04d\n", + printf("log{%s(%" PRIu64 ") %s <%s> %" PRIu64 " %d\n", log.refname, log.update_index, log.value.update.name ? log.value.update.name : "", log.value.update.email ? log.value.update.email : "", diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh index 35e98b43db..579657467d 100755 --- a/t/t0610-reftable-basics.sh +++ b/t/t0610-reftable-basics.sh @@ -837,6 +837,41 @@ test_expect_success 'reflog: renaming branch writes reflog entry' ' ) ' +test_expect_success 'reflog: timezone offset is stored in minutes' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + GIT_COMMITTER_DATE="1234567890 -1200" git commit --allow-empty -m min && + GIT_COMMITTER_DATE="1234567890 +0530" git commit --allow-empty -m east && + GIT_COMMITTER_DATE="1234567890 -0800" git commit --allow-empty -m west && + GIT_COMMITTER_DATE="1234567890 +1400" git commit --allow-empty -m max && + + # The reftable format specifies the timezone as the offset from + # UTC in minutes, whereas Git uses the parsed form of "+HHMM" + # internally. Verify that we do the conversion when writing. + for table in .git/reftable/*.ref + do + test-tool dump-reftable -t "$table" || return 1 + done >dump && + sed -n "s/^log{refs\/heads\/main([0-9]*) .* 1234567890 //p" dump >actual && + cat >expect <<-\EOF && + 840 + -480 + 330 + -720 + EOF + test_cmp expect actual && + + # And verify that we convert back when reading. + test-tool ref-store main for-each-reflog-ent refs/heads/main >entries && + test_grep "1234567890 -1200 commit (initial): min" entries && + test_grep "1234567890 +0530 commit: east" entries && + test_grep "1234567890 -0800 commit: west" entries && + test_grep "1234567890 +1400 commit: max" entries + ) +' + test_expect_success 'reflog: can store empty logs' ' test_when_finished "rm -rf repo" && git init repo && -- 2.56.0.rc2.329.gd58861e689.dirty ``` ## Karthik Nayak, 2026-09-30 11:47 Subject: Re: [PATCH 1/3] date: add helpers to convert between "+HHMM" timezones and minutes Message-ID: In-Reply-To: <20260929-pks-reftables-fix-timezone-format-v1-1-3df105a95ed1@pks.im> ``` Patrick Steinhardt writes: > The timezones that we store in commits as part of the identity > information are encoded in "[+-]HHMM", for example "-0700" for UTC-7. > Internally we typically pass around this timezone either as string or as > a parsed integer (-700). > > Some sites want to convert between this format and minutes or vice > versa, and that conversion is performed ad-hoc. We're about to introduce > another site though that wants to have access to this logic, and having > it cluttered across our codebase is a bit awkward. > > Introduce two new helpers `tz_to_minutes()` and `minutes_to_tz()` that > perform the conversion for us and convert call sites to use them. > > Note that we used to perform a dance in `gm_time_t()` where we first > convert `tz` into a positive value, then calculate the minutes, and > finally turn the minutes into a negative value again. This dance is > performed because it is implementation-defined in C89 whether the > division on negative values truncates towards zero or not [1]: > > If either operand is negative, whether the result of the / operator is > the largest integer less than the algebraic quotient or the smallest > integer greater than the algebraic quotient is implementation-defined, > as is the sign of the result of the % operator. > > So under C89, `-130 / 100` could legitimately result in -1 or -2, and > `-130 % 100` could result in either -30 or 70. For us though, the result > that we want is the first one (-1 and -30), which is called truncation > toward zero. > We divide by '100' because we represent "-0700" as '-700' in integer. So we need to separate out the 'HH' from 'MM'. Okay. > This part of the C language has changed in C99, where this edge case is > now well-defined to always truncate towards zero [2]: > > When integers are divided, the result of the / operator is the > algebraic quotient with any fractional part discarded.90) If the > quotient a/b is representable, the expression (a/b)*b + a%b shall > equal a. > > 90) This is often called ''truncation toward zero''. > > So in theory it's unlikely that we still need this logic. In practice > though it feels safer to just retain it as we don't require a fully > C99-compliant compiler in Git. > > [1]: https://port70.net/~nsz/c/c89/c89-draft.html#3.3.5 > [2]: https://port70.net/~nsz/c/c99/n1256.html#6.5.5p6 > > Signed-off-by: Patrick Steinhardt > --- > apply.c | 3 ++- > date.c | 25 +++++++++++++++++-------- > date.h | 9 +++++++++ > strbuf.c | 3 +-- > 4 files changed, 29 insertions(+), 11 deletions(-) > > diff --git a/apply.c b/apply.c > index f00b7ba4d3..367271b8ac 100644 > --- a/apply.c > +++ b/apply.c > @@ -14,6 +14,7 @@ > #include "abspath.h" > #include "base85.h" > #include "config.h" > +#include "date.h" > #include "odb.h" > #include "delta.h" > #include "diff.h" > @@ -851,7 +852,7 @@ static int has_epoch_timestamp(const char *nameline) > if (*colon == ':') > zoneoffset = zoneoffset * 60 + strtol(colon + 1, NULL, 10); > else > - zoneoffset = (zoneoffset / 100) * 60 + (zoneoffset % 100); > + zoneoffset = tz_to_minutes(zoneoffset); > if (timestamp[m[3].rm_so] == '-') > zoneoffset = -zoneoffset; > > diff --git a/date.c b/date.c > index 014065b419..63ea9dbc76 100644 > --- a/date.c > +++ b/date.c > @@ -45,13 +45,23 @@ static const char *weekday_names[] = { > "Sundays", "Mondays", "Tuesdays", "Wednesdays", "Thursdays", "Fridays", "Saturdays" > }; > > -static time_t gm_time_t(timestamp_t time, int tz) > +int tz_to_minutes(int tz) > { > - int minutes; > + int minutes = tz < 0 ? -tz : tz; This is the part which we could skip as we're C99 compliant, but keeping to be on the safe side. > + minutes = (minutes / 100) * 60 + (minutes % 100); > + return tz < 0 ? -minutes : minutes; > +} > > - minutes = tz < 0 ? -tz : tz; > - minutes = (minutes / 100)*60 + (minutes % 100); > - minutes = tz < 0 ? -minutes : minutes; > +int minutes_to_tz(int minutes) > +{ > + int tz = minutes < 0 ? -minutes : minutes; > + tz = (tz / 60) * 100 + (tz % 60); > + return minutes < 0 ? -tz : tz; > +} > + > +static time_t gm_time_t(timestamp_t time, int tz) > +{ > + int minutes = tz_to_minutes(tz); > > if (minutes > 0) { > if (unsigned_add_overflows(time, minutes * 60)) > @@ -103,8 +113,7 @@ static int local_time_tzoffset(time_t t, struct tm *tm) > offset = t_local - t; > } > offset /= 60; /* in minutes */ > - offset = (offset % 60) + ((offset / 60) * 100); > - return offset * eastwest; > + return minutes_to_tz(offset * eastwest); While mathematically it's the same, but shouldn't this have been `minutes_to_tz(offset) * eastwest`? > } > > /* > @@ -862,7 +871,7 @@ static int match_object_header_date(const char *date, timestamp_t *timestamp, in > ofs = strtol(date, &end, 10); > if ((*end != '\0' && (*end != '\n')) || end != date + 4) > return -1; > - ofs = (ofs / 100) * 60 + (ofs % 100); > + ofs = tz_to_minutes(ofs); > if (date[-1] == '-') > ofs = -ofs; > *timestamp = stamp; > diff --git a/date.h b/date.h > index 0747864fd7..816df5b833 100644 > --- a/date.h > +++ b/date.h > @@ -70,4 +70,13 @@ void datestamp(struct strbuf *out); > timestamp_t approxidate_careful(const char *, int *); > int date_overflows(timestamp_t date); > time_t tm_to_time_t(const struct tm *tm); > + > +/** > + * Convert between the "[+-]HHMM" timezone format and minutes. This format is > + * used for example as part of commit headers and reflogs. For example, the > + * timezone -0100 is converted to -60 minutes. > + */ > +int tz_to_minutes(int tz); > +int minutes_to_tz(int minutes); > + > #endif > diff --git a/strbuf.c b/strbuf.c > index 44955669e8..c3baa47b3f 100644 > --- a/strbuf.c > +++ b/strbuf.c > @@ -1023,8 +1023,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm, > else if (skip_prefix(fmt, "s", &fmt)) > strbuf_addf(&munged_fmt, "%"PRItime, > (timestamp_t)tm_to_time_t(tm) - > - 3600 * (tz_offset / 100) - > - 60 * (tz_offset % 100)); > + 60 * tz_to_minutes(tz_offset)); > else if (skip_prefix(fmt, "z", &fmt)) > strbuf_addf(&munged_fmt, "%+05d", tz_offset); > else if (suppress_tz_name && skip_prefix(fmt, "Z", &fmt)) > > -- > 2.56.0.rc2.329.gd58861e689.dirty The rest looks good. ``` ## Karthik Nayak, 2026-09-30 11:51 Subject: Re: [PATCH 3/3] refs/reftable: fix on-disk representation of reflog timezones Message-ID: In-Reply-To: <20260929-pks-reftables-fix-timezone-format-v1-3-3df105a95ed1@pks.im> ``` Patrick Steinhardt writes: > When writing reflog entries to disk we also record authorship > information for the reflog. Besides the author name and mail address, > it also contains the date and timezone at which the record has been > created. > > The timezone information is typically encoded in the "[+-]HHMM" format, > and we often pass it around as parsed integer. For example, the timezone > "-0700" would be passed around as -700. And this is also the value that > we eventually store in the reftable on disk. > > But the specification in "Documentation/technical/reftable.adoc" notes > that the timezone is a "2-byte timezone offset in minutes (signed)". So > instead of storing -700 in the above example, we have to first convert > that value into minutes and then store -420. We don't though, so we have > a mismatch between specification and implementation. > > Ideally, we'd just adapt the specification to match the implementation. > But that's easier said than done, because the specification is 11 years > old by now and reftables have already been implemented by JGit for a > long time. So if we now changed the specification, those libraries would > have to make a backwards-incompatible change. > > Another alternative would be to bump the reftable format version, but > that feels suboptimal, too. Other libraries would all have to adapt, and > it wouldn't really help us to fix the discrepancy between alternative > implementations and our implementation as older versions would still be > misinterpreted. > > The only viable option seems to be that we simply treat this as a bug > and fix it. This will of course make us misinterpret older reftables > that already exist on disk: > > ┌───────┬───────────────┬─────────────────┬────────────┐ > │ tz │ HHMM encoding │ correct minutes │ divergence │ > ├───────┼───────────────┼─────────────────┼────────────┤ > │ +1400 │ 1400 │ 840 │ 560 │ > ├───────┼───────────────┼─────────────────┼────────────┤ > │ -1200 │ -1200 │ -720 │ 480 │ > ├───────┼───────────────┼─────────────────┼────────────┤ > │ +0530 │ 530 │ 330 │ 200 │ > ├───────┼───────────────┼─────────────────┼────────────┤ > │ +0000 │ 0 │ 0 │ 0 │ > └───────┴───────────────┴─────────────────┴────────────┘ > > But this divergence ultimately doesn't matter much, as Git only uses the > timezone of reflog entries for display purposes anyway. We don't take > the timezone into account when parsing "HEAD@{1.hour.ago}" syntax, and > `should_expire_reflog_ent()` doesn't use it either to decide whether > reflog entries should be pruned. > > In summary, the fallout from this change is quite contained. Adapt the > reftable backend accordingly and simply reinterpret the timezones with > the specified meaning. > > Add a test to verify that we properly encode the timezone as offset in > minutes. Adapt the test helper accordingly to no longer zero-pad the > offset with "%04d", as that can be easily misinterpreted as the "HHMM" > encoding. > > Reported-by: Josh McKinney > Signed-off-by: Patrick Steinhardt > --- > refs/reftable-backend.c | 7 ++++--- > t/helper/test-reftable.c | 2 +- > t/t0610-reftable-basics.sh | 35 +++++++++++++++++++++++++++++++++++ > 3 files changed, 40 insertions(+), 4 deletions(-) > > diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c > index 10db03991e..d0de066355 100644 > --- a/refs/reftable-backend.c > +++ b/refs/reftable-backend.c > @@ -2,6 +2,7 @@ > #include "../abspath.h" > #include "../chdir-notify.h" > #include "../config.h" > +#include "../date.h" > #include "../dir.h" > #include "../environment.h" > #include "../fsck.h" > @@ -317,7 +318,7 @@ static void fill_reftable_log_record(struct reftable_log_record *log, const stru > tz_begin++; > } > > - log->value.update.tz_offset = sign * atoi(tz_begin); > + log->value.update.tz_offset = tz_to_minutes(sign * atoi(tz_begin)); > } > > static int reftable_be_config(const char *var, const char *value, > @@ -2186,7 +2187,7 @@ static int yield_log_record(struct reftable_ref_store *refs, > full_committer = fmt_ident(log->value.update.name, log->value.update.email, > WANT_COMMITTER_IDENT, NULL, IDENT_NO_DATE); > return fn(log->refname, &old_oid, &new_oid, full_committer, > - log->value.update.time, log->value.update.tz_offset, > + log->value.update.time, minutes_to_tz(log->value.update.tz_offset), > log->value.update.message, cb_data); > } > > @@ -2690,7 +2691,7 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store, > > if (should_prune_fn(&old_oid, &new_oid, logs[i].value.update.email, > (timestamp_t)logs[i].value.update.time, > - logs[i].value.update.tz_offset, > + minutes_to_tz(logs[i].value.update.tz_offset), > logs[i].value.update.message, > policy_cb_data)) { > dest->value_type = REFTABLE_LOG_DELETION; > diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c > index 57758936b0..d9f2ca1d0e 100644 > --- a/t/helper/test-reftable.c > +++ b/t/helper/test-reftable.c > @@ -163,7 +163,7 @@ static int dump_table(struct reftable_merged_table *mt) > log.update_index); > break; > case REFTABLE_LOG_UPDATE: > - printf("log{%s(%" PRIu64 ") %s <%s> %" PRIu64 " %04d\n", > + printf("log{%s(%" PRIu64 ") %s <%s> %" PRIu64 " %d\n", > log.refname, log.update_index, > log.value.update.name ? log.value.update.name : "", > log.value.update.email ? log.value.update.email : "", > diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh > index 35e98b43db..579657467d 100755 > --- a/t/t0610-reftable-basics.sh > +++ b/t/t0610-reftable-basics.sh > @@ -837,6 +837,41 @@ test_expect_success 'reflog: renaming branch writes reflog entry' ' > ) > ' > > +test_expect_success 'reflog: timezone offset is stored in minutes' ' > + test_when_finished "rm -rf repo" && > + git init repo && > + ( > + cd repo && > + GIT_COMMITTER_DATE="1234567890 -1200" git commit --allow-empty -m min && > + GIT_COMMITTER_DATE="1234567890 +0530" git commit --allow-empty -m east && > + GIT_COMMITTER_DATE="1234567890 -0800" git commit --allow-empty -m west && > + GIT_COMMITTER_DATE="1234567890 +1400" git commit --allow-empty -m max && Nit: it would be nice to have a negative timezone with MM filled in too. > + # The reftable format specifies the timezone as the offset from > + # UTC in minutes, whereas Git uses the parsed form of "+HHMM" > + # internally. Verify that we do the conversion when writing. > + for table in .git/reftable/*.ref > + do > + test-tool dump-reftable -t "$table" || return 1 > + done >dump && > + sed -n "s/^log{refs\/heads\/main([0-9]*) .* 1234567890 //p" dump >actual && > + cat >expect <<-\EOF && > + 840 > + -480 > + 330 > + -720 > + EOF > + test_cmp expect actual && > + > + # And verify that we convert back when reading. > + test-tool ref-store main for-each-reflog-ent refs/heads/main >entries && > + test_grep "1234567890 -1200 commit (initial): min" entries && > + test_grep "1234567890 +0530 commit: east" entries && > + test_grep "1234567890 -0800 commit: west" entries && > + test_grep "1234567890 +1400 commit: max" entries > + ) > +' > + > test_expect_success 'reflog: can store empty logs' ' > test_when_finished "rm -rf repo" && > git init repo && > > -- > 2.56.0.rc2.329.gd58861e689.dirty Apart from the nit, the changes look good. ``` ## Patrick Steinhardt, 2026-09-30 12:05 Subject: Re: [PATCH 1/3] date: add helpers to convert between "+HHMM" timezones and minutes Message-ID: In-Reply-To: ``` On Wed, Sep 30, 2026 at 04:47:02AM -0700, Karthik Nayak wrote: One suggestion: I'd recommend trimming the mails you're responding to a bit more aggressively. Otherwise one is hunting for responses in files and hunks that are not relevant to your remarks :) > Patrick Steinhardt writes: > > diff --git a/date.c b/date.c > > index 014065b419..63ea9dbc76 100644 > > --- a/date.c > > +++ b/date.c > > @@ -103,8 +113,7 @@ static int local_time_tzoffset(time_t t, struct tm *tm) > > offset = t_local - t; > > } > > offset /= 60; /* in minutes */ > > - offset = (offset % 60) + ((offset / 60) * 100); > > - return offset * eastwest; > > + return minutes_to_tz(offset * eastwest); > > While mathematically it's the same, but shouldn't this have been > `minutes_to_tz(offset) * eastwest`? I guess we can. It's probably less confusing if we do it this way indeed. Patrick ``` ## Patrick Steinhardt, 2026-09-30 12:05 Subject: Re: [PATCH 3/3] refs/reftable: fix on-disk representation of reflog timezones Message-ID: In-Reply-To: ``` On Wed, Sep 30, 2026 at 04:51:15AM -0700, Karthik Nayak wrote: > Patrick Steinhardt writes: > > diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh > > index 35e98b43db..579657467d 100755 > > --- a/t/t0610-reftable-basics.sh > > +++ b/t/t0610-reftable-basics.sh > > @@ -837,6 +837,41 @@ test_expect_success 'reflog: renaming branch writes reflog entry' ' > > ) > > ' > > > > +test_expect_success 'reflog: timezone offset is stored in minutes' ' > > + test_when_finished "rm -rf repo" && > > + git init repo && > > + ( > > + cd repo && > > + GIT_COMMITTER_DATE="1234567890 -1200" git commit --allow-empty -m min && > > + GIT_COMMITTER_DATE="1234567890 +0530" git commit --allow-empty -m east && > > + GIT_COMMITTER_DATE="1234567890 -0800" git commit --allow-empty -m west && > > + GIT_COMMITTER_DATE="1234567890 +1400" git commit --allow-empty -m max && > > Nit: it would be nice to have a negative timezone with MM filled in too. Sure, can do. I'll just change -0800 to -0830. Thanks! Patrick ``` ## Junio C Hamano, 2026-09-30 19:54 Subject: Re: [PATCH 1/3] date: add helpers to convert between "+HHMM" timezones and minutes Message-ID: In-Reply-To: ``` Karthik Nayak writes: >> - int minutes; >> + int minutes = tz < 0 ? -tz : tz; > > This is the part which we could skip as we're C99 compliant, but keeping > to be on the safe side. > >> + minutes = (minutes / 100) * 60 + (minutes % 100); >> + return tz < 0 ? -minutes : minutes; >> +} I was wondering exactly the same thing yesterday. As written, it is clear even to those unfamiliar with the C89/C99 signed division rules, because we deal only with non-negative numbers, which is a plus. The fewer things readers need to worry about, the better. >> offset /= 60; /* in minutes */ >> - offset = (offset % 60) + ((offset / 60) * 100); >> - return offset * eastwest; >> + return minutes_to_tz(offset * eastwest); > > While mathematically it's the same, but shouldn't this have been > `minutes_to_tz(offset) * eastwest`? The way you suggest is more faithful rewrite of the original. Thanks. ``` ## Patrick Steinhardt, 2026-10-01 05:38 Subject: [PATCH v2 0/3] refs/reftable: fix on-disk representation of reflog timezones Message-ID: <20261001-pks-reftables-fix-timezone-format-v2-0-a4fd1f7cd21a@pks.im> In-Reply-To: <20260929-pks-reftables-fix-timezone-format-v1-0-3df105a95ed1@pks.im> ``` Hi, it was reported [1] that the way we store reflog timezones with the reftable format has a mismatch with the reftable specification. While the spec says that reftables should be stored as a signed offset in minutes, we store them in the "[+-]HHMM" format that we typically use in commit headers, for example. This patch series fixes this bug by making our on-disk representation match the specification. This will of course make us reinterpret old reftables. But ultimately, the fallout caused by this change is somewhat limited as we only ever use reflog timezones for display purposes. So yes, we'll display a wrong timezone. But it's not used as part of any kind of computations. The series is built on top of v2.56.0. Changes in v2: - Improve readability of one of the converted sites that now use `minutes_to_tz()`. - Improve test coverage. - Link to v1: https://patch.msgid.link/20260929-pks-reftables-fix-timezone-format-v1-0-3df105a95ed1@pks.im Thanks! Patrick [1]: <85f7daa8-d60b-4348-ac2f-b1a68628af7b@app.fastmail.com> --- Patrick Steinhardt (3): date: add helpers to convert between "+HHMM" timezones and minutes t/helper: fix segfault in "dump-reftable -t" refs/reftable: fix on-disk representation of reflog timezones apply.c | 3 ++- date.c | 25 +++++++++++++++++-------- date.h | 9 +++++++++ refs/reftable-backend.c | 7 ++++--- strbuf.c | 3 +-- t/helper/test-reftable.c | 13 +++++++++++-- t/t0610-reftable-basics.sh | 35 +++++++++++++++++++++++++++++++++++ 7 files changed, 79 insertions(+), 16 deletions(-) Range-diff versus v1: 1: 91b9739506 ! 1: 4f950cf7de date: add helpers to convert between "+HHMM" timezones and minutes @@ date.c: static int local_time_tzoffset(time_t t, struct tm *tm) offset /= 60; /* in minutes */ - offset = (offset % 60) + ((offset / 60) * 100); - return offset * eastwest; -+ return minutes_to_tz(offset * eastwest); ++ return minutes_to_tz(offset) * eastwest; } /* 2: 112e414c34 = 2: 9ef28821a3 t/helper: fix segfault in "dump-reftable -t" 3: 26eee10d6a ! 3: c66e467554 refs/reftable: fix on-disk representation of reflog timezones @@ t/t0610-reftable-basics.sh: test_expect_success 'reflog: renaming branch writes + cd repo && + GIT_COMMITTER_DATE="1234567890 -1200" git commit --allow-empty -m min && + GIT_COMMITTER_DATE="1234567890 +0530" git commit --allow-empty -m east && -+ GIT_COMMITTER_DATE="1234567890 -0800" git commit --allow-empty -m west && ++ GIT_COMMITTER_DATE="1234567890 -0830" git commit --allow-empty -m west && + GIT_COMMITTER_DATE="1234567890 +1400" git commit --allow-empty -m max && + + # The reftable format specifies the timezone as the offset from @@ t/t0610-reftable-basics.sh: test_expect_success 'reflog: renaming branch writes + sed -n "s/^log{refs\/heads\/main([0-9]*) .* 1234567890 //p" dump >actual && + cat >expect <<-\EOF && + 840 -+ -480 ++ -510 + 330 + -720 + EOF @@ t/t0610-reftable-basics.sh: test_expect_success 'reflog: renaming branch writes + test-tool ref-store main for-each-reflog-ent refs/heads/main >entries && + test_grep "1234567890 -1200 commit (initial): min" entries && + test_grep "1234567890 +0530 commit: east" entries && -+ test_grep "1234567890 -0800 commit: west" entries && ++ test_grep "1234567890 -0830 commit: west" entries && + test_grep "1234567890 +1400 commit: max" entries + ) +' --- base-commit: a018953688f1b10bddf91bff8747068f5f4746a4 change-id: 20260929-pks-reftables-fix-timezone-format-93ecd0e7a1d1 ``` ## Patrick Steinhardt, 2026-10-01 05:39 Subject: [PATCH v2 1/3] date: add helpers to convert between "+HHMM" timezones and minutes Message-ID: <20261001-pks-reftables-fix-timezone-format-v2-1-a4fd1f7cd21a@pks.im> In-Reply-To: <20261001-pks-reftables-fix-timezone-format-v2-0-a4fd1f7cd21a@pks.im> ``` The timezones that we store in commits as part of the identity information are encoded in "[+-]HHMM", for example "-0700" for UTC-7. Internally we typically pass around this timezone either as string or as a parsed integer (-700). Some sites want to convert between this format and minutes or vice versa, and that conversion is performed ad-hoc. We're about to introduce another site though that wants to have access to this logic, and having it cluttered across our codebase is a bit awkward. Introduce two new helpers `tz_to_minutes()` and `minutes_to_tz()` that perform the conversion for us and convert call sites to use them. Note that we used to perform a dance in `gm_time_t()` where we first convert `tz` into a positive value, then calculate the minutes, and finally turn the minutes into a negative value again. This dance is performed because it is implementation-defined in C89 whether the division on negative values truncates towards zero or not [1]: If either operand is negative, whether the result of the / operator is the largest integer less than the algebraic quotient or the smallest integer greater than the algebraic quotient is implementation-defined, as is the sign of the result of the % operator. So under C89, `-130 / 100` could legitimately result in -1 or -2, and `-130 % 100` could result in either -30 or 70. For us though, the result that we want is the first one (-1 and -30), which is called truncation toward zero. This part of the C language has changed in C99, where this edge case is now well-defined to always truncate towards zero [2]: When integers are divided, the result of the / operator is the algebraic quotient with any fractional part discarded.90) If the quotient a/b is representable, the expression (a/b)*b + a%b shall equal a. 90) This is often called ''truncation toward zero''. So in theory it's unlikely that we still need this logic. In practice though it feels safer to just retain it as we don't require a fully C99-compliant compiler in Git. [1]: https://port70.net/~nsz/c/c89/c89-draft.html#3.3.5 [2]: https://port70.net/~nsz/c/c99/n1256.html#6.5.5p6 Signed-off-by: Patrick Steinhardt --- apply.c | 3 ++- date.c | 25 +++++++++++++++++-------- date.h | 9 +++++++++ strbuf.c | 3 +-- 4 files changed, 29 insertions(+), 11 deletions(-) diff --git a/apply.c b/apply.c index f00b7ba4d3..367271b8ac 100644 --- a/apply.c +++ b/apply.c @@ -14,6 +14,7 @@ #include "abspath.h" #include "base85.h" #include "config.h" +#include "date.h" #include "odb.h" #include "delta.h" #include "diff.h" @@ -851,7 +852,7 @@ static int has_epoch_timestamp(const char *nameline) if (*colon == ':') zoneoffset = zoneoffset * 60 + strtol(colon + 1, NULL, 10); else - zoneoffset = (zoneoffset / 100) * 60 + (zoneoffset % 100); + zoneoffset = tz_to_minutes(zoneoffset); if (timestamp[m[3].rm_so] == '-') zoneoffset = -zoneoffset; diff --git a/date.c b/date.c index 014065b419..c50f45d310 100644 --- a/date.c +++ b/date.c @@ -45,13 +45,23 @@ static const char *weekday_names[] = { "Sundays", "Mondays", "Tuesdays", "Wednesdays", "Thursdays", "Fridays", "Saturdays" }; -static time_t gm_time_t(timestamp_t time, int tz) +int tz_to_minutes(int tz) { - int minutes; + int minutes = tz < 0 ? -tz : tz; + minutes = (minutes / 100) * 60 + (minutes % 100); + return tz < 0 ? -minutes : minutes; +} - minutes = tz < 0 ? -tz : tz; - minutes = (minutes / 100)*60 + (minutes % 100); - minutes = tz < 0 ? -minutes : minutes; +int minutes_to_tz(int minutes) +{ + int tz = minutes < 0 ? -minutes : minutes; + tz = (tz / 60) * 100 + (tz % 60); + return minutes < 0 ? -tz : tz; +} + +static time_t gm_time_t(timestamp_t time, int tz) +{ + int minutes = tz_to_minutes(tz); if (minutes > 0) { if (unsigned_add_overflows(time, minutes * 60)) @@ -103,8 +113,7 @@ static int local_time_tzoffset(time_t t, struct tm *tm) offset = t_local - t; } offset /= 60; /* in minutes */ - offset = (offset % 60) + ((offset / 60) * 100); - return offset * eastwest; + return minutes_to_tz(offset) * eastwest; } /* @@ -862,7 +871,7 @@ static int match_object_header_date(const char *date, timestamp_t *timestamp, in ofs = strtol(date, &end, 10); if ((*end != '\0' && (*end != '\n')) || end != date + 4) return -1; - ofs = (ofs / 100) * 60 + (ofs % 100); + ofs = tz_to_minutes(ofs); if (date[-1] == '-') ofs = -ofs; *timestamp = stamp; diff --git a/date.h b/date.h index 0747864fd7..816df5b833 100644 --- a/date.h +++ b/date.h @@ -70,4 +70,13 @@ void datestamp(struct strbuf *out); timestamp_t approxidate_careful(const char *, int *); int date_overflows(timestamp_t date); time_t tm_to_time_t(const struct tm *tm); + +/** + * Convert between the "[+-]HHMM" timezone format and minutes. This format is + * used for example as part of commit headers and reflogs. For example, the + * timezone -0100 is converted to -60 minutes. + */ +int tz_to_minutes(int tz); +int minutes_to_tz(int minutes); + #endif diff --git a/strbuf.c b/strbuf.c index 44955669e8..c3baa47b3f 100644 --- a/strbuf.c +++ b/strbuf.c @@ -1023,8 +1023,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm, else if (skip_prefix(fmt, "s", &fmt)) strbuf_addf(&munged_fmt, "%"PRItime, (timestamp_t)tm_to_time_t(tm) - - 3600 * (tz_offset / 100) - - 60 * (tz_offset % 100)); + 60 * tz_to_minutes(tz_offset)); else if (skip_prefix(fmt, "z", &fmt)) strbuf_addf(&munged_fmt, "%+05d", tz_offset); else if (suppress_tz_name && skip_prefix(fmt, "Z", &fmt)) -- 2.56.0.353.g0856645cf6.dirty ``` ## Patrick Steinhardt, 2026-10-01 05:39 Subject: [PATCH v2 2/3] t/helper: fix segfault in "dump-reftable -t" Message-ID: <20261001-pks-reftables-fix-timezone-format-v2-2-a4fd1f7cd21a@pks.im> In-Reply-To: <20261001-pks-reftables-fix-timezone-format-v2-0-a4fd1f7cd21a@pks.im> ``` The `test-tool dump-reftable` command can be used to dump the on-disk contents of reftables. The "-t" subcommand specifically can be used to dump a single table from disk. When trying to use this subcommand though one will quickly realize that it is broken, as it always segfaults. The root cause of this segfault is that we try to detect the hash algorithm via the merged table's hash ID. But that hash ID is not the same as Git's understanding of a hash ID, and consequently we fail to look up the correct algorithm. This will then lead to a segfault later on when we try to dereference a NULL pointer. This breakage went undetected until now because this particular subcommand is not used anywhere in our test suite. So the obvious way to fix the bug is by just removing the code outright. But in the next commit we're about to add a user. Fix the issue by properly converting between the two hash IDs. Signed-off-by: Patrick Steinhardt --- t/helper/test-reftable.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c index fc49fafc34..57758936b0 100644 --- a/t/helper/test-reftable.c +++ b/t/helper/test-reftable.c @@ -103,7 +103,16 @@ static int dump_table(struct reftable_merged_table *mt) if (err < 0) return err; - algop = &hash_algos[hash_algo_by_id(reftable_merged_table_hash_id(mt))]; + switch (reftable_merged_table_hash_id(mt)) { + case REFTABLE_HASH_SHA1: + algop = &hash_algos[GIT_HASH_SHA1]; + break; + case REFTABLE_HASH_SHA256: + algop = &hash_algos[GIT_HASH_SHA256]; + break; + default: + die("unknown reftable hash function: %d", reftable_merged_table_hash_id(mt)); + } while (1) { err = reftable_iterator_next_ref(&it, &ref); -- 2.56.0.353.g0856645cf6.dirty ``` ## Patrick Steinhardt, 2026-10-01 05:39 Subject: [PATCH v2 3/3] refs/reftable: fix on-disk representation of reflog timezones Message-ID: <20261001-pks-reftables-fix-timezone-format-v2-3-a4fd1f7cd21a@pks.im> In-Reply-To: <20261001-pks-reftables-fix-timezone-format-v2-0-a4fd1f7cd21a@pks.im> ``` When writing reflog entries to disk we also record authorship information for the reflog. Besides the author name and mail address, it also contains the date and timezone at which the record has been created. The timezone information is typically encoded in the "[+-]HHMM" format, and we often pass it around as parsed integer. For example, the timezone "-0700" would be passed around as -700. And this is also the value that we eventually store in the reftable on disk. But the specification in "Documentation/technical/reftable.adoc" notes that the timezone is a "2-byte timezone offset in minutes (signed)". So instead of storing -700 in the above example, we have to first convert that value into minutes and then store -420. We don't though, so we have a mismatch between specification and implementation. Ideally, we'd just adapt the specification to match the implementation. But that's easier said than done, because the specification is 11 years old by now and reftables have already been implemented by JGit for a long time. So if we now changed the specification, those libraries would have to make a backwards-incompatible change. Another alternative would be to bump the reftable format version, but that feels suboptimal, too. Other libraries would all have to adapt, and it wouldn't really help us to fix the discrepancy between alternative implementations and our implementation as older versions would still be misinterpreted. The only viable option seems to be that we simply treat this as a bug and fix it. This will of course make us misinterpret older reftables that already exist on disk: ┌───────┬───────────────┬─────────────────┬────────────┐ │ tz │ HHMM encoding │ correct minutes │ divergence │ ├───────┼───────────────┼─────────────────┼────────────┤ │ +1400 │ 1400 │ 840 │ 560 │ ├───────┼───────────────┼─────────────────┼────────────┤ │ -1200 │ -1200 │ -720 │ 480 │ ├───────┼───────────────┼─────────────────┼────────────┤ │ +0530 │ 530 │ 330 │ 200 │ ├───────┼───────────────┼─────────────────┼────────────┤ │ +0000 │ 0 │ 0 │ 0 │ └───────┴───────────────┴─────────────────┴────────────┘ But this divergence ultimately doesn't matter much, as Git only uses the timezone of reflog entries for display purposes anyway. We don't take the timezone into account when parsing "HEAD@{1.hour.ago}" syntax, and `should_expire_reflog_ent()` doesn't use it either to decide whether reflog entries should be pruned. In summary, the fallout from this change is quite contained. Adapt the reftable backend accordingly and simply reinterpret the timezones with the specified meaning. Add a test to verify that we properly encode the timezone as offset in minutes. Adapt the test helper accordingly to no longer zero-pad the offset with "%04d", as that can be easily misinterpreted as the "HHMM" encoding. Reported-by: Josh McKinney Signed-off-by: Patrick Steinhardt --- refs/reftable-backend.c | 7 ++++--- t/helper/test-reftable.c | 2 +- t/t0610-reftable-basics.sh | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 4 deletions(-) diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 10db03991e..d0de066355 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -2,6 +2,7 @@ #include "../abspath.h" #include "../chdir-notify.h" #include "../config.h" +#include "../date.h" #include "../dir.h" #include "../environment.h" #include "../fsck.h" @@ -317,7 +318,7 @@ static void fill_reftable_log_record(struct reftable_log_record *log, const stru tz_begin++; } - log->value.update.tz_offset = sign * atoi(tz_begin); + log->value.update.tz_offset = tz_to_minutes(sign * atoi(tz_begin)); } static int reftable_be_config(const char *var, const char *value, @@ -2186,7 +2187,7 @@ static int yield_log_record(struct reftable_ref_store *refs, full_committer = fmt_ident(log->value.update.name, log->value.update.email, WANT_COMMITTER_IDENT, NULL, IDENT_NO_DATE); return fn(log->refname, &old_oid, &new_oid, full_committer, - log->value.update.time, log->value.update.tz_offset, + log->value.update.time, minutes_to_tz(log->value.update.tz_offset), log->value.update.message, cb_data); } @@ -2690,7 +2691,7 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store, if (should_prune_fn(&old_oid, &new_oid, logs[i].value.update.email, (timestamp_t)logs[i].value.update.time, - logs[i].value.update.tz_offset, + minutes_to_tz(logs[i].value.update.tz_offset), logs[i].value.update.message, policy_cb_data)) { dest->value_type = REFTABLE_LOG_DELETION; diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c index 57758936b0..d9f2ca1d0e 100644 --- a/t/helper/test-reftable.c +++ b/t/helper/test-reftable.c @@ -163,7 +163,7 @@ static int dump_table(struct reftable_merged_table *mt) log.update_index); break; case REFTABLE_LOG_UPDATE: - printf("log{%s(%" PRIu64 ") %s <%s> %" PRIu64 " %04d\n", + printf("log{%s(%" PRIu64 ") %s <%s> %" PRIu64 " %d\n", log.refname, log.update_index, log.value.update.name ? log.value.update.name : "", log.value.update.email ? log.value.update.email : "", diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh index 35e98b43db..2253705a19 100755 --- a/t/t0610-reftable-basics.sh +++ b/t/t0610-reftable-basics.sh @@ -837,6 +837,41 @@ test_expect_success 'reflog: renaming branch writes reflog entry' ' ) ' +test_expect_success 'reflog: timezone offset is stored in minutes' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + GIT_COMMITTER_DATE="1234567890 -1200" git commit --allow-empty -m min && + GIT_COMMITTER_DATE="1234567890 +0530" git commit --allow-empty -m east && + GIT_COMMITTER_DATE="1234567890 -0830" git commit --allow-empty -m west && + GIT_COMMITTER_DATE="1234567890 +1400" git commit --allow-empty -m max && + + # The reftable format specifies the timezone as the offset from + # UTC in minutes, whereas Git uses the parsed form of "+HHMM" + # internally. Verify that we do the conversion when writing. + for table in .git/reftable/*.ref + do + test-tool dump-reftable -t "$table" || return 1 + done >dump && + sed -n "s/^log{refs\/heads\/main([0-9]*) .* 1234567890 //p" dump >actual && + cat >expect <<-\EOF && + 840 + -510 + 330 + -720 + EOF + test_cmp expect actual && + + # And verify that we convert back when reading. + test-tool ref-store main for-each-reflog-ent refs/heads/main >entries && + test_grep "1234567890 -1200 commit (initial): min" entries && + test_grep "1234567890 +0530 commit: east" entries && + test_grep "1234567890 -0830 commit: west" entries && + test_grep "1234567890 +1400 commit: max" entries + ) +' + test_expect_success 'reflog: can store empty logs' ' test_when_finished "rm -rf repo" && git init repo && -- 2.56.0.353.g0856645cf6.dirty ``` ## Karthik Nayak, 2026-10-01 10:18 Subject: Re: [PATCH 1/3] date: add helpers to convert between "+HHMM" timezones and minutes Message-ID: In-Reply-To: ``` Patrick Steinhardt writes: > On Wed, Sep 30, 2026 at 04:47:02AM -0700, Karthik Nayak wrote: > > One suggestion: I'd recommend trimming the mails you're responding to a > bit more aggressively. Otherwise one is hunting for responses in files > and hunks that are not relevant to your remarks :) > Sure, will do that more henceforth. >> Patrick Steinhardt writes: >> > diff --git a/date.c b/date.c >> > index 014065b419..63ea9dbc76 100644 >> > --- a/date.c >> > +++ b/date.c >> > @@ -103,8 +113,7 @@ static int local_time_tzoffset(time_t t, struct tm *tm) >> > offset = t_local - t; >> > } >> > offset /= 60; /* in minutes */ >> > - offset = (offset % 60) + ((offset / 60) * 100); >> > - return offset * eastwest; >> > + return minutes_to_tz(offset * eastwest); >> >> While mathematically it's the same, but shouldn't this have been >> `minutes_to_tz(offset) * eastwest`? > > I guess we can. It's probably less confusing if we do it this way > indeed. > > Patrick Yup. Thanks ``` ## Karthik Nayak, 2026-10-01 10:19 Subject: Re: [PATCH v2 0/3] refs/reftable: fix on-disk representation of reflog timezones Message-ID: In-Reply-To: <20261001-pks-reftables-fix-timezone-format-v2-0-a4fd1f7cd21a@pks.im> ``` Patrick Steinhardt writes: > Hi, > > it was reported [1] that the way we store reflog timezones with the > reftable format has a mismatch with the reftable specification. While > the spec says that reftables should be stored as a signed offset in > minutes, we store them in the "[+-]HHMM" format that we typically use in > commit headers, for example. > > This patch series fixes this bug by making our on-disk representation > match the specification. This will of course make us reinterpret old > reftables. But ultimately, the fallout caused by this change is somewhat > limited as we only ever use reflog timezones for display purposes. So > yes, we'll display a wrong timezone. But it's not used as part of any > kind of computations. > > The series is built on top of v2.56.0. > > Changes in v2: > - Improve readability of one of the converted sites that now use > `minutes_to_tz()`. > - Improve test coverage. > - Link to v1: https://patch.msgid.link/20260929-pks-reftables-fix-timezone-format-v1-0-3df105a95ed1@pks.im > The range-diff looks in order. This version looks good to me! [snip] ``` ## Junio C Hamano, 2026-10-01 17:11 Subject: Re: [PATCH v2 0/3] refs/reftable: fix on-disk representation of reflog timezones Message-ID: In-Reply-To: ``` Karthik Nayak writes: > Patrick Steinhardt writes: > >> Hi, >> >> it was reported [1] that the way we store reflog timezones with the >> reftable format has a mismatch with the reftable specification. While >> the spec says that reftables should be stored as a signed offset in >> minutes, we store them in the "[+-]HHMM" format that we typically use in >> commit headers, for example. >> >> This patch series fixes this bug by making our on-disk representation >> match the specification. This will of course make us reinterpret old >> reftables. But ultimately, the fallout caused by this change is somewhat >> limited as we only ever use reflog timezones for display purposes. So >> yes, we'll display a wrong timezone. But it's not used as part of any >> kind of computations. >> >> The series is built on top of v2.56.0. >> >> Changes in v2: >> - Improve readability of one of the converted sites that now use >> `minutes_to_tz()`. >> - Improve test coverage. >> - Link to v1: https://patch.msgid.link/20260929-pks-reftables-fix-timezone-format-v1-0-3df105a95ed1@pks.im >> > > The range-diff looks in order. This version looks good to me! Thanks for writing and reviewing. The previous round was good enough already but with an extra polish, this looks really ready. Will mark the topic for 'next'. ```