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

From: Raphaël Gertz <git@rapsys.eu>
Date: Apr 26, 2020, 06:17 UTC
Message-ID: <b64c0b21fedd0033c237ad4c0b18c18b@rapsys.eu>
In-Reply-To: <20200426001740.GB877@gmail.com>

Hi,

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

Show 46 quoted lines

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

Show 28 quoted lines

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

Show 16 quoted lines

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