Re: gitweb pathinfo improvements

[Jakub Narebski <jnareb@gmail.com>]

Hi, Giuseppe!

I'm sorry for the delay in reviewing this series

On Wed, 3 September 2008, Giuseppe Bilotta wrote:
> 
> The following patchset improves on gitweb's support for PATH_INFO
> by supporting paths in the form project/action/[parent..]hash,
> both in generating them and in accepting them. The old path info
> style project/hash is still supported as long as it doesn't
> conflict with the new style
> 
> For those that prefer git trees to patch bombs, my git tree is
> available for gitweb browsing at http://git.oblomov.eu/git and for
> git cloning at git://git.oblomov.eu/git/git
> 
> The changes are very local to the PATH_INFO parsing and creation,
> so I hope they don't conflict with Lea's cache work.

Let me comment first on _intent_ of the patches, and on series as
a whole, before examining individual patches in detail.

Below there is table of contents / shortlog of this series, which I 
think is a good practice to include in cover letter describing the 
series:

Table of contents:
==================
 * [PATCH 1/5] gitweb: action in path with use_pathinfo
 * [PATCH 2/5] gitweb: use_pathinfo filenames start with /
 * [PATCH 3/5] gitweb: parse parent..current syntax from pathinfo
 * [PATCH 4/5] gitweb: use_pathinfo creates parent..current paths
 * [PATCH 5/5] gitweb: remove PATH_INFO from $my_url and $my_uri

This series of patches deal with moving more parameters from CGI query 
to pathinfo (beside 2nd and 5th, which are about relative navigation in 
HTML files ('blob_plain' view) and ability to act as DirectoryIndex, 
which means that they are about using gitweb as web server).

There are two sides of those pathinfo improvement: first is to be able 
to get more parameters from pathinfo, second is to use pathinfo-URLs
in gitweb links if requested/configured.

The problem with fitting more parameters in pathinfo is first backwards 
compatibility (this is not strict requirement, but we would rather not 
make existing bookmarked pathinfo URLs invalid), and second with 
ordering those parameters and detecting when one parameter ends and 
next one begins (which is made more complicated by the fact that some 
parameters, like action or hash/hash_base can be skipped).

This trouble with fitting parameters in pathinfo creates some 
limitations and tradeoffs. For example (optionally!) embedding
the action in the pathinfo, by putting it after project and before 
hash/hash_base (usually refname) and filename makes old-style 
$project/$branch lead to incorrect view. This also means that we have 
to be careful about creating pathinfo links, either by always putting 
an action, or using full ref name (which I think we do anyway to avoid 
tag/head ambiguities); or doing it only in the case of possible 
conflict i.e. branch named like one of gitweb actions.

Using ':' or ':/' to separate branch name (hash or hash_base) from 
filename doesn't lead to problems, as pathinfo is split on _first_ 
occurrence of ':', and refnames cannot contain ':'. Using '..' to 
separate $hash_parent_base:$file_parent from $hash_base:$filename
is IMVHO a very good idea... but when creating pathinfo links we have
to consider filenames with '..' in them; an example is there in git 
repository: "t/t5515/refs.master_.._.git" file. Then we probably want 
to fallback on CGI query/CGI parameters. NOTE: I have not read the 
patch yet, perhaps it does this.


By the way, this is perhaps slightly outside the issue of this series, 
but having a..b syntax would tempt to handcraft gitweb URLs for 
equivalents of "git log a..b", "git log a...b" and "git diff a...b",
neither of which works yet.

-- 
Jakub Narebski
Poland