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

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

 



On Fri, Apr 25, 2014 at 4:16 PM, Michael Haggerty <mhagger@xxxxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>>  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

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 ?



>
>> +     }
>> +
>>       if (real_ref && track)
>>               setup_tracking(ref.buf + 11, real_ref, track, quiet);
>>
>> -     if (!dont_change_ref)
>> -             if (write_ref_sha1(lock, sha1, msg) < 0)
>> -                     die_errno(_("Failed to write ref"));
>> -
>>       strbuf_release(&ref);
>>       free(real_ref);
>>  }
>>
>
>
> --
> Michael Haggerty
> mhagger@xxxxxxxxxxxx
> http://softwareswirl.blogspot.com/
--
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]