Bug: git log showing nothing when using --since and --until flags with specific dates

Hi all,

Apologies if this has already been raised or PEBCAK, but I've noticed a bug where git log with certain date ranges breaks things. It appears to be any --since date with a --until date in the future between 2014-12-01 and 2014-12-09. Dates from 2014-12-10 appear to work, and so does the date 2015-12-01.

Tested with the following versions:

git version 2.2.0.rc1.18.gf6f61cb on Ubuntu

git version 2.0.1 on whatever the latest version of OS X is.

To reproduce, on a git repo with a recent enough change to view a checkin after October 1 2014, run 'git log --since=2014-10-01 --until=2014-12-02' - no errors or anything to indicate the command failed are shown, now run 'git log --since=2014-10-01 --until=2014-12-10'.

Btw, the mail daemon appears to reject emails with '550 5.7.1 Content-Policy reject msg: The message contains HTML subpart, therefore we consider it SPAM or Outlook Virus. TEXT/PLAIN is accepted.! BF:<U 0.499737>; S1752168AbaKMAGd' - makes reporting bugs a bit of a hassle...

Cheers,

Colin Smith

Ugh. Approxidate strikes again:

for i in 2014-11-01 2013-12-01 2014-12-01; do
./test-date approxidate $i
done

produces:

2014-11-01 -> 2014-11-01 09:35:19 +0000
2013-12-01 -> 2013-12-01 09:35:19 +0000
2014-12-01 -> 2014-01-12 09:35:19 +0000

The first two are right, but the fourth one is not. It's probably something simple and stupid.

-Peff

Less simple and stupid than I thought, but I think I have a fix. It is not about December specifically, but about the date being in the future. The first patch is a cleanup to help us more accurately test the bug; the interesting bits are in the second one.

[1/2]: pass TIME_DATE_NOW to approxidate future-check [2/2]: approxidate: allow ISO-like dates far in the future

-Peff

The approxidate functions accept an extra "now" parameter to avoid calling time() themselves. We use this in our test suite to make sure we have a consistent time for computing relative dates. However, deep in the bowels of approxidate, we also call time() to check whether possible dates are far in the future. Let's make sure that the "now" override makes it to that spot, too, so we can consistently test that feature.

---
This code path also gets called from the more-strict parse_date (which does not know about the current time). This continues to call time() dynamically, but it is not clear to me if that ever actually matters in practice.

 date.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

index 59dfe57..e1a4d49 100644
@@ -405,9 +405,9 @@ static int is_date(int year, int month, int day, struct tm *now_tm, time_t now,
 	return 0;
 }
 
