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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jun 9, 2015 at 3:10 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote:
> On 06/08/2015 06:43 PM, Stefan Beller wrote:
>> On Mon, Jun 8, 2015 at 4:45 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> 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.

Ok that's what I was missing.

>
> * 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.
>
>> 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@xxxxxxxxxx>
>
> 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.
>
> ?

This is very clear to me.

>
> Michael
>
> --
> Michael Haggerty
> mhagger@xxxxxxxxxxxx
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]