New-ish warning in refs.c with GCC (at least 11.2) under -O3

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 15, 2021, 17:41 UTC
Message-ID: <211115.86a6i5s4bn.gmgdl@evledraar.gmail.com>

(Sent earlier as <211115.86ee7hsa58.gmgdl@evledraar.gmail.com>, but I typo'd the list address)

This happens on "master", but is old, going back to at least v2.25.0 (I
didn't bother to test older versions):
    
    $ gcc --version
    gcc (Debian 11.2.0-10) 11.2.0
    [...]
    $ make refs.o CC=gcc CFLAGS=-O2
        * new build flags
        CC refs.o
    $ make refs.o CC=gcc CFLAGS=-O3
        * new build flags
        CC refs.o
    In file included from hashmap.h:4,
                     from cache.h:6,
                     from refs.c:5:
    In function ‘oidcpy’,
        inlined from ‘ref_transaction_add_update’ at refs.c:1065:3,
        inlined from ‘ref_transaction_update’ at refs.c:1094:2,
        inlined from ‘ref_transaction_verify’ at refs.c:1132:9:
    hash.h:262:9: error: argument 2 null where non-null expected [-Werror=nonnull]
      262 |         memcpy(dst->hash, src->hash, GIT_MAX_RAWSZ);
          |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    In file included from git-compat-util.h:177,
                     from cache.h:4,
                     from refs.c:5:
    refs.c: In function ‘ref_transaction_verify’:
    /usr/include/string.h:43:14: note: in a call to function ‘memcpy’ declared ‘nonnull’
       43 | extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
          |              ^~~~~~
    cc1: all warnings being treated as errors
    make: *** [Makefile:2500: refs.o] Error 1

I don't have time to dig into it right now, but if anyone's interested...

It seems like the whole business of passing REF_HAVE_{NEW,OLD} is at least partially redundant to simply checking the relevant variables. Maybe.

I.e. our entire test suite passes with the patch below. We "could" unconditionally check the variable name itself, but I haven't dug enough to see if that's introducing a subtle bug we're not testing for, or just a lot of redundant work.

diff --git a/refs.c b/refs.c
index d7cc0a23a3b..335244f756f 100644
--- a/refs.c
+++ b/refs.c
@@ -1061,10 +1061,22 @@ struct ref_update *ref_transaction_add_update(
 
 	update->flags = flags;
 
-	if (flags & REF_HAVE_NEW)
+	if (new_oid && flags & REF_HAVE_NEW)
 		oidcpy(&update->new_oid, new_oid);
-	if (flags & REF_HAVE_OLD)
+	else if (!new_oid && flags & REF_HAVE_NEW)
+		BUG("would have passed NULL to memcpy() with REF_HAVE_NEW");
+	else if (new_oid && !(flags & REF_HAVE_NEW))
+		oidcpy(&update->new_oid, new_oid);
+		//BUG("missed a memcpy() new_oid due to no REF_HAVE_NEW");
+
+	if (old_oid && flags & REF_HAVE_OLD)
 		oidcpy(&update->old_oid, old_oid);
+	else if (!old_oid && flags & REF_HAVE_OLD)
+		BUG("would have passed NULL to memcpy() with REF_HAVE_OLD");
+	else if (old_oid && !(flags & REF_HAVE_OLD))
+		oidcpy(&update->old_oid, old_oid);
+		//BUG("missed a memcpy() old_oid due to no REF_HAVE_OLD");
+
 	update->msg = normalize_reflog_message(msg);
 	return update;
 }