timezone related bug of git

timezone related bug of git

[Dongsheng Song]

Hi，

 I found a timezone related bug in the git:

1. git log 11990eba -1 --date=format:%s

commit 11990eba0be50d1ad0655ede4062b7130326c41f (HEAD -> trunk,
origin/trunk, origin/HEAD)
Author: rillig <rillig@NetBSD.org>
Date:   1635604878

    indent: move debugging functions to a separate section

2. git cat-file -p 11990eba

tree 5d62150f5e2bafd3db76641450ca5d902302a039
parent 892557a74bd49983fac28366b772b53c9216ca73
author rillig <rillig@NetBSD.org> 1635633678 +0000
committer rillig <rillig@NetBSD.org> 1635633678 +0000

indent: move debugging functions to a separate section

3. conclusion

The unix time stored in git repository not same as the git log output,
then there must be a timezone offset bug:

1635633678 - 1635604878 = 28800 = 8 hours (local timezone offset)

Best regards,
Cauchy Song

Re: timezone related bug of git

[Jeff King]

On Sun, Oct 31, 2021 at 11:23:24AM +0800, Dongsheng Song wrote:

>  I found a timezone related bug in the git:
> 
> 1. git log 11990eba -1 --date=format:%s
> 
> commit 11990eba0be50d1ad0655ede4062b7130326c41f (HEAD -> trunk,
> origin/trunk, origin/HEAD)
> Author: rillig <rillig@NetBSD.org>
> Date:   1635604878
> 
>     indent: move debugging functions to a separate section
> 
> 2. git cat-file -p 11990eba
> 
> tree 5d62150f5e2bafd3db76641450ca5d902302a039
> parent 892557a74bd49983fac28366b772b53c9216ca73
> author rillig <rillig@NetBSD.org> 1635633678 +0000
> committer rillig <rillig@NetBSD.org> 1635633678 +0000
> 
> indent: move debugging functions to a separate section
> 
> 3. conclusion
> 
> The unix time stored in git repository not same as the git log output,
> then there must be a timezone offset bug:
> 
> 1635633678 - 1635604878 = 28800 = 8 hours (local timezone offset)

The short answer is: don't do that. Use --date=unix instead.

The longer one is:

The problem is that the strftime() "%s" specifier is a bit broken.
That function (which is what is interpreting your format) takes a
broken-down "struct tm", which can only be converted back to an epoch
time if you know which time zone it's in.

But we have no way to tell the function that; the standard indicates
that it always assumes the local system timezone, and there's no
provision at all for formatting times in other zones (which is what we
usually try to do, showing the date in the author's zone). There's no
field in the "struct tm" to carry any zone information[1].

Even when you're in the same timezone, there's a similar problem with
the is_dst field. There's some discussion in [2], including the
possibility of intercepting "%s" and handling it ourselves, like we do
for "%z". I don't think anybody has cared enough to work on it.

-Peff

[1] Some implementations (like glibc) actually _do_ carry this
    information in private fields of "struct tm". But we can't rely on
    it, and even where it's available, it's confusing (e.g., mktime()
    ignores it!). If you're a real masochist, you can read all of:

      https://lore.kernel.org/git/22824.29946.305300.380299@a1i15.kph.uni-mainz.de/

[2] This is a similar bug report from 2020:

      https://lore.kernel.org/git/CAGqZTUu2U6FFXGTXihC64O0gB5Bz_Z3MbD750kMoJWMciAGH6w@mail.gmail.com/

Re: timezone related bug of git

[Dongsheng Song]

Thank you for the clarification, it's really a disappointing answer.

Perhaps the manual needs to be clearer about this limitation.

On Sun, Oct 31, 2021 at 4:53 PM Jeff King <peff@peff.net> wrote:
>
> On Sun, Oct 31, 2021 at 11:23:24AM +0800, Dongsheng Song wrote:
>
> >  I found a timezone related bug in the git:
> >
> > 1. git log 11990eba -1 --date=format:%s
> >
> > commit 11990eba0be50d1ad0655ede4062b7130326c41f (HEAD -> trunk,
> > origin/trunk, origin/HEAD)
> > Author: rillig <rillig@NetBSD.org>
> > Date:   1635604878
> >
> >     indent: move debugging functions to a separate section
> >
> > 2. git cat-file -p 11990eba
> >
> > tree 5d62150f5e2bafd3db76641450ca5d902302a039
> > parent 892557a74bd49983fac28366b772b53c9216ca73
> > author rillig <rillig@NetBSD.org> 1635633678 +0000
> > committer rillig <rillig@NetBSD.org> 1635633678 +0000
> >
> > indent: move debugging functions to a separate section
> >
> > 3. conclusion
> >
> > The unix time stored in git repository not same as the git log output,
> > then there must be a timezone offset bug:
> >
> > 1635633678 - 1635604878 = 28800 = 8 hours (local timezone offset)
>
> The short answer is: don't do that. Use --date=unix instead.
>
> The longer one is:
>
> The problem is that the strftime() "%s" specifier is a bit broken.
> That function (which is what is interpreting your format) takes a
> broken-down "struct tm", which can only be converted back to an epoch
> time if you know which time zone it's in.
>
> But we have no way to tell the function that; the standard indicates
> that it always assumes the local system timezone, and there's no
> provision at all for formatting times in other zones (which is what we
> usually try to do, showing the date in the author's zone). There's no
> field in the "struct tm" to carry any zone information[1].
>
> Even when you're in the same timezone, there's a similar problem with
> the is_dst field. There's some discussion in [2], including the
> possibility of intercepting "%s" and handling it ourselves, like we do
> for "%z". I don't think anybody has cared enough to work on it.
>
> -Peff
>
> [1] Some implementations (like glibc) actually _do_ carry this
>     information in private fields of "struct tm". But we can't rely on
>     it, and even where it's available, it's confusing (e.g., mktime()
>     ignores it!). If you're a real masochist, you can read all of:
>
>       https://lore.kernel.org/git/22824.29946.305300.380299@a1i15.kph.uni-mainz.de/
>
> [2] This is a similar bug report from 2020:
>
>       https://lore.kernel.org/git/CAGqZTUu2U6FFXGTXihC64O0gB5Bz_Z3MbD750kMoJWMciAGH6w@mail.gmail.com/

Re: timezone related bug of git

[Junio C Hamano]

Dongsheng Song <dongsheng.song@gmail.com> writes:

> Thank you for the clarification, it's really a disappointing answer.

The situation may be disappointing, but I found the answer eminently
clear and helpful.

> Perhaps the manual needs to be clearer about this limitation.

Sounds like we have a volunteer ;-)?

Thanks for a report and discussion.

Re: timezone related bug of git

[Jeff King]

On Sun, Oct 31, 2021 at 11:46:40AM -0700, Junio C Hamano wrote:

> Dongsheng Song <dongsheng.song@gmail.com> writes:
> 
> > Thank you for the clarification, it's really a disappointing answer.
> 
> The situation may be disappointing, but I found the answer eminently
> clear and helpful.

The most disappointing thing IMHO is the lousy state of system-level
date routines. ;)

I have some patches working towards allowing timestamps before 1970, and
the system routines are quite unreliable (both in giving insufficient
portable interfaces, but also just doing weird things with negative
values).

> > Perhaps the manual needs to be clearer about this limitation.
> 
> Sounds like we have a volunteer ;-)?

Yeah, I'd be happy if somebody wanted to note this in the manual. But if
anybody wants to pursue manually intercepting %s, I think the patch
below might point them in the right direction.

I won't be at all surprised if it has funny corner cases. Our
tm_to_time_t() is pretty basic and hacky. We can't use mktime() because
it only handles the current system timezone. OTOH, I think the tz_offset
we're undoing here originally came from comparing mktime() versus
tm_to_time_t() via local_time_tzoffset(), so it could be cancelling out
any bugs exactly. :)

So maybe the code below is sufficient, but we'd probably at least want
some tests on top. Maybe something somebody interested would like to
pick up and run with?

---
diff --git a/cache.h b/cache.h
index eba12487b9..aa6f380d10 100644
--- a/cache.h
+++ b/cache.h
@@ -1588,6 +1588,7 @@ timestamp_t approxidate_careful(const char *, int *);
 timestamp_t approxidate_relative(const char *date);
 void parse_date_format(const char *format, struct date_mode *mode);
 int date_overflows(timestamp_t date);
