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

Previously, a commit from 1 year and 7 months ago would display as
"2 years, 7 months ago".

Signed-off-by: David Reiss <redacted>
---

Here's my test script.  Let me know if you'd rather have it as part
of the test suite.


#!/bin/sh
set -e
REPO="git-relative-dates-test"
rm -rf "$REPO"
mkdir "$REPO"
cd "$REPO"
git init
NOW=`date +%s`
env GIT_AUTHOR_DATE=`expr $NOW - \( 365 + 220 \) \* 24 \* 60 \* 60` git commit --allow-empty -m old-commit
git log --date=relative


 date.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/date.c b/date.c
index 1de1845..e848d96 100644
--- a/date.c
+++ b/date.c

@@ -137,7 +137,7 @@ const char *show_date(unsigned long time, int tz, enum date_mode mode)
 		}
 		/* Give years and months for 5 years or so */
 		if (diff < 1825) {
-			unsigned long years = (diff + 183) / 365;
+			unsigned long years = diff / 365;
 			unsigned long months = (diff % 365 + 15) / 30;
 			int n;
 			n = snprintf(timebuf, sizeof(timebuf), "%lu year%s",

-- 
1.6.0.4

On Thu, Aug 27, 2009 at 04:39:38PM -0700, David Reiss wrote:

Previously, a commit from 1 year and 7 months ago would display as
"2 years, 7 months ago".

Wow, embarrassing.

Acked-by: Jeff King <redacted>

Here's my test script.  Let me know if you'd rather have it as part
of the test suite.

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).

But maybe a patch like this is worth doing, which would allow us to test
in a repeatable fashion:

---

diff --git a/date.c b/date.c
index e848d96..db2f831 100644
--- a/date.c
+++ b/date.c

@@ -86,6 +86,33 @@ static int local_tzoffset(unsigned long time)
 	return offset * eastwest;
 }
 
+static int current_time(struct timeval *now)
+{
+	static struct timeval fake_time;
+	static int use_fake_time = -1;
+
+	if (use_fake_time == -1) {
+		const char *x = getenv("GIT_FAKE_TIME");
+		if (x) {
+			char buf[50];
+			if (parse_date(x, buf, sizeof(buf)) <= 0)
+				die("unable to parse GIT_FAKE_TIME");
+			fake_time.tv_sec = strtoul(buf, NULL, 10);
+			fake_time.tv_usec = 0;
+			use_fake_time = 1;
+		}
+		else
+			use_fake_time = 0;
+	}
+
+	if (use_fake_time == 1) {
+		memcpy(now, &fake_time, sizeof(*now));
+		return 0;
+	}
+
+	return gettimeofday(now, NULL);
+}
+
 const char *show_date(unsigned long time, int tz, enum date_mode mode)
 {
 	struct tm *tm;

@@ -99,7 +126,7 @@ 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);
+		current_time(&now);
 		if (now.tv_sec < time)
 			return "in the future";
 		diff = now.tv_sec - time;

@@ -929,7 +956,7 @@ unsigned long approxidate(const char *date)
 	if (parse_date(date, buffer, sizeof(buffer)) > 0)
 		return strtoul(buffer, NULL, 10);
 
-	gettimeofday(&tv, NULL);
+	current_time(&tv);
 	time_sec = tv.tv_sec;
 	localtime_r(&time_sec, &tm);
 	now = tm;

On Fri, Aug 28, 2009 at 08:05, Jeff King[off-list ref] wrote:

On Thu, Aug 27, 2009 at 04:39:38PM -0700, David Reiss wrote:

quoted

Previously, a commit from 1 year and 7 months ago would display as
"2 years, 7 months ago".

Wow, embarrassing.

Acked-by: Jeff King <redacted>

quoted

Here's my test script.  Let me know if you'd rather have it as part
of the test suite.

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:

  $ git grep -n '"GIT_'| perl -ne '/"(GIT_\w+)/ && print "$1\n"' |
sort |uniq | wc -l
  49

  $ git grep -n '"GIT_'|grep ^Documentation
  $

GIT_FLUSH? GIT_SEND_EMAIL_NOTTY?! GIT_CHERRY_PICK_HELP?!!

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

quoted

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

On Fri, Aug 28, 2009 at 17:02, Jeff King[off-list ref] wrote:

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?

How _many_ do you need?

On Fri, Aug 28, 2009 at 07:00:59PM +0200, Alex Riesen wrote:

On Fri, Aug 28, 2009 at 17:02, Jeff King[off-list ref] wrote:

quoted

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?

How _many_ do you need?

I don't understand what you're trying to say. My point is that if you
are checking results to a one-second precision, you need to know whether
zero seconds elapsed, or one second, or two seconds, or whatever to get
a consistent result.

-Peff

On Fri, 28 Aug 2009, Jeff King wrote:

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

quoted

quoted

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

On Fri, Aug 28, 2009 at 01:28:34PM -0400, Nicolas Pitre wrote:

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:

