On Sat, Oct 20, 2018 at 12:08:59AM -0700, Carlo Marcelo Arenas Belón wrote:
> NONCE_BAD is explicitly set when needed with the fallback
> instead as NONCE_SLOP
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@xxxxxxxxx>
> ---
> 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.
