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