Re: [PATCH] Prevent warning

From: Jeff King <peff@peff.net>
Date: Oct 29, 2018, 16:33 UTC
Message-ID: <20181029163312.GB31497@sigill.intra.peff.net>
In-Reply-To: <01020166c07adf27-0c2cb5ac-2292-4ce3-808e-b627202fd45f-000000@eu-west-1.amazonses.com>

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