git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH 0/3] Add 'human' date format
@ 2018-12-31  0:31 Stephen P. Smith
  2018-12-31  0:31 ` [PATCH 1/3] " Stephen P. Smith
                   ` (3 more replies)
  0 siblings, 4 replies; 33+ messages in thread
From: Stephen P. Smith @ 2018-12-31  0:31 UTC (permalink / raw)
  To: git
  Cc: Linus Torvalds, Junio C Hamano, Ævar Arnfjörð Bjarmason

Added documentation and tests for the previously submitted patch.  The
previous patch was rebased and the conflict in cache.h was resolved.

Range diff for Linus' original code:

1:  74e8221b52 ! 1:  dd8ea66414 Add 'human' date format
    @@ -26,6 +26,10 @@
     
         Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
    +    Rebased lt/date-human to git version 2.20.1 and resolved a conflict.
    +
    +    Intentionally did not change logic or add documentation/tests to the
    +    original commit so that the sign-offs would still be legitimate.
     
      diff --git a/builtin/blame.c b/builtin/blame.c
      --- a/builtin/blame.c
    @@ -46,13 +50,13 @@
      --- a/cache.h
      +++ b/cache.h
     @@
    - struct date_mode {
    - 	enum date_mode_type {
    - 		DATE_NORMAL = 0,
    -+		DATE_HUMAN,
    - 		DATE_RELATIVE,
    - 		DATE_SHORT,
    - 		DATE_ISO8601,
    + 
    + enum date_mode_type {
    + 	DATE_NORMAL = 0,
    ++	DATE_HUMAN,
    + 	DATE_RELATIVE,
    + 	DATE_SHORT,
    + 	DATE_ISO8601,
     
      diff --git a/date.c b/date.c
      --- a/date.c


Linus Torvalds (1):
  Add 'human' date format

Stephen P. Smith (2):
  Add 'human' date format documentation
  t0006-date.sh: add `human` date format tests.

 Documentation/rev-list-options.txt |   8 ++
 builtin/blame.c                    |   4 +
 cache.h                            |   1 +
 date.c                             | 130 ++++++++++++++++++++++++-----
 t/t0006-date.sh                    |  24 ++++++
 t/t4202-log.sh                     |  24 ++++++
 t/t7007-show.sh                    |  25 ++++++
 7 files changed, 196 insertions(+), 20 deletions(-)

-- 
2.20.1.2.gb21ebb671b


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 1/3] Add 'human' date format
  2018-12-31  0:31 [PATCH 0/3] Add 'human' date format Stephen P. Smith
@ 2018-12-31  0:31 ` " Stephen P. Smith
  2019-01-03  7:37   ` Jeff King
  2018-12-31  0:31 ` [PATCH 2/3] Add 'human' date format documentation Stephen P. Smith
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 33+ messages in thread
From: Stephen P. Smith @ 2018-12-31  0:31 UTC (permalink / raw)
  To: git
  Cc: Linus Torvalds, Junio C Hamano,
	Ævar Arnfjörð Bjarmason, Stephen P . Smith

From: Linus Torvalds <torvalds@linux-foundation.org>

This adds --date=human, which skips the timezone if it matches the
current time-zone, and doesn't print the whole date if that matches (ie
skip printing year for dates that are "this year", but also skip the
whole date itself if it's in the last few days and we can just say what
weekday it was).

For really recent dates (same day), use the relative date stamp, while
for old dates (year doesn't match), don't bother with time and timezone.

Also add 'auto' date mode, which defaults to human if we're using the
pager.  So you can do

	git config --add log.date auto

and your "git log" commands will show the human-legible format unless
you're scripting things.

Note that this time format still shows the timezone for recent enough
events (but not so recent that they show up as relative dates).  You can
combine it with the "-local" suffix to never show timezones for an even
more simplified view.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stephen P. Smith <ischis2@cox.net>
---
 builtin/blame.c |   4 ++
 cache.h         |   1 +
 date.c          | 130 ++++++++++++++++++++++++++++++++++++++++--------
 3 files changed, 115 insertions(+), 20 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 6d798f9939..f684e31d82 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -925,6 +925,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		 */
 		blame_date_width = utf8_strwidth(_("4 years, 11 months ago")) + 1; /* add the null */
 		break;
+	case DATE_HUMAN:
+		/* If the year is shown, no time is shown */
+		blame_date_width = sizeof("Thu Oct 19 16:00");
+		break;
 	case DATE_NORMAL:
 		blame_date_width = sizeof("Thu Oct 19 16:00:04 2006 -0700");
 		break;
diff --git a/cache.h b/cache.h
index ca36b44ee0..c4396ebaa6 100644
--- a/cache.h
+++ b/cache.h
@@ -1439,6 +1439,7 @@ extern struct object *peel_to_type(const char *name, int namelen,
 
 enum date_mode_type {
 	DATE_NORMAL = 0,
+	DATE_HUMAN,
 	DATE_RELATIVE,
 	DATE_SHORT,
 	DATE_ISO8601,
diff --git a/date.c b/date.c
index 9bc15df6f9..a8d50eb206 100644
--- a/date.c
+++ b/date.c
@@ -77,22 +77,16 @@ static struct tm *time_to_tm_local(timestamp_t time)
 }
 
 /*
- * What value of "tz" was in effect back then at "time" in the
- * local timezone?
+ * Fill in the localtime 'struct tm' for the supplied time,
+ * and return the local tz.
  */
-static int local_tzoffset(timestamp_t time)
+static int local_time_tzoffset(time_t t, struct tm *tm)
 {
-	time_t t, t_local;
-	struct tm tm;
+	time_t t_local;
 	int offset, eastwest;
 
-	if (date_overflows(time))
-		die("Timestamp too large for this system: %"PRItime, time);
-
-	t = (time_t)time;
-	localtime_r(&t, &tm);
-	t_local = tm_to_time_t(&tm);
-
+	localtime_r(&t, tm);
+	t_local = tm_to_time_t(tm);
 	if (t_local == -1)
 		return 0; /* error; just use +0000 */
 	if (t_local < t) {
@@ -107,6 +101,20 @@ static int local_tzoffset(timestamp_t time)
 	return offset * eastwest;
 }
 
+/*
+ * What value of "tz" was in effect back then at "time" in the
+ * local timezone?
+ */
+static int local_tzoffset(timestamp_t time)
+{
+	struct tm tm;
+
+	if (date_overflows(time))
+		die("Timestamp too large for this system: %"PRItime, time);
+
+	return local_time_tzoffset((time_t)time, &tm);
+}
+
 void show_date_relative(timestamp_t time, int tz,
 			       const struct timeval *now,
 			       struct strbuf *timebuf)
@@ -191,9 +199,80 @@ struct date_mode *date_mode_from_type(enum date_mode_type type)
 	return &mode;
 }
 
+static void show_date_normal(struct strbuf *buf, timestamp_t time, struct tm *tm, int tz, struct tm *human_tm, int human_tz, int local)
+{
+	struct {
+		unsigned int	year:1,
+				date:1,
+				wday:1,
+				time:1,
+				seconds:1,
+				tz:1;
+	} hide = { 0 };
+
+	hide.tz = local || tz == human_tz;
+	hide.year = tm->tm_year == human_tm->tm_year;
+	if (hide.year) {
+		if (tm->tm_mon == human_tm->tm_mon) {
+			if (tm->tm_mday > human_tm->tm_mday) {
+				/* Future date: think timezones */
+			} else if (tm->tm_mday == human_tm->tm_mday) {
+				hide.date = hide.wday = 1;
+			} else if (tm->tm_mday + 5 > human_tm->tm_mday) {
+				/* Leave just weekday if it was a few days ago */
+				hide.date = 1;
+			}
+		}
+	}
+
+	/* Show "today" times as just relative times */
+	if (hide.wday) {
+		struct timeval now;
+		gettimeofday(&now, NULL);
+		show_date_relative(time, tz, &now, buf);
+		return;
+	}
+
+	/*
+	 * Always hide seconds for human-readable.
+	 * Hide timezone if showing date.
+	 * Hide weekday and time if showing year.
+	 *
+	 * The logic here is two-fold:
+	 *  (a) only show details when recent enough to matter
+	 *  (b) keep the maximum length "similar", and in check
+	 */
+	if (human_tm->tm_year) {
+		hide.seconds = 1;
+		hide.tz |= !hide.date;
+		hide.wday = hide.time = !hide.year;
+	}
+
+	if (!hide.wday)
+		strbuf_addf(buf, "%.3s ", weekday_names[tm->tm_wday]);
+	if (!hide.date)
+		strbuf_addf(buf, "%.3s %d ", month_names[tm->tm_mon], tm->tm_mday);
+
+	/* Do we want AM/PM depending on locale? */
+	if (!hide.time) {
+		strbuf_addf(buf, "%02d:%02d", tm->tm_hour, tm->tm_min);
+		if (!hide.seconds)
+			strbuf_addf(buf, ":%02d", tm->tm_sec);
+	} else
+		strbuf_rtrim(buf);
+
+	if (!hide.year)
+		strbuf_addf(buf, " %d", tm->tm_year + 1900);
+
+	if (!hide.tz)
+		strbuf_addf(buf, " %+05d", tz);
+}
+
 const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 {
 	struct tm *tm;
+	struct tm human_tm = { 0 };
+	int human_tz = -1;
 	static struct strbuf timebuf = STRBUF_INIT;
 
 	if (mode->type == DATE_UNIX) {
@@ -202,6 +281,15 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 		return timebuf.buf;
 	}
 
+	if (mode->type == DATE_HUMAN) {
+		struct timeval now;
+
+		gettimeofday(&now, NULL);
+
+		/* Fill in the data for "current time" in human_tz and human_tm */
+		human_tz = local_time_tzoffset(now.tv_sec, &human_tm);
+	}
+
 	if (mode->local)
 		tz = local_tzoffset(time);
 
@@ -258,14 +346,7 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 		strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
 				!mode->local);
 	else
-		strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
-				weekday_names[tm->tm_wday],
-				month_names[tm->tm_mon],
-				tm->tm_mday,
-				tm->tm_hour, tm->tm_min, tm->tm_sec,
-				tm->tm_year + 1900,
-				mode->local ? 0 : ' ',
-				tz);
+		show_date_normal(&timebuf, time, tm, tz, &human_tm, human_tz, mode->local);
 	return timebuf.buf;
 }
 
@@ -802,6 +883,11 @@ int parse_date(const char *date, struct strbuf *result)
 	return 0;
 }
 
+static int auto_date_style(void)
+{
+	return (isatty(1) || pager_in_use()) ? DATE_HUMAN : DATE_NORMAL;
+}
+
 static enum date_mode_type parse_date_type(const char *format, const char **end)
 {
 	if (skip_prefix(format, "relative", end))
@@ -819,6 +905,10 @@ static enum date_mode_type parse_date_type(const char *format, const char **end)
 		return DATE_SHORT;
 	if (skip_prefix(format, "default", end))
 		return DATE_NORMAL;
+	if (skip_prefix(format, "human", end))
+		return DATE_HUMAN;
+	if (skip_prefix(format, "auto", end))
+		return auto_date_style();
 	if (skip_prefix(format, "raw", end))
 		return DATE_RAW;
 	if (skip_prefix(format, "unix", end))
-- 
2.20.1.2.gb21ebb671b


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 2/3] Add 'human' date format documentation
  2018-12-31  0:31 [PATCH 0/3] Add 'human' date format Stephen P. Smith
  2018-12-31  0:31 ` [PATCH 1/3] " Stephen P. Smith
@ 2018-12-31  0:31 ` Stephen P. Smith
  2018-12-31  0:31 ` [PATCH 3/3] t0006-date.sh: add `human` date format tests Stephen P. Smith
  2019-01-18  6:18 ` [PATCH v2 0/5] Re-roll of 'human' date format patch set Stephen P. Smith
  3 siblings, 0 replies; 33+ messages in thread
From: Stephen P. Smith @ 2018-12-31  0:31 UTC (permalink / raw)
  To: git
  Cc: Linus Torvalds, Junio C Hamano,
	Ævar Arnfjörð Bjarmason, Stephen P . Smith

Display date and time information in a format similar to how people
write dates in other contexts. If the year isn't specified then, the
reader infers the date is given is in the current year.

By not displaying the redundant information, the reader concentrates
on the information that is different. The patch reports relative dates
based on information inferred from the date on the machine running the
git command at the time the command is executed.

While the format is more useful to humans by dropping inferred
information, there is nothing that makes it actually human. If the
'relative' date format wasn't already implemented then using
'relative' would have been appropriate.

Signed-off-by: Stephen P. Smith <ischis2@cox.net>
---
 Documentation/rev-list-options.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index bab5f50b17..b491c3b999 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -835,6 +835,14 @@ Note that the `-local` option does not affect the seconds-since-epoch
 value (which is always measured in UTC), but does switch the accompanying
 timezone value.
 +
+`--date=human` shows the timezone if it matches the current time-zone,
+and doesn't print the whole date if that matches (ie skip printing
+year for dates that are "this year", but also skip the whole date
+itself if it's in the last few days and we can just say what weekday
+it was).
++
+`--date=auto` defaults to human if we're using the pager.
++
 `--date=unix` shows the date as a Unix epoch timestamp (seconds since
 1970).  As with `--raw`, this is always in UTC and therefore `-local`
 has no effect.
-- 
2.20.1.2.gb21ebb671b


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 3/3] t0006-date.sh: add `human` date format tests.
  2018-12-31  0:31 [PATCH 0/3] Add 'human' date format Stephen P. Smith
  2018-12-31  0:31 ` [PATCH 1/3] " Stephen P. Smith
  2018-12-31  0:31 ` [PATCH 2/3] Add 'human' date format documentation Stephen P. Smith
@ 2018-12-31  0:31 ` Stephen P. Smith
  2019-01-02 18:15   ` Junio C Hamano
                     ` (2 more replies)
  2019-01-18  6:18 ` [PATCH v2 0/5] Re-roll of 'human' date format patch set Stephen P. Smith
  3 siblings, 3 replies; 33+ messages in thread
From: Stephen P. Smith @ 2018-12-31  0:31 UTC (permalink / raw)
  To: git
  Cc: Linus Torvalds, Junio C Hamano,
	Ævar Arnfjörð Bjarmason, Stephen P . Smith

The `human` date format varies based on two inputs: the date in the
reference time which is constant and the local computers date which
varies.  Using hardcoded test expected output dates would require
holding the local machines date and time constant which is not
desireable.

Alternatively, letting the local date vary, which is the normal
situation, implies that the tests would be checking for formating
changes based on on a ref date relative to the local computers time.

When using `human` several fields are suppressed depending on the time
difference between the reference date and the local computer date. In
cases where the difference is less than a year, the year field is
supppressed. If the time is less than a day; the month and year is
suppressed.

Test using a regular expression to verify that fields that are
expected to be suppressed are not displayed.

Signed-off-by: Stephen P. Smith <ischis2@cox.net>
---
 t/t0006-date.sh | 24 ++++++++++++++++++++++++
 t/t4202-log.sh  | 24 ++++++++++++++++++++++++
 t/t7007-show.sh | 25 +++++++++++++++++++++++++
 3 files changed, 73 insertions(+)

diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index ffb2975e48..f208a80867 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh
@@ -40,6 +40,16 @@ check_show () {
 	'
 }
 
+check_human_date () {
+	time=$1
+	expect=$2
+	test_expect_success "check date ($format:$time)" '
+		echo "$time -> $expect" >expect &&
+		TZ=${zone:-$TZ} test-tool date show:"$format" "$time" >actual &&
+		grep "$expect" actual 
+	'
+}
+
 # arbitrary but sensible time for examples
 TIME='1466000000 +0200'
 check_show iso8601 "$TIME" '2016-06-15 16:13:20 +0200'
@@ -52,6 +62,20 @@ check_show unix "$TIME" '1466000000'
 check_show iso-local "$TIME" '2016-06-15 14:13:20 +0000'
 check_show raw-local "$TIME" '1466000000 +0000'
 check_show unix-local "$TIME" '1466000000'
+check_show human "$TIME" 'Jun 15 2016'
+
+# Subtract some known constant time and look for expected field format
+TODAY_REGEX='5 hours ago'
+THIS_YEAR_REGEX='[A-Z][a-z][a-z] [A-Z][a-z][a-z] [0-9]* [012][0-9]:[0-6][0-9]'
+MORE_THAN_A_YEAR_REGEX='[A-Z][a-z][a-z] [A-Z][a-z][a-z] [0-9]* [0-9][0-9][0-9][0-9]'
+check_human_date "$(($(date +%s)-18000)) +0200" $TODAY_REGEX # 5 hours ago
+check_human_date "$(($(date +%s)-432000)) +0200" $THIS_YEAR_REGEX  # 5 days ago
+check_human_date "$(($(date +%s)-1728000)) +0200" $THIS_YEAR_REGEX # 3 weeks ago
+check_human_date "$(($(date +%s)-13000000)) +0200" $THIS_YEAR_REGEX # 5 months ago
+check_human_date "$(($(date +%s)-31449600)) +0200" $THIS_YEAR_REGEX # 12 months ago
+check_human_date "$(($(date +%s)-37500000)) +0200" $MORE_THAN_A_YEAR_REGEX # 1 year, 2 months ago
+check_human_date "$(($(date +%s)-55188000)) +0200" $MORE_THAN_A_YEAR_REGEX # 1 year, 9 months ago
+check_human_date "$(($(date +%s)-630000000)) +0200" $MORE_THAN_A_YEAR_REGEX # 20 years ago
 
 check_show 'format:%z' "$TIME" '+0200'
 check_show 'format-local:%z' "$TIME" '+0000'
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 819c24d10e..d7f3b73650 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1707,4 +1707,28 @@ test_expect_success '--exclude-promisor-objects does not BUG-crash' '
 	test_must_fail git log --exclude-promisor-objects source-a
 '
 
+check_human_date() {
+	commit_date=$1
+	expect=$2
+	test_expect_success "$commit_date" "
+		echo $expect $commit_date >dates && 
+		git add dates &&
+		git commit -m 'Expect String' --date=\"$commit_date\" dates &&
+		git log -1 --date=human | grep \"^Date:\" >actual &&
+		grep \"$expect\" actual
+"
+}
+
+TODAY_REGEX='[A-Z][a-z][a-z] [012][0-9]:[0-6][0-9] .0200'
+THIS_YEAR_REGEX='[A-Z][a-z][a-z] [A-Z][a-z][a-z] [0-9]* [012][0-9]:[0-6][0-9]'
+MORE_THAN_A_YEAR_REGEX='[A-Z][a-z][a-z] [A-Z][a-z][a-z] [0-9]* [0-9][0-9][0-9][0-9]'
+check_human_date "$(($(date +%s)-18000)) +0200" $TODAY_REGEX # 5 hours ago
+check_human_date "$(($(date +%s)-432000)) +0200" $THIS_YEAR_REGEX  # 5 days ago
+check_human_date "$(($(date +%s)-1728000)) +0200" $THIS_YEAR_REGEX # 3 weeks ago
+check_human_date "$(($(date +%s)-13000000)) +0200" $THIS_YEAR_REGEX # 5 months ago
+check_human_date "$(($(date +%s)-31449600)) +0200" $THIS_YEAR_REGEX # 12 months ago
+check_human_date "$(($(date +%s)-37500000)) +0200" $MORE_THAN_A_YEAR_REGEX # 1 year, 2 months ago
+check_human_date "$(($(date +%s)-55188000)) +0200" $MORE_THAN_A_YEAR_REGEX # 1 year, 9 months ago
+check_human_date "$(($(date +%s)-630000000)) +0200" $MORE_THAN_A_YEAR_REGEX # 20 years ago
+
 test_done
diff --git a/t/t7007-show.sh b/t/t7007-show.sh
index 42d3db6246..0a0334a8b5 100755
--- a/t/t7007-show.sh
+++ b/t/t7007-show.sh
@@ -128,4 +128,29 @@ test_expect_success 'show --graph is forbidden' '
   test_must_fail git show --graph HEAD
 '
 
+check_human_date() {
+	commit_date=$1
+	expect=$2
+	test_expect_success "$commit_date" "
+		echo $expect $commit_date >dates && 
+		git add dates &&
+		git commit -m 'Expect String' --date=\"$commit_date\" dates &&
+		git show --date=human | grep \"^Date:\" >actual &&
+		grep \"$expect\" actual
+"
+}
+
+TODAY_REGEX='[A-Z][a-z][a-z] [012][0-9]:[0-6][0-9] .0200'
+THIS_YEAR_REGEX='[A-Z][a-z][a-z] [A-Z][a-z][a-z] [0-9]* [012][0-9]:[0-6][0-9]'
+MORE_THAN_A_YEAR_REGEX='[A-Z][a-z][a-z] [A-Z][a-z][a-z] [0-9]* [0-9][0-9][0-9][0-9]'
+check_human_date "$(($(date +%s)-18000)) +0200" $TODAY_REGEX # 5 hours ago
+check_human_date "$(($(date +%s)-432000)) +0200" $THIS_YEAR_REGEX  # 5 days ago
+check_human_date "$(($(date +%s)-1728000)) +0200" $THIS_YEAR_REGEX # 3 weeks ago
+check_human_date "$(($(date +%s)-13000000)) +0200" $THIS_YEAR_REGEX # 5 months ago
+check_human_date "$(($(date +%s)-31449600)) +0200" $THIS_YEAR_REGEX # 12 months ago
+check_human_date "$(($(date +%s)-37500000)) +0200" $MORE_THAN_A_YEAR_REGEX # 1 year, 2 months ago
+check_human_date "$(($(date +%s)-55188000)) +0200" $MORE_THAN_A_YEAR_REGEX # 1 year, 9 months ago
+check_human_date "$(($(date +%s)-630000000)) +0200" $MORE_THAN_A_YEAR_REGEX # 20 years ago
+
+
 test_done
-- 
2.20.1.2.gb21ebb671b


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 3/3] t0006-date.sh: add `human` date format tests.
  2018-12-31  0:31 ` [PATCH 3/3] t0006-date.sh: add `human` date format tests Stephen P. Smith
@ 2019-01-02 18:15   ` Junio C Hamano
  2019-01-03  2:36     ` Stephen & Linda Smith
  2019-01-03 21:14     ` Philip Oakley
  2019-01-03  7:44   ` Jeff King
  2019-01-08 21:27   ` Johannes Sixt
  2 siblings, 2 replies; 33+ messages in thread
From: Junio C Hamano @ 2019-01-02 18:15 UTC (permalink / raw)
  To: Stephen P. Smith
  Cc: git, Linus Torvalds, Ævar Arnfjörð Bjarmason

"Stephen P. Smith" <ischis2@cox.net> writes:

> +# Subtract some known constant time and look for expected field format
> +TODAY_REGEX='5 hours ago'
> +THIS_YEAR_REGEX='[A-Z][a-z][a-z] [A-Z][a-z][a-z] [0-9]* [012][0-9]:[0-6][0-9]'
> +MORE_THAN_A_YEAR_REGEX='[A-Z][a-z][a-z] [A-Z][a-z][a-z] [0-9]* [0-9][0-9][0-9][0-9]'
> +check_human_date "$(($(date +%s)-18000)) +0200" $TODAY_REGEX # 5 hours ago
> +check_human_date "$(($(date +%s)-432000)) +0200" $THIS_YEAR_REGEX  # 5 days ago

'date +%s' is used everywhere in this patch but has never been used
in our test suite before.  It is not portable.

We perhaps can use "test-tool date timestamp", like so

	check_human_date $(test-tool date timestamp "18000 seconds ago") ...

or moving the part that munges 18000 into the above form inside
check_human_date helper function, e.g.

	check_human_date () {
		commit_date=$(test-tool date timestamp "$1 seconds ago")
		commit_date="$commit_date +0200"
                expect=$2
		...
	}

which would let us write

	check_human_date 432000" $THIS_YEAR_REGEX  # 5 days ago

> +check_human_date() {
> +	commit_date=$1
> +	expect=$2
> +	test_expect_success "$commit_date" "
> +		echo $expect $commit_date >dates && 
> +		git add dates &&
> +		git commit -m 'Expect String' --date=\"$commit_date\" dates &&
> +		git log -1 --date=human | grep \"^Date:\" >actual &&
> +		grep \"$expect\" actual
> +"

As the body of the test_expect_success helper is eval'ed, variables
$commit_date and $expect should be visible to it, without turning
them into values before executing test_expect_success function,
i.e.

	test_expect_success "$commit_date" '
		echo "$expect $commit_date" >dates &&
		...
		git commit -m "Expect String" --date="$commit_date" dates &&
		git show -s --date=human | grep '^Date:" >actual &&
		grep "$expect" actual
	'

which would reduce the need for unreadable backslashes.

Instead of duplicating, perhaps move this to a more common place?
Would it make sense to make it "check_date_format ()" helper by
passing another argument to parameterize --date=human part

> +check_human_date() {
> +	commit_date=$1
> +	expect=$2
> +	test_expect_success "$commit_date" "
> +		echo $expect $commit_date >dates && 
> +		git add dates &&
> +		git commit -m 'Expect String' --date=\"$commit_date\" dates &&
> +		git show --date=human | grep \"^Date:\" >actual &&

Using "show" here is much better than "log -1" above; using "show
-s" would be even better.

> +		grep \"$expect\" actual
> +"
> +}
> +
> +TODAY_REGEX='[A-Z][a-z][a-z] [012][0-9]:[0-6][0-9] .0200'
> +THIS_YEAR_REGEX='[A-Z][a-z][a-z] [A-Z][a-z][a-z] [0-9]* [012][0-9]:[0-6][0-9]'
> +MORE_THAN_A_YEAR_REGEX='[A-Z][a-z][a-z] [A-Z][a-z][a-z] [0-9]* [0-9][0-9][0-9][0-9]'
> +check_human_date "$(($(date +%s)-18000)) +0200" $TODAY_REGEX # 5 hours ago
> +check_human_date "$(($(date +%s)-432000)) +0200" $THIS_YEAR_REGEX  # 5 days ago
> +check_human_date "$(($(date +%s)-1728000)) +0200" $THIS_YEAR_REGEX # 3 weeks ago
> +check_human_date "$(($(date +%s)-13000000)) +0200" $THIS_YEAR_REGEX # 5 months ago
> +check_human_date "$(($(date +%s)-31449600)) +0200" $THIS_YEAR_REGEX # 12 months ago
> +check_human_date "$(($(date +%s)-37500000)) +0200" $MORE_THAN_A_YEAR_REGEX # 1 year, 2 months ago
> +check_human_date "$(($(date +%s)-55188000)) +0200" $MORE_THAN_A_YEAR_REGEX # 1 year, 9 months ago
> +check_human_date "$(($(date +%s)-630000000)) +0200" $MORE_THAN_A_YEAR_REGEX # 20 years ago
> +
> +
>  test_done

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 3/3] t0006-date.sh: add `human` date format tests.
  2019-01-02 18:15   ` Junio C Hamano
@ 2019-01-03  2:36     ` Stephen & Linda Smith
  2019-01-03  6:42       ` Junio C Hamano
  2019-01-03 21:14     ` Philip Oakley
  1 sibling, 1 reply; 33+ messages in thread
From: Stephen & Linda Smith @ 2019-01-03  2:36 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, Linus Torvalds, Ævar Arnfjörð Bjarmason

On Wednesday, January 2, 2019 11:15:02 AM MST Junio C Hamano wrote:
> 'date +%s' is used everywhere in this patch but has never been used
> in our test suite before.  It is not portable.
So I don't make this mistake again, Is there a reference somewhere for that is 
and is not portable?

> 
> We perhaps can use "test-tool date timestamp", like so
> 
> 	check_human_date $(test-tool date timestamp "18000 seconds ago") ...
> 
> or moving the part that munges 18000 into the above form inside
> check_human_date helper function, e.g.
> 
> 	check_human_date () {
> 		commit_date=$(test-tool date timestamp "$1 seconds ago")
> 		commit_date="$commit_date +0200"
>                 expect=$2
> 		...
> 	}
> 
> which would let us write
> 
> 	check_human_date 432000" $THIS_YEAR_REGEX  # 5 days ago
Thanks

>
> > +check_human_date() {
> > +	commit_date=$1
> > +	expect=$2
> > +	test_expect_success "$commit_date" "
> > +		echo $expect $commit_date >dates &&
> > +		git add dates &&
> > +		git commit -m 'Expect String' --date=\"$commit_date\" dates &&
> > +		git log -1 --date=human | grep \"^Date:\" >actual &&
> > +		grep \"$expect\" actual
> > +"
> 
> As the body of the test_expect_success helper is eval'ed, variables
> $commit_date and $expect should be visible to it, without turning
> them into values before executing test_expect_success function,
> i.e.
> 
> 	test_expect_success "$commit_date" '
> 		echo "$expect $commit_date" >dates &&
> 		...
> 		git commit -m "Expect String" --date="$commit_date" dates &&
> 		git show -s --date=human | grep '^Date:" >actual &&
> 		grep "$expect" actual
> 	'
> 
> which would reduce the need for unreadable backslashes.
I was worried about embedded spaces that might not be parsed correctly by the 
called function.  I will update

> 
> Instead of duplicating, perhaps move this to a more common place?
> Would it make sense to make it "check_date_format ()" helper by
> passing another argument to parameterize --date=human part
I had considered that, but then noted that for the other formats specific 
strings were being used.  The use of specific strings was possible since the 
other formats were always guarenteed to have the same string literal due to a 
singe unvarying input.

I don't mind parameterize the format and it would make the solution more 
general.


> > "Stephen P. Smith" <ischis2@cox.net> writes:
> > +# Subtract some known constant time and look for expected field format
> > +TODAY_REGEX='5 hours ago'
> > +THIS_YEAR_REGEX='[A-Z][a-z][a-z] [A-Z][a-z][a-z] [0-9]*
> > [012][0-9]:[0-6][0-9]' +MORE_THAN_A_YEAR_REGEX='[A-Z][a-z][a-z]
> > [A-Z][a-z][a-z] [0-9]* [0-9][0-9][0-9][0-9]' +check_human_date "$(($(date
> > +%s)-18000)) +0200" $TODAY_REGEX # 5 hours ago +check_human_date
> > "$(($(date +%s)-432000)) +0200" $THIS_YEAR_REGEX  # 5 days ago
> > +check_human_date() {
> > +	commit_date=$1
> > +	expect=$2
> > +	test_expect_success "$commit_date" "
> > +		echo $expect $commit_date >dates &&
> > +		git add dates &&
> > +		git commit -m 'Expect String' --date=\"$commit_date\" dates &&
> > +		git show --date=human | grep \"^Date:\" >actual &&
> 
> Using "show" here is much better than "log -1" above; using "show
> -s" would be even better.

I was attempting to test both git log and git show.  For get log the `-1` was 
to only get the latest commit.

Are you suggesting that t4202-log.sh not be updated and that only and  t7007-
show.sh and t0006-date.sh updated?  

Side note:  I found when updating that all three scripts that log and show 
returned the same formats, but date returned a different string if the delta 
date was less than 24hours

I just noted that the patch 3/3 should be re-titled since the tests are 
currently for three commands.

Hope you are better.
sps




^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 3/3] t0006-date.sh: add `human` date format tests.
  2019-01-03  2:36     ` Stephen & Linda Smith
@ 2019-01-03  6:42       ` Junio C Hamano
  2019-01-03 13:20         ` Stephen P. Smith
  0 siblings, 1 reply; 33+ messages in thread
From: Junio C Hamano @ 2019-01-03  6:42 UTC (permalink / raw)
  To: Stephen & Linda Smith
  Cc: git, Linus Torvalds, Ævar Arnfjörð Bjarmason

Stephen & Linda Smith <ischis2@cox.net> writes:

> On Wednesday, January 2, 2019 11:15:02 AM MST Junio C Hamano wrote:
>> 'date +%s' is used everywhere in this patch but has never been used
>> in our test suite before.  It is not portable.
> So I don't make this mistake again, Is there a reference somewhere for that is 
> and is not portable?

I usually go to http://pubs.opengroup.org/onlinepubs/9699919799/

Even though we do not say "We'll use anything that is in POSIX.1; it
is your problem if your platform does not support it", we tend to
say "It's not even in POSIX, so let's see if we can avoid it".

>> Using "show" here is much better than "log -1" above; using "show
>> -s" would be even better.
>
> I was attempting to test both git log and git show.  For get log the `-1` was 
> to only get the latest commit.
>
> Are you suggesting that t4202-log.sh not be updated and that only and  t7007-
> show.sh and t0006-date.sh updated?  

I am saying that using "log -1" and "show" in different tests _only_
for the value of "Date:" field does not buy us much.  And by unifying,
I was hoping that the single helper can be placed in a common file
that is dot-sourced by these three scripts more easily.

Thanks.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 1/3] Add 'human' date format
  2018-12-31  0:31 ` [PATCH 1/3] " Stephen P. Smith
@ 2019-01-03  7:37   ` Jeff King
  2019-01-03 13:19     ` Stephen P. Smith
  0 siblings, 1 reply; 33+ messages in thread
From: Jeff King @ 2019-01-03  7:37 UTC (permalink / raw)
  To: Stephen P . Smith
  Cc: git, Linus Torvalds, Junio C Hamano,
	Ævar Arnfjörð Bjarmason

On Sun, Dec 30, 2018 at 05:31:48PM -0700, Stephen P. Smith wrote:

> Also add 'auto' date mode, which defaults to human if we're using the
> pager.  So you can do
> 
> 	git config --add log.date auto
> 
> and your "git log" commands will show the human-legible format unless
> you're scripting things.

I like the idea of "human", and I like the idea of "auto", but it seems
to me that these are really two orthogonal things. E.g., might some
people not want to do something like:

  git config log.date auto:relative

?

I don't personally care about using this myself, but we already had to
deal with retrofitting "local" as a modifier. I'd prefer to avoid making
the same mistake again.

(I'd actually argue that "log.date" should basically _always_ have the
"auto" behavior, since it tends to get treated as plumbing anyway, and I
suspect that anybody who sets log.date now would see subtle breakage
from scripts. But maybe it's too late at this point?).

> diff --git a/builtin/blame.c b/builtin/blame.c
> index 6d798f9939..f684e31d82 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -925,6 +925,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>  		 */
>  		blame_date_width = utf8_strwidth(_("4 years, 11 months ago")) + 1; /* add the null */
>  		break;
> +	case DATE_HUMAN:
> +		/* If the year is shown, no time is shown */
> +		blame_date_width = sizeof("Thu Oct 19 16:00");
> +		break;

OK, and we expect the year to be less than 5 characters. I briefly
wondered what would happen at Y100K (or somebody maliciously using a
bogus year), but it is not a buffer overflow. It is simply a mis-aligned
blame line (and actually, the same goes for the existing entries, which
use a 4-digit year).

-Peff

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 3/3] t0006-date.sh: add `human` date format tests.
  2018-12-31  0:31 ` [PATCH 3/3] t0006-date.sh: add `human` date format tests Stephen P. Smith
  2019-01-02 18:15   ` Junio C Hamano
@ 2019-01-03  7:44   ` Jeff King
  2019-01-03 13:12     ` Stephen & Linda Smith
  2019-01-08 21:27   ` Johannes Sixt
  2 siblings, 1 reply; 33+ messages in thread
From: Jeff King @ 2019-01-03  7:44 UTC (permalink / raw)
  To: Stephen P . Smith
  Cc: git, Linus Torvalds, Junio C Hamano,
	Ævar Arnfjörð Bjarmason

On Sun, Dec 30, 2018 at 05:31:50PM -0700, Stephen P. Smith wrote:

> The `human` date format varies based on two inputs: the date in the
> reference time which is constant and the local computers date which
> varies.  Using hardcoded test expected output dates would require
> holding the local machines date and time constant which is not
> desireable.
> 
> Alternatively, letting the local date vary, which is the normal
> situation, implies that the tests would be checking for formating
> changes based on on a ref date relative to the local computers time.

We already have $TEST_DATE_NOW, which "test-tool date" will respect for
various commands to pretend that it's currently a particular time. I
think you'd need to add a sub-command similar to "relative" (which
directly calls show_date_relative()) which calls into the "human" code.

