Re: [PATCH v3 16/19] branch.c: use ref transaction for all ref updates

[Michael Haggerty <mhagger@alum.mit.edu>]

On 04/28/2014 09:16 PM, Ronnie Sahlberg wrote:
> On Fri, Apr 25, 2014 at 4:16 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:
>>> Change create_branch to use a ref transaction when creating the new branch.
>>> ref_transaction_create will check that the ref does not already exist and fail
>>> otherwise meaning that we no longer need to keep a lock on the ref during the
>>> setup_tracking. This simplifies the code since we can now do the transaction
>>> in one single step.
>>>
>>> If the forcing flag is false then use ref_transaction_create since this will
>>> fail if the ref already exist. Otherwise use ref_transaction_update.
>>>
>>> This also fixes a race condition in the old code where two concurrent
>>> create_branch could race since the lock_any_ref_for_update/write_ref_sha1
>>> did not protect against the ref already existsing. I.e. one thread could end up
>>> overwriting a branch even if the forcing flag is false.
>>>
>>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>>> ---
>>>  branch.c | 39 +++++++++++++++++++++++++--------------
>>>  1 file changed, 25 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/branch.c b/branch.c
>>> index 660097b..23cde1e 100644
>>> --- a/branch.c
>>> +++ b/branch.c
>>> @@ -226,7 +226,6 @@ void create_branch(const char *head,
>>>                  int force, int reflog, int clobber_head,
>>>                  int quiet, enum branch_track track)
>>>  {
>>> -     struct ref_lock *lock = NULL;
>>>       struct commit *commit;
>>>       unsigned char sha1[20];
>>>       char *real_ref, msg[PATH_MAX + 20];
>>> @@ -285,15 +284,6 @@ void create_branch(const char *head,
>>>               die(_("Not a valid branch point: '%s'."), start_name);
>>>       hashcpy(sha1, commit->object.sha1);
>>>
>>> -     if (!dont_change_ref) {
>>> -             lock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL);
>>> -             if (!lock)
>>> -                     die_errno(_("Failed to lock ref for update"));
>>> -     }
>>> -
>>> -     if (reflog)
>>> -             log_all_ref_updates = 1;
>>> -
>>>       if (forcing)
>>>               snprintf(msg, sizeof msg, "branch: Reset to %s",
>>>                        start_name);
>>> @@ -301,13 +291,34 @@ void create_branch(const char *head,
>>>               snprintf(msg, sizeof msg, "branch: Created from %s",
>>>                        start_name);
>>>
>>> +     if (reflog)
>>> +             log_all_ref_updates = 1;
>>> +
>>> +     if (!dont_change_ref) {
>>> +             struct ref_transaction *transaction;
>>> +             char *err = NULL;
>>> +
>>> +             transaction = ref_transaction_begin();
>>> +             if (forcing) {
>>> +                     if (!transaction ||
>>> +                         ref_transaction_update(transaction, ref.buf, sha1,
>>> +                                                NULL, 0, 0) ||
>>> +                         ref_transaction_commit(transaction, msg, &err))
>>> +                       die_errno(_("%s: failed to write ref: %s"),
>>> +                                 ref.buf, err);
>>> +             } else {
>>> +                     if (!transaction ||
>>> +                         ref_transaction_create(transaction, ref.buf, sha1,
>>> +                                                0) ||
>>> +                         ref_transaction_commit(transaction, msg, &err))
>>> +                       die_errno(_("%s: failed to write ref: %s"),
>>> +                                 ref.buf, err);
>>> +             }
>>
>> You've got some indentation problems above.
>>
>> But actually, there seems like a lot of duplicated code here.  Couldn't
>> you instead do a single block with have_old set based on forcing:
>>
>>     ref_transaction_update(transaction, ref.buf, sha1,
>>                            null_sha1, 0, !forcing)
>>
>> ?
> 
> Done, thanks.
> 
> 
> I am not sure how I feel about using _update to create new refs
> since we already have ref_transaction_create for that purpose.
> 
> ref_transaction_update can either be used to update an existing ref
> or it can be used to create new refs, either by passing have_old==0
> or by passing old_sha1==null_sha1 and have_old==1

Hold onto your socks then, because I think in the future update() should
get a have_new parameter too.  That way it can also be used to verify
the current value of a reference by passing have_old=1, have_new=0
without also re-setting the reference unnecessarily like now.  Though I
admit, have_old=have_new=0 might *not* be so useful :-)

> Maybe the api would be cleaner if we would change it so that update
> and create does
> not overlap and thus change _update so that it can only modify refs
> that must already exist ?

I have no compunctions about using update() to create or delete a
reference.  My point of view is that update() is the general case, and
create() and delete() are special-cases that exist only for the
convenience of callers.  For example, our future pluggable backends
might only have to implement update(), and the other two functions could
delegate to it at the abstract layer.

Plus, making this stricter would make it impossible to eliminate
duplicate code like in the example above, which is itself evidence that
update() is a useful abstraction.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/