[PATCH 1/2] Add date formatting and parsing functions relative to a given time

[PATCH 1/2] Add date formatting and parsing functions relative to a given time

[Alex Riesen]

The main purpose is to allow predictable testing of the code.
---

Rebased on current master.

 cache.h |    2 +
 date.c  |  150 ++++++++++++++++++++++++++++++++++++--------------------------
 2 files changed, 89 insertions(+), 63 deletions(-)

diff --git a/cache.h b/cache.h
index dd7f71e..3fb0166 100644
--- a/cache.h
+++ b/cache.h
@@ -731,9 +731,11 @@ enum date_mode {
 };
 
 const char *show_date(unsigned long time, int timezone, enum date_mode mode);
+const char *show_date_relative(unsigned long time, int tz, const struct timeval *now);
 int parse_date(const char *date, char *buf, int bufsize);
 void datestamp(char *buf, int bufsize);
 unsigned long approxidate(const char *);
+unsigned long approxidate_relative(const char *date, const struct timeval *now);
 enum date_mode parse_date_format(const char *format);
 
 #define IDENT_WARN_ON_NO_NAME  1
diff --git a/date.c b/date.c
index f011692..0840e77 100644
--- a/date.c
+++ b/date.c
@@ -84,6 +84,67 @@ static int local_tzoffset(unsigned long time)
 	return offset * eastwest;
 }
 
+const char *show_date_relative(unsigned long time, int tz, const struct timeval *now)
+{
+	static char timebuf[100 /* TODO: can be optimized */];
+	unsigned long diff;
+	if (now->tv_sec < time)
+		return "in the future";
+	diff = now->tv_sec - time;
+	if (diff < 90) {
+		snprintf(timebuf, sizeof(timebuf), "%lu seconds ago", diff);
+		return timebuf;
+	}
+	/* Turn it into minutes */
+	diff = (diff + 30) / 60;
+	if (diff < 90) {
+		snprintf(timebuf, sizeof(timebuf), "%lu minutes ago", diff);
+		return timebuf;
+	}
+	/* Turn it into hours */
+	diff = (diff + 30) / 60;
+	if (diff < 36) {
+		snprintf(timebuf, sizeof(timebuf), "%lu hours ago", diff);
+		return timebuf;
+	}
+	/* We deal with number of days from here on */
+	diff = (diff + 12) / 24;
+	if (diff < 14) {
+		snprintf(timebuf, sizeof(timebuf), "%lu days ago", diff);
+		return timebuf;
+	}
+	/* Say weeks for the past 10 weeks or so */
+	if (diff < 70) {
+		snprintf(timebuf, sizeof(timebuf), "%lu weeks ago", (diff + 3) / 7);
+		return timebuf;
+	}
+	/* Say months for the past 12 months or so */
+	if (diff < 360) {
+		snprintf(timebuf, sizeof(timebuf), "%lu months ago", (diff + 15) / 30);
+		return timebuf;
+	}
+	/* Give years and months for 5 years or so */
+	if (diff < 1825) {
+		unsigned long years = diff / 365;
+		unsigned long months = (diff % 365 + 15) / 30;
+		int n;
+		n = snprintf(timebuf, sizeof(timebuf), "%lu year%s",
+			     years, (years > 1 ? "s" : ""));
+		if (months)
+			snprintf(timebuf + n, sizeof(timebuf) - n,
+				 ", %lu month%s ago",
+				 months, (months > 1 ? "s" : ""));
+		else
+			snprintf(timebuf + n, sizeof(timebuf) - n,
+				 " ago");
+		return timebuf;
+	}
+	/* Otherwise, just years. Centuries is probably overkill. */
+	snprintf(timebuf, sizeof(timebuf), "%lu years ago", (diff + 183) / 365);
+	return timebuf;
+
+}
+
 const char *show_date(unsigned long time, int tz, enum date_mode mode)
 {
 	struct tm *tm;
@@ -95,63 +156,9 @@ const char *show_date(unsigned long time, int tz, enum date_mode mode)
 	}
 
 	if (mode == DATE_RELATIVE) {
-		unsigned long diff;
 		struct timeval now;
 		gettimeofday(&now, NULL);
-		if (now.tv_sec < time)
-			return "in the future";
-		diff = now.tv_sec - time;
-		if (diff < 90) {
-			snprintf(timebuf, sizeof(timebuf), "%lu seconds ago", diff);
-			return timebuf;
-		}
-		/* Turn it into minutes */
-		diff = (diff + 30) / 60;
-		if (diff < 90) {
-			snprintf(timebuf, sizeof(timebuf), "%lu minutes ago", diff);
-			return timebuf;
-		}
-		/* Turn it into hours */
-		diff = (diff + 30) / 60;
-		if (diff < 36) {
-			snprintf(timebuf, sizeof(timebuf), "%lu hours ago", diff);
-			return timebuf;
-		}
-		/* We deal with number of days from here on */
-		diff = (diff + 12) / 24;
-		if (diff < 14) {
-			snprintf(timebuf, sizeof(timebuf), "%lu days ago", diff);
-			return timebuf;
-		}
-		/* Say weeks for the past 10 weeks or so */
-		if (diff < 70) {
-			snprintf(timebuf, sizeof(timebuf), "%lu weeks ago", (diff + 3) / 7);
-			return timebuf;
-		}
-		/* Say months for the past 12 months or so */
-		if (diff < 360) {
-			snprintf(timebuf, sizeof(timebuf), "%lu months ago", (diff + 15) / 30);
-			return timebuf;
-		}
-		/* Give years and months for 5 years or so */
-		if (diff < 1825) {
-			unsigned long years = diff / 365;
-			unsigned long months = (diff % 365 + 15) / 30;
-			int n;
-			n = snprintf(timebuf, sizeof(timebuf), "%lu year%s",
-					years, (years > 1 ? "s" : ""));
-			if (months)
-				snprintf(timebuf + n, sizeof(timebuf) - n,
-					", %lu month%s ago",
-					months, (months > 1 ? "s" : ""));
-			else
-				snprintf(timebuf + n, sizeof(timebuf) - n,
-					" ago");
-			return timebuf;
-		}
-		/* Otherwise, just years. Centuries is probably overkill. */
-		snprintf(timebuf, sizeof(timebuf), "%lu years ago", (diff + 183) / 365);
-		return timebuf;
+		return show_date_relative(time, tz, &now);
 	}
 
 	if (mode == DATE_LOCAL)
