threads / patch / 49708

patchPrevent warning

Subject: [PATCH] Prevent warning

## tl;dr

3 messages between Oct 29, 2018 and Oct 29, 2018. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Pete· Oct 29, 2018, 15:39 UTC · lore

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

Show changes to gitweb/gitweb.perl +1 −2

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

Torsten Bögershausen· Oct 29, 2018, 15:54 UTC · re: Pete · lore

Re: [PATCH] Prevent warning

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:

Show 22 quoted lines

> 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, 16:33 UTC · re: Pete · lore

Re: [PATCH] Prevent warning

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

Show 5 quoted lines

> 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