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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 20, 2009, 16:59 UTC
Message-ID: <7vwsblrqm7.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1235136252-29649-1-git-send-email-eletuchy@gmail.com>

eletuchy@gmail.com writes:

Show 15 quoted lines

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

Show 19 quoted lines

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

Show 8 quoted lines

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

Show 6 quoted lines

> @@ -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.

Show 5 quoted lines

> @@ -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.

Show 8 quoted lines

> ...
> +	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.

Show 12 quoted lines

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