Re: [PATCH v2 04/12] ref_transaction_update(): remove "have_old" parameter

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Feb 13, 2015, 17:15 UTC
Message-ID: <54DE3135.7070902@alum.mit.edu>
In-Reply-To: <CAPc5daX80Mk2cRNAijTckHZ-EFJhiF4GHWMxBK4wYJYSwaEe0w@mail.gmail.com>

On 02/12/2015 06:32 PM, Junio C Hamano wrote:

Show 16 quoted lines

> On Thu, Feb 12, 2015 at 3:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> Instead, verify the reference's old value if and only if old_sha1 is
>> non-NULL.
>>
>> ...
>> @@ -3664,9 +3664,6 @@ int ref_transaction_update(struct ref_transaction *transaction,
>>         if (transaction->state != REF_TRANSACTION_OPEN)
>>                 die("BUG: update called for transaction that is not open");
>>
>> -       if (have_old && !old_sha1)
>> -               die("BUG: have_old is true but old_sha1 is NULL");
>> -
> 
> In the old world, old_sha1 here used to be one of
>  (1) NULL, (2) null_sha1[], or (3) a real object name.
> What is the rule in the new world?

The new world is explained in the docstring in refs.h, which was updated in this same commit:

    If old_sha1 is non-NULL, then it the value
    that the reference should have had before the update, or null_sha1
    if it must not have existed beforehand.

The docstring is further revised later in the patch series to

    old_sha1 is the value that the reference must have
    before the update, or null_sha1 if it must not have existed
    beforehand. The old value is checked after the lock is taken to
    prevent races. If the old value doesn't agree with old_sha1, the
    whole transaction fails. If old_sha1 is NULL, then the previous
    value is not checked.

The new rule is extended to ref_transaction_delete() in the subsequent commit. I like the new semantics because it removes redundancy in the interpretation of parameters.

Is that explanation adequate?

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu