* [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local
@ 2011-04-20 2:53 Michael Witten
2011-04-20 2:45 ` [RFC 1/5] Light refactoring of date infrastructure Michael Witten
` (5 more replies)
0 siblings, 6 replies; 37+ messages in thread
From: Michael Witten @ 2011-04-20 2:53 UTC (permalink / raw)
To: git
One of the possible values for a date format is `local', which
specifies that a date should be output as though the date format
were instead `default' but in terms of the user's time zone
instead of the time zone stored by git; clearly, then, `local'
does not really provide just another format, but rather the
combination of 2 specifications:
* A format for the date (`default')
* A time zone in which to interpret the date (`local', if you will)
Interestingly, the existing documentation suggests that the original
purpose of the `--date' option was to specify the time zone rather
than a format:
$ git grep -hC1 '`--date=local`\|`--date=default`' \
> 6ceb270ce6f65cf4bc2a22830f96e0cc838b3fec \
> -- Documentation/rev-list-options.txt
+
`--date=local` shows timestamps in user's local timezone.
+
--
+
`--date=default` shows timestamps in the original timezone
(either committer's or author's).
This patch series reimplements the original purpose of `--date' by allowing
the time zone mode to be specified independently of the date format
(see the commit message for [2] and the documentation provided by [3]):
[0] Light refactoring of date infrastructure
[1] Pretty Print: show tz when using DATE_LOCAL
[2] Date Mode: Implementation
[3] Date Mode: Documentation
[4] Date Mode: Tests
The date format `local' has thus been deprecated, though it is still
supported (with a warning on stderr).
These patches apply cleanly to the current tip of Junio's `next' branch:
commit 63e4ee5f87eede11d1377370c385c26c5b90c6e7
Merge: 359ea0b 6ceb270
Author: Junio C Hamano <gitster@pobox.com>
Date: Tue Apr 19 11:51:25 2011 -0700
Merge branch 'master' into next
* master:
Git 1.7.5-rc3
Git 1.7.4.5
git-svn.txt: Document --mergeinfo
and it would appear that `make test' runs successfully (including the
extensive testing that I have implemented for date mode features, both
old and new).
There are some whitespace warnings (a la `git diff --check'), but I
have reviewed them and personally approve of them; if you think that
appraisal is incorrect, then you don't know what you're talking about :-P
Sincerely,
Michael Witten
Documentation/blame-options.txt | 9 +--
Documentation/config.txt | 32 ++++---
Documentation/date-mode-docs.txt | 87 +++++++++++++++++++
Documentation/git-for-each-ref.txt | 19 ++++-
Documentation/git-log.txt | 43 ---------
Documentation/git-rev-list.txt | 4 +-
Documentation/git-svn.txt | 2 +-
Documentation/rev-list-options.txt | 28 +------
archive.c | 2 +-
builtin/blame.c | 24 +++---
builtin/cat-file.c | 3 +-
builtin/commit.c | 2 +-
builtin/for-each-ref.c | 65 ++++++++++----
builtin/log.c | 17 ++--
builtin/shortlog.c | 2 +-
builtin/show-branch.c | 3 +-
cache.h | 41 ++++++++-
commit.h | 4 +-
date.c | 84 ++++++++++++++++---
fast-import.c | 4 +-
http-backend.c | 3 +-
log-tree.c | 10 ++-
pretty.c | 18 +++--
reflog-walk.c | 6 +-
reflog-walk.h | 4 +-
refs.c | 6 +-
revision.c | 11 ++-
revision.h | 4 +-
sha1_name.c | 5 +-
submodule.c | 2 +-
t/date-modes-lib.sh | 168 ++++++++++++++++++++++++++++++++++++
t/date-modes-permutator.perl | 94 ++++++++++++++++++++
t/t4202-log.sh | 3 +
t/t6300-for-each-ref.sh | 15 +++-
t/t8002-blame.sh | 9 ++
test-date.c | 11 ++-
36 files changed, 654 insertions(+), 190 deletions(-)
create mode 100644 Documentation/date-mode-docs.txt
create mode 100755 t/date-modes-lib.sh
create mode 100755 t/date-modes-permutator.perl
--
1.7.4.18.g68fe8
^ permalink raw reply [flat|nested] 37+ messages in thread* [RFC 1/5] Light refactoring of date infrastructure 2011-04-20 2:53 [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local Michael Witten @ 2011-04-20 2:45 ` Michael Witten 2011-04-20 2:45 ` [RFC 2/5] Pretty Print: show tz when using DATE_LOCAL Michael Witten ` (4 subsequent siblings) 5 siblings, 0 replies; 37+ messages in thread From: Michael Witten @ 2011-04-20 2:45 UTC (permalink / raw) To: git Date: Fri, 11 Feb 2011 18:34:29 +0000 date_mode_explicit -> date_mode_from_command_line It's more understandable by itself now. DATE_NORMAL -> DATE_DEFAULT The user input to select this value is `default'. Signed-off-by: Michael Witten <mfwitten@gmail.com> --- archive.c | 2 +- builtin/blame.c | 3 +-- builtin/commit.c | 2 +- builtin/for-each-ref.c | 2 +- builtin/shortlog.c | 2 +- cache.h | 2 +- date.c | 2 +- log-tree.c | 6 +++--- revision.c | 4 ++-- revision.h | 2 +- submodule.c | 2 +- 11 files changed, 14 insertions(+), 15 deletions(-) diff --git a/archive.c b/archive.c index 42f2d2f..dd871ca 100644 --- a/archive.c +++ b/archive.c @@ -32,7 +32,7 @@ static void format_subst(const struct commit *commit, char *to_free = NULL; struct strbuf fmt = STRBUF_INIT; struct pretty_print_context ctx = {0}; - ctx.date_mode = DATE_NORMAL; + ctx.date_mode = DATE_DEFAULT; ctx.abbrev = DEFAULT_ABBREV; if (src == buf->buf) diff --git a/builtin/blame.c b/builtin/blame.c index 4639788..dd597f4 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -2377,9 +2377,8 @@ parse_done: blame_date_width = sizeof("2006-10-19"); break; case DATE_RELATIVE: - /* "normal" is used as the fallback for "relative" */ case DATE_LOCAL: - case DATE_NORMAL: + case DATE_DEFAULT: blame_date_width = sizeof("Thu Oct 19 16:00:04 2006 -0700"); break; } diff --git a/builtin/commit.c b/builtin/commit.c index 67757e9..194db99 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -933,7 +933,7 @@ static const char *find_author_by_nickname(const char *name) commit = get_revision(&revs); if (commit) { struct pretty_print_context ctx = {0}; - ctx.date_mode = DATE_NORMAL; + ctx.date_mode = DATE_DEFAULT; strbuf_release(&buf); format_commit_message(commit, "%an <%ae>", &buf, &ctx); return strbuf_detach(&buf, NULL); diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index 89e75c6..60d6b32 100644 --- a/builtin/for-each-ref.c +++ b/builtin/for-each-ref.c @@ -367,7 +367,7 @@ static void grab_date(const char *buf, struct atom_value *v, const char *atomnam char *zone; unsigned long timestamp; long tz; - enum date_mode date_mode = DATE_NORMAL; + enum date_mode date_mode = DATE_DEFAULT; const char *formatp; /* diff --git a/builtin/shortlog.c b/builtin/shortlog.c index f5efc67..5815f55 100644 --- a/builtin/shortlog.c +++ b/builtin/shortlog.c @@ -165,7 +165,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit) ctx.abbrev = log->abbrev; ctx.subject = ""; ctx.after_subject = ""; - ctx.date_mode = DATE_NORMAL; + ctx.date_mode = DATE_DEFAULT; pretty_print_commit(CMIT_FMT_USERFORMAT, commit, &ufbuf, &ctx); buffer = ufbuf.buf; } else if (*buffer) { diff --git a/cache.h b/cache.h index 5b896d9..bf639f7 100644 --- a/cache.h +++ b/cache.h @@ -829,7 +829,7 @@ extern struct object *peel_to_type(const char *name, int namelen, struct object *o, enum object_type); enum date_mode { - DATE_NORMAL = 0, + DATE_DEFAULT = 0, DATE_RELATIVE, DATE_SHORT, DATE_LOCAL, diff --git a/date.c b/date.c index 00f9eb5..096468f 100644 --- a/date.c +++ b/date.c @@ -669,7 +669,7 @@ enum date_mode parse_date_format(const char *format) else if (!strcmp(format, "local")) return DATE_LOCAL; else if (!strcmp(format, "default")) - return DATE_NORMAL; + return DATE_DEFAULT; else if (!strcmp(format, "raw")) return DATE_RAW; else diff --git a/log-tree.c b/log-tree.c index 2a1e3a9..b059446 100644 --- a/log-tree.c +++ b/log-tree.c @@ -272,7 +272,7 @@ void get_patch_filename(struct commit *commit, int nr, const char *suffix, if (commit) { int max_len = start_len + FORMAT_PATCH_NAME_MAX - suffix_len; struct pretty_print_context ctx = {0}; - ctx.date_mode = DATE_NORMAL; + ctx.date_mode = DATE_DEFAULT; format_commit_message(commit, "%f", buf, &ctx); if (max_len < buf->len) @@ -465,9 +465,9 @@ void show_log(struct rev_info *opt) */ show_reflog_message(opt->reflog_info, opt->commit_format == CMIT_FMT_ONELINE, - opt->date_mode_explicit ? + opt->date_mode_from_command_line ? opt->date_mode : - DATE_NORMAL); + DATE_DEFAULT); if (opt->commit_format == CMIT_FMT_ONELINE) return; } diff --git a/revision.c b/revision.c index 541f09e..462c311 100644 --- a/revision.c +++ b/revision.c @@ -1434,10 +1434,10 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg revs->simplify_history = 0; } else if (!strcmp(arg, "--relative-date")) { revs->date_mode = DATE_RELATIVE; - revs->date_mode_explicit = 1; + revs->date_mode_from_command_line = 1; } else if ((argcount = parse_long_opt("date", argv, &optarg))) { revs->date_mode = parse_date_format(optarg); - revs->date_mode_explicit = 1; + revs->date_mode_from_command_line = 1; return argcount; } else if (!strcmp(arg, "--log-size")) { revs->show_log_size = 1; diff --git a/revision.h b/revision.h index 9fd8f30..e5ca939 100644 --- a/revision.h +++ b/revision.h @@ -92,7 +92,7 @@ struct rev_info { abbrev_commit:1, use_terminator:1, missing_newline:1, - date_mode_explicit:1; + date_mode_from_command_line:1; unsigned int disable_stdin:1; enum date_mode date_mode; diff --git a/submodule.c b/submodule.c index 5294cef..0a45da9 100644 --- a/submodule.c +++ b/submodule.c @@ -605,7 +605,7 @@ static void print_commit(struct commit *commit) { struct strbuf sb = STRBUF_INIT; struct pretty_print_context ctx = {0}; - ctx.date_mode = DATE_NORMAL; + ctx.date_mode = DATE_DEFAULT; format_commit_message(commit, " %h: %m %s", &sb, &ctx); fprintf(stderr, "%s\n", sb.buf); strbuf_release(&sb); -- 1.7.4.18.g68fe8 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [RFC 2/5] Pretty Print: show tz when using DATE_LOCAL 2011-04-20 2:53 [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local Michael Witten 2011-04-20 2:45 ` [RFC 1/5] Light refactoring of date infrastructure Michael Witten @ 2011-04-20 2:45 ` Michael Witten 2011-04-21 22:34 ` Junio C Hamano 2011-04-20 2:45 ` [RFC 3/5] Date Mode: Implementation Michael Witten ` (3 subsequent siblings) 5 siblings, 1 reply; 37+ messages in thread From: Michael Witten @ 2011-04-20 2:45 UTC (permalink / raw) To: git Date: Fri, 11 Feb 2011 16:06:36 +0000 Currently, when the date mode is DATE_LOCAL, the time zone is never pretty printed; this seems to be an unnecessary pecularliarity, especially when the time zone data could be useful to the user. This commit removes that special handling of time zones. Now, for instance, fast-import.c's write_crash_report() will produce reports that provide more meaningful dates. Signed-off-by: Michael Witten <mfwitten@gmail.com> --- date.c | 3 +-- t/t6300-for-each-ref.sh | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/date.c b/date.c index 096468f..caa14fe 100644 --- a/date.c +++ b/date.c @@ -186,13 +186,12 @@ const char *show_date(unsigned long time, int tz, enum date_mode mode) month_names[tm->tm_mon], tm->tm_year + 1900, tm->tm_hour, tm->tm_min, tm->tm_sec, tz); else - sprintf(timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d", + sprintf(timebuf, "%.3s %.3s %d %02d:%02d:%02d %d %+05d", weekday_names[tm->tm_wday], month_names[tm->tm_mon], tm->tm_mday, tm->tm_hour, tm->tm_min, tm->tm_sec, tm->tm_year + 1900, - (mode == DATE_LOCAL) ? 0 : ' ', tz); return timebuf; } diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 7dc8a51..050ed7d 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -175,8 +175,8 @@ test_expect_success 'Check format "short" date fields output' ' ' cat >expected <<\EOF -'refs/heads/master' 'Mon Jul 3 15:18:43 2006' 'Mon Jul 3 15:18:44 2006' -'refs/tags/testtag' 'Mon Jul 3 15:18:45 2006' +'refs/heads/master' 'Mon Jul 3 15:18:43 2006 +0000' 'Mon Jul 3 15:18:44 2006 +0000' +'refs/tags/testtag' 'Mon Jul 3 15:18:45 2006 +0000' EOF test_expect_success 'Check format "local" date fields output' ' -- 1.7.4.18.g68fe8 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [RFC 2/5] Pretty Print: show tz when using DATE_LOCAL 2011-04-20 2:45 ` [RFC 2/5] Pretty Print: show tz when using DATE_LOCAL Michael Witten @ 2011-04-21 22:34 ` Junio C Hamano 2011-04-22 14:08 ` Dates in Commits and other issues of style (Re: [RFC 2/5] Pretty Print: show tz when using DATE_LOCAL) Michael Witten 2011-04-22 14:36 ` [RFC 2/5] Pretty Print: show tz when using DATE_LOCAL Michael Witten 0 siblings, 2 replies; 37+ messages in thread From: Junio C Hamano @ 2011-04-21 22:34 UTC (permalink / raw) To: Michael Witten; +Cc: git Michael Witten <mfwitten@gmail.com> writes: > Date: Fri, 11 Feb 2011 16:06:36 +0000 > Currently, when the date mode is DATE_LOCAL, the > time zone is never pretty printed;... I am not opposed to have a new mode that shows timestamps in local timezone with zone information, but this change is a regression to people who have known and relied upon that asking for timestamps in their local timezone will give everything in the local timezone and result in a short output by the virtue of not needing to repeat the zone string, as they are the same (this is not strictly true near your dst boundary, and that is why I am open to the idea as a separate option). This is a tangent but it is funny to see the unnecessary Date: in-body header for a series about date display. Please drop it. Backdating the author timestamp will make it harder to find the related discussion from the list archive; the only plausible benefit I can see is that you may get "I thought of this much earlier than when I posted it to the public for the first time" pee-in-the-snow value out of doing so, but that is done at the cost to all others who need to inspect the history later. Please don't. As a future reference, when you have a valid reason to override the header information your MUA would give your message with an in-body header, please leave a blank line after the in-body header to make the result easier to read, like this: Date: Fri, 11 Feb 2011 16:06:36 +0000 Currently, when the date mode is DATE_LOCAL, the time zone is never pretty printed;... Also paragraphs that wrap lines at too narrow a margin is just as hard to read as paragraphs wrapped at a margin that is too wide. You seem to have wrapped at around 50 columns? ^ permalink raw reply [flat|nested] 37+ messages in thread
* Dates in Commits and other issues of style (Re: [RFC 2/5] Pretty Print: show tz when using DATE_LOCAL) 2011-04-21 22:34 ` Junio C Hamano @ 2011-04-22 14:08 ` Michael Witten 2011-04-25 1:26 ` Miles Bader 2011-04-22 14:36 ` [RFC 2/5] Pretty Print: show tz when using DATE_LOCAL Michael Witten 1 sibling, 1 reply; 37+ messages in thread From: Michael Witten @ 2011-04-22 14:08 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Thu, 21 Apr 2011 15:34:49 -0700, Junio C Hamano wrote: > Michael Witten <mfwitten@gmail.com> writes: > >> Date: Fri, 11 Feb 2011 16:06:36 +0000 >> Currently, when the date mode is DATE_LOCAL, the >> time zone is never pretty printed;... > > This is a tangent but it is funny to see the unnecessary Date: in-body > header for a series about date display. Please drop it. No. > Backdating the author timestamp will make it harder to find the related > discussion from the list archive; the only plausible benefit I can see is > that you may get "I thought of this much earlier than when I posted it to > the public for the first time" pee-in-the-snow value out of doing so, but > that is done at the cost to all others who need to inspect the history > later. Please don't. What if I had submitted a pull request instead of inlined patches? Would you be asking me to wipe the dates in my repository? Would you rewrite the commits on your end? Let's suppose that I like backdating specifically for the "pee-in-the-snow" value; well, that's one of the prime motivators for doing unpaid, volunteer work, and that's one of the reasons that distributed SCM tools like git are so great: Unlike with, say, CVS, the actual author gets his or her information officially recorded. Perhaps you think we should dispense with identity information as well, given that it's just a pee-in-the-snow value... BUT WAIT! Names and email addresses are important because of copyright issues; we need to know whence came a contribution, after all. Well... don't you think the particular date at which something was written might be similarly valuable in a dispute over copyright? Junio, you'll take my pee-in-the-snow and you'll like it. > As a future reference, when you have a valid reason to override the > header information your MUA would give your message with an in-body > header, please leave a blank line after the in-body header to make the > result easier to read, like this: > > Date: Fri, 11 Feb 2011 16:06:36 +0000 > > Currently, when the date mode is DATE_LOCAL, the > time zone is never pretty printed;... Fair enough. > Also paragraphs that wrap lines at too narrow a margin is just as hard to > read as paragraphs wrapped at a margin that is too wide. I disagree that it's too narrow, and I feel like you are now nitpicking. Sincerely, Michael Witten ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Dates in Commits and other issues of style (Re: [RFC 2/5] Pretty Print: show tz when using DATE_LOCAL) 2011-04-22 14:08 ` Dates in Commits and other issues of style (Re: [RFC 2/5] Pretty Print: show tz when using DATE_LOCAL) Michael Witten @ 2011-04-25 1:26 ` Miles Bader 2011-04-25 3:57 ` Junio C Hamano 0 siblings, 1 reply; 37+ messages in thread From: Miles Bader @ 2011-04-25 1:26 UTC (permalink / raw) To: Michael Witten; +Cc: Junio C Hamano, git Michael Witten <mfwitten@gmail.com> writes: > and I feel like > you are now > nitpicking. Wait, isn't nitpicking his job?! -miles -- Apologize, v. To lay the foundation for a future offense. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Dates in Commits and other issues of style (Re: [RFC 2/5] Pretty Print: show tz when using DATE_LOCAL) 2011-04-25 1:26 ` Miles Bader @ 2011-04-25 3:57 ` Junio C Hamano 2011-04-25 10:45 ` Jakub Narebski 0 siblings, 1 reply; 37+ messages in thread From: Junio C Hamano @ 2011-04-25 3:57 UTC (permalink / raw) To: Miles Bader; +Cc: Michael Witten, git Miles Bader <miles@gnu.org> writes: > Michael Witten <mfwitten@gmail.com> writes: >> and I feel like >> you are now >> nitpicking. > > Wait, isn't nitpicking his job?! Enforcing consistency is one of the important tasks the maintainers do in their projects. Besides ensuring that the intent of the change each patch brings to the codebase is good, that the log entry describes the change in a useful way for future readers, and that the patch correctly implements the described change, we also need to make sure that the resulting code matches the style of the surrounding code, and the style, structure and tone the log messages are delivered in a consistent voice. Otherwise it would quickly get very tiring when you have to dig into the history of the codebase. The code and the history are read a lot more often than are written. When a casual and occasional contributor sends in a good change whose only sin is style violation, I do not mind touching up its proposed commit log message or the patch text, and I often do. But I expect contributors who return here often to be more considerate than one-time contributors, and that is why I give style reminders. The purpose of the style reminder is so that they can help me and other reviewers concentrate on the substance (Is what it does good? Is it explained well? Is it implemented well?) without having to spend unnecessary time fixing the form (Does the new code fit well with the surrounding code? Does the message flow well in "git log" and easy to understand?) when they submit their changes the next time. Please do not mistake a style reminder as an opportunity to promote your own style that would not match what we have established here. I could make a black-list of people I should avoid giving style reminders and instead fix all of their submissions silently, because giving style remainders to them will waste people's time by creating a discussion thread like this one. I really wish I do not have to do so. Such an arrangement would not scale. Given two sets of patches with equal goodness in substance, if one also matches our style and the other deliberately asks me to spend extra time to whip it into our style, the latter naturally has to be assigned a lower priority. After all, there is only 24 hours in a day, and my time is better spent on substance not form, and definitely not on responding to quibblings about styles. The only two "workable" solutions are either (1) everybody tries to be consistent with the project's style, or (2) allow everybody to stick to their own style, making the resulting code and history unpleasant to read. I'd be failing the community if I took the latter approach. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Dates in Commits and other issues of style (Re: [RFC 2/5] Pretty Print: show tz when using DATE_LOCAL) 2011-04-25 3:57 ` Junio C Hamano @ 2011-04-25 10:45 ` Jakub Narebski 2011-04-25 18:29 ` Junio C Hamano 0 siblings, 1 reply; 37+ messages in thread From: Jakub Narebski @ 2011-04-25 10:45 UTC (permalink / raw) To: Junio C Hamano; +Cc: Miles Bader, Michael Witten, git Junio C Hamano <gitster@pobox.com> writes: > Enforcing consistency is one of the important tasks the maintainers do in > their projects. Besides ensuring that the intent of the change each patch > brings to the codebase is good, that the log entry describes the change in > a useful way for future readers, and that the patch correctly implements > the described change, we also need to make sure that the resulting code > matches the style of the surrounding code, and the style, structure and > tone the log messages are delivered in a consistent voice. Otherwise it > would quickly get very tiring when you have to dig into the history of the > codebase. The code and the history are read a lot more often than are > written. [...] This information should be put e.g. in SubmittingPatches, or CodingGuidelines, or MaintNotes, isn't it? -- Jakub Narebski Poland ShadeHawk on #git ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Dates in Commits and other issues of style (Re: [RFC 2/5] Pretty Print: show tz when using DATE_LOCAL) 2011-04-25 10:45 ` Jakub Narebski @ 2011-04-25 18:29 ` Junio C Hamano 0 siblings, 0 replies; 37+ messages in thread From: Junio C Hamano @ 2011-04-25 18:29 UTC (permalink / raw) To: Jakub Narebski; +Cc: Miles Bader, Michael Witten, git Jakub Narebski <jnareb@gmail.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Enforcing consistency is one of the important tasks the maintainers do... > > This information should be put e.g. in SubmittingPatches, or > CodingGuidelines, or MaintNotes, isn't it? The above "we value consistency" should be already inferrable by reading SubmittingPatches and CodingStyle in reverse? If it were up to the contributors to ignore them, there is no point to have these guides to begin with. If we want to keep these words somewhere, I think the place to put it would probably be the Policy section of Doc/howto/maintain-git.txt. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 2/5] Pretty Print: show tz when using DATE_LOCAL 2011-04-21 22:34 ` Junio C Hamano 2011-04-22 14:08 ` Dates in Commits and other issues of style (Re: [RFC 2/5] Pretty Print: show tz when using DATE_LOCAL) Michael Witten @ 2011-04-22 14:36 ` Michael Witten 2011-04-22 15:06 ` Junio C Hamano 1 sibling, 1 reply; 37+ messages in thread From: Michael Witten @ 2011-04-22 14:36 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Thu, 21 Apr 2011 15:34:49 -0700, Junio C Hamano wrote: > Michael Witten <mfwitten@gmail.com> writes: > >> Date: Fri, 11 Feb 2011 16:06:36 +0000 >> Currently, when the date mode is DATE_LOCAL, the >> time zone is never pretty printed;... > > I am not opposed to have a new mode that shows timestamps in local > timezone with zone information, but this change is a regression to people > who have known and relied upon that asking for timestamps in their local > timezone will give everything in the local timezone and result in a short > output by the virtue of not needing to repeat the zone string, as they are > the same (this is not strictly true near your dst boundary, and that is > why I am open to the idea as a separate option). Allow me to restate what I think you are saying: How about having [the still deprecated] --date=local (that is, DATE_LOCAL) option produce the old behavior, but have every combination of date mode options that does not involve DATE_LOCAL always emit the time zone. In other words, all of these do not emit a time zone: --date=local --date=local --time-zone=local --date=local --time-zone=default # --time-zone is ignored for simplicity. and all of these always emit a time zone: --date=default --date=rfc --date=rfc2822 --date=iso --date=iso8601 --date=short --date=raw --date=default --time-zone=default --date=rfc --time-zone=default --date=rfc2822 --time-zone=default --date=iso --time-zone=default --date=iso8601 --time-zone=default --date=short --time-zone=default --date=raw --time-zone=default --date=default --time-zone=local --date=rfc2822 --time-zone=local --date=rfc --time-zone=local --date=iso --time-zone=local --date=iso8601 --time-zone=local --date=short --time-zone=local --date=raw --time-zone=local --time-zone=local --time-zone=default ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 2/5] Pretty Print: show tz when using DATE_LOCAL 2011-04-22 14:36 ` [RFC 2/5] Pretty Print: show tz when using DATE_LOCAL Michael Witten @ 2011-04-22 15:06 ` Junio C Hamano 0 siblings, 0 replies; 37+ messages in thread From: Junio C Hamano @ 2011-04-22 15:06 UTC (permalink / raw) To: Michael Witten; +Cc: git Michael Witten <mfwitten@gmail.com> writes: > Allow me to restate what I think you are saying: > > How about having [the still deprecated] --date=local (that is, DATE_LOCAL) > option produce the old behavior, but have every combination of date mode > options that does not involve DATE_LOCAL always emit the time zone. > > In other words, all of these do not emit a time zone: > > --date=local > --date=local --time-zone=local > --date=local --time-zone=default # --time-zone is ignored for simplicity. Yeah. It is even plausible to extend --date further so that we can express things like --date='%a %b %d %H:%M:%S %Y' --date='%a %b %d %H:%M:%S %Y %z' The former would be equivalent to --date[-format]=local, the latter would be equivalent to --date[-format]=default. Or something like that [warning: without morning caffeine yet] :-) ^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC 3/5] Date Mode: Implementation 2011-04-20 2:53 [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local Michael Witten 2011-04-20 2:45 ` [RFC 1/5] Light refactoring of date infrastructure Michael Witten 2011-04-20 2:45 ` [RFC 2/5] Pretty Print: show tz when using DATE_LOCAL Michael Witten @ 2011-04-20 2:45 ` Michael Witten 2011-04-21 22:44 ` Junio C Hamano 2011-04-20 2:45 ` [RFC 4/5] Date Mode: Documentation Michael Witten ` (2 subsequent siblings) 5 siblings, 1 reply; 37+ messages in thread From: Michael Witten @ 2011-04-20 2:45 UTC (permalink / raw) To: git 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: * `struct date_mode' along with functions and macros for working with it. * The new `--time-zone' option (which takes as a value either `default' or `local'). * The new date-type mode specifiers `@default' and `@local' for use with the formats of "git for-each-ref". * The configuration variables `log.timezone' and `blame.timezone' and the functions for handling their parsing. Moreover, the value `local' for a date mode format has been deprecated in favor of the value `local' for a date mode time zone; a use of the deprecated `local' format causes an informational warning to be emitted on stderr (though this message might be missed quite easily due to further normal output on stdout). Essentially, what was: git log --date=local should now be: git log --time-zone=local or, perhaps more accurately (depending on what the actual unspecified date mode format is): git log --date=default --time-zone=local The other valid time zone value is `default', which specifies that the time zone stored by git be used. Signed-off-by: Michael Witten <mfwitten@gmail.com> --- archive.c | 2 +- builtin/blame.c | 23 +++++++------- builtin/cat-file.c | 3 +- builtin/commit.c | 2 +- builtin/for-each-ref.c | 65 +++++++++++++++++++++++++++++---------- builtin/log.c | 17 ++++++---- builtin/shortlog.c | 2 +- builtin/show-branch.c | 3 +- cache.h | 39 +++++++++++++++++++++-- commit.h | 4 +- date.c | 79 +++++++++++++++++++++++++++++++++++++++++++----- fast-import.c | 4 ++- http-backend.c | 3 +- log-tree.c | 10 ++++-- pretty.c | 18 +++++++---- reflog-walk.c | 6 ++-- reflog-walk.h | 4 +- refs.c | 6 ++- revision.c | 11 ++++-- revision.h | 4 +- sha1_name.c | 5 ++- submodule.c | 2 +- test-date.c | 11 ++++-- 23 files changed, 236 insertions(+), 87 deletions(-) diff --git a/archive.c b/archive.c index dd871ca..cb3e731 100644 --- a/archive.c +++ b/archive.c @@ -32,7 +32,7 @@ static void format_subst(const struct commit *commit, char *to_free = NULL; struct strbuf fmt = STRBUF_INIT; struct pretty_print_context ctx = {0}; - ctx.date_mode = DATE_DEFAULT; + SET_DATE_MODE_DEFAULT(ctx.date_mode); ctx.abbrev = DEFAULT_ABBREV; if (src == buf->buf) diff --git a/builtin/blame.c b/builtin/blame.c index dd597f4..c28d870 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -43,8 +43,8 @@ static int incremental; static int xdl_opts; static int abbrev = -1; -static enum date_mode blame_date_mode = DATE_ISO8601; static size_t blame_date_width; +static struct date_mode blame_date_mode = INIT_DATE_MODE_FORMAT(DATE_ISO8601); static struct string_list mailmap; @@ -2023,6 +2023,8 @@ static void prepare_blame_range(struct scoreboard *sb, static int git_blame_config(const char *var, const char *value, void *cb) { + int ret; + if (!strcmp(var, "blame.showroot")) { show_root = git_config_bool(var, value); return 0; @@ -2031,12 +2033,9 @@ static int git_blame_config(const char *var, const char *value, void *cb) blank_boundary = git_config_bool(var, value); return 0; } - if (!strcmp(var, "blame.date")) { - if (!value) - return config_error_nonbool(var); - blame_date_mode = parse_date_format(value); - return 0; - } + + if (parse_date_mode_config("blame", var, value, &blame_date_mode, &ret)) + return ret; switch (userdiff_config(var, value)) { case 0: @@ -2347,6 +2346,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix) parse_done: argc = parse_options_end(&ctx); + sanitize_date_mode("blame", &revs.date_mode); + if (abbrev == -1) abbrev = default_abbrev; /* one more abbrev length is needed for the boundary commit */ @@ -2357,13 +2358,13 @@ parse_done: if (cmd_is_annotate) { output_option |= OUTPUT_ANNOTATE_COMPAT; - blame_date_mode = DATE_ISO8601; + blame_date_mode.format = DATE_ISO8601; } else { blame_date_mode = revs.date_mode; } /* The maximum width used to show the dates */ - switch (blame_date_mode) { + switch (blame_date_mode.format) { case DATE_RFC2822: blame_date_width = sizeof("Thu, 19 Oct 2006 16:00:04 -0700"); break; @@ -2376,9 +2377,7 @@ parse_done: case DATE_SHORT: blame_date_width = sizeof("2006-10-19"); break; - case DATE_RELATIVE: - case DATE_LOCAL: - case DATE_DEFAULT: + default: blame_date_width = sizeof("Thu Oct 19 16:00:04 2006 -0700"); break; } diff --git a/builtin/cat-file.c b/builtin/cat-file.c index 94632db..2252c83 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -46,6 +46,7 @@ static void pprint_tag(const unsigned char *sha1, const char *buf, unsigned long char *ep; unsigned long date; long tz; + struct date_mode d = INIT_DATE_MODE; while (sp < cp && *sp != '>') sp++; if (sp == cp) { @@ -60,7 +61,7 @@ static void pprint_tag(const unsigned char *sha1, const char *buf, unsigned long write_or_die(1, tagger, sp - tagger); date = strtoul(sp, &ep, 10); tz = strtol(ep, NULL, 10); - sp = show_date(date, tz, 0); + sp = show_date(date, tz, d); write_or_die(1, sp, strlen(sp)); xwrite(1, "\n", 1); break; diff --git a/builtin/commit.c b/builtin/commit.c index 194db99..79343c4 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -933,7 +933,7 @@ static const char *find_author_by_nickname(const char *name) commit = get_revision(&revs); if (commit) { struct pretty_print_context ctx = {0}; - ctx.date_mode = DATE_DEFAULT; + SET_DATE_MODE_DEFAULT(ctx.date_mode); strbuf_release(&buf); format_commit_message(commit, "%an <%ae>", &buf, &ctx); return strbuf_detach(&buf, NULL); 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 @@ -118,9 +118,8 @@ static int parse_atom(const char *atom, const char *ep) * shouldn't be used for checking against the valid_atom * table. */ - const char *formatp = strchr(sp, ':'); - if (!formatp || ep < formatp) - formatp = ep; + const char *formatp = sp; + for (; formatp < ep && *formatp != ':' && *formatp != '@'; ++formatp); if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len)) break; } @@ -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; \ + } \ + } while (*++atomname); \ + date_mode_member_0 = parser_0(spec_0); \ + goto finish; \ + } + +static void parse_date_mode(const char *atomname, struct date_mode *date_mode) +{ + static int need_to_warn = 1; + + do { + /* :date_format@time_zone */ + PARSE_DATE_MODE(':', parse_date_format, date_mode->format, + '@', parse_time_zone, date_mode->zone); + + /* @time_zone:date_format */ + PARSE_DATE_MODE('@', parse_time_zone, date_mode->zone, + ':', parse_date_format, date_mode->format); + + } while (*++atomname); + +finish: + + if (sanitize_date_mode(NULL, date_mode) && need_to_warn) { + need_to_warn = 0; + warning("The date format `:local' is deprecated; " + "use the time zone specifier `@local' instead."); + } +} + +#undef PARSE_DATE_MODE + static void grab_date(const char *buf, struct atom_value *v, const char *atomname) { const char *eoemail = strstr(buf, "> "); char *zone; unsigned long timestamp; long tz; - enum date_mode date_mode = DATE_DEFAULT; - const char *formatp; + struct date_mode date_mode = INIT_DATE_MODE; - /* - * We got here because atomname ends in "date" or "date<something>"; - * it's not possible that <something> is not ":<format>" because - * parse_atom() wouldn't have allowed it, so we can assume that no - * ":" means no format is specified, and use the default. - */ - formatp = strchr(atomname, ':'); - if (formatp != NULL) { - formatp++; - date_mode = parse_date_format(formatp); - } + parse_date_mode(atomname, &date_mode); if (!eoemail) goto bad; diff --git a/builtin/log.c b/builtin/log.c index d43ad3a..fd6b212 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -20,8 +20,7 @@ #include "string-list.h" #include "parse-options.h" -/* Set a default date-time format for git log ("log.date" config variable) */ -static const char *default_date_mode = NULL; +static struct date_mode default_date_mode = INIT_DATE_MODE; static int default_show_root = 1; static int decoration_style; @@ -61,8 +60,7 @@ static void cmd_log_init_defaults(struct rev_info *rev) rev->subject_prefix = fmt_patch_subject_prefix; DIFF_OPT_SET(&rev->diffopt, ALLOW_TEXTCONV); - if (default_date_mode) - rev->date_mode = parse_date_format(default_date_mode); + rev->date_mode = default_date_mode; } static void cmd_log_init_finish(int argc, const char **argv, const char *prefix, @@ -79,6 +77,8 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix, usage(builtin_log_usage); argc = setup_revisions(argc, argv, rev, opt); + sanitize_date_mode("log", &rev->date_mode); + memset(&w, 0, sizeof(w)); userformat_find_requirements(NULL, &w); @@ -305,12 +305,14 @@ static int cmd_log_walk(struct rev_info *rev) static int git_log_config(const char *var, const char *value, void *cb) { + int ret; + if (!strcmp(var, "format.pretty")) return git_config_string(&fmt_pretty, var, value); if (!strcmp(var, "format.subjectprefix")) return git_config_string(&fmt_patch_subject_prefix, var, value); - if (!strcmp(var, "log.date")) - return git_config_string(&default_date_mode, var, value); + if (parse_date_mode_config("log", var, value, &default_date_mode, &ret)) + return ret; if (!strcmp(var, "log.decorate")) { decoration_style = parse_decoration_style(var, value); if (decoration_style < 0) @@ -748,6 +750,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout, struct diff_options opts; int need_8bit_cte = 0; struct commit *commit = NULL; + struct date_mode date_mode = INIT_DATE_MODE_FORMAT(DATE_RFC2822); if (rev->commit_format != CMIT_FMT_EMAIL) die(_("Cover letter needs email format")); @@ -787,7 +790,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout, need_8bit_cte = 1; msg = body; - pp_user_info(NULL, CMIT_FMT_EMAIL, &sb, committer, DATE_RFC2822, + pp_user_info(NULL, CMIT_FMT_EMAIL, &sb, committer, date_mode, encoding); pp_title_line(CMIT_FMT_EMAIL, &msg, &sb, subject_start, extra_headers, encoding, need_8bit_cte); diff --git a/builtin/shortlog.c b/builtin/shortlog.c index 5815f55..8560847 100644 --- a/builtin/shortlog.c +++ b/builtin/shortlog.c @@ -165,7 +165,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit) ctx.abbrev = log->abbrev; ctx.subject = ""; ctx.after_subject = ""; - ctx.date_mode = DATE_DEFAULT; + SET_DATE_MODE_DEFAULT(ctx.date_mode); pretty_print_commit(CMIT_FMT_USERFORMAT, commit, &ufbuf, &ctx); buffer = ufbuf.buf; } else if (*buffer) { diff --git a/builtin/show-branch.c b/builtin/show-branch.c index 1abcd9e..4981db2 100644 --- a/builtin/show-branch.c +++ b/builtin/show-branch.c @@ -765,6 +765,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix) const char *msg; unsigned long timestamp; int tz; + struct date_mode d = INIT_DATE_MODE_FORMAT(DATE_RELATIVE); if (read_ref_at(ref, 0, base+i, sha1, &logmsg, ×tamp, &tz, NULL)) { @@ -778,7 +779,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix) msg++; m = xmalloc(strlen(msg) + 200); sprintf(m, "(%s) %s", - show_date(timestamp, tz, 1), + show_date(timestamp, tz, d), msg); reflog_msg[i] = m; free(logmsg); diff --git a/cache.h b/cache.h index bf639f7..910eccf 100644 --- a/cache.h +++ b/cache.h @@ -828,17 +828,47 @@ extern void *read_object_with_reference(const unsigned char *sha1, extern struct object *peel_to_type(const char *name, int namelen, struct object *o, enum object_type); -enum date_mode { +enum date_format { DATE_DEFAULT = 0, DATE_RELATIVE, DATE_SHORT, - DATE_LOCAL, + DATE_LOCAL, /* Deprecated; use TIME_ZONE_LOCAL */ DATE_ISO8601, DATE_RFC2822, DATE_RAW }; -const char *show_date(unsigned long time, int timezone, enum date_mode mode); +enum time_zone { + TIME_ZONE_DEFAULT = 0, + TIME_ZONE_LOCAL +}; + +struct date_mode { + enum date_format format; + enum time_zone zone; +}; + +#define INIT_DATE_MODE_FORMAT_AND_ZONE(m,z) { .format = m, .zone = z } +#define INIT_DATE_MODE_FORMAT(m) INIT_DATE_MODE_FORMAT_AND_ZONE(m, TIME_ZONE_DEFAULT) +#define INIT_DATE_MODE_ZONE(z) INIT_DATE_MODE_FORMAT_AND_ZONE(DATE_DEFAULT, z) +#define INIT_DATE_MODE INIT_DATE_MODE_FORMAT_AND_ZONE(DATE_DEFAULT, TIME_ZONE_DEFAULT) +#define SET_DATE_MODE_DEFAULT(date_mode) \ + date_mode.format = DATE_DEFAULT; \ + date_mode.zone = TIME_ZONE_DEFAULT; + +int parse_date_mode_config_internal(const char *var_date, + const char *var_time_zone, + const char *var, + const char *value, + struct date_mode *d, + int *ret); + +#define parse_date_mode_config(command_name, var, value, date_mode, ret) \ + parse_date_mode_config_internal(command_name ".date", command_name ".timezone", var, value, date_mode, ret) + +int sanitize_date_mode(const char* command_name, struct date_mode *d); + +const char *show_date(unsigned long time, int timezone, struct date_mode date_mode); const char *show_date_relative(unsigned long time, int tz, const struct timeval *now, char *timebuf, @@ -849,7 +879,8 @@ void datestamp(char *buf, int bufsize); #define approxidate(s) approxidate_careful((s), NULL) unsigned long approxidate_careful(const char *, int *); unsigned long approxidate_relative(const char *date, const struct timeval *now); -enum date_mode parse_date_format(const char *format); +enum date_format parse_date_format(const char *format); +enum time_zone parse_time_zone(const char *time_zone); #define IDENT_WARN_ON_NO_NAME 1 #define IDENT_ERROR_ON_NO_NAME 2 diff --git a/commit.h b/commit.h index 4198513..04b03d1 100644 --- a/commit.h +++ b/commit.h @@ -72,7 +72,7 @@ struct pretty_print_context { int abbrev; const char *subject; const char *after_subject; - enum date_mode date_mode; + struct date_mode date_mode; int need_8bit_cte; int show_notes; struct reflog_walk_info *reflog_info; @@ -98,7 +98,7 @@ extern void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit, struct strbuf *sb, const struct pretty_print_context *context); void pp_user_info(const char *what, enum cmit_fmt fmt, struct strbuf *sb, - const char *line, enum date_mode dmode, + const char *line, struct date_mode dmode, const char *encoding); void pp_title_line(enum cmit_fmt fmt, const char **msg_p, 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) { struct tm *tm; static char timebuf[200]; - if (mode == DATE_RAW) { + enum date_format format = date_mode.format; + + if (format == DATE_RAW) { snprintf(timebuf, sizeof(timebuf), "%lu %+05d", time, tz); return timebuf; } - if (mode == DATE_RELATIVE) { + if (format == DATE_RELATIVE) { struct timeval now; gettimeofday(&now, NULL); return show_date_relative(time, tz, &now, timebuf, sizeof(timebuf)); } - if (mode == DATE_LOCAL) + if (date_mode.zone == TIME_ZONE_LOCAL) tz = local_tzoffset(time); tm = time_to_tm(time, tz); if (!tm) return NULL; - if (mode == DATE_SHORT) + if (format == DATE_SHORT) sprintf(timebuf, "%04d-%02d-%02d", tm->tm_year + 1900, tm->tm_mon + 1, tm->tm_mday); - else if (mode == DATE_ISO8601) + else if (format == DATE_ISO8601) sprintf(timebuf, "%04d-%02d-%02d %02d:%02d:%02d %+05d", tm->tm_year + 1900, tm->tm_mon + 1, tm->tm_mday, tm->tm_hour, tm->tm_min, tm->tm_sec, tz); - else if (mode == DATE_RFC2822) + else if (format == DATE_RFC2822) sprintf(timebuf, "%.3s, %d %.3s %d %02d:%02d:%02d %+05d", weekday_names[tm->tm_wday], tm->tm_mday, month_names[tm->tm_mon], tm->tm_year + 1900, @@ -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); +} + +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) +{ + if (!strcmp(var, var_date)) { + if (!value) + *ret = config_error_nonbool(var); + + d->format = parse_date_format(value); + + return 1; + } + + if (!strcmp(var, var_timezone)) { + if (!value) + *ret = config_error_nonbool(var); + + d->zone = parse_time_zone(value); + + return 1; + } + + return 0; +} + +int sanitize_date_mode(const char* command_name, struct date_mode *d) +{ + if (d->format == DATE_LOCAL) { + if (command_name) + warning("The following are now deprecated:\n\n" + " %s.date = local\n" + " --date=local\n\n" + "Use these instead:\n\n" + " %s.timezone = local\n" + " --time-zone=local\n\n" + "Now assuming you meant:\n\n" + " --date=default --time-zone=local", + command_name, command_name); + + d->format = DATE_DEFAULT; + d->zone = TIME_ZONE_LOCAL; + + return 1; + } + + return 0; +} + void datestamp(char *buf, int bufsize) { time_t now; diff --git a/fast-import.c b/fast-import.c index 3e4e655..bd5c113 100644 --- a/fast-import.c +++ b/fast-import.c @@ -401,6 +401,8 @@ static void write_crash_report(const char *err) unsigned long lu; struct recent_command *rc; + struct date_mode date_mode = INIT_DATE_MODE_ZONE(TIME_ZONE_LOCAL); + if (!rpt) { error("can't write crash report %s: %s", loc, strerror(errno)); return; @@ -411,7 +413,7 @@ static void write_crash_report(const char *err) fprintf(rpt, "fast-import crash report:\n"); fprintf(rpt, " fast-import process: %"PRIuMAX"\n", (uintmax_t) getpid()); fprintf(rpt, " parent process : %"PRIuMAX"\n", (uintmax_t) getppid()); - fprintf(rpt, " at %s\n", show_date(time(NULL), 0, DATE_LOCAL)); + fprintf(rpt, " at %s\n", show_date(time(NULL), 0, date_mode)); fputc('\n', rpt); fputs("fatal: ", rpt); diff --git a/http-backend.c b/http-backend.c index 8501504..15794c9 100644 --- a/http-backend.c +++ b/http-backend.c @@ -89,7 +89,8 @@ static void hdr_int(const char *name, uintmax_t value) static void hdr_date(const char *name, unsigned long when) { - const char *value = show_date(when, 0, DATE_RFC2822); + struct date_mode d = INIT_DATE_MODE_FORMAT(DATE_RFC2822); + const char *value = show_date(when, 0, d); hdr_str(name, value); } diff --git a/log-tree.c b/log-tree.c index b059446..3a6e110 100644 --- a/log-tree.c +++ b/log-tree.c @@ -272,7 +272,7 @@ void get_patch_filename(struct commit *commit, int nr, const char *suffix, if (commit) { int max_len = start_len + FORMAT_PATCH_NAME_MAX - suffix_len; struct pretty_print_context ctx = {0}; - ctx.date_mode = DATE_DEFAULT; + SET_DATE_MODE_DEFAULT(ctx.date_mode); format_commit_message(commit, "%f", buf, &ctx); if (max_len < buf->len) @@ -463,11 +463,13 @@ void show_log(struct rev_info *opt) * so we don't need to worry about printing the * graph info here. */ + + if (!opt->date_format_from_command_line) + opt->date_mode.format = DATE_DEFAULT; + show_reflog_message(opt->reflog_info, opt->commit_format == CMIT_FMT_ONELINE, - opt->date_mode_from_command_line ? - opt->date_mode : - DATE_DEFAULT); + opt->date_mode); if (opt->commit_format == CMIT_FMT_ONELINE) return; } diff --git a/pretty.c b/pretty.c index e1d8a8f..d702575 100644 --- a/pretty.c +++ b/pretty.c @@ -267,7 +267,7 @@ needquote: } void pp_user_info(const char *what, enum cmit_fmt fmt, struct strbuf *sb, - const char *line, enum date_mode dmode, + const char *line, struct date_mode dmode, const char *encoding) { char *date; @@ -306,7 +306,8 @@ void pp_user_info(const char *what, enum cmit_fmt fmt, struct strbuf *sb, strbuf_addf(sb, "Date: %s\n", show_date(time, tz, dmode)); break; case CMIT_FMT_EMAIL: - strbuf_addf(sb, "Date: %s\n", show_date(time, tz, DATE_RFC2822)); + dmode.format = DATE_RFC2822; + strbuf_addf(sb, "Date: %s\n", show_date(time, tz, dmode)); break; case CMIT_FMT_FULLER: strbuf_addf(sb, "%sDate: %s\n", what, show_date(time, tz, dmode)); @@ -458,7 +459,7 @@ static int mailmap_name(char *email, int email_len, char *name, int name_len) } static size_t format_person_part(struct strbuf *sb, char part, - const char *msg, int len, enum date_mode dmode) + const char *msg, int len, struct date_mode dmode) { /* currently all placeholders have same length */ const int placeholder_len = 2; @@ -540,13 +541,16 @@ static size_t format_person_part(struct strbuf *sb, char part, strbuf_addstr(sb, show_date(date, tz, dmode)); return placeholder_len; case 'D': /* date, RFC2822 style */ - strbuf_addstr(sb, show_date(date, tz, DATE_RFC2822)); + dmode.format = DATE_RFC2822; + strbuf_addstr(sb, show_date(date, tz, dmode)); return placeholder_len; case 'r': /* date, relative */ - strbuf_addstr(sb, show_date(date, tz, DATE_RELATIVE)); + dmode.format = DATE_RELATIVE; + strbuf_addstr(sb, show_date(date, tz, dmode)); return placeholder_len; case 'i': /* date, ISO 8601 */ - strbuf_addstr(sb, show_date(date, tz, DATE_ISO8601)); + dmode.format = DATE_ISO8601; + strbuf_addstr(sb, show_date(date, tz, dmode)); return placeholder_len; } @@ -1050,7 +1054,7 @@ void format_commit_message(const struct commit *commit, static void pp_header(enum cmit_fmt fmt, int abbrev, - enum date_mode dmode, + struct date_mode dmode, const char *encoding, const struct commit *commit, const char **msg_p, diff --git a/reflog-walk.c b/reflog-walk.c index 5d81d39..e1bc64a 100644 --- a/reflog-walk.c +++ b/reflog-walk.c @@ -243,7 +243,7 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit) void get_reflog_selector(struct strbuf *sb, struct reflog_walk_info *reflog_info, - enum date_mode dmode, + struct date_mode dmode, int shorten) { struct commit_reflog *commit_reflog = reflog_info->last_commit_reflog; @@ -263,7 +263,7 @@ void get_reflog_selector(struct strbuf *sb, } strbuf_addf(sb, "%s@{", printed_ref); - if (commit_reflog->flag || dmode) { + if (commit_reflog->flag || dmode.format) { info = &commit_reflog->reflogs->items[commit_reflog->recno+1]; strbuf_addstr(sb, show_date(info->timestamp, info->tz, dmode)); } else { @@ -292,7 +292,7 @@ void get_reflog_message(struct strbuf *sb, } void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline, - enum date_mode dmode) + struct date_mode dmode) { if (reflog_info && reflog_info->last_commit_reflog) { struct commit_reflog *commit_reflog = reflog_info->last_commit_reflog; diff --git a/reflog-walk.h b/reflog-walk.h index 7bd2cd4..30805cc 100644 --- a/reflog-walk.h +++ b/reflog-walk.h @@ -11,12 +11,12 @@ extern int add_reflog_for_walk(struct reflog_walk_info *info, extern void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit); extern void show_reflog_message(struct reflog_walk_info *info, int, - enum date_mode); + struct date_mode); extern void get_reflog_message(struct strbuf *sb, struct reflog_walk_info *reflog_info); extern void get_reflog_selector(struct strbuf *sb, struct reflog_walk_info *reflog_info, - enum date_mode dmode, + struct date_mode dmode, int shorten); #endif diff --git a/refs.c b/refs.c index e3c0511..75ad7e6 100644 --- a/refs.c +++ b/refs.c @@ -1633,8 +1633,9 @@ int read_ref_at(const char *ref, unsigned long at_time, int cnt, unsigned char * if (get_sha1_hex(rec + 41, sha1)) die("Log %s is corrupt.", logfile); if (hashcmp(logged_sha1, sha1)) { + struct date_mode d = INIT_DATE_MODE_FORMAT(DATE_RFC2822); warning("Log %s has gap after %s.", - logfile, show_date(date, tz, DATE_RFC2822)); + logfile, show_date(date, tz, d)); } } else if (date == at_time) { @@ -1645,8 +1646,9 @@ int read_ref_at(const char *ref, unsigned long at_time, int cnt, unsigned char * if (get_sha1_hex(rec + 41, logged_sha1)) die("Log %s is corrupt.", logfile); if (hashcmp(logged_sha1, sha1)) { + struct date_mode d = INIT_DATE_MODE_FORMAT(DATE_RFC2822); warning("Log %s unexpectedly ended on %s.", - logfile, show_date(date, tz, DATE_RFC2822)); + logfile, show_date(date, tz, d)); } } munmap(log_mapped, mapsz); diff --git a/revision.c b/revision.c index 462c311..fd87c67 100644 --- a/revision.c +++ b/revision.c @@ -1433,11 +1433,14 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg } else if (!strcmp(arg, "--full-history")) { revs->simplify_history = 0; } else if (!strcmp(arg, "--relative-date")) { - revs->date_mode = DATE_RELATIVE; - revs->date_mode_from_command_line = 1; + revs->date_mode.format = DATE_RELATIVE; + revs->date_format_from_command_line = 1; } else if ((argcount = parse_long_opt("date", argv, &optarg))) { - revs->date_mode = parse_date_format(optarg); - revs->date_mode_from_command_line = 1; + revs->date_mode.format = parse_date_format(optarg); + revs->date_format_from_command_line = 1; + return argcount; + } else if ((argcount = parse_long_opt("time-zone", argv, &optarg))) { + revs->date_mode.zone = parse_time_zone(optarg); return argcount; } else if (!strcmp(arg, "--log-size")) { revs->show_log_size = 1; diff --git a/revision.h b/revision.h index e5ca939..0ced0fb 100644 --- a/revision.h +++ b/revision.h @@ -92,10 +92,10 @@ struct rev_info { abbrev_commit:1, use_terminator:1, missing_newline:1, - date_mode_from_command_line:1; + date_format_from_command_line:1; unsigned int disable_stdin:1; - enum date_mode date_mode; + struct date_mode date_mode; unsigned int abbrev; enum cmit_fmt commit_format; diff --git a/sha1_name.c b/sha1_name.c index 69cd6c8..718093c 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -428,9 +428,12 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1) if (read_ref_at(real_ref, at_time, nth, sha1, NULL, &co_time, &co_tz, &co_cnt)) { if (at_time) + { + struct date_mode d = INIT_DATE_MODE_FORMAT(DATE_RFC2822); warning("Log for '%.*s' only goes " "back to %s.", len, str, - show_date(co_time, co_tz, DATE_RFC2822)); + show_date(co_time, co_tz, d)); + } else { free(real_ref); die("Log for '%.*s' only has %d entries.", diff --git a/submodule.c b/submodule.c index 0a45da9..f61a0a2 100644 --- a/submodule.c +++ b/submodule.c @@ -605,7 +605,7 @@ static void print_commit(struct commit *commit) { struct strbuf sb = STRBUF_INIT; struct pretty_print_context ctx = {0}; - ctx.date_mode = DATE_DEFAULT; + SET_DATE_MODE_DEFAULT(ctx.date_mode); format_commit_message(commit, " %h: %m %s", &sb, &ctx); fprintf(stderr, "%s\n", sb.buf); strbuf_release(&sb); diff --git a/test-date.c b/test-date.c index 6bcd5b0..6d49c2d 100644 --- a/test-date.c +++ b/test-date.c @@ -26,8 +26,11 @@ static void parse_dates(char **argv, struct timeval *now) result[0] = 0; parse_date(*argv, result, sizeof(result)); if (sscanf(result, "%lu %d", &t, &tz) == 2) + { + struct date_mode d = INIT_DATE_MODE_FORMAT(DATE_ISO8601); printf("%s -> %s\n", - *argv, show_date(t, tz, DATE_ISO8601)); + *argv, show_date(t, tz, d)); + } else printf("%s -> bad\n", *argv); } @@ -36,9 +39,9 @@ static void parse_dates(char **argv, struct timeval *now) static void parse_approxidate(char **argv, struct timeval *now) { for (; *argv; argv++) { - time_t t; - t = approxidate_relative(*argv, now); - printf("%s -> %s\n", *argv, show_date(t, 0, DATE_ISO8601)); + time_t t = approxidate_relative(*argv, now); + struct date_mode d = INIT_DATE_MODE_FORMAT(DATE_ISO8601); + printf("%s -> %s\n", *argv, show_date(t, 0, d)); } } -- 1.7.4.18.g68fe8 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [RFC 3/5] Date Mode: Implementation 2011-04-20 2:45 ` [RFC 3/5] Date Mode: Implementation Michael Witten @ 2011-04-21 22:44 ` Junio C Hamano 0 siblings, 0 replies; 37+ messages in thread From: Junio C Hamano @ 2011-04-21 22:44 UTC (permalink / raw) To: Michael Witten; +Cc: git Michael Witten <mfwitten@gmail.com> 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC 4/5] Date Mode: Documentation 2011-04-20 2:53 [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local Michael Witten ` (2 preceding siblings ...) 2011-04-20 2:45 ` [RFC 3/5] Date Mode: Implementation Michael Witten @ 2011-04-20 2:45 ` Michael Witten 2011-04-20 2:45 ` [RFC 5/5] Date Mode: Tests Michael Witten 2011-04-20 6:43 ` [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local Jeff King 5 siblings, 0 replies; 37+ messages in thread From: Michael Witten @ 2011-04-20 2:45 UTC (permalink / raw) To: git Date: Tue, 19 Apr 2011 19:06:02 +0000 This commit updates the git documentation to reflect the new date mode time zone features. As a result, the common date mode documentation has been factored out into one file: Documentation/date-mode-docs.txt which is included where necessary (sometimes with attribute reference substitutions); this file is somewhat messy at first glance (AsciiDoc is difficult to tame), but now all of the essential information is centralized and quite modular. Signed-off-by: Michael Witten <mfwitten@gmail.com> --- Documentation/blame-options.txt | 9 +--- Documentation/config.txt | 32 +++++++------ Documentation/date-mode-docs.txt | 87 ++++++++++++++++++++++++++++++++++++ Documentation/git-for-each-ref.txt | 19 +++++++- Documentation/git-log.txt | 43 ------------------ Documentation/git-rev-list.txt | 4 +- Documentation/git-svn.txt | 2 +- Documentation/rev-list-options.txt | 28 +----------- 8 files changed, 128 insertions(+), 96 deletions(-) create mode 100644 Documentation/date-mode-docs.txt diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt index 16e3c68..deb824e 100644 --- a/Documentation/blame-options.txt +++ b/Documentation/blame-options.txt @@ -70,13 +70,8 @@ of lines before or after the line given by <start>. tree copy has the contents of the named file (specify `-` to make the command read from the standard input). ---date <format>:: - The value is one of the following alternatives: - {relative,local,default,iso,rfc,short}. If --date is not - provided, the value of the blame.date config variable is - used. If the blame.date config variable is also not set, the - iso format is used. For more information, See the discussion - of the --date option at linkgit:git-log[1]. +:date mode options: +include::date-mode-docs.txt[] -M|<num>|:: Detect moved or copied lines within a file. When a commit diff --git a/Documentation/config.txt b/Documentation/config.txt index 480dd0a..5bf9366 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -607,6 +607,9 @@ apply.whitespace:: Tells 'git apply' how to handle whitespaces, in the same way as the '--whitespace' option. See linkgit:git-apply[1]. +:date mode config: blame +include::date-mode-docs.txt[] + branch.autosetupmerge:: Tells 'git branch' and 'git checkout' to set up new branches so that linkgit:git-pull[1] will appropriately merge from the @@ -1262,7 +1265,8 @@ i18n.commitEncoding:: i18n.logOutputEncoding:: Character encoding the commit messages are converted to when - running 'git log' and friends. + running 'git log' and friends. Defaults to the value of + `i18n.commitEncoding` if set, UTF-8 otherwise. imap:: The configuration variables in the 'imap' section are described @@ -1300,12 +1304,8 @@ interactive.singlekey:: linkgit:git-add[1]. Note that this setting is silently ignored if portable keystroke input is not available. -log.date:: - Set the default date-time mode for the 'log' command. - Setting a value for log.date is similar to using 'git log''s - `\--date` option. Possible values are `relative`, `local`, - `default`, `iso`, `rfc`, and `short`; see linkgit:git-log[1] - for details. +:date mode config: log +include::date-mode-docs.txt[] log.decorate:: Print out the ref names of any commits that are shown by the log @@ -1315,10 +1315,11 @@ log.decorate:: This is the same as the log commands '--decorate' option. log.showroot:: - If true, the initial commit will be shown as a big creation event. - This is equivalent to a diff against an empty tree. - Tools like linkgit:git-log[1] or linkgit:git-whatchanged[1], which - normally hide the root commit will now show it. True by default. + If `true`, the initial commit will be shown as a big creation event; + this is equivalent to a diff against an empty tree. If `false`, + 'git log' and related commands will not treat the initial commit as + a big creation event. Any root commits in `git log -p` output would + be shown without a diff attached. The default is `true`. mailmap.file:: The location of an augmenting mailmap file. The default @@ -1391,13 +1392,14 @@ notes.displayRef:: exist, but a glob that does not match any refs is silently ignored. + -This setting can be overridden with the `GIT_NOTES_DISPLAY_REF` -environment variable, which must be a colon separated list of refs or -globs. -+ The effective value of "core.notesRef" (possibly overridden by GIT_NOTES_REF) is also implicitly added to the list of refs to be displayed. ++ +This setting can be overridden with the `GIT_NOTES_DISPLAY_REF` +environment variable, which must be a colon separated list of refs or +globs. It can also be overridden with 'git log''s `--notes` option. +Also, this setting can be disabled by using 'git log''s `--no-notes` option. notes.rewrite.<command>:: When rewriting commits with <command> (currently `amend` or diff --git a/Documentation/date-mode-docs.txt b/Documentation/date-mode-docs.txt new file mode 100644 index 0000000..8f07d3f --- /dev/null +++ b/Documentation/date-mode-docs.txt @@ -0,0 +1,87 @@ +ifdef::datemodeoptions[] + +:datemodeoptions!: + +--time-zone=<zone>:: + + Only takes effect for timestamps shown in a human-readable format (such + as when using "--pretty") and when the timestamp is interpreted in terms + of a time zone (for instance, this option has no effect when + `--date=raw` is specified). ++ +The <zone> may be one of the following: ++ +:date mode time zone values: +include::date-mode-docs.txt[] + +--date=<format>:: + + Only takes effect for timestamps shown in a human-readable format (such + as when using "--pretty"). ++ +The <format> may be one of the following: ++ +:date mode format values: +include::date-mode-docs.txt[] + +--relative-date:: + + Synonym for `--date=relative`. + +endif::datemodeoptions[] + +ifdef::datemodetimezonevalues[] +* *default* causes timestamps to be interpreted in terms of the time zone + recorded in the history. +* *local* causes timestamps to be interpreted in terms of the user's + time zone; on POSIX systems, the time zone may be selected + by setting the `TZ` environment variable appropriately + (for example, on GNU systems, the timestamps could be + interpreted in the `+0000` (UTC) time zone by something + like the following: `TZ=:UTC git log --time-zone=local`). + +:datemodetimezonevalues!: +endif::datemodetimezonevalues[] + +ifdef::datemodeformatvalues[] +* *default* shows timestamps in git's original human-friendly format, + e.g. "Sat Jul 20 00:00:00 1985 -0400". +* *relative* shows timestamps relative to the current time, + e.g. "2 hours ago". +* *iso8601* (or *iso*) shows timestamps in ISO 8601 format, + kke.g. "1985-07-20 00:00:00 -0400". +* *rfc2822* (or *rfc*) shows timestamps in RFC 2822 format + (which is used in email messages), + e.g. "Sat, 20 Jul 1985 00:00:00 -0400". +* *short* shows timestamps as only the date (no time) in `YYYY-MM-DD` format, + e.g. "1985-07-20". +* *raw* shows timestamps in the internal git format (as in GNU `date`'s + `"+%s %z"` format; that is, + "<seconds since The Epoch> <time zone in +HHMM format>"), + e.g. "490680000 -0400". + +:datemodeformatvalues!: +endif::datemodeformatvalues[] + +ifdef::datemodeconfig[] +:command: {datemodeconfig} +:datemodeconfig!: + +{command}.timezone:: + Set the default date mode time zone for the '{command}' command. + Setting a value for `{command}.timezone` is similar to using + 'git {command}''s `\--time-zone` option. Possible values are: ++ +:date mode time zone values: +include::date-mode-docs.txt[] + +{command}.date:: + Set the default date mode format for the '{command}' command. + Setting a value for `{command}.date` is similar to using + 'git {command}''s `\--date` option. Possible values are: ++ +:date mode format values: +include::date-mode-docs.txt[] + +:command!: +endif::datemodeconfig[] diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 152e695..a91d2bf 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -114,9 +114,22 @@ the object referred by the ref does not cause an error. It returns an empty string instead. As a special case for the date-type fields, you may specify a format for -the date by adding one of `:default`, `:relative`, `:short`, `:local`, -`:iso8601` or `:rfc2822` to the end of the fieldname; e.g. -`%(taggerdate:relative)`. +the date by adding one of the following to the end of the fieldname +(e.g. `%(taggerdate:rfc)`): + +:date mode format values: +include::date-mode-docs.txt[] + +Similarly, you may specify the time zone in which to interpret the +date by adding one of the following to the end of the fieldname +(e.g. `%(taggerdate@local)`): + +:date mode time zone values: +include::date-mode-docs.txt[] + +Of course, both the format and time zone specifications may be added +at the same time (e.g. `%(taggerdate:rfc@local)` or, equivalently, +`%(taggerdate@local:rfc)`). EXAMPLES diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt index de5c0d3..03e0ae3 100644 --- a/Documentation/git-log.txt +++ b/Documentation/git-log.txt @@ -139,49 +139,6 @@ Discussion include::i18n.txt[] -Configuration -------------- - -See linkgit:git-config[1] for core variables and linkgit:git-diff[1] -for settings related to diff generation. - -format.pretty:: - Default for the `--format` option. (See "PRETTY FORMATS" above.) - Defaults to "medium". - -i18n.logOutputEncoding:: - Encoding to use when displaying logs. (See "Discussion", above.) - Defaults to the value of `i18n.commitEncoding` if set, UTF-8 - otherwise. - -log.date:: - Default format for human-readable dates. (Compare the - `--date` option.) Defaults to "default", which means to write - dates like `Sat May 8 19:35:34 2010 -0500`. - -log.showroot:: - If `false`, 'git log' and related commands will not treat the - initial commit as a big creation event. Any root commits in - `git log -p` output would be shown without a diff attached. - The default is `true`. - -mailmap.file:: - See linkgit:git-shortlog[1]. - -notes.displayRef:: - Which refs, in addition to the default set by `core.notesRef` - or 'GIT_NOTES_REF', to read notes from when showing commit - messages with the 'log' family of commands. See - linkgit:git-notes[1]. -+ -May be an unabbreviated ref name or a glob and may be specified -multiple times. A warning will be issued for refs that do not exist, -but a glob that does not match any refs is silently ignored. -+ -This setting can be disabled by the `--no-notes` option, -overridden by the 'GIT_NOTES_DISPLAY_REF' environment variable, -and overridden by the `--notes=<ref>` option. - GIT --- Part of the linkgit:git[1] suite diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt index 415f4f0..5909913 100644 --- a/Documentation/git-rev-list.txt +++ b/Documentation/git-rev-list.txt @@ -44,7 +44,9 @@ SYNOPSIS [ \--regexp-ignore-case | -i ] [ \--extended-regexp | -E ] [ \--fixed-strings | -F ] - [ \--date=(local|relative|default|iso|rfc|short) ] + [ \--time-zone=<zone> ] + [ \--date=<format> ] + [ \--time-zone=<zone> ] [ [\--objects | \--objects-edge] [ \--unpacked ] ] [ \--pretty | \--header ] [ \--bisect ] diff --git a/Documentation/git-svn.txt b/Documentation/git-svn.txt index 71fc0ae..3e3ef7f 100644 --- a/Documentation/git-svn.txt +++ b/Documentation/git-svn.txt @@ -104,7 +104,7 @@ COMMANDS --localtime;; Store Git commit times in the local timezone instead of UTC. This - makes 'git log' (even without --date=local) show the same times + makes 'git log' (even without --time-zone=local) show the same times that `svn log` would in the local timezone. + This doesn't interfere with interoperating with the Subversion diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt index 73111bb..1494247 100644 --- a/Documentation/rev-list-options.txt +++ b/Documentation/rev-list-options.txt @@ -634,32 +634,8 @@ endif::git-rev-list[] include::pretty-options.txt[] ---relative-date:: - - Synonym for `--date=relative`. - ---date=(relative|local|default|iso|rfc|short|raw):: - - Only takes effect for dates shown in human-readable format, such - as when using "--pretty". `log.date` config variable sets a default - value for log command's --date option. -+ -`--date=relative` shows dates relative to the current time, -e.g. "2 hours ago". -+ -`--date=local` shows timestamps in user's local timezone. -+ -`--date=iso` (or `--date=iso8601`) shows timestamps in ISO 8601 format. -+ -`--date=rfc` (or `--date=rfc2822`) shows timestamps in RFC 2822 -format, often found in E-mail messages. -+ -`--date=short` shows only date but not time, in `YYYY-MM-DD` format. -+ -`--date=raw` shows the date in the internal raw git format `%s %z` format. -+ -`--date=default` shows timestamps in the original timezone -(either committer's or author's). +:date mode options: +include::date-mode-docs.txt[] ifdef::git-rev-list[] --header:: -- 1.7.4.18.g68fe8 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [RFC 5/5] Date Mode: Tests 2011-04-20 2:53 [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local Michael Witten ` (3 preceding siblings ...) 2011-04-20 2:45 ` [RFC 4/5] Date Mode: Documentation Michael Witten @ 2011-04-20 2:45 ` Michael Witten 2011-04-21 22:44 ` Junio C Hamano 2011-04-23 3:59 ` [RFC 5/5] Date Mode: Tests Michael Witten 2011-04-20 6:43 ` [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local Jeff King 5 siblings, 2 replies; 37+ messages in thread From: Michael Witten @ 2011-04-20 2:45 UTC (permalink / raw) To: git Date: Mon, 14 Mar 2011 20:39:44 +0000 This commit introduces infrastructure for testing nearly all combinations of date mode input (only the format `relative' is currently excluded); test scripts source a date mode shell library to set up the environment and define the general date mode testing driver that is then started with a test-specific callback and optional parameters. Care was taken to reduce the amount of runtime overhead, particularly by only updating the values of git configuration variables when necessary. Signed-off-by: Michael Witten <mfwitten@gmail.com> --- t/date-modes-lib.sh | 168 ++++++++++++++++++++++++++++++++++++++++++ t/date-modes-permutator.perl | 94 +++++++++++++++++++++++ t/t4202-log.sh | 3 + t/t6300-for-each-ref.sh | 11 +++ t/t8002-blame.sh | 9 ++ 5 files changed, 285 insertions(+), 0 deletions(-) create mode 100755 t/date-modes-lib.sh create mode 100755 t/date-modes-permutator.perl diff --git a/t/date-modes-lib.sh b/t/date-modes-lib.sh new file mode 100755 index 0000000..5ff6192 --- /dev/null +++ b/t/date-modes-lib.sh @@ -0,0 +1,168 @@ +#!/bin/sh +# +# Copyright (c) 2011 Michael F Witten + +if ! test_have_prereq PERL; then + skip_all='skipping all date mode tests; perl not available' + test_done +fi + +date_mode_file=d.$$ + +test_expect_success 'date mode: prepare reference commit' " + time='2131675200 -0400' + + export GIT_COMMITTER_DATE=\"\$time\" \ + GIT_COMMITTER_NAME=C \ + GIT_COMMITTER_EMAIL=C@test.git \ + GIT_AUTHOR_DATE=\"\$time\" \ + GIT_AUTHOR_NAME=A \ + GIT_AUTHOR_EMAIL=A@test.git + + d=$date_mode_file + + echo > \$d && git add \$d && git commit -am d +" + +date_mode_perm=$TEST_DIRECTORY/date-modes-permutator.perl + +# $1 : Configuration section (the command name being configured, such as +# the `log' in `log.date', etc.). +# +date_mode_unset_configs() +{ + var_t=$1.timezone + var_d=$1.date + + test_expect_success "date mode: unset configs: '$var_t' and '$var_d'" " + git config --unset-all $var_t + git config --unset-all $var_d + ! git config $var_t && + ! git config $var_d + " + + return 0 +} + +# $1 : `section.variable' +# +# stdin : One line that is a value that is suitable for setting "$1"; +# an empty line commands that "$1" be unset. +# +date_mode_set_config() +{ + read -r v || return 1 + + if [ -z "$v" ]; then + test_expect_success \ + "date mode: unset config '$1'" \ + "git config --unset-all '$1';! git config '$1'" + else + test_expect_success \ + "date mode: set config: '$1 = $v'" \ + "git config '$1' '$v'" + fi + + return 0 +} + +date_mode_set_configs() +{ + date_mode_set_config "$1.timezone" && + read -r; # skip `=' line + date_mode_set_config "$1.date" +} + +# $1 : A value that is suitable as a date mode time zone. +# +# stdin : 2 lines: +# +# A value that is suitable as a date mode format. +# +# Expected output from running "$date_mode_test_code" (see below) +# +# If "$1" or the first line is empty, then the corresponding option +# is not used. +# +# Global variable `date_mode_test_code': +# +# Shell code that can accept the arguments "$1" and the value of the +# first line of stdin, and then output a date that is equal to the +# second line of stdin. +# +date_mode_run_test() +{ + read -r format && + read -r expected || return 1 + + test_expect_success "date mode:${1:+ '$1'}${format:+ '$format'}" " + $date_mode_test_code '$1' '$format' > actual && + echo '$expected' > expected && + test_cmp expected actual + " + + return 0 +} + +date_mode_error() +{ + error "date mode: The test is in a confused state; aborting..." +} + +# $1 : Shell code that can accept the following input (empty strings +# represent no specified value): +# +# $1 : A value suitable as a date mode time zone. +# $2 : A value suitable as a date mode format. +# +# and then output a date. +# +# $2 : Configuration section (the command name being configured, such as +# the `log' in `log.date', etc.); this may be the empty string, in +# which case git configuration variables are not tested. +# +# $3 : The date format that is the default; the default is `default'. +# $4 : The date timzezone that is the default; the default is `default'. +# +# stdin : See the "$date_mode_perm" script. +# +date_mode_run_tests() +{ + date_mode_test_code=$1 + + if [ -z "$2" ]; then + + "$PERL_PATH" "$date_mode_perm" '' "$3" "$4" | + while read -r line; do + date_mode_run_test "$line" || date_mode_error + done + + else + date_mode_unset_configs "$2" + + "$PERL_PATH" "$date_mode_perm" config "$3" "$4" | + while read -r line; do + + if [ "$line" = = ]; then + date_mode_set_config "$2.date" && read -r line + elif [ "$line" = - ]; then + date_mode_set_configs "$2" && read -r line + fi && + + date_mode_run_test "$line" || date_mode_error + + done + fi + + return 0 +} + +date_mode_std_args() +{ + echo "_date_mode_std_args '$(echo "$1" | sed "s/'/'\\\\''/g")'"; +} + +_date_mode_std_args() +{ + eval "$1${2:+ --time-zone='$2'}${3:+ --date='$3'}" +} diff --git a/t/date-modes-permutator.perl b/t/date-modes-permutator.perl new file mode 100755 index 0000000..729e3e7 --- /dev/null +++ b/t/date-modes-permutator.perl @@ -0,0 +1,94 @@ +#!/usr/bin/perl +# +# Copyright (c) 2011 Michael F Witten + +use strict; +use warnings; + +# The values expected to be generated by the test +# when the given {zone}{format} is in use; +my %expected = (); + +######### ZONES FORMATS EXPECTED +$expected {default} {default} = 'Mon Jul 20 00:00:00 2037 -0400'; +$expected {default} { '' } = $expected{default}{default}; +$expected {default} {rfc2822} = 'Mon, 20 Jul 2037 00:00:00 -0400'; +$expected {default} { rfc } = $expected{default}{rfc2822}; +$expected {default} {iso8601} = '2037-07-20 00:00:00 -0400'; +$expected {default} { iso } = $expected{default}{iso8601}; +$expected {default} { short } = '2037-07-20'; +$expected {default} { raw } = '2131675200 -0400'; +$expected {default} { local } = 'Mon Jul 20 04:00:00 2037 +0000'; +$expected { '' } = $expected{default}; +$expected { local } {default} = 'Mon Jul 20 04:00:00 2037 +0000'; +$expected { local } { '' } = $expected{local}{default}; +$expected { local } {rfc2822} = 'Mon, 20 Jul 2037 04:00:00 +0000'; +$expected { local } { rfc } = $expected{local}{rfc2822}; +$expected { local } {iso8601} = '2037-07-20 04:00:00 +0000'; +$expected { local } { iso } = $expected{local}{iso8601}; +$expected { local } { short } = '2037-07-20'; +$expected { local } { raw } = '2131675200 -0400'; +$expected { local } { local } = 'Mon Jul 20 04:00:00 2037 +0000'; + +my @zones = keys %expected; +my @formats = keys %{$expected{$zones[0]}}; # Assuming unvarying formats + +my ($config_zone, $config_format); + +sub print_option_sets_and_expected_outputs +{ + foreach my $z (@zones) + { + foreach my $f (@formats) + { + print $z, "\n", $f, "\n"; + print $expected { $z || $config_zone } + { $f || $config_format }, + "\n"; + } + } +} + +sub print_config_sets_and_option_sets_and_expected_outputs +{ + foreach (@zones) + { + $config_zone = $_; + print "-\n", $_, "\n"; + + foreach (@formats) + { + $config_format = $_; + print "=\n", $_, "\n"; + print_option_sets_and_expected_outputs(); + } + } +} + +######################### +#### Start of Output #### +######################### + +if (defined $ARGV[1]) +{ + foreach (@zones) + { + my $e = $expected{$_}; + $e->{''} = $e->{$ARGV[1]}; + } +} + +if (defined $ARGV[2]) +{ + foreach (@formats) { + $expected{''} = $expected{$ARGV[2]}; + } +} + + +if (defined $ARGV[0] && $ARGV[0] eq 'config') { + print_config_sets_and_option_sets_and_expected_outputs(); +} else { + $config_zone = $config_format = ''; + print_option_sets_and_expected_outputs(); +} diff --git a/t/t4202-log.sh b/t/t4202-log.sh index 2fcc31a..cdfea8b 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -463,4 +463,7 @@ test_expect_success 'show added path under "--follow -M"' ' ) ' +. "$TEST_DIRECTORY"/date-modes-lib.sh +date_mode_run_tests "$(date_mode_std_args 'git log -1 --format=%ad')" log + test_done diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 050ed7d..d6ac4dd 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -359,4 +359,15 @@ test_expect_success 'an unusual tag with an incomplete line' ' ' +. "$TEST_DIRECTORY"/date-modes-lib.sh +date_mode_run_tests ' + b() + { + git for-each-ref \ + --count=1 \ + --format="%(authordate${2:+:$2}${1:+@$1})" \ + "$(git symbolic-ref HEAD)" + } + b' + test_done diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh index d3a51e1..f049439 100755 --- a/t/t8002-blame.sh +++ b/t/t8002-blame.sh @@ -11,4 +11,13 @@ test_expect_success 'Blame --show-email works' ' check_count "<A@test.git>" 1 "<B@test.git>" 1 "<B1@test.git>" 1 "<B2@test.git>" 1 "<author@example.com>" 1 "<C@test.git>" 1 "<D@test.git>" 1 ' +. "$TEST_DIRECTORY"/date-modes-lib.sh +date_mode_run_tests "$(date_mode_std_args " + b() + { + git blame -c -L1,1 \"\$@\" '$date_mode_file' | + awk -F'\t' '{print \$3}' + } + b")" blame iso8601 + test_done -- 1.7.4.18.g68fe8 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [RFC 5/5] Date Mode: Tests 2011-04-20 2:45 ` [RFC 5/5] Date Mode: Tests Michael Witten @ 2011-04-21 22:44 ` Junio C Hamano 2011-04-23 3:42 ` Michael Witten 2011-04-23 3:45 ` Time zone option name (Re: [RFC 5/5] Date Mode: Tests) Michael Witten 2011-04-23 3:59 ` [RFC 5/5] Date Mode: Tests Michael Witten 1 sibling, 2 replies; 37+ messages in thread From: Junio C Hamano @ 2011-04-21 22:44 UTC (permalink / raw) To: Michael Witten; +Cc: git I'd like to have part of the tests in this patch at the beginning of the series to document and protect the behaviour of the current --date related options, then your enhancement that adds --zone that lets the users specify the timezone and the format independently, and finally the remainder of this patch as an addition to the test script to document and protect the interaction between the two options (e.g. what happens when none or only one is specified? what happens when conflicting options such as "--date=local --zone=gmt" is given?). Thanks. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 5/5] Date Mode: Tests 2011-04-21 22:44 ` Junio C Hamano @ 2011-04-23 3:42 ` Michael Witten 2011-04-23 5:06 ` Michael Witten 2011-04-23 3:45 ` Time zone option name (Re: [RFC 5/5] Date Mode: Tests) Michael Witten 1 sibling, 1 reply; 37+ messages in thread From: Michael Witten @ 2011-04-23 3:42 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Thu, 21 Apr 2011 15:44:41 -0700, Junio C Hamano wrote: > I'd like to have part of the tests in this patch at the beginning of the > series to document and protect the behaviour of the current --date related > options, then your enhancement that adds --zone that lets the users > specify the timezone and the format independently, and finally the > remainder of this patch as an addition to the test script to document and > protect the interaction between the two options (e.g. what happens when > none or only one is specified? what happens when conflicting options such > as "--date=local --zone=gmt" is given?). I would split it up as requested, but I'm not sure that it's worthwhile; while there is a certain appeal to such a natural progression, my feeling is that the separate patches won't turn out to be usefully smaller (and thus more easily verified, as I'm sure you desire), because every data structure and code path deals with both simultaneously. Moreover, the beauty of your proposed progression is lost on the fact that these patches will be applied so close in time to each other; why bother dealing with solely date mode formats when such a version will almost certainly never be used? Thus, it seems cleaner just to introduce the whole set of tests in one go. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 5/5] Date Mode: Tests 2011-04-23 3:42 ` Michael Witten @ 2011-04-23 5:06 ` Michael Witten 0 siblings, 0 replies; 37+ messages in thread From: Michael Witten @ 2011-04-23 5:06 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Sat, 23 Apr 2011 03:42:24 +0000 > On Thu, 21 Apr 2011 15:44:41 -0700, Junio C Hamano wrote: > >> I'd like to have part of the tests in this patch at the beginning of the >> series to document and protect the behaviour of the current --date related >> options, then your enhancement that adds --zone that lets the users >> specify the timezone and the format independently, and finally the >> remainder of this patch as an addition to the test script to document and >> protect the interaction between the two options (e.g. what happens when >> none or only one is specified? what happens when conflicting options such >> as "--date=local --zone=gmt" is given?). > > I would split it up as requested, but I'm not sure that it's worthwhile; > while there is a certain appeal to such a natural progression, my feeling > is that the separate patches won't turn out to be usefully smaller (and > thus more easily verified, as I'm sure you desire), because every data > structure and code path deals with both simultaneously. Maybe I spoke to soon; let me play around with it. Still, though: > Moreover, the beauty of your proposed progression is lost on the fact that > these patches will be applied so close in time to each other; why bother > dealing with solely date mode formats when such a version will almost > certainly never be used? > > Thus, it seems cleaner just to introduce the whole set of tests in one go. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Time zone option name (Re: [RFC 5/5] Date Mode: Tests) 2011-04-21 22:44 ` Junio C Hamano 2011-04-23 3:42 ` Michael Witten @ 2011-04-23 3:45 ` Michael Witten 2011-04-23 5:27 ` Junio C Hamano 1 sibling, 1 reply; 37+ messages in thread From: Michael Witten @ 2011-04-23 3:45 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Thu, 21 Apr 2011 15:44:41 -0700, Junio C Hamano wrote: > I'd like to have part of the tests in this patch at the beginning of the > series to document and protect the behaviour of the current --date related > options, then your enhancement that adds --zone that lets the users > specify the timezone and the format independently, and finally the > remainder of this patch as an addition to the test script to document and > protect the interaction between the two options (e.g. what happens when > none or only one is specified? what happens when conflicting options such > as "--date=local --zone=gmt" is given?). I'd like to clarify what you intend the new time zone option to be named; my patch series currently uses `--time-zone' (which is admittedly long), but it would appear here that you find `--zone' attractive. My only qualm there is that `zone' is such a rich word; not only is it somewhat incomprehensible out of context, but it also might be useful for something else in the future. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Time zone option name (Re: [RFC 5/5] Date Mode: Tests) 2011-04-23 3:45 ` Time zone option name (Re: [RFC 5/5] Date Mode: Tests) Michael Witten @ 2011-04-23 5:27 ` Junio C Hamano 0 siblings, 0 replies; 37+ messages in thread From: Junio C Hamano @ 2011-04-23 5:27 UTC (permalink / raw) To: Michael Witten; +Cc: git Michael Witten <mfwitten@gmail.com> writes: > I'd like to clarify what you intend the new time zone option to be named; my > patch series currently uses `--time-zone' (which is admittedly long), but it > would appear here that you find `--zone' attractive. Sorry, that was just me being sloppy, writing barely enough for you and readers to be able to tell which one of the two options you used in your patch is what I was referring to. The word "zone" may mean something other than timezone in future versions of git, and I do not want to see us regret this feature squatting on a short and sweet "--zone" option if/when it happens. Your "--time-zone" (or a single-word "--timezone") would be far better. The "--date={local,default,short,...}" option might want to have a longer synonym "--date-format={...ditto...}", as some commands take the value of the timestamp as a parameter to their "--date=" option, but that is minor and definitely should not be part of this series. Thanks. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 5/5] Date Mode: Tests 2011-04-20 2:45 ` [RFC 5/5] Date Mode: Tests Michael Witten 2011-04-21 22:44 ` Junio C Hamano @ 2011-04-23 3:59 ` Michael Witten 1 sibling, 0 replies; 37+ messages in thread From: Michael Witten @ 2011-04-23 3:59 UTC (permalink / raw) To: git On Wed, 20 Apr 2011 02:45:23 +0000, Michael Witten wrote: > + foreach (@formats) { > + $expected{''} = $expected{$ARGV[2]}; > + } Woops! Well, I guess nobody read through this patch (including myself) :-P ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local 2011-04-20 2:53 [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local Michael Witten ` (4 preceding siblings ...) 2011-04-20 2:45 ` [RFC 5/5] Date Mode: Tests Michael Witten @ 2011-04-20 6:43 ` Jeff King 2011-04-20 14:21 ` Michael Witten ` (3 more replies) 5 siblings, 4 replies; 37+ messages in thread From: Jeff King @ 2011-04-20 6:43 UTC (permalink / raw) To: Michael Witten; +Cc: git On Wed, Apr 20, 2011 at 02:53:36AM +0000, Michael Witten wrote: > One of the possible values for a date format is `local', which > specifies that a date should be output as though the date format > were instead `default' but in terms of the user's time zone > instead of the time zone stored by git; clearly, then, `local' > does not really provide just another format, but rather the > combination of 2 specifications: > > * A format for the date (`default') > * A time zone in which to interpret the date (`local', if you will) > > [...] > > This patch series reimplements the original purpose of `--date' by allowing > the time zone mode to be specified independently of the date format > (see the commit message for [2] and the documentation provided by [3]): I think the intent of this series is good. See also this thread from quite a while back: http://article.gmane.org/gmane.comp.version-control.git/112026 > The date format `local' has thus been deprecated, though it is still > supported (with a warning on stderr). I don't know if we need to go that far. We can leave it forever as a historical compatibility feature. Its existence is not really hurting anyone as long as the documentation marks it as deprecated (or doesn't even mention it). > These patches apply cleanly to the current tip of Junio's `next' branch: Why not "master"? Usually we try to develop features independently on top of master, and then merge them. That way topics can graduate independently to master, and it is easier to see which topics are responsible for breakages. If you really need something in another topic (because you are directly building on it, or the merge would just be too painful), then build straight on that topic's commits, not on top of next (and of course tell everyone which topic it's built on). > There are some whitespace warnings (a la `git diff --check'), but I > have reviewed them and personally approve of them; if you think that > appraisal is incorrect, then you don't know what you're talking about :-P All of the warnings I saw are related to indentation with spaces, and it looks like your intent in each case is to line up the opening paren of a function call with a line of arguments below, like: foo(bar, baz bleep, moof); That's fine, style-wise, but the run of spaces should be collapsed into tabs followed by spaces, with each tab representing 8 spaces (some would argue that it should be "one tab for each level of structural indentation, plus spaces to line up the arguments", but I don't find that we tend to follow such a rule in git). -Peff ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local 2011-04-20 6:43 ` [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local Jeff King @ 2011-04-20 14:21 ` Michael Witten 2011-04-21 1:50 ` Junio C Hamano 2011-04-20 14:22 ` Michael Witten ` (2 subsequent siblings) 3 siblings, 1 reply; 37+ messages in thread From: Michael Witten @ 2011-04-20 14:21 UTC (permalink / raw) To: Jeff King; +Cc: git On Wed, 20 Apr 2011 02:43:18 -0400, Jeff King wrote: > On Wed, Apr 20, 2011 at 02:53:36AM +0000, Michael Witten wrote: > >> One of the possible values for a date format is `local', which >> specifies that a date should be output as though the date format >> were instead `default' but in terms of the user's time zone >> instead of the time zone stored by git; clearly, then, `local' >> does not really provide just another format, but rather the >> combination of 2 specifications: >> >> * A format for the date (`default') >> * A time zone in which to interpret the date (`local', if you will) >> >> [...] >> >> This patch series reimplements the original purpose of `--date' by allowing >> the time zone mode to be specified independently of the date format >> (see the commit message for [2] and the documentation provided by [3]): > > I think the intent of this series is good. See also this thread from > quite a while back: > > http://article.gmane.org/gmane.comp.version-control.git/112026 I took a cursory look, but I've spent so much time on this series already that I don't really care what it says; if there are particular concerns that you think I might not have met, then let me know in this discussion :-) In any case, my approach is not only infintely cleaner than some of the stuff I saw in that thread, but it's also complete and working (and it is extensive without being invasive). ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local 2011-04-20 14:21 ` Michael Witten @ 2011-04-21 1:50 ` Junio C Hamano 2011-04-21 2:14 ` Michael Witten 0 siblings, 1 reply; 37+ messages in thread From: Junio C Hamano @ 2011-04-21 1:50 UTC (permalink / raw) To: Michael Witten; +Cc: Jeff King, git Michael Witten <mfwitten@gmail.com> writes: >> I think the intent of this series is good. See also this thread from >> quite a while back: >> >> http://article.gmane.org/gmane.comp.version-control.git/112026 > > I took a cursory look, but I've spent so much time on this series already > that I don't really care what it says... This is not a very good way to motivate somebody who is already tired at the end of the day to reivew the RFC series, I would have to say. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local 2011-04-21 1:50 ` Junio C Hamano @ 2011-04-21 2:14 ` Michael Witten 2011-04-21 3:57 ` Junio C Hamano 0 siblings, 1 reply; 37+ messages in thread From: Michael Witten @ 2011-04-21 2:14 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git On Thu, Apr 21, 2011 at 01:50, Junio C Hamano <gitster@pobox.com> wrote: > Michael Witten <mfwitten@gmail.com> writes: > >>> I think the intent of this series is good. See also this thread from >>> quite a while back: >>> >>> http://article.gmane.org/gmane.comp.version-control.git/112026 >> >> I took a cursory look, but I've spent so much time on this series already >> that I don't really care what it says... > > This is not a very good way to motivate somebody who is already tired at > the end of the day to reivew the RFC series, I would have to say. Then you can save us both time and just apply my series immediately :-P You're taking it wholly out of context; the full sentence is indeed: "I took a cursory look, but I've spent so much time on this series already that I don't really care what it says; if there are particular concerns that you think I might not have met, then let me know in this discussion :-)" Two points, though: * Mentioning another thread is not in any way a review of this current patch series. * I'm passed the phase of worrying about which cases I might have missed; that's why I have submitted my work to the list in order to give less-weary eyes a chance to think about it. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local 2011-04-21 2:14 ` Michael Witten @ 2011-04-21 3:57 ` Junio C Hamano 2011-04-21 4:09 ` Michael Witten 0 siblings, 1 reply; 37+ messages in thread From: Junio C Hamano @ 2011-04-21 3:57 UTC (permalink / raw) To: Michael Witten; +Cc: Jeff King, git Michael Witten <mfwitten@gmail.com> writes: >>> I took a cursory look, but I've spent so much time on this series already >>> that I don't really care what it says... >> >> This is not a very good way to motivate somebody who is already tired at >> the end of the day to reivew the RFC series, I would have to say. > > Then you can save us both time and just apply my series immediately :-P That's not how things work around here. The default is not to apply, unless the goal of the change is worthy and the patch implements that change well. It is submitter's job to convince people that the change is worthy, justify that it is worth reviewers' time to review the patch, and respond to questions and suggestions for improvements. By doing these responsibly, you win support for the particular patchset, and win trust from others in you, which would affect further patches from you. It is not exactly a good way to win support to dump a patchset, to declare that you are beyond the point of rethinking, to say a take-it-or-leave-it, nor to defend your private style that goes against the project's coding style (style is subjective and there is no right or wrong). I know you know all of the above from your ":-P" (and your past patch submissions), but I am writing these down so that people new to the list do not get a wrong impression from this exchange. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local 2011-04-21 3:57 ` Junio C Hamano @ 2011-04-21 4:09 ` Michael Witten 0 siblings, 0 replies; 37+ messages in thread From: Michael Witten @ 2011-04-21 4:09 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git On Thu, Apr 21, 2011 at 03:57, Junio C Hamano <gitster@pobox.com> wrote: > It is not exactly a good way to win support to dump a patchset, to declare > that you are beyond the point of rethinking, to say a take-it-or-leave-it, > nor to defend your private style that goes against the project's coding > style (style is subjective and there is no right or wrong). Well, I wanted to respond here with: "But I did nothing of the sort!" However, you stole my thunder: > I know you know all of the above from your ":-P" (and your past patch > submissions), but I am writing these down so that people new to the list > do not get a wrong impression from this exchange. Firstly, I'm not sure that this exchange has been that bad; perhaps one of us perceives too little or too much when there is just email text. Secondly, don't you think that particular worry is a bit paranoid? ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local 2011-04-20 6:43 ` [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local Jeff King 2011-04-20 14:21 ` Michael Witten @ 2011-04-20 14:22 ` Michael Witten 2011-04-20 14:22 ` Michael Witten 2011-04-20 14:23 ` Michael Witten 3 siblings, 0 replies; 37+ messages in thread From: Michael Witten @ 2011-04-20 14:22 UTC (permalink / raw) To: Jeff King; +Cc: git On Wed, 20 Apr 2011 02:43:18 -0400, Jeff King wrote: > On Wed, Apr 20, 2011 at 02:53:36AM +0000, Michael Witten wrote: > >> The date format `local' has thus been deprecated, though it is still >> supported (with a warning on stderr). > > I don't know if we need to go that far. We can leave it forever as a > historical compatibility feature. Its existence is not really hurting > anyone as long as the documentation marks it as deprecated (or doesn't > even mention it). The references to a `local' *format* have indeed been removed from the documentation, and the warning is just an indication that the code that supports it will also be removed at some point too. However, the warning mechanism (in general) could be improved by producing the message only when exiting the program (after the pager has been destroyed); currently, the message can be quite easy to miss otherwise (depending on the way the terminal handles the buffer history). I don't see the point of leaving crufty code in place forever; it's ridiculous that my modern CPU starts up like a 35 year old chip when the very few people who need that functionality (if anybody) can emulate it if necessary. As an aside, I've always found it amusing that most programmers are adamant about justifying an existing use case for a new feature before even considering an implementation for it, but yet they will gladly keep around dead code until the next Big Bang. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local 2011-04-20 6:43 ` [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local Jeff King 2011-04-20 14:21 ` Michael Witten 2011-04-20 14:22 ` Michael Witten @ 2011-04-20 14:22 ` Michael Witten 2011-04-20 14:23 ` Michael Witten 3 siblings, 0 replies; 37+ messages in thread From: Michael Witten @ 2011-04-20 14:22 UTC (permalink / raw) To: Jeff King; +Cc: git On Wed, 20 Apr 2011 02:43:18 -0400, Jeff King wrote: > On Wed, Apr 20, 2011 at 02:53:36AM +0000, Michael Witten wrote: > >> These patches apply cleanly to the current tip of Junio's `next' branch: > > Why not "master"? Usually we try to develop features independently on > top of master, and then merge them. That way topics can graduate > independently to master, and it is easier to see which topics are > responsible for breakages. If you really need something in another topic > (because you are directly building on it, or the merge would just be too > painful), then build straight on that topic's commits, not on top of > next (and of course tell everyone which topic it's built on). Interesting points. I was building on `master', but I figured I would expedite the testing of my patch by making sure that it applies to the recent tip of `next' without trouble. In fact, when I did rebase onto `next', there were a couple of minor conflicts that I had to resolve, so I can safely say that the current patch series depends upon the following 2 topics: commit 84393bfd731c435120dc1dda63432a70124821cb Author: Namhyung Kim <namhyung@gmail.com> Date: Wed Apr 6 11:20:50 2011 +0900 blame: add --abbrev command line option and make it honor core.abbrev If user sets config.abbrev option, use it as if --abbrev was given. This is the default value and user can override different abbrev length by specifying the --abbrev=N command line option. Signed-off-by: Namhyung Kim <namhyung@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> commit ef803fd4b09bca707c7c27669a2789bb050b488c Author: Michael J Gruber <git@drmicha.warpmail.net> Date: Fri Apr 1 11:20:31 2011 +0200 builtin/log.c: separate default and setup of cmd_log_init() cmd_log_init() sets up some default rev options and then calls setup_revisions(), so that a caller cannot set up own defaults: Either they get overriden by cmd_log_init() (if set before) or they override the command line (if set after). We even complain about this in a comment to cmd_log_reflog(). Therefore, separate the two steps so that one can still call cmd_log_init() or, alternatively, cmd_log_init_defaults() followed by cmd_log_init_finish() (and set defaults in between). No functional change so far. Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net> Signed-off-by: Junio C Hamano <gitster@pobox.com> Those look likely to go into master, so it actually seems like a good idea to have gone ahead and taken care of the conflict. However, in the future, I will send patches to `master' only. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local 2011-04-20 6:43 ` [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local Jeff King ` (2 preceding siblings ...) 2011-04-20 14:22 ` Michael Witten @ 2011-04-20 14:23 ` Michael Witten 2011-04-21 0:07 ` Tabs and spaces (Re: [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local) Jonathan Nieder 3 siblings, 1 reply; 37+ messages in thread From: Michael Witten @ 2011-04-20 14:23 UTC (permalink / raw) To: Jeff King; +Cc: git On Wed, 20 Apr 2011 02:43:18 -0400, Jeff King wrote: >> There are some whitespace warnings (a la `git diff --check'), but I >> have reviewed them and personally approve of them; if you think that >> appraisal is incorrect, then you don't know what you're talking about :-P > > All of the warnings I saw are related to indentation with spaces, and it > looks like your intent in each case is to line up the opening paren of a > function call with a line of arguments below, like: > > foo(bar, baz > bleep, moof); > > That's fine, style-wise, but the run of spaces should be collapsed into > tabs followed by spaces, with each tab representing 8 spaces. I understand this, as I've read: Documentation/CodingGuidelines and I'm also responsible for this massive git flamewar of yore on the same subject: http://article.gmane.org/gmane.comp.version-control.git/61095 Message-ID: 634393B0-734A-4884-93E3-42F7D3CB157F@mit.edu However - and this is the key point - if you are going to be mixing tabs and spaces ANYWAY, then you might as well do it in a way that maintains alignment within a tab level regardless of the current setting for the tabwidth: > (some would argue that it should be "one tab for each level of > structural indentation, plus spaces to line up the arguments", > but I don't find that we tend to follow such a rule in git). That approach is the most logical and the most robust, and if somebody messes it up, then the whitespace simply reduces (or degenerates) back to just the very approach you espouse anyway. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Tabs and spaces (Re: [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local) 2011-04-20 14:23 ` Michael Witten @ 2011-04-21 0:07 ` Jonathan Nieder 2011-04-21 1:51 ` Tabs and spaces Michael Witten 0 siblings, 1 reply; 37+ messages in thread From: Jonathan Nieder @ 2011-04-21 0:07 UTC (permalink / raw) To: Michael Witten; +Cc: Jeff King, git Hi Michael, Michael Witten wrote: > and I'm also responsible for this massive git flamewar of yore on the same > subject: > > http://article.gmane.org/gmane.comp.version-control.git/61095 > Message-ID: 634393B0-734A-4884-93E3-42F7D3CB157F@mit.edu Heh. :) > However - and this is the key point - if you are going to be mixing > tabs and spaces ANYWAY, then you might as well do it in a way that > maintains alignment within a tab level regardless of the current > setting for the tabwidth: In principle, I generally agree. But as mentioned in the thread you reference, most text editors don't make that very easy. I personally use a tabwidth of 6 when I really want to concentrate on reading. When coding in a rush for other people, that leads to using tabstop of 8 and only aligning text that is much shorter than one tab: if (foo && bar && baz && qux && quux) { ... } else if (quuux(quuuux, quuuuux, long_expresion_comes_here(quuuuuux))) { ... } As you can see, this is following the "put continuation lines near the right margin" convention advocated in linux-2.6's Documentation/CodingStyle. Two advantages: - looks sensible with any tabstop - no need to cascade changes on following lines when the width of a function name changes One major disadvantage: - annoys people who like everything nicely lined up. For what it's worth. (Not much, of course --- the best rule is as always to make sure your code fits well with the code around it.) Hope that helps, Jonathan ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Tabs and spaces 2011-04-21 0:07 ` Tabs and spaces (Re: [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local) Jonathan Nieder @ 2011-04-21 1:51 ` Michael Witten 2011-04-21 2:18 ` Jonathan Nieder 0 siblings, 1 reply; 37+ messages in thread From: Michael Witten @ 2011-04-21 1:51 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Jeff King, git On Wed, 20 Apr 2011 19:07:01 -0500, Jonathan Nieder wrote: >> However - and this is the key point - if you are going to be mixing >> tabs and spaces ANYWAY, then you might as well do it in a way that >> maintains alignment within a tab level regardless of the current >> setting for the tabwidth: > > In principle, I generally agree. But as mentioned in the thread you > reference, most text editors don't make that very easy. You just need an editor that can insert into a newly created line the same leading whitespace characters that were on the previous line (of course, it's nice if lines with only whitespace are automatically converted to empty lines); if your text editor can't do that, then any style of indentation/alignment is going to be painful. > I personally use a tabwidth of 6 when I really want to concentrate on > reading. When coding in a rush for other people, that leads to using > tabstop of 8 and only aligning text that is much shorter than one tab: > > if (foo && bar && baz && > qux && quux) { > ... > } else if (quuux(quuuux, quuuuux, > long_expresion_comes_here(quuuuuux))) { > ... > } > > As you can see, this is following the "put continuation lines near the > right margin" convention advocated in linux-2.6's > Documentation/CodingStyle. It's not hard to see why "near the right margin" is an important remark; with a tabstop of 8, your code is rendered like this: if (foo && bar && baz && qux && quux) { ... } else if (quuux(quuuux, quuuuux, long_expresion_comes_here(quuuuuux))) { ... } However, with a tabstop of 2, it looks like this: if (foo && bar && baz && qux && quux) { ... } else if (quuux(quuuux, quuuuux, long_expresion_comes_here(quuuuuux))) { ... } Notice that your `qux && quux' line (which follows the method we both like) remained nicely aligned, but your `long_expression_comes_here' line has moved back so much that it doesn't even satisfy the `right margin' rule anymore. > Two advantages: > > - looks sensible with any tabstop As we have seen, the sensibility of using tabs for *alignment* is precarious; on the contrary, using spaces for alignment [across lines of the same level of tabular indentation] remains *exactly* as sensible as intended by the original author, regardless of the tabstop setting. The problem is that there are really two issues that are being conflated" * Indentation : This can be implemented with tabs * Alignment : This should be implemented with only spaces > - no need to cascade changes on following lines when the width of a > function name changes That is indeed the best reason to avoid alignment (after all, by its very nature, alignment implies a coupling between lines), and I find myself in constant intellectual conflict over that very issue. However, if the number of lines to be aligned is rather small or alignment obviously helps readability, then it's a rather minor risk. Now, your example of a changing function name is a good one; for instance, consider one of the functions that my patch series introduces: int parse_date_mode_config_internal(const char *var_date, const char *var_time_zone, const char *var, const char *value, struct date_mode *d, int *ret); I just noticed that the parameters are misaligned by one space; it should be the following: int parse_date_mode_config_internal(const char *var_date, const char *var_time_zone, const char *var, const char *value, struct date_mode *d, int *ret); I probably changed the function name by 1 character at some point and then forgot to update the alignment of the parameters. :-( Now, I actually hate that style of continuation, and I only used it because that's basically what everybody else on the planet (and in the git project) tries to do (albeit with some apalling combination of tabs and spaces). In such a case, I would much rather treat parameters as `subelements' of the function construct (prototype, call, etc.) by explicitly giving them their own indentation level; this transforms the problem from one of indentation and alignment to a problem of just indentation, thereby allowing for nothing but tabs: int parse_date_mode_config_internal ( const char *var_date, const char *var_time_zone, const char *var, const char *value, struct date_mode *d, int *ret ); A function call might look like this: parse_date_mode_config_internal ( "Some value of some sort", "Another value of some sort", "dingleberry", d, ret ); Of course, this opens up the debate about where the opening parenthesis should go; if the `{' of a block is routinely put on the same line as, say, an `if', then perhaps this would look more natural: parse_date_mode_config_internal ( "Some value of some sort", "Another value of some sort", "dingleberry", date_mode, return_value ); An `if' statement is kind of gross to mold in this way due to the fact that there are two parts, each with its own subelements; at some point, you might as well just make some temporary variables with reasonably short names in order to hold the condition value, thereby obviating this nightmare in the first place. However, if you must write every expression explicitly in place, then your example could be written thusly: if ( foo && bar && baz && qux && quux ){ ... } else if ( quuux ( quuuux, quuuuux, long_expresion_comes_here ( quuuuuux ) ) ){ ... } or more drawn out: if ( foo && bar && baz && qux && quux ) { ... } else if ( quuux ( quuuux, quuuuux, long_expresion_comes_here ( quuuuuux ) ) ) { ... } Again, it should be recognized that there are 2 issues: * Indentation : This can be implemented with tabs * Alignment : This should be implemented with only spaces and it should also be recognized that treating tabs as a primitive means of space-saving compression by having it always represent some constant number of spaces (8) is just as flaky as expecting people to properly use spaces for alignment; thus you might as well go for the latter in order to get the best of both worlds, because somebody is going to mess it up either way. Sincerely, Michael Witten ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Tabs and spaces 2011-04-21 1:51 ` Tabs and spaces Michael Witten @ 2011-04-21 2:18 ` Jonathan Nieder 2011-04-21 3:15 ` Michael Witten 0 siblings, 1 reply; 37+ messages in thread From: Jonathan Nieder @ 2011-04-21 2:18 UTC (permalink / raw) To: Michael Witten; +Cc: Jeff King, git Michael Witten wrote: > You just need an editor that [...] which many contributors don't have (or haven't configured that way). Really. [...] > However, with a tabstop of 2, it looks like this: > > if (foo && bar && baz && > qux && quux) { > ... > } else if (quuux(quuuux, quuuuux, > long_expresion_comes_here(quuuuuux))) { > ... > } > > Notice that your `qux && quux' line (which follows the method we both like) > remained nicely aligned, but your `long_expression_comes_here' line has > moved back so much that it doesn't even satisfy the `right margin' rule > anymore. Oh, I still think a tabstop of 2 is insane (for various reasons, some explained in the thread you mentioned). No matter --- Junio will use --whitespace=fixup to convert your spaces to tabs anyway. Just thought I could give some survival tips. Regards, Jonathan ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Tabs and spaces 2011-04-21 2:18 ` Jonathan Nieder @ 2011-04-21 3:15 ` Michael Witten 2011-04-21 3:25 ` Thiago Farina 0 siblings, 1 reply; 37+ messages in thread From: Michael Witten @ 2011-04-21 3:15 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Jeff King, git On Thu, Apr 21, 2011 at 02:18, Jonathan Nieder <jrnieder@gmail.com> wrote: > Michael Witten wrote: > >> You just need an editor that [...] > > which many contributors don't have (or haven't configured that way). > Really. to which I already said: "if your text editor can't do that, then any style of indentation/alignment is going to be painful." > > [...] >> However, with a tabstop of 2, it looks like this: >> >> if (foo && bar && baz && >> qux && quux) { >> ... >> } else if (quuux(quuuux, quuuuux, >> long_expresion_comes_here(quuuuuux))) { >> ... >> } >> >> Notice that your `qux && quux' line (which follows the method we both like) >> remained nicely aligned, but your `long_expression_comes_here' line has >> moved back so much that it doesn't even satisfy the `right margin' rule >> anymore. > > Oh, I still think a tabstop of 2 is insane (for various reasons, some > explained in the thread you mentioned). That's not really the point, though, is it. > No matter --- Junio will use --whitespace=fixup to convert your spaces > to tabs anway. Good! That means I can be satisfied with having written sane patches, and Junio can be satisfied with having forced the insane use of tabs. It's a win-win :-D ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Tabs and spaces 2011-04-21 3:15 ` Michael Witten @ 2011-04-21 3:25 ` Thiago Farina 2011-04-21 10:46 ` Alex Riesen 0 siblings, 1 reply; 37+ messages in thread From: Thiago Farina @ 2011-04-21 3:25 UTC (permalink / raw) To: Michael Witten; +Cc: Jonathan Nieder, Jeff King, git On Thu, Apr 21, 2011 at 12:15 AM, Michael Witten <mfwitten@gmail.com> wrote: >> Oh, I still think a tabstop of 2 is insane (for various reasons, some >> explained in the thread you mentioned). > tabstop of 2 is what we use on Chromium code (with spaces, no tabs). And works very well. Just my 0.02 cents. ;) ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Tabs and spaces 2011-04-21 3:25 ` Thiago Farina @ 2011-04-21 10:46 ` Alex Riesen 2011-04-21 12:57 ` Michael Witten 0 siblings, 1 reply; 37+ messages in thread From: Alex Riesen @ 2011-04-21 10:46 UTC (permalink / raw) To: Thiago Farina; +Cc: Michael Witten, Jonathan Nieder, Jeff King, git On Thu, Apr 21, 2011 at 05:25, Thiago Farina <tfransosi@gmail.com> wrote: > On Thu, Apr 21, 2011 at 12:15 AM, Michael Witten <mfwitten@gmail.com> wrote: >>> Oh, I still think a tabstop of 2 is insane (for various reasons, some >>> explained in the thread you mentioned). >> > tabstop of 2 is what we use on Chromium code (with spaces, no tabs). It's indentation. You indent your code by 2 space. It has nothing to do with tabstop. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Tabs and spaces 2011-04-21 10:46 ` Alex Riesen @ 2011-04-21 12:57 ` Michael Witten 0 siblings, 0 replies; 37+ messages in thread From: Michael Witten @ 2011-04-21 12:57 UTC (permalink / raw) To: Alex Riesen; +Cc: Thiago Farina, Jonathan Nieder, Jeff King, git On Thu, Apr 21, 2011 at 10:46, Alex Riesen <raa.lkml@gmail.com> wrote: > On Thu, Apr 21, 2011 at 05:25, Thiago Farina <tfransosi@gmail.com> wrote: >> On Thu, Apr 21, 2011 at 12:15 AM, Michael Witten <mfwitten@gmail.com> wrote: >>>> Oh, I still think a tabstop of 2 is insane (for various reasons, some >>>> explained in the thread you mentioned). >>> >> tabstop of 2 is what we use on Chromium code (with spaces, no tabs). > > It's indentation. You indent your code by 2 space. > It has nothing to do with tabstop. In this discussion, it has everything to do with the tabstops (1235e29d-6cbb-445b-9b6f-4e174c03ba8f-mfwitten@gmail.com): > Again, it should be recognized that there are 2 issues: > > * Indentation : This *can* be implemented with tabs > * Alignment : This *should* be implemented with only spaces > > and it should also be recognized that treating tabs as a primitive > means of space-saving compression by having it always represent > some constant number of spaces (8) is just as flaky as expecting > people to properly use spaces for alignment; thus you might as > well go for the latter in order to get the best of both worlds, > because somebody is going to mess it up either way. ^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2011-04-25 18:30 UTC | newest] Thread overview: 37+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-04-20 2:53 [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local Michael Witten 2011-04-20 2:45 ` [RFC 1/5] Light refactoring of date infrastructure Michael Witten 2011-04-20 2:45 ` [RFC 2/5] Pretty Print: show tz when using DATE_LOCAL Michael Witten 2011-04-21 22:34 ` Junio C Hamano 2011-04-22 14:08 ` Dates in Commits and other issues of style (Re: [RFC 2/5] Pretty Print: show tz when using DATE_LOCAL) Michael Witten 2011-04-25 1:26 ` Miles Bader 2011-04-25 3:57 ` Junio C Hamano 2011-04-25 10:45 ` Jakub Narebski 2011-04-25 18:29 ` Junio C Hamano 2011-04-22 14:36 ` [RFC 2/5] Pretty Print: show tz when using DATE_LOCAL Michael Witten 2011-04-22 15:06 ` Junio C Hamano 2011-04-20 2:45 ` [RFC 3/5] Date Mode: Implementation Michael Witten 2011-04-21 22:44 ` Junio C Hamano 2011-04-20 2:45 ` [RFC 4/5] Date Mode: Documentation Michael Witten 2011-04-20 2:45 ` [RFC 5/5] Date Mode: Tests Michael Witten 2011-04-21 22:44 ` Junio C Hamano 2011-04-23 3:42 ` Michael Witten 2011-04-23 5:06 ` Michael Witten 2011-04-23 3:45 ` Time zone option name (Re: [RFC 5/5] Date Mode: Tests) Michael Witten 2011-04-23 5:27 ` Junio C Hamano 2011-04-23 3:59 ` [RFC 5/5] Date Mode: Tests Michael Witten 2011-04-20 6:43 ` [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local Jeff King 2011-04-20 14:21 ` Michael Witten 2011-04-21 1:50 ` Junio C Hamano 2011-04-21 2:14 ` Michael Witten 2011-04-21 3:57 ` Junio C Hamano 2011-04-21 4:09 ` Michael Witten 2011-04-20 14:22 ` Michael Witten 2011-04-20 14:22 ` Michael Witten 2011-04-20 14:23 ` Michael Witten 2011-04-21 0:07 ` Tabs and spaces (Re: [RFC 0/5] Date Mode: Add --time-zone; deprecate --date=local) Jonathan Nieder 2011-04-21 1:51 ` Tabs and spaces Michael Witten 2011-04-21 2:18 ` Jonathan Nieder 2011-04-21 3:15 ` Michael Witten 2011-04-21 3:25 ` Thiago Farina 2011-04-21 10:46 ` Alex Riesen 2011-04-21 12:57 ` Michael Witten
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).