Michael Haggerty <mhagger@xxxxxxxxxxxx> writes: > I did just realize one thing: `ref_transaction_update()` takes `flags` > as an argument and alters it using > >> flags |= (new_sha1 ? REF_HAVE_NEW : 0) | (old_sha1 ? REF_HAVE_OLD : 0); > > Perhaps gcc is *more* intelligent than we give it credit for, and is > actually worried that the `flags` argument passed in by the caller > might *already* have one of these bits set. In that case > `ref_transaction_add_update()` would indeed be called incorrectly. > Does the warning go away if you change that line to > >> if (new_sha1) >> flags |=REF_HAVE_NEW; >> else >> flags &= ~REF_HAVE_NEW; >> if (old_sha1) >> flags |=REF_HAVE_OLD; >> else >> flags &= ~REF_HAVE_OLD; > > ? This might be a nice change to have anyway, to isolate > `ref_transaction_update()` from mistakes by its callers. I understand "drop HAVE_NEW bit if new_sha1 is NULL" part, but not the other side "add HAVE_NEW if new_SHA1 is not NULL"---doesn't the NEW/OLD flag exist exactly because some callers pass the address of an embedded oid.hash[] or null_sha1, instead of NULL, when one side does not exist? So new|old being NULL is a definite signal that we need to drop HAVE_NEW|OLD, but the reverse may not be true, no? Is it OK to overwrite null_sha1[] that is passed from some codepaths? ref_transaction_create and _delete pass null_sha1 on the missing side, while ref_transaction_verify passes NULL, while calling _update(). Should this distinction affect how _add_update() gets called?
