orthogonal cases of log --date option

orthogonal cases of log --date option

[Miles Bader]

I can use "git log --date=iso" to get YYYY-MM-DD format for dates, or
"git log --date=local" to force the dates to use my local time zone, but
if I use _both_ of these options together, it uses only the last one,
and ignores any preceding --date (even those in this case, the two
--date options affect orthogonal properties of dates).  Is there a way
to get YYYY-MM-DD format dates, but in my local time-zone?

Thanks,

-Miles

-- 
I'd rather be consing.

Re: orthogonal cases of log --date option

[Jeff King]

On Tue, Mar 03, 2009 at 05:18:56PM +0900, Miles Bader wrote:

> I can use "git log --date=iso" to get YYYY-MM-DD format for dates, or
> "git log --date=local" to force the dates to use my local time zone, but
> if I use _both_ of these options together, it uses only the last one,
> and ignores any preceding --date (even those in this case, the two
> --date options affect orthogonal properties of dates).

Yuck. It sounds like --date=local is really the wrong way to have
implemented it. It really should be:

  git log --date=iso --local-dates

I don't think there is currently a way to do what you want, but it is
not too late to add an option (and keep --date=local as a synonym for
--date=default --local-dates for compatibility).

> Is there a way to get YYYY-MM-DD format dates, but in my local
> time-zone?

Short of using --date=raw and munging the output with perl, I don't
think so.

-Peff

Re: orthogonal cases of log --date option

[Junio C Hamano]

Miles Bader <miles@gnu.org> writes:

> I can use "git log --date=iso" to get YYYY-MM-DD format for dates, or
> "git log --date=local" to force the dates to use my local time zone, but
> if I use _both_ of these options together, it uses only the last one,
> and ignores any preceding --date (even those in this case, the two
> --date options affect orthogonal properties of dates).  Is there a way
> to get YYYY-MM-DD format dates, but in my local time-zone?

No, there isn't.

But this patch may help you get started.

Just like any other patches I send out to only show a way to competent
people I can trust to carry it forward, it is not even compile tested,
though.

---
 builtin-for-each-ref.c |    2 +-
 builtin-log.c          |    2 +-
 cache.h                |   15 ++++++++-------
 date.c                 |   27 +++++++++++++++------------
 revision.c             |    2 +-
 5 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