Note that there _isn't_ a way to have actual non-test git programs read
the current time from an environment variable (as opposed to actually
calling gettimeofday()).

-Peff

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 3/3] t0006-date.sh: add `human` date format tests.
  2019-01-03  7:44   ` Jeff King
@ 2019-01-03 13:12     ` Stephen & Linda Smith
  0 siblings, 0 replies; 33+ messages in thread
From: Stephen & Linda Smith @ 2019-01-03 13:12 UTC (permalink / raw)
  To: Jeff King
  Cc: git, Linus Torvalds, Junio C Hamano,
	Ævar Arnfjörð Bjarmason

On Thursday, January 3, 2019 12:44:22 AM MST Jeff King wrote:
> We already have $TEST_DATE_NOW, which "test-tool date" will respect for
> various commands to pretend that it's currently a particular time. I
> think you'd need to add a sub-command similar to "relative" (which
> directly calls show_date_relative()) which calls into the "human" code.

I'll investigate.  Looks like this comment is related other comments.

> Note that there _isn't_ a way to have actual non-test git programs read
> the current time from an environment variable (as opposed to actually
> calling gettimeofday()).
Agreed

> 
> -Peff





^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 1/3] Add 'human' date format
  2019-01-03  7:37   ` Jeff King
@ 2019-01-03 13:19     ` Stephen P. Smith
  2019-01-04  7:50       ` Jeff King
  0 siblings, 1 reply; 33+ messages in thread
From: Stephen P. Smith @ 2019-01-03 13:19 UTC (permalink / raw)
  To: Jeff King
  Cc: git, Linus Torvalds, Junio C Hamano,
	Ævar Arnfjörð Bjarmason

On Thursday, January 3, 2019 12:37:35 AM MST Jeff King wrote:
> I like the idea of "human", and I like the idea of "auto", but it seems
> to me that these are really two orthogonal things. E.g., might some
> people not want to do something like:
> 
>   git config log.date auto:relative
I didn't see anything in the code which would prohibit setting something like 
that.  

> 
> I don't personally care about using this myself, but we already had to
> deal with retrofitting "local" as a modifier. I'd prefer to avoid making
> the same mistake again.
Since I wasn't involved could you summarize the you are referring to?

> 
> (I'd actually argue that "log.date" should basically _always_ have the
> "auto" behavior, since it tends to get treated as plumbing anyway, and I
> suspect that anybody who sets log.date now would see subtle breakage
> from scripts. But maybe it's too late at this point?).
If auto isn't added to the "log.date" file, then the date behaviour is not 
changed from is currently in the code base.   Therefore, there shouldn't be 
any breakage.
> 
> > diff --git a/builtin/blame.c b/builtin/blame.c
> > index 6d798f9939..f684e31d82 100644
> > --- a/builtin/blame.c
> > +++ b/builtin/blame.c
> > @@ -925,6 +925,10 @@ int cmd_blame(int argc, const char **argv, const char
> > *prefix)> 
> >  		 */
> >  		
> >  		blame_date_width = utf8_strwidth(_("4 years, 11 months ago")) + 1; /*
> >  		add the null */ break;
> > 
> > +	case DATE_HUMAN:
> > +		/* If the year is shown, no time is shown */
> > +		blame_date_width = sizeof("Thu Oct 19 16:00");
> > +		break;
> 
> OK, and we expect the year to be less than 5 characters. I briefly
> wondered what would happen at Y100K (or somebody maliciously using a
> bogus year), but it is not a buffer overflow. It is simply a mis-aligned
> blame line (and actually, the same goes for the existing entries, which
> use a 4-digit year).
> 
> -Peff





^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 3/3] t0006-date.sh: add `human` date format tests.
  2019-01-03  6:42       ` Junio C Hamano
@ 2019-01-03 13:20         ` Stephen P. Smith
  0 siblings, 0 replies; 33+ messages in thread
From: Stephen P. Smith @ 2019-01-03 13:20 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, Linus Torvalds, Ævar Arnfjörð Bjarmason

On Wednesday, January 2, 2019 11:42:20 PM MST Junio C Hamano wrote:
> > Are you suggesting that t4202-log.sh not be updated and that only and 
> > t7007- show.sh and t0006-date.sh updated?
> 
> I am saying that using "log -1" and "show" in different tests _only_
> for the value of "Date:" field does not buy us much.  And by unifying,
> I was hoping that the single helper can be placed in a common file
> that is dot-sourced by these three scripts more easily.

Thanks for the clarification.




^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 3/3] t0006-date.sh: add `human` date format tests.
  2019-01-02 18:15   ` Junio C Hamano
  2019-01-03  2:36     ` Stephen & Linda Smith
@ 2019-01-03 21:14     ` Philip Oakley
  2019-01-03 21:45       ` Junio C Hamano
  1 sibling, 1 reply; 33+ messages in thread
From: Philip Oakley @ 2019-01-03 21:14 UTC (permalink / raw)
  To: Junio C Hamano, Stephen P. Smith
  Cc: git, Linus Torvalds, Ævar Arnfjörð Bjarmason

On 02/01/2019 18:15, Junio C Hamano wrote:
> We perhaps can use "test-tool date timestamp", like so
>
> 	check_human_date $(test-tool date timestamp "18000 seconds ago") ...
>
> or moving the part that munges 18000 into the above form inside
> check_human_date helper function, e.g.
>
> 	check_human_date () {
> 		commit_date=$(test-tool date timestamp "$1 seconds ago")
> 		commit_date="$commit_date +0200"
>                  expect=$2
> 		...
> 	}
>
> which would let us write
>
> 	check_human_date 432000" $THIS_YEAR_REGEX  # 5 days ago


Just a quick bikeshed: if used, would this have a year end 5 day 
roll-over error potential, or will it always use the single date?

(I appreciate it is just suggestion code, not tested)

-- 

Philip


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 3/3] t0006-date.sh: add `human` date format tests.
  2019-01-03 21:14     ` Philip Oakley
