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

> 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