git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [RFC 3/5] Date Mode: Implementation

From
Junio C Hamano <gitster@pobox.com>
Date
Apr 21, 2011, 22:44 UTC
Message-ID
<7vk4enusvt.fsf@alter.siamese.dyndns.org>
In-Reply-To
<4e4fea2a-c9fe-47ff-b947-64aa4662adae-mfwitten@gmail.com>
Michael Witten <mfwitten@gmail.com> writes:
Show 6 quoted lines
> Date: Sat, 12 Feb 2011 03:56:17 +0000
> This introduces and employs infrastructure for manipulating
> and bundling time zone and date format specifications in
> code, on the command line, and in git configuration files.
>
> In particular, this commit includes the following:

Even though I think it is going in the right direction to separate "in what timezone do we show timestamps (original vs user-specified)" and "in what format do we show them (short? with tz? like in e-mail? iso? etc.), this particular commit seems to do too many things at once.

Can you think of a way split this into smaller steps? I am not asking "Could you pretty please split this?" nor "Are you willing to do so?" I am asking if the task is possible to begin with.

A valid answer could be "everything is so intertwined that this patch is the smallest logical unit of change", and I would be somewhat sympathetic to that. Once you change the type of revs->date_mode from the current single scalar to a struct, all the callers need to change the way they initialize and update the field, and I understand that such a change would have to touch a major part of the system.

But if that step can be done without changing the semantics nor behaviour at all, it would be easier to review and verify. The the next patch can enhance the struct in revs->date_mode to express timezone information as a new field, opening the door to the new command line option.

Show 7 quoted lines
> Essentially, what was:
>
>   git log --date=local
>
> should now be:
>
>   git log --time-zone=local
The above _should_ read something like:
	"git log --date=local"
        is now a short-hand for
        "git log --date-format=local --time-zone=local"
        It specifies that the timestamps should be stringified using the
        local timezone, and the value should be shown like "Tue Apr 19
        12:34:56 2011" without the timezone.
        "git log --time-zone=local" would give the same output but you
        will also see the timezone, e.g. "Tue Apr 19 12:34:56 2011 -0800",
        because --date-format defaults to "default".

In other words, this new feature should allow people who do not care about it to keep saying "--date=local" without being told that they now _should_ say things differently.

If it is absolutely impossible to implement the separation between the timezone and the format without breaking existing option handling, we would have to consider if the new feature (i.e. separation of zone and format) is worth breaking scripts and existing users, and we may end up rejecting the implementation of new feature. I however do think this particular feature is possible to implement without such a regression.

This patch makes the date-related infrastructure rich enough to deserve its own "date.h" that is included by "cache.h" near the beginning of it. The documentation part in [4/5] has such a separation, which I think is a sensible thing to do, except that date-mode-docs.txt is probably a wrong name for the file. If you followed the precedence set by most other includable pieces about the set of options, you would probably call it date-opts.txt or something.