@ 2019-01-03 21:45       ` Junio C Hamano
  2019-01-03 23:57         ` Stephen P. Smith
  0 siblings, 1 reply; 33+ messages in thread
From: Junio C Hamano @ 2019-01-03 21:45 UTC (permalink / raw)
  To: Philip Oakley
  Cc: Stephen P. Smith, git, Linus Torvalds,
	Ævar Arnfjörð Bjarmason

Philip Oakley <philipoakley@iee.org> writes:

> On 02/01/2019 18:15, Junio C Hamano wrote:
>> We perhaps can use "test-tool date timestamp", like so
>>
>> 	check_human_date $(test-tool date timestamp "18000 seconds ago") ...
>>
>> or moving the part that munges 18000 into the above form inside
>> check_human_date helper function, e.g.
>>
>> 	check_human_date () {
>> 		commit_date=$(test-tool date timestamp "$1 seconds ago")
>> 		commit_date="$commit_date +0200"
>>                  expect=$2
>> 		...
>> 	}
>>
>> which would let us write
>>
>> 	check_human_date 432000 "$THIS_YEAR_REGEX" # 5 days ago
>
>
> Just a quick bikeshed: if used, would this have a year end 5 day
> roll-over error potential, or will it always use the single date?

Hmph, interesting point.  Indeed, date.c::show_date_normal() decides
to hide the year portion if the timestamp and the current time share
the same year, so on Thu Jan 3rd, an attempt to show a commit made
on Mon Dec 31st of the same week would end up showing the year, so
yes, I agree with you that the above would break.

+TODAY_REGEX='5 hours ago'
+THIS_YEAR_REGEX='[A-Z][a-z][a-z] [A-Z][a-z][a-z] [0-9]* [012][0-9]:[0-6][0-9]'
+MORE_THAN_A_YEAR_REGEX='[A-Z][a-z][a-z] [A-Z][a-z][a-z] [0-9]* [0-9][0-9][0-9][0-9]'

>
> (I appreciate it is just suggestion code, not tested)

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 3/3] t0006-date.sh: add `human` date format tests.
  2019-01-03 21:45       ` Junio C Hamano
@ 2019-01-03 23:57         ` Stephen P. Smith
  0 siblings, 0 replies; 33+ messages in thread
From: Stephen P. Smith @ 2019-01-03 23:57 UTC (permalink / raw)
  To: Junio C Hamano, Philip Oakley
  Cc: git, Linus Torvalds, Ævar Arnfjörð Bjarmason

On Thursday, January 3, 2019 2:45:39 PM MST Junio C Hamano wrote:
> Philip Oakley <philipoakley@iee.org> writes:
> >> 
> >> 	check_human_date 432000 "$THIS_YEAR_REGEX" # 5 days ago
> > 
> > Just a quick bikeshed: if used, would this have a year end 5 day
> > roll-over error potential, or will it always use the single date?
> 
> Hmph, interesting point.  Indeed, date.c::show_date_normal() decides
> to hide the year portion if the timestamp and the current time share
> the same year, so on Thu Jan 3rd, an attempt to show a commit made
> on Mon Dec 31st of the same week would end up showing the year, so
> yes, I agree with you that the above would break.
> 

Thanks Philip.

I wrote the test just before the new year so didn't see the rollover.   I 
haven't run the test this year.
sps







^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 1/3] Add 'human' date format
  2019-01-03 13:19     ` Stephen P. Smith
@ 2019-01-04  7:50       ` Jeff King
  2019-01-04 13:03         ` Stephen P Smith
  0 siblings, 1 reply; 33+ messages in thread
From: Jeff King @ 2019-01-04  7:50 UTC (permalink / raw)
  To: Stephen P. Smith
  Cc: git, Linus Torvalds, Junio C Hamano,
	Ævar Arnfjörð Bjarmason

On Thu, Jan 03, 2019 at 06:19:56AM -0700, Stephen P. Smith wrote:

> On Thursday, January 3, 2019 12:37:35 AM MST Jeff King wrote:
> > I like the idea of "human", and I like the idea of "auto", but it seems
> > to me that these are really two orthogonal things. E.g., might some
> > people not want to do something like:
> > 
> >   git config log.date auto:relative
> I didn't see anything in the code which would prohibit setting something like 
> that.

Yeah, I don't think supporting that is too hard. I was thinking
something like this:

diff --git a/date.c b/date.c
index 4486c028ac..f731803872 100644
--- a/date.c
+++ b/date.c
@@ -883,11 +883,6 @@ int parse_date(const char *date, struct strbuf *result)
 	return 0;
 }
 
-static int auto_date_style(void)
-{
-	return (isatty(1) || pager_in_use()) ? DATE_HUMAN : DATE_NORMAL;
-}
-
 static enum date_mode_type parse_date_type(const char *format, const char **end)
 {
 	if (skip_prefix(format, "relative", end))
@@ -907,8 +902,6 @@ static enum date_mode_type parse_date_type(const char *format, const char **end)
 		return DATE_NORMAL;
 	if (skip_prefix(format, "human", end))
 		return DATE_HUMAN;
-	if (skip_prefix(format, "auto", end))
-		return auto_date_style();
 	if (skip_prefix(format, "raw", end))
 		return DATE_RAW;
 	if (skip_prefix(format, "unix", end))
@@ -923,6 +916,14 @@ void parse_date_format(const char *format, struct date_mode *mode)
 {
 	const char *p;
 
+	/* "auto:foo" is "if tty/pager, then foo, otherwise normal" */
+	if (skip_prefix(format, "auto:", &p)) {
+		if (isatty(1) || pager_in_use())
+			format = p;
+		else
+			format = "default";
+	}
+
 	/* historical alias */
 	if (!strcmp(format, "local"))
 		format = "default-local";

That removes "auto" completely. We could still support it as an alias
for "auto:human" with something like:

  if (!strcmp(format, "auto"))
	format = "auto:human";

but IMHO it is a simpler interface to just have the user be explicit
(this is meant to be set once in config, after all).

> > I don't personally care about using this myself, but we already had to
> > deal with retrofitting "local" as a modifier. I'd prefer to avoid making
> > the same mistake again.
> Since I wasn't involved could you summarize the you are referring to?

The format "local" was a variant of "default" that would use the local
timezone instead of the author's. But there was no way to format, say,
iso8601 in the local timezone. So we had to invent a new syntax that was
compatible ("iso8601-local"), and keep "local" around forever for
backwards compatibility. Not the end of the world, but we can avoid it
in this case with a little preparation.

> > (I'd actually argue that "log.date" should basically _always_ have the
> > "auto" behavior, since it tends to get treated as plumbing anyway, and I
> > suspect that anybody who sets log.date now would see subtle breakage
> > from scripts. But maybe it's too late at this point?).
> If auto isn't added to the "log.date" file, then the date behaviour is not 
> changed from is currently in the code base.   Therefore, there shouldn't be 
> any breakage.

Right, this isn't a problem with your patches. I mean that the existing
"log.date" is arguably mis-designed, and we ought to have had something
like "auto" from day one (or even made it the default for log.date).

-Peff

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 1/3] Add 'human' date format
  2019-01-04  7:50       ` Jeff King
