Re: [PATCH] builtin/receive-pack: dead initializer for retval in check_nonce

From: Torsten Bögershausen <tboegi@web.de>
Date: Oct 20, 2018, 16:45 UTC
Message-ID: <20181020164526.GA1077@tor.lan>
In-Reply-To: <20181020070859.48172-1-carenas@gmail.com>

On Sat, Oct 20, 2018 at 12:08:59AM -0700, Carlo Marcelo Arenas Belón wrote:

Show 21 quoted lines

> NONCE_BAD is explicitly set when needed with the fallback
> instead as NONCE_SLOP
> 
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  builtin/receive-pack.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 95740f4f0e..ecce3d4043 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -507,7 +507,7 @@ static const char *check_nonce(const char *buf, size_t len)
>  	char *nonce = find_header(buf, len, "nonce", NULL);
>  	timestamp_t stamp, ostamp;
>  	char *bohmac, *expect = NULL;
> -	const char *retval = NONCE_BAD;
> +	const char *retval;
>  
>  	if (!nonce) {
>  		retval = NONCE_MISSING;

Thanks for the patch.
The motivation feels a little bit weak, at least to me.
Initializing a variable to "BAD" in the beginning can be a good thing
for two reasons:
- There is a complex if-elseif chain, which should set retval
  in any case, this is at least what I expect taking a very quick look at the
  code:
	const char *retval = NONCE_BAD;

	if (!nonce) {
		retval = NONCE_MISSING;
		goto leave;
	} else if (!push_cert_nonce) {
		retval = NONCE_UNSOLICITED;
		goto leave;
	} else if (!strcmp(push_cert_nonce, nonce)) {
		retval = NONCE_OK;
		goto leave;
	}
	# And here I started to wonder if we should have an else or not.
	# Having retval NONCE_BAD set to NONCE:BAD in the beginning makes
	# it clear, that we are save without the else.
	# As an alternative, we could have coded like this:
	
	const char *retval;

	if (!nonce) {
		retval = NONCE_MISSING;
		goto leave;
	} else if (!push_cert_nonce) {
		retval = NONCE_UNSOLICITED;
		goto leave;
	} else if (!strcmp(push_cert_nonce, nonce)) {
		retval = NONCE_OK;
		goto leave;
	} else {
		/* Set to BAD, until we know better further down */
		retval = NONCE_BAD;
	}

# The second reason is that some compilers don't understand this complex # stuff either, and through out a warning, like # "retval may be uninitialized" or something in that style. # This is very compiler dependent.

So yes, the current code may seem to be over-eager and ask for optimization, but we don't gain more that a couple of nano-seconds or so. The good thing is that we have the code a little bit more robust, when changes are done in the future.