diff options
author | Michael Haggerty <mhagger@alum.mit.edu> | 2016-04-25 18:48:32 +0300 |
---|---|---|
committer | Michael Haggerty <mhagger@alum.mit.edu> | 2016-06-13 12:23:50 +0300 |
commit | 6e30b2f652d0a6748e2041dee5b5612cafca29b2 (patch) | |
tree | ccbaead4bf43149ce01d8af6fcae26d50491527e | |
parent | 8169d0d06ad721aa54d95f044f4b097d79151ea2 (diff) |
lock_ref_for_update(): don't resolve symrefs
If a transaction includes a non-NODEREF update to a symbolic reference,
we don't have to look it up in lock_ref_for_update(). The reference will
be dereferenced anyway when the split-off update is processed.
This change requires that we store a backpointer from the split-off
update to its parent update, for two reasons:
* We still want to report the original reference name in error messages.
So if an error occurs when checking the split-off update's old_sha1,
walk the parent_update pointers back to find the original reference
name, and report that one.
* We still need to write the old_sha1 of the symref to its reflog. So
after we read the split-off update's reference value, walk the
parent_update pointers back and fill in their old_sha1 fields.
Aside from eliminating unnecessary reads, this change fixes a
subtle (though not very serious) race condition: in the old code, the
old_sha1 of the symref was resolved before the reference that it pointed
at was locked. So it was possible that the old_sha1 value logged to the
symref's reflog could be wrong if another process changed the downstream
reference before it was locked.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
-rw-r--r-- | refs/files-backend.c | 108 | ||||
-rw-r--r-- | refs/refs-internal.h | 17 |
2 files changed, 95 insertions, 30 deletions
diff --git a/refs/files-backend.c b/refs/files-backend.c index 7bc18322aa..a783513435 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3376,8 +3376,15 @@ static int split_symref_update(struct ref_update *update, update->new_sha1, update->old_sha1, update->msg); - /* Change the symbolic ref update to log only: */ + new_update->parent_update = update; + + /* + * Change the symbolic ref update to log only. Also, it + * doesn't need to check its old SHA-1 value, as that will be + * done when new_update is processed. + */ update->flags |= REF_LOG_ONLY | REF_NODEREF; + update->flags &= ~REF_HAVE_OLD; item->util = new_update; @@ -3385,6 +3392,17 @@ static int split_symref_update(struct ref_update *update, } /* + * Return the refname under which update was originally requested. + */ +static const char *original_update_refname(struct ref_update *update) +{ + while (update->parent_update) + update = update->parent_update; + + return update->refname; +} + +/* * Prepare for carrying out update: * - Lock the reference referred to by update. * - Read the reference under lock. @@ -3437,44 +3455,74 @@ static int lock_ref_for_update(struct ref_update *update, lock = update->lock; if (update->type & REF_ISSYMREF) { - if (read_ref_full(update->refname, - mustexist ? RESOLVE_REF_READING : 0, - lock->old_oid.hash, NULL)) { - if (update->flags & REF_HAVE_OLD) { - strbuf_addf(err, "cannot lock ref '%s': can't resolve old value", - update->refname); + if (update->flags & REF_NODEREF) { + /* + * We won't be reading the referent as part of + * the transaction, so we have to read it here + * to record and possibly check old_sha1: + */ + if (read_ref_full(update->refname, + mustexist ? RESOLVE_REF_READING : 0, + lock->old_oid.hash, NULL)) { + if (update->flags & REF_HAVE_OLD) { + strbuf_addf(err, "cannot lock ref '%s': " + "can't resolve old value", + update->refname); + return TRANSACTION_GENERIC_ERROR; + } else { + hashclr(lock->old_oid.hash); + } + } + if ((update->flags & REF_HAVE_OLD) && + hashcmp(lock->old_oid.hash, update->old_sha1)) { + strbuf_addf(err, "cannot lock ref '%s': " + "is at %s but expected %s", + update->refname, + sha1_to_hex(lock->old_oid.hash), + sha1_to_hex(update->old_sha1)); return TRANSACTION_GENERIC_ERROR; - } else { - hashclr(lock->old_oid.hash); } - } - if ((update->flags & REF_HAVE_OLD) && - hashcmp(lock->old_oid.hash, update->old_sha1)) { - strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s", - update->refname, - sha1_to_hex(lock->old_oid.hash), - sha1_to_hex(update->old_sha1)); - return TRANSACTION_GENERIC_ERROR; - } - if (!(update->flags & REF_NODEREF)) { + } else { + /* + * Create a new update for the reference this + * symref is pointing at. Also, we will record + * and verify old_sha1 for this update as part + * of processing the split-off update, so we + * don't have to do it here. + */ ret = split_symref_update(update, referent.buf, transaction, affected_refnames, err); if (ret) return ret; } - } else if ((update->flags & REF_HAVE_OLD) && - hashcmp(lock->old_oid.hash, update->old_sha1)) { - if (is_null_sha1(update->old_sha1)) - strbuf_addf(err, "cannot lock ref '%s': reference already exists", - update->refname); - else - strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s", - update->refname, - sha1_to_hex(lock->old_oid.hash), - sha1_to_hex(update->old_sha1)); + } else { + struct ref_update *parent_update; + + /* + * If this update is happening indirectly because of a + * symref update, record the old SHA-1 in the parent + * update: + */ + for (parent_update = update->parent_update; + parent_update; + parent_update = parent_update->parent_update) { + oidcpy(&parent_update->lock->old_oid, &lock->old_oid); + } - return TRANSACTION_GENERIC_ERROR; + if ((update->flags & REF_HAVE_OLD) && + hashcmp(lock->old_oid.hash, update->old_sha1)) { + if (is_null_sha1(update->old_sha1)) + strbuf_addf(err, "cannot lock ref '%s': reference already exists", + original_update_refname(update)); + else + strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s", + original_update_refname(update), + sha1_to_hex(lock->old_oid.hash), + sha1_to_hex(update->old_sha1)); + + return TRANSACTION_GENERIC_ERROR; + } } if ((update->flags & REF_HAVE_NEW) && diff --git a/refs/refs-internal.h b/refs/refs-internal.h index cccd76b28c..1bb3d87dc5 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -143,24 +143,41 @@ int should_autocreate_reflog(const char *refname); * not exist before update. */ struct ref_update { + /* * If (flags & REF_HAVE_NEW), set the reference to this value: */ unsigned char new_sha1[20]; + /* * If (flags & REF_HAVE_OLD), check that the reference * previously had this value: */ unsigned char old_sha1[20]; + /* * One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF, * REF_DELETING, REF_ISPRUNING, REF_LOG_ONLY, and * REF_UPDATE_VIA_HEAD: */ unsigned int flags; + struct ref_lock *lock; unsigned int type; char *msg; + + /* + * If this ref_update was split off of a symref update via + * split_symref_update(), then this member points at that + * update. This is used for two purposes: + * 1. When reporting errors, we report the refname under which + * the update was originally requested. + * 2. When we read the old value of this reference, we + * propagate it back to its parent update for recording in + * the latter's reflog. + */ + struct ref_update *parent_update; + const char refname[FLEX_ARRAY]; }; |