On Tue, Apr 29, 2014 at 2:35 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote:
> On 04/28/2014 09:16 PM, Ronnie Sahlberg wrote:
>> 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
>
> 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.
>
Ok, Fair enough.
In that case, in the future we should change
ref_transaction_create/ref_transaction_delete to just
become wrappers that invoke ref_transaction_update.
> Michael
>
> --
> 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