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

[Jeff King <peff@peff.net>]

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.

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