Thanks, that is a much better solution. And I don't know offhand of any
portability problems in overriding the library at link time.

-Peff

On Fri, Aug 28, 2009 at 19:15, Jeff King[off-list ref] wrote:

On Fri, Aug 28, 2009 at 07:00:59PM +0200, Alex Riesen wrote:

quoted

On Fri, Aug 28, 2009 at 17:02, Jeff King[off-list ref] wrote:

quoted

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?

How _many_ do you need?

I don't understand what you're trying to say. My point is that if you
are checking results to a one-second precision, you need to know whether
zero seconds elapsed, or one second, or two seconds, or whatever to get
a consistent result.

Taking this particular case as an example, can't we just set the time
of the _commit_ back in time? We can.
And we don't need to know the difference precisely, it can be
something like /[0-9]+ ago/, can't it?
Ok, it is possible, that something goes terribly wrong and the test suite
freezes for an extended period of time, so the pattern above does
not apply anymore. In this case, wont you prefer the test suite to
break? Ok, maybe not, if the freeze was an Ctrl-Z pressed at
unlucky moment. Which involves an operator online and looking,
and action and reaction will be both visible.

So, yes, it is not absolutely safe, but this approach has no side effects
on the working code. And very low probability of something go wrong.

On Fri, Aug 28, 2009 at 20:01, Jeff King[off-list ref] wrote:

On Fri, Aug 28, 2009 at 01:28:34PM -0400, Nicolas Pitre wrote:

quoted

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:

Thanks, that is a much better solution. And I don't know offhand of any
portability problems in overriding the library at link time.

Microsoft's compiler and libraries? MacOSX?

On Fri, Aug 28, 2009 at 08:27:06PM +0200, Alex Riesen wrote:

quoted

Thanks, that is a much better solution. And I don't know offhand of any
portability problems in overriding the library at link time.

Microsoft's compiler and libraries? MacOSX?

Are you saying you know those to be platforms with problems, or are you
asking whether those platforms will have problems?

-Peff

On Fri, Aug 28, 2009 at 20:39, Jeff King[off-list ref] wrote:

On Fri, Aug 28, 2009 at 08:27:06PM +0200, Alex Riesen wrote:

quoted

quoted

Thanks, that is a much better solution. And I don't know offhand of any
portability problems in overriding the library at link time.

Microsoft's compiler and libraries? MacOSX?

Are you saying you know those to be platforms with problems, or are you
asking whether those platforms will have problems?

Both: MS never had weak/vague linkage, but I don't know about MacOSX.
I suspect them to have the same deficiency, but I'd be glad to be wrong.

On Fri, Aug 28, 2009 at 20:42, Alex Riesen[off-list ref] wrote:

On Fri, Aug 28, 2009 at 20:39, Jeff King[off-list ref] wrote:

quoted

On Fri, Aug 28, 2009 at 08:27:06PM +0200, Alex Riesen wrote:

quoted

quoted

Thanks, that is a much better solution. And I don't know offhand of any
portability problems in overriding the library at link time.

Hm, how about supplying show_date and approxidate with current time?
Like this:

/* exported */
const char *show_date_rel(unsigned long time, int tz, struct timeval *now)
{
... the DATE_RELATIVE path of show_date
}

const char *show_date(unsigned long time, int tz, enum date_mode mode)
{
  struct timeval now;
  if (mode == DATE_RELATIVE) {
    gettimeofday(&now, NULL);
    return show_date_rel(time, tz, &now);
  }
  ... other paths of old show_date
}

Still affects the performance, but much less, and no side effects.
And you can test show_date_rel path in test-date.c

On Fri, 28 Aug 2009, Alex Riesen wrote:

On Fri, Aug 28, 2009 at 20:39, Jeff King[off-list ref] wrote:

quoted

On Fri, Aug 28, 2009 at 08:27:06PM +0200, Alex Riesen wrote:

quoted

quoted

Thanks, that is a much better solution. And I don't know offhand of any
portability problems in overriding the library at link time.

Microsoft's compiler and libraries? MacOSX?

Are you saying you know those to be platforms with problems, or are you
asking whether those platforms will have problems?

Both: MS never had weak/vague linkage, but I don't know about MacOSX.

This is not about weak or vague linkage.  This is plain basic linker 
feature where no library object needs to be linked if there is no symbol 
to resolve.

I suspect them to have the same deficiency, but I'd be glad to be wrong.

Are you able to test it?


Nicolas

From b51bc56816490c71cd37f52be73a06cef6b9bf14 Mon Sep 17 00:00:00 2001

From: Alex Riesen <redacted>
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:

quoted

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

quoted

quoted

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

On Fri, Aug 28, 2009 at 21:00, Nicolas Pitre[off-list ref] wrote:

quoted

quoted

quoted

Microsoft's compiler and libraries? MacOSX?

Are you saying you know those to be platforms with problems, or are you
asking whether those platforms will have problems?

Both: MS never had weak/vague linkage, but I don't know about MacOSX.

