{"thread":{"id":"63998","subject":"[PATCH 0/3] [2.51.0 Bug] Missing singleton objects in 'git repack -adf --path-walk'","startedAt":"2025-08-20T18:40:00Z","lastAt":"2025-09-02T11:19:39Z","messageCount":22,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Derrick Stolee","Patrick Steinhardt","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"524542","messageId":"pull.1956.git.1755715196.gitgitgadget@gmail.com","threadId":"63998","inReplyTo":null,"subject":"[PATCH 0/3] [2.51.0 Bug] Missing singleton objects in 'git repack -adf --path-walk'","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-20T18:39:53Z","receivedAt":"2025-08-20T18:40:00Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Now that the --path-walk feature for git repack is out in the wild and\ngetting more visibility than it did in the Git for Windows fork, the\nfollowing issue was brought to my attention:\n\nSome folks would report missing objects after git repack -adf --path-walk!\n\nIt turns out that this snuck through the cracks because it was pretty\ndifficult to create a reproducing test case (patch 1) but it boils down to:\n\n 1. A path has exactly one version across all of the history being repacked.\n 2. That path is in the index.\n 3. The object at that path is not a loose object.\n 4. pack.useSparse=true in the config (this is the default)\n\nIt is also something where users don't necessarily notice the missing\nobjects until they fetch and a missing object is used as a delta base. Doing\nnormal checkouts doesn't cause changes to these files, so they are never\nopened by Git. Users hitting this issue can usually recover using git fetch\n--refetch to repopulate the missing objects from a remote (unless they never\nhad a remote at all).\n\nPatch 2 introduces the fix for this issue, which is related to forgetting to\ninitialize a struct indicator when walking the pending objects.\n\nWhen reflecting on the ways that I missed this when building the feature, I\nthink the core issue was an overreliance on using bare repos in testing. I\nalso think that the way that the UNINTERESTING object exploration was\nimplemented was particularly fragile to missing updates to the\ninitialization of the struct, so patch 3 adds a new initializer to reduce\nduplicate code and to help avoid this mistake in the future.\n\nThanks, -Stolee\n\nP.S. CC'ing all original reviewers of the series.\n\nDerrick Stolee (3):\n  t7700: add failing --path-walk test\n  path-walk: fix setup of pending objects\n  path-walk: create initializer for path lists\n\n path-walk.c       | 55 +++++++++++++++++++++--------------------------\n t/t7700-repack.sh | 43 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 68 insertions(+), 30 deletions(-)\n\n\nbase-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1956%2Fderrickstolee%2Fpath-walk-missing-objects-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1956/derrickstolee/path-walk-missing-objects-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1956\n-- \ngitgitgadget\n"},{"id":"524543","messageId":"5b19173c03da676b3e1effda7ba6d2ef5666cad6.1755715196.git.gitgitgadget@gmail.com","threadId":"63998","inReplyTo":"pull.1956.git.1755715196.gitgitgadget@gmail.com","subject":"[PATCH 1/3] t7700: add failing --path-walk test","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-20T18:39:54Z","receivedAt":"2025-08-20T18:40:00Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <stolee@gmail.com>\n\nUsers reported an issue where objects were missing from their local\nenlistments after a full repack using 'git repack -adf --path-walk'.\nThis was alarming, but took a while to create a reproducer.\n\nThe root cause is that certain objects existed in the index and had no\nsecond versions. These objects are usually blobs, though trees can be\nincluded if a cache-tree exists. The issue is that the revision walk\nadds these objects to the \"pending\" list and the path-walk API forgets\nto mark the lists it creates at this point as \"maybe_interesting\". If\nthese paths only ever have a single version in the history of the repo\n(including the current staged version) then the parent directory never\ntries to add a new object to the list and mark the list as\n\"maybe_interesting\". Thus, when walking the list later, the group is\nskipped as it is expected that no objects are interesting. This happens\neven when there are actually no UNINTERESTING objects at all! This is\nbased on the optimization enabled by the pack.useSparse=true config\noption, which is the default.\n\nThus, we create a test case that demonstrates the many cases of this\nissue for reproducibility:\n\n 1. File a/b/c has only one committed version.\n 2. Files a/i and x/y only exists as staged changes.\n 3. Tree x/ only exists in the cache-tree.\n\nAfter performing a non-path-walk repack to force all loose objects into\npackfiles, run a --path-walk repack followed by 'git fsck'. This fsck is\nwhat fails with the following errors:\n\n  error: invalid object 100644 f2e41136... for 'a/b/c'\n\n    This is the dropped instance of the single-versioned a/b/c file.\n\n  broken link from    tree cfda31d8...\n                to    tree 3f725fcd...\n\n    This is the missing tree for the single-versioned a/b/ directory.\n\n  missing blob 0ddf2bae... (a/i)\n  missing blob 975fbec8... (x/y)\n  missing blob a60d869d... (file)\n  missing blob f2e41136... (a/b/c)\n\n  missing tree 3f725fcd... (a/b/)\n\n  dangling tree 5896d7e... (staged root tree)\n\nNote that since the staged root tree is missing, the fsck output cannot\neven report that the staged x/ tree is missing as well.\n\nThis bug will be fixed in the next change.\n\nSigned-off-by: Derrick Stolee <stolee@gmail.com>\n---\n t/t7700-repack.sh | 43 +++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 43 insertions(+)\n\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 611755cc139b..1998d9bf291c 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -838,4 +838,47 @@ test_expect_success '-n overrides repack.updateServerInfo=true' '\n \ttest_server_info_missing\n '\n \n+test_expect_failure 'pending objects are repacked appropriately' '\n+\tgit init pending &&\n+\n+\t(\n+\t\tcd pending &&\n+\n+\t\tmkdir -p a/b &&\n+\t\techo singleton >file &&\n+\t\techo stuff >a/b/c &&\n+\t\techo more >a/d &&\n+\t\tgit add file a &&\n+\t\tgit commit -m \"single blobs\" &&\n+\n+\t\techo d >a/d &&\n+\t\techo e >a/e &&\n+\t\tgit add a &&\n+\t\tgit commit -m \"more blobs\" &&\n+\n+\t\t# This use of a sparse index helps to force\n+\t\t# test that the cache-tree is walked, too.\n+\t\tgit sparse-checkout set --sparse-index a x &&\n+\n+\t\t# Just _stage_ the changes.\n+\t\techo f >a/d &&\n+\t\techo h >a/e &&\n+\t\techo i >a/i &&\n+\t\tmkdir x &&\n+\t\techo y >x/y &&\n+\t\tgit add a x &&\n+\n+\t\t# Bring the loose objects into a packfile to avoid\n+\t\t# leftovers in next test. Without this, the loose\n+\t\t# objects persist and the test succeeds for other\n+\t\t# reasons.\n+\t\tgit repack -adf &&\n+\t\tgit fsck &&\n+\n+\t\t# Test path walk version with pack.useSparse.\n+\t\tgit -c pack.useSparse=true repack -adf --path-walk &&\n+\t\tgit fsck\n+\t)\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"524545","messageId":"0dc4a6323e66598070b403d286ee1918e6a9b791.1755715196.git.gitgitgadget@gmail.com","threadId":"63998","inReplyTo":"pull.1956.git.1755715196.gitgitgadget@gmail.com","subject":"[PATCH 2/3] path-walk: fix setup of pending objects","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-20T18:39:55Z","receivedAt":"2025-08-20T18:40:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <stolee@gmail.com>\n\nThe previous change established a buggy instance of 'git repack -adf\n--path-walk' when there exist paths that are tracked in the index and\nthat is the only instance of those paths in the history of the\nrepository. This change fixes that bug.\n\nThe core problem here is that the \"maybe_interesting\" member of 'struct\ntype_and_oid_list' is not initialized to '1'. This member was added in\n6333e7ae0b (path-walk: mark trees and blobs as UNINTERESTING,\n2024-12-20) in a way to help when creating packfiles for a small commit\nrange using the sparse path algorithm (enabled by pack.useSparse=true).\n\nThe idea here is that the list is marked as \"maybe_interesting\" if an\nobject is added that does not have the UNINITERSTING flag on it. Later,\nthis is checked again in case all objects in the list were marked\nUNINTERESTING after that point in time. In this case, the algorithm\nskips the list as there is no reason to visit it.\n\nThis leads to the problem where the \"maybe_interesting\" member was not\nappropriately initialized when the list is created from pending objects.\nThis is the fix for now.\n\nTo help avoid this from happening in the future, a follow-up change will\nmake initializing lists use a shared method instead of allowing for an\nupdate to this initialization process to miss some existing copies.\n\nSigned-off-by: Derrick Stolee <stolee@gmail.com>\n---\n path-walk.c       | 2 ++\n t/t7700-repack.sh | 2 +-\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/path-walk.c b/path-walk.c\nindex 2d4ddbadd50f..1215ed398f4f 100644\n--- a/path-walk.c\n+++ b/path-walk.c\n@@ -385,6 +385,7 @@ static int setup_pending_objects(struct path_walk_info *info,\n \t\t\t\t\tlist->type = OBJ_TREE;\n \t\t\t\t\tstrmap_put(&ctx->paths_to_lists, path, list);\n \t\t\t\t}\n+\t\t\t\tlist->maybe_interesting = 1;\n \t\t\t\toid_array_append(&list->oids, &obj->oid);\n \t\t\t\tfree(path);\n \t\t\t} else {\n@@ -404,6 +405,7 @@ static int setup_pending_objects(struct path_walk_info *info,\n \t\t\t\t\tlist->type = OBJ_BLOB;\n \t\t\t\t\tstrmap_put(&ctx->paths_to_lists, path, list);\n \t\t\t\t}\n+\t\t\t\tlist->maybe_interesting = 1;\n \t\t\t\toid_array_append(&list->oids, &obj->oid);\n \t\t\t} else {\n \t\t\t\t/* assume a root tree, such as a lightweight tag. */\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 1998d9bf291c..030e9e5b2dc7 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -838,7 +838,7 @@ test_expect_success '-n overrides repack.updateServerInfo=true' '\n \ttest_server_info_missing\n '\n \n-test_expect_failure 'pending objects are repacked appropriately' '\n+test_expect_success 'pending objects are repacked appropriately' '\n \tgit init pending &&\n \n \t(\n-- \ngitgitgadget\n\n"},{"id":"524544","messageId":"dd458e043548a8ed33070657cc98128efb606847.1755715196.git.gitgitgadget@gmail.com","threadId":"63998","inReplyTo":"pull.1956.git.1755715196.gitgitgadget@gmail.com","subject":"[PATCH 3/3] path-walk: create initializer for path lists","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-20T18:39:56Z","receivedAt":"2025-08-20T18:40:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <stolee@gmail.com>\n\nThe previous change fixed a bug in 'git repack -adf --path-walk' that\nwas due to an update to how path lists are initialized and missing some\nimportant cases when processing the pending objects.\n\nThis change takes the three critical places where path lists are\ninitialized and combines them into a static method. This simplifies the\ncallers somewhat while also helping to avoid a missed update in the\nfuture.\n\nThe other places where a path list (struct type_and_oid_list) is\ninitialized is for the following \"fixed\" lists:\n\n * Tag objects.\n * Commit objects.\n * Root trees.\n * Tagged trees.\n * Tagged blobs.\n\nThese lists are created and consumed in different ways, with only the\nroot trees being passed into the logic that cares about the\n\"maybe_interesting\" bit. It is appropriate to keep these uses separate.\n\nSigned-off-by: Derrick Stolee <stolee@gmail.com>\n---\n path-walk.c | 57 +++++++++++++++++++++++------------------------------\n 1 file changed, 25 insertions(+), 32 deletions(-)\n\ndiff --git a/path-walk.c b/path-walk.c\nindex 1215ed398f4f..f1ceed99e94c 100644\n--- a/path-walk.c\n+++ b/path-walk.c\n@@ -105,6 +105,24 @@ static void push_to_stack(struct path_walk_context *ctx,\n \tprio_queue_put(&ctx->path_stack, xstrdup(path));\n }\n \n+static void add_path_to_list(struct path_walk_context *ctx,\n+\t\t\t     const char *path,\n+\t\t\t     enum object_type type,\n+\t\t\t     struct object_id *oid,\n+\t\t\t     int interesting)\n+{\n+\tstruct type_and_oid_list *list = strmap_get(&ctx->paths_to_lists, path);\n+\n+\tif (!list) {\n+\t\tCALLOC_ARRAY(list, 1);\n+\t\tlist->type = type;\n+\t\tstrmap_put(&ctx->paths_to_lists, path, list);\n+\t}\n+\n+\tlist->maybe_interesting |= interesting;\n+\toid_array_append(&list->oids, oid);\n+}\n+\n static int add_tree_entries(struct path_walk_context *ctx,\n \t\t\t    const char *base_path,\n \t\t\t    struct object_id *oid)\n@@ -129,7 +147,6 @@ static int add_tree_entries(struct path_walk_context *ctx,\n \n \tinit_tree_desc(&desc, &tree->object.oid, tree->buffer, tree->size);\n \twhile (tree_entry(&desc, &entry)) {\n-\t\tstruct type_and_oid_list *list;\n \t\tstruct object *o;\n \t\t/* Not actually true, but we will ignore submodules later. */\n \t\tenum object_type type = S_ISDIR(entry.mode) ? OBJ_TREE : OBJ_BLOB;\n@@ -190,17 +207,10 @@ static int add_tree_entries(struct path_walk_context *ctx,\n \t\t\t\tcontinue;\n \t\t}\n \n-\t\tif (!(list = strmap_get(&ctx->paths_to_lists, path.buf))) {\n-\t\t\tCALLOC_ARRAY(list, 1);\n-\t\t\tlist->type = type;\n-\t\t\tstrmap_put(&ctx->paths_to_lists, path.buf, list);\n-\t\t}\n-\t\tpush_to_stack(ctx, path.buf);\n-\n-\t\tif (!(o->flags & UNINTERESTING))\n-\t\t\tlist->maybe_interesting = 1;\n+\t\tadd_path_to_list(ctx, path.buf, type, &entry.oid,\n+\t\t\t\t !(o->flags & UNINTERESTING));\n \n-\t\toid_array_append(&list->oids, &entry.oid);\n+\t\tpush_to_stack(ctx, path.buf);\n \t}\n \n \tfree_tree_buffer(tree);\n@@ -377,16 +387,9 @@ static int setup_pending_objects(struct path_walk_info *info,\n \t\t\tif (!info->trees)\n \t\t\t\tcontinue;\n \t\t\tif (pending->path) {\n-\t\t\t\tstruct type_and_oid_list *list;\n \t\t\t\tchar *path = *pending->path ? xstrfmt(\"%s/\", pending->path)\n \t\t\t\t\t\t\t    : xstrdup(\"\");\n-\t\t\t\tif (!(list = strmap_get(&ctx->paths_to_lists, path))) {\n-\t\t\t\t\tCALLOC_ARRAY(list, 1);\n-\t\t\t\t\tlist->type = OBJ_TREE;\n-\t\t\t\t\tstrmap_put(&ctx->paths_to_lists, path, list);\n-\t\t\t\t}\n-\t\t\t\tlist->maybe_interesting = 1;\n-\t\t\t\toid_array_append(&list->oids, &obj->oid);\n+\t\t\t\tadd_path_to_list(ctx, path, OBJ_TREE, &obj->oid, 1);\n \t\t\t\tfree(path);\n \t\t\t} else {\n \t\t\t\t/* assume a root tree, such as a lightweight tag. */\n@@ -397,20 +400,10 @@ static int setup_pending_objects(struct path_walk_info *info,\n \t\tcase OBJ_BLOB:\n \t\t\tif (!info->blobs)\n \t\t\t\tcontinue;\n-\t\t\tif (pending->path) {\n-\t\t\t\tstruct type_and_oid_list *list;\n-\t\t\t\tchar *path = pending->path;\n-\t\t\t\tif (!(list = strmap_get(&ctx->paths_to_lists, path))) {\n-\t\t\t\t\tCALLOC_ARRAY(list, 1);\n-\t\t\t\t\tlist->type = OBJ_BLOB;\n-\t\t\t\t\tstrmap_put(&ctx->paths_to_lists, path, list);\n-\t\t\t\t}\n-\t\t\t\tlist->maybe_interesting = 1;\n-\t\t\t\toid_array_append(&list->oids, &obj->oid);\n-\t\t\t} else {\n-\t\t\t\t/* assume a root tree, such as a lightweight tag. */\n+\t\t\tif (pending->path)\n+\t\t\t\tadd_path_to_list(ctx, pending->path, OBJ_BLOB, &obj->oid, 1);\n+\t\t\telse\n \t\t\t\toid_array_append(&tagged_blobs->oids, &obj->oid);\n-\t\t\t}\n \t\t\tbreak;\n \n \t\tcase OBJ_COMMIT:\n-- \ngitgitgadget\n"},{"id":"524546","messageId":"xmqqh5y1ak9o.fsf@gitster.g","threadId":"63998","inReplyTo":"0dc4a6323e66598070b403d286ee1918e6a9b791.1755715196.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] path-walk: fix setup of pending objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-20T19:02:59Z","receivedAt":"2025-08-20T19:03:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> The core problem here is that the \"maybe_interesting\" member of 'struct\n> type_and_oid_list' is not initialized to '1'. This member was added in\n> 6333e7ae0b (path-walk: mark trees and blobs as UNINTERESTING,\n> 2024-12-20) in a way to help when creating packfiles for a small commit\n> range using the sparse path algorithm (enabled by pack.useSparse=true).\n\nOK, in other words, the bug is fairly contained within the path-walk\ntraversal.  We treat things as reachable not just from ref tips and\nreflogs (where path-walk code can use the tree object to compute on\nwhat pathname each blob comes from) and the main index array (that\nhas paths, even though it needs separate way to compute than those\nfor trees), but also from places like REUC and TREE extensions that\nmake associations between pathnames and objects.  Are they also OK?\n\n> To help avoid this from happening in the future, a follow-up change will\n> make initializing lists use a shared method instead of allowing for an\n> update to this initialization process to miss some existing copies.\n\nGreat.  Future-proofing is 100 times better than just a bugfix.\n\n>\n> Signed-off-by: Derrick Stolee <stolee@gmail.com>\n> ---\n>  path-walk.c       | 2 ++\n>  t/t7700-repack.sh | 2 +-\n>  2 files changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/path-walk.c b/path-walk.c\n> index 2d4ddbadd50f..1215ed398f4f 100644\n> --- a/path-walk.c\n> +++ b/path-walk.c\n> @@ -385,6 +385,7 @@ static int setup_pending_objects(struct path_walk_info *info,\n>  \t\t\t\t\tlist->type = OBJ_TREE;\n>  \t\t\t\t\tstrmap_put(&ctx->paths_to_lists, path, list);\n>  \t\t\t\t}\n> +\t\t\t\tlist->maybe_interesting = 1;\n>  \t\t\t\toid_array_append(&list->oids, &obj->oid);\n>  \t\t\t\tfree(path);\n>  \t\t\t} else {\n> @@ -404,6 +405,7 @@ static int setup_pending_objects(struct path_walk_info *info,\n>  \t\t\t\t\tlist->type = OBJ_BLOB;\n>  \t\t\t\t\tstrmap_put(&ctx->paths_to_lists, path, list);\n>  \t\t\t\t}\n> +\t\t\t\tlist->maybe_interesting = 1;\n>  \t\t\t\toid_array_append(&list->oids, &obj->oid);\n>  \t\t\t} else {\n>  \t\t\t\t/* assume a root tree, such as a lightweight tag. */\n> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n> index 1998d9bf291c..030e9e5b2dc7 100755\n> --- a/t/t7700-repack.sh\n> +++ b/t/t7700-repack.sh\n> @@ -838,7 +838,7 @@ test_expect_success '-n overrides repack.updateServerInfo=true' '\n>  \ttest_server_info_missing\n>  '\n>  \n> -test_expect_failure 'pending objects are repacked appropriately' '\n> +test_expect_success 'pending objects are repacked appropriately' '\n>  \tgit init pending &&\n>  \n>  \t(\n"},{"id":"524553","messageId":"c6ab0985-2a74-406b-9685-cc65f8748515@gmail.com","threadId":"63998","inReplyTo":"xmqqh5y1ak9o.fsf@gitster.g","subject":"Re: [PATCH 2/3] path-walk: fix setup of pending objects","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2025-08-20T19:42:11Z","receivedAt":"2025-08-20T19:42:34Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/20/2025 3:02 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> The core problem here is that the \"maybe_interesting\" member of 'struct\n>> type_and_oid_list' is not initialized to '1'. This member was added in\n>> 6333e7ae0b (path-walk: mark trees and blobs as UNINTERESTING,\n>> 2024-12-20) in a way to help when creating packfiles for a small commit\n>> range using the sparse path algorithm (enabled by pack.useSparse=true).\n> \n> OK, in other words, the bug is fairly contained within the path-walk\n> traversal.  We treat things as reachable not just from ref tips and\n> reflogs (where path-walk code can use the tree object to compute on\n> what pathname each blob comes from) and the main index array (that\n> has paths, even though it needs separate way to compute than those\n> for trees), but also from places like REUC and TREE extensions that\n> make associations between pathnames and objects.  Are they also OK?\n\nThe key integration point is the \"pending\" list operating a bit\ndifferent from walking directly from tags or commits. I was trying\nto reproduce the issue from all of those other sources before unlocking\nthe \"singleton\" nature of the problem, and failed to do so.\n\nThe resolve-undo cache (REUC) is something that I had not tested\npreviously. Adding \"git rm --cached x/y\" to the test in the previous\ncase leads to the 'git fsck' call giving a \"dangling blob\" warning,\nso that could be an interesting way to strengthen the test. Thanks,\n-Stolee\n\n"},{"id":"524627","messageId":"aKbSObIzXwUtjAdE@pks.im","threadId":"63998","inReplyTo":"5b19173c03da676b3e1effda7ba6d2ef5666cad6.1755715196.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] t7700: add failing --path-walk test","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-21T08:00:57Z","receivedAt":"2025-08-21T08:01:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Aug 20, 2025 at 06:39:54PM +0000, Derrick Stolee via GitGitGadget wrote:\n> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n> index 611755cc139b..1998d9bf291c 100755\n> --- a/t/t7700-repack.sh\n> +++ b/t/t7700-repack.sh\n> @@ -838,4 +838,47 @@ test_expect_success '-n overrides repack.updateServerInfo=true' '\n\nTiny nit: I would've probably squashed this patch into the second patch,\nas we usually don't use the add-failing-test-and-then-fix-it-later\ndance. On the other hand though it gives some nice context, so I\nultimately don't mind it all that much. So please feel free to ignore\nthis nit.\n\n>  \ttest_server_info_missing\n>  '\n>  \n> +test_expect_failure 'pending objects are repacked appropriately' '\n> +\tgit init pending &&\n\nWe probably also want `test_when_finished \"rm -rf pending\"` before\ncalling git-init(1).\n\n> +\n> +\t(\n> +\t\tcd pending &&\n> +\n> +\t\tmkdir -p a/b &&\n> +\t\techo singleton >file &&\n> +\t\techo stuff >a/b/c &&\n> +\t\techo more >a/d &&\n> +\t\tgit add file a &&\n> +\t\tgit commit -m \"single blobs\" &&\n> +\n> +\t\techo d >a/d &&\n> +\t\techo e >a/e &&\n> +\t\tgit add a &&\n> +\t\tgit commit -m \"more blobs\" &&\n> +\n> +\t\t# This use of a sparse index helps to force\n> +\t\t# test that the cache-tree is walked, too.\n> +\t\tgit sparse-checkout set --sparse-index a x &&\n> +\n> +\t\t# Just _stage_ the changes.\n> +\t\techo f >a/d &&\n> +\t\techo h >a/e &&\n> +\t\techo i >a/i &&\n> +\t\tmkdir x &&\n> +\t\techo y >x/y &&\n> +\t\tgit add a x &&\n\nNit: I think I would've moved the explanations you have in the commit\nmessage into these hunks so that the test becomes a bit more\nself-explanatory.\n\n> +\t\t# Bring the loose objects into a packfile to avoid\n> +\t\t# leftovers in next test. Without this, the loose\n> +\t\t# objects persist and the test succeeds for other\n> +\t\t# reasons.\n> +\t\tgit repack -adf &&\n> +\t\tgit fsck &&\n> +\n> +\t\t# Test path walk version with pack.useSparse.\n> +\t\tgit -c pack.useSparse=true repack -adf --path-walk &&\n> +\t\tgit fsck\n> +\t)\n> +'\n\nPatrick\n"},{"id":"524628","messageId":"aKbSRQJCPh3Lsew8@pks.im","threadId":"63998","inReplyTo":"0dc4a6323e66598070b403d286ee1918e6a9b791.1755715196.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] path-walk: fix setup of pending objects","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-21T08:01:09Z","receivedAt":"2025-08-21T08:01:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Aug 20, 2025 at 06:39:55PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee <stolee@gmail.com>\n> \n> The previous change established a buggy instance of 'git repack -adf\n> --path-walk' when there exist paths that are tracked in the index and\n> that is the only instance of those paths in the history of the\n> repository. This change fixes that bug.\n> \n> The core problem here is that the \"maybe_interesting\" member of 'struct\n> type_and_oid_list' is not initialized to '1'. This member was added in\n> 6333e7ae0b (path-walk: mark trees and blobs as UNINTERESTING,\n> 2024-12-20) in a way to help when creating packfiles for a small commit\n> range using the sparse path algorithm (enabled by pack.useSparse=true).\n> \n> The idea here is that the list is marked as \"maybe_interesting\" if an\n> object is added that does not have the UNINITERSTING flag on it. Later,\n\ns/UNINITERSTING/UNINTERESTING/\n\n> this is checked again in case all objects in the list were marked\n> UNINTERESTING after that point in time. In this case, the algorithm\n> skips the list as there is no reason to visit it.\n> \n> This leads to the problem where the \"maybe_interesting\" member was not\n> appropriately initialized when the list is created from pending objects.\n> This is the fix for now.\n> \n> To help avoid this from happening in the future, a follow-up change will\n> make initializing lists use a shared method instead of allowing for an\n> update to this initialization process to miss some existing copies.\n\nYeah, I wanted to say that this feels quite fragile to me and very easy\nto miss. Does this mechanism buy us a lot of performance in the first\nplace? Because if not we might as well just remove it entirely.\n\nBut if the answer is \"yes\" then adding APIs around it feels like a good\nalternative.\n\nPatrick\n"},{"id":"524629","messageId":"aKbSTN-rfGzadPsm@pks.im","threadId":"63998","inReplyTo":"c6ab0985-2a74-406b-9685-cc65f8748515@gmail.com","subject":"Re: [PATCH 2/3] path-walk: fix setup of pending objects","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-21T08:01:16Z","receivedAt":"2025-08-21T08:01:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Aug 20, 2025 at 03:42:11PM -0400, Derrick Stolee wrote:\n> On 8/20/2025 3:02 PM, Junio C Hamano wrote:\n> > \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> > \n> >> The core problem here is that the \"maybe_interesting\" member of 'struct\n> >> type_and_oid_list' is not initialized to '1'. This member was added in\n> >> 6333e7ae0b (path-walk: mark trees and blobs as UNINTERESTING,\n> >> 2024-12-20) in a way to help when creating packfiles for a small commit\n> >> range using the sparse path algorithm (enabled by pack.useSparse=true).\n> > \n> > OK, in other words, the bug is fairly contained within the path-walk\n> > traversal.  We treat things as reachable not just from ref tips and\n> > reflogs (where path-walk code can use the tree object to compute on\n> > what pathname each blob comes from) and the main index array (that\n> > has paths, even though it needs separate way to compute than those\n> > for trees), but also from places like REUC and TREE extensions that\n> > make associations between pathnames and objects.  Are they also OK?\n> \n> The key integration point is the \"pending\" list operating a bit\n> different from walking directly from tags or commits. I was trying\n> to reproduce the issue from all of those other sources before unlocking\n> the \"singleton\" nature of the problem, and failed to do so.\n> \n> The resolve-undo cache (REUC) is something that I had not tested\n> previously. Adding \"git rm --cached x/y\" to the test in the previous\n> case leads to the 'git fsck' call giving a \"dangling blob\" warning,\n> so that could be an interesting way to strengthen the test. Thanks,\n\nI also wonder a bit about the future -- if we ever add a new source for\npending objects, would the author have to amend \"path-walk.c\" to take\nthis new pending source into account?\n\nI guess the answer is \"yes\", which does make me feel a bit uneasy as\nit is very easy to now corrupt the repository.\n\nPatrick\n"},{"id":"524648","messageId":"d9f69281-6aa3-4ef7-b52f-9660bc60a46d@gmail.com","threadId":"63998","inReplyTo":"aKbSObIzXwUtjAdE@pks.im","subject":"Re: [PATCH 1/3] t7700: add failing --path-walk test","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2025-08-21T12:42:58Z","receivedAt":"2025-08-21T12:43:21Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/21/2025 4:00 AM, Patrick Steinhardt wrote:\n> On Wed, Aug 20, 2025 at 06:39:54PM +0000, Derrick Stolee via GitGitGadget wrote:\n>> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n>> index 611755cc139b..1998d9bf291c 100755\n>> --- a/t/t7700-repack.sh\n>> +++ b/t/t7700-repack.sh\n>> @@ -838,4 +838,47 @@ test_expect_success '-n overrides repack.updateServerInfo=true' '\n> \n> Tiny nit: I would've probably squashed this patch into the second patch,\n> as we usually don't use the add-failing-test-and-then-fix-it-later\n> dance. On the other hand though it gives some nice context, so I\n> ultimately don't mind it all that much. So please feel free to ignore\n> this nit.\n\nI'm probably the person who is always asking folks to create a test\nthat either fails or demonstrates the \"before\" behavior before making\nthe actual change that updates the case. This allows the ability to\n\"test the test\" by checking it in place to confirm that it is indeed\nfailing.\n\nUsing test_expect_failure allows us to avoid breaking bisect. \n>>  \ttest_server_info_missing\n>>  '\n>>  \n>> +test_expect_failure 'pending objects are repacked appropriately' '\n>> +\tgit init pending &&\n> \n> We probably also want `test_when_finished \"rm -rf pending\"` before\n> calling git-init(1).\n\nGood idea.\n\n>> +\n>> +\t(\n>> +\t\tcd pending &&\n>> +\n>> +\t\tmkdir -p a/b &&\n>> +\t\techo singleton >file &&\n>> +\t\techo stuff >a/b/c &&\n>> +\t\techo more >a/d &&\n>> +\t\tgit add file a &&\n>> +\t\tgit commit -m \"single blobs\" &&\n>> +\n>> +\t\techo d >a/d &&\n>> +\t\techo e >a/e &&\n>> +\t\tgit add a &&\n>> +\t\tgit commit -m \"more blobs\" &&\n>> +\n>> +\t\t# This use of a sparse index helps to force\n>> +\t\t# test that the cache-tree is walked, too.\n>> +\t\tgit sparse-checkout set --sparse-index a x &&\n>> +\n>> +\t\t# Just _stage_ the changes.\n>> +\t\techo f >a/d &&\n>> +\t\techo h >a/e &&\n>> +\t\techo i >a/i &&\n>> +\t\tmkdir x &&\n>> +\t\techo y >x/y &&\n>> +\t\tgit add a x &&\n> \n> Nit: I think I would've moved the explanations you have in the commit\n> message into these hunks so that the test becomes a bit more\n> self-explanatory.\n\nHm. That seems to go against our typical pattern of leaving comments\nsparse and having the longer explanation available in commit messages\nbut maybe I'm out of date or tests are a different beast. I'll see\nwhat I can do to make the test more self-documented.\n\nThanks,\n-Stolee\n\n"},{"id":"524649","messageId":"9263e8f7-8772-4b8a-a3f2-6ca2a70d645d@gmail.com","threadId":"63998","inReplyTo":"aKbSTN-rfGzadPsm@pks.im","subject":"Re: [PATCH 2/3] path-walk: fix setup of pending objects","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2025-08-21T12:55:48Z","receivedAt":"2025-08-21T12:56:12Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/21/2025 4:01 AM, Patrick Steinhardt wrote:\n> On Wed, Aug 20, 2025 at 03:42:11PM -0400, Derrick Stolee wrote:\n>> On 8/20/2025 3:02 PM, Junio C Hamano wrote:\n>>> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n>> The key integration point is the \"pending\" list operating a bit\n>> different from walking directly from tags or commits. I was trying\n>> to reproduce the issue from all of those other sources before unlocking\n>> the \"singleton\" nature of the problem, and failed to do so.\n>>\n>> The resolve-undo cache (REUC) is something that I had not tested\n>> previously. Adding \"git rm --cached x/y\" to the test in the previous\n>> case leads to the 'git fsck' call giving a \"dangling blob\" warning,\n>> so that could be an interesting way to strengthen the test. Thanks,\n> \n> I also wonder a bit about the future -- if we ever add a new source for\n> pending objects, would the author have to amend \"path-walk.c\" to take\n> this new pending source into account?\n\nI don't think the risk comes from new ways to add pending objects,\nbut that the path-walk algorithm added an \"optimization\" without\nappropriately modifying how it handled the pending objects. New\nsources of pending objects shouldn't cause any issues, unless the\nrevision API itself changed substantially.\n\nFor example, in my draft for v2 I add changes to my test to account\nfor the REUC in the repack. The new logic for the path-walk feature\nworked just fine. It does point out that 'git fsck' doesn't follow\nlinks in the REUC and so reports the blob as dangling.\n\nIf we have a new source for pending objects in the context of\npack-objects or repack, one would hope that tests would be added to\ndemonstrate the behavior and then that could be double-checked with\nGIT_TEST_PACK_PATH_WALK=1. One problem in this situation is that\nthe tests were not substantial enough for these sources. We are\ncorrecting for that now. \n> I guess the answer is \"yes\", which does make me feel a bit uneasy as\n> it is very easy to now corrupt the repository.\n\nPerhaps this has always been very easy to make a bug that leads to\nlosing objects, but we're purposefully wary to touch the logic around\nthe repacking processes.\n\nOne feature that could provide increased confidence is some step that\ndouble-checks the object list before and after a step like this to\nmake sure that no objects are dropped (or only the set we expect is\ndropped). Something like comparing the object lists in the pack-indexes\nbefore and after a repack. This might be too problematic to enable in\nall cases, but could be enabled in the test suite and certain critical\nplaces. Azure DevOps has something similar in its backend to prevent\nany change to the Git object database that can't be undone, but these\nsteps happen asynchronously to \"production data\" so it may not be\nappropriate for all server architectures.\n\nThanks,\n-Stolee\n\n"},{"id":"524667","messageId":"xmqqfrdk3aqy.fsf@gitster.g","threadId":"63998","inReplyTo":"d9f69281-6aa3-4ef7-b52f-9660bc60a46d@gmail.com","subject":"Re: [PATCH 1/3] t7700: add failing --path-walk test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-21T16:22:45Z","receivedAt":"2025-08-21T16:22:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> On 8/21/2025 4:00 AM, Patrick Steinhardt wrote:\n>> On Wed, Aug 20, 2025 at 06:39:54PM +0000, Derrick Stolee via GitGitGadget wrote:\n>>> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n>>> index 611755cc139b..1998d9bf291c 100755\n>>> --- a/t/t7700-repack.sh\n>>> +++ b/t/t7700-repack.sh\n>>> @@ -838,4 +838,47 @@ test_expect_success '-n overrides repack.updateServerInfo=true' '\n>> \n>> Tiny nit: I would've probably squashed this patch into the second patch,\n>> as we usually don't use the add-failing-test-and-then-fix-it-later\n>> dance. On the other hand though it gives some nice context, so I\n>> ultimately don't mind it all that much. So please feel free to ignore\n>> this nit.\n>\n> I'm probably the person who is always asking folks to create a test\n> that either fails or demonstrates the \"before\" behavior before making\n> the actual change that updates the case. This allows the ability to\n> \"test the test\" by checking it in place to confirm that it is indeed\n> failing.\n> Using test_expect_failure allows us to avoid breaking bisect. \n\nYes, you can develop that way, but on the reviewing and receiving\nend, the second patch that shows only the change from expect_failure\nto expect_success pushes the more important \"what behaviour was this\nthing testing?\" out of the hunk context, if you split them into two.\n\nIf we really wanted to verify the claim that without the fix this\nwas broken and we have a test to demonstrate a failure on the\nreceiving end, we can \"checkout\" paths touched by that commit\noutside t/ from HEAD^ to build to see how the system behaved without\nthe code change just fine, so such a split does not buy us much.\n\nUnless there is a strong reason not to, please always present such\ntest in the same patch as the code change to fix that breakage.\n\nThanks.\n"},{"id":"524685","messageId":"b37e2690-fe6a-4fde-ab7e-58368c2914c3@gmail.com","threadId":"63998","inReplyTo":"aKbSRQJCPh3Lsew8@pks.im","subject":"Re: [PATCH 2/3] path-walk: fix setup of pending objects","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2025-08-21T20:33:21Z","receivedAt":"2025-08-21T20:33:24Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/21/2025 4:01 AM, Patrick Steinhardt wrote:\n> On Wed, Aug 20, 2025 at 06:39:55PM +0000, Derrick Stolee via GitGitGadget wrote:\n>> From: Derrick Stolee <stolee@gmail.com>\n>>\n>> The previous change established a buggy instance of 'git repack -adf\n>> --path-walk' when there exist paths that are tracked in the index and\n>> that is the only instance of those paths in the history of the\n>> repository. This change fixes that bug.\n>>\n>> The core problem here is that the \"maybe_interesting\" member of 'struct\n>> type_and_oid_list' is not initialized to '1'. This member was added in\n>> 6333e7ae0b (path-walk: mark trees and blobs as UNINTERESTING,\n>> 2024-12-20) in a way to help when creating packfiles for a small commit\n>> range using the sparse path algorithm (enabled by pack.useSparse=true).\n>>\n>> The idea here is that the list is marked as \"maybe_interesting\" if an\n>> object is added that does not have the UNINITERSTING flag on it. Later,\n> \n> s/UNINITERSTING/UNINTERESTING/\n\nThanks!\n\n>> this is checked again in case all objects in the list were marked\n>> UNINTERESTING after that point in time. In this case, the algorithm\n>> skips the list as there is no reason to visit it.\n>>\n>> This leads to the problem where the \"maybe_interesting\" member was not\n>> appropriately initialized when the list is created from pending objects.\n>> This is the fix for now.\n>>\n>> To help avoid this from happening in the future, a follow-up change will\n>> make initializing lists use a shared method instead of allowing for an\n>> update to this initialization process to miss some existing copies.\n> \n> Yeah, I wanted to say that this feels quite fragile to me and very easy\n> to miss. Does this mechanism buy us a lot of performance in the first\n> place? Because if not we might as well just remove it entirely.\n\nThe details for the space savings and moderate time cost are listed in\ne5394794a5 (pack-objects: thread the path-based compression, 2025-05-16).\nThese improvements are on top of those from --name-hash-version=2 by\nusing a different way to focus delta calculations.\n\nA larger, internal example for this can be seen in this table (based on\ntesting today):\n\nMode      |  Size    | Time\n----------+----------+------\nversion 1 |  16.0 GB | 83min\nversion 2 |   9.9 GB | 77min\npath walk |   6.4 GB | 74min\n\n> But if the answer is \"yes\" then adding APIs around it feels like a good\n> alternative.\nMaking the code less brittle to changes is good, but also I'm interested\nin ways to improve our test infrastructure or adding defensive features.\nI mentioned a couple of ideas in an earlier message.\n\nThanks,\n-Stolee\n"},{"id":"524690","messageId":"CABPp-BH5OLfCcAgwtLfUdT6_=ZhiXYUBd+HwUgNzQ+LyRoA=jA@mail.gmail.com","threadId":"63998","inReplyTo":"5b19173c03da676b3e1effda7ba6d2ef5666cad6.1755715196.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] t7700: add failing --path-walk test","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-08-21T23:21:46Z","receivedAt":"2025-08-21T23:21:58Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Aug 20, 2025 at 11:39 AM Derrick Stolee via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Derrick Stolee <stolee@gmail.com>\n>\n> Users reported an issue where objects were missing from their local\n> enlistments after a full repack using 'git repack -adf --path-walk'.\n\nWhat is an enlistment?\n\n> This was alarming, but took a while to create a reproducer.\n\nbut => and ?\n\n> The root cause is that certain objects existed in the index and had no\n> second versions. These objects are usually blobs, though trees can be\n> included if a cache-tree exists. The issue is that the revision walk\n> adds these objects to the \"pending\" list and the path-walk API forgets\n> to mark the lists it creates at this point as \"maybe_interesting\". If\n> these paths only ever have a single version in the history of the repo\n> (including the current staged version) then the parent directory never\n> tries to add a new object to the list and mark the list as\n> \"maybe_interesting\". Thus, when walking the list later, the group is\n> skipped as it is expected that no objects are interesting. This happens\n> even when there are actually no UNINTERESTING objects at all! This is\n> based on the optimization enabled by the pack.useSparse=true config\n> option, which is the default.\n>\n> Thus, we create a test case that demonstrates the many cases of this\n> issue for reproducibility:\n>\n>  1. File a/b/c has only one committed version.\n>  2. Files a/i and x/y only exists as staged changes.\n\nexists => exist\n\n\nI didn't have any questions or spot any issues on the rest of the patch.\n"},{"id":"524691","messageId":"CABPp-BFtfofyrJYbfp0fDTTprZZtwaH_EbPkppNEq-hgRqmwhQ@mail.gmail.com","threadId":"63998","inReplyTo":"0dc4a6323e66598070b403d286ee1918e6a9b791.1755715196.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] path-walk: fix setup of pending objects","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-08-21T23:21:55Z","receivedAt":"2025-08-21T23:22:07Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Aug 20, 2025 at 11:40 AM Derrick Stolee via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Derrick Stolee <stolee@gmail.com>\n>\n> The previous change established a buggy instance of 'git repack -adf\n> --path-walk' when there exist paths that are tracked in the index and\n> that is the only instance of those paths in the history of the\n> repository. This change fixes that bug.\n>\n> The core problem here is that the \"maybe_interesting\" member of 'struct\n> type_and_oid_list' is not initialized to '1'. This member was added in\n> 6333e7ae0b (path-walk: mark trees and blobs as UNINTERESTING,\n> 2024-12-20) in a way to help when creating packfiles for a small commit\n> range using the sparse path algorithm (enabled by pack.useSparse=true).\n>\n> The idea here is that the list is marked as \"maybe_interesting\" if an\n> object is added that does not have the UNINITERSTING flag on it. Later,\n> this is checked again in case all objects in the list were marked\n> UNINTERESTING after that point in time. In this case, the algorithm\n> skips the list as there is no reason to visit it.\n>\n> This leads to the problem where the \"maybe_interesting\" member was not\n> appropriately initialized when the list is created from pending objects.\n> This is the fix for now.\n\nWhat is the fix for now?  I think I can deduce it from the above\nparagraphs, but then either this sentence is unnecessary or it's just\nconfusing.  Perhaps change the last sentence to \"Initialize it to fix\nthis problem.\" or something along those lines?\n\n> To help avoid this from happening in the future, a follow-up change will\n\nI appreciate the future-proofing, which I think you are alluding to,\nbut to me, \"To help avoid this...\" suggests your change might not\n(fully?) fix the bug you are discussing.  Perhaps you mean \"To help\navoid similar problems...\" or \"A follow-up change will add some\nfuture-proofing to prevent similar problems by...\"?  Or maybe I'm just\nreading your sentence weird...\n\n> make initializing lists use a shared method instead of allowing for an\n> update to this initialization process to miss some existing copies.\n>\n> Signed-off-by: Derrick Stolee <stolee@gmail.com>\n> ---\n>  path-walk.c       | 2 ++\n>  t/t7700-repack.sh | 2 +-\n>  2 files changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/path-walk.c b/path-walk.c\n> index 2d4ddbadd50f..1215ed398f4f 100644\n> --- a/path-walk.c\n> +++ b/path-walk.c\n> @@ -385,6 +385,7 @@ static int setup_pending_objects(struct path_walk_info *info,\n>                                         list->type = OBJ_TREE;\n>                                         strmap_put(&ctx->paths_to_lists, path, list);\n>                                 }\n> +                               list->maybe_interesting = 1;\n>                                 oid_array_append(&list->oids, &obj->oid);\n>                                 free(path);\n>                         } else {\n> @@ -404,6 +405,7 @@ static int setup_pending_objects(struct path_walk_info *info,\n>                                         list->type = OBJ_BLOB;\n>                                         strmap_put(&ctx->paths_to_lists, path, list);\n>                                 }\n> +                               list->maybe_interesting = 1;\n>                                 oid_array_append(&list->oids, &obj->oid);\n>                         } else {\n>                                 /* assume a root tree, such as a lightweight tag. */\n> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n> index 1998d9bf291c..030e9e5b2dc7 100755\n> --- a/t/t7700-repack.sh\n> +++ b/t/t7700-repack.sh\n> @@ -838,7 +838,7 @@ test_expect_success '-n overrides repack.updateServerInfo=true' '\n>         test_server_info_missing\n>  '\n>\n> -test_expect_failure 'pending objects are repacked appropriately' '\n> +test_expect_success 'pending objects are repacked appropriately' '\n>         git init pending &&\n>\n>         (\n> --\n> gitgitgadget\n\nPatch looks good.\n"},{"id":"524692","messageId":"CABPp-BEvFL8WYyn404u09y6UyTmbPbyrj6N=YvpMZZmpui0mvQ@mail.gmail.com","threadId":"63998","inReplyTo":"dd458e043548a8ed33070657cc98128efb606847.1755715196.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] path-walk: create initializer for path lists","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-08-21T23:22:01Z","receivedAt":"2025-08-21T23:22:13Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Aug 20, 2025 at 11:40 AM Derrick Stolee via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Derrick Stolee <stolee@gmail.com>\n>\n> The previous change fixed a bug in 'git repack -adf --path-walk' that\n> was due to an update to how path lists are initialized and missing some\n> important cases when processing the pending objects.\n>\n> This change takes the three critical places where path lists are\n> initialized and combines them into a static method. This simplifies the\n> callers somewhat while also helping to avoid a missed update in the\n> future.\n>\n> The other places where a path list (struct type_and_oid_list) is\n> initialized is for the following \"fixed\" lists:\n>\n>  * Tag objects.\n>  * Commit objects.\n>  * Root trees.\n>  * Tagged trees.\n>  * Tagged blobs.\n>\n> These lists are created and consumed in different ways, with only the\n> root trees being passed into the logic that cares about the\n> \"maybe_interesting\" bit. It is appropriate to keep these uses separate.\n>\n> Signed-off-by: Derrick Stolee <stolee@gmail.com>\n> ---\n>  path-walk.c | 57 +++++++++++++++++++++++------------------------------\n>  1 file changed, 25 insertions(+), 32 deletions(-)\n>\n> diff --git a/path-walk.c b/path-walk.c\n> index 1215ed398f4f..f1ceed99e94c 100644\n> --- a/path-walk.c\n> +++ b/path-walk.c\n> @@ -105,6 +105,24 @@ static void push_to_stack(struct path_walk_context *ctx,\n>         prio_queue_put(&ctx->path_stack, xstrdup(path));\n>  }\n>\n> +static void add_path_to_list(struct path_walk_context *ctx,\n> +                            const char *path,\n> +                            enum object_type type,\n> +                            struct object_id *oid,\n> +                            int interesting)\n> +{\n> +       struct type_and_oid_list *list = strmap_get(&ctx->paths_to_lists, path);\n> +\n> +       if (!list) {\n> +               CALLOC_ARRAY(list, 1);\n> +               list->type = type;\n> +               strmap_put(&ctx->paths_to_lists, path, list);\n> +       }\n> +\n> +       list->maybe_interesting |= interesting;\n> +       oid_array_append(&list->oids, oid);\n> +}\n> +\n>  static int add_tree_entries(struct path_walk_context *ctx,\n>                             const char *base_path,\n>                             struct object_id *oid)\n> @@ -129,7 +147,6 @@ static int add_tree_entries(struct path_walk_context *ctx,\n>\n>         init_tree_desc(&desc, &tree->object.oid, tree->buffer, tree->size);\n>         while (tree_entry(&desc, &entry)) {\n> -               struct type_and_oid_list *list;\n>                 struct object *o;\n>                 /* Not actually true, but we will ignore submodules later. */\n>                 enum object_type type = S_ISDIR(entry.mode) ? OBJ_TREE : OBJ_BLOB;\n> @@ -190,17 +207,10 @@ static int add_tree_entries(struct path_walk_context *ctx,\n>                                 continue;\n>                 }\n>\n> -               if (!(list = strmap_get(&ctx->paths_to_lists, path.buf))) {\n> -                       CALLOC_ARRAY(list, 1);\n> -                       list->type = type;\n> -                       strmap_put(&ctx->paths_to_lists, path.buf, list);\n> -               }\n> -               push_to_stack(ctx, path.buf);\n> -\n> -               if (!(o->flags & UNINTERESTING))\n> -                       list->maybe_interesting = 1;\n> +               add_path_to_list(ctx, path.buf, type, &entry.oid,\n> +                                !(o->flags & UNINTERESTING));\n>\n> -               oid_array_append(&list->oids, &entry.oid);\n> +               push_to_stack(ctx, path.buf);\n>         }\n>\n>         free_tree_buffer(tree);\n> @@ -377,16 +387,9 @@ static int setup_pending_objects(struct path_walk_info *info,\n>                         if (!info->trees)\n>                                 continue;\n>                         if (pending->path) {\n> -                               struct type_and_oid_list *list;\n>                                 char *path = *pending->path ? xstrfmt(\"%s/\", pending->path)\n>                                                             : xstrdup(\"\");\n> -                               if (!(list = strmap_get(&ctx->paths_to_lists, path))) {\n> -                                       CALLOC_ARRAY(list, 1);\n> -                                       list->type = OBJ_TREE;\n> -                                       strmap_put(&ctx->paths_to_lists, path, list);\n> -                               }\n> -                               list->maybe_interesting = 1;\n> -                               oid_array_append(&list->oids, &obj->oid);\n> +                               add_path_to_list(ctx, path, OBJ_TREE, &obj->oid, 1);\n>                                 free(path);\n>                         } else {\n>                                 /* assume a root tree, such as a lightweight tag. */\n> @@ -397,20 +400,10 @@ static int setup_pending_objects(struct path_walk_info *info,\n>                 case OBJ_BLOB:\n>                         if (!info->blobs)\n>                                 continue;\n> -                       if (pending->path) {\n> -                               struct type_and_oid_list *list;\n> -                               char *path = pending->path;\n> -                               if (!(list = strmap_get(&ctx->paths_to_lists, path))) {\n> -                                       CALLOC_ARRAY(list, 1);\n> -                                       list->type = OBJ_BLOB;\n> -                                       strmap_put(&ctx->paths_to_lists, path, list);\n> -                               }\n> -                               list->maybe_interesting = 1;\n> -                               oid_array_append(&list->oids, &obj->oid);\n> -                       } else {\n> -                               /* assume a root tree, such as a lightweight tag. */\n> +                       if (pending->path)\n> +                               add_path_to_list(ctx, pending->path, OBJ_BLOB, &obj->oid, 1);\n> +                       else\n>                                 oid_array_append(&tagged_blobs->oids, &obj->oid);\n> -                       }\n>                         break;\n>\n>                 case OBJ_COMMIT:\n> --\n> gitgitgadget\n\nLooks like a straightforward collapsing of common code into a new function.\n"},{"id":"524860","messageId":"pull.1956.v2.git.1756126197.gitgitgadget@gmail.com","threadId":"63998","inReplyTo":"pull.1956.git.1755715196.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] [2.51.0 Bug] Missing singleton objects in 'git repack -adf --path-walk'","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-25T12:49:55Z","receivedAt":"2025-08-25T12:50:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Now that the --path-walk feature for git repack is out in the wild and\ngetting more visibility than it did in the Git for Windows fork, the\nfollowing issue was brought to my attention:\n\nSome folks would report missing objects after git repack -adf --path-walk!\n\nIt turns out that this snuck through the cracks because it was pretty\ndifficult to create a reproducing test case (patch 1) but it boils down to:\n\n 1. A path has exactly one version across all of the history being repacked.\n 2. That path is in the index.\n 3. The object at that path is not a loose object.\n 4. pack.useSparse=true in the config (this is the default)\n\nIt is also something where users don't necessarily notice the missing\nobjects until they fetch and a missing object is used as a delta base. Doing\nnormal checkouts doesn't cause changes to these files, so they are never\nopened by Git. Users hitting this issue can usually recover using git fetch\n--refetch to repopulate the missing objects from a remote (unless they never\nhad a remote at all).\n\nPatch 1 introduces the fix for this issue, which is related to forgetting to\ninitialize a struct indicator when walking the pending objects.\n\nWhen reflecting on the ways that I missed this when building the feature, I\nthink the core issue was an overreliance on using bare repos in testing. I\nalso think that the way that the UNINTERESTING object exploration was\nimplemented was particularly fragile to missing updates to the\ninitialization of the struct, so patch 2 adds a new initializer to reduce\nduplicate code and to help avoid this mistake in the future.\n\n\nUpdates in v2\n=============\n\nThank you for the quick and careful review of these patches.\n\n * The test and bug fix are now in the same patch.\n * Several commit message typos/grammar edits.\n\nThanks, -Stolee\n\nP.S. CC'ing all original reviewers of the series.\n\nDerrick Stolee (2):\n  path-walk: fix setup of pending objects\n  path-walk: create initializer for path lists\n\n path-walk.c       | 55 +++++++++++++++++++----------------------\n t/t7700-repack.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 88 insertions(+), 30 deletions(-)\n\n\nbase-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1956%2Fderrickstolee%2Fpath-walk-missing-objects-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1956/derrickstolee/path-walk-missing-objects-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1956\n\nRange-diff vs v1:\n\n 1:  5b19173c03d ! 1:  2feedf5a1ee t7700: add failing --path-walk test\n     @@ Metadata\n      Author: Derrick Stolee <dstolee@microsoft.com>\n      \n       ## Commit message ##\n     -    t7700: add failing --path-walk test\n     +    path-walk: fix setup of pending objects\n      \n          Users reported an issue where objects were missing from their local\n     -    enlistments after a full repack using 'git repack -adf --path-walk'.\n     -    This was alarming, but took a while to create a reproducer.\n     +    repositories after a full repack using 'git repack -adf --path-walk'.\n     +    This was alarming and took a while to create a reproducer. Here, we fix\n     +    the bug and include a test case that would fail without this fix.\n      \n          The root cause is that certain objects existed in the index and had no\n          second versions. These objects are usually blobs, though trees can be\n     @@ Commit message\n          issue for reproducibility:\n      \n           1. File a/b/c has only one committed version.\n     -     2. Files a/i and x/y only exists as staged changes.\n     +     2. Files a/i and x/y only exist as staged changes.\n           3. Tree x/ only exists in the cache-tree.\n      \n          After performing a non-path-walk repack to force all loose objects into\n     @@ Commit message\n          Note that since the staged root tree is missing, the fsck output cannot\n          even report that the staged x/ tree is missing as well.\n      \n     -    This bug will be fixed in the next change.\n     +    The core problem here is that the \"maybe_interesting\" member of 'struct\n     +    type_and_oid_list' is not initialized to '1'. This member was added in\n     +    6333e7ae0b (path-walk: mark trees and blobs as UNINTERESTING,\n     +    2024-12-20) in a way to help when creating packfiles for a small commit\n     +    range using the sparse path algorithm (enabled by pack.useSparse=true).\n     +\n     +    The idea here is that the list is marked as \"maybe_interesting\" if an\n     +    object is added that does not have the UNINTERESTING flag on it. Later,\n     +    this is checked again in case all objects in the list were marked\n     +    UNINTERESTING after that point in time. In this case, the algorithm\n     +    skips the list as there is no reason to visit it.\n     +\n     +    This leads to the problem where the \"maybe_interesting\" member was not\n     +    appropriately initialized when the list is created from pending objects.\n     +    Initializing this in the correct places fixes the bug.\n     +\n     +    To reduce risk of similar bugs around initializing this structure, a\n     +    follow-up change will make initializing lists use a shared method.\n      \n          Signed-off-by: Derrick Stolee <stolee@gmail.com>\n      \n     + ## path-walk.c ##\n     +@@ path-walk.c: static int setup_pending_objects(struct path_walk_info *info,\n     + \t\t\t\t\tlist->type = OBJ_TREE;\n     + \t\t\t\t\tstrmap_put(&ctx->paths_to_lists, path, list);\n     + \t\t\t\t}\n     ++\t\t\t\tlist->maybe_interesting = 1;\n     + \t\t\t\toid_array_append(&list->oids, &obj->oid);\n     + \t\t\t\tfree(path);\n     + \t\t\t} else {\n     +@@ path-walk.c: static int setup_pending_objects(struct path_walk_info *info,\n     + \t\t\t\t\tlist->type = OBJ_BLOB;\n     + \t\t\t\t\tstrmap_put(&ctx->paths_to_lists, path, list);\n     + \t\t\t\t}\n     ++\t\t\t\tlist->maybe_interesting = 1;\n     + \t\t\t\toid_array_append(&list->oids, &obj->oid);\n     + \t\t\t} else {\n     + \t\t\t\t/* assume a root tree, such as a lightweight tag. */\n     +\n       ## t/t7700-repack.sh ##\n      @@ t/t7700-repack.sh: test_expect_success '-n overrides repack.updateServerInfo=true' '\n       \ttest_server_info_missing\n       '\n       \n     -+test_expect_failure 'pending objects are repacked appropriately' '\n     ++test_expect_success 'pending objects are repacked appropriately' '\n     ++\ttest_when_finished rm -rf pending &&\n      +\tgit init pending &&\n      +\n      +\t(\n      +\t\tcd pending &&\n      +\n     ++\t\t# Commit file, a/b/c and never change them.\n      +\t\tmkdir -p a/b &&\n      +\t\techo singleton >file &&\n      +\t\techo stuff >a/b/c &&\n     @@ t/t7700-repack.sh: test_expect_success '-n overrides repack.updateServerInfo=tru\n      +\t\tgit add file a &&\n      +\t\tgit commit -m \"single blobs\" &&\n      +\n     ++\t\t# Files a/d and a/e will not be singletons.\n      +\t\techo d >a/d &&\n      +\t\techo e >a/e &&\n      +\t\tgit add a &&\n     @@ t/t7700-repack.sh: test_expect_success '-n overrides repack.updateServerInfo=tru\n      +\t\t# test that the cache-tree is walked, too.\n      +\t\tgit sparse-checkout set --sparse-index a x &&\n      +\n     -+\t\t# Just _stage_ the changes.\n     ++\t\t# Create staged changes:\n     ++\t\t# * a/e now has multiple versions.\n     ++\t\t# * a/i now has only one version.\n      +\t\techo f >a/d &&\n      +\t\techo h >a/e &&\n      +\t\techo i >a/i &&\n     ++\t\tgit add a &&\n     ++\n     ++\t\t# Stage and unstage a change to make use of\n     ++\t\t# resolve-undo cache and how that impacts fsck.\n      +\t\tmkdir x &&\n      +\t\techo y >x/y &&\n     -+\t\tgit add a x &&\n     ++\t\tgit add x &&\n     ++\t\txy=$(git rev-parse :x/y) &&\n     ++\t\tgit rm --cached x/y &&\n     ++\n     ++\t\t# The blob for x/y must persist through repacks,\n     ++\t\t# but fsck currently ignores the REUC extension\n     ++\t\t# for finding links to the blob.\n     ++\t\tcat >expect <<-EOF &&\n     ++\t\tdangling blob $xy\n     ++\t\tEOF\n      +\n      +\t\t# Bring the loose objects into a packfile to avoid\n      +\t\t# leftovers in next test. Without this, the loose\n      +\t\t# objects persist and the test succeeds for other\n      +\t\t# reasons.\n      +\t\tgit repack -adf &&\n     -+\t\tgit fsck &&\n     ++\t\tgit fsck >out &&\n     ++\t\ttest_cmp expect out &&\n      +\n      +\t\t# Test path walk version with pack.useSparse.\n      +\t\tgit -c pack.useSparse=true repack -adf --path-walk &&\n     -+\t\tgit fsck\n     ++\t\tgit fsck >out &&\n     ++\t\ttest_cmp expect out\n      +\t)\n      +'\n      +\n 2:  0dc4a6323e6 < -:  ----------- path-walk: fix setup of pending objects\n 3:  dd458e04354 = 2:  fc2c171f52d path-walk: create initializer for path lists\n\n-- \ngitgitgadget\n"},{"id":"524859","messageId":"2feedf5a1ee3cf7d7d4e06c088a2e43ccb841dcc.1756126197.git.gitgitgadget@gmail.com","threadId":"63998","inReplyTo":"pull.1956.v2.git.1756126197.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] path-walk: fix setup of pending objects","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-25T12:49:56Z","receivedAt":"2025-08-25T12:50:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <stolee@gmail.com>\n\nUsers reported an issue where objects were missing from their local\nrepositories after a full repack using 'git repack -adf --path-walk'.\nThis was alarming and took a while to create a reproducer. Here, we fix\nthe bug and include a test case that would fail without this fix.\n\nThe root cause is that certain objects existed in the index and had no\nsecond versions. These objects are usually blobs, though trees can be\nincluded if a cache-tree exists. The issue is that the revision walk\nadds these objects to the \"pending\" list and the path-walk API forgets\nto mark the lists it creates at this point as \"maybe_interesting\". If\nthese paths only ever have a single version in the history of the repo\n(including the current staged version) then the parent directory never\ntries to add a new object to the list and mark the list as\n\"maybe_interesting\". Thus, when walking the list later, the group is\nskipped as it is expected that no objects are interesting. This happens\neven when there are actually no UNINTERESTING objects at all! This is\nbased on the optimization enabled by the pack.useSparse=true config\noption, which is the default.\n\nThus, we create a test case that demonstrates the many cases of this\nissue for reproducibility:\n\n 1. File a/b/c has only one committed version.\n 2. Files a/i and x/y only exist as staged changes.\n 3. Tree x/ only exists in the cache-tree.\n\nAfter performing a non-path-walk repack to force all loose objects into\npackfiles, run a --path-walk repack followed by 'git fsck'. This fsck is\nwhat fails with the following errors:\n\n  error: invalid object 100644 f2e41136... for 'a/b/c'\n\n    This is the dropped instance of the single-versioned a/b/c file.\n\n  broken link from    tree cfda31d8...\n                to    tree 3f725fcd...\n\n    This is the missing tree for the single-versioned a/b/ directory.\n\n  missing blob 0ddf2bae... (a/i)\n  missing blob 975fbec8... (x/y)\n  missing blob a60d869d... (file)\n  missing blob f2e41136... (a/b/c)\n\n  missing tree 3f725fcd... (a/b/)\n\n  dangling tree 5896d7e... (staged root tree)\n\nNote that since the staged root tree is missing, the fsck output cannot\neven report that the staged x/ tree is missing as well.\n\nThe core problem here is that the \"maybe_interesting\" member of 'struct\ntype_and_oid_list' is not initialized to '1'. This member was added in\n6333e7ae0b (path-walk: mark trees and blobs as UNINTERESTING,\n2024-12-20) in a way to help when creating packfiles for a small commit\nrange using the sparse path algorithm (enabled by pack.useSparse=true).\n\nThe idea here is that the list is marked as \"maybe_interesting\" if an\nobject is added that does not have the UNINTERESTING flag on it. Later,\nthis is checked again in case all objects in the list were marked\nUNINTERESTING after that point in time. In this case, the algorithm\nskips the list as there is no reason to visit it.\n\nThis leads to the problem where the \"maybe_interesting\" member was not\nappropriately initialized when the list is created from pending objects.\nInitializing this in the correct places fixes the bug.\n\nTo reduce risk of similar bugs around initializing this structure, a\nfollow-up change will make initializing lists use a shared method.\n\nSigned-off-by: Derrick Stolee <stolee@gmail.com>\n---\n path-walk.c       |  2 ++\n t/t7700-repack.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 65 insertions(+)\n\ndiff --git a/path-walk.c b/path-walk.c\nindex 2d4ddbadd50f..1215ed398f4f 100644\n--- a/path-walk.c\n+++ b/path-walk.c\n@@ -385,6 +385,7 @@ static int setup_pending_objects(struct path_walk_info *info,\n \t\t\t\t\tlist->type = OBJ_TREE;\n \t\t\t\t\tstrmap_put(&ctx->paths_to_lists, path, list);\n \t\t\t\t}\n+\t\t\t\tlist->maybe_interesting = 1;\n \t\t\t\toid_array_append(&list->oids, &obj->oid);\n \t\t\t\tfree(path);\n \t\t\t} else {\n@@ -404,6 +405,7 @@ static int setup_pending_objects(struct path_walk_info *info,\n \t\t\t\t\tlist->type = OBJ_BLOB;\n \t\t\t\t\tstrmap_put(&ctx->paths_to_lists, path, list);\n \t\t\t\t}\n+\t\t\t\tlist->maybe_interesting = 1;\n \t\t\t\toid_array_append(&list->oids, &obj->oid);\n \t\t\t} else {\n \t\t\t\t/* assume a root tree, such as a lightweight tag. */\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 611755cc139b..73b78bdd887d 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -838,4 +838,67 @@ test_expect_success '-n overrides repack.updateServerInfo=true' '\n \ttest_server_info_missing\n '\n \n+test_expect_success 'pending objects are repacked appropriately' '\n+\ttest_when_finished rm -rf pending &&\n+\tgit init pending &&\n+\n+\t(\n+\t\tcd pending &&\n+\n+\t\t# Commit file, a/b/c and never change them.\n+\t\tmkdir -p a/b &&\n+\t\techo singleton >file &&\n+\t\techo stuff >a/b/c &&\n+\t\techo more >a/d &&\n+\t\tgit add file a &&\n+\t\tgit commit -m \"single blobs\" &&\n+\n+\t\t# Files a/d and a/e will not be singletons.\n+\t\techo d >a/d &&\n+\t\techo e >a/e &&\n+\t\tgit add a &&\n+\t\tgit commit -m \"more blobs\" &&\n+\n+\t\t# This use of a sparse index helps to force\n+\t\t# test that the cache-tree is walked, too.\n+\t\tgit sparse-checkout set --sparse-index a x &&\n+\n+\t\t# Create staged changes:\n+\t\t# * a/e now has multiple versions.\n+\t\t# * a/i now has only one version.\n+\t\techo f >a/d &&\n+\t\techo h >a/e &&\n+\t\techo i >a/i &&\n+\t\tgit add a &&\n+\n+\t\t# Stage and unstage a change to make use of\n+\t\t# resolve-undo cache and how that impacts fsck.\n+\t\tmkdir x &&\n+\t\techo y >x/y &&\n+\t\tgit add x &&\n+\t\txy=$(git rev-parse :x/y) &&\n+\t\tgit rm --cached x/y &&\n+\n+\t\t# The blob for x/y must persist through repacks,\n+\t\t# but fsck currently ignores the REUC extension\n+\t\t# for finding links to the blob.\n+\t\tcat >expect <<-EOF &&\n+\t\tdangling blob $xy\n+\t\tEOF\n+\n+\t\t# Bring the loose objects into a packfile to avoid\n+\t\t# leftovers in next test. Without this, the loose\n+\t\t# objects persist and the test succeeds for other\n+\t\t# reasons.\n+\t\tgit repack -adf &&\n+\t\tgit fsck >out &&\n+\t\ttest_cmp expect out &&\n+\n+\t\t# Test path walk version with pack.useSparse.\n+\t\tgit -c pack.useSparse=true repack -adf --path-walk &&\n+\t\tgit fsck >out &&\n+\t\ttest_cmp expect out\n+\t)\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"524861","messageId":"fc2c171f52d94022709bc86110711a3a9ae10a6a.1756126198.git.gitgitgadget@gmail.com","threadId":"63998","inReplyTo":"pull.1956.v2.git.1756126197.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] path-walk: create initializer for path lists","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-25T12:49:57Z","receivedAt":"2025-08-25T12:50:03Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <stolee@gmail.com>\n\nThe previous change fixed a bug in 'git repack -adf --path-walk' that\nwas due to an update to how path lists are initialized and missing some\nimportant cases when processing the pending objects.\n\nThis change takes the three critical places where path lists are\ninitialized and combines them into a static method. This simplifies the\ncallers somewhat while also helping to avoid a missed update in the\nfuture.\n\nThe other places where a path list (struct type_and_oid_list) is\ninitialized is for the following \"fixed\" lists:\n\n * Tag objects.\n * Commit objects.\n * Root trees.\n * Tagged trees.\n * Tagged blobs.\n\nThese lists are created and consumed in different ways, with only the\nroot trees being passed into the logic that cares about the\n\"maybe_interesting\" bit. It is appropriate to keep these uses separate.\n\nSigned-off-by: Derrick Stolee <stolee@gmail.com>\n---\n path-walk.c | 57 +++++++++++++++++++++++------------------------------\n 1 file changed, 25 insertions(+), 32 deletions(-)\n\ndiff --git a/path-walk.c b/path-walk.c\nindex 1215ed398f4f..f1ceed99e94c 100644\n--- a/path-walk.c\n+++ b/path-walk.c\n@@ -105,6 +105,24 @@ static void push_to_stack(struct path_walk_context *ctx,\n \tprio_queue_put(&ctx->path_stack, xstrdup(path));\n }\n \n+static void add_path_to_list(struct path_walk_context *ctx,\n+\t\t\t     const char *path,\n+\t\t\t     enum object_type type,\n+\t\t\t     struct object_id *oid,\n+\t\t\t     int interesting)\n+{\n+\tstruct type_and_oid_list *list = strmap_get(&ctx->paths_to_lists, path);\n+\n+\tif (!list) {\n+\t\tCALLOC_ARRAY(list, 1);\n+\t\tlist->type = type;\n+\t\tstrmap_put(&ctx->paths_to_lists, path, list);\n+\t}\n+\n+\tlist->maybe_interesting |= interesting;\n+\toid_array_append(&list->oids, oid);\n+}\n+\n static int add_tree_entries(struct path_walk_context *ctx,\n \t\t\t    const char *base_path,\n \t\t\t    struct object_id *oid)\n@@ -129,7 +147,6 @@ static int add_tree_entries(struct path_walk_context *ctx,\n \n \tinit_tree_desc(&desc, &tree->object.oid, tree->buffer, tree->size);\n \twhile (tree_entry(&desc, &entry)) {\n-\t\tstruct type_and_oid_list *list;\n \t\tstruct object *o;\n \t\t/* Not actually true, but we will ignore submodules later. */\n \t\tenum object_type type = S_ISDIR(entry.mode) ? OBJ_TREE : OBJ_BLOB;\n@@ -190,17 +207,10 @@ static int add_tree_entries(struct path_walk_context *ctx,\n \t\t\t\tcontinue;\n \t\t}\n \n-\t\tif (!(list = strmap_get(&ctx->paths_to_lists, path.buf))) {\n-\t\t\tCALLOC_ARRAY(list, 1);\n-\t\t\tlist->type = type;\n-\t\t\tstrmap_put(&ctx->paths_to_lists, path.buf, list);\n-\t\t}\n-\t\tpush_to_stack(ctx, path.buf);\n-\n-\t\tif (!(o->flags & UNINTERESTING))\n-\t\t\tlist->maybe_interesting = 1;\n+\t\tadd_path_to_list(ctx, path.buf, type, &entry.oid,\n+\t\t\t\t !(o->flags & UNINTERESTING));\n \n-\t\toid_array_append(&list->oids, &entry.oid);\n+\t\tpush_to_stack(ctx, path.buf);\n \t}\n \n \tfree_tree_buffer(tree);\n@@ -377,16 +387,9 @@ static int setup_pending_objects(struct path_walk_info *info,\n \t\t\tif (!info->trees)\n \t\t\t\tcontinue;\n \t\t\tif (pending->path) {\n-\t\t\t\tstruct type_and_oid_list *list;\n \t\t\t\tchar *path = *pending->path ? xstrfmt(\"%s/\", pending->path)\n \t\t\t\t\t\t\t    : xstrdup(\"\");\n-\t\t\t\tif (!(list = strmap_get(&ctx->paths_to_lists, path))) {\n-\t\t\t\t\tCALLOC_ARRAY(list, 1);\n-\t\t\t\t\tlist->type = OBJ_TREE;\n-\t\t\t\t\tstrmap_put(&ctx->paths_to_lists, path, list);\n-\t\t\t\t}\n-\t\t\t\tlist->maybe_interesting = 1;\n-\t\t\t\toid_array_append(&list->oids, &obj->oid);\n+\t\t\t\tadd_path_to_list(ctx, path, OBJ_TREE, &obj->oid, 1);\n \t\t\t\tfree(path);\n \t\t\t} else {\n \t\t\t\t/* assume a root tree, such as a lightweight tag. */\n@@ -397,20 +400,10 @@ static int setup_pending_objects(struct path_walk_info *info,\n \t\tcase OBJ_BLOB:\n \t\t\tif (!info->blobs)\n \t\t\t\tcontinue;\n-\t\t\tif (pending->path) {\n-\t\t\t\tstruct type_and_oid_list *list;\n-\t\t\t\tchar *path = pending->path;\n-\t\t\t\tif (!(list = strmap_get(&ctx->paths_to_lists, path))) {\n-\t\t\t\t\tCALLOC_ARRAY(list, 1);\n-\t\t\t\t\tlist->type = OBJ_BLOB;\n-\t\t\t\t\tstrmap_put(&ctx->paths_to_lists, path, list);\n-\t\t\t\t}\n-\t\t\t\tlist->maybe_interesting = 1;\n-\t\t\t\toid_array_append(&list->oids, &obj->oid);\n-\t\t\t} else {\n-\t\t\t\t/* assume a root tree, such as a lightweight tag. */\n+\t\t\tif (pending->path)\n+\t\t\t\tadd_path_to_list(ctx, pending->path, OBJ_BLOB, &obj->oid, 1);\n+\t\t\telse\n \t\t\t\toid_array_append(&tagged_blobs->oids, &obj->oid);\n-\t\t\t}\n \t\t\tbreak;\n \n \t\tcase OBJ_COMMIT:\n-- \ngitgitgadget\n"},{"id":"524960","messageId":"CABPp-BHFxxGrqKc0m==TjQNjDGdO=H5Rf6EFsf2nfE1=TuraOQ@mail.gmail.com","threadId":"63998","inReplyTo":"pull.1956.v2.git.1756126197.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] [2.51.0 Bug] Missing singleton objects in 'git repack -adf --path-walk'","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-08-26T15:03:56Z","receivedAt":"2025-08-26T15:04:08Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Aug 25, 2025 at 5:50 AM Derrick Stolee via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> Updates in v2\n> =============\n>\n> Thank you for the quick and careful review of these patches.\n>\n>  * The test and bug fix are now in the same patch.\n>  * Several commit message typos/grammar edits.\n\nThanks, this round addresses all of my feedback and looks good to me.\n"},{"id":"524970","messageId":"xmqqh5xukrcb.fsf@gitster.g","threadId":"63998","inReplyTo":"CABPp-BHFxxGrqKc0m==TjQNjDGdO=H5Rf6EFsf2nfE1=TuraOQ@mail.gmail.com","subject":"Re: [PATCH v2 0/2] [2.51.0 Bug] Missing singleton objects in 'git repack -adf --path-walk'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-26T15:58:12Z","receivedAt":"2025-08-26T15:58:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> On Mon, Aug 25, 2025 at 5:50 AM Derrick Stolee via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n>>\n>> Updates in v2\n>> =============\n>>\n>> Thank you for the quick and careful review of these patches.\n>>\n>>  * The test and bug fix are now in the same patch.\n>>  * Several commit message typos/grammar edits.\n>\n> Thanks, this round addresses all of my feedback and looks good to me.\n\nLet's mark it for 'next', then.\n\nThanks, both.\n\n"},{"id":"525333","messageId":"aLbSxDenGqOGgEqD@pks.im","threadId":"63998","inReplyTo":"xmqqh5xukrcb.fsf@gitster.g","subject":"Re: [PATCH v2 0/2] [2.51.0 Bug] Missing singleton objects in 'git repack -adf --path-walk'","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-02T11:19:32Z","receivedAt":"2025-09-02T11:19:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Aug 26, 2025 at 08:58:12AM -0700, Junio C Hamano wrote:\n> Elijah Newren <newren@gmail.com> writes:\n> \n> > On Mon, Aug 25, 2025 at 5:50 AM Derrick Stolee via GitGitGadget\n> > <gitgitgadget@gmail.com> wrote:\n> >>\n> >> Updates in v2\n> >> =============\n> >>\n> >> Thank you for the quick and careful review of these patches.\n> >>\n> >>  * The test and bug fix are now in the same patch.\n> >>  * Several commit message typos/grammar edits.\n> >\n> > Thanks, this round addresses all of my feedback and looks good to me.\n> \n> Let's mark it for 'next', then.\n\nAgreed, this version looks good to me and addresses my feedback. But the\nseries has been merged to 'next' already anyway :)\n\nPatrick\n"}]}