[RFC/PATCH] date: allow any format to display local time

[RFC/PATCH] date: allow any format to display local time

[John Keeping]

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 <john@keeping.me.uk>
---
I primarily want this to make life easier in CGit (so that we can reuse
libgit.a for formatting dates in the originator's timezone), but I think
it makes sense to expose these options to the user in general.

 builtin/blame.c |  3 +--
 cache.h         |  9 +++++++--
 date.c          | 31 ++++++++++++++++++++++++-------
 3 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 4db01c1..dff6934 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -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;
diff --git a/cache.h b/cache.h
index 4e25271..cda5c51 100644
--- a/cache.h
+++ b/cache.h
@@ -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_RAW,
+		DATE_LOCAL = 0x80,
+		DATE_SHORT_LOCAL = (DATE_SHORT | DATE_LOCAL),
+		DATE_ISO8601_LOCAL = (DATE_ISO8601 | DATE_LOCAL),
+		DATE_ISO8601_STRICT_LOCAL = (DATE_ISO8601_STRICT | DATE_LOCAL),
+		DATE_RFC2822_LOCAL = (DATE_RFC2822 | DATE_LOCAL),
+		DATE_STRFTIME_LOCAL = (DATE_STRFTIME | DATE_LOCAL),
 	} type;
 	const char *strftime_fmt;
 };
diff --git a/date.c b/date.c
index 8f91569..e0e0f3b 100644
--- a/date.c
+++ b/date.c
@@ -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)
+	} else if (type == DATE_RFC2822)
 		strbuf_addf(&timebuf, "%.3s, %d %.3s %d %02d:%02d:%02d %+05d",
 			weekday_names[tm->tm_wday], tm->tm_mday,
 			month_names[tm->tm_mon], tm->tm_year + 1900,
 			tm->tm_hour, tm->tm_min, tm->tm_sec, tz);
-	else if (mode->type == DATE_STRFTIME)
+	else if (type == DATE_STRFTIME)
 		strbuf_addftime(&timebuf, mode->strftime_fmt, tm);
 	else
 		strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
@@ -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);
 }
-- 
2.5.0.466.g9af26fa

Re: [RFC/PATCH] date: allow any format to display local time

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> 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 <john@keeping.me.uk>
> ---

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.

> diff --git a/builtin/blame.c b/builtin/blame.c
> index 4db01c1..dff6934 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -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;

> diff --git a/cache.h b/cache.h
> index 4e25271..cda5c51 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -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_RAW,
> +		DATE_LOCAL = 0x80,
> +		DATE_SHORT_LOCAL = (DATE_SHORT | DATE_LOCAL),
> +		DATE_ISO8601_LOCAL = (DATE_ISO8601 | DATE_LOCAL),
> +		DATE_ISO8601_STRICT_LOCAL = (DATE_ISO8601_STRICT | DATE_LOCAL),
> +		DATE_RFC2822_LOCAL = (DATE_RFC2822 | DATE_LOCAL),
> +		DATE_STRFTIME_LOCAL = (DATE_STRFTIME | DATE_LOCAL),
>  	} type;
>  	const char *strftime_fmt;
>  };

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?

But this point is minor.  A bigger design issue is in show_date()
below.

> diff --git a/date.c b/date.c
> index 8f91569..e0e0f3b 100644
> --- a/date.c
> +++ b/date.c
> @@ -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)
> +	} else if (type == DATE_RFC2822)
>  		strbuf_addf(&timebuf, "%.3s, %d %.3s %d %02d:%02d:%02d %+05d",
>  			weekday_names[tm->tm_wday], tm->tm_mday,
>  			month_names[tm->tm_mon], tm->tm_year + 1900,
>  			tm->tm_hour, tm->tm_min, tm->tm_sec, tz);
> -	else if (mode->type == DATE_STRFTIME)
> +	else if (type == DATE_STRFTIME)
>  		strbuf_addftime(&timebuf, mode->strftime_fmt, tm);
>  	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).

> @@ -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);
>  }

Re: [RFC/PATCH] date: allow any format to display local time

[Jeff King]

On Mon, Aug 31, 2015 at 10:28:24AM -0700, Junio C Hamano wrote:

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

Thanks. I'm definitely interested.

> 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. So for callers like blame:

