Re: gitweb pathinfo improvements

From: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
Date: Sep 6, 2008, 11:55 UTC
Message-ID: <cb7bb73a0809060455n78145ccdw99beeebbd7e0439a@mail.gmail.com>
In-Reply-To: <200809061322.31094.jnareb@gmail.com>

On Sat, Sep 6, 2008 at 1:22 PM, Jakub Narebski <jnareb@gmail.com> wrote:

Show 11 quoted lines

> 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

Yes, I really have to learn proper behaviour when sending a patchest. I'll make treasure of yours and Junio's hints on the matter 8-)

I'll probably have to resend this patch series anyway, since I've already found a quirk for which an additional patch is ready, and the double-dot-filename thing you mention below needs fixing as well.

BTW, is there a way to automate this summary generation when using format-patch or send-email?

Show 16 quoted lines

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

Yes, this was something that got me thinking for a while. I've tried as hard as possible to preserve backwards compatibility, and old-style paths should still work correctly except for projects whose branched are named exactly like gitweb actions.

Show 9 quoted lines

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

Actually, this was not a case I had taken into consideration (a filename with two dots). It should be straightforward to change the link-creation code to switch to CGI parameters in this case. Should I change the corresponding patch, or would it be enough to add a patch to the series clearing this issue?

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

I do have a patch to that effect for the shortlog action: http://git.oblomov.eu/git/commitdiff/refs/heads/gitweb/shortlog and you can see it in effect on my gitweb with links such as http://git.oblomov.eu/git/master..gitweb/pathinfo.

-- 
Giuseppe "Oblomov" Bilotta