From 04aea8d4df199836da3802f08cb5738eae66fa6c Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:34 +0200 Subject: files-backend: use `die("BUG: ...")`, not `die("internal error: ...")` The former is by far more common in our codebase. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index cb1f528cbe..fa5d2b6f08 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -105,7 +105,7 @@ static void clear_packed_ref_cache(struct files_ref_store *refs) struct packed_ref_cache *packed_refs = refs->packed; if (packed_refs->lock) - die("internal error: packed-ref cache cleared while locked"); + die("BUG: packed-ref cache cleared while locked"); refs->packed = NULL; release_packed_ref_cache(packed_refs); } @@ -397,7 +397,7 @@ static void add_packed_ref(struct files_ref_store *refs, struct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs); if (!packed_ref_cache->lock) - die("internal error: packed refs not locked"); + die("BUG: packed refs not locked"); add_ref_entry(get_packed_ref_dir(packed_ref_cache), create_ref_entry(refname, oid, REF_ISPACKED, 1)); } @@ -1324,7 +1324,7 @@ static int commit_packed_refs(struct files_ref_store *refs) files_assert_main_repository(refs, "commit_packed_refs"); if (!packed_ref_cache->lock) - die("internal error: packed-refs not locked"); + die("BUG: packed-refs not locked"); out = fdopen_lock_file(packed_ref_cache->lock, "w"); if (!out) @@ -1367,7 +1367,7 @@ static void rollback_packed_refs(struct files_ref_store *refs) files_assert_main_repository(refs, "rollback_packed_refs"); if (!packed_ref_cache->lock) - die("internal error: packed-refs not locked"); + die("BUG: packed-refs not locked"); rollback_lock_file(packed_ref_cache->lock); packed_ref_cache->lock = NULL; release_packed_ref_cache(packed_ref_cache); -- cgit 1.2.3-korg From 43a2dfde76a4a47ffa31be11fd5cd7fe0b57bb84 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:37 +0200 Subject: refs: use `size_t` indexes when iterating over ref transaction updates Eliminate any chance of integer overflow on platforms where the two types have different sizes. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 2 +- refs/files-backend.c | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs.c b/refs.c index 8494b1f20d..71139ba74e 100644 --- a/refs.c +++ b/refs.c @@ -848,7 +848,7 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err) void ref_transaction_free(struct ref_transaction *transaction) { - int i; + size_t i; if (!transaction) return; diff --git a/refs/files-backend.c b/refs/files-backend.c index fa5d2b6f08..b2559b5585 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2850,7 +2850,8 @@ static int files_transaction_commit(struct ref_store *ref_store, struct files_ref_store *refs = files_downcast(ref_store, REF_STORE_WRITE, "ref_transaction_commit"); - int ret = 0, i; + size_t i; + int ret = 0; struct string_list refs_to_delete = STRING_LIST_INIT_NODUP; struct string_list_item *ref_to_delete; struct string_list affected_refnames = STRING_LIST_INIT_NODUP; @@ -3057,7 +3058,8 @@ static int files_initial_transaction_commit(struct ref_store *ref_store, struct files_ref_store *refs = files_downcast(ref_store, REF_STORE_WRITE, "initial_ref_transaction_commit"); - int ret = 0, i; + size_t i; + int ret = 0; struct string_list affected_refnames = STRING_LIST_INIT_NODUP; assert(err); -- cgit 1.2.3-korg From 64da41993a2c33e9187858808d5a6c87e6d6d101 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:38 +0200 Subject: ref_store: take a `msg` parameter when deleting references Just because the files backend can't retain reflogs for deleted references is no reason that they shouldn't be supported by the virtual method interface. Also, `delete_ref()` and `refs_delete_ref()` have already gained `msg` parameters. Now let's add them to `delete_refs()` and `refs_delete_refs()`. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/fetch.c | 2 +- builtin/remote.c | 4 ++-- refs.c | 11 ++++++----- refs.h | 12 +++++++----- refs/files-backend.c | 4 ++-- refs/refs-internal.h | 2 +- t/helper/test-ref-store.c | 3 ++- t/t1405-main-ref-store.sh | 2 +- t/t1406-submodule-ref-store.sh | 2 +- 9 files changed, 23 insertions(+), 19 deletions(-) (limited to 'refs/files-backend.c') diff --git a/builtin/fetch.c b/builtin/fetch.c index d4d573b985..47708451bc 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -941,7 +941,7 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map, for (ref = stale_refs; ref; ref = ref->next) string_list_append(&refnames, ref->name); - result = delete_refs(&refnames, 0); + result = delete_refs("fetch: prune", &refnames, 0); string_list_clear(&refnames, 0); } diff --git a/builtin/remote.c b/builtin/remote.c index addf97ad29..5f52c5a6d6 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -786,7 +786,7 @@ static int rm(int argc, const char **argv) strbuf_release(&buf); if (!result) - result = delete_refs(&branches, REF_NODEREF); + result = delete_refs("remote: remove", &branches, REF_NODEREF); string_list_clear(&branches, 0); if (skipped.nr) { @@ -1301,7 +1301,7 @@ static int prune_remote(const char *remote, int dry_run) string_list_sort(&refs_to_prune); if (!dry_run) - result |= delete_refs(&refs_to_prune, 0); + result |= delete_refs("remote: prune", &refs_to_prune, 0); for_each_string_list_item(item, &states.stale) { const char *refname = item->util; diff --git a/refs.c b/refs.c index 71139ba74e..989462c972 100644 --- a/refs.c +++ b/refs.c @@ -1902,15 +1902,16 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction, return refs->be->initial_transaction_commit(refs, transaction, err); } -int refs_delete_refs(struct ref_store *refs, struct string_list *refnames, - unsigned int flags) +int refs_delete_refs(struct ref_store *refs, const char *msg, + struct string_list *refnames, unsigned int flags) { - return refs->be->delete_refs(refs, refnames, flags); + return refs->be->delete_refs(refs, msg, refnames, flags); } -int delete_refs(struct string_list *refnames, unsigned int flags) +int delete_refs(const char *msg, struct string_list *refnames, + unsigned int flags) { - return refs_delete_refs(get_main_ref_store(), refnames, flags); + return refs_delete_refs(get_main_ref_store(), msg, refnames, flags); } int refs_rename_ref(struct ref_store *refs, const char *oldref, diff --git a/refs.h b/refs.h index ec8c6bfbbb..b62722fb81 100644 --- a/refs.h +++ b/refs.h @@ -331,7 +331,8 @@ int reflog_exists(const char *refname); * verify that the current value of the reference is old_sha1 before * deleting it. If old_sha1 is NULL, delete the reference if it * exists, regardless of its old value. It is an error for old_sha1 to - * be NULL_SHA1. flags is passed through to ref_transaction_delete(). + * be NULL_SHA1. msg and flags are passed through to + * ref_transaction_delete(). */ int refs_delete_ref(struct ref_store *refs, const char *msg, const char *refname, @@ -343,12 +344,13 @@ int delete_ref(const char *msg, const char *refname, /* * Delete the specified references. If there are any problems, emit * errors but attempt to keep going (i.e., the deletes are not done in - * an all-or-nothing transaction). flags is passed through to + * an all-or-nothing transaction). msg and flags are passed through to * ref_transaction_delete(). */ -int refs_delete_refs(struct ref_store *refs, struct string_list *refnames, - unsigned int flags); -int delete_refs(struct string_list *refnames, unsigned int flags); +int refs_delete_refs(struct ref_store *refs, const char *msg, + struct string_list *refnames, unsigned int flags); +int delete_refs(const char *msg, struct string_list *refnames, + unsigned int flags); /** Delete a reflog */ int refs_delete_reflog(struct ref_store *refs, const char *refname); diff --git a/refs/files-backend.c b/refs/files-backend.c index b2559b5585..fce8265aa7 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1595,7 +1595,7 @@ static int repack_without_refs(struct files_ref_store *refs, return ret; } -static int files_delete_refs(struct ref_store *ref_store, +static int files_delete_refs(struct ref_store *ref_store, const char *msg, struct string_list *refnames, unsigned int flags) { struct files_ref_store *refs = @@ -1627,7 +1627,7 @@ static int files_delete_refs(struct ref_store *ref_store, for (i = 0; i < refnames->nr; i++) { const char *refname = refnames->items[i].string; - if (refs_delete_ref(&refs->base, NULL, refname, NULL, flags)) + if (refs_delete_ref(&refs->base, msg, refname, NULL, flags)) result |= error(_("could not remove reference %s"), refname); } diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 7020e51cb7..95edf6f234 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -508,7 +508,7 @@ typedef int create_symref_fn(struct ref_store *ref_store, const char *ref_target, const char *refs_heads_master, const char *logmsg); -typedef int delete_refs_fn(struct ref_store *ref_store, +typedef int delete_refs_fn(struct ref_store *ref_store, const char *msg, struct string_list *refnames, unsigned int flags); typedef int rename_ref_fn(struct ref_store *ref_store, const char *oldref, const char *newref, diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c index fba85e7da5..05d8c4d8af 100644 --- a/t/helper/test-ref-store.c +++ b/t/helper/test-ref-store.c @@ -93,12 +93,13 @@ static int cmd_create_symref(struct ref_store *refs, const char **argv) static int cmd_delete_refs(struct ref_store *refs, const char **argv) { unsigned int flags = arg_flags(*argv++, "flags"); + const char *msg = *argv++; struct string_list refnames = STRING_LIST_INIT_NODUP; while (*argv) string_list_append(&refnames, *argv++); - return refs_delete_refs(refs, &refnames, flags); + return refs_delete_refs(refs, msg, &refnames, flags); } static int cmd_rename_ref(struct ref_store *refs, const char **argv) diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh index 490521f8cb..e8115df5ba 100755 --- a/t/t1405-main-ref-store.sh +++ b/t/t1405-main-ref-store.sh @@ -31,7 +31,7 @@ test_expect_success 'create_symref(FOO, refs/heads/master)' ' test_expect_success 'delete_refs(FOO, refs/tags/new-tag)' ' git rev-parse FOO -- && git rev-parse refs/tags/new-tag -- && - $RUN delete-refs 0 FOO refs/tags/new-tag && + $RUN delete-refs 0 nothing FOO refs/tags/new-tag && test_must_fail git rev-parse FOO -- && test_must_fail git rev-parse refs/tags/new-tag -- ' diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh index 13b5454c56..c32d4cc465 100755 --- a/t/t1406-submodule-ref-store.sh +++ b/t/t1406-submodule-ref-store.sh @@ -31,7 +31,7 @@ test_expect_success 'create_symref() not allowed' ' ' test_expect_success 'delete_refs() not allowed' ' - test_must_fail $RUN delete-refs 0 FOO refs/tags/new-tag + test_must_fail $RUN delete-refs 0 nothing FOO refs/tags/new-tag ' test_expect_success 'rename_refs() not allowed' ' -- cgit 1.2.3-korg From 55c6bc37c90e134e9da39b8cce1f3084318374d3 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:40 +0200 Subject: files-backend: move `lock` member to `files_ref_store` Move the `lock` member from `packed_ref_cache` to `files_ref_store`, since at most one cache can have a locked "packed-refs" file associated with it. Rename it to `packed_refs_lock` to make its purpose clearer in its new home. More changes are coming here shortly. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index fce8265aa7..bfc555a417 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -43,15 +43,6 @@ struct packed_ref_cache { */ unsigned int referrers; - /* - * Iff the packed-refs file associated with this instance is - * currently locked for writing, this points at the associated - * lock (which is owned by somebody else). The referrer count - * is also incremented when the file is locked and decremented - * when it is unlocked. - */ - struct lock_file *lock; - /* The metadata from when this packed-refs cache was read */ struct stat_validity validity; }; @@ -70,6 +61,13 @@ struct files_ref_store { struct ref_cache *loose; struct packed_ref_cache *packed; + + /* + * Iff the packed-refs file associated with this instance is + * currently locked for writing, this points at the associated + * lock (which is owned by somebody else). + */ + struct lock_file *packed_refs_lock; }; /* Lock used for the main packed-refs file: */ @@ -104,7 +102,7 @@ static void clear_packed_ref_cache(struct files_ref_store *refs) if (refs->packed) { struct packed_ref_cache *packed_refs = refs->packed; - if (packed_refs->lock) + if (refs->packed_refs_lock) die("BUG: packed-ref cache cleared while locked"); refs->packed = NULL; release_packed_ref_cache(packed_refs); @@ -396,7 +394,7 @@ static void add_packed_ref(struct files_ref_store *refs, { struct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs); - if (!packed_ref_cache->lock) + if (!refs->packed_refs_lock) die("BUG: packed refs not locked"); add_ref_entry(get_packed_ref_dir(packed_ref_cache), create_ref_entry(refname, oid, REF_ISPACKED, 1)); @@ -1300,7 +1298,7 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags) * the packed-refs file. */ packed_ref_cache = get_packed_ref_cache(refs); - packed_ref_cache->lock = &packlock; + refs->packed_refs_lock = &packlock; /* Increment the reference count to prevent it from being freed: */ acquire_packed_ref_cache(packed_ref_cache); return 0; @@ -1323,10 +1321,10 @@ static int commit_packed_refs(struct files_ref_store *refs) files_assert_main_repository(refs, "commit_packed_refs"); - if (!packed_ref_cache->lock) + if (!refs->packed_refs_lock) die("BUG: packed-refs not locked"); - out = fdopen_lock_file(packed_ref_cache->lock, "w"); + out = fdopen_lock_file(refs->packed_refs_lock, "w"); if (!out) die_errno("unable to fdopen packed-refs descriptor"); @@ -1344,11 +1342,11 @@ static int commit_packed_refs(struct files_ref_store *refs) if (ok != ITER_DONE) die("error while iterating over references"); - if (commit_lock_file(packed_ref_cache->lock)) { + if (commit_lock_file(refs->packed_refs_lock)) { save_errno = errno; error = -1; } - packed_ref_cache->lock = NULL; + refs->packed_refs_lock = NULL; release_packed_ref_cache(packed_ref_cache); errno = save_errno; return error; @@ -1366,10 +1364,10 @@ static void rollback_packed_refs(struct files_ref_store *refs) files_assert_main_repository(refs, "rollback_packed_refs"); - if (!packed_ref_cache->lock) + if (!refs->packed_refs_lock) die("BUG: packed-refs not locked"); - rollback_lock_file(packed_ref_cache->lock); - packed_ref_cache->lock = NULL; + rollback_lock_file(refs->packed_refs_lock); + refs->packed_refs_lock = NULL; release_packed_ref_cache(packed_ref_cache); clear_packed_ref_cache(refs); } -- cgit 1.2.3-korg From 00d174489e9905411dbae5f895758f2ca489bd9f Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:41 +0200 Subject: files_ref_store: put the packed files lock directly in this struct Instead of using a global `lock_file` instance for the main "packed-refs" file and using a pointer in `files_ref_store` to keep track of whether it is locked, embed the `lock_file` instance directly in the `files_ref_store` struct and use the new `is_lock_file_locked()` function to keep track of whether it is locked. This keeps related data together and makes the main reference store less of a special case. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index bfc555a417..1db40432af 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -63,16 +63,12 @@ struct files_ref_store { struct packed_ref_cache *packed; /* - * Iff the packed-refs file associated with this instance is - * currently locked for writing, this points at the associated - * lock (which is owned by somebody else). + * Lock used for the "packed-refs" file. Note that this (and + * thus the enclosing `files_ref_store`) must not be freed. */ - struct lock_file *packed_refs_lock; + struct lock_file packed_refs_lock; }; -/* Lock used for the main packed-refs file: */ -static struct lock_file packlock; - /* * Increment the reference count of *packed_refs. */ @@ -102,7 +98,7 @@ static void clear_packed_ref_cache(struct files_ref_store *refs) if (refs->packed) { struct packed_ref_cache *packed_refs = refs->packed; - if (refs->packed_refs_lock) + if (is_lock_file_locked(&refs->packed_refs_lock)) die("BUG: packed-ref cache cleared while locked"); refs->packed = NULL; release_packed_ref_cache(packed_refs); @@ -394,7 +390,7 @@ static void add_packed_ref(struct files_ref_store *refs, { struct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs); - if (!refs->packed_refs_lock) + if (!is_lock_file_locked(&refs->packed_refs_lock)) die("BUG: packed refs not locked"); add_ref_entry(get_packed_ref_dir(packed_ref_cache), create_ref_entry(refname, oid, REF_ISPACKED, 1)); @@ -1288,7 +1284,7 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags) } if (hold_lock_file_for_update_timeout( - &packlock, files_packed_refs_path(refs), + &refs->packed_refs_lock, files_packed_refs_path(refs), flags, timeout_value) < 0) return -1; /* @@ -1298,7 +1294,6 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags) * the packed-refs file. */ packed_ref_cache = get_packed_ref_cache(refs); - refs->packed_refs_lock = &packlock; /* Increment the reference count to prevent it from being freed: */ acquire_packed_ref_cache(packed_ref_cache); return 0; @@ -1321,10 +1316,10 @@ static int commit_packed_refs(struct files_ref_store *refs) files_assert_main_repository(refs, "commit_packed_refs"); - if (!refs->packed_refs_lock) + if (!is_lock_file_locked(&refs->packed_refs_lock)) die("BUG: packed-refs not locked"); - out = fdopen_lock_file(refs->packed_refs_lock, "w"); + out = fdopen_lock_file(&refs->packed_refs_lock, "w"); if (!out) die_errno("unable to fdopen packed-refs descriptor"); @@ -1342,11 +1337,10 @@ static int commit_packed_refs(struct files_ref_store *refs) if (ok != ITER_DONE) die("error while iterating over references"); - if (commit_lock_file(refs->packed_refs_lock)) { + if (commit_lock_file(&refs->packed_refs_lock)) { save_errno = errno; error = -1; } - refs->packed_refs_lock = NULL; release_packed_ref_cache(packed_ref_cache); errno = save_errno; return error; @@ -1364,10 +1358,9 @@ static void rollback_packed_refs(struct files_ref_store *refs) files_assert_main_repository(refs, "rollback_packed_refs"); - if (!refs->packed_refs_lock) + if (!is_lock_file_locked(&refs->packed_refs_lock)) die("BUG: packed-refs not locked"); - rollback_lock_file(refs->packed_refs_lock); - refs->packed_refs_lock = NULL; + rollback_lock_file(&refs->packed_refs_lock); release_packed_ref_cache(packed_ref_cache); clear_packed_ref_cache(refs); } -- cgit 1.2.3-korg From c0ca9357640ae5efbdbfed4c5b476c820a839e85 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:42 +0200 Subject: files_transaction_cleanup(): new helper function Extract the cleanup functionality from `files_transaction_commit()` into a new function. It will soon have another caller. Use the common cleanup code even on early exit if the transaction is empty, to reduce code duplication. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 1db40432af..2c70de5209 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2834,6 +2834,27 @@ static int lock_ref_for_update(struct files_ref_store *refs, return 0; } +/* + * Unlock any references in `transaction` that are still locked, and + * mark the transaction closed. + */ +static void files_transaction_cleanup(struct ref_transaction *transaction) +{ + size_t i; + + for (i = 0; i < transaction->nr; i++) { + struct ref_update *update = transaction->updates[i]; + struct ref_lock *lock = update->backend_data; + + if (lock) { + unlock_ref(lock); + update->backend_data = NULL; + } + } + + transaction->state = REF_TRANSACTION_CLOSED; +} + static int files_transaction_commit(struct ref_store *ref_store, struct ref_transaction *transaction, struct strbuf *err) @@ -2856,10 +2877,8 @@ static int files_transaction_commit(struct ref_store *ref_store, if (transaction->state != REF_TRANSACTION_OPEN) die("BUG: commit called for transaction that is not open"); - if (!transaction->nr) { - transaction->state = REF_TRANSACTION_CLOSED; - return 0; - } + if (!transaction->nr) + goto cleanup; /* * Fail if a refname appears more than once in the @@ -3005,15 +3024,11 @@ static int files_transaction_commit(struct ref_store *ref_store, clear_loose_ref_cache(refs); cleanup: + files_transaction_cleanup(transaction); strbuf_release(&sb); - transaction->state = REF_TRANSACTION_CLOSED; for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; - struct ref_lock *lock = update->backend_data; - - if (lock) - unlock_ref(lock); if (update->flags & REF_DELETED_LOOSE) { /* -- cgit 1.2.3-korg From 8d4240d3c8a2d31b7bedda8408c0b3c217c76998 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:43 +0200 Subject: ref_transaction_commit(): check for valid `transaction->state` Move the check that `transaction->state` is valid from `files_transaction_commit()` to `ref_transaction_commit()`, where other future reference backends can benefit from it as well. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 12 ++++++++++++ refs/files-backend.c | 3 --- 2 files changed, 12 insertions(+), 3 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs.c b/refs.c index 989462c972..f8f41ffb04 100644 --- a/refs.c +++ b/refs.c @@ -1694,6 +1694,18 @@ int ref_transaction_commit(struct ref_transaction *transaction, { struct ref_store *refs = transaction->ref_store; + switch (transaction->state) { + case REF_TRANSACTION_OPEN: + /* Good. */ + break; + case REF_TRANSACTION_CLOSED: + die("BUG: prepare called on a closed reference transaction"); + break; + default: + die("BUG: unexpected reference transaction state"); + break; + } + if (getenv(GIT_QUARANTINE_ENVIRONMENT)) { strbuf_addstr(err, _("ref updates forbidden inside quarantine environment")); diff --git a/refs/files-backend.c b/refs/files-backend.c index 2c70de5209..a4261d4683 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2874,9 +2874,6 @@ static int files_transaction_commit(struct ref_store *ref_store, assert(err); - if (transaction->state != REF_TRANSACTION_OPEN) - die("BUG: commit called for transaction that is not open"); - if (!transaction->nr) goto cleanup; -- cgit 1.2.3-korg From 30173b8851bb7203de938a638386cb9e6d7c501b Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:44 +0200 Subject: ref_transaction_prepare(): new optional step for reference updates In the future, compound reference stores will sometimes need to modify references in two different reference stores at the same time, meaning that a single logical reference transaction might have to be implemented as two internal sub-transactions. They won't want to call `ref_transaction_commit()` for the two sub-transactions one after the other, because that wouldn't be atomic (the first commit could succeed and the second one fail). Instead, they will want to prepare both sub-transactions (i.e., obtain any necessary locks and do any pre-checks), and only if both prepare steps succeed, then commit both sub-transactions. Start preparing for that day by adding a new, optional `ref_transaction_prepare()` step to the reference transaction sequence, which obtains the locks and does any prechecks, reporting any errors that occur. Also add a `ref_transaction_abort()` function that can be used to abort a sub-transaction even if it has already been prepared. That is on the side of the public-facing API. On the side of the `ref_store` VTABLE, get rid of `transaction_commit` and instead add methods `transaction_prepare`, `transaction_finish`, and `transaction_abort`. A `ref_transaction_commit()` now basically calls methods `transaction_prepare` then `transaction_finish`. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 74 ++++++++++++++++++++++++++++-- refs.h | 124 ++++++++++++++++++++++++++++++++++++++++----------- refs/files-backend.c | 63 ++++++++++++++++++++------ refs/refs-internal.h | 45 ++++++++++++++----- 4 files changed, 253 insertions(+), 53 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs.c b/refs.c index f8f41ffb04..b3860a9e33 100644 --- a/refs.c +++ b/refs.c @@ -853,6 +853,19 @@ void ref_transaction_free(struct ref_transaction *transaction) if (!transaction) return; + switch (transaction->state) { + case REF_TRANSACTION_OPEN: + case REF_TRANSACTION_CLOSED: + /* OK */ + break; + case REF_TRANSACTION_PREPARED: + die("BUG: free called on a prepared reference transaction"); + break; + default: + die("BUG: unexpected reference transaction state"); + break; + } + for (i = 0; i < transaction->nr; i++) { free(transaction->updates[i]->msg); free(transaction->updates[i]); @@ -1689,8 +1702,8 @@ int create_symref(const char *ref_target, const char *refs_heads_master, refs_heads_master, logmsg); } -int ref_transaction_commit(struct ref_transaction *transaction, - struct strbuf *err) +int ref_transaction_prepare(struct ref_transaction *transaction, + struct strbuf *err) { struct ref_store *refs = transaction->ref_store; @@ -1698,6 +1711,9 @@ int ref_transaction_commit(struct ref_transaction *transaction, case REF_TRANSACTION_OPEN: /* Good. */ break; + case REF_TRANSACTION_PREPARED: + die("BUG: prepare called twice on reference transaction"); + break; case REF_TRANSACTION_CLOSED: die("BUG: prepare called on a closed reference transaction"); break; @@ -1712,7 +1728,59 @@ int ref_transaction_commit(struct ref_transaction *transaction, return -1; } - return refs->be->transaction_commit(refs, transaction, err); + return refs->be->transaction_prepare(refs, transaction, err); +} + +int ref_transaction_abort(struct ref_transaction *transaction, + struct strbuf *err) +{ + struct ref_store *refs = transaction->ref_store; + int ret = 0; + + switch (transaction->state) { + case REF_TRANSACTION_OPEN: + /* No need to abort explicitly. */ + break; + case REF_TRANSACTION_PREPARED: + ret = refs->be->transaction_abort(refs, transaction, err); + break; + case REF_TRANSACTION_CLOSED: + die("BUG: abort called on a closed reference transaction"); + break; + default: + die("BUG: unexpected reference transaction state"); + break; + } + + ref_transaction_free(transaction); + return ret; +} + +int ref_transaction_commit(struct ref_transaction *transaction, + struct strbuf *err) +{ + struct ref_store *refs = transaction->ref_store; + int ret; + + switch (transaction->state) { + case REF_TRANSACTION_OPEN: + /* Need to prepare first. */ + ret = ref_transaction_prepare(transaction, err); + if (ret) + return ret; + break; + case REF_TRANSACTION_PREPARED: + /* Fall through to finish. */ + break; + case REF_TRANSACTION_CLOSED: + die("BUG: commit called on a closed reference transaction"); + break; + default: + die("BUG: unexpected reference transaction state"); + break; + } + + return refs->be->transaction_finish(refs, transaction, err); } int refs_verify_refname_available(struct ref_store *refs, diff --git a/refs.h b/refs.h index b62722fb81..4be14c4b3c 100644 --- a/refs.h +++ b/refs.h @@ -143,30 +143,71 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref); int dwim_log(const char *str, int len, unsigned char *sha1, char **ref); /* - * A ref_transaction represents a collection of ref updates - * that should succeed or fail together. + * A ref_transaction represents a collection of reference updates that + * should succeed or fail together. * * Calling sequence * ---------------- + * * - Allocate and initialize a `struct ref_transaction` by calling * `ref_transaction_begin()`. * - * - List intended ref updates by calling functions like - * `ref_transaction_update()` and `ref_transaction_create()`. - * - * - Call `ref_transaction_commit()` to execute the transaction. - * If this succeeds, the ref updates will have taken place and - * the transaction cannot be rolled back. - * - * - Instead of `ref_transaction_commit`, use - * `initial_ref_transaction_commit()` if the ref database is known - * to be empty (e.g. during clone). This is likely to be much - * faster. - * - * - At any time call `ref_transaction_free()` to discard the - * transaction and free associated resources. In particular, - * this rolls back the transaction if it has not been - * successfully committed. + * - Specify the intended ref updates by calling one or more of the + * following functions: + * - `ref_transaction_update()` + * - `ref_transaction_create()` + * - `ref_transaction_delete()` + * - `ref_transaction_verify()` + * + * - Then either: + * + * - Optionally call `ref_transaction_prepare()` to prepare the + * transaction. This locks all references, checks preconditions, + * etc. but doesn't finalize anything. If this step fails, the + * transaction has been closed and can only be freed. If this step + * succeeds, then `ref_transaction_commit()` is almost certain to + * succeed. However, you can still call `ref_transaction_abort()` + * if you decide not to commit the transaction after all. + * + * - Call `ref_transaction_commit()` to execute the transaction, + * make the changes permanent, and release all locks. If you + * haven't already called `ref_transaction_prepare()`, then + * `ref_transaction_commit()` calls it for you. + * + * Or + * + * - Call `initial_ref_transaction_commit()` if the ref database is + * known to be empty and have no other writers (e.g. during + * clone). This is likely to be much faster than + * `ref_transaction_commit()`. `ref_transaction_prepare()` should + * *not* be called before `initial_ref_transaction_commit()`. + * + * - Then finally, call `ref_transaction_free()` to free the + * `ref_transaction` data structure. + * + * At any time before calling `ref_transaction_commit()`, you can call + * `ref_transaction_abort()` to abort the transaction, rollback any + * locks, and free any associated resources (including the + * `ref_transaction` data structure). + * + * Putting it all together, a complete reference update looks like + * + * struct ref_transaction *transaction; + * struct strbuf err = STRBUF_INIT; + * int ret = 0; + * + * transaction = ref_store_transaction_begin(refs, &err); + * if (!transaction || + * ref_transaction_update(...) || + * ref_transaction_create(...) || + * ...etc... || + * ref_transaction_commit(transaction, &err)) { + * error("%s", err.buf); + * ret = -1; + * } + * ref_transaction_free(transaction); + * strbuf_release(&err); + * return ret; * * Error handling * -------------- @@ -183,8 +224,9 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **ref); * ------- * * Note that no locks are taken, and no refs are read, until - * `ref_transaction_commit` is called. So `ref_transaction_verify` - * won't report a verification failure until the commit is attempted. + * `ref_transaction_prepare()` or `ref_transaction_commit()` is + * called. So, for example, `ref_transaction_verify()` won't report a + * verification failure until the commit is attempted. */ struct ref_transaction; @@ -523,19 +565,47 @@ int ref_transaction_verify(struct ref_transaction *transaction, unsigned int flags, struct strbuf *err); -/* - * Commit all of the changes that have been queued in transaction, as - * atomically as possible. - * - * Returns 0 for success, or one of the below error codes for errors. - */ /* Naming conflict (for example, the ref names A and A/B conflict). */ #define TRANSACTION_NAME_CONFLICT -1 /* All other errors. */ #define TRANSACTION_GENERIC_ERROR -2 + +/* + * Perform the preparatory stages of commiting `transaction`. Acquire + * any needed locks, check preconditions, etc.; basically, do as much + * as possible to ensure that the transaction will be able to go + * through, stopping just short of making any irrevocable or + * user-visible changes. The updates that this function prepares can + * be finished up by calling `ref_transaction_commit()` or rolled back + * by calling `ref_transaction_abort()`. + * + * On success, return 0 and leave the transaction in "prepared" state. + * On failure, abort the transaction, write an error message to `err`, + * and return one of the `TRANSACTION_*` constants. + * + * Callers who don't need such fine-grained control over commiting + * reference transactions should just call `ref_transaction_commit()`. + */ +int ref_transaction_prepare(struct ref_transaction *transaction, + struct strbuf *err); + +/* + * Commit all of the changes that have been queued in transaction, as + * atomically as possible. On success, return 0 and leave the + * transaction in "closed" state. On failure, roll back the + * transaction, write an error message to `err`, and return one of the + * `TRANSACTION_*` constants + */ int ref_transaction_commit(struct ref_transaction *transaction, struct strbuf *err); +/* + * Abort `transaction`, which has been begun and possibly prepared, + * but not yet committed. + */ +int ref_transaction_abort(struct ref_transaction *transaction, + struct strbuf *err); + /* * Like ref_transaction_commit(), but optimized for creating * references when originally initializing a repository (e.g., by "git @@ -551,7 +621,7 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction, struct strbuf *err); /* - * Free an existing transaction and all associated data. + * Free `*transaction` and all associated data. */ void ref_transaction_free(struct ref_transaction *transaction); diff --git a/refs/files-backend.c b/refs/files-backend.c index a4261d4683..19842d2e56 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2855,22 +2855,19 @@ static void files_transaction_cleanup(struct ref_transaction *transaction) transaction->state = REF_TRANSACTION_CLOSED; } -static int files_transaction_commit(struct ref_store *ref_store, - struct ref_transaction *transaction, - struct strbuf *err) +static int files_transaction_prepare(struct ref_store *ref_store, + struct ref_transaction *transaction, + struct strbuf *err) { struct files_ref_store *refs = files_downcast(ref_store, REF_STORE_WRITE, - "ref_transaction_commit"); + "ref_transaction_prepare"); size_t i; int ret = 0; - struct string_list refs_to_delete = STRING_LIST_INIT_NODUP; - struct string_list_item *ref_to_delete; struct string_list affected_refnames = STRING_LIST_INIT_NODUP; char *head_ref = NULL; int head_type; struct object_id head_oid; - struct strbuf sb = STRBUF_INIT; assert(err); @@ -2934,6 +2931,8 @@ static int files_transaction_commit(struct ref_store *ref_store, * that new values are valid, and write new values to the * lockfiles, ready to be activated. Only keep one lockfile * open at a time to avoid running out of file descriptors. + * Note that lock_ref_for_update() might append more updates + * to the transaction. */ for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; @@ -2941,7 +2940,38 @@ static int files_transaction_commit(struct ref_store *ref_store, ret = lock_ref_for_update(refs, update, transaction, head_ref, &affected_refnames, err); if (ret) - goto cleanup; + break; + } + +cleanup: + free(head_ref); + string_list_clear(&affected_refnames, 0); + + if (ret) + files_transaction_cleanup(transaction); + else + transaction->state = REF_TRANSACTION_PREPARED; + + return ret; +} + +static int files_transaction_finish(struct ref_store *ref_store, + struct ref_transaction *transaction, + struct strbuf *err) +{ + struct files_ref_store *refs = + 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; + + assert(err); + + if (!transaction->nr) { + transaction->state = REF_TRANSACTION_CLOSED; + return 0; } /* Perform updates first so live commits remain referenced */ @@ -3022,7 +3052,6 @@ static int files_transaction_commit(struct ref_store *ref_store, cleanup: files_transaction_cleanup(transaction); - strbuf_release(&sb); for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; @@ -3039,13 +3068,19 @@ static int files_transaction_commit(struct ref_store *ref_store, } } + strbuf_release(&sb); string_list_clear(&refs_to_delete, 0); - free(head_ref); - string_list_clear(&affected_refnames, 0); - return ret; } +static int files_transaction_abort(struct ref_store *ref_store, + struct ref_transaction *transaction, + struct strbuf *err) +{ + files_transaction_cleanup(transaction); + return 0; +} + static int ref_present(const char *refname, const struct object_id *oid, int flags, void *cb_data) { @@ -3316,7 +3351,9 @@ struct ref_storage_be refs_be_files = { "files", files_ref_store_create, files_init_db, - files_transaction_commit, + files_transaction_prepare, + files_transaction_finish, + files_transaction_abort, files_initial_transaction_commit, files_pack_refs, diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 95edf6f234..4d3dd17f9f 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -185,17 +185,27 @@ struct ref_update *ref_transaction_add_update( /* * Transaction states. - * OPEN: The transaction is in a valid state and can accept new updates. - * An OPEN transaction can be committed. - * CLOSED: A closed transaction is no longer active and no other operations - * than free can be used on it in this state. - * A transaction can either become closed by successfully committing - * an active transaction or if there is a failure while building - * the transaction thus rendering it failed/inactive. + * + * OPEN: The transaction is initialized and new updates can still be + * added to it. An OPEN transaction can be prepared, + * committed, freed, or aborted (freeing and aborting an open + * transaction are equivalent). + * + * PREPARED: ref_transaction_prepare(), which locks all of the + * references involved in the update and checks that the + * update has no errors, has been called successfully for the + * transaction. A PREPARED transaction can be committed or + * aborted. + * + * CLOSED: The transaction is no longer active. A transaction becomes + * CLOSED if there is a failure while building the transaction + * or if a transaction is committed or aborted. A CLOSED + * transaction can only be freed. */ enum ref_transaction_state { - REF_TRANSACTION_OPEN = 0, - REF_TRANSACTION_CLOSED = 1 + REF_TRANSACTION_OPEN = 0, + REF_TRANSACTION_PREPARED = 1, + REF_TRANSACTION_CLOSED = 2 }; /* @@ -497,6 +507,18 @@ typedef struct ref_store *ref_store_init_fn(const char *gitdir, typedef int ref_init_db_fn(struct ref_store *refs, struct strbuf *err); +typedef int ref_transaction_prepare_fn(struct ref_store *refs, + struct ref_transaction *transaction, + struct strbuf *err); + +typedef int ref_transaction_finish_fn(struct ref_store *refs, + struct ref_transaction *transaction, + struct strbuf *err); + +typedef int ref_transaction_abort_fn(struct ref_store *refs, + struct ref_transaction *transaction, + struct strbuf *err); + typedef int ref_transaction_commit_fn(struct ref_store *refs, struct ref_transaction *transaction, struct strbuf *err); @@ -600,7 +622,10 @@ struct ref_storage_be { const char *name; ref_store_init_fn *init; ref_init_db_fn *init_db; - ref_transaction_commit_fn *transaction_commit; + + ref_transaction_prepare_fn *transaction_prepare; + ref_transaction_finish_fn *transaction_finish; + ref_transaction_abort_fn *transaction_abort; ref_transaction_commit_fn *initial_transaction_commit; pack_refs_fn *pack_refs; -- cgit 1.2.3-korg From 2ced105cb1df064b9400aef5f4d35e20026ab267 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:45 +0200 Subject: ref_update_reject_duplicates(): expose function to whole refs module It will soon have some other users. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 17 +++++++++++++++++ refs/files-backend.c | 17 ----------------- refs/refs-internal.h | 8 ++++++++ 3 files changed, 25 insertions(+), 17 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs.c b/refs.c index b3860a9e33..beb49fb297 100644 --- a/refs.c +++ b/refs.c @@ -1702,6 +1702,23 @@ int create_symref(const char *ref_target, const char *refs_heads_master, refs_heads_master, logmsg); } +int ref_update_reject_duplicates(struct string_list *refnames, + struct strbuf *err) +{ + int i, n = refnames->nr; + + assert(err); + + for (i = 1; i < n; i++) + if (!strcmp(refnames->items[i - 1].string, refnames->items[i].string)) { + strbuf_addf(err, + "multiple updates for ref '%s' not allowed.", + refnames->items[i].string); + return 1; + } + return 0; +} + int ref_transaction_prepare(struct ref_transaction *transaction, struct strbuf *err) { diff --git a/refs/files-backend.c b/refs/files-backend.c index 19842d2e56..8d0ce739a6 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2512,23 +2512,6 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st return ref_iterator; } -static int ref_update_reject_duplicates(struct string_list *refnames, - struct strbuf *err) -{ - int i, n = refnames->nr; - - assert(err); - - for (i = 1; i < n; i++) - if (!strcmp(refnames->items[i - 1].string, refnames->items[i].string)) { - strbuf_addf(err, - "multiple updates for ref '%s' not allowed.", - refnames->items[i].string); - return 1; - } - return 0; -} - /* * If update is a direct update of head_ref (the reference pointed to * by HEAD), then add an extra REF_LOG_ONLY update for HEAD. diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 4d3dd17f9f..192f9f85c9 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -169,6 +169,14 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname, unsigned char *sha1, struct strbuf *referent, unsigned int *type); +/* + * Write an error to `err` and return a nonzero value iff the same + * refname appears multiple times in `refnames`. `refnames` must be + * sorted on entry to this function. + */ +int ref_update_reject_duplicates(struct string_list *refnames, + struct strbuf *err); + /* * Add a ref_update with the specified properties to transaction, and * return a pointer to the new object. This function does not verify -- cgit 1.2.3-korg From 531cc4a56da1eff87cb57d90012197eb6d721edd Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:48 +0200 Subject: should_pack_ref(): new function, extracted from `files_pack_refs()` Extract a function for deciding whether a reference should be packed. It is a self-contained bit of logic, so splitting it out improves readability. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 42 ++++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 14 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 8d0ce739a6..29514392b0 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1455,6 +1455,32 @@ static void prune_refs(struct files_ref_store *refs, struct ref_to_prune *r) } } +/* + * Return true if the specified reference should be packed. + */ +static int should_pack_ref(const char *refname, + const struct object_id *oid, unsigned int ref_flags, + unsigned int pack_flags) +{ + /* Do not pack per-worktree refs: */ + if (ref_type(refname) != REF_TYPE_NORMAL) + return 0; + + /* Do not pack non-tags unless PACK_REFS_ALL is set: */ + if (!(pack_flags & PACK_REFS_ALL) && !starts_with(refname, "refs/tags/")) + return 0; + + /* Do not pack symbolic refs: */ + if (ref_flags & REF_ISSYMREF) + return 0; + + /* Do not pack broken refs: */ + if (!ref_resolves_to_object(refname, oid, ref_flags)) + return 0; + + return 1; +} + static int files_pack_refs(struct ref_store *ref_store, unsigned int flags) { struct files_ref_store *refs = @@ -1476,21 +1502,9 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags) * pruned, also add it to refs_to_prune. */ struct ref_entry *packed_entry; - int is_tag_ref = starts_with(iter->refname, "refs/tags/"); - - /* Do not pack per-worktree refs: */ - if (ref_type(iter->refname) != REF_TYPE_NORMAL) - continue; - - /* ALWAYS pack tags */ - if (!(flags & PACK_REFS_ALL) && !is_tag_ref) - continue; - - /* Do not pack symbolic or broken refs: */ - if (iter->flags & REF_ISSYMREF) - continue; - if (!ref_resolves_to_object(iter->refname, iter->oid, iter->flags)) + if (!should_pack_ref(iter->refname, iter->oid, iter->flags, + flags)) continue; /* -- cgit 1.2.3-korg From 28ed9830b1dc65df37bdb3c9b8ef743f1f266262 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:49 +0200 Subject: get_packed_ref_cache(): assume "packed-refs" won't change while locked If we've got the "packed-refs" file locked, then it can't change; there's no need to keep calling `stat_validity_check()` on it. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 29514392b0..c4bc9550d3 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -342,13 +342,18 @@ static void files_ref_path(struct files_ref_store *refs, /* * Get the packed_ref_cache for the specified files_ref_store, - * creating it if necessary. + * creating and populating it if it hasn't been read before or if the + * file has been changed (according to its `validity` field) since it + * was last read. On the other hand, if we hold the lock, then assume + * that the file hasn't been changed out from under us, so skip the + * extra `stat()` call in `stat_validity_check()`. */ static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *refs) { const char *packed_refs_file = files_packed_refs_path(refs); if (refs->packed && + !is_lock_file_locked(&refs->packed_refs_lock) && !stat_validity_check(&refs->packed->validity, packed_refs_file)) clear_packed_ref_cache(refs); @@ -1288,10 +1293,11 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags) flags, timeout_value) < 0) return -1; /* - * Get the current packed-refs while holding the lock. If the - * packed-refs file has been modified since we last read it, - * this will automatically invalidate the cache and re-read - * the packed-refs file. + * Get the current packed-refs while holding the lock. It is + * important that we call `get_packed_ref_cache()` before + * setting `packed_ref_cache->lock`, because otherwise the + * former will see that the file is locked and assume that the + * cache can't be stale. */ packed_ref_cache = get_packed_ref_cache(refs); /* Increment the reference count to prevent it from being freed: */ -- cgit 1.2.3-korg From 099a912a279415dd27716ee5de07ff347bfc49da Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:50 +0200 Subject: read_packed_refs(): do more of the work of reading packed refs Teach `read_packed_refs()` to also * Allocate and initialize the new `packed_ref_cache` * Open and close the `packed-refs` file * Update the `validity` field of the new object This decreases the coupling between `packed_refs_cache` and `files_ref_store` by a little bit. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 40 ++++++++++++++++++++++++---------------- refs/ref-cache.h | 3 ++- 2 files changed, 26 insertions(+), 17 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index c4bc9550d3..b4fa745cd7 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -209,7 +209,9 @@ static const char *parse_ref_line(struct strbuf *line, struct object_id *oid) } /* - * Read f, which is a packed-refs file, into dir. + * Read from `packed_refs_file` into a newly-allocated + * `packed_ref_cache` and return it. The return value will already + * have its reference count incremented. * * A comment line of the form "# pack-refs with: " may contain zero or * more traits. We interpret the traits as follows: @@ -235,12 +237,26 @@ static const char *parse_ref_line(struct strbuf *line, struct object_id *oid) * compatibility with older clients, but we do not require it * (i.e., "peeled" is a no-op if "fully-peeled" is set). */ -static void read_packed_refs(FILE *f, struct ref_dir *dir) +static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file) { + FILE *f; + struct packed_ref_cache *packed_refs = xcalloc(1, sizeof(*packed_refs)); struct ref_entry *last = NULL; struct strbuf line = STRBUF_INIT; enum { PEELED_NONE, PEELED_TAGS, PEELED_FULLY } peeled = PEELED_NONE; + struct ref_dir *dir; + acquire_packed_ref_cache(packed_refs); + packed_refs->cache = create_ref_cache(NULL, NULL); + packed_refs->cache->root->flag &= ~REF_INCOMPLETE; + + f = fopen(packed_refs_file, "r"); + if (!f) + return packed_refs; + + stat_validity_update(&packed_refs->validity, fileno(f)); + + dir = get_ref_dir(packed_refs->cache->root); while (strbuf_getwholeline(&line, f, '\n') != EOF) { struct object_id oid; const char *refname; @@ -287,7 +303,10 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir) } } + fclose(f); strbuf_release(&line); + + return packed_refs; } static const char *files_packed_refs_path(struct files_ref_store *refs) @@ -357,20 +376,9 @@ static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *ref !stat_validity_check(&refs->packed->validity, packed_refs_file)) clear_packed_ref_cache(refs); - if (!refs->packed) { - FILE *f; - - refs->packed = xcalloc(1, sizeof(*refs->packed)); - acquire_packed_ref_cache(refs->packed); - refs->packed->cache = create_ref_cache(&refs->base, NULL); - refs->packed->cache->root->flag &= ~REF_INCOMPLETE; - f = fopen(packed_refs_file, "r"); - if (f) { - stat_validity_update(&refs->packed->validity, fileno(f)); - read_packed_refs(f, get_ref_dir(refs->packed->cache->root)); - fclose(f); - } - } + if (!refs->packed) + refs->packed = read_packed_refs(packed_refs_file); + return refs->packed; } diff --git a/refs/ref-cache.h b/refs/ref-cache.h index 1f65e2f9ed..fbfee7ce79 100644 --- a/refs/ref-cache.h +++ b/refs/ref-cache.h @@ -194,7 +194,8 @@ struct ref_entry *create_ref_entry(const char *refname, * function called to fill in incomplete directories in the * `ref_cache` when they are accessed. If it is NULL, then the whole * `ref_cache` must be filled (including clearing its directories' - * `REF_INCOMPLETE` bits) before it is used. + * `REF_INCOMPLETE` bits) before it is used, and `refs` can be NULL, + * too. */ struct ref_cache *create_ref_cache(struct ref_store *refs, fill_ref_dir_fn *fill_ref_dir); -- cgit 1.2.3-korg From 89c571da56a1e84fe12308f727fac0e82c1d5be6 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:51 +0200 Subject: read_packed_refs(): report unexpected fopen() failures The old code ignored any errors encountered when trying to fopen the "packed-refs" file, treating all such failures as if the file didn't exist. But it could be that there is some other error opening the file (e.g., permissions problems), and we don't want to silently ignore such problems. So report any failures that are not due to ENOENT. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index b4fa745cd7..dbfd03f989 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -251,8 +251,18 @@ static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file) packed_refs->cache->root->flag &= ~REF_INCOMPLETE; f = fopen(packed_refs_file, "r"); - if (!f) - return packed_refs; + if (!f) { + if (errno == ENOENT) { + /* + * This is OK; it just means that no + * "packed-refs" file has been written yet, + * which is equivalent to it being empty. + */ + return packed_refs; + } else { + die_errno("couldn't read %s", packed_refs_file); + } + } stat_validity_update(&packed_refs->validity, fileno(f)); -- cgit 1.2.3-korg From 0a0865b8f168b7195bd15440d15eb0e7817d6526 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:52 +0200 Subject: refs_ref_iterator_begin(): handle `GIT_REF_PARANOIA` Instead of handling `GIT_REF_PARANOIA` in `files_ref_iterator_begin()`, handle it in `refs_ref_iterator_begin()`, where it will cover all reference stores. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 5 +++++ refs/files-backend.c | 11 ++++------- 2 files changed, 9 insertions(+), 7 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs.c b/refs.c index d1c781d94e..f0685c9251 100644 --- a/refs.c +++ b/refs.c @@ -1259,6 +1259,11 @@ struct ref_iterator *refs_ref_iterator_begin( { struct ref_iterator *iter; + if (ref_paranoia < 0) + ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 0); + if (ref_paranoia) + flags |= DO_FOR_EACH_INCLUDE_BROKEN; + iter = refs->be->iterator_begin(refs, prefix, flags); /* diff --git a/refs/files-backend.c b/refs/files-backend.c index dbfd03f989..5de36fc335 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1074,15 +1074,12 @@ static struct ref_iterator *files_ref_iterator_begin( struct ref_iterator *loose_iter, *packed_iter; struct files_ref_iterator *iter; struct ref_iterator *ref_iterator; + unsigned int required_flags = REF_STORE_READ; - if (ref_paranoia < 0) - ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 0); - if (ref_paranoia) - flags |= DO_FOR_EACH_INCLUDE_BROKEN; + if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN)) + required_flags |= REF_STORE_ODB; - refs = files_downcast(ref_store, - REF_STORE_READ | (ref_paranoia ? 0 : REF_STORE_ODB), - "ref_iterator_begin"); + refs = files_downcast(ref_store, required_flags, "ref_iterator_begin"); iter = xcalloc(1, sizeof(*iter)); ref_iterator = &iter->base; -- cgit 1.2.3-korg From c1da06c6f1a77370341d93d80af027caa6a19a94 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:53 +0200 Subject: create_ref_entry(): remove `check_name` option Only one caller was using it, so move the check to that caller. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 12 ++++++++---- refs/ref-cache.c | 6 +----- refs/ref-cache.h | 3 +-- 3 files changed, 10 insertions(+), 11 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 5de36fc335..d8b3f73147 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -291,7 +291,7 @@ static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file) oidclr(&oid); flag |= REF_BAD_NAME | REF_ISBROKEN; } - last = create_ref_entry(refname, &oid, flag, 0); + last = create_ref_entry(refname, &oid, flag); if (peeled == PEELED_FULLY || (peeled == PEELED_TAGS && starts_with(refname, "refs/tags/"))) last->flag |= REF_KNOWS_PEELED; @@ -415,8 +415,12 @@ static void add_packed_ref(struct files_ref_store *refs, if (!is_lock_file_locked(&refs->packed_refs_lock)) die("BUG: packed refs not locked"); + + if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) + die("Reference has invalid format: '%s'", refname); + add_ref_entry(get_packed_ref_dir(packed_ref_cache), - create_ref_entry(refname, oid, REF_ISPACKED, 1)); + create_ref_entry(refname, oid, REF_ISPACKED)); } /* @@ -493,7 +497,7 @@ static void loose_fill_ref_dir(struct ref_store *ref_store, flag |= REF_BAD_NAME | REF_ISBROKEN; } add_entry_to_dir(dir, - create_ref_entry(refname.buf, &oid, flag, 0)); + create_ref_entry(refname.buf, &oid, flag)); } strbuf_setlen(&refname, dirnamelen); strbuf_setlen(&path, path_baselen); @@ -1541,7 +1545,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags) oidcpy(&packed_entry->u.value.oid, iter->oid); } else { packed_entry = create_ref_entry(iter->refname, iter->oid, - REF_ISPACKED, 0); + REF_ISPACKED); add_ref_entry(packed_refs, packed_entry); } oidclr(&packed_entry->u.value.peeled); diff --git a/refs/ref-cache.c b/refs/ref-cache.c index 6b11d9cd12..ec97f3a38a 100644 --- a/refs/ref-cache.c +++ b/refs/ref-cache.c @@ -32,14 +32,10 @@ struct ref_dir *get_ref_dir(struct ref_entry *entry) } struct ref_entry *create_ref_entry(const char *refname, - const struct object_id *oid, int flag, - int check_name) + const struct object_id *oid, int flag) { struct ref_entry *ref; - if (check_name && - check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) - die("Reference has invalid format: '%s'", refname); FLEX_ALLOC_STR(ref, name, refname); oidcpy(&ref->u.value.oid, oid); oidclr(&ref->u.value.peeled); diff --git a/refs/ref-cache.h b/refs/ref-cache.h index fbfee7ce79..794f000fd3 100644 --- a/refs/ref-cache.h +++ b/refs/ref-cache.h @@ -185,8 +185,7 @@ struct ref_entry *create_dir_entry(struct ref_cache *cache, int incomplete); struct ref_entry *create_ref_entry(const char *refname, - const struct object_id *oid, int flag, - int check_name); + const struct object_id *oid, int flag); /* * Return a pointer to a new `ref_cache`. Its top-level starts out -- cgit 1.2.3-korg