Show 20 quoted lines
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index 60d6b32..add9299 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -361,26 +360,58 @@ static const char *copy_email(const char *buf)
>  	return xmemdupz(email, eoemail + 1 - email);
>  }
>  
> +#define PARSE_DATE_MODE(marker_0, parser_0, date_mode_member_0, \
> +                        marker_1, parser_1, date_mode_member_1) \
> +	if (*atomname == marker_0) { \
> +		const char *spec_0 = ++atomname; \
> +		do { \
> +			if (*atomname == marker_1) { \
> +				date_mode_member_1 = parser_1(atomname+1); \
> +				char *spec_0_dup  = xmemdupz(spec_0, atomname - spec_0); \
> +				date_mode_member_0 = parser_0(spec_0_dup); \
> +				free(spec_0_dup); \
> +				goto finish; \
> +			} \
This introduces:

builtin/for-each-ref.c:386: error: ISO C90 forbids mixed declarations and code builtin/for-each-ref.c:390: error: ISO C90 forbids mixed declarations and code

Show 12 quoted lines
> diff --git a/date.c b/date.c
> index caa14fe..879b0e1 100644
> --- a/date.c
> +++ b/date.c
> @@ -147,40 +147,42 @@ const char *show_date_relative(unsigned long time, int tz,
>  	return timebuf;
>  }
>  
> -const char *show_date(unsigned long time, int tz, enum date_mode mode)
> +const char *show_date(unsigned long time, int tz, struct date_mode date_mode)
>  {
> ...

I may be being old-fashioned, but I'd really prefer to see structures passed by pointer, not by value, even if the structure starts out as a small container for just two enum fields. There are a few other places you pass this structure by value.

Show 22 quoted lines
> @@ -653,7 +655,7 @@ int parse_date(const char *date, char *result, int maxlen)
>  	return date_string(timestamp, offset, result, maxlen);
>  }
>  
> -enum date_mode parse_date_format(const char *format)
> +enum date_format parse_date_format(const char *format)
>  {
>  	if (!strcmp(format, "relative"))
>  		return DATE_RELATIVE;
> @@ -675,6 +677,67 @@ enum date_mode parse_date_format(const char *format)
>  		die("unknown date format %s", format);
>  }
>  
> +enum time_zone parse_time_zone(const char *time_zone)
> +{
> +	if (!strcmp(time_zone, "local"))
> +		return TIME_ZONE_LOCAL;
> +	else if (!strcmp(time_zone, "default"))
> +		return TIME_ZONE_DEFAULT;
> +	else
> +		die("unknown time zone %s", time_zone);
> +}

Hmm, I was hoping that we can enhance this to allow feeding a string (e.g. "GMT", "+0900"), but "enum time_zone" makes it almost impossible.

Show 7 quoted lines
> +int parse_date_mode_config_internal(const char *var_date,
> +                                     const char *var_timezone,
> +                                     const char *var,
> +                                     const char *value,
> +                                     struct date_mode *d,
> +                                     int *ret)
> +{

I'd rather see you drop the preprocessor trick to synthesize "blame.date" and "blame.timezone" out of "blame", drop _internal from this function (which is not static to the file scope to begin with), and take a single "const char *var" as the first parameter. Otherwise, future users of the API cannot pass a variable to (your version of) parse_date_mode_config(). It is not implausible that someday somebody might want to parse per-branch timezone specification out of "branch.master.timezone", and most likely "branch.master" part would be created runtime in a strbuf, not limited to hardcoded program names such as "blame".

No other config callback function uses "a pointer to int as an argument to store the return value"; if you absolutely need a new calling convention, it needs to be documented better. What do *ret and the return value from this function mean from the caller's point of view?

Thanks.
Previous: Michael WittenNext: Michael Witten
Message 13 of 37 in “[RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local”
  1. Michael WittenApr 20, 2011
  2. 1/5 Light refactoring of date infrastructureMichael Witten, Apr 20, 2011
  3. 2/5 Pretty Print: show tz when using DATE_LOCALMichael Witten, Apr 20, 2011
  4. Junio C HamanoApr 21, 2011
  5. Dates in Commits and other issues of style (Re: [RFC 2/5] Pretty Print: show tz when using DATE_LOCAL)Michael Witten, Apr 22, 2011
  6. Miles BaderApr 25, 2011
  7. Junio C HamanoApr 25, 2011
  8. Jakub NarebskiApr 25, 2011
  9. Junio C HamanoApr 25, 2011
  10. Michael WittenApr 22, 2011
  11. Junio C HamanoApr 22, 2011
  12. 3/5 Date Mode: ImplementationMichael Witten, Apr 20, 2011
  13. Junio C HamanoApr 21, 2011
  14. 4/5 Date Mode: DocumentationMichael Witten, Apr 20, 2011
  15. 5/5 Date Mode: TestsMichael Witten, Apr 20, 2011
  16. Junio C HamanoApr 21, 2011
  17. Michael WittenApr 23, 2011
  18. Michael WittenApr 23, 2011
  19. Time zone option name (Re: [RFC 5/5] Date Mode: Tests)Michael Witten, Apr 23, 2011
  20. Junio C HamanoApr 23, 2011
  21. Michael WittenApr 23, 2011
  22. Jeff KingApr 20, 2011
  23. Michael WittenApr 20, 2011
  24. Junio C HamanoApr 21, 2011
  25. Michael WittenApr 21, 2011
  26. Junio C HamanoApr 21, 2011
  27. Michael WittenApr 21, 2011
  28. Michael WittenApr 20, 2011
  29. Michael WittenApr 20, 2011
  30. Michael WittenApr 20, 2011
  31. Tabs and spaces (Re: [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local)Jonathan Nieder, Apr 21, 2011
  32. Michael WittenApr 21, 2011
  33. Jonathan NiederApr 21, 2011
  34. Michael WittenApr 21, 2011
  35. Thiago FarinaApr 21, 2011
  36. Alex RiesenApr 21, 2011
  37. Michael WittenApr 21, 2011

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.