Skip to contentSkip to conversation

commit:fix use of uninitialized value [...] in server log

PATCH In review Raphaël Gertz via GitGitGadget 3 participants 3 messages Apr 25, 2020 – Apr 26, 2020

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 commit:fix use of uninitialized value [...] in server log

From: =?UTF-8?q?Rapha=C3=ABl=20Gertz?= <[email protected]>

This change fix the message about uninitialized value when trying to access undefined hash indexes.

The error message fixed:
Use of uninitialized value $params{"action"} in string eq at gitweb.cgi line 1377

Add myself as warning fix author.

---
gitweb: fix tests on uninitialized hash indexes to avoid uninitialized value warnings in server log

It's happened that I tried to solve lots of meaningless warnings in web server error log.

I think it makes no sense to spam error log with warnings about uninitialized value when trying to run careless tests on undefined hash indexes.

This is a simple fix that I did long ago that check carefully the index before running tests on it.

I added myself as warning fix author as well.

My goal is to avoid re-applying the patch on each distribution update.

The warning message fixed in web server error log : Use of uninitialized value $params{"action"} in string eq at gitweb.cgi line 1377

Raphaël Gertz: gitweb/README: add myself as warning fix author gitweb/gitweb.perl: fix careless test on undefined hash indexes

gitweb/README | 3 +++ gitweb/gitweb.perl | 4 ++-- 2 files changed, 5
insertions(+), 2 deletions(-)

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-767%2Frapsys%2Fmaster-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-767/rapsys/master-v1
Pull-Request: https://github.com/git/git/pull/767

 gitweb/README      | 3 +++
 gitweb/gitweb.perl | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)
gitweb/README +3 -0
index 471dcfb691b..8964478a3fc 100644
@@ -66,5 +66,8 @@ AUTHORS
 Originally written by:
   Kay Sievers <[email protected]>
 
+Perl warning fix:
+  Raphaël Gertz <[email protected]>
+
 Any comment/question/concern to:
   Git mailing list <[email protected]>
