On Wed, Jun 15, 2011 at 04:33:33PM -0700, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
> > 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).
> > +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.
> > +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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html