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

John Keeping [off-list ref] 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.

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

John Keeping [off-list ref] writes:

quoted

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

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

John Keeping [off-list ref] writes:

quoted

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

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

quoted

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

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

On Tue, Sep 1, 2015 at 3:39 PM, Jeff King [off-list ref] wrote:

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

quoted

quoted

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