Re: [PATCH] refs: make sure we never pass NULL to hashcpy

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 8, 2017, 00:46 UTC
Message-ID: <xmqqvaku10gj.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAMy9T_ED1KBqkE9GCHrOrt0frnYAx1vka7Xx1DrXmjJBNNKahw@mail.gmail.com>

Michael Haggerty <mhagger@alum.mit.edu> writes:

Show 22 quoted lines

> 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?