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@xxxxxxxxxxxx>
---
refs/files-backend.c | 108 +++++++++++++++++++++++++++++++++++++--------------
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 a9066ba..08ec293 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3371,8 +3371,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;
@@ -3380,6 +3387,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.
@@ -3431,44 +3449,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;
- return TRANSACTION_GENERIC_ERROR;
+ /*
+ * 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);
+ }
+
+ 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 cccd76b..1bb3d87 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];
};
--
2.8.1
--
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