Re: gitweb pathinfo improvements

From: Jakub Narebski <jnareb@gmail.com>
Date: Sep 6, 2008, 11:22 UTC
Message-ID: <200809061322.31094.jnareb@gmail.com>
In-Reply-To: <1220435839-29360-1-git-send-email-giuseppe.bilotta@gmail.com>

Hi, Giuseppe!

I'm sorry for the delay in reviewing this series

On Wed, 3 September 2008, Giuseppe Bilotta wrote:

Show 13 quoted lines

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