On 13/08/2019 18:06, Junio C Hamano wrote:
Phillip Wood <phillip.wood123@xxxxxxxxx> 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
