Re: [PATCH 2/6] Specify explicitly where we parse timestamps

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 27, 2017, 22:37 UTC
Message-ID: <xmqqwpcbmfai.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <12b60c14dad15e3252e314771b3fe369305bbfc5.1488231002.git.johannes.schindelin@gmx.de>

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

Show 5 quoted lines

> Currently, Git's source code represents all timestamps as `unsigned
> long`. In preparation for using `time_t` instead, let's introduce a
> symbol `parse_timestamp` (currently being defined to `strtoul`) where
> appropriate, so that we can later easily switch to use `strtoull()`
> instead.

This definitely is a very good thing to do as a separate step.

Show 11 quoted lines

> diff --git a/date.c b/date.c
> index a996331f5b3..a8848f6e141 100644
> --- a/date.c
> +++ b/date.c
> ...
> @@ -1066,7 +1066,7 @@ static const char *approxidate_digit(const char *date, struct tm *tm, int *num,
>  				     time_t now)
>  {
>  	char *end;
> -	unsigned long number = strtoul(date, &end, 10);
> +	time_t number = parse_timestamp(date, &end, 10);

This hunk does not belong to this step. Everybody else in this step still receives parse_timestamp()'s return value in ulong, not time_t. I presume that that will happen in the final step 6/6 (which could be a huge patch that exceeds 100k?)