> >  	/* 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;

We can get rid of this hunk, which is good.

> > @@ -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;

This hunk stays, because there is no more DATE_LOCAL. We could keep it
for historical compatibility, but the compiler will let us know if new
callers arrive.

There is one call-site in fast-import which uses DATE_MODE(LOCAL). In my
patch I convert it use a real "struct date_mode", as it is not much more
code, and is arguably more clear.

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

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

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

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

diff --git a/builtin/blame.c b/builtin/blame.c
index 4db01c1..6fd1a63 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -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;
diff --git a/cache.h b/cache.h
index 4e25271..9a91b1d 100644
--- a/cache.h
+++ b/cache.h
@@ -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;
 };
 
 /*
diff --git a/date.c b/date.c
index 8f91569..aa57cad 100644
--- a/date.c
+++ b/date.c
@@ -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)
diff --git a/fast-import.c b/fast-import.c
index 6c7c3c9..b19a1b5 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -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);
-- 
2.5.1.739.g7891f6b

Re: [RFC/PATCH] date: allow any format to display local time

[Jeff King]

On Mon, Aug 31, 2015 at 02:50:18PM -0400, Jeff King wrote:

> diff --git a/fast-import.c b/fast-import.c
> index 6c7c3c9..b19a1b5 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -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);

I also considered just switching this hunk to another format. I doubt
that anybody cares what format the fast-import crash-report timestamp is
in. And in fact LOCAL is kind of a bad choice. Using the user's timezone
is fine, but _omitting_ the timezone in a crash report seems like a
recipe for confusion. Probably iso8601-local would be saner.

-Peff

Re: [RFC/PATCH] date: allow any format to display local time

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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 found it a much cleaner approach from my cursory look.  John, what
do you think?

Re: [RFC/PATCH] date: allow any format to display local time

[John Keeping]

On Mon, Aug 31, 2015 at 02:50:18PM -0400, Jeff King wrote:
> On Mon, Aug 31, 2015 at 10:28:24AM -0700, Junio C Hamano wrote:
> > 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.

Yes, the result below is much cleaner than my version.

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

I like that the parsing indicates that the format and "local-ness" are
orthogonal in this version.

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

> ---
>  builtin/blame.c |  1 -
>  cache.h         |  2 +-
>  date.c          | 77 ++++++++++++++++++++++++++++++++++++++-------------------
>  fast-import.c   |  6 ++++-
>  4 files changed, 57 insertions(+), 29 deletions(-)
> 
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 4db01c1..6fd1a63 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -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;
> diff --git a/cache.h b/cache.h
> index 4e25271..9a91b1d 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -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;
>  };
>  
>  /*
> diff --git a/date.c b/date.c
> index 8f91569..aa57cad 100644
> --- a/date.c
> +++ b/date.c
> @@ -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)
> diff --git a/fast-import.c b/fast-import.c
> index 6c7c3c9..b19a1b5 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -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);
> -- 
> 2.5.1.739.g7891f6b

Re: [RFC/PATCH] date: allow any format to display local time

[Jeff King]

On Mon, Aug 31, 2015 at 09:00:16PM +0100, John Keeping wrote:

> Are you willing to resend this as a proper patch?

Yup.

  [1/2]: fast-import: switch crash-report date to iso8601
  [2/2]: date: make "local" orthogonal to date format

> 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

I've added documentation, but not tests. Tests on top would be very much
appreciated.

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

I think it is the opposite for testing: we test only for-each-ref.  I
remember when I did the original patch finding that there were no
existing tests for log --date, and I was too lazy to add them. It would
be nice to add some now.

I agree that the documentation for for-each-ref is not up to date. I
think it should do a better job of just pointing to the existing
date-format (and then we do not have to keep documenting new features in
two places).

We could also push the content into a new file and include it from both
places. But I'd be much more in favor of actually pulling the date
discussion into a separate "gitdates(7)" manpage and just referencing it
from both places.  Even though it creates more work for a user following
the reference (in manpages, at least, which are not hyperlinked), I
think there is a cognitive benefit to the user in realizing that the
same concept can be applied in both places.

-Peff

[PATCH 1/2] fast-import: switch crash-report date to iso8601

[Jeff King]

When fast-import emits a crash report, it does so in the
user's local timezone. But because we omit the timezone
completely for DATE_LOCAL, a reader of the report does not
immediately know which time zone was used. Let's switch this
to ISO8601 instead, which includes the time zone.

This does mean we will show the time in UTC, but that's not
a big deal. A crash report like this will either be looked
at immediately (in which case nobody even looks at the
timestamp), or it will be passed along to a developer to
debug, in which case the original timezone is less likely to
be of interest.

Signed-off-by: Jeff King <peff@peff.net>
---
After patch 2/2, we could make this iso8601-local. But frankly, I have a
hard time believing anybody really cares about this format.

 fast-import.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fast-import.c b/fast-import.c
index 6c7c3c9..adcbfc6 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -424,7 +424,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, DATE_MODE(ISO8601)));
 	fputc('\n', rpt);
 
 	fputs("fatal: ", rpt);
-- 
2.5.1.739.g7891f6b

[PATCH 2/2] date: make "local" orthogonal to date format

[Jeff King]

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 <peff@peff.net>
---
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(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index a9b808f..35dc44b 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -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::
diff --git a/builtin/blame.c b/builtin/blame.c
index 4db01c1..6fd1a63 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -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;
diff --git a/cache.h b/cache.h
index 4e25271..9a91b1d 100644
--- a/cache.h
+++ b/cache.h
@@ -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;
 };
 
 /*
diff --git a/date.c b/date.c
index 8f91569..aa57cad 100644
--- a/date.c
+++ b/date.c
@@ -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)
-- 
2.5.1.739.g7891f6b

Re: [PATCH 2/2] date: make "local" orthogonal to date format

[John Keeping]

On Mon, Aug 31, 2015 at 04:48:32PM -0400, Jeff King wrote:
> 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 <peff@peff.net>
> ---

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:

diff --git a/date.c b/date.c
index aa57cad..3aa8002 100644
--- a/date.c
+++ b/date.c
@@ -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);
 }
 

> 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(-)
> 
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index a9b808f..35dc44b 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -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::
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 4db01c1..6fd1a63 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -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;
> diff --git a/cache.h b/cache.h
> index 4e25271..9a91b1d 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -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;
>  };
>  
>  /*
> diff --git a/date.c b/date.c
> index 8f91569..aa57cad 100644
> --- a/date.c
> +++ b/date.c
> @@ -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)
> -- 
> 2.5.1.739.g7891f6b

Re: [PATCH 2/2] date: make "local" orthogonal to date format

[Jeff King]

On Mon, Aug 31, 2015 at 10:27:54PM +0100, John Keeping wrote:

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

Whoops. I obviously did not actually run the test suite. I think when
writing the commit message, my brain said "we are not changing anything,
so no need to run the tests", forgetting that I did not run them the
first time when posting an untested "eh, how about this" patch.

> The following squash-in fixes it:
> 
> diff --git a/date.c b/date.c
> index aa57cad..3aa8002 100644
> --- a/date.c
> +++ b/date.c
> @@ -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.

-Peff

Re: [PATCH 2/2] date: make "local" orthogonal to date format

[Jeff King]

On Mon, Aug 31, 2015 at 05:33:37PM -0400, Jeff King wrote:

> > diff --git a/date.c b/date.c
> > index aa57cad..3aa8002 100644
> > --- a/date.c
> > +++ b/date.c
> > @@ -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.

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.

-Peff

Re: [PATCH 2/2] date: make "local" orthogonal to date format

[John Keeping]

On Mon, Aug 31, 2015 at 06:05:09PM -0400, Jeff King wrote:
> On Mon, Aug 31, 2015 at 05:33:37PM -0400, Jeff King wrote:
> 
> > > diff --git a/date.c b/date.c
> > > index aa57cad..3aa8002 100644
> > > --- a/date.c
> > > +++ b/date.c
> > > @@ -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.
> 
> 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.

OK, I'll send them with some additions to t6300 built on top, although
it may take a couple of days.

The other documentation improvements feel like an independent topic that
isn't necessary for this series to graduate; I'd prefer not to block
this waiting for those changes.

[PATCH v2 0/6] Make "local" orthogonal to date format

[John Keeping]

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

 Documentation/rev-list-options.txt |  21 ++++--
 builtin/blame.c                    |   1 -
 cache.h                            |   2 +-
 date.c                             |  77 +++++++++++++-------
 fast-import.c                      |   2 +-
 t/t6300-for-each-ref.sh            | 139 +++++++++++++++++++------------------
 6 files changed, 140 insertions(+), 102 deletions(-)

-- 
2.5.0.466.g9af26fa

[PATCH v2 1/6] fast-import: switch crash-report date to iso8601

[John Keeping]

From: Jeff King <peff@peff.net>

When fast-import emits a crash report, it does so in the
user's local timezone. But because we omit the timezone
completely for DATE_LOCAL, a reader of the report does not
immediately know which time zone was used. Let's switch this
to ISO8601 instead, which includes the time zone.

This does mean we will show the time in UTC, but that's not
a big deal. A crash report like this will either be looked
at immediately (in which case nobody even looks at the
timestamp), or it will be passed along to a developer to
debug, in which case the original timezone is less likely to
be of interest.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: John Keeping <john@keeping.me.uk>
---
 fast-import.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fast-import.c b/fast-import.c
index 6c7c3c9..adcbfc6 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -424,7 +424,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, DATE_MODE(ISO8601)));
 	fputc('\n', rpt);
 
 	fputs("fatal: ", rpt);
-- 
2.5.0.466.g9af26fa

[PATCH v2 2/6] date: make "local" orthogonal to date format

[John Keeping]

From: Jeff King <peff@peff.net>

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 <peff@peff.net>
Signed-off-by: John Keeping <john@keeping.me.uk>
---
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).

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

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index a9b808f..5d28133 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -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
+`--raw` or `--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::
diff --git a/builtin/blame.c b/builtin/blame.c
index 4db01c1..6fd1a63 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -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;
diff --git a/cache.h b/cache.h
index 4e25271..9a91b1d 100644
--- a/cache.h
+++ b/cache.h
@@ -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;
 };
 
 /*
diff --git a/date.c b/date.c
index 8f91569..f048416 100644
--- a/date.c
+++ b/date.c
@@ -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");
+		if (mode->type == DATE_RAW)
+			die("raw-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);
+	} else if (*p)
+		die("unknown date-mode modifier: %s", p);
 }
 
 void datestamp(struct strbuf *out)
-- 
2.5.0.466.g9af26fa

[PATCH v2 3/6] t6300: introduce test_date() helper

[John Keeping]

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 <john@keeping.me.uk>
---
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.

 t/t6300-for-each-ref.sh | 73 ++++++++++++++-----------------------------------
 1 file changed, 21 insertions(+), 52 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 7c9bec7..5fdb964 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -146,85 +146,54 @@ test_expect_success 'Check invalid format specifiers are errors' '
 	test_must_fail git for-each-ref --format="%(authordate:INVALID)" refs/heads
 '
 
-cat >expected <<\EOF
-'refs/heads/master' 'Mon Jul 3 17:18:43 2006 +0200' 'Mon Jul 3 17:18:44 2006 +0200'
-'refs/tags/testtag' 'Mon Jul 3 17:18:45 2006 +0200'
-EOF
+test_date () {
+	f=$1
+	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
+}
 
 test_expect_success 'Check unformatted date fields output' '
-	(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"
 '
 
 test_expect_success 'Check format "default" formatted date fields output' '
-	f=default &&
-	(git for-each-ref --shell --format="%(refname) %(committerdate:$f) %(authordate:$f)" refs/heads &&
-	git for-each-ref --shell --format="%(refname) %(taggerdate:$f)" refs/tags) >actual &&
-	test_cmp expected actual
+	test_date default "Mon Jul 3 17:18:43 2006 +0200" "Mon Jul 3 17:18:44 2006 +0200" "Mon Jul 3 17:18:45 2006 +0200"
 '
 
 # Don't know how to do relative check because I can't know when this script
 # is going to be run and can't fake the current time to git, and hence can't
 # provide expected output.  Instead, I'll just make sure that "relative"
 # doesn't exit in error
-#
-#cat >expected <<\EOF
-#
-#EOF
-#
 test_expect_success 'Check format "relative" date fields output' '
 	f=relative &&
 	(git for-each-ref --shell --format="%(refname) %(committerdate:$f) %(authordate:$f)" refs/heads &&
 	git for-each-ref --shell --format="%(refname) %(taggerdate:$f)" refs/tags) >actual
 '
 
-cat >expected <<\EOF
-'refs/heads/master' '2006-07-03' '2006-07-03'
-'refs/tags/testtag' '2006-07-03'
-EOF
-
 test_expect_success 'Check format "short" date fields output' '
-	f=short &&
-	(git for-each-ref --shell --format="%(refname) %(committerdate:$f) %(authordate:$f)" refs/heads &&
-	git for-each-ref --shell --format="%(refname) %(taggerdate:$f)" refs/tags) >actual &&
-	test_cmp expected actual
+	test_date short 2006-07-03 2006-07-03 2006-07-03
 '
 
-cat >expected <<\EOF
-'refs/heads/master' 'Mon Jul 3 15:18:43 2006' 'Mon Jul 3 15:18:44 2006'
-'refs/tags/testtag' 'Mon Jul 3 15:18:45 2006'
-EOF
-
 test_expect_success 'Check format "local" date fields output' '
-	f=local &&
-	(git for-each-ref --shell --format="%(refname) %(committerdate:$f) %(authordate:$f)" refs/heads &&
-	git for-each-ref --shell --format="%(refname) %(taggerdate:$f)" refs/tags) >actual &&
-	test_cmp expected actual
+	test_date local "Mon Jul 3 15:18:43 2006" "Mon Jul 3 15:18:44 2006" "Mon Jul 3 15:18:45 2006"
 '
 
-cat >expected <<\EOF
-'refs/heads/master' '2006-07-03 17:18:43 +0200' '2006-07-03 17:18:44 +0200'
-'refs/tags/testtag' '2006-07-03 17:18:45 +0200'
-EOF
-
 test_expect_success 'Check format "iso8601" date fields output' '
-	f=iso8601 &&
-	(git for-each-ref --shell --format="%(refname) %(committerdate:$f) %(authordate:$f)" refs/heads &&
-	git for-each-ref --shell --format="%(refname) %(taggerdate:$f)" refs/tags) >actual &&
-	test_cmp expected actual
+	test_date iso8601 "2006-07-03 17:18:43 +0200" "2006-07-03 17:18:44 +0200" "2006-07-03 17:18:45 +0200"
 '
 
-cat >expected <<\EOF
-'refs/heads/master' 'Mon, 3 Jul 2006 17:18:43 +0200' 'Mon, 3 Jul 2006 17:18:44 +0200'
-'refs/tags/testtag' 'Mon, 3 Jul 2006 17:18:45 +0200'
-EOF
-
 test_expect_success 'Check format "rfc2822" date fields output' '
-	f=rfc2822 &&
-	(git for-each-ref --shell --format="%(refname) %(committerdate:$f) %(authordate:$f)" refs/heads &&
-	git for-each-ref --shell --format="%(refname) %(taggerdate:$f)" refs/tags) >actual &&
-	test_cmp expected actual
+	test_date rfc2822 "Mon, 3 Jul 2006 17:18:43 +0200" "Mon, 3 Jul 2006 17:18:44 +0200" "Mon, 3 Jul 2006 17:18:45 +0200"
 '
 
 test_expect_success 'Check format of strftime date fields' '
-- 
2.5.0.466.g9af26fa

[PATCH v2 4/6] t6300: make UTC and local dates different

[John Keeping]

By setting the UTC time to 23:18:43 the date in +0200 is the following
day, 2006-07-04.  This will ensure that the test for "short-local" to be
added in a following patch tests for different output from the "short"
format.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 t/t6300-for-each-ref.sh | 48 ++++++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 5fdb964..9e0096f 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -8,8 +8,8 @@ test_description='for-each-ref test'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-gpg.sh
 
-# Mon Jul 3 15:18:43 2006 +0000
-datestamp=1151939923
+# Mon Jul 3 23:18:43 2006 +0000
+datestamp=1151968723
 setdate_and_increment () {
     GIT_COMMITTER_DATE="$datestamp +0200"
     datestamp=$(expr "$datestamp" + 1)
@@ -61,21 +61,21 @@ test_atom head object ''
 test_atom head type ''
 test_atom head '*objectname' ''
 test_atom head '*objecttype' ''
-test_atom head author 'A U Thor <author@example.com> 1151939924 +0200'
+test_atom head author 'A U Thor <author@example.com> 1151968724 +0200'
 test_atom head authorname 'A U Thor'
 test_atom head authoremail '<author@example.com>'
-test_atom head authordate 'Mon Jul 3 17:18:44 2006 +0200'
-test_atom head committer 'C O Mitter <committer@example.com> 1151939923 +0200'
+test_atom head authordate 'Tue Jul 4 01:18:44 2006 +0200'
+test_atom head committer 'C O Mitter <committer@example.com> 1151968723 +0200'
 test_atom head committername 'C O Mitter'
 test_atom head committeremail '<committer@example.com>'
-test_atom head committerdate 'Mon Jul 3 17:18:43 2006 +0200'
+test_atom head committerdate 'Tue Jul 4 01:18:43 2006 +0200'
 test_atom head tag ''
 test_atom head tagger ''
 test_atom head taggername ''
 test_atom head taggeremail ''
 test_atom head taggerdate ''
-test_atom head creator 'C O Mitter <committer@example.com> 1151939923 +0200'
-test_atom head creatordate 'Mon Jul 3 17:18:43 2006 +0200'
+test_atom head creator 'C O Mitter <committer@example.com> 1151968723 +0200'
+test_atom head creatordate 'Tue Jul 4 01:18:43 2006 +0200'
 test_atom head subject 'Initial'
 test_atom head contents:subject 'Initial'
 test_atom head body ''
@@ -96,7 +96,7 @@ test_atom tag parent ''
 test_atom tag numparent ''
 test_atom tag object $(git rev-parse refs/tags/testtag^0)
 test_atom tag type 'commit'
-test_atom tag '*objectname' '67a36f10722846e891fbada1ba48ed035de75581'
+test_atom tag '*objectname' 'ea122842f48be4afb2d1fc6a4b96c05885ab7463'
 test_atom tag '*objecttype' 'commit'
 test_atom tag author ''
 test_atom tag authorname ''
@@ -107,18 +107,18 @@ test_atom tag committername ''
 test_atom tag committeremail ''
 test_atom tag committerdate ''
 test_atom tag tag 'testtag'
-test_atom tag tagger 'C O Mitter <committer@example.com> 1151939925 +0200'
+test_atom tag tagger 'C O Mitter <committer@example.com> 1151968725 +0200'
 test_atom tag taggername 'C O Mitter'
 test_atom tag taggeremail '<committer@example.com>'
-test_atom tag taggerdate 'Mon Jul 3 17:18:45 2006 +0200'
-test_atom tag creator 'C O Mitter <committer@example.com> 1151939925 +0200'
-test_atom tag creatordate 'Mon Jul 3 17:18:45 2006 +0200'
-test_atom tag subject 'Tagging at 1151939927'
-test_atom tag contents:subject 'Tagging at 1151939927'
+test_atom tag taggerdate 'Tue Jul 4 01:18:45 2006 +0200'
+test_atom tag creator 'C O Mitter <committer@example.com> 1151968725 +0200'
+test_atom tag creatordate 'Tue Jul 4 01:18:45 2006 +0200'
+test_atom tag subject 'Tagging at 1151968727'
+test_atom tag contents:subject 'Tagging at 1151968727'
 test_atom tag body ''
 test_atom tag contents:body ''
 test_atom tag contents:signature ''
-test_atom tag contents 'Tagging at 1151939927
+test_atom tag contents 'Tagging at 1151968727
 '
 test_atom tag HEAD ' '
 
@@ -163,11 +163,11 @@ test_date () {
 }
 
 test_expect_success 'Check unformatted date fields output' '
-	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"
+	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"
 '
 
 test_expect_success 'Check format "default" formatted date fields output' '
-	test_date default "Mon Jul 3 17:18:43 2006 +0200" "Mon Jul 3 17:18:44 2006 +0200" "Mon Jul 3 17:18:45 2006 +0200"
+	test_date default "Tue Jul 4 01:18:43 2006 +0200" "Tue Jul 4 01:18:44 2006 +0200" "Tue Jul 4 01:18:45 2006 +0200"
 '
 
 # Don't know how to do relative check because I can't know when this script
@@ -181,23 +181,23 @@ test_expect_success 'Check format "relative" date fields output' '
 '
 
 test_expect_success 'Check format "short" date fields output' '
-	test_date short 2006-07-03 2006-07-03 2006-07-03
+	test_date short 2006-07-04 2006-07-04 2006-07-04
 '
 
 test_expect_success 'Check format "local" date fields output' '
-	test_date local "Mon Jul 3 15:18:43 2006" "Mon Jul 3 15:18:44 2006" "Mon Jul 3 15:18:45 2006"
+	test_date local "Mon Jul 3 23:18:43 2006" "Mon Jul 3 23:18:44 2006" "Mon Jul 3 23:18:45 2006"
 '
 
 test_expect_success 'Check format "iso8601" date fields output' '
-	test_date iso8601 "2006-07-03 17:18:43 +0200" "2006-07-03 17:18:44 +0200" "2006-07-03 17:18:45 +0200"
+	test_date iso8601 "2006-07-04 01:18:43 +0200" "2006-07-04 01:18:44 +0200" "2006-07-04 01:18:45 +0200"
 '
 
 test_expect_success 'Check format "rfc2822" date fields output' '
-	test_date rfc2822 "Mon, 3 Jul 2006 17:18:43 +0200" "Mon, 3 Jul 2006 17:18:44 +0200" "Mon, 3 Jul 2006 17:18:45 +0200"
+	test_date rfc2822 "Tue, 4 Jul 2006 01:18:43 +0200" "Tue, 4 Jul 2006 01:18:44 +0200" "Tue, 4 Jul 2006 01:18:45 +0200"
 '
 
 test_expect_success 'Check format of strftime date fields' '
-	echo "my date is 2006-07-03" >expected &&
+	echo "my date is 2006-07-04" >expected &&
 	git for-each-ref \
 	  --format="%(authordate:format:my date is %Y-%m-%d)" \
 	  refs/heads >actual &&
@@ -515,8 +515,8 @@ body contents
 $sig"
 
 cat >expected <<EOF
-$(git rev-parse refs/tags/master) <committer@example.com> refs/tags/master
 $(git rev-parse refs/tags/bogo) <committer@example.com> refs/tags/bogo
+$(git rev-parse refs/tags/master) <committer@example.com> refs/tags/master
 EOF
 
 test_expect_success 'Verify sort with multiple keys' '
-- 
2.5.0.466.g9af26fa

[PATCH v2 5/6] t6300: add test for "raw" date format

[John Keeping]

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 t/t6300-for-each-ref.sh | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 9e0096f..2e76ca9 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -196,6 +196,10 @@ test_expect_success 'Check format "rfc2822" date fields output' '
 	test_date rfc2822 "Tue, 4 Jul 2006 01:18:43 +0200" "Tue, 4 Jul 2006 01:18:44 +0200" "Tue, 4 Jul 2006 01:18:45 +0200"
 '
 
+test_expect_success 'Check format "raw" date fields output' '
+	test_date raw "1151968723 +0200" "1151968724 +0200" "1151968725 +0200"
+'
+
 test_expect_success 'Check format of strftime date fields' '
 	echo "my date is 2006-07-04" >expected &&
 	git for-each-ref \
-- 
2.5.0.466.g9af26fa

[PATCH v2 6/6] t6300: add tests for "-local" date formats

[John Keeping]

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 t/t6300-for-each-ref.sh | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 2e76ca9..c3aee70 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -170,6 +170,10 @@ test_expect_success 'Check format "default" formatted date fields output' '
 	test_date default "Tue Jul 4 01:18:43 2006 +0200" "Tue Jul 4 01:18:44 2006 +0200" "Tue Jul 4 01:18:45 2006 +0200"
 '
 
+test_expect_success 'Check format "default-local" date fields output' '
+	test_date default-local "Mon Jul 3 23:18:43 2006" "Mon Jul 3 23:18:44 2006" "Mon Jul 3 23:18:45 2006"
+'
+
 # Don't know how to do relative check because I can't know when this script
 # is going to be run and can't fake the current time to git, and hence can't
 # provide expected output.  Instead, I'll just make sure that "relative"
@@ -180,10 +184,18 @@ test_expect_success 'Check format "relative" date fields output' '
 	git for-each-ref --shell --format="%(refname) %(taggerdate:$f)" refs/tags) >actual
 '
 
+test_expect_success 'Format "relative-local" is not allowed' '
+	test_must_fail git for-each-ref --format="%(authordate:relative-local)" refs/heads
+'
+
 test_expect_success 'Check format "short" date fields output' '
 	test_date short 2006-07-04 2006-07-04 2006-07-04
 '
 
+test_expect_success 'Check format "short-local" date fields output' '
+	test_date short-local 2006-07-03 2006-07-03 2006-07-03
+'
+
 test_expect_success 'Check format "local" date fields output' '
 	test_date local "Mon Jul 3 23:18:43 2006" "Mon Jul 3 23:18:44 2006" "Mon Jul 3 23:18:45 2006"
 '
@@ -192,14 +204,26 @@ test_expect_success 'Check format "iso8601" date fields output' '
 	test_date iso8601 "2006-07-04 01:18:43 +0200" "2006-07-04 01:18:44 +0200" "2006-07-04 01:18:45 +0200"
 '
 
+test_expect_success 'Check format "iso8601-local" date fields output' '
+	test_date iso8601-local "2006-07-03 23:18:43 +0000" "2006-07-03 23:18:44 +0000" "2006-07-03 23:18:45 +0000"
+'
+
 test_expect_success 'Check format "rfc2822" date fields output' '
 	test_date rfc2822 "Tue, 4 Jul 2006 01:18:43 +0200" "Tue, 4 Jul 2006 01:18:44 +0200" "Tue, 4 Jul 2006 01:18:45 +0200"
 '
 
+test_expect_success 'Check format "rfc2822-local" date fields output' '
+	test_date rfc2822-local "Mon, 3 Jul 2006 23:18:43 +0000" "Mon, 3 Jul 2006 23:18:44 +0000" "Mon, 3 Jul 2006 23:18:45 +0000"
+'
+
 test_expect_success 'Check format "raw" date fields output' '
 	test_date raw "1151968723 +0200" "1151968724 +0200" "1151968725 +0200"
 '
 
+test_expect_success 'Format "raw-local" is not allowed' '
+	test_must_fail git for-each-ref --format="%(authordate:raw-local)" refs/heads
+'
+
 test_expect_success 'Check format of strftime date fields' '
 	echo "my date is 2006-07-04" >expected &&
 	git for-each-ref \
@@ -208,6 +232,14 @@ test_expect_success 'Check format of strftime date fields' '
 	test_cmp expected actual
 '
 
+test_expect_success 'Check format of strftime-local date fields' '
+	echo "my date is 2006-07-03" >expected &&
+	git for-each-ref \
+	  --format="%(authordate:format-local:my date is %Y-%m-%d)" \
+	  refs/heads >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'exercise strftime with odd fields' '
 	echo >expected &&
 	git for-each-ref --format="%(authordate:format:)" refs/heads >actual &&
-- 
2.5.0.466.g9af26fa

Re: [PATCH v2 2/6] date: make "local" orthogonal to date format

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

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

Re: [PATCH v2 3/6] t6300: introduce test_date() helper

[Junio C Hamano]

Nicely done.

Re: [PATCH v2 2/6] date: make "local" orthogonal to date format

[Jeff King]

On Tue, Sep 01, 2015 at 03:16:50PM -0700, Junio C Hamano wrote:

> John Keeping <john@keeping.me.uk> writes:
> 
> > 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.

Yeah, that's why I didn't change it in the original. But to be honest, I
cannot imagine any case where that is _useful_, so I do not mind at all
to declare it off-limits, even though it is not nonsensical (though it
is a little strange to ask for "raw" data and then ask for it to be
munged).

IOW, I do not mind either way, but the fact that we have to _add_ code
to disallow it makes me slightly in favor of allowing it. :)

-Peff

Re: [PATCH v2 3/6] t6300: introduce test_date() helper

[Eric Sunshine]

On Tue, Sep 1, 2015 at 5:55 PM, John Keeping <john@keeping.me.uk> wrote:
> 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 <john@keeping.me.uk>
> ---
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 7c9bec7..5fdb964 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -146,85 +146,54 @@ test_expect_success 'Check invalid format specifiers are errors' '
>         test_must_fail git for-each-ref --format="%(authordate:INVALID)" refs/heads
>  '
>
> -cat >expected <<\EOF
> -'refs/heads/master' 'Mon Jul 3 17:18:43 2006 +0200' 'Mon Jul 3 17:18:44 2006 +0200'
> -'refs/tags/testtag' 'Mon Jul 3 17:18:45 2006 +0200'
> -EOF
> +test_date () {
> +       f=$1

f=$1 &&

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

Re: [PATCH v2 3/6] t6300: introduce test_date() helper

[Jeff King]

On Tue, Sep 01, 2015 at 10:55:41PM +0100, John Keeping wrote:

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

>  test_expect_success 'Check unformatted date fields output' '
> -	(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?

-Peff

Re: [PATCH v2 2/6] date: make "local" orthogonal to date format

[John Keeping]

On Tue, Sep 01, 2015 at 03:16:50PM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> > 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.

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.

-- >8 --
diff --git a/date.c b/date.c
index f048416..345890f 100644
--- a/date.c
+++ b/date.c
@@ -175,12 +175,6 @@ 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_RAW) {
-		strbuf_reset(&timebuf);
-		strbuf_addf(&timebuf, "%lu %+05d", time, tz);
-		return timebuf.buf;
-	}
-
 	if (mode->type == DATE_RELATIVE) {
 		struct timeval now;
 
@@ -193,6 +187,12 @@ const char *show_date(unsigned long time, int tz, const struct date_mode *mode)
 	if (mode->local)
 		tz = local_tzoffset(time);
 
+	if (mode->type == DATE_RAW) {
+		strbuf_reset(&timebuf);
+		strbuf_addf(&timebuf, "%lu %+05d", time, tz);
+		return timebuf.buf;
+	}
+
 	tm = time_to_tm(time, tz);
 	if (!tm) {
 		tm = time_to_tm(0, 0);

Re: [PATCH v2 2/6] date: make "local" orthogonal to date format

[Jeff King]

On Tue, Sep 01, 2015 at 11:33:08PM +0100, John Keeping wrote:

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

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

Re: [PATCH v2 3/6] t6300: introduce test_date() helper

[John Keeping]

On Tue, Sep 01, 2015 at 06:31:58PM -0400, Jeff King wrote:
> On Tue, Sep 01, 2015 at 10:55:41PM +0100, John Keeping wrote:
> 
> > 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. :)
> 
> >  test_expect_success 'Check unformatted date fields output' '
> > -	(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?

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!

Re: [PATCH v2 2/6] date: make "local" orthogonal to date format

[Junio C Hamano]

OK by me. Thanks, I also forgot that need for preparatory code movement.

On Tue, Sep 1, 2015 at 3:39 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Sep 01, 2015 at 11:33:08PM +0100, John Keeping wrote:
>
>> > 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.
>
> 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

Re: [PATCH v2 3/6] t6300: introduce test_date() helper

[Jeff King]

On Tue, Sep 01, 2015 at 11:40:13PM +0100, John Keeping wrote:

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

Yeah, that also looks good, and is probably less confusing that the
here-doc.

-Peff

Re: [PATCH v2 0/6] Make "local" orthogonal to date format

[Jeff King]

On Tue, Sep 01, 2015 at 10:55:38PM +0100, John Keeping wrote:

> 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

Looks like we've agreed on disallowing "raw-local"[1] for the 2nd patch.
The other 4 all look good to me, with the exception of the line-wrapping
on 3/6.  Thanks for taking care of this.

-Peff

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

Re: [PATCH v2 0/6] Make "local" orthogonal to date format

[John Keeping]

On Tue, Sep 01, 2015 at 06:44:31PM -0400, Jeff King wrote:
> [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.

Re: [PATCH v2 0/6] Make "local" orthogonal to date format

[Jeff King]

On Wed, Sep 02, 2015 at 08:48:26AM +0100, John Keeping wrote:

> On Tue, Sep 01, 2015 at 06:44:31PM -0400, Jeff King wrote:
> > [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.

-Peff

Re: [PATCH v2 0/6] Make "local" orthogonal to date format

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Sep 02, 2015 at 08:48:26AM +0100, John Keeping wrote:
>
>> On Tue, Sep 01, 2015 at 06:44:31PM -0400, Jeff King wrote:
>> > [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.

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.

Re: [PATCH v2 2/6] date: make "local" orthogonal to date format

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index a9b808f..5d28133 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -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
> +`--raw` or `--relative`.
>  +
> -`--date=local` shows timestamps in user's local time zone.
> +`--date=local` is an alias for `--date=default-local`.
>  +

This can also affect for-each-ref's "%(authordate:short)" and
friends.  We have this rather unfortunately detailed description:

    As a special case for the date-type fields, you may specify a format for
    the date by adding one of `:default`, `:relative`, `:short`, `:local`,
    `:iso8601`, `:rfc2822` or `:raw` to the end of the fieldname; e.g.
    `%(taggerdate:relative)`.

We would probably want to make this more vague by replacing "adding
one of..." with "adding ':' followed by date format name (see the
values the --date option to linkgit::git-rev-list[1] takes)" or
something like that.

Re: [PATCH 2/2] date: make "local" orthogonal to date format

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> +	/* 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"?

Re: [PATCH v2 0/6] Make "local" orthogonal to date format

[John Keeping]

On Wed, Sep 02, 2015 at 08:16:59AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > On Wed, Sep 02, 2015 at 08:48:26AM +0100, John Keeping wrote:
> >
> >> On Tue, Sep 01, 2015 at 06:44:31PM -0400, Jeff King wrote:
> >> > [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.
> 
> 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.

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.

Re: [PATCH v2 0/6] Make "local" orthogonal to date format

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> On Wed, Sep 02, 2015 at 08:16:59AM -0700, Junio C Hamano wrote:
>> Jeff King <peff@peff.net> writes:
>> 
>> > 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.

OK.

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

Re: [PATCH v2 0/6] Make "local" orthogonal to date format

[John Keeping]

On Wed, Sep 02, 2015 at 01:11:35PM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> > 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.

Re: [PATCH v2 0/6] Make "local" orthogonal to date format

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

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

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

As we'll go into pre-release feature freeze soonish, we have enough
time to add missing support to the code (the new feature won't have
to make the upcoming 2.6 wait).

Thanks.

Re: [PATCH v2 0/6] Make "local" orthogonal to date format

[Jeff King]

On Wed, Sep 02, 2015 at 01:29:08PM -0700, Junio C Hamano wrote:

> John Keeping <john@keeping.me.uk> writes:
> 
> > 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.
> 
> 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".

That sounds fine to me. Then we don't have to worry about the error
messages. ;)

-Peff

Re: [PATCH 2/2] date: make "local" orthogonal to date format

[Jeff King]

On Wed, Sep 02, 2015 at 10:41:34AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > +	/* 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

Yeah, that's not ideal.

> 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

-Peff

Re: [PATCH 2/2] date: make "local" orthogonal to date format

[John Keeping]

On Wed, Sep 02, 2015 at 05:30:14PM -0400, Jeff King wrote:
> On Wed, Sep 02, 2015 at 10:41:34AM -0700, Junio C Hamano wrote:
> 
> > Jeff King <peff@peff.net> writes:
> > 
> > > +	/* 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
> 
> Yeah, that's not ideal.
> 
> > 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

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.

That has the benefit of keeping the code simple, otherwise we have to
worry about "shorter" as well (in the patch as it stands that gives
"unknown date-mode modifier: er").

Re: [PATCH 2/2] date: make "local" orthogonal to date format

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> On Wed, Sep 02, 2015 at 05:30:14PM -0400, Jeff King wrote:
>> 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.

Either is acceptable and better than the 'e' output.  I think yours
is better because it would equally work well for those who think in
terms of "modifier" and those who think in terms of "flat list of
possibilities".

Thanks.

[PATCH v3 00/11] Make "local" orthogonal to date format

[John Keeping]

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.

I've also reordered the test cleanup patches earlier so that the test
for "--date=raw" is added before the new patch that moves "local"
processing before the "raw" case.  The tests also now wrap long lines
and a missing "&&" has been added.

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.

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

John Keeping (9):
  Documentation/blame-options: don't list date formats
  Documentation/config: don't list date formats
  Documentation/git-for-each-ref: don't list date formats
  Documentation/rev-list: don't list date formats
  t6300: introduce test_date() helper
  t6300: add test for "raw" date format
  date: check for "local" before anything else
  t6300: make UTC and local dates different
  t6300: add tests for "-local" date formats

 Documentation/blame-options.txt    |   5 +-
 Documentation/config.txt           |   4 +-
 Documentation/git-for-each-ref.txt |   5 +-
 Documentation/git-rev-list.txt     |   2 +-
 Documentation/rev-list-options.txt |  23 ++++--
 builtin/blame.c                    |   1 -
 cache.h                            |   2 +-
 date.c                             |  74 ++++++++++-------
 fast-import.c                      |   2 +-
 t/t6300-for-each-ref.sh            | 162 ++++++++++++++++++++++---------------
 10 files changed, 166 insertions(+), 114 deletions(-)

-- 
2.5.0.466.g9af26fa

[PATCH v3 01/11] Documentation/blame-options: don't list date formats

[John Keeping]

This list is already incomplete (missing "raw") and we're about to add
new formats.  Remove it and refer to the canonical documentation in
git-log(1).

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 Documentation/blame-options.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index a09969b..760eab7 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -63,11 +63,10 @@ include::line-range-format.txt[]
 	`-` to make the command read from the standard input).
 
 --date <format>::
-	The value is one of the following alternatives:
-	{relative,local,default,iso,rfc,short}. If --date is not
+	Specifies the format used to output dates. If --date is not
 	provided, the value of the blame.date config variable is
 	used. If the blame.date config variable is also not set, the
-	iso format is used. For more information, See the discussion
+	iso format is used. For supported values, see the discussion
 	of the --date option at linkgit:git-log[1].
 
 -M|<num>|::
-- 
2.5.0.466.g9af26fa

[PATCH v3 02/11] Documentation/config: don't list date formats

[John Keeping]

This list is already incomplete (missing "raw") and we're about to add
new formats.  Since this option sets a default for git-log's --date
option, just refer to git-log(1).

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 Documentation/config.txt | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f5d15ff..49f9fa8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1829,9 +1829,7 @@ log.abbrevCommit::
 log.date::
 	Set the default date-time mode for the 'log' command.
 	Setting a value for log.date is similar to using 'git log''s
-	`--date` option.  Possible values are `relative`, `local`,
-	`default`, `iso`, `rfc`, and `short`; see linkgit:git-log[1]
-	for details.
+	`--date` option.  See linkgit:git-log[1] for details.
 
 log.decorate::
 	Print out the ref names of any commits that are shown by the log
-- 
2.5.0.466.g9af26fa

[PATCH v3 03/11] Documentation/git-for-each-ref: don't list date formats

[John Keeping]

We are about to add a new set of supported date formats and do not want
to have to maintain the same list in several different bits of
documentation.  Refer to git-rev-list(1) which contains the full list of
supported formats.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 Documentation/git-for-each-ref.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 7f8d9a5..d062639 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -134,9 +134,8 @@ the object referred by the ref does not cause an error.  It
 returns an empty string instead.
 
 As a special case for the date-type fields, you may specify a format for
-the date by adding one of `:default`, `:relative`, `:short`, `:local`,
-`:iso8601`, `:rfc2822` or `:raw` to the end of the fieldname; e.g.
-`%(taggerdate:relative)`.
+the date by adding `:` followed by date format name (see the
+values the `--date` option to linkgit::git-rev-list[1] takes).
 
 
 EXAMPLES
-- 
2.5.0.466.g9af26fa

[PATCH v3 04/11] Documentation/rev-list: don't list date formats

[John Keeping]

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 <john@keeping.me.uk>
---
 Documentation/git-rev-list.txt     | 2 +-
 Documentation/rev-list-options.txt | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
index 7b49c85..ef22f17 100644
--- a/Documentation/git-rev-list.txt
+++ b/Documentation/git-rev-list.txt
@@ -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 ]
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index a9b808f..14c4cce 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -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.
-- 
2.5.0.466.g9af26fa

[PATCH v3 05/11] fast-import: switch crash-report date to iso8601

[John Keeping]

From: Jeff King <peff@peff.net>

When fast-import emits a crash report, it does so in the
user's local timezone. But because we omit the timezone
completely for DATE_LOCAL, a reader of the report does not
immediately know which time zone was used. Let's switch this
to ISO8601 instead, which includes the time zone.

This does mean we will show the time in UTC, but that's not
a big deal. A crash report like this will either be looked
at immediately (in which case nobody even looks at the
timestamp), or it will be passed along to a developer to
debug, in which case the original timezone is less likely to
be of interest.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: John Keeping <john@keeping.me.uk>
---
 fast-import.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fast-import.c b/fast-import.c
index 6c7c3c9..adcbfc6 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -424,7 +424,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, DATE_MODE(ISO8601)));
 	fputc('\n', rpt);
 
 	fputs("fatal: ", rpt);
-- 
2.5.0.466.g9af26fa

[PATCH v3 06/11] t6300: introduce test_date() helper

[John Keeping]

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 <john@keeping.me.uk>
---
Changes since v2:
- add missing "&&" after "f=$1"
- wrap long lines

 t/t6300-for-each-ref.sh | 92 +++++++++++++++++++++----------------------------
 1 file changed, 40 insertions(+), 52 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 7c9bec7..0bf709b 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -146,85 +146,73 @@ test_expect_success 'Check invalid format specifiers are errors' '
 	test_must_fail git for-each-ref --format="%(authordate:INVALID)" refs/heads
 '
 
-cat >expected <<\EOF
-'refs/heads/master' 'Mon Jul 3 17:18:43 2006 +0200' 'Mon Jul 3 17:18:44 2006 +0200'
-'refs/tags/testtag' 'Mon Jul 3 17:18:45 2006 +0200'
-EOF
+test_date () {
+	f=$1 &&
+	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
+}
 
 test_expect_success 'Check unformatted date fields output' '
-	(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"
 '
 
 test_expect_success 'Check format "default" formatted date fields output' '
-	f=default &&
-	(git for-each-ref --shell --format="%(refname) %(committerdate:$f) %(authordate:$f)" refs/heads &&
-	git for-each-ref --shell --format="%(refname) %(taggerdate:$f)" refs/tags) >actual &&
-	test_cmp expected actual
+	test_date default \
+		"Mon Jul 3 17:18:43 2006 +0200" \
+		"Mon Jul 3 17:18:44 2006 +0200" \
+		"Mon Jul 3 17:18:45 2006 +0200"
 '
 
 # Don't know how to do relative check because I can't know when this script
 # is going to be run and can't fake the current time to git, and hence can't
 # provide expected output.  Instead, I'll just make sure that "relative"
 # doesn't exit in error
-#
-#cat >expected <<\EOF
-#
-#EOF
-#
 test_expect_success 'Check format "relative" date fields output' '
 	f=relative &&
 	(git for-each-ref --shell --format="%(refname) %(committerdate:$f) %(authordate:$f)" refs/heads &&
 	git for-each-ref --shell --format="%(refname) %(taggerdate:$f)" refs/tags) >actual
 '
 
-cat >expected <<\EOF
-'refs/heads/master' '2006-07-03' '2006-07-03'
-'refs/tags/testtag' '2006-07-03'
-EOF
-
 test_expect_success 'Check format "short" date fields output' '
-	f=short &&
-	(git for-each-ref --shell --format="%(refname) %(committerdate:$f) %(authordate:$f)" refs/heads &&
-	git for-each-ref --shell --format="%(refname) %(taggerdate:$f)" refs/tags) >actual &&
-	test_cmp expected actual
+	test_date short 2006-07-03 2006-07-03 2006-07-03
 '
 
-cat >expected <<\EOF
-'refs/heads/master' 'Mon Jul 3 15:18:43 2006' 'Mon Jul 3 15:18:44 2006'
-'refs/tags/testtag' 'Mon Jul 3 15:18:45 2006'
-EOF
-
 test_expect_success 'Check format "local" date fields output' '
-	f=local &&
-	(git for-each-ref --shell --format="%(refname) %(committerdate:$f) %(authordate:$f)" refs/heads &&
-	git for-each-ref --shell --format="%(refname) %(taggerdate:$f)" refs/tags) >actual &&
-	test_cmp expected actual
+	test_date local \
+		"Mon Jul 3 15:18:43 2006" \
+		"Mon Jul 3 15:18:44 2006" \
+		"Mon Jul 3 15:18:45 2006"
 '
 
-cat >expected <<\EOF
-'refs/heads/master' '2006-07-03 17:18:43 +0200' '2006-07-03 17:18:44 +0200'
-'refs/tags/testtag' '2006-07-03 17:18:45 +0200'
-EOF
-
 test_expect_success 'Check format "iso8601" date fields output' '
-	f=iso8601 &&
-	(git for-each-ref --shell --format="%(refname) %(committerdate:$f) %(authordate:$f)" refs/heads &&
-	git for-each-ref --shell --format="%(refname) %(taggerdate:$f)" refs/tags) >actual &&
-	test_cmp expected actual
+	test_date iso8601 \
+		"2006-07-03 17:18:43 +0200" \
+		"2006-07-03 17:18:44 +0200" \
+		"2006-07-03 17:18:45 +0200"
 '
 
-cat >expected <<\EOF
-'refs/heads/master' 'Mon, 3 Jul 2006 17:18:43 +0200' 'Mon, 3 Jul 2006 17:18:44 +0200'
-'refs/tags/testtag' 'Mon, 3 Jul 2006 17:18:45 +0200'
-EOF
-
 test_expect_success 'Check format "rfc2822" date fields output' '
-	f=rfc2822 &&
-	(git for-each-ref --shell --format="%(refname) %(committerdate:$f) %(authordate:$f)" refs/heads &&
-	git for-each-ref --shell --format="%(refname) %(taggerdate:$f)" refs/tags) >actual &&
-	test_cmp expected actual
+	test_date rfc2822 \
+		"Mon, 3 Jul 2006 17:18:43 +0200" \
+		"Mon, 3 Jul 2006 17:18:44 +0200" \
+		"Mon, 3 Jul 2006 17:18:45 +0200"
 '
 
 test_expect_success 'Check format of strftime date fields' '
-- 
2.5.0.466.g9af26fa

[PATCH v3 07/11] t6300: add test for "raw" date format

[John Keeping]

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 t/t6300-for-each-ref.sh | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 0bf709b..6afcff6 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -215,6 +215,10 @@ test_expect_success 'Check format "rfc2822" date fields output' '
 		"Mon, 3 Jul 2006 17:18:45 +0200"
 '
 
+test_expect_success 'Check format "raw" date fields output' '
+	test_date raw "1151939923 +0200" "1151939924 +0200" "1151939925 +0200"
+'
+
 test_expect_success 'Check format of strftime date fields' '
 	echo "my date is 2006-07-03" >expected &&
 	git for-each-ref \
-- 
2.5.0.466.g9af26fa

[PATCH v3 08/11] date: check for "local" before anything else

[John Keeping]

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 <john@keeping.me.uk>
---
 date.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/date.c b/date.c
index 8f91569..9f0a5dd 100644
--- a/date.c
+++ b/date.c
@@ -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);
-- 
2.5.0.466.g9af26fa

[PATCH v3 09/11] date: make "local" orthogonal to date format

[John Keeping]

From: Jeff King <peff@peff.net>

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 <peff@peff.net>
Signed-off-by: John Keeping <john@keeping.me.uk>
---
Changes since v2:
- "local" check has moved above DATE_RAW processing as a result of an
  earlier patch
- "relative-local" and "raw-local" are now allowed
- the error message if the format starts with a valid sequence but is
  invalid as a whole is now consistent with the case where there is no
  valid prefix

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

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 14c4cce..359587c 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -702,12 +702,16 @@ include::pretty-options.txt[]
 --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.
+	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
+`--raw` or `--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::
diff --git a/builtin/blame.c b/builtin/blame.c
index 4db01c1..6fd1a63 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -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;
diff --git a/cache.h b/cache.h
index 4e25271..9a91b1d 100644
--- a/cache.h
+++ b/cache.h
@@ -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;
 };
 
 /*
diff --git a/date.c b/date.c
index 9f0a5dd..7c9f769 100644
--- a/date.c
+++ b/date.c
@@ -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;
 }
 
@@ -174,7 +175,7 @@ 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)
+	if (mode->local)
 		tz = local_tzoffset(time);
 
 	if (mode->type == DATE_RAW) {
@@ -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,31 +771,50 @@ 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
+	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))
+		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);
+	} else if (*p)
 		die("unknown date format %s", format);
 }
 
-- 
2.5.0.466.g9af26fa

[PATCH v3 10/11] t6300: make UTC and local dates different

[John Keeping]

By setting the UTC time to 23:18:43 the date in +0200 is the following
day, 2006-07-04.  This will ensure that the test for "short-local" to be
added in the following patch tests for different output from the "short"
format.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 t/t6300-for-each-ref.sh | 70 ++++++++++++++++++++++++-------------------------
 1 file changed, 35 insertions(+), 35 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 6afcff6..899251e 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -8,8 +8,8 @@ test_description='for-each-ref test'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-gpg.sh
 
-# Mon Jul 3 15:18:43 2006 +0000
-datestamp=1151939923
+# Mon Jul 3 23:18:43 2006 +0000
+datestamp=1151968723
 setdate_and_increment () {
     GIT_COMMITTER_DATE="$datestamp +0200"
     datestamp=$(expr "$datestamp" + 1)
@@ -61,21 +61,21 @@ test_atom head object ''
 test_atom head type ''
 test_atom head '*objectname' ''
 test_atom head '*objecttype' ''
-test_atom head author 'A U Thor <author@example.com> 1151939924 +0200'
+test_atom head author 'A U Thor <author@example.com> 1151968724 +0200'
 test_atom head authorname 'A U Thor'
 test_atom head authoremail '<author@example.com>'
-test_atom head authordate 'Mon Jul 3 17:18:44 2006 +0200'
-test_atom head committer 'C O Mitter <committer@example.com> 1151939923 +0200'
+test_atom head authordate 'Tue Jul 4 01:18:44 2006 +0200'
+test_atom head committer 'C O Mitter <committer@example.com> 1151968723 +0200'
 test_atom head committername 'C O Mitter'
 test_atom head committeremail '<committer@example.com>'
-test_atom head committerdate 'Mon Jul 3 17:18:43 2006 +0200'
+test_atom head committerdate 'Tue Jul 4 01:18:43 2006 +0200'
 test_atom head tag ''
 test_atom head tagger ''
 test_atom head taggername ''
 test_atom head taggeremail ''
 test_atom head taggerdate ''
-test_atom head creator 'C O Mitter <committer@example.com> 1151939923 +0200'
-test_atom head creatordate 'Mon Jul 3 17:18:43 2006 +0200'
+test_atom head creator 'C O Mitter <committer@example.com> 1151968723 +0200'
+test_atom head creatordate 'Tue Jul 4 01:18:43 2006 +0200'
 test_atom head subject 'Initial'
 test_atom head contents:subject 'Initial'
 test_atom head body ''
@@ -96,7 +96,7 @@ test_atom tag parent ''
 test_atom tag numparent ''
 test_atom tag object $(git rev-parse refs/tags/testtag^0)
 test_atom tag type 'commit'
-test_atom tag '*objectname' '67a36f10722846e891fbada1ba48ed035de75581'
+test_atom tag '*objectname' 'ea122842f48be4afb2d1fc6a4b96c05885ab7463'
 test_atom tag '*objecttype' 'commit'
 test_atom tag author ''
 test_atom tag authorname ''
@@ -107,18 +107,18 @@ test_atom tag committername ''
 test_atom tag committeremail ''
 test_atom tag committerdate ''
 test_atom tag tag 'testtag'
-test_atom tag tagger 'C O Mitter <committer@example.com> 1151939925 +0200'
+test_atom tag tagger 'C O Mitter <committer@example.com> 1151968725 +0200'
 test_atom tag taggername 'C O Mitter'
 test_atom tag taggeremail '<committer@example.com>'
-test_atom tag taggerdate 'Mon Jul 3 17:18:45 2006 +0200'
-test_atom tag creator 'C O Mitter <committer@example.com> 1151939925 +0200'
-test_atom tag creatordate 'Mon Jul 3 17:18:45 2006 +0200'
-test_atom tag subject 'Tagging at 1151939927'
-test_atom tag contents:subject 'Tagging at 1151939927'
+test_atom tag taggerdate 'Tue Jul 4 01:18:45 2006 +0200'
+test_atom tag creator 'C O Mitter <committer@example.com> 1151968725 +0200'
+test_atom tag creatordate 'Tue Jul 4 01:18:45 2006 +0200'
+test_atom tag subject 'Tagging at 1151968727'
+test_atom tag contents:subject 'Tagging at 1151968727'
 test_atom tag body ''
 test_atom tag contents:body ''
 test_atom tag contents:signature ''
-test_atom tag contents 'Tagging at 1151939927
+test_atom tag contents 'Tagging at 1151968727
 '
 test_atom tag HEAD ' '
 
@@ -168,16 +168,16 @@ test_date () {
 
 test_expect_success 'Check unformatted date fields output' '
 	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"
+		"Tue Jul 4 01:18:43 2006 +0200" \
+		"Tue Jul 4 01:18:44 2006 +0200" \
+		"Tue Jul 4 01:18:45 2006 +0200"
 '
 
 test_expect_success 'Check format "default" formatted date fields output' '
 	test_date default \
-		"Mon Jul 3 17:18:43 2006 +0200" \
-		"Mon Jul 3 17:18:44 2006 +0200" \
-		"Mon Jul 3 17:18:45 2006 +0200"
+		"Tue Jul 4 01:18:43 2006 +0200" \
+		"Tue Jul 4 01:18:44 2006 +0200" \
+		"Tue Jul 4 01:18:45 2006 +0200"
 '
 
 # Don't know how to do relative check because I can't know when this script
@@ -191,36 +191,36 @@ test_expect_success 'Check format "relative" date fields output' '
 '
 
 test_expect_success 'Check format "short" date fields output' '
-	test_date short 2006-07-03 2006-07-03 2006-07-03
+	test_date short 2006-07-04 2006-07-04 2006-07-04
 '
 
 test_expect_success 'Check format "local" date fields output' '
 	test_date local \
-		"Mon Jul 3 15:18:43 2006" \
-		"Mon Jul 3 15:18:44 2006" \
-		"Mon Jul 3 15:18:45 2006"
+		"Mon Jul 3 23:18:43 2006" \
+		"Mon Jul 3 23:18:44 2006" \
+		"Mon Jul 3 23:18:45 2006"
 '
 
 test_expect_success 'Check format "iso8601" date fields output' '
 	test_date iso8601 \
-		"2006-07-03 17:18:43 +0200" \
-		"2006-07-03 17:18:44 +0200" \
-		"2006-07-03 17:18:45 +0200"
+		"2006-07-04 01:18:43 +0200" \
+		"2006-07-04 01:18:44 +0200" \
+		"2006-07-04 01:18:45 +0200"
 '
 
 test_expect_success 'Check format "rfc2822" date fields output' '
 	test_date rfc2822 \
-		"Mon, 3 Jul 2006 17:18:43 +0200" \
-		"Mon, 3 Jul 2006 17:18:44 +0200" \
-		"Mon, 3 Jul 2006 17:18:45 +0200"
+		"Tue, 4 Jul 2006 01:18:43 +0200" \
+		"Tue, 4 Jul 2006 01:18:44 +0200" \
+		"Tue, 4 Jul 2006 01:18:45 +0200"
 '
 
 test_expect_success 'Check format "raw" date fields output' '
-	test_date raw "1151939923 +0200" "1151939924 +0200" "1151939925 +0200"
+	test_date raw "1151968723 +0200" "1151968724 +0200" "1151968725 +0200"
 '
 
 test_expect_success 'Check format of strftime date fields' '
-	echo "my date is 2006-07-03" >expected &&
+	echo "my date is 2006-07-04" >expected &&
 	git for-each-ref \
 	  --format="%(authordate:format:my date is %Y-%m-%d)" \
 	  refs/heads >actual &&
@@ -538,8 +538,8 @@ body contents
 $sig"
 
 cat >expected <<EOF
-$(git rev-parse refs/tags/master) <committer@example.com> refs/tags/master
 $(git rev-parse refs/tags/bogo) <committer@example.com> refs/tags/bogo
+$(git rev-parse refs/tags/master) <committer@example.com> refs/tags/master
 EOF
 
 test_expect_success 'Verify sort with multiple keys' '
-- 
2.5.0.466.g9af26fa

[PATCH v3 11/11] t6300: add tests for "-local" date formats

[John Keeping]

Signed-off-by: John Keeping <john@keeping.me.uk>
---
Changes since v2:
- "relative-local" and "raw-local" are now allowed

 t/t6300-for-each-ref.sh | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 899251e..03873b0 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -180,6 +180,10 @@ test_expect_success 'Check format "default" formatted date fields output' '
 		"Tue Jul 4 01:18:45 2006 +0200"
 '
 
+test_expect_success 'Check format "default-local" date fields output' '
+	test_date default-local "Mon Jul 3 23:18:43 2006" "Mon Jul 3 23:18:44 2006" "Mon Jul 3 23:18:45 2006"
+'
+
 # Don't know how to do relative check because I can't know when this script
 # is going to be run and can't fake the current time to git, and hence can't
 # provide expected output.  Instead, I'll just make sure that "relative"
@@ -190,10 +194,22 @@ test_expect_success 'Check format "relative" date fields output' '
 	git for-each-ref --shell --format="%(refname) %(taggerdate:$f)" refs/tags) >actual
 '
 
+# We just check that this is the same as "relative" for now.
+test_expect_success 'Check format "relative-local" date fields output' '
+	test_date relative-local \
+		"$(git for-each-ref --format="%(committerdate:relative)" refs/heads)" \
+		"$(git for-each-ref --format="%(authordate:relative)" refs/heads)" \
+		"$(git for-each-ref --format="%(taggerdate:relative)" refs/tags)"
+'
+
 test_expect_success 'Check format "short" date fields output' '
 	test_date short 2006-07-04 2006-07-04 2006-07-04
 '
 
+test_expect_success 'Check format "short-local" date fields output' '
+	test_date short-local 2006-07-03 2006-07-03 2006-07-03
+'
+
 test_expect_success 'Check format "local" date fields output' '
 	test_date local \
 		"Mon Jul 3 23:18:43 2006" \
@@ -208,6 +224,10 @@ test_expect_success 'Check format "iso8601" date fields output' '
 		"2006-07-04 01:18:45 +0200"
 '
 
+test_expect_success 'Check format "iso8601-local" date fields output' '
+	test_date iso8601-local "2006-07-03 23:18:43 +0000" "2006-07-03 23:18:44 +0000" "2006-07-03 23:18:45 +0000"
+'
+
 test_expect_success 'Check format "rfc2822" date fields output' '
 	test_date rfc2822 \
 		"Tue, 4 Jul 2006 01:18:43 +0200" \
@@ -215,10 +235,18 @@ test_expect_success 'Check format "rfc2822" date fields output' '
 		"Tue, 4 Jul 2006 01:18:45 +0200"
 '
 
+test_expect_success 'Check format "rfc2822-local" date fields output' '
+	test_date rfc2822-local "Mon, 3 Jul 2006 23:18:43 +0000" "Mon, 3 Jul 2006 23:18:44 +0000" "Mon, 3 Jul 2006 23:18:45 +0000"
+'
+
 test_expect_success 'Check format "raw" date fields output' '
 	test_date raw "1151968723 +0200" "1151968724 +0200" "1151968725 +0200"
 '
 
+test_expect_success 'Check format "raw-local" date fields output' '
+	test_date raw-local "1151968723 +0000" "1151968724 +0000" "1151968725 +0000"
+'
+
 test_expect_success 'Check format of strftime date fields' '
 	echo "my date is 2006-07-04" >expected &&
 	git for-each-ref \
@@ -227,6 +255,14 @@ test_expect_success 'Check format of strftime date fields' '
 	test_cmp expected actual
 '
 
+test_expect_success 'Check format of strftime-local date fields' '
+	echo "my date is 2006-07-03" >expected &&
+	git for-each-ref \
+	  --format="%(authordate:format-local:my date is %Y-%m-%d)" \
+	  refs/heads >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'exercise strftime with odd fields' '
 	echo >expected &&
 	git for-each-ref --format="%(authordate:format:)" refs/heads >actual &&
-- 
2.5.0.466.g9af26fa

Re: [PATCH v3 04/11] Documentation/rev-list: don't list date formats

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> 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 <john@keeping.me.uk>
> ---
>  Documentation/git-rev-list.txt     | 2 +-
>  Documentation/rev-list-options.txt | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

After seeing 1-3/11 this makes me wonder if we still have a list
somewhere in the documentation.  I'll read on first but I may have
to come back to this later.

Thanks.

>
> diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
> index 7b49c85..ef22f17 100644
> --- a/Documentation/git-rev-list.txt
> +++ b/Documentation/git-rev-list.txt
> @@ -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 ]
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index a9b808f..14c4cce 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -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.

Re: [PATCH v3 08/11] date: check for "local" before anything else

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

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

"local" is a request to show the timestamp in the output in our
local timezone regardless of the format, so even though "relative"
is not affected by the timezone (because "relative" does not show a
timestamp at all---it only shows the duration), this change
conceptually makes sense.

Thanks.


>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
>  date.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/date.c b/date.c
> index 8f91569..9f0a5dd 100644
> --- a/date.c
> +++ b/date.c
> @@ -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);

Re: [PATCH v3 00/11] Make "local" orthogonal to date format

[Jeff King]

On Thu, Sep 03, 2015 at 10:48:50PM +0100, John Keeping wrote:

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

This version looks good to me. It turned out to be a bigger job than we
expected at first; thanks for seeing it through.

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

That sounds OK. We have enough information in our parsing state to say
"I understood the 'iso8601', but '-foobar' did not make any sense to
me". But it's doubtful that would ever come up in practice.

-Peff