@ 2019-01-04 13:03         ` Stephen P Smith
  2019-01-06  6:19           ` Jeff King
  0 siblings, 1 reply; 33+ messages in thread
From: Stephen P Smith @ 2019-01-04 13:03 UTC (permalink / raw)
  To: Jeff King
  Cc: git, Linus Torvalds, Junio C Hamano,
	Ævar Arnfjörð Bjarmason

On Friday, January 4, 2019 12:50:35 AM MST Jeff King wrote:
> On Thu, Jan 03, 2019 at 06:19:56AM -0700, Stephen P. Smith wrote:
> > 
> > I didn't see anything in the code which would prohibit setting something
> > like that.
> 
> Yeah, I don't think supporting that is too hard. I was thinking
> something like this:

I take it that if I update Linus's patch, I still keep Junio's and Linus' 
sign-off line for the purpose of the chain of custody?  Of should I use a 
second patch?

Just trying to follow the rules.
sps



^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 1/3] Add 'human' date format
  2019-01-04 13:03         ` Stephen P Smith
@ 2019-01-06  6:19           ` Jeff King
  0 siblings, 0 replies; 33+ messages in thread
From: Jeff King @ 2019-01-06  6:19 UTC (permalink / raw)
  To: Stephen P Smith
  Cc: git, Linus Torvalds, Junio C Hamano,
	Ævar Arnfjörð Bjarmason

On Fri, Jan 04, 2019 at 06:03:18AM -0700, Stephen P Smith wrote:

> On Friday, January 4, 2019 12:50:35 AM MST Jeff King wrote:
> > On Thu, Jan 03, 2019 at 06:19:56AM -0700, Stephen P. Smith wrote:
> > > 
> > > I didn't see anything in the code which would prohibit setting something
> > > like that.
> > 
> > Yeah, I don't think supporting that is too hard. I was thinking
> > something like this:
> 
> I take it that if I update Linus's patch, I still keep Junio's and Linus' 
> sign-off line for the purpose of the chain of custody?  Of should I use a 
> second patch?

I think the most interesting question is the actual authorship (i.e.,
the "From:" field).  I think people are generally OK with having their
patches polished a bit to fix obvious bugs or short-comings. But at some
point if you make too many changes they or may not want to have the
result attributed to them. ;)

For the particular change I suggested, it's borderline to me on whether
it hits that case, so I'd probably err on the side of caution. And I'd
either expect Linus to say "yeah, that sounds like a good direction", or
I'd do it as a separate patch. And if a separate patch, I'd probably
tease Linus's patch out into two separate ones: one to add "human", and
one to implement "auto". And then drop the "auto" one in favor of your
new patch (with you as the author).

And I think that makes the signoff questions go away for this instance
(keep the signoffs for Linus's, and just signoff the new patch
yourself). But here's some general pontificating in that direction:

    Normally you can just drop Junio's signoff. The chain of custody is
    usually "author, then maintainer" and he'll re-add his maintainer
    signoff when he picks up your patch. In this case of this patch it's
    "author, then polisher, then maintainer", but Junio is still at the
    end.

    Now one can argue that Junio picked up Linus's patch, which you then
    picked up from Junio's repository and fed back to Junio. But you
    could just as well have picked Linus's patch up from the mailing
    list and then polished it. So I don't know that having Junio twice
    in the chain is really that interesting.

    Generally, yes, I'd keep Linus's signoff in a situation like this.
    He is asserting that the original work done meets the DCO
    requirements. You polishing the patch does not change that (of
    course you could introduce a bunch of new code that doesn't meet the
    DCO and sign it off anyway, but that's why there's ordering in the
    chain of custody. Somebody investigating would probably walk
    backwards up the chain).

-Peff

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 3/3] t0006-date.sh: add `human` date format tests.
  2018-12-31  0:31 ` [PATCH 3/3] t0006-date.sh: add `human` date format tests Stephen P. Smith
  2019-01-02 18:15   ` Junio C Hamano
  2019-01-03  7:44   ` Jeff King
@ 2019-01-08 21:27   ` Johannes Sixt
  2019-01-09  0:44     ` Stephen P. Smith
  2 siblings, 1 reply; 33+ messages in thread
From: Johannes Sixt @ 2019-01-08 21:27 UTC (permalink / raw)
  To: Stephen P . Smith
  Cc: git, Linus Torvalds, Junio C Hamano,
	Ævar Arnfjörð Bjarmason

Am 31.12.18 um 01:31 schrieb Stephen P. Smith:
> +check_human_date () {
> +	time=$1
> +	expect=$2
> +	test_expect_success "check date ($format:$time)" '
> +		echo "$time -> $expect" >expect &&
> +		TZ=${zone:-$TZ} test-tool date show:"$format" "$time" >actual &&
> +		grep "$expect" actual
> +	'
> +}
> +
>   # arbitrary but sensible time for examples
>   TIME='1466000000 +0200'
>   check_show iso8601 "$TIME" '2016-06-15 16:13:20 +0200'
> @@ -52,6 +62,20 @@ check_show unix "$TIME" '1466000000'
>   check_show iso-local "$TIME" '2016-06-15 14:13:20 +0000'
>   check_show raw-local "$TIME" '1466000000 +0000'
>   check_show unix-local "$TIME" '1466000000'
> +check_show human "$TIME" 'Jun 15 2016'
> +
> +# Subtract some known constant time and look for expected field format
> +TODAY_REGEX='5 hours ago'
> +THIS_YEAR_REGEX='[A-Z][a-z][a-z] [A-Z][a-z][a-z] [0-9]* [012][0-9]:[0-6][0-9]'
> +MORE_THAN_A_YEAR_REGEX='[A-Z][a-z][a-z] [A-Z][a-z][a-z] [0-9]* [0-9][0-9][0-9][0-9]'
> +check_human_date "$(($(date +%s)-18000)) +0200" $TODAY_REGEX # 5 hours ago
> +check_human_date "$(($(date +%s)-432000)) +0200" $THIS_YEAR_REGEX  # 5 days ago
> +check_human_date "$(($(date +%s)-1728000)) +0200" $THIS_YEAR_REGEX # 3 weeks ago
> +check_human_date "$(($(date +%s)-13000000)) +0200" $THIS_YEAR_REGEX # 5 months ago
> +check_human_date "$(($(date +%s)-31449600)) +0200" $THIS_YEAR_REGEX # 12 months ago
> +check_human_date "$(($(date +%s)-37500000)) +0200" $MORE_THAN_A_YEAR_REGEX # 1 year, 2 months ago
> +check_human_date "$(($(date +%s)-55188000)) +0200" $MORE_THAN_A_YEAR_REGEX # 1 year, 9 months ago
> +check_human_date "$(($(date +%s)-630000000)) +0200" $MORE_THAN_A_YEAR_REGEX # 20 years ago
>   
>   check_show 'format:%z' "$TIME" '+0200'
>   check_show 'format-local:%z' "$TIME" '+0000'
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 819c24d10e..d7f3b73650 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -1707,4 +1707,28 @@ test_expect_success '--exclude-promisor-objects does not BUG-crash' '
>   	test_must_fail git log --exclude-promisor-objects source-a
>   '
>   
> +check_human_date() {
> +	commit_date=$1
> +	expect=$2
> +	test_expect_success "$commit_date" "
> +		echo $expect $commit_date >dates &&
> +		git add dates &&
> +		git commit -m 'Expect String' --date=\"$commit_date\" dates &&
> +		git log -1 --date=human | grep \"^Date:\" >actual &&
> +		grep \"$expect\" actual
> +"
> +}
> +
> +TODAY_REGEX='[A-Z][a-z][a-z] [012][0-9]:[0-6][0-9] .0200'
> +THIS_YEAR_REGEX='[A-Z][a-z][a-z] [A-Z][a-z][a-z] [0-9]* [012][0-9]:[0-6][0-9]'
> +MORE_THAN_A_YEAR_REGEX='[A-Z][a-z][a-z] [A-Z][a-z][a-z] [0-9]* [0-9][0-9][0-9][0-9]'
> +check_human_date "$(($(date +%s)-18000)) +0200" $TODAY_REGEX # 5 hours ago
> +check_human_date "$(($(date +%s)-432000)) +0200" $THIS_YEAR_REGEX  # 5 days ago
> +check_human_date "$(($(date +%s)-1728000)) +0200" $THIS_YEAR_REGEX # 3 weeks ago
> +check_human_date "$(($(date +%s)-13000000)) +0200" $THIS_YEAR_REGEX # 5 months ago
> +check_human_date "$(($(date +%s)-31449600)) +0200" $THIS_YEAR_REGEX # 12 months ago
> +check_human_date "$(($(date +%s)-37500000)) +0200" $MORE_THAN_A_YEAR_REGEX # 1 year, 2 months ago
> +check_human_date "$(($(date +%s)-55188000)) +0200" $MORE_THAN_A_YEAR_REGEX # 1 year, 9 months ago
> +check_human_date "$(($(date +%s)-630000000)) +0200" $MORE_THAN_A_YEAR_REGEX # 20 years ago
> +
>   test_done
> diff --git a/t/t7007-show.sh b/t/t7007-show.sh
> index 42d3db6246..0a0334a8b5 100755
> --- a/t/t7007-show.sh
> +++ b/t/t7007-show.sh
> @@ -128,4 +128,29 @@ test_expect_success 'show --graph is forbidden' '
>     test_must_fail git show --graph HEAD
>   '
>   
> +check_human_date() {
> +	commit_date=$1
> +	expect=$2
> +	test_expect_success "$commit_date" "
> +		echo $expect $commit_date >dates &&
> +		git add dates &&
> +		git commit -m 'Expect String' --date=\"$commit_date\" dates &&
> +		git show --date=human | grep \"^Date:\" >actual &&
> +		grep \"$expect\" actual
> +"
> +}
> +
> +TODAY_REGEX='[A-Z][a-z][a-z] [012][0-9]:[0-6][0-9] .0200'
> +THIS_YEAR_REGEX='[A-Z][a-z][a-z] [A-Z][a-z][a-z] [0-9]* [012][0-9]:[0-6][0-9]'
> +MORE_THAN_A_YEAR_REGEX='[A-Z][a-z][a-z] [A-Z][a-z][a-z] [0-9]* [0-9][0-9][0-9][0-9]'
> +check_human_date "$(($(date +%s)-18000)) +0200" $TODAY_REGEX # 5 hours ago
> +check_human_date "$(($(date +%s)-432000)) +0200" $THIS_YEAR_REGEX  # 5 days ago
> +check_human_date "$(($(date +%s)-1728000)) +0200" $THIS_YEAR_REGEX # 3 weeks ago
> +check_human_date "$(($(date +%s)-13000000)) +0200" $THIS_YEAR_REGEX # 5 months ago
> +check_human_date "$(($(date +%s)-31449600)) +0200" $THIS_YEAR_REGEX # 12 months ago
> +check_human_date "$(($(date +%s)-37500000)) +0200" $MORE_THAN_A_YEAR_REGEX # 1 year, 2 months ago
> +check_human_date "$(($(date +%s)-55188000)) +0200" $MORE_THAN_A_YEAR_REGEX # 1 year, 9 months ago
> +check_human_date "$(($(date +%s)-630000000)) +0200" $MORE_THAN_A_YEAR_REGEX # 20 years ago

The $...REGEX expansions must be put in double-quotes to protect them 
from field splitting. But then the tests do not pass anymore (I tested 
only t4202). Please revisit this change.

-- Hannes

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 3/3] t0006-date.sh: add `human` date format tests.
  2019-01-08 21:27   ` Johannes Sixt
