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

Subsystems: documentation, the rest

DORMANTno replies

8 messages, 4 authors, 2016-06-15 · open the first message on its own page

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

From: <hidden>
Date: 2016-06-15 22:46:15

From: Eugene Letuchy <redacted>

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 <redacted>
---
 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

From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:46:15

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 <redacted>

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

From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:46:15

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

From: Jeff King <hidden>
Date: 2016-06-15 22:46:15

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

From: Eugene Letuchy <hidden>
Date: 2016-06-15 22:46:15

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 <redacted>
"""

-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:
quoted
From: Eugene Letuchy<redacted>

Adds the following:
We try to use the imperative form; from my experience it makes for an
easier read: "Add the following:"
quoted
  - 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

From: Eugene Letuchy <hidden>
Date: 2016-06-15 22:46:15

Thanks for the feedback. Comments inline.

On Fri, Feb 20, 2009 at 6:27 AM, Jeff King [off-list ref] wrote:
On Fri, Feb 20, 2009 at 05:24:12AM -0800, eletuchy@gmail.com wrote:
quoted
 - 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.
quoted
 - 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?
quoted
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

From: Eugene Letuchy <hidden>
Date: 2016-06-15 22:46:15

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<redacted>
"""

-Eugene

+ cc: junio

On 2/20/09 5:40 AM, Johannes Schindelin wrote:
quoted
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:
quoted
From: Eugene Letuchy<redacted>

Adds the following:
We try to use the imperative form; from my experience it makes for an
easier read: "Add the following:"
quoted
   - 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

From: Jeff King <hidden>
Date: 2016-06-15 22:46:15

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.
quoted
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.
quoted
    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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help