Re: [PATCHv2 7/9] archive: implement configurable tar filters

[Jeff King <peff@github.com>]

On Wed, Jun 22, 2011 at 08:09:51AM +0200, René Scharfe wrote:

> just a quick comment before I drop off the net for a few days.  I like
> the series a lot, especially the refactorings in patches 1 to 6.

Thanks. I didn't know if I was going overboard, but the result looked
cleaner to me, so it is good to have a second opinion.

> > +tar.<format>.command::
> 
> Would switching around format and "command" be better?
> 
> 	[tar "command"]
> 		tar.gz = gzip -cn
> 		tar.xz = xz -c

That violates our usual convention that the first and final components
are static, and the middle part can contain everything. So doing:

  git config tar.command.tar.gz "gzip -cn"

is going to end up as:

  [tar "command.tar"]
    gz = gzip -cn

Plus it doesn't leave room for any additional per-command config keys if
we want to add them in the future.

> > +	ar = find_tar_filter(name, namelen);
> > +	if (!ar) {
> > +		ar = xcalloc(1, sizeof(*ar));
> > +		ar->name = xmemdupz(name, namelen);
> > +		ar->write_archive = write_tar_filter_archive;
> > +		ar->flags = ARCHIVER_WANT_COMPRESSION_LEVELS;
> > +		ALLOC_GROW(tar_filters, nr_tar_filters + 1, alloc_tar_filters);
> > +		tar_filters[nr_tar_filters++] = ar;
> > +	}
> > +
> > +	if (!strcmp(type, "command")) {
> > +		if (!value)
> > +			return config_error_nonbool(var);
> > +		free(ar->data);
> > +		ar->data = xstrdup(value);
> > +		return 0;
> > +	}
> 
> Why not register it right here instead of adding it to the intermediate
> list?

If it were just this patch, you could do that. But as soon as you add
more keys (e.g., a later patch adds tar.*.remote), then you run into the
situation of getting only part of the config at a time, and maybe not
getting the full config for a command at all. For example:

  [tar "tar.gz"]
    remote = true

would make an archiver with no "command" set. We would need to
special-case it everywhere to ignore it when we looked at the list, or
later just remove it.  This patch takes the approach of having a
secondary list of all of the configured bits, and then only registering
those that are actually valid.

It also keeps the configured and builtin lists separate. Otherwise I
have to special-case:

  [tar "zip"]
    command = ...

to ignore the builtin zip archiver, which I think is not something we
want to be able to override in this way.

> And are duplicates handled properly, e.g. system has "gzip -cn"
> and local wants "gzip -c"?

Yes. We look up the archiver in the list of configured ones and
overwrite its command field (that's why the .tar.gz patch actually calls
the config parser as if you had those lines in your config file, instead
of registering static archiver structs).

I should probably include a test for that, though.

-Peff