[PATCH 0/3] refs/reftable: fix on-disk representation of reflog timezones

[PATCH 0/3] refs/reftable: fix on-disk representation of reflog timezones

[Patrick Steinhardt]

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

[PATCH 1/3] date: add helpers to convert between "+HHMM" timezones and minutes

[Patrick Steinhardt]

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

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

Re: [PATCH 1/3] date: add helpers to convert between "+HHMM" timezones and minutes

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 6328 bytes --]

Patrick Steinhardt <ps@pks.im> 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 <ps@pks.im>
> ---
>  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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 1/3] date: add helpers to convert between "+HHMM" timezones and minutes

[Patrick Steinhardt]

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 <ps@pks.im> 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

Re: [PATCH 1/3] date: add helpers to convert between "+HHMM" timezones and minutes

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1009 bytes --]

Patrick Steinhardt <ps@pks.im> 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 <ps@pks.im> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 1/3] date: add helpers to convert between "+HHMM" timezones and minutes

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> 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.

[PATCH 2/3] t/helper: fix segfault in "dump-reftable -t"

[Patrick Steinhardt]

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

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

[PATCH 3/3] refs/reftable: fix on-disk representation of reflog timezones

[Patrick Steinhardt]

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

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

Re: [PATCH 3/3] refs/reftable: fix on-disk representation of reflog timezones

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 8494 bytes --]

Patrick Steinhardt <ps@pks.im> 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 <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(-)
>
> 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 3/3] refs/reftable: fix on-disk representation of reflog timezones

[Patrick Steinhardt]

On Wed, Sep 30, 2026 at 04:51:15AM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> 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

[PATCH v2 0/3] refs/reftable: fix on-disk representation of reflog timezones

[Patrick Steinhardt]

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

[PATCH v2 1/3] date: add helpers to convert between "+HHMM" timezones and minutes

[Patrick Steinhardt]

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

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

[PATCH v2 2/3] t/helper: fix segfault in "dump-reftable -t"

[Patrick Steinhardt]

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

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

[PATCH v2 3/3] refs/reftable: fix on-disk representation of reflog timezones

[Patrick Steinhardt]

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

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

Re: [PATCH v2 0/3] refs/reftable: fix on-disk representation of reflog timezones

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1123 bytes --]

Patrick Steinhardt <ps@pks.im> 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]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v2 0/3] refs/reftable: fix on-disk representation of reflog timezones

[Junio C Hamano]

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

> Patrick Steinhardt <ps@pks.im> 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'.