lock_ref_for_update(): make error handling more uniform
To aid the effort, extract a new function, check_old_oid(), and use it in the two places where the read value of the reference has to be checked against update->old_sha1. Update tests to reflect the improvements. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Michael Haggerty committed
Jun 7, 2016 at 09:29 UTC
e3f510393c9d373f2969badc2b8afe179803a0fa
2 files changed
+49
-39
refs/files-backend.c
+42
-32
@@ -3388,6 +3388,38 @@ static const char *original_update_refname(struct ref_update *update)
3388
return update->refname;
3389
}
3390
3391
+/*
3392
+ * Check whether the REF_HAVE_OLD and old_oid values stored in update
3393
+ * are consistent with oid, which is the reference's current value. If
3394
+ * everything is OK, return 0; otherwise, write an error message to
3395
+ * err and return -1.
3396
+ */
3397
+static int check_old_oid(struct ref_update *update, struct object_id *oid,
3398
+ struct strbuf *err)
3399
+{
3400
+ if (!(update->flags & REF_HAVE_OLD) ||
3401
+ !hashcmp(oid->hash, update->old_sha1))
3402
+ return 0;
3403
+
3404
+ if (is_null_sha1(update->old_sha1))
3405
+ strbuf_addf(err, "cannot lock ref '%s': "
3406
+ "reference already exists",
3407
+ original_update_refname(update));
3408
+ else if (is_null_oid(oid))
3409
+ strbuf_addf(err, "cannot lock ref '%s': "
3410
+ "reference is missing but expected %s",
3411
+ original_update_refname(update),
3412
+ sha1_to_hex(update->old_sha1));
3413
+ else
3414
+ strbuf_addf(err, "cannot lock ref '%s': "
3415
+ "is at %s but expected %s",
3416
+ original_update_refname(update),
3417
+ oid_to_hex(oid),
3418
+ sha1_to_hex(update->old_sha1));
3419
+
3420
+ return -1;
3421
+}
3422
+
3423
/*
3424
* Prepare for carrying out update:
3425
* - Lock the reference referred to by update.
@@ -3433,7 +3465,7 @@ static int lock_ref_for_update(struct ref_update *update,
3465
3466
reason = strbuf_detach(err, NULL);
3467
strbuf_addf(err, "cannot lock ref '%s': %s",
3436
- update->refname, reason);
3468
+ original_update_refname(update), reason);
3469
free(reason);
3470
return ret;
3471
}
@@ -3447,28 +3479,17 @@ static int lock_ref_for_update(struct ref_update *update,
3479
* the transaction, so we have to read it here
3480
* to record and possibly check old_sha1:
3481
*/
3450
- if (read_ref_full(update->refname,
3451
- mustexist ? RESOLVE_REF_READING : 0,
3482
+ if (read_ref_full(update->refname, 0,
3483
lock->old_oid.hash, NULL)) {
3484
if (update->flags & REF_HAVE_OLD) {
3485
strbuf_addf(err, "cannot lock ref '%s': "
3455
- "can't resolve old value",
3456
- update->refname);
3457
- return TRANSACTION_GENERIC_ERROR;
3458
- } else {
3459
- hashclr(lock->old_oid.hash);
3486
+ "error reading reference",
3487
+ original_update_refname(update));
3488
+ return -1;
3489
}
3461
- }
3462
- if ((update->flags & REF_HAVE_OLD) &&
3463
- hashcmp(lock->old_oid.hash, update->old_sha1)) {
3464
- strbuf_addf(err, "cannot lock ref '%s': "
3465
- "is at %s but expected %s",
3466
- update->refname,
3467
- sha1_to_hex(lock->old_oid.hash),
3468
- sha1_to_hex(update->old_sha1));
3490
+ } else if (check_old_oid(update, &lock->old_oid, err)) {
3491
return TRANSACTION_GENERIC_ERROR;
3492
}
3471
-
3493
} else {
3494
/*
3495
* Create a new update for the reference this
@@ -3485,6 +3506,9 @@ static int lock_ref_for_update(struct ref_update *update,
3506
} else {
3507
struct ref_update *parent_update;
3508
3509
+ if (check_old_oid(update, &lock->old_oid, err))
3510
+ return TRANSACTION_GENERIC_ERROR;
3511
+
3512
/*
3513
* If this update is happening indirectly because of a
3514
* symref update, record the old SHA-1 in the parent
@@ -3495,20 +3519,6 @@ static int lock_ref_for_update(struct ref_update *update,
3519
parent_update = parent_update->parent_update) {
3520
oidcpy(&parent_update->lock->old_oid, &lock->old_oid);
3521
}
3498
-
3499
- if ((update->flags & REF_HAVE_OLD) &&
3500
- hashcmp(lock->old_oid.hash, update->old_sha1)) {
3501
- if (is_null_sha1(update->old_sha1))
3502
- strbuf_addf(err, "cannot lock ref '%s': reference already exists",
3503
- original_update_refname(update));
3504
- else
3505
- strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
3506
- original_update_refname(update),
3507
- sha1_to_hex(lock->old_oid.hash),
3508
- sha1_to_hex(update->old_sha1));
3509
-
3510
- return TRANSACTION_GENERIC_ERROR;
3511
- }
3522
}
3523
3524
if ((update->flags & REF_HAVE_NEW) &&
@@ -3530,7 +3540,7 @@ static int lock_ref_for_update(struct ref_update *update,
3540
*/
3541
update->lock = NULL;
3542
strbuf_addf(err,
3533
- "cannot update the ref '%s': %s",
3543
+ "cannot update ref '%s': %s",
3544
update->refname, write_err);
3545
free(write_err);
3546
return TRANSACTION_GENERIC_ERROR;
t/t1404-update-ref-errors.sh
+7
-7
@@ -237,7 +237,7 @@ test_expect_success 'missing old value blocks indirect update' '
237
prefix=refs/missing-indirect-update &&
238
git symbolic-ref $prefix/symref $prefix/foo &&
239
cat >expected <<-EOF &&
240
- fatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q
240
+ fatal: cannot lock ref $Q$prefix/symref$Q: unable to resolve reference $Q$prefix/foo$Q
241
EOF
242
printf "%s\n" "update $prefix/symref $E $D" |
243
test_must_fail git update-ref --stdin 2>output.err &&
@@ -284,7 +284,7 @@ test_expect_success 'missing old value blocks indirect no-deref update' '
284
prefix=refs/missing-noderef-update &&
285
git symbolic-ref $prefix/symref $prefix/foo &&
286
cat >expected <<-EOF &&
287
- fatal: cannot lock ref $Q$prefix/symref$Q: can${Q}t resolve old value
287
+ fatal: cannot lock ref $Q$prefix/symref$Q: reference is missing but expected $D
288
EOF
289
printf "%s\n" "option no-deref" "update $prefix/symref $E $D" |
290
test_must_fail git update-ref --stdin 2>output.err &&
@@ -303,7 +303,7 @@ test_expect_success 'incorrect old value blocks indirect no-deref update' '
303
test_cmp expected output.err
304
'
305
306
-test_expect_failure 'existing old value blocks indirect no-deref create' '
306
+test_expect_success 'existing old value blocks indirect no-deref create' '
307
prefix=refs/existing-noderef-create &&
308
git symbolic-ref $prefix/symref $prefix/foo &&
309
git update-ref $prefix/foo $C &&
@@ -372,13 +372,13 @@ test_expect_success 'non-empty directory blocks indirect create' '
372
: >.git/$prefix/foo/bar/baz.lock &&
373
test_when_finished "rm -f .git/$prefix/foo/bar/baz.lock" &&
374
cat >expected <<-EOF &&
375
- fatal: cannot lock ref $Q$prefix/foo$Q: there is a non-empty directory $Q.git/$prefix/foo$Q blocking reference $Q$prefix/foo$Q
375
+ fatal: cannot lock ref $Q$prefix/symref$Q: there is a non-empty directory $Q.git/$prefix/foo$Q blocking reference $Q$prefix/foo$Q
376
EOF
377
printf "%s\n" "update $prefix/symref $C" |
378
test_must_fail git update-ref --stdin 2>output.err &&
379
test_cmp expected output.err &&
380
cat >expected <<-EOF &&
381
- fatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q
381
+ fatal: cannot lock ref $Q$prefix/symref$Q: unable to resolve reference $Q$prefix/foo$Q
382
EOF
383
printf "%s\n" "update $prefix/symref $D $C" |
384
test_must_fail git update-ref --stdin 2>output.err &&
@@ -391,13 +391,13 @@ test_expect_success 'broken reference blocks indirect create' '
391
echo "gobbledigook" >.git/$prefix/foo &&
392
test_when_finished "rm -f .git/$prefix/foo" &&
393
cat >expected <<-EOF &&
394
- fatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken
394
+ fatal: cannot lock ref $Q$prefix/symref$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken
395
EOF
396
printf "%s\n" "update $prefix/symref $C" |
397
test_must_fail git update-ref --stdin 2>output.err &&
398
test_cmp expected output.err &&
399
cat >expected <<-EOF &&
400
- fatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken
400
+ fatal: cannot lock ref $Q$prefix/symref$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken
401
EOF
402
printf "%s\n" "update $prefix/symref $D $C" |
403
test_must_fail git update-ref --stdin 2>output.err &&