From: Karthik Nayak Date: Wed, 30 Sep 2026 11:51:15 GMT 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.