From ed693078e988adf66c969feb6de9f83c7abe7d24 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 19 Feb 2026 08:57:49 +0100 Subject: [PATCH 1/4] pack-bitmap: deduplicate logic to iterate over preferred bitmap tips We have two locations that iterate over the preferred bitmap tips as configured by the user via "pack.preferBitmapTips". Both of these callsites are subtly wrong: when the preferred bitmap tips contain an exact refname match, then we will hit a `BUG()`. Prepare for the fix by unifying the two callsites into a new `for_each_preferred_bitmap_tip()` function. This removes the last callsite of `bitmap_preferred_tips()` outside of "pack-bitmap.c". As such, convert the function to be local to that file only. Note that the function is still used by a second caller, so we cannot just inline it. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/pack-objects.c | 19 ++----------------- pack-bitmap.c | 18 +++++++++++++++++- pack-bitmap.h | 9 ++++++++- repack-midx.c | 14 +++----------- 4 files changed, 30 insertions(+), 30 deletions(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 5846b6a293..979470e402 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -4554,22 +4554,6 @@ static int mark_bitmap_preferred_tip(const struct reference *ref, void *data UNU return 0; } -static void mark_bitmap_preferred_tips(void) -{ - struct string_list_item *item; - const struct string_list *preferred_tips; - - preferred_tips = bitmap_preferred_tips(the_repository); - if (!preferred_tips) - return; - - for_each_string_list_item(item, preferred_tips) { - refs_for_each_ref_in(get_main_ref_store(the_repository), - item->string, mark_bitmap_preferred_tip, - NULL); - } -} - static inline int is_oid_uninteresting(struct repository *repo, struct object_id *oid) { @@ -4710,7 +4694,8 @@ static void get_object_list(struct rev_info *revs, struct strvec *argv) load_delta_islands(the_repository, progress); if (write_bitmap_index) - mark_bitmap_preferred_tips(); + for_each_preferred_bitmap_tip(the_repository, mark_bitmap_preferred_tip, + NULL); if (!fn_show_object) fn_show_object = show_object; diff --git a/pack-bitmap.c b/pack-bitmap.c index 972203f12b..2f5cb34009 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -3314,7 +3314,7 @@ int bitmap_is_midx(struct bitmap_index *bitmap_git) return !!bitmap_git->midx; } -const struct string_list *bitmap_preferred_tips(struct repository *r) +static const struct string_list *bitmap_preferred_tips(struct repository *r) { const struct string_list *dest; @@ -3323,6 +3323,22 @@ const struct string_list *bitmap_preferred_tips(struct repository *r) return NULL; } +void for_each_preferred_bitmap_tip(struct repository *repo, + each_ref_fn cb, void *cb_data) +{ + struct string_list_item *item; + const struct string_list *preferred_tips; + + preferred_tips = bitmap_preferred_tips(repo); + if (!preferred_tips) + return; + + for_each_string_list_item(item, preferred_tips) { + refs_for_each_ref_in(get_main_ref_store(repo), + item->string, cb, cb_data); + } +} + int bitmap_is_preferred_refname(struct repository *r, const char *refname) { const struct string_list *preferred_tips = bitmap_preferred_tips(r); diff --git a/pack-bitmap.h b/pack-bitmap.h index 1bd7a791e2..d0611d0481 100644 --- a/pack-bitmap.h +++ b/pack-bitmap.h @@ -5,6 +5,7 @@ #include "khash.h" #include "pack.h" #include "pack-objects.h" +#include "refs.h" #include "string-list.h" struct commit; @@ -99,6 +100,13 @@ int for_each_bitmapped_object(struct bitmap_index *bitmap_git, show_reachable_fn show_reach, void *payload); +/* + * Iterate over all references that are configured as preferred bitmap tips via + * "pack.preferBitmapTips" and invoke the callback on each function. + */ +void for_each_preferred_bitmap_tip(struct repository *repo, + each_ref_fn cb, void *cb_data); + #define GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL \ "GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL" @@ -182,7 +190,6 @@ char *pack_bitmap_filename(struct packed_git *p); int bitmap_is_midx(struct bitmap_index *bitmap_git); -const struct string_list *bitmap_preferred_tips(struct repository *r); int bitmap_is_preferred_refname(struct repository *r, const char *refname); int verify_bitmap_files(struct repository *r); diff --git a/repack-midx.c b/repack-midx.c index 74bdfa3a6e..0682b80c42 100644 --- a/repack-midx.c +++ b/repack-midx.c @@ -40,7 +40,6 @@ static int midx_snapshot_ref_one(const struct reference *ref, void *_data) void midx_snapshot_refs(struct repository *repo, struct tempfile *f) { struct midx_snapshot_ref_data data; - const struct string_list *preferred = bitmap_preferred_tips(repo); data.repo = repo; data.f = f; @@ -51,16 +50,9 @@ void midx_snapshot_refs(struct repository *repo, struct tempfile *f) die(_("could not open tempfile %s for writing"), get_tempfile_path(f)); - if (preferred) { - struct string_list_item *item; - - data.preferred = 1; - for_each_string_list_item(item, preferred) - refs_for_each_ref_in(get_main_ref_store(repo), - item->string, - midx_snapshot_ref_one, &data); - data.preferred = 0; - } + data.preferred = 1; + for_each_preferred_bitmap_tip(repo, midx_snapshot_ref_one, &data); + data.preferred = 0; refs_for_each_ref(get_main_ref_store(repo), midx_snapshot_ref_one, &data); From 7174098834e30d2da1834bbfd0aaf575f6d6bb1a Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 19 Feb 2026 08:57:50 +0100 Subject: [PATCH 2/4] pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips" The "pack.preferBitmapTips" configuration allows the user to specify which references should be preferred when generating bitmaps. This option is typically expected to be set to a reference prefix, like for example "refs/heads/". It's not unreasonable though for a user to configure one specific reference as preferred. But if they do, they'll hit a `BUG()`: $ git -c pack.preferBitmapTips=refs/heads/main repack -adb BUG: ../refs/iterator.c:366: attempt to trim too many characters error: pack-objects died of signal 6 The root cause for this bug is how we enumerate these references. We call `refs_for_each_ref_in()`, which will: - Yield all references that have a user-specified prefix. - Trim each of these references so that the prefix is removed. Typically, this function is called with a trailing slash, like "refs/heads/", and in that case things work alright. But if the function is called with the name of an existing reference then we'll try to trim the full reference name, which would leave us with an empty name. And as this would not really leave us with anything sensible, we call `BUG()` instead of yielding this reference. One could argue that this is a bug in `refs_for_each_ref_in()`. But the question then becomes what the correct behaviour would be: - Do we want to skip exact matches? In our case we certainly don't want that, as the user has asked us to generate a bitmap for it. - Do we want to yield the reference with the empty refname? That would lead to a somewhat weird result. Neither of these feel like viable options, so calling `BUG()` feels like a sensible way out. The root cause ultimately is that we even try to trim the whole refname in the first place. There are two possible ways to fix this issue: - We can fix the bug by using `refs_for_each_fullref_in()` instead, which does not strip the prefix at all. Consequently, we would now start to accept all references that start with the configured prefix, including exact matches. So if we had "refs/heads/main", we would both match "refs/heads/main" and "refs/heads/main-branch". - Or we can fix the bug by appending a slash to the prefix if it doesn't already have one. This would mean that we only match ref hierarchies that start with this prefix. While the first fix leaves the user with strictly _more_ configuration options, we have already fixed a similar case in 10e8a9352b (refs.c: stop matching non-directory prefixes in exclude patterns, 2025-03-06) by using the second option. So for the sake of consistency, let's apply the same fix here. Clarify the documentation accordingly. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- Documentation/config/pack.adoc | 9 +++---- pack-bitmap.c | 13 +++++++++- t/t5310-pack-bitmaps.sh | 41 ++++++++++++++++++++++++++++++++ t/t5319-multi-pack-index.sh | 43 ++++++++++++++++++++++++++++++++++ 4 files changed, 101 insertions(+), 5 deletions(-) diff --git a/Documentation/config/pack.adoc b/Documentation/config/pack.adoc index 75402d5579..fa997c8597 100644 --- a/Documentation/config/pack.adoc +++ b/Documentation/config/pack.adoc @@ -160,12 +160,13 @@ pack.usePathWalk:: processes. See linkgit:git-pack-objects[1] for full details. pack.preferBitmapTips:: + Specifies a ref hierarchy (e.g., "refs/heads/"); can be + given multiple times to specify more than one hierarchies. When selecting which commits will receive bitmaps, prefer a - commit at the tip of any reference that is a suffix of any value - of this configuration over any other commits in the "selection - window". + commit at the tip of a reference that is contained in any of + the configured hierarchies. + -Note that setting this configuration to `refs/foo` does not mean that +Note that setting this configuration to `refs/foo/` does not mean that the commits at the tips of `refs/foo/bar` and `refs/foo/baz` will necessarily be selected. This is because commits are selected for bitmaps from within a series of windows of variable length. diff --git a/pack-bitmap.c b/pack-bitmap.c index 2f5cb34009..1c93871484 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -3328,15 +3328,26 @@ void for_each_preferred_bitmap_tip(struct repository *repo, { struct string_list_item *item; const struct string_list *preferred_tips; + struct strbuf buf = STRBUF_INIT; preferred_tips = bitmap_preferred_tips(repo); if (!preferred_tips) return; for_each_string_list_item(item, preferred_tips) { + const char *pattern = item->string; + + if (!ends_with(pattern, "/")) { + strbuf_reset(&buf); + strbuf_addf(&buf, "%s/", pattern); + pattern = buf.buf; + } + refs_for_each_ref_in(get_main_ref_store(repo), - item->string, cb, cb_data); + pattern, cb, cb_data); } + + strbuf_release(&buf); } int bitmap_is_preferred_refname(struct repository *r, const char *refname) diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh index 6718fb98c0..310b708c5c 100755 --- a/t/t5310-pack-bitmaps.sh +++ b/t/t5310-pack-bitmaps.sh @@ -466,6 +466,47 @@ test_bitmap_cases () { ) ' + test_expect_success 'pack.preferBitmapTips interprets patterns as hierarchy' ' + git init repo && + test_when_finished "rm -fr repo" && + ( + cd repo && + + # Create enough commits that not all will receive bitmap + # coverage even if they are all at the tip of some reference. + test_commit_bulk --message="%s" 103 && + git log --format="create refs/tags/%s/tag %H" HEAD >refs && + git update-ref --stdin commits-with-bitmap && + + # Verify that we have at least one commit that did not + # receive a bitmap. + git rev-list HEAD >commits.raw && + sort commits && + comm -13 commits-with-bitmap commits >commits-wo-bitmap && + test_file_not_empty commits-wo-bitmap && + commit_id=$(head commits-wo-bitmap) && + ref_without_bitmap=$(git for-each-ref --points-at="$commit_id" --format="%(refname)") && + + # When passing the full refname we do not expect a + # bitmap to be generated, as it should be interpreted + # as if a slash was appended to the pattern. + git -c pack.preferBitmapTips="$ref_without_bitmap" repack -adb && + test-tool bitmap list-commits >after && + test_grep ! "$commit_id" after && + + # But if we pass the parent directory of the ref we + # should see a bitmap. + ref_namespace=$(dirname "$ref_without_bitmap") && + git -c pack.preferBitmapTips="$ref_namespace" repack -adb && + test-tool bitmap list-commits >after && + test_grep "$commit_id" after + ) + ' + test_expect_success 'complains about multiple pack bitmaps' ' rm -fr repo && git init repo && diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh index faae98c7e7..449353416f 100755 --- a/t/t5319-multi-pack-index.sh +++ b/t/t5319-multi-pack-index.sh @@ -1345,4 +1345,47 @@ test_expect_success 'bitmapped packs are stored via the BTMP chunk' ' ) ' +test_expect_success 'pack.preferBitmapTips interprets patterns as hierarchy' ' + git init repo && + test_when_finished "rm -fr repo" && + ( + cd repo && + + # Create enough commits that not all will receive bitmap + # coverage even if they are all at the tip of some reference. + test_commit_bulk --message="%s" 103 && + git log --format="create refs/tags/%s %H" HEAD >refs && + git update-ref --stdin commits-with-bitmap && + + # Verify that we have at least one commit that did not + # receive a bitmap. + git rev-list HEAD >commits.raw && + sort commits && + comm -13 commits-with-bitmap commits >commits-wo-bitmap && + test_file_not_empty commits-wo-bitmap && + commit_id=$(head commits-wo-bitmap) && + ref_without_bitmap=$(git for-each-ref --points-at="$commit_id" --format="%(refname)") && + + # When passing the full refname we do not expect a bitmap to be + # generated, as it should be interpreted as if a slash was + # appended to the pattern. + rm .git/objects/pack/multi-pack-index* && + git -c pack.preferBitmapTips="$ref_without_bitmap" repack -adb --write-midx && + test-tool bitmap list-commits >after && + test_grep ! "$commit_id" after && + + # But if we pass the parent directory of the ref we should see + # a bitmap. + ref_namespace=$(dirname "$ref_without_bitmap") && + rm .git/objects/pack/multi-pack-index* && + git -c pack.preferBitmapTips="$ref_namespace" repack -adb --write-midx && + test-tool bitmap list-commits >after && + test_grep "$commit_id" after + ) +' + test_done From 9e86e1a05b032d712658bbe70231447455f83fb6 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 19 Feb 2026 08:57:51 +0100 Subject: [PATCH 3/4] bisect: fix misuse of `refs_for_each_ref_in()` All callers of `refs_for_each_ref_in()` pass in a string that is terminated with a trailing slash to indicate that they only want to see refs in that specific ref hierarchy. This is in fact a requirement if one wants to use this function, as the function trims the prefix from each yielded ref. So if there was a reference that was called "refs/bisect" as in our example, the result after trimming would be the empty string, and that's something we disallow. Fix this by adding the trailing slash. Furthermore, taking a closer look, we strip the prefix only to re-add it in `mark_for_removal()`. This is somewhat roundabout, as we can instead call `refs_for_each_fullref_in()` to not do any stripping at all. Do so to simplify the code a bit. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- bisect.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/bisect.c b/bisect.c index 326b59c0dc..4f0d1a1853 100644 --- a/bisect.c +++ b/bisect.c @@ -1180,7 +1180,7 @@ int estimate_bisect_steps(int all) static int mark_for_removal(const struct reference *ref, void *cb_data) { struct string_list *refs = cb_data; - char *bisect_ref = xstrfmt("refs/bisect%s", ref->name); + char *bisect_ref = xstrdup(ref->name); string_list_append(refs, bisect_ref); return 0; } @@ -1191,9 +1191,9 @@ int bisect_clean_state(void) /* There may be some refs packed during bisection */ struct string_list refs_for_removal = STRING_LIST_INIT_NODUP; - refs_for_each_ref_in(get_main_ref_store(the_repository), - "refs/bisect", mark_for_removal, - (void *) &refs_for_removal); + refs_for_each_fullref_in(get_main_ref_store(the_repository), + "refs/bisect/", NULL, mark_for_removal, + &refs_for_removal); string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD")); string_list_append(&refs_for_removal, xstrdup("BISECT_EXPECTED_REV")); result = refs_delete_refs(get_main_ref_store(the_repository), From 6375a00ef16071eadbb383bd9915e277ef304bfe Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 19 Feb 2026 08:57:52 +0100 Subject: [PATCH 4/4] bisect: simplify string_list memory handling We declare the refs_for_removal string_list as NODUP, forcing us to manually allocate strings we insert. And then when it comes time to clean up, we set strdup_strings so that string_list_clear() will free them for us. This is a confusing pattern, and can be done much more simply by just declaring the list with the DUP initializer in the first place. It was written this way originally because one of the callsites generated the item using xstrfmt(). But that spot switched to a plain xstrdup() in the preceding commit. That means we can now just let the string_list code handle allocation itself. Signed-off-by: Jeff King Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- bisect.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/bisect.c b/bisect.c index 4f0d1a1853..268f5e36f8 100644 --- a/bisect.c +++ b/bisect.c @@ -1180,8 +1180,7 @@ int estimate_bisect_steps(int all) static int mark_for_removal(const struct reference *ref, void *cb_data) { struct string_list *refs = cb_data; - char *bisect_ref = xstrdup(ref->name); - string_list_append(refs, bisect_ref); + string_list_append(refs, ref->name); return 0; } @@ -1190,16 +1189,15 @@ int bisect_clean_state(void) int result = 0; /* There may be some refs packed during bisection */ - struct string_list refs_for_removal = STRING_LIST_INIT_NODUP; + struct string_list refs_for_removal = STRING_LIST_INIT_DUP; refs_for_each_fullref_in(get_main_ref_store(the_repository), "refs/bisect/", NULL, mark_for_removal, &refs_for_removal); - string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD")); - string_list_append(&refs_for_removal, xstrdup("BISECT_EXPECTED_REV")); + string_list_append(&refs_for_removal, "BISECT_HEAD"); + string_list_append(&refs_for_removal, "BISECT_EXPECTED_REV"); result = refs_delete_refs(get_main_ref_store(the_repository), "bisect: remove", &refs_for_removal, REF_NO_DEREF); - refs_for_removal.strdup_strings = 1; string_list_clear(&refs_for_removal, 0); unlink_or_warn(git_path_bisect_ancestors_ok()); unlink_or_warn(git_path_bisect_log());