threads / patch / 5763

patchgitweb: start to generate PATH_INFO URLs

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

## tl;dr

19 messages between Sep 29, 2006 and Oct 7, 2006. Diffs are folded; open one to read it.

replies: 18people: 4as markdown or json

Martin Waitz· Sep 29, 2006, 22:16 UTC · lore

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

Show changes to gitweb/gitweb.perl +14 −1

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

Junio C Hamano· Sep 29, 2006, 22:30 UTC · re: Martin Waitz · lore

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

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.

Martin Waitz· Sep 30, 2006, 18:14 UTC · re: Junio C Hamano · lore

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

hoi :)

On Fri, Sep 29, 2006 at 03:30:47PM -0700, Junio C Hamano wrote:

Show 12 quoted lines

> 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

Junio C Hamano· Sep 30, 2006, 19:42 UTC · re: Martin Waitz · lore

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

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.

Martin Waitz· Oct 1, 2006, 21:57 UTC · re: Junio C Hamano · lore

[PATCH] gitweb: start to generate PATH_INFO URLs.

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

Show changes to gitweb/gitweb.perl +21 −1

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

Jakub Narebski· Oct 3, 2006, 12:18 UTC · re: Martin Waitz · lore

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

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

Junio C Hamano· Oct 3, 2006, 17:39 UTC · re: Jakub Narebski · lore

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

Jakub Narebski <jnareb@gmail.com> writes:

Show 8 quoted lines

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

Jakub Narebski· Oct 3, 2006, 17:50 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:

Show 9 quoted lines

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

Show 9 quoted lines

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

Junio C Hamano· Oct 3, 2006, 20:16 UTC · re: Jakub Narebski · lore

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

Jakub Narebski <jnareb@gmail.com> writes:

Show 24 quoted lines

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

Martin Waitz· Oct 3, 2006, 20:28 UTC · re: Jakub Narebski · lore

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

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

Martin Waitz· Oct 3, 2006, 18:07 UTC · re: Junio C Hamano · lore

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

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

Show changes to gitweb/gitweb.perl +4 −0

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

Jakub Narebski· Oct 3, 2006, 16:43 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:

Show 12 quoted lines

> 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

Martin Waitz· Oct 3, 2006, 18:08 UTC · re: Jakub Narebski · lore

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

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

Show changes to gitweb/gitweb.perl +9 −2

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

Petr Baudis· Oct 6, 2006, 15:30 UTC · re: Junio C Hamano · lore

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

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

Show 12 quoted lines

> 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*',/((..)*)$/)

Junio C Hamano· Oct 7, 2006, 08:40 UTC · re: Petr Baudis · lore

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

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?

Jakub Narebski· Oct 3, 2006, 12:16 UTC · re: Martin Waitz · lore

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

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

Martin Waitz· Oct 3, 2006, 17:12 UTC · re: Jakub Narebski · lore

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

hoi :)

On Tue, Oct 03, 2006 at 02:16:05PM +0200, Jakub Narebski wrote:

Show 7 quoted lines

> 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

Jakub Narebski· Oct 3, 2006, 17:19 UTC · re: Martin Waitz · lore

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

Martin Waitz wrote:

Show 11 quoted lines

> 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

Junio C Hamano· Oct 3, 2006, 17:36 UTC · re: Martin Waitz · lore

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

Martin Waitz <tali@admingilde.org> writes:

Show 8 quoted lines

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