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

rapsys · 2020-04-24T12:20:39Z

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(-)

Signed-off-by: Raphaël Gertz [email protected]

gitgitgadget-git · 2020-04-24T12:21:20Z

Welcome to GitGitGadget

Hi @rapsys, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that your Pull Request has a good description, as it will be used as cover letter.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

the lines should not exceed 76 columns,
the first line should be like a header and typically start with a prefix like "tests:" or "commit:", and
the commit messages' body should be describing the "why?" of the change.
Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

Search: is:pr is:open "/allow"

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the FreeNode IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail).

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the (raw) link), then import it into your mail program. If you use GMail, you can do this via:

curl -g --user "<EMailAddress>:<Password>" \
    --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

To iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description):

Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...

To send a new iteration, just add another PR comment with the contents: /submit.

Need help?

New contributors who want advice are encouraged to join [email protected], where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join.

You may also be able to find help in real time in the developer IRC channel, #git-devel on Freenode. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.

gitgitgadget-git · 2020-04-24T12:21:20Z

There is an issue in commit bf32a93d7896bacd1244e820e148d29a2446c3d7:
Commit checks stopped - the message is too short

gitgitgadget-git · 2020-04-24T12:21:21Z

There is an issue in commit 1f2caafb0ef48397bc6ff8a7e160e9e10fdbf30a:
Commit checks stopped - the message is too short

dscho · 2020-04-24T12:26:26Z

/allow

gitgitgadget-git · 2020-04-24T12:27:15Z

User rapsys is now allowed to use GitGitGadget.

WARNING: rapsys has no public email address set on GitHub

dscho · 2020-04-24T12:28:21Z

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

* the lines should not exceed 76 columns,

* the first line should be like a header and typically start with a prefix like "tests:" or "commit:", and

* the commit messages' body should be describing the "why?" of the change.

* Finally, the commit messages should end in a [Signed-off-by:](https://git-scm.com/docs/SubmittingPatches#dco) line matching the commits' author.

@rapsys please do review those. If you have a look e.g. at 4c5971e you will realize what kind of commit messages the Git project is looking for (hint: it's not one-liners like CVS/Subversion commit messages, it is much more verbose what they want).

gitgitgadget-git · 2020-04-24T12:29:00Z

There is an issue in commit bf32a93d7896bacd1244e820e148d29a2446c3d7:
Commit checks stopped - the message is too short

gitgitgadget-git · 2020-04-24T12:29:01Z

There is an issue in commit 1a0b82dc24bc7b688c2198a0b7b139e2f3c132e9:
Prefixed commit message must be in lower case: commit:Add author mention

gitgitgadget-git · 2020-04-24T12:36:17Z

There is an issue in commit 1c5be9a0a4c6620a47edc1d30f86f4c16428d4c3:
Prefixed commit message must be in lower case: commit:Fix use of uninitialized value [...] in server log

gitgitgadget-git · 2020-04-24T12:36:18Z

There is an issue in commit 36793ca9a063c98f6bb4f283caf27611afc292d5:
Prefixed commit message must be in lower case: commit:Add author mention

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. Signed-off-by: Raphaël Gertz <[email protected]>

rapsys · 2020-04-24T12:46:24Z

commit: Fix 'use of uninitialized value' warnings in server error log

This is a patch I tried to submit long ago.

With the current perl script, there are a lot of errors in web server log
because of undefined hash indexes.

This patch change that by testing that a hash index is defined before doing
tests on it.

Plz merge :)

Signed-off-by: Raphaël Gertz [email protected]

rapsys · 2020-04-24T13:39:34Z

@dscho I tried redo the pull request there, as a bugfix should be on maint branch if I am right :
gitgitgadget#617

Is everything fine, may I send a mail to the mailing list with reference to this pull request or should I use this one on master branch ?

rapsys · 2020-04-24T21:56:27Z

Updated desc and title.

rapsys · 2020-04-25T20:40:11Z

/submit

gitgitgadget-git · 2020-04-25T20:42:28Z

Submitted as [email protected]

WARNING: rapsys has no public email address set on GitHub

dscho · 2020-04-25T20:53:45Z

But why submit both this and gitgitgadget#617, after sending https://lore.kernel.org/git/[email protected]/?

In total, you submitted this patch three times now. I am not sure that reviewers like to be prodded like this.

rapsys · 2020-04-25T23:33:56Z

Sorry to them. You know I understand only half of the process, and this is just for a trivial 2 lines bugfix patch...

gitgitgadget-git · 2020-04-26T00:19:52Z

On the Git mailing list, Jonathan Nieder wrote (reply to this):

(cc-ing Jakub Narębski, gitweb expert; Giuseppe Bilotta, who contributed
 snapshot_format)
Raphaël Gertz wrote:

>     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. :)

