Re: [PATCH 1/1] archive: init archivers before determining format

From: Josh Steadmon <steadmon@google.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 1/1] archive: init archivers before determining format
Date: Mon, 22 Oct 2018 14:47:56 -0700	[thread overview]
Message-ID: <20181022214756.GC233961@google.com> (raw)
In-Reply-To: <20181019235945.GB7326@sigill.intra.peff.net>

On 2018.10.19 19:59, Jeff King wrote:
> On Fri, Oct 19, 2018 at 04:19:28PM -0700, steadmon@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.

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!