@ 2019-01-09  0:44     ` Stephen P. Smith
  2019-01-09  6:58       ` Johannes Sixt
  0 siblings, 1 reply; 33+ messages in thread
From: Stephen P. Smith @ 2019-01-09  0:44 UTC (permalink / raw)
  To: Johannes Sixt
  Cc: git, Linus Torvalds, Junio C Hamano,
	Ævar Arnfjörð Bjarmason

On Tuesday, January 8, 2019 2:27:22 PM MST Johannes Sixt wrote:
> Am 31.12.18 um 01:31 schrieb Stephen P. Smith:
> > +
> > +TODAY_REGEX='[A-Z][a-z][a-z] [012][0-9]:[0-6][0-9] .0200'
<snip>
> The $...REGEX expansions must be put in double-quotes to protect them
> from field splitting. But then the tests do not pass anymore (I tested
> only t4202). Please revisit this change.
> 
> -- Hannes

I will later figure out why you are seeing the fields splitting but I am not.   
In the mean time I will change the quoting.

I started working on test updates based on prior comments this past weekend.

sps





^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 3/3] t0006-date.sh: add `human` date format tests.
  2019-01-09  0:44     ` Stephen P. Smith
@ 2019-01-09  6:58       ` Johannes Sixt
  2019-01-10  1:50         ` Stephen & Linda Smith
  0 siblings, 1 reply; 33+ messages in thread
From: Johannes Sixt @ 2019-01-09  6:58 UTC (permalink / raw)
  To: Stephen P. Smith
  Cc: git, Linus Torvalds, Junio C Hamano,
	Ævar Arnfjörð Bjarmason

Am 09.01.19 um 01:44 schrieb Stephen P. Smith:
> On Tuesday, January 8, 2019 2:27:22 PM MST Johannes Sixt wrote:
>> Am 31.12.18 um 01:31 schrieb Stephen P. Smith:
>>> +
>>> +TODAY_REGEX='[A-Z][a-z][a-z] [012][0-9]:[0-6][0-9] .0200'
> <snip>
>> The $...REGEX expansions must be put in double-quotes to protect them
>> from field splitting. But then the tests do not pass anymore (I tested
>> only t4202). Please revisit this change.
> 
> I will later figure out why you are seeing the fields splitting but I am not.
> In the mean time I will change the quoting.

In this line

TODAY_REGEX='[A-Z][a-z][a-z] [012][0-9]:[0-6][0-9] .0200'

no field splitting occurs. The quoting is fine here. But notice that the 
value of $TODAY_REGEX contains blanks.

In this line

check_human_date "$(($(date +%s)-18000)) +0200" $TODAY_REGEX

the value of $TODAY_REGEX is substituted and then the value is split 
into fields at the blanks because the expansion is not quoted.

As a consequence, function check_human_date considers only the first 
part of $TODAY_REGEX, i.e. 'A-Z][a-z][a-z]' (which is parameter $2), but 
ignores everything else (because it does not use $3 or $4).

-- Hannes

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 3/3] t0006-date.sh: add `human` date format tests.
  2019-01-09  6:58       ` Johannes Sixt
@ 2019-01-10  1:50         ` Stephen & Linda Smith
  0 siblings, 0 replies; 33+ messages in thread
From: Stephen & Linda Smith @ 2019-01-10  1:50 UTC (permalink / raw)
  To: Johannes Sixt
  Cc: git, Linus Torvalds, Junio C Hamano,
	Ævar Arnfjörð Bjarmason

On Tuesday, January 8, 2019 11:58:29 PM MST Johannes Sixt wrote:
> But notice that the value of $TODAY_REGEX contains blanks.
> 
> In this line
> 
> check_human_date "$(($(date +%s)-18000)) +0200" $TODAY_REGEX
> 
> the value of $TODAY_REGEX is substituted and then the value is split
> into fields at the blanks because the expansion is not quoted.
> 
> As a consequence, function check_human_date considers only the first
> part of $TODAY_REGEX, i.e. 'A-Z][a-z][a-z]' (which is parameter $2), but
> ignores everything else (because it does not use $3 or $4).
> 
> -- Hannes

I hadn't understood your original comment, but now i understand.   Will fix.



^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH v2 0/5] Re-roll of 'human' date format patch set
  2018-12-31  0:31 [PATCH 0/3] Add 'human' date format Stephen P. Smith
                   ` (2 preceding siblings ...)
  2018-12-31  0:31 ` [PATCH 3/3] t0006-date.sh: add `human` date format tests Stephen P. Smith
@ 2019-01-18  6:18 ` Stephen P. Smith
  2019-01-18  6:18   ` [PATCH v2 1/5] Add 'human' date format Stephen P. Smith
                     ` (4 more replies)
  3 siblings, 5 replies; 33+ messages in thread
From: Stephen P. Smith @ 2019-01-18  6:18 UTC (permalink / raw)
  To: git
  Cc: Linus Torvalds, Junio C Hamano,
	Ævar Arnfjörð Bjarmason, Jeff King, Philip Oakley,
	Johannes Sixt

Reworked documentation and tests for the previously submitted patch set. 


Linus Torvalds (1):
  Add 'human' date format

Stephen P. Smith (4):
  Remove the proposed use of auto as secondary way to specify human
  Add 'human' date format documentation
  Add `human` format to test-tool
  Add `human` date format tests.

 Documentation/git-log.txt          |   4 +
 Documentation/rev-list-options.txt |   6 ++
 builtin/blame.c                    |   4 +
 cache.h                            |   3 +
 date.c                             | 153 +++++++++++++++++++++++++----
 t/helper/test-date.c               |  15 +++
 t/t0006-date.sh                    |  20 ++++
 7 files changed, 185 insertions(+), 20 deletions(-)

-- 
2.20.1.2.gb21ebb671b


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH v2 1/5] Add 'human' date format
  2019-01-18  6:18 ` [PATCH v2 0/5] Re-roll of 'human' date format patch set Stephen P. Smith
@ 2019-01-18  6:18   ` Stephen P. Smith
  2019-01-18  6:18   ` [PATCH v2 2/5] Remove the proposed use of auto as secondary way to specify human Stephen P. Smith
                     ` (3 subsequent siblings)
  4 siblings, 0 replies; 33+ messages in thread
From: Stephen P. Smith @ 2019-01-18  6:18 UTC (permalink / raw)
  To: git
  Cc: Linus Torvalds, Junio C Hamano,
	Ævar Arnfjörð Bjarmason, Jeff King, Philip Oakley,
	Johannes Sixt

From: Linus Torvalds <torvalds@linux-foundation.org>

This adds --date=human, which skips the timezone if it matches the
current time-zone, and doesn't print the whole date if that matches (ie
skip printing year for dates that are "this year", but also skip the
whole date itself if it's in the last few days and we can just say what
weekday it was).

For really recent dates (same day), use the relative date stamp, while
for old dates (year doesn't match), don't bother with time and timezone.

Also add 'auto' date mode, which defaults to human if we're using the
pager.  So you can do

	git config --add log.date auto

and your "git log" commands will show the human-legible format unless
you're scripting things.

Note that this time format still shows the timezone for recent enough
events (but not so recent that they show up as relative dates).  You can
combine it with the "-local" suffix to never show timezones for an even
more simplified view.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Stephen P. Smith <ischis2@cox.net>
---
 builtin/blame.c |   4 ++
 cache.h         |   1 +
 date.c          | 130 ++++++++++++++++++++++++++++++++++++++++--------
 3 files changed, 115 insertions(+), 20 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 6d798f9939..f684e31d82 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -925,6 +925,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		 */
 		blame_date_width = utf8_strwidth(_("4 years, 11 months ago")) + 1; /* add the null */
 		break;
+	case DATE_HUMAN:
+		/* If the year is shown, no time is shown */
+		blame_date_width = sizeof("Thu Oct 19 16:00");
+		break;
 	case DATE_NORMAL:
 		blame_date_width = sizeof("Thu Oct 19 16:00:04 2006 -0700");
 		break;
diff --git a/cache.h b/cache.h
index 49713cc5a5..34c33e6a28 100644
--- a/cache.h
+++ b/cache.h
@@ -1439,6 +1439,7 @@ extern struct object *peel_to_type(const char *name, int namelen,
 
 enum date_mode_type {
 	DATE_NORMAL = 0,
+	DATE_HUMAN,
 	DATE_RELATIVE,
 	DATE_SHORT,
 	DATE_ISO8601,
diff --git a/date.c b/date.c
index 9bc15df6f9..a8d50eb206 100644
--- a/date.c
+++ b/date.c
@@ -77,22 +77,16 @@ static struct tm *time_to_tm_local(timestamp_t time)
 }
 
 /*
- * What value of "tz" was in effect back then at "time" in the
- * local timezone?
+ * Fill in the localtime 'struct tm' for the supplied time,
+ * and return the local tz.
  */
-static int local_tzoffset(timestamp_t time)
+static int local_time_tzoffset(time_t t, struct tm *tm)
 {
-	time_t t, t_local;
-	struct tm tm;
+	time_t t_local;
 	int offset, eastwest;
 
-	if (date_overflows(time))
-		die("Timestamp too large for this system: %"PRItime, time);
-
-	t = (time_t)time;
-	localtime_r(&t, &tm);
-	t_local = tm_to_time_t(&tm);
-
+	localtime_r(&t, tm);
+	t_local = tm_to_time_t(tm);
 	if (t_local == -1)
 		return 0; /* error; just use +0000 */
 	if (t_local < t) {
@@ -107,6 +101,20 @@ static int local_tzoffset(timestamp_t time)
 	return offset * eastwest;
 }
 
+/*
+ * What value of "tz" was in effect back then at "time" in the
+ * local timezone?
+ */
+static int local_tzoffset(timestamp_t time)
+{
+	struct tm tm;
+
+	if (date_overflows(time))
+		die("Timestamp too large for this system: %"PRItime, time);
+
+	return local_time_tzoffset((time_t)time, &tm);
+}
+
 void show_date_relative(timestamp_t time, int tz,
 			       const struct timeval *now,
 			       struct strbuf *timebuf)
@@ -191,9 +199,80 @@ struct date_mode *date_mode_from_type(enum date_mode_type type)
 	return &mode;
 }
 
+static void show_date_normal(struct strbuf *buf, timestamp_t time, struct tm *tm, int tz, struct tm *human_tm, int human_tz, int local)
+{
+	struct {
+		unsigned int	year:1,
+				date:1,
+				wday:1,
+				time:1,
+				seconds:1,
+				tz:1;
+	} hide = { 0 };
+
+	hide.tz = local || tz == human_tz;
+	hide.year = tm->tm_year == human_tm->tm_year;
+	if (hide.year) {
+		if (tm->tm_mon == human_tm->tm_mon) {
+			if (tm->tm_mday > human_tm->tm_mday) {
+				/* Future date: think timezones */
+			} else if (tm->tm_mday == human_tm->tm_mday) {
+				hide.date = hide.wday = 1;
+			} else if (tm->tm_mday + 5 > human_tm->tm_mday) {
+				/* Leave just weekday if it was a few days ago */
+				hide.date = 1;
+			}
+		}
+	}
+
+	/* Show "today" times as just relative times */
+	if (hide.wday) {
+		struct timeval now;
+		gettimeofday(&now, NULL);
+		show_date_relative(time, tz, &now, buf);
+		return;
+	}
+
+	/*
+	 * Always hide seconds for human-readable.
+	 * Hide timezone if showing date.
+	 * Hide weekday and time if showing year.
+	 *
+	 * The logic here is two-fold:
+	 *  (a) only show details when recent enough to matter
+	 *  (b) keep the maximum length "similar", and in check
+	 */
+	if (human_tm->tm_year) {
+		hide.seconds = 1;
+		hide.tz |= !hide.date;
+		hide.wday = hide.time = !hide.year;
+	}
+
+	if (!hide.wday)
+		strbuf_addf(buf, "%.3s ", weekday_names[tm->tm_wday]);
+	if (!hide.date)
+		strbuf_addf(buf, "%.3s %d ", month_names[tm->tm_mon], tm->tm_mday);
+
+	/* Do we want AM/PM depending on locale? */
+	if (!hide.time) {
+		strbuf_addf(buf, "%02d:%02d", tm->tm_hour, tm->tm_min);
+		if (!hide.seconds)
+			strbuf_addf(buf, ":%02d", tm->tm_sec);
+	} else
+		strbuf_rtrim(buf);
+
+	if (!hide.year)
+		strbuf_addf(buf, " %d", tm->tm_year + 1900);
+
+	if (!hide.tz)
+		strbuf_addf(buf, " %+05d", tz);
+}
+
 const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 {
 	struct tm *tm;
+	struct tm human_tm = { 0 };
+	int human_tz = -1;
 	static struct strbuf timebuf = STRBUF_INIT;
 
 	if (mode->type == DATE_UNIX) {
@@ -202,6 +281,15 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 		return timebuf.buf;
 	}
 
+	if (mode->type == DATE_HUMAN) {
+		struct timeval now;
+
+		gettimeofday(&now, NULL);
+
+		/* Fill in the data for "current time" in human_tz and human_tm */
+		human_tz = local_time_tzoffset(now.tv_sec, &human_tm);
+	}
+
 	if (mode->local)
 		tz = local_tzoffset(time);
 
@@ -258,14 +346,7 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 		strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
 				!mode->local);
 	else
-		strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
-				weekday_names[tm->tm_wday],
-				month_names[tm->tm_mon],
-				tm->tm_mday,
-				tm->tm_hour, tm->tm_min, tm->tm_sec,
-				tm->tm_year + 1900,
-				mode->local ? 0 : ' ',
-				tz);
+		show_date_normal(&timebuf, time, tm, tz, &human_tm, human_tz, mode->local);
 	return timebuf.buf;
 }
 
