[BUG] incorrect search result returned when using git log with a future date parameter

[BUG] incorrect search result returned when using git log with a future date parameter

[Caspar Zhang]

Hi there,

when I'm using the commit limit option `--before/--until` when doing 
`git log` search, I meet a bug when the upper-bound date is 10days later 
in the future. Here is an example:

$ date +%F
2013-01-30
$ git log --oneline --since=2013-01-01 --until=2013-02-01
<several git log entry from 2013-01-01 to 2013-01-30 printed>
$ git log --oneline --since=2013-01-01 --until=2013-02-13
<null>

I debugged into the problem with ./test-date program in git source tree, 
got:

$ ./test-date approxidate 2013-02-01
2013-02-01 -> 2013-02-01 10:47:13 +0000   // correctly parsed
$ ./test-date approxidate 2013-02-13
2013-02-13 -> 2013-01-02 10:47:20 +0000   // incorrectly parsed

When looking into the codes of date.c, in is_date() function, I found:

   382         /* Be it commit time or author time, it does not make
   383          * sense to specify timestamp way into the future.  Make
   384          * sure it is not later than ten days from now...
   385          */
   386         if (now + 10*24*3600 < specified)
   387                  return 0;
   388         tm->tm_mon = r->tm_mon;
   389         tm->tm_mday = r->tm_mday;
   390         if (year != -1)
   391                 tm->tm_year = r->tm_year;
   392         return 1;

If I comment Line 386 & 387 out, the parsing works correctly. So I guess 
here is the cause of the problem.

Then I checked the git history, the change was introduced in commit 
38035cf4 by Junio C Hamano (cc-ed):

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
commit 38035cf4a51c48cccf6c5e3977130261bc0c03a7
Author: Junio C Hamano <junkio@cox.net>
Date:   Wed Apr 5 15:31:12 2006 -0700

      date parsing: be friendlier to our European friends.

      This does three things, only applies to cases where the user
      manually tries to override the author/commit time by environment
      variables, with non-ISO, non-2822 format date-string:

       - Refuses to use the interpretation to put the date in the
         future; recent kernel history has a commit made with
         10/03/2006 which is recorded as October 3rd.

       - Adds '.' as the possible year-month-date separator.  We
         learned from our European friends on the #git channel that
         dd.mm.yyyy is the norm there.

       - When the separator is '.', we prefer dd.mm.yyyy over
         mm.dd.yyyy; otherwise mm/dd/yy[yy] takes precedence over
         dd/mm/yy[yy].

      Signed-off-by: Junio C Hamano <junkio@cox.net>
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

It seems like the original commit was going to fix European date style, 
but it fixed(?) the future date problem as well. However, this part of 
fix is not perfect:

1) it makes date parsing not working correctly. (see my test examples 
above).

IMO, it should be in another place (maybe in commit.c or somewhere 
else?) we check if commit date is valid or not, instead of in the date 
parsing function. A date parsing function should parse _all dates with 
correctly format_, despite if it's an old date, or the date in the future.

2) from the test example I gave above, in fact the codes don't really 
prevent git from accepting the changes with illegal date, e.g., if there 
is a commit recorded as "2013-02-13", it will be parsed to "2013-01-02", 
which is a legal (old) date, thus, this commit will be accepted, but 
this is wrong.

My suggestion is we might need to revert the first part of commit 
38035cf4 since this part of code doesn't work correctly and causes 
problems; then we should create a new checking mechanism to prevent 
those "future date commits" to be accepted in other functions. I'm not 
able to do the second part since I'm not familiar with git codes yet.. :-(

Thoughts?

Caspar

Re: [BUG] incorrect search result returned when using git log with a future date parameter

[Junio C Hamano]

Caspar Zhang <caspar@casparzhang.com> writes:

> .... A date parsing function should parse _all dates with
> correctly format_, despite if it's an old date, or the date in the
> future.

When it is fed 2013-02-12, it is ambiguous and "approxidate" can and
should use whatever heuristics (including rejection of future) to
guess what the user wanted, but 2013-02-13 cannot be interpreted in
any other way, so we should parse it as such.

Patches welcome, as long as the fix does not make things worse for
cases other than you observed.

Thanks.

Re: [BUG] incorrect search result returned when using git log with a future date parameter

[Jonathan Nieder]

Junio C Hamano wrote:

> When it is fed 2013-02-12, it is ambiguous and "approxidate" can and
> should use whatever heuristics (including rejection of future) to
> guess what the user wanted, but 2013-02-13 cannot be interpreted in
> any other way, so we should parse it as such.

