Re: [PATCH] adds --date=raw support to git blame and related documentation

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 23, 2009, 17:10 UTC
Message-ID: <7vprh9t6xt.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1235379429-20934-1-git-send-email-eletuchy@gmail.com>

eletuchy@gmail.com writes:

Show 5 quoted lines

> From: Eugene Letuchy <eugene@facebook.com>
>
> In the wake of Linus' 7dff9b3, git blame --date support needs to
> incorporate --date=raw in addition to the previously supported
> date formats.

Thanks, but I do not understand what you meant by the following two lines:

> Test: > git grep relative | grep iso | grep -v raw
>       > git blame --date=raw builtin-blame.c

With the patch to add --date=raw format already on 'master', I'd prefer a reroll of the original patch (it needs a fix for the config "don't ignore a misconfiguration" bug Peff pointed out anyway) with this documentation update patch squashed in.

Show 10 quoted lines

> diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
> index e6717af..1316d4e 100644
> --- a/Documentation/blame-options.txt
> +++ b/Documentation/blame-options.txt
> @@ -36,7 +36,7 @@ of lines before or after the line given by <start>.
>  	Show long rev (Default: off).
>  
>  -t::
> -	Show raw timestamp (Default: off).
> +	Synomym for --date=raw (Default: off).

This is interesting. It suggests that we should internally get rid of show_raw_time variable (and need to error out when --date= and -t options are given at the same time, as they are mutually incompatible).

But do -t and --date=raw really behave identically? I think they should but I didn't check.

Show 12 quoted lines

> diff --git a/builtin-blame.c b/builtin-blame.c
> index 48cedfd..bb0d20b 100644
> --- a/builtin-blame.c
> +++ b/builtin-blame.c
> @@ -2288,12 +2288,16 @@ parse_done:
>  	case DATE_RELATIVE:
>  		blame_date_width = sizeof("14 minutes ago");
>  		break;
> +	case DATE_RAW:
> +		blame_date_width = sizeof("1235155266 -0800");
> +		output_option |= OUTPUT_RAW_TIMESTAMP;
> +		break;

I'd prefer it to see a same timestamp used consistently here. You seem to have used "Thu, 19 Oct 2006 16:00:04 -0700" for other case arms (I do not know what significant things happened at that time) and what I queued in 'pu' has sizeof("1161298804 -0700") there instead.

Show 7 quoted lines

>  	case DATE_LOCAL:
>  	case DATE_NORMAL:
>  		blame_date_width = sizeof("Thu Oct 19 16:00:04 2006 -0700");
>  		break;
>  	}
> -	blame_date_width -= 1; /* strip the null */
> +	blame_date_width -= 1; /* strip the terminating null */

The character with byte value 0 is called NUL.

Thanks.