{"thread":{"id":"33004","subject":"[PATCH 1/1] Fix date checking in case if time was not initialized.","startedAt":"2013-02-25T08:36:29Z","lastAt":"2013-02-26T18:58:20Z","messageCount":9,"participants":["Mike Gorchak","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"210238","messageId":"CAHXAxrOOqn6ZSVT1AFyO3a3paD1tokBtcnaX68a+ddhodOvZ6Q@mail.gmail.com","threadId":"33004","inReplyTo":null,"subject":"[PATCH 1/1] Fix date checking in case if time was not initialized.","fromName":"Mike Gorchak","fromEmail":"mike.gorchak.qnx@gmail.com","sentAt":"2013-02-25T08:36:29Z","receivedAt":"2013-02-25T08:36:29Z","isPatch":true,"sender":{"key":"mike.gorchak.qnx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1337711?v=4"},"body":"Fix is_date() function failings in detection of correct date in case\nif time was not properly initialized.\n\nFrom: Mike Gorchak <mike.gorchak.qnx@gmail.com>\nSigned-off-by: Mike Gorchak <mike.gorchak.qnx@gmail.com>\n---\n date.c | 12 +++++++++++-\n 1 file changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/date.c b/date.c\nindex 57331ed..ec758f4 100644\n--- a/date.c\n+++ b/date.c\n@@ -357,6 +357,7 @@ static int is_date(int year, int month, int day,\nstruct tm *now_tm, time_t now,\n \tif (month > 0 && month < 13 && day > 0 && day < 32) {\n \t\tstruct tm check = *tm;\n \t\tstruct tm *r = (now_tm ? &check : tm);\n+\t\tstruct tm fixed_r;\n \t\ttime_t specified;\n\n \t\tr->tm_mon = month - 1;\n@@ -377,7 +378,16 @@ static int is_date(int year, int month, int day,\nstruct tm *now_tm, time_t now,\n \t\tif (!now_tm)\n \t\t\treturn 1;\n\n-\t\tspecified = tm_to_time_t(r);\n+\t\t/* Fix tm structure in case if time was not initialized */\n+\t\tfixed_r = *r;\n+\t\tif (fixed_r.tm_hour==-1)\n+\t\t\tfixed_r.tm_hour=0;\n+\t\tif (fixed_r.tm_min==-1)\n+\t\t\tfixed_r.tm_min=0;\n+\t\tif (fixed_r.tm_sec==-1)\n+\t\t\tfixed_r.tm_sec=0;\n+\n+\t\tspecified = tm_to_time_t(&fixed_r);\n\n \t\t/* Be it commit time or author time, it does not make\n \t\t * sense to specify timestamp way into the future.  Make\n-- \n1.8.2-rc0\n"},{"id":"210261","messageId":"7vzjysxnb1.fsf@alter.siamese.dyndns.org","threadId":"33004","inReplyTo":"CAHXAxrOOqn6ZSVT1AFyO3a3paD1tokBtcnaX68a+ddhodOvZ6Q@mail.gmail.com","subject":"Re: [PATCH 1/1] Fix date checking in case if time was not initialized.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-25T18:26:10Z","receivedAt":"2013-02-25T18:26:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Gorchak <mike.gorchak.qnx@gmail.com> writes:\n\n> Fix is_date() function failings in detection of correct date in case\n> if time was not properly initialized.\n\nPlease explain why this patch is needed and what problem this patch\nis trying to fix (if any) a bit better in the proposed log message.\nFor example, on what input do we call this function with partially\nfilled *r, and is that an error in the code, or is it an indication\nthat the input has only been consumed partially?\n\ntm_to_time_t() is designed to reject underspecified date input, and\nits callers call is_date() starting with partially filled tm on\npurpose to reject such input. Doesn't \"fixing\" partially filled tm\nbefore calling tm_to_time_t() inside is_date() break that logic?\n\n> From: Mike Gorchak <mike.gorchak.qnx@gmail.com>\n> Signed-off-by: Mike Gorchak <mike.gorchak.qnx@gmail.com>\n> ---\n>  date.c | 12 +++++++++++-\n>  1 file changed, 11 insertions(+), 1 deletion(-)\n>\n> diff --git a/date.c b/date.c\n> index 57331ed..ec758f4 100644\n> --- a/date.c\n> +++ b/date.c\n> @@ -357,6 +357,7 @@ static int is_date(int year, int month, int day,\n> struct tm *now_tm, time_t now,\n>  \tif (month > 0 && month < 13 && day > 0 && day < 32) {\n>  \t\tstruct tm check = *tm;\n>  \t\tstruct tm *r = (now_tm ? &check : tm);\n> +\t\tstruct tm fixed_r;\n>  \t\ttime_t specified;\n>\n>  \t\tr->tm_mon = month - 1;\n> @@ -377,7 +378,16 @@ static int is_date(int year, int month, int day,\n> struct tm *now_tm, time_t now,\n>  \t\tif (!now_tm)\n>  \t\t\treturn 1;\n>\n> -\t\tspecified = tm_to_time_t(r);\n> +\t\t/* Fix tm structure in case if time was not initialized */\n> +\t\tfixed_r = *r;\n> +\t\tif (fixed_r.tm_hour==-1)\n> +\t\t\tfixed_r.tm_hour=0;\n> +\t\tif (fixed_r.tm_min==-1)\n> +\t\t\tfixed_r.tm_min=0;\n> +\t\tif (fixed_r.tm_sec==-1)\n> +\t\t\tfixed_r.tm_sec=0;\n> +\n> +\t\tspecified = tm_to_time_t(&fixed_r);\n>\n>  \t\t/* Be it commit time or author time, it does not make\n>  \t\t * sense to specify timestamp way into the future.  Make\n"},{"id":"210267","messageId":"CAHXAxrMaQRdBxSvNO+no_9d==v0tVnkpXtguTKyfvnm-VfR_xA@mail.gmail.com","threadId":"33004","inReplyTo":"7vzjysxnb1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/1] Fix date checking in case if time was not initialized.","fromName":"Mike Gorchak","fromEmail":"mike.gorchak.qnx@gmail.com","sentAt":"2013-02-25T18:43:37Z","receivedAt":"2013-02-25T18:43:37Z","isPatch":true,"sender":{"key":"mike.gorchak.qnx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1337711?v=4"},"body":">> Fix is_date() function failings in detection of correct date in case\n>> if time was not properly initialized.\n>\n> Please explain why this patch is needed and what problem this patch\n> is trying to fix (if any) a bit better in the proposed log message.\n> For example, on what input do we call this function with partially\n> filled *r, and is that an error in the code, or is it an indication\n> that the input has only been consumed partially?\n\nfunction is_date() must not fail if time fields are not set. Currently\nis_date() invokes tm_to_time_t() which perform check of time fields.\nWith these fixes t0006-date.sh test is no longer fail on these tests:\n\ncheck_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 +0000'\ncheck_parse '2008-02-14 20:30:45 -0500' '2008-02-14 20:30:45 -0500'\ncheck_parse '2008-02-14 20:30:45 -0015' '2008-02-14 20:30:45 -0015'\ncheck_parse '2008-02-14 20:30:45 -5' '2008-02-14 20:30:45 +0000'\ncheck_parse '2008-02-14 20:30:45 -5:' '2008-02-14 20:30:45 +0000'\ncheck_parse '2008-02-14 20:30:45 -05' '2008-02-14 20:30:45 -0500'\ncheck_parse '2008-02-14 20:30:45 -:30' '2008-02-14 20:30:45 +0000'\ncheck_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'\n\nBy the way following test is always fails due to absence of EST5\ntimezone in timezones array in date.c module.\n\ncheck_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5\n\n> tm_to_time_t() is designed to reject underspecified date input, and\n> its callers call is_date() starting with partially filled tm on\n> purpose to reject such input. Doesn't \"fixing\" partially filled tm\n> before calling tm_to_time_t() inside is_date() break that logic?\n\nAccording to logic is_date() have no any relations with time\n(hours/mins/secs). In the tests described above date is defined first,\nbefore time, so at the point when date is checked for correctness time\nis not defined yet.\n\nThanks.\n"},{"id":"210273","messageId":"7vr4k4xlie.fsf@alter.siamese.dyndns.org","threadId":"33004","inReplyTo":"CAHXAxrMaQRdBxSvNO+no_9d==v0tVnkpXtguTKyfvnm-VfR_xA@mail.gmail.com","subject":"Re: [PATCH 1/1] Fix date checking in case if time was not initialized.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-25T19:04:57Z","receivedAt":"2013-02-25T19:04:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Gorchak <mike.gorchak.qnx@gmail.com> writes:\n\n>>> Fix is_date() function failings in detection of correct date in case\n>>> if time was not properly initialized.\n>>\n>> Please explain why this patch is needed and what problem this patch\n>> is trying to fix (if any) a bit better in the proposed log message.\n>> For example, on what input do we call this function with partially\n>> filled *r, and is that an error in the code, or is it an indication\n>> that the input has only been consumed partially?\n>\n> function is_date() must not fail if time fields are not set.\n\n> Currently is_date() invokes tm_to_time_t() which perform check of\n> time fields.  With these fixes t0006-date.sh test is no longer\n> fail on these tests:\n\nThe thing that puzzles me is that nobody reported that the following\nfail on their platforms (and they do not fail for me on platforms I\nhave to test in my real/virtual boxes).\n\n> check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 +0000'\n> check_parse '2008-02-14 20:30:45 -0500' '2008-02-14 20:30:45 -0500'\n> check_parse '2008-02-14 20:30:45 -0015' '2008-02-14 20:30:45 -0015'\n> check_parse '2008-02-14 20:30:45 -5' '2008-02-14 20:30:45 +0000'\n> check_parse '2008-02-14 20:30:45 -5:' '2008-02-14 20:30:45 +0000'\n> check_parse '2008-02-14 20:30:45 -05' '2008-02-14 20:30:45 -0500'\n> check_parse '2008-02-14 20:30:45 -:30' '2008-02-14 20:30:45 +0000'\n> check_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'\n\nSo there must be something _else_ going on, and I cannot tell what\nthat something else is from your code change nor from the log\nmessage.  I'd like to see that explained.\n\nStill puzzled...\n"},{"id":"210281","messageId":"CAHXAxrOjSS5jGLcCw4KTxP_F_uRQhi0cPSvzbx58jx9dP25XPA@mail.gmail.com","threadId":"33004","inReplyTo":"7vr4k4xlie.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/1] Fix date checking in case if time was not initialized.","fromName":"Mike Gorchak","fromEmail":"mike.gorchak.qnx@gmail.com","sentAt":"2013-02-25T19:32:52Z","receivedAt":"2013-02-25T19:32:52Z","isPatch":true,"sender":{"key":"mike.gorchak.qnx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1337711?v=4"},"body":"> The thing that puzzles me is that nobody reported that the following\n> fail on their platforms (and they do not fail for me on platforms I\n> have to test in my real/virtual boxes).\n\nOk, check_parse calls function parse_date(), it calls\nparse_date_basic(), where following code is present:\n\n\tmemset(&tm, 0, sizeof(tm));\n\ttm.tm_year = -1;\n\ttm.tm_mon = -1;\n\ttm.tm_mday = -1;\n\ttm.tm_isdst = -1;\n\ttm.tm_hour = -1;\n\ttm.tm_min = -1;\n\ttm.tm_sec = -1;\n\nAlmost all fields are set to -1. A bit later match_multi_number() is\ncalled. It parses the date and calls is_date() with partially filled\ntm structure, where all time fields: tm_hour, tm_min, tm_sec are set\nto -1. tm_to_time_t() function is called by is_date() and has special\ncheck:\n\n\tif (tm->tm_hour < 0 || tm->tm_min < 0 || tm->tm_sec < 0)\n\t\treturn -1;\n\nSo is_date() always return negative result for the text string where\ndate is placed before time like '2008-02-14 20:30:45'. It must fail on\nother platforms as well.\n"},{"id":"210283","messageId":"7va9qsxjzk.fsf@alter.siamese.dyndns.org","threadId":"33004","inReplyTo":"CAHXAxrOjSS5jGLcCw4KTxP_F_uRQhi0cPSvzbx58jx9dP25XPA@mail.gmail.com","subject":"Re: [PATCH 1/1] Fix date checking in case if time was not initialized.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-25T19:37:51Z","receivedAt":"2013-02-25T19:37:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Gorchak <mike.gorchak.qnx@gmail.com> writes:\n\n> \tif (tm->tm_hour < 0 || tm->tm_min < 0 || tm->tm_sec < 0)\n> \t\treturn -1;\n>\n> So is_date() always return negative result for the text string where\n> date is placed before time like '2008-02-14 20:30:45'.\n\nYes, it returns this -1 on other platforms, but...\n\n> It must fail on\n> other platforms as well.\n\n... the input '2008-02-14 20:30:45' still parses fine for others\n(including me).  That is what is puzzling.\n\nA shot in the dark: perhaps your time_t is unsigned?\n"},{"id":"210296","messageId":"CAHXAxrO4c=s0pjNpXK171HUbQT06jm-VAxNNK1DAqZEZfz6OtA@mail.gmail.com","threadId":"33004","inReplyTo":"7va9qsxjzk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/1] Fix date checking in case if time was not initialized.","fromName":"Mike Gorchak","fromEmail":"mike.gorchak.qnx@gmail.com","sentAt":"2013-02-25T21:47:47Z","receivedAt":"2013-02-25T21:47:47Z","isPatch":true,"sender":{"key":"mike.gorchak.qnx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1337711?v=4"},"body":">> So is_date() always return negative result for the text string where\n>> date is placed before time like '2008-02-14 20:30:45'.\n> Yes, it returns this -1 on other platforms, but...\n>> It must fail on\n>> other platforms as well.\n\nIt also fails under Linux, but real problem is not here, it is just an\nunoptimal date parser.\n\n> ... the input '2008-02-14 20:30:45' still parses fine for others\n> (including me).  That is what is puzzling.\n> A shot in the dark: perhaps your time_t is unsigned?\n\nYeah, it was a headshot :) It really due to unsigned time_t. I will\nsend two patches right now with fixes regarding unsigned time_t\ncomparison.\n"},{"id":"210300","messageId":"7v4nh0oxnv.fsf@alter.siamese.dyndns.org","threadId":"33004","inReplyTo":"CAHXAxrO4c=s0pjNpXK171HUbQT06jm-VAxNNK1DAqZEZfz6OtA@mail.gmail.com","subject":"Re: [PATCH 1/1] Fix date checking in case if time was not initialized.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-25T22:07:16Z","receivedAt":"2013-02-25T22:07:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Gorchak <mike.gorchak.qnx@gmail.com> writes:\n\n>>> So is_date() always return negative result for the text string where\n>>> date is placed before time like '2008-02-14 20:30:45'.\n>> Yes, it returns this -1 on other platforms, but...\n>>> It must fail on\n>>> other platforms as well.\n>\n> It also fails under Linux, but real problem is not here, it is just an\n> unoptimal date parser.\n\nThe test does _not_ fail.  That if condition does return -1 on Linux\nand BSD, and making tm_to_time_t() return a failure, but the caller\ngoes on, ending up with the right values in year/month/date in the\ntm struct, which is the primary thing the function is used for.\n\nAs you said, what is_date() wants to see is if the caller guessed\nthe order of three combinations of year/month/date correctly, it\ncannot necessarily assume the caller already has seen the\nhour/minutes/seconds yet, so _temporarily_ feeding a valud set of\nvalues to hour/minutes/seconds when calling tm_to_time_t() is a good\nworkaround.  So the change in your patch sounds correct and use of a\ntemporary tm to avoid contaminating the hour/minutes/seconds passed\nto the is_date() function while doing so looks good.\n\n>> ... the input '2008-02-14 20:30:45' still parses fine for others\n>> (including me).  That is what is puzzling.\n>\n>> A shot in the dark: perhaps your time_t is unsigned?\n>\n> Yeah, it was a headshot :) It really due to unsigned time_t. I will\n> send two patches right now with fixes regarding unsigned time_t\n> comparison.\n\nIf your time_t is unsigned, the error returned from tm_to_time_t()\nwill appear to be a time in a distant future, which will prevent\nis_date() to return \"Yeah, you guessed the order of year, month, and\ndate correctly\" to its caller.  The code would need to pick a safer\nmechanism to signal a failure from tm_to_time_t() to its callers.\n"},{"id":"210347","messageId":"CAHXAxrM=X9q_65VcVkc+1JQk9QCjvGi1rFhEDcPN27S9ed7yMA@mail.gmail.com","threadId":"33004","inReplyTo":"7v4nh0oxnv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/1] Fix date checking in case if time was not initialized.","fromName":"Mike Gorchak","fromEmail":"mike.gorchak.qnx@gmail.com","sentAt":"2013-02-26T18:58:20Z","receivedAt":"2013-02-26T18:58:20Z","isPatch":true,"sender":{"key":"mike.gorchak.qnx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1337711?v=4"},"body":"> The test does _not_ fail.  That if condition does return -1 on Linux\n> and BSD, and making tm_to_time_t() return a failure, but the caller\n> goes on, ending up with the right values in year/month/date in the\n> tm struct, which is the primary thing the function is used for.\n\nI said it wrong, test itself is not failed, but numerous is_date()\nchecks are failed due to incomplete time.\n"}]}