[PATCH] Make git blame date output format configurable, a la git log

[PATCH] Make git blame date output format configurable, a la git log

[eletuchy]

From: Eugene Letuchy <eugene@facebook.com>

Adds the following:
 - git config value blame.date that expects one of the git log date
   formats ({relative,local,default,iso,rfc,short})
 - git blame command line option --date-format expects one of the git
   log date formats ({relative,local,default,iso,rfc,short})
 - documentation in blame-options.txt
 - git blame uses the appropriate date.c functions and enums to
   make sense of the date format and provide appropriate data

The tests pass. The mailmap test needed to be modified to expect iso
formatted blames rather than the new "default".

Signed-off-by: Eugene Letuchy <eugene@facebook.com>
---
 Documentation/blame-options.txt |    6 ++++++
 builtin-blame.c                 |   31 ++++++++++++++++++-------------
 t/t4203-mailmap.sh              |    2 +-
 3 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index 1ab1b96..75663ec 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -63,6 +63,12 @@ of lines before or after the line given by <start>.
 	tree copy has the contents of the named file (specify
 	`-` to make the command read from the standard input).
 
+--date-format <format>::
+	The value is one of the following alternatives:
+	{relative,local,default,iso,rfc,short}.  The default format
+	can be set using the blame.date config variable. See the
+	discussion of the --date option at linkgit:git-log[1].
+
 -M|<num>|::
 	Detect moving lines in the file as well.  When a commit
 	moves a block of lines in a file (e.g. the original file
diff --git a/builtin-blame.c b/builtin-blame.c
index 114a214..9ebab43 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -1,5 +1,5 @@
 /*
- * Pickaxe
+ * Blame / Pickaxe
  *
  * Copyright (c) 2006, Junio C Hamano
  */
@@ -40,6 +40,9 @@ static int reverse;
 static int blank_boundary;
 static int incremental;
 static int xdl_opts = XDF_NEED_MINIMAL;
+
+static enum date_mode date_mode;
+
 static struct string_list mailmap;
 
 #ifndef DEBUG
@@ -1507,9 +1510,7 @@ static const char *format_time(unsigned long time, const char *tz_str,
 			       int show_raw_time)
 {
 	static char time_buf[128];
-	time_t t = time;
-	int minutes, tz;
-	struct tm *tm;
+	int tz;
 
 	if (show_raw_time) {
 		sprintf(time_buf, "%lu %s", time, tz_str);
@@ -1517,15 +1518,7 @@ static const char *format_time(unsigned long time, const char *tz_str,
 	}
 
 	tz = atoi(tz_str);
-	minutes = tz < 0 ? -tz : tz;
-	minutes = (minutes / 100)*60 + (minutes % 100);
-	minutes = tz < 0 ? -minutes : minutes;
-	t = time + minutes * 60;
-	tm = gmtime(&t);
-
-	strftime(time_buf, sizeof(time_buf), "%Y-%m-%d %H:%M:%S ", tm);
-	strcat(time_buf, tz_str);
-	return time_buf;
+	return show_date(time, tz, date_mode);
 }
 
 #define OUTPUT_ANNOTATE_COMPAT	001
@@ -1967,6 +1960,8 @@ static void prepare_blame_range(struct scoreboard *sb,
 
 static int git_blame_config(const char *var, const char *value, void *cb)
 {
+	const char *default_date_mode;
+
 	if (!strcmp(var, "blame.showroot")) {
 		show_root = git_config_bool(var, value);
 		return 0;
@@ -1975,6 +1970,11 @@ static int git_blame_config(const char *var, const char *value, void *cb)
 		blank_boundary = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "blame.date")) {
+		git_config_string(&default_date_mode, var, value);
+		date_mode = parse_date_format(default_date_mode);
+		return 0;
+	}
 	return git_default_config(var, value, cb);
 }
 
@@ -2212,6 +2212,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	static int show_stats = 0;
 	static const char *revs_file = NULL;
 	static const char *contents_from = NULL;
+	static const char *date_format = NULL;
 	static const struct option options[] = {
 		OPT_BOOLEAN(0, "incremental", &incremental, "Show blame entries as we find them, incrementally"),
 		OPT_BOOLEAN('b', NULL, &blank_boundary, "Show blank SHA-1 for boundary commits (Default: off)"),
@@ -2228,6 +2229,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		OPT_BIT('w', NULL, &xdl_opts, "Ignore whitespace differences", XDF_IGNORE_WHITESPACE),
 		OPT_STRING('S', NULL, &revs_file, "file", "Use revisions from <file> instead of calling git-rev-list"),
 		OPT_STRING(0, "contents", &contents_from, "file", "Use <file>'s contents as the final image"),
+		OPT_STRING(0, "date-format", &date_format, "date mode", "Specify date formatting: relative,local,default,iso,rfc,short. ."),
 		{ OPTION_CALLBACK, 'C', NULL, &opt, "score", "Find line copies within and across files", PARSE_OPT_OPTARG, blame_copy_callback },
 		{ OPTION_CALLBACK, 'M', NULL, &opt, "score", "Find line movements within and across files", PARSE_OPT_OPTARG, blame_move_callback },
 		OPT_CALLBACK('L', NULL, &bottomtop, "n,m", "Process only line range n,m, counting from 1", blame_bottomtop_callback),
@@ -2266,6 +2268,9 @@ parse_done:
 	if (cmd_is_annotate)
 		output_option |= OUTPUT_ANNOTATE_COMPAT;
 
+	if (date_format)
+		date_mode = parse_date_format(date_format);
+
 	if (DIFF_OPT_TST(&revs.diffopt, FIND_COPIES_HARDER))
 		opt |= (PICKAXE_BLAME_COPY | PICKAXE_BLAME_MOVE |
 			PICKAXE_BLAME_COPY_HARDER);
diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 9a7d1b4..13b64dc 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -208,7 +208,7 @@ ff859d96 (Other Author 2005-04-07 15:15:13 -0700 4) four
 EOF
 
 test_expect_success 'Blame output (complex mapping)' '
-	git blame one >actual &&
+	git blame --date-format=iso one >actual &&
 	test_cmp expect actual
 '
 
-- 
1.6.2.rc1.14.g397c24.dirty

Re: [PATCH] Make git blame date output format configurable, a la git log

[Johannes Schindelin]

Hi,

Disclaimer: if you are offended by constructive criticism, or likely to 
answer with insults to the comments I offer, please stop reading this mail 
now (and please to not answer my mail, either). :-)

Still with me?  Good.  Nice to meet you.

Just out of curiosity: why Cc: Marius?  I would have expected Junio, Git's 
maintainer.

May I suggest the commit subject to say "as for git log"?  I mistook "a la 
git log" for a change in the way git-blame works...

On Fri, 20 Feb 2009, eletuchy@gmail.com wrote:

> From: Eugene Letuchy <eugene@facebook.com>
> 
> Adds the following:

We try to use the imperative form; from my experience it makes for an 
easier read: "Add the following:"

>  - git config value blame.date that expects one of the git log date
>    formats ({relative,local,default,iso,rfc,short})
>  - git blame command line option --date-format expects one of the git
>    log date formats ({relative,local,default,iso,rfc,short})
>  - documentation in blame-options.txt
>  - git blame uses the appropriate date.c functions and enums to
>    make sense of the date format and provide appropriate data
> 
> The tests pass. The mailmap test needed to be modified to expect iso
> formatted blames rather than the new "default".

IMHO the "The tests pass." should be removed.

Other than that, nicely done!

Ciao,
Dscho

Re: [PATCH] Make git blame date output format configurable, a la git log

[Eugene Letuchy]

Hi Johannes,

Thanks for your feedback. Any comments on the .c changes?

I'll modify the commit message to read as follows:
"""

Add the following:
  - git config value blame.date that expects one of the git log date
    formats ({relative,local,default,iso,rfc,short})
  - git blame command line option --date-format expects one of the git
    log date formats ({relative,local,default,iso,rfc,short})
  - documentation in blame-options.txt
  - git blame uses the appropriate date.c functions and enums to
    make sense of the date format and provide appropriate data

The tests pass. The mailmap test needed to be modified to expect iso
formatted blames rather than the new "default".

Signed-off-by: Eugene Letuchy <eugene@facebook.com>
"""

-Eugene

+ cc: junio

On 2/20/09 5:40 AM, Johannes Schindelin wrote:
> Hi,
>
> Disclaimer: if you are offended by constructive criticism, or likely to
> answer with insults to the comments I offer, please stop reading this mail
> now (and please to not answer my mail, either). :-)
>
> Still with me?  Good.  Nice to meet you.
>
> Just out of curiosity: why Cc: Marius?  I would have expected Junio, Git's
> maintainer.
>
> May I suggest the commit subject to say "as for git log"?  I mistook "a la
> git log" for a change in the way git-blame works...
>
> On Fri, 20 Feb 2009, eletuchy@gmail.com wrote:
>
>> From: Eugene Letuchy<eugene@facebook.com>
>>
>> Adds the following:
>
> We try to use the imperative form; from my experience it makes for an
> easier read: "Add the following:"
>
>>   - git config value blame.date that expects one of the git log date
>>     formats ({relative,local,default,iso,rfc,short})
>>   - git blame command line option --date-format expects one of the git
>>     log date formats ({relative,local,default,iso,rfc,short})
>>   - documentation in blame-options.txt
>>   - git blame uses the appropriate date.c functions and enums to
>>     make sense of the date format and provide appropriate data
>>
>> The tests pass. The mailmap test needed to be modified to expect iso
>> formatted blames rather than the new "default".
>
> IMHO the "The tests pass." should be removed.
>
> Other than that, nicely done!
>
> Ciao,
> Dscho
>

Re: [PATCH] Make git blame date output format configurable, a la git log

[Eugene Letuchy]

Sigh. Make that:
"""
The mailmap test needed to be modified to expect iso formatted blames
rather than the new "default".
"""

- Eugene

On 2/20/09 5:55 AM, Eugene Letuchy wrote:
> Hi Johannes,
>
> Thanks for your feedback. Any comments on the .c changes?
>
> I'll modify the commit message to read as follows:
> """
>
> Add the following:
>    - git config value blame.date that expects one of the git log date
>      formats ({relative,local,default,iso,rfc,short})
>    - git blame command line option --date-format expects one of the git
>      log date formats ({relative,local,default,iso,rfc,short})
>    - documentation in blame-options.txt
>    - git blame uses the appropriate date.c functions and enums to
>      make sense of the date format and provide appropriate data
>
> The tests pass. The mailmap test needed to be modified to expect iso
> formatted blames rather than the new "default".
>
> Signed-off-by: Eugene Letuchy<eugene@facebook.com>
> """
>
> -Eugene
>
> + cc: junio
>
> On 2/20/09 5:40 AM, Johannes Schindelin wrote:
>> Hi,
>>
>> Disclaimer: if you are offended by constructive criticism, or likely to
>> answer with insults to the comments I offer, please stop reading this mail
>> now (and please to not answer my mail, either). :-)
>>
>> Still with me?  Good.  Nice to meet you.
>>
>> Just out of curiosity: why Cc: Marius?  I would have expected Junio, Git's
>> maintainer.
>>
>> May I suggest the commit subject to say "as for git log"?  I mistook "a la
>> git log" for a change in the way git-blame works...
>>
>> On Fri, 20 Feb 2009, eletuchy@gmail.com wrote:
>>
>>> From: Eugene Letuchy<eugene@facebook.com>
>>>
>>> Adds the following:
>> We try to use the imperative form; from my experience it makes for an
>> easier read: "Add the following:"
>>
>>>    - git config value blame.date that expects one of the git log date
>>>      formats ({relative,local,default,iso,rfc,short})
>>>    - git blame command line option --date-format expects one of the git
>>>      log date formats ({relative,local,default,iso,rfc,short})
>>>    - documentation in blame-options.txt
>>>    - git blame uses the appropriate date.c functions and enums to
>>>      make sense of the date format and provide appropriate data
>>>
>>> The tests pass. The mailmap test needed to be modified to expect iso
>>> formatted blames rather than the new "default".
>> IMHO the "The tests pass." should be removed.
>>
>> Other than that, nicely done!
>>
>> Ciao,
>> Dscho
>>