@@ -802,6 +883,11 @@ int parse_date(const char *date, struct strbuf *result)
 	return 0;
 }
 
+static int auto_date_style(void)
+{
+	return (isatty(1) || pager_in_use()) ? DATE_HUMAN : DATE_NORMAL;
+}
+
 static enum date_mode_type parse_date_type(const char *format, const char **end)
 {
 	if (skip_prefix(format, "relative", end))
@@ -819,6 +905,10 @@ static enum date_mode_type parse_date_type(const char *format, const char **end)
 		return DATE_SHORT;
 	if (skip_prefix(format, "default", end))
 		return DATE_NORMAL;
+	if (skip_prefix(format, "human", end))
+		return DATE_HUMAN;
+	if (skip_prefix(format, "auto", end))
+		return auto_date_style();
 	if (skip_prefix(format, "raw", end))
 		return DATE_RAW;
 	if (skip_prefix(format, "unix", end))
-- 
2.20.1.2.gb21ebb671b


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH v2 2/5] Remove the proposed use of auto as secondary way to specify human
  2019-01-18  6:18 ` [PATCH v2 0/5] Re-roll of 'human' date format patch set Stephen P. Smith
  2019-01-18  6:18   ` [PATCH v2 1/5] Add 'human' date format Stephen P. Smith
@ 2019-01-18  6:18   ` Stephen P. Smith
  2019-01-18 18:35     ` Junio C Hamano
  2019-01-18  6:18   ` [PATCH v2 3/5] Add 'human' date format documentation Stephen P. Smith
                     ` (2 subsequent siblings)
  4 siblings, 1 reply; 33+ messages in thread
From: Stephen P. Smith @ 2019-01-18  6:18 UTC (permalink / raw)
  To: git
  Cc: Linus Torvalds, Junio C Hamano,
	Ævar Arnfjörð Bjarmason, Jeff King, Philip Oakley,
	Johannes Sixt

In addition to adding the 'human' format, the patch added the auto
keyword which could be used in the config file as an alternate way to
specify the human format.  Removing 'auto' cleans up the 'human'
format interface.

Instead add 'auto:human' date mode which defaults to human if we're
using the pager.  So you can do

	git config --add log.date auto:human

and your "git log" commands will show the human-legible format unless
you're scripting things.

Signed-off-by: Stephen P. Smith <ischis2@cox.net>
---
 date.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/date.c b/date.c
index a8d50eb206..43c3a84e25 100644
--- a/date.c
+++ b/date.c
@@ -883,11 +883,6 @@ int parse_date(const char *date, struct strbuf *result)
 	return 0;
 }
 
-static int auto_date_style(void)
-{
-	return (isatty(1) || pager_in_use()) ? DATE_HUMAN : DATE_NORMAL;
-}
-
 static enum date_mode_type parse_date_type(const char *format, const char **end)
 {
 	if (skip_prefix(format, "relative", end))
@@ -907,8 +902,6 @@ static enum date_mode_type parse_date_type(const char *format, const char **end)
 		return DATE_NORMAL;
 	if (skip_prefix(format, "human", end))
 		return DATE_HUMAN;
-	if (skip_prefix(format, "auto", end))
-		return auto_date_style();
 	if (skip_prefix(format, "raw", end))
 		return DATE_RAW;
 	if (skip_prefix(format, "unix", end))
@@ -923,6 +916,14 @@ void parse_date_format(const char *format, struct date_mode *mode)
 {
 	const char *p;
 
+	/* "auto:foo" is "if tty/pager, then foo, otherwise normal" */
+	if (skip_prefix(format, "auto:", &p)) {
+		if (isatty(1) || pager_in_use())
+			format = p;
+		else
+			format = "default";
+	}
+
 	/* historical alias */
 	if (!strcmp(format, "local"))
 		format = "default-local";
-- 
2.20.1.2.gb21ebb671b


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH v2 3/5] Add 'human' date format documentation
  2019-01-18  6:18 ` [PATCH v2 0/5] Re-roll of 'human' date format patch set Stephen P. Smith
  2019-01-18  6:18   ` [PATCH v2 1/5] Add 'human' date format Stephen P. Smith
  2019-01-18  6:18   ` [PATCH v2 2/5] Remove the proposed use of auto as secondary way to specify human Stephen P. Smith
@ 2019-01-18  6:18   ` Stephen P. Smith
  2019-01-18 18:47     ` Junio C Hamano
  2019-01-18  6:18   ` [PATCH v2 4/5] Add `human` format to test-tool Stephen P. Smith
  2019-01-18  6:18   ` [PATCH v2 5/5] Add `human` date format tests Stephen P. Smith
  4 siblings, 1 reply; 33+ messages in thread
From: Stephen P. Smith @ 2019-01-18  6:18 UTC (permalink / raw)
  To: git
  Cc: Linus Torvalds, Junio C Hamano,
	Ævar Arnfjörð Bjarmason, Jeff King, Philip Oakley,
	Johannes Sixt

Display date and time information in a format similar to how people
write dates in other contexts. If the year isn't specified then, the
reader infers the date is given is in the current year.

By not displaying the redundant information, the reader concentrates
on the information that is different. The patch reports relative dates
based on information inferred from the date on the machine running the
git command at the time the command is executed.

While the format is more useful to humans by dropping inferred
information, there is nothing that makes it actually human. If the
'relative' date format wasn't already implemented then using
'relative' would have been appropriate.

Signed-off-by: Stephen P. Smith <ischis2@cox.net>
---
 Documentation/git-log.txt          | 4 ++++
 Documentation/rev-list-options.txt | 6 ++++++
 2 files changed, 10 insertions(+)

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 90761f1694..1d2d932c76 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -193,6 +193,10 @@ log.date::
 	`--date` option.)  Defaults to "default", which means to write
 	dates like `Sat May 8 19:35:34 2010 -0500`.
 
+	If the format is set to "auto:foo", then if the pager is in
+	use format "foo" will be the used for the date format, otherwise
+	"default" will be used.
+
 log.follow::
 	If `true`, `git log` will act as if the `--follow` option was used when
 	a single <path> is given.  This has the same limitations as `--follow`,
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index bab5f50b17..5d58f35d19 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -835,6 +835,12 @@ Note that the `-local` option does not affect the seconds-since-epoch
 value (which is always measured in UTC), but does switch the accompanying
 timezone value.
 +
+`--date=human` shows the timezone if it matches the current time-zone,
+and doesn't print the whole date if that matches (ie skip printing
+year for dates that are "this year", but also skip the whole date
+itself if it's in the last few days and we can just say what weekday
+it was).
++
 `--date=unix` shows the date as a Unix epoch timestamp (seconds since
 1970).  As with `--raw`, this is always in UTC and therefore `-local`
 has no effect.
-- 
2.20.1.2.gb21ebb671b


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH v2 4/5] Add `human` format to test-tool
  2019-01-18  6:18 ` [PATCH v2 0/5] Re-roll of 'human' date format patch set Stephen P. Smith
                     ` (2 preceding siblings ...)
  2019-01-18  6:18   ` [PATCH v2 3/5] Add 'human' date format documentation Stephen P. Smith
@ 2019-01-18  6:18   ` Stephen P. Smith
  2019-01-18 19:03     ` Junio C Hamano
  2019-01-18  6:18   ` [PATCH v2 5/5] Add `human` date format tests Stephen P. Smith
  4 siblings, 1 reply; 33+ messages in thread
From: Stephen P. Smith @ 2019-01-18  6:18 UTC (permalink / raw)
  To: git
  Cc: Linus Torvalds, Junio C Hamano,
	Ævar Arnfjörð Bjarmason, Jeff King, Philip Oakley,
	Johannes Sixt

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

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"))
-- 
2.20.1.2.gb21ebb671b


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH v2 5/5] Add `human` date format tests.
  2019-01-18  6:18 ` [PATCH v2 0/5] Re-roll of 'human' date format patch set Stephen P. Smith
                     ` (3 preceding siblings ...)
  2019-01-18  6:18   ` [PATCH v2 4/5] Add `human` format to test-tool Stephen P. Smith
@ 2019-01-18  6:18   ` Stephen P. Smith
  2019-01-18 19:24     ` Junio C Hamano
  4 siblings, 1 reply; 33+ messages in thread
From: Stephen P. Smith @ 2019-01-18  6:18 UTC (permalink / raw)
  To: git
  Cc: Linus Torvalds, Junio C Hamano,
	Ævar Arnfjörð Bjarmason, Jeff King, Philip Oakley,
	Johannes Sixt

When using `human` several fields are suppressed depending on the time
difference between the reference date and the local computer date. In
cases where the difference is less than a year, the year field is
supppressed. If the time is less than a day; the month and year is
suppressed.

Use TEST_DATE_NOW environment variable when using the test-tool to
hold the expected output strings constant.

Signed-off-by: Stephen P. Smith <ischis2@cox.net>
---
 t/t0006-date.sh | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index ffb2975e48..c7c0786b24 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh
@@ -128,4 +128,24 @@ check_approxidate '6AM, June 7, 2009' '2009-06-07 06:00:00'
 check_approxidate '2008-12-01' '2008-12-01 19:20:00'
 check_approxidate '2009-12-01' '2009-12-01 19:20:00'
 
+check_date_format() {
+	format=$1
+	t=$(($TEST_DATE_NOW - $2))
+	expect=$3
+	test_expect_success "human date $t" "
+	echo $TEST_DATE_NOW >now &&
+	test-tool date human $t >actual &&
+	grep '$expect' actual
+"
+}
+
+check_date_format human 18000 "5 hours ago" # 5 hours ago
+check_date_format human 432000 "Tue Aug 25 19:20" # 5 days ago
+check_date_format human 1728000 "Mon Aug 10 19:20" # 3 weeks ago
+check_date_format human 13000000 "Thu Apr 2 08:13" # 5 months ago
+check_date_format human 31449600 "Aug 31 2008" # 12 months ago
+check_date_format human 37500000 "Jun 22 2008" # 1 year, 2 months ago
+check_date_format human 55188000 "Dec 1 2007" # 1 year, 9 months ago
+check_date_format human 630000000 "Sep 13 1989" # 20 years ago
+
 test_done
-- 
2.20.1.2.gb21ebb671b


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v2 2/5] Remove the proposed use of auto as secondary way to specify human
  2019-01-18  6:18   ` [PATCH v2 2/5] Remove the proposed use of auto as secondary way to specify human Stephen P. Smith
@ 2019-01-18 18:35     ` Junio C Hamano
  2019-01-19  3:44       ` Stephen & Linda Smith
  0 siblings, 1 reply; 33+ messages in thread
From: Junio C Hamano @ 2019-01-18 18:35 UTC (permalink / raw)
  To: Stephen P. Smith
  Cc: git, Linus Torvalds, Ævar Arnfjörð Bjarmason,
	Jeff King, Philip Oakley, Johannes Sixt

"Stephen P. Smith" <ischis2@cox.net> writes:

> In addition to adding the 'human' format, the patch added the auto
> keyword which could be used in the config file as an alternate way to
> specify the human format.  Removing 'auto' cleans up the 'human'
> format interface.
>
> Instead add 'auto:human' date mode which defaults to human if we're
> using the pager.  So you can do
>
> 	git config --add log.date auto:human
>
> and your "git log" commands will show the human-legible format unless
> you're scripting things.

I think doing two things in this step (i.e. reverting Linus's "auto"
support from 1/5, and adding "auto" that is similar to color's auto)
is OK, but then the title should list both.  It sounded like it was
this step is doing only the former.

>
> Signed-off-by: Stephen P. Smith <ischis2@cox.net>
> ---
>  date.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/date.c b/date.c
> index a8d50eb206..43c3a84e25 100644
> --- a/date.c
> +++ b/date.c
> @@ -883,11 +883,6 @@ int parse_date(const char *date, struct strbuf *result)
>  	return 0;
>  }
>  
> -static int auto_date_style(void)
> -{
> -	return (isatty(1) || pager_in_use()) ? DATE_HUMAN : DATE_NORMAL;
> -}
> -
>  static enum date_mode_type parse_date_type(const char *format, const char **end)
>  {
>  	if (skip_prefix(format, "relative", end))
> @@ -907,8 +902,6 @@ static enum date_mode_type parse_date_type(const char *format, const char **end)
>  		return DATE_NORMAL;
>  	if (skip_prefix(format, "human", end))
>  		return DATE_HUMAN;
> -	if (skip_prefix(format, "auto", end))
> -		return auto_date_style();
>  	if (skip_prefix(format, "raw", end))
>  		return DATE_RAW;
>  	if (skip_prefix(format, "unix", end))
> @@ -923,6 +916,14 @@ void parse_date_format(const char *format, struct date_mode *mode)
>  {
>  	const char *p;
>  
> +	/* "auto:foo" is "if tty/pager, then foo, otherwise normal" */
> +	if (skip_prefix(format, "auto:", &p)) {
> +		if (isatty(1) || pager_in_use())
> +			format = p;
> +		else
> +			format = "default";
> +	}
> +
>  	/* historical alias */
>  	if (!strcmp(format, "local"))
>  		format = "default-local";

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v2 3/5] Add 'human' date format documentation
  2019-01-18  6:18   ` [PATCH v2 3/5] Add 'human' date format documentation Stephen P. Smith
