On 2018.10.19 19:59, Jeff King wrote:
> On Fri, Oct 19, 2018 at 04:19:28PM -0700, steadmon@xxxxxxxxxx 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.
Done.
> Does this work with configured archiver extensions, too? I think so,
> because we load them via init_tar_archiver().
If you mean things like .tgz and .tar.gz, then yes, they are affected by
the bug as well, and this patch fixes them. The test included in v2 uses
a .tgz file.
> 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?
Done.
> 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).
Done.
> Other than those minor points (and the lack of test), your fix looks
> good to me.
Thanks for the review!
