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

From: Jeff King <peff@github.com>
Date: Jun 22, 2011, 14:59 UTC
Message-ID: <20110622145916.GA9266@sigill.intra.peff.net>
In-Reply-To: <4E01872F.8070503@lsrfire.ath.cx>

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.

Show 7 quoted lines

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

Show 20 quoted lines

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