| Message ID | 20190118061805.19086-5-ischis2@cox.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Re-roll of 'human' date format patch set | expand |
"Stephen P. Smith" <ischis2@cox.net> writes: > Add the human format support to the test tool so that TEST_DATE_NOW > can be used to specify the current time. > > A static variable is used for passing the tool specified value to > get_date. The get_date helper function eliminates the need to > refactor up the show_date and show_date normal functions to pass the > time value. Hmph. An interesting approach, but the implementation is a bit too messy. > diff --git a/date.c b/date.c > index 43c3a84e25..24435b1e1d 100644 > --- a/date.c > +++ b/date.c > @@ -115,6 +115,28 @@ static int local_tzoffset(timestamp_t time) > return local_time_tzoffset((time_t)time, &tm); > } > > +const struct timeval *test_time = 0; Shouldn't this be file-scope static? Let BSS take care of initializing a variable to 0/NULL; drop " = 0" at the end. > +void show_date_human(timestamp_t time, int tz, > + const struct timeval *now, > + struct strbuf *timebuf) > +{ > + test_time = (const struct timeval *) now; > + strbuf_addstr( timebuf, show_date(time, tz, DATE_MODE(HUMAN))); Style: strbuf_addstr(timebuf, show_date(time, tz, DATE_MODE(HUMAN))); > + test_time = (const struct timeval *) 0; > +} It is a shame that you introduced a nicely reusable get_time() mechanism to let external callers of show_date() specify what time to format, instead of the returned timestamp of gettimeofday(), but limited its usefulness to only testing "human" format output. If somebody wants to extend "test-tool date" for other formats, they also have to add a similar "show_date_XXX" hack for their format. How about doing it slightly differently? E.g. - Get rid of show_date_human(). - Keep get_time(), but have it pay attention to GIT_TEST_TIMESTAMP environment variable, and when it is set, use that as if it is the returned value from gettimeofday(). - If there are gettimeofday() calls in date.c this patch did not touch (because they were not part of the "human-format" codepath), adjust them to use get_time() instead. - Have "test-tool date" excersize show_date() directly. > +static void get_time(struct timeval *now) > +{ > + if(test_time != 0) > + /* > + * If show_date was called via the test > + * interface use the test_tool time > + */ > + *now = *test_time; > + else > + gettimeofday(now, NULL); > +} > + > void show_date_relative(timestamp_t time, int tz, > const struct timeval *now, > struct strbuf *timebuf) > @@ -228,7 +250,7 @@ static void show_date_normal(struct strbuf *buf, timestamp_t time, struct tm *tm > /* Show "today" times as just relative times */ > if (hide.wday) { > struct timeval now; > - gettimeofday(&now, NULL); > + get_time(&now); > show_date_relative(time, tz, &now, buf); > return; > } > @@ -284,7 +306,7 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode) > if (mode->type == DATE_HUMAN) { > struct timeval now; > > - gettimeofday(&now, NULL); > + get_time(&now); > > /* Fill in the data for "current time" in human_tz and human_tm */ > human_tz = local_time_tzoffset(now.tv_sec, &human_tm); > diff --git a/t/helper/test-date.c b/t/helper/test-date.c > index a0837371ab..22d42a2174 100644 > --- a/t/helper/test-date.c > +++ b/t/helper/test-date.c > @@ -3,6 +3,7 @@ > > static const char *usage_msg = "\n" > " test-tool date relative [time_t]...\n" > +" test-tool date human [time_t]...\n" > " test-tool date show:<format> [time_t]...\n" > " test-tool date parse [date]...\n" > " test-tool date approxidate [date]...\n" > @@ -22,6 +23,18 @@ static void show_relative_dates(const char **argv, struct timeval *now) > strbuf_release(&buf); > } > > +static void show_human_dates(const char **argv, struct timeval *now) > +{ > + struct strbuf buf = STRBUF_INIT; > + > + for (; *argv; argv++) { > + time_t t = atoi(*argv); > + show_date_human(t, 0, now, &buf); > + printf("%s -> %s\n", *argv, buf.buf); > + } > + strbuf_release(&buf); > +} > + > static void show_dates(const char **argv, const char *format) > { > struct date_mode mode; > @@ -100,6 +113,8 @@ int cmd__date(int argc, const char **argv) > usage(usage_msg); > if (!strcmp(*argv, "relative")) > show_relative_dates(argv+1, &now); > + else if (!strcmp(*argv, "human")) > + show_human_dates(argv+1, &now); > else if (skip_prefix(*argv, "show:", &x)) > show_dates(argv+1, x); > else if (!strcmp(*argv, "parse"))
On Friday, January 18, 2019 12:03:40 PM MST Junio C Hamano wrote: > It is a shame that you introduced a nicely reusable get_time() > mechanism to let external callers of show_date() specify what time > to format, instead of the returned timestamp of gettimeofday(), > but limited its usefulness to only testing "human" format output. > If somebody wants to extend "test-tool date" for other formats, they > also have to add a similar "show_date_XXX" hack for their format. > > How about doing it slightly differently? E.g. > > - Get rid of show_date_human(). > > - Keep get_time(), but have it pay attention to GIT_TEST_TIMESTAMP > environment variable, and when it is set, use that as if it is > the returned value from gettimeofday(). > > - If there are gettimeofday() calls in date.c this patch did not > touch (because they were not part of the "human-format" > codepath), adjust them to use get_time() instead. > > - Have "test-tool date" excersize show_date() directly. > I did follow the pattern set for relative (which is why I created show_date_human() to mimic show_date_relative() ) as had been suggested. I like this pattern better. Why don't I create a second patch set after I git this one to next for relative to match your suggestion. I don't like the idea of conflating two topics. sps
"Stephen P. Smith" <ischis2@cox.net> writes: > Why don't I create a second patch set after I git this one to next for > relative to match your suggestion. I don't like the idea of conflating two > topics. I'd prefer two separate patches, too, and I'd prefer to see clean-up first and then a new feature. Thanks.
diff --git a/cache.h b/cache.h index 34c33e6a28..fe00ddf910 100644 --- a/cache.h +++ b/cache.h @@ -1467,6 +1467,8 @@ struct date_mode *date_mode_from_type(enum date_mode_type type); const char *show_date(timestamp_t time, int timezone, const struct date_mode *mode); void show_date_relative(timestamp_t time, int tz, const struct timeval *now, struct strbuf *timebuf); +void show_date_human(timestamp_t time, int tz, const struct timeval *now, + struct strbuf *timebuf); int parse_date(const char *date, struct strbuf *out); int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset); int parse_expiry_date(const char *date, timestamp_t *timestamp); diff --git a/date.c b/date.c index 43c3a84e25..24435b1e1d 100644 --- a/date.c +++ b/date.c @@ -115,6 +115,28 @@ static int local_tzoffset(timestamp_t time) return local_time_tzoffset((time_t)time, &tm); } +const struct timeval *test_time = 0; +void show_date_human(timestamp_t time, int tz, + const struct timeval *now, + struct strbuf *timebuf) +{ + test_time = (const struct timeval *) now; + strbuf_addstr( timebuf, show_date(time, tz, DATE_MODE(HUMAN))); + test_time = (const struct timeval *) 0; +} + +static void get_time(struct timeval *now) +{ + if(test_time != 0) + /* + * If show_date was called via the test + * interface use the test_tool time + */ + *now = *test_time; + else + gettimeofday(now, NULL); +} + void show_date_relative(timestamp_t time, int tz, const struct timeval *now, struct strbuf *timebuf) @@ -228,7 +250,7 @@ static void show_date_normal(struct strbuf *buf, timestamp_t time, struct tm *tm /* Show "today" times as just relative times */ if (hide.wday) { struct timeval now; - gettimeofday(&now, NULL); + get_time(&now); show_date_relative(time, tz, &now, buf); return; } @@ -284,7 +306,7 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode) if (mode->type == DATE_HUMAN) { struct timeval now; - gettimeofday(&now, NULL); + get_time(&now); /* Fill in the data for "current time" in human_tz and human_tm */ human_tz = local_time_tzoffset(now.tv_sec, &human_tm); diff --git a/t/helper/test-date.c b/t/helper/test-date.c index a0837371ab..22d42a2174 100644 --- a/t/helper/test-date.c +++ b/t/helper/test-date.c @@ -3,6 +3,7 @@ static const char *usage_msg = "\n" " test-tool date relative [time_t]...\n" +" test-tool date human [time_t]...\n" " test-tool date show:<format> [time_t]...\n" " test-tool date parse [date]...\n" " test-tool date approxidate [date]...\n" @@ -22,6 +23,18 @@ static void show_relative_dates(const char **argv, struct timeval *now) strbuf_release(&buf); } +static void show_human_dates(const char **argv, struct timeval *now) +{ + struct strbuf buf = STRBUF_INIT; + + for (; *argv; argv++) { + time_t t = atoi(*argv); + show_date_human(t, 0, now, &buf); + printf("%s -> %s\n", *argv, buf.buf); + } + strbuf_release(&buf); +} + static void show_dates(const char **argv, const char *format) { struct date_mode mode; @@ -100,6 +113,8 @@ int cmd__date(int argc, const char **argv) usage(usage_msg); if (!strcmp(*argv, "relative")) show_relative_dates(argv+1, &now); + else if (!strcmp(*argv, "human")) + show_human_dates(argv+1, &now); else if (skip_prefix(*argv, "show:", &x)) show_dates(argv+1, x); else if (!strcmp(*argv, "parse"))
Add the human format support to the test tool so that TEST_DATE_NOW can be used to specify the current time. A static variable is used for passing the tool specified value to get_date. The get_date helper function eliminates the need to refactor up the show_date and show_date normal functions to pass the time value. Signed-off-by: Stephen P. Smith <ischis2@cox.net> --- cache.h | 2 ++ date.c | 26 ++++++++++++++++++++++++-- t/helper/test-date.c | 15 +++++++++++++++ 3 files changed, 41 insertions(+), 2 deletions(-)