Re: [PATCH] Make git blame date output format configurable, a la git log

[Johannes Schindelin]

Hi Eugene,

On Fri, 20 Feb 2009, Eugene Letuchy wrote:

> Thanks for your feedback. Any comments on the .c changes?

Yes: they look fine to me :-)

(Please excuse if I only point out things I'd like you to change, and not 
praise the rest as verbosely; the fact that I take the time to comment on 
the patch is meant to show you that I am interested in your work; 
often I am terse because I have to squeeze commenting on patches 
in-between my day job.)

Ciao,
Dscho

Re: [PATCH] Make git blame date output format configurable, a la git log

[Jeff King]

On Fri, Feb 20, 2009 at 05:24:12AM -0800, eletuchy@gmail.com wrote:

>  - git config value blame.date that expects one of the git log date
>    formats ({relative,local,default,iso,rfc,short})

OK. I was concerned that this might muck with scripts, but it looks like
the --porcelain and --incremental codepaths are properly unaffected.
Good.

>  - git blame command line option --date-format expects one of the git
>    log date formats ({relative,local,default,iso,rfc,short})

Why not --date= ?

It is currently accepted by the revision option parsing, but not used;
you would just need to pull the value from revs.date_mode instead of
adding a new option.

> The tests pass. The mailmap test needed to be modified to expect iso
> formatted blames rather than the new "default".