@@ -866,19 +873,13 @@ static const char *approxidate_digit(const char *date, struct tm *tm, int *num)
 	return end;
 }
 
-unsigned long approxidate(const char *date)
+static unsigned long approximation(const char *date, const struct timeval *tv)
 {
 	int number = 0;
 	struct tm tm, now;
-	struct timeval tv;
 	time_t time_sec;
-	char buffer[50];
 
-	if (parse_date(date, buffer, sizeof(buffer)) > 0)
-		return strtoul(buffer, NULL, 10);
-
-	gettimeofday(&tv, NULL);
-	time_sec = tv.tv_sec;
+	time_sec = tv->tv_sec;
 	localtime_r(&time_sec, &tm);
 	now = tm;
 	for (;;) {
@@ -899,3 +900,26 @@ unsigned long approxidate(const char *date)
 		tm.tm_year--;
 	return mktime(&tm);
 }
+
+unsigned long approxidate_relative(const char *date, const struct timeval *tv)
+{
+	char buffer[50];
+
+	if (parse_date(date, buffer, sizeof(buffer)) > 0)
+		return strtoul(buffer, NULL, 10);
+
+	return approximation(date, tv);
+}
+
+unsigned long approxidate(const char *date)
+{
+	struct timeval tv;
+	char buffer[50];
+
+	if (parse_date(date, buffer, sizeof(buffer)) > 0)
+		return strtoul(buffer, NULL, 10);
+
+	gettimeofday(&tv, NULL);
+	return approximation(date, &tv);
+}
+
-- 
1.6.4.1.263.g468a

[PATCH 2/2] Allow testing of _relative family of time formatting and parsing functions

[Alex Riesen]

To complement the testability of approxidate.
---
 test-date.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/test-date.c b/test-date.c
index 62e8f23..dcc7973 100644
--- a/test-date.c
+++ b/test-date.c
@@ -4,6 +4,17 @@ int main(int argc, char **argv)
 {
 	int i;
 
+	struct tm tm;
+	struct timeval when = {0, 0};
+	tm.tm_sec  = 0;
+	tm.tm_min  = 0;
+	tm.tm_hour = 12;
+	tm.tm_mday = 1;
+	tm.tm_mon  = 0  /* January */;
+	tm.tm_year = 90 /* 1990 */ ;
+	tm.tm_isdst = -1;
+	when.tv_sec = mktime(&tm);
+
 	for (i = 1; i < argc; i++) {
 		char result[100];
 		time_t t;
@@ -15,6 +26,12 @@ int main(int argc, char **argv)
 
 		t = approxidate(argv[i]);
 		printf("%s -> %s\n", argv[i], ctime(&t));
+
+		t = approxidate_relative(argv[i], &when);
+		printf("relative: %s -> %s\n", argv[i], ctime(&t));
+
+		printf("relative: %s, out of %s\n",
+		       show_date_relative(t, 0, &when), ctime(&t));
 	}
 	return 0;
 }
-- 
1.6.4.1.263.g468a

Re: [PATCH 1/2] Add date formatting and parsing functions relative to a given time

[Jeff King]

On Fri, Aug 28, 2009 at 11:04:04PM +0200, Alex Riesen wrote:

> +const char *show_date_relative(unsigned long time, int tz, const struct timeval *now)
> +{
> +	static char timebuf[100 /* TODO: can be optimized */];

This was 200 in the original version. I doubt that it makes a
difference, but I think in a refactoring patch I think it is best to
simply reorganize and make no other changes.

> +static unsigned long approximation(const char *date, const struct timeval *tv)

I know it's static, but this is a terribly undescriptive function name.
Approximation of what?  Can we call it approxidate_internal or
something?

-Peff

Re: [PATCH 1/2] Add date formatting and parsing functions relative to a given time

[Alex Riesen]

On Sun, Aug 30, 2009 at 09:36, Jeff King<peff@peff.net> wrote:
> On Fri, Aug 28, 2009 at 11:04:04PM +0200, Alex Riesen wrote:
>
>> +const char *show_date_relative(unsigned long time, int tz, const struct timeval *now)
>> +{
>> +     static char timebuf[100 /* TODO: can be optimized */];
>
> This was 200 in the original version. I doubt that it makes a
> difference, but I think in a refactoring patch I think it is best to
> simply reorganize and make no other changes.

Yes, I just noticed that 200 was much too much, made the note
to fix it sometime and forgot the note in the final submission.

>> +static unsigned long approximation(const char *date, const struct timeval *tv)
>
> I know it's static, but this is a terribly undescriptive function name.
> Approximation of what?  Can we call it approxidate_internal or
> something?

Been there, tried that. Didn't like it, because it didn't feel enough
approxidate (the original) anymore. Not even internally, because
of missing parse_date. My other attempts were guessdate and
approxidate_bottom_half (but only very shortly).

The "approximation", if you consider the functions arguments,
seems to me the closest to what the function _is_. OTOH,
maybe I should have used a verb...

Re: [PATCH] Round-down years in "years+months" relative date view

[Jeff King]

On Fri, Aug 28, 2009 at 09:58:27AM +0200, Alex Riesen wrote:

> > I couldn't find any tests related to relative date processing, so it
> > would be really nice to have some. But I'm not sure of the best way to
> > do it without dealing with race conditions. Annoyingly, show_date calls
> > gettimeofday at a pretty low level, so there isn't a way of
> > instrumenting it short of LD_PRELOAD trickery (which is probably not
> > very portable).
> 
> Maybe better prepare the _test_ so that it uses current time and time
> arithmetics then put yet another cludge in operational code? Especially
> when we already have a greate number of GIT_ environment variables,
> documented nowhere, with effects not immediately obvious:

But that's the point: you can't do that without a race condition. Your
test gets a sense of the current time, then runs git, which checks the
current time again. How many seconds elapsed between the two checks?

I guess it is good enough for testing large time spans, but I was hoping
for a comprehensive time test.

-Peff

Re: [PATCH] Round-down years in "years+months" relative date view

[Nicolas Pitre]

On Fri, 28 Aug 2009, Jeff King wrote:

> On Fri, Aug 28, 2009 at 09:58:27AM +0200, Alex Riesen wrote:
> 
> > > I couldn't find any tests related to relative date processing, so it
> > > would be really nice to have some. But I'm not sure of the best way to
> > > do it without dealing with race conditions. Annoyingly, show_date calls
> > > gettimeofday at a pretty low level, so there isn't a way of
> > > instrumenting it short of LD_PRELOAD trickery (which is probably not
> > > very portable).
> > 
> > Maybe better prepare the _test_ so that it uses current time and time
> > arithmetics then put yet another cludge in operational code? Especially
> > when we already have a greate number of GIT_ environment variables,
> > documented nowhere, with effects not immediately obvious:
> 
> But that's the point: you can't do that without a race condition. Your
> test gets a sense of the current time, then runs git, which checks the
> current time again. How many seconds elapsed between the two checks?
> 
> I guess it is good enough for testing large time spans, but I was hoping
> for a comprehensive time test.

I agree with your concern.  This is why I created the --index-version 
switch to pack-objects.

However I was hoping for a current time trickery solution that could 
live in test-date.c instead of interfering with the main code in such a 
way.

Did a quick test to override the library version:

diff --git a/test-date.c b/test-date.c
index 62e8f23..0bcd0c9 100644
--- a/test-date.c
+++ b/test-date.c
@@ -1,5 +1,10 @@
 #include "cache.h"
 
+int gettimeofday(struct timeval *tv, struct timezone *tz)
+{
+	return 0;
+}
+
 int main(int argc, char **argv)
 {
 	int i;

Result:

$ ./test-date now
now -> bad -> Wed Dec 31 19:00:00 1969
now -> Tue Jan 22 10:48:24 10199

So this seems to work.  ;-)


Nicolas

Re: [PATCH] Round-down years in "years+months" relative date view

[Alex Riesen]

>From b51bc56816490c71cd37f52be73a06cef6b9bf14 Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Fri, 28 Aug 2009 20:59:59 +0200
Subject: [PATCH] Add date formatting functions with current time explicitely formatted

It should allow safe testing of this part of the code.
---
Nicolas Pitre, Fri, Aug 28, 2009 19:28:34 +0200:
> On Fri, 28 Aug 2009, Jeff King wrote:
> > On Fri, Aug 28, 2009 at 09:58:27AM +0200, Alex Riesen wrote:
> > 
> > > > I couldn't find any tests related to relative date processing, so it
> > > > would be really nice to have some. But I'm not sure of the best way to
> > > > do it without dealing with race conditions. Annoyingly, show_date calls
> > > > gettimeofday at a pretty low level, so there isn't a way of
> > > > instrumenting it short of LD_PRELOAD trickery (which is probably not
> > > > very portable).
> > > 
> > > Maybe better prepare the _test_ so that it uses current time and time
> > > arithmetics then put yet another cludge in operational code? Especially
> > > when we already have a greate number of GIT_ environment variables,
> > > documented nowhere, with effects not immediately obvious:
> > 
> > But that's the point: you can't do that without a race condition. Your
> > test gets a sense of the current time, then runs git, which checks the
> > current time again. How many seconds elapsed between the two checks?
> > 
> > I guess it is good enough for testing large time spans, but I was hoping
> > for a comprehensive time test.
> 
> I agree with your concern.  This is why I created the --index-version 
> switch to pack-objects.
> 

This is what I mean with "supplying current time":

 cache.h |    2 +
 date.c  |  130 ++++++++++++++++++++++++++++++++++----------------------------
 2 files changed, 73 insertions(+), 59 deletions(-)

diff --git a/cache.h b/cache.h
index dd7f71e..3fb0166 100644
--- a/cache.h
+++ b/cache.h
@@ -731,9 +731,11 @@ enum date_mode {
 };
 
 const char *show_date(unsigned long time, int timezone, enum date_mode mode);
+const char *show_date_relative(unsigned long time, int tz, const struct timeval *now);
 int parse_date(const char *date, char *buf, int bufsize);
 void datestamp(char *buf, int bufsize);
 unsigned long approxidate(const char *);
+unsigned long approxidate_relative(const char *date, const struct timeval *now);
 enum date_mode parse_date_format(const char *format);
 
 #define IDENT_WARN_ON_NO_NAME  1
diff --git a/date.c b/date.c
index 409a17d..08b4b49 100644
--- a/date.c
+++ b/date.c
@@ -84,6 +84,67 @@ static int local_tzoffset(unsigned long time)
 	return offset * eastwest;
 }
 
+const char *show_date_relative(unsigned long time, int tz, const struct timeval *now)
+{
+	static char timebuf[100 /* TODO: can be optimized */];
+	unsigned long diff;
+	if (now->tv_sec < time)
+		return "in the future";
+	diff = now->tv_sec - time;
+	if (diff < 90) {
+		snprintf(timebuf, sizeof(timebuf), "%lu seconds ago", diff);
+		return timebuf;
+	}
+	/* Turn it into minutes */
+	diff = (diff + 30) / 60;
+	if (diff < 90) {
+		snprintf(timebuf, sizeof(timebuf), "%lu minutes ago", diff);
+		return timebuf;
+	}
+	/* Turn it into hours */
+	diff = (diff + 30) / 60;
+	if (diff < 36) {
+		snprintf(timebuf, sizeof(timebuf), "%lu hours ago", diff);
+		return timebuf;
+	}
+	/* We deal with number of days from here on */
+	diff = (diff + 12) / 24;
+	if (diff < 14) {
+		snprintf(timebuf, sizeof(timebuf), "%lu days ago", diff);
+		return timebuf;
+	}
+	/* Say weeks for the past 10 weeks or so */
+	if (diff < 70) {
+		snprintf(timebuf, sizeof(timebuf), "%lu weeks ago", (diff + 3) / 7);
+		return timebuf;
+	}
+	/* Say months for the past 12 months or so */
+	if (diff < 360) {
+		snprintf(timebuf, sizeof(timebuf), "%lu months ago", (diff + 15) / 30);
+		return timebuf;
+	}
+	/* Give years and months for 5 years or so */
+	if (diff < 1825) {
+		unsigned long years = (diff + 183) / 365;
+		unsigned long months = (diff % 365 + 15) / 30;
+		int n;
+		n = snprintf(timebuf, sizeof(timebuf), "%lu year%s",
+			     years, (years > 1 ? "s" : ""));
+		if (months)
+			snprintf(timebuf + n, sizeof(timebuf) - n,
+				 ", %lu month%s ago",
+				 months, (months > 1 ? "s" : ""));
+		else
+			snprintf(timebuf + n, sizeof(timebuf) - n,
+				 " ago");
+		return timebuf;
+	}
+	/* Otherwise, just years. Centuries is probably overkill. */
+	snprintf(timebuf, sizeof(timebuf), "%lu years ago", (diff + 183) / 365);
+	return timebuf;
+
+}
+
 const char *show_date(unsigned long time, int tz, enum date_mode mode)
 {
 	struct tm *tm;
@@ -95,63 +156,9 @@ const char *show_date(unsigned long time, int tz, enum date_mode mode)
 	}
 
 	if (mode == DATE_RELATIVE) {
-		unsigned long diff;
 		struct timeval now;
 		gettimeofday(&now, NULL);
-		if (now.tv_sec < time)
-			return "in the future";
-		diff = now.tv_sec - time;
-		if (diff < 90) {
-			snprintf(timebuf, sizeof(timebuf), "%lu seconds ago", diff);
-			return timebuf;
-		}
-		/* Turn it into minutes */
-		diff = (diff + 30) / 60;
-		if (diff < 90) {
-			snprintf(timebuf, sizeof(timebuf), "%lu minutes ago", diff);
-			return timebuf;
-		}
-		/* Turn it into hours */
-		diff = (diff + 30) / 60;
-		if (diff < 36) {
-			snprintf(timebuf, sizeof(timebuf), "%lu hours ago", diff);
-			return timebuf;
-		}
-		/* We deal with number of days from here on */
-		diff = (diff + 12) / 24;
-		if (diff < 14) {
-			snprintf(timebuf, sizeof(timebuf), "%lu days ago", diff);
-			return timebuf;
-		}
-		/* Say weeks for the past 10 weeks or so */
-		if (diff < 70) {
-			snprintf(timebuf, sizeof(timebuf), "%lu weeks ago", (diff + 3) / 7);
-			return timebuf;
-		}
-		/* Say months for the past 12 months or so */
-		if (diff < 360) {
-			snprintf(timebuf, sizeof(timebuf), "%lu months ago", (diff + 15) / 30);
-			return timebuf;
-		}
-		/* Give years and months for 5 years or so */
-		if (diff < 1825) {
-			unsigned long years = (diff + 183) / 365;
-			unsigned long months = (diff % 365 + 15) / 30;
-			int n;
-			n = snprintf(timebuf, sizeof(timebuf), "%lu year%s",
-					years, (years > 1 ? "s" : ""));
-			if (months)
-				snprintf(timebuf + n, sizeof(timebuf) - n,
-					", %lu month%s ago",
-					months, (months > 1 ? "s" : ""));
-			else
-				snprintf(timebuf + n, sizeof(timebuf) - n,
-					" ago");
-			return timebuf;
-		}
-		/* Otherwise, just years. Centuries is probably overkill. */
-		snprintf(timebuf, sizeof(timebuf), "%lu years ago", (diff + 183) / 365);
-		return timebuf;
+		return show_date_relative(time, tz, &now);
 	}
 
 	if (mode == DATE_LOCAL)
@@ -866,19 +873,17 @@ static const char *approxidate_digit(const char *date, struct tm *tm, int *num)
 	return end;
 }
 
-unsigned long approxidate(const char *date)
+unsigned long approxidate_relative(const char *date, const struct timeval *tv)
 {
 	int number = 0;
 	struct tm tm, now;
-	struct timeval tv;
 	time_t time_sec;
 	char buffer[50];
 
 	if (parse_date(date, buffer, sizeof(buffer)) > 0)
 		return strtoul(buffer, NULL, 10);
 
-	gettimeofday(&tv, NULL);
-	time_sec = tv.tv_sec;
+	time_sec = tv->tv_sec;
 	localtime_r(&time_sec, &tm);
 	now = tm;
 	for (;;) {
@@ -899,3 +904,10 @@ unsigned long approxidate(const char *date)
 		tm.tm_year--;
 	return mktime(&tm);
 }
+
+unsigned long approxidate(const char *date)
+{
+	struct timeval tv;
+	gettimeofday(&tv, NULL);
+	return approxidate_relative(date, &tv);
+}
-- 
1.6.4.1.261.gf9874

Re: [PATCH] Round-down years in "years+months" relative date view

[Jeff King]

On Fri, Aug 28, 2009 at 09:03:19PM +0200, Alex Riesen wrote:

> +unsigned long approxidate(const char *date)
> +{
> +	struct timeval tv;
> +	gettimeofday(&tv, NULL);
> +	return approxidate_relative(date, &tv);
> +}

This now always calls gettimeofday, whereas the original approxidate
only did if parse_date failed.

I think you could also make this patch much smaller by just wrapping the
whole function and using a '0' sentinel for "you need to fill in the
time." Like:

---
diff --git a/date.c b/date.c
index 409a17d..b084d19 100644
--- a/date.c
+++ b/date.c
@@ -86,6 +86,14 @@ static int local_tzoffset(unsigned long time)
 
 const char *show_date(unsigned long time, int tz, enum date_mode mode)
 {
+	struct timeval now;
+	now.tv_sec = 0;
+	show_date_at_time(time, tz, mode, &now);
+}
+
+const char *show_date_at_time(unsigned long time, int tz, enum date_mode mode,
+		struct timeval now)
+{
 	struct tm *tm;
 	static char timebuf[200];
 
@@ -96,8 +104,8 @@ const char *show_date(unsigned long time, int tz, enum date_mode mode)
 
 	if (mode == DATE_RELATIVE) {
 		unsigned long diff;
-		struct timeval now;
-		gettimeofday(&now, NULL);
+		if (!now.tv_sec)
+			gettimeofday(&now, NULL);
 		if (now.tv_sec < time)
 			return "in the future";
 		diff = now.tv_sec - time;

On the other hand, refactoring the relative date code into its own
function is probably a good thing in the long run.

-Peff

Re: [PATCH] Round-down years in "years+months" relative date view

[Alex Riesen]

On Fri, Aug 28, 2009 at 21:15, Jeff King<peff@peff.net> wrote:
> On Fri, Aug 28, 2009 at 09:03:19PM +0200, Alex Riesen wrote:
>
>> +unsigned long approxidate(const char *date)
>> +{
>> +     struct timeval tv;
>> +     gettimeofday(&tv, NULL);
>> +     return approxidate_relative(date, &tv);
>> +}
>
> This now always calls gettimeofday, whereas the original approxidate
> only did if parse_date failed.

Oh, bugger...

> On the other hand, refactoring the relative date code into its own
> function is probably a good thing in the long run.

Exactly.

Re: [PATCH] Round-down years in "years+months" relative date view

[Alex Riesen]

From fe67532bdf095dc9ebc0c7dd67be384e807a197c Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Fri, 28 Aug 2009 20:59:59 +0200
Subject: [PATCH] Add date formatting functions with current time explicitely formatted

It should allow safe testing of this part of the code.
---
Alex Riesen, Fri, Aug 28, 2009 21:20:50 +0200:
> On Fri, Aug 28, 2009 at 21:15, Jeff King<peff@peff.net> wrote:
> > On Fri, Aug 28, 2009 at 09:03:19PM +0200, Alex Riesen wrote:
> >
> >> +unsigned long approxidate(const char *date)
> >> +{
> >> +     struct timeval tv;
> >> +     gettimeofday(&tv, NULL);
> >> +     return approxidate_relative(date, &tv);
> >> +}
> >
> > This now always calls gettimeofday, whereas the original approxidate
> > only did if parse_date failed.
> 
> Oh, bugger...
> 

Rather like this, I mean :)

 cache.h |    2 +
 date.c  |  150 ++++++++++++++++++++++++++++++++++++--------------------------
 2 files changed, 89 insertions(+), 63 deletions(-)

diff --git a/cache.h b/cache.h
index dd7f71e..3fb0166 100644
--- a/cache.h
+++ b/cache.h
@@ -731,9 +731,11 @@ enum date_mode {
 };
 
 const char *show_date(unsigned long time, int timezone, enum date_mode mode);
+const char *show_date_relative(unsigned long time, int tz, const struct timeval *now);
 int parse_date(const char *date, char *buf, int bufsize);
 void datestamp(char *buf, int bufsize);
 unsigned long approxidate(const char *);
+unsigned long approxidate_relative(const char *date, const struct timeval *now);
 enum date_mode parse_date_format(const char *format);
 
 #define IDENT_WARN_ON_NO_NAME  1
diff --git a/date.c b/date.c
index 409a17d..171e68f 100644
--- a/date.c
+++ b/date.c
@@ -84,6 +84,67 @@ static int local_tzoffset(unsigned long time)
 	return offset * eastwest;
 }
 
+const char *show_date_relative(unsigned long time, int tz, const struct timeval *now)
+{
+	static char timebuf[100 /* TODO: can be optimized */];
+	unsigned long diff;
+	if (now->tv_sec < time)
+		return "in the future";
+	diff = now->tv_sec - time;
+	if (diff < 90) {
+		snprintf(timebuf, sizeof(timebuf), "%lu seconds ago", diff);
+		return timebuf;
+	}
+	/* Turn it into minutes */
+	diff = (diff + 30) / 60;
+	if (diff < 90) {
+		snprintf(timebuf, sizeof(timebuf), "%lu minutes ago", diff);
+		return timebuf;
+	}
+	/* Turn it into hours */
+	diff = (diff + 30) / 60;
+	if (diff < 36) {
+		snprintf(timebuf, sizeof(timebuf), "%lu hours ago", diff);
+		return timebuf;
+	}
+	/* We deal with number of days from here on */
+	diff = (diff + 12) / 24;
+	if (diff < 14) {
+		snprintf(timebuf, sizeof(timebuf), "%lu days ago", diff);
+		return timebuf;
+	}
+	/* Say weeks for the past 10 weeks or so */
+	if (diff < 70) {
+		snprintf(timebuf, sizeof(timebuf), "%lu weeks ago", (diff + 3) / 7);
+		return timebuf;
+	}
+	/* Say months for the past 12 months or so */
+	if (diff < 360) {
+		snprintf(timebuf, sizeof(timebuf), "%lu months ago", (diff + 15) / 30);
+		return timebuf;
+	}
+	/* Give years and months for 5 years or so */
+	if (diff < 1825) {
+		unsigned long years = (diff + 183) / 365;
+		unsigned long months = (diff % 365 + 15) / 30;
+		int n;
+		n = snprintf(timebuf, sizeof(timebuf), "%lu year%s",
+			     years, (years > 1 ? "s" : ""));
+		if (months)
+			snprintf(timebuf + n, sizeof(timebuf) - n,
+				 ", %lu month%s ago",
+				 months, (months > 1 ? "s" : ""));
+		else
+			snprintf(timebuf + n, sizeof(timebuf) - n,
+				 " ago");
+		return timebuf;
+	}
+	/* Otherwise, just years. Centuries is probably overkill. */
+	snprintf(timebuf, sizeof(timebuf), "%lu years ago", (diff + 183) / 365);
+	return timebuf;
+
+}
+
 const char *show_date(unsigned long time, int tz, enum date_mode mode)
 {
 	struct tm *tm;
@@ -95,63 +156,9 @@ const char *show_date(unsigned long time, int tz, enum date_mode mode)
 	}
 
 	if (mode == DATE_RELATIVE) {
-		unsigned long diff;
 		struct timeval now;
 		gettimeofday(&now, NULL);
-		if (now.tv_sec < time)
-			return "in the future";
-		diff = now.tv_sec - time;
-		if (diff < 90) {
-			snprintf(timebuf, sizeof(timebuf), "%lu seconds ago", diff);
-			return timebuf;
-		}
-		/* Turn it into minutes */
-		diff = (diff + 30) / 60;
-		if (diff < 90) {
-			snprintf(timebuf, sizeof(timebuf), "%lu minutes ago", diff);
-			return timebuf;
-		}
-		/* Turn it into hours */
-		diff = (diff + 30) / 60;
-		if (diff < 36) {
-			snprintf(timebuf, sizeof(timebuf), "%lu hours ago", diff);
-			return timebuf;
-		}
-		/* We deal with number of days from here on */
-		diff = (diff + 12) / 24;
-		if (diff < 14) {
-			snprintf(timebuf, sizeof(timebuf), "%lu days ago", diff);
-			return timebuf;
-		}
-		/* Say weeks for the past 10 weeks or so */
-		if (diff < 70) {
-			snprintf(timebuf, sizeof(timebuf), "%lu weeks ago", (diff + 3) / 7);
-			return timebuf;
-		}
-		/* Say months for the past 12 months or so */
-		if (diff < 360) {
-			snprintf(timebuf, sizeof(timebuf), "%lu months ago", (diff + 15) / 30);
-			return timebuf;
-		}
-		/* Give years and months for 5 years or so */
-		if (diff < 1825) {
-			unsigned long years = (diff + 183) / 365;
-			unsigned long months = (diff % 365 + 15) / 30;
-			int n;
-			n = snprintf(timebuf, sizeof(timebuf), "%lu year%s",
-					years, (years > 1 ? "s" : ""));
-			if (months)
-				snprintf(timebuf + n, sizeof(timebuf) - n,
-					", %lu month%s ago",
-					months, (months > 1 ? "s" : ""));
-			else
-				snprintf(timebuf + n, sizeof(timebuf) - n,
-					" ago");
-			return timebuf;
-		}
-		/* Otherwise, just years. Centuries is probably overkill. */
-		snprintf(timebuf, sizeof(timebuf), "%lu years ago", (diff + 183) / 365);
-		return timebuf;
+		return show_date_relative(time, tz, &now);
 	}
 
 	if (mode == DATE_LOCAL)
@@ -866,19 +873,13 @@ static const char *approxidate_digit(const char *date, struct tm *tm, int *num)
 	return end;
 }
 
-unsigned long approxidate(const char *date)
+static unsigned long approximation(const char *date, const struct timeval *tv)
 {
 	int number = 0;
 	struct tm tm, now;
-	struct timeval tv;
 	time_t time_sec;
-	char buffer[50];
 
-	if (parse_date(date, buffer, sizeof(buffer)) > 0)
-		return strtoul(buffer, NULL, 10);
-
-	gettimeofday(&tv, NULL);
-	time_sec = tv.tv_sec;
+	time_sec = tv->tv_sec;
 	localtime_r(&time_sec, &tm);
 	now = tm;
 	for (;;) {
@@ -899,3 +900,26 @@ unsigned long approxidate(const char *date)
 		tm.tm_year--;
 	return mktime(&tm);
 }
+
+unsigned long approxidate_relative(const char *date, const struct timeval *tv)
+{
+	char buffer[50];
+
+	if (parse_date(date, buffer, sizeof(buffer)) > 0)
+		return strtoul(buffer, NULL, 10);
+
+	return approximation(date, tv);
+}
+
+unsigned long approxidate(const char *date)
+{
+	struct timeval tv;
+	char buffer[50];
+
+	if (parse_date(date, buffer, sizeof(buffer)) > 0)
+		return strtoul(buffer, NULL, 10);
+
+	gettimeofday(&tv, NULL);
+	return approximation(date, &tv);
+}
+
-- 
1.6.4.1.261.gf9874

[PATCH] Allow testing of _relative family of time formatting and parsing functions

[Alex Riesen]

To complement the testability of approxidate.
---
Alex Riesen, Fri, Aug 28, 2009 21:33:02 +0200:
> 
> It should allow safe testing of this part of the code.

And this should really allow testing of it:

    $ ./test-date '10.days.ago'
    10.days.ago -> bad -> Thu Jan  1 01:00:00 1970
    10.days.ago -> Tue Aug 18 22:50:20 2009

    relative: 10.days.ago -> Fri Dec 22 12:00:00 1989

    relative: 10 days ago, out of Fri Dec 22 12:00:00 1989

    $

According to Wikipedia, absolutely nothing of note happened
at the day 10 January, 1990.

 test-date.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/test-date.c b/test-date.c
index 62e8f23..dcc7973 100644
--- a/test-date.c
+++ b/test-date.c
@@ -4,6 +4,17 @@ int main(int argc, char **argv)
 {
 	int i;
 
+	struct tm tm;
+	struct timeval when = {0, 0};
+	tm.tm_sec  = 0;
+	tm.tm_min  = 0;
+	tm.tm_hour = 12;
+	tm.tm_mday = 1;
+	tm.tm_mon  = 0  /* January */;
+	tm.tm_year = 90 /* 1990 */ ;
+	tm.tm_isdst = -1;
+	when.tv_sec = mktime(&tm);
+
 	for (i = 1; i < argc; i++) {
 		char result[100];
 		time_t t;
@@ -15,6 +26,12 @@ int main(int argc, char **argv)
 
 		t = approxidate(argv[i]);
 		printf("%s -> %s\n", argv[i], ctime(&t));
+
+		t = approxidate_relative(argv[i], &when);
+		printf("relative: %s -> %s\n", argv[i], ctime(&t));
+
+		printf("relative: %s, out of %s\n",
+		       show_date_relative(t, 0, &when), ctime(&t));
 	}
 	return 0;
 }
-- 
1.6.4.1.263.g468a

Re: [PATCH] Allow testing of _relative family of time formatting and parsing functions

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> To complement the testability of approxidate.
> ---
> Alex Riesen, Fri, Aug 28, 2009 21:33:02 +0200:
>> 
>> It should allow safe testing of this part of the code.
>
> And this should really allow testing of it:
>
>     $ ./test-date '10.days.ago'
>     10.days.ago -> bad -> Thu Jan  1 01:00:00 1970
>     10.days.ago -> Tue Aug 18 22:50:20 2009
>
>     relative: 10.days.ago -> Fri Dec 22 12:00:00 1989
>
>     relative: 10 days ago, out of Fri Dec 22 12:00:00 1989
>
>     $

What are these blank lines for?  Is this intended as a serious submission
for inclusion?  I am having a hrad time to guess, especially you did not
sign this off, nor Cc'ed me.

Re: [PATCH] Allow testing of _relative family of time formatting and parsing functions

[Alex Riesen]

On Sat, Aug 29, 2009 at 23:46, Junio C Hamano<gitster@pobox.com> wrote:
> Alex Riesen <raa.lkml@gmail.com> writes:
>> To complement the testability of approxidate.
>> ---
>> Alex Riesen, Fri, Aug 28, 2009 21:33:02 +0200:
>>>
>>> It should allow safe testing of this part of the code.
>>
>> And this should really allow testing of it:
>>
>>     $ ./test-date '10.days.ago'
>>     10.days.ago -> bad -> Thu Jan  1 01:00:00 1970
>>     10.days.ago -> Tue Aug 18 22:50:20 2009
>>
>>     relative: 10.days.ago -> Fri Dec 22 12:00:00 1989
>>
>>     relative: 10 days ago, out of Fri Dec 22 12:00:00 1989
>>
>>     $
>
> What are these blank lines for?

ctime(3) artifact (it adds a \n in the output buffer), which I missed.

> Is this intended as a serious submission for inclusion?

Not yet. AFAICS, test-date is never used in our test suite.

> I am having a hrad time to guess, especially you did not
> sign this off, nor Cc'ed me.

Right, that's because I'm not sure myself. Frankly, I'm not
convinced we have to test every single thing. In my experience,
the bigger a test suite, the less are people inclined to use it
(including setting up automatic test runs).

Jeff, Nicolas? Is this test enough? Are there any other code
paths you want to include in the test?

And sorry for having you missed in Cc:, that wasn't intended.

[PATCH 1/2] Add date formatting and parsing functions relative to a given time

[Alex Riesen]

The main purpose is to allow predictable testing of the code.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

Have show_date_relative supplied the output buffer. As it is a new
interface, it can as well be a little bit more generic than its sole
caller. test-date.c is updated and shall follow in a moment.

And, after a while thinking, I am convinced that Jeff has a point
and used a more "internal" name for approxidate's recent "bottom half".

 cache.h |    5 ++
 date.c  |  152 +++++++++++++++++++++++++++++++++++++--------------------------
 2 files changed, 94 insertions(+), 63 deletions(-)

diff --git a/cache.h b/cache.h
index dd7f71e..1586f33 100644
--- a/cache.h
+++ b/cache.h
@@ -731,9 +731,14 @@ enum date_mode {
 };
 
 const char *show_date(unsigned long time, int timezone, enum date_mode mode);
+const char *show_date_relative(unsigned long time, int tz,
+			       const struct timeval *now,
+			       char *timebuf,
+			       size_t timebuf_size);
 int parse_date(const char *date, char *buf, int bufsize);
 void datestamp(char *buf, int bufsize);
 unsigned long approxidate(const char *);
+unsigned long approxidate_relative(const char *date, const struct timeval *now);
 enum date_mode parse_date_format(const char *format);
 
 #define IDENT_WARN_ON_NO_NAME  1
diff --git a/date.c b/date.c
index f011692..0b0f7a7 100644
--- a/date.c
+++ b/date.c
@@ -84,6 +84,68 @@ static int local_tzoffset(unsigned long time)
 	return offset * eastwest;
 }
 
+const char *show_date_relative(unsigned long time, int tz,
+			       const struct timeval *now,
+			       char *timebuf,
+			       size_t timebuf_size)
+{
+	unsigned long diff;
+	if (now->tv_sec < time)
+		return "in the future";
+	diff = now->tv_sec - time;
+	if (diff < 90) {
+		snprintf(timebuf, timebuf_size, "%lu seconds ago", diff);
+		return timebuf;
+	}
+	/* Turn it into minutes */
+	diff = (diff + 30) / 60;
+	if (diff < 90) {
+		snprintf(timebuf, timebuf_size, "%lu minutes ago", diff);
+		return timebuf;
+	}
+	/* Turn it into hours */
+	diff = (diff + 30) / 60;
+	if (diff < 36) {
+		snprintf(timebuf, timebuf_size, "%lu hours ago", diff);
+		return timebuf;
+	}
+	/* We deal with number of days from here on */
+	diff = (diff + 12) / 24;
+	if (diff < 14) {
+		snprintf(timebuf, timebuf_size, "%lu days ago", diff);
+		return timebuf;
+	}
+	/* Say weeks for the past 10 weeks or so */
+	if (diff < 70) {
+		snprintf(timebuf, timebuf_size, "%lu weeks ago", (diff + 3) / 7);
+		return timebuf;
+	}
+	/* Say months for the past 12 months or so */
+	if (diff < 360) {
+		snprintf(timebuf, timebuf_size, "%lu months ago", (diff + 15) / 30);
+		return timebuf;
+	}
+	/* Give years and months for 5 years or so */
+	if (diff < 1825) {
+		unsigned long years = diff / 365;
+		unsigned long months = (diff % 365 + 15) / 30;
+		int n;
+		n = snprintf(timebuf, timebuf_size, "%lu year%s",
+			     years, (years > 1 ? "s" : ""));
+		if (months)
+			snprintf(timebuf + n, timebuf_size - n,
+				 ", %lu month%s ago",
+				 months, (months > 1 ? "s" : ""));
+		else
+			snprintf(timebuf + n, timebuf_size - n,
+				 " ago");
+		return timebuf;
+	}
+	/* Otherwise, just years. Centuries is probably overkill. */
+	snprintf(timebuf, timebuf_size, "%lu years ago", (diff + 183) / 365);
+	return timebuf;
+}
+
 const char *show_date(unsigned long time, int tz, enum date_mode mode)
 {
 	struct tm *tm;
@@ -95,63 +157,10 @@ const char *show_date(unsigned long time, int tz, enum date_mode mode)
 	}
 
 	if (mode == DATE_RELATIVE) {
-		unsigned long diff;
 		struct timeval now;
 		gettimeofday(&now, NULL);
-		if (now.tv_sec < time)
-			return "in the future";
-		diff = now.tv_sec - time;
-		if (diff < 90) {
-			snprintf(timebuf, sizeof(timebuf), "%lu seconds ago", diff);
-			return timebuf;
-		}
-		/* Turn it into minutes */
-		diff = (diff + 30) / 60;
-		if (diff < 90) {
-			snprintf(timebuf, sizeof(timebuf), "%lu minutes ago", diff);
-			return timebuf;
-		}
-		/* Turn it into hours */
-		diff = (diff + 30) / 60;
-		if (diff < 36) {
-			snprintf(timebuf, sizeof(timebuf), "%lu hours ago", diff);
-			return timebuf;
-		}
-		/* We deal with number of days from here on */
-		diff = (diff + 12) / 24;
-		if (diff < 14) {
-			snprintf(timebuf, sizeof(timebuf), "%lu days ago", diff);
-			return timebuf;
-		}
-		/* Say weeks for the past 10 weeks or so */
-		if (diff < 70) {
-			snprintf(timebuf, sizeof(timebuf), "%lu weeks ago", (diff + 3) / 7);
-			return timebuf;
-		}
-		/* Say months for the past 12 months or so */
-		if (diff < 360) {
-			snprintf(timebuf, sizeof(timebuf), "%lu months ago", (diff + 15) / 30);
-			return timebuf;
-		}
-		/* Give years and months for 5 years or so */
-		if (diff < 1825) {
-			unsigned long years = diff / 365;
-			unsigned long months = (diff % 365 + 15) / 30;
-			int n;
-			n = snprintf(timebuf, sizeof(timebuf), "%lu year%s",
-					years, (years > 1 ? "s" : ""));
-			if (months)
-				snprintf(timebuf + n, sizeof(timebuf) - n,
-					", %lu month%s ago",
-					months, (months > 1 ? "s" : ""));
-			else
-				snprintf(timebuf + n, sizeof(timebuf) - n,
-					" ago");
-			return timebuf;
-		}
-		/* Otherwise, just years. Centuries is probably overkill. */
-		snprintf(timebuf, sizeof(timebuf), "%lu years ago", (diff + 183) / 365);
-		return timebuf;
+		return show_date_relative(time, tz, &now,
+					  timebuf, sizeof(timebuf));
 	}
 
 	if (mode == DATE_LOCAL)
@@ -866,19 +875,13 @@ static const char *approxidate_digit(const char *date, struct tm *tm, int *num)
 	return end;
 }
 
