Re: [PATCH] archive: add --mtime

From: René Scharfe <l.s.r@web.de>
Date: Feb 19, 2023, 10:44 UTC
Message-ID: <57b6643a-b9ff-3ea4-d60d-1a434d9ea75e@web.de>
In-Reply-To: <xmqqilfykhsf.fsf@gitster.g>

Am 18.02.23 um 18:25 schrieb Junio C Hamano:

Show 24 quoted lines

> 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.

I actually like Peff's solution more, because it's short and solves the specific problem of non-deterministic timestamps for tree archives. The downside of spreading ancient timestamps seems only cosmetic to me. It would be ideal if there was a way to specify a NULL timestamp or none at all. The --mtime option on the other hand mimics GNU tar, so it is more familiar and proven, though.

> I wonder if we want to use approxidate_careful() to deal with bogus
> input?  The code is perfectly serviceable without it (users who feed
> bogus input deserve what they get), but some folks might prefer to
> be "nicer" than necessary ;-)

It isn't all that careful, but you're right that we should do what we can. Like this on top? The message string is borrowed from commit's handling of --date.

---
 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