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

From: Jakub Narebski <jnareb@gmail.com>
Date: Jul 12, 2007, 11:07 UTC
Message-ID: <200707121307.03612.jnareb@gmail.com>
In-Reply-To: <3bbc18d20707111815i1de3cb35sadfa316ddee7f3f6@mail.gmail.com>

On Thu, 12 July 2007, Matt McCutchen wrote:

Show 11 quoted lines

> On 7/11/07, Junio C Hamano <gitster@pobox.com> wrote:
>> Jakub Narebski <jnareb@gmail.com> writes:
>>
>>> I'm not sure if we want to store whole 'application/x-gzip' or only
>>> 'x-gzip' part of mime type, and if we want to store compressor as
>>> '| gzip' or simply as 'gzip'.
> 
> Storing only 'x-gzip' assumes that all archive formats have MIME types
> beginning with 'application/'.  Even if this assumption is justified
> by the MIME specification, I felt it was inappropriate to code it into
> gitweb.

Good argument. Besides, now we use it only in one place, but if we would in the future use it in other place having full mimetype would make it easier.

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

Show 22 quoted lines

>>> This would break not only existing _gitweb_ configuration (when
>>> gitweb admin installs new gitweb it isn't that hard to correct
>>> gitweb config), but also git _repositories_ config: gitweb.snapshot
>>> no longer work as it worked before, for example neither 'gzip'
>>> nor 'bzip2' values work anymore ('zip' doesn't stop working).
>>
>> I realized after seeing your other message on this patch that
>> this can be done while retaining backward compatibility, as you
>> suggested.  Matt, does Jakub's suggestion make sense to you?
> 
> It's not clear to me what the suggestion is: offer format names 'gzip'
> and 'bzip2' instead of 'tgz' and 'tbz2', or in addition to them, or
> what?  I prefer 'tgz' and 'tbz2' because they carry more information
> and are properly analogous to 'zip', so I don't want to offer 'gzip'
> and 'bzip2' instead of them.  Furthermore, I would like the user to
> see 'snapshot (tgz tbz2)' even if the repository owner wrote 'gzip
> bzip2', so just adding two rows to %known_snapshot_formats is
> insufficient.  Either an additional column could be added to
> %known_snapshot_formats for the display name, or 'gzip' and 'bzip2'
> could be specified as aliases in %known_snapshot_formats and
> feature_snapshot could be taught to resolve them.  I would prefer the
> second option; shall I implement it?

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.

Show 7 quoted lines

> 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. But I'm not sure if it is worth it.

But we should probably error out with some error message on incompatibile gitweb site configuration; I'm not sure...

-- 
Jakub Narebski
Poland