{"thread":{"id":"53321","subject":"[PATCH] commit:fix use of uninitialized value [...] in server log","startedAt":"2020-04-25T20:42:23Z","lastAt":"2020-04-26T06:36:10Z","messageCount":3,"participants":["Raphaël Gertz via GitGitGadget","Jonathan Nieder","Raphaël Gertz"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"396267","messageId":"pull.767.git.git.1587847338677.gitgitgadget@gmail.com","threadId":"53321","inReplyTo":null,"subject":"[PATCH] commit:fix use of uninitialized value [...] in server log","fromName":"Raphaël Gertz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-25T20:42:18Z","receivedAt":"2020-04-25T20:42:23Z","isPatch":true,"sender":{"key":"git@rapsys.eu","avatar":null},"body":"From: =?UTF-8?q?Rapha=C3=ABl=20Gertz?= <git@rapsys.eu>\n\nThis change fix the message about uninitialized value when trying to\naccess undefined hash indexes.\n\nThe error message fixed:\nUse of uninitialized value $params{\"action\"} in string eq at gitweb.cgi\nline 1377\n\nAdd myself as warning fix author.\n\nSigned-off-by: Raphaël Gertz <git@rapsys.eu>\n---\n    gitweb: fix tests on uninitialized hash indexes to avoid uninitialized\n    value warnings in server log\n    \n    It's happened that I tried to solve lots of meaningless warnings in web\n    server error log.\n    \n    I think it makes no sense to spam error log with warnings about\n    uninitialized value when trying to run careless tests on undefined hash\n    indexes.\n    \n    This is a simple fix that I did long ago that check carefully the index\n    before running tests on it.\n    \n    I added myself as warning fix author as well.\n    \n    My goal is to avoid re-applying the patch on each distribution update.\n    \n    The warning message fixed in web server error log : Use of uninitialized\n    value $params{\"action\"} in string eq at gitweb.cgi line 1377\n    \n    Raphaël Gertz: gitweb/README: add myself as warning fix author\n    gitweb/gitweb.perl: fix careless test on undefined hash indexes\n    \n     gitweb/README | 3 +++ gitweb/gitweb.perl | 4 ++-- 2 files changed, 5\n    insertions(+), 2 deletions(-)\n    \n    Signed-off-by: Raphaël Gertz git@rapsys.eu [git@rapsys.eu]\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-767%2Frapsys%2Fmaster-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-767/rapsys/master-v1\nPull-Request: https://github.com/git/git/pull/767\n\n gitweb/README      | 3 +++\n gitweb/gitweb.perl | 4 ++--\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/README b/gitweb/README\nindex 471dcfb691b..8964478a3fc 100644\n--- a/gitweb/README\n+++ b/gitweb/README\n@@ -66,5 +66,8 @@ AUTHORS\n Originally written by:\n   Kay Sievers <kay.sievers@vrfy.org>\n \n+Perl warning fix:\n+  Raphaël Gertz <git@rapsys.eu>\n+\n Any comment/question/concern to:\n   Git mailing list <git@vger.kernel.org>\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 1a02a1242d5..37dfce202a0 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1420,7 +1420,7 @@ sub href {\n \n \t\t# since we destructively absorb parameters, we keep this\n \t\t# boolean that remembers if we're handling a snapshot\n-\t\tmy $is_snapshot = $params{'action'} eq 'snapshot';\n+\t\tmy $is_snapshot = defined $params{'action'} && $params{'action'} eq 'snapshot';\n \n \t\t# Summary just uses the project path URL, any other action is\n \t\t# added to the URL\n@@ -6012,7 +6012,7 @@ sub git_history_body {\n \t\t      $cgi->a({-href => href(action=>$ftype, hash_base=>$commit, file_name=>$file_name)}, $ftype) . \" | \" .\n \t\t      $cgi->a({-href => href(action=>\"commitdiff\", hash=>$commit)}, \"commitdiff\");\n \n-\t\tif ($ftype eq 'blob') {\n+\t\tif (defined $ftype && $ftype eq 'blob') {\n \t\t\tprint \" | \" .\n \t\t\t      $cgi->a({-href => href(action=>\"blob_plain\", hash_base=>$commit, file_name=>$file_name)}, \"raw\");\n \n\nbase-commit: e870325ee8575d5c3d7afe0ba2c9be072c692b65\n-- \ngitgitgadget\n"},{"id":"396274","messageId":"20200426001740.GB877@gmail.com","threadId":"53321","inReplyTo":"pull.767.git.git.1587847338677.gitgitgadget@gmail.com","subject":"Re: [PATCH] commit:fix use of uninitialized value [...] in server log","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-26T00:17:40Z","receivedAt":"2020-04-26T00:17:47Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(cc-ing Jakub Narębski, gitweb expert; Giuseppe Bilotta, who contributed\n snapshot_format)\nRaphaël Gertz wrote:\n\n>     This is a simple fix that I did long ago that check carefully the index\n>     before running tests on it.\n>     \n>     My goal is to avoid re-applying the patch on each distribution update.\n\nThanks for contributing.  That certainly seems like a good goal. :)\n\n[...]\n> Subject: commit:fix use of uninitialized value [...] in server log\n>\n> This change fix the message about uninitialized value when trying to\n> access undefined hash indexes.\n>\n> The error message fixed:\n> Use of uninitialized value $params{\"action\"} in string eq at gitweb.cgi\n> line 1377\n\nSome nitpicks about the commit message (see\nDocumentation/SubmittingPatches \"Describe your changes well\" for more on\nthis subject):\n\n- the subject line should start with the subsystem being improved, a\n  colon, and a space.  Here, that subsystem is gitweb.\n\n- focus on describing what improvement the patch intends to make.  The\n  description should be in the imperative mood, as though ordering the\n  codebase to improve.\n\n- try to cover what a person trying to understand whether to apply\n  this patch would want to know beyond what is already in the patch\n  itself.  What user-facing behavior change does the patch make?  How\n  was the problem discovered?  What downsides are there, if any?\n\nI think that would mean something like\n\n\tgitweb: check whether query params are defined before use\n\n\tIn normal use, gitweb spams the web server error log:\n\n\t  Use of uninitialized value $params{\"action\"} in string eq at gitweb.cgi line 1377\n\n\tThe 'action' parameter is optional so this is not warning about\n\tanything meaningful. Check whether the parameter is defined\n\tbefore using it to avoid the warning.\n\n\tSigned-off-by: ...\n\n> Add myself as warning fix author.\n> \n> Signed-off-by: Raphaël Gertz <git@rapsys.eu>\n> ---\n[...]\n>  gitweb/README      | 3 +++\n>  gitweb/gitweb.perl | 4 ++--\n>  2 files changed, 5 insertions(+), 2 deletions(-)\n> \n> diff --git a/gitweb/README b/gitweb/README\n> index 471dcfb691b..8964478a3fc 100644\n> --- a/gitweb/README\n> +++ b/gitweb/README\n> @@ -66,5 +66,8 @@ AUTHORS\n>  Originally written by:\n>    Kay Sievers <kay.sievers@vrfy.org>\n>  \n> +Perl warning fix:\n> +  Raphaël Gertz <git@rapsys.eu>\n\nPlease don't.  A contributor list can be obtained using \"git shortlog\n-n -s -- gitweb\".  A second changelog would fall out of sync with\nthat.\n\n[...]\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1420,7 +1420,7 @@ sub href {\n>  \n>  \t\t# since we destructively absorb parameters, we keep this\n>  \t\t# boolean that remembers if we're handling a snapshot\n> -\t\tmy $is_snapshot = $params{'action'} eq 'snapshot';\n> +\t\tmy $is_snapshot = defined $params{'action'} && $params{'action'} eq 'snapshot';\n\nnit: long line\n\nOther parameters like 'project' similarly use a defined check like\nthis, so it's consistent.  Good.\n\n>  \n>  \t\t# Summary just uses the project path URL, any other action is\n>  \t\t# added to the URL\n> \t\tif (defined $params{'action'}) {\n\noptional: should we reuse this \"if\"?  That is, something like\n\n\t\tmy $is_snapshot = 0;\n\n\t\tif (defined $params{'action'}) {\n\t\t\t$is_snapshot = $params{'action'} eq 'snapshot';\n\t\t\t$href .= ...\n\n[...]\n> @@ -6012,7 +6012,7 @@ sub git_history_body {\n>  \t\t      $cgi->a({-href => href(action=>$ftype, hash_base=>$commit, file_name=>$file_name)}, $ftype) . \" | \" .\n>  \t\t      $cgi->a({-href => href(action=>\"commitdiff\", hash=>$commit)}, \"commitdiff\");\n>  \n> -\t\tif ($ftype eq 'blob') {\n> +\t\tif (defined $ftype && $ftype eq 'blob') {\n\nWhat is this part about?  The commit message doesn't describe it.\n\n>  \t\t\tprint \" | \" .\n>  \t\t\t      $cgi->a({-href => href(action=>\"blob_plain\", hash_base=>$commit, file_name=>$file_name)}, \"raw\");\n>  \n> \n\nThanks and hope that helps,\nJonathan\n"},{"id":"396284","messageId":"b64c0b21fedd0033c237ad4c0b18c18b@rapsys.eu","threadId":"53321","inReplyTo":"20200426001740.GB877@gmail.com","subject":"Re: [PATCH] commit:fix use of uninitialized value [...] in server log","fromName":"Raphaël Gertz","fromEmail":"git@rapsys.eu","sentAt":"2020-04-26T06:17:55Z","receivedAt":"2020-04-26T06:36:10Z","isPatch":true,"sender":{"key":"git@rapsys.eu","avatar":null},"body":"Hi,\n\nLe 26.04.2020 02:17, Jonathan Nieder a écrit :\n> (cc-ing Jakub Narębski, gitweb expert; Giuseppe Bilotta, who \n> contributed\n>  snapshot_format)\n> Raphaël Gertz wrote:\n>> Subject: commit:fix use of uninitialized value [...] in server log\n>> \n>> This change fix the message about uninitialized value when trying to\n>> access undefined hash indexes.\n>> \n>> The error message fixed:\n>> Use of uninitialized value $params{\"action\"} in string eq at \n>> gitweb.cgi\n>> line 1377\n> \n> Some nitpicks about the commit message (see\n> Documentation/SubmittingPatches \"Describe your changes well\" for more \n> on\n> this subject):\n> \n> - the subject line should start with the subsystem being improved, a\n>   colon, and a space.  Here, that subsystem is gitweb.\n> \n> - focus on describing what improvement the patch intends to make.  The\n>   description should be in the imperative mood, as though ordering the\n>   codebase to improve.\n> \n> - try to cover what a person trying to understand whether to apply\n>   this patch would want to know beyond what is already in the patch\n>   itself.  What user-facing behavior change does the patch make?  How\n>   was the problem discovered?  What downsides are there, if any?\n> \n> I think that would mean something like\n> \n> \tgitweb: check whether query params are defined before use\n> \n> \tIn normal use, gitweb spams the web server error log:\n> \n> \t  Use of uninitialized value $params{\"action\"} in string eq at\n> gitweb.cgi line 1377\n> \n> \tThe 'action' parameter is optional so this is not warning about\n> \tanything meaningful. Check whether the parameter is defined\n> \tbefore using it to avoid the warning.\n> \n> \tSigned-off-by: ...\n> \nThank's for explaining how to improve the bug report.\n\n> Please don't.  A contributor list can be obtained using \"git shortlog\n> -n -s -- gitweb\".  A second changelog would fall out of sync with\n> that.\n> \nDidn't know sorry.\n\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -1420,7 +1420,7 @@ sub href {\n>> \n>>  \t\t# since we destructively absorb parameters, we keep this\n>>  \t\t# boolean that remembers if we're handling a snapshot\n>> -\t\tmy $is_snapshot = $params{'action'} eq 'snapshot';\n>> +\t\tmy $is_snapshot = defined $params{'action'} && $params{'action'} eq \n>> 'snapshot';\n> \n> nit: long line\n> \n> Other parameters like 'project' similarly use a defined check like\n> this, so it's consistent.  Good.\n> \n>> \n>>  \t\t# Summary just uses the project path URL, any other action is\n>>  \t\t# added to the URL\n>> \t\tif (defined $params{'action'}) {\n> \n> optional: should we reuse this \"if\"?  That is, something like\n> \n> \t\tmy $is_snapshot = 0;\n> \n> \t\tif (defined $params{'action'}) {\n> \t\t\t$is_snapshot = $params{'action'} eq 'snapshot';\n> \t\t\t$href .= ...\n> \nIt may involve less risk to keep the variabe undefined:\n\t\tmy $is_snapshot = undef;\ninstead of:\n\t\tmy $is_snapshot = 0;\n\nAs I don't have time to maintain the changes and am not a perl Jedi,\nI may have wanted to avoid rewriting lot of code.\n\nI only intended to make minimalist code modification to avoid breaking\nwithout noticing something and get cursed for it :)\n\n>> @@ -6012,7 +6012,7 @@ sub git_history_body {\n>>  \t\t      $cgi->a({-href => href(action=>$ftype, hash_base=>$commit, \n>> file_name=>$file_name)}, $ftype) . \" | \" .\n>>  \t\t      $cgi->a({-href => href(action=>\"commitdiff\", hash=>$commit)}, \n>> \"commitdiff\");\n>> \n>> -\t\tif ($ftype eq 'blob') {\n>> +\t\tif (defined $ftype && $ftype eq 'blob') {\n> \n> What is this part about?  The commit message doesn't describe it.\n> \n>>  \t\t\tprint \" | \" .\n>>  \t\t\t      $cgi->a({-href => href(action=>\"blob_plain\", \n>> hash_base=>$commit, file_name=>$file_name)}, \"raw\");\n>> \n>> \nThis is an other test on possibly undefined value.\n\nTHis fix intent to remove an other warning in server log when ftype is \nnot defined.\n\nError log will look like :\ngitweb.cgi: Use of uninitialized value $ftype in string eq at \n/path/gitweb.cgi line 5962.: /path/gitweb.cgi\n\nI just went to notice I missed an other error message spammed as well :\ngitweb.cgi: Use of uninitialized value $commit_id in open at \n/path/gitweb.cgi line 3568.: /path/gitweb.cgi\n\nI will try to add the patch for this when possible as well.\n\nBest regards\n"}]}