-unsigned long approxidate(const char *date)
+static unsigned long approxidate_str(const char *date, const struct timeval *tv)
 {
 	int number = 0;
 	struct tm tm, now;
-	struct timeval tv;
 	time_t time_sec;
-	char buffer[50];
 
-	if (parse_date(date, buffer, sizeof(buffer)) > 0)
-		return strtoul(buffer, NULL, 10);
-
-	gettimeofday(&tv, NULL);
-	time_sec = tv.tv_sec;
+	time_sec = tv->tv_sec;
 	localtime_r(&time_sec, &tm);
 	now = tm;
 	for (;;) {
@@ -899,3 +902,26 @@ unsigned long approxidate(const char *date)
 		tm.tm_year--;
 	return mktime(&tm);
 }
+
+unsigned long approxidate_relative(const char *date, const struct timeval *tv)
+{
+	char buffer[50];
+
+	if (parse_date(date, buffer, sizeof(buffer)) > 0)
+		return strtoul(buffer, NULL, 10);
+
+	return approxidate_str(date, tv);
+}
+
+unsigned long approxidate(const char *date)
+{
+	struct timeval tv;
+	char buffer[50];
+
+	if (parse_date(date, buffer, sizeof(buffer)) > 0)
+		return strtoul(buffer, NULL, 10);
+
+	gettimeofday(&tv, NULL);
+	return approxidate_str(date, &tv);
+}
+
-- 
1.6.4.1.294.g16262

[PATCH 2/2] Allow testing of _relative family of time formatting and parsing functions

[Alex Riesen]

To complement the testability of approxidate.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

Alex Riesen, Sun, Aug 30, 2009 11:13:46 +0200:
> test-date.c is updated and shall follow in a moment.

here it goes.

 test-date.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/test-date.c b/test-date.c
index 62e8f23..4cb146d 100644
--- a/test-date.c
+++ b/test-date.c
@@ -4,6 +4,19 @@ int main(int argc, char **argv)
 {
 	int i;
 
+	/* see date.c, function show_date_relative */
+	char timebuf[(sizeof(long) * 5 / 2 + sizeof(" minutes ago,")) * 2];
+	struct tm tm;
+	struct timeval when = {0, 0};
+	tm.tm_sec  = 0;
+	tm.tm_min  = 0;
+	tm.tm_hour = 12;
+	tm.tm_mday = 1;
+	tm.tm_mon  = 0  /* January */;
+	tm.tm_year = 90 /* 1990 */ ;
+	tm.tm_isdst = -1;
+	when.tv_sec = mktime(&tm);
+
 	for (i = 1; i < argc; i++) {
 		char result[100];
 		time_t t;
@@ -15,6 +28,13 @@ int main(int argc, char **argv)
 
 		t = approxidate(argv[i]);
 		printf("%s -> %s\n", argv[i], ctime(&t));
+
+		t = approxidate_relative(argv[i], &when);
+		printf("relative: %s -> %s", argv[i], ctime(&t));
+
+		printf("relative: %s, out of %s",
+		       show_date_relative(t, 0, &when, timebuf,sizeof(timebuf)),
+		       ctime(&t));
 	}
 	return 0;
 }
-- 
1.6.4.1.294.g16262