orthogonal cases of log --date option

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

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

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

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

-Peff

Miles Bader <miles@gnu.org> writes:

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

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

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

On Thu, Mar 5, 2009 at 5:43 AM, Jeff King <peff@peff.net> wrote:

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.

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

I meant the CLI.

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.

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

-Peff

Jeff King <peff@peff.net> writes:

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.

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

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.

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

Jeff King <peff@peff.net> writes:

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.

On Fri, Mar 6, 2009 at 1:50 AM, Junio C Hamano <gitster@pobox.com> wrote:

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.

Jay Soffian <jaysoffian@gmail.com> writes:

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

On Fri, Mar 6, 2009 at 3:02 AM, Junio C Hamano <gitster@pobox.com> wrote:

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.

Jay Soffian <jaysoffian@gmail.com> writes:

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

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

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

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

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

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

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

Jeff King <peff@peff.net> writes:

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