From 23739aa2b3d2546c3b3ff0e06eb5171b37e31a90 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:31 +0200 Subject: [PATCH 01/25] t3600: clean up permissions test properly The test of failing `git rm -f` removes the write permissions on the test directory, but fails to restore them if the test fails. This means that the test temporary directory cannot be cleaned up, which means that subsequent attempts to run the test fail mysteriously. Instead, do the cleanup in a `test_when_finished` block so that it can't be skipped. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- t/t3600-rm.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh index 5f9913ba33..f8568f8841 100755 --- a/t/t3600-rm.sh +++ b/t/t3600-rm.sh @@ -97,9 +97,9 @@ test_expect_success FUNNYNAMES \ embedded'" test_expect_success SANITY 'Test that "git rm -f" fails if its rm fails' ' + test_when_finished "chmod 775 ." && chmod a-w . && - test_must_fail git rm -f baz && - chmod 775 . + test_must_fail git rm -f baz ' test_expect_success \ From fd2ce9c01c91a093fbc8f7e444d4d80c0d89432a Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:32 +0200 Subject: [PATCH 02/25] refs.h: clarify docstring for the ref_transaction_update()-related fns In particular, make it clear that they make copies of the sha1 arguments. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.h | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/refs.h b/refs.h index 685a979a0e..ec8c6bfbbb 100644 --- a/refs.h +++ b/refs.h @@ -427,6 +427,19 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err); * * refname -- the name of the reference to be affected. * + * new_sha1 -- the SHA-1 that should be set to be the new value of + * the reference. Some functions allow this parameter to be + * NULL, meaning that the reference is not changed, or + * null_sha1, meaning that the reference should be deleted. A + * copy of this value is made in the transaction. + * + * old_sha1 -- the SHA-1 value that the reference must have before + * the update. Some functions allow this parameter to be NULL, + * meaning that the old value of the reference is not checked, + * or null_sha1, meaning that the reference must not exist + * before the update. A copy of this value is made in the + * transaction. + * * flags -- flags affecting the update, passed to * update_ref_lock(). Can be REF_NODEREF, which means that * symbolic references should not be followed. From e186057138666058a2c67c3389509c9430d95b24 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:33 +0200 Subject: [PATCH 03/25] ref_iterator_begin_fn(): fix docstring The iterator returned by this function only includes references whose names start with the whole prefix, not all of those in `find_containing_dir(prefix)` as the old docstring claimed. This docstring was probably copy-pasted from old ref-cache code, which had the old specification. But now, `cache_ref_iterator_begin()` (from which the files reference iterator gets its values) automatically wraps its output using `prefix_ref_iterator_begin()` when necessary, so it has the stricter behavior. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/refs-internal.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/refs/refs-internal.h b/refs/refs-internal.h index b6b291cf00..7020e51cb7 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -515,9 +515,10 @@ typedef int rename_ref_fn(struct ref_store *ref_store, const char *logmsg); /* - * Iterate over the references in the specified ref_store that are - * within find_containing_dir(prefix). If prefix is NULL or the empty - * string, iterate over all references in the submodule. + * Iterate over the references in `ref_store` whose names start with + * `prefix`. `prefix` is matched as a literal string, without regard + * for path separators. If prefix is NULL or the empty string, iterate + * over all references in `ref_store`. */ typedef struct ref_iterator *ref_iterator_begin_fn( struct ref_store *ref_store, From 04aea8d4df199836da3802f08cb5738eae66fa6c Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:34 +0200 Subject: [PATCH 04/25] 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(-) 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); From b9c8e7f2fb6ee19defeaa2927a0af42b525d8b33 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:35 +0200 Subject: [PATCH 05/25] prefix_ref_iterator: don't trim too much The `trim` parameter can be set independently of `prefix`. So if some caller were to set `trim` to be greater than `strlen(prefix)`, we could end up pointing the `refname` field of the iterator past the NUL of the actual reference name string. That can't happen currently, because `trim` is always set either to zero or to `strlen(prefix)`. But even the latter could lead to confusion, if a refname is exactly equal to the prefix, because then we would set the outgoing `refname` to the empty string. And we're about to decouple the `prefix` and `trim` arguments even more, so let's be cautious here. Report a bug if ever asked to trim a reference whose name is not longer than `trim`. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/iterator.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/refs/iterator.c b/refs/iterator.c index bce1f192f7..4cf449ef66 100644 --- a/refs/iterator.c +++ b/refs/iterator.c @@ -292,7 +292,23 @@ static int prefix_ref_iterator_advance(struct ref_iterator *ref_iterator) if (!starts_with(iter->iter0->refname, iter->prefix)) continue; - iter->base.refname = iter->iter0->refname + iter->trim; + if (iter->trim) { + /* + * It is nonsense to trim off characters that + * you haven't already checked for via a + * prefix check, whether via this + * `prefix_ref_iterator` or upstream in + * `iter0`). So if there wouldn't be at least + * one character left in the refname after + * trimming, report it as a bug: + */ + if (strlen(iter->iter0->refname) <= iter->trim) + die("BUG: attempt to trim too many characters"); + iter->base.refname = iter->iter0->refname + iter->trim; + } else { + iter->base.refname = iter->iter0->refname; + } + iter->base.oid = iter->iter0->oid; iter->base.flags = iter->iter0->flags; return ITER_OK; From c7599718167de62c437490e9ea300eeb9284a572 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:36 +0200 Subject: [PATCH 06/25] refs_ref_iterator_begin(): don't check prefixes redundantly The backend already correctly restricts its output to references whose names start with the prefix. By passing the prefix again to `prefix_ref_iterator`, we were forcing that iterator to do redundant prefix comparisons. So set it to the empty string. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/refs.c b/refs.c index 8af9641aa1..8494b1f20d 100644 --- a/refs.c +++ b/refs.c @@ -1247,7 +1247,13 @@ struct ref_iterator *refs_ref_iterator_begin( struct ref_iterator *iter; iter = refs->be->iterator_begin(refs, prefix, flags); - iter = prefix_ref_iterator_begin(iter, prefix, trim); + + /* + * `iterator_begin()` already takes care of prefix, but we + * might need to do some trimming: + */ + if (trim) + iter = prefix_ref_iterator_begin(iter, "", trim); return iter; } From 43a2dfde76a4a47ffa31be11fd5cd7fe0b57bb84 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:37 +0200 Subject: [PATCH 07/25] 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(-) 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); From 64da41993a2c33e9187858808d5a6c87e6d6d101 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:38 +0200 Subject: [PATCH 08/25] 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(-) 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' ' From 0978f4ba7fe571d96b9f13827bdac6c30eeebfa2 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:39 +0200 Subject: [PATCH 09/25] lockfile: add a new method, is_lock_file_locked() It will soon prove useful. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- lockfile.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lockfile.h b/lockfile.h index 7b715f9e77..572064939c 100644 --- a/lockfile.h +++ b/lockfile.h @@ -175,6 +175,14 @@ static inline int hold_lock_file_for_update( return hold_lock_file_for_update_timeout(lk, path, flags, 0); } +/* + * Return a nonzero value iff `lk` is currently locked. + */ +static inline int is_lock_file_locked(struct lock_file *lk) +{ + return is_tempfile_active(&lk->tempfile); +} + /* * Append an appropriate error message to `buf` following the failure * of `hold_lock_file_for_update()` to lock `path`. `err` should be the From 55c6bc37c90e134e9da39b8cce1f3084318374d3 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:40 +0200 Subject: [PATCH 10/25] 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(-) 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); } From 00d174489e9905411dbae5f895758f2ca489bd9f Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:41 +0200 Subject: [PATCH 11/25] 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(-) 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); } From c0ca9357640ae5efbdbfed4c5b476c820a839e85 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:42 +0200 Subject: [PATCH 12/25] 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(-) 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) { /* From 8d4240d3c8a2d31b7bedda8408c0b3c217c76998 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:43 +0200 Subject: [PATCH 13/25] 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(-) 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; From 30173b8851bb7203de938a638386cb9e6d7c501b Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:44 +0200 Subject: [PATCH 14/25] 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 | 118 ++++++++++++++++++++++++++++++++++--------- refs/files-backend.c | 63 ++++++++++++++++++----- refs/refs-internal.h | 45 +++++++++++++---- 4 files changed, 250 insertions(+), 50 deletions(-) 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()`. + * - 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()` * - * - 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. + * - Then either: * - * - 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. + * - 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. * - * - 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. + * - 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; From 2ced105cb1df064b9400aef5f4d35e20026ab267 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:45 +0200 Subject: [PATCH 15/25] 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(-) 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 From a552e50e5a82b9219c6b2911320931417a36af32 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:46 +0200 Subject: [PATCH 16/25] ref_update_reject_duplicates(): use `size_t` rather than `int` Eliminate a theoretical risk of integer overflow if the two types have different sizes. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/refs.c b/refs.c index beb49fb297..143936a9c3 100644 --- a/refs.c +++ b/refs.c @@ -1705,7 +1705,7 @@ int create_symref(const char *ref_target, const char *refs_heads_master, int ref_update_reject_duplicates(struct string_list *refnames, struct strbuf *err) { - int i, n = refnames->nr; + size_t i, n = refnames->nr; assert(err); From 8556f8d61330ec677dc48b0ef39e2017d6927708 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:47 +0200 Subject: [PATCH 17/25] ref_update_reject_duplicates(): add a sanity check It's pretty cheap to make sure that the caller didn't pass us an unsorted list by accident, so do so. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/refs.c b/refs.c index 143936a9c3..d1c781d94e 100644 --- a/refs.c +++ b/refs.c @@ -1709,13 +1709,19 @@ int ref_update_reject_duplicates(struct string_list *refnames, assert(err); - for (i = 1; i < n; i++) - if (!strcmp(refnames->items[i - 1].string, refnames->items[i].string)) { + for (i = 1; i < n; i++) { + int cmp = strcmp(refnames->items[i - 1].string, + refnames->items[i].string); + + if (!cmp) { strbuf_addf(err, "multiple updates for ref '%s' not allowed.", refnames->items[i].string); return 1; + } else if (cmp > 0) { + die("BUG: ref_update_reject_duplicates() received unsorted list"); } + } return 0; } From 531cc4a56da1eff87cb57d90012197eb6d721edd Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:48 +0200 Subject: [PATCH 18/25] 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(-) 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; /* From 28ed9830b1dc65df37bdb3c9b8ef743f1f266262 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:49 +0200 Subject: [PATCH 19/25] 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(-) 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: */ From 099a912a279415dd27716ee5de07ff347bfc49da Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:50 +0200 Subject: [PATCH 20/25] 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 | 38 +++++++++++++++++++++++--------------- refs/ref-cache.h | 3 ++- 2 files changed, 25 insertions(+), 16 deletions(-) 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; + if (!refs->packed) + refs->packed = read_packed_refs(packed_refs_file); - 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); - } - } 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); From 89c571da56a1e84fe12308f727fac0e82c1d5be6 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:51 +0200 Subject: [PATCH 21/25] 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(-) 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)); From 0a0865b8f168b7195bd15440d15eb0e7817d6526 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:52 +0200 Subject: [PATCH 22/25] 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(-) 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; From c1da06c6f1a77370341d93d80af027caa6a19a94 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:53 +0200 Subject: [PATCH 23/25] 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(-) 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 From cfe004a5a9e5116d003332a64142b0f73f8f9cdf Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 22 May 2017 16:17:54 +0200 Subject: [PATCH 24/25] ref-filter: limit traversal to prefix When we are matching refnames against a pattern, then we know that the beginning of any refname that can match the pattern has to match the part of the pattern up to the first glob character. For example, if the pattern is `refs/heads/foo*bar`, then it can only match a reference that has the prefix `refs/heads/foo`. So pass that prefix to `for_each_fullref_in()`. This lets the ref code avoid passing us the full set of refs, and in some cases avoid reading them in the first place. Note that this applies only when the `match_as_path` flag is set (i.e., when `for-each-ref` is the caller), as the matching rules for git-branch and git-tag are subtly different. This could be generalized to the case of multiple patterns, but (a) it probably doesn't come up that often, and (b) it is more awkward to deal with multiple patterns (e.g., the patterns might not be disjoint). So, since this is just an optimization, punt on the case of multiple patterns. Signed-off-by: Jeff King Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- ref-filter.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-) diff --git a/ref-filter.c b/ref-filter.c index 6cc93dcd9f..25ca56d62f 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1665,6 +1665,68 @@ static int filter_pattern_match(struct ref_filter *filter, const char *refname) return match_pattern(filter, refname); } +/* + * Find the longest prefix of pattern we can pass to + * `for_each_fullref_in()`, namely the part of pattern preceding the + * first glob character. (Note that `for_each_fullref_in()` is + * perfectly happy working with a prefix that doesn't end at a + * pathname component boundary.) + */ +static void find_longest_prefix(struct strbuf *out, const char *pattern) +{ + const char *p; + + for (p = pattern; *p && !is_glob_special(*p); p++) + ; + + strbuf_add(out, pattern, p - pattern); +} + +/* + * This is the same as for_each_fullref_in(), but it tries to iterate + * only over the patterns we'll care about. Note that it _doesn't_ do a full + * pattern match, so the callback still has to match each ref individually. + */ +static int for_each_fullref_in_pattern(struct ref_filter *filter, + each_ref_fn cb, + void *cb_data, + int broken) +{ + struct strbuf prefix = STRBUF_INIT; + int ret; + + if (!filter->match_as_path) { + /* + * in this case, the patterns are applied after + * prefixes like "refs/heads/" etc. are stripped off, + * so we have to look at everything: + */ + return for_each_fullref_in("", cb, cb_data, broken); + } + + if (!filter->name_patterns[0]) { + /* no patterns; we have to look at everything */ + return for_each_fullref_in("", cb, cb_data, broken); + } + + if (filter->name_patterns[1]) { + /* + * multiple patterns; in theory this could still work as long + * as the patterns are disjoint. We'd just make multiple calls + * to for_each_ref(). But if they're not disjoint, we'd end up + * reporting the same ref multiple times. So let's punt on that + * for now. + */ + return for_each_fullref_in("", cb, cb_data, broken); + } + + find_longest_prefix(&prefix, filter->name_patterns[0]); + + ret = for_each_fullref_in(prefix.buf, cb, cb_data, broken); + strbuf_release(&prefix); + return ret; +} + /* * Given a ref (sha1, refname), check if the ref belongs to the array * of sha1s. If the given ref is a tag, check if the given tag points @@ -1911,7 +1973,7 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int else if (filter->kind == FILTER_REFS_TAGS) ret = for_each_fullref_in("refs/tags/", ref_filter_handler, &ref_cbdata, broken); else if (filter->kind & FILTER_REFS_ALL) - ret = for_each_fullref_in("", ref_filter_handler, &ref_cbdata, broken); + ret = for_each_fullref_in_pattern(filter, ref_filter_handler, &ref_cbdata, broken); if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD)) head_ref(ref_filter_handler, &ref_cbdata); } From f23092f19e73f5d8c3480ef02104af627a90361f Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 22 May 2017 16:17:55 +0200 Subject: [PATCH 25/25] cache_ref_iterator_begin(): avoid priming unneeded directories When iterating over references, reference priming is used to make sure that loose references are read into the ref-cache before packed references, to avoid races. It used to be that the prefix passed to reference iterators almost always ended in `/`, for example `refs/heads/`. In that case, the priming code would read all loose references under `find_containing_dir("refs/heads/")`, which is "refs/heads/". That's just what we want. But now that `ref-filter` knows how to pass refname prefixes to `for_each_fullref_in()`, the prefix might come from user input; for example, git for-each-ref refs/heads Since the argument doesn't include a trailing slash, the reference iteration code would prime all of the loose references under `find_containing_dir("refs/heads")`, which is "refs/". Thus we would unnecessarily read tags, remote-tracking references, etc., when the user is only interested in branches. It is a bit awkward to get around this problem. We can't just append a slash to the argument, because we don't know ab initio whether an argument like `refs/tags/release` corresponds to a single tag or to a directory containing tags. Moreover, until now a `prefix_ref_iterator` was used to make the final decision about which references fall within the prefix (the `cache_ref_iterator` only did a rough cut). This is also inefficient, because the `prefix_ref_iterator` can't know, for example, that while you are in a subdirectory that is completely within the prefix, you don't have to do the prefix check. So: * Move the responsibility for doing the prefix check directly to `cache_ref_iterator`. This means that `cache_ref_iterator_begin()` never has to wrap its return value in a `prefix_ref_iterator`. * Teach `cache_ref_iterator_begin()` (and `prime_ref_dir()`) to be stricter about what they iterate over and what directories they prime. * Teach `cache_ref_iterator` to keep track of whether the current `cache_ref_iterator_level` is fully within the prefix. If so, skip the prefix checks entirely. The main benefit of these optimizations is for loose references, since packed references are always read all at once. Note that after this change, `prefix_ref_iterator` is only ever used for its trimming feature and not for its "prefix" feature. But I'm not ripping out the latter yet, because it might be useful for another patch series that I'm working on. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/ref-cache.c | 95 +++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 85 insertions(+), 10 deletions(-) diff --git a/refs/ref-cache.c b/refs/ref-cache.c index ec97f3a38a..af2fcb2c12 100644 --- a/refs/ref-cache.c +++ b/refs/ref-cache.c @@ -312,11 +312,42 @@ static void sort_ref_dir(struct ref_dir *dir) dir->sorted = dir->nr = i; } +enum prefix_state { + /* All refs within the directory would match prefix: */ + PREFIX_CONTAINS_DIR, + + /* Some, but not all, refs within the directory might match prefix: */ + PREFIX_WITHIN_DIR, + + /* No refs within the directory could possibly match prefix: */ + PREFIX_EXCLUDES_DIR +}; + /* - * Load all of the refs from `dir` (recursively) into our in-memory - * cache. + * Return a `prefix_state` constant describing the relationship + * between the directory with the specified `dirname` and `prefix`. */ -static void prime_ref_dir(struct ref_dir *dir) +static enum prefix_state overlaps_prefix(const char *dirname, + const char *prefix) +{ + while (*prefix && *dirname == *prefix) { + dirname++; + prefix++; + } + if (!*prefix) + return PREFIX_CONTAINS_DIR; + else if (!*dirname) + return PREFIX_WITHIN_DIR; + else + return PREFIX_EXCLUDES_DIR; +} + +/* + * Load all of the refs from `dir` (recursively) that could possibly + * contain references matching `prefix` into our in-memory cache. If + * `prefix` is NULL, prime unconditionally. + */ +static void prime_ref_dir(struct ref_dir *dir, const char *prefix) { /* * The hard work of loading loose refs is done by get_ref_dir(), so we @@ -327,8 +358,29 @@ static void prime_ref_dir(struct ref_dir *dir) int i; for (i = 0; i < dir->nr; i++) { struct ref_entry *entry = dir->entries[i]; - if (entry->flag & REF_DIR) - prime_ref_dir(get_ref_dir(entry)); + if (!(entry->flag & REF_DIR)) { + /* Not a directory; no need to recurse. */ + } else if (!prefix) { + /* Recurse in any case: */ + prime_ref_dir(get_ref_dir(entry), NULL); + } else { + switch (overlaps_prefix(entry->name, prefix)) { + case PREFIX_CONTAINS_DIR: + /* + * Recurse, and from here down we + * don't have to check the prefix + * anymore: + */ + prime_ref_dir(get_ref_dir(entry), NULL); + break; + case PREFIX_WITHIN_DIR: + prime_ref_dir(get_ref_dir(entry), prefix); + break; + case PREFIX_EXCLUDES_DIR: + /* No need to prime this directory. */ + break; + } + } } } @@ -343,6 +395,8 @@ struct cache_ref_iterator_level { */ struct ref_dir *dir; + enum prefix_state prefix_state; + /* * The index of the current entry within dir (which might * itself be a directory). If index == -1, then the iteration @@ -369,6 +423,13 @@ struct cache_ref_iterator { /* The number of levels that have been allocated on the stack */ size_t levels_alloc; + /* + * Only include references with this prefix in the iteration. + * The prefix is matched textually, without regard for path + * component boundaries. + */ + const char *prefix; + /* * A stack of levels. levels[0] is the uppermost level that is * being iterated over in this iteration. (This is not @@ -390,6 +451,7 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator) &iter->levels[iter->levels_nr - 1]; struct ref_dir *dir = level->dir; struct ref_entry *entry; + enum prefix_state entry_prefix_state; if (level->index == -1) sort_ref_dir(dir); @@ -404,6 +466,14 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator) entry = dir->entries[level->index]; + if (level->prefix_state == PREFIX_WITHIN_DIR) { + entry_prefix_state = overlaps_prefix(entry->name, iter->prefix); + if (entry_prefix_state == PREFIX_EXCLUDES_DIR) + continue; + } else { + entry_prefix_state = level->prefix_state; + } + if (entry->flag & REF_DIR) { /* push down a level */ ALLOC_GROW(iter->levels, iter->levels_nr + 1, @@ -411,6 +481,7 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator) level = &iter->levels[iter->levels_nr++]; level->dir = get_ref_dir(entry); + level->prefix_state = entry_prefix_state; level->index = -1; } else { iter->base.refname = entry->name; @@ -471,6 +542,7 @@ static int cache_ref_iterator_abort(struct ref_iterator *ref_iterator) struct cache_ref_iterator *iter = (struct cache_ref_iterator *)ref_iterator; + free((char *)iter->prefix); free(iter->levels); base_ref_iterator_free(ref_iterator); return ITER_DONE; @@ -496,10 +568,10 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache, dir = find_containing_dir(dir, prefix, 0); if (!dir) /* There's nothing to iterate over. */ - return empty_ref_iterator_begin(); + return empty_ref_iterator_begin(); if (prime_dir) - prime_ref_dir(dir); + prime_ref_dir(dir, prefix); iter = xcalloc(1, sizeof(*iter)); ref_iterator = &iter->base; @@ -511,9 +583,12 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache, level->index = -1; level->dir = dir; - if (prefix && *prefix) - ref_iterator = prefix_ref_iterator_begin(ref_iterator, - prefix, 0); + if (prefix && *prefix) { + iter->prefix = xstrdup(prefix); + level->prefix_state = PREFIX_WITHIN_DIR; + } else { + level->prefix_state = PREFIX_CONTAINS_DIR; + } return ref_iterator; }