[PATCH 0/2] approxidate: tweak special date formats

[PATCH 0/2] approxidate: tweak special date formats

[Tuomas Ahola]

I made a couple of somewhat hacky fixes in approxidate special date
formats after noticing that some tests succeeded only because of the
specified test time which happened to be too late to spot some
irregularities.

Thanks a lot in advance!

Tuomas Ahola (2):
  approxidate: make "specials" respect fixed day-of-month
  approxidate: overwrite tm_mday for `now` and `yesterday`

 date.c          | 5 ++++-
 t/t0006-date.sh | 9 ++++++---
 2 files changed, 10 insertions(+), 4 deletions(-)


base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
-- 
2.30.2

[PATCH 1/2] approxidate: make "specials" respect fixed day-of-month

[Tuomas Ahola]

The behaviour of noon and tea depends on the current time even when
the date is given.  In other words, "last Friday tea" is dated to
Thursday if the command is run before 17 pm.

This can be fixed by checking whether tm->tm_mday already holds a
determined value and tested by setting current time before 12 or 17 pm
for noon and tea respectively.

Signed-off-by: Tuomas Ahola <taahol@utu.fi>
---
 date.c          | 2 +-
 t/t0006-date.sh | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/date.c b/date.c
index 17a95077cf..482a2f8c99 100644
--- a/date.c
+++ b/date.c
@@ -1133,7 +1133,7 @@ static void date_yesterday(struct tm *tm, struct tm *now, int *num)
 static void date_time(struct tm *tm, struct tm *now, int hour)
 {
 	if (tm->tm_hour < hour)
-		update_tm(tm, now, 24*60*60);
+		update_tm(tm, now, tm->tm_mday < 0 ? 24*60*60 : 0);
 	tm->tm_hour = hour;
 	tm->tm_min = 0;
 	tm->tm_sec = 0;
diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index 53ced36df4..5db4b23e0b 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh
@@ -180,7 +180,10 @@ check_approxidate '3:00' '2009-08-30 03:00:00'
 check_approxidate '15:00' '2009-08-30 15:00:00'
 check_approxidate 'noon today' '2009-08-30 12:00:00'
 check_approxidate 'noon yesterday' '2009-08-29 12:00:00'
-check_approxidate 'January 5th noon pm' '2009-01-05 12:00:00'
+(
+	GIT_TEST_DATE_NOW=$(($GIT_TEST_DATE_NOW-12*60*60)); export GIT_TEST_DATE_NOW
+	check_approxidate 'January 5th noon pm' '2009-01-05 12:00:00'
+)
 check_approxidate '10am noon' '2009-08-29 12:00:00'
 
 check_approxidate 'last tuesday' '2009-08-25 19:20:00'
-- 
2.30.2

Re: [PATCH 1/2] approxidate: make "specials" respect fixed day-of-month

[Jeff King]

On Tue, Mar 18, 2025 at 08:02:00PM +0200, Tuomas Ahola wrote:

> The behaviour of noon and tea depends on the current time even when
> the date is given.  In other words, "last Friday tea" is dated to
> Thursday if the command is run before 17 pm.

Knowing what approxidate's code is like, I'm not too surprised we'd have
corner cases like this. ;)

> This can be fixed by checking whether tm->tm_mday already holds a
> determined value and tested by setting current time before 12 or 17 pm
> for noon and tea respectively.

That makes sense for "last Friday tea", but should "tea last Friday" or
"noon last Friday" work, too? I suspect it plays quite badly with
approxidate's left-to-right parsing, so it might not be worth crossing
that bridge.

> --- a/date.c
> +++ b/date.c
> @@ -1133,7 +1133,7 @@ static void date_yesterday(struct tm *tm, struct tm *now, int *num)
>  static void date_time(struct tm *tm, struct tm *now, int hour)
>  {
>  	if (tm->tm_hour < hour)
> -		update_tm(tm, now, 24*60*60);
> +		update_tm(tm, now, tm->tm_mday < 0 ? 24*60*60 : 0);
>  	tm->tm_hour = hour;
>  	tm->tm_min = 0;
>  	tm->tm_sec = 0;

My reading of that conditional is "if the time computed so far is before
the specified hour, then go back a day to yesterday's version of it". So
"noon" would be yesterday's noon if it is only 11am.

But if we already have a date, I'd think we would skip that logic
entirely. I.e., do we need to call update_tm() at all? Certainly in your
patch we'd pass 0, which would mean no adjustment. Do we need any other
parts of update_tm()? It looks like it fills the month, day, and year
from the "now" struct if they aren't already set, but presumably they'd
all be set together (and if they're not, then that raises even more
questions about whether just checking tm_mday is correct in your patch).

So it seems like:

  /*
   * If we do not yet have a specified day, we'll use the most recent
   * version of "hour" relative to now. But that may be yesterday.
  */
  if (tm->tm_mday < 0 && tm->tm_hour < hour)
	update_tm(tm, now, 24*60*60);

would be equivalent and is IMHO a bit easier to understand.

> diff --git a/t/t0006-date.sh b/t/t0006-date.sh
> index 53ced36df4..5db4b23e0b 100755
> --- a/t/t0006-date.sh
> +++ b/t/t0006-date.sh
> @@ -180,7 +180,10 @@ check_approxidate '3:00' '2009-08-30 03:00:00'
>  check_approxidate '15:00' '2009-08-30 15:00:00'
>  check_approxidate 'noon today' '2009-08-30 12:00:00'
>  check_approxidate 'noon yesterday' '2009-08-29 12:00:00'
> -check_approxidate 'January 5th noon pm' '2009-01-05 12:00:00'
> +(
> +	GIT_TEST_DATE_NOW=$(($GIT_TEST_DATE_NOW-12*60*60)); export GIT_TEST_DATE_NOW
> +	check_approxidate 'January 5th noon pm' '2009-01-05 12:00:00'
> +)
>  check_approxidate '10am noon' '2009-08-29 12:00:00'

I'm glad there's a test, but two comments. One, wouldn't we still want
to keep the existing test to make sure that we continue to do the right
thing when the "now" hour is after noon?

And two, I'm pretty sure this sub-shell will confuse the test harness,
because you're running test_expect_success inside it. So any variable
updates won't be seen by the parent shell, and I wouldn't be surprised
if attempts to exit from "-i", etc, are broken.

Hmm, yeah, running the test yields this output:

  ok 110 - parse approxidate (noon yesterday)
  ok 111 - parse approxidate (January 5th noon pm)
  ok 111 - parse approxidate (10am noon)
  ok 112 - parse approxidate (last tuesday)

because the update of the test counter is lost in the sub-shell.

I don't think there's a trivial solution. You can't do a one-shot
variable-set like:

  TEST_DATE_NOW=... check_approxidate ...

because the behavior of that construct varies between shells. So you
have to either add support for an extra parameter to check_approxidate,
or just save and restore like:

  old_date=$GIT_TEST_DATE_NOW
  GIT_TEST_DATE_NOW=$((GIT_TEST_DATE_NOW-12*60*60))
  check_approxidate ...
  GIT_TEST_DATE_NOW=$old_date

It would be nice to add a comment there, too, on what the 12-hour
parameter means and what we expect. Maybe something like:

  # The "now" date is usually at 1900 in the evening. Roll it back to
  # the morning so that it is before the hour of named times like
  # "noon" and "tea".

-Peff

[PATCH 2/2] approxidate: overwrite tm_mday for `now` and `yesterday`

[Tuomas Ahola]

Date specifications now (or today) and yesterday should refer to
actual current or previous day.  Especially "noon today" or "noon
yesterday" should override the usual logic of using the first previous
noon depending on the current time.

Signed-off-by: Tuomas Ahola <taahol@utu.fi>
---
 date.c          | 3 +++
 t/t0006-date.sh | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/date.c b/date.c
index 482a2f8c99..2a8a942d64 100644
--- a/date.c
+++ b/date.c
@@ -1121,12 +1121,14 @@ static void pending_number(struct tm *tm, int *num)
 static void date_now(struct tm *tm, struct tm *now, int *num)
 {
 	*num = 0;
+	tm->tm_mday = -1;
 	update_tm(tm, now, 0);
 }
 
 static void date_yesterday(struct tm *tm, struct tm *now, int *num)
 {
 	*num = 0;
+	tm->tm_mday = -1;
 	update_tm(tm, now, 24*60*60);
 }
 
@@ -1204,6 +1206,7 @@ static const struct special {
 	{ "AM", date_am },
 	{ "never", date_never },
 	{ "now", date_now },
+	{ "today", date_now },
 	{ NULL }
 };
 
diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index 5db4b23e0b..6ad931dfb3 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh
@@ -178,10 +178,10 @@ check_approxidate '6am yesterday' '2009-08-29 06:00:00'
 check_approxidate '6pm yesterday' '2009-08-29 18:00:00'
 check_approxidate '3:00' '2009-08-30 03:00:00'
 check_approxidate '15:00' '2009-08-30 15:00:00'
-check_approxidate 'noon today' '2009-08-30 12:00:00'
-check_approxidate 'noon yesterday' '2009-08-29 12:00:00'
 (
 	GIT_TEST_DATE_NOW=$(($GIT_TEST_DATE_NOW-12*60*60)); export GIT_TEST_DATE_NOW
+	check_approxidate 'noon today' '2009-08-30 12:00:00'
+	check_approxidate 'noon yesterday' '2009-08-29 12:00:00'
 	check_approxidate 'January 5th noon pm' '2009-01-05 12:00:00'
 )
 check_approxidate '10am noon' '2009-08-29 12:00:00'
-- 
2.30.2

Re: [PATCH 2/2] approxidate: overwrite tm_mday for `now` and `yesterday`

[Jeff King]

On Tue, Mar 18, 2025 at 08:02:01PM +0200, Tuomas Ahola wrote:

> Date specifications now (or today) and yesterday should refer to
> actual current or previous day.  Especially "noon today" or "noon
> yesterday" should override the usual logic of using the first previous
> noon depending on the current time.

OK. So if I understand the issue correctly, it is that saying "noon"
sets the day field of the "struct tm", and then "today" or "yesterday"
won't override that, because update_tm() tries not to touch those fields
if they're already set.

This presumably works for "10am yesterday" because "10am" just sets the
time, and never the date (though I didn't check).

> diff --git a/date.c b/date.c
> index 482a2f8c99..2a8a942d64 100644
> --- a/date.c
> +++ b/date.c
> @@ -1121,12 +1121,14 @@ static void pending_number(struct tm *tm, int *num)
>  static void date_now(struct tm *tm, struct tm *now, int *num)
>  {
>  	*num = 0;
> +	tm->tm_mday = -1;
>  	update_tm(tm, now, 0);
>  }

So OK, this approach makes sense: throw out the old date we computed so
that update_tm() can use today's.

But is resetting tm_mday enough? If we previously set tm_mon, then
update_tm() won't override that, but it would need to follow along with
tm_mday, wouldn't it?

So if it is the morning of April 2nd, that will work fine, since
yesterday and today have the same month. And it seems to (this is with
your patch):

  $ GIT_TEST_DATE_NOW=$(date --date='2025-04-02 11:00' +%s) \
    t/helper/test-tool date approxidate 'noon today'
  noon today -> 2025-04-02 16:00:00 +0000

but if we try the same thing on the 1st:

  $ GIT_TEST_DATE_NOW=$(date --date='2025-04-01 11:00' +%s) \
    t/helper/test-tool date approxidate 'noon today'
  noon today -> 2025-03-01 16:00:00 +0000

Oops, we went back a month. And presumably the same thing would happen
with the year on Jan 1st.

Interestingly, since we are passing "0" to update_tm() as its
adjustment, I think this might be easier to read as just:

  tm->tm_mday = now->tm_mday;
  tm->tm_mon = now->tm_mon;
  tm->tm_year = now->tm_year;

But it's possible that the round-trip through mktime() and localtime_r()
in update_tm() has some value (I wonder what would happen at 2:30am on a
day when DST springs forward from 2am to 3am).

>  static void date_yesterday(struct tm *tm, struct tm *now, int *num)
>  {
>  	*num = 0;
> +	tm->tm_mday = -1;
>  	update_tm(tm, now, 24*60*60);
>  }

OK, same here, except obviously we _do_ need to call update_tm() for its
adjustment.

> @@ -1204,6 +1206,7 @@ static const struct special {
>  	{ "AM", date_am },
>  	{ "never", date_never },
>  	{ "now", date_now },
> +	{ "today", date_now },
>  	{ NULL }
>  };

So before we did not understand "today" at all, but just ignored it.
Which worked because we try to make everything relative to "now" by
default anyway. Adding it as you do here makes sense, since now we are
using its ability to "override" the date set by things like "noon".

It is a little weird that "now" and "today" share the same
implementation. So notably, "noon now" does not override the time
(ignoring "noon" completely). But that is already the case before your
patch (and I think is just one of those approxidate "if it hurts, don't
do it" quirks).

> --- a/t/t0006-date.sh
> +++ b/t/t0006-date.sh
> @@ -178,10 +178,10 @@ check_approxidate '6am yesterday' '2009-08-29 06:00:00'
>  check_approxidate '6pm yesterday' '2009-08-29 18:00:00'
>  check_approxidate '3:00' '2009-08-30 03:00:00'
>  check_approxidate '15:00' '2009-08-30 15:00:00'
> -check_approxidate 'noon today' '2009-08-30 12:00:00'
> -check_approxidate 'noon yesterday' '2009-08-29 12:00:00'
>  (
>  	GIT_TEST_DATE_NOW=$(($GIT_TEST_DATE_NOW-12*60*60)); export GIT_TEST_DATE_NOW
> +	check_approxidate 'noon today' '2009-08-30 12:00:00'
> +	check_approxidate 'noon yesterday' '2009-08-29 12:00:00'
>  	check_approxidate 'January 5th noon pm' '2009-01-05 12:00:00'
>  )

Same comments on the tests as in patch 1. I think we'd want to add,
rather than replace, and put these in the same save/restore block rather
than using a subshell.

-Peff