Feature request: Add --mtime option to git archive

> On Thu, Feb 16, 2023 at 12:41:42PM -0700, Raul E Rangel wrote:
>
>> When generating a tarball with `git archive <tree>`, `git archive` will
>> use the current time as the mtime. This results in a non-hermetic
>> tarball. Could we should add a --mtime option that allows passing in
>> the time? 
>
> That seems like a very reasonable feature to have. Just to sketch out
> the implementation, in case anybody wants to work on it: ...

> >> When generating a tarball with `git archive <tree>`, `git archive` will
> >> use the current time as the mtime. This results in a non-hermetic
> >> tarball. Could we should add a --mtime option that allows passing in
> >> the time? 
> >
> > That seems like a very reasonable feature to have. Just to sketch out
> > the implementation, in case anybody wants to work on it: ...
> 
> There has been a discussion on not just "fix mtime" but coming up
> with a lot more stable tar archive format specification.

> A similar option in is to simply start using "0" in the meantime, like:
>
> diff --git a/archive.c b/archive.c
> index 81ff76fce9..48d89785c3 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -470,7 +470,7 @@ static void parse_treeish_arg(const char **argv,
>  		archive_time = commit->date;
>  	} else {
>  		commit_oid = NULL;
> -		archive_time = time(NULL);
> +		archive_time = 0;
>  	}
>  
>  	tree = parse_tree_indirect(&oid);
>
> Nobody will complain about changing the byte-for-byte format, since by definition it
> was already changing once per second (cue somebody complaining that they
> have been using LD_PRELOAD tricks to simulate --mtime).
>
> I do wonder if people would complain (both with the patch above and with
> brian's proposal) that the resulting tarballs extract everything with a
> date in 1970. That's not functionally a problem, but it looks kind of
> weird in "ls -l".

>
> Jeff King <peff@peff.net> writes:
>
> > A similar option in is to simply start using "0" in the meantime, like:
> >
> > diff --git a/archive.c b/archive.c
> > index 81ff76fce9..48d89785c3 100644
> > --- a/archive.c
> > +++ b/archive.c
> > @@ -470,7 +470,7 @@ static void parse_treeish_arg(const char **argv,
> >               archive_time = commit->date;
> >       } else {
> >               commit_oid = NULL;
> > -             archive_time = time(NULL);
> > +             archive_time = 0;
> >       }
> >
> >       tree = parse_tree_indirect(&oid);
> >
> > Nobody will complain about changing the byte-for-byte format, since by definition it
> > was already changing once per second (cue somebody complaining that they
> > have been using LD_PRELOAD tricks to simulate --mtime).
> >
> > I do wonder if people would complain (both with the patch above and with
> > brian's proposal) that the resulting tarballs extract everything with a
> > date in 1970. That's not functionally a problem, but it looks kind of
> > weird in "ls -l".
>
> And owned by root:root ;-)

>
> I am sure people would complain.  What matters is if these
> complaints have merit, and in this case, I doubt it.  I especially
> like your "it has been already changing once per second" reasoning
> for this change.

> On Thu, Feb 16, 2023 at 7:04 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Jeff King <peff@peff.net> writes:
>>
>>> A similar option in is to simply start using "0" in the meantime, like:
>>>
>>> diff --git a/archive.c b/archive.c
>>> index 81ff76fce9..48d89785c3 100644
>>> --- a/archive.c
>>> +++ b/archive.c
>>> @@ -470,7 +470,7 @@ static void parse_treeish_arg(const char **argv,
>>>               archive_time = commit->date;
>>>       } else {
>>>               commit_oid = NULL;
>>> -             archive_time = time(NULL);
>>> +             archive_time = 0;
>>>       }
>>>
>>>       tree = parse_tree_indirect(&oid);
>>>
>>> Nobody will complain about changing the byte-for-byte format, since by definition it
>>> was already changing once per second (cue somebody complaining that they
>>> have been using LD_PRELOAD tricks to simulate --mtime).

>> And owned by root:root ;-)
>
> I fully support both of those easy changes. Only reason I proposed
> --mtime was that https://reproducible-builds.org/docs/archives/
> recommends setting SOURCE_DATE_EPOCH, but honestly for my purposes I
> would always use 0 and root:root.

> > I do wonder if people would complain (both with the patch above and with
> > brian's proposal) that the resulting tarballs extract everything with a
> > date in 1970. That's not functionally a problem, but it looks kind of
> > weird in "ls -l".
> 
> And owned by root:root ;-)

