Re: [PATCH 01/13] delete_ref(): move declaration to refs.h

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Jun 9, 2015, 10:10 UTC
Message-ID: <5576BBB1.5020903@alum.mit.edu>
In-Reply-To: <CAGZ79kYy3zUezRzAG3Uo7bDbLgJ7srj3F1k-ama1C44ZdbHr=Q@mail.gmail.com>

On 06/08/2015 06:43 PM, Stefan Beller wrote:

Show 22 quoted lines

> On Mon, Jun 8, 2015 at 4:45 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> [...]
>> +/*
>> + * Delete the specified reference. If old_sha1 is non-NULL and not
>> + * NULL_SHA1, then verify that the current value of the reference is
>> + * old_sha1 before deleting it.
> 
> And here I wondered what the distinction between NULL and non-NULL,
> but NULL_SHA1
> is and digging into the code, there is none. (As you can also see in
> this patch above with
>     (old_sha1 && !is_null_sha1(old_sha1)) ? old_sha1 : NULL,
> but when digging deeper, the !is_null_sha1(old_sha1) is an arbitrary
> limitation (i.e.
> ref_transaction_delete and ref_transaction_update don't differ between
> those two cases.)
> 
> The distinction comes in at lock_ref_sha1_basic, where I think we may
> want to get rid of
> the is_null_sha1 check and depend on NULL/non-NULL as the difference
> for valid and invalid
> sha1's?

I'm having a little trouble understanding your comment.

The convention for ref_transaction_update() and friends is that

* old_sha1 == NULL

  We don't care whether the reference existed prior to the
  update, nor what its value was.

* *old_sha1 is NULL_SHA1

  (by which I mean that old_sha1 points at 20 zero bytes; I hope
  that's clear even though NULL_SHA1 is not actually defined
  anywhere): The reference must *not* have existed prior to the
  update.

* old_sha1 has some other value

  The reference must have had that value prior to the update.

lock_ref_sha1_basic() distinguishes between { NULL vs. NULL_SHA1 vs. other values } in the same way that ref_transaction_update() does.

The delete_ref() function doesn't follow the same convention. It treats NULL and NULL_SHA1 identically, as "don't care".

It probably makes sense to change delete_ref() use the same convention as ref_transaction_update(), but there are quite a few callers and I didn't have the energy to review them all as part of this patch series. So I left it unchanged and just documented the status quo better.

Show 5 quoted lines

> That said, do we want to add another sentence to the doc here saying
> non-NULL and not
> NULL_SHA1 are treated the same or is it clear enough?
> With or without this question addressed:
> Reviewed-by: Stefan Beller <sbeller@google.com>

In set notation,

    "non-NULL" =
        "non-NULL and not NULL_SHA1" ∪
        "non-NULL and equal to NULL_SHA1"

The latter two are *not* treated the same, so I don't see how we can claim that "non-NULL" and "not NULL_SHA1" are treated the same. I must be misunderstanding you.

Would it help if I changed the comment to

    Delete the specified reference. If old_sha1 is non-NULL and not
    NULL_SHA1, then verify that the current value of the reference is
    old_sha1 before deleting it. If old_sha1 is NULL or NULL_SHA1,
    delete the reference it it exists, regardless of its old value.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu