date: allow any format to display local time

Make DATE_LOCAL a bit flag that may be combined with the other formats. In order to keep date_mode_type as a true enumeration the possible combinations are included explicitly (except "relative local time" which is nonsensical).

Signed-off-by John Keeping <[email protected]>

---

builtin/blame.c +1 -2

index 4db01c1..dff6934 100644
@@ -2575,7 +2575,7 @@ parse_done:
 	}
 
 	/* The maximum width used to show the dates */
-	switch (blame_date_mode.type) {
+	switch (blame_date_mode.type & ~DATE_LOCAL) {
 	case DATE_RFC2822:
 		blame_date_width = sizeof("Thu, 19 Oct 2006 16:00:04 -0700");
 		break;

@@ -2600,7 +2600,6 @@ parse_done:
 		   fewer display columns. */
 		blame_date_width = utf8_strwidth(_("4 years, 11 months ago")) + 1; /* add the null */
 		break;
-	case DATE_LOCAL:
 	case DATE_NORMAL:
 		blame_date_width = sizeof("Thu Oct 19 16:00:04 2006 -0700");
 		break;

cache.h +0 -2

index 4e25271..cda5c51 100644
@@ -1091,12 +1091,17 @@ struct date_mode {
 		DATE_NORMAL = 0,
 		DATE_RELATIVE,
 		DATE_SHORT,
-		DATE_LOCAL,
 		DATE_ISO8601,
 		DATE_ISO8601_STRICT,
 		DATE_RFC2822,
 		DATE_STRFTIME,
-		DATE_RAW

date.c +8 -6

index 8f91569..e0e0f3b 100644
@@ -163,7 +163,7 @@ void show_date_relative(unsigned long time, int tz,
 struct date_mode *date_mode_from_type(enum date_mode_type type)
 {
 	static struct date_mode mode;
-	if (type == DATE_STRFTIME)
+	if (type == DATE_STRFTIME || type == DATE_STRFTIME_LOCAL)
 		die("BUG: cannot create anonymous strftime date_mode struct");
 	mode.type = type;
 	return &mode;

@@ -173,6 +173,7 @@ const char *show_date(unsigned long time, int tz, const struct date_mode *mode)
 {
 	struct tm *tm;
 	static struct strbuf timebuf = STRBUF_INIT;
+	enum date_mode_type type = mode->type;
 
 	if (mode->type == DATE_RAW) {
 		strbuf_reset(&timebuf);

@@ -189,8 +190,10 @@ const char *show_date(unsigned long time, int tz, const struct date_mode *mode)
 		return timebuf.buf;
 	}
 
-	if (mode->type == DATE_LOCAL)
+	if (type & DATE_LOCAL) {
 		tz = local_tzoffset(time);
+		type &= ~DATE_LOCAL;
+	}
 
 	tm = time_to_tm(time, tz);
 	if (!tm) {

@@ -199,17 +202,17 @@ const char *show_date(unsigned long time, int tz, const struct date_mode *mode)
 	}
 
 	strbuf_reset(&timebuf);
-	if (mode->type == DATE_SHORT)
+	if (type == DATE_SHORT)
 		strbuf_addf(&timebuf, "%04d-%02d-%02d", tm->tm_year + 1900,
 				tm->tm_mon + 1, tm->tm_mday);
-	else if (mode->type == DATE_ISO8601)
+	else if (type == DATE_ISO8601)
 		strbuf_addf(&timebuf, "%04d-%02d-%02d %02d:%02d:%02d %+05d",
 				tm->tm_year + 1900,
 				tm->tm_mon + 1,
 				tm->tm_mday,
 				tm->tm_hour, tm->tm_min, tm->tm_sec,
 				tz);
-	else if (mode->type == DATE_ISO8601_STRICT) {
+	else if (type == DATE_ISO8601_STRICT) {
 		char sign = (tz >= 0) ? '+' : '-';
 		tz = abs(tz);
 		strbuf_addf(&timebuf, "%04d-%02d-%02dT%02d:%02d:%02d%c%02d:%02d",

@@ -218,12 +221,12 @@ const char *show_date(unsigned long time, int tz, const struct date_mode *mode)
 				tm->tm_mday,
 				tm->tm_hour, tm->tm_min, tm->tm_sec,
 				sign, tz / 100, tz % 100);
-	} else if (mode->type == DATE_RFC2822)

date.c +14 -0

@@ -777,14 +780,25 @@ void parse_date_format(const char *format, struct date_mode *mode)
 	else if (!strcmp(format, "iso8601") ||
 		 !strcmp(format, "iso"))
 		mode->type = DATE_ISO8601;
+	else if (!strcmp(format, "iso8601-local") ||
+		 !strcmp(format, "iso-local"))
+		mode->type = DATE_ISO8601_LOCAL;
 	else if (!strcmp(format, "iso8601-strict") ||
 		 !strcmp(format, "iso-strict"))
 		mode->type = DATE_ISO8601_STRICT;
+	else if (!strcmp(format, "iso8601-strict-local") ||
+		 !strcmp(format, "iso-strict-local"))
+		mode->type = DATE_ISO8601_STRICT_LOCAL;
 	else if (!strcmp(format, "rfc2822") ||
 		 !strcmp(format, "rfc"))
 		mode->type = DATE_RFC2822;
+	else if (!strcmp(format, "rfc2822-local") ||
+		 !strcmp(format, "rfc-local"))
+		mode->type = DATE_RFC2822_LOCAL;
 	else if (!strcmp(format, "short"))
 		mode->type = DATE_SHORT;
+	else if (!strcmp(format, "short-local"))
+		mode->type = DATE_SHORT_LOCAL;
 	else if (!strcmp(format, "local"))
 		mode->type = DATE_LOCAL;
 	else if (!strcmp(format, "default"))

@@ -794,6 +808,9 @@ void parse_date_format(const char *format, struct date_mode *mode)
 	else if (skip_prefix(format, "format:", &format)) {
 		mode->type = DATE_STRFTIME;
 		mode->strftime_fmt = xstrdup(format);
+	} else if (skip_prefix(format, "format-local:", &format)) {
+		mode->type = DATE_STRFTIME_LOCAL;
+		mode->strftime_fmt = xstrdup(format);
 	} else
 		die("unknown date format %s", format);
 }

This needs to be CC'ed to the author of a5481a6c (convert "enum date_mode" into a struct, 2015-06-25), which I just did.

I am unhappy with the change to blame.c, and that is not because I do not want "blame" to be touched.

The fact that this change has to touch it indicates that it is easy for other new callers of date formatting code to forget masking DATE_LOCAL bit like this patch does when they want to change their behaviour based on the settings of date-mode. And it would be hard to catch such a subtle breakage during future reviews.

Review

Quoted 6 lines

(quoted diff) +1 -1

 	/* The maximum width used to show the dates */
-	switch (blame_date_mode.type) {
+	switch (blame_date_mode.type & ~DATE_LOCAL) {
 	case DATE_RFC2822:
 		blame_date_width = sizeof("Thu, 19 Oct 2006 16:00:04 -0700");
 		break;

Review parse_done: · line 2605

Quoted 8 lines

(quoted diff) +0 -1

@@ -2600,7 +2600,6 @@ parse_done:
 		   fewer display columns. */
 		blame_date_width = utf8_strwidth(_("4 years, 11 months ago")) + 1; /* add the null */
 		break;
-	case DATE_LOCAL:
 	case DATE_NORMAL:
 		blame_date_width = sizeof("Thu Oct 19 16:00:04 2006 -0700");
 		break;

I agree that among "enum date_mode_type", DATE_LOCAL is an oddball. It should be able to act as an orthogonal and independent flag with at least some, like NORMAL, SHORT, etc. Specifying DATE_LOCAL with some others at the same time, however, would not make much sense, would it? What does it even mean to say time as relative under DATE_LOCAL bit?

Review

Quoted 2 lines

(quoted diff) +0 -0

 	else
 		strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",

What cannot be seen in the post-context of this hunk is that we deliberately drop the timezone information when tweaking the "normal" format to "local". This is done only in the final else clause in show_date() because the code knows that LOCAL is not an independent bit.

I think the original motivation is that there is no need to show the timezone information when the user knows the time is shown in the local timezone---after all, the --date=local option was given by her in the first place. This kind of tweaking should be made consistent with other variants, now the other variants are interacting with this LOCAL bit. If we were to go forward with this patch, I think we probably should remove this special casing of "showing normal date in localzone, drop the zone information", as we cannot sanely drop the TZ offset from the output of some of the formats and stay valid (e.g RFC2822).

Review void parse_date_format(const char *format, struct date_mode *mode) · line 785

Quoted 7 lines

(quoted diff) +3 -0

@@ -777,14 +780,25 @@ void parse_date_format(const char *format, struct date_mode *mode)
 	else if (!strcmp(format, "iso8601") ||
 		 !strcmp(format, "iso"))
 		mode->type = DATE_ISO8601;
+	else if (!strcmp(format, "iso8601-local") ||
+		 !strcmp(format, "iso-local"))
+		mode->type = DATE_ISO8601_LOCAL;

fast-import.c +4 -0

index 6c7c3c9..b19a1b5 100644
@@ -412,6 +412,10 @@ static void write_crash_report(const char *err)
 	struct branch *b;
 	unsigned long lu;
 	struct recent_command *rc;
+	struct date_mode dm;
+
+	dm.type = DATE_NORMAL;
+	dm.local = 1;
 
 	if (!rpt) {
 		error("can't write crash report %s: %s", loc, strerror(errno));

Quoted 8 lines · Junio C Hamano

I am unhappy with the change to blame.c, and that is not because I do not want "blame" to be touched.

The fact that this change has to touch it indicates that it is easy for other new callers of date formatting code to forget masking DATE_LOCAL bit like this patch does when they want to change their behaviour based on the settings of date-mode. And it would be hard to catch such a subtle breakage during future reviews.

Yeah, my first instinct on seeing the bitfield was that it would probably be much simpler to keep the enum pure, and add a bit to the struct. A patch in that direction is below. I think the result is that the using code is much cleaner, and the complexity of converting "foo-local" into the enum/bitfield combination is contained in parse_date_format.

Quoted 6 lines

I agree that among "enum date_mode_type", DATE_LOCAL is an oddball. It should be able to act as an orthogonal and independent flag with at least some, like NORMAL, SHORT, etc. Specifying DATE_LOCAL with some others at the same time, however, would not make much sense, would it? What does it even mean to say time as relative under DATE_LOCAL bit?

I think the "relative local" thing is not _too_ bad, as John's patch enforces it at the user-level (it does not parse "relative-local" at all, and mine similarly complains).

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +0 -0

 	else
 		strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",

What cannot be seen in the post-context of this hunk is that we deliberately drop the timezone information when tweaking the "normal" format to "local". This is done only in the final else clause in show_date() because the code knows that LOCAL is not an independent bit.

I didn't address this in my patch (except to convert the check of DATE_LOCAL to mode->local). I agree that we should handle it in other formats, too, but I think it must be format-dependent. Certainly "rfc" and "iso" must always show the timezone. I'd argue that "raw" probably should, as well. That leaves only "short" and "relative", which do not display the time zone in the first place. So all of the formats are covered, I think.

Quoted 10 lines

I think the original motivation is that there is no need to show the timezone information when the user knows the time is shown in the local timezone---after all, the --date=local option was given by her in the first place. This kind of tweaking should be made consistent with other variants, now the other variants are interacting with this LOCAL bit. If we were to go forward with this patch, I think we probably should remove this special casing of "showing normal date in localzone, drop the zone information", as we cannot sanely drop the TZ offset from the output of some of the formats and stay valid (e.g RFC2822).

I think I'd rather remain inconsistent between the formats (which, after all, do not need to show exactly the same information), then have people complain that "--date=local" is regressed and now shows a timezone.

Quoted 7 lines

Review void parse_date_format(const char *format, struct date_mode *mode) · line 785

Quoted 7 lines

(quoted diff) +3 -0

@@ -777,14 +780,25 @@ void parse_date_format(const char *format, struct date_mode *mode)
 	else if (!strcmp(format, "iso8601") ||
 		 !strcmp(format, "iso"))
 		mode->type = DATE_ISO8601;
+	else if (!strcmp(format, "iso8601-local") ||
+		 !strcmp(format, "iso-local"))
+		mode->type = DATE_ISO8601_LOCAL;

I found the manual "-local" handling here to be a little, well...manual. So in the patch below I've revamped the parsing to look left-to-right for each component: type, local modifier, strftime format.

It ends up being about the same amount of code, but has two advantages:

1. It's more flexible if we want to make more modifiers later. In fact, it would be trivial to implement the current "-strict" as a separate flag if we chose. I don't think there is much point in doing so, but we could do something like "default-local-strict" for showing the local time _with_ the timezone.

2. We can provide more specific error messages (like "relative does not make sense with -local", rather than "unknown date format:
relative-local").

But that is somewhat orthogonal to the enum thing. We could leave the parsing as John has it, and just set mode->local as appropriate in each conditional block.

---
 builtin/blame.c |  1 -
 cache.h         |  2 +-
 date.c          | 77 ++++++++++++++++++++++++++++++++++++++-------------------
 fast-import.c   |  6 ++++-
 4 files changed, 57 insertions(+), 29 deletions(-)

builtin/blame.c +0 -1

index 4db01c1..6fd1a63 100644
@@ -2600,7 +2600,6 @@ parse_done:
 		   fewer display columns. */
 		blame_date_width = utf8_strwidth(_("4 years, 11 months ago")) + 1; /* add the null */
 		break;
-	case DATE_LOCAL:
 	case DATE_NORMAL:
 		blame_date_width = sizeof("Thu Oct 19 16:00:04 2006 -0700");
 		break;

cache.h +1 -1

index 4e25271..9a91b1d 100644
@@ -1091,7 +1091,6 @@ struct date_mode {
 		DATE_NORMAL = 0,
 		DATE_RELATIVE,
 		DATE_SHORT,
-		DATE_LOCAL,
 		DATE_ISO8601,
 		DATE_ISO8601_STRICT,
 		DATE_RFC2822,

@@ -1099,6 +1098,7 @@ struct date_mode {
 		DATE_RAW
 	} type;
 	const char *strftime_fmt;
+	int local;
 };
 
 /*

date.c +51 -26

index 8f91569..aa57cad 100644
@@ -166,6 +166,7 @@ struct date_mode *date_mode_from_type(enum date_mode_type type)
 	if (type == DATE_STRFTIME)
 		die("BUG: cannot create anonymous strftime date_mode struct");
 	mode.type = type;
+	mode.local = 0;
 	return &mode;
 }
 

@@ -189,7 +190,7 @@ const char *show_date(unsigned long time, int tz, const struct date_mode *mode)
 		return timebuf.buf;
 	}
 
-	if (mode->type == DATE_LOCAL)
+	if (mode->local)
 		tz = local_tzoffset(time);
 
 	tm = time_to_tm(time, tz);

@@ -232,7 +233,7 @@ const char *show_date(unsigned long time, int tz, const struct date_mode *mode)
 				tm->tm_mday,
 				tm->tm_hour, tm->tm_min, tm->tm_sec,
 				tm->tm_year + 1900,
-				(mode->type == DATE_LOCAL) ? 0 : ' ',
+				mode->local ? 0 : ' ',
 				tz);
 	return timebuf.buf;
 }

@@ -770,32 +771,56 @@ int parse_date(const char *date, struct strbuf *result)
 	return 0;
 }
 
+static enum date_mode_type parse_date_type(const char *format, const char **end)
+{
+	if (skip_prefix(format, "relative", end))
+		return DATE_RELATIVE;
+	if (skip_prefix(format, "iso8601-strict", end) ||
+	    skip_prefix(format, "iso-strict", end))
+		return DATE_ISO8601_STRICT;
+	if (skip_prefix(format, "iso8601", end) ||
+	    skip_prefix(format, "iso", end))
+		return DATE_ISO8601;
+	if (skip_prefix(format, "rfc2822", end) ||
+	    skip_prefix(format, "rfc", end))
+		return DATE_RFC2822;
+	if (skip_prefix(format, "short", end))
+		return DATE_SHORT;
+	if (skip_prefix(format, "default", end))
+		return DATE_NORMAL;
+	if (skip_prefix(format, "raw", end))
+		return DATE_RAW;
+	if (skip_prefix(format, "format", end))
+		return DATE_STRFTIME;
+
+	die("unknown date format %s", format);
+}
+
 void parse_date_format(const char *format, struct date_mode *mode)
 {
-	if (!strcmp(format, "relative"))
-		mode->type = DATE_RELATIVE;
-	else if (!strcmp(format, "iso8601") ||
-		 !strcmp(format, "iso"))
-		mode->type = DATE_ISO8601;
-	else if (!strcmp(format, "iso8601-strict") ||
-		 !strcmp(format, "iso-strict"))
-		mode->type = DATE_ISO8601_STRICT;
-	else if (!strcmp(format, "rfc2822") ||
-		 !strcmp(format, "rfc"))
-		mode->type = DATE_RFC2822;
-	else if (!strcmp(format, "short"))
-		mode->type = DATE_SHORT;
-	else if (!strcmp(format, "local"))
-		mode->type = DATE_LOCAL;
-	else if (!strcmp(format, "default"))
-		mode->type = DATE_NORMAL;
-	else if (!strcmp(format, "raw"))
-		mode->type = DATE_RAW;
-	else if (skip_prefix(format, "format:", &format)) {
-		mode->type = DATE_STRFTIME;
-		mode->strftime_fmt = xstrdup(format);
-	} else
-		die("unknown date format %s", format);
+	const char *p;
+
+	/* historical alias */
+	if (!strcmp(format, "local"))
+		format = "default-local";
+
+	mode->type = parse_date_type(format, &p);
+	mode->local = 0;
+
+	if (skip_prefix(p, "-local", &p)) {
+		if (mode->type == DATE_RELATIVE)
+			die("relative-local date format is nonsensical");
+		mode->local = 1;
+	}
+
+	if (mode->type == DATE_STRFTIME) {
+		if (!skip_prefix(p, ":", &p))
+			die("date format missing colon separator: %s", format);
+		mode->strftime_fmt = xstrdup(p);
+	}
+
+	if (*p)
+		die("unknown date-mode modifier: %s", p);
 }
 
 void datestamp(struct strbuf *out)

fast-import.c +5 -1

index 6c7c3c9..b19a1b5 100644
@@ -412,6 +412,10 @@ static void write_crash_report(const char *err)
 	struct branch *b;
 	unsigned long lu;
 	struct recent_command *rc;
+	struct date_mode dm;
+
+	dm.type = DATE_NORMAL;
+	dm.local = 1;
 
 	if (!rpt) {
 		error("can't write crash report %s: %s", loc, strerror(errno));

@@ -424,7 +428,7 @@ static void write_crash_report(const char *err)
 	fprintf(rpt, "fast-import crash report:\n");
 	fprintf(rpt, "    fast-import process: %"PRIuMAX"\n", (uintmax_t) getpid());
 	fprintf(rpt, "    parent process     : %"PRIuMAX"\n", (uintmax_t) getppid());
-	fprintf(rpt, "    at %s\n", show_date(time(NULL), 0, DATE_MODE(LOCAL)));
+	fprintf(rpt, "    at %s\n", show_date(time(NULL), 0, &dm));
 	fputc('\n', rpt);
 
 	fputs("fatal: ", rpt);

Signature · 1 line

2.5.1.739.g7891f6b

Are you willing to resend this as a proper patch?

I'm happy to build documentation & tests on top, although there don't seem to be any tests for date formats outside t6300-for-each-ref.sh at the moment

(and the "format" mode is neither tested nor documented for git-for-each-ref although it does work)..

Most of our "--date" modes are about the format of the date:
which items we show and in what order. But "--date=local" is a bit of an oddball. It means "show the date in the normal format, but using the local timezone". The timezone we use is orthogonal to the actual format, and there is no reason we could not have "localized iso8601", etc.

This patch adds a "local" boolean field to "struct date_mode", and drops the DATE_LOCAL element from the date_mode_type enum (it's now just DATE_NORMAL plus local=1). The new feature is accessible to users by adding "-local" to any date mode (e.g., "iso-local"), and we retain "local" as an alias for "default-local" for backwards compatibility.

Signed-off-by Jeff King <[email protected]>

---

I wonder if anybody would be confused and try "iso-local-strict", which does not work with this code. If we bumped "-strict" to become a modifier (like "-local"), we could easily make the order interchangeable.

 Documentation/rev-list-options.txt | 21 ++++++++---
 builtin/blame.c                    |  1 -
 cache.h                            |  2 +-
 date.c                             | 77 +++++++++++++++++++++++++-------------
 4 files changed, 67 insertions(+), 34 deletions(-)

Documentation/rev-list-options.txt +15 -6

index a9b808f..35dc44b 100644
@@ -702,12 +702,16 @@ include::pretty-options.txt[]
 --date=(relative|local|default|iso|iso-strict|rfc|short|raw)::
 	Only takes effect for dates shown in human-readable format, such
 	as when using `--pretty`. `log.date` config variable sets a default
-	value for the log command's `--date` option.
+	value for the log command's `--date` option. By default, dates
+	are shown in the original time zone (either committer's or
+	author's). If `-local` is appended to the format (e.g.,
+	`iso-local`), the user's local time zone is used instead.
 +
 `--date=relative` shows dates relative to the current time,
-e.g. ``2 hours ago''.
+e.g. ``2 hours ago''. The `-local` option cannot be used with
+`--relative`.
 +
-`--date=local` shows timestamps in user's local time zone.
+`--date=local` is an alias for `--date=default-local`.
 +
 `--date=iso` (or `--date=iso8601`) shows timestamps in a ISO 8601-like format.
 The differences to the strict ISO 8601 format are:

@@ -730,10 +734,15 @@ format, often found in email messages.
 `--date=format:...` feeds the format `...` to your system `strftime`.
 Use `--date=format:%c` to show the date in your system locale's
 preferred format.  See the `strftime` manual for a complete list of
-format placeholders.
+format placeholders. When using `-local`, the correct syntax is
+`--date=format-local:...`.
 +
-`--date=default` shows timestamps in the original time zone
-(either committer's or author's).
+`--date=default` is the default format, and is similar to
+`--date=rfc2822`, with a few exceptions:
+
+	- there is no comma after the day-of-week
+
+	- the time zone is omitted when the local time zone is used
 
 ifdef::git-rev-list[]
 --header::

builtin/blame.c +0 -1

index 4db01c1..6fd1a63 100644
@@ -2600,7 +2600,6 @@ parse_done:
 		   fewer display columns. */
 		blame_date_width = utf8_strwidth(_("4 years, 11 months ago")) + 1; /* add the null */
 		break;
-	case DATE_LOCAL:
 	case DATE_NORMAL:
 		blame_date_width = sizeof("Thu Oct 19 16:00:04 2006 -0700");
 		break;

cache.h +1 -1

index 4e25271..9a91b1d 100644
@@ -1091,7 +1091,6 @@ struct date_mode {
 		DATE_NORMAL = 0,
 		DATE_RELATIVE,
 		DATE_SHORT,
-		DATE_LOCAL,
 		DATE_ISO8601,
 		DATE_ISO8601_STRICT,
 		DATE_RFC2822,

@@ -1099,6 +1098,7 @@ struct date_mode {
 		DATE_RAW
 	} type;
 	const char *strftime_fmt;
+	int local;
 };
 
 /*

date.c +51 -26

index 8f91569..aa57cad 100644
@@ -166,6 +166,7 @@ struct date_mode *date_mode_from_type(enum date_mode_type type)
 	if (type == DATE_STRFTIME)
 		die("BUG: cannot create anonymous strftime date_mode struct");
 	mode.type = type;
+	mode.local = 0;
 	return &mode;
 }
 

@@ -189,7 +190,7 @@ const char *show_date(unsigned long time, int tz, const struct date_mode *mode)
 		return timebuf.buf;
 	}
 
-	if (mode->type == DATE_LOCAL)
+	if (mode->local)
 		tz = local_tzoffset(time);
 
 	tm = time_to_tm(time, tz);

@@ -232,7 +233,7 @@ const char *show_date(unsigned long time, int tz, const struct date_mode *mode)
 				tm->tm_mday,
 				tm->tm_hour, tm->tm_min, tm->tm_sec,
 				tm->tm_year + 1900,
-				(mode->type == DATE_LOCAL) ? 0 : ' ',
+				mode->local ? 0 : ' ',
 				tz);
 	return timebuf.buf;
 }

@@ -770,32 +771,56 @@ int parse_date(const char *date, struct strbuf *result)
 	return 0;
 }
 
+static enum date_mode_type parse_date_type(const char *format, const char **end)
+{
+	if (skip_prefix(format, "relative", end))
+		return DATE_RELATIVE;
+	if (skip_prefix(format, "iso8601-strict", end) ||
+	    skip_prefix(format, "iso-strict", end))
+		return DATE_ISO8601_STRICT;
+	if (skip_prefix(format, "iso8601", end) ||
+	    skip_prefix(format, "iso", end))
+		return DATE_ISO8601;
+	if (skip_prefix(format, "rfc2822", end) ||
+	    skip_prefix(format, "rfc", end))
+		return DATE_RFC2822;
+	if (skip_prefix(format, "short", end))
+		return DATE_SHORT;
+	if (skip_prefix(format, "default", end))
+		return DATE_NORMAL;
+	if (skip_prefix(format, "raw", end))
+		return DATE_RAW;
+	if (skip_prefix(format, "format", end))
+		return DATE_STRFTIME;
+
+	die("unknown date format %s", format);
+}
+
 void parse_date_format(const char *format, struct date_mode *mode)
 {
-	if (!strcmp(format, "relative"))
-		mode->type = DATE_RELATIVE;
-	else if (!strcmp(format, "iso8601") ||
-		 !strcmp(format, "iso"))
-		mode->type = DATE_ISO8601;
-	else if (!strcmp(format, "iso8601-strict") ||
-		 !strcmp(format, "iso-strict"))
-		mode->type = DATE_ISO8601_STRICT;
-	else if (!strcmp(format, "rfc2822") ||
-		 !strcmp(format, "rfc"))
-		mode->type = DATE_RFC2822;
-	else if (!strcmp(format, "short"))
-		mode->type = DATE_SHORT;
-	else if (!strcmp(format, "local"))
-		mode->type = DATE_LOCAL;
-	else if (!strcmp(format, "default"))
-		mode->type = DATE_NORMAL;
-	else if (!strcmp(format, "raw"))
-		mode->type = DATE_RAW;
-	else if (skip_prefix(format, "format:", &format)) {
-		mode->type = DATE_STRFTIME;
-		mode->strftime_fmt = xstrdup(format);
-	} else
-		die("unknown date format %s", format);
+	const char *p;
+
+	/* historical alias */
+	if (!strcmp(format, "local"))
+		format = "default-local";
+
+	mode->type = parse_date_type(format, &p);
+	mode->local = 0;
+
+	if (skip_prefix(p, "-local", &p)) {
+		if (mode->type == DATE_RELATIVE)
+			die("relative-local date format is nonsensical");
+		mode->local = 1;
+	}
+
+	if (mode->type == DATE_STRFTIME) {
+		if (!skip_prefix(p, ":", &p))
+			die("date format missing colon separator: %s", format);
+		mode->strftime_fmt = xstrdup(p);
+	}
+
+	if (*p)
+		die("unknown date-mode modifier: %s", p);
 }
 
 void datestamp(struct strbuf *out)

Signature · 1 line

2.5.1.739.g7891f6b

This fails t6300 with:

fatal: unknown date-mode modifier: my date is %Y-%m-%d not ok 83 - Check format of strftime date fields #
# echo "my date is 2006-07-03" >expected &&
# git for-each-ref \
# --format="%(authordate:format:my date is %Y-%m-%d)" \
# refs/heads >actual &&
# test_cmp expected actual
#

The following squash-in fixes it:

date.c +0 -0

index aa57cad..3aa8002 100644

date.c void parse_date_format(const char *format, struct date_mode *mode) · line 821

Quoted 13 lines

date.c +1 -3

index aa57cad..3aa8002 100644
@@ -817,9 +817,7 @@ void parse_date_format(const char *format, struct date_mode *mode)
 		if (!skip_prefix(p, ":", &p))
 			die("date format missing colon separator: %s", format);
 		mode->strftime_fmt = xstrdup(p);
-	}
-
-	if (*p)
+	} else if (*p)
 		die("unknown date-mode modifier: %s", p);

Yeah, that works. We could also advance "p" in the DATE_STRFTIME conditional, but I think your solution is less ugly.

Thanks for debugging my mess.

Quoted 17 lines · Jeff King

date.c void parse_date_format(const char *format, struct date_mode *mode) · line 821

Quoted 13 lines

date.c +1 -3

index aa57cad..3aa8002 100644
@@ -817,9 +817,7 @@ void parse_date_format(const char *format, struct date_mode *mode)
 		if (!skip_prefix(p, ":", &p))
 			die("date format missing colon separator: %s", format);
 		mode->strftime_fmt = xstrdup(p);
-	}
-
-	if (*p)
+	} else if (*p)
 		die("unknown date-mode modifier: %s", p);

Yeah, that works. We could also advance "p" in the DATE_STRFTIME conditional, but I think your solution is less ugly.

Quoted 1 line

Thanks for debugging my mess.

By the way, I was imagining you would pick these up and add to them with more tests and documentation. If that's the case, please feel free to squash that in and keep my signoff. If not, then I can post a re-roll after waiting for other comments.

Jeff's first patch is unmodified but I've squashed the fixed currently on "pu" into the second. I also realised while adding the tests that "raw-local" is meaningless so I've modified the code to reject it in the same way as "relative-local".

Jeff King (2):
fast-import: switch crash-report date to iso8601
date: make "local" orthogonal to date format

John Keeping (4):
t6300: introduce test_date() helper
t6300: make UTC and local dates different
t6300: add test for "raw" date format
t6300: add tests for "-local" date formats

[1] I do think the error message for "relative-local is nonsense" could perhaps be more explanatory, but I couldn't come up with any better wording. But if you have ideas, feel free to switch it.

Quoted 3 lines · Jeff King

[1] I do think the error message for "relative-local is nonsense" could perhaps be more explanatory, but I couldn't come up with any better wording. But if you have ideas, feel free to switch it.

My only suggestion would be to reuse the "unknown date format: %s" message and avoid having a special message in this case.

Quoted 7 lines · John Keeping

Quoted 3 lines · Jeff King

[1] I do think the error message for "relative-local is nonsense" could perhaps be more explanatory, but I couldn't come up with any better wording. But if you have ideas, feel free to switch it.

My only suggestion would be to reuse the "unknown date format: %s" message and avoid having a special message in this case.

Heh, that was what I was trying to avoid. I wanted to avoid "I do not understand our request" and have it more like "I understand what you're _trying_ to do, but it doesn't make sense".

I guess "relative dates do not depend on timezones, so -local is meaningless" would be the closest thing.

I don't think it is that big a deal whichever way we go, though.

Quoted 10 lines · Jeff King

Quoted 7 lines · John Keeping

Quoted 3 lines · Jeff King

[1] I do think the error message for "relative-local is nonsense" could perhaps be more explanatory, but I couldn't come up with any better wording. But if you have ideas, feel free to switch it.

My only suggestion would be to reuse the "unknown date format: %s" message and avoid having a special message in this case.

Quoted 8 lines

Heh, that was what I was trying to avoid. I wanted to avoid "I do not understand our request" and have it more like "I understand what you're _trying_ to do, but it doesn't make sense".

I guess "relative dates do not depend on timezones, so -local is meaningless" would be the closest thing.

I don't think it is that big a deal whichever way we go, though.

I somehow thought that the discussion was about raw-local, not relative-local, but anyway, I think it would make more sense to allow both of them. If you define the meaning of "-local" as:

Pretend that the event in question was recorded with your timezone, and show the timestamp using the specified format sans -local suffix.

that explains what happens for all the other formats well, and it also makes sense for what would happen to raw and even relative, I think.

Quoted 4 lines · Junio C Hamano

Quoted 2 lines · Jeff King

I guess "relative dates do not depend on timezones, so -local is meaningless" would be the closest thing.

The discussion about "raw-local" was in a separate subthread, I think we're just bikeshedding the particular error message here.

OTOH, I don't think there's any disagreement about what "relative-local" and "raw-local" would output were they supported, just whether they are useful. There doesn't seem to be any harm in supporting them; "relative-local" will be identical to "relative" and "raw-local" will require preparatory code movement for the raw output.

Quoted 5 lines · John Keeping

OTOH, I don't think there's any disagreement about what "relative-local" and "raw-local" would output were they supported, just whether they are useful. There doesn't seem to be any harm in supporting them; "relative-local" will be identical to "relative" and "raw-local" will require preparatory code movement for the raw output.

Sure.

Bikeshedding further, while Peff's message "-local is meaningless" is a correct statement of the fact, I do not think it explains well why we chose to error out instead of giving the most natural result (i.e. exactly the same as 'relative').

Perhaps stating "relative-local is not supported" without saying why would be better. "Because it is meaningless, we refuse to support the option." is a very strong statement that tells aspiring future Git hackers not to attempt to add a support for it, which is probably a wrong message to send.

In which case, should we just support it now?

Normally I'd suggest banning controversial options on the basis that it's easier in the future to allow something that was previously banned than change the meaning of an options, but in this case I can't see any other meaning for these options than that described above.

Quoted 2 lines · John Keeping

In which case, should we just support it now?

Quoted 4 lines

Normally I'd suggest banning controversial options on the basis that it's easier in the future to allow something that was previously banned than change the meaning of an options, but in this case I can't see any other meaning for these options than that described above.

My usual stance is the same as yours, and I agree that there is no other sane and useful behaviour for "relative-local" to do the same as "relative".

Documentation/rev-list-options.txt +3 -1

index a9b808f..5d28133 100644
@@ -702,12 +702,16 @@ include::pretty-options.txt[]
 --date=(relative|local|default|iso|iso-strict|rfc|short|raw)::
 	Only takes effect for dates shown in human-readable format, such
 	as when using `--pretty`. `log.date` config variable sets a default
-	value for the log command's `--date` option.
+	value for the log command's `--date` option. By default, dates
+	are shown in the original time zone (either committer's or
+	author's). If `-local` is appended to the format (e.g.,

This is Jeff's original patch with my fixup for DATE_STRFTIME squashed in and a new change to reject "raw-local" (in both Documentation/ and date.c).

Quoted 3 lines · John Keeping

This is Jeff's original patch with my fixup for DATE_STRFTIME squashed in and a new change to reject "raw-local" (in both Documentation/ and date.c).

Even in --date=raw, we do show the timezone offset, so I do not necessarily agree that raw-local is nonsensical. That's the only difference between the one I queued yesterday and this one.

Quoted 3 lines · John Keeping

This is Jeff's original patch with my fixup for DATE_STRFTIME squashed in and a new change to reject "raw-local" (in both Documentation/ and date.c).

Even in --date=raw, we do show the timezone offset, so I do not necessarily agree that raw-local is nonsensical. That's the only difference between the one I queued yesterday and this one.

Quoted 3 lines

Even in --date=raw, we do show the timezone offset, so I do not necessarily agree that raw-local is nonsensical. That's the only difference between the one I queued yesterday and this one.

I suspect it depends on the interpretation of "raw"; the code currently interprets raw to mean "exactly what exists in the commit/tag", in which case converting it to the local timezone is wrong. But the documentation describes "raw" as "the raw Git %s %z format", and if we interpret it to mean "Git's internal date format" then "raw-local" makes sense.

The alternative would be the patch below as a preparatory step.

Quoted 4 lines · John Keeping

Quoted 3 lines

Even in --date=raw, we do show the timezone offset, so I do not necessarily agree that raw-local is nonsensical. That's the only difference between the one I queued yesterday and this one.

Quoted 8 lines

I suspect it depends on the interpretation of "raw"; the code currently interprets raw to mean "exactly what exists in the commit/tag", in which case converting it to the local timezone is wrong. But the documentation describes "raw" as "the raw Git %s %z format", and if we interpret it to mean "Git's internal date format" then "raw-local" makes sense.

The alternative would be the patch below as a preparatory step.

Ah, right, I forgot that we need to refactor show_date() to actually do the right thing.

I think I'd be in favor of just disallowing "raw-local", then.

-Peff

I considered moving the test_expect_success into the helper, like with test_atom earlier in the file, but it doesn't make the code much more concise and we still need either to setup the output outside the test case or to escape SQ inside SQ.

Quoted 4 lines · John Keeping

I considered moving the test_expect_success into the helper, like with test_atom earlier in the file, but it doesn't make the code much more concise and we still need either to setup the output outside the test case or to escape SQ inside SQ.

Moving it inside also makes it harder to test_expect_failure. :)

t/t6300-for-each-ref.sh test_expect_success 'Check invalid format specifiers are errors' ' · line 166

Quoted 5 lines

test_expect_success 'Check unformatted date fields output' '

t/t6300-for-each-ref.sh +1 -3

-	(git for-each-ref --shell --format="%(refname) %(committerdate) %(authordate)" refs/heads &&
-	git for-each-ref --shell --format="%(refname) %(taggerdate)" refs/tags) >actual &&
-	test_cmp expected actual
+	test_date "" "Mon Jul 3 17:18:43 2006 +0200" "Mon Jul 3 17:18:44 2006 +0200" "Mon Jul 3 17:18:45 2006 +0200"

I notice we end up with rather long lines for some of these. Would:

test_date "" <<-\EOF
Mon Jul 3 17:18:43 2006 +0200
Mon Jul 3 17:18:44 2006 +0200
Mon Jul 3 17:18:45 2006 +0200
EOF

be more readable?

Quoted 9 lines

I notice we end up with rather long lines for some of these. Would:

test_date "" <<-\EOF
Mon Jul 3 17:18:43 2006 +0200
Mon Jul 3 17:18:44 2006 +0200
Mon Jul 3 17:18:45 2006 +0200
EOF

be more readable?

We could just do:

	test_date "" \
		"Tue Jul 4 01:18:43 2006 +0200" \
		"Tue Jul 4 01:18:44 2006 +0200" \
		"Tue Jul 4 01:18:45 2006 +0200"

I'm not entirely sure why I didn't now that I look at it!

This moves the setup of the "expected" file inside the test case. The helper function has the advantage that we can use SQ in the file content without needing to escape the quotes.

Signed-off-by John Keeping <[email protected]>

---

t/t6300-for-each-ref.sh +0 -0

index 7c9bec7..5fdb964 100755

t/t6300-for-each-ref.sh +13 -0

+       committer_date=$2 &&
+       author_date=$3 &&
+       tagger_date=$4 &&
+       cat >expected <<-EOF &&
+       'refs/heads/master' '$committer_date' '$author_date'
+       'refs/tags/testtag' '$tagger_date'
+       EOF
+       (
+               git for-each-ref --shell --format="%(refname) %(committerdate${f:+:$f}) %(authordate${f:+:$f})" refs/heads &&
+               git for-each-ref --shell --format="%(refname) %(taggerdate${f:+:$f})" refs/tags
+       ) >actual &&
+       test_cmp expected actual
+}

We are about to add several new date formats which will make this list too long to display in a single line.

Signed-off-by John Keeping <[email protected]>

---
 Documentation/git-rev-list.txt     | 2 +-
 Documentation/rev-list-options.txt | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Documentation/git-rev-list.txt +1 -1

index 7b49c85..ef22f17 100644
@@ -45,7 +45,7 @@ SYNOPSIS
 	     [ --regexp-ignore-case | -i ]
 	     [ --extended-regexp | -E ]
 	     [ --fixed-strings | -F ]
-	     [ --date=(local|relative|default|iso|iso-strict|rfc|short) ]
+	     [ --date=<format>]
 	     [ [ --objects | --objects-edge | --objects-edge-aggressive ]
 	       [ --unpacked ] ]
 	     [ --pretty | --header ]

Documentation/rev-list-options.txt +1 -1

index a9b808f..14c4cce 100644
@@ -699,7 +699,7 @@ include::pretty-options.txt[]
 --relative-date::
 	Synonym for `--date=relative`.
 
---date=(relative|local|default|iso|iso-strict|rfc|short|raw)::
+--date=<format>::
 	Only takes effect for dates shown in human-readable format, such
 	as when using `--pretty`. `log.date` config variable sets a default
 	value for the log command's `--date` option.

In a following commit we will make "local" orthogonal to the format. Although this will not apply to "relative", which does not use the timezone, it applies to all other formats so move the timezone conversion to the start of the function.

Signed-off-by John Keeping <[email protected]>

---
 date.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

date.c +3 -3

index 8f91569..9f0a5dd 100644
@@ -174,6 +174,9 @@ const char *show_date(unsigned long time, int tz, const struct date_mode *mode)
 	struct tm *tm;
 	static struct strbuf timebuf = STRBUF_INIT;
 
+	if (mode->type == DATE_LOCAL)
+		tz = local_tzoffset(time);
+
 	if (mode->type == DATE_RAW) {
 		strbuf_reset(&timebuf);
 		strbuf_addf(&timebuf, "%lu %+05d", time, tz);

@@ -189,9 +192,6 @@ const char *show_date(unsigned long time, int tz, const struct date_mode *mode)
 		return timebuf.buf;
 	}
 
-	if (mode->type == DATE_LOCAL)
-		tz = local_tzoffset(time);
-
 	tm = time_to_tm(time, tz);
 	if (!tm) {
 		tm = time_to_tm(0, 0);

Since version 2 there are four new preparatory patches which remove lists of date formats from documentation in favour of referring to the detailed list in git-rev-list(1) or git-log(1) (both generated from Documentation/rev-list-options.txt) depending on whether the page in question is plumbing/porcelain.

In patch 7 (date: check for "local" before anything else), we no longer reject "relative-local" and "raw-local" now prints the user's local timezone offset. The error message for invalid formats that are prefixed with a valid format name is now the same as that if there is no valid prefix.

date.c int parse_date(const char *date, struct strbuf *result) · line 814

Quoted 12 lines · Jeff King

date.c +12 -0

+	/* historical alias */
+	if (!strcmp(format, "local"))
+		format = "default-local";
+
+	mode->type = parse_date_type(format, &p);
+	mode->local = 0;
+
+	if (skip_prefix(p, "-local", &p)) {
+		if (mode->type == DATE_RELATIVE)
+			die("relative-local date format is nonsensical");
+		mode->local = 1;
+	}

I notice that we give something funny like this:

    $ git show --date=short-locale
    fatal: unknown date-mode modifier: e

What is the intention here? In other words, what kind of things can plausibly follow "--date=short-local" in enhanced versions of Git in the future? "--date=short-local:some other magic"?

Quoted 17 lines · Junio C Hamano

date.c int parse_date(const char *date, struct strbuf *result) · line 814

Quoted 12 lines · Jeff King

date.c +12 -0

+	/* historical alias */
+	if (!strcmp(format, "local"))
+		format = "default-local";
+
+	mode->type = parse_date_type(format, &p);
+	mode->local = 0;
+
+	if (skip_prefix(p, "-local", &p)) {
+		if (mode->type == DATE_RELATIVE)
+			die("relative-local date format is nonsensical");
+		mode->local = 1;
+	}

I notice that we give something funny like this:

Quoted 2 lines

    $ git show --date=short-locale
    fatal: unknown date-mode modifier: e

Yeah, that's not ideal.

Quoted 3 lines

What is the intention here? In other words, what kind of things can plausibly follow "--date=short-local" in enhanced versions of Git in the future? "--date=short-local:some other magic"?

I had assumed it would be "short-local-othermagic", since ":" is already the separator for "format:". But I admit I have no idea what other modifiers would be interesting.

I think the error message would be a lot nicer if we indicate that "-" is syntactically interesting, and say:

fatal: unknown date-mode modifier: locale

Quoted 4 lines · Jeff King

I think the error message would be a lot nicer if we indicate that "-" is syntactically interesting, and say:

fatal: unknown date-mode modifier: locale

I wonder if we'd be better just saying:

fatal: unknown date format: short-locale

I'm not sure users will consider "local" to be a modifier, there is simply a list of formats that happens to include pairs of matching "-local" and "not -local" variants.

Yeah, my first instinct on seeing the bitfield was that it would probably be much simpler to keep the enum pure, and add a bit to the struct. A patch in that direction is below. ...