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

From: Matt McCutchen <matt@mattmccutchen.net>
Date: Dec 14, 2008, 01:43 UTC
Message-ID: <1229219030.3360.44.camel@mattlaptop2.local>
In-Reply-To: <m3tz97g329.fsf@localhost.localdomain>

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.

Show 18 quoted lines

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

Show 32 quoted lines

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

Show 8 quoted lines

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