+time_t tm_to_time_t(const struct tm *tm);
 
 #define IDENT_STRICT	       1
 #define IDENT_NO_DATE	       2
diff --git a/date.c b/date.c
index c55ea47e96..84bb4451c1 100644
--- a/date.c
+++ b/date.c
@@ -9,7 +9,7 @@
 /*
  * This is like mktime, but without normalization of tm_wday and tm_yday.
  */
-static time_t tm_to_time_t(const struct tm *tm)
+time_t tm_to_time_t(const struct tm *tm)
 {
 	static const int mdays[] = {
 	    0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334
diff --git a/strbuf.c b/strbuf.c
index b22e981655..8b8b1900bc 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -1019,6 +1019,13 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
 			strbuf_addstr(&munged_fmt, "%%");
 			fmt++;
 			break;
+		case 's':
+			strbuf_addf(&munged_fmt, "%"PRItime,
+				    tm_to_time_t(tm) -
+				    3600 * (tz_offset / 100) -
+				    60 * (tz_offset % 100));
+			fmt++;
+			break;
 		case 'z':
 			strbuf_addf(&munged_fmt, "%+05d", tz_offset);
 			fmt++;

Re: timezone related bug of git

[Dongsheng Song]

Hi Jeff,

Your patch looks good to me.  Here is my test result:

$ patch -p1 < x.patch
patching file cache.h
patching file date.c
patching file strbuf.c

$ make

$ ~/git-testing/bin/git --version

$ ~/git-testing/bin/git log 11990eba -1 --date=format:%s
commit 11990eba0be50d1ad0655ede4062b7130326c41f (HEAD -> trunk,
origin/trunk, origin/HEAD)
Author: rillig <rillig@NetBSD.org>
Date:   1635633678

    indent: move debugging functions to a separate section


$ ~/git-testing/bin/git cat-file -p 11990eba
tree 5d62150f5e2bafd3db76641450ca5d902302a039
parent 892557a74bd49983fac28366b772b53c9216ca73
author rillig <rillig@NetBSD.org> 1635633678 +0000
committer rillig <rillig@NetBSD.org> 1635633678 +0000

indent: move debugging functions to a separate section


PS:
Thanks Junio for clarification, Jeff's explanation is very clear,
I'm just frustrated with the limitations of the system, I greatly
appreciate Jeff's work.

Best regards,
Dongsheng Song

On Mon, Nov 1, 2021 at 12:03 PM Jeff King <peff@peff.net> wrote:
>
> On Sun, Oct 31, 2021 at 11:46:40AM -0700, Junio C Hamano wrote:
>
> > Dongsheng Song <dongsheng.song@gmail.com> writes:
> >
> > > Thank you for the clarification, it's really a disappointing answer.
> >
> > The situation may be disappointing, but I found the answer eminently
> > clear and helpful.
>
> The most disappointing thing IMHO is the lousy state of system-level
> date routines. ;)
>
> I have some patches working towards allowing timestamps before 1970, and
> the system routines are quite unreliable (both in giving insufficient
> portable interfaces, but also just doing weird things with negative
> values).
>
> > > Perhaps the manual needs to be clearer about this limitation.
> >
> > Sounds like we have a volunteer ;-)?
>
> Yeah, I'd be happy if somebody wanted to note this in the manual. But if
> anybody wants to pursue manually intercepting %s, I think the patch
> below might point them in the right direction.
>
> I won't be at all surprised if it has funny corner cases. Our
> tm_to_time_t() is pretty basic and hacky. We can't use mktime() because
> it only handles the current system timezone. OTOH, I think the tz_offset
> we're undoing here originally came from comparing mktime() versus
> tm_to_time_t() via local_time_tzoffset(), so it could be cancelling out
> any bugs exactly. :)
>
> So maybe the code below is sufficient, but we'd probably at least want
> some tests on top. Maybe something somebody interested would like to
> pick up and run with?
>
> ---
> diff --git a/cache.h b/cache.h
> index eba12487b9..aa6f380d10 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1588,6 +1588,7 @@ timestamp_t approxidate_careful(const char *, int *);
>  timestamp_t approxidate_relative(const char *date);
>  void parse_date_format(const char *format, struct date_mode *mode);
>  int date_overflows(timestamp_t date);
> +time_t tm_to_time_t(const struct tm *tm);
>
>  #define IDENT_STRICT          1
>  #define IDENT_NO_DATE         2
> diff --git a/date.c b/date.c
> index c55ea47e96..84bb4451c1 100644
> --- a/date.c
> +++ b/date.c
> @@ -9,7 +9,7 @@
>  /*
>   * This is like mktime, but without normalization of tm_wday and tm_yday.
>   */
> -static time_t tm_to_time_t(const struct tm *tm)
> +time_t tm_to_time_t(const struct tm *tm)
>  {
>         static const int mdays[] = {
>             0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334
> diff --git a/strbuf.c b/strbuf.c
> index b22e981655..8b8b1900bc 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -1019,6 +1019,13 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
>                         strbuf_addstr(&munged_fmt, "%%");
>                         fmt++;
>                         break;
> +               case 's':
> +                       strbuf_addf(&munged_fmt, "%"PRItime,
> +                                   tm_to_time_t(tm) -
> +                                   3600 * (tz_offset / 100) -
> +                                   60 * (tz_offset % 100));
> +                       fmt++;
> +                       break;
>                 case 'z':
>                         strbuf_addf(&munged_fmt, "%+05d", tz_offset);
>                         fmt++;

Re: timezone related bug of git

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I won't be at all surprised if it has funny corner cases. Our
> tm_to_time_t() is pretty basic and hacky. We can't use mktime() because
> it only handles the current system timezone. OTOH, I think the tz_offset
> we're undoing here originally came from comparing mktime() versus
> tm_to_time_t() via local_time_tzoffset(), so it could be cancelling out
> any bugs exactly. :)
>
> So maybe the code below is sufficient, but we'd probably at least want
> some tests on top. Maybe something somebody interested would like to
> pick up and run with?

It would be very hard to write a code that does not work correctly
on a timestamp created in the same zone in the same season. It is
easy to get the direction of the offset wrong and not notice with
such a test, but with another test to show a timestamp from a zone
in a different zone (or across season boundary in an area where
daylight saving time is s thing), such an error can easily be
caught.

> ---
> diff --git a/cache.h b/cache.h
> index eba12487b9..aa6f380d10 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1588,6 +1588,7 @@ timestamp_t approxidate_careful(const char *, int *);
>  timestamp_t approxidate_relative(const char *date);
>  void parse_date_format(const char *format, struct date_mode *mode);
>  int date_overflows(timestamp_t date);
> +time_t tm_to_time_t(const struct tm *tm);
>  
>  #define IDENT_STRICT	       1
>  #define IDENT_NO_DATE	       2
> diff --git a/date.c b/date.c
> index c55ea47e96..84bb4451c1 100644
> --- a/date.c
> +++ b/date.c
> @@ -9,7 +9,7 @@
>  /*
>   * This is like mktime, but without normalization of tm_wday and tm_yday.
>   */
> -static time_t tm_to_time_t(const struct tm *tm)
> +time_t tm_to_time_t(const struct tm *tm)
>  {
>  	static const int mdays[] = {
>  	    0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334
> diff --git a/strbuf.c b/strbuf.c
> index b22e981655..8b8b1900bc 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -1019,6 +1019,13 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
>  			strbuf_addstr(&munged_fmt, "%%");
>  			fmt++;
>  			break;
> +		case 's':
> +			strbuf_addf(&munged_fmt, "%"PRItime,
> +				    tm_to_time_t(tm) -
> +				    3600 * (tz_offset / 100) -
> +				    60 * (tz_offset % 100));
> +			fmt++;
> +			break;

In show_date(), we start from UNIX time and go to "struct tm" using
either the system gmtime_r() (after adjusting the value with the tz
offset of the original timestamp) or localtime_r() (when we are
trying to show the value in our local timestamp), but this codepath
needs to undo that.  Our tm_to_time_t() indeed is basic but should
work correctly on a broken down UTC.  So the caller needs to further
compensate for the tz offset.

I have to wonder why gm_time_t() needs to use two separate codepaths
for positive and negative tz_offset, while the new code here can get
away without.  Does it have something to do with the direction of
truncation during division and modulo operation?

Thanks.

Re: timezone related bug of git

[Jeff King]

On Mon, Nov 01, 2021 at 11:18:02AM -0700, Junio C Hamano wrote:

> I have to wonder why gm_time_t() needs to use two separate codepaths
> for positive and negative tz_offset, while the new code here can get
> away without.  Does it have something to do with the direction of
> truncation during division and modulo operation?

Hmm. Unless I am missing something, this part of gm_time_t() is simply
over-complicating things:

  minutes = tz < 0 ? -tz : tz;
  minutes = (minutes / 100)*60 + (minutes % 100);
  minutes = tz < 0 ? -minutes : minutes;

We switch to doing the computation in absolute-value units, but then
restore the sign. But just:

  minutes = (tz / 100) * 60 + (tz % 100);

is equivalent and shorter. If tz is negative, then both terms will be
negative, which is what you want (they sum to a larger absolute-value
negative number). This comes from f80cd783c6 (date.c: add "show_date()"
function., 2005-05-06), so I don't see any sign that there was specific
thought given to some obscure handling. And indeed later fixes like
fbab835c03 ([PATCH] fix show_date() for positive timezones, 2005-05-18)
imply to me that the original was just confused.

Later we do:

  if (minutes > 0) {
          if (unsigned_add_overflows(time, minutes * 60))
                  die("Timestamp+tz too large: %"PRItime" +%04d",
                      time, tz);
  } else if (time < -minutes * 60)
          die("Timestamp before Unix epoch: %"PRItime" %04d", time, tz);

And that does need separate paths for the overflow check, since we're
checking different boundaries. I suspect for the strftime() code that we
wouldn't need similar checks, because the earlier ones would have caught
any problems (i.e., we would not get as far as having a "struct tm" that
represented something outside the range of our time_t).

-Peff

[PATCH] strbuf_addftime(): handle "%s" manually

[Jeff King]

On Mon, Nov 01, 2021 at 11:18:02AM -0700, Junio C Hamano wrote:

> > diff --git a/strbuf.c b/strbuf.c
> > index b22e981655..8b8b1900bc 100644
> > --- a/strbuf.c
> > +++ b/strbuf.c
> > @@ -1019,6 +1019,13 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
> >  			strbuf_addstr(&munged_fmt, "%%");
> >  			fmt++;
> >  			break;
> > +		case 's':
> > +			strbuf_addf(&munged_fmt, "%"PRItime,
> > +				    tm_to_time_t(tm) -
> > +				    3600 * (tz_offset / 100) -
> > +				    60 * (tz_offset % 100));
> > +			fmt++;
> > +			break;
> 
> In show_date(), we start from UNIX time and go to "struct tm" using
> either the system gmtime_r() (after adjusting the value with the tz
> offset of the original timestamp) or localtime_r() (when we are
> trying to show the value in our local timestamp), but this codepath
> needs to undo that.  Our tm_to_time_t() indeed is basic but should
> work correctly on a broken down UTC.  So the caller needs to further
> compensate for the tz offset.

This made me wonder how things interact with format-local. There's some
subtlety, but I think all is well.

I hadn't planned to work further on this, but now having thought about
it some more, it seemed worth capturing that and putting the finishing
touches on a real commit.

-- >8 --
Subject: [PATCH] strbuf_addftime(): handle "%s" manually

The strftime() function has a non-standard "%s" extension, which prints
the number of seconds since the epoch. But the "struct tm" we get has
already been adjusted for a particular time zone; going back to an epoch
time requires knowing that zone offset. Since strftime() doesn't take
such an argument, round-tripping to a "struct tm" and back to the "%s"
format may produce the wrong value (off by tz_offset seconds).

Since we're already passing in the zone offset courtesy of c3fbf81a85
(strbuf: let strbuf_addftime handle %z and %Z itself, 2017-06-15), we
can use that same value to adjust our epoch seconds accordingly.

Note that the description above makes it sound like strftime()'s "%s" is
useless (and really, the issue is shared by mktime(), which is what
strftime() would use under the hood). But it gets the two cases for
which it's designed correct:

  - the result of gmtime() will have a zero offset, so no adjustment is
    necessary

  - the result of localtime() will be offset by the local zone offset,
    and mktime() and strftime() are defined to assume this offset when
    converting back (there's actually some magic here; some
    implementations record this in the "struct tm", but we can't
    portably access or manipulate it. But they somehow "know" whether a
    "struct tm" is from gmtime() or localtime()).

This latter point means that "format-local:%s" actually works correctly
already, because in that case we rely on the system routines due to
6eced3ec5e (date: use localtime() for "-local" time formats,
2017-06-15). Our problem comes when trying to show times in the author's
zone, as the system routines provide no mechanism for converting in
non-local zones. So in those cases we have a "struct tm" that came from
gmtime(), but has been manipulated according to our offset.

The tests cover the broken round-trip by formatting "%s" for a time in a
non-system timezone. We use the made-up "+1234" here, which has two
advantages. One, we know it won't ever be the real system zone (and so
we're actually testing a case that would break). And two, since it has a
minute component, we're testing the full decoding of the +HHMM zone into
a number of seconds. Likewise, we test the "-1234" variant to make sure
there aren't any sign mistakes.

There's one final test, which covers "format-local:%s". As noted, this
already passes, but it's important to check that we didn't regress this
case. In particular, the caller in show_date() is relying on localtime()
to have done the zone adjustment, independent of any tz_offset we
compute ourselves. These should match up, since our local_tzoffset() is
likewise built around localtime(). But it would be easy for a caller to
forget to pass in a correct tz_offset to strbuf_addftime(). Fortunately
show_date() does this correctly (it has to because of the existing
handling of %z), and the test continues to pass. So this one is just
future-proofing against a change in our assumptions.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h         |  1 +
 date.c          |  2 +-
 strbuf.c        | 14 +++++++++++++-
 t/t0006-date.sh |  4 ++++
 4 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index eba12487b9..aa6f380d10 100644
--- a/cache.h
+++ b/cache.h
@@ -1588,6 +1588,7 @@ timestamp_t approxidate_careful(const char *, int *);
 timestamp_t approxidate_relative(const char *date);
 void parse_date_format(const char *format, struct date_mode *mode);
 int date_overflows(timestamp_t date);
+time_t tm_to_time_t(const struct tm *tm);
 
 #define IDENT_STRICT	       1
 #define IDENT_NO_DATE	       2
diff --git a/date.c b/date.c
index c55ea47e96..84bb4451c1 100644
--- a/date.c
+++ b/date.c
@@ -9,7 +9,7 @@
 /*
  * This is like mktime, but without normalization of tm_wday and tm_yday.
  */
-static time_t tm_to_time_t(const struct tm *tm)
+time_t tm_to_time_t(const struct tm *tm)
 {
 	static const int mdays[] = {
 	    0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334
diff --git a/strbuf.c b/strbuf.c
index b22e981655..a569b99ab9 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -1006,7 +1006,12 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
 
 	/*
 	 * There is no portable way to pass timezone information to
-	 * strftime, so we handle %z and %Z here.
+	 * strftime, so we handle %z and %Z here. Likewise '%s', because
+	 * going back to an epoch time requires knowing the zone.
+	 *
+	 * Note that tz_offset is in the "[-+]HHMM" decimal form; this is what
+	 * we want for %z, but the computation for %s has to convert to number
+	 * of seconds.
 	 */
 	for (;;) {
 		const char *percent = strchrnul(fmt, '%');
@@ -1019,6 +1024,13 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
 			strbuf_addstr(&munged_fmt, "%%");
 			fmt++;
 			break;
+		case 's':
+			strbuf_addf(&munged_fmt, "%"PRItime,
+				    tm_to_time_t(tm) -
+				    3600 * (tz_offset / 100) -
+				    60 * (tz_offset % 100));
+			fmt++;
+			break;
 		case 'z':
 			strbuf_addf(&munged_fmt, "%+05d", tz_offset);
 			fmt++;
diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index 6b757d7169..794186961e 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh
@@ -63,6 +63,10 @@ check_show 'format-local:%%z' "$TIME" '%z'
 check_show 'format:%Y-%m-%d %H:%M:%S' "$TIME" '2016-06-15 16:13:20'
 check_show 'format-local:%Y-%m-%d %H:%M:%S' "$TIME" '2016-06-15 09:13:20' '' EST5
 
+check_show 'format:%s' '123456789 +1234' 123456789
+check_show 'format:%s' '123456789 -1234' 123456789
+check_show 'format-local:%s' '123456789 -1234' 123456789
+
 # arbitrary time absurdly far in the future
 FUTURE="5758122296 -0400"
 check_show iso       "$FUTURE" "2152-06-19 18:24:56 -0400" TIME_IS_64BIT,TIME_T_IS_64BIT
-- 
2.34.0.rc0.612.g98bfd90890

Re: [PATCH] strbuf_addftime(): handle "%s" manually

[Jeff King]

On Tue, Nov 02, 2021 at 07:35:35AM -0400, Jeff King wrote:

> @@ -1019,6 +1024,13 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
>  			strbuf_addstr(&munged_fmt, "%%");
>  			fmt++;
>  			break;
> +		case 's':
> +			strbuf_addf(&munged_fmt, "%"PRItime,
> +				    tm_to_time_t(tm) -
> +				    3600 * (tz_offset / 100) -
> +				    60 * (tz_offset % 100));
> +			fmt++;
> +			break;

Looks like we may need something like this squashed in:

diff --git a/strbuf.c b/strbuf.c
index 33015b33df..995394f38e 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -1026,7 +1026,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
 			break;
 		case 's':
 			strbuf_addf(&munged_fmt, "%"PRItime,
-				    tm_to_time_t(tm) -
+				    (timestamp_t)tm_to_time_t(tm) -
 				    3600 * (tz_offset / 100) -
 				    60 * (tz_offset % 100));
 			fmt++;

because tm_to_time_t() returns an actual time_t, which will vary in
size. The 32-bit CI job complains:

  strbuf.c:1028:29: error: format '%lld' expects argument of type 'long long int', but argument 3 has type 'long int' [-Werror=format=]

-Peff

Re: [PATCH] strbuf_addftime(): handle "%s" manually

[Junio C Hamano]

I think this also needs squashing in?

 Documentation/rev-list-options.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git c/Documentation/rev-list-options.txt w/Documentation/rev-list-options.txt
index 24569b06d1..43a86fa562 100644
--- c/Documentation/rev-list-options.txt
+++ w/Documentation/rev-list-options.txt
@@ -1047,7 +1047,7 @@ omitted.
 has no effect.
 
 `--date=format:...` feeds the format `...` to your system `strftime`,
-except for %z and %Z, which are handled internally.
+except for %s, %z, and %Z, which are handled internally.
 Use `--date=format:%c` to show the date in your system locale's
 preferred format.  See the `strftime` manual for a complete list of
 format placeholders. When using `-local`, the correct syntax is

Re: [PATCH] strbuf_addftime(): handle "%s" manually

[Jeff King]

On Wed, Nov 03, 2021 at 01:28:00PM -0700, Junio C Hamano wrote:

> I think this also needs squashing in?
> 
>  Documentation/rev-list-options.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git c/Documentation/rev-list-options.txt w/Documentation/rev-list-options.txt
> index 24569b06d1..43a86fa562 100644
> --- c/Documentation/rev-list-options.txt
> +++ w/Documentation/rev-list-options.txt
> @@ -1047,7 +1047,7 @@ omitted.
>  has no effect.
>  
>  `--date=format:...` feeds the format `...` to your system `strftime`,
> -except for %z and %Z, which are handled internally.
> +except for %s, %z, and %Z, which are handled internally.
>  Use `--date=format:%c` to show the date in your system locale's
>  preferred format.  See the `strftime` manual for a complete list of
>  format placeholders. When using `-local`, the correct syntax is

Ah, thanks. I didn't even think to look in the documentation, because I
didn't imagine that we would expose these implementation details. But
since we do mention %z there, I think adding %s makes sense.

BTW, I also noticed that stftime supports some locale modifiers. So
"%Es" ends up printing the epoch seconds, but eludes our manual
intervention (and so does the old, wrong thing). I'm fine with stopping
here, though. There's no reason to use %Es over %s (from what I gather,
the %E is about handling year eras for locales that support them, but
that's meaningless for an epoch time), and I'm not sure it is even a
portable thing (glibc does not mention it in the manpage along with
other %E values, but it does work; POSIX does not even define %s, so of
course does not mention %Es).

-Peff