From: Eugene Letuchy <eletuchy@facebook.com>
Date: Mon, 23 Feb 2009 17:48:32 GMT
Subject: Re: [PATCH] adds --date=raw support to git blame and related documentation
Message-ID: <49A2E170.9030807@facebook.com>
In-Reply-To: <7vprh9t6xt.fsf@gitster.siamese.dyndns.org>

On 2/23/09 9:10 AM, Junio C Hamano wrote:
> eletuchy@gmail.com writes:
>
>> 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.
>

Yeah I can do that.

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

The output of -t and --date=raw are exactly identical (well, after this patch 
they are); for that reason, I think providing both is redundant but not an 
error. However, I wanted to retain -t for "git annotate" compatibility, which 
has -t as the sole date option. In git-annotate mode, no other --date mode 
options can apply.

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

Thu, 19 Oct 2006 16:00:04 -0700 is the date for the first line of builtin-blame.c
1161298804 -0700 is fine by me.

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

OK.

>
> Thanks.