Re: [PATCH 2/7] archive: add user-configurable tar-filter infrastructure

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 15, 2011, 23:33 UTC
Message-ID: <7vr56uisaa.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20110615223301.GB16807@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 11 quoted lines

> Archive supports two output formats: tar and zip. The tar
> ...
> +static struct tar_filter *tar_filter_by_namelen(const char *name,
> +						int len)
> +{
> +	struct tar_filter *p;
> +	for (p = tar_filters; p; p = p->next)
> +		if (!strncmp(p->name, name, len) && !p->name[len])
> +			return p;
> +	return NULL;
> +}

Makes me wonder if we want to have a generic table that is keyed by name whose contents can be looked up by counted string. string_list is the closest thing we already have, but I do not think it has counted string interface (shouldn't be a rocket surgery to add it, though).

Show 7 quoted lines

> +static int tar_filter_config(const char *var, const char *value, void *data)
> +{
> ...
> +	if (!strcmp(type, "command")) {
> +		if (!value)
> +			return config_error_nonbool(var);
> +		tf->command = xstrdup(value);

Does this result in small leak if the same filter is multiply defined, say in /etc/gitconfig and then in ~/.gitconfig?

Show 22 quoted lines

> diff --git a/archive.h b/archive.h
> index 038ac35..8386c46 100644
> --- a/archive.h
> +++ b/archive.h
> @@ -1,6 +1,8 @@
>  #ifndef ARCHIVE_H
>  #define ARCHIVE_H
>  
> +#include "string-list.h"
> +
>  struct archiver_args {
>  	const char *base;
>  	size_t baselen;
> @@ -27,4 +29,17 @@ extern int write_zip_archive(struct archiver_args *);
>  extern int write_archive_entries(struct archiver_args *args, write_archive_entry_fn_t write_entry);
>  extern int write_archive(int argc, const char **argv, const char *prefix, int setup_prefix);
>  
> +struct tar_filter {
> +	char *name;
> +	char *command;
> +	struct string_list extensions;
> +	unsigned use_compression:1;

I suspect that you plan to pass sprintf("-%d", level) for the ones marked with this bit, but I wonder if we want to give a bit more control on how a compression level option is shaped for the particular command, and where on the command line the option comes. As long as we are targetting gzip and nothing else it is fine, and I suspect newer compression commands would try to mimic the -[0-9] command line interface gzip has (e.g. xz), so this probably is not an issue in practice.