{"thread":{"id":"17921","subject":"[PATCH] Make git blame date output format configurable, a la git log","startedAt":"2009-02-20T13:24:12Z","lastAt":"2009-02-20T17:18:25Z","messageCount":9,"participants":["eletuchy@gmail.com","Johannes Schindelin","Jeff King","Eugene Letuchy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"105606","messageId":"1235136252-29649-1-git-send-email-eletuchy@gmail.com","threadId":"17921","inReplyTo":null,"subject":"[PATCH] Make git blame date output format configurable, a la git log","fromName":"","fromEmail":"eletuchy@gmail.com","sentAt":"2009-02-20T13:24:12Z","receivedAt":"2009-02-20T13:24:12Z","isPatch":true,"sender":{"key":"eletuchy@gmail.com","avatar":null},"body":"From: Eugene Letuchy \n\nAdds the following:\n - git config value blame.date that expects one of the git log date\n formats ({relative,local,default,iso,rfc,short})\n - git blame command line option --date-format expects one of the git\n log date formats ({relative,local,default,iso,rfc,short})\n - documentation in blame-options.txt\n - git blame uses the appropriate date.c functions and enums to\n make sense of the date format and provide appropriate data\n\nThe tests pass. The mailmap test needed to be modified to expect iso\nformatted blames rather than the new \"default\".\n\nSigned-off-by: Eugene Letuchy \n---\n Documentation/blame-options.txt | 6 ++++++\n builtin-blame.c | 31 ++++++++++++++++++-------------\n t/t4203-mailmap.sh | 2 +-\n 3 files changed, 25 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\nindex 1ab1b96..75663ec 100644\n--- a/Documentation/blame-options.txt\n+++ b/Documentation/blame-options.txt\n@@ -63,6 +63,12 @@ of lines before or after the line given by .\n \ttree copy has the contents of the named file (specify\n \t`-` to make the command read from the standard input).\n \n+--date-format ::\n+\tThe value is one of the following alternatives:\n+\t{relative,local,default,iso,rfc,short}. The default format\n+\tcan be set using the blame.date config variable. See the\n+\tdiscussion of the --date option at linkgit:git-log[1].\n+\n -M||::\n \tDetect moving lines in the file as well. When a commit\n \tmoves a block of lines in a file (e.g. the original file\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex 114a214..9ebab43 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -1,5 +1,5 @@\n /*\n- * Pickaxe\n+ * Blame / Pickaxe\n *\n * Copyright (c) 2006, Junio C Hamano\n */\n@@ -40,6 +40,9 @@ static int reverse;\n static int blank_boundary;\n static int incremental;\n static int xdl_opts = XDF_NEED_MINIMAL;\n+\n+static enum date_mode date_mode;\n+\n static struct string_list mailmap;\n \n #ifndef DEBUG\n@@ -1507,9 +1510,7 @@ static const char *format_time(unsigned long time, const char *tz_str,\n \t\t\t int show_raw_time)\n {\n \tstatic char time_buf[128];\n-\ttime_t t = time;\n-\tint minutes, tz;\n-\tstruct tm *tm;\n+\tint tz;\n \n \tif (show_raw_time) {\n \t\tsprintf(time_buf, \"%lu %s\", time, tz_str);\n@@ -1517,15 +1518,7 @@ static const char *format_time(unsigned long time, const char *tz_str,\n \t}\n \n \ttz = atoi(tz_str);\n-\tminutes = tz < 0 ? -tz : tz;\n-\tminutes = (minutes / 100)*60 + (minutes % 100);\n-\tminutes = tz < 0 ? -minutes : minutes;\n-\tt = time + minutes * 60;\n-\ttm = gmtime(&t);\n-\n-\tstrftime(time_buf, sizeof(time_buf), \"%Y-%m-%d %H:%M:%S \", tm);\n-\tstrcat(time_buf, tz_str);\n-\treturn time_buf;\n+\treturn show_date(time, tz, date_mode);\n }\n \n #define OUTPUT_ANNOTATE_COMPAT\t001\n@@ -1967,6 +1960,8 @@ static void prepare_blame_range(struct scoreboard *sb,\n \n static int git_blame_config(const char *var, const char *value, void *cb)\n {\n+\tconst char *default_date_mode;\n+\n \tif (!strcmp(var, \"blame.showroot\")) {\n \t\tshow_root = git_config_bool(var, value);\n \t\treturn 0;\n@@ -1975,6 +1970,11 @@ static int git_blame_config(const char *var, const char *value, void *cb)\n \t\tblank_boundary = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"blame.date\")) {\n+\t\tgit_config_string(&default_date_mode, var, value);\n+\t\tdate_mode = parse_date_format(default_date_mode);\n+\t\treturn 0;\n+\t}\n \treturn git_default_config(var, value, cb);\n }\n \n@@ -2212,6 +2212,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tstatic int show_stats = 0;\n \tstatic const char *revs_file = NULL;\n \tstatic const char *contents_from = NULL;\n+\tstatic const char *date_format = NULL;\n \tstatic const struct option options[] = {\n \t\tOPT_BOOLEAN(0, \"incremental\", &incremental, \"Show blame entries as we find them, incrementally\"),\n \t\tOPT_BOOLEAN('b', NULL, &blank_boundary, \"Show blank SHA-1 for boundary commits (Default: off)\"),\n@@ -2228,6 +2229,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT('w', NULL, &xdl_opts, \"Ignore whitespace differences\", XDF_IGNORE_WHITESPACE),\n \t\tOPT_STRING('S', NULL, &revs_file, \"file\", \"Use revisions from instead of calling git-rev-list\"),\n \t\tOPT_STRING(0, \"contents\", &contents_from, \"file\", \"Use 's contents as the final image\"),\n+\t\tOPT_STRING(0, \"date-format\", &date_format, \"date mode\", \"Specify date formatting: relative,local,default,iso,rfc,short. .\"),\n \t\t{ OPTION_CALLBACK, 'C', NULL, &opt, \"score\", \"Find line copies within and across files\", PARSE_OPT_OPTARG, blame_copy_callback },\n \t\t{ OPTION_CALLBACK, 'M', NULL, &opt, \"score\", \"Find line movements within and across files\", PARSE_OPT_OPTARG, blame_move_callback },\n \t\tOPT_CALLBACK('L', NULL, &bottomtop, \"n,m\", \"Process only line range n,m, counting from 1\", blame_bottomtop_callback),\n@@ -2266,6 +2268,9 @@ parse_done:\n \tif (cmd_is_annotate)\n \t\toutput_option |= OUTPUT_ANNOTATE_COMPAT;\n \n+\tif (date_format)\n+\t\tdate_mode = parse_date_format(date_format);\n+\n \tif (DIFF_OPT_TST(&revs.diffopt, FIND_COPIES_HARDER))\n \t\topt |= (PICKAXE_BLAME_COPY | PICKAXE_BLAME_MOVE |\n \t\t\tPICKAXE_BLAME_COPY_HARDER);\ndiff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh\nindex 9a7d1b4..13b64dc 100755\n--- a/t/t4203-mailmap.sh\n+++ b/t/t4203-mailmap.sh\n@@ -208,7 +208,7 @@ ff859d96 (Other Author 2005-04-07 15:15:13 -0700 4) four\n EOF\n \n test_expect_success 'Blame output (complex mapping)' '\n-\tgit blame one >actual &&\n+\tgit blame --date-format=iso one >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n1.6.2.rc1.14.g397c24.dirty\n"},{"id":"105607","messageId":"alpine.DEB.1.00.0902201434460.6302@intel-tinevez-2-302","threadId":"17921","inReplyTo":"1235136252-29649-1-git-send-email-eletuchy@gmail.com","subject":"Re: [PATCH] Make git blame date output format configurable, a la git log","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-20T13:40:23Z","receivedAt":"2009-02-20T13:40:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nDisclaimer: if you are offended by constructive criticism, or likely to \nanswer with insults to the comments I offer, please stop reading this mail \nnow (and please to not answer my mail, either). :-)\n\nStill with me? Good. Nice to meet you.\n\nJust out of curiosity: why Cc: Marius? I would have expected Junio, Git's \nmaintainer.\n\nMay I suggest the commit subject to say \"as for git log\"? I mistook \"a la \ngit log\" for a change in the way git-blame works...\n\nOn Fri, 20 Feb 2009, eletuchy@gmail.com wrote:\n\n> From: Eugene Letuchy \n> \n> Adds the following:\n\nWe try to use the imperative form; from my experience it makes for an \neasier read: \"Add the following:\"\n\n> - git config value blame.date that expects one of the git log date\n> formats ({relative,local,default,iso,rfc,short})\n> - git blame command line option --date-format expects one of the git\n> log date formats ({relative,local,default,iso,rfc,short})\n> - documentation in blame-options.txt\n> - git blame uses the appropriate date.c functions and enums to\n> make sense of the date format and provide appropriate data\n> \n> The tests pass. The mailmap test needed to be modified to expect iso\n> formatted blames rather than the new \"default\".\n\nIMHO the \"The tests pass.\" should be removed.\n\nOther than that, nicely done!\n\nCiao,\nDscho\n"},{"id":"105618","messageId":"499EB647.30606@facebook.com","threadId":"17921","inReplyTo":"alpine.DEB.1.00.0902201434460.6302@intel-tinevez-2-302","subject":"Re: [PATCH] Make git blame date output format configurable, a la git log","fromName":"Eugene Letuchy","fromEmail":"eletuchy@facebook.com","sentAt":"2009-02-20T13:55:19Z","receivedAt":"2009-02-20T13:55:19Z","isPatch":true,"sender":{"key":"eletuchy@facebook.com","avatar":null},"body":"Hi Johannes,\n\nThanks for your feedback. Any comments on the .c changes?\n\nI'll modify the commit message to read as follows:\n\"\"\"\n\nAdd the following:\n - git config value blame.date that expects one of the git log date\n formats ({relative,local,default,iso,rfc,short})\n - git blame command line option --date-format expects one of the git\n log date formats ({relative,local,default,iso,rfc,short})\n - documentation in blame-options.txt\n - git blame uses the appropriate date.c functions and enums to\n make sense of the date format and provide appropriate data\n\nThe tests pass. The mailmap test needed to be modified to expect iso\nformatted blames rather than the new \"default\".\n\nSigned-off-by: Eugene Letuchy \n\"\"\"\n\n-Eugene\n\n+ cc: junio\n\nOn 2/20/09 5:40 AM, Johannes Schindelin wrote:\n> Hi,\n>\n> Disclaimer: if you are offended by constructive criticism, or likely to\n> answer with insults to the comments I offer, please stop reading this mail\n> now (and please to not answer my mail, either). :-)\n>\n> Still with me? Good. Nice to meet you.\n>\n> Just out of curiosity: why Cc: Marius? I would have expected Junio, Git's\n> maintainer.\n>\n> May I suggest the commit subject to say \"as for git log\"? I mistook \"a la\n> git log\" for a change in the way git-blame works...\n>\n> On Fri, 20 Feb 2009, eletuchy@gmail.com wrote:\n>\n>> From: Eugene Letuchy\n>>\n>> Adds the following:\n>\n> We try to use the imperative form; from my experience it makes for an\n> easier read: \"Add the following:\"\n>\n>> - git config value blame.date that expects one of the git log date\n>> formats ({relative,local,default,iso,rfc,short})\n>> - git blame command line option --date-format expects one of the git\n>> log date formats ({relative,local,default,iso,rfc,short})\n>> - documentation in blame-options.txt\n>> - git blame uses the appropriate date.c functions and enums to\n>> make sense of the date format and provide appropriate data\n>>\n>> The tests pass. The mailmap test needed to be modified to expect iso\n>> formatted blames rather than the new \"default\".\n>\n> IMHO the \"The tests pass.\" should be removed.\n>\n> Other than that, nicely done!\n>\n> Ciao,\n> Dscho\n>\n"},{"id":"105625","messageId":"499EB6CD.1060800@facebook.com","threadId":"17921","inReplyTo":"499EB647.30606@facebook.com","subject":"Re: [PATCH] Make git blame date output format configurable, a la git log","fromName":"Eugene Letuchy","fromEmail":"eletuchy@facebook.com","sentAt":"2009-02-20T13:57:33Z","receivedAt":"2009-02-20T13:57:33Z","isPatch":true,"sender":{"key":"eletuchy@facebook.com","avatar":null},"body":"Sigh. Make that:\n\"\"\"\nThe mailmap test needed to be modified to expect iso formatted blames\nrather than the new \"default\".\n\"\"\"\n\n- Eugene\n\nOn 2/20/09 5:55 AM, Eugene Letuchy wrote:\n> Hi Johannes,\n>\n> Thanks for your feedback. Any comments on the .c changes?\n>\n> I'll modify the commit message to read as follows:\n> \"\"\"\n>\n> Add the following:\n> - git config value blame.date that expects one of the git log date\n> formats ({relative,local,default,iso,rfc,short})\n> - git blame command line option --date-format expects one of the git\n> log date formats ({relative,local,default,iso,rfc,short})\n> - documentation in blame-options.txt\n> - git blame uses the appropriate date.c functions and enums to\n> make sense of the date format and provide appropriate data\n>\n> The tests pass. The mailmap test needed to be modified to expect iso\n> formatted blames rather than the new \"default\".\n>\n> Signed-off-by: Eugene Letuchy\n> \"\"\"\n>\n> -Eugene\n>\n> + cc: junio\n>\n> On 2/20/09 5:40 AM, Johannes Schindelin wrote:\n>> Hi,\n>>\n>> Disclaimer: if you are offended by constructive criticism, or likely to\n>> answer with insults to the comments I offer, please stop reading this mail\n>> now (and please to not answer my mail, either). :-)\n>>\n>> Still with me? Good. Nice to meet you.\n>>\n>> Just out of curiosity: why Cc: Marius? I would have expected Junio, Git's\n>> maintainer.\n>>\n>> May I suggest the commit subject to say \"as for git log\"? I mistook \"a la\n>> git log\" for a change in the way git-blame works...\n>>\n>> On Fri, 20 Feb 2009, eletuchy@gmail.com wrote:\n>>\n>>> From: Eugene Letuchy\n>>>\n>>> Adds the following:\n>> We try to use the imperative form; from my experience it makes for an\n>> easier read: \"Add the following:\"\n>>\n>>> - git config value blame.date that expects one of the git log date\n>>> formats ({relative,local,default,iso,rfc,short})\n>>> - git blame command line option --date-format expects one of the git\n>>> log date formats ({relative,local,default,iso,rfc,short})\n>>> - documentation in blame-options.txt\n>>> - git blame uses the appropriate date.c functions and enums to\n>>> make sense of the date format and provide appropriate data\n>>>\n>>> The tests pass. The mailmap test needed to be modified to expect iso\n>>> formatted blames rather than the new \"default\".\n>> IMHO the \"The tests pass.\" should be removed.\n>>\n>> Other than that, nicely done!\n>>\n>> Ciao,\n>> Dscho\n>>\n"},{"id":"105609","messageId":"alpine.DEB.1.00.0902201503280.6302@intel-tinevez-2-302","threadId":"17921","inReplyTo":"499EB647.30606@facebook.com","subject":"Re: [PATCH] Make git blame date output format configurable, a la git log","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-20T14:06:14Z","receivedAt":"2009-02-20T14:06:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eugene,\n\nOn Fri, 20 Feb 2009, Eugene Letuchy wrote:\n\n> Thanks for your feedback. Any comments on the .c changes?\n\nYes: they look fine to me :-)\n\n(Please excuse if I only point out things I'd like you to change, and not \npraise the rest as verbosely; the fact that I take the time to comment on \nthe patch is meant to show you that I am interested in your work; \noften I am terse because I have to squeeze commenting on patches \nin-between my day job.)\n\nCiao,\nDscho\n"},{"id":"105611","messageId":"20090220142730.GA32751@coredump.intra.peff.net","threadId":"17921","inReplyTo":"1235136252-29649-1-git-send-email-eletuchy@gmail.com","subject":"Re: [PATCH] Make git blame date output format configurable, a la git log","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-20T14:27:30Z","receivedAt":"2009-02-20T14:27:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 20, 2009 at 05:24:12AM -0800, eletuchy@gmail.com wrote:\n\n> - git config value blame.date that expects one of the git log date\n> formats ({relative,local,default,iso,rfc,short})\n\nOK. I was concerned that this might muck with scripts, but it looks like\nthe --porcelain and --incremental codepaths are properly unaffected.\nGood.\n\n> - git blame command line option --date-format expects one of the git\n> log date formats ({relative,local,default,iso,rfc,short})\n\nWhy not --date= ?\n\nIt is currently accepted by the revision option parsing, but not used;\nyou would just need to pull the value from revs.date_mode instead of\nadding a new option.\n\n> The tests pass. The mailmap test needed to be modified to expect iso\n> formatted blames rather than the new \"default\".\n\nSo there are actually two changes here:\n\n 1. support specifying date format\n\n 2. changing the default date format\n\nI think (1) is a good change, but it should definitely not be lumped in\nwith (2), as people might like one and not the other (and I happen not\nto like (2)).\n\n\nAll of that being said, I think there are two code issues to be dealt\nwith:\n\n 1. There seems to be a bug. With your patch, running a simple test\n like:\n\n git blame --date-format=relative wt-status.c\n\n gives me relative output on some lines, and not on others. E.g.,\n the first 10 lines are:\n\n85023577 (Junio C Hamano Tue Dec 19 14:34:12 2006 -0800 1) #include \"cache.h\"\nc91f0d92 (Jeff King Fri Sep 8 04:05:34 2006 -0400 2) #include \"wt-status.h\"\nc91f0d92 (Jeff King Fri Sep 8 04:05:34 2006 -0400 3) #include \"color.h\"\nc91f0d92 (Jeff King Fri Sep 8 04:05:34 2006 -0400 4) #include \"object.h\"\nc91f0d92 (Jeff King Fri Sep 8 04:05:34 2006 -0400 5) #include \"dir.h\"\nc91f0d92 (Jeff King Fri Sep 8 04:05:34 2006 -0400 6) #include \"commit.h\"\nc91f0d92 (Jeff King Fri Sep 8 04:05:34 2006 -0400 7) #include \"diff.h\"\nc91f0d92 (Jeff King Fri Sep 8 04:05:34 2006 -0400 8) #include \"revision.h\"\nc91f0d92 (Jeff King Fri Sep 8 04:05:34 2006 -0400 9) #include \"diffcore.h\"\na734d0b1 (Dmitry Potapov 12 months ago 10) #include \"quote.h\"\nac8d5afc (Ping Yin 10 months ago 11) #include \"run-command.h\"\nb6975ab5 (Junio C Hamano 8 months ago 12) #include \"remote.h\"\nc91f0d92 (Jeff King Fri Sep 8 04:05:34 2006 -0400 13)\n\n 2. As you can see in the output above, there are potential alignment\n issues. The original date format had a fixed width, whereas\n arbitrary date formats can be variable. Obviously the mixture of\n relative and ISO dates makes it much worse, but even within an ISO\n date there are problems (e.g., \"19\" versus \"8\").\n\n-Peff\n"},{"id":"105620","messageId":"fbb390660902200813h2455eak4e72144c7c491ef9@mail.gmail.com","threadId":"17921","inReplyTo":"20090220142730.GA32751@coredump.intra.peff.net","subject":"Re: [PATCH] Make git blame date output format configurable, a la git log","fromName":"Eugene Letuchy","fromEmail":"eletuchy@gmail.com","sentAt":"2009-02-20T16:13:34Z","receivedAt":"2009-02-20T16:13:34Z","isPatch":true,"sender":{"key":"eletuchy@gmail.com","avatar":null},"body":"Thanks for the feedback. Comments inline.\n\nOn Fri, Feb 20, 2009 at 6:27 AM, Jeff King wrote:\n> On Fri, Feb 20, 2009 at 05:24:12AM -0800, eletuchy@gmail.com wrote:\n>\n>> - git config value blame.date that expects one of the git log date\n>> formats ({relative,local,default,iso,rfc,short})\n>\n> OK. I was concerned that this might muck with scripts, but it looks like\n> the --porcelain and --incremental codepaths are properly unaffected.\n> Good.\n>\n>> - git blame command line option --date-format expects one of the git\n>> log date formats ({relative,local,default,iso,rfc,short})\n>\n> Why not --date= ?\n>\n> It is currently accepted by the revision option parsing, but not used;\n> you would just need to pull the value from revs.date_mode instead of\n> adding a new option.\n>\n\nGood call. I can change to using --date instead of --date-format. It\nwasn't clear that this was an unused option. For parity with\nlog.date, config blame.date still makes sense, right?\n\n>> The tests pass. The mailmap test needed to be modified to expect iso\n>> formatted blames rather than the new \"default\".\n>\n> So there are actually two changes here:\n>\n> 1. support specifying date format\n>\n> 2. changing the default date format\n>\n> I think (1) is a good change, but it should definitely not be lumped in\n> with (2), as people might like one and not the other (and I happen not\n> to like (2)).\n>\n\nWhat about consistency with all git-rev-list clients?\n\n>\n> All of that being said, I think there are two code issues to be dealt\n> with:\n>\n> 1. There seems to be a bug. With your patch, running a simple test\n> like:\n>\n> git blame --date-format=relative wt-status.c\n>\n> gives me relative output on some lines, and not on others. E.g.,\n> the first 10 lines are:\n>\n> 85023577 (Junio C Hamano Tue Dec 19 14:34:12 2006 -0800 1) #include \"cache.h\"\n> c91f0d92 (Jeff King Fri Sep 8 04:05:34 2006 -0400 2) #include \"wt-status.h\"\n> c91f0d92 (Jeff King Fri Sep 8 04:05:34 2006 -0400 3) #include \"color.h\"\n> c91f0d92 (Jeff King Fri Sep 8 04:05:34 2006 -0400 4) #include \"object.h\"\n> c91f0d92 (Jeff King Fri Sep 8 04:05:34 2006 -0400 5) #include \"dir.h\"\n> c91f0d92 (Jeff King Fri Sep 8 04:05:34 2006 -0400 6) #include \"commit.h\"\n> c91f0d92 (Jeff King Fri Sep 8 04:05:34 2006 -0400 7) #include \"diff.h\"\n> c91f0d92 (Jeff King Fri Sep 8 04:05:34 2006 -0400 8) #include \"revision.h\"\n> c91f0d92 (Jeff King Fri Sep 8 04:05:34 2006 -0400 9) #include \"diffcore.h\"\n> a734d0b1 (Dmitry Potapov 12 months ago 10) #include \"quote.h\"\n> ac8d5afc (Ping Yin 10 months ago 11) #include \"run-command.h\"\n> b6975ab5 (Junio C Hamano 8 months ago 12) #include \"remote.h\"\n> c91f0d92 (Jeff King Fri Sep 8 04:05:34 2006 -0400 13)\n>\n\nAccording to date.c comments, this is a \"feature\" of DATE_RELATIVE:\n /* Say months for the past 12 months or so */\n if (diff < 360) {\n snprintf(timebuf, sizeof(timebuf), \"%lu months\nago\", (diff + 15) / 30);\n return timebuf;\n }\n /* Else fall back on absolute format.. */\n\nA single line fixes that to be a bit more logical:\n- /* Else fall back on absolute format.. */\n+ /* Else fall back to the short format */\n+ mode = DATE_SHORT;\n\nbut i think that's a separate commit, no?\n\n> 2. As you can see in the output above, there are potential alignment\n> issues. The original date format had a fixed width, whereas\n> arbitrary date formats can be variable. Obviously the mixture of\n> relative and ISO dates makes it much worse, but even within an ISO\n> date there are problems (e.g., \"19\" versus \"8\").\n>\n\nI have a patch to fix the alignment issues: it figures out the max\nwidth of each date format and memsets in that number of spaces in\nformat_time. Is it better to submit that as a separate commit, or send\na revised patch?\n\nThe output is as follows:\n\n> ./git blame --date=relative wt-status.c | head -10\n85023577 (Junio C Hamano 2006-12-19 1) #include \"cache.h\"\nc91f0d92 (Jeff King 2006-09-08 2) #include \"wt-status.h\"\nc91f0d92 (Jeff King 2006-09-08 3) #include \"color.h\"\nc91f0d92 (Jeff King 2006-09-08 4) #include \"object.h\"\nc91f0d92 (Jeff King 2006-09-08 5) #include \"dir.h\"\nc91f0d92 (Jeff King 2006-09-08 6) #include \"commit.h\"\nc91f0d92 (Jeff King 2006-09-08 7) #include \"diff.h\"\nc91f0d92 (Jeff King 2006-09-08 8) #include \"revision.h\"\nc91f0d92 (Jeff King 2006-09-08 9) #include \"diffcore.h\"\na734d0b1 (Dmitry Potapov 12 months ago 10) #include \"quote.h\"\n\n> -Peff\n>\n\n\n\n-- \nEugene\n"},{"id":"105628","messageId":"7vwsblrqm7.fsf@gitster.siamese.dyndns.org","threadId":"17921","inReplyTo":"1235136252-29649-1-git-send-email-eletuchy@gmail.com","subject":"Re: [PATCH] Make git blame date output format configurable, a la git log","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-20T16:59:44Z","receivedAt":"2009-02-20T16:59:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"eletuchy@gmail.com writes:\n\n> From: Eugene Letuchy \n>\n> Adds the following:\n> - git config value blame.date that expects one of the git log date\n> formats ({relative,local,default,iso,rfc,short})\n> - git blame command line option --date-format expects one of the git\n> log date formats ({relative,local,default,iso,rfc,short})\n> - documentation in blame-options.txt\n> - git blame uses the appropriate date.c functions and enums to\n> make sense of the date format and provide appropriate data\n>\n> The tests pass. The mailmap test needed to be modified to expect iso\n> formatted blames rather than the new \"default\".\n>\n> Signed-off-by: Eugene Letuchy \n\nDoesn't your need to modify existing tests mean you broke the default\noutput? IOW, shouldn't the defeault output format stay the same?\n\nIf you are proposing to change the default to use a different format, then\nthe commit log must explain why such a change is a good thing, and the\nbenefit of changing outweighs the downside of backward imcompatibility.\n\nI am not opposed to hearing such an argument but I think it should be a\nseparate patch that comes after this patch, that flips the default and\ndoes nothing else.\n\n> ---\n> Documentation/blame-options.txt | 6 ++++++\n> builtin-blame.c | 31 ++++++++++++++++++-------------\n> t/t4203-mailmap.sh | 2 +-\n> 3 files changed, 25 insertions(+), 14 deletions(-)\n>\n> diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\n> index 1ab1b96..75663ec 100644\n> --- a/Documentation/blame-options.txt\n> +++ b/Documentation/blame-options.txt\n> @@ -63,6 +63,12 @@ of lines before or after the line given by .\n> \ttree copy has the contents of the named file (specify\n> \t`-` to make the command read from the standard input).\n> \n> +--date-format ::\n> +\tThe value is one of the following alternatives:\n> +\t{relative,local,default,iso,rfc,short}. The default format\n> +\tcan be set using the blame.date config variable. See the\n> +\tdiscussion of the --date option at linkgit:git-log[1].\n\nPlease specify what format is used when this option is not used and there\nis no blame.date configuration variable.\n\nYou need an entry in Documentation/config.txt as well.\n\n> diff --git a/builtin-blame.c b/builtin-blame.c\n> index 114a214..9ebab43 100644\n> --- a/builtin-blame.c\n> +++ b/builtin-blame.c\n> @@ -1,5 +1,5 @@\n> /*\n> - * Pickaxe\n> + * Blame / Pickaxe\n\nIt's time we drop \"/ Pickaxe\" ;-) It was a codename while the algorithm\nwas being polished to replace two old \"blame\" implementations we had.\n\n> @@ -40,6 +40,9 @@ static int reverse;\n> static int blank_boundary;\n> static int incremental;\n> static int xdl_opts = XDF_NEED_MINIMAL;\n> +\n> +static enum date_mode date_mode;\n\nEven though this is file-scope static today, I'd prefer to call it\n\"blame_date_mode\".\n\n> @@ -1507,9 +1510,7 @@ static const char *format_time(unsigned long time, const char *tz_str,\n> ...\n> +\treturn show_date(time, tz, date_mode);\n\nNice code reduction.\n\n> @@ -1967,6 +1960,8 @@ static void prepare_blame_range(struct scoreboard *sb,\n> \n> static int git_blame_config(const char *var, const char *value, void *cb)\n> {\n> +\tconst char *default_date_mode;\n\nThat is misnamed, placed in a wrong scope, and is unneeded.\n\n> ...\n> +\tif (!strcmp(var, \"blame.date\")) {\n> +\t\tgit_config_string(&default_date_mode, var, value);\n> +\t\tdate_mode = parse_date_format(default_date_mode);\n> +\t\treturn 0;\n> +\t}\n> \treturn git_default_config(var, value, cb);\n> }\n\n * It is not \"default\" in the sense that \"this is the format used when no\n option nor configuration is given\". It is \"configured_date_mode\" if \n you want to be explicit, but...\n\n * You could scope it in the relevant \"if (!strcmp()) {...}\", and then\n just call it \"str\" or something less specific, but ...\n\n * You do not use the value as the string in the rest of the code, so you\n do not need this variable. Stop using git_config_string(), and\n inside \"if (!strcmp()) {...}\":\n \n - detect \"[blame] date\" with missing value (means \"boolean true\") as an\n error;\n\n - otherwise feed value directly to parse_date_format().\n\n That way you save one xstrdup() and one less memleak.\n\n> diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh\n> index 9a7d1b4..13b64dc 100755\n> --- a/t/t4203-mailmap.sh\n> +++ b/t/t4203-mailmap.sh\n> @@ -208,7 +208,7 @@ ff859d96 (Other Author 2005-04-07 15:15:13 -0700 4) four\n> EOF\n> \n> test_expect_success 'Blame output (complex mapping)' '\n> -\tgit blame one >actual &&\n> +\tgit blame --date-format=iso one >actual &&\n> \ttest_cmp expect actual\n> '\n\nThis strongly suggests that the file-scope variable should be initialized\nto keep backward compatibility:\n\n> +static enum date_mode date_mode = DATE_ISO8601;\n\nOther than that the idea sounds good.\n\nI didn't check if the code tries to align columns properly (the default\nISO format is of uniform length so existing code wouldn't have needed to\ntake variable length into account); if not, it should.\n\nI didn't check if \"git annotate compatibility mode\" ignores this new\nsetting either; if not, it should.\n\nThanks.\n"},{"id":"105633","messageId":"20090220171825.GA4636@coredump.intra.peff.net","threadId":"17921","inReplyTo":"fbb390660902200813h2455eak4e72144c7c491ef9@mail.gmail.com","subject":"Re: [PATCH] Make git blame date output format configurable, a la git log","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-20T17:18:25Z","receivedAt":"2009-02-20T17:18:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 20, 2009 at 08:13:34AM -0800, Eugene Letuchy wrote:\n\n> Good call. I can change to using --date instead of --date-format. It\n> wasn't clear that this was an unused option.\n\nYeah, it is a slight confusion both to developers and to users that\nprograms which take revision arguments sometimes accept but ignore them.\n\nBut the revs.date_mode set by the revision library is basically just\nused by log-tree, which is not used by blame. So it is safe to reuse,\nand doing so actually reduces confusion.\n\n> For parity with log.date, config blame.date still makes sense, right?\n\nSure. It might even make sense to have an unset blame.date default to\nthe value of log.date. But I don't use log.date, nor do I directly use\nblame (I use tig's blame mode). So I don't know what people expect or\nwould find useful.\n\n> > So there are actually two changes here:\n> >\n> > 1. support specifying date format\n> >\n> > 2. changing the default date format\n> >\n> > I think (1) is a good change, but it should definitely not be lumped in\n> > with (2), as people might like one and not the other (and I happen not\n> > to like (2)).\n> >\n> What about consistency with all git-rev-list clients?\n\nI think blame is a bit different than other clients because it is\nshowing the date on a line with a bunch of other stuff, whereas most\nclients use \"Date: \" on a separate line. So it has to be a bit\nmore careful about how much space is used.\n\nThat being said, I think this discussion proves my main point, which is\nthat it should be split into two patches. Then discussion over the\ndefault format will not hold up the --date support.\n\n> > gives me relative output on some lines, and not on others. E.g.,\n> [...]\n> According to date.c comments, this is a \"feature\" of DATE_RELATIVE:\n\nOh, right. Sorry for the noise, I totally forgot about that that feature\n(which I now remember annoying me in the past, too).\n\n> /* Say months for the past 12 months or so */\n> if (diff < 360) {\n> snprintf(timebuf, sizeof(timebuf), \"%lu months\n> ago\", (diff + 15) / 30);\n> return timebuf;\n> }\n> /* Else fall back on absolute format.. */\n> \n> A single line fixes that to be a bit more logical:\n> - /* Else fall back on absolute format.. */\n> + /* Else fall back to the short format */\n> + mode = DATE_SHORT;\n> \n> but i think that's a separate commit, no?\n\nI do think that's a reasonable change; there's no point in giving a very\nprecise date for things more than a year past when we have already\ndropped precision to \"month\" for everything else. But definitely a\nseparate commit.\n\nPersonally, I think I would rather see \"months\" up until about 2-3\nyears, and then simply \"N years ago\" after that.\n\n> I have a patch to fix the alignment issues: it figures out the max\n> width of each date format and memsets in that number of spaces in\n> format_time. Is it better to submit that as a separate commit, or send\n> a revised patch?\n\nI think it makes sense to send a revised patch with all of the changes\nwe've discussed (please mark it as v2 and give a brief summary of what's\nchanged below the \"---\" marker to help out other reviewers).\n\n-Peff\n"}]}