gitweb/gitweb.perl +2 -2
index 1a02a1242d5..37dfce202a0 100755
@@ -1420,7 +1420,7 @@ sub href {
 
 		# since we destructively absorb parameters, we keep this
 		# boolean that remembers if we're handling a snapshot
-		my $is_snapshot = $params{'action'} eq 'snapshot';
+		my $is_snapshot = defined $params{'action'} && $params{'action'} eq 'snapshot';
 
 		# Summary just uses the project path URL, any other action is
 		# added to the URL
@@ -6012,7 +6012,7 @@ sub git_history_body {
 		      $cgi->a({-href => href(action=>$ftype, hash_base=>$commit, file_name=>$file_name)}, $ftype) . " | " .
 		      $cgi->a({-href => href(action=>"commitdiff", hash=>$commit)}, "commitdiff");
 
-		if ($ftype eq 'blob') {
+		if (defined $ftype && $ftype eq 'blob') {
 			print " | " .
 			      $cgi->a({-href => href(action=>"blob_plain", hash_base=>$commit, file_name=>$file_name)}, "raw");
 

base-commit: e870325ee8575d5c3d7afe0ba2c9be072c692b65

Signature · 1 line
gitgitgadget
#1 lore raw

(cc-ing Jakub Narębski, gitweb expert; Giuseppe Bilotta, who contributed
snapshot_format)

Quoted 4 lines · Raphaël Gertz

This is a simple fix that I did long ago that check carefully the index before running tests on it.

My goal is to avoid re-applying the patch on each distribution update.

Thanks for contributing. That certainly seems like a good goal. :)

[...]

Quoted 8 lines

Subject: commit:fix use of uninitialized value [...] in server log

This change fix the message about uninitialized value when trying to access undefined hash indexes.

The error message fixed:
Use of uninitialized value $params{"action"} in string eq at gitweb.cgi line 1377

Some nitpicks about the commit message (see Documentation/SubmittingPatches "Describe your changes well" for more on this subject):

- the subject line should start with the subsystem being improved, a colon, and a space. Here, that subsystem is gitweb.

- focus on describing what improvement the patch intends to make. The description should be in the imperative mood, as though ordering the codebase to improve.

- try to cover what a person trying to understand whether to apply this patch would want to know beyond what is already in the patch itself. What user-facing behavior change does the patch make? How was the problem discovered? What downsides are there, if any?

I think that would mean something like

gitweb: check whether query params are defined before use

In normal use, gitweb spams the web server error log:

Use of uninitialized value $params{"action"} in string eq at gitweb.cgi line 1377

The 'action' parameter is optional so this is not warning about anything meaningful. Check whether the parameter is defined before using it to avoid the warning.

Signed-off-by ...
Quoted 4 lines

Add myself as warning fix author.

---

[...]

Quoted 9 lines
 gitweb/README      | 3 +++
 gitweb/gitweb.perl | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)
gitweb/README +0 -0
index 471dcfb691b..8964478a3fc 100644

gitweb/README AUTHORS · line 70

Quoted 5 lines
gitweb/README +2 -0
@@ -66,5 +66,8 @@ AUTHORS
 Originally written by:
   Kay Sievers <[email protected]>
 
+Perl warning fix:
+  Raphaël Gertz <[email protected]>

Please don't. A contributor list can be obtained using "git shortlog -n -s -- gitweb". A second changelog would fall out of sync with that.

[...]

gitweb/gitweb.perl sub href { · line 1423

Quoted 8 lines
gitweb/gitweb.perl +1 -1
@@ -1420,7 +1420,7 @@ sub href {
 
 		# since we destructively absorb parameters, we keep this
 		# boolean that remembers if we're handling a snapshot
-		my $is_snapshot = $params{'action'} eq 'snapshot';
+		my $is_snapshot = defined $params{'action'} && $params{'action'} eq 'snapshot';

nit: long line

Other parameters like 'project' similarly use a defined check like this, so it's consistent. Good.

gitweb/gitweb.perl sub href {

Quoted 4 lines
gitweb/gitweb.perl +0 -0
 		# Summary just uses the project path URL, any other action is
 		# added to the URL
 		if (defined $params{'action'}) {

optional: should we reuse this "if"? That is, something like

		my $is_snapshot = 0;

		if (defined $params{'action'}) {
			$is_snapshot = $params{'action'} eq 'snapshot';
			$href .= ...

[...]

gitweb/gitweb.perl sub git_history_body { · line 6015

Quoted 6 lines
gitweb/gitweb.perl +1 -1
@@ -6012,7 +6012,7 @@ sub git_history_body {
 		      $cgi->a({-href => href(action=>$ftype, hash_base=>$commit, file_name=>$file_name)}, $ftype) . " | " .
 		      $cgi->a({-href => href(action=>"commitdiff", hash=>$commit)}, "commitdiff");
 
-		if ($ftype eq 'blob') {
+		if (defined $ftype && $ftype eq 'blob') {

What is this part about? The commit message doesn't describe it.

gitweb/gitweb.perl sub git_history_body { · line 6018

Quoted 4 lines
gitweb/gitweb.perl +0 -0
 			print " | " .
 			      $cgi->a({-href => href(action=>"blob_plain", hash_base=>$commit, file_name=>$file_name)}, "raw");
 

Thanks and hope that helps,
Jonathan

#2 lore raw

Hi,

Le 26.04.2020 02:17, Jonathan Nieder a écrit :

Quoted 34 lines

(cc-ing Jakub Narębski, gitweb expert; Giuseppe Bilotta, who contributed
snapshot_format)

Quoted 8 lines · Raphaël Gertz

Subject: commit:fix use of uninitialized value [...] in server log

This change fix the message about uninitialized value when trying to access undefined hash indexes.

The error message fixed:
Use of uninitialized value $params{"action"} in string eq at gitweb.cgi line 1377

Some nitpicks about the commit message (see Documentation/SubmittingPatches "Describe your changes well" for more on this subject):

- the subject line should start with the subsystem being improved, a colon, and a space. Here, that subsystem is gitweb.

- focus on describing what improvement the patch intends to make. The description should be in the imperative mood, as though ordering the codebase to improve.

- try to cover what a person trying to understand whether to apply this patch would want to know beyond what is already in the patch itself. What user-facing behavior change does the patch make? How was the problem discovered? What downsides are there, if any?

I think that would mean something like

gitweb: check whether query params are defined before use

In normal use, gitweb spams the web server error log:

Quoted 9 lines

Use of uninitialized value $params{"action"} in string eq at
gitweb.cgi line 1377

The 'action' parameter is optional so this is not warning about anything meaningful. Check whether the parameter is defined before using it to avoid the warning.

Signed-off-by ...

Thank's for explaining how to improve the bug report.

Quoted 4 lines

Please don't. A contributor list can be obtained using "git shortlog -n -s -- gitweb". A second changelog would fall out of sync with that.

Didn't know sorry.

Quoted 20 lines

gitweb/gitweb.perl sub href { · line 1424

Quoted 9 lines
gitweb/gitweb.perl +1 -1
@@ -1420,7 +1420,7 @@ sub href {
 
 		# since we destructively absorb parameters, we keep this
 		# boolean that remembers if we're handling a snapshot
-		my $is_snapshot = $params{'action'} eq 'snapshot';
+		my $is_snapshot = defined $params{'action'} && $params{'action'} eq 
 'snapshot';

nit: long line

Other parameters like 'project' similarly use a defined check like this, so it's consistent. Good.

Quoted 4 lines
gitweb/gitweb.perl +0 -0
 		# Summary just uses the project path URL, any other action is
 		# added to the URL
		if (defined $params{'action'}) {
Quoted 8 lines

optional: should we reuse this "if"? That is, something like

		my $is_snapshot = 0;

		if (defined $params{'action'}) {
			$is_snapshot = $params{'action'} eq 'snapshot';
			$href .= ...

It may involve less risk to keep the variabe undefined:

		my $is_snapshot = undef;
instead of:
		my $is_snapshot = 0;

As I don't have time to maintain the changes and am not a perl Jedi, I may have wanted to avoid rewriting lot of code.

I only intended to make minimalist code modification to avoid breaking without noticing something and get cursed for it :)

Quoted 6 lines
Quoted 6 lines
gitweb/gitweb.perl +0 -0
@@ -6012,7 +6012,7 @@ sub git_history_body {
 		      $cgi->a({-href => href(action=>$ftype, hash_base=>$commit, 

file_name=>$file_name)}, $ftype) . " | " .
$cgi->a({-href => href(action=>"commitdiff", hash=>$commit)},
"commitdiff");

Quoted 10 lines

gitweb/gitweb.perl sub git_history_body { · line 6015

Quoted 2 lines
gitweb/gitweb.perl +1 -1
-		if ($ftype eq 'blob') {
+		if (defined $ftype && $ftype eq 'blob') {

What is this part about? The commit message doesn't describe it.

Quoted 5 lines
gitweb/gitweb.perl +0 -0
 			print " | " .
 			      $cgi->a({-href => href(action=>"blob_plain", 

hash_base=>$commit, file_name=>$file_name)}, "raw");

This is an other test on possibly undefined value.

THis fix intent to remove an other warning in server log when ftype is not defined.

Error log will look like :
gitweb.cgi: Use of uninitialized value $ftype in string eq at /path/gitweb.cgi line 5962.: /path/gitweb.cgi

I just went to notice I missed an other error message spammed as well :
gitweb.cgi: Use of uninitialized value $commit_id in open at /path/gitweb.cgi line 3568.: /path/gitweb.cgi

I will try to add the patch for this when possible as well.

Best regards