Re: [PATCH 2/3] path-walk: fix setup of pending objects
From: Elijah Newren <hidden>
Date: 2025-08-21 23:22:07
On Wed, Aug 20, 2025 at 11:40 AM Derrick Stolee via GitGitGadget [off-list ref] wrote:
From: Derrick Stolee <redacted> The previous change established a buggy instance of 'git repack -adf --path-walk' when there exist paths that are tracked in the index and that is the only instance of those paths in the history of the repository. This change fixes that bug. 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 UNINITERSTING 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. This is the fix for now.
What is the fix for now? I think I can deduce it from the above paragraphs, but then either this sentence is unnecessary or it's just confusing. Perhaps change the last sentence to "Initialize it to fix this problem." or something along those lines?
To help avoid this from happening in the future, a follow-up change will
I appreciate the future-proofing, which I think you are alluding to, but to me, "To help avoid this..." suggests your change might not (fully?) fix the bug you are discussing. Perhaps you mean "To help avoid similar problems..." or "A follow-up change will add some future-proofing to prevent similar problems by..."? Or maybe I'm just reading your sentence weird...
quoted hunk ↗ jump to hunk
make initializing lists use a shared method instead of allowing for an update to this initialization process to miss some existing copies. Signed-off-by: Derrick Stolee <redacted> --- path-walk.c | 2 ++ t/t7700-repack.sh | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-)diff --git a/path-walk.c b/path-walk.c index 2d4ddbadd50f..1215ed398f4f 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 1998d9bf291c..030e9e5b2dc7 100755 --- a/t/t7700-repack.sh +++ b/t/t7700-repack.sh@@ -838,7 +838,7 @@ test_expect_success '-n overrides repack.updateServerInfo=true' ' test_server_info_missing ' -test_expect_failure 'pending objects are repacked appropriately' ' +test_expect_success 'pending objects are repacked appropriately' ' git init pending && ( --gitgitgadget
Patch looks good.