From 2fb330ca7238088eea5c1926380feb187f4867bc Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 8 Sep 2017 15:51:46 +0200 Subject: packed_delete_refs(): implement method Implement `packed_delete_refs()` using a reference transaction. This means that `files_delete_refs()` can use `refs_delete_refs()` instead of `repack_without_refs()` to delete any packed references, decreasing the coupling between the classes. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index fccbc24ac4..2c78f63494 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1157,7 +1157,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg, if (packed_refs_lock(refs->packed_ref_store, 0, &err)) goto error; - if (repack_without_refs(refs->packed_ref_store, refnames, &err)) { + if (refs_delete_refs(refs->packed_ref_store, msg, refnames, flags)) { packed_refs_unlock(refs->packed_ref_store); goto error; } -- cgit v1.2.1 From 27d03d04d5b7e86a260d7afa283c9905f64de350 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 8 Sep 2017 15:51:47 +0200 Subject: files_pack_refs(): use a reference transaction to write packed refs Now that the packed reference store supports transactions, we can use a transaction to write the packed versions of references that we want to pack. This decreases the coupling between `files_ref_store` and `packed_ref_store`. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 2c78f63494..3475c6f8a2 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1100,6 +1100,11 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags) int ok; struct ref_to_prune *refs_to_prune = NULL; struct strbuf err = STRBUF_INIT; + struct ref_transaction *transaction; + + transaction = ref_store_transaction_begin(refs->packed_ref_store, &err); + if (!transaction) + return -1; packed_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR, &err); @@ -1115,12 +1120,14 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags) continue; /* - * Create an entry in the packed-refs cache equivalent - * to the one from the loose ref cache, except that - * we don't copy the peeled status, because we want it - * to be re-peeled. + * Add a reference creation for this reference to the + * packed-refs transaction: */ - add_packed_ref(refs->packed_ref_store, iter->refname, iter->oid); + if (ref_transaction_update(transaction, iter->refname, + iter->oid->hash, NULL, + REF_NODEREF, NULL, &err)) + die("failure preparing to create packed reference %s: %s", + iter->refname, err.buf); /* Schedule the loose reference for pruning if requested. */ if ((flags & PACK_REFS_PRUNE)) { @@ -1134,8 +1141,11 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags) if (ok != ITER_DONE) die("error while iterating over references"); - if (commit_packed_refs(refs->packed_ref_store, &err)) - die("unable to overwrite old ref-pack file: %s", err.buf); + if (ref_transaction_commit(transaction, &err)) + die("unable to write new packed-refs: %s", err.buf); + + ref_transaction_free(transaction); + packed_refs_unlock(refs->packed_ref_store); prune_refs(refs, refs_to_prune); -- cgit v1.2.1 From 22b09cdfadf7a48f6503fddf51082c66541cf1d6 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 8 Sep 2017 15:51:48 +0200 Subject: prune_refs(): also free the linked list At least since v1.7, the elements of the `refs_to_prune` linked list have been leaked. Fix the leak by teaching `prune_refs()` to free the list elements as it processes them. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 3475c6f8a2..60031fe3ae 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1057,11 +1057,17 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r) strbuf_release(&err); } -static void prune_refs(struct files_ref_store *refs, struct ref_to_prune *r) +/* + * Prune the loose versions of the references in the linked list + * `*refs_to_prune`, freeing the entries in the list as we go. + */ +static void prune_refs(struct files_ref_store *refs, struct ref_to_prune **refs_to_prune) { - while (r) { + while (*refs_to_prune) { + struct ref_to_prune *r = *refs_to_prune; + *refs_to_prune = r->next; prune_ref(refs, r); - r = r->next; + free(r); } } @@ -1148,7 +1154,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags) packed_refs_unlock(refs->packed_ref_store); - prune_refs(refs, refs_to_prune); + prune_refs(refs, &refs_to_prune); strbuf_release(&err); return 0; } -- cgit v1.2.1 From 1444bfe0271a1dd46283a298e1a43c6565c38271 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 8 Sep 2017 15:51:49 +0200 Subject: files_initial_transaction_commit(): use a transaction for packed refs Use a `packed_ref_store` transaction in the implementation of `files_initial_transaction_commit()` rather than using internal features of the packed ref store. This further decouples `files_ref_store` from `packed_ref_store`. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 60031fe3ae..2700e3b5d5 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2669,6 +2669,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store, size_t i; int ret = 0; struct string_list affected_refnames = STRING_LIST_INIT_NODUP; + struct ref_transaction *packed_transaction = NULL; assert(err); @@ -2701,6 +2702,12 @@ static int files_initial_transaction_commit(struct ref_store *ref_store, &affected_refnames)) die("BUG: initial ref transaction called with existing refs"); + packed_transaction = ref_store_transaction_begin(refs->packed_ref_store, err); + if (!packed_transaction) { + ret = TRANSACTION_GENERIC_ERROR; + goto cleanup; + } + for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; @@ -2713,6 +2720,15 @@ static int files_initial_transaction_commit(struct ref_store *ref_store, ret = TRANSACTION_NAME_CONFLICT; goto cleanup; } + + /* + * Add a reference creation for this reference to the + * packed-refs transaction: + */ + ref_transaction_add_update(packed_transaction, update->refname, + update->flags & ~REF_HAVE_OLD, + update->new_oid.hash, update->old_oid.hash, + NULL); } if (packed_refs_lock(refs->packed_ref_store, 0, err)) { @@ -2720,21 +2736,14 @@ static int files_initial_transaction_commit(struct ref_store *ref_store, goto cleanup; } - for (i = 0; i < transaction->nr; i++) { - struct ref_update *update = transaction->updates[i]; - - if ((update->flags & REF_HAVE_NEW) && - !is_null_oid(&update->new_oid)) - add_packed_ref(refs->packed_ref_store, update->refname, - &update->new_oid); - } - - if (commit_packed_refs(refs->packed_ref_store, err)) { + if (initial_ref_transaction_commit(packed_transaction, err)) { ret = TRANSACTION_GENERIC_ERROR; goto cleanup; } cleanup: + if (packed_transaction) + ref_transaction_free(packed_transaction); packed_refs_unlock(refs->packed_ref_store); transaction->state = REF_TRANSACTION_CLOSED; string_list_clear(&affected_refnames, 0); -- cgit v1.2.1 From dc39e099422b1d44f6230f557f94f7945c7521a7 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 8 Sep 2017 15:51:51 +0200 Subject: 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 Signed-off-by: Junio C Hamano --- refs/files-backend.c | 132 +++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 101 insertions(+), 31 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 2700e3b5d5..29eb5e826f 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2397,13 +2397,22 @@ static int lock_ref_for_update(struct files_ref_store *refs, return 0; } +struct files_transaction_backend_data { + struct ref_transaction *packed_transaction; + int packed_refs_locked; +}; + /* * Unlock any references in `transaction` that are still locked, and * mark the transaction closed. */ -static void files_transaction_cleanup(struct ref_transaction *transaction) +static void files_transaction_cleanup(struct files_ref_store *refs, + struct ref_transaction *transaction) { size_t i; + struct files_transaction_backend_data *backend_data = + transaction->backend_data; + struct strbuf err = STRBUF_INIT; for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; @@ -2415,6 +2424,17 @@ static void files_transaction_cleanup(struct ref_transaction *transaction) } } + if (backend_data->packed_transaction && + ref_transaction_abort(backend_data->packed_transaction, &err)) { + error("error aborting transaction: %s", err.buf); + strbuf_release(&err); + } + + if (backend_data->packed_refs_locked) + packed_refs_unlock(refs->packed_ref_store); + + free(backend_data); + transaction->state = REF_TRANSACTION_CLOSED; } @@ -2431,12 +2451,17 @@ static int files_transaction_prepare(struct ref_store *ref_store, char *head_ref = NULL; int head_type; struct object_id head_oid; + struct files_transaction_backend_data *backend_data; + struct ref_transaction *packed_transaction = NULL; assert(err); if (!transaction->nr) goto cleanup; + backend_data = xcalloc(1, sizeof(*backend_data)); + transaction->backend_data = backend_data; + /* * Fail if a refname appears more than once in the * transaction. (If we end up splitting up any updates using @@ -2503,6 +2528,41 @@ static int files_transaction_prepare(struct ref_store *ref_store, head_ref, &affected_refnames, err); if (ret) break; + + if (update->flags & REF_DELETING && + !(update->flags & REF_LOG_ONLY) && + !(update->flags & REF_ISPRUNING)) { + /* + * This reference has to be deleted from + * packed-refs if it exists there. + */ + if (!packed_transaction) { + packed_transaction = ref_store_transaction_begin( + refs->packed_ref_store, err); + if (!packed_transaction) { + ret = TRANSACTION_GENERIC_ERROR; + goto cleanup; + } + + backend_data->packed_transaction = + packed_transaction; + } + + ref_transaction_add_update( + packed_transaction, update->refname, + update->flags & ~REF_HAVE_OLD, + update->new_oid.hash, update->old_oid.hash, + NULL); + } + } + + if (packed_transaction) { + if (packed_refs_lock(refs->packed_ref_store, 0, err)) { + ret = TRANSACTION_GENERIC_ERROR; + goto cleanup; + } + backend_data->packed_refs_locked = 1; + ret = ref_transaction_prepare(packed_transaction, err); } cleanup: @@ -2510,7 +2570,7 @@ cleanup: string_list_clear(&affected_refnames, 0); if (ret) - files_transaction_cleanup(transaction); + files_transaction_cleanup(refs, transaction); else transaction->state = REF_TRANSACTION_PREPARED; @@ -2525,9 +2585,10 @@ static int files_transaction_finish(struct ref_store *ref_store, files_downcast(ref_store, 0, "ref_transaction_finish"); size_t i; int ret = 0; - struct string_list refs_to_delete = STRING_LIST_INIT_NODUP; - struct string_list_item *ref_to_delete; struct strbuf sb = STRBUF_INIT; + struct files_transaction_backend_data *backend_data; + struct ref_transaction *packed_transaction; + assert(err); @@ -2536,6 +2597,9 @@ static int files_transaction_finish(struct ref_store *ref_store, return 0; } + backend_data = transaction->backend_data; + packed_transaction = backend_data->packed_transaction; + /* Perform updates first so live commits remain referenced */ for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; @@ -2571,7 +2635,23 @@ static int files_transaction_finish(struct ref_store *ref_store, } } } - /* Perform deletes now that updates are safely completed */ + + /* + * Perform deletes now that updates are safely completed. + * + * First delete any packed versions of the references, while + * retaining the packed-refs lock: + */ + if (packed_transaction) { + ret = ref_transaction_commit(packed_transaction, err); + ref_transaction_free(packed_transaction); + packed_transaction = NULL; + backend_data->packed_transaction = NULL; + if (ret) + goto cleanup; + } + + /* Now delete the loose versions of the references: */ for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; struct ref_lock *lock = update->backend_data; @@ -2589,39 +2669,27 @@ static int files_transaction_finish(struct ref_store *ref_store, } update->flags |= REF_DELETED_LOOSE; } - - if (!(update->flags & REF_ISPRUNING)) - string_list_append(&refs_to_delete, - lock->ref_name); } } - if (packed_refs_lock(refs->packed_ref_store, 0, err)) { - ret = TRANSACTION_GENERIC_ERROR; - goto cleanup; - } - - if (repack_without_refs(refs->packed_ref_store, &refs_to_delete, err)) { - ret = TRANSACTION_GENERIC_ERROR; - packed_refs_unlock(refs->packed_ref_store); - goto cleanup; - } - - packed_refs_unlock(refs->packed_ref_store); - /* Delete the reflogs of any references that were deleted: */ - for_each_string_list_item(ref_to_delete, &refs_to_delete) { - strbuf_reset(&sb); - files_reflog_path(refs, &sb, ref_to_delete->string); - if (!unlink_or_warn(sb.buf)) - try_remove_empty_parents(refs, ref_to_delete->string, - REMOVE_EMPTY_PARENTS_REFLOG); + for (i = 0; i < transaction->nr; i++) { + struct ref_update *update = transaction->updates[i]; + if (update->flags & REF_DELETING && + !(update->flags & REF_LOG_ONLY) && + !(update->flags & REF_ISPRUNING)) { + strbuf_reset(&sb); + files_reflog_path(refs, &sb, update->refname); + if (!unlink_or_warn(sb.buf)) + try_remove_empty_parents(refs, update->refname, + REMOVE_EMPTY_PARENTS_REFLOG); + } } clear_loose_ref_cache(refs); cleanup: - files_transaction_cleanup(transaction); + files_transaction_cleanup(refs, transaction); for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; @@ -2639,7 +2707,6 @@ cleanup: } strbuf_release(&sb); - string_list_clear(&refs_to_delete, 0); return ret; } @@ -2647,7 +2714,10 @@ static int files_transaction_abort(struct ref_store *ref_store, struct ref_transaction *transaction, struct strbuf *err) { - files_transaction_cleanup(transaction); + struct files_ref_store *refs = + files_downcast(ref_store, 0, "ref_transaction_abort"); + + files_transaction_cleanup(refs, transaction); return 0; } -- cgit v1.2.1 From 5e00a6c873981f87165adfecf29ad0ecc2c2c5df Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 8 Sep 2017 15:51:53 +0200 Subject: files_transaction_finish(): delete reflogs before references If the deletion steps unexpectedly fail, it is less bad to leave a reference without its reflog than it is to leave a reflog without its reference, since the latter is an invalid repository state. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 29eb5e826f..961424a4ea 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2636,6 +2636,27 @@ static int files_transaction_finish(struct ref_store *ref_store, } } + /* + * Now that updates are safely completed, we can perform + * deletes. First delete the reflogs of any references that + * will be deleted, since (in the unexpected event of an + * error) leaving a reference without a reflog is less bad + * than leaving a reflog without a reference (the latter is a + * mildly invalid repository state): + */ + for (i = 0; i < transaction->nr; i++) { + struct ref_update *update = transaction->updates[i]; + if (update->flags & REF_DELETING && + !(update->flags & REF_LOG_ONLY) && + !(update->flags & REF_ISPRUNING)) { + strbuf_reset(&sb); + files_reflog_path(refs, &sb, update->refname); + if (!unlink_or_warn(sb.buf)) + try_remove_empty_parents(refs, update->refname, + REMOVE_EMPTY_PARENTS_REFLOG); + } + } + /* * Perform deletes now that updates are safely completed. * @@ -2672,20 +2693,6 @@ static int files_transaction_finish(struct ref_store *ref_store, } } - /* Delete the reflogs of any references that were deleted: */ - for (i = 0; i < transaction->nr; i++) { - struct ref_update *update = transaction->updates[i]; - if (update->flags & REF_DELETING && - !(update->flags & REF_LOG_ONLY) && - !(update->flags & REF_ISPRUNING)) { - strbuf_reset(&sb); - files_reflog_path(refs, &sb, update->refname); - if (!unlink_or_warn(sb.buf)) - try_remove_empty_parents(refs, update->refname, - REMOVE_EMPTY_PARENTS_REFLOG); - } - } - clear_loose_ref_cache(refs); cleanup: -- cgit v1.2.1