Michael Witten <mfwitten@xxxxxxxxx> writes:
> 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.
> 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.
> 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
> 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.
> @@ -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.
> +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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html