gitweb: start to generate PATH_INFO URLs

PATCH In review Martin Waitz 4 participants 21 messages Jun 15, 2016

Share & tools

Martin Waitz

author

2016-06-15 22:42 UTC #0 lore raw

PATCH 1/1 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.

Signed-off-by Martin Waitz <[email protected]>

---
 gitweb/gitweb.perl |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

gitweb/gitweb.perl +14 -1

index 597d29f..e507ce9 100755
@@ -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;
 }

Signature · 4 lines

1.4.2.gb8b6b

-- 
Martin Waitz

Junio C Hamano

2016-06-15 22:42 UTC #1 lore raw

Quoted 4 lines · 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.

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

author

2016-06-15 22:42 UTC #2 lore raw

hoi :)

Quoted 7 lines · Junio C Hamano

Quoted 4 lines · 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.

Quoted 5 lines

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?

Signature · 1 line

Martin Waitz

Junio C Hamano

2016-06-15 22:42 UTC #3 lore raw

Quoted 2 lines · Martin Waitz

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

author

2016-06-15 22:42 UTC #4 lore raw

PATCH 1/1 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 <[email protected]>

---
 gitweb/gitweb.perl |   22 +++++++++++++++++++++-
 1 files changed, 21 insertions(+), 1 deletions(-)

gitweb/gitweb.perl +21 -1

index 597d29f..edbd3ea 100755
@@ -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;
 }

Signature · 4 lines

1.4.2.gb8b6b

-- 
Martin Waitz

Jakub Narebski

2016-06-15 22:42 UTC #5 lore raw

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

Quoted 3 lines · Martin Waitz

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

Signature · 3 lines

Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Junio C Hamano

2016-06-15 22:42 UTC #6 lore raw

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

Quoted 8 lines · Jakub Narebski

Quoted 3 lines · Martin Waitz
+       '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.

Martin Waitz

author

2016-06-15 22:42 UTC #7 lore raw

PATCH 1/1 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 <[email protected]>

---
 gitweb/gitweb.perl |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

gitweb/gitweb.perl +4 -0

index 10e803a..0ff6f7c 100755
@@ -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);
 }

Signature · 4 lines

1.4.2.3

-- 
Martin Waitz

Jakub Narebski

2016-06-15 22:42 UTC #8 lore raw

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

In reply to Junio C Hamano

Quoted 9 lines · Junio C Hamano

Quoted 7 lines · Jakub Narebski
Quoted 3 lines · Martin Waitz
+       'pathinfo' => {
+               'override' => 0,
+               'default' => [1]},
You should add failsafe to gitweb_check_feature for when 'sub' is not

set;

Quoted 9 lines

Quoted 1 line
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'.

Signature · 3 lines

Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Martin Waitz

author

2016-06-15 22:42 UTC #9 lore raw

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

hoi :-)

Quoted 2 lines · Jakub Narebski

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.

Signature · 1 line

Martin Waitz

Junio C Hamano

2016-06-15 22:42 UTC #10 lore raw

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

In reply to Jakub Narebski

Quoted 18 lines · Jakub Narebski

Quoted 9 lines · Junio C Hamano
Quoted 7 lines · Jakub Narebski
Quoted 3 lines · Martin Waitz
+       'pathinfo' => {
+               'override' => 0,
+               'default' => [1]},
You should add failsafe to gitweb_check_feature for when 'sub' is not
set;
Quoted 6 lines
Quoted 1 line
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).

Quoted 6 lines

Quoted 3 lines
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.

Petr Baudis

2016-06-15 22:42 UTC #11 lore raw

In reply to Junio C Hamano

Dear diary, on Sat, Sep 30, 2006 at 12:30:47AM CEST, I got a letter where Junio C Hamano <[email protected]> said that...

Quoted 7 lines

Quoted 4 lines · 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.

Quoted 5 lines

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.

Signature · 5 lines

				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

2016-06-15 22:42 UTC #12 lore raw

