From fed6ebebf132eb32478a5cbbebba3760c943970a Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 12 Jun 2017 10:06:13 +0200 Subject: [PATCH 1/2] lock_packed_refs(): fix cache validity check Commit 28ed9830b1 (get_packed_ref_cache(): assume "packed-refs" won't change while locked, 2017-05-22) assumes that the "packed-refs" file cannot change while we hold the lock. That assumption is justified *if* the lock has been held the whole time since the "packed-refs" file was last read. But in `lock_packed_refs()`, we ourselves lock the "packed-refs" file and then call `get_packed_ref_cache()` to ensure that the cache agrees with the file. The intent is to guard against the possibility that another process changed the "packed-refs" file the moment before we locked it. This check was defeated because `get_packed_ref_cache()` saw that the file was locked, and therefore didn't do the `stat_validity_check()` that we want. The mistake was compounded with a misleading comment in `lock_packed_refs()` claiming that it was doing the right thing. That comment came from an earlier draft of the mh/packed-ref-store-prep patch series when the commits were in a different order. So instead: * Extract a function `validate_packed_ref_cache()` that does the validity check independent of whether the lock is held. * Change `get_packed_ref_cache()` to call the new function, but only if the lock *isn't* held. * Change `lock_packed_refs()` to call the new function in any case before calling `get_packed_ref_cache()`. * Fix the comment in `lock_packed_refs()`. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index d8b3f73147..b040bb3b0a 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -369,6 +369,18 @@ static void files_ref_path(struct files_ref_store *refs, } } +/* + * Check that the packed refs cache (if any) still reflects the + * contents of the file. If not, clear the cache. + */ +static void validate_packed_ref_cache(struct files_ref_store *refs) +{ + if (refs->packed && + !stat_validity_check(&refs->packed->validity, + files_packed_refs_path(refs))) + clear_packed_ref_cache(refs); +} + /* * Get the packed_ref_cache for the specified files_ref_store, * creating and populating it if it hasn't been read before or if the @@ -381,10 +393,8 @@ static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *ref { 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); + if (!is_lock_file_locked(&refs->packed_refs_lock)) + validate_packed_ref_cache(refs); if (!refs->packed) refs->packed = read_packed_refs(packed_refs_file); @@ -1311,13 +1321,17 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags) &refs->packed_refs_lock, files_packed_refs_path(refs), flags, timeout_value) < 0) return -1; + /* - * 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. + * Now that we hold the `packed-refs` lock, make sure that our + * cache matches the current version of the file. Normally + * `get_packed_ref_cache()` does that for us, but that + * function assumes that when the file is locked, any existing + * cache is still valid. We've just locked the file, but it + * might have changed the moment *before* we locked it. */ + validate_packed_ref_cache(refs); + packed_ref_cache = get_packed_ref_cache(refs); /* Increment the reference count to prevent it from being freed: */ acquire_packed_ref_cache(packed_ref_cache); From 03df567fbf6afeca32f6a27d04656c1a3a162453 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sun, 18 Jun 2017 15:39:41 +0200 Subject: [PATCH 2/2] for_each_bisect_ref(): don't trim refnames `for_each_bisect_ref()` is called by `for_each_bad_bisect_ref()` with a term "bad". This used to make it call `for_each_ref_in_submodule()` with a prefix "refs/bisect/bad". But the latter is the name of the reference that is being sought, so the empty string was being passed to the callback as the trimmed refname. Moreover, this questionable practice was turned into an error by b9c8e7f2fb prefix_ref_iterator: don't trim too much, 2017-05-22 It makes more sense (and agrees better with the documentation of `--bisect`) for the callers to receive the full reference names. So * Add a new function, `for_each_fullref_in_submodule()`, to the refs API. This plugs a gap in the existing functionality, analogous to `for_each_fullref_in()` but accepting a `submodule` argument. * Change `for_each_bad_bisect_ref()` to call the new function rather than `for_each_ref_in_submodule()`. * Add a test. Signed-off-by: Michael Haggerty Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- refs.c | 12 ++++++++++++ refs.h | 5 ++++- revision.c | 2 +- t/t6002-rev-list-bisect.sh | 14 ++++++++++++++ 4 files changed, 31 insertions(+), 2 deletions(-) diff --git a/refs.c b/refs.c index f0685c9251..32177969f0 100644 --- a/refs.c +++ b/refs.c @@ -1341,6 +1341,18 @@ int for_each_ref_in_submodule(const char *submodule, const char *prefix, prefix, fn, cb_data); } +int for_each_fullref_in_submodule(const char *submodule, const char *prefix, + each_ref_fn fn, void *cb_data, + unsigned int broken) +{ + unsigned int flag = 0; + + if (broken) + flag = DO_FOR_EACH_INCLUDE_BROKEN; + return do_for_each_ref(get_submodule_ref_store(submodule), + prefix, fn, 0, flag, cb_data); +} + int for_each_replace_ref(each_ref_fn fn, void *cb_data) { return do_for_each_ref(get_main_ref_store(), diff --git a/refs.h b/refs.h index 4be14c4b3c..aa4ecc83d0 100644 --- a/refs.h +++ b/refs.h @@ -303,7 +303,10 @@ int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data); int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data); int for_each_ref_in_submodule(const char *submodule, const char *prefix, - each_ref_fn fn, void *cb_data); + each_ref_fn fn, void *cb_data); +int for_each_fullref_in_submodule(const char *submodule, const char *prefix, + each_ref_fn fn, void *cb_data, + unsigned int broken); int for_each_tag_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data); int for_each_branch_ref_submodule(const char *submodule, diff --git a/revision.c b/revision.c index 9c67cb6026..50039c92d6 100644 --- a/revision.c +++ b/revision.c @@ -2044,7 +2044,7 @@ static int for_each_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_d struct strbuf bisect_refs = STRBUF_INIT; int status; strbuf_addf(&bisect_refs, "refs/bisect/%s", term); - status = for_each_ref_in_submodule(submodule, bisect_refs.buf, fn, cb_data); + status = for_each_fullref_in_submodule(submodule, bisect_refs.buf, fn, cb_data, 0); strbuf_release(&bisect_refs); return status; } diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh index 3bf2759eae..534903bbd2 100755 --- a/t/t6002-rev-list-bisect.sh +++ b/t/t6002-rev-list-bisect.sh @@ -235,4 +235,18 @@ test_sequence "--bisect" # # + +test_expect_success '--bisect can default to good/bad refs' ' + git update-ref refs/bisect/bad c3 && + good=$(git rev-parse b1) && + git update-ref refs/bisect/good-$good $good && + good=$(git rev-parse c1) && + git update-ref refs/bisect/good-$good $good && + + # the only thing between c3 and c1 is c2 + git rev-parse c2 >expect && + git rev-list --bisect >actual && + test_cmp expect actual +' + test_done