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

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 <ps@pks.im>
---
 apply.c  |  3 ++-
 date.c   | 25 +++++++++++++++++--------
 date.h   |  9 +++++++++
 strbuf.c |  3 +--
 4 files changed, 29 insertions(+), 11 deletions(-)

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 <ps@pks.im>
---
 t/helper/test-reftable.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

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 <git-bugs@lists.joshka.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/reftable-backend.c    |  7 ++++---
 t/helper/test-reftable.c   |  2 +-
 t/t0610-reftable-basics.sh | 35 +++++++++++++++++++++++++++++++++++
 3 files changed, 40 insertions(+), 4 deletions(-)

Patrick Steinhardt <ps@pks.im> writes:

We divide by '100' because we represent "-0700" as '-700' in integer. So we need to separate out the 'HH' from 'MM'. Okay.

This is the part which we could skip as we're C99 compliant, but keeping to be on the safe side.

While mathematically it's the same, but shouldn't this have been `minutes_to_tz(offset) * eastwest`?

The rest looks good.

Patrick Steinhardt <ps@pks.im> writes:

Nit: it would be nice to have a negative timezone with MM filled in too.

Apart from the nit, the changes look good.

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 :)

I guess we can. It's probably less confusing if we do it this way indeed.

Patrick

On Wed, Sep 30, 2026 at 04:51:15AM -0700, Karthik Nayak wrote:

Sure, can do. I'll just change -0800 to -0830. Thanks!

Patrick

Karthik Nayak <karthik.188@gmail.com> writes:

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.

The way you suggest is more faithful rewrite of the original.

Thanks.

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

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 <ps@pks.im>
---
 apply.c  |  3 ++-
 date.c   | 25 +++++++++++++++++--------
 date.h   |  9 +++++++++
 strbuf.c |  3 +--
 4 files changed, 29 insertions(+), 11 deletions(-)

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 <ps@pks.im>
---
 t/helper/test-reftable.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

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 <git-bugs@lists.joshka.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/reftable-backend.c    |  7 ++++---
 t/helper/test-reftable.c   |  2 +-
 t/t0610-reftable-basics.sh | 35 +++++++++++++++++++++++++++++++++++
 3 files changed, 40 insertions(+), 4 deletions(-)

Patrick Steinhardt <ps@pks.im> writes:

Sure, will do that more henceforth.

Yup. Thanks

Patrick Steinhardt <ps@pks.im> writes:

The range-diff looks in order. This version looks good to me!

[snip]

Karthik Nayak <karthik.188@gmail.com> writes:

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'.