index e46b7ad..3a9f64b 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c
@@ -361,7 +361,7 @@ static void grab_date(const char *buf, struct atom_value *v, const char *atomnam
 	formatp = strchr(atomname, ':');
 	if (formatp != NULL) {
 		formatp++;
-		date_mode = parse_date_format(formatp);
+		date_mode = parse_date_format(formatp, 0);
 	}
 
 	if (!eoemail)
diff --git a/builtin-log.c b/builtin-log.c
index 2ae39af..618922a 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -41,7 +41,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 	DIFF_OPT_SET(&rev->diffopt, ALLOW_TEXTCONV);
 
 	if (default_date_mode)
-		rev->date_mode = parse_date_format(default_date_mode);
+		rev->date_mode = parse_date_format(default_date_mode, 0);
 
 	argc = setup_revisions(argc, argv, rev, "HEAD");
 
diff --git a/cache.h b/cache.h
index 189151d..a3ecf63 100644
--- a/cache.h
+++ b/cache.h
@@ -692,19 +692,20 @@ extern struct object *peel_to_type(const char *name, int namelen,
 
 enum date_mode {
 	DATE_NORMAL = 0,
-	DATE_RELATIVE,
-	DATE_SHORT,
-	DATE_LOCAL,
-	DATE_ISO8601,
-	DATE_RFC2822,
-	DATE_RAW
+	DATE_RELATIVE = 1,
+	DATE_SHORT = 2,
+	DATE_ISO8601 = 3,
+	DATE_RFC2822 = 4,
+	DATE_RAW = 5,
+
+	DATE_LOCAL = 16, /* OR'ed in to others */
 };
 
 const char *show_date(unsigned long time, int timezone, enum date_mode mode);
 int parse_date(const char *date, char *buf, int bufsize);
 void datestamp(char *buf, int bufsize);
 unsigned long approxidate(const char *);
-enum date_mode parse_date_format(const char *format);
+enum date_mode parse_date_format(const char *format, enum date_mode so_far);
 
 #define IDENT_WARN_ON_NO_NAME  1
 #define IDENT_ERROR_ON_NO_NAME 2
diff --git a/date.c b/date.c
index d75dff4..8d04418 100644
--- a/date.c
+++ b/date.c
@@ -89,6 +89,10 @@ const char *show_date(unsigned long time, int tz, enum date_mode mode)
 	struct tm *tm;
 	static char timebuf[200];
 
+	if (mode & DATE_LOCAL)
+		tz = local_tzoffset(time);
+	mode &= ~DATE_LOCAL;
+
 	if (mode == DATE_RAW) {
 		snprintf(timebuf, sizeof(timebuf), "%lu %+05d", time, tz);
 		return timebuf;
@@ -136,9 +140,6 @@ const char *show_date(unsigned long time, int tz, enum date_mode mode)
 		/* Else fall back on absolute format.. */
 	}
 
-	if (mode == DATE_LOCAL)
-		tz = local_tzoffset(time);
-
 	tm = time_to_tm(time, tz);
 	if (!tm)
 		return NULL;
@@ -604,24 +605,26 @@ int parse_date(const char *date, char *result, int maxlen)
 	return date_string(then, offset, result, maxlen);
 }
 
-enum date_mode parse_date_format(const char *format)
+enum date_mode parse_date_format(const char *format, enum date_mode so_far)
 {
+	int or_in_local = so_far & DATE_LOCAL;
+
 	if (!strcmp(format, "relative"))
-		return DATE_RELATIVE;
+		return DATE_RELATIVE | or_in_local;
 	else if (!strcmp(format, "iso8601") ||
 		 !strcmp(format, "iso"))
-		return DATE_ISO8601;
+		return DATE_ISO8601 | or_in_local;
 	else if (!strcmp(format, "rfc2822") ||
 		 !strcmp(format, "rfc"))
-		return DATE_RFC2822;
+		return DATE_RFC2822 | or_in_local;
 	else if (!strcmp(format, "short"))
-		return DATE_SHORT;
-	else if (!strcmp(format, "local"))
-		return DATE_LOCAL;
+		return DATE_SHORT | or_in_local;
 	else if (!strcmp(format, "default"))
-		return DATE_NORMAL;
+		return DATE_NORMAL | or_in_local;
 	else if (!strcmp(format, "raw"))
-		return DATE_RAW;
+		return DATE_RAW | or_in_local;
+	else if (!strcmp(format, "local"))
+		return DATE_LOCAL | so_far;
 	else
 		die("unknown date format %s", format);
 }
diff --git a/revision.c b/revision.c
index 286e416..be9bbc4 100644
--- a/revision.c
+++ b/revision.c
@@ -1177,7 +1177,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if (!strcmp(arg, "--relative-date")) {
 		revs->date_mode = DATE_RELATIVE;
 	} else if (!strncmp(arg, "--date=", 7)) {
-		revs->date_mode = parse_date_format(arg + 7);
+		revs->date_mode = parse_date_format(arg + 7, revs->date_mode);
 	} else if (!strcmp(arg, "--log-size")) {
 		revs->show_log_size = 1;
 	}

Re: orthogonal cases of log --date option

[Jeff King]

On Tue, Mar 03, 2009 at 12:45:37AM -0800, Junio C Hamano wrote:

> Miles Bader <miles@gnu.org> writes:
> 
> > I can use "git log --date=iso" to get YYYY-MM-DD format for dates, or
> > "git log --date=local" to force the dates to use my local time zone, but
> > if I use _both_ of these options together, it uses only the last one,
> > and ignores any preceding --date (even those in this case, the two
> > --date options affect orthogonal properties of dates).  Is there a way
> > to get YYYY-MM-DD format dates, but in my local time-zone?
> 
> No, there isn't.
> 
> But this patch may help you get started.

FWIW, I think this is the wrong direction. You are working around the
lack of orthogonality in the interface by tweaking things in the
implementation. I think you are better to fix the interface, but support
--date=local for historical reasons. IOW,

  git log --local-dates --date=short

with

  git log --date=local

as a historical synonym for

  git log --local-dates --date=default

This makes the interface simpler to understand: --date remains a
selector, and --date=local is a special case that new people don't need
to think about or understand.

-Peff

Re: orthogonal cases of log --date option

[Jay Soffian]

On Thu, Mar 5, 2009 at 5:43 AM, Jeff King <peff@peff.net> wrote:
>> But this patch may help you get started.
>
> FWIW, I think this is the wrong direction. You are working around the
> lack of orthogonality in the interface by tweaking things in the
> implementation. I think you are better to fix the interface, but support
> --date=local for historical reasons. IOW,
>
>  git log --local-dates --date=short
>
> with
>
>  git log --date=local
>
> as a historical synonym for
>
>  git log --local-dates --date=default
>
> This makes the interface simpler to understand: --date remains a
> selector, and --date=local is a special case that new people don't need
> to think about or understand.

I started to pick this up and I want to clarify what you meant by
interface. Was it the CLI you had an issue with? Because that I
understand and it's easy to support the CLI changes you outline above.

Or did you have a problem with how Junio was going about passing along
both bits (i.e. 1. date format; 2. local or not) in an enum? Because I
have to tell you, I started looking at what it would take to switch
the enum to something like:

struct date_mode {
	enum {
		DATE_NORMAL = 0,
		DATE_RELATIVE,
		DATE_SHORT,
		DATE_ISO8601,
		DATE_RFC2822,
		DATE_RAW
	} format;
	unsigned int local;
};

It's a significantly more invasive change.

j.

Re: orthogonal cases of log --date option

[Jeff King]

On Thu, Mar 05, 2009 at 04:04:44PM -0500, Jay Soffian wrote:

> > This makes the interface simpler to understand: --date remains a
> > selector, and --date=local is a special case that new people don't need
> > to think about or understand.
> 
> I started to pick this up and I want to clarify what you meant by
> interface. Was it the CLI you had an issue with? Because that I
> understand and it's easy to support the CLI changes you outline above.

I meant the CLI.

> Or did you have a problem with how Junio was going about passing along
> both bits (i.e. 1. date format; 2. local or not) in an enum? Because I
> have to tell you, I started looking at what it would take to switch
> the enum to something like:

I find that a bit confusing, too, but at least it is not something users
see. So I don't feel as strongly about it.

> struct date_mode {
> 	enum {
> 		DATE_NORMAL = 0,
> 		DATE_RELATIVE,
> 		DATE_SHORT,
> 		DATE_ISO8601,
> 		DATE_RFC2822,
> 		DATE_RAW
> 	} format;
> 	unsigned int local;
> };
> 
> It's a significantly more invasive change.

Yep, it is more invasive. But I consider it more maintainable in the
long run.

-Peff

Re: orthogonal cases of log --date option

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yep, it is more invasive. But I consider it more maintainable in the
> long run.

Why do you think it is more confusing to ask "--date=local --date=iso"
than asking "--local-time --date=iso"?  If the patch under discussion were
not mine, I would have said that --date=local that flips the "lie about
timezone" bit and tells us to use the "default" format is a brilliant and
elegant solution.

I honestly do not see the point of what you are proposing to make
"selector" and "format" independent; unless you are shooting for
"--use-tz=Indian/Christmas --date=iso", that is.

The "--use-tz=zonename" might make some sense.  The required change would
look more like:

 (1) Introduce:

        const char *force_output_tz;

     that defaults to NULL;

 (2) Option parser for --use-tz=Indian/Christmas would store
     arg+9 to force_output_tz;

 (3) Option parser for --date=local would set the date format to
     "default", and store "localtime" to force_output_tz.  We discard
     DATE_LOCAL enum.

 (4) In show_date(), instead of 

	if (mode == DATE_LOCAL)
		tz = local_tzoffset(time);

     you'd do:

	if (forced_output_tz)
		tz = zonename_to_tzoffset(time, force_output_tz);

     zonename_to_tzoffset() would look up the zoneinfo database and
     compute the offset in our internal (hour*100+min) format.

But I do not think that is what you were proposing.

Re: orthogonal cases of log --date option

[Miles Bader]

Jeff King <peff@peff.net> writes:
>   git log --local-dates --date=default
>
> This makes the interface simpler to understand: --date remains a
> selector, and --date=local is a special case that new people don't need
> to think about or understand.

I agree, a separate option is more clear.

I suppose (as Junio later commented) that an explicit "--date-tz" option
might be cool too, though I suppose 99% of the time, people would only
ever specify "--date-tz=local" or "--date-gz=gmt"...

[one use-case I can think of for a more general option would be
if you want to see a log from somebody else's point of view ... "when
was john in san francisco doing all that hacking?"]

-Miles

-- 
The trouble with most people is that they think with their hopes or
fears or wishes rather than with their minds.  -- Will Durant

Re: orthogonal cases of log --date option

[Jeff King]

On Thu, Mar 05, 2009 at 02:21:27PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Yep, it is more invasive. But I consider it more maintainable in the
> > long run.
> 
> Why do you think it is more confusing to ask "--date=local --date=iso"
> than asking "--local-time --date=iso"?  If the patch under discussion were
> not mine, I would have said that --date=local that flips the "lie about
> timezone" bit and tells us to use the "default" format is a brilliant and
> elegant solution.

Because from the user's perspective --foo={bar,baz,bleep} is about
selecting exactly one of {bar,baz,bleep}. Having --date=local is like
having --pretty=abbrev-commit. Yes, it is vaguely related to date
display, but it is orthogonal to the selection of one of the mutually
exclusive options.

Maybe I am alone in viewing the options this way. But at the very least,
the documentation that reads:

  --date={relative,local,default,iso,rfc,short}

should be updated to show that _some_ options are mutually exclusive but
others are not.

> I honestly do not see the point of what you are proposing to make
> "selector" and "format" independent; unless you are shooting for
> "--use-tz=Indian/Christmas --date=iso", that is.

No, I am not really proposing --use-tz as I have no use for it. But it
is an example of why syntactically keeping the orthogonal concept of
"which timezone to use" away from "which date style to use" is useful.
The only reason --date=local _doesn't_ look terrible is that it is a
simple boolean. Any date-related flags that took an argument would look
stupid as --date=option=foo.

-Peff

Re: orthogonal cases of log --date option

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Because from the user's perspective --foo={bar,baz,bleep} is about
> selecting exactly one of {bar,baz,bleep}.

I do not feel very strongly about this either way, and without any prior
end user "Huh?" input, I would probably have argued like you myself, but
I saw the original message from Miles about giving more than one --date
and getting perplexed to see that it did not work, so...

I am not likely to use --tz=Indian/Christmas myself; GMT and local might
however be useful in some situations, though.

Re: orthogonal cases of log --date option

[Jay Soffian]

On Fri, Mar 6, 2009 at 1:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> Because from the user's perspective --foo={bar,baz,bleep} is about
>> selecting exactly one of {bar,baz,bleep}.
>
> I do not feel very strongly about this either way, and without any prior
> end user "Huh?" input, I would probably have argued like you myself, but
> I saw the original message from Miles about giving more than one --date
> and getting perplexed to see that it did not work, so...
>
> I am not likely to use --tz=Indian/Christmas myself; GMT and local might
> however be useful in some situations, though.

So I don't mind picking this up, but I'd like some guidance. There are
two issues:

1) The CLI. You and Jeff don't seem to have an agreement here, but
frankly, this is the easy part.

2) The internal implementation. Your implementation (enum -> bitfield)
is clever, but Jeff seems to prefer what I suggested (going to a
struct). The latter is quite a bit more work.

