[PATCH] gitweb: Add option to put a trailing slash on pathinfo-style project URLs

[PATCH] gitweb: Add option to put a trailing slash on pathinfo-style project URLs

[Matt McCutchen]

My Web site uses pathinfo mode and some rewrite magic to show the gitweb
interface at the URL of the real repository directory (which users also
pull from).  In this case, it's desirable to end generated links to the
project in a trailing slash so the Web server doesn't have to redirect
the client to add the slash.  This patch adds a second element to the
"pathinfo" feature configuration to control the trailing slash.
---

What do you think of this?  I've been using it on my Web site for a
while now.

Matt

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 6eb370d..86511cf 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -270,6 +270,11 @@ our %feature = (
 	# $feature{'pathinfo'}{'default'} = [1];
 	# Project specific override is not supported.
 
+	# If you want a trailing slash on the project path (because, for
+	# example, you have a real directory at that URL and are using
+	# some rewrite magic to invoke gitweb), then set:
+	# $feature{'pathinfo'}{'default'} = [1, 1];
+
 	# Note that you will need to change the default location of CSS,
 	# favicon, logo and possibly other files to an absolute URL. Also,
 	# if gitweb.cgi serves as your indexfile, you will need to force
@@ -829,8 +834,8 @@ sub href (%) {
 		}
 	}
 
-	my $use_pathinfo = gitweb_check_feature('pathinfo');
-	if ($use_pathinfo) {
+	my @use_pathinfo = gitweb_get_feature('pathinfo');
+	if ($use_pathinfo[0]) {
 		# try to put as many parameters as possible in PATH_INFO:
 		#   - project name
 		#   - action
@@ -845,7 +850,12 @@ sub href (%) {
 		$href =~ s,/$,,;
 
 		# Then add the project name, if present
-		$href .= "/".esc_url($params{'project'}) if defined $params{'project'};
+		my $proj_href = undef;
+		if (defined $params{'project'}) {
+			$href .= "/".esc_url($params{'project'});
+			# Save for trailing-slash check below.
+			$proj_href = $href;
+		}
 		delete $params{'project'};
 
 		# since we destructively absorb parameters, we keep this
@@ -903,6 +913,10 @@ sub href (%) {
 			$href .= $known_snapshot_formats{$fmt}{'suffix'};
 			delete $params{'snapshot_format'};
 		}
+
+		# If requested in the configuration, add a trailing slash to a URL that
+		# has nothing appended after the project path.
+		$href .= '/' if ($use_pathinfo[1] && defined $proj_href && $href eq $proj_href);
 	}
 
 	# now encode the parameters explicitly
@@ -2987,13 +3001,15 @@ EOF
 			$search_hash = "HEAD";
 		}
 		my $action = $my_uri;
-		my $use_pathinfo = gitweb_check_feature('pathinfo');
-		if ($use_pathinfo) {
+		my @use_pathinfo = gitweb_get_feature('pathinfo');
+		if ($use_pathinfo[0]) {
 			$action .= "/".esc_url($project);
+			# Add a trailing slash if requested in the configuration.
+			$action .= '/' if ($use_pathinfo[1]);
 		}
 		print $cgi->startform(-method => "get", -action => $action) .
 		      "<div class=\"search\">\n" .
-		      (!$use_pathinfo &&
+		      (!$use_pathinfo[0] &&
 		      $cgi->input({-name=>"p", -value=>$project, -type=>"hidden"}) . "\n") .
 		      $cgi->input({-name=>"a", -value=>"search", -type=>"hidden"}) . "\n" .
 		      $cgi->input({-name=>"h", -value=>$search_hash, -type=>"hidden"}) . "\n" .
-- 
1.6.1.rc2.24.gf17b3c.dirty

[PATCH try 2] gitweb: Add option to put a trailing slash on pathinfo-style project URLs

[Matt McCutchen]

My Web site uses pathinfo mode and some rewrite magic to show the gitweb
interface at the URL of the real repository directory (which users also
pull from).  In this case, it's desirable to end generated links to the
project in a trailing slash so the Web server doesn't have to redirect
the client to add the slash.  This patch adds a second element to the
"pathinfo" feature configuration to control the trailing slash.

Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>
---
Resending with a sign-off.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 6eb370d..86511cf 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -270,6 +270,11 @@ our %feature = (
 	# $feature{'pathinfo'}{'default'} = [1];
 	# Project specific override is not supported.
 
+	# If you want a trailing slash on the project path (because, for
+	# example, you have a real directory at that URL and are using
+	# some rewrite magic to invoke gitweb), then set:
+	# $feature{'pathinfo'}{'default'} = [1, 1];
+
 	# Note that you will need to change the default location of CSS,
 	# favicon, logo and possibly other files to an absolute URL. Also,
 	# if gitweb.cgi serves as your indexfile, you will need to force
@@ -829,8 +834,8 @@ sub href (%) {
 		}
 	}
 
-	my $use_pathinfo = gitweb_check_feature('pathinfo');
-	if ($use_pathinfo) {
+	my @use_pathinfo = gitweb_get_feature('pathinfo');
+	if ($use_pathinfo[0]) {
 		# try to put as many parameters as possible in PATH_INFO:
 		#   - project name
 		#   - action
@@ -845,7 +850,12 @@ sub href (%) {
 		$href =~ s,/$,,;
 
 		# Then add the project name, if present
-		$href .= "/".esc_url($params{'project'}) if defined $params{'project'};
+		my $proj_href = undef;
+		if (defined $params{'project'}) {
+			$href .= "/".esc_url($params{'project'});
+			# Save for trailing-slash check below.
+			$proj_href = $href;
+		}
 		delete $params{'project'};
 
 		# since we destructively absorb parameters, we keep this
@@ -903,6 +913,10 @@ sub href (%) {
 			$href .= $known_snapshot_formats{$fmt}{'suffix'};
 			delete $params{'snapshot_format'};
 		}
+
+		# If requested in the configuration, add a trailing slash to a URL that
+		# has nothing appended after the project path.
+		$href .= '/' if ($use_pathinfo[1] && defined $proj_href && $href eq $proj_href);
 	}
 
 	# now encode the parameters explicitly
@@ -2987,13 +3001,15 @@ EOF
 			$search_hash = "HEAD";
 		}
 		my $action = $my_uri;
-		my $use_pathinfo = gitweb_check_feature('pathinfo');
-		if ($use_pathinfo) {
+		my @use_pathinfo = gitweb_get_feature('pathinfo');
+		if ($use_pathinfo[0]) {
 			$action .= "/".esc_url($project);
+			# Add a trailing slash if requested in the configuration.
+			$action .= '/' if ($use_pathinfo[1]);
 		}
 		print $cgi->startform(-method => "get", -action => $action) .
 		      "<div class=\"search\">\n" .
-		      (!$use_pathinfo &&
+		      (!$use_pathinfo[0] &&
 		      $cgi->input({-name=>"p", -value=>$project, -type=>"hidden"}) . "\n") .
 		      $cgi->input({-name=>"a", -value=>"search", -type=>"hidden"}) . "\n" .
 		      $cgi->input({-name=>"h", -value=>$search_hash, -type=>"hidden"}) . "\n" .
-- 
1.6.1.rc2.27.gc7114

Re: [PATCH try 2] gitweb: Add option to put a trailing slash on pathinfo-style project URLs

[Jakub Narebski]

Matt McCutchen <matt@mattmccutchen.net> writes:

> My Web site uses pathinfo mode and some rewrite magic to show the gitweb
> interface at the URL of the real repository directory (which users also
> pull from).  In this case, it's desirable to end generated links to the
> project in a trailing slash so the Web server doesn't have to redirect
> the client to add the slash.  This patch adds a second element to the
> "pathinfo" feature configuration to control the trailing slash.
> 
> Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>

Did you check that it does not confuse gitweb if filename parameter is
passed using pathinfo?  Gitweb used to rely on final '/' to
distinguish directory pathnames from ordinary pathnames, but I think
currently thanks to the fact that gitweb now embeds action in pathinfo
URL, and does not need to guess type, it is not an issue.

Or only project URLs (i.e. only with project parameter, i.e. only
"http://git.example.com/project.git/" but not other path_info links)
have trailing slash added?

Errr... I see that it adds trailing slash only for project-only
path_info links, but the commit message was not entirely clear for me.

(CC-ed author of path_info enhancements, Giuseppe Bilotta)

> ---
> Resending with a sign-off.

Thanks.

>  gitweb/gitweb.perl |   28 ++++++++++++++++++++++------
>  1 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 6eb370d..86511cf 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -270,6 +270,11 @@ our %feature = (
>  	# $feature{'pathinfo'}{'default'} = [1];
>  	# Project specific override is not supported.
>  
> +	# If you want a trailing slash on the project path (because, for
> +	# example, you have a real directory at that URL and are using
> +	# some rewrite magic to invoke gitweb), then set:
> +	# $feature{'pathinfo'}{'default'} = [1, 1];
> +

Are any disadvantages to having it always enabled?

BTW. encoding data in position in array feels a bit hacky to me, but
I guess that is the limitation of current %feature design, with
'default' having to be array (reference).

>  	# Note that you will need to change the default location of CSS,
>  	# favicon, logo and possibly other files to an absolute URL. Also,
>  	# if gitweb.cgi serves as your indexfile, you will need to force
> @@ -829,8 +834,8 @@ sub href (%) {
>  		}
>  	}
>  
> -	my $use_pathinfo = gitweb_check_feature('pathinfo');
> -	if ($use_pathinfo) {
> +	my @use_pathinfo = gitweb_get_feature('pathinfo');

Why not name those variables for better readability?

+       my ($use_pathinfo, $trailing_slash) = gitweb_get_feature('pathinfo');

> +	if ($use_pathinfo[0]) {
>  		# try to put as many parameters as possible in PATH_INFO:
>  		#   - project name
>  		#   - action
> @@ -845,7 +850,12 @@ sub href (%) {
>  		$href =~ s,/$,,;
>  
>  		# Then add the project name, if present
> -		$href .= "/".esc_url($params{'project'}) if defined $params{'project'};
> +		my $proj_href = undef;
> +		if (defined $params{'project'}) {
> +			$href .= "/".esc_url($params{'project'});
> +			# Save for trailing-slash check below.
> +			$proj_href = $href;
> +		}
>  		delete $params{'project'};
>  
>  		# since we destructively absorb parameters, we keep this
> @@ -903,6 +913,10 @@ sub href (%) {
>  			$href .= $known_snapshot_formats{$fmt}{'suffix'};
>  			delete $params{'snapshot_format'};
>  		}
> +
> +		# If requested in the configuration, add a trailing slash to a URL that
> +		# has nothing appended after the project path.
> +		$href .= '/' if ($use_pathinfo[1] && defined $proj_href && $href eq $proj_href);
>  	}

The check _feels_ inefficient.  I think (but feel free to disagree) that
it would be better to use something like $project_pathinfo, set it
when adding project as pathinfo, and unset if we add anything else as
pathinfo.

>  
>  	# now encode the parameters explicitly
> @@ -2987,13 +3001,15 @@ EOF
>  			$search_hash = "HEAD";
>  		}
>  		my $action = $my_uri;
> -		my $use_pathinfo = gitweb_check_feature('pathinfo');
> -		if ($use_pathinfo) {
> +		my @use_pathinfo = gitweb_get_feature('pathinfo');

Same comment as above: better named variable instead of relying on
position in array.

> +		if ($use_pathinfo[0]) {
>  			$action .= "/".esc_url($project);
> +			# Add a trailing slash if requested in the configuration.
> +			$action .= '/' if ($use_pathinfo[1]);

Hmmm... let me check something... you rely on the fact that $project
doesn't end with slash, while I think (but please check it) that it
can end with slash if it is provided by CGI query.

>  		}
>  		print $cgi->startform(-method => "get", -action => $action) .
>  		      "<div class=\"search\">\n" .
> -		      (!$use_pathinfo &&
> +		      (!$use_pathinfo[0] &&
>  		      $cgi->input({-name=>"p", -value=>$project, -type=>"hidden"}) . "\n") .
>  		      $cgi->input({-name=>"a", -value=>"search", -type=>"hidden"}) . "\n" .
>  		      $cgi->input({-name=>"h", -value=>$search_hash, -type=>"hidden"}) . "\n" .
> -- 
> 1.6.1.rc2.27.gc7114
> 

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH try 2] gitweb: Add option to put a trailing slash on pathinfo-style project URLs

[Giuseppe Bilotta]

On Sat, Dec 13, 2008 at 10:47 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> Matt McCutchen <matt@mattmccutchen.net> writes:
>
>> My Web site uses pathinfo mode and some rewrite magic to show the gitweb
>> interface at the URL of the real repository directory (which users also
>> pull from).  In this case, it's desirable to end generated links to the
>> project in a trailing slash so the Web server doesn't have to redirect
>> the client to add the slash.  This patch adds a second element to the
>> "pathinfo" feature configuration to control the trailing slash.
>>
>> Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>
>
> Did you check that it does not confuse gitweb if filename parameter is
> passed using pathinfo?  Gitweb used to rely on final '/' to
> distinguish directory pathnames from ordinary pathnames, but I think
> currently thanks to the fact that gitweb now embeds action in pathinfo
> URL, and does not need to guess type, it is not an issue.
>
> Or only project URLs (i.e. only with project parameter, i.e. only
> "http://git.example.com/project.git/" but not other path_info links)
> have trailing slash added?
>
> Errr... I see that it adds trailing slash only for project-only
> path_info links, but the commit message was not entirely clear for me.

If indeed the additional / is only asked for in summary view, I think
there's no need for a feature toggle, we can always put it there. If
not, I'm really curious about seeing the rewrite rules (they might
also be worth adding to the gitweb documentation as examples of 'power
usage').

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH try 2] gitweb: Add option to put a trailing slash on pathinfo-style project URLs

[Matt McCutchen]

On Sat, 2008-12-13 at 23:23 +0100, Giuseppe Bilotta wrote:
> On Sat, Dec 13, 2008 at 10:47 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> > Matt McCutchen <matt@mattmccutchen.net> writes:
> >
> >> My Web site uses pathinfo mode and some rewrite magic to show the gitweb
> >> interface at the URL of the real repository directory (which users also
> >> pull from).  In this case, it's desirable to end generated links to the
> >> project in a trailing slash so the Web server doesn't have to redirect
> >> the client to add the slash.  This patch adds a second element to the
> >> "pathinfo" feature configuration to control the trailing slash.
> >>
> >> Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>
> >
> > Did you check that it does not confuse gitweb if filename parameter is
> > passed using pathinfo?  Gitweb used to rely on final '/' to
> > distinguish directory pathnames from ordinary pathnames, but I think
> > currently thanks to the fact that gitweb now embeds action in pathinfo
> > URL, and does not need to guess type, it is not an issue.
> >
> > Or only project URLs (i.e. only with project parameter, i.e. only
> > "http://git.example.com/project.git/" but not other path_info links)
> > have trailing slash added?
> >
> > Errr... I see that it adds trailing slash only for project-only
> > path_info links, but the commit message was not entirely clear for me.
> 
> If indeed the additional / is only asked for in summary view, I think
> there's no need for a feature toggle, we can always put it there. If
> not, I'm really curious about seeing the rewrite rules (they might
> also be worth adding to the gitweb documentation as examples of 'power
> usage').

The trailing slash is used only when the URL refers to a project with no
appended parameters (i.e., summary view), because the URL refers to the
real git dir on disk (hence, pulling from the same URL) and it plays
nicer with the Web server configuration to have the trailing slash.

I was wary about changing the default behavior, but if you and Jakub
both think it's OK, that's great.

I was thinking of proposing the addition of some info about my setup,
including the rewrite rules, to the documentation.  Maybe we could do
that after dealing with the patches.

-- 
Matt

Re: [PATCH try 2] gitweb: Add option to put a trailing slash on pathinfo-style project URLs

[Jakub Narebski]

On Sun, 14 Dec 2008, Matt McCutchen wrote:
> On Sat, 2008-12-13 at 23:23 +0100, Giuseppe Bilotta wrote:
>> On Sat, Dec 13, 2008 at 10:47 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>>> Matt McCutchen <matt@mattmccutchen.net> writes:
>>>
>>>> My Web site uses pathinfo mode and some rewrite magic to show the gitweb
>>>> interface at the URL of the real repository directory (which users also
>>>> pull from).  In this case, it's desirable to end generated links to the
>>>> project in a trailing slash so the Web server doesn't have to redirect
>>>> the client to add the slash.  This patch adds a second element to the
>>>> "pathinfo" feature configuration to control the trailing slash.
>>>>
>>>> Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>
[...]
>>>
>>> Errr... I see that it adds trailing slash only for project-only
>>> path_info links, but the commit message was not entirely clear for me.
>> 
>> If indeed the additional / is only asked for in summary view, I think
>> there's no need for a feature toggle, we can always put it there. If
>> not, I'm really curious about seeing the rewrite rules (they might
>> also be worth adding to the gitweb documentation as examples of 'power
>> usage').
> 
> The trailing slash is used only when the URL refers to a project with no
> appended parameters (i.e., summary view), because the URL refers to the
> real git dir on disk (hence, pulling from the same URL) and it plays
> nicer with the Web server configuration to have the trailing slash.

It would be nice to have in commit message that we want to have
trailing slash in the cases where URL can correspond to filesystem
path.

But there are two cases: 
 * http://example.com/ corresponding to $projectroot on filesystem,
   and giving projects_list in gitweb
 * http://example.com/project.git/ corresponding to project.git dir
   on filesystem, and giving project summary view in gitweb.

> I was wary about changing the default behavior, but if you and Jakub
> both think it's OK, that's great.

Now I'm not so sure... but I guess the performance cost would be
negligible, and I'm not sure if it would be worth slight complication
in the code (and configuration).

> I was thinking of proposing the addition of some info about my setup,
> including the rewrite rules, to the documentation.  Maybe we could do
> that after dealing with the patches.

Do you plan updating "Webserver configuration" section in 
gitweb/README? 

BTW. could you please check if the $my_uri and $my_link need to be set
in gitweb config for your configuration, or did some of Giuseppe's
path_info improvements took care of that, and it is no longer needed?

-- 
Jakub Narebski
Poland

Re: [PATCH try 2] gitweb: Add option to put a trailing slash on pathinfo-style project URLs

[Jakub Narebski]

Jakub Narebski wrote:
> On Sun, 14 Dec 2008, Matt McCutchen wrote:

> > I was thinking of proposing the addition of some info about my setup,
> > including the rewrite rules, to the documentation.  Maybe we could do
> > that after dealing with the patches.
> 
> Do you plan updating "Webserver configuration" section in 
> gitweb/README? 
> 
> BTW. could you please check if the $my_uri and $my_link need to be set
> in gitweb config for your configuration, or did some of Giuseppe's
> path_info improvements took care of that, and it is no longer needed?

It would be I think good idea to describe there _and_ in the commit
message why would you prefer for gitweb path_info links to be generated
with trailing slash; how it looks resolution for URL with and without
trailing slash. 

-- 
Jakub Narebski
Poland

Re: [PATCH try 2] gitweb: Add option to put a trailing slash on pathinfo-style project URLs

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

>> +	# If you want a trailing slash on the project path (because, for
>> +	# example, you have a real directory at that URL and are using
>> +	# some rewrite magic to invoke gitweb), then set:
>> +	# $feature{'pathinfo'}{'default'} = [1, 1];
>> +
>
> Are any disadvantages to having it always enabled?

Good question.

>> +	my @use_pathinfo = gitweb_get_feature('pathinfo');
>
> Why not name those variables for better readability?
>
> +       my ($use_pathinfo, $trailing_slash) = gitweb_get_feature('pathinfo');

Good suggestion.

Re: [PATCH try 2] gitweb: Add option to put a trailing slash on pathinfo-style project URLs

[Matt McCutchen]

On Sat, 2008-12-13 at 13:47 -0800, Jakub Narebski wrote:
> Errr... I see that it adds trailing slash only for project-only
> path_info links, but the commit message was not entirely clear for me.

I will clarify the message.

> BTW. encoding data in position in array feels a bit hacky to me, but
> I guess that is the limitation of current %feature design, with
> 'default' having to be array (reference).
> 
> >  	# Note that you will need to change the default location of CSS,
> >  	# favicon, logo and possibly other files to an absolute URL. Also,
> >  	# if gitweb.cgi serves as your indexfile, you will need to force
> > @@ -829,8 +834,8 @@ sub href (%) {
> >  		}
> >  	}
> >  
> > -	my $use_pathinfo = gitweb_check_feature('pathinfo');
> > -	if ($use_pathinfo) {
> > +	my @use_pathinfo = gitweb_get_feature('pathinfo');
> 
> Why not name those variables for better readability?
> 
> +       my ($use_pathinfo, $trailing_slash) = gitweb_get_feature('pathinfo');

I'll do that.

> > +	if ($use_pathinfo[0]) {
> >  		# try to put as many parameters as possible in PATH_INFO:
> >  		#   - project name
> >  		#   - action
> > @@ -845,7 +850,12 @@ sub href (%) {
> >  		$href =~ s,/$,,;
> >  
> >  		# Then add the project name, if present
> > -		$href .= "/".esc_url($params{'project'}) if defined $params{'project'};
> > +		my $proj_href = undef;
> > +		if (defined $params{'project'}) {
> > +			$href .= "/".esc_url($params{'project'});
> > +			# Save for trailing-slash check below.
> > +			$proj_href = $href;
> > +		}
> >  		delete $params{'project'};
> >  
> >  		# since we destructively absorb parameters, we keep this
> > @@ -903,6 +913,10 @@ sub href (%) {
> >  			$href .= $known_snapshot_formats{$fmt}{'suffix'};
> >  			delete $params{'snapshot_format'};
> >  		}
> > +
> > +		# If requested in the configuration, add a trailing slash to a URL that
> > +		# has nothing appended after the project path.
> > +		$href .= '/' if ($use_pathinfo[1] && defined $proj_href && $href eq $proj_href);
> >  	}
> 
> The check _feels_ inefficient.  I think (but feel free to disagree) that
> it would be better to use something like $project_pathinfo, set it
> when adding project as pathinfo, and unset if we add anything else as
> pathinfo.

I considered doing that, but I decided that not having to litter the
preceding code with manipulation of $project_pathinfo outweighed
whatever negligible performance difference there might be.

> > +		if ($use_pathinfo[0]) {
> >  			$action .= "/".esc_url($project);
> > +			# Add a trailing slash if requested in the configuration.
> > +			$action .= '/' if ($use_pathinfo[1]);
> 
> Hmmm... let me check something... you rely on the fact that $project
> doesn't end with slash, while I think (but please check it) that it
> can end with slash if it is provided by CGI query.

You are right; in fact, this is already a problem for the strict_export
check.  Gitweb should probably strip trailing slashes when it reads the
"p" parameter.  I will submit a separate patch for that.

-- 
Matt

Re: [PATCH try 2] gitweb: Add option to put a trailing slash on pathinfo-style project URLs

[Jakub Narebski]

On Sun, 14 Dec 2008, Matt McCutchen wrote:
> On Sat, 2008-12-13 at 13:47 -0800, Jakub Narebski wrote:
> >
> > Errr... I see that it adds trailing slash only for project-only
> > path_info links, but the commit message was not entirely clear for me.
> 
> I will clarify the message.

It would be nice (e.g. to have example URL with trailing slash ensured).

[...]
> > > @@ -829,8 +834,8 @@ sub href (%) {
> > >  		}
> > >  	}
> > >  
> > > -	my $use_pathinfo = gitweb_check_feature('pathinfo');
> > > -	if ($use_pathinfo) {
> > > +	my @use_pathinfo = gitweb_get_feature('pathinfo');
> > 
> > Why not name those variables for better readability?
> > 
> > +       my ($use_pathinfo, $trailing_slash) = gitweb_get_feature('pathinfo');
> 
> I'll do that.

Note that you wouldn't need that if you decide that it makes sense
(and doesn't have disadvantages) to *always* add trailing slash to
the end of path_info for some kinds of gitweb links.

> > > +	if ($use_pathinfo[0]) {
> > >  		# try to put as many parameters as possible in PATH_INFO:
> > >  		#   - project name
> > >  		#   - action
> > > @@ -845,7 +850,12 @@ sub href (%) {
> > >  		$href =~ s,/$,,;
> > >  
> > >  		# Then add the project name, if present
> > > -		$href .= "/".esc_url($params{'project'}) if defined $params{'project'};
> > > +		my $proj_href = undef;
> > > +		if (defined $params{'project'}) {
> > > +			$href .= "/".esc_url($params{'project'});
> > > +			# Save for trailing-slash check below.
> > > +			$proj_href = $href;
> > > +		}
> > >  		delete $params{'project'};
> > >  
> > >  		# since we destructively absorb parameters, we keep this
> > > @@ -903,6 +913,10 @@ sub href (%) {
> > >  			$href .= $known_snapshot_formats{$fmt}{'suffix'};
> > >  			delete $params{'snapshot_format'};
> > >  		}
> > > +
> > > +		# If requested in the configuration, add a trailing slash to a URL that
> > > +		# has nothing appended after the project path.
> > > +		$href .= '/' if ($use_pathinfo[1] && defined $proj_href && $href eq $proj_href);
> > >  	}
> > 
> > The check _feels_ inefficient.  I think (but feel free to disagree) that
> > it would be better to use something like $project_pathinfo, set it
> > when adding project as pathinfo, and unset if we add anything else as
> > pathinfo.
> 
> I considered doing that, but I decided that not having to litter the
> preceding code with manipulation of $project_pathinfo outweighed
> whatever negligible performance difference there might be.

On the other hand, with having boolean variable named for example
$trailing_slash or $add_trailing_slash, you can set it to appropriate
value by default (should project list URL: http://example.com/ have
trailing slash), and at [almost] each 'delete $params{<param>}' either
set it to true, or set it to false. This way it would be easy to
extend to have trailing slash also for example for OPML link
http://example.com/opml/ or not have it and use http://example.com/opml

I think it is not only more efficient, but is also more flexible.
Admittedly it is also more complicated...
 
> > > +		if ($use_pathinfo[0]) {
> > >  			$action .= "/".esc_url($project);
> > > +			# Add a trailing slash if requested in the configuration.
> > > +			$action .= '/' if ($use_pathinfo[1]);
> > 
> > Hmmm... let me check something... you rely on the fact that $project
> > doesn't end with slash, while I think (but please check it) that it
> > can end with slash if it is provided by CGI query.
> 
> You are right; in fact, this is already a problem for the strict_export
> check.  Gitweb should probably strip trailing slashes when it reads the
> "p" parameter.  I will submit a separate patch for that.

That would be nice. TIA.

-- 
Jakub Narebski
Poland

Re: [PATCH try 2] gitweb: Add option to put a trailing slash on pathinfo-style project URLs

[Jakub Narebski]

On Mon, 15 Dec 2008, Jakub Narebski wrote:
> On Sun, 14 Dec 2008, Matt McCutchen wrote:
>> On Sat, 2008-12-13 at 13:47 -0800, Jakub Narebski wrote:

>>>> @@ -845,7 +850,12 @@ sub href (%) {
>>>>  		$href =~ s,/$,,;
>>>>  
>>>>  		# Then add the project name, if present
>>>> -		$href .= "/".esc_url($params{'project'}) if defined $params{'project'};
>>>> +		my $proj_href = undef;
>>>> +		if (defined $params{'project'}) {
>>>> +			$href .= "/".esc_url($params{'project'});
>>>> +			# Save for trailing-slash check below.
>>>> +			$proj_href = $href;
>>>> +		}
>>>>  		delete $params{'project'};
>>>>  
>>>>  		# since we destructively absorb parameters, we keep this
>>>> @@ -903,6 +913,10 @@ sub href (%) {
>>>>  			$href .= $known_snapshot_formats{$fmt}{'suffix'};
>>>>  			delete $params{'snapshot_format'};
>>>>  		}
>>>> +
>>>> +		# If requested in the configuration, add a trailing slash to a URL that
>>>> +		# has nothing appended after the project path.
>>>> +		$href .= '/' if ($use_pathinfo[1] && defined $proj_href && $href eq $proj_href);
>>>>  	}
>>> 
>>> The check _feels_ inefficient.  I think (but feel free to disagree) that
>>> it would be better to use something like $project_pathinfo, set it
>>> when adding project as pathinfo, and unset if we add anything else as
>>> pathinfo.
>> 
>> I considered doing that, but I decided that not having to litter the
>> preceding code with manipulation of $project_pathinfo outweighed
>> whatever negligible performance difference there might be.
> 
> On the other hand, with having boolean variable named for example
> $trailing_slash or $add_trailing_slash, you can set it to appropriate
> value by default (should project list URL: http://example.com/ have
> trailing slash), and at [almost] each 'delete $params{<param>}' either
> set it to true, or set it to false. This way it would be easy to
> extend to have trailing slash also for example for OPML link
> http://example.com/opml/ or not have it and use http://example.com/opml
> 
> I think it is not only more efficient, but is also more flexible.
> Admittedly it is also more complicated...

On the other hand you don't need such flexibility, so perhaps simpler
code (or at least less changes) outweights this issue...

-- 
Jakub Narebski
Poland