So there are actually two changes here:

  1. support specifying date format

  2. changing the default date format

I think (1) is a good change, but it should definitely not be lumped in
with (2), as people might like one and not the other (and I happen not
to like (2)).


All of that being said, I think there are two code issues to be dealt
with:

  1. There seems to be a bug. With your patch, running a simple test
     like:

       git blame --date-format=relative wt-status.c

     gives me relative output on some lines, and not on others. E.g.,
     the first 10 lines are:

85023577 (Junio C Hamano      Tue Dec 19 14:34:12 2006 -0800   1) #include "cache.h"
c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400   2) #include "wt-status.h"
c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400   3) #include "color.h"
c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400   4) #include "object.h"
c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400   5) #include "dir.h"
c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400   6) #include "commit.h"
c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400   7) #include "diff.h"
c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400   8) #include "revision.h"
c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400   9) #include "diffcore.h"
a734d0b1 (Dmitry Potapov      12 months ago  10) #include "quote.h"
ac8d5afc (Ping Yin            10 months ago  11) #include "run-command.h"
b6975ab5 (Junio C Hamano      8 months ago  12) #include "remote.h"
c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400  13)

  2. As you can see in the output above, there are potential alignment
     issues. The original date format had a fixed width, whereas
     arbitrary date formats can be variable. Obviously the mixture of
     relative and ISO dates makes it much worse, but even within an ISO
     date there are problems (e.g., "19" versus "8").

