Re: [PATCH] gitweb: snapshot cleanups & support for offering multiple formats

["Matt McCutchen" <hashproduct@gmail.com>]

On 7/12/07, Jakub Narebski <jnareb@gmail.com> wrote:
> > The advantage of '| gzip' is that the lack of a compressor is
> > not a special case.  This is why I wrote %known_snapshot_formats the
> > way I did, but of course you all are welcome to overrule me.
>
> I wrote about this because I'm thinking about replacing the few
> pipelines we use in gitweb[*1*], including the snapshot one, with
> the list form, which has the advantage of avoiding spawning the shell
> (performance) and not dealing with shell quoting of arguments (security
> and errors), like we did for simple calling of git commands to read
> from in the commit b9182987a80f7e820cbe1f8c7c4dc26f8586e8cd
>   "gitweb: Use list for of open for running git commands, thorougly"
>
> Thus I'd rather have list of extra commands and arguments instead of
> pipe as a string, i.e. 'gzip' instead of '| gzip', and 'gzip', '-9'
> instead of '| gzip -9'.
>
> [*1*] We currently use pipelines for snapshots which need external
> compressor, like tgz and tbz2, and for pickaxe search.

OK, I changed the extra commands to list form.  The field is now a
reference to the compressor argv like ['gzip'] or undef if there is no
compressor.

> I also prefer the second option, perhaps as simple as 'gzip' => 'tbz'
> and 'bzip2' => 'tbz2', and of course accompaning code to deal with this.
>
> As to "display name" column: I'm not sure if for example 'tar.gz'
> instead of 'tgz' would be not easier to understand.

I went ahead and added both aliases and display names (currently
'tar.gz', 'tar.bz2', and 'zip').  Aliases are resolved in
&feature_snapshot for repository configuration but not for side-wide
configuration because then aliases in side-wide configuration would be
resolved only if override is on, which would be weird.  For the same
reason, I changed the filtering out of unknown formats in
&feature_snapshot to apply only to repository configuration.

> > It would be possible to make the gitweb site configuration
> > backward-compatible too; here's one possible approach.  On startup,
> > gitweb would check whether $feature{'snapshot'}{'default'} is a
> > three-element list that appears to be in the old format.  If so, it
> > would save the settings in $known_snapshot_formats{'default'} and then
> > set $feature{'snapshot'}{'default'} = 'default' .  This is a hack; is
> > it justified by the compatibility benefit?
>
> If you implement 'gzip' and 'bzip2' as aliases, it could be as simple
> as just taking last non-false element of array if the array has more
> than one element.

No, because it must be possible for the site default to consist of
multiple formats.  It would be possible to take all elements that are
recognized as snapshot formats and ignore the others, but I would like
to be able to distinguish between "formats" that are part of a legacy
specification and completely bogus formats so that we can issue a
warning or error for the latter.

> But I'm not sure if it is worth it.

At this point I am leaning against backward compatibility for the site
configuration, and if I do implement it, I would just recognize the
three exact specifications that currently appear in feature_snapshot.
Recognizing other legacy specifications is iffy in the first place,
and handling them would require the hack I described.

I will send the revised patch soon.

Matt