FWIW, if you said 02/12/2013, I'd agree, but I've never seen someone
using 2013-02-12 to mean December 2.

So that suggests another possible tweak on top. :)

Thanks,
Jonathan

Re: [BUG] incorrect search result returned when using git log with a future date parameter

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> When it is fed 2013-02-12, it is ambiguous and "approxidate" can and
>> should use whatever heuristics (including rejection of future) to
>> guess what the user wanted, but 2013-02-13 cannot be interpreted in
>> any other way, so we should parse it as such.
>
> FWIW, if you said 02/12/2013, I'd agree, but I've never seen someone
> using 2013-02-12 to mean December 2.
>
> So that suggests another possible tweak on top. :)

I think in the original context that triggered the "be more friendly
to Europeans" discussion long time ago, some correlations between
the delimiting characters (i.e. 'xxxx-xx-xx' vs 'xxxx.xx.xx' vs
'xxxx/xx/xx') and month-day ordering convention.  I do not recall if
we actually added that as a signal when disambiguating, though.

[BUG] incorrect search result returned when using git log with a future date parameter

[Caspar Zhang]

Hi there,

when I'm using the commit limit option `--before/--until` when doing 
`git log` search, I meet a bug when the upper-bound date is 10days later 
in the future. Here is an example:

$ date +%F
2013-01-30
$ git log --oneline --since=2013-01-01 --until=2013-02-01
<several git log entry from 2013-01-01 to 2013-01-30 printed>
$ git log --oneline --since=2013-01-01 --until=2013-02-13
<null>

I debugged into the problem with ./test-date program in git source tree, 
got:

$ ./test-date approxidate 2013-02-01
2013-02-01 -> 2013-02-01 10:47:13 +0000   // correctly parsed
$ ./test-date approxidate 2013-02-13
2013-02-13 -> 2013-01-02 10:47:20 +0000   // incorrectly parsed

When looking into the codes of date.c, in is_date() function, I found:

  382         /* Be it commit time or author time, it does not make
  383          * sense to specify timestamp way into the future.  Make
  384          * sure it is not later than ten days from now...
  385          */
  386         if (now + 10*24*3600 < specified)
  387                  return 0;
  388         tm->tm_mon = r->tm_mon;
  389         tm->tm_mday = r->tm_mday;
  390         if (year != -1)
  391                 tm->tm_year = r->tm_year;
  392         return 1;

If I comment Line 386 & 387 out, the parsing works correctly. So I guess 
here is the cause of the problem.

Then I checked the git history, the change was introduced in commit 
38035cf4 by Junio C Hamano (cc-ed):

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
commit 38035cf4a51c48cccf6c5e3977130261bc0c03a7
Author: Junio C Hamano <junkio@cox.net>
Date:   Wed Apr 5 15:31:12 2006 -0700

     date parsing: be friendlier to our European friends.

     This does three things, only applies to cases where the user
     manually tries to override the author/commit time by environment
     variables, with non-ISO, non-2822 format date-string:

      - Refuses to use the interpretation to put the date in the
        future; recent kernel history has a commit made with
        10/03/2006 which is recorded as October 3rd.

      - Adds '.' as the possible year-month-date separator.  We
        learned from our European friends on the #git channel that
        dd.mm.yyyy is the norm there.

      - When the separator is '.', we prefer dd.mm.yyyy over
        mm.dd.yyyy; otherwise mm/dd/yy[yy] takes precedence over
        dd/mm/yy[yy].

     Signed-off-by: Junio C Hamano <junkio@cox.net>
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

It seems like the original commit was going to fix European date style, 
but it fixed(?) the future date problem as well. However, this part of 
fix is not perfect:

1) it makes date parsing not working correctly. (see my test examples 
above).

IMO, it should be in another place (maybe in commit.c or somewhere 
else?) we check if commit date is valid or not, instead of in the date 
parsing function. A date parsing function should parse _all dates with 
correctly format_, despite if it's an old date, or the date in the future.

2) from the test example I gave above, in fact the codes don't really 
prevent git from accepting the changes with illegal date, e.g., if there 
is a commit recorded as "2013-02-13", it will be parsed to "2013-01-02", 
which is a legal (old) date, thus, this commit will be accepted, but 
this is wrong.

My suggestion is we might need to revert the first part of commit 
38035cf4 since this part of code doesn't work correctly and causes 
problems; then we should create a new checking mechanism to prevent 
those "future date commits" to be accepted in other functions. I'm not 
able to do the second part since I'm not familiar with git codes yet.. :-(

Thoughts?

Caspar