[PATCH] gitweb: start to generate PATH_INFO URLs

[PATCH] gitweb: start to generate PATH_INFO URLs

[Martin Waitz]

Instead of providing the project as a ?p= parameter it is simply appended
to the base URI.
All other parameters are appended to that, except for ?a=summary which
is the default and can be omitted.

Signed-off-by: Martin Waitz <tali@admingilde.org>
---
 gitweb/gitweb.perl |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 597d29f..e507ce9 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -355,6 +355,7 @@ ## action links
 
 sub href(%) {
 	my %params = @_;
+	my $href;
 
 	my @mapping = (
 		project => "p",
@@ -373,6 +374,16 @@ sub href(%) {
 
 	$params{'project'} = $project unless exists $params{'project'};
 
+	# first encode base url and project
+	$href = "$my_uri/$params{'project'}";
+	delete $params{'project'};
+
+	# Summary just uses the project path URL
+	if ($params{'action'} eq 'summary') {
+		return $href;
+	}
+
+	# now encode the parameters explicitly
 	my @result = ();
 	for (my $i = 0; $i < @mapping; $i += 2) {
 		my ($name, $symbol) = ($mapping[$i], $mapping[$i+1]);
@@ -380,7 +391,9 @@ sub href(%) {
 			push @result, $symbol . "=" . esc_param($params{$name});
 		}
 	}
-	return "$my_uri?" . join(';', @result);
+	$href .= "?" . join(';', @result);
+
+	return $href;
 }
 
 
-- 
1.4.2.gb8b6b

-- 
Martin Waitz

Re: [PATCH] gitweb: start to generate PATH_INFO URLs

[Junio C Hamano]

Martin Waitz <tali@admingilde.org> writes:

> Instead of providing the project as a ?p= parameter it is simply appended
> to the base URI.
> All other parameters are appended to that, except for ?a=summary which
> is the default and can be omitted.

Supporting PATH_INFO in the sense that we do sensible things
when we get called with one is one thing, but generating such a
URL that uses PATH_INFO is a different thing.  I suspect not
everybody's webserver is configured to call us with PATH_INFO,
so this should be conditional.

Re: [PATCH] gitweb: start to generate PATH_INFO URLs

[Martin Waitz]

[-- Attachment #1: Type: text/plain, Size: 944 bytes --]

hoi :)

On Fri, Sep 29, 2006 at 03:30:47PM -0700, Junio C Hamano wrote:
> Martin Waitz <tali@admingilde.org> writes:
> 
> > Instead of providing the project as a ?p= parameter it is simply appended
> > to the base URI.
> > All other parameters are appended to that, except for ?a=summary which
> > is the default and can be omitted.
> 
> Supporting PATH_INFO in the sense that we do sensible things
> when we get called with one is one thing, but generating such a
> URL that uses PATH_INFO is a different thing.  I suspect not
> everybody's webserver is configured to call us with PATH_INFO,
> so this should be conditional.

right, and in fact it was more intended as a RFC, to see what
people think about such a thing.  Obviously I wanted to have
it for my repository, so I implemented it unconditional first.

Should we use the gitweb feature mechanism to enable/disable
PATH_INFO URL generation?

-- 
Martin Waitz

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] gitweb: start to generate PATH_INFO URLs

[Junio C Hamano]

Martin Waitz <tali@admingilde.org> writes:

> Should we use the gitweb feature mechanism to enable/disable
> PATH_INFO URL generation?

That sounds sensible.  I personally do not think many people
would object if you made the default to true.  Also I do not
think it would make much sense to make this overridable by
repository configuration.

[PATCH] gitweb: start to generate PATH_INFO URLs.

[Martin Waitz]

Instead of providing the project as a ?p= parameter it is simply appended to
the base URI.  All other parameters are appended to that, except for ?a=summary
which is the default and can be omitted.

The old URL generation can be selected by disabling the "pathinfo" feature
in gitweb_config.perl.

Signed-off-by: Martin Waitz <tali@admingilde.org>
---
 gitweb/gitweb.perl |   22 +++++++++++++++++++++-
 1 files changed, 21 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 597d29f..edbd3ea 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -102,6 +102,10 @@ our %feature = (
 		'sub' => \&feature_pickaxe,
 		'override' => 0,
 		'default' => [1]},
+
+	'pathinfo' => {
+		'override' => 0,
+		'default' => [1]},
 );
 
 sub gitweb_check_feature {
@@ -355,6 +359,7 @@ ## action links
 
 sub href(%) {
 	my %params = @_;
+	my $href = $my_uri;
 
 	my @mapping = (
 		project => "p",
@@ -373,6 +378,19 @@ sub href(%) {
 
 	$params{'project'} = $project unless exists $params{'project'};
 
+	my ($use_pathinfo) = gitweb_check_feature('pathinfo');
+	if ($use_pathinfo) {
+		# use PATH_INFO for project name
+		$href .= "/$params{'project'}" if defined $params{'project'};
+		delete $params{'project'};
+
+		# Summary just uses the project path URL
+		if (defined $params{'action'} && $params{'action'} eq 'summary') {
+			delete $params{'action'};
+		}
+	}
+
+	# now encode the parameters explicitly
 	my @result = ();
 	for (my $i = 0; $i < @mapping; $i += 2) {
 		my ($name, $symbol) = ($mapping[$i], $mapping[$i+1]);
@@ -380,7 +398,9 @@ sub href(%) {
 			push @result, $symbol . "=" . esc_param($params{$name});
 		}
 	}
-	return "$my_uri?" . join(';', @result);
+	$href .= "?" . join(';', @result) if scalar @result;
+
+	return $href;
 }
 
 
-- 
1.4.2.gb8b6b

-- 
Martin Waitz

Re: [PATCH] gitweb: start to generate PATH_INFO URLs.

[Jakub Narebski]

Martin Waitz wrote:

> +       'pathinfo' => {
> +               'override' => 0,
> +               'default' => [1]},

You should add failsafe to gitweb_check_feature for when 'sub' is not set;
for example when somebody sets $feature{'pathinfo'}{'override'} to 1.
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] gitweb: start to generate PATH_INFO URLs.

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Martin Waitz wrote:
>
>> +       'pathinfo' => {
>> +               'override' => 0,
>> +               'default' => [1]},
>
> You should add failsafe to gitweb_check_feature for when 'sub' is not set;
> for example when somebody sets $feature{'pathinfo'}{'override'} to 1.

Yes, I noticed this last night while playing with it.  We would
at least need a big warning that says this should not be made
overridable (which does not make any sense anyway).

Setting 'sub' to a failsafe one that only returns what is in the
default without looking at individual repository would be the
cleanest, I think.

Re: [PATCH] gitweb: start to generate PATH_INFO URLs.

[Jakub Narebski]

Junio C Hamano wrote:

> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> Martin Waitz wrote:
>>
>>> +       'pathinfo' => {
>>> +               'override' => 0,
>>> +               'default' => [1]},
>>
>> You should add failsafe to gitweb_check_feature for when 'sub' is not
set;
>> for example when somebody sets $feature{'pathinfo'}{'override'} to 1.
> 
> Yes, I noticed this last night while playing with it.  We would
> at least need a big warning that says this should not be made
> overridable (which does not make any sense anyway).
> 
> Setting 'sub' to a failsafe one that only returns what is in the
> default without looking at individual repository would be the
> cleanest, I think.

Perhaps we should not add 'override' key, and test for existence
of 'override' to fallback on 'sub'.
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] gitweb: start to generate PATH_INFO URLs.

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Junio C Hamano wrote:
>
>> Jakub Narebski <jnareb@gmail.com> writes:
>> 
>>> Martin Waitz wrote:
>>>
>>>> +       'pathinfo' => {
>>>> +               'override' => 0,
>>>> +               'default' => [1]},
>>>
>>> You should add failsafe to gitweb_check_feature for when 'sub' is not
> set;
>>> for example when somebody sets $feature{'pathinfo'}{'override'} to 1.
>> 
>> Yes, I noticed this last night while playing with it.  We would
>> at least need a big warning that says this should not be made
>> overridable (which does not make any sense anyway).
>> 
>> Setting 'sub' to a failsafe one that only returns what is in the
>> default without looking at individual repository would be the
>> cleanest, I think.
>
> Perhaps we should not add 'override' key, and test for existence
> of 'override' to fallback on 'sub'.

Excellent idea.  Please make it so.

Re: [PATCH] gitweb: start to generate PATH_INFO URLs.

[Martin Waitz]

[-- Attachment #1: Type: text/plain, Size: 366 bytes --]

hoi :-)

On Tue, Oct 03, 2006 at 07:50:00PM +0200, Jakub Narebski wrote:
> Perhaps we should not add 'override' key, and test for existence
> of 'override' to fallback on 'sub'.

this is a no-op: if the config file sets override to 1 then
we still break.

Of course one say: don't do it then.
But then we don't need the check at all.

-- 
Martin Waitz

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[PATCH] gitweb: warn if feature cannot be overridden.

[Martin Waitz]

If the administrator configures pathinfo to be overrideable by the
local repository a warning is shown.

Signed-off-by: Martin Waitz <tali@admingilde.org>
---
 gitweb/gitweb.perl |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 10e803a..0ff6f7c 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -116,6 +116,10 @@ sub gitweb_check_feature {
 		$feature{$name}{'override'},
 		@{$feature{$name}{'default'}});
 	if (!$override) { return @defaults; }
+	if (!defined $sub) {
+		warn "feature $name is not overrideable";
+		return @defaults;
+	}
 	return $sub->(@defaults);
 }
 
