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

From: Jakub Narebski <jnareb@gmail.com>
Date: Jul 10, 2007, 23:41 UTC
Message-ID: <f715g6$2sv$1@sea.gmane.org>
In-Reply-To: <1184023318.9703.1.camel@mattlaptop2>

Matt McCutchen wrote:

Show 18 quoted lines

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

Show 8 quoted lines

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

Show 8 quoted lines

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

Show 14 quoted lines

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