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

classic Classic list List threaded Threaded
47 messages Options
123
Reply | Threaded
Open this post in threaded view
|

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

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

Signed-off-by: David Reiss <[hidden email]>
---

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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Jeff King
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 <[hidden email]>

> 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;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Alex Riesen
On Fri, Aug 28, 2009 at 08:05, Jeff King<[hidden email]> wrote:

> 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 <[hidden email]>
>
>> 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?!!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Alex Riesen
On Fri, Aug 28, 2009 at 17:02, Jeff King<[hidden email]> 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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

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

> On Fri, Aug 28, 2009 at 17:02, Jeff King<[hidden email]> 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?

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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Nicolas Pitre
In reply to this post by Jeff King
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Jeff King
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Alex Riesen
In reply to this post by Jeff King
On Fri, Aug 28, 2009 at 19:15, Jeff King<[hidden email]> wrote:

> On Fri, Aug 28, 2009 at 07:00:59PM +0200, Alex Riesen wrote:
>
>> On Fri, Aug 28, 2009 at 17:02, Jeff King<[hidden email]> 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?
>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Alex Riesen
In reply to this post by Jeff King
On Fri, Aug 28, 2009 at 20:01, Jeff King<[hidden email]> wrote:

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

Microsoft's compiler and libraries? MacOSX?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

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

> > 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Alex Riesen
On Fri, Aug 28, 2009 at 20:39, Jeff King<[hidden email]> wrote:
> On Fri, Aug 28, 2009 at 08:27:06PM +0200, Alex Riesen wrote:
>
>> > 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Alex Riesen
On Fri, Aug 28, 2009 at 20:42, Alex Riesen<[hidden email]> wrote:
> On Fri, Aug 28, 2009 at 20:39, Jeff King<[hidden email]> wrote:
>> On Fri, Aug 28, 2009 at 08:27:06PM +0200, Alex Riesen wrote:
>>
>>> > 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Nicolas Pitre
In reply to this post by Alex Riesen
On Fri, 28 Aug 2009, Alex Riesen wrote:

> On Fri, Aug 28, 2009 at 20:39, Jeff King<[hidden email]> wrote:
> > On Fri, Aug 28, 2009 at 08:27:06PM +0200, Alex Riesen wrote:
> >
> >> > 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Alex Riesen
In reply to this post by Nicolas Pitre
From b51bc56816490c71cd37f52be73a06cef6b9bf14 Mon Sep 17 00:00:00 2001
From: Alex Riesen <[hidden email]>
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


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Alex Riesen
In reply to this post by Nicolas Pitre
On Fri, Aug 28, 2009 at 21:00, Nicolas Pitre<[hidden email]> wrote:

>> >> 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?

>> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Jeff King
In reply to this post by Alex Riesen
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Alex Riesen
On Fri, Aug 28, 2009 at 21:15, Jeff King<[hidden email]> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Nicolas Pitre
In reply to this post by Alex Riesen
On Fri, 28 Aug 2009, Alex Riesen wrote:

> On Fri, Aug 28, 2009 at 21:00, Nicolas Pitre<[hidden email]> wrote:
> >> >> 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
Reply | Threaded
Open this post in threaded view
|

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

Alex Riesen
In reply to this post by Alex Riesen
From fe67532bdf095dc9ebc0c7dd67be384e807a197c Mon Sep 17 00:00:00 2001
From: Alex Riesen <[hidden email]>
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<[hidden email]> 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
123