From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 14 Aug 2019 18:38:45 GMT
Subject: Re: [GSoC][PATCH v2 3/6] rebase -i: support --committer-date-is-author-date
Message-ID: <0a0a6fd3-933e-c0a4-03c2-8c1c69e5ffca@gmail.com>
In-Reply-To: <xmqqsgq5805u.fsf@gitster-ct.c.googlers.com>

On 13/08/2019 18:06, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>  [...]
>>> +
>>> +		strbuf_addf(&date, "@%s",ident.date_begin);
>>
>> I think we should use %s.* and ident.date_end to be sure we getting
>> what we want. Your version is OK if the author is formatted correctly
>> but I'm uneasy about relying on that when we can get the verified end
>> from ident.
> 
> If the author line is not formatted correctly, split_ident_line()
> would notice and return NULL in these fields, I think (in other
> words, my take on the call to split_ident_line() above is not
> necessarily done in order to "split", but primarily to validate that
> the line is formatted correctly---and find the beginning of the
> timestamp field).

I just had a read through split_ident_line() and it looks to me like it 
will ignore any junk after the timezone. So long as it sees '+' or '-' 
followed by at least one digit it will use that for the time zone and 
return success regardless of what follows it so I think we want to pay 
attention to the end data it returns for the date and timezone.

> But your "pay attention to date_end" raises an interesting point.
> 
> The string that follows ident.date_begin would be a large integer
> (i.e. number of seconds since epoch), a SP, a positive or negative
> sign (i.e. east or west of GMT), 4 digits (i.e. timezone offset), so
> if you want to leave something like "@1544328981" in the buffer, you
> need to stop at .date_end to omit the timezone information.
> 
> On the other hand, if you do want the timezone information as well
> (which I think is the case for this codepath), you should not stop
> at there and have something like "@1544328981 +0900", the code as
> written would give better result.

Good point, I had forgotten that split_ident_line() returned separate 
fields for the date and timezone. I agree that we want the timezone here 
too. I  I think it would be a good idea to beef up the tests to use a 
non default timezone to check that we are actually setting it correctly.

Best Wishes

Phillip