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

[Jakub Narebski <jnareb@gmail.com>]

Matt McCutchen wrote:

> - Centralize knowledge about snapshot formats (mime types, extensions,
>   commands) in %known_snapshot_formats and improve how some of that
>   information is specified.  In particular, zip files are no longer a
>   special case.
> 
> - Add support for offering multiple snapshot formats to the user so
>   that he/she can download a snapshot in the format he/she prefers.
>   The site-wide or project configuration now gives a list of formats
>   to offer, and the "_snapshot_" link is replaced with, say,
>   "snapshot (_tbz2_ _zip_)".
> 
> - Fix out-of-date "tarball" -> "archive" in comment.
> 
> Alert for gitweb site administrators: This patch changes the format of
> $feature{'snapshot'}{'default'} in gitweb_config.perl from a list of three
> pieces of information about a single format to a list of one or more formats
> you wish to offer from the set ('tgz', 'tbz2', 'zip').  Update your
> gitweb_config.perl appropriately.

Quite nice and I think needed refactoring of a snapshot code. Nevertheless
I have some comments on the changes introduced by this patch; not only
change to gitweb_config.perl is needed (which gitweb admin has control
over), but also repo config for individual repositories might need to
be changed (which gitweb admin might not have control over, and which is
much harder to do).

> +# information about snapshot formats that gitweb is capable of serving
> +# name => [mime type, filename suffix, --format for git-archive,
> +#          compressor command suffix]
> +our %known_snapshot_formats = (
> +     'tgz'  => ['application/x-gzip' , '.tar.gz' , 'tar', '| gzip' ],
> +     'tbz2' => ['application/x-bzip2', '.tar.bz2', 'tar', '| bzip2'],
> +     'zip'  => ['application/zip'    , '.zip'    , 'zip', ''       ],
> +);

First, is full mimetype really needed? Earlier code assumed that mimetype
for snapshot is of the form of application/<something>, and it provided
only <something>.

Second, I'd rather have 'gzip' and 'bzip2' aliases to 'tgz' and 'tbz2',
so the old config continues to work. I can see that it would be hard
to do without special-casing code, or changing the assumption that list
of default available snapshot formats is keys of above hash.

> -     # and in project config gitweb.snapshot = none|gzip|bzip2|zip;
> +     # and in project config, a comma-separated list of formats or "none"
> +     # to disable.  Example: gitweb.snapshot = tbz2,zip;

I would relax the syntax, so "tbz2, zip" would also work, or even
"tbz2 zip". I'd like for old config to also work, meaning that "gzip"
would be the same as "tgz" and "bzip2" as "tbz2".

> -     if ($val eq 'gzip') {
> -             return ('x-gzip', 'gz', 'gzip');
> -     } elsif ($val eq 'bzip2') {
> -             return ('x-bzip2', 'bz2', 'bzip2');
> -     } elsif ($val eq 'zip') {
> -             return ('x-zip', 'zip', '');
> -     } elsif ($val eq 'none') {
> -             return ();

Very nice getting rid of this swith-like statement...

> +     if ($val) {
> +             @fmts = ($val eq 'none' ? () : split /,/, $val);

... but I would relax this regexp.

> +# Generates undef or something like "snapshot (tbz2 zip)", linked.
> +# Pass the hash.
> +sub format_snapshot_links {
> +     my ($hash) = @_;
> +     my @snapshot_fmts = gitweb_check_feature('snapshot');
> +     if (@snapshot_fmts) {
> +             return "snapshot (" . join(' ', map $cgi->a(
> +                     {-href => href(action=>"snapshot", hash=>$hash, snapshot_format=>$_)}, "$_"),
> +                     @snapshot_fmts)
> +             . ")";
> +     } else {
> +             return undef;
> +     }
> +}

Nice separation into subroutine.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git