2009/7/31 Jakub Narebski <jnareb@xxxxxxxxx>: > 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 -- 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