Re: [PATCH try 2] gitweb: Add option to put a trailing slash on pathinfo-style project URLs
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Dec 14, 2008, 23:20 UTC
- Message-ID
- <200812150020.53370.jnareb@gmail.com>
- In-Reply-To
- <1229219030.3360.44.camel@mattlaptop2.local>
On Sun, 14 Dec 2008, Matt McCutchen wrote:
Show 6 quoted lines
> 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).
[...]
Show 13 quoted lines
> > > @@ -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.
Show 36 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.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...
Show 12 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.That would be nice. TIA.
-- Jakub Narebski Poland