Re: [PATCHv4] gitweb: generate project/action/hash URLs

From: Jakub Narebski <jnareb@gmail.com>
Date: Oct 3, 2008, 11:24 UTC
Message-ID: <200810031324.53811.jnareb@gmail.com>
In-Reply-To: <cb7bb73a0810022330l498bdb20h703dec7833a443e@mail.gmail.com>

On Fri, 3 October 2008, Giuseppe Bilotta wrote:

> On Fri, Oct 3, 2008 at 3:48 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Tue, 2 Oct 2008, Giuseppe Bilotta wrote:

Show 23 quoted lines

>>> +             # Finally, we put either hash_base:file_name or hash
>>> +             if (defined $params{'hash_base'}) {
>>> +                     $href .= "/".esc_url($params{'hash_base'});
>>> +                     if (defined $params{'file_name'}) {
>>> +                             $href .= ":".esc_url($params{'file_name'});
>>> +                             delete $params{'file_name'};
>>> +                     }
>>> +                     delete $params{'hash'};
>>> +                     delete $params{'hash_base'};
>>> +             } elsif (defined $params{'hash'}) {
>>> +                     $href .= "/".esc_url($params{'hash'});
>>> +                     delete $params{'hash'};
>>> +             }
>>
>> Hmmmm...
>>
>> Shouldn't the code first check for $file_name, then add either
>> "$hash_base:$file_name" (url-escaped), or "$hash" (not "$hash_base")?
> 
> Hm, your idea is probably better indeed, if we can ensure that
> $file_name is always set for generated links that need $hash_base (I
> mean, as opposed to the root tree case we were discussing for the
> first patch).

Hmm... I'm just worrying here about diluting meaning of params passed via path_info. We had either project/hash, or project/hash_base:file_name; now the hashy parameter in path_info can be hash or hash_base and we don't know which. But perhaps I am worrying over nothing...

A short comment though would be nice (that we can have $hash or $hash_base for case without $file_name)

-- 
Jakub Narebski
Poland