Re: [PATCH v5 4/8] Specify explicitly where we parse timestamps

[Junio C Hamano <gitster@pobox.com>]

oJohannes Schindelin <johannes.schindelin@gmx.de> writes:

> diff --git a/pretty.c b/pretty.c
> index d0f86f5d85c..24fb0c79062 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -409,7 +409,7 @@ const char *show_ident_date(const struct ident_split *ident,
>  	long tz = 0;
>  
>  	if (ident->date_begin && ident->date_end)
> -		date = strtoul(ident->date_begin, NULL, 10);
> +		date = parse_timestamp(ident->date_begin, NULL, 10);
>  	if (date_overflows(date))
>  		date = 0;
>  	else {

Good.  We'd need to choose a different sentinel when we allow signed
timestamps, but that is outside the scope of this series.  

It would not hurt if we used a symbolic constant instead of 0 here
to save duplicated effort for future developers, though, and it is
in line with the spirit of this exact patch where it uses a more
specific function name to differentiate strtoul() used for times
from other uses of the same function.

In any case, I think another change to this section of the code made
later in another patch was where t4121 was broken in the previous
round, and it is good to see the breakage go.  As Peff was hinting,
we might want to revisit "should we substitute with a sentinel, or
make this a die?" decision, but discussing it is totally outside the
scope of this series.