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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 26, 2020, 00:17 UTC
Message-ID: <20200426001740.GB877@gmail.com>
In-Reply-To: <pull.767.git.git.1587847338677.gitgitgadget@gmail.com>

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

[...]

Show 8 quoted 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: ...

> Add myself as warning fix author.
> 
> Signed-off-by: Raphaël Gertz <git@rapsys.eu>
> ---

[...]

Show 14 quoted lines

>  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 <kay.sievers@vrfy.org>
>  
> +Perl warning fix:
> +  Raphaël Gertz <git@rapsys.eu>

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.

[...]

Show 8 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 .= ...

[...]

Show 6 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");
>  
>

Thanks and hope that helps, Jonathan