-- 
1.4.2.3

-- 
Martin Waitz

Re: [PATCH] gitweb: start to generate PATH_INFO URLs

[Jakub Narebski]

Junio C Hamano wrote:

> Martin Waitz <tali@admingilde.org> writes:
> 
>> Instead of providing the project as a ?p= parameter it is simply appended
>> to the base URI.
>> All other parameters are appended to that, except for ?a=summary which
>> is the default and can be omitted.
> 
> Supporting PATH_INFO in the sense that we do sensible things
> when we get called with one is one thing, but generating such a
> URL that uses PATH_INFO is a different thing.  I suspect not
> everybody's webserver is configured to call us with PATH_INFO,
> so this should be conditional.

Or perhaps we should use PATH_INFO if the URL of current page uses
PATH_INFO too. The only place where we have to decide is the projects
list page (i.e. no arguments).
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

[PATCH] gitweb: make PATHINFO URL generation conditional on input URL.

[Martin Waitz]

Now the feature 'pathinfo' configuration only applies to the project
list.  All other URLs are generated in the form the webpage was
called itself.

Signed-off-by: Martin Waitz <tali@admingilde.org>
---
 gitweb/gitweb.perl |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 0ff6f7c..70246de 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -206,6 +206,8 @@ our $git_version = qx($GIT --version) =~
 
 $projects_list ||= $projectroot;
 