@ 2019-01-18 18:47     ` Junio C Hamano
  0 siblings, 0 replies; 33+ messages in thread
From: Junio C Hamano @ 2019-01-18 18:47 UTC (permalink / raw)
  To: Stephen P. Smith
  Cc: git, Linus Torvalds, Ævar Arnfjörð Bjarmason,
	Jeff King, Philip Oakley, Johannes Sixt

"Stephen P. Smith" <ischis2@cox.net> writes:

> diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
> index 90761f1694..1d2d932c76 100644
> --- a/Documentation/git-log.txt
> +++ b/Documentation/git-log.txt
> @@ -193,6 +193,10 @@ log.date::
>  	`--date` option.)  Defaults to "default", which means to write
>  	dates like `Sat May 8 19:35:34 2010 -0500`.
>  
> +	If the format is set to "auto:foo", then if the pager is in
> +	use format "foo" will be the used for the date format, otherwise
> +	"default" will be used.
> +

This text is good, but this would break ASCIIdoc formatting,
wouldn't it?  Observe how "notes.displayRef::" section does
three-paragraph description and mimick it to make this two-paragraph
description, perhaps.

>  log.follow::
>  	If `true`, `git log` will act as if the `--follow` option was used when
>  	a single <path> is given.  This has the same limitations as `--follow`,
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index bab5f50b17..5d58f35d19 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -835,6 +835,12 @@ Note that the `-local` option does not affect the seconds-since-epoch
>  value (which is always measured in UTC), but does switch the accompanying
>  timezone value.
>  +
> +`--date=human` shows the timezone if it matches the current time-zone,

Is it clear in the context that "it" refers to "the timestamp being
shown"?

I think the behaviour is that timezone is shown only the timestamp
being shown is from a different timezone (i.e. if it *does* *not*
match), though.

> +and doesn't print the whole date if that matches (ie skip printing
> +year for dates that are "this year", but also skip the whole date
> +itself if it's in the last few days and we can just say what weekday
> +it was).
> ++

... and also omit hour/minute part for a timestamp that is old
enough.

>  `--date=unix` shows the date as a Unix epoch timestamp (seconds since
>  1970).  As with `--raw`, this is always in UTC and therefore `-local`
>  has no effect.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v2 4/5] Add `human` format to test-tool
  2019-01-18  6:18   ` [PATCH v2 4/5] Add `human` format to test-tool Stephen P. Smith
@ 2019-01-18 19:03     ` Junio C Hamano
  0 siblings, 0 replies; 33+ messages in thread
From: Junio C Hamano @ 2019-01-18 19:03 UTC (permalink / raw)
  To: Stephen P. Smith
  Cc: git, Linus Torvalds, Ævar Arnfjörð Bjarmason,
	Jeff King, Philip Oakley, Johannes Sixt

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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v2 5/5] Add `human` date format tests.
  2019-01-18  6:18   ` [PATCH v2 5/5] Add `human` date format tests Stephen P. Smith
@ 2019-01-18 19:24     ` Junio C Hamano
  0 siblings, 0 replies; 33+ messages in thread
From: Junio C Hamano @ 2019-01-18 19:24 UTC (permalink / raw)
  To: Stephen P. Smith
  Cc: git, Linus Torvalds, Ævar Arnfjörð Bjarmason,
	Jeff King, Philip Oakley, Johannes Sixt

"Stephen P. Smith" <ischis2@cox.net> writes:

> +check_date_format() {
> +	format=$1
> +	t=$(($TEST_DATE_NOW - $2))
> +	expect=$3

Notice that neither $format nor $1 is used in this test, which means
that "check_date_format" is not a generic "I can take a format
parameter to check the specified one".  So perhaps

	check_date_format_human () {

and then lose the first parameter?

> +	test_expect_success "human date $t" "
> +	echo $TEST_DATE_NOW >now &&
> +	test-tool date human $t >actual &&
> +	grep '$expect' actual
> +"

Hopefully $3 does not have a single quote in it ;-)  

But the test block can see the shell variables just fine, so writing
it like the following is more in line with how the test framework is
designed to be used.

	test_expect_success "human date $t" '
		echo "$TEST_DATE_NOW" >now &&
		test-tool date human "$t" >actual &&
		grep "$expect" actual
	'

How is the file 'now' get used?  Nobody seems to read it around here.
Is the last one supposed to be "grep"?  Or should we do

	echo "$expect" >expect &&
	test_cmp expect actual

instead?

> +}
> +
> +check_date_format human 18000 "5 hours ago" # 5 hours ago
> +check_date_format human 432000 "Tue Aug 25 19:20" # 5 days ago
> +check_date_format human 1728000 "Mon Aug 10 19:20" # 3 weeks ago
> +check_date_format human 13000000 "Thu Apr 2 08:13" # 5 months ago
> +check_date_format human 31449600 "Aug 31 2008" # 12 months ago
> +check_date_format human 37500000 "Jun 22 2008" # 1 year, 2 months ago
> +check_date_format human 55188000 "Dec 1 2007" # 1 year, 9 months ago
> +check_date_format human 630000000 "Sep 13 1989" # 20 years ago
> +
>  test_done

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v2 2/5] Remove the proposed use of auto as secondary way to specify human
  2019-01-18 18:35     ` Junio C Hamano
@ 2019-01-19  3:44       ` Stephen & Linda Smith
  0 siblings, 0 replies; 33+ messages in thread
From: Stephen & Linda Smith @ 2019-01-19  3:44 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, Linus Torvalds, Ævar Arnfjörð Bjarmason,
	Jeff King, Philip Oakley, Johannes Sixt

On Friday, January 18, 2019 11:35:22 AM MST Junio C Hamano wrote:
> "Stephen P. Smith" <ischis2@cox.net> writes:
> I think doing two things in this step (i.e. reverting Linus's "auto"
> support from 1/5, and adding "auto" that is similar to color's auto)
> is OK, but then the title should list both.  It sounded like it was
> this step is doing only the former.

Will change as part of a re-roll.




^ permalink raw reply	[flat|nested] 33+ messages in thread

end of thread, back to index

Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-31  0:31 [PATCH 0/3] Add 'human' date format Stephen P. Smith
2018-12-31  0:31 ` [PATCH 1/3] " Stephen P. Smith
2019-01-03  7:37   ` Jeff King
2019-01-03 13:19     ` Stephen P. Smith
2019-01-04  7:50       ` Jeff King
2019-01-04 13:03         ` Stephen P Smith
2019-01-06  6:19           ` Jeff King
2018-12-31  0:31 ` [PATCH 2/3] Add 'human' date format documentation Stephen P. Smith
2018-12-31  0:31 ` [PATCH 3/3] t0006-date.sh: add `human` date format tests Stephen P. Smith
2019-01-02 18:15   ` Junio C Hamano
2019-01-03  2:36     ` Stephen & Linda Smith
2019-01-03  6:42       ` Junio C Hamano
2019-01-03 13:20         ` Stephen P. Smith
2019-01-03 21:14     ` Philip Oakley
2019-01-03 21:45       ` Junio C Hamano
2019-01-03 23:57         ` Stephen P. Smith
2019-01-03  7:44   ` Jeff King
2019-01-03 13:12     ` Stephen & Linda Smith
2019-01-08 21:27   ` Johannes Sixt
2019-01-09  0:44     ` Stephen P. Smith
2019-01-09  6:58       ` Johannes Sixt
2019-01-10  1:50         ` Stephen & Linda Smith
2019-01-18  6:18 ` [PATCH v2 0/5] Re-roll of 'human' date format patch set Stephen P. Smith
2019-01-18  6:18   ` [PATCH v2 1/5] Add 'human' date format Stephen P. Smith
2019-01-18  6:18   ` [PATCH v2 2/5] Remove the proposed use of auto as secondary way to specify human Stephen P. Smith
2019-01-18 18:35     ` Junio C Hamano
2019-01-19  3:44       ` Stephen & Linda Smith
2019-01-18  6:18   ` [PATCH v2 3/5] Add 'human' date format documentation Stephen P. Smith
2019-01-18 18:47     ` Junio C Hamano
2019-01-18  6:18   ` [PATCH v2 4/5] Add `human` format to test-tool Stephen P. Smith
2019-01-18 19:03     ` Junio C Hamano
2019-01-18  6:18   ` [PATCH v2 5/5] Add `human` date format tests Stephen P. Smith
2019-01-18 19:24     ` Junio C Hamano

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox