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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
