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

From: Jeff King <peff@peff.net>
Date: Oct 19, 2018, 23:59 UTC
Message-ID: <20181019235945.GB7326@sigill.intra.peff.net>
In-Reply-To: <b1f8e288dde9a9dd46386524189da66c7ad2c333.1539990488.git.steadmon@google.com>

On Fri, Oct 19, 2018 at 04:19:28PM -0700, steadmon@google.com wrote:

Show 15 quoted lines

> 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.

-Peff