> > I do wonder if people would complain (both with the patch above and with
> > brian's proposal) that the resulting tarballs extract everything with a
> > date in 1970. That's not functionally a problem, but it looks kind of
> > weird in "ls -l".
>
> And owned by root:root ;-)
>
> I am sure people would complain.  What matters is if these
> complaints have merit, and in this case, I doubt it.  I especially
> like your "it has been already changing once per second" reasoning
> for this change.

> I could understand this position if letting the user set the --mtime
> implied some harm that must be mitigated, but it seems like an odd
> choice if there is none.  Especially given it would also future proof
> git against people coming up with a good reason not to use the 0 epoch
> in the future that you haven't thought of right now. It is not like
> epoch's and unix time have a totally uncontroversial and stable
> history. 0 has meant multiple dates over time, and the current
> definition of 1 Jan 1970, 00:00:00 UTC is problematic as UTC didn't
> exist until 1972!  Given it clearly wouldn't be hard to allow users to
> select the epoch in these archives then why not do so?

> I have seen devs have issues with stuff like this in the past.
> Unpacking an archive on one machine showing a different date than one
> another, or other weird artifacts. Mac used to use a different 0 epoch
> than windows and linux as I recall, etc etc.  I dont remember the gory
> details, but i have definitely seen people gnash their teeth over
> these kind of decisions before. Why not side-step that if you can and
> let people choose their own defaults?

> +--mtime=<time>::
> +	Set modification time of archive entries.  Without this option
> +	the committer time is used if `<tree-ish>` is a commit or tag,
> +	and the current time if it is a tree.
> +
>  <extra>::
>  	This can be any options that the archiver backend understands.
>  	See next section.
> diff --git a/archive.c b/archive.c
> index 81ff76fce9..122860b39d 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -472,6 +472,8 @@ static void parse_treeish_arg(const char **argv,
>  		commit_oid = NULL;
>  		archive_time = time(NULL);
>  	}
> +	if (ar_args->mtime_option)
> +		archive_time = approxidate(ar_args->mtime_option);

> René Scharfe <l.s.r@web.de> writes:
>
>> +--mtime=<time>::
>> +	Set modification time of archive entries.  Without this option
>> +	the committer time is used if `<tree-ish>` is a commit or tag,
>> +	and the current time if it is a tree.
>> +
>>  <extra>::
>>  	This can be any options that the archiver backend understands.
>>  	See next section.
>> diff --git a/archive.c b/archive.c
>> index 81ff76fce9..122860b39d 100644
>> --- a/archive.c
>> +++ b/archive.c
>> @@ -472,6 +472,8 @@ static void parse_treeish_arg(const char **argv,
>>  		commit_oid = NULL;
>>  		archive_time = time(NULL);
>>  	}
>> +	if (ar_args->mtime_option)
>> +		archive_time = approxidate(ar_args->mtime_option);
>
> This is the solution with least damage, letting the existing code to
> set archive_time and then discard the result and overwrite with the
> command line option.

>> This is the solution with least damage, letting the existing code to
>> set archive_time and then discard the result and overwrite with the
>> command line option.
>
> I actually like Peff's solution more, because it's short and solves the
> specific problem of non-deterministic timestamps for tree archives.

> ---
>  archive.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/archive.c b/archive.c
> index 122860b39d..871d80ee79 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -438,6 +438,15 @@ static void parse_pathspec_arg(const char **pathspec,
>  	}
>  }
>
> +static timestamp_t approxidate_or_die(const char *date_str)
> +{
> +	int errors = 0;
> +	timestamp_t date = approxidate_careful(date_str, &errors);
> +	if (errors)
> +		die(_("invalid date format: %s"), date_str);
> +	return date;
> +}
> +
>  static void parse_treeish_arg(const char **argv,
>  		struct archiver_args *ar_args, const char *prefix,
>  		int remote)
> @@ -473,7 +482,7 @@ static void parse_treeish_arg(const char **argv,
>  		archive_time = time(NULL);
>  	}
>  	if (ar_args->mtime_option)
> -		archive_time = approxidate(ar_args->mtime_option);
> +		archive_time = approxidate_or_die(ar_args->mtime_option);
>
>  	tree = parse_tree_indirect(&oid);
>  	if (!tree)
> --
> 2.39.2

> René Scharfe <l.s.r@web.de> writes:
> 
> >> This is the solution with least damage, letting the existing code to
> >> set archive_time and then discard the result and overwrite with the
> >> command line option.
> >
> > I actually like Peff's solution more, because it's short and solves the
> > specific problem of non-deterministic timestamps for tree archives.
> 
> Yes.  That would be my preference as well.  Without any UI to
> educate users about.