* Feature request: Add --mtime option to git archive
@ 2023-02-16 19:41 Raul E Rangel
2023-02-16 21:05 ` Jeff King
2023-02-18 8:36 ` [PATCH] archive: add --mtime René Scharfe
0 siblings, 2 replies; 16+ messages in thread
From: Raul E Rangel @ 2023-02-16 19:41 UTC (permalink / raw
To: git
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?
Thanks!
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Feature request: Add --mtime option to git archive
2023-02-16 19:41 Feature request: Add --mtime option to git archive Raul E Rangel
@ 2023-02-16 21:05 ` Jeff King
2023-02-16 22:21 ` Junio C Hamano
2023-02-18 8:36 ` [PATCH] archive: add --mtime René Scharfe
1 sibling, 1 reply; 16+ messages in thread
From: Jeff King @ 2023-02-16 21:05 UTC (permalink / raw
To: Raul E Rangel; +Cc: git
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:
- options are read first in cmd_archive(), but it passes unknown ones
on to write_archive(), which is where we parse the interesting ones.
- that relies on parse_archive_args() in archive.c. So we'd want a new
option in opts[] there, and to write the value into a new field in
the archiver_args struct.
- the time is filled in via parse_treeish_arg(), where we use
time(NULL) if there's no commit. And there we'd just use the value
from archiver_args.
If there is a commit and they've specified --mtime, what should
happen? Quietly ignore it? Override the commit's timestamp? Complain
and bail?
- I didn't look at how this might interact with "git archive
--remote". I think it might just all work, as we just ship the
options to the other side, which parses them itself using the same
code.
-Peff
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Feature request: Add --mtime option to git archive
2023-02-16 21:05 ` Jeff King
@ 2023-02-16 22:21 ` Junio C Hamano
2023-02-17 0:50 ` Jeff King
0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2023-02-16 22:21 UTC (permalink / raw
To: Jeff King, brian m. carlson; +Cc: Raul E Rangel, git
Jeff King <peff@peff.net> writes:
> 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: ...
There has been a discussion on not just "fix mtime" but coming up
with a lot more stable tar archive format specification.
The first link is about documenting the status quo. The second one
is about the proposed format that aims to be stable.
https://lore.kernel.org/git/20230203080629.31492-1-ray@ameretat.dev/
https://lore.kernel.org/git/20230205221728.4179674-2-sandals@crustytoothpaste.net/
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Feature request: Add --mtime option to git archive
2023-02-16 22:21 ` Junio C Hamano
@ 2023-02-17 0:50 ` Jeff King
2023-02-17 2:04 ` Junio C Hamano
0 siblings, 1 reply; 16+ messages in thread
From: Jeff King @ 2023-02-17 0:50 UTC (permalink / raw
To: Junio C Hamano; +Cc: brian m. carlson, Raul E Rangel, git
On Thu, Feb 16, 2023 at 02:21:36PM -0800, Junio C Hamano 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: ...
>
> There has been a discussion on not just "fix mtime" but coming up
> with a lot more stable tar archive format specification.
Yes, I think in brian's proposal the mtime would always be 0. I don't
mind that either (and really, I doubt anybody would really want to set
--mtime to anything but a fixed, known value anyway). This is a much
smaller change that could be done in the meantime with less effort, but
I guess we'd be stuck with --mtime forever, then.
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".
-Peff
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: Feature request: Add --mtime option to git archive
2023-02-17 0:50 ` Jeff King
@ 2023-02-17 2:04 ` Junio C Hamano
2023-02-17 15:43 ` Raul Rangel
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Junio C Hamano @ 2023-02-17 2:04 UTC (permalink / raw
To: Jeff King; +Cc: brian m. carlson, Raul E Rangel, git
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Feature request: Add --mtime option to git archive
2023-02-17 2:04 ` Junio C Hamano
@ 2023-02-17 15:43 ` Raul Rangel
2023-02-17 20:31 ` René Scharfe
2023-02-17 20:25 ` Jeff King
2023-02-18 3:04 ` demerphq
2 siblings, 1 reply; 16+ messages in thread
From: Raul Rangel @ 2023-02-17 15:43 UTC (permalink / raw
To: Junio C Hamano; +Cc: Jeff King, brian m. carlson, git
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).
> >
> > 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 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 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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Feature request: Add --mtime option to git archive
2023-02-17 2:04 ` Junio C Hamano
2023-02-17 15:43 ` Raul Rangel
@ 2023-02-17 20:25 ` Jeff King
2023-02-18 3:04 ` demerphq
2 siblings, 0 replies; 16+ messages in thread
From: Jeff King @ 2023-02-17 20:25 UTC (permalink / raw
To: Junio C Hamano; +Cc: brian m. carlson, Raul E Rangel, git
On Thu, Feb 16, 2023 at 06:04:38PM -0800, Junio C Hamano wrote:
> > 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 ;-)
Yeah, though I think it's pretty standard these days to use
--no-same-owner or equivalent (and it's the default for non-root users).
And ditto for modes. I wondered if there was an equivalent for
timestamps. It looks like "-m" works (it just sets everything to the
current timestamp at time of extraction), but I don't know how portable
that is (and POSIX is no help with tar, as usual).
-Peff
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Feature request: Add --mtime option to git archive
2023-02-17 15:43 ` Raul Rangel
@ 2023-02-17 20:31 ` René Scharfe
0 siblings, 0 replies; 16+ messages in thread
From: René Scharfe @ 2023-02-17 20:31 UTC (permalink / raw
To: Raul Rangel, Junio C Hamano; +Cc: Jeff King, brian m. carlson, git
Am 17.02.23 um 16:43 schrieb Raul Rangel:
> 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 how are you now going to tell the current time on a remote git
server? ;)
>>> 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".
Yes, 21st century artifacts being sent back in time looks a bit strange.
GNU tar and bsdtar provide option -m to ignore mtimes when extracting.
>> 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.
git archive already records root as user and group.
>> 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 find it quite convincing as well.
René
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Feature request: Add --mtime option to git archive
2023-02-17 2:04 ` Junio C Hamano
2023-02-17 15:43 ` Raul Rangel
2023-02-17 20:25 ` Jeff King
@ 2023-02-18 3:04 ` demerphq
2023-02-18 17:08 ` brian m. carlson
2 siblings, 1 reply; 16+ messages in thread
From: demerphq @ 2023-02-18 3:04 UTC (permalink / raw
To: Junio C Hamano; +Cc: Jeff King, brian m. carlson, Raul E Rangel, git
On Fri, 17 Feb 2023 at 03:39, Junio C Hamano <gitster@pobox.com> wrote:
> > 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 don't really get the argument as far as it is presented as a reason
/not/ to support a --mtime option as was requested in this thread.
You yourself say "people will complain", and you seem to be planning
to just ignore those complaints as lacking merit. I honestly don't
understand that, wouldn't it be less noise, less stress, and better
"customer service" to let the user choose what constant is in the
archive? Then no complaints, and you don't have to explain/argue
to/with everybody why their complaint is without merit over and over.
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?
Just my $0.02.
cheers,
Yves
--
perl -Mre=debug -e "/just|another|perl|hacker/"
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] archive: add --mtime
2023-02-16 19:41 Feature request: Add --mtime option to git archive Raul E Rangel
2023-02-16 21:05 ` Jeff King
@ 2023-02-18 8:36 ` René Scharfe
2023-02-18 17:25 ` Junio C Hamano
1 sibling, 1 reply; 16+ messages in thread
From: René Scharfe @ 2023-02-18 8:36 UTC (permalink / raw
To: Raul E Rangel, git; +Cc: Jeff King, Junio C Hamano, demerphq
Allow users to specify the modification time of archive entries. The
new option --mtime uses approxidate() to parse a time specification and
overrides the default of using the current time for trees and the commit
time for tags and commits. It can be used to create a reproducible
archive for a tree, or to use a specific mtime without creating a commit
with GIT_COMMITTER_DATE set.
This implementation doesn't support the negated form of the new option,
i.e. --no-mtime is not accepted. It is not possible to have no mtime at
all. We could use the Unix epoch or revert to the default behavior, but
since negation is not necessary for the intended use it's left undecided
for now.
Requested-by: Raul E Rangel <rrangel@chromium.org>
Suggested-by: demerphq <demerphq@gmail.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
Documentation/git-archive.txt | 5 +++++
archive.c | 7 +++++++
archive.h | 1 +
t/t5000-tar-tree.sh | 19 +++++++++++++++++++
4 files changed, 32 insertions(+)
diff --git a/Documentation/git-archive.txt b/Documentation/git-archive.txt
index 60c040988b..6bab201d37 100644
--- a/Documentation/git-archive.txt
+++ b/Documentation/git-archive.txt
@@ -86,6 +86,11 @@ cases, write an untracked file and use `--add-file` instead.
Look for attributes in .gitattributes files in the working tree
as well (see <<ATTRIBUTES>>).
+--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);
tree = parse_tree_indirect(&oid);
if (!tree)
@@ -586,6 +588,7 @@ static int parse_archive_args(int argc, const char **argv,
const char *remote = NULL;
const char *exec = NULL;
const char *output = NULL;
+ const char *mtime_option = NULL;
int compression_level = -1;
int verbose = 0;
int i;
@@ -607,6 +610,9 @@ static int parse_archive_args(int argc, const char **argv,
OPT_BOOL(0, "worktree-attributes", &worktree_attributes,
N_("read .gitattributes in working directory")),
OPT__VERBOSE(&verbose, N_("report archived files on stderr")),
+ { OPTION_STRING, 0, "mtime", &mtime_option, N_("time"),
+ N_("set modification time of archive entries"),
+ PARSE_OPT_NONEG },
OPT_NUMBER_CALLBACK(&compression_level,
N_("set compression level"), number_callback),
OPT_GROUP(""),
@@ -668,6 +674,7 @@ static int parse_archive_args(int argc, const char **argv,
args->base = base;
args->baselen = strlen(base);
args->worktree_attributes = worktree_attributes;
+ args->mtime_option = mtime_option;
return argc;
}
diff --git a/archive.h b/archive.h
index 08bed3ed3a..7178e2a9a2 100644
--- a/archive.h
+++ b/archive.h
@@ -16,6 +16,7 @@ struct archiver_args {
struct tree *tree;
const struct object_id *commit_oid;
const struct commit *commit;
+ const char *mtime_option;
timestamp_t time;
struct pathspec pathspec;
unsigned int verbose : 1;
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index eb3214bc17..918a2fc7c6 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -105,6 +105,18 @@ check_added() {
'
}
+check_mtime() {
+ dir=$1
+ path_in_archive=$2
+ mtime=$3
+
+ test_expect_success " validate mtime of $path_in_archive" '
+ test-tool chmtime --get $dir/$path_in_archive >actual.mtime &&
+ echo $mtime >expect.mtime &&
+ test_cmp expect.mtime actual.mtime
+ '
+}
+
test_expect_success 'setup' '
test_oid_cache <<-EOF
obj sha1:19f9c8273ec45a8938e6999cb59b3ff66739902a
@@ -174,6 +186,13 @@ test_expect_success 'git archive' '
check_tar b
+test_expect_success 'git archive --mtime' '
+ git archive --mtime=2002-02-02T02:02:02-0200 HEAD >with_mtime.tar
+'
+
+check_tar with_mtime
+check_mtime with_mtime a/a 1012622522
+
test_expect_success 'git archive --prefix=prefix/' '
git archive --prefix=prefix/ HEAD >with_prefix.tar
'
--
2.39.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: Feature request: Add --mtime option to git archive
2023-02-18 3:04 ` demerphq
@ 2023-02-18 17:08 ` brian m. carlson
0 siblings, 0 replies; 16+ messages in thread
From: brian m. carlson @ 2023-02-18 17:08 UTC (permalink / raw
To: demerphq; +Cc: Junio C Hamano, Jeff King, Raul E Rangel, git
[-- Attachment #1: Type: text/plain, Size: 1910 bytes --]
On 2023-02-18 at 03:04:12, demerphq wrote:
> 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?
If folks want such an option, they're welcome to send a patch to do
that. My approach is providing a stable tar format that doesn't change,
and for people who want to use that format, it has fixed timestamps for
trees (so it's completely rigid). If people want to use the default
format with an arbitrary timestamp, that's fine, and we can support
that, but we won't guarantee that that format won't change its
serialization in the future.
> 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?
The ustar format is defined by POSIX and uses the Unix epoch, just like
the zip format has its own epoch. If you're processing a tar archive,
you need to use the Unix epoch.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] archive: add --mtime
2023-02-18 8:36 ` [PATCH] archive: add --mtime René Scharfe
@ 2023-02-18 17:25 ` Junio C Hamano
2023-02-19 10:44 ` René Scharfe
0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2023-02-18 17:25 UTC (permalink / raw
To: René Scharfe; +Cc: Raul E Rangel, git, Jeff King, demerphq
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 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 ;-)
Other than that, looks very good. Thanks.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] archive: add --mtime
2023-02-18 17:25 ` Junio C Hamano
@ 2023-02-19 10:44 ` René Scharfe
2023-02-21 5:37 ` Junio C Hamano
0 siblings, 1 reply; 16+ messages in thread
From: René Scharfe @ 2023-02-19 10:44 UTC (permalink / raw
To: Junio C Hamano; +Cc: Raul E Rangel, git, Jeff King, demerphq
Am 18.02.23 um 18:25 schrieb Junio C Hamano:
> 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] archive: add --mtime
2023-02-19 10:44 ` René Scharfe
@ 2023-02-21 5:37 ` Junio C Hamano
2023-02-22 19:51 ` Jeff King
0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2023-02-21 5:37 UTC (permalink / raw
To: René Scharfe; +Cc: Raul E Rangel, git, Jeff King, demerphq
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.
> The --mtime option on the other hand mimics GNU tar, so it is more
> familiar and proven, though.
And that gives us the second best option ;-)
> 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.
Yeah, something like that, I would think.
Thanks.
> ---
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] archive: add --mtime
2023-02-21 5:37 ` Junio C Hamano
@ 2023-02-22 19:51 ` Jeff King
2023-02-22 23:23 ` Junio C Hamano
0 siblings, 1 reply; 16+ messages in thread
From: Jeff King @ 2023-02-22 19:51 UTC (permalink / raw
To: Junio C Hamano; +Cc: René Scharfe, Raul E Rangel, git, demerphq
On Mon, Feb 20, 2023 at 09:37:18PM -0800, Junio C Hamano wrote:
> 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.
My biggest concern with the patch I showed is that it gives no escape
hatch if people don't like the change. Like I said earlier, I don't
think anybody has grounds to complain about the byte-for-byte output
hash changing, as it would be changing once per second. But they may
complain about the cosmetic problem.
One "escape hatch" is to tell them to use "tar -m", but I don't know how
friendly that is. The more obvious one at the Git level is to have
"--current-mtime" or something to get the old behavior. But at that
point, you may as well support "--mtime=now", which is the same amount
of work, and much more flexible.
-Peff
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] archive: add --mtime
2023-02-22 19:51 ` Jeff King
@ 2023-02-22 23:23 ` Junio C Hamano
0 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2023-02-22 23:23 UTC (permalink / raw
To: Jeff King; +Cc: René Scharfe, Raul E Rangel, git, demerphq
Jeff King <peff@peff.net> writes:
> My biggest concern with the patch I showed is that it gives no escape
> hatch if people don't like the change.
Yeah.
> Like I said earlier, I don't
> think anybody has grounds to complain about the byte-for-byte output
> hash changing, as it would be changing once per second. But they may
> complain about the cosmetic problem.
Yeah, they may complain "it no longer is possible when I took the
archive out of a tree". To be honest, I didn't think of that line
of complaints before.
> The more obvious one at the Git level is to have
> "--current-mtime" or something to get the old behavior. But at that
> point, you may as well support "--mtime=now", which is the same amount
> of work, and much more flexible.
Yup. Let's merge it down to 'next' then.
Thanks.
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2023-02-22 23:24 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-16 19:41 Feature request: Add --mtime option to git archive Raul E Rangel
2023-02-16 21:05 ` Jeff King
2023-02-16 22:21 ` Junio C Hamano
2023-02-17 0:50 ` Jeff King
2023-02-17 2:04 ` Junio C Hamano
2023-02-17 15:43 ` Raul Rangel
2023-02-17 20:31 ` René Scharfe
2023-02-17 20:25 ` Jeff King
2023-02-18 3:04 ` demerphq
2023-02-18 17:08 ` brian m. carlson
2023-02-18 8:36 ` [PATCH] archive: add --mtime René Scharfe
2023-02-18 17:25 ` Junio C Hamano
2023-02-19 10:44 ` René Scharfe
2023-02-21 5:37 ` Junio C Hamano
2023-02-22 19:51 ` Jeff King
2023-02-22 23:23 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).