-Peff

Re: [PATCH] Make git blame date output format configurable, a la git log

[Eugene Letuchy]

Thanks for the feedback. Comments inline.

On Fri, Feb 20, 2009 at 6:27 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Feb 20, 2009 at 05:24:12AM -0800, eletuchy@gmail.com wrote:
>
>>  - git config value blame.date that expects one of the git log date
>>    formats ({relative,local,default,iso,rfc,short})
>
> OK. I was concerned that this might muck with scripts, but it looks like
> the --porcelain and --incremental codepaths are properly unaffected.
> Good.
>
>>  - git blame command line option --date-format expects one of the git
>>    log date formats ({relative,local,default,iso,rfc,short})
>
> Why not --date= ?
>
> It is currently accepted by the revision option parsing, but not used;
> you would just need to pull the value from revs.date_mode instead of
> adding a new option.
>

Good call. I can change to using --date instead of --date-format. It
wasn't clear that this was an unused option.  For parity with
log.date, config blame.date still makes sense, right?

>> The tests pass. The mailmap test needed to be modified to expect iso
>> formatted blames rather than the new "default".
>
> So there are actually two changes here:
>
>  1. support specifying date format
>
>  2. changing the default date format
>
> I think (1) is a good change, but it should definitely not be lumped in
> with (2), as people might like one and not the other (and I happen not
> to like (2)).
>

