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:58 UTC
- Message-ID
- <200812150058.36038.jnareb@gmail.com>
- In-Reply-To
- <200812150020.53370.jnareb@gmail.com>
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:
Show 43 quoted lines
>>>> @@ -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