If we only care about fixing the original issue, I'll just pickup your
patch, make sure it compiles, and add some tests. I certainly don't
want to do more work than is needed, unless there's  a good reason to
do so.

j.

Re: orthogonal cases of log --date option

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> On Fri, Mar 6, 2009 at 1:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jeff King <peff@peff.net> writes:
>>
>>> Because from the user's perspective --foo={bar,baz,bleep} is about
>>> selecting exactly one of {bar,baz,bleep}.
>>
>> I do not feel very strongly about this either way, and without any prior
>> end user "Huh?" input, I would probably have argued like you myself, but
>> I saw the original message from Miles about giving more than one --date
>> and getting perplexed to see that it did not work, so...
>>
>> I am not likely to use --tz=Indian/Christmas myself; GMT and local might
>> however be useful in some situations, though.
>
> So I don't mind picking this up, but I'd like some guidance. There are
> two issues:
>
> 1) The CLI. You and Jeff don't seem to have an agreement here, but
> frankly, this is the easy part.
>
> 2) The internal implementation. Your implementation (enum -> bitfield)
> is clever, but Jeff seems to prefer what I suggested (going to a
> struct). The latter is quite a bit more work.

Is it?  Isn't it just the matter of doing something like this?

	struct date_mode {
        	enum {
                DATE_NORMAL = 0,
                DATE_RELATIVE,
                ...
                DATE_RAW
                } format;
                enum {
                DATE_ORIGINAL = 0,
                DATE_LOCAL
                /* perhaps ",DATE_GMT" later... */
                } tz_offset;
	};

	/* In revision.c::handle_revision_opt() */
        ...
	} else if (!strcmp(arg, "--date=local")) {
		revs->date_mode.format = DATE_NORMAL;
        	revs->date_mode.tz_offset = DATE_LOCAL;
	} else if (!prefixcmp(arg, "--date=")) {
        	revs->date_mode.format = parse_date_format(arg + 7);
	} else if (!strcmp(arg, "--tz=local")) {
        	revs->date_mode.tz_offset = DATE_LOCAL;
	}
	...

        /* In date.c::show_date() */
	...
        const char *show_date(unsigned long time, int tz, struct date_mode *mode_)
	{
        	int mode = mode_->format;

		if (mode_->tz_offset == DATE_LOCAL)
			tz = local_tzoffset(time);

		...
		/* and remove the existing
                if (mode == DATE_LOCAL)
                	tz = local_tzoffset(time);
		   that appears later in the code
		*/
	...

Re: orthogonal cases of log --date option

[Jay Soffian]

On Fri, Mar 6, 2009 at 3:02 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Is it?  Isn't it just the matter of doing something like this?
>
>        struct date_mode {
>                enum {
>                DATE_NORMAL = 0,
>                DATE_RELATIVE,
>                ...
>                DATE_RAW
>                } format;
>                enum {
>                DATE_ORIGINAL = 0,
>                DATE_LOCAL
>                /* perhaps ",DATE_GMT" later... */
>                } tz_offset;
>        };
>
>        /* In revision.c::handle_revision_opt() */
>        ...
>        } else if (!strcmp(arg, "--date=local")) {
>                revs->date_mode.format = DATE_NORMAL;
>                revs->date_mode.tz_offset = DATE_LOCAL;
>        } else if (!prefixcmp(arg, "--date=")) {
>                revs->date_mode.format = parse_date_format(arg + 7);
>        } else if (!strcmp(arg, "--tz=local")) {
>                revs->date_mode.tz_offset = DATE_LOCAL;
>        }
>        ...
>
>        /* In date.c::show_date() */
>        ...
>        const char *show_date(unsigned long time, int tz, struct date_mode *mode_)
>        {
>                int mode = mode_->format;
>
>                if (mode_->tz_offset == DATE_LOCAL)
>                        tz = local_tzoffset(time);
>
>                ...
>                /* and remove the existing
>                if (mode == DATE_LOCAL)
>                        tz = local_tzoffset(time);
>                   that appears later in the code
>                */
>        ...


Yeah, that part is easy. I wasn't sure the best way to handle places
where a constant date_mode is used e.g.:

pp_user_info(NULL, CMIT_FMT_EMAIL, &sb, committer, DATE_RFC2822,
		     encoding);

I had started on:

static const struct date_mode date_rfc2822 = {DATE_RFC2822, DATE_ORIGINAL};
...
pp_user_info(NULL, CMIT_FMT_EMAIL, &sb, committer, &date_rfc2822,
		     encoding);

But I imagine there's maybe a better way to do that.

j.

Re: orthogonal cases of log --date option

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> Yeah, that part is easy. I wasn't sure the best way to handle places
> where a constant date_mode is used e.g.:
>
> pp_user_info(NULL, CMIT_FMT_EMAIL, &sb, committer, DATE_RFC2822,
> 		     encoding);

One approach that will be hated by libgit2 effort would be to keep the
date_mode an enum as before (sans DATE_LOCAL) and make the "tz_offset"
thing as a setting global to the process, defined in environ.c

Re: orthogonal cases of log --date option

[Jeff King]

On Thu, Mar 05, 2009 at 10:50:42PM -0800, Junio C Hamano wrote:

> > Because from the user's perspective --foo={bar,baz,bleep} is about
> > selecting exactly one of {bar,baz,bleep}.
> 
> I do not feel very strongly about this either way, and without any prior
> end user "Huh?" input, I would probably have argued like you myself, but
> I saw the original message from Miles about giving more than one --date
> and getting perplexed to see that it did not work, so...

Interesting. I saw the original message from Miles as "I tried to use
more than one --date, but that seems wrong to me because these concepts
are orthogonal".  But I do recognize that it is somewhat a matter of
perception and style.

Anyway, as our one user in this thread, he has said the separate option
is more clear. So I think we should go with that.

-Peff

Re: orthogonal cases of log --date option

[Jeff King]

On Fri, Mar 06, 2009 at 01:58:36AM -0500, Jay Soffian wrote:

> 1) The CLI. You and Jeff don't seem to have an agreement here, but
> frankly, this is the easy part.
> 
> 2) The internal implementation. Your implementation (enum -> bitfield)
> is clever, but Jeff seems to prefer what I suggested (going to a
> struct). The latter is quite a bit more work.

My argument for (2), btw, is that the code is easier to read and more
maintainable if it follows the structure of the CLI. But I don't think
it's as important as getting the CLI right.

-Peff

Re: orthogonal cases of log --date option

[Jeff King]

On Fri, Mar 06, 2009 at 12:58:33AM -0800, Junio C Hamano wrote:

> Jay Soffian <jaysoffian@gmail.com> writes:
> 
> > Yeah, that part is easy. I wasn't sure the best way to handle places
> > where a constant date_mode is used e.g.:
> >
> > pp_user_info(NULL, CMIT_FMT_EMAIL, &sb, committer, DATE_RFC2822,
> > 		     encoding);
> 
> One approach that will be hated by libgit2 effort would be to keep the
> date_mode an enum as before (sans DATE_LOCAL) and make the "tz_offset"
> thing as a setting global to the process, defined in environ.c

I haven't looked, but might it not be possible to put it in the rev_info
struct next to the date format, but not as part of a struct? You will
then sometimes have to pass two options around (the format and the tz
offset) instead of one (the struct with both), but it makes split usage
much easier:

  pp_user_info(NULL, CMIT_FMT_EMAIL, &sb, committer, DATE_RFC2822,
               revs->tz_offset, encoding);

-Peff