[...]
> 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: ...

> Add myself as warning fix author.
> 
> Signed-off-by: Raphaël Gertz <[email protected]>
> ---
[...]
>  gitweb/README      | 3 +++
>  gitweb/gitweb.perl | 4 ++--
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/gitweb/README b/gitweb/README
> index 471dcfb691b..8964478a3fc 100644
> --- a/gitweb/README
> +++ b/gitweb/README
> @@ -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.

[...]
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -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.

>  
>  		# 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 .= ...

[...]
> @@ -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.

>  			print " | " .
>  			      $cgi->a({-href => href(action=>"blob_plain", hash_base=>$commit, file_name=>$file_name)}, "raw");
>  
> 

Thanks and hope that helps,
Jonathan

gitgitgadget-git · 2020-04-26T06:38:06Z

On the Git mailing list, Raphaël Gertz wrote (reply to this):

Hi,

Le 26.04.2020 02:17, Jonathan Nieder a écrit :
> (cc-ing Jakub Narębski, gitweb expert; Giuseppe Bilotta, who 
> contributed
>  snapshot_format)
> Raphaël Gertz wrote:
>> 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: ...
> 
Thank's for explaining how to improve the bug report.

> 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.

>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -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.
> 
>> 
>>  		# 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 .= ...
> 
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 :)

>> @@ -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.
> 
>>  			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

rapsys force-pushed the master branch from 1f2caaf to 1a0b82d Compare April 24, 2020 12:28

rapsys force-pushed the master branch from 1a0b82d to 36793ca Compare April 24, 2020 12:35

rapsys force-pushed the master branch from 36793ca to 7d0749a Compare April 24, 2020 12:44

rapsys mentioned this pull request Apr 24, 2020

gitweb: fix tests on uninitialized hash indexes to avoid uninitialized value warnings in server log gitgitgadget/git#617

Open

rapsys changed the title ~~Fix 'Use of uninitialized value $params{"action"} in string eq at gitweb.cgi line 1377' errors in server log~~ gitweb:fix tests on uninitialized hash indexes to avoid uninitialized value warnings in server log Apr 24, 2020

rapsys changed the title ~~gitweb:fix tests on uninitialized hash indexes to avoid uninitialized value warnings in server log~~ gitweb: fix tests on uninitialized hash indexes to avoid uninitialized value warnings in server log Apr 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rapsys commented Apr 24, 2020 •

edited

Loading

gitgitgadget-git bot commented Apr 24, 2020

gitgitgadget-git bot commented Apr 24, 2020

gitgitgadget-git bot commented Apr 24, 2020

dscho commented Apr 24, 2020

gitgitgadget-git bot commented Apr 24, 2020

dscho commented Apr 24, 2020

gitgitgadget-git bot commented Apr 24, 2020

gitgitgadget-git bot commented Apr 24, 2020

gitgitgadget-git bot commented Apr 24, 2020

gitgitgadget-git bot commented Apr 24, 2020

rapsys commented Apr 24, 2020

rapsys commented Apr 24, 2020

rapsys commented Apr 24, 2020

rapsys commented Apr 25, 2020

gitgitgadget-git bot commented Apr 25, 2020

dscho commented Apr 25, 2020

rapsys commented Apr 25, 2020

gitgitgadget-git bot commented Apr 26, 2020

gitgitgadget-git bot commented Apr 26, 2020

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

Are you sure you want to change the base?

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

Conversation

rapsys commented Apr 24, 2020 • edited Loading

gitgitgadget-git bot commented Apr 24, 2020

Welcome to GitGitGadget

Contributing the patches

Need help?

gitgitgadget-git bot commented Apr 24, 2020

gitgitgadget-git bot commented Apr 24, 2020

dscho commented Apr 24, 2020

gitgitgadget-git bot commented Apr 24, 2020

dscho commented Apr 24, 2020

gitgitgadget-git bot commented Apr 24, 2020

gitgitgadget-git bot commented Apr 24, 2020

gitgitgadget-git bot commented Apr 24, 2020

gitgitgadget-git bot commented Apr 24, 2020

rapsys commented Apr 24, 2020

rapsys commented Apr 24, 2020

rapsys commented Apr 24, 2020

rapsys commented Apr 25, 2020

gitgitgadget-git bot commented Apr 25, 2020

dscho commented Apr 25, 2020

rapsys commented Apr 25, 2020

gitgitgadget-git bot commented Apr 26, 2020

gitgitgadget-git bot commented Apr 26, 2020

rapsys commented Apr 24, 2020 •

edited

Loading