Re: [PATCH] Make git blame date output format configurable, a la git log
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:46:15
eletuchy@gmail.com writes:
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>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.
quoted hunk
--- 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.
quoted hunk
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.
quoted hunk
@@ -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".
quoted hunk
@@ -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.
quoted hunk
@@ -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.
quoted hunk
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.