This is not about weak or vague linkage.  This is plain basic linker
feature where no library object needs to be linked if there is no symbol
to resolve.

Maybe I missed something, but wasn't the idea to overwrite gettimeofday
with a public gettimeofday, defined in one of the object files?
And shouldn't a linker complain regarding duplicated symbols, unless
the other (library) symbol is defined as a weak symbol, allowing
overriding it with another symbol of stronger linkage?

quoted

I suspect them to have the same deficiency, but I'd be glad to be wrong.

Are you able to test it?

Only the MS, but it is not interesting.

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

On Fri, Aug 28, 2009 at 21:15, Jeff King[off-list ref] wrote:

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

quoted

+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.

On Fri, 28 Aug 2009, Alex Riesen wrote:

On Fri, Aug 28, 2009 at 21:00, Nicolas Pitre[off-list ref] wrote:

quoted

quoted

quoted

quoted

Microsoft's compiler and libraries? MacOSX?

Are you saying you know those to be platforms with problems, or are you
asking whether those platforms will have problems?

Both: MS never had weak/vague linkage, but I don't know about MacOSX.

This is not about weak or vague linkage.  This is plain basic linker
feature where no library object needs to be linked if there is no symbol
to resolve.

Maybe I missed something, but wasn't the idea to overwrite gettimeofday
with a public gettimeofday, defined in one of the object files?

Yes, in test-date.o.

And shouldn't a linker complain regarding duplicated symbols, unless
the other (library) symbol is defined as a weak symbol, allowing
overriding it with another symbol of stronger linkage?

Normally a linker would search for new objects to link only when there 
are still symbols to resolve.  If the library is well architected (mind 
you I don't know if that is the case on Windows or OS X) you should find 
many small object files in a library, so to have only related functions 
together in a single object for only the needed code to be linked in the 
final binary. Hence the printf symbol should be in a separate object 
file than gettimeofday, etc.

Only if the library's object file containing gettimeofday also contains 
another symbol pulled by the linker will you see a duplicated symbol 
error.  But this is still a possibility.  So your proposal is probably 
cleaner.


Nicolas

From fe67532bdf095dc9ebc0c7dd67be384e807a197c Mon Sep 17 00:00:00 2001
From: Alex Riesen <redacted>
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[off-list ref] wrote:

quoted

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

quoted

+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

Nicolas Pitre, Fri, Aug 28, 2009 21:27:53 +0200:

On Fri, 28 Aug 2009, Alex Riesen wrote:

quoted

And shouldn't a linker complain regarding duplicated symbols, unless
the other (library) symbol is defined as a weak symbol, allowing
overriding it with another symbol of stronger linkage?

Normally a linker would search for new objects to link only when there 
are still symbols to resolve.  If the library is well architected (mind 
you I don't know if that is the case on Windows or OS X) you should find 
many small object files in a library, so to have only related functions 
together in a single object for only the needed code to be linked in the 
final binary. Hence the printf symbol should be in a separate object 
file than gettimeofday, etc.

Only if the library's object file containing gettimeofday also contains 
another symbol pulled by the linker will you see a duplicated symbol 
error.  But this is still a possibility.  So your proposal is probably 
cleaner.

Is it so for dynamic linking as well? Like in libc.so?

On Fri, 28 Aug 2009, Alex Riesen wrote:

Nicolas Pitre, Fri, Aug 28, 2009 21:27:53 +0200:

quoted

On Fri, 28 Aug 2009, Alex Riesen wrote:

quoted

And shouldn't a linker complain regarding duplicated symbols, unless
the other (library) symbol is defined as a weak symbol, allowing
overriding it with another symbol of stronger linkage?

Normally a linker would search for new objects to link only when there 
are still symbols to resolve.  If the library is well architected (mind 
you I don't know if that is the case on Windows or OS X) you should find 
many small object files in a library, so to have only related functions 
together in a single object for only the needed code to be linked in the 
final binary. Hence the printf symbol should be in a separate object 
file than gettimeofday, etc.

Only if the library's object file containing gettimeofday also contains 
another symbol pulled by the linker will you see a duplicated symbol 
error.  But this is still a possibility.  So your proposal is probably 
cleaner.

Is it so for dynamic linking as well? Like in libc.so?

Yes.  The linker still links against stubs in that case.


Nicolas

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

Alex Riesen, Fri, Aug 28, 2009 22:52:32 +0200:

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

1 Jan. 1990, not 10.

Jeff King wrote:

On Fri, Aug 28, 2009 at 07:00:59PM +0200, Alex Riesen wrote:

quoted

On Fri, Aug 28, 2009 at 17:02, Jeff King[off-list ref] wrote:

quoted

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?

How _many_ do you need?

I don't understand what you're trying to say. My point is that if you
are checking results to a one-second precision, you need to know whether
zero seconds elapsed, or one second, or two seconds, or whatever to get
a consistent result.

To no-one in particular, Gitzilla mumbles "To do this right(tm) would 
probably require LD_PRELOAD magic with a environment variable invocation.