# [PATCH] gitweb: start to generate PATH_INFO URLs 19 messages from 2006-09-29 to 2006-10-07. Participants: Martin Waitz, Junio C Hamano, Jakub Narebski, Petr Baudis. Thread: https://gitlist.dev/t/5763 ## Martin Waitz, 2006-09-29 22:16 Subject: [PATCH] gitweb: start to generate PATH_INFO URLs Message-ID: <20060929221641.GC2871@admingilde.org> URL: https://gitlist.dev/e/20060929221641.GC2871%40admingilde.org ``` 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 --- 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 ``` ## Junio C Hamano, 2006-09-29 22:30 Subject: Re: [PATCH] gitweb: start to generate PATH_INFO URLs Message-ID: <7v8xk2jofc.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v8xk2jofc.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20060929221641.GC2871@admingilde.org> ``` Martin Waitz 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, 2006-09-30 18:14 Subject: Re: [PATCH] gitweb: start to generate PATH_INFO URLs Message-ID: <20060930181408.GD2871@admingilde.org> URL: https://gitlist.dev/e/20060930181408.GD2871%40admingilde.org In-Reply-To: <7v8xk2jofc.fsf@assigned-by-dhcp.cox.net> ``` hoi :) On Fri, Sep 29, 2006 at 03:30:47PM -0700, Junio C Hamano wrote: > Martin Waitz 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, 2006-09-30 19:42 Subject: Re: [PATCH] gitweb: start to generate PATH_INFO URLs Message-ID: <7vfye9dtv7.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vfye9dtv7.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20060930181408.GD2871@admingilde.org> ``` Martin Waitz 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, 2006-10-01 21:57 Subject: [PATCH] gitweb: start to generate PATH_INFO URLs. Message-ID: <20061001215748.GG2871@admingilde.org> URL: https://gitlist.dev/e/20061001215748.GG2871%40admingilde.org In-Reply-To: <7vfye9dtv7.fsf@assigned-by-dhcp.cox.net> ``` 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 --- 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 ``` ## Jakub Narebski, 2006-10-03 12:16 Subject: Re: [PATCH] gitweb: start to generate PATH_INFO URLs Message-ID: URL: https://gitlist.dev/e/eftk98%242ii%242%40sea.gmane.org In-Reply-To: <20060929221641.GC2871@admingilde.org> ``` 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 ``` ## Jakub Narebski, 2006-10-03 12:18 Subject: Re: [PATCH] gitweb: start to generate PATH_INFO URLs. Message-ID: URL: https://gitlist.dev/e/eftkdk%242ii%243%40sea.gmane.org In-Reply-To: <20061001215748.GG2871@admingilde.org> ``` 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 ``` ## Jakub Narebski, 2006-10-03 16:43 Subject: Re: [PATCH] gitweb: start to generate PATH_INFO URLs Message-ID: URL: https://gitlist.dev/e/efu3u4%24tc9%241%40sea.gmane.org In-Reply-To: <7v8xk2jofc.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano wrote: > Martin Waitz 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, 2006-10-03 17:12 Subject: Re: [PATCH] gitweb: start to generate PATH_INFO URLs Message-ID: <20061003171229.GN2871@admingilde.org> URL: https://gitlist.dev/e/20061003171229.GN2871%40admingilde.org In-Reply-To: ``` 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 ``` ## Jakub Narebski, 2006-10-03 17:19 Subject: Re: [PATCH] gitweb: start to generate PATH_INFO URLs Message-ID: URL: https://gitlist.dev/e/efu62g%247f8%241%40sea.gmane.org In-Reply-To: <20061003171229.GN2871@admingilde.org> ``` 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 ``` ## Junio C Hamano, 2006-10-03 17:36 Subject: Re: [PATCH] gitweb: start to generate PATH_INFO URLs Message-ID: <7vfye5pai1.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vfye5pai1.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20061003171229.GN2871@admingilde.org> ``` Martin Waitz 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. ``` ## Junio C Hamano, 2006-10-03 17:39 Subject: Re: [PATCH] gitweb: start to generate PATH_INFO URLs. Message-ID: <7vbqotpadg.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vbqotpadg.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Jakub Narebski 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. ``` ## Jakub Narebski, 2006-10-03 17:50 Subject: Re: [PATCH] gitweb: start to generate PATH_INFO URLs. Message-ID: URL: https://gitlist.dev/e/efu7r9%24fra%241%40sea.gmane.org In-Reply-To: <7vbqotpadg.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano wrote: > Jakub Narebski 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 ``` ## Martin Waitz, 2006-10-03 18:07 Subject: [PATCH] gitweb: warn if feature cannot be overridden. Message-ID: <20061003180743.GO2871@admingilde.org> URL: https://gitlist.dev/e/20061003180743.GO2871%40admingilde.org In-Reply-To: <7vbqotpadg.fsf@assigned-by-dhcp.cox.net> ``` If the administrator configures pathinfo to be overrideable by the local repository a warning is shown. Signed-off-by: Martin Waitz --- 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 ``` ## Martin Waitz, 2006-10-03 18:08 Subject: [PATCH] gitweb: make PATHINFO URL generation conditional on input URL. Message-ID: <20061003180832.GP2871@admingilde.org> URL: https://gitlist.dev/e/20061003180832.GP2871%40admingilde.org In-Reply-To: ``` 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 --- 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 ``` ## Junio C Hamano, 2006-10-03 20:16 Subject: Re: [PATCH] gitweb: start to generate PATH_INFO URLs. Message-ID: <7vwt7hm9yq.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vwt7hm9yq.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Jakub Narebski writes: > Junio C Hamano wrote: > >> Jakub Narebski 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, 2006-10-03 20:28 Subject: Re: [PATCH] gitweb: start to generate PATH_INFO URLs. Message-ID: <20061003202831.GS2871@admingilde.org> URL: https://gitlist.dev/e/20061003202831.GS2871%40admingilde.org In-Reply-To: ``` 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 ``` ## Petr Baudis, 2006-10-06 15:30 Subject: Re: [PATCH] gitweb: start to generate PATH_INFO URLs Message-ID: <20061006152925.GP20017@pasky.or.cz> URL: https://gitlist.dev/e/20061006152925.GP20017%40pasky.or.cz In-Reply-To: <7v8xk2jofc.fsf@assigned-by-dhcp.cox.net> ``` Dear diary, on Sat, Sep 30, 2006 at 12:30:47AM CEST, I got a letter where Junio C Hamano said that... > Martin Waitz 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 URL: https://gitlist.dev/e/7vbqoowmb4.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20061006152925.GP20017@pasky.or.cz> ``` Petr Baudis 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? ```