{"thread":{"id":"20301","subject":"[PATCH] gitweb: suppress 'Use of uninitialized value' error","startedAt":"2009-07-30T21:15:40Z","lastAt":"2009-08-04T07:26:37Z","messageCount":9,"participants":["Giuseppe Bilotta","Jakub Narebski","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"119192","messageId":"1248988540-8971-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"20301","inReplyTo":null,"subject":"[PATCH] gitweb: suppress 'Use of uninitialized value' error","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-07-30T21:15:40Z","receivedAt":"2009-07-30T21:15:40Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>\n---\n gitweb/gitweb.perl |   11 +++++++----\n 1 files changed, 7 insertions(+), 4 deletions(-)\n\nThe patch could have been a one-liner by adding the defined check at\nline 943, but that pushed the line to 120 char, so I decided for this\nslightly more complex form.\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 7fbd5ff..c7f257e 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -940,10 +940,13 @@ sub href {\n \t\t\tif (defined $params{'hash_parent_base'}) {\n \t\t\t\t$href .= esc_url($params{'hash_parent_base'});\n \t\t\t\t# skip the file_parent if it's the same as the file_name\n-\t\t\t\tdelete $params{'file_parent'} if $params{'file_parent'} eq $params{'file_name'};\n-\t\t\t\tif (defined $params{'file_parent'} && $params{'file_parent'} !~ /\\.\\./) {\n-\t\t\t\t\t$href .= \":/\".esc_url($params{'file_parent'});\n-\t\t\t\t\tdelete $params{'file_parent'};\n+\t\t\t\tif (defined $params{'file_parent'}) {\n+\t\t\t\t\tif ($params{'file_parent'} eq $params{'file_name'}) {\n+\t\t\t\t\t\tdelete $params{'file_parent'};\n+\t\t\t\t\t} else if ($params{'file_parent'} !~ /\\.\\./) {\n+\t\t\t\t\t\t$href .= \":/\".esc_url($params{'file_parent'});\n+\t\t\t\t\t\tdelete $params{'file_parent'};\n+\t\t\t\t\t}\n \t\t\t\t}\n \t\t\t\t$href .= \"..\";\n \t\t\t\tdelete $params{'hash_parent'};\n-- \n1.6.3.rc1.192.gdbfcb\n"},{"id":"119194","messageId":"200907310100.42786.jnareb@gmail.com","threadId":"20301","inReplyTo":"1248988540-8971-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH] gitweb: suppress 'Use of uninitialized value' error","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-07-30T23:00:40Z","receivedAt":"2009-07-30T23:00:40Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 30 July 2009, Giuseppe Bilotta wrote:\n\nThanks for catching this.\n\nHowever... First, the subject should be more specific and less generic.  \nAt least provide where 'Use of uninitialized' value was generated, e.g.:\n\n  Subject: [PATCH] gitweb: Fix 'Use of uninitialized value' error in href()\n\nor a bit shorter\n\n  Subject: [PATCH] gitweb: Fix 'Use of uninitialized value' in href()\n\nSecond, it would be nice to have more detailed description in the body\nof a commit message, in this case stating when mentioned error occur:\n\n  This warning was generated when 'hash_parent_base' parameter was\n  defined, and either 'file_name' or 'file_parent' was not defined.\n\nNote that from above description you can notice that you protect against\n'file_parent' being not defined, but not against 'file_name', so you\nshould I think write why it is not a problem:\n\n  In gitweb code 'file_parent' is used only if 'file_name' is filled.\n\nWell... almost.  This does not cover href(..., -replay=>1) with hand\ncrafted broken/invalid gitweb URL.  BTW. a question for you: how did\nyou detect/found this breakage?  I don't think gitweb generates such\nbroken links (with 'hash_parent_base' but not 'file_parent') normally,\nbut I might be mistaken.\n\nThird, we would probably want to have additional case in t/t9500 test\nto protect against regression here.  But that is not as important,\nI think.\n\n> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>\n\nOtherwise, for what it is worth:\n\nAcked-by: Jakub Narebski <jnareb@gmail.com>\n\n> ---\n>  gitweb/gitweb.perl |   11 +++++++----\n>  1 files changed, 7 insertions(+), 4 deletions(-)\n> \n> The patch could have been a one-liner by adding the defined check at\n> line 943, but that pushed the line to 120 char, so I decided for this\n> slightly more complex form.\n\nThat is IMHO a good solution.  Better not abuse 'if' modifier form.\n\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 7fbd5ff..c7f257e 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -940,10 +940,13 @@ sub href {\n>  \t\t\tif (defined $params{'hash_parent_base'}) {\n>  \t\t\t\t$href .= esc_url($params{'hash_parent_base'});\n>  \t\t\t\t# skip the file_parent if it's the same as the file_name\n> -\t\t\t\tdelete $params{'file_parent'} if $params{'file_parent'} eq $params{'file_name'};\n> -\t\t\t\tif (defined $params{'file_parent'} && $params{'file_parent'} !~ /\\.\\./) {\n> -\t\t\t\t\t$href .= \":/\".esc_url($params{'file_parent'});\n> -\t\t\t\t\tdelete $params{'file_parent'};\n> +\t\t\t\tif (defined $params{'file_parent'}) {\n> +\t\t\t\t\tif ($params{'file_parent'} eq $params{'file_name'}) {\n> +\t\t\t\t\t\tdelete $params{'file_parent'};\n> +\t\t\t\t\t} else if ($params{'file_parent'} !~ /\\.\\./) {\n> +\t\t\t\t\t\t$href .= \":/\".esc_url($params{'file_parent'});\n> +\t\t\t\t\t\tdelete $params{'file_parent'};\n> +\t\t\t\t\t}\n>  \t\t\t\t}\n>  \t\t\t\t$href .= \"..\";\n>  \t\t\t\tdelete $params{'hash_parent'};\n> -- \n> 1.6.3.rc1.192.gdbfcb\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"119214","messageId":"cb7bb73a0907302306k691b1d4fy85d7b460077bc639@mail.gmail.com","threadId":"20301","inReplyTo":"200907310100.42786.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: suppress 'Use of uninitialized value' error","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-07-31T06:06:27Z","receivedAt":"2009-07-31T06:06:27Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2009/7/31 Jakub Narebski <jnareb@gmail.com>:\n> On Thu, 30 July 2009, Giuseppe Bilotta wrote:\n>\n> Thanks for catching this.\n>\n> However... First, the subject should be more specific and less generic.\n> At least provide where 'Use of uninitialized' value was generated, e.g.:\n>\n>  Subject: [PATCH] gitweb: Fix 'Use of uninitialized value' error in href()\n>\n> or a bit shorter\n>\n>  Subject: [PATCH] gitweb: Fix 'Use of uninitialized value' in href()\n\nAgreed.\n\n> Second, it would be nice to have more detailed description in the body\n> of a commit message, in this case stating when mentioned error occur:\n>\n>  This warning was generated when 'hash_parent_base' parameter was\n>  defined, and either 'file_name' or 'file_parent' was not defined.\n>\n> Note that from above description you can notice that you protect against\n> 'file_parent' being not defined, but not against 'file_name', so you\n> should I think write why it is not a problem:\n>\n>  In gitweb code 'file_parent' is used only if 'file_name' is filled.\n>\n> Well... almost.  This does not cover href(..., -replay=>1) with hand\n> crafted broken/invalid gitweb URL.  BTW. a question for you: how did\n> you detect/found this breakage?\n\nI was looking at the error log of my webserver and spotted the lines,\ncoming from a spider indexing my git pages. An URL that caused it is\nhttp://git.oblomov.eu/git/commit/32ae83194b0f287a9b6644cdad175c56417c31f3\n(the tree link, I suspect).\n\n> I don't think gitweb generates such\n> broken links (with 'hash_parent_base' but not 'file_parent') normally,\n> but I might be mistaken.\n\nDo you think it would be worth to protect against this case?\n\n> Third, we would probably want to have additional case in t/t9500 test\n> to protect against regression here.  But that is not as important,\n> I think.\n\nI'll see if I can cook that up.\n\n>>  gitweb/gitweb.perl |   11 +++++++----\n>>  1 files changed, 7 insertions(+), 4 deletions(-)\n>>\n>> The patch could have been a one-liner by adding the defined check at\n>> line 943, but that pushed the line to 120 char, so I decided for this\n>> slightly more complex form.\n>\n> That is IMHO a good solution.  Better not abuse 'if' modifier form.\n>\n>>\n>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>> index 7fbd5ff..c7f257e 100755\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -940,10 +940,13 @@ sub href {\n>>                       if (defined $params{'hash_parent_base'}) {\n>>                               $href .= esc_url($params{'hash_parent_base'});\n>>                               # skip the file_parent if it's the same as the file_name\n>> -                             delete $params{'file_parent'} if $params{'file_parent'} eq $params{'file_name'};\n>> -                             if (defined $params{'file_parent'} && $params{'file_parent'} !~ /\\.\\./) {\n>> -                                     $href .= \":/\".esc_url($params{'file_parent'});\n>> -                                     delete $params{'file_parent'};\n>> +                             if (defined $params{'file_parent'}) {\n>> +                                     if ($params{'file_parent'} eq $params{'file_name'}) {\n>> +                                             delete $params{'file_parent'};\n>> +                                     } else if ($params{'file_parent'} !~ /\\.\\./) {\n\nAnd I'm an idiot 'cause this should be 'elsif'.\n\nResend coming soon.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"119215","messageId":"200907310824.42953.jnareb@gmail.com","threadId":"20301","inReplyTo":"cb7bb73a0907302306k691b1d4fy85d7b460077bc639@mail.gmail.com","subject":"Re: [PATCH] gitweb: suppress 'Use of uninitialized value' error","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-07-31T06:24:41Z","receivedAt":"2009-07-31T06:24:41Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Giuseppe Bilotta wrote:\n> 2009/7/31 Jakub Narebski <jnareb@gmail.com>:\n\n> > I don't think gitweb generates such\n> > broken links (with 'hash_parent_base' but not 'file_parent') normally,\n> > but I might be mistaken.\n> \n> Do you think it would be worth to protect against this case?\n\nYes, of course it is worth protecting against this case (better checking\nif both sides of 'eq' comparison are defined).  We want to return error\nmessage from gitweb if URL is borked, not have Perl error in logs.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"119216","messageId":"1249022929-21037-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"20301","inReplyTo":"200907310824.42953.jnareb@gmail.com","subject":"[PATCH] gitweb: fix 'Use of uninitialized value' error in href()","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-07-31T06:48:49Z","receivedAt":"2009-07-31T06:48:49Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Equality between file_parent and file_name was being checked without a\npreliminary check for existence of the parameters.\n\nFix by wrapping the equality check in appropriate if (defined ...),\nrearranging the lines to prevent excessive length.\n\nSigned-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>\n---\n gitweb/gitweb.perl |   11 +++++++----\n 1 files changed, 7 insertions(+), 4 deletions(-)\n\nThe funny thing is that I seem to get the error for something as simple\nas a commit view (_any_ commit view, for the matter), but I wasn't able\nto reproduce it from the shell, which is why I'm not adding a testcase.\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 7fbd5ff..37120a3 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -940,10 +940,13 @@ sub href {\n \t\t\tif (defined $params{'hash_parent_base'}) {\n \t\t\t\t$href .= esc_url($params{'hash_parent_base'});\n \t\t\t\t# skip the file_parent if it's the same as the file_name\n-\t\t\t\tdelete $params{'file_parent'} if $params{'file_parent'} eq $params{'file_name'};\n-\t\t\t\tif (defined $params{'file_parent'} && $params{'file_parent'} !~ /\\.\\./) {\n-\t\t\t\t\t$href .= \":/\".esc_url($params{'file_parent'});\n-\t\t\t\t\tdelete $params{'file_parent'};\n+\t\t\t\tif (defined $params{'file_parent'}) {\n+\t\t\t\t\tif (defined $params{'file_name'} && $params{'file_parent'} eq $params{'file_name'}) {\n+\t\t\t\t\t\tdelete $params{'file_parent'};\n+\t\t\t\t\t} elsif ($params{'file_parent'} !~ /\\.\\./) {\n+\t\t\t\t\t\t$href .= \":/\".esc_url($params{'file_parent'});\n+\t\t\t\t\t\tdelete $params{'file_parent'};\n+\t\t\t\t\t}\n \t\t\t\t}\n \t\t\t\t$href .= \"..\";\n \t\t\t\tdelete $params{'hash_parent'};\n-- \n1.6.3.rc1.192.gdbfcb\n"},{"id":"119220","messageId":"200907311030.52643.jnareb@gmail.com","threadId":"20301","inReplyTo":"1249022929-21037-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH] gitweb: fix 'Use of uninitialized value' error in href()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-07-31T08:30:50Z","receivedAt":"2009-07-31T08:30:50Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 31 July 2009, Giuseppe Bilotta wrote:\n\n> Equality between file_parent and file_name was being checked without a\n> preliminary check for existence of the parameters.\n> \n> Fix by wrapping the equality check in appropriate if (defined ...),\n> rearranging the lines to prevent excessive length.\n> \n> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>\n\nFor what it is worth:\n\nAcked-by: Jakub Narebski <jnareb@gmail.com>\n\n> ---\n>  gitweb/gitweb.perl |   11 +++++++----\n>  1 files changed, 7 insertions(+), 4 deletions(-)\n> \n> The funny thing is that I seem to get the error for something as simple\n> as a commit view (_any_ commit view, for the matter), but I wasn't able\n> to reproduce it from the shell, which is why I'm not adding a testcase.\n\nGaaah! A Heisenbug! ;-)\n\nWell, it worth fixing if it is real case scenario.\n\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 7fbd5ff..37120a3 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -940,10 +940,13 @@ sub href {\n>  \t\t\tif (defined $params{'hash_parent_base'}) {\n>  \t\t\t\t$href .= esc_url($params{'hash_parent_base'});\n>  \t\t\t\t# skip the file_parent if it's the same as the file_name\n> -\t\t\t\tdelete $params{'file_parent'} if $params{'file_parent'} eq $params{'file_name'};\n> -\t\t\t\tif (defined $params{'file_parent'} && $params{'file_parent'} !~ /\\.\\./) {\n> -\t\t\t\t\t$href .= \":/\".esc_url($params{'file_parent'});\n> -\t\t\t\t\tdelete $params{'file_parent'};\n> +\t\t\t\tif (defined $params{'file_parent'}) {\n> +\t\t\t\t\tif (defined $params{'file_name'} && $params{'file_parent'} eq $params{'file_name'}) {\n\nor, to reduce line length even more:\n\n  +\t\t\t\t\tif (defined $params{'file_name'} &&\n  +\t\t\t\t\t    $params{'file_parent'} eq $params{'file_name'}) {\n\n> +\t\t\t\t\t\tdelete $params{'file_parent'};\n> +\t\t\t\t\t} elsif ($params{'file_parent'} !~ /\\.\\./) {\n> +\t\t\t\t\t\t$href .= \":/\".esc_url($params{'file_parent'});\n> +\t\t\t\t\t\tdelete $params{'file_parent'};\n> +\t\t\t\t\t}\n>  \t\t\t\t}\n>  \t\t\t\t$href .= \"..\";\n>  \t\t\t\tdelete $params{'hash_parent'};\n> -- \n> 1.6.3.rc1.192.gdbfcb\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"119221","messageId":"cb7bb73a0907310137o6adb57a5tb05b6745b41b0f01@mail.gmail.com","threadId":"20301","inReplyTo":"200907311030.52643.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: fix 'Use of uninitialized value' error in href()","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-07-31T08:37:42Z","receivedAt":"2009-07-31T08:37:42Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2009/7/31 Jakub Narebski <jnareb@gmail.com>:\n> On Fri, 31 July 2009, Giuseppe Bilotta wrote:\n>>\n>> The funny thing is that I seem to get the error for something as simple\n>> as a commit view (_any_ commit view, for the matter), but I wasn't able\n>> to reproduce it from the shell, which is why I'm not adding a testcase.\n>\n> Gaaah! A Heisenbug! ;-)\n\nWell, I can reproduce it consistently on the gitweb repository, with\nany shortlog or commit view. I also think I understand why it happens\n(i.e. in some cases the check for file_parent eq file_name is done\nbefore calling href). The question, of course, is why wouldn't it\nhappen in the test repository.\n\n>> +                             if (defined $params{'file_parent'}) {\n>> +                                     if (defined $params{'file_name'} && $params{'file_parent'} eq $params{'file_name'}) {\n>\n> or, to reduce line length even more:\n>\n>  +                                     if (defined $params{'file_name'} &&\n>  +                                         $params{'file_parent'} eq $params{'file_name'}) {\n\nOh right ... should I resend?\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"119274","messageId":"7vljm4x5vm.fsf@alter.siamese.dyndns.org","threadId":"20301","inReplyTo":"cb7bb73a0907310137o6adb57a5tb05b6745b41b0f01@mail.gmail.com","subject":"Re: [PATCH] gitweb: fix 'Use of uninitialized value' error in href()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-31T14:51:41Z","receivedAt":"2009-07-31T14:51:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n\n> 2009/7/31 Jakub Narebski <jnareb@gmail.com>:\n>> On Fri, 31 July 2009, Giuseppe Bilotta wrote:\n>>>\n>>> The funny thing is that I seem to get the error for something as simple\n>>> as a commit view (_any_ commit view, for the matter), but I wasn't able\n>>> to reproduce it from the shell, which is why I'm not adding a testcase.\n>>\n>> Gaaah! A Heisenbug! ;-)\n>\n> Well, I can reproduce it consistently on the gitweb repository, with\n> any shortlog or commit view. I also think I understand why it happens\n> (i.e. in some cases the check for file_parent eq file_name is done\n> before calling href). The question, of course, is why wouldn't it\n> happen in the test repository.\n\nThat sounds like something worth figuring out, independent of the issue\nthis patch addresses.\n"},{"id":"119475","messageId":"7v4oso3uqa.fsf@alter.siamese.dyndns.org","threadId":"20301","inReplyTo":"200907311030.52643.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: fix 'Use of uninitialized value' error in href()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-04T07:26:37Z","receivedAt":"2009-08-04T07:26:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> Acked-by: Jakub Narebski <jnareb@gmail.com>\n\nThanks, both.\n"}]}