files_ref_store: use a transaction to update packed refs
When processing a `files_ref_store` transaction, it is sometimes necessary to delete some references from the "packed-refs" file. Do that using a reference transaction conducted against the `packed_ref_store`. This change further decouples `files_ref_store` from `packed_ref_store`. It also fixes multiple problems, including the two revealed by test cases added in the previous commit. First, the old code didn't obtain the `packed-refs` lock until `files_transaction_finish()`. This means that a failure to acquire the `packed-refs` lock (e.g., due to contention with another process) wasn't detected until it was too late (problems like this are supposed to be detected in the "prepare" phase). The new code acquires the `packed-refs` lock in `files_transaction_prepare()`, the same stage of the processing when the loose reference locks are being acquired, removing another reason why the "prepare" phase might succeed and the "finish" phase might nevertheless fail. Second, the old code deleted the loose version of a reference before deleting any packed version of the same reference. This left a moment when another process might think that the packed version of the reference is current, which is incorrect. (Even worse, the packed version of the reference can be arbitrarily old, and might even point at an object that has since been garbage-collected.) Third, if a reference deletion fails to acquire the `packed-refs` lock altogether, then the old code might leave the repository in the incorrect state (possibly corrupt) described in the previous paragraph. Now we activate the new "packed-refs" file (sans any references that are being deleted) *before* deleting the corresponding loose references. But we hold the "packed-refs" lock until after the loose references have been finalized, thus preventing a simultaneous "pack-refs" process from packing the loose version of the reference in the time gap, which would otherwise defeat our attempt to delete it. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Michael Haggerty committed
Sep 8, 2017 at 15:51 UTC
dc39e099422b1d44f6230f557f94f7945c7521a7
2 files changed
+103
-33
refs/files-backend.c
+101
-31
@@ -2397,13 +2397,22 @@ static int lock_ref_for_update(struct files_ref_store *refs,
2397
return 0;
2398
}
2399
2400
+struct files_transaction_backend_data {
2401
+ struct ref_transaction *packed_transaction;
2402
+ int packed_refs_locked;
2403
+};
2404
+
2405
/*
2406
* Unlock any references in `transaction` that are still locked, and
2407
* mark the transaction closed.
2408
*/
2404
-static void files_transaction_cleanup(struct ref_transaction *transaction)
2409
+static void files_transaction_cleanup(struct files_ref_store *refs,
2410
+ struct ref_transaction *transaction)
2411
{
2412
size_t i;
2413
+ struct files_transaction_backend_data *backend_data =
2414
+ transaction->backend_data;
2415
+ struct strbuf err = STRBUF_INIT;
2416
2417
for (i = 0; i < transaction->nr; i++) {
2418
struct ref_update *update = transaction->updates[i];
@@ -2415,6 +2424,17 @@ static void files_transaction_cleanup(struct ref_transaction *transaction)
2424
}
2425
}
2426
2427
+ if (backend_data->packed_transaction &&
2428
+ ref_transaction_abort(backend_data->packed_transaction, &err)) {
2429
+ error("error aborting transaction: %s", err.buf);
2430
+ strbuf_release(&err);
2431
+ }
2432
+
2433
+ if (backend_data->packed_refs_locked)
2434
+ packed_refs_unlock(refs->packed_ref_store);
2435
+
2436
+ free(backend_data);
2437
+
2438
transaction->state = REF_TRANSACTION_CLOSED;
2439
}
2440
@@ -2431,12 +2451,17 @@ static int files_transaction_prepare(struct ref_store *ref_store,
2451
char *head_ref = NULL;
2452
int head_type;
2453
struct object_id head_oid;
2454
+ struct files_transaction_backend_data *backend_data;
2455
+ struct ref_transaction *packed_transaction = NULL;
2456
2457
assert(err);
2458
2459
if (!transaction->nr)
2460
goto cleanup;
2461
2462
+ backend_data = xcalloc(1, sizeof(*backend_data));
2463
+ transaction->backend_data = backend_data;
2464
+
2465
/*
2466
* Fail if a refname appears more than once in the
2467
* transaction. (If we end up splitting up any updates using
@@ -2503,6 +2528,41 @@ static int files_transaction_prepare(struct ref_store *ref_store,
2528
head_ref, &affected_refnames, err);
2529
if (ret)
2530
break;
2531
+
2532
+ if (update->flags & REF_DELETING &&
2533
+ !(update->flags & REF_LOG_ONLY) &&
2534
+ !(update->flags & REF_ISPRUNING)) {
2535
+ /*
2536
+ * This reference has to be deleted from
2537
+ * packed-refs if it exists there.
2538
+ */
2539
+ if (!packed_transaction) {
2540
+ packed_transaction = ref_store_transaction_begin(
2541
+ refs->packed_ref_store, err);
2542
+ if (!packed_transaction) {
2543
+ ret = TRANSACTION_GENERIC_ERROR;
2544
+ goto cleanup;
2545
+ }
2546
+
2547
+ backend_data->packed_transaction =
2548
+ packed_transaction;
2549
+ }
2550
+
2551
+ ref_transaction_add_update(
2552
+ packed_transaction, update->refname,
2553
+ update->flags & ~REF_HAVE_OLD,
2554
+ update->new_oid.hash, update->old_oid.hash,
2555
+ NULL);
2556
+ }
2557
+ }
2558
+
2559
+ if (packed_transaction) {
2560
+ if (packed_refs_lock(refs->packed_ref_store, 0, err)) {
2561
+ ret = TRANSACTION_GENERIC_ERROR;
2562
+ goto cleanup;
2563
+ }
2564
+ backend_data->packed_refs_locked = 1;
2565
+ ret = ref_transaction_prepare(packed_transaction, err);
2566
}
2567
2568
cleanup:
@@ -2510,7 +2570,7 @@ cleanup:
2570
string_list_clear(&affected_refnames, 0);
2571
2572
if (ret)
2513
- files_transaction_cleanup(transaction);
2573
+ files_transaction_cleanup(refs, transaction);
2574
else
2575
transaction->state = REF_TRANSACTION_PREPARED;
2576
@@ -2525,9 +2585,10 @@ static int files_transaction_finish(struct ref_store *ref_store,
2585
files_downcast(ref_store, 0, "ref_transaction_finish");
2586
size_t i;
2587
int ret = 0;
2528
- struct string_list refs_to_delete = STRING_LIST_INIT_NODUP;
2529
- struct string_list_item *ref_to_delete;
2588
struct strbuf sb = STRBUF_INIT;
2589
+ struct files_transaction_backend_data *backend_data;
2590
+ struct ref_transaction *packed_transaction;
2591
+
2592
2593
assert(err);
2594
@@ -2536,6 +2597,9 @@ static int files_transaction_finish(struct ref_store *ref_store,
2597
return 0;
2598
}
2599
2600
+ backend_data = transaction->backend_data;
2601
+ packed_transaction = backend_data->packed_transaction;
2602
+
2603
/* Perform updates first so live commits remain referenced */
2604
for (i = 0; i < transaction->nr; i++) {
2605
struct ref_update *update = transaction->updates[i];
@@ -2571,7 +2635,23 @@ static int files_transaction_finish(struct ref_store *ref_store,
2635
}
2636
}
2637
}
2574
- /* Perform deletes now that updates are safely completed */
2638
+
2639
+ /*
2640
+ * Perform deletes now that updates are safely completed.
2641
+ *
2642
+ * First delete any packed versions of the references, while
2643
+ * retaining the packed-refs lock:
2644
+ */
2645
+ if (packed_transaction) {
2646
+ ret = ref_transaction_commit(packed_transaction, err);
2647
+ ref_transaction_free(packed_transaction);
2648
+ packed_transaction = NULL;
2649
+ backend_data->packed_transaction = NULL;
2650
+ if (ret)
2651
+ goto cleanup;
2652
+ }
2653
+
2654
+ /* Now delete the loose versions of the references: */
2655
for (i = 0; i < transaction->nr; i++) {
2656
struct ref_update *update = transaction->updates[i];
2657
struct ref_lock *lock = update->backend_data;
@@ -2589,39 +2669,27 @@ static int files_transaction_finish(struct ref_store *ref_store,
2669
}
2670
update->flags |= REF_DELETED_LOOSE;
2671
}
2592
-
2593
- if (!(update->flags & REF_ISPRUNING))
2594
- string_list_append(&refs_to_delete,
2595
- lock->ref_name);
2672
}
2673
}
2674
2599
- if (packed_refs_lock(refs->packed_ref_store, 0, err)) {
2600
- ret = TRANSACTION_GENERIC_ERROR;
2601
- goto cleanup;
2602
- }
2603
-
2604
- if (repack_without_refs(refs->packed_ref_store, &refs_to_delete, err)) {
2605
- ret = TRANSACTION_GENERIC_ERROR;
2606
- packed_refs_unlock(refs->packed_ref_store);
2607
- goto cleanup;
2608
- }
2609
-
2610
- packed_refs_unlock(refs->packed_ref_store);
2611
-
2675
/* Delete the reflogs of any references that were deleted: */
2613
- for_each_string_list_item(ref_to_delete, &refs_to_delete) {
2614
- strbuf_reset(&sb);
2615
- files_reflog_path(refs, &sb, ref_to_delete->string);
2616
- if (!unlink_or_warn(sb.buf))
2617
- try_remove_empty_parents(refs, ref_to_delete->string,
2618
- REMOVE_EMPTY_PARENTS_REFLOG);
2676
+ for (i = 0; i < transaction->nr; i++) {
2677
+ struct ref_update *update = transaction->updates[i];
2678
+ if (update->flags & REF_DELETING &&
2679
+ !(update->flags & REF_LOG_ONLY) &&
2680
+ !(update->flags & REF_ISPRUNING)) {
2681
+ strbuf_reset(&sb);
2682
+ files_reflog_path(refs, &sb, update->refname);
2683
+ if (!unlink_or_warn(sb.buf))
2684
+ try_remove_empty_parents(refs, update->refname,
2685
+ REMOVE_EMPTY_PARENTS_REFLOG);
2686
+ }
2687
}
2688
2689
clear_loose_ref_cache(refs);
2690
2691
cleanup:
2624
- files_transaction_cleanup(transaction);
2692
+ files_transaction_cleanup(refs, transaction);
2693
2694
for (i = 0; i < transaction->nr; i++) {
2695
struct ref_update *update = transaction->updates[i];
@@ -2639,7 +2707,6 @@ cleanup:
2707
}
2708
2709
strbuf_release(&sb);
2642
- string_list_clear(&refs_to_delete, 0);
2710
return ret;
2711
}
2712
@@ -2647,7 +2714,10 @@ static int files_transaction_abort(struct ref_store *ref_store,
2714
struct ref_transaction *transaction,
2715
struct strbuf *err)
2716
{
2650
- files_transaction_cleanup(transaction);
2717
+ struct files_ref_store *refs =
2718
+ files_downcast(ref_store, 0, "ref_transaction_abort");
2719
+
2720
+ files_transaction_cleanup(refs, transaction);
2721
return 0;
2722
}
2723
t/t1404-update-ref-errors.sh
+2
-2
@@ -404,7 +404,7 @@ test_expect_success 'broken reference blocks indirect create' '
404
test_cmp expected output.err
405
'
406
407
-test_expect_failure 'no bogus intermediate values during delete' '
407
+test_expect_success 'no bogus intermediate values during delete' '
408
prefix=refs/slow-transaction &&
409
# Set up a reference with differing loose and packed versions:
410
git update-ref $prefix/foo $C &&
@@ -461,7 +461,7 @@ test_expect_failure 'no bogus intermediate values during delete' '
461
test_must_fail git rev-parse --verify --quiet $prefix/foo
462
'
463
464
-test_expect_failure 'delete fails cleanly if packed-refs file is locked' '
464
+test_expect_success 'delete fails cleanly if packed-refs file is locked' '
465
prefix=refs/locked-packed-refs &&
466
# Set up a reference with differing loose and packed versions:
467
git update-ref $prefix/foo $C &&