Re: [PATCH] gitweb: suppress 'Use of uninitialized value' error

[Giuseppe Bilotta <giuseppe.bilotta@gmail.com>]

2009/7/31 Jakub Narebski <jnareb@gmail.com>:
> On Thu, 30 July 2009, Giuseppe Bilotta wrote:
>
> Thanks for catching this.
>
> However... First, the subject should be more specific and less generic.
> At least provide where 'Use of uninitialized' value was generated, e.g.:
>
>  Subject: [PATCH] gitweb: Fix 'Use of uninitialized value' error in href()
>
> or a bit shorter
>
>  Subject: [PATCH] gitweb: Fix 'Use of uninitialized value' in href()

Agreed.

> Second, it would be nice to have more detailed description in the body
> of a commit message, in this case stating when mentioned error occur:
>
>  This warning was generated when 'hash_parent_base' parameter was
>  defined, and either 'file_name' or 'file_parent' was not defined.
>
> Note that from above description you can notice that you protect against
> 'file_parent' being not defined, but not against 'file_name', so you
> should I think write why it is not a problem:
>
>  In gitweb code 'file_parent' is used only if 'file_name' is filled.
>
> Well... almost.  This does not cover href(..., -replay=>1) with hand
> crafted broken/invalid gitweb URL.  BTW. a question for you: how did
> you detect/found this breakage?

I was looking at the error log of my webserver and spotted the lines,
coming from a spider indexing my git pages. An URL that caused it is
http://git.oblomov.eu/git/commit/32ae83194b0f287a9b6644cdad175c56417c31f3
(the tree link, I suspect).

> I don't think gitweb generates such
> broken links (with 'hash_parent_base' but not 'file_parent') normally,
> but I might be mistaken.

Do you think it would be worth to protect against this case?

> Third, we would probably want to have additional case in t/t9500 test
> to protect against regression here.  But that is not as important,
> I think.

I'll see if I can cook that up.

>>  gitweb/gitweb.perl |   11 +++++++----
>>  1 files changed, 7 insertions(+), 4 deletions(-)
>>
>> The patch could have been a one-liner by adding the defined check at
>> line 943, but that pushed the line to 120 char, so I decided for this
>> slightly more complex form.
>
> That is IMHO a good solution.  Better not abuse 'if' modifier form.
>
>>
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 7fbd5ff..c7f257e 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -940,10 +940,13 @@ sub href {
>>                       if (defined $params{'hash_parent_base'}) {
>>                               $href .= esc_url($params{'hash_parent_base'});
>>                               # skip the file_parent if it's the same as the file_name
>> -                             delete $params{'file_parent'} if $params{'file_parent'} eq $params{'file_name'};
>> -                             if (defined $params{'file_parent'} && $params{'file_parent'} !~ /\.\./) {
>> -                                     $href .= ":/".esc_url($params{'file_parent'});
>> -                                     delete $params{'file_parent'};
>> +                             if (defined $params{'file_parent'}) {
>> +                                     if ($params{'file_parent'} eq $params{'file_name'}) {
>> +                                             delete $params{'file_parent'};
>> +                                     } else if ($params{'file_parent'} !~ /\.\./) {

And I'm an idiot 'cause this should be 'elsif'.

Resend coming soon.

-- 
Giuseppe "Oblomov" Bilotta