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)
?
> + }
> +
> 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