{"thread":{"id":"49621","subject":"[PATCH 0/1] Fix format detection when archiving remotely","startedAt":"2018-10-19T23:19:33Z","lastAt":"2018-10-26T01:17:13Z","messageCount":20,"participants":["steadmon@google.com","Jeff King","Junio C Hamano","Josh Steadmon"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"360987","messageId":"cover.1539990488.git.steadmon@google.com","threadId":"49621","inReplyTo":null,"subject":"[PATCH 0/1] Fix format detection when archiving remotely","fromName":"","fromEmail":"steadmon@google.com","sentAt":"2018-10-19T23:19:27Z","receivedAt":"2018-10-19T23:19:33Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Currently, git-archive does not properly determine the desired archive\nformat when both --output and --remote are provided, because\nrun_remote_archiver() does not initialize the archivers prior to calling\narchive_format_from_filename(). This results in the remote archiver\nalways returning a TAR file, regardless of the requested format.\n\nThis patch initializes the TAR and ZIP archivers before calling\narchive_format_from_filename(), which fixes format detection.\n\nSteps to reproduce:\n\n∫ git version\ngit version 2.19.1.568.g152ad8e336-goog\n∫ cd ~/src/git\n∫ git archive --output ~/good.zip HEAD\n∫ file ~/good.zip\n/home/steadmon/good.zip: Zip archive data, at least v1.0 to extract\n∫ git archive --output ~/bad.zip --remote=. HEAD\n∫ file ~/bad.zip\n/home/steadmon/bad.zip: POSIX tar archive\n\n(apply patch and build)\n\n∫ ./bin-wrappers/git archive --output ~/fixed.zip --remote=. HEAD\n∫ file ~/fixed.zip\n/home/steadmon/fixed.zip: Zip archive data, at least v1.0 to extract\n\n\nJosh Steadmon (1):\n  archive: init archivers before determining format\n\n builtin/archive.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\n-- \n2.19.1.568.g152ad8e336-goog\n\n"},{"id":"360988","messageId":"b1f8e288dde9a9dd46386524189da66c7ad2c333.1539990488.git.steadmon@google.com","threadId":"49621","inReplyTo":"cover.1539990488.git.steadmon@google.com","subject":"[PATCH 1/1] archive: init archivers before determining format","fromName":"","fromEmail":"steadmon@google.com","sentAt":"2018-10-19T23:19:28Z","receivedAt":"2018-10-19T23:19:36Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When passing both --remote and --output to git-archive, initialize the\narchivers before attempting to determine the format from the output\nfilename. Without initialization, the format cannot be determined.\n\nSigned-off-by: Josh Steadmon <steadmon@google.com>\n---\n builtin/archive.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex e74f675390..dd3283a247 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -45,7 +45,10 @@ static int run_remote_archiver(int argc, const char **argv,\n \t * it.\n \t */\n \tif (name_hint) {\n-\t\tconst char *format = archive_format_from_filename(name_hint);\n+\t\tconst char *format;\n+\t\tinit_tar_archiver();\n+\t\tinit_zip_archiver();\n+\t\tformat = archive_format_from_filename(name_hint);\n \t\tif (format)\n \t\t\tpacket_write_fmt(fd[1], \"argument --format=%s\\n\", format);\n \t}\n-- \n2.19.1.568.g152ad8e336-goog\n\n"},{"id":"360990","messageId":"20181019234126.GA7326@sigill.intra.peff.net","threadId":"49621","inReplyTo":"cover.1539990488.git.steadmon@google.com","subject":"Re: [PATCH 0/1] Fix format detection when archiving remotely","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-19T23:41:26Z","receivedAt":"2018-10-19T23:41:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 19, 2018 at 04:19:27PM -0700, steadmon@google.com wrote:\n\n> Currently, git-archive does not properly determine the desired archive\n> format when both --output and --remote are provided, because\n> run_remote_archiver() does not initialize the archivers prior to calling\n> archive_format_from_filename(). This results in the remote archiver\n> always returning a TAR file, regardless of the requested format.\n> \n> This patch initializes the TAR and ZIP archivers before calling\n> archive_format_from_filename(), which fixes format detection.\n\nIt seems like some of this content could be in the commit message of the\nactual patch.\n\n> Steps to reproduce:\n> \n> ∫ git version\n> git version 2.19.1.568.g152ad8e336-goog\n> ∫ cd ~/src/git\n> ∫ git archive --output ~/good.zip HEAD\n> ∫ file ~/good.zip\n> /home/steadmon/good.zip: Zip archive data, at least v1.0 to extract\n> ∫ git archive --output ~/bad.zip --remote=. HEAD\n> ∫ file ~/bad.zip\n> /home/steadmon/bad.zip: POSIX tar archive\n\nAnd this could be in a test script in the actual patch. :)\n\n-Peff\n"},{"id":"360991","messageId":"20181019235945.GB7326@sigill.intra.peff.net","threadId":"49621","inReplyTo":"b1f8e288dde9a9dd46386524189da66c7ad2c333.1539990488.git.steadmon@google.com","subject":"Re: [PATCH 1/1] archive: init archivers before determining format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-19T23:59:46Z","receivedAt":"2018-10-19T23:59:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 19, 2018 at 04:19:28PM -0700, steadmon@google.com wrote:\n\n> diff --git a/builtin/archive.c b/builtin/archive.c\n> index e74f675390..dd3283a247 100644\n> --- a/builtin/archive.c\n> +++ b/builtin/archive.c\n> @@ -45,7 +45,10 @@ static int run_remote_archiver(int argc, const char **argv,\n>  \t * it.\n>  \t */\n>  \tif (name_hint) {\n> -\t\tconst char *format = archive_format_from_filename(name_hint);\n> +\t\tconst char *format;\n> +\t\tinit_tar_archiver();\n> +\t\tinit_zip_archiver();\n> +\t\tformat = archive_format_from_filename(name_hint);\n>  \t\tif (format)\n>  \t\t\tpacket_write_fmt(fd[1], \"argument --format=%s\\n\", format);\n\nHrm. This code was added back in 56baa61d01 (archive: move file\nextension format-guessing lower, 2011-06-21), and your example\ninvocation worked back then!\n\nUnfortunately it was broken by the very next patch in the series,\n08716b3c11 (archive: refactor file extension format-guessing,\n2011-06-21). I guess that's what I get for not adding regression tests.\n\nIt's probably worth mentioning those points in the commit message.\n\nDoes this work with configured archiver extensions, too? I think so,\nbecause we load them via init_tar_archiver().\n\nCan we avoid repeating the list of archivers here? This needs to stay in\nsync with the list in write_archive(). I know there are only two, but\ncan we factor out an init_archivers() call or something?\n\nWe also should probably just call it unconditionally when we start the\narchiver command (I don't think there are any other bugs like this\nlurking, but it doesn't cost very much to initialize these; it makes\nsense to just do it early).\n\nOther than those minor points (and the lack of test), your fix looks\ngood to me.\n\n-Peff\n"},{"id":"361127","messageId":"xmqq4ldey876.fsf@gitster-ct.c.googlers.com","threadId":"49621","inReplyTo":"20181019235945.GB7326@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] archive: init archivers before determining format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-22T03:24:45Z","receivedAt":"2018-10-22T03:24:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Oct 19, 2018 at 04:19:28PM -0700, steadmon@google.com wrote:\n>\n>> diff --git a/builtin/archive.c b/builtin/archive.c\n>> index e74f675390..dd3283a247 100644\n>> --- a/builtin/archive.c\n>> +++ b/builtin/archive.c\n>> @@ -45,7 +45,10 @@ static int run_remote_archiver(int argc, const char **argv,\n>>  \t * it.\n>>  \t */\n>>  \tif (name_hint) {\n>> -\t\tconst char *format = archive_format_from_filename(name_hint);\n>> +\t\tconst char *format;\n>> +\t\tinit_tar_archiver();\n>> +\t\tinit_zip_archiver();\n>> +\t\tformat = archive_format_from_filename(name_hint);\n>>  \t\tif (format)\n>>  \t\t\tpacket_write_fmt(fd[1], \"argument --format=%s\\n\", format);\n>\n> Hrm. This code was added back in 56baa61d01 (archive: move file\n> extension format-guessing lower, 2011-06-21), and your example\n> invocation worked back then!\n>\n> Unfortunately it was broken by the very next patch in the series,\n> 08716b3c11 (archive: refactor file extension format-guessing,\n> 2011-06-21). I guess that's what I get for not adding regression tests.\n>\n> It's probably worth mentioning those points in the commit message.\n>\n> Does this work with configured archiver extensions, too? I think so,\n> because we load them via init_tar_archiver().\n>\n> Can we avoid repeating the list of archivers here? This needs to stay in\n> sync with the list in write_archive(). I know there are only two, but\n> can we factor out an init_archivers() call or something?\n>\n> We also should probably just call it unconditionally when we start the\n> archiver command (I don't think there are any other bugs like this\n> lurking, but it doesn't cost very much to initialize these; it makes\n> sense to just do it early).\n>\n> Other than those minor points (and the lack of test), your fix looks\n> good to me.\n\nThanks for a patch and an excellent review.  Looking forward to the\nfinalized version.\n\nThanks, both.\n"},{"id":"361207","messageId":"20181022214617.GB233961@google.com","threadId":"49621","inReplyTo":"20181019234126.GA7326@sigill.intra.peff.net","subject":"Re: [PATCH 0/1] Fix format detection when archiving remotely","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-10-22T21:46:17Z","receivedAt":"2018-10-22T21:46:25Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.10.19 19:41, Jeff King wrote:\n> On Fri, Oct 19, 2018 at 04:19:27PM -0700, steadmon@google.com wrote:\n> \n> > Currently, git-archive does not properly determine the desired archive\n> > format when both --output and --remote are provided, because\n> > run_remote_archiver() does not initialize the archivers prior to calling\n> > archive_format_from_filename(). This results in the remote archiver\n> > always returning a TAR file, regardless of the requested format.\n> > \n> > This patch initializes the TAR and ZIP archivers before calling\n> > archive_format_from_filename(), which fixes format detection.\n> \n> It seems like some of this content could be in the commit message of the\n> actual patch.\n\nAck. I'll be sending v2 shortly, please let me know if I've missed\nanything that should be included.\n\n> > Steps to reproduce:\n> > \n> > ∫ git version\n> > git version 2.19.1.568.g152ad8e336-goog\n> > ∫ cd ~/src/git\n> > ∫ git archive --output ~/good.zip HEAD\n> > ∫ file ~/good.zip\n> > /home/steadmon/good.zip: Zip archive data, at least v1.0 to extract\n> > ∫ git archive --output ~/bad.zip --remote=. HEAD\n> > ∫ file ~/bad.zip\n> > /home/steadmon/bad.zip: POSIX tar archive\n> \n> And this could be in a test script in the actual patch. :)\n\nDone.\n"},{"id":"361208","messageId":"20181022214756.GC233961@google.com","threadId":"49621","inReplyTo":"20181019235945.GB7326@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] archive: init archivers before determining format","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-10-22T21:47:56Z","receivedAt":"2018-10-22T21:48:04Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.10.19 19:59, Jeff King wrote:\n> On Fri, Oct 19, 2018 at 04:19:28PM -0700, steadmon@google.com wrote:\n> \n> > diff --git a/builtin/archive.c b/builtin/archive.c\n> > index e74f675390..dd3283a247 100644\n> > --- a/builtin/archive.c\n> > +++ b/builtin/archive.c\n> > @@ -45,7 +45,10 @@ static int run_remote_archiver(int argc, const char **argv,\n> >  \t * it.\n> >  \t */\n> >  \tif (name_hint) {\n> > -\t\tconst char *format = archive_format_from_filename(name_hint);\n> > +\t\tconst char *format;\n> > +\t\tinit_tar_archiver();\n> > +\t\tinit_zip_archiver();\n> > +\t\tformat = archive_format_from_filename(name_hint);\n> >  \t\tif (format)\n> >  \t\t\tpacket_write_fmt(fd[1], \"argument --format=%s\\n\", format);\n> \n> Hrm. This code was added back in 56baa61d01 (archive: move file\n> extension format-guessing lower, 2011-06-21), and your example\n> invocation worked back then!\n> \n> Unfortunately it was broken by the very next patch in the series,\n> 08716b3c11 (archive: refactor file extension format-guessing,\n> 2011-06-21). I guess that's what I get for not adding regression tests.\n> \n> It's probably worth mentioning those points in the commit message.\n\nDone.\n\n> Does this work with configured archiver extensions, too? I think so,\n> because we load them via init_tar_archiver().\n\nIf you mean things like .tgz and .tar.gz, then yes, they are affected by\nthe bug as well, and this patch fixes them. The test included in v2 uses\na .tgz file.\n\n> Can we avoid repeating the list of archivers here? This needs to stay in\n> sync with the list in write_archive(). I know there are only two, but\n> can we factor out an init_archivers() call or something?\n\nDone.\n\n> We also should probably just call it unconditionally when we start the\n> archiver command (I don't think there are any other bugs like this\n> lurking, but it doesn't cost very much to initialize these; it makes\n> sense to just do it early).\n\nDone.\n\n> Other than those minor points (and the lack of test), your fix looks\n> good to me.\n\nThanks for the review!\n"},{"id":"361209","messageId":"bc6f20274dfe11f1451745e0accb065544cc59ca.1540244445.git.steadmon@google.com","threadId":"49621","inReplyTo":"cover.1539990488.git.steadmon@google.com","subject":"[PATCH v2] archive: initialize archivers earlier","fromName":"","fromEmail":"steadmon@google.com","sentAt":"2018-10-22T21:48:11Z","receivedAt":"2018-10-22T21:48:16Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Initialize archivers as soon as possible when running git-archive and\ngit-upload-archive. Various non-obvious behavior depends on having the\narchivers initialized, such as determining the desired archival format\nfrom the provided filename.\n\nSince 08716b3c11 (\"archive: refactor file extension format-guessing\",\n2011-06-21), archive_format_from_filename() has used the registered\narchivers to match filenames (provided via --output) to archival\nformats. However, when git-archive is executed with --remote, format\ndetection happens before the archivers have been registered. This causes\narchives from remotes to always be generated as TAR files, regardless of\nthe actual filename (unless an explicit --format is provided).\n\nThis patch fixes that behavior; archival format is determined properly\nfrom the output filename, even when --remote is used.\n\nSigned-off-by: Josh Steadmon <steadmon@google.com>\nHelped-by: Jeff King <peff@peff.net>\n---\n archive.c                | 9 ++++++---\n archive.h                | 1 +\n builtin/archive.c        | 2 ++\n builtin/upload-archive.c | 1 +\n t/t5000-tar-tree.sh      | 6 ++++++\n 5 files changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex c1870105eb..ce0f8a0362 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -29,6 +29,12 @@ void register_archiver(struct archiver *ar)\n \tarchivers[nr_archivers++] = ar;\n }\n \n+void init_archivers(void)\n+{\n+\tinit_tar_archiver();\n+\tinit_zip_archiver();\n+}\n+\n static void format_subst(const struct commit *commit,\n                          const char *src, size_t len,\n                          struct strbuf *buf)\n@@ -531,9 +537,6 @@ int write_archive(int argc, const char **argv, const char *prefix,\n \tgit_config_get_bool(\"uploadarchive.allowunreachable\", &remote_allow_unreachable);\n \tgit_config(git_default_config, NULL);\n \n-\tinit_tar_archiver();\n-\tinit_zip_archiver();\n-\n \targs.repo = repo;\n \targc = parse_archive_args(argc, argv, &ar, &args, name_hint, remote);\n \tif (!startup_info->have_repository) {\ndiff --git a/archive.h b/archive.h\nindex d4f97a00f5..21ac010699 100644\n--- a/archive.h\n+++ b/archive.h\n@@ -43,6 +43,7 @@ extern void register_archiver(struct archiver *);\n \n extern void init_tar_archiver(void);\n extern void init_zip_archiver(void);\n+extern void init_archivers(void);\n \n typedef int (*write_archive_entry_fn_t)(struct archiver_args *args,\n \t\t\t\t\tconst struct object_id *oid,\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex e74f675390..d2455237ce 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -97,6 +97,8 @@ int cmd_archive(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, local_opts, NULL,\n \t\t\t     PARSE_OPT_KEEP_ALL);\n \n+\tinit_archivers();\n+\n \tif (output)\n \t\tcreate_output_file(output);\n \ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex 25d9116356..3f35ebcfe8 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -43,6 +43,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n \t}\n \n \t/* parse all options sent by the client */\n+\tinit_archivers();\n \treturn write_archive(sent_argv.argc, sent_argv.argv, prefix,\n \t\t\t     the_repository, NULL, 1);\n }\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 2a97b27b0a..3e95fdf660 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -206,6 +206,12 @@ test_expect_success 'git archive with --output, override inferred format' '\n \ttest_cmp_bin b.tar d4.zip\n '\n \n+test_expect_success GZIP 'git archive with --output and --remote uses expected format' '\n+\tgit archive --output=d5.tgz --remote=. HEAD &&\n+\tgzip -d -c < d5.tgz > d5.tar &&\n+\ttest_cmp_bin b.tar d5.tar\n+'\n+\n test_expect_success 'git archive --list outside of a git repo' '\n \tnongit git archive --list\n '\n-- \n2.19.1.568.g152ad8e336-goog\n\n"},{"id":"361222","messageId":"20181022223041.GA19019@sigill.intra.peff.net","threadId":"49621","inReplyTo":"20181022214756.GC233961@google.com","subject":"Re: [PATCH 1/1] archive: init archivers before determining format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-22T22:30:41Z","receivedAt":"2018-10-22T22:30:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 22, 2018 at 02:47:56PM -0700, Josh Steadmon wrote:\n\n> > Does this work with configured archiver extensions, too? I think so,\n> > because we load them via init_tar_archiver().\n> \n> If you mean things like .tgz and .tar.gz, then yes, they are affected by\n> the bug as well, and this patch fixes them. The test included in v2 uses\n> a .tgz file.\n\nYes, but also ones that are provided by the user. E.g., does:\n\n  git config tar.foo.command \"foo\"\n  git archive -o out.tar.foo HEAD\n\nwork? (I think the answer is yes, because we read git_tar_config in the\nsame place).\n\n-Peff\n"},{"id":"361224","messageId":"20181022223536.GB19019@sigill.intra.peff.net","threadId":"49621","inReplyTo":"bc6f20274dfe11f1451745e0accb065544cc59ca.1540244445.git.steadmon@google.com","subject":"Re: [PATCH v2] archive: initialize archivers earlier","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-22T22:35:36Z","receivedAt":"2018-10-22T22:35:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 22, 2018 at 02:48:11PM -0700, steadmon@google.com wrote:\n\n> Initialize archivers as soon as possible when running git-archive and\n> git-upload-archive. Various non-obvious behavior depends on having the\n> archivers initialized, such as determining the desired archival format\n> from the provided filename.\n> \n> Since 08716b3c11 (\"archive: refactor file extension format-guessing\",\n> 2011-06-21), archive_format_from_filename() has used the registered\n> archivers to match filenames (provided via --output) to archival\n> formats. However, when git-archive is executed with --remote, format\n> detection happens before the archivers have been registered. This causes\n> archives from remotes to always be generated as TAR files, regardless of\n> the actual filename (unless an explicit --format is provided).\n> \n> This patch fixes that behavior; archival format is determined properly\n> from the output filename, even when --remote is used.\n> \n> Signed-off-by: Josh Steadmon <steadmon@google.com>\n> Helped-by: Jeff King <peff@peff.net>\n\nThanks, this looks good overall.\n\nA few minor comments (that I'm not even sure are worth re-rolling for):\n\n> diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\n> index 25d9116356..3f35ebcfe8 100644\n> --- a/builtin/upload-archive.c\n> +++ b/builtin/upload-archive.c\n> @@ -43,6 +43,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n>  \t}\n>  \n>  \t/* parse all options sent by the client */\n> +\tinit_archivers();\n>  \treturn write_archive(sent_argv.argc, sent_argv.argv, prefix,\n>  \t\t\t     the_repository, NULL, 1);\n>  }\n\nThis seems to separate the comment from what it describes. Any reason\nnot to just init_archivers() closer to the top of the function here\n(probably after the enter_repo() call)?\n\n> diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\n> index 2a97b27b0a..3e95fdf660 100755\n> --- a/t/t5000-tar-tree.sh\n> +++ b/t/t5000-tar-tree.sh\n> @@ -206,6 +206,12 @@ test_expect_success 'git archive with --output, override inferred format' '\n>  \ttest_cmp_bin b.tar d4.zip\n>  '\n>  \n> +test_expect_success GZIP 'git archive with --output and --remote uses expected format' '\n> +\tgit archive --output=d5.tgz --remote=. HEAD &&\n> +\tgzip -d -c < d5.tgz > d5.tar &&\n> +\ttest_cmp_bin b.tar d5.tar\n> +'\n\nThis nicely tests the more-interesting tgz case. But unfortunately it\nwon't run on machines without the GZIP prerequisite. I'd think that\nwould really be _most_ machines, but is it worth having a separate zip\ntest to cover machines without gzip? I guess that just creates the\nopposite problem: not everybody has ZIP.\n\n-Peff\n"},{"id":"361229","messageId":"20181022235127.GE233961@google.com","threadId":"49621","inReplyTo":"20181022223536.GB19019@sigill.intra.peff.net","subject":"Re: [PATCH v2] archive: initialize archivers earlier","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-10-22T23:51:27Z","receivedAt":"2018-10-22T23:51:35Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.10.22 18:35, Jeff King wrote:\n> On Mon, Oct 22, 2018 at 02:48:11PM -0700, steadmon@google.com wrote:\n> \n> > Initialize archivers as soon as possible when running git-archive and\n> > git-upload-archive. Various non-obvious behavior depends on having the\n> > archivers initialized, such as determining the desired archival format\n> > from the provided filename.\n> > \n> > Since 08716b3c11 (\"archive: refactor file extension format-guessing\",\n> > 2011-06-21), archive_format_from_filename() has used the registered\n> > archivers to match filenames (provided via --output) to archival\n> > formats. However, when git-archive is executed with --remote, format\n> > detection happens before the archivers have been registered. This causes\n> > archives from remotes to always be generated as TAR files, regardless of\n> > the actual filename (unless an explicit --format is provided).\n> > \n> > This patch fixes that behavior; archival format is determined properly\n> > from the output filename, even when --remote is used.\n> > \n> > Signed-off-by: Josh Steadmon <steadmon@google.com>\n> > Helped-by: Jeff King <peff@peff.net>\n> \n> Thanks, this looks good overall.\n> \n> A few minor comments (that I'm not even sure are worth re-rolling for):\n> \n> > diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\n> > index 25d9116356..3f35ebcfe8 100644\n> > --- a/builtin/upload-archive.c\n> > +++ b/builtin/upload-archive.c\n> > @@ -43,6 +43,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n> >  \t}\n> >  \n> >  \t/* parse all options sent by the client */\n> > +\tinit_archivers();\n> >  \treturn write_archive(sent_argv.argc, sent_argv.argv, prefix,\n> >  \t\t\t     the_repository, NULL, 1);\n> >  }\n> \n> This seems to separate the comment from what it describes. Any reason\n> not to just init_archivers() closer to the top of the function here\n> (probably after the enter_repo() call)?\n\nAck, fixed.\n\n\n> > diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\n> > index 2a97b27b0a..3e95fdf660 100755\n> > --- a/t/t5000-tar-tree.sh\n> > +++ b/t/t5000-tar-tree.sh\n> > @@ -206,6 +206,12 @@ test_expect_success 'git archive with --output, override inferred format' '\n> >  \ttest_cmp_bin b.tar d4.zip\n> >  '\n> >  \n> > +test_expect_success GZIP 'git archive with --output and --remote uses expected format' '\n> > +\tgit archive --output=d5.tgz --remote=. HEAD &&\n> > +\tgzip -d -c < d5.tgz > d5.tar &&\n> > +\ttest_cmp_bin b.tar d5.tar\n> > +'\n> \n> This nicely tests the more-interesting tgz case. But unfortunately it\n> won't run on machines without the GZIP prerequisite. I'd think that\n> would really be _most_ machines, but is it worth having a separate zip\n> test to cover machines without gzip? I guess that just creates the\n> opposite problem: not everybody has ZIP.\n\nAdded a test to compare the file lists from the .zip file to the\nreference .tar file. I'm not sure if this is the best way to do things,\nbut it at least verifies that a .zip is produced. However, it's brittle\nif the output of \"zip -sf\" changes. Let me know if you have a better\nidea.\n"},{"id":"361230","messageId":"39a4e7bf8f3ebc5803393f357d1ee7dc9806252f.1540251936.git.steadmon@google.com","threadId":"49621","inReplyTo":"bc6f20274dfe11f1451745e0accb065544cc59ca.1540244445.git.steadmon@google.com","subject":"[PATCH v3] archive: initialize archivers earlier","fromName":"","fromEmail":"steadmon@google.com","sentAt":"2018-10-22T23:54:47Z","receivedAt":"2018-10-22T23:54:51Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Initialize archivers as soon as possible when running git-archive.\nVarious non-obvious behavior depends on having the archivers\ninitialized, such as determining the desired archival format from the\nprovided filename.\n\nSince 08716b3c11 (\"archive: refactor file extension format-guessing\",\n2011-06-21), archive_format_from_filename() has used the registered\narchivers to match filenames (provided via --output) to archival\nformats. However, when git-archive is executed with --remote, format\ndetection happens before the archivers have been registered. This causes\narchives from remotes to always be generated as TAR files, regardless of\nthe actual filename (unless an explicit --format is provided).\n\nThis patch fixes that behavior; archival format is determined properly\nfrom the output filename, even when --remote is used.\n\nSigned-off-by: Josh Steadmon <steadmon@google.com>\nHelped-by: Jeff King <peff@peff.net>\n---\nRange-diff against v2:\n1:  bc6f20274d ! 1:  39a4e7bf8f archive: initialize archivers earlier\n    @@ -78,26 +78,43 @@\n      --- a/builtin/upload-archive.c\n      +++ b/builtin/upload-archive.c\n     @@\n    - \t}\n    + \tif (!enter_repo(argv[1], 0))\n    + \t\tdie(\"'%s' does not appear to be a git repository\", argv[1]);\n      \n    - \t/* parse all options sent by the client */\n     +\tinit_archivers();\n    - \treturn write_archive(sent_argv.argc, sent_argv.argv, prefix,\n    - \t\t\t     the_repository, NULL, 1);\n    - }\n    ++\n    + \t/* put received options in sent_argv[] */\n    + \targv_array_push(&sent_argv, \"git-upload-archive\");\n    + \tfor (;;) {\n     \n      diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\n      --- a/t/t5000-tar-tree.sh\n      +++ b/t/t5000-tar-tree.sh\n     @@\n    + \n    + test_lazy_prereq GZIP 'gzip --version'\n    + \n    ++test_lazy_prereq ZIP 'zip --version'\n    ++\n    + get_pax_header() {\n    + \tfile=$1\n    + \theader=$2=\n    +@@\n      \ttest_cmp_bin b.tar d4.zip\n      '\n      \n    -+test_expect_success GZIP 'git archive with --output and --remote uses expected format' '\n    ++test_expect_success GZIP 'git archive with --output and --remote creates .tgz' '\n     +\tgit archive --output=d5.tgz --remote=. HEAD &&\n     +\tgzip -d -c < d5.tgz > d5.tar &&\n     +\ttest_cmp_bin b.tar d5.tar\n     +'\n    ++\n    ++test_expect_success ZIP 'git archive with --output and --remote creates .zip' '\n    ++\tgit archive --output=d6.zip --remote=. HEAD &&\n    ++\tzip -sf d6.zip | sed \"/^[^ ]/d\" | sed \"s/^  //\" | sort > zip_manifest &&\n    ++\t\"$TAR\" tf b.tar | sort > tar_manifest &&\n    ++\ttest_cmp zip_manifest tar_manifest\n    ++'\n     +\n      test_expect_success 'git archive --list outside of a git repo' '\n      \tnongit git archive --list\n\n archive.c                |  9 ++++++---\n archive.h                |  1 +\n builtin/archive.c        |  2 ++\n builtin/upload-archive.c |  2 ++\n t/t5000-tar-tree.sh      | 15 +++++++++++++++\n 5 files changed, 26 insertions(+), 3 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex c1870105eb..ce0f8a0362 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -29,6 +29,12 @@ void register_archiver(struct archiver *ar)\n \tarchivers[nr_archivers++] = ar;\n }\n \n+void init_archivers(void)\n+{\n+\tinit_tar_archiver();\n+\tinit_zip_archiver();\n+}\n+\n static void format_subst(const struct commit *commit,\n                          const char *src, size_t len,\n                          struct strbuf *buf)\n@@ -531,9 +537,6 @@ int write_archive(int argc, const char **argv, const char *prefix,\n \tgit_config_get_bool(\"uploadarchive.allowunreachable\", &remote_allow_unreachable);\n \tgit_config(git_default_config, NULL);\n \n-\tinit_tar_archiver();\n-\tinit_zip_archiver();\n-\n \targs.repo = repo;\n \targc = parse_archive_args(argc, argv, &ar, &args, name_hint, remote);\n \tif (!startup_info->have_repository) {\ndiff --git a/archive.h b/archive.h\nindex d4f97a00f5..21ac010699 100644\n--- a/archive.h\n+++ b/archive.h\n@@ -43,6 +43,7 @@ extern void register_archiver(struct archiver *);\n \n extern void init_tar_archiver(void);\n extern void init_zip_archiver(void);\n+extern void init_archivers(void);\n \n typedef int (*write_archive_entry_fn_t)(struct archiver_args *args,\n \t\t\t\t\tconst struct object_id *oid,\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex e74f675390..d2455237ce 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -97,6 +97,8 @@ int cmd_archive(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, local_opts, NULL,\n \t\t\t     PARSE_OPT_KEEP_ALL);\n \n+\tinit_archivers();\n+\n \tif (output)\n \t\tcreate_output_file(output);\n \ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex 25d9116356..018879737a 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -28,6 +28,8 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n \tif (!enter_repo(argv[1], 0))\n \t\tdie(\"'%s' does not appear to be a git repository\", argv[1]);\n \n+\tinit_archivers();\n+\n \t/* put received options in sent_argv[] */\n \targv_array_push(&sent_argv, \"git-upload-archive\");\n \tfor (;;) {\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 2a97b27b0a..cfd5ca492f 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -39,6 +39,8 @@ test_lazy_prereq TAR_NEEDS_PAX_FALLBACK '\n \n test_lazy_prereq GZIP 'gzip --version'\n \n+test_lazy_prereq ZIP 'zip --version'\n+\n get_pax_header() {\n \tfile=$1\n \theader=$2=\n@@ -206,6 +208,19 @@ test_expect_success 'git archive with --output, override inferred format' '\n \ttest_cmp_bin b.tar d4.zip\n '\n \n+test_expect_success GZIP 'git archive with --output and --remote creates .tgz' '\n+\tgit archive --output=d5.tgz --remote=. HEAD &&\n+\tgzip -d -c < d5.tgz > d5.tar &&\n+\ttest_cmp_bin b.tar d5.tar\n+'\n+\n+test_expect_success ZIP 'git archive with --output and --remote creates .zip' '\n+\tgit archive --output=d6.zip --remote=. HEAD &&\n+\tzip -sf d6.zip | sed \"/^[^ ]/d\" | sed \"s/^  //\" | sort > zip_manifest &&\n+\t\"$TAR\" tf b.tar | sort > tar_manifest &&\n+\ttest_cmp zip_manifest tar_manifest\n+'\n+\n test_expect_success 'git archive --list outside of a git repo' '\n \tnongit git archive --list\n '\n-- \n2.19.1.568.g152ad8e336-goog\n\n"},{"id":"361231","messageId":"20181023000616.GA27393@sigill.intra.peff.net","threadId":"49621","inReplyTo":"20181022235127.GE233961@google.com","subject":"Re: [PATCH v2] archive: initialize archivers earlier","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-23T00:06:16Z","receivedAt":"2018-10-23T00:06:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 22, 2018 at 04:51:27PM -0700, Josh Steadmon wrote:\n\n> > > +test_expect_success GZIP 'git archive with --output and --remote uses expected format' '\n> > > +\tgit archive --output=d5.tgz --remote=. HEAD &&\n> > > +\tgzip -d -c < d5.tgz > d5.tar &&\n> > > +\ttest_cmp_bin b.tar d5.tar\n> > > +'\n> > \n> > This nicely tests the more-interesting tgz case. But unfortunately it\n> > won't run on machines without the GZIP prerequisite. I'd think that\n> > would really be _most_ machines, but is it worth having a separate zip\n> > test to cover machines without gzip? I guess that just creates the\n> > opposite problem: not everybody has ZIP.\n> \n> Added a test to compare the file lists from the .zip file to the\n> reference .tar file. I'm not sure if this is the best way to do things,\n> but it at least verifies that a .zip is produced. However, it's brittle\n> if the output of \"zip -sf\" changes. Let me know if you have a better\n> idea.\n\nI wonder if we could do something more black-box. What we really care\nabout here is not the exact output, but rather that \"-o foo.zip\"\nproduces the same output as \"--format zip\". Could we do that without\neven relying on ZIP?\n\nI think it should follow even for tgz, because we use \"-n\" for a\nrepeatable output. But there we are relying on an external gzip just to\n_create_ the file, so we'd still need the GZIP prereq.\n\nHmm. Looks like we already have a similar test in t5003. So maybe just:\n\ndiff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh\nindex 55c7870997..cf19f56924 100755\n--- a/t/t5003-archive-zip.sh\n+++ b/t/t5003-archive-zip.sh\n@@ -158,11 +158,16 @@ test_expect_success 'git archive --format=zip with --output' \\\n     'git archive --format=zip --output=d2.zip HEAD &&\n     test_cmp_bin d.zip d2.zip'\n \n-test_expect_success 'git archive with --output, inferring format' '\n+test_expect_success 'git archive with --output, inferring format (local)' '\n \tgit archive --output=d3.zip HEAD &&\n \ttest_cmp_bin d.zip d3.zip\n '\n \n+test_expect_success 'git archive with --output, ferring format (remote)' '\n+\tgit archive --remote=. --output=d4.zip HEAD &&\n+\ttest_cmp_bin d.zip d4.zip\n+'\n+\n test_expect_success \\\n     'git archive --format=zip with prefix' \\\n     'git archive --format=zip --prefix=prefix/ HEAD >e.zip'\n\nwhich I think exposes the bug and can run everywhere?\n\n-Peff\n"},{"id":"361232","messageId":"1d7b07092872e4735dbdedfbfdd86a1c9e71290a.1540253913.git.steadmon@google.com","threadId":"49621","inReplyTo":"39a4e7bf8f3ebc5803393f357d1ee7dc9806252f.1540251936.git.steadmon@google.com","subject":"[PATCH v4] archive: initialize archivers earlier","fromName":"","fromEmail":"steadmon@google.com","sentAt":"2018-10-23T00:20:14Z","receivedAt":"2018-10-23T00:20:18Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Initialize archivers as soon as possible when running git-archive.\nVarious non-obvious behavior depends on having the archivers\ninitialized, such as determining the desired archival format from the\nprovided filename.\n\nSince 08716b3c11 (\"archive: refactor file extension format-guessing\",\n2011-06-21), archive_format_from_filename() has used the registered\narchivers to match filenames (provided via --output) to archival\nformats. However, when git-archive is executed with --remote, format\ndetection happens before the archivers have been registered. This causes\narchives from remotes to always be generated as TAR files, regardless of\nthe actual filename (unless an explicit --format is provided).\n\nThis patch fixes that behavior; archival format is determined properly\nfrom the output filename, even when --remote is used.\n\nSigned-off-by: Josh Steadmon <steadmon@google.com>\nHelped-by: Jeff King <peff@peff.net>\n---\nRange-diff against v3:\n1:  39a4e7bf8f ! 1:  1d7b070928 archive: initialize archivers earlier\n    @@ -91,15 +91,6 @@\n      --- a/t/t5000-tar-tree.sh\n      +++ b/t/t5000-tar-tree.sh\n     @@\n    - \n    - test_lazy_prereq GZIP 'gzip --version'\n    - \n    -+test_lazy_prereq ZIP 'zip --version'\n    -+\n    - get_pax_header() {\n    - \tfile=$1\n    - \theader=$2=\n    -@@\n      \ttest_cmp_bin b.tar d4.zip\n      '\n      \n    @@ -108,14 +99,29 @@\n     +\tgzip -d -c < d5.tgz > d5.tar &&\n     +\ttest_cmp_bin b.tar d5.tar\n     +'\n    -+\n    -+test_expect_success ZIP 'git archive with --output and --remote creates .zip' '\n    -+\tgit archive --output=d6.zip --remote=. HEAD &&\n    -+\tzip -sf d6.zip | sed \"/^[^ ]/d\" | sed \"s/^  //\" | sort > zip_manifest &&\n    -+\t\"$TAR\" tf b.tar | sort > tar_manifest &&\n    -+\ttest_cmp zip_manifest tar_manifest\n    -+'\n     +\n      test_expect_success 'git archive --list outside of a git repo' '\n      \tnongit git archive --list\n      '\n    +\n    + diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh\n    + --- a/t/t5003-archive-zip.sh\n    + +++ b/t/t5003-archive-zip.sh\n    +@@\n    +     'git archive --format=zip --output=d2.zip HEAD &&\n    +     test_cmp_bin d.zip d2.zip'\n    + \n    +-test_expect_success 'git archive with --output, inferring format' '\n    ++test_expect_success 'git archive with --output, inferring format (local)' '\n    + \tgit archive --output=d3.zip HEAD &&\n    + \ttest_cmp_bin d.zip d3.zip\n    + '\n    + \n    ++test_expect_success 'git archive with --output, inferring format (remote)' '\n    ++\tgit archive --remote=. --output=d4.zip HEAD &&\n    ++\ttest_cmp_bin d.zip d4.zip\n    ++'\n    ++\n    + test_expect_success \\\n    +     'git archive --format=zip with prefix' \\\n    +     'git archive --format=zip --prefix=prefix/ HEAD >e.zip'\n\n archive.c                | 9 ++++++---\n archive.h                | 1 +\n builtin/archive.c        | 2 ++\n builtin/upload-archive.c | 2 ++\n t/t5000-tar-tree.sh      | 6 ++++++\n t/t5003-archive-zip.sh   | 7 ++++++-\n 6 files changed, 23 insertions(+), 4 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex c1870105eb..ce0f8a0362 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -29,6 +29,12 @@ void register_archiver(struct archiver *ar)\n \tarchivers[nr_archivers++] = ar;\n }\n \n+void init_archivers(void)\n+{\n+\tinit_tar_archiver();\n+\tinit_zip_archiver();\n+}\n+\n static void format_subst(const struct commit *commit,\n                          const char *src, size_t len,\n                          struct strbuf *buf)\n@@ -531,9 +537,6 @@ int write_archive(int argc, const char **argv, const char *prefix,\n \tgit_config_get_bool(\"uploadarchive.allowunreachable\", &remote_allow_unreachable);\n \tgit_config(git_default_config, NULL);\n \n-\tinit_tar_archiver();\n-\tinit_zip_archiver();\n-\n \targs.repo = repo;\n \targc = parse_archive_args(argc, argv, &ar, &args, name_hint, remote);\n \tif (!startup_info->have_repository) {\ndiff --git a/archive.h b/archive.h\nindex d4f97a00f5..21ac010699 100644\n--- a/archive.h\n+++ b/archive.h\n@@ -43,6 +43,7 @@ extern void register_archiver(struct archiver *);\n \n extern void init_tar_archiver(void);\n extern void init_zip_archiver(void);\n+extern void init_archivers(void);\n \n typedef int (*write_archive_entry_fn_t)(struct archiver_args *args,\n \t\t\t\t\tconst struct object_id *oid,\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex e74f675390..d2455237ce 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -97,6 +97,8 @@ int cmd_archive(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, local_opts, NULL,\n \t\t\t     PARSE_OPT_KEEP_ALL);\n \n+\tinit_archivers();\n+\n \tif (output)\n \t\tcreate_output_file(output);\n \ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex 25d9116356..018879737a 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -28,6 +28,8 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n \tif (!enter_repo(argv[1], 0))\n \t\tdie(\"'%s' does not appear to be a git repository\", argv[1]);\n \n+\tinit_archivers();\n+\n \t/* put received options in sent_argv[] */\n \targv_array_push(&sent_argv, \"git-upload-archive\");\n \tfor (;;) {\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 2a97b27b0a..e2ee920b06 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -206,6 +206,12 @@ test_expect_success 'git archive with --output, override inferred format' '\n \ttest_cmp_bin b.tar d4.zip\n '\n \n+test_expect_success GZIP 'git archive with --output and --remote creates .tgz' '\n+\tgit archive --output=d5.tgz --remote=. HEAD &&\n+\tgzip -d -c < d5.tgz > d5.tar &&\n+\ttest_cmp_bin b.tar d5.tar\n+'\n+\n test_expect_success 'git archive --list outside of a git repo' '\n \tnongit git archive --list\n '\ndiff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh\nindex 55c7870997..106eddbd85 100755\n--- a/t/t5003-archive-zip.sh\n+++ b/t/t5003-archive-zip.sh\n@@ -158,11 +158,16 @@ test_expect_success 'git archive --format=zip with --output' \\\n     'git archive --format=zip --output=d2.zip HEAD &&\n     test_cmp_bin d.zip d2.zip'\n \n-test_expect_success 'git archive with --output, inferring format' '\n+test_expect_success 'git archive with --output, inferring format (local)' '\n \tgit archive --output=d3.zip HEAD &&\n \ttest_cmp_bin d.zip d3.zip\n '\n \n+test_expect_success 'git archive with --output, inferring format (remote)' '\n+\tgit archive --remote=. --output=d4.zip HEAD &&\n+\ttest_cmp_bin d.zip d4.zip\n+'\n+\n test_expect_success \\\n     'git archive --format=zip with prefix' \\\n     'git archive --format=zip --prefix=prefix/ HEAD >e.zip'\n-- \n2.19.1.568.g152ad8e336-goog\n\n"},{"id":"361233","messageId":"20181023002313.GF233961@google.com","threadId":"49621","inReplyTo":"20181023000616.GA27393@sigill.intra.peff.net","subject":"Re: [PATCH v2] archive: initialize archivers earlier","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-10-23T00:23:13Z","receivedAt":"2018-10-23T00:23:23Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.10.22 20:06, Jeff King wrote:\n> On Mon, Oct 22, 2018 at 04:51:27PM -0700, Josh Steadmon wrote:\n> \n> > > > +test_expect_success GZIP 'git archive with --output and --remote uses expected format' '\n> > > > +\tgit archive --output=d5.tgz --remote=. HEAD &&\n> > > > +\tgzip -d -c < d5.tgz > d5.tar &&\n> > > > +\ttest_cmp_bin b.tar d5.tar\n> > > > +'\n> > > \n> > > This nicely tests the more-interesting tgz case. But unfortunately it\n> > > won't run on machines without the GZIP prerequisite. I'd think that\n> > > would really be _most_ machines, but is it worth having a separate zip\n> > > test to cover machines without gzip? I guess that just creates the\n> > > opposite problem: not everybody has ZIP.\n> > \n> > Added a test to compare the file lists from the .zip file to the\n> > reference .tar file. I'm not sure if this is the best way to do things,\n> > but it at least verifies that a .zip is produced. However, it's brittle\n> > if the output of \"zip -sf\" changes. Let me know if you have a better\n> > idea.\n> \n> I wonder if we could do something more black-box. What we really care\n> about here is not the exact output, but rather that \"-o foo.zip\"\n> produces the same output as \"--format zip\". Could we do that without\n> even relying on ZIP?\n> \n> I think it should follow even for tgz, because we use \"-n\" for a\n> repeatable output. But there we are relying on an external gzip just to\n> _create_ the file, so we'd still need the GZIP prereq.\n> \n> Hmm. Looks like we already have a similar test in t5003. So maybe just:\n> \n> diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh\n> index 55c7870997..cf19f56924 100755\n> --- a/t/t5003-archive-zip.sh\n> +++ b/t/t5003-archive-zip.sh\n> @@ -158,11 +158,16 @@ test_expect_success 'git archive --format=zip with --output' \\\n>      'git archive --format=zip --output=d2.zip HEAD &&\n>      test_cmp_bin d.zip d2.zip'\n>  \n> -test_expect_success 'git archive with --output, inferring format' '\n> +test_expect_success 'git archive with --output, inferring format (local)' '\n>  \tgit archive --output=d3.zip HEAD &&\n>  \ttest_cmp_bin d.zip d3.zip\n>  '\n>  \n> +test_expect_success 'git archive with --output, ferring format (remote)' '\n> +\tgit archive --remote=. --output=d4.zip HEAD &&\n> +\ttest_cmp_bin d.zip d4.zip\n> +'\n> +\n>  test_expect_success \\\n>      'git archive --format=zip with prefix' \\\n>      'git archive --format=zip --prefix=prefix/ HEAD >e.zip'\n> \n> which I think exposes the bug and can run everywhere?\n\nMakes sense, thanks!\n"},{"id":"361255","messageId":"xmqq4lddtibc.fsf@gitster-ct.c.googlers.com","threadId":"49621","inReplyTo":"39a4e7bf8f3ebc5803393f357d1ee7dc9806252f.1540251936.git.steadmon@google.com","subject":"Re: [PATCH v3] archive: initialize archivers earlier","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-23T04:09:43Z","receivedAt":"2018-10-23T04:09:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"steadmon@google.com writes:\n\n> diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\n> index 2a97b27b0a..cfd5ca492f 100755\n> --- a/t/t5000-tar-tree.sh\n> +++ b/t/t5000-tar-tree.sh\n> @@ -39,6 +39,8 @@ test_lazy_prereq TAR_NEEDS_PAX_FALLBACK '\n>  \n>  test_lazy_prereq GZIP 'gzip --version'\n>  \n> +test_lazy_prereq ZIP 'zip --version'\n> +\n\nThere are a handful of zip implementations; Info-ZIP found on many\nLinux distros does support 'zip --version', but we may want to make\nsure this test covers different implementations of zip sufficiently.\n\nQueuing this patch (or an update of it) on 'pu' and hoping those\nwith zip from different origins to try it would not help very much,\neither, as zip implementations that do not react to \"zip --version\"\nwould silently turn the prereq off without breaking anything.\n\nIn any case, please refrain from adding any ZIP prerequiste to t5000\nwhich is about tar; t5003-archive-zip may be a much better fit.  It\nhas an already working machinery that validates the generated zip\narchive under UNZIP prerequisite, so we may not even have to invent\nour own ZIP prereq if we did so.\n\n> @@ -206,6 +208,19 @@ test_expect_success 'git archive with --output, override inferred format' '\n>  \ttest_cmp_bin b.tar d4.zip\n>  '\n>  \n> +test_expect_success GZIP 'git archive with --output and --remote creates .tgz' '\n> +\tgit archive --output=d5.tgz --remote=. HEAD &&\n> +\tgzip -d -c < d5.tgz > d5.tar &&\n> +\ttest_cmp_bin b.tar d5.tar\n> +'\n\nWe try to write redirections without SP between redirection operator\nand target filename, i.e. \"gzip -d -c <d5.tgz >d5.tar\".\n\nThanks.\n"},{"id":"361555","messageId":"20181025202915.GH233961@google.com","threadId":"49621","inReplyTo":"xmqq4lddtibc.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3] archive: initialize archivers earlier","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-10-25T20:29:15Z","receivedAt":"2018-10-25T20:29:24Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.10.23 13:09, Junio C Hamano wrote:\n> steadmon@google.com writes:\n> \n> > diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\n> > index 2a97b27b0a..cfd5ca492f 100755\n> > --- a/t/t5000-tar-tree.sh\n> > +++ b/t/t5000-tar-tree.sh\n> > @@ -39,6 +39,8 @@ test_lazy_prereq TAR_NEEDS_PAX_FALLBACK '\n> >  \n> >  test_lazy_prereq GZIP 'gzip --version'\n> >  \n> > +test_lazy_prereq ZIP 'zip --version'\n> > +\n> \n> There are a handful of zip implementations; Info-ZIP found on many\n> Linux distros does support 'zip --version', but we may want to make\n> sure this test covers different implementations of zip sufficiently.\n> \n> Queuing this patch (or an update of it) on 'pu' and hoping those\n> with zip from different origins to try it would not help very much,\n> either, as zip implementations that do not react to \"zip --version\"\n> would silently turn the prereq off without breaking anything.\n> \n> In any case, please refrain from adding any ZIP prerequiste to t5000\n> which is about tar; t5003-archive-zip may be a much better fit.  It\n> has an already working machinery that validates the generated zip\n> archive under UNZIP prerequisite, so we may not even have to invent\n> our own ZIP prereq if we did so.\n\nAck. This has been removed in v4. V4 also has a test case in t5003 based\non Jeff's advice.\n\n> > @@ -206,6 +208,19 @@ test_expect_success 'git archive with --output, override inferred format' '\n> >  \ttest_cmp_bin b.tar d4.zip\n> >  '\n> >  \n> > +test_expect_success GZIP 'git archive with --output and --remote creates .tgz' '\n> > +\tgit archive --output=d5.tgz --remote=. HEAD &&\n> > +\tgzip -d -c < d5.tgz > d5.tar &&\n> > +\ttest_cmp_bin b.tar d5.tar\n> > +'\n> \n> We try to write redirections without SP between redirection operator\n> and target filename, i.e. \"gzip -d -c <d5.tgz >d5.tar\".\n\nFixed in v5.\n"},{"id":"361556","messageId":"c85673cee76c1c4a7e2b2a489d6139f5c89ba88c.1540499413.git.steadmon@google.com","threadId":"49621","inReplyTo":"39a4e7bf8f3ebc5803393f357d1ee7dc9806252f.1540251936.git.steadmon@google.com","subject":"[PATCH v5] archive: initialize archivers earlier","fromName":"","fromEmail":"steadmon@google.com","sentAt":"2018-10-25T20:32:14Z","receivedAt":"2018-10-25T20:32:20Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Initialize archivers as soon as possible when running git-archive.\nVarious non-obvious behavior depends on having the archivers\ninitialized, such as determining the desired archival format from the\nprovided filename.\n\nSince 08716b3c11 (\"archive: refactor file extension format-guessing\",\n2011-06-21), archive_format_from_filename() has used the registered\narchivers to match filenames (provided via --output) to archival\nformats. However, when git-archive is executed with --remote, format\ndetection happens before the archivers have been registered. This causes\narchives from remotes to always be generated as TAR files, regardless of\nthe actual filename (unless an explicit --format is provided).\n\nThis patch fixes that behavior; archival format is determined properly\nfrom the output filename, even when --remote is used.\n\nSigned-off-by: Josh Steadmon <steadmon@google.com>\nHelped-by: Jeff King <peff@peff.net>\n---\nRange-diff against v4:\n1:  1d7b070928 ! 1:  c85673cee7 archive: initialize archivers earlier\n    @@ -96,7 +96,7 @@\n      \n     +test_expect_success GZIP 'git archive with --output and --remote creates .tgz' '\n     +\tgit archive --output=d5.tgz --remote=. HEAD &&\n    -+\tgzip -d -c < d5.tgz > d5.tar &&\n    ++\tgzip -d -c <d5.tgz >d5.tar &&\n     +\ttest_cmp_bin b.tar d5.tar\n     +'\n     +\n\n archive.c                | 9 ++++++---\n archive.h                | 1 +\n builtin/archive.c        | 2 ++\n builtin/upload-archive.c | 2 ++\n t/t5000-tar-tree.sh      | 6 ++++++\n t/t5003-archive-zip.sh   | 7 ++++++-\n 6 files changed, 23 insertions(+), 4 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex c1870105eb..ce0f8a0362 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -29,6 +29,12 @@ void register_archiver(struct archiver *ar)\n \tarchivers[nr_archivers++] = ar;\n }\n \n+void init_archivers(void)\n+{\n+\tinit_tar_archiver();\n+\tinit_zip_archiver();\n+}\n+\n static void format_subst(const struct commit *commit,\n                          const char *src, size_t len,\n                          struct strbuf *buf)\n@@ -531,9 +537,6 @@ int write_archive(int argc, const char **argv, const char *prefix,\n \tgit_config_get_bool(\"uploadarchive.allowunreachable\", &remote_allow_unreachable);\n \tgit_config(git_default_config, NULL);\n \n-\tinit_tar_archiver();\n-\tinit_zip_archiver();\n-\n \targs.repo = repo;\n \targc = parse_archive_args(argc, argv, &ar, &args, name_hint, remote);\n \tif (!startup_info->have_repository) {\ndiff --git a/archive.h b/archive.h\nindex d4f97a00f5..21ac010699 100644\n--- a/archive.h\n+++ b/archive.h\n@@ -43,6 +43,7 @@ extern void register_archiver(struct archiver *);\n \n extern void init_tar_archiver(void);\n extern void init_zip_archiver(void);\n+extern void init_archivers(void);\n \n typedef int (*write_archive_entry_fn_t)(struct archiver_args *args,\n \t\t\t\t\tconst struct object_id *oid,\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex e74f675390..d2455237ce 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -97,6 +97,8 @@ int cmd_archive(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, local_opts, NULL,\n \t\t\t     PARSE_OPT_KEEP_ALL);\n \n+\tinit_archivers();\n+\n \tif (output)\n \t\tcreate_output_file(output);\n \ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex 25d9116356..018879737a 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -28,6 +28,8 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n \tif (!enter_repo(argv[1], 0))\n \t\tdie(\"'%s' does not appear to be a git repository\", argv[1]);\n \n+\tinit_archivers();\n+\n \t/* put received options in sent_argv[] */\n \targv_array_push(&sent_argv, \"git-upload-archive\");\n \tfor (;;) {\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 2a97b27b0a..602bfd9574 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -206,6 +206,12 @@ test_expect_success 'git archive with --output, override inferred format' '\n \ttest_cmp_bin b.tar d4.zip\n '\n \n+test_expect_success GZIP 'git archive with --output and --remote creates .tgz' '\n+\tgit archive --output=d5.tgz --remote=. HEAD &&\n+\tgzip -d -c <d5.tgz >d5.tar &&\n+\ttest_cmp_bin b.tar d5.tar\n+'\n+\n test_expect_success 'git archive --list outside of a git repo' '\n \tnongit git archive --list\n '\ndiff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh\nindex 55c7870997..106eddbd85 100755\n--- a/t/t5003-archive-zip.sh\n+++ b/t/t5003-archive-zip.sh\n@@ -158,11 +158,16 @@ test_expect_success 'git archive --format=zip with --output' \\\n     'git archive --format=zip --output=d2.zip HEAD &&\n     test_cmp_bin d.zip d2.zip'\n \n-test_expect_success 'git archive with --output, inferring format' '\n+test_expect_success 'git archive with --output, inferring format (local)' '\n \tgit archive --output=d3.zip HEAD &&\n \ttest_cmp_bin d.zip d3.zip\n '\n \n+test_expect_success 'git archive with --output, inferring format (remote)' '\n+\tgit archive --remote=. --output=d4.zip HEAD &&\n+\ttest_cmp_bin d.zip d4.zip\n+'\n+\n test_expect_success \\\n     'git archive --format=zip with prefix' \\\n     'git archive --format=zip --prefix=prefix/ HEAD >e.zip'\n-- \n2.19.1.568.g152ad8e336-goog\n\n"},{"id":"361562","messageId":"20181025211203.GA23050@sigill.intra.peff.net","threadId":"49621","inReplyTo":"c85673cee76c1c4a7e2b2a489d6139f5c89ba88c.1540499413.git.steadmon@google.com","subject":"Re: [PATCH v5] archive: initialize archivers earlier","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-25T21:12:04Z","receivedAt":"2018-10-25T21:12:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 25, 2018 at 01:32:14PM -0700, steadmon@google.com wrote:\n\n> Initialize archivers as soon as possible when running git-archive.\n> Various non-obvious behavior depends on having the archivers\n> initialized, such as determining the desired archival format from the\n> provided filename.\n> \n> Since 08716b3c11 (\"archive: refactor file extension format-guessing\",\n> 2011-06-21), archive_format_from_filename() has used the registered\n> archivers to match filenames (provided via --output) to archival\n> formats. However, when git-archive is executed with --remote, format\n> detection happens before the archivers have been registered. This causes\n> archives from remotes to always be generated as TAR files, regardless of\n> the actual filename (unless an explicit --format is provided).\n> \n> This patch fixes that behavior; archival format is determined properly\n> from the output filename, even when --remote is used.\n\nThanks, this version looks great to me!\n\n-Peff\n"},{"id":"361585","messageId":"xmqq5zxpfr06.fsf@gitster-ct.c.googlers.com","threadId":"49621","inReplyTo":"20181025211203.GA23050@sigill.intra.peff.net","subject":"Re: [PATCH v5] archive: initialize archivers earlier","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-26T01:14:49Z","receivedAt":"2018-10-26T01:17:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Oct 25, 2018 at 01:32:14PM -0700, steadmon@google.com wrote:\n>\n>> Initialize archivers as soon as possible when running git-archive.\n>> Various non-obvious behavior depends on having the archivers\n>> initialized, such as determining the desired archival format from the\n>> provided filename.\n>> \n>> Since 08716b3c11 (\"archive: refactor file extension format-guessing\",\n>> 2011-06-21), archive_format_from_filename() has used the registered\n>> archivers to match filenames (provided via --output) to archival\n>> formats. However, when git-archive is executed with --remote, format\n>> detection happens before the archivers have been registered. This causes\n>> archives from remotes to always be generated as TAR files, regardless of\n>> the actual filename (unless an explicit --format is provided).\n>> \n>> This patch fixes that behavior; archival format is determined properly\n>> from the output filename, even when --remote is used.\n>\n> Thanks, this version looks great to me!\n\nTo me too.  Thanks, both.\n"}]}