Quoted 4 lines · Petr Baudis

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?

Petr Baudis

2016-06-15 22:42 UTC #13 lore raw

PATCH 1/1 gitweb: Document features better

This expands gitweb/README to talk some more about GITWEB_CONFIG, moves feature-specific documentation in gitweb.cgi to the inside of the %features array, and adds some short description of all the features.

Signed-off-by Petr Baudis <[email protected]>

---

 gitweb/README      |   16 +++++++++++++-
 gitweb/gitweb.perl |   59 ++++++++++++++++++++++++++++++++++++----------------
 2 files changed, 56 insertions(+), 19 deletions(-)

gitweb/README +15 -1

index 78e6fc0..e02e90f 100644
@@ -26,12 +26,26 @@ You can specify the following configurat
  * GITWEB_LOGO
    Points to the location where you put git-logo.png on your web server.
  * GITWEB_CONFIG
-   This file will be loaded using 'require'.  If the environment
+   This file will be loaded using 'require' and can be used to override any
+   of the options above as well as some other options - see the top of
+   'gitweb.cgi' for their full list and description.  If the environment
    $GITWEB_CONFIG is set when gitweb.cgi is executed the file in the
    environment variable will be loaded instead of the file
    specified when gitweb.cgi was created.
 
 
+Runtime gitweb configuration
+----------------------------
+
+You can adjust gitweb behaviour using the file specified in `GITWEB_CONFIG`
+(defaults to 'gitweb_config.perl' in the same directory as the CGI).
+See the top of 'gitweb.cgi' for the list of variables and some description.
+The most notable thing that is not configurable at compile time are the
+optional features, stored in the '%features' variable. You can find further
+description on how to reconfigure the default features setting in your
+`GITWEB_CONFIG` or per-project in `project.git/config` inside 'gitweb.cgi'.
+
+
 Webserver configuration
 -----------------------

gitweb/gitweb.perl +41 ~~-18~~

105 lines not on this page · raw message

Jakub Narebski

2016-06-15 22:42 UTC #14 lore raw

Re: [PATCH] gitweb: Document features better

Quoted 3 lines · Petr Baudis

This expands gitweb/README to talk some more about GITWEB_CONFIG, moves feature-specific documentation in gitweb.cgi to the inside of the %features array, and adds some short description of all the features.

Thanks. Very nice.

Signature · 3 lines

Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Jakub Narebski

2016-06-15 22:42 UTC #15 lore raw

In reply to Junio C Hamano

Quoted 7 lines · Junio C Hamano

Quoted 4 lines · 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.

Quoted 5 lines

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

Signature · 3 lines

Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Martin Waitz

author

2016-06-15 22:42 UTC #16 lore raw

PATCH 1/1 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 <[email protected]>

---
 gitweb/gitweb.perl |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

gitweb/gitweb.perl +9 -2

index 0ff6f7c..70246de 100755
@@ -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'};

Signature · 4 lines

1.4.2.3

-- 
Martin Waitz

Jakub Narebski

2016-06-15 22:42 UTC #17 lore raw

In reply to Martin Waitz

Quoted 2 lines · Martin Waitz

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.

Signature · 3 lines

Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Martin Waitz

author

2016-06-15 22:42 UTC #18 lore raw

hoi :)

Quoted 7 lines · Jakub Narebski

Quoted 2 lines · Martin Waitz
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?

Signature · 1 line

Martin Waitz

Junio C Hamano

2016-06-15 22:42 UTC #19 lore raw

Quoted 8 lines · Martin Waitz

Quoted 4 lines · Jakub Narebski
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.

Jakub Narebski

2016-06-15 22:42 UTC #20 lore raw

In reply to Martin Waitz

Quoted 11 lines · Martin Waitz

Quoted 7 lines · Jakub Narebski
Quoted 2 lines · Martin Waitz
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...

Signature · 3 lines

Jakub Narebski
Warsaw, Poland
ShadeHawk on #git