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>
Michael Haggerty committed
Apr 25, 2016 at 17:48 UTC
6e30b2f652d0a6748e2041dee5b5612cafca29b2
2 files changed
+95
-30
refs/files-backend.c
+78
-30
@@ -3376,14 +3376,32 @@ static int split_symref_update(struct ref_update *update,
3376
update->new_sha1, update->old_sha1,
3377
update->msg);
3378
3379
- /* Change the symbolic ref update to log only: */
3379
+ new_update->parent_update = update;
3380
+
3381
+ /*
3382
+ * Change the symbolic ref update to log only. Also, it
3383
+ * doesn't need to check its old SHA-1 value, as that will be
3384
+ * done when new_update is processed.
3385
+ */
3386
update->flags |= REF_LOG_ONLY | REF_NODEREF;
3387
+ update->flags &= ~REF_HAVE_OLD;
3388
3389
item->util = new_update;
3390
3391
return 0;
3392
}
3393
3394
+/*
3395
+ * Return the refname under which update was originally requested.
3396
+ */
3397
+static const char *original_update_refname(struct ref_update *update)
3398
+{
3399
+ while (update->parent_update)
3400
+ update = update->parent_update;
3401
+
3402
+ return update->refname;
3403
+}
3404
+
3405
/*
3406
* Prepare for carrying out update:
3407
* - Lock the reference referred to by update.
@@ -3437,44 +3455,74 @@ static int lock_ref_for_update(struct ref_update *update,
3455
lock = update->lock;
3456
3457
if (update->type & REF_ISSYMREF) {
3440
- if (read_ref_full(update->refname,
3441
- mustexist ? RESOLVE_REF_READING : 0,
3442
- lock->old_oid.hash, NULL)) {
3443
- if (update->flags & REF_HAVE_OLD) {
3444
- strbuf_addf(err, "cannot lock ref '%s': can't resolve old value",
3445
- update->refname);
3458
+ if (update->flags & REF_NODEREF) {
3459
+ /*
3460
+ * We won't be reading the referent as part of
3461
+ * the transaction, so we have to read it here
3462
+ * to record and possibly check old_sha1:
3463
+ */
3464
+ if (read_ref_full(update->refname,
3465
+ mustexist ? RESOLVE_REF_READING : 0,
3466
+ lock->old_oid.hash, NULL)) {
3467
+ if (update->flags & REF_HAVE_OLD) {
3468
+ strbuf_addf(err, "cannot lock ref '%s': "
3469
+ "can't resolve old value",
3470
+ update->refname);
3471
+ return TRANSACTION_GENERIC_ERROR;
3472
+ } else {
3473
+ hashclr(lock->old_oid.hash);
3474
+ }
3475
+ }
3476
+ if ((update->flags & REF_HAVE_OLD) &&
3477
+ hashcmp(lock->old_oid.hash, update->old_sha1)) {
3478
+ strbuf_addf(err, "cannot lock ref '%s': "
3479
+ "is at %s but expected %s",
3480
+ update->refname,
3481
+ sha1_to_hex(lock->old_oid.hash),
3482
+ sha1_to_hex(update->old_sha1));
3483
return TRANSACTION_GENERIC_ERROR;
3447
- } else {
3448
- hashclr(lock->old_oid.hash);
3484
}
3450
- }
3451
- if ((update->flags & REF_HAVE_OLD) &&
3452
- hashcmp(lock->old_oid.hash, update->old_sha1)) {
3453
- strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
3454
- update->refname,
3455
- sha1_to_hex(lock->old_oid.hash),
3456
- sha1_to_hex(update->old_sha1));
3457
- return TRANSACTION_GENERIC_ERROR;
3458
- }
3485
3460
- if (!(update->flags & REF_NODEREF)) {
3486
+ } else {
3487
+ /*
3488
+ * Create a new update for the reference this
3489
+ * symref is pointing at. Also, we will record
3490
+ * and verify old_sha1 for this update as part
3491
+ * of processing the split-off update, so we
3492
+ * don't have to do it here.
3493
+ */
3494
ret = split_symref_update(update, referent.buf, transaction,
3495
affected_refnames, err);
3496
if (ret)
3497
return ret;
3498
}
3466
- } else if ((update->flags & REF_HAVE_OLD) &&
3467
- hashcmp(lock->old_oid.hash, update->old_sha1)) {
3468
- if (is_null_sha1(update->old_sha1))
3469
- strbuf_addf(err, "cannot lock ref '%s': reference already exists",
3470
- update->refname);
3471
- else
3472
- strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
3473
- update->refname,
3474
- sha1_to_hex(lock->old_oid.hash),
3475
- sha1_to_hex(update->old_sha1));
3499
+ } else {
3500
+ struct ref_update *parent_update;
3501
+
3502
+ /*
3503
+ * If this update is happening indirectly because of a
3504
+ * symref update, record the old SHA-1 in the parent
3505
+ * update:
3506
+ */
3507
+ for (parent_update = update->parent_update;
3508
+ parent_update;
3509
+ parent_update = parent_update->parent_update) {
3510
+ oidcpy(&parent_update->lock->old_oid, &lock->old_oid);
3511
+ }
3512
3477
- return TRANSACTION_GENERIC_ERROR;
3513
+ if ((update->flags & REF_HAVE_OLD) &&
3514
+ hashcmp(lock->old_oid.hash, update->old_sha1)) {
3515
+ if (is_null_sha1(update->old_sha1))
3516
+ strbuf_addf(err, "cannot lock ref '%s': reference already exists",
3517
+ original_update_refname(update));
3518
+ else
3519
+ strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
3520
+ original_update_refname(update),
3521
+ sha1_to_hex(lock->old_oid.hash),
3522
+ sha1_to_hex(update->old_sha1));
3523
+
3524
+ return TRANSACTION_GENERIC_ERROR;
3525
+ }
3526
}
3527
3528
if ((update->flags & REF_HAVE_NEW) &&
refs/refs-internal.h
+17
@@ -143,24 +143,41 @@ int should_autocreate_reflog(const char *refname);
143
* not exist before update.
144
*/
145
struct ref_update {
146
+
147
/*
148
* If (flags & REF_HAVE_NEW), set the reference to this value:
149
*/
150
unsigned char new_sha1[20];
151
+
152
/*
153
* If (flags & REF_HAVE_OLD), check that the reference
154
* previously had this value:
155
*/
156
unsigned char old_sha1[20];
157
+
158
/*
159
* One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF,
160
* REF_DELETING, REF_ISPRUNING, REF_LOG_ONLY, and
161
* REF_UPDATE_VIA_HEAD:
162
*/
163
unsigned int flags;
164
+
165
struct ref_lock *lock;
166
unsigned int type;
167
char *msg;
168
+
169
+ /*
170
+ * If this ref_update was split off of a symref update via
171
+ * split_symref_update(), then this member points at that
172
+ * update. This is used for two purposes:
173
+ * 1. When reporting errors, we report the refname under which
174
+ * the update was originally requested.
175
+ * 2. When we read the old value of this reference, we
176
+ * propagate it back to its parent update for recording in
177
+ * the latter's reflog.
178
+ */
179
+ struct ref_update *parent_update;
180
+
181
const char refname[FLEX_ARRAY];
182
};
183