[PATCH 1/1] Fix date checking in case if time was not initialized.

[PATCH 1/1] Fix date checking in case if time was not initialized.

[Mike Gorchak]

Fix is_date() function failings in detection of correct date in case
if time was not properly initialized.

From: Mike Gorchak <mike.gorchak.qnx@gmail.com>
Signed-off-by: Mike Gorchak <mike.gorchak.qnx@gmail.com>
---
 date.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/date.c b/date.c
index 57331ed..ec758f4 100644
--- a/date.c
+++ b/date.c
@@ -357,6 +357,7 @@ static int is_date(int year, int month, int day,
struct tm *now_tm, time_t now,
 	if (month > 0 && month < 13 && day > 0 && day < 32) {
 		struct tm check = *tm;
 		struct tm *r = (now_tm ? &check : tm);
+		struct tm fixed_r;
 		time_t specified;

 		r->tm_mon = month - 1;
@@ -377,7 +378,16 @@ static int is_date(int year, int month, int day,
struct tm *now_tm, time_t now,
 		if (!now_tm)
 			return 1;

-		specified = tm_to_time_t(r);
+		/* Fix tm structure in case if time was not initialized */
+		fixed_r = *r;
+		if (fixed_r.tm_hour==-1)
+			fixed_r.tm_hour=0;
+		if (fixed_r.tm_min==-1)
+			fixed_r.tm_min=0;
+		if (fixed_r.tm_sec==-1)
+			fixed_r.tm_sec=0;
+
+		specified = tm_to_time_t(&fixed_r);

 		/* Be it commit time or author time, it does not make
 		 * sense to specify timestamp way into the future.  Make
-- 
1.8.2-rc0

Re: [PATCH 1/1] Fix date checking in case if time was not initialized.

[Junio C Hamano]

Mike Gorchak <mike.gorchak.qnx@gmail.com> writes:

> Fix is_date() function failings in detection of correct date in case
> if time was not properly initialized.

Please explain why this patch is needed and what problem this patch
is trying to fix (if any) a bit better in the proposed log message.
For example, on what input do we call this function with partially
filled *r, and is that an error in the code, or is it an indication
that the input has only been consumed partially?

tm_to_time_t() is designed to reject underspecified date input, and
its callers call is_date() starting with partially filled tm on
purpose to reject such input. Doesn't "fixing" partially filled tm
before calling tm_to_time_t() inside is_date() break that logic?

> From: Mike Gorchak <mike.gorchak.qnx@gmail.com>
> Signed-off-by: Mike Gorchak <mike.gorchak.qnx@gmail.com>
> ---
>  date.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/date.c b/date.c
> index 57331ed..ec758f4 100644
> --- a/date.c
> +++ b/date.c
> @@ -357,6 +357,7 @@ static int is_date(int year, int month, int day,
> struct tm *now_tm, time_t now,
>  	if (month > 0 && month < 13 && day > 0 && day < 32) {
>  		struct tm check = *tm;
>  		struct tm *r = (now_tm ? &check : tm);
> +		struct tm fixed_r;
>  		time_t specified;
>
>  		r->tm_mon = month - 1;
> @@ -377,7 +378,16 @@ static int is_date(int year, int month, int day,
> struct tm *now_tm, time_t now,
>  		if (!now_tm)
>  			return 1;
>
> -		specified = tm_to_time_t(r);
> +		/* Fix tm structure in case if time was not initialized */
> +		fixed_r = *r;
> +		if (fixed_r.tm_hour==-1)
> +			fixed_r.tm_hour=0;
> +		if (fixed_r.tm_min==-1)
> +			fixed_r.tm_min=0;
> +		if (fixed_r.tm_sec==-1)
> +			fixed_r.tm_sec=0;
> +
> +		specified = tm_to_time_t(&fixed_r);
>
>  		/* Be it commit time or author time, it does not make
>  		 * sense to specify timestamp way into the future.  Make

Re: [PATCH 1/1] Fix date checking in case if time was not initialized.

[Mike Gorchak]

>> Fix is_date() function failings in detection of correct date in case
>> if time was not properly initialized.
>
> Please explain why this patch is needed and what problem this patch
> is trying to fix (if any) a bit better in the proposed log message.
> For example, on what input do we call this function with partially
> filled *r, and is that an error in the code, or is it an indication
> that the input has only been consumed partially?

function is_date() must not fail if time fields are not set. Currently
is_date() invokes tm_to_time_t() which perform check of time fields.
With these fixes t0006-date.sh test is no longer fail on these tests:

check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 +0000'
check_parse '2008-02-14 20:30:45 -0500' '2008-02-14 20:30:45 -0500'
check_parse '2008-02-14 20:30:45 -0015' '2008-02-14 20:30:45 -0015'
check_parse '2008-02-14 20:30:45 -5' '2008-02-14 20:30:45 +0000'
check_parse '2008-02-14 20:30:45 -5:' '2008-02-14 20:30:45 +0000'
check_parse '2008-02-14 20:30:45 -05' '2008-02-14 20:30:45 -0500'
check_parse '2008-02-14 20:30:45 -:30' '2008-02-14 20:30:45 +0000'
check_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'

By the way following test is always fails due to absence of EST5
timezone in timezones array in date.c module.

check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5

> tm_to_time_t() is designed to reject underspecified date input, and
> its callers call is_date() starting with partially filled tm on
> purpose to reject such input. Doesn't "fixing" partially filled tm
> before calling tm_to_time_t() inside is_date() break that logic?

According to logic is_date() have no any relations with time
(hours/mins/secs). In the tests described above date is defined first,
before time, so at the point when date is checked for correctness time
is not defined yet.

Thanks.

Re: [PATCH 1/1] Fix date checking in case if time was not initialized.

[Junio C Hamano]

Mike Gorchak <mike.gorchak.qnx@gmail.com> writes:

>>> Fix is_date() function failings in detection of correct date in case
>>> if time was not properly initialized.
>>
>> Please explain why this patch is needed and what problem this patch
>> is trying to fix (if any) a bit better in the proposed log message.
>> For example, on what input do we call this function with partially
>> filled *r, and is that an error in the code, or is it an indication
>> that the input has only been consumed partially?
>
> function is_date() must not fail if time fields are not set.

> Currently is_date() invokes tm_to_time_t() which perform check of
> time fields.  With these fixes t0006-date.sh test is no longer
> fail on these tests:

The thing that puzzles me is that nobody reported that the following
fail on their platforms (and they do not fail for me on platforms I
have to test in my real/virtual boxes).

> check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 +0000'
> check_parse '2008-02-14 20:30:45 -0500' '2008-02-14 20:30:45 -0500'
> check_parse '2008-02-14 20:30:45 -0015' '2008-02-14 20:30:45 -0015'
> check_parse '2008-02-14 20:30:45 -5' '2008-02-14 20:30:45 +0000'
> check_parse '2008-02-14 20:30:45 -5:' '2008-02-14 20:30:45 +0000'
> check_parse '2008-02-14 20:30:45 -05' '2008-02-14 20:30:45 -0500'
> check_parse '2008-02-14 20:30:45 -:30' '2008-02-14 20:30:45 +0000'
> check_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'

So there must be something _else_ going on, and I cannot tell what
that something else is from your code change nor from the log
message.  I'd like to see that explained.

Still puzzled...

Re: [PATCH 1/1] Fix date checking in case if time was not initialized.

[Mike Gorchak]

> The thing that puzzles me is that nobody reported that the following
> fail on their platforms (and they do not fail for me on platforms I
> have to test in my real/virtual boxes).

Ok, check_parse calls function parse_date(), it calls
parse_date_basic(), where following code is present:

	memset(&tm, 0, sizeof(tm));
	tm.tm_year = -1;
	tm.tm_mon = -1;
	tm.tm_mday = -1;
	tm.tm_isdst = -1;
	tm.tm_hour = -1;
	tm.tm_min = -1;
	tm.tm_sec = -1;

Almost all fields are set to -1. A bit later match_multi_number() is
called. It parses the date and calls is_date() with partially filled
tm structure, where all time fields: tm_hour, tm_min, tm_sec are set
to -1. tm_to_time_t() function is called by is_date() and has special
check:

	if (tm->tm_hour < 0 || tm->tm_min < 0 || tm->tm_sec < 0)
		return -1;

So is_date() always return negative result for the text string where
date is placed before time like '2008-02-14 20:30:45'. It must fail on
other platforms as well.

Re: [PATCH 1/1] Fix date checking in case if time was not initialized.

[Junio C Hamano]

Mike Gorchak <mike.gorchak.qnx@gmail.com> writes:

> 	if (tm->tm_hour < 0 || tm->tm_min < 0 || tm->tm_sec < 0)
> 		return -1;
>
> So is_date() always return negative result for the text string where
> date is placed before time like '2008-02-14 20:30:45'.

Yes, it returns this -1 on other platforms, but...

> It must fail on
> other platforms as well.

... the input '2008-02-14 20:30:45' still parses fine for others
(including me).  That is what is puzzling.

A shot in the dark: perhaps your time_t is unsigned?

Re: [PATCH 1/1] Fix date checking in case if time was not initialized.

[Mike Gorchak]

>> So is_date() always return negative result for the text string where
>> date is placed before time like '2008-02-14 20:30:45'.
> Yes, it returns this -1 on other platforms, but...
>> It must fail on
>> other platforms as well.

It also fails under Linux, but real problem is not here, it is just an
unoptimal date parser.

> ... the input '2008-02-14 20:30:45' still parses fine for others
> (including me).  That is what is puzzling.
> A shot in the dark: perhaps your time_t is unsigned?

Yeah, it was a headshot :) It really due to unsigned time_t. I will
send two patches right now with fixes regarding unsigned time_t
comparison.

Re: [PATCH 1/1] Fix date checking in case if time was not initialized.

[Junio C Hamano]

Mike Gorchak <mike.gorchak.qnx@gmail.com> writes:

>>> So is_date() always return negative result for the text string where
>>> date is placed before time like '2008-02-14 20:30:45'.
>> Yes, it returns this -1 on other platforms, but...
>>> It must fail on
>>> other platforms as well.
>
> It also fails under Linux, but real problem is not here, it is just an
> unoptimal date parser.

The test does _not_ fail.  That if condition does return -1 on Linux
and BSD, and making tm_to_time_t() return a failure, but the caller
goes on, ending up with the right values in year/month/date in the
tm struct, which is the primary thing the function is used for.

As you said, what is_date() wants to see is if the caller guessed
the order of three combinations of year/month/date correctly, it
cannot necessarily assume the caller already has seen the
hour/minutes/seconds yet, so _temporarily_ feeding a valud set of
values to hour/minutes/seconds when calling tm_to_time_t() is a good
workaround.  So the change in your patch sounds correct and use of a
temporary tm to avoid contaminating the hour/minutes/seconds passed
to the is_date() function while doing so looks good.

>> ... the input '2008-02-14 20:30:45' still parses fine for others
>> (including me).  That is what is puzzling.
>
>> A shot in the dark: perhaps your time_t is unsigned?
>
> Yeah, it was a headshot :) It really due to unsigned time_t. I will
> send two patches right now with fixes regarding unsigned time_t
> comparison.

If your time_t is unsigned, the error returned from tm_to_time_t()
will appear to be a time in a distant future, which will prevent
is_date() to return "Yeah, you guessed the order of year, month, and
date correctly" to its caller.  The code would need to pick a safer
mechanism to signal a failure from tm_to_time_t() to its callers.

Re: [PATCH 1/1] Fix date checking in case if time was not initialized.

[Mike Gorchak]

> The test does _not_ fail.  That if condition does return -1 on Linux
> and BSD, and making tm_to_time_t() return a failure, but the caller
> goes on, ending up with the right values in year/month/date in the
> tm struct, which is the primary thing the function is used for.

I said it wrong, test itself is not failed, but numerous is_date()
checks are failed due to incomplete time.