Re: [PATCH 1/1] Fix date checking in case if time was not initialized.
> 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] Fix date checking in case if time was not initialized.
Mike Gorchak 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] Fix date checking in case if time was not initialized.
>> 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] Fix date checking in case if time was not initialized.
Mike Gorchak 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? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] Fix date checking in case if time was not initialized.
> 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] Fix date checking in case if time was not initialized.
Mike Gorchak 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 +' > 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 +' > check_parse '2008-02-14 20:30:45 -5:' '2008-02-14 20:30:45 +' > 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 +' > 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... -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] Fix date checking in case if time was not initialized.
>> 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 +' 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 +' check_parse '2008-02-14 20:30:45 -5:' '2008-02-14 20:30:45 +' 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 +' 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] Fix date checking in case if time was not initialized.
Mike Gorchak 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
> Signed-off-by: Mike Gorchak
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] Fix date checking in case if time was not initialized.
Fix is_date() function failings in detection of correct date in case
if time was not properly initialized.
From: Mike Gorchak
Signed-off-by: Mike Gorchak
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
