From 2feedf5a1ee3cf7d7d4e06c088a2e43ccb841dcc Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 20 Aug 2025 13:39:07 -0400 Subject: [PATCH 1/2] path-walk: fix setup of pending objects Users reported an issue where objects were missing from their local repositories after a full repack using 'git repack -adf --path-walk'. This was alarming and took a while to create a reproducer. Here, we fix the bug and include a test case that would fail without this fix. The root cause is that certain objects existed in the index and had no second versions. These objects are usually blobs, though trees can be included if a cache-tree exists. The issue is that the revision walk adds these objects to the "pending" list and the path-walk API forgets to mark the lists it creates at this point as "maybe_interesting". If these paths only ever have a single version in the history of the repo (including the current staged version) then the parent directory never tries to add a new object to the list and mark the list as "maybe_interesting". Thus, when walking the list later, the group is skipped as it is expected that no objects are interesting. This happens even when there are actually no UNINTERESTING objects at all! This is based on the optimization enabled by the pack.useSparse=true config option, which is the default. Thus, we create a test case that demonstrates the many cases of this issue for reproducibility: 1. File a/b/c has only one committed version. 2. Files a/i and x/y only exist as staged changes. 3. Tree x/ only exists in the cache-tree. After performing a non-path-walk repack to force all loose objects into packfiles, run a --path-walk repack followed by 'git fsck'. This fsck is what fails with the following errors: error: invalid object 100644 f2e41136... for 'a/b/c' This is the dropped instance of the single-versioned a/b/c file. broken link from tree cfda31d8... to tree 3f725fcd... This is the missing tree for the single-versioned a/b/ directory. missing blob 0ddf2bae... (a/i) missing blob 975fbec8... (x/y) missing blob a60d869d... (file) missing blob f2e41136... (a/b/c) missing tree 3f725fcd... (a/b/) dangling tree 5896d7e... (staged root tree) Note that since the staged root tree is missing, the fsck output cannot even report that the staged x/ tree is missing as well. The core problem here is that the "maybe_interesting" member of 'struct type_and_oid_list' is not initialized to '1'. This member was added in 6333e7ae0b (path-walk: mark trees and blobs as UNINTERESTING, 2024-12-20) in a way to help when creating packfiles for a small commit range using the sparse path algorithm (enabled by pack.useSparse=true). The idea here is that the list is marked as "maybe_interesting" if an object is added that does not have the UNINTERESTING flag on it. Later, this is checked again in case all objects in the list were marked UNINTERESTING after that point in time. In this case, the algorithm skips the list as there is no reason to visit it. This leads to the problem where the "maybe_interesting" member was not appropriately initialized when the list is created from pending objects. Initializing this in the correct places fixes the bug. To reduce risk of similar bugs around initializing this structure, a follow-up change will make initializing lists use a shared method. Signed-off-by: Derrick Stolee --- path-walk.c | 2 ++ t/t7700-repack.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/path-walk.c b/path-walk.c index 2d4ddbadd50f78..1215ed398f4fb6 100644 --- a/path-walk.c +++ b/path-walk.c @@ -385,6 +385,7 @@ static int setup_pending_objects(struct path_walk_info *info, list->type = OBJ_TREE; strmap_put(&ctx->paths_to_lists, path, list); } + list->maybe_interesting = 1; oid_array_append(&list->oids, &obj->oid); free(path); } else { @@ -404,6 +405,7 @@ static int setup_pending_objects(struct path_walk_info *info, list->type = OBJ_BLOB; strmap_put(&ctx->paths_to_lists, path, list); } + list->maybe_interesting = 1; oid_array_append(&list->oids, &obj->oid); } else { /* assume a root tree, such as a lightweight tag. */ diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh index 611755cc139b96..73b78bdd887d80 100755 --- a/t/t7700-repack.sh +++ b/t/t7700-repack.sh @@ -838,4 +838,67 @@ test_expect_success '-n overrides repack.updateServerInfo=true' ' test_server_info_missing ' +test_expect_success 'pending objects are repacked appropriately' ' + test_when_finished rm -rf pending && + git init pending && + + ( + cd pending && + + # Commit file, a/b/c and never change them. + mkdir -p a/b && + echo singleton >file && + echo stuff >a/b/c && + echo more >a/d && + git add file a && + git commit -m "single blobs" && + + # Files a/d and a/e will not be singletons. + echo d >a/d && + echo e >a/e && + git add a && + git commit -m "more blobs" && + + # This use of a sparse index helps to force + # test that the cache-tree is walked, too. + git sparse-checkout set --sparse-index a x && + + # Create staged changes: + # * a/e now has multiple versions. + # * a/i now has only one version. + echo f >a/d && + echo h >a/e && + echo i >a/i && + git add a && + + # Stage and unstage a change to make use of + # resolve-undo cache and how that impacts fsck. + mkdir x && + echo y >x/y && + git add x && + xy=$(git rev-parse :x/y) && + git rm --cached x/y && + + # The blob for x/y must persist through repacks, + # but fsck currently ignores the REUC extension + # for finding links to the blob. + cat >expect <<-EOF && + dangling blob $xy + EOF + + # Bring the loose objects into a packfile to avoid + # leftovers in next test. Without this, the loose + # objects persist and the test succeeds for other + # reasons. + git repack -adf && + git fsck >out && + test_cmp expect out && + + # Test path walk version with pack.useSparse. + git -c pack.useSparse=true repack -adf --path-walk && + git fsck >out && + test_cmp expect out + ) +' + test_done From fc2c171f52d94022709bc86110711a3a9ae10a6a Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 20 Aug 2025 14:16:22 -0400 Subject: [PATCH 2/2] path-walk: create initializer for path lists The previous change fixed a bug in 'git repack -adf --path-walk' that was due to an update to how path lists are initialized and missing some important cases when processing the pending objects. This change takes the three critical places where path lists are initialized and combines them into a static method. This simplifies the callers somewhat while also helping to avoid a missed update in the future. The other places where a path list (struct type_and_oid_list) is initialized is for the following "fixed" lists: * Tag objects. * Commit objects. * Root trees. * Tagged trees. * Tagged blobs. These lists are created and consumed in different ways, with only the root trees being passed into the logic that cares about the "maybe_interesting" bit. It is appropriate to keep these uses separate. Signed-off-by: Derrick Stolee --- path-walk.c | 57 +++++++++++++++++++++++------------------------------ 1 file changed, 25 insertions(+), 32 deletions(-) diff --git a/path-walk.c b/path-walk.c index 1215ed398f4fb6..f1ceed99e94ca9 100644 --- a/path-walk.c +++ b/path-walk.c @@ -105,6 +105,24 @@ static void push_to_stack(struct path_walk_context *ctx, prio_queue_put(&ctx->path_stack, xstrdup(path)); } +static void add_path_to_list(struct path_walk_context *ctx, + const char *path, + enum object_type type, + struct object_id *oid, + int interesting) +{ + struct type_and_oid_list *list = strmap_get(&ctx->paths_to_lists, path); + + if (!list) { + CALLOC_ARRAY(list, 1); + list->type = type; + strmap_put(&ctx->paths_to_lists, path, list); + } + + list->maybe_interesting |= interesting; + oid_array_append(&list->oids, oid); +} + static int add_tree_entries(struct path_walk_context *ctx, const char *base_path, struct object_id *oid) @@ -129,7 +147,6 @@ static int add_tree_entries(struct path_walk_context *ctx, init_tree_desc(&desc, &tree->object.oid, tree->buffer, tree->size); while (tree_entry(&desc, &entry)) { - struct type_and_oid_list *list; struct object *o; /* Not actually true, but we will ignore submodules later. */ enum object_type type = S_ISDIR(entry.mode) ? OBJ_TREE : OBJ_BLOB; @@ -190,17 +207,10 @@ static int add_tree_entries(struct path_walk_context *ctx, continue; } - if (!(list = strmap_get(&ctx->paths_to_lists, path.buf))) { - CALLOC_ARRAY(list, 1); - list->type = type; - strmap_put(&ctx->paths_to_lists, path.buf, list); - } - push_to_stack(ctx, path.buf); - - if (!(o->flags & UNINTERESTING)) - list->maybe_interesting = 1; + add_path_to_list(ctx, path.buf, type, &entry.oid, + !(o->flags & UNINTERESTING)); - oid_array_append(&list->oids, &entry.oid); + push_to_stack(ctx, path.buf); } free_tree_buffer(tree); @@ -377,16 +387,9 @@ static int setup_pending_objects(struct path_walk_info *info, if (!info->trees) continue; if (pending->path) { - struct type_and_oid_list *list; char *path = *pending->path ? xstrfmt("%s/", pending->path) : xstrdup(""); - if (!(list = strmap_get(&ctx->paths_to_lists, path))) { - CALLOC_ARRAY(list, 1); - list->type = OBJ_TREE; - strmap_put(&ctx->paths_to_lists, path, list); - } - list->maybe_interesting = 1; - oid_array_append(&list->oids, &obj->oid); + add_path_to_list(ctx, path, OBJ_TREE, &obj->oid, 1); free(path); } else { /* assume a root tree, such as a lightweight tag. */ @@ -397,20 +400,10 @@ static int setup_pending_objects(struct path_walk_info *info, case OBJ_BLOB: if (!info->blobs) continue; - if (pending->path) { - struct type_and_oid_list *list; - char *path = pending->path; - if (!(list = strmap_get(&ctx->paths_to_lists, path))) { - CALLOC_ARRAY(list, 1); - list->type = OBJ_BLOB; - strmap_put(&ctx->paths_to_lists, path, list); - } - list->maybe_interesting = 1; - oid_array_append(&list->oids, &obj->oid); - } else { - /* assume a root tree, such as a lightweight tag. */ + if (pending->path) + add_path_to_list(ctx, pending->path, OBJ_BLOB, &obj->oid, 1); + else oid_array_append(&tagged_blobs->oids, &obj->oid); - } break; case OBJ_COMMIT: