Prevent warning

Commit Message

Pete Oct. 29, 2018, 3:39 p.m. UTC

Prevent the following warning in the web server error log:
gitweb.cgi: Odd number of elements in anonymous hash at /usr/share/gitweb/gitweb.cgi line 3305
---
 gitweb/gitweb.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


--
https://github.com/git/git/pull/548

Comments

Torsten Bögershausen Oct. 29, 2018, 3:54 p.m. UTC | #1

Thanks for the patch.
Could you please sign it off ?
The other remark would be if the header line could be written longer than
just
"Prevent warning".
to give people digging into the Git history an initial information,
where the warning occured and from which module it was caused.
May be something like this as a head line ?

gitweb.perl: Fix Odd number of elements warning



On Mon, Oct 29, 2018 at 03:39:30PM +0000, Pete wrote:
> Prevent the following warning in the web server error log:
> gitweb.cgi: Odd number of elements in anonymous hash at /usr/share/gitweb/gitweb.cgi line 3305
> ---
>  gitweb/gitweb.perl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 2594a4badb3d7..200647b683225 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3302,7 +3302,7 @@ sub git_get_remotes_list {
>  		next if $wanted and not $remote eq $wanted;
>  		my ($url, $key) = ($1, $2);
>  
> -		$remotes{$remote} ||= { 'heads' => () };
> +		$remotes{$remote} ||= { 'heads' => [] };
>  		$remotes{$remote}{$key} = $url;
>  	}
>  	close $fd or return;
> 
> --
> https://github.com/git/git/pull/548

Jeff King Oct. 29, 2018, 4:33 p.m. UTC | #2

On Mon, Oct 29, 2018 at 03:39:30PM +0000, Pete wrote:

> Prevent the following warning in the web server error log:
> gitweb.cgi: Odd number of elements in anonymous hash at /usr/share/gitweb/gitweb.cgi line 3305
> [...]
> -		$remotes{$remote} ||= { 'heads' => () };
> +		$remotes{$remote} ||= { 'heads' => [] };

This will indeed silence the warning, but the end result is different.
In the original, 'heads' would be set to undef, but now it is set to an
empty array reference.

Do the later users of the struct care about the distinction?

Grepping around, I see two callers that look at "heads":

 1. fill_remote_heads() assigns to it unconditionally, overwriting
    whatever was there before.

 2. git_remote_block() dereferences it as an array reference, and so
    probably would have complained if we ever got there with the undef.
    But we never would, because every call to git_remote_block() is
    preceded by a call to fill_remote_heads().

So nobody actually ever looks at the value we set here. Is an empty
array reference better than undef as a default value? I'd actually argue
no. If we add new code that does not call fill_remote_heads(), it's
probably better for it to be undef to distinguish it from the empty list
(and possibly raise an error that would tell us that we forgot to call
fill_remote_heads().

So I'd say that:

  $remotes{$remote} ||= { heads => undef };

is a preferable conversion. Or simply deleting the line entirely, which
has roughly the same effect.

-Peff

Patch

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 2594a4badb3d7..200647b683225 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3302,7 +3302,7 @@  sub git_get_remotes_list {
 		next if $wanted and not $remote eq $wanted;
 		my ($url, $key) = ($1, $2);
 
-		$remotes{$remote} ||= { 'heads' => () };
+		$remotes{$remote} ||= { 'heads' => [] };
 		$remotes{$remote}{$key} = $url;
 	}
 	close $fd or return;