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

[Jakub Narebski <jnareb@gmail.com>]

On Tue, 17 July 2007, Matt McCutchen napisał:
> - 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 if more than one format is offered, the "_snapshot_"
>   link becomes something like "snapshot (_tar.bz2_ _zip_)".
> 
> - If only one format is offered, a tooltip on the "_snapshot_" link
>   tells the user what it is.

Nice idea.

> - 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.  The preferred names for
> gitweb.snapshot in repository configuration have also changed from
> 'gzip' and 'bzip2' to 'tgz' and 'tbz2', but the old names are still
> recognized for compatibility.

This alert/warning should probably be put in RelNotes for when it would
be in git.git

> Signed-off-by: Matt McCutchen <hashproduct@gmail.com>
> ---
> 
> Changes since the previous revision of the patch:
> 
> - Added display names.
> - Changed compressor command line to list form.
> - Added compatibility format aliases for repository configuration.
> - Tweaked filtering of unknown formats to apply only to repository
>   configuration.
> - Reformatted format_snapshot_links and added/modified comments to make it much
>   easier to understand.
> - When a single snapshot format is offered, added a tooltip showing the format
>   to the "snapshot" link.  This helps Junio's hypothetical end user without
>   using additional screen real estate.
> 
> I thought of another incompatibility: previously bookmarked snapshot
> URLs will no longer work because they lack the new "sf" parameter.  I
> don't care about this; do any of you?

I think either having good error message, or using first format avaiable
would be good enough.

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

First I'm not sure if we want to do the way it had to be done when
those info was in the subfield of %feature hash, or to imitate %feature
hash using instead:

+our %known_snapshot_formats = (
+	'tgz'  => {
+		'display'  => 'tar.gz',
+		'mimetype' => 'application/x-gzip',
+		'suffix'   => '.tar.gz',
+		'format'   => 'tar',
+		'compressor' => ['gzip' ]},
... 

which means that when using %known_snapshot_formats we don't have
to remember for example which of the elements in array is mimetype,
which display name, and which format to be passed to git-archive.


Second, I have thought that we might want to simply use the rest of
array for the compressor and it's arguments instead of adding it as
anonymous array reference (inner array). But this way we could in
princile add more pipelines... although I think it would be not useful.
I'd rather have first option implemented, even if it does not allow for
multiple pipelines.

Third, I think we don't need to say "undef" explicitely, I think.
"defined ('a')[1]" returns the same as "defined ('a', undef)[1]".

> +# Aliases so we understand old gitweb.snapshot values in repository
> +# configuration.
> +our %known_snapshot_format_aliases = (
> +	'gzip'  => 'tgz' ,
> +	'bzip2' => 'tbz2',
> +);

Good idea, better than tring to fit it in %known_snapshot_formats.

-- 
Jakub Narebski
Poland