Re: [PATCH v5 6/8] Introduce a new data type for timestamps

["René Scharfe" <l.s.r@web.de>]

Am 24.04.2017 um 15:58 schrieb Johannes Schindelin:
> diff --git a/archive-tar.c b/archive-tar.c
> index 380e3aedd23..695339a2369 100644
> --- a/archive-tar.c
> +++ b/archive-tar.c
> @@ -27,9 +27,12 @@ static int write_tar_filter_archive(const struct archiver *ar,
>    */
>   #if ULONG_MAX == 0xFFFFFFFF
>   #define USTAR_MAX_SIZE ULONG_MAX
> -#define USTAR_MAX_MTIME ULONG_MAX
>   #else
>   #define USTAR_MAX_SIZE 077777777777UL
> +#endif
> +#if TIME_MAX == 0xFFFFFFFF
> +#define USTAR_MAX_MTIME TIME_MAX
> +#else
>   #define USTAR_MAX_MTIME 077777777777UL
>   #endif
>   
> diff --git a/archive-zip.c b/archive-zip.c
> index b429a8d974a..68df3d64402 100644
> --- a/archive-zip.c
> +++ b/archive-zip.c
> @@ -545,9 +545,17 @@ static void write_zip_trailer(const unsigned char *sha1)
>   		write_or_die(1, sha1_to_hex(sha1), GIT_SHA1_HEXSZ);
>   }
>   
> -static void dos_time(time_t *time, int *dos_date, int *dos_time)
> +static void dos_time(timestamp_t *timestamp, int *dos_date, int *dos_time)
>   {
> -	struct tm *t = localtime(time);
> +	time_t time;
> +	struct tm *t;
> +
> +	if (date_overflows(*timestamp))
> +		die("timestamp too large for this system: %"PRItime,
> +		    *timestamp);
> +	time = (time_t)*timestamp;
> +	t = localtime(&time);
> +	*timestamp = time;
>   
>   	*dos_date = t->tm_mday + (t->tm_mon + 1) * 32 +
>   	            (t->tm_year + 1900 - 1980) * 512;
> diff --git a/archive.h b/archive.h
> index 415e0152e2c..62d1d82c1af 100644
> --- a/archive.h
> +++ b/archive.h
> @@ -9,7 +9,7 @@ struct archiver_args {
>   	struct tree *tree;
>   	const unsigned char *commit_sha1;
>   	const struct commit *commit;
> -	time_t time;
> +	timestamp_t time;
>   	struct pathspec pathspec;
>   	unsigned int verbose : 1;
>   	unsigned int worktree_attributes : 1;

time_t is converted to timestamp_t here.  Hmm.

Truncation can already occur in archive.c::parse_treeish_arg() when
assigning commit->date (of type timestamp_t) to archive_time (of type
time_t).  The overflow check should either be moved there or the type of
the latter variable should be changed, right?

Compilation and tests are still successful on Debian x86 and MinGW64
even after removing the changes above.  On the former the test "generate
tar with future mtime" in t5000 writes Epoch time stamps in both cases
(and doesn't notice because tar is unable to handle times after 2038, so
that check is skipped), on the latter it emits the correct future dates.

So what are the benefits of these changes?

Unless I'm missing something (very possible at this time of day) I'd say
drop these hunks and let me handle the fallout of the series in archive.
I was planning on improving its date handling code anyway (eventually)..

René