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

[Jeff King <peff@peff.net>]

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