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@gmail.com>
Otherwise, for what it is worth:
Acked-by: Jakub Narebski <jnareb@gmail.com>
Show 7 quoted lines
> ---
> 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.
Show 27 quoted lines
>
> 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
Giuseppe Bilotta
