{"thread":{"id":"39722","subject":"several date related issues","startedAt":"2015-06-25T11:19:01Z","lastAt":"2015-07-21T01:19:31Z","messageCount":31,"participants":["H.Merijn Brand","Jeff King","John Keeping","Eric Sunshine","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"264830","messageId":"20150625131901.5b962336@pc09.procura.nl","threadId":"39722","inReplyTo":null,"subject":"several date related issues","fromName":"H.Merijn Brand","fromEmail":"h.m.brand@xs4all.nl","sentAt":"2015-06-25T11:19:01Z","receivedAt":"2015-06-25T11:19:01Z","isPatch":false,"sender":{"key":"h.m.brand@xs4all.nl","avatar":"https://gravatar.com/avatar/5b8f83ee35c427a646cbea3b104346e00ab3663b99bbf435cddeb75cd4b3857b?d=mp&s=160"},"body":"Running 2.4.4\n\n*** Dates do not respect LC_TIME\n\n$ date\nThu 25 Jun 2015 13:02:48 CEST\n$ git log --pretty=fuller --date=local | head -6 | grep -i date\nAuthorDate: Mon Feb 16 16:47:08 2015\nCommitDate: Mon Feb 16 16:47:08 2015\n$ locale -ck LC_TIME | grep fmt\nd_t_fmt=\"%a %d %b %Y %r %Z\"\nd_fmt=\"%d-%m-%Y\"\nt_fmt=\"%r\"\nt_fmt_ampm=\"%H:%M:%S\"\nera_d_fmt=\"\"\nera_d_t_fmt=\"\"\nera_t_fmt=\"\"\ndate_fmt=\"%a %e %b %Y %H:%M:%S %Z\"\n\n*** git log --date-order and --author-date-order do not order by date\n\n$ mkdir git-test\n$ cd git-test\n$ git init\nInitialized empty Git repository in /home/merijn/git-test/.git/\n$ touch foo\n$ git add foo\n$ git commit -m boo\n[master (root-commit) 09483e5] boo\n 1 file changed, 0 insertions(+), 0 deletions(-)\n create mode 100644 foo\n$ git log | cat\ncommit 09483e527d6a4d4a9e49f82a11871ab55133cf72\nAuthor: H.Merijn Brand \nDate: Thu Jun 25 13:14:37 2015 +0200\n\n boo\n$ touch bar\n$ env GIT_AUTHOR_DATE=\"2015-01-15 12:13:14\" GIT_COMMITTER_DATE=\"2015-01-15 12:13:14\" git add bar\n$ env GIT_AUTHOR_DATE=\"2015-01-15 12:13:14\" GIT_COMMITTER_DATE=\"2015-01-15 12:13:14\" git commit -m \"Commit in past\"\n[master 6acc7f3] Commit in past\n 1 file changed, 0 insertions(+), 0 deletions(-)\n create mode 100644 bar\n$ git log | cat\ncommit 6acc7f3d2cbaca7176b63ffac51005ce07b90b91\nAuthor: H.Merijn Brand \nDate: Thu Jan 15 12:13:14 2015 +0100\n\n Commit in past\n\ncommit 09483e527d6a4d4a9e49f82a11871ab55133cf72\nAuthor: H.Merijn Brand \nDate: Thu Jun 25 13:14:37 2015 +0200\n\n boo\n$ git log --date-order | cat\ncommit 6acc7f3d2cbaca7176b63ffac51005ce07b90b91\nAuthor: H.Merijn Brand \nDate: Thu Jan 15 12:13:14 2015 +0100\n\n Commit in past\n\ncommit 09483e527d6a4d4a9e49f82a11871ab55133cf72\nAuthor: H.Merijn Brand \nDate: Thu Jun 25 13:14:37 2015 +0200\n\n boo\n$ git log --author-date-order | cat\ncommit 6acc7f3d2cbaca7176b63ffac51005ce07b90b91\nAuthor: H.Merijn Brand \nDate: Thu Jan 15 12:13:14 2015 +0100\n\n Commit in past\n\ncommit 09483e527d6a4d4a9e49f82a11871ab55133cf72\nAuthor: H.Merijn Brand \nDate: Thu Jun 25 13:14:37 2015 +0200\n\n boo\n$\n\n\n-- \nH.Merijn Brand http://tux.nl Perl Monger http://amsterdam.pm.org/\nusing perl5.00307 .. 5.21 porting perl5 on HP-UX, AIX, and openSUSE\nhttp://mirrors.develooper.com/hpux/ http://www.test-smoke.org/\nhttp://qa.perl.org http://www.goldmark.org/jeff/stupid-disclaimers/\n"},{"id":"264842","messageId":"20150625124445.GA18395@peff.net","threadId":"39722","inReplyTo":"20150625131901.5b962336@pc09.procura.nl","subject":"Re: several date related issues","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-25T12:44:45Z","receivedAt":"2015-06-25T12:44:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 25, 2015 at 01:19:01PM +0200, H.Merijn Brand wrote:\n\n> *** Dates do not respect LC_TIME\n\nRight, we use our own routines for formatting the dates, and not\nstrftime. And it probably should stay that way in general, as git's\noutput is often meant to be parsed.\n\nThat being said, I do not think it would be wrong to have a date-mode\nthat just showed strftime(\"%c\"), or some other arbitrary strftime\nstring. You could then set log.date as appropriate for human\nconsumption.\n\n> *** git log --date-order and --author-date-order do not order by date\n\nCorrect. The documentation says:\n\n --date-order\n Show no parents before all of its children are shown, but otherwise\n show commits in the commit timestamp order.\n\nIn your example, one commit is the parent of the other, so it hits the\nfirst part of the sentence, and the dates are never even compared.\n\nThere is not a simple way to show commits in arbitrary order without\nrespect to parentage. I think you'd have to do something like:\n\n git log --format='%at %H' |\n sort -rn |\n cut -d' ' -f2 |\n git log --stdin --no-walk\n\n-Peff\n"},{"id":"264843","messageId":"20150625145659.09285538@pc09.procura.nl","threadId":"39722","inReplyTo":"20150625124445.GA18395@peff.net","subject":"Re: several date related issues","fromName":"H.Merijn Brand","fromEmail":"h.m.brand@xs4all.nl","sentAt":"2015-06-25T12:56:59Z","receivedAt":"2015-06-25T12:56:59Z","isPatch":false,"sender":{"key":"h.m.brand@xs4all.nl","avatar":"https://gravatar.com/avatar/5b8f83ee35c427a646cbea3b104346e00ab3663b99bbf435cddeb75cd4b3857b?d=mp&s=160"},"body":"On Thu, 25 Jun 2015 08:44:45 -0400, Jeff King wrote:\n\n> On Thu, Jun 25, 2015 at 01:19:01PM +0200, H.Merijn Brand wrote:\n> \n> > *** Dates do not respect LC_TIME\n> \n> Right, we use our own routines for formatting the dates, and not\n> strftime. And it probably should stay that way in general, as git's\n> output is often meant to be parsed.\n> \n> That being said, I do not think it would be wrong to have a date-mode\n> that just showed strftime(\"%c\"), or some other arbitrary strftime\n> string. You could then set log.date as appropriate for human\n> consumption.\n\nYes please :)\n\n --date=lc\n --date=lc_time\n --date=locale\n\nall spring to mind as valid options\n\n> > *** git log --date-order and --author-date-order do not order by date\n> \n> Correct. The documentation says:\n> \n> --date-order\n> Show no parents before all of its children are shown, but otherwise\n> show commits in the commit timestamp order.\n> \n> In your example, one commit is the parent of the other, so it hits the\n> first part of the sentence, and the dates are never even compared.\n\nThat is what I gathered, and concluded that the option name is\nmisleading\n\n> There is not a simple way to show commits in arbitrary order without\n> respect to parentage. I think you'd have to do something like:\n> \n> git log --format='%at %H' |\n> sort -rn |\n> cut -d' ' -f2 |\n> git log --stdin --no-walk\n\nI'd like that as gitk option!\n\n-- \nH.Merijn Brand http://tux.nl Perl Monger http://amsterdam.pm.org/\nusing perl5.00307 .. 5.21 porting perl5 on HP-UX, AIX, and openSUSE\nhttp://mirrors.develooper.com/hpux/ http://www.test-smoke.org/\nhttp://qa.perl.org http://www.goldmark.org/jeff/stupid-disclaimers/\n"},{"id":"264867","messageId":"20150625165341.GA21949@peff.net","threadId":"39722","inReplyTo":"20150625145659.09285538@pc09.procura.nl","subject":"[PATCH 0/3] localized date format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-25T16:53:41Z","receivedAt":"2015-06-25T16:53:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 25, 2015 at 02:56:59PM +0200, H.Merijn Brand wrote:\n\n> > That being said, I do not think it would be wrong to have a date-mode\n> > that just showed strftime(\"%c\"), or some other arbitrary strftime\n> > string. You could then set log.date as appropriate for human\n> > consumption.\n> \n> Yes please :)\n> \n> --date=lc\n> --date=lc_time\n> --date=locale\n> \n> all spring to mind as valid options\n\nHere's a patch series that allows `--date=format:%c`, which is the same\nthing. We could possibly do `--date=locale` as syntactic sugar, I guess.\n\n [1/3]: show-branch: use DATE_RELATIVE instead of magic number\n [2/3]: convert \"enum date_mode\" into a struct\n [3/3]: introduce \"format\" date-mode\n\n-Peff\n"},{"id":"264868","messageId":"20150625165430.GA23503@peff.net","threadId":"39722","inReplyTo":"20150625165341.GA21949@peff.net","subject":"[PATCH 1/3] show-branch: use DATE_RELATIVE instead of magic number","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-25T16:54:31Z","receivedAt":"2015-06-25T16:54:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is more readable, and won't break if we ever change the\norder of the date_mode enum.\n\nSigned-off-by: Jeff King \n---\n builtin/show-branch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 323f857..d784613 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -784,7 +784,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\telse\n \t\t\t\tmsg++;\n \t\t\treflog_msg[i] = xstrfmt(\"(%s) %s\",\n-\t\t\t\t\t\tshow_date(timestamp, tz, 1),\n+\t\t\t\t\t\tshow_date(timestamp, tz, DATE_RELATIVE),\n \t\t\t\t\t\tmsg);\n \t\t\tfree(logmsg);\n \t\t\tsprintf(nth_desc, \"%s@{%d}\", *av, base+i);\n-- \n2.4.4.742.g862750c\n"},{"id":"264870","messageId":"20150625165501.GB23503@peff.net","threadId":"39722","inReplyTo":"20150625165341.GA21949@peff.net","subject":"[PATCH 2/3] convert \"enum date_mode\" into a struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-25T16:55:02Z","receivedAt":"2015-06-25T16:55:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In preparation for adding date modes that may carry extra\ninformation beyond the mode itself, this patch converts the\ndate_mode enum into a struct.\n\nMost of the conversion is fairly straightforward; we pass\nthe struct as a pointer and dereference the type field where\nnecessary. Locations that declare a date_mode can use a \"{}\"\nconstructor. However, the tricky case is where we use the\nenum labels as constants, like:\n\n show_date(t, tz, DATE_NORMAL);\n\nIdeally we could say:\n\n show_date(t, tz, &{ DATE_NORMAL });\n\nbut of course C does not allow that. Likewise, we cannot\ncast the constant to a struct, because we need to pass an\nactual address. Our options are basically:\n\n 1. Manually add a \"struct date_mode d = { DATE_NORMAL }\"\n definition to each caller, and pass \"&d\". This makes\n the callers uglier, because they sometimes do not even\n have their own scope (e.g., they are inside a switch\n statement).\n\n 2. Provide a pre-made global \"date_normal\" struct that can\n be passed by address. We'd also need \"date_rfc2822\",\n \"date_iso8601\", and so forth. But at least the ugliness\n is defined in one place.\n\n 3. Provide a wrapper that generates the correct struct on\n the fly. The big downside is that we end up pointing to\n a single global, which makes our wrapper non-reentrant.\n But show_date is already not reentrant, so it does not\n matter.\n\nThis patch implements 3, along with a minor macro to keep\nthe size of the callers sane.\n\nSigned-off-by: Jeff King \n---\n archive.c | 2 +-\n builtin/blame.c | 10 +++++-----\n builtin/commit.c | 4 ++--\n builtin/for-each-ref.c | 6 +++---\n builtin/log.c | 4 ++--\n builtin/shortlog.c | 2 +-\n builtin/show-branch.c | 3 ++-\n cache.h | 35 +++++++++++++++++++++++------------\n commit.h | 2 +-\n date.c | 43 +++++++++++++++++++++++++------------------\n fast-import.c | 2 +-\n http-backend.c | 2 +-\n log-tree.c | 2 +-\n pretty.c | 29 +++++++++++++++--------------\n reflog-walk.c | 4 ++--\n reflog-walk.h | 4 ++--\n refs.c | 4 ++--\n revision.c | 4 ++--\n revision.h | 2 +-\n sha1_name.c | 2 +-\n submodule.c | 2 +-\n test-date.c | 4 ++--\n test-revision-walking.c | 2 +-\n 23 files changed, 97 insertions(+), 77 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex d37c41d..30bda56 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -33,7 +33,7 @@ static void format_subst(const struct commit *commit,\n \tchar *to_free = NULL;\n \tstruct strbuf fmt = STRBUF_INIT;\n \tstruct pretty_print_context ctx = {0};\n-\tctx.date_mode = DATE_NORMAL;\n+\tctx.date_mode.type = DATE_NORMAL;\n \tctx.abbrev = DEFAULT_ABBREV;\n \n \tif (src == buf->buf)\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex b3e948e..43e8473 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -50,7 +50,7 @@ static int xdl_opts;\n static int abbrev = -1;\n static int no_whole_file_rename;\n \n-static enum date_mode blame_date_mode = DATE_ISO8601;\n+static struct date_mode blame_date_mode = { DATE_ISO8601 };\n static size_t blame_date_width;\n \n static struct string_list mailmap;\n@@ -1827,7 +1827,7 @@ static const char *format_time(unsigned long time, const char *tz_str,\n \t\tsize_t time_width;\n \t\tint tz;\n \t\ttz = atoi(tz_str);\n-\t\ttime_str = show_date(time, tz, blame_date_mode);\n+\t\ttime_str = show_date(time, tz, &blame_date_mode);\n \t\tstrbuf_addstr(&time_buf, time_str);\n \t\t/*\n \t\t * Add space paddings to time_buf to display a fixed width\n@@ -2179,7 +2179,7 @@ static int git_blame_config(const char *var, const char *value, void *cb)\n \tif (!strcmp(var, \"blame.date\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-\t\tblame_date_mode = parse_date_format(value);\n+\t\tparse_date_format(value, &blame_date_mode);\n \t\treturn 0;\n \t}\n \n@@ -2561,13 +2561,13 @@ parse_done:\n \n \tif (cmd_is_annotate) {\n \t\toutput_option |= OUTPUT_ANNOTATE_COMPAT;\n-\t\tblame_date_mode = DATE_ISO8601;\n+\t\tblame_date_mode.type = DATE_ISO8601;\n \t} else {\n \t\tblame_date_mode = revs.date_mode;\n \t}\n \n \t/* The maximum width used to show the dates */\n-\tswitch (blame_date_mode) {\n+\tswitch (blame_date_mode.type) {\n \tcase DATE_RFC2822:\n \t\tblame_date_width = sizeof(\"Thu, 19 Oct 2006 16:00:04 -0700\");\n \t\tbreak;\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 254477f..7314f33 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -856,7 +856,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\t\t_(\"%s\"\n \t\t\t\t\"Date: %s\"),\n \t\t\t\tident_shown++ ? \"\" : \"\\n\",\n-\t\t\t\tshow_ident_date(&ai, DATE_NORMAL));\n+\t\t\t\tshow_ident_date(&ai, DATE_MODE(NORMAL)));\n \n \t\tif (!committer_ident_sufficiently_given())\n \t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL,\n@@ -1046,7 +1046,7 @@ static const char *find_author_by_nickname(const char *name)\n \tcommit = get_revision(&revs);\n \tif (commit) {\n \t\tstruct pretty_print_context ctx = {0};\n-\t\tctx.date_mode = DATE_NORMAL;\n+\t\tctx.date_mode.type = DATE_NORMAL;\n \t\tstrbuf_release(&buf);\n \t\tformat_commit_message(commit, \"%aN <%aE>\", &buf, &ctx);\n \t\tclear_mailmap(&mailmap);\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex f7e51a7..691daf1 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -389,7 +389,7 @@ static void grab_date(const char *buf, struct atom_value *v, const char *atomnam\n \tchar *zone;\n \tunsigned long timestamp;\n \tlong tz;\n-\tenum date_mode date_mode = DATE_NORMAL;\n+\tstruct date_mode date_mode = { DATE_NORMAL };\n \tconst char *formatp;\n \n \t/*\n@@ -401,7 +401,7 @@ static void grab_date(const char *buf, struct atom_value *v, const char *atomnam\n \tformatp = strchr(atomname, ':');\n \tif (formatp != NULL) {\n \t\tformatp++;\n-\t\tdate_mode = parse_date_format(formatp);\n+\t\tparse_date_format(formatp, &date_mode);\n \t}\n \n \tif (!eoemail)\n@@ -412,7 +412,7 @@ static void grab_date(const char *buf, struct atom_value *v, const char *atomnam\n \ttz = strtol(zone, NULL, 10);\n \tif ((tz == LONG_MIN || tz == LONG_MAX) && errno == ERANGE)\n \t\tgoto bad;\n-\tv->s = xstrdup(show_date(timestamp, tz, date_mode));\n+\tv->s = xstrdup(show_date(timestamp, tz, &date_mode));\n \tv->ul = timestamp;\n \treturn;\n bad:\ndiff --git a/builtin/log.c b/builtin/log.c\nindex e67671e..ce1581e 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -112,7 +112,7 @@ static void cmd_log_init_defaults(struct rev_info *rev)\n \tDIFF_OPT_SET(&rev->diffopt, ALLOW_TEXTCONV);\n \n \tif (default_date_mode)\n-\t\trev->date_mode = parse_date_format(default_date_mode);\n+\t\tparse_date_format(default_date_mode, &rev->date_mode);\n \trev->diffopt.touched_flags = 0;\n }\n \n@@ -939,7 +939,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \n \tmsg = body;\n \tpp.fmt = CMIT_FMT_EMAIL;\n-\tpp.date_mode = DATE_RFC2822;\n+\tpp.date_mode.type = DATE_RFC2822;\n \tpp_user_info(&pp, NULL, &sb, committer, encoding);\n \tpp_title_line(&pp, &msg, &sb, encoding, need_8bit_cte);\n \tpp_remainder(&pp, &msg, &sb, 0);\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex c0bab6a..007cc66 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -138,7 +138,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \t\tctx.abbrev = log->abbrev;\n \t\tctx.subject = \"\";\n \t\tctx.after_subject = \"\";\n-\t\tctx.date_mode = DATE_NORMAL;\n+\t\tctx.date_mode.type = DATE_NORMAL;\n \t\tctx.output_encoding = get_log_output_encoding();\n \t\tpretty_print_commit(&ctx, commit, &ufbuf);\n \t\tbuffer = ufbuf.buf;\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex d784613..c87c46e 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -784,7 +784,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\telse\n \t\t\t\tmsg++;\n \t\t\treflog_msg[i] = xstrfmt(\"(%s) %s\",\n-\t\t\t\t\t\tshow_date(timestamp, tz, DATE_RELATIVE),\n+\t\t\t\t\t\tshow_date(timestamp, tz,\n+\t\t\t\t\t\t\t DATE_MODE(RELATIVE)),\n \t\t\t\t\t\tmsg);\n \t\t\tfree(logmsg);\n \t\t\tsprintf(nth_desc, \"%s@{%d}\", *av, base+i);\ndiff --git a/cache.h b/cache.h\nindex 571c98f..1759011 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1096,18 +1096,28 @@ extern void *read_object_with_reference(const unsigned char *sha1,\n extern struct object *peel_to_type(const char *name, int namelen,\n \t\t\t\t struct object *o, enum object_type);\n \n-enum date_mode {\n-\tDATE_NORMAL = 0,\n-\tDATE_RELATIVE,\n-\tDATE_SHORT,\n-\tDATE_LOCAL,\n-\tDATE_ISO8601,\n-\tDATE_ISO8601_STRICT,\n-\tDATE_RFC2822,\n-\tDATE_RAW\n+struct date_mode {\n+\tenum date_mode_type {\n+\t\tDATE_NORMAL = 0,\n+\t\tDATE_RELATIVE,\n+\t\tDATE_SHORT,\n+\t\tDATE_LOCAL,\n+\t\tDATE_ISO8601,\n+\t\tDATE_ISO8601_STRICT,\n+\t\tDATE_RFC2822,\n+\t\tDATE_RAW\n+\t} type;\n };\n \n-const char *show_date(unsigned long time, int timezone, enum date_mode mode);\n+/*\n+ * Convenience helper for passing a constant type, like:\n+ *\n+ * show_date(t, tz, DATE_MODE(NORMAL));\n+ */\n+#define DATE_MODE(t) date_mode_from_type(DATE_##t)\n+struct date_mode *date_mode_from_type(enum date_mode_type type);\n+\n+const char *show_date(unsigned long time, int timezone, const struct date_mode *mode);\n void show_date_relative(unsigned long time, int tz, const struct timeval *now,\n \t\t\tstruct strbuf *timebuf);\n int parse_date(const char *date, struct strbuf *out);\n@@ -1117,7 +1127,7 @@ void datestamp(struct strbuf *out);\n #define approxidate(s) approxidate_careful((s), NULL)\n unsigned long approxidate_careful(const char *, int *);\n unsigned long approxidate_relative(const char *date, const struct timeval *now);\n-enum date_mode parse_date_format(const char *format);\n+void parse_date_format(const char *format, struct date_mode *mode);\n int date_overflows(unsigned long date);\n \n #define IDENT_STRICT\t 1\n@@ -1154,7 +1164,8 @@ extern int split_ident_line(struct ident_split *, const char *, int);\n * the ident_split. It will also sanity-check the values and produce\n * a well-known sentinel date if they appear bogus.\n */\n-const char *show_ident_date(const struct ident_split *id, enum date_mode mode);\n+const char *show_ident_date(const struct ident_split *id,\n+\t\t\t const struct date_mode *mode);\n \n /*\n * Compare split idents for equality or strict ordering. Note that we\ndiff --git a/commit.h b/commit.h\nindex 9a1fa96..1496882 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -145,7 +145,7 @@ struct pretty_print_context {\n \tconst char *subject;\n \tconst char *after_subject;\n \tint preserve_subject;\n-\tenum date_mode date_mode;\n+\tstruct date_mode date_mode;\n \tunsigned date_mode_explicit:1;\n \tint need_8bit_cte;\n \tchar *notes_message;\ndiff --git a/date.c b/date.c\nindex 733d1b2..cdad4db 100644\n--- a/date.c\n+++ b/date.c\n@@ -160,18 +160,25 @@ void show_date_relative(unsigned long time, int tz,\n \t\t (diff + 183) / 365);\n }\n \n-const char *show_date(unsigned long time, int tz, enum date_mode mode)\n+struct date_mode *date_mode_from_type(enum date_mode_type type)\n+{\n+\tstatic struct date_mode mode;\n+\tmode.type = type;\n+\treturn &mode;\n+}\n+\n+const char *show_date(unsigned long time, int tz, const struct date_mode *mode)\n {\n \tstruct tm *tm;\n \tstatic struct strbuf timebuf = STRBUF_INIT;\n \n-\tif (mode == DATE_RAW) {\n+\tif (mode->type == DATE_RAW) {\n \t\tstrbuf_reset(&timebuf);\n \t\tstrbuf_addf(&timebuf, \"%lu %+05d\", time, tz);\n \t\treturn timebuf.buf;\n \t}\n \n-\tif (mode == DATE_RELATIVE) {\n+\tif (mode->type == DATE_RELATIVE) {\n \t\tstruct timeval now;\n \n \t\tstrbuf_reset(&timebuf);\n@@ -180,7 +187,7 @@ const char *show_date(unsigned long time, int tz, enum date_mode mode)\n \t\treturn timebuf.buf;\n \t}\n \n-\tif (mode == DATE_LOCAL)\n+\tif (mode->type == DATE_LOCAL)\n \t\ttz = local_tzoffset(time);\n \n \ttm = time_to_tm(time, tz);\n@@ -190,17 +197,17 @@ const char *show_date(unsigned long time, int tz, enum date_mode mode)\n \t}\n \n \tstrbuf_reset(&timebuf);\n-\tif (mode == DATE_SHORT)\n+\tif (mode->type == DATE_SHORT)\n \t\tstrbuf_addf(&timebuf, \"%04d-%02d-%02d\", tm->tm_year + 1900,\n \t\t\t\ttm->tm_mon + 1, tm->tm_mday);\n-\telse if (mode == DATE_ISO8601)\n+\telse if (mode->type == DATE_ISO8601)\n \t\tstrbuf_addf(&timebuf, \"%04d-%02d-%02d %02d:%02d:%02d %+05d\",\n \t\t\t\ttm->tm_year + 1900,\n \t\t\t\ttm->tm_mon + 1,\n \t\t\t\ttm->tm_mday,\n \t\t\t\ttm->tm_hour, tm->tm_min, tm->tm_sec,\n \t\t\t\ttz);\n-\telse if (mode == DATE_ISO8601_STRICT) {\n+\telse if (mode->type == DATE_ISO8601_STRICT) {\n \t\tchar sign = (tz >= 0) ? '+' : '-';\n \t\ttz = abs(tz);\n \t\tstrbuf_addf(&timebuf, \"%04d-%02d-%02dT%02d:%02d:%02d%c%02d:%02d\",\n@@ -209,7 +216,7 @@ const char *show_date(unsigned long time, int tz, enum date_mode mode)\n \t\t\t\ttm->tm_mday,\n \t\t\t\ttm->tm_hour, tm->tm_min, tm->tm_sec,\n \t\t\t\tsign, tz / 100, tz % 100);\n-\t} else if (mode == DATE_RFC2822)\n+\t} else if (mode->type == DATE_RFC2822)\n \t\tstrbuf_addf(&timebuf, \"%.3s, %d %.3s %d %02d:%02d:%02d %+05d\",\n \t\t\tweekday_names[tm->tm_wday], tm->tm_mday,\n \t\t\tmonth_names[tm->tm_mon], tm->tm_year + 1900,\n@@ -221,7 +228,7 @@ const char *show_date(unsigned long time, int tz, enum date_mode mode)\n \t\t\t\ttm->tm_mday,\n \t\t\t\ttm->tm_hour, tm->tm_min, tm->tm_sec,\n \t\t\t\ttm->tm_year + 1900,\n-\t\t\t\t(mode == DATE_LOCAL) ? 0 : ' ',\n+\t\t\t\t(mode->type == DATE_LOCAL) ? 0 : ' ',\n \t\t\t\ttz);\n \treturn timebuf.buf;\n }\n@@ -759,27 +766,27 @@ int parse_date(const char *date, struct strbuf *result)\n \treturn 0;\n }\n \n-enum date_mode parse_date_format(const char *format)\n+void parse_date_format(const char *format, struct date_mode *mode)\n {\n \tif (!strcmp(format, \"relative\"))\n-\t\treturn DATE_RELATIVE;\n+\t\tmode->type = DATE_RELATIVE;\n \telse if (!strcmp(format, \"iso8601\") ||\n \t\t !strcmp(format, \"iso\"))\n-\t\treturn DATE_ISO8601;\n+\t\tmode->type = DATE_ISO8601;\n \telse if (!strcmp(format, \"iso8601-strict\") ||\n \t\t !strcmp(format, \"iso-strict\"))\n-\t\treturn DATE_ISO8601_STRICT;\n+\t\tmode->type = DATE_ISO8601_STRICT;\n \telse if (!strcmp(format, \"rfc2822\") ||\n \t\t !strcmp(format, \"rfc\"))\n-\t\treturn DATE_RFC2822;\n+\t\tmode->type = DATE_RFC2822;\n \telse if (!strcmp(format, \"short\"))\n-\t\treturn DATE_SHORT;\n+\t\tmode->type = DATE_SHORT;\n \telse if (!strcmp(format, \"local\"))\n-\t\treturn DATE_LOCAL;\n+\t\tmode->type = DATE_LOCAL;\n \telse if (!strcmp(format, \"default\"))\n-\t\treturn DATE_NORMAL;\n+\t\tmode->type = DATE_NORMAL;\n \telse if (!strcmp(format, \"raw\"))\n-\t\treturn DATE_RAW;\n+\t\tmode->type = DATE_RAW;\n \telse\n \t\tdie(\"unknown date format %s\", format);\n }\ndiff --git a/fast-import.c b/fast-import.c\nindex 6378726..9363cc7 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -421,7 +421,7 @@ static void write_crash_report(const char *err)\n \tfprintf(rpt, \"fast-import crash report:\\n\");\n \tfprintf(rpt, \" fast-import process: %\"PRIuMAX\"\\n\", (uintmax_t) getpid());\n \tfprintf(rpt, \" parent process : %\"PRIuMAX\"\\n\", (uintmax_t) getppid());\n-\tfprintf(rpt, \" at %s\\n\", show_date(time(NULL), 0, DATE_LOCAL));\n+\tfprintf(rpt, \" at %s\\n\", show_date(time(NULL), 0, DATE_MODE(LOCAL)));\n \tfputc('\\n', rpt);\n \n \tfputs(\"fatal: \", rpt);\ndiff --git a/http-backend.c b/http-backend.c\nindex 501bf79..b977c00 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -92,7 +92,7 @@ static void hdr_int(const char *name, uintmax_t value)\n \n static void hdr_date(const char *name, unsigned long when)\n {\n-\tconst char *value = show_date(when, 0, DATE_RFC2822);\n+\tconst char *value = show_date(when, 0, DATE_MODE(RFC2822));\n \thdr_str(name, value);\n }\n \ndiff --git a/log-tree.c b/log-tree.c\nindex 01beb11..e57e6b6 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -639,7 +639,7 @@ void show_log(struct rev_info *opt)\n \t\t\t */\n \t\t\tshow_reflog_message(opt->reflog_info,\n \t\t\t\t\t opt->commit_format == CMIT_FMT_ONELINE,\n-\t\t\t\t\t opt->date_mode,\n+\t\t\t\t\t &opt->date_mode,\n \t\t\t\t\t opt->date_mode_explicit);\n \t\t\tif (opt->commit_format == CMIT_FMT_ONELINE)\n \t\t\t\treturn;\ndiff --git a/pretty.c b/pretty.c\nindex 7b49304..151c2ae 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -399,7 +399,7 @@ static void add_rfc2047(struct strbuf *sb, const char *line, size_t len,\n }\n \n const char *show_ident_date(const struct ident_split *ident,\n-\t\t\t enum date_mode mode)\n+\t\t\t const struct date_mode *mode)\n {\n \tunsigned long date = 0;\n \tlong tz = 0;\n@@ -489,15 +489,15 @@ void pp_user_info(struct pretty_print_context *pp,\n \tswitch (pp->fmt) {\n \tcase CMIT_FMT_MEDIUM:\n \t\tstrbuf_addf(sb, \"Date: %s\\n\",\n-\t\t\t show_ident_date(&ident, pp->date_mode));\n+\t\t\t show_ident_date(&ident, &pp->date_mode));\n \t\tbreak;\n \tcase CMIT_FMT_EMAIL:\n \t\tstrbuf_addf(sb, \"Date: %s\\n\",\n-\t\t\t show_ident_date(&ident, DATE_RFC2822));\n+\t\t\t show_ident_date(&ident, DATE_MODE(RFC2822)));\n \t\tbreak;\n \tcase CMIT_FMT_FULLER:\n \t\tstrbuf_addf(sb, \"%sDate: %s\\n\", what,\n-\t\t\t show_ident_date(&ident, pp->date_mode));\n+\t\t\t show_ident_date(&ident, &pp->date_mode));\n \t\tbreak;\n \tdefault:\n \t\t/* notin' */\n@@ -671,7 +671,8 @@ static int mailmap_name(const char **email, size_t *email_len,\n }\n \n static size_t format_person_part(struct strbuf *sb, char part,\n-\t\t\t\t const char *msg, int len, enum date_mode dmode)\n+\t\t\t\t const char *msg, int len,\n+\t\t\t\t const struct date_mode *dmode)\n {\n \t/* currently all placeholders have same length */\n \tconst int placeholder_len = 2;\n@@ -711,16 +712,16 @@ static size_t format_person_part(struct strbuf *sb, char part,\n \t\tstrbuf_addstr(sb, show_ident_date(&s, dmode));\n \t\treturn placeholder_len;\n \tcase 'D':\t/* date, RFC2822 style */\n-\t\tstrbuf_addstr(sb, show_ident_date(&s, DATE_RFC2822));\n+\t\tstrbuf_addstr(sb, show_ident_date(&s, DATE_MODE(RFC2822)));\n \t\treturn placeholder_len;\n \tcase 'r':\t/* date, relative */\n-\t\tstrbuf_addstr(sb, show_ident_date(&s, DATE_RELATIVE));\n+\t\tstrbuf_addstr(sb, show_ident_date(&s, DATE_MODE(RELATIVE)));\n \t\treturn placeholder_len;\n \tcase 'i':\t/* date, ISO 8601-like */\n-\t\tstrbuf_addstr(sb, show_ident_date(&s, DATE_ISO8601));\n+\t\tstrbuf_addstr(sb, show_ident_date(&s, DATE_MODE(ISO8601)));\n \t\treturn placeholder_len;\n \tcase 'I':\t/* date, ISO 8601 strict */\n-\t\tstrbuf_addstr(sb, show_ident_date(&s, DATE_ISO8601_STRICT));\n+\t\tstrbuf_addstr(sb, show_ident_date(&s, DATE_MODE(ISO8601_STRICT)));\n \t\treturn placeholder_len;\n \t}\n \n@@ -933,7 +934,7 @@ static void rewrap_message_tail(struct strbuf *sb,\n static int format_reflog_person(struct strbuf *sb,\n \t\t\t\tchar part,\n \t\t\t\tstruct reflog_walk_info *log,\n-\t\t\t\tenum date_mode dmode)\n+\t\t\t\tconst struct date_mode *dmode)\n {\n \tconst char *ident;\n \n@@ -1185,7 +1186,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\tif (c->pretty_ctx->reflog_info)\n \t\t\t\tget_reflog_selector(sb,\n \t\t\t\t\t\t c->pretty_ctx->reflog_info,\n-\t\t\t\t\t\t c->pretty_ctx->date_mode,\n+\t\t\t\t\t\t &c->pretty_ctx->date_mode,\n \t\t\t\t\t\t c->pretty_ctx->date_mode_explicit,\n \t\t\t\t\t\t (placeholder[1] == 'd'));\n \t\t\treturn 2;\n@@ -1200,7 +1201,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\treturn format_reflog_person(sb,\n \t\t\t\t\t\t placeholder[1],\n \t\t\t\t\t\t c->pretty_ctx->reflog_info,\n-\t\t\t\t\t\t c->pretty_ctx->date_mode);\n+\t\t\t\t\t\t &c->pretty_ctx->date_mode);\n \t\t}\n \t\treturn 0;\t/* unknown %g placeholder */\n \tcase 'N':\n@@ -1251,11 +1252,11 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \tcase 'a':\t/* author ... */\n \t\treturn format_person_part(sb, placeholder[1],\n \t\t\t\t msg + c->author.off, c->author.len,\n-\t\t\t\t c->pretty_ctx->date_mode);\n+\t\t\t\t &c->pretty_ctx->date_mode);\n \tcase 'c':\t/* committer ... */\n \t\treturn format_person_part(sb, placeholder[1],\n \t\t\t\t msg + c->committer.off, c->committer.len,\n-\t\t\t\t c->pretty_ctx->date_mode);\n+\t\t\t\t &c->pretty_ctx->date_mode);\n \tcase 'e':\t/* encoding */\n \t\tif (c->commit_encoding)\n \t\t\tstrbuf_addstr(sb, c->commit_encoding);\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex 222de76..f8e743a 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -249,7 +249,7 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)\n \n void get_reflog_selector(struct strbuf *sb,\n \t\t\t struct reflog_walk_info *reflog_info,\n-\t\t\t enum date_mode dmode, int force_date,\n+\t\t\t const struct date_mode *dmode, int force_date,\n \t\t\t int shorten)\n {\n \tstruct commit_reflog *commit_reflog = reflog_info->last_commit_reflog;\n@@ -311,7 +311,7 @@ const char *get_reflog_ident(struct reflog_walk_info *reflog_info)\n }\n \n void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline,\n-\t\t\t enum date_mode dmode, int force_date)\n+\t\t\t const struct date_mode *dmode, int force_date)\n {\n \tif (reflog_info && reflog_info->last_commit_reflog) {\n \t\tstruct commit_reflog *commit_reflog = reflog_info->last_commit_reflog;\ndiff --git a/reflog-walk.h b/reflog-walk.h\nindex a9bd60e..27886f7 100644\n--- a/reflog-walk.h\n+++ b/reflog-walk.h\n@@ -11,13 +11,13 @@ extern int add_reflog_for_walk(struct reflog_walk_info *info,\n extern void fake_reflog_parent(struct reflog_walk_info *info,\n \t\tstruct commit *commit);\n extern void show_reflog_message(struct reflog_walk_info *info, int,\n-\t\t\t\tenum date_mode, int force_date);\n+\t\t\t\tconst struct date_mode *, int force_date);\n extern void get_reflog_message(struct strbuf *sb,\n \t\tstruct reflog_walk_info *reflog_info);\n extern const char *get_reflog_ident(struct reflog_walk_info *reflog_info);\n extern void get_reflog_selector(struct strbuf *sb,\n \t\tstruct reflog_walk_info *reflog_info,\n-\t\tenum date_mode dmode, int force_date,\n+\t\tconst struct date_mode *dmode, int force_date,\n \t\tint shorten);\n \n #endif\ndiff --git a/refs.c b/refs.c\nindex 26d1ac1..f9b828c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3356,14 +3356,14 @@ static int read_ref_at_ent(unsigned char *osha1, unsigned char *nsha1,\n \t\t\thashcpy(cb->sha1, nsha1);\n \t\t\tif (hashcmp(cb->osha1, nsha1))\n \t\t\t\twarning(\"Log for ref %s has gap after %s.\",\n-\t\t\t\t\tcb->refname, show_date(cb->date, cb->tz, DATE_RFC2822));\n+\t\t\t\t\tcb->refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));\n \t\t}\n \t\telse if (cb->date == cb->at_time)\n \t\t\thashcpy(cb->sha1, nsha1);\n \t\telse if (hashcmp(nsha1, cb->sha1))\n \t\t\twarning(\"Log for ref %s unexpectedly ended on %s.\",\n \t\t\t\tcb->refname, show_date(cb->date, cb->tz,\n-\t\t\t\t\t\t DATE_RFC2822));\n+\t\t\t\t\t\t DATE_MODE(RFC2822)));\n \t\thashcpy(cb->osha1, osha1);\n \t\thashcpy(cb->nsha1, nsha1);\n \t\tcb->found_it = 1;\ndiff --git a/revision.c b/revision.c\nindex 3ff8723..887e65e 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1996,10 +1996,10 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else if (!strcmp(arg, \"--full-history\")) {\n \t\trevs->simplify_history = 0;\n \t} else if (!strcmp(arg, \"--relative-date\")) {\n-\t\trevs->date_mode = DATE_RELATIVE;\n+\t\trevs->date_mode.type = DATE_RELATIVE;\n \t\trevs->date_mode_explicit = 1;\n \t} else if ((argcount = parse_long_opt(\"date\", argv, &optarg))) {\n-\t\trevs->date_mode = parse_date_format(optarg);\n+\t\tparse_date_format(optarg, &revs->date_mode);\n \t\trevs->date_mode_explicit = 1;\n \t\treturn argcount;\n \t} else if (!strcmp(arg, \"--log-size\")) {\ndiff --git a/revision.h b/revision.h\nindex 0ea8b4e..5bc9686 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -146,7 +146,7 @@ struct rev_info {\n \t\t\ttrack_first_time:1,\n \t\t\tlinear:1;\n \n-\tenum date_mode date_mode;\n+\tstruct date_mode date_mode;\n \n \tunsigned int\tabbrev;\n \tenum cmit_fmt\tcommit_format;\ndiff --git a/sha1_name.c b/sha1_name.c\nindex e57513e..da6874c 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -576,7 +576,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1,\n \t\t\t\tif (!(flags & GET_SHA1_QUIETLY)) {\n \t\t\t\t\twarning(\"Log for '%.*s' only goes \"\n \t\t\t\t\t\t\"back to %s.\", len, str,\n-\t\t\t\t\t\tshow_date(co_time, co_tz, DATE_RFC2822));\n+\t\t\t\t\t\tshow_date(co_time, co_tz, DATE_MODE(RFC2822)));\n \t\t\t\t}\n \t\t\t} else {\n \t\t\t\tif (flags & GET_SHA1_QUIETLY) {\ndiff --git a/submodule.c b/submodule.c\nindex 15e90d1..700bbf4 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -993,7 +993,7 @@ static void print_commit(struct commit *commit)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct pretty_print_context ctx = {0};\n-\tctx.date_mode = DATE_NORMAL;\n+\tctx.date_mode.type = DATE_NORMAL;\n \tformat_commit_message(commit, \" %h: %m %s\", &sb, &ctx);\n \tfprintf(stderr, \"%s\\n\", sb.buf);\n \tstrbuf_release(&sb);\ndiff --git a/test-date.c b/test-date.c\nindex 94a6997..63f3735 100644\n--- a/test-date.c\n+++ b/test-date.c\n@@ -29,7 +29,7 @@ static void parse_dates(char **argv, struct timeval *now)\n \t\tparse_date(*argv, &result);\n \t\tif (sscanf(result.buf, \"%lu %d\", &t, &tz) == 2)\n \t\t\tprintf(\"%s -> %s\\n\",\n-\t\t\t *argv, show_date(t, tz, DATE_ISO8601));\n+\t\t\t *argv, show_date(t, tz, DATE_MODE(ISO8601)));\n \t\telse\n \t\t\tprintf(\"%s -> bad\\n\", *argv);\n \t}\n@@ -41,7 +41,7 @@ static void parse_approxidate(char **argv, struct timeval *now)\n \tfor (; *argv; argv++) {\n \t\ttime_t t;\n \t\tt = approxidate_relative(*argv, now);\n-\t\tprintf(\"%s -> %s\\n\", *argv, show_date(t, 0, DATE_ISO8601));\n+\t\tprintf(\"%s -> %s\\n\", *argv, show_date(t, 0, DATE_MODE(ISO8601)));\n \t}\n }\n \ndiff --git a/test-revision-walking.c b/test-revision-walking.c\nindex 3ade02c..285f06b 100644\n--- a/test-revision-walking.c\n+++ b/test-revision-walking.c\n@@ -17,7 +17,7 @@ static void print_commit(struct commit *commit)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct pretty_print_context ctx = {0};\n-\tctx.date_mode = DATE_NORMAL;\n+\tctx.date_mode.type = DATE_NORMAL;\n \tformat_commit_message(commit, \" %m %s\", &sb, &ctx);\n \tprintf(\"%s\\n\", sb.buf);\n \tstrbuf_release(&sb);\n-- \n2.4.4.742.g862750c\n"},{"id":"264871","messageId":"20150625165545.GC23503@peff.net","threadId":"39722","inReplyTo":"20150625165341.GA21949@peff.net","subject":"[PATCH 3/3] introduce \"format\" date-mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-25T16:55:45Z","receivedAt":"2015-06-25T16:55:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This feeds the format directly to strftime. Besides being a\nlittle more flexible, the main advantage is that your system\nstrftime may know more about your locale's preferred format\n(e.g., how to spell the days of the week).\n\nSigned-off-by: Jeff King \n---\n Documentation/rev-list-options.txt | 5 +++++\n builtin/blame.c | 3 +++\n cache.h | 2 ++\n date.c | 9 ++++++++-\n gettext.c | 1 +\n strbuf.c | 29 +++++++++++++++++++++++++++++\n strbuf.h | 5 +++++\n t/t6300-for-each-ref.sh | 8 ++++++++\n 8 files changed, 61 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 77ac439..a9b808f 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -727,6 +727,11 @@ format, often found in email messages.\n +\n `--date=raw` shows the date in the internal raw Git format `%s %z` format.\n +\n+`--date=format:...` feeds the format `...` to your system `strftime`.\n+Use `--date=format:%c` to show the date in your system locale's\n+preferred format. See the `strftime` manual for a complete list of\n+format placeholders.\n++\n `--date=default` shows timestamps in the original time zone\n (either committer's or author's).\n \ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 43e8473..e2e3e75 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2596,6 +2596,9 @@ parse_done:\n \tcase DATE_NORMAL:\n \t\tblame_date_width = sizeof(\"Thu Oct 19 16:00:04 2006 -0700\");\n \t\tbreak;\n+\tcase DATE_STRFTIME:\n+\t\tblame_date_width = strlen(show_date(0, 0, &blame_date_mode)) + 1; /* add the null */\n+\t\tbreak;\n \t}\n \tblame_date_width -= 1; /* strip the null */\n \ndiff --git a/cache.h b/cache.h\nindex 1759011..bb63a58 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1105,8 +1105,10 @@ struct date_mode {\n \t\tDATE_ISO8601,\n \t\tDATE_ISO8601_STRICT,\n \t\tDATE_RFC2822,\n+\t\tDATE_STRFTIME,\n \t\tDATE_RAW\n \t} type;\n+\tconst char *strftime_fmt;\n };\n \n /*\ndiff --git a/date.c b/date.c\nindex cdad4db..8f91569 100644\n--- a/date.c\n+++ b/date.c\n@@ -163,6 +163,8 @@ void show_date_relative(unsigned long time, int tz,\n struct date_mode *date_mode_from_type(enum date_mode_type type)\n {\n \tstatic struct date_mode mode;\n+\tif (type == DATE_STRFTIME)\n+\t\tdie(\"BUG: cannot create anonymous strftime date_mode struct\");\n \tmode.type = type;\n \treturn &mode;\n }\n@@ -221,6 +223,8 @@ const char *show_date(unsigned long time, int tz, const struct date_mode *mode)\n \t\t\tweekday_names[tm->tm_wday], tm->tm_mday,\n \t\t\tmonth_names[tm->tm_mon], tm->tm_year + 1900,\n \t\t\ttm->tm_hour, tm->tm_min, tm->tm_sec, tz);\n+\telse if (mode->type == DATE_STRFTIME)\n+\t\tstrbuf_addftime(&timebuf, mode->strftime_fmt, tm);\n \telse\n \t\tstrbuf_addf(&timebuf, \"%.3s %.3s %d %02d:%02d:%02d %d%c%+05d\",\n \t\t\t\tweekday_names[tm->tm_wday],\n@@ -787,7 +791,10 @@ void parse_date_format(const char *format, struct date_mode *mode)\n \t\tmode->type = DATE_NORMAL;\n \telse if (!strcmp(format, \"raw\"))\n \t\tmode->type = DATE_RAW;\n-\telse\n+\telse if (skip_prefix(format, \"format:\", &format)) {\n+\t\tmode->type = DATE_STRFTIME;\n+\t\tmode->strftime_fmt = xstrdup(format);\n+\t} else\n \t\tdie(\"unknown date format %s\", format);\n }\n \ndiff --git a/gettext.c b/gettext.c\nindex 7378ba2..a268a2c 100644\n--- a/gettext.c\n+++ b/gettext.c\n@@ -162,6 +162,7 @@ void git_setup_gettext(void)\n \t\tpodir = GIT_LOCALE_PATH;\n \tbindtextdomain(\"git\", podir);\n \tsetlocale(LC_MESSAGES, \"\");\n+\tsetlocale(LC_TIME, \"\");\n \tinit_gettext_charset(\"git\");\n \ttextdomain(\"git\");\n }\ndiff --git a/strbuf.c b/strbuf.c\nindex 0d4f4e5..a7ba028 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -709,3 +709,32 @@ char *xstrfmt(const char *fmt, ...)\n \n \treturn ret;\n }\n+\n+void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)\n+{\n+\tsize_t len;\n+\n+\t/*\n+\t * strftime reports \"0\" if it could not fit the result in the buffer.\n+\t * Unfortunately, it also reports \"0\" if the requested time string\n+\t * takes 0 bytes. So if we were to probe and grow, we have to choose\n+\t * some arbitrary cap beyond which we guess that the format probably\n+\t * just results in a 0-length output. Since we have to choose some\n+\t * reasonable cap anyway, and since it is not that big, we may\n+\t * as well just grow to their in the first place.\n+\t */\n+\tstrbuf_grow(sb, 128);\n+\tlen = strftime(sb->buf + sb->len, sb->alloc - sb->len, fmt, tm);\n+\n+\tif (!len) {\n+\t\t/*\n+\t\t * Either we failed, or the format actually produces a 0-length\n+\t\t * output. There's not much we can do, so we leave it blank.\n+\t\t * However, the output array is left in an undefined state, so\n+\t\t * we must re-assert our NUL terminator.\n+\t\t */\n+\t\tsb->buf[sb->len] = '\\0';\n+\t} else {\n+\t\tsb->len += len;\n+\t}\n+}\ndiff --git a/strbuf.h b/strbuf.h\nindex 01c5c63..8c8f8d5 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -345,6 +345,11 @@ __attribute__((format (printf,2,0)))\n extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);\n \n /**\n+ * Add the time specified by `tm`, as formatted by `strftime`.\n+ */\n+extern void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm);\n+\n+/**\n * Read a given size of data from a FILE* pointer to the buffer.\n *\n * NOTE: The buffer is rewound if the read fails. If -1 is returned,\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 24fc2ba..c7f368c 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -227,6 +227,14 @@ test_expect_success 'Check format \"rfc2822\" date fields output' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'Check format of strftime date fields' '\n+\techo \"my date is 2006-07-03\" >expected &&\n+\tgit for-each-ref \\\n+\t --format=\"%(authordate:format:my date is %Y-%m-%d)\" \\\n+\t refs/heads >actual &&\n+\ttest_cmp expected actual\n+'\n+\n cat >expected <<\\EOF\n refs/heads/master\n refs/remotes/origin/master\n-- \n2.4.4.742.g862750c\n"},{"id":"264872","messageId":"20150625170328.GV18226@serenity.lan","threadId":"39722","inReplyTo":"20150625165501.GB23503@peff.net","subject":"Re: [PATCH 2/3] convert \"enum date_mode\" into a struct","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2015-06-25T17:03:28Z","receivedAt":"2015-06-25T17:03:28Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, Jun 25, 2015 at 12:55:02PM -0400, Jeff King wrote:\n> In preparation for adding date modes that may carry extra\n> information beyond the mode itself, this patch converts the\n> date_mode enum into a struct.\n> \n> Most of the conversion is fairly straightforward; we pass\n> the struct as a pointer and dereference the type field where\n> necessary. Locations that declare a date_mode can use a \"{}\"\n> constructor. However, the tricky case is where we use the\n> enum labels as constants, like:\n> \n> show_date(t, tz, DATE_NORMAL);\n> \n> Ideally we could say:\n> \n> show_date(t, tz, &{ DATE_NORMAL });\n> \n> but of course C does not allow that.\n\nYes it does, e.g. in 6.5.2.5 of C11, example 3 shows:\n\n\tdrawline(&(struct point){.x=1, .y=1},\n\t\t&(struct point){.x=3, .y=4});\n\nThe cast is required, but if the argument is pointer-to-const you can\nconstruct a temporary in the function call.\n\nOf course, whether all of the compilers we target support it is a\ndifferent question. If they do, perhaps something like:\n\n#define SIMPLE_DATE(f)\t\t&(struct date_mode) { DATE_NORMAL }\n\nwould allow the callers to remain reasonably sane.\n\n> Likewise, we cannot\n> cast the constant to a struct, because we need to pass an\n> actual address. Our options are basically:\n> \n> 1. Manually add a \"struct date_mode d = { DATE_NORMAL }\"\n> definition to each caller, and pass \"&d\". This makes\n> the callers uglier, because they sometimes do not even\n> have their own scope (e.g., they are inside a switch\n> statement).\n> \n> 2. Provide a pre-made global \"date_normal\" struct that can\n> be passed by address. We'd also need \"date_rfc2822\",\n> \"date_iso8601\", and so forth. But at least the ugliness\n> is defined in one place.\n> \n> 3. Provide a wrapper that generates the correct struct on\n> the fly. The big downside is that we end up pointing to\n> a single global, which makes our wrapper non-reentrant.\n> But show_date is already not reentrant, so it does not\n> matter.\n> \n> This patch implements 3, along with a minor macro to keep\n> the size of the callers sane.\n"},{"id":"264874","messageId":"20150625172246.GA24744@peff.net","threadId":"39722","inReplyTo":"20150625170328.GV18226@serenity.lan","subject":"Re: [PATCH 2/3] convert \"enum date_mode\" into a struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-25T17:22:47Z","receivedAt":"2015-06-25T17:22:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 25, 2015 at 06:03:28PM +0100, John Keeping wrote:\n\n> > Ideally we could say:\n> > \n> > show_date(t, tz, &{ DATE_NORMAL });\n> > \n> > but of course C does not allow that.\n> \n> Yes it does, e.g. in 6.5.2.5 of C11, example 3 shows:\n\nWell, yes. But we generally restrict ourselves to C89 here, so we are\nnot even close.\n\n> Of course, whether all of the compilers we target support it is a\n> different question. If they do, perhaps something like:\n> \n> #define SIMPLE_DATE(f)\t\t&(struct date_mode) { DATE_NORMAL }\n> \n> would allow the callers to remain reasonably sane.\n\nMy patch already introduces DATE_MODE, so you could conditionally hide\nit there, and fall back to date_mode_from_type when the compiler is too\nold for this. But then, what is the advantage over the existing\nsolution? It's reentrant, but I don't think that is a problem here.\n\nAnd in patch 3, you'll see that I add an extra assertion to\ndate_mode_from_type that this cannot support (to make sure that we do\nnot create a DATE_STRFTIME mode with no matching format string). The\nsyntax above would at least give us NULL for the string, which is better\nthan random garbage, but I think the assert is better still.\n\n-Peff\n"},{"id":"265195","messageId":"20150629222247.GA31607@flurp.local","threadId":"39722","inReplyTo":"20150625165545.GC23503@peff.net","subject":"Re: [PATCH 3/3] introduce \"format\" date-mode","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-06-29T22:22:47Z","receivedAt":"2015-06-29T22:22:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jun 25, 2015 at 12:55:45PM -0400, Jeff King wrote:\n> This feeds the format directly to strftime. Besides being a\n> little more flexible, the main advantage is that your system\n> strftime may know more about your locale's preferred format\n> (e.g., how to spell the days of the week).\n> \n> Signed-off-by: Jeff King \n> ---\n> diff --git a/strbuf.c b/strbuf.c\n> index 0d4f4e5..a7ba028 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -709,3 +709,32 @@ char *xstrfmt(const char *fmt, ...)\n> +void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)\n> +{\n> +\tsize_t len;\n> +\n> +\t/*\n> +\t * strftime reports \"0\" if it could not fit the result in the buffer.\n> +\t * Unfortunately, it also reports \"0\" if the requested time string\n> +\t * takes 0 bytes. So if we were to probe and grow, we have to choose\n> +\t * some arbitrary cap beyond which we guess that the format probably\n> +\t * just results in a 0-length output. Since we have to choose some\n> +\t * reasonable cap anyway, and since it is not that big, we may\n> +\t * as well just grow to their in the first place.\n> +\t */\n> +\tstrbuf_grow(sb, 128);\n> +\tlen = strftime(sb->buf + sb->len, sb->alloc - sb->len, fmt, tm);\n> +\n> +\tif (!len) {\n> +\t\t/*\n> +\t\t * Either we failed, or the format actually produces a 0-length\n> +\t\t * output. There's not much we can do, so we leave it blank.\n> +\t\t * However, the output array is left in an undefined state, so\n> +\t\t * we must re-assert our NUL terminator.\n> +\t\t */\n> +\t\tsb->buf[sb->len] = '\\0';\n> +\t} else {\n> +\t\tsb->len += len;\n> +\t}\n> +}\n\nClients of strbuf rightly expect the buffer to grow as needed in\norder to complete the requested operation. It is, therefore, both\nweird and expectation-breaking for strbuf_addftime() to lack this\nbehavior. Worse, it doesn't even signal when the format has failed\ndue to insufficient buffer space.\n\nHow about taking this approach (or something similar), instead, which\ngrows the strbuf as needed?\n\n--- 8< ---\nvoid strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)\n{\n\tsize_t len;\n\tstruct strbuf f = STRBUF_INIT;\n\n\t/*\n\t * This is a bit tricky since strftime returns 0 if the result did not\n\t * fit in the supplied buffer, as well as when the formatted time has\n\t * zero length. In the former case, we need to grow the buffer and try\n\t * again. To distinguish between the two cases, we supply strftime with\n\t * a format string one character longer than what the client supplied,\n\t * which ensures that a successful format will have non-zero length,\n\t * and then drop the extra character from the formatted time before\n\t * returning.\n\t */\n\tstrbuf_addf(&f, \"%s \", fmt);\n\n\tdo {\n\t\tstrbuf_grow(sb, 128);\n\t\tlen = strftime(sb->buf + sb->len, sb->alloc - sb->len,\n\t\t\t f.buf, tm);\n\t} while (!len);\n\tstrbuf_setlen(sb, sb->len + len - 1);\n\n\tstrbuf_release(&f);\n}\n--- 8< ---\n\nIf this is performance critical code, then the augmented format\nstring can be constructed with less expensive functions than\nstrbuf_addf().\n"},{"id":"265209","messageId":"20150630102055.GA11928@peff.net","threadId":"39722","inReplyTo":"20150629222247.GA31607@flurp.local","subject":"Re: [PATCH 3/3] introduce \"format\" date-mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-30T10:20:56Z","receivedAt":"2015-06-30T10:20:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 29, 2015 at 06:22:47PM -0400, Eric Sunshine wrote:\n\n> Clients of strbuf rightly expect the buffer to grow as needed in\n> order to complete the requested operation. It is, therefore, both\n> weird and expectation-breaking for strbuf_addftime() to lack this\n> behavior. Worse, it doesn't even signal when the format has failed\n> due to insufficient buffer space.\n\nAgreed on all points.\n\n> --- 8< ---\n> void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)\n> {\n> \tsize_t len;\n> \tstruct strbuf f = STRBUF_INIT;\n> \n> \t/*\n> \t * This is a bit tricky since strftime returns 0 if the result did not\n> \t * fit in the supplied buffer, as well as when the formatted time has\n> \t * zero length. In the former case, we need to grow the buffer and try\n> \t * again. To distinguish between the two cases, we supply strftime with\n> \t * a format string one character longer than what the client supplied,\n> \t * which ensures that a successful format will have non-zero length,\n> \t * and then drop the extra character from the formatted time before\n> \t * returning.\n> \t */\n> \tstrbuf_addf(&f, \"%s \", fmt);\n\nBasically I was trying to avoid making any assumptions about exactly how\nstrftime works. But presumably \"stick a space in the format\" is a\nuniversally reasonable thing to do. It's a hack, but it's contained to\nthe function.\n\n> \tdo {\n> \t\tstrbuf_grow(sb, 128);\n> \t\tlen = strftime(sb->buf + sb->len, sb->alloc - sb->len,\n> \t\t\t f.buf, tm);\n> \t} while (!len);\n\nI think we need to keep growing this 128 ourselves, or else each loop\niteration will just say \"yup, we have 128 bytes available; no need to\ngrow\".\n\n\n> [...]\n> \n> If this is performance critical code, then the augmented format\n> string can be constructed with less expensive functions than\n> strbuf_addf().\n\nThis does get called a lot (e.g., once per commit). One extra allocation\nwould probably not kill us there, but I think we could fairly trivially\nput this on the unlikely path:\n\n size_t hint = 128;\n size_t len;\n\n /* optimize out obvious 0-length case */\n if (!*fmt)\n\treturn;\n\n strbuf_grow(sb, hint);\n len = strftime(sb->buf + sb->len, sb->alloc - sb->len, fmt, tm);\n\n /* maybe not enough room, or maybe 0-length output */\n if (!len) {\n\tstruct strbuf f = STRBUF_INIT;\n\tstrbuf_addf(&f, \"%s \", fmt);\n\twhile (!len) {\n\t\thint *= 2;\n\t\tstrbuf_grow(sb, hint);\n\t\tlen = strftime(sb->buf + sb->len, sb->alloc - sb->len, f.buf, tm);\n\t}\n }\n\nI'd guess most cases will fit in 128 bytes and never even hit this code\npath. You could also get fancier and start the buffer smaller, but only\ndo the fmt hack when we cross a threshold.\n\n-Peff\n"},{"id":"265230","messageId":"20150630132653.GA25742@peff.net","threadId":"39722","inReplyTo":"20150629222247.GA31607@flurp.local","subject":"Re: [PATCH 3/3] introduce \"format\" date-mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-30T13:26:53Z","receivedAt":"2015-06-30T13:26:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 29, 2015 at 06:22:47PM -0400, Eric Sunshine wrote:\n\n> Clients of strbuf rightly expect the buffer to grow as needed in\n> order to complete the requested operation. It is, therefore, both\n> weird and expectation-breaking for strbuf_addftime() to lack this\n> behavior. Worse, it doesn't even signal when the format has failed\n> due to insufficient buffer space.\n> \n> How about taking this approach (or something similar), instead, which\n> grows the strbuf as needed?\n\nHere's a patch, on top of jk/date-mode-format (I think it would also be\nfine to just squash into the tip commit; the explanation in the commit\nmessage is sufficiently mirrored in the code comment).\n\n-- >8 --\nSubject: [PATCH] strbuf: make strbuf_addftime more robust\n\nThe return value of strftime is poorly designed; when it\nreturns 0, the caller cannot tell if the buffer was not\nlarge enough, or if the output was actually 0 bytes. In the\noriginal implementation of strbuf_addftime, we simply punted\nand guessed that our 128-byte hint would be large enough.\n\nWe can do better, though, if we're willing to treat strftime\nlike less of a black box. We can munge the incoming format\nto make sure that it never produces 0-length output, and\nthen \"fix\" the resulting output. That lets us reliably grow\nthe buffer based on strftime's return value.\n\nClever-idea-by: Eric Sunshine \nSigned-off-by: Jeff King \n---\n strbuf.c | 38 +++++++++++++++++++++-----------------\n t/t6300-for-each-ref.sh | 10 ++++++++++\n 2 files changed, 31 insertions(+), 17 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex a7ba028..e5e7370 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -712,29 +712,33 @@ char *xstrfmt(const char *fmt, ...)\n \n void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)\n {\n+\tsize_t hint = 128;\n \tsize_t len;\n \n-\t/*\n-\t * strftime reports \"0\" if it could not fit the result in the buffer.\n-\t * Unfortunately, it also reports \"0\" if the requested time string\n-\t * takes 0 bytes. So if we were to probe and grow, we have to choose\n-\t * some arbitrary cap beyond which we guess that the format probably\n-\t * just results in a 0-length output. Since we have to choose some\n-\t * reasonable cap anyway, and since it is not that big, we may\n-\t * as well just grow to their in the first place.\n-\t */\n-\tstrbuf_grow(sb, 128);\n+\tif (!*fmt)\n+\t\treturn;\n+\n+\tstrbuf_grow(sb, hint);\n \tlen = strftime(sb->buf + sb->len, sb->alloc - sb->len, fmt, tm);\n \n \tif (!len) {\n \t\t/*\n-\t\t * Either we failed, or the format actually produces a 0-length\n-\t\t * output. There's not much we can do, so we leave it blank.\n-\t\t * However, the output array is left in an undefined state, so\n-\t\t * we must re-assert our NUL terminator.\n+\t\t * strftime reports \"0\" if it could not fit the result in the buffer.\n+\t\t * Unfortunately, it also reports \"0\" if the requested time string\n+\t\t * takes 0 bytes. So our strategy is to munge the format so that the\n+\t\t * output contains at least one character, and then drop the extra\n+\t\t * character before returning.\n \t\t */\n-\t\tsb->buf[sb->len] = '\\0';\n-\t} else {\n-\t\tsb->len += len;\n+\t\tstruct strbuf munged_fmt = STRBUF_INIT;\n+\t\tstrbuf_addf(&munged_fmt, \"%s \", fmt);\n+\t\twhile (!len) {\n+\t\t\thint *= 2;\n+\t\t\tstrbuf_grow(sb, hint);\n+\t\t\tlen = strftime(sb->buf + sb->len, sb->alloc - sb->len,\n+\t\t\t\t munged_fmt.buf, tm);\n+\t\t}\n+\t\tstrbuf_release(&munged_fmt);\n+\t\tlen--; /* drop munged space */\n \t}\n+\tstrbuf_setlen(sb, sb->len + len);\n }\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex c7f368c..7c9bec7 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -235,6 +235,16 @@ test_expect_success 'Check format of strftime date fields' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'exercise strftime with odd fields' '\n+\techo >expected &&\n+\tgit for-each-ref --format=\"%(authordate:format:)\" refs/heads >actual &&\n+\ttest_cmp expected actual &&\n+\tlong=\"long format -- $_z40$_z40$_z40$_z40$_z40$_z40$_z40\" &&\n+\techo $long >expected &&\n+\tgit for-each-ref --format=\"%(authordate:format:$long)\" refs/heads >actual &&\n+\ttest_cmp expected actual\n+'\n+\n cat >expected <<\\EOF\n refs/heads/master\n refs/remotes/origin/master\n-- \n2.5.0.rc0.336.g8460790\n"},{"id":"265248","messageId":"xmqq381988ud.fsf@gitster.dls.corp.google.com","threadId":"39722","inReplyTo":"20150630102055.GA11928@peff.net","subject":"Re: [PATCH 3/3] introduce \"format\" date-mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-30T16:22:18Z","receivedAt":"2015-06-30T16:22:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> \tstrbuf_addf(&f, \"%s \", fmt);\n>\n> Basically I was trying to avoid making any assumptions about exactly how\n> strftime works. But presumably \"stick a space in the format\" is a\n> universally reasonable thing to do. It's a hack, but it's contained to\n> the function.\n\nWhy can't I shake this feeling that (\" %s\", fmt), i.e. prepend not\nappend, is the safer thing to do than to append?\n"},{"id":"265250","messageId":"CAPig+cTXc_RXbOAOaF2MFjrg+DSet=g0XQMZY0ErMYAmNVSV+g@mail.gmail.com","threadId":"39722","inReplyTo":"20150630102055.GA11928@peff.net","subject":"Re: [PATCH 3/3] introduce \"format\" date-mode","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-06-30T16:58:33Z","receivedAt":"2015-06-30T16:58:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jun 30, 2015 at 6:20 AM, Jeff King wrote:\n> On Mon, Jun 29, 2015 at 06:22:47PM -0400, Eric Sunshine wrote:\n>> void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)\n>> {\n>> size_t len;\n>> struct strbuf f = STRBUF_INIT;\n>>\n>> /*\n>> * This is a bit tricky since strftime returns 0 if the result did not\n>> * fit in the supplied buffer, as well as when the formatted time has\n>> * zero length. In the former case, we need to grow the buffer and try\n>> * again. To distinguish between the two cases, we supply strftime with\n>> * a format string one character longer than what the client supplied,\n>> * which ensures that a successful format will have non-zero length,\n>> * and then drop the extra character from the formatted time before\n>> * returning.\n>> */\n>> strbuf_addf(&f, \"%s \", fmt);\n>\n> Basically I was trying to avoid making any assumptions about exactly how\n> strftime works. But presumably \"stick a space in the format\" is a\n> universally reasonable thing to do. It's a hack, but it's contained to\n> the function.\n\nI don't think we're making any assumptions about strftime(). POSIX states:\n\n The format string consists of zero or more conversion\n specifications and ordinary characters. [...] All ordinary\n characters (including the terminating NUL character) are copied\n unchanged into the array.\n\nSo, we seem to be on solid footing with this approach (even though\nit's a localized hack).\n\n>> do {\n>> strbuf_grow(sb, 128);\n>> len = strftime(sb->buf + sb->len, sb->alloc - sb->len,\n>> f.buf, tm);\n>> } while (!len);\n>\n> I think we need to keep growing this 128 ourselves, or else each loop\n> iteration will just say \"yup, we have 128 bytes available; no need to\n> grow\".\n\nYeah, I toyed with the idea of increasing the requested amount each\niteration but wanted to keep the example simple, thus left it out.\nHowever, for some reason, I was thinking that strbuf_grow() was\nunconditionally expanding the buffer by the requested amount rather\nthan merely ensuring that that amount was availabile, so the amount\nclearly needs to be increased on each iteration. Thanks for pointing\nthat out.\n\n>> If this is performance critical code, then the augmented format\n>> string can be constructed with less expensive functions than\n>> strbuf_addf().\n>\n> This does get called a lot (e.g., once per commit). One extra allocation\n> would probably not kill us there [...]\n\nBeyond the extra allocation, I was also concerned about the\nsledgehammer approach of \"%s \" to append a single character when there\nare much less expensive ways to do so.\n\n> [...] but I think we could fairly trivially put this on the unlikely path:\n>\n> size_t hint = 128;\n> size_t len;\n>\n> /* optimize out obvious 0-length case */\n> if (!*fmt)\n> return;\n>\n> strbuf_grow(sb, hint);\n> len = strftime(sb->buf + sb->len, sb->alloc - sb->len, fmt, tm);\n>\n> /* maybe not enough room, or maybe 0-length output */\n> if (!len) {\n> struct strbuf f = STRBUF_INIT;\n> strbuf_addf(&f, \"%s \", fmt);\n> while (!len) {\n> hint *= 2;\n> strbuf_grow(sb, hint);\n> len = strftime(sb->buf + sb->len, sb->alloc - sb->len, f.buf, tm);\n> }\n> }\n>\n> I'd guess most cases will fit in 128 bytes and never even hit this code\n> path. You could also get fancier and start the buffer smaller, but only\n> do the fmt hack when we cross a threshold.\n\nYep, I toyed with other looping constructs as well before settling\nupon do-while in the example for its simplicity. If called often\nenough, this sort of optimization seems reasonable enough.\n"},{"id":"265253","messageId":"xmqqr3ot6s9u.fsf@gitster.dls.corp.google.com","threadId":"39722","inReplyTo":"20150630102055.GA11928@peff.net","subject":"Re: [PATCH 3/3] introduce \"format\" date-mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-30T17:05:33Z","receivedAt":"2015-06-30T17:05:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This does get called a lot (e.g., once per commit). One extra allocation\n> would probably not kill us there, but I think we could fairly trivially\n> put this on the unlikely path:\n>\n> size_t hint = 128;\n> size_t len;\n>\n> /* optimize out obvious 0-length case */\n> if (!*fmt)\n> \treturn;\n>\n> strbuf_grow(sb, hint);\n> len = strftime(sb->buf + sb->len, sb->alloc - sb->len, fmt, tm);\n>\n> /* maybe not enough room, or maybe 0-length output */\n> if (!len) {\n> \tstruct strbuf f = STRBUF_INIT;\n> \tstrbuf_addf(&f, \"%s \", fmt);\n> \twhile (!len) {\n> \t\thint *= 2;\n> \t\tstrbuf_grow(sb, hint);\n> \t\tlen = strftime(sb->buf + sb->len, sb->alloc - sb->len, f.buf, tm);\n> \t}\n> }\n>\n> I'd guess most cases will fit in 128 bytes and never even hit this code\n> path. You could also get fancier and start the buffer smaller, but only\n> do the fmt hack when we cross a threshold.\n\nI'd assume that the \"hint\" thing will persist across calls somehow?\nIn an invocation of \"git log --date=format:\" for\nmillions of commits, it is likely that the length of the formatted\ndate string will stay the same or close to the same (yeah, I know\n\"Wednesday\" would be longer than \"Monday\").\n\nAnswering myself to my earlier question, the reason is because I was\nworried what happens when given fmt is a malformed strftime format\nspecifier. Perhaps it ends with a lone % and \"% \" may format to\nsomething unexpected, or something.\n\nAre we checking an error from strftime(3)?\n"},{"id":"265254","messageId":"CAPig+cTbOkDJDwhKEbboCQkeossBzvPmw=goag01Sbsw2pisSA@mail.gmail.com","threadId":"39722","inReplyTo":"20150630132653.GA25742@peff.net","subject":"Re: [PATCH 3/3] introduce \"format\" date-mode","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-06-30T17:05:37Z","receivedAt":"2015-06-30T17:05:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jun 30, 2015 at 9:26 AM, Jeff King wrote:\n> On Mon, Jun 29, 2015 at 06:22:47PM -0400, Eric Sunshine wrote:\n>\n>> Clients of strbuf rightly expect the buffer to grow as needed in\n>> order to complete the requested operation. It is, therefore, both\n>> weird and expectation-breaking for strbuf_addftime() to lack this\n>> behavior. Worse, it doesn't even signal when the format has failed\n>> due to insufficient buffer space.\n>>\n>> How about taking this approach (or something similar), instead, which\n>> grows the strbuf as needed?\n>\n> Here's a patch, on top of jk/date-mode-format (I think it would also be\n> fine to just squash into the tip commit; the explanation in the commit\n> message is sufficiently mirrored in the code comment).\n\nAs a logical change in itself, I could also see introduction of\nstrbuf_addftime() split out into its own patch (with this patch\nsquashed in). Either way, it's a welcome improvement over the\noriginal.\n\n> -- >8 --\n> Subject: [PATCH] strbuf: make strbuf_addftime more robust\n>\n> The return value of strftime is poorly designed; when it\n> returns 0, the caller cannot tell if the buffer was not\n> large enough, or if the output was actually 0 bytes. In the\n> original implementation of strbuf_addftime, we simply punted\n> and guessed that our 128-byte hint would be large enough.\n>\n> We can do better, though, if we're willing to treat strftime\n> like less of a black box. We can munge the incoming format\n> to make sure that it never produces 0-length output, and\n> then \"fix\" the resulting output. That lets us reliably grow\n> the buffer based on strftime's return value.\n>\n> Clever-idea-by: Eric Sunshine \n> Signed-off-by: Jeff King \n> ---\n> strbuf.c | 38 +++++++++++++++++++++-----------------\n> t/t6300-for-each-ref.sh | 10 ++++++++++\n> 2 files changed, 31 insertions(+), 17 deletions(-)\n>\n> diff --git a/strbuf.c b/strbuf.c\n> index a7ba028..e5e7370 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -712,29 +712,33 @@ char *xstrfmt(const char *fmt, ...)\n>\n> void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)\n> {\n> + size_t hint = 128;\n> size_t len;\n>\n> - /*\n> - * strftime reports \"0\" if it could not fit the result in the buffer.\n> - * Unfortunately, it also reports \"0\" if the requested time string\n> - * takes 0 bytes. So if we were to probe and grow, we have to choose\n> - * some arbitrary cap beyond which we guess that the format probably\n> - * just results in a 0-length output. Since we have to choose some\n> - * reasonable cap anyway, and since it is not that big, we may\n> - * as well just grow to their in the first place.\n> - */\n> - strbuf_grow(sb, 128);\n> + if (!*fmt)\n> + return;\n> +\n> + strbuf_grow(sb, hint);\n> len = strftime(sb->buf + sb->len, sb->alloc - sb->len, fmt, tm);\n>\n> if (!len) {\n> /*\n> - * Either we failed, or the format actually produces a 0-length\n> - * output. There's not much we can do, so we leave it blank.\n> - * However, the output array is left in an undefined state, so\n> - * we must re-assert our NUL terminator.\n> + * strftime reports \"0\" if it could not fit the result in the buffer.\n> + * Unfortunately, it also reports \"0\" if the requested time string\n> + * takes 0 bytes. So our strategy is to munge the format so that the\n> + * output contains at least one character, and then drop the extra\n> + * character before returning.\n> */\n> - sb->buf[sb->len] = '\\0';\n> - } else {\n> - sb->len += len;\n> + struct strbuf munged_fmt = STRBUF_INIT;\n> + strbuf_addf(&munged_fmt, \"%s \", fmt);\n> + while (!len) {\n> + hint *= 2;\n> + strbuf_grow(sb, hint);\n> + len = strftime(sb->buf + sb->len, sb->alloc - sb->len,\n> + munged_fmt.buf, tm);\n> + }\n> + strbuf_release(&munged_fmt);\n> + len--; /* drop munged space */\n> }\n> + strbuf_setlen(sb, sb->len + len);\n> }\n> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n> index c7f368c..7c9bec7 100755\n> --- a/t/t6300-for-each-ref.sh\n> +++ b/t/t6300-for-each-ref.sh\n> @@ -235,6 +235,16 @@ test_expect_success 'Check format of strftime date fields' '\n> test_cmp expected actual\n> '\n>\n> +test_expect_success 'exercise strftime with odd fields' '\n> + echo >expected &&\n> + git for-each-ref --format=\"%(authordate:format:)\" refs/heads >actual &&\n> + test_cmp expected actual &&\n> + long=\"long format -- $_z40$_z40$_z40$_z40$_z40$_z40$_z40\" &&\n> + echo $long >expected &&\n> + git for-each-ref --format=\"%(authordate:format:$long)\" refs/heads >actual &&\n> + test_cmp expected actual\n> +'\n> +\n> cat >expected <<\\EOF\n> refs/heads/master\n> refs/remotes/origin/master\n> --\n> 2.5.0.rc0.336.g8460790\n"},{"id":"265258","messageId":"CAPig+cS3_KVHvDL=nW+9dDveAp3MvAtjKyChPAOVXGfwZCQ5Ag@mail.gmail.com","threadId":"39722","inReplyTo":"xmqqr3ot6s9u.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/3] introduce \"format\" date-mode","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-06-30T17:48:11Z","receivedAt":"2015-06-30T17:48:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jun 30, 2015 at 1:05 PM, Junio C Hamano wrote:\n> Answering myself to my earlier question, the reason is because I was\n> worried what happens when given fmt is a malformed strftime format\n> specifier. Perhaps it ends with a lone % and \"% \" may format to\n> something unexpected, or something.\n\nThat's a good point. I had considered prepending the extra character\n(space) rather than appending it but eventually rejected it to avoid\nthe expense of shifting the characters down by one before returning\nthe formatted string.\n\nHowever, is it our responsibility to guard against a malformed format?\nPOSIX doesn't state the behavior of \"% \", so I don't think we are any\nworse off by appending space to a malformed format ending with \"%\"\nsince the malformed format could wreak havoc even without our\ntransformation.\n\n> Are we checking an error from strftime(3)?\n\nAccording to the \"BUGS\" section in POSIX:\n\n If the output string would exceed max bytes, errno is not set.\n This makes it impossible to distinguish this error case from\n cases where the format string legitimately produces a zero-length\n output string. POSIX.1-2001 does not specify any errno settings\n for strftime().\n\nSo, there does not seem to be a point in checking 'errno'.\n"},{"id":"265259","messageId":"20150630175011.GA5349@peff.net","threadId":"39722","inReplyTo":"xmqq381988ud.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/3] introduce \"format\" date-mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-30T17:50:11Z","receivedAt":"2015-06-30T17:50:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 30, 2015 at 09:22:18AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> \tstrbuf_addf(&f, \"%s \", fmt);\n> >\n> > Basically I was trying to avoid making any assumptions about exactly how\n> > strftime works. But presumably \"stick a space in the format\" is a\n> > universally reasonable thing to do. It's a hack, but it's contained to\n> > the function.\n> \n> Why can't I shake this feeling that (\" %s\", fmt), i.e. prepend not\n> append, is the safer thing to do than to append?\n\nBecause then removing the extra space involves `memmove` of the buffer,\nrather than just shortening the length by one.\n\n-Peff\n"},{"id":"265260","messageId":"20150630175852.GB5349@peff.net","threadId":"39722","inReplyTo":"CAPig+cTXc_RXbOAOaF2MFjrg+DSet=g0XQMZY0ErMYAmNVSV+g@mail.gmail.com","subject":"Re: [PATCH 3/3] introduce \"format\" date-mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-30T17:58:53Z","receivedAt":"2015-06-30T17:58:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 30, 2015 at 12:58:33PM -0400, Eric Sunshine wrote:\n\n> > Basically I was trying to avoid making any assumptions about exactly how\n> > strftime works. But presumably \"stick a space in the format\" is a\n> > universally reasonable thing to do. It's a hack, but it's contained to\n> > the function.\n> \n> I don't think we're making any assumptions about strftime(). POSIX states:\n> \n> The format string consists of zero or more conversion\n> specifications and ordinary characters. [...] All ordinary\n> characters (including the terminating NUL character) are copied\n> unchanged into the array.\n> \n> So, we seem to be on solid footing with this approach (even though\n> it's a localized hack).\n\nYeah, sorry I wasn't more clear. I had originally been thinking of\nmaking assumptions like \"well, %c cannot ever be blank\". But your\nsolution does not suffer from that level of knowledge. I think it is\nreasonably clever.\n\n> Yeah, I toyed with the idea of increasing the requested amount each\n> iteration but wanted to keep the example simple, thus left it out.\n> However, for some reason, I was thinking that strbuf_grow() was\n> unconditionally expanding the buffer by the requested amount rather\n> than merely ensuring that that amount was availabile, so the amount\n> clearly needs to be increased on each iteration. Thanks for pointing\n> that out.\n\nFWIW, I had to look at it to double-check. I've often made the same\nmistake.\n\n> Beyond the extra allocation, I was also concerned about the\n> sledgehammer approach of \"%s \" to append a single character when there\n> are much less expensive ways to do so.\n\nI don't think there's any other way. We have to feed a contiguous buffer\nto strftime, and we don't own the buffer, so we have to make a new copy.\n\n-Peff\n"},{"id":"265262","messageId":"CAPig+cSJXZjrZ=OcV+C6bkCEHQFrw35-cjQ3At_Qo5KrpYxaQA@mail.gmail.com","threadId":"39722","inReplyTo":"20150630175852.GB5349@peff.net","subject":"Re: [PATCH 3/3] introduce \"format\" date-mode","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-06-30T18:13:53Z","receivedAt":"2015-06-30T18:13:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jun 30, 2015 at 1:58 PM, Jeff King wrote:\n> On Tue, Jun 30, 2015 at 12:58:33PM -0400, Eric Sunshine wrote:\n>> Beyond the extra allocation, I was also concerned about the\n>> sledgehammer approach of \"%s \" to append a single character when there\n>> are much less expensive ways to do so.\n>\n> I don't think there's any other way. We have to feed a contiguous buffer\n> to strftime, and we don't own the buffer, so we have to make a new copy.\n\nSorry, I meant that the interpolation expense of \"%s \". A cheaper (but\nmore verbose) alternative might be:\n\n size_t n = strlen(fmt);\n const char *f = xmalloc(n + 2);\n strcpy(f, fmt);\n f[n] = ' ';\n f[n + 1] = '\\0';\n ...\n free(f);\n\nor something similar.\n"},{"id":"265266","messageId":"20150630191758.GA6845@peff.net","threadId":"39722","inReplyTo":"xmqqr3ot6s9u.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/3] introduce \"format\" date-mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-30T19:17:58Z","receivedAt":"2015-06-30T19:17:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 30, 2015 at 10:05:33AM -0700, Junio C Hamano wrote:\n\n> > I'd guess most cases will fit in 128 bytes and never even hit this code\n> > path. You could also get fancier and start the buffer smaller, but only\n> > do the fmt hack when we cross a threshold.\n> \n> I'd assume that the \"hint\" thing will persist across calls somehow?\n> In an invocation of \"git log --date=format:\" for\n> millions of commits, it is likely that the length of the formatted\n> date string will stay the same or close to the same (yeah, I know\n> \"Wednesday\" would be longer than \"Monday\").\n\nI hadn't thought about that. It could persist, but I don't think this is\nnecessarily the right place to do it. For two reasons:\n\n 1. You have no idea in strbuf_addftime if it's the same fmt being\n added over and over. This is the wrong place to make that\n optimization.\n\n 2. If you are interested in efficiency in a loop, then you should be\n reusing the same strbuf over and over, and avoiding the extra\n allocation in the first place. And that is indeed what we do for\n \"git log --date\", as we will always use the same static-local\n buffer in show_date().\n\n> Answering myself to my earlier question, the reason is because I was\n> worried what happens when given fmt is a malformed strftime format\n> specifier. Perhaps it ends with a lone % and \"% \" may format to\n> something unexpected, or something.\n> \n> Are we checking an error from strftime(3)?\n\nPOSIX doesn't define any errno values for strftime (and in fact says \"No\nerrors are defined\"). The return value description for POSIX (and the\nglibc manpage) talk about only whether or not the output fits. However,\nPOSIX does say \"If a conversion specifier is not one of the above, the\nbehavior is undefined\".\n\nSo certainly I could imagine an implementation that returns \"0\" when you\nfeed it a bogus value. If you (as a user) feed us crap to give to\nstrftime, I am not particularly concerned with whether you get crap out.\nMy main concern is that it would return \"0\" and we would loop forever.\nOTOH, I think any sane implementation would simply copy unknown\nplaceholders out (certainly glibc does that). So I think we could simply\nconsider it a quality of implementation issue, and deal with any\nparticular crappy implementations if and when they get reported. We\ncould add something tricky (like \"--date=format:%\") to the test suite to\nmake it likelier to catch such a thing.\n\n-Peff\n"},{"id":"265267","messageId":"20150630192215.GB6845@peff.net","threadId":"39722","inReplyTo":"CAPig+cSJXZjrZ=OcV+C6bkCEHQFrw35-cjQ3At_Qo5KrpYxaQA@mail.gmail.com","subject":"Re: [PATCH 3/3] introduce \"format\" date-mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-30T19:22:15Z","receivedAt":"2015-06-30T19:22:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 30, 2015 at 02:13:53PM -0400, Eric Sunshine wrote:\n\n> Sorry, I meant that the interpolation expense of \"%s \". A cheaper (but\n> more verbose) alternative might be:\n> \n> size_t n = strlen(fmt);\n> const char *f = xmalloc(n + 2);\n> strcpy(f, fmt);\n> f[n] = ' ';\n> f[n + 1] = '\\0';\n> ...\n> free(f);\n> \n> or something similar.\n\nI think you're probably getting into premature optimization here. Using\nstrbuf_vaddf should be within the same order of magnitude of\ninstructions (and I think we should almost never hit this code path\nanyway, because we'll be reading into a static strbuf generally which\nwill come pre-sized to hold a date).\n\n-Peff\n"},{"id":"265268","messageId":"xmqqbnfxc86b.fsf@gitster.dls.corp.google.com","threadId":"39722","inReplyTo":"20150630175011.GA5349@peff.net","subject":"Re: [PATCH 3/3] introduce \"format\" date-mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-30T19:23:08Z","receivedAt":"2015-06-30T19:23:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Jun 30, 2015 at 09:22:18AM -0700, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> >> \tstrbuf_addf(&f, \"%s \", fmt);\n>> >\n>> > Basically I was trying to avoid making any assumptions about exactly how\n>> > strftime works. But presumably \"stick a space in the format\" is a\n>> > universally reasonable thing to do. It's a hack, but it's contained to\n>> > the function.\n>> \n>> Why can't I shake this feeling that (\" %s\", fmt), i.e. prepend not\n>> append, is the safer thing to do than to append?\n>\n> Because then removing the extra space involves `memmove` of the buffer,\n> rather than just shortening the length by one.\n\nThat does not explain why I feel the other way is safer, though ;-)\n\nIn any case, responding myself to my other question, strftime(3)\ndoes not define any useful error information even for a case where\nyou feed nonsense format to it, so it is even not possible to\nprotect ourselves by checking errno or doing something similar X-<.\n"},{"id":"265269","messageId":"20150630193319.GA8365@peff.net","threadId":"39722","inReplyTo":"xmqqbnfxc86b.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/3] introduce \"format\" date-mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-30T19:33:19Z","receivedAt":"2015-06-30T19:33:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 30, 2015 at 12:23:08PM -0700, Junio C Hamano wrote:\n\n> >> Why can't I shake this feeling that (\" %s\", fmt), i.e. prepend not\n> >> append, is the safer thing to do than to append?\n> >\n> > Because then removing the extra space involves `memmove` of the buffer,\n> > rather than just shortening the length by one.\n> \n> That does not explain why I feel the other way is safer, though ;-)\n\nSorry, I think I read it as \"saner\". ;)\n\n> In any case, responding myself to my other question, strftime(3)\n> does not define any useful error information even for a case where\n> you feed nonsense format to it, so it is even not possible to\n> protect ourselves by checking errno or doing something similar X-<.\n\nI do think \" %\" is probably safer than \"% \" in that it is less likely to\ncause weird output from strftime (e.g., I could see an implementation\ntreat \"%X\", where \"X\" is unknown, as just \"X\", but treat a trailing \"%\"\nas a raw \"%\"). But I doubt there is an implementation that would return\n\"0\" for one but not the other (anything's possible of course, but we are\ndeep in the realm of guessing how systems might implement the\nstandard, so it's really our best guess until somebody can present a\nparticular data point; I've looked only at glibc and it is sane).\n\n-Peff\n"},{"id":"265766","messageId":"xmqqbnfn3dsb.fsf@gitster.dls.corp.google.com","threadId":"39722","inReplyTo":"20150625165501.GB23503@peff.net","subject":"Re: [PATCH 2/3] convert \"enum date_mode\" into a struct","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-07T20:37:08Z","receivedAt":"2015-07-07T20:37:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... However, the tricky case is where we use the\n> enum labels as constants, like:\n>\n> show_date(t, tz, DATE_NORMAL);\n>\n> Ideally we could say:\n>\n> show_date(t, tz, &{ DATE_NORMAL });\n>\n> but of course C does not allow that.\n> ...\n> 3. Provide a wrapper that generates the correct struct on\n> the fly. The big downside is that we end up pointing to\n> a single global, which makes our wrapper non-reentrant.\n> But show_date is already not reentrant, so it does not\n> matter.\n>\n> This patch implements 3, along with a minor macro to keep\n> the size of the callers sane.\n\nAnother big downside is that DATE_NORMAL is defined to be \"0\".\n\nThis makes it very cumbersome to merge a side branch that uses an\noutdated definition of show_date() and its friends and tell them\nto show date normally. The compiler does not help detecting\nplaces that need to be adjusted during merge and instead just pass\na NULL pointer as a pointer to the new struct.\n"},{"id":"265767","messageId":"20150707204837.GA15483@peff.net","threadId":"39722","inReplyTo":"xmqqbnfn3dsb.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/3] convert \"enum date_mode\" into a struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-07-07T20:48:37Z","receivedAt":"2015-07-07T20:48:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 07, 2015 at 01:37:08PM -0700, Junio C Hamano wrote:\n\n> > 3. Provide a wrapper that generates the correct struct on\n> > the fly. The big downside is that we end up pointing to\n> > a single global, which makes our wrapper non-reentrant.\n> > But show_date is already not reentrant, so it does not\n> > matter.\n> >\n> > This patch implements 3, along with a minor macro to keep\n> > the size of the callers sane.\n> \n> Another big downside is that DATE_NORMAL is defined to be \"0\".\n> \n> This makes it very cumbersome to merge a side branch that uses an\n> outdated definition of show_date() and its friends and tell them\n> to show date normally. The compiler does not help detecting\n> places that need to be adjusted during merge and instead just pass\n> a NULL pointer as a pointer to the new struct.\n\nMy assumption was that using the raw \"0\" is something we would frowned\nupon in new code. There was a single historical instance that I fixed in\nthe series, but I wouldn't expect new ones (and actually, that instance\nwas \"1\", which would be caught by the compiler).\n\nHowever, if you're concerned, I think we could have show_date massage a\nNULL date, like:\n\ndiff --git a/date.c b/date.c\nindex 8f91569..a04d089 100644\n--- a/date.c\n+++ b/date.c\n@@ -173,6 +173,10 @@ const char *show_date(unsigned long time, int tz, const struct date_mode *mode)\n {\n \tstruct tm *tm;\n \tstatic struct strbuf timebuf = STRBUF_INIT;\n+\tstatic const struct fallback_mode = { DATE_NORMAL };\n+\n+\tif (!mode)\n+\t\tmode = &fallback_mode;\n \n \tif (mode->type == DATE_RAW) {\n \t\tstrbuf_reset(&timebuf);\n\n\nThat would also allow people to explicitly call:\n\n show_date(t, tz, NULL);\n\nto get the default format, though I personally prefer spelling it out.\n\nI guess we _could_ introduce:\n\n #define DATE_MODE(x) ((struct date_mode *)(x))\n\nand then take any numeric value, under the assumption that the first\npage of memory will never be a valid pointer:\n\ndiff --git a/date.c b/date.c\nindex 8f91569..f388fee 100644\n--- a/date.c\n+++ b/date.c\n@@ -173,6 +173,18 @@ const char *show_date(unsigned long time, int tz, const struct date_mode *mode)\n {\n \tstruct tm *tm;\n \tstatic struct strbuf timebuf = STRBUF_INIT;\n+\tstruct date_mode fallback;\n+\n+\t/* hysterical compatibility */\n+\tif (mode < 1024) {\n+\t\tif (mode == DATE_STRFTIME)\n+\t\t\tdie(\"BUG: nice try\");\n+\t\tfallback.type = mode;\n+\t\tmode = &fallback;\n+\t}\n+\n+\tif (!mode)\n+\t\tmode = &fallback_mode;\n \n \tif (mode->type == DATE_RAW) {\n \t\tstrbuf_reset(&timebuf);\n\nThat's kind of nasty, but at least it's hidden from the callers.\n\n-Peff\n"},{"id":"265768","messageId":"xmqq4mlf3cgf.fsf@gitster.dls.corp.google.com","threadId":"39722","inReplyTo":"20150707204837.GA15483@peff.net","subject":"Re: [PATCH 2/3] convert \"enum date_mode\" into a struct","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-07T21:05:52Z","receivedAt":"2015-07-07T21:05:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> My assumption was that using the raw \"0\" is something we would frowned\n> upon in new code. There was a single historical instance that I fixed in\n> the series, but I wouldn't expect new ones (and actually, that instance\n> was \"1\", which would be caught by the compiler).\n\nThat is not the problem.\n\nThe code on the side branch may add a new callsite, something like\nthis:\n\n\tshow_ident_date(&ident_split, DATE_NORMAL);\n\nbased on the current codebase (e.g. 'master' as of today).\n\nThe merge goes cleanly, it compiles, even though the new function\nsignature of show_ident_date(), similar to the updated show_date(),\ntakes a pointer to a struct where they used to take DATE_$format\nconstants.\n\nAnd that is because DATE_NORMAL is defined to be 0; we can claim\nthat the compiler is being stupid to take one of the enum\ndate_mode_type values that happens to be 0 and misinterpret it as\nthe program wanted to pass a NULL pointer to a structure, but that\nis not what happened.\n\n> However, if you're concerned, I think we could have show_date massage a\n> NULL date, like:\n>\n> diff --git a/date.c b/date.c\n> index 8f91569..a04d089 100644\n> --- a/date.c\n> +++ b/date.c\n> @@ -173,6 +173,10 @@ const char *show_date(unsigned long time, int tz, const struct date_mode *mode)\n> {\n> \tstruct tm *tm;\n> \tstatic struct strbuf timebuf = STRBUF_INIT;\n> +\tstatic const struct fallback_mode = { DATE_NORMAL };\n\nYes, that is nasty. Renumbering the enum to begin with 1 may be a\nmuch saner solution, unless somebody does\n\n\tif (!mode->type)\n \t/* we know DATE_NORMAL is zero, he he */\n\t do the normal thing;\n\nIn any case, I did another evil merge to fix it.\n"},{"id":"265769","messageId":"20150707211304.GA16683@peff.net","threadId":"39722","inReplyTo":"xmqq4mlf3cgf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/3] convert \"enum date_mode\" into a struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-07-07T21:13:04Z","receivedAt":"2015-07-07T21:13:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 07, 2015 at 02:05:52PM -0700, Junio C Hamano wrote:\n\n> And that is because DATE_NORMAL is defined to be 0; we can claim\n> that the compiler is being stupid to take one of the enum\n> date_mode_type values that happens to be 0 and misinterpret it as\n> the program wanted to pass a NULL pointer to a structure, but that\n> is not what happened.\n\nAh, I didn't think the compiler would coerce an enum into a pointer\nconstant. That seems kind of insane. But it is indeed what gcc does.\n\nIn that case, we can indeed do the NULL-pointer thing I mentioned. Which\nis not even _that_ ugly; it follows the standard.\n\nThe \"cast DATE_RELATIVE to a pointer and uncast it on the other side\"\nthing _does_ violate the standard. It is not needed for this, but it\nwould make the DATE_MODE() macro reentrant.\n\n> > +\tstatic const struct fallback_mode = { DATE_NORMAL };\n> \n> Yes, that is nasty. Renumbering the enum to begin with 1 may be a\n> much saner solution, unless somebody does\n\nI am worried more about somebody who does memset(0) on a struct, and\nexpects that to default to DATE_NORMAL.\n\n> In any case, I did another evil merge to fix it.\n\nOK. Do you want to leave it be, then, or would you prefer me to do the\nNULL fallback? Or we could bump the enum to start with 1, and then\nexplicitly treat \"0\" as a synonym for DATE_NORMAL (in case it comes in\nthrough a memset or similar).\n\n-Peff\n"},{"id":"265770","messageId":"xmqqzj371x9s.fsf@gitster.dls.corp.google.com","threadId":"39722","inReplyTo":"20150707211304.GA16683@peff.net","subject":"Re: [PATCH 2/3] convert \"enum date_mode\" into a struct","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-07T21:19:11Z","receivedAt":"2015-07-07T21:19:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> OK. Do you want to leave it be, then, or would you prefer me to do the\n> NULL fallback? Or we could bump the enum to start with 1, and then\n> explicitly treat \"0\" as a synonym for DATE_NORMAL (in case it comes in\n> through a memset or similar).\n\nI didn't think about the memset() initialization, and keeping the\nnormal case to be 0 makes tons of sense.\n\nI'd prefer to see the stale code dump core rather than leaving it\nstale without anybody noticing. Hopefully the date-mode change can\nhit 'master' fairly soon after the upcoming release, so unless a fix\nthat involves show_date() need to be written and applied to 2.4\ncodebase and upwards at the same time, I do not think it is a huge\nissue.\n\nThanks.\n"},{"id":"266529","messageId":"CAPig+cT_3pFH6XKBO1F2rbNyiGz5NgLMMiA_T+NWBroW5Sj7cg@mail.gmail.com","threadId":"39722","inReplyTo":"20150630132653.GA25742@peff.net","subject":"Re: [PATCH 3/3] introduce \"format\" date-mode","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-07-21T00:41:08Z","receivedAt":"2015-07-21T00:41:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jun 30, 2015 at 9:26 AM, Jeff King wrote:\n> On Mon, Jun 29, 2015 at 06:22:47PM -0400, Eric Sunshine wrote:\n>> Clients of strbuf rightly expect the buffer to grow as needed in\n>> order to complete the requested operation. It is, therefore, both\n>> weird and expectation-breaking for strbuf_addftime() to lack this\n>> behavior. Worse, it doesn't even signal when the format has failed\n>> due to insufficient buffer space.\n>>\n>> How about taking this approach (or something similar), instead, which\n>> grows the strbuf as needed?\n>\n> Here's a patch, on top of jk/date-mode-format (I think it would also be\n> fine to just squash into the tip commit; the explanation in the commit\n> message is sufficiently mirrored in the code comment).\n\nWhile cleaning up old local branches, I noticed that, although the\njk/date-mode-format topic[1] made it into 'next' (and will be merged\nto 'master' according to \"What's cooking\"[2]), the below follow-on\npatch[3] which improves strbuf_addftime() never got picked up. Was\nthis omission intentional? Based upon the discussion[4], I was under\nthe impression that the patch was considered reasonably acceptable\n(and did not worsen problems with bogus format strings -- which are\nbogus anyway).\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/272658/focus=272695\n[2]: http://news.gmane.org/gmane.comp.version-control.git\n[3]: http://article.gmane.org/gmane.comp.version-control.git/273061\n[4]: http://thread.gmane.org/gmane.comp.version-control.git/272658/focus=273026\n\n> -- >8 --\n> Subject: [PATCH] strbuf: make strbuf_addftime more robust\n>\n> The return value of strftime is poorly designed; when it\n> returns 0, the caller cannot tell if the buffer was not\n> large enough, or if the output was actually 0 bytes. In the\n> original implementation of strbuf_addftime, we simply punted\n> and guessed that our 128-byte hint would be large enough.\n>\n> We can do better, though, if we're willing to treat strftime\n> like less of a black box. We can munge the incoming format\n> to make sure that it never produces 0-length output, and\n> then \"fix\" the resulting output. That lets us reliably grow\n> the buffer based on strftime's return value.\n>\n> Clever-idea-by: Eric Sunshine \n> Signed-off-by: Jeff King \n> ---\n> strbuf.c | 38 +++++++++++++++++++++-----------------\n> t/t6300-for-each-ref.sh | 10 ++++++++++\n> 2 files changed, 31 insertions(+), 17 deletions(-)\n>\n> diff --git a/strbuf.c b/strbuf.c\n> index a7ba028..e5e7370 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -712,29 +712,33 @@ char *xstrfmt(const char *fmt, ...)\n>\n> void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)\n> {\n> + size_t hint = 128;\n> size_t len;\n>\n> - /*\n> - * strftime reports \"0\" if it could not fit the result in the buffer.\n> - * Unfortunately, it also reports \"0\" if the requested time string\n> - * takes 0 bytes. So if we were to probe and grow, we have to choose\n> - * some arbitrary cap beyond which we guess that the format probably\n> - * just results in a 0-length output. Since we have to choose some\n> - * reasonable cap anyway, and since it is not that big, we may\n> - * as well just grow to their in the first place.\n> - */\n> - strbuf_grow(sb, 128);\n> + if (!*fmt)\n> + return;\n> +\n> + strbuf_grow(sb, hint);\n> len = strftime(sb->buf + sb->len, sb->alloc - sb->len, fmt, tm);\n>\n> if (!len) {\n> /*\n> - * Either we failed, or the format actually produces a 0-length\n> - * output. There's not much we can do, so we leave it blank.\n> - * However, the output array is left in an undefined state, so\n> - * we must re-assert our NUL terminator.\n> + * strftime reports \"0\" if it could not fit the result in the buffer.\n> + * Unfortunately, it also reports \"0\" if the requested time string\n> + * takes 0 bytes. So our strategy is to munge the format so that the\n> + * output contains at least one character, and then drop the extra\n> + * character before returning.\n> */\n> - sb->buf[sb->len] = '\\0';\n> - } else {\n> - sb->len += len;\n> + struct strbuf munged_fmt = STRBUF_INIT;\n> + strbuf_addf(&munged_fmt, \"%s \", fmt);\n> + while (!len) {\n> + hint *= 2;\n> + strbuf_grow(sb, hint);\n> + len = strftime(sb->buf + sb->len, sb->alloc - sb->len,\n> + munged_fmt.buf, tm);\n> + }\n> + strbuf_release(&munged_fmt);\n> + len--; /* drop munged space */\n> }\n> + strbuf_setlen(sb, sb->len + len);\n> }\n> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n> index c7f368c..7c9bec7 100755\n> --- a/t/t6300-for-each-ref.sh\n> +++ b/t/t6300-for-each-ref.sh\n> @@ -235,6 +235,16 @@ test_expect_success 'Check format of strftime date fields' '\n> test_cmp expected actual\n> '\n>\n> +test_expect_success 'exercise strftime with odd fields' '\n> + echo >expected &&\n> + git for-each-ref --format=\"%(authordate:format:)\" refs/heads >actual &&\n> + test_cmp expected actual &&\n> + long=\"long format -- $_z40$_z40$_z40$_z40$_z40$_z40$_z40\" &&\n> + echo $long >expected &&\n> + git for-each-ref --format=\"%(authordate:format:$long)\" refs/heads >actual &&\n> + test_cmp expected actual\n> +'\n> +\n> cat >expected <<\\EOF\n> refs/heads/master\n> refs/remotes/origin/master\n> --\n> 2.5.0.rc0.336.g8460790\n>\n"},{"id":"266531","messageId":"20150721011931.GB28255@peff.net","threadId":"39722","inReplyTo":"CAPig+cT_3pFH6XKBO1F2rbNyiGz5NgLMMiA_T+NWBroW5Sj7cg@mail.gmail.com","subject":"Re: [PATCH 3/3] introduce \"format\" date-mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-07-21T01:19:31Z","receivedAt":"2015-07-21T01:19:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 20, 2015 at 08:41:08PM -0400, Eric Sunshine wrote:\n\n> > Here's a patch, on top of jk/date-mode-format (I think it would also be\n> > fine to just squash into the tip commit; the explanation in the commit\n> > message is sufficiently mirrored in the code comment).\n> \n> While cleaning up old local branches, I noticed that, although the\n> jk/date-mode-format topic[1] made it into 'next' (and will be merged\n> to 'master' according to \"What's cooking\"[2]), the below follow-on\n> patch[3] which improves strbuf_addftime() never got picked up. Was\n> this omission intentional? Based upon the discussion[4], I was under\n> the impression that the patch was considered reasonably acceptable\n> (and did not worsen problems with bogus format strings -- which are\n> bogus anyway).\n\nThanks for noticing. I do think the patch you quoted (to loop and grow\nthe strbuf) is a good change. The original code would easily bite\nsomebody with a really large date format, whereas this should work\nsanely everywhere, short of malformed inputs. And even then, I'd expect\nreasonable behavior on most systems. The obvious thing to worry about is\na system where feeding a malformed \"% \" causes strftime to return 0, no\nmatter what, and we reallocated and loop forever. But:\n\n 1. I don't even know if such a system exists.\n\n 2. We probably would blow up on malloc() eventually, so it wouldn't\n even be a \"real\" infinite loop.\n\nSo I think the worst case is probably that we get a report later on from\nsomebody on an arcane system that says \"I fed crap to --date=format, and\nmy git died with an out-of-memory error\", and then we figure out exactly\n_how_ their system is weird and deal with it then.\n\n-Peff\n"}]}