From: Jakub Narebski <jnareb@gmail.com>
Date: Sat, 31 Jan 2009 01:14:47 GMT
Subject: Re: [PATCHv4 1/2] gitweb: make static files accessible with PATH_INFO
Message-ID: <200901310214.49247.jnareb@gmail.com>
In-Reply-To: <1233139832-24124-1-git-send-email-giuseppe.bilotta@gmail.com>

On Wed, 28 Jan 2009, Giuseppe Bilotta wrote:

> When gitweb is invoked with PATH_INFO and links to static files such as
> the CSS and favicon/shortcut icon are relative URLs with relative paths
> (as is the case when using the default Makefile), these files are not
> accessible beyond the project list and summary page (e.g. in shortlog or
> commit view).
> 
> Fix this by adding a <base> tag pointing to the script's own URL, that
> ensure that all relative paths will be based on this.

I think it is a very good idea, good patch (now with esc_url, just in
case, even though I think it should be needed), and commit message has
all info that it should have. But I think that it could be phrased
better: instead of one long sentence, perhaps split it into few
sentences, each dealing with one issue. Something like below:

-- >8 --
Gitweb links to a few static files: CSS stylesheet, favicon/shortcut
icon and git logo.  When links to those files are given by relative
URLs with relative paths (not starting with '/'), and gitweb is invoked
with (non empty) PATH_INFO, these files are not accessible beyond
projects list and 'summary' view for a project (e.g. in 'shortlog' or
'commit' view).  Default Makefile rules use base filenames for those
static files.

Fix this by adding <base> element pointing to script's own URL,
which ensures that all relative paths relative URLs will be resolved
correctly.
-- 8< --

Gaaahhh... I think this version could be better, but I cannot think
how it should look like...

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 931db4f..f7aaf9a 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2901,6 +2901,11 @@ sub git_header_html {
>  <meta name="robots" content="index, nofollow"/>
>  <title>$title</title>
>  EOF
> +# the stylesheet, favicon etc urls won't work correctly with path_info unless we
> +# set the appropriate base URL

BTW. I think there should be independent patch making those comments
indented properly...

> +	if ($ENV{'PATH_INFO'}) {
> +		print "<base href='".esc_url($my_url)."' />\n";

Errr... here we use ' as attribute delimiter, while everywhere else
we use "; I don't know if esc_url deals correctly with this... it does
as neither " nor ' is in "allowed" list, but I'd personally use

+		print '<base href="'.esc_url($my_url).'" />'."\n";


> +	}
>  # print out each stylesheet that exist
>  	if (defined $stylesheet) {
>  #provides backwards capability for those people who define style sheet in a config file
> -- 
> 1.5.6.5
> 
> 

-- 
Jakub Narebski
Poland