Show 136 quoted lines
> 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 &&Karthik Nayak <karthik.188@gmail.com>