Jeff King <p...@peff.net> writes:
> On Fri, Oct 19, 2018 at 04:19:28PM -0700, stead...@google.com wrote:
>
>> diff --git a/builtin/archive.c b/builtin/archive.c
>> index e74f675390..dd3283a247 100644
>> --- a/builtin/archive.c
>> +++ b/builtin/archive.c
>> @@ -45,7 +45,10 @@ static int run_remote_archiver(int argc, const char
>> **argv,
>> * it.
>> */
>> if (name_hint) {
>> - const char *format = archive_format_from_filename(name_hint);
>> + const char *format;
>> + init_tar_archiver();
>> + init_zip_archiver();
>> + format = archive_format_from_filename(name_hint);
>> if (format)
>> packet_write_fmt(fd[1], "argument --format=%s\n",
>> format);
>
> Hrm. This code was added back in 56baa61d01 (archive: move file
> extension format-guessing lower, 2011-06-21), and your example
> invocation worked back then!
>
> Unfortunately it was broken by the very next patch in the series,
> 08716b3c11 (archive: refactor file extension format-guessing,
> 2011-06-21). I guess that's what I get for not adding regression tests.
>
> It's probably worth mentioning those points in the commit message.
>
> Does this work with configured archiver extensions, too? I think so,
> because we load them via init_tar_archiver().
>
> Can we avoid repeating the list of archivers here? This needs to stay in
> sync with the list in write_archive(). I know there are only two, but
> can we factor out an init_archivers() call or something?
>
> We also should probably just call it unconditionally when we start the
> archiver command (I don't think there are any other bugs like this
> lurking, but it doesn't cost very much to initialize these; it makes
> sense to just do it early).
>
> Other than those minor points (and the lack of test), your fix looks
> good to me.
Thanks for a patch and an excellent review. Looking forward to the
finalized version.
Thanks, both.