What about consistency with all git-rev-list clients?

>
> All of that being said, I think there are two code issues to be dealt
> with:
>
>  1. There seems to be a bug. With your patch, running a simple test
>     like:
>
>       git blame --date-format=relative wt-status.c
>
>     gives me relative output on some lines, and not on others. E.g.,
>     the first 10 lines are:
>
> 85023577 (Junio C Hamano      Tue Dec 19 14:34:12 2006 -0800   1) #include "cache.h"
> c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400   2) #include "wt-status.h"
> c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400   3) #include "color.h"
> c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400   4) #include "object.h"
> c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400   5) #include "dir.h"
> c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400   6) #include "commit.h"
> c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400   7) #include "diff.h"
> c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400   8) #include "revision.h"
> c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400   9) #include "diffcore.h"
> a734d0b1 (Dmitry Potapov      12 months ago  10) #include "quote.h"
> ac8d5afc (Ping Yin            10 months ago  11) #include "run-command.h"
> b6975ab5 (Junio C Hamano      8 months ago  12) #include "remote.h"
> c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400  13)
>

According to date.c comments, this is a "feature" of DATE_RELATIVE:
                /* Say months for the past 12 months or so */
                if (diff < 360) {
                        snprintf(timebuf, sizeof(timebuf), "%lu months
ago", (diff + 15) / 30);
                        return timebuf;
                }
                /* Else fall back on absolute format.. */

A single line fixes that to be a bit more logical:
-               /* Else fall back on absolute format.. */
+               /* Else fall back to the short format */
+                mode = DATE_SHORT;

but i think that's a separate commit, no?

>  2. As you can see in the output above, there are potential alignment
>     issues. The original date format had a fixed width, whereas
>     arbitrary date formats can be variable. Obviously the mixture of
>     relative and ISO dates makes it much worse, but even within an ISO
>     date there are problems (e.g., "19" versus "8").
>

I have a patch to fix the alignment issues: it figures out the max
width of each date format and memsets in that number of spaces in
format_time. Is it better to submit that as a separate commit, or send
a revised patch?

The output is as follows:

> ./git blame --date=relative wt-status.c | head -10
85023577 (Junio C Hamano      2006-12-19       1) #include "cache.h"
c91f0d92 (Jeff King           2006-09-08       2) #include "wt-status.h"
c91f0d92 (Jeff King           2006-09-08       3) #include "color.h"
c91f0d92 (Jeff King           2006-09-08       4) #include "object.h"
c91f0d92 (Jeff King           2006-09-08       5) #include "dir.h"
c91f0d92 (Jeff King           2006-09-08       6) #include "commit.h"
c91f0d92 (Jeff King           2006-09-08       7) #include "diff.h"
c91f0d92 (Jeff King           2006-09-08       8) #include "revision.h"
c91f0d92 (Jeff King           2006-09-08       9) #include "diffcore.h"
a734d0b1 (Dmitry Potapov      12 months ago   10) #include "quote.h"

> -Peff
>



-- 
Eugene

Re: [PATCH] Make git blame date output format configurable, a la git log

[Junio C Hamano]

eletuchy@gmail.com writes:

> From: Eugene Letuchy <eugene@facebook.com>
>
> Adds the following:
>  - git config value blame.date that expects one of the git log date
>    formats ({relative,local,default,iso,rfc,short})
>  - git blame command line option --date-format expects one of the git
>    log date formats ({relative,local,default,iso,rfc,short})
>  - documentation in blame-options.txt
>  - git blame uses the appropriate date.c functions and enums to
>    make sense of the date format and provide appropriate data
>
> The tests pass. The mailmap test needed to be modified to expect iso
> formatted blames rather than the new "default".
>
> Signed-off-by: Eugene Letuchy <eugene@facebook.com>

Doesn't your need to modify existing tests mean you broke the default
output?  IOW, shouldn't the defeault output format stay the same?

If you are proposing to change the default to use a different format, then
the commit log must explain why such a change is a good thing, and the
benefit of changing outweighs the downside of backward imcompatibility.

I am not opposed to hearing such an argument but I think it should be a
separate patch that comes after this patch, that flips the default and
does nothing else.

> ---
>  Documentation/blame-options.txt |    6 ++++++
>  builtin-blame.c                 |   31 ++++++++++++++++++-------------
>  t/t4203-mailmap.sh              |    2 +-
>  3 files changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
> index 1ab1b96..75663ec 100644
> --- a/Documentation/blame-options.txt
> +++ b/Documentation/blame-options.txt
> @@ -63,6 +63,12 @@ of lines before or after the line given by <start>.
>  	tree copy has the contents of the named file (specify
>  	`-` to make the command read from the standard input).
>  
> +--date-format <format>::
> +	The value is one of the following alternatives:
> +	{relative,local,default,iso,rfc,short}.  The default format
> +	can be set using the blame.date config variable. See the
> +	discussion of the --date option at linkgit:git-log[1].

Please specify what format is used when this option is not used and there
is no blame.date configuration variable.

You need an entry in Documentation/config.txt as well.

> diff --git a/builtin-blame.c b/builtin-blame.c
> index 114a214..9ebab43 100644
> --- a/builtin-blame.c
> +++ b/builtin-blame.c
> @@ -1,5 +1,5 @@
>  /*
> - * Pickaxe
> + * Blame / Pickaxe

It's time we drop "/ Pickaxe" ;-)  It was a codename while the algorithm
was being polished to replace two old "blame" implementations we had.

> @@ -40,6 +40,9 @@ static int reverse;
>  static int blank_boundary;
>  static int incremental;
>  static int xdl_opts = XDF_NEED_MINIMAL;
> +
> +static enum date_mode date_mode;

Even though this is file-scope static today, I'd prefer to call it
"blame_date_mode".

> @@ -1507,9 +1510,7 @@ static const char *format_time(unsigned long time, const char *tz_str,
> ...
> +	return show_date(time, tz, date_mode);

Nice code reduction.

> @@ -1967,6 +1960,8 @@ static void prepare_blame_range(struct scoreboard *sb,
>  
>  static int git_blame_config(const char *var, const char *value, void *cb)
>  {
> +	const char *default_date_mode;

That is misnamed, placed in a wrong scope, and is unneeded.

> ...
> +	if (!strcmp(var, "blame.date")) {
> +		git_config_string(&default_date_mode, var, value);
> +		date_mode = parse_date_format(default_date_mode);
> +		return 0;
> +	}
>  	return git_default_config(var, value, cb);
>  }

 * It is not "default" in the sense that "this is the format used when no
   option nor configuration is given".  It is "configured_date_mode" if 
   you want to be explicit, but...

 * You could scope it in the relevant "if (!strcmp()) {...}", and then
   just call it "str" or something less specific, but ...

 * You do not use the value as the string in the rest of the code, so you
   do not need this variable.  Stop using git_config_string(), and
   inside "if (!strcmp()) {...}":
   
   - detect "[blame] date" with missing value (means "boolean true") as an
     error;

   - otherwise feed value directly to parse_date_format().

   That way you save one xstrdup() and one less memleak.

> diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
> index 9a7d1b4..13b64dc 100755
> --- a/t/t4203-mailmap.sh
> +++ b/t/t4203-mailmap.sh
> @@ -208,7 +208,7 @@ ff859d96 (Other Author 2005-04-07 15:15:13 -0700 4) four
>  EOF
>  
>  test_expect_success 'Blame output (complex mapping)' '
> -	git blame one >actual &&
> +	git blame --date-format=iso one >actual &&
>  	test_cmp expect actual
>  '

This strongly suggests that the file-scope variable should be initialized
to keep backward compatibility:

> +static enum date_mode date_mode = DATE_ISO8601;

Other than that the idea sounds good.

I didn't check if the code tries to align columns properly (the default
ISO format is of uniform length so existing code wouldn't have needed to
take variable length into account); if not, it should.

I didn't check if "git annotate compatibility mode" ignores this new
setting either; if not, it should.

Thanks.

Re: [PATCH] Make git blame date output format configurable, a la git log

[Jeff King]

On Fri, Feb 20, 2009 at 08:13:34AM -0800, Eugene Letuchy wrote:

> Good call. I can change to using --date instead of --date-format. It
> wasn't clear that this was an unused option.

Yeah, it is a slight confusion both to developers and to users that
programs which take revision arguments sometimes accept but ignore them.

But the revs.date_mode set by the revision library is basically just
used by log-tree, which is not used by blame. So it is safe to reuse,
and doing so actually reduces confusion.

> For parity with log.date, config blame.date still makes sense, right?

Sure. It might even make sense to have an unset blame.date default to
the value of log.date. But I don't use log.date, nor do I directly use
blame (I use tig's blame mode). So I don't know what people expect or
would find useful.

> > So there are actually two changes here:
> >
> >  1. support specifying date format
> >
> >  2. changing the default date format
> >
> > I think (1) is a good change, but it should definitely not be lumped in
> > with (2), as people might like one and not the other (and I happen not
> > to like (2)).
> >
> What about consistency with all git-rev-list clients?

I think blame is a bit different than other clients because it is
showing the date on a line with a bunch of other stuff, whereas most
clients use "Date: <whatever>" on a separate line. So it has to be a bit
more careful about how much space is used.

That being said, I think this discussion proves my main point, which is
that it should be split into two patches. Then discussion over the
default format will not hold up the --date support.

> >     gives me relative output on some lines, and not on others. E.g.,
> [...]
> According to date.c comments, this is a "feature" of DATE_RELATIVE:

Oh, right. Sorry for the noise, I totally forgot about that that feature
(which I now remember annoying me in the past, too).

>                 /* Say months for the past 12 months or so */
>                 if (diff < 360) {
>                         snprintf(timebuf, sizeof(timebuf), "%lu months
> ago", (diff + 15) / 30);
>                         return timebuf;
>                 }
>                 /* Else fall back on absolute format.. */
> 
> A single line fixes that to be a bit more logical:
> -               /* Else fall back on absolute format.. */
> +               /* Else fall back to the short format */
> +                mode = DATE_SHORT;
> 
> but i think that's a separate commit, no?

I do think that's a reasonable change; there's no point in giving a very
precise date for things more than a year past when we have already
dropped precision to "month" for everything else. But definitely a
separate commit.

Personally, I think I would rather see "months" up until about 2-3
years, and then simply "N years ago" after that.

> I have a patch to fix the alignment issues: it figures out the max
> width of each date format and memsets in that number of spaces in
> format_time. Is it better to submit that as a separate commit, or send
> a revised patch?

I think it makes sense to send a revised patch with all of the changes
we've discussed (please mark it as v2 and give a brief summary of what's
changed below the "---" marker to help out other reviewers).

-Peff