-static int match_multi_number(unsigned long num, char c, const char *date, char *end, struct tm *tm)
+static int match_multi_number(unsigned long num, char c, const char *date,
+			      char *end, struct tm *tm, time_t now)
 {
-	time_t now;
 	struct tm now_tm;
 	struct tm *refuse_future;
 	long num2, num3;

@@ -433,7 +433,8 @@ static int match_multi_number(unsigned long num, char c, const char *date, char
 	case '-':
 	case '/':
 	case '.':
-		now = time(NULL);
+		if (!now)
+			now = time(NULL);
 		refuse_future = NULL;
 		if (gmtime_r(&now, &now_tm))
 			refuse_future = &now_tm;

@@ -513,7 +514,7 @@ static int match_digit(const char *date, struct tm *tm, int *offset, int *tm_gmt
 	case '/':
 	case '-':
 		if (isdigit(end[1])) {
-			int match = match_multi_number(num, *end, date, end, tm);
+			int match = match_multi_number(num, *end, date, end, tm, 0);
 			if (match)
 				return match;
 		}

@@ -1013,7 +1014,8 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm
 	return end;
 }
 
-static const char *approxidate_digit(const char *date, struct tm *tm, int *num)
+static const char *approxidate_digit(const char *date, struct tm *tm, int *num,
+				     time_t now)
 {
 	char *end;
 	unsigned long number = strtoul(date, &end, 10);

@@ -1024,7 +1026,8 @@ static const char *approxidate_digit(const char *date, struct tm *tm, int *num)
 	case '/':
 	case '-':
 		if (isdigit(end[1])) {
-			int match = match_multi_number(number, *end, date, end, tm);
+			int match = match_multi_number(number, *end, date, end,
+						       tm, now);
 			if (match)
 				return date + match;
 		}

@@ -1087,7 +1090,7 @@ static unsigned long approxidate_str(const char *date,
 		date++;
 		if (isdigit(c)) {
 			pending_number(&tm, &number);
-			date = approxidate_digit(date-1, &tm, &number);
+			date = approxidate_digit(date-1, &tm, &number, time_sec);
 			touched = 1;
 			continue;
 		}

2.1.2.596.g7379948

When we are parsing approxidate strings and we find three numbers separate by one of ":/-.", we guess that it may be a date. We feed the numbers to match_multi_number, which checks whether it makes sense as a date in various orderings (e.g., dd/mm/yy or mm/dd/yy, etc).

One of the checks we do is to see whether it is a date more than 10 days in the future. This was added in 38035cf (date
parsing: be friendlier to our European friends., 2006-04-05), and lets us guess that if it is currently April 2014, then "10/03/2014" is probably March 10th, not October 3rd.

This has a downside, though; if you want to be overly generous with your "--until" date specification, we may wrongly parse "2014-12-01" as "2014-01-12" (because the latter is an in-the-past date). If the year is a future year (i.e., both are future dates), it gets even weirder. Due to the vagaries of approxidate, months _after_ the current date (no matter the year) get flipped, but ones before do not.

This patch drops the "in the future" check for dates of this form, letting us treat them always as yyyy-mm-dd, even if they are in the future. This does not affect the normal dd/mm/yyyy versus mm/dd/yyyy lookup, because this code path only kicks in when the first number is greater than 70 (i.e., it must be a year, and cannot be either a date or a month).

The one possible casualty is that "yyyy-dd-mm" is less likely to be chosen over "yyyy-mm-dd". That's probably OK, though because:

1. The difference happens only when the date is in the future. Already we prefer yyyy-mm-dd for dates in the past.

2. It's unclear whether anybody even uses yyyy-dd-mm regularly. It does not appear in lists of common date formats in Wikipedia[1,2].

3. Even if (2) is wrong, it is better to prefer ISO-like dates, as that is consistent with what we use elsewhere in git.

[1] http://en.wikipedia.org/wiki/Date_and_time_representation_by_country
[2] http://en.wikipedia.org/wiki/Calendar_date

---
I did not single out "yyyy-mm-dd" here versus "yyyy/mm/dd"; the change applies no matter which separator is used. If we wanted to be more conservative, we could apply it only to the more ISO-looking form with dashes, but I tend to think the reasoning makes sense no matter which separate you use (i.e., I do not think any variant with year then day is in common use).

 date.c          | 29 ++++++++++++++++-------------
 t/t0006-date.sh |  3 +++
 2 files changed, 19 insertions(+), 13 deletions(-)

index e1a4d49..b9eecfb 100644
@@ -363,7 +363,8 @@ static int match_alpha(const char *date, struct tm *tm, int *offset)
 	return skip_alpha(date);
 }
 
-static int is_date(int year, int month, int day, struct tm *now_tm, time_t now, struct tm *tm)
+static int is_date(int year, int month, int day, struct tm *now_tm, time_t now,
+		   struct tm *tm, int allow_future)
 {
 	if (month > 0 && month < 13 && day > 0 && day < 32) {
 		struct tm check = *tm;

@@ -388,14 +389,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);
+		if (!allow_future) {
+			specified = tm_to_time_t(r);
 
-		/* Be it commit time or author time, it does not make
-		 * sense to specify timestamp way into the future.  Make
-		 * sure it is not later than ten days from now...
-		 */
-		if ((specified != -1) && (now + 10*24*3600 < specified))
-			return 0;
+			/* Be it commit time or author time, it does not make
+			 * sense to specify timestamp way into the future.  Make
+			 * sure it is not later than ten days from now...
+			 */
+			if ((specified != -1) && (now + 10*24*3600 < specified))
+				return 0;
+		}
 		tm->tm_mon = r->tm_mon;
 		tm->tm_mday = r->tm_mday;
 		if (year != -1)

@@ -441,10 +444,10 @@ static int match_multi_number(unsigned long num, char c, const char *date,
 
 		if (num > 70) {
 			/* yyyy-mm-dd? */
-			if (is_date(num, num2, num3, refuse_future, now, tm))
+			if (is_date(num, num2, num3, refuse_future, now, tm, 1))
 				break;
 			/* yyyy-dd-mm? */
-			if (is_date(num, num3, num2, refuse_future, now, tm))
+			if (is_date(num, num3, num2, refuse_future, now, tm, 1))
 				break;
 		}
 		/* Our eastern European friends say dd.mm.yy[yy]

@@ -452,14 +455,14 @@ static int match_multi_number(unsigned long num, char c, const char *date,
 		 * mm/dd/yy[yy] form only when separator is not '.'
 		 */
 		if (c != '.' &&
-		    is_date(num3, num, num2, refuse_future, now, tm))
+		    is_date(num3, num, num2, refuse_future, now, tm, 0))
 			break;
 		/* European dd.mm.yy[yy] or funny US dd/mm/yy[yy] */
-		if (is_date(num3, num2, num, refuse_future, now, tm))
+		if (is_date(num3, num2, num, refuse_future, now, tm, 0))
 			break;
 		/* Funny European mm.dd.yy */
 		if (c == '.' &&
-		    is_date(num3, num, num2, refuse_future, now, tm))
+		    is_date(num3, num, num2, refuse_future, now, tm, 0))
 			break;
 		return 0;
 	}

index e53cf6d..fac0986 100755
@@ -82,4 +82,7 @@ check_approxidate 'Jun 6, 5AM' '2009-06-06 05:00:00'
 check_approxidate '5AM Jun 6' '2009-06-06 05:00:00'
 check_approxidate '6AM, June 7, 2009' '2009-06-07 06:00:00'
 
+check_approxidate '2008-12-01' '2008-12-01 19:20:00'
+check_approxidate '2009-12-01' '2009-12-01 19:20:00'
+
 test_done

2.1.2.596.g7379948

It is not good to omit any explanation and just include a link, like:

Fixes the bug reported in http://...

because people who are reading "git log" have to follow that link to even see what you are talking about (and the link might go away, or they might not have access at the time).

It is fine, and even desirable, to summarize the relevant content of a resource and provide a link for people who want to dig further. In this case, I am saying "Wikipedia claims that nobody uses this format" and backing it up with a link to indicate which pages I checked. You do not have to follow the link to know what I am saying, but if you want to dig deeper, you at least know where I left off my research.

Does that make sense?

-Peff

What you wrote matches the level of details I have been trying to stick to when writing references in my own log messages and when tweaking others' log messages.

Thanks.

To be honest, I had trouble figuring out what the name "refuse_future" really meant. We do skip the future check, but it also means that is_date will munge the "struct tm" directly, even if we do not find a valid date. That worried me a bit.

But yeah, in theory, the callers I wanted to tweak can just pass in a NULL refuse_future.

-Peff

So here's what the patch looks like just using refuse_future.

It's definitely nicer to read, and it passes the tests. But I am still concerned there is some unknown case that is impacted by us half-filling out the tm_mon and tm_mday fields of the "struct tm" in the first half of is_date.

-- >8 --
Subject: approxidate: allow ISO-like dates far in the future

When we are parsing approxidate strings and we find three numbers separate by one of ":/-.", we guess that it may be a date. We feed the numbers to match_multi_number, which checks whether it makes sense as a date in various orderings (e.g., dd/mm/yy or mm/dd/yy, etc).

One of the checks we do is to see whether it is a date more than 10 days in the future. This was added in 38035cf (date
parsing: be friendlier to our European friends., 2006-04-05), and lets us guess that if it is currently April 2014, then "10/03/2014" is probably March 10th, not October 3rd.

This has a downside, though; if you want to be overly generous with your "--until" date specification, we may wrongly parse "2014-12-01" as "2014-01-12" (because the latter is an in-the-past date). If the year is a future year (i.e., both are future dates), it gets even weirder. Due to the vagaries of approxidate, months _after_ the current date (no matter the year) get flipped, but ones before do not.

This patch drops the "in the future" check for dates of this form, letting us treat them always as yyyy-mm-dd, even if they are in the future. This does not affect the normal dd/mm/yyyy versus mm/dd/yyyy lookup, because this code path only kicks in when the first number is greater than 70 (i.e., it must be a year, and cannot be either a date or a month).

The one possible casualty is that "yyyy-dd-mm" is less likely to be chosen over "yyyy-mm-dd". That's probably OK, though because:

1. The difference happens only when the date is in the future. Already we prefer yyyy-mm-dd for dates in the past.

2. It's unclear whether anybody even uses yyyy-dd-mm regularly. It does not appear in lists of common date formats in Wikipedia[1,2].

3. Even if (2) is wrong, it is better to prefer ISO-like dates, as that is consistent with what we use elsewhere in git.

[1] http://en.wikipedia.org/wiki/Date_and_time_representation_by_country
[2] http://en.wikipedia.org/wiki/Calendar_date

---
 date.c          | 4 ++--
 t/t0006-date.sh | 3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

index e1a4d49..3eba2df 100644
@@ -441,10 +441,10 @@ static int match_multi_number(unsigned long num, char c, const char *date,
 
 		if (num > 70) {
 			/* yyyy-mm-dd? */
-			if (is_date(num, num2, num3, refuse_future, now, tm))
+			if (is_date(num, num2, num3, NULL, now, tm))
 				break;
 			/* yyyy-dd-mm? */
-			if (is_date(num, num3, num2, refuse_future, now, tm))
+			if (is_date(num, num3, num2, NULL, now, tm))
 				break;
 		}
 		/* Our eastern European friends say dd.mm.yy[yy]

index e53cf6d..fac0986 100755
@@ -82,4 +82,7 @@ check_approxidate 'Jun 6, 5AM' '2009-06-06 05:00:00'
 check_approxidate '5AM Jun 6' '2009-06-06 05:00:00'
 check_approxidate '6AM, June 7, 2009' '2009-06-07 06:00:00'
 
+check_approxidate '2008-12-01' '2008-12-01 19:20:00'
+check_approxidate '2009-12-01' '2009-12-01 19:20:00'
+
 test_done

2.1.2.596.g7379948

Ah, OK. That worries me, too, now you mention it. I just didn't see it myself ;-)