+our $use_pathinfo_url = gitweb_check_feature('pathinfo');
+
 # ======================================================================
 # input validation and dispatch
 our $action = $cgi->param('a');
@@ -226,6 +228,8 @@ if (defined $project) {
 		undef $project;
 		die_error(undef, "No such project");
 	}
+	# we got called without PATH_INFO, let's keep it that way.
+	$use_pathinfo_url = 0;
 }
 
 our $file_name = $cgi->param('f');
@@ -308,6 +312,10 @@ sub evaluate_path_info {
 		undef $project;
 		return;
 	}
+	
+	# we were called using a PATH_INFO URL, let's keep it that way.
+	$use_pathinfo_url = 1;
+
 	# do not change any parameters if an action is given using the query string
 	return if $action;
 	$path_info =~ s,^$project/*,,;
@@ -402,8 +410,7 @@ sub href(%) {
 
 	$params{'project'} = $project unless exists $params{'project'};
 
-	my ($use_pathinfo) = gitweb_check_feature('pathinfo');
-	if ($use_pathinfo) {
+	if ($use_pathinfo_url) {
 		# use PATH_INFO for project name
 		$href .= "/$params{'project'}" if defined $params{'project'};
 		delete $params{'project'};
-- 
1.4.2.3

-- 
Martin Waitz

Re: [PATCH] gitweb: start to generate PATH_INFO URLs

[Petr Baudis]

Dear diary, on Sat, Sep 30, 2006 at 12:30:47AM CEST, I got a letter
where Junio C Hamano <junkio@cox.net> said that...
> Martin Waitz <tali@admingilde.org> writes:
> 
> > Instead of providing the project as a ?p= parameter it is simply appended
> > to the base URI.
> > All other parameters are appended to that, except for ?a=summary which
> > is the default and can be omitted.
> 
> Supporting PATH_INFO in the sense that we do sensible things
> when we get called with one is one thing, but generating such a
> URL that uses PATH_INFO is a different thing.  I suspect not
> everybody's webserver is configured to call us with PATH_INFO,
> so this should be conditional.

Hmm, which webservers support CGI but don't pass PATH_INFO?


BTW, couple of notes for people who will want to try it: if gitweb.cgi
serves as your indexfile, this will break; you need to override $my_uri
in gitweb_config. Also, you need to change the default location of CSS,
favicon and logo to an absolute URL.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)

Re: [PATCH] gitweb: start to generate PATH_INFO URLs

[Junio C Hamano]

Petr Baudis <pasky@suse.cz> writes:

> BTW, couple of notes for people who will want to try it: if gitweb.cgi
> serves as your indexfile, this will break; you need to override $my_uri
> in gitweb_config. Also, you need to change the default location of CSS,
> favicon and logo to an absolute URL.

A patch to gitweb/README is in order perhaps?

Re: [PATCH] gitweb: start to generate PATH_INFO URLs

[Jakub Narebski]

Martin Waitz wrote:

> Instead of providing the project as a ?p= parameter it is simply appended
> to the base URI.

I have just modified href() to be able to use it for actions which don't
need the ?p= parameter... and you didn't take into consideration the case
when $params{'project'} is set, but undefined. Undefined params don't get
added, but "unless exists $params{'project'}" in the

        $params{'project'} = $project unless exists $params{'project'};

line prevents of adding 'p' param.

It is important for the project list (and equivalent) views.
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] gitweb: start to generate PATH_INFO URLs

[Martin Waitz]

[-- Attachment #1: Type: text/plain, Size: 506 bytes --]

hoi :)

On Tue, Oct 03, 2006 at 02:16:05PM +0200, Jakub Narebski wrote:
> Martin Waitz wrote:
> > Instead of providing the project as a ?p= parameter it is simply appended
> > to the base URI.
> 
> I have just modified href() to be able to use it for actions which don't
> need the ?p= parameter... and you didn't take into consideration the case
> when $params{'project'} is set, but undefined.

is this handled correctly in the second patch which got committed to
next?

-- 
Martin Waitz

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] gitweb: start to generate PATH_INFO URLs

[Jakub Narebski]

Martin Waitz wrote:

> On Tue, Oct 03, 2006 at 02:16:05PM +0200, Jakub Narebski wrote:
>> Martin Waitz wrote:
>> > Instead of providing the project as a ?p= parameter it is simply appended
>> > to the base URI.
>> 
>> I have just modified href() to be able to use it for actions which don't
>> need the ?p= parameter... and you didn't take into consideration the case
>> when $params{'project'} is set, but undefined.
> 
> is this handled correctly in the second patch which got committed to
> next?

I think it is (from the examining the patch, not from testing).

Perhaps we should use PATH_INFO if current URL uses PATH_INFO... 
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] gitweb: start to generate PATH_INFO URLs

[Junio C Hamano]

Martin Waitz <tali@admingilde.org> writes:

> On Tue, Oct 03, 2006 at 02:16:05PM +0200, Jakub Narebski wrote:
>> 
>> I have just modified href() to be able to use it for actions which don't
>> need the ?p= parameter... and you didn't take into consideration the case
>> when $params{'project'} is set, but undefined.
>
> is this handled correctly in the second patch which got committed to
> next?

I did only a light testing last night and I do not remember
testing this particular issue I did not see an obvious
breakage.  Could you two please verify and send in fixes if you
find any more issues around this area?

I think Jakub's suggestion to respond in PATH_INFO to requests
that does use PATH_INFO is a good one.