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

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

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 don't think gitweb generates such
broken links (with 'hash_parent_base' but not 'file_parent') normally,
but I might be mistaken.

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.

> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@xxxxxxxxx>

Otherwise, for what it is worth:

Acked-by: Jakub Narebski <jnareb@xxxxxxxxx>

> ---
>  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'} !~ 
> /\.\./) {
> +                                             $href .= 
> ":/".esc_url($params{'file_parent'});
> +                                             delete $params{'file_parent'};
> +                                     }
>                               }
>                               $href .= "..";
>                               delete $params{'hash_parent'};
> -- 
> 1.6.3.rc1.192.gdbfcb
> 
> 

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