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