* [PATCH] gitweb: suppress 'Use of uninitialized value' error @ 2009-07-30 21:15 Giuseppe Bilotta 2009-07-30 23:00 ` Jakub Narebski 0 siblings, 1 reply; 9+ messages in thread From: Giuseppe Bilotta @ 2009-07-30 21:15 UTC (permalink / raw) To: git; +Cc: Jakub Narebski, Junio C Hamano, Petr Baudis, Giuseppe Bilotta Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com> --- 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. 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] gitweb: suppress 'Use of uninitialized value' error 2009-07-30 21:15 [PATCH] gitweb: suppress 'Use of uninitialized value' error Giuseppe Bilotta @ 2009-07-30 23:00 ` Jakub Narebski 2009-07-31 6:06 ` Giuseppe Bilotta 0 siblings, 1 reply; 9+ messages in thread From: Jakub Narebski @ 2009-07-30 23:00 UTC (permalink / raw) To: Giuseppe Bilotta; +Cc: git, Junio C Hamano, Petr Baudis 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> > --- > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gitweb: suppress 'Use of uninitialized value' error 2009-07-30 23:00 ` Jakub Narebski @ 2009-07-31 6:06 ` Giuseppe Bilotta 2009-07-31 6:24 ` Jakub Narebski 0 siblings, 1 reply; 9+ messages in thread From: Giuseppe Bilotta @ 2009-07-31 6:06 UTC (permalink / raw) To: Jakub Narebski; +Cc: git, Junio C Hamano, Petr Baudis 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gitweb: suppress 'Use of uninitialized value' error 2009-07-31 6:06 ` Giuseppe Bilotta @ 2009-07-31 6:24 ` Jakub Narebski 2009-07-31 6:48 ` [PATCH] gitweb: fix 'Use of uninitialized value' error in href() Giuseppe Bilotta 0 siblings, 1 reply; 9+ messages in thread From: Jakub Narebski @ 2009-07-31 6:24 UTC (permalink / raw) To: Giuseppe Bilotta; +Cc: git, Junio C Hamano, Petr Baudis Giuseppe Bilotta wrote: > 2009/7/31 Jakub Narebski <jnareb@gmail.com>: > > 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? Yes, of course it is worth protecting against this case (better checking if both sides of 'eq' comparison are defined). We want to return error message from gitweb if URL is borked, not have Perl error in logs. -- Jakub Narebski Poland ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] gitweb: fix 'Use of uninitialized value' error in href() 2009-07-31 6:24 ` Jakub Narebski @ 2009-07-31 6:48 ` Giuseppe Bilotta 2009-07-31 8:30 ` Jakub Narebski 0 siblings, 1 reply; 9+ messages in thread From: Giuseppe Bilotta @ 2009-07-31 6:48 UTC (permalink / raw) To: git; +Cc: Jakub Narebski, Junio C Hamano, Petr Baudis, Giuseppe Bilotta Equality between file_parent and file_name was being checked without a preliminary check for existence of the parameters. Fix by wrapping the equality check in appropriate if (defined ...), rearranging the lines to prevent excessive length. Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com> --- gitweb/gitweb.perl | 11 +++++++---- 1 files changed, 7 insertions(+), 4 deletions(-) The funny thing is that I seem to get the error for something as simple as a commit view (_any_ commit view, for the matter), but I wasn't able to reproduce it from the shell, which is why I'm not adding a testcase. diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 7fbd5ff..37120a3 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 (defined $params{'file_name'} && $params{'file_parent'} eq $params{'file_name'}) { + delete $params{'file_parent'}; + } elsif ($params{'file_parent'} !~ /\.\./) { + $href .= ":/".esc_url($params{'file_parent'}); + delete $params{'file_parent'}; + } } $href .= ".."; delete $params{'hash_parent'}; -- 1.6.3.rc1.192.gdbfcb ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] gitweb: fix 'Use of uninitialized value' error in href() 2009-07-31 6:48 ` [PATCH] gitweb: fix 'Use of uninitialized value' error in href() Giuseppe Bilotta @ 2009-07-31 8:30 ` Jakub Narebski 2009-07-31 8:37 ` Giuseppe Bilotta 2009-08-04 7:26 ` Junio C Hamano 0 siblings, 2 replies; 9+ messages in thread From: Jakub Narebski @ 2009-07-31 8:30 UTC (permalink / raw) To: Giuseppe Bilotta; +Cc: git, Junio C Hamano, Petr Baudis On Fri, 31 July 2009, Giuseppe Bilotta wrote: > Equality between file_parent and file_name was being checked without a > preliminary check for existence of the parameters. > > Fix by wrapping the equality check in appropriate if (defined ...), > rearranging the lines to prevent excessive length. > > Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com> For what it is worth: Acked-by: Jakub Narebski <jnareb@gmail.com> > --- > gitweb/gitweb.perl | 11 +++++++---- > 1 files changed, 7 insertions(+), 4 deletions(-) > > The funny thing is that I seem to get the error for something as simple > as a commit view (_any_ commit view, for the matter), but I wasn't able > to reproduce it from the shell, which is why I'm not adding a testcase. Gaaah! A Heisenbug! ;-) Well, it worth fixing if it is real case scenario. > > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl > index 7fbd5ff..37120a3 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 (defined $params{'file_name'} && $params{'file_parent'} eq $params{'file_name'}) { or, to reduce line length even more: + if (defined $params{'file_name'} && + $params{'file_parent'} eq $params{'file_name'}) { > + delete $params{'file_parent'}; > + } elsif ($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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gitweb: fix 'Use of uninitialized value' error in href() 2009-07-31 8:30 ` Jakub Narebski @ 2009-07-31 8:37 ` Giuseppe Bilotta 2009-07-31 14:51 ` Junio C Hamano 2009-08-04 7:26 ` Junio C Hamano 1 sibling, 1 reply; 9+ messages in thread From: Giuseppe Bilotta @ 2009-07-31 8:37 UTC (permalink / raw) To: Jakub Narebski; +Cc: git, Junio C Hamano, Petr Baudis 2009/7/31 Jakub Narebski <jnareb@gmail.com>: > On Fri, 31 July 2009, Giuseppe Bilotta wrote: >> >> The funny thing is that I seem to get the error for something as simple >> as a commit view (_any_ commit view, for the matter), but I wasn't able >> to reproduce it from the shell, which is why I'm not adding a testcase. > > Gaaah! A Heisenbug! ;-) Well, I can reproduce it consistently on the gitweb repository, with any shortlog or commit view. I also think I understand why it happens (i.e. in some cases the check for file_parent eq file_name is done before calling href). The question, of course, is why wouldn't it happen in the test repository. >> + if (defined $params{'file_parent'}) { >> + if (defined $params{'file_name'} && $params{'file_parent'} eq $params{'file_name'}) { > > or, to reduce line length even more: > > + if (defined $params{'file_name'} && > + $params{'file_parent'} eq $params{'file_name'}) { Oh right ... should I resend? -- Giuseppe "Oblomov" Bilotta ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gitweb: fix 'Use of uninitialized value' error in href() 2009-07-31 8:37 ` Giuseppe Bilotta @ 2009-07-31 14:51 ` Junio C Hamano 0 siblings, 0 replies; 9+ messages in thread From: Junio C Hamano @ 2009-07-31 14:51 UTC (permalink / raw) To: Giuseppe Bilotta; +Cc: Jakub Narebski, git, Petr Baudis Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes: > 2009/7/31 Jakub Narebski <jnareb@gmail.com>: >> On Fri, 31 July 2009, Giuseppe Bilotta wrote: >>> >>> The funny thing is that I seem to get the error for something as simple >>> as a commit view (_any_ commit view, for the matter), but I wasn't able >>> to reproduce it from the shell, which is why I'm not adding a testcase. >> >> Gaaah! A Heisenbug! ;-) > > Well, I can reproduce it consistently on the gitweb repository, with > any shortlog or commit view. I also think I understand why it happens > (i.e. in some cases the check for file_parent eq file_name is done > before calling href). The question, of course, is why wouldn't it > happen in the test repository. That sounds like something worth figuring out, independent of the issue this patch addresses. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gitweb: fix 'Use of uninitialized value' error in href() 2009-07-31 8:30 ` Jakub Narebski 2009-07-31 8:37 ` Giuseppe Bilotta @ 2009-08-04 7:26 ` Junio C Hamano 1 sibling, 0 replies; 9+ messages in thread From: Junio C Hamano @ 2009-08-04 7:26 UTC (permalink / raw) To: Jakub Narebski; +Cc: Giuseppe Bilotta, git, Petr Baudis Jakub Narebski <jnareb@gmail.com> writes: > Acked-by: Jakub Narebski <jnareb@gmail.com> Thanks, both. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2009-08-04 7:26 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-07-30 21:15 [PATCH] gitweb: suppress 'Use of uninitialized value' error Giuseppe Bilotta 2009-07-30 23:00 ` Jakub Narebski 2009-07-31 6:06 ` Giuseppe Bilotta 2009-07-31 6:24 ` Jakub Narebski 2009-07-31 6:48 ` [PATCH] gitweb: fix 'Use of uninitialized value' error in href() Giuseppe Bilotta 2009-07-31 8:30 ` Jakub Narebski 2009-07-31 8:37 ` Giuseppe Bilotta 2009-07-31 14:51 ` Junio C Hamano 2009-08-04 7:26 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).