On Sat, May 17, 2014 at 6:33 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote:
> On 05/16/2014 07:37 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.
>
> s/exist/exists/
>
> And the references to ref_transaction_create() in the commit message are
> obsolete.
>
Thanks. Fixed typo and deleted obsolete text.
>>
>> 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 existing. I.e. one thread could end up
>> overwriting a branch even if the forcing flag is false.
>>
>> Reviewed-by: Jonathan Nieder <jrnieder@xxxxxxxxx>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@xxxxxxxxxx>
>> ---
>> branch.c | 29 +++++++++++++++--------------
>> 1 file changed, 15 insertions(+), 14 deletions(-)
>>
>> diff --git a/branch.c b/branch.c
>> index 660097b..8bf7588 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,24 @@ 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;
>> + struct strbuf err = STRBUF_INIT;
>> +
>> + transaction = ref_transaction_begin();
>> + if (!transaction ||
>> + ref_transaction_update(transaction, ref.buf, sha1,
>> + null_sha1, 0, !forcing, &err) ||
>> + ref_transaction_commit(transaction, msg, &err))
>> + die("%s", err.buf);
>> + }
>> +
>> 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