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

On Wed, Jun 15, 2011 at 04:33:33PM -0700, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

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

I don't know that it would actually make this code significantly clearer
or more efficient. If it were a sorted array, one could do a binary
search, but we are really talking about a handful of elements (if you
did want to refactor, this is almost identical to the matching code in
userdiff, too).

quoted

+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?

Yeah, it does. My original version had the builtin gzip statically
allocated, and it wasn't safe to free() anything. But I ended up having
to allocate it dynamically anyway because of the variable-sized list of
extensions, so it would be safe to free(tf->command) here. I'll do that
in my re-roll.

quoted

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

Yeah, I assumed everyone who would want this would support -[0-9]. After
all, all the flag is doing is passing -[0-9] that was supplied to
git-archive. We could allow something like:

  [tarfilter "gzip"]
    command = gzip %(compression)

but I don't see much point. Either you want it or you don't. If there is
a complex mapping of those numbers to some other options in your
command, then point git to a helper script which does the conversion
and then execs your command.

If somebody has a counterexample, I'd be curious to hear it.

-Peff