Missing singleton objects in 'git repack -adf --path-walk'

Now that the --path-walk feature for git repack is out in the wild and getting more visibility than it did in the Git for Windows fork, the following issue was brought to my attention:

Some folks would report missing objects after git repack -adf --path-walk!

It turns out that this snuck through the cracks because it was pretty difficult to create a reproducing test case (patch 1) but it boils down to:

1. A path has exactly one version across all of the history being repacked.
2. That path is in the index.
3. The object at that path is not a loose object.
4. pack.useSparse=true in the config (this is the default)

It is also something where users don't necessarily notice the missing objects until they fetch and a missing object is used as a delta base. Doing normal checkouts doesn't cause changes to these files, so they are never opened by Git. Users hitting this issue can usually recover using git fetch --refetch to repopulate the missing objects from a remote (unless they never had a remote at all).

Patch 2 introduces the fix for this issue, which is related to forgetting to initialize a struct indicator when walking the pending objects.

When reflecting on the ways that I missed this when building the feature, I think the core issue was an overreliance on using bare repos in testing. I also think that the way that the UNINTERESTING object exploration was implemented was particularly fragile to missing updates to the initialization of the struct, so patch 3 adds a new initializer to reduce duplicate code and to help avoid this mistake in the future.

Thanks, -Stolee

P.S. CC'ing all original reviewers of the series.

Derrick Stolee (3):
t7700: add failing --path-walk test
path-walk: fix setup of pending objects
path-walk: create initializer for path lists

 path-walk.c       | 55 +++++++++++++++++++++--------------------------
 t/t7700-repack.sh | 43 ++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+), 30 deletions(-)

base-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1956%2Fderrickstolee%2Fpath-walk-missing-objects-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1956/derrickstolee/path-walk-missing-objects-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1956

gitgitgadget

From: Derrick Stolee <[email protected]>

Users reported an issue where objects were missing from their local enlistments after a full repack using 'git repack -adf --path-walk'. This was alarming, but took a while to create a reproducer.

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 exists 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.

This bug will be fixed in the next change.

---
 t/t7700-repack.sh | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

index 611755cc139b..1998d9bf291c 100755
@@ -838,4 +838,47 @@ test_expect_success '-n overrides repack.updateServerInfo=true' '
 	test_server_info_missing
 '
 
+test_expect_failure 'pending objects are repacked appropriately' '
+	git init pending &&
+
+	(
+		cd pending &&
+
+		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" &&
+
+		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 &&
+
+		# Just _stage_ the changes.
+		echo f >a/d &&
+		echo h >a/e &&
+		echo i >a/i &&
+		mkdir x &&
+		echo y >x/y &&
+		git add a x &&
+
+		# 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 &&
+
+		# Test path walk version with pack.useSparse.
+		git -c pack.useSparse=true repack -adf --path-walk &&
+		git fsck
+	)
+'
+
 test_done

gitgitgadget

I'm probably the person who is always asking folks to create a test that either fails or demonstrates the "before" behavior before making the actual change that updates the case. This allows the ability to "test the test" by checking it in place to confirm that it is indeed failing.

Using test_expect_failure allows us to avoid breaking bisect.

Good idea.

Hm. That seems to go against our typical pattern of leaving comments sparse and having the longer explanation available in commit messages but maybe I'm out of date or tests are a different beast. I'll see what I can do to make the test more self-documented.

Thanks,
-Stolee

Yes, you can develop that way, but on the reviewing and receiving end, the second patch that shows only the change from expect_failure to expect_success pushes the more important "what behaviour was this thing testing?" out of the hunk context, if you split them into two.

If we really wanted to verify the claim that without the fix this was broken and we have a test to demonstrate a failure on the receiving end, we can "checkout" paths touched by that commit outside t/ from HEAD^ to build to see how the system behaved without the code change just fine, so such a split does not buy us much.

Unless there is a strong reason not to, please always present such test in the same patch as the code change to fix that breakage.

Thanks.

What is an enlistment?

but => and ?

exists => exist

I didn't have any questions or spot any issues on the rest of the patch.

From: Derrick Stolee <[email protected]>

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.

To help avoid this from happening in the future, a follow-up change will make initializing lists use a shared method instead of allowing for an update to this initialization process to miss some existing copies.

---
 path-walk.c       | 2 ++
 t/t7700-repack.sh | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

index 2d4ddbadd50f..1215ed398f4f 100644
@@ -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. */

index 1998d9bf291c..030e9e5b2dc7 100755
@@ -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

OK, in other words, the bug is fairly contained within the path-walk traversal. We treat things as reachable not just from ref tips and reflogs (where path-walk code can use the tree object to compute on what pathname each blob comes from) and the main index array (that has paths, even though it needs separate way to compute than those for trees), but also from places like REUC and TREE extensions that make associations between pathnames and objects. Are they also OK?

Great. Future-proofing is 100 times better than just a bugfix.

The key integration point is the "pending" list operating a bit different from walking directly from tags or commits. I was trying to reproduce the issue from all of those other sources before unlocking the "singleton" nature of the problem, and failed to do so.

The resolve-undo cache (REUC) is something that I had not tested previously. Adding "git rm --cached x/y" to the test in the previous case leads to the 'git fsck' call giving a "dangling blob" warning, so that could be an interesting way to strengthen the test. Thanks, -Stolee

I also wonder a bit about the future -- if we ever add a new source for pending objects, would the author have to amend "path-walk.c" to take this new pending source into account?

I guess the answer is "yes", which does make me feel a bit uneasy as it is very easy to now corrupt the repository.

Patrick

I don't think the risk comes from new ways to add pending objects, but that the path-walk algorithm added an "optimization" without appropriately modifying how it handled the pending objects. New sources of pending objects shouldn't cause any issues, unless the revision API itself changed substantially.

For example, in my draft for v2 I add changes to my test to account for the REUC in the repack. The new logic for the path-walk feature worked just fine. It does point out that 'git fsck' doesn't follow links in the REUC and so reports the blob as dangling.

If we have a new source for pending objects in the context of pack-objects or repack, one would hope that tests would be added to demonstrate the behavior and then that could be double-checked with GIT_TEST_PACK_PATH_WALK=1. One problem in this situation is that the tests were not substantial enough for these sources. We are correcting for that now.

Perhaps this has always been very easy to make a bug that leads to losing objects, but we're purposefully wary to touch the logic around the repacking processes.

One feature that could provide increased confidence is some step that double-checks the object list before and after a step like this to make sure that no objects are dropped (or only the set we expect is dropped). Something like comparing the object lists in the pack-indexes before and after a repack. This might be too problematic to enable in all cases, but could be enabled in the test suite and certain critical places. Azure DevOps has something similar in its backend to prevent any change to the Git object database that can't be undone, but these steps happen asynchronously to "production data" so it may not be appropriate for all server architectures.

Thanks,
-Stolee

s/UNINITERSTING/UNINTERESTING/

Yeah, I wanted to say that this feels quite fragile to me and very easy to miss. Does this mechanism buy us a lot of performance in the first place? Because if not we might as well just remove it entirely.

But if the answer is "yes" then adding APIs around it feels like a good alternative.

Patrick

Thanks!

The details for the space savings and moderate time cost are listed in e5394794a5 (pack-objects: thread the path-based compression, 2025-05-16). These improvements are on top of those from --name-hash-version=2 by using a different way to focus delta calculations.

A larger, internal example for this can be seen in this table (based on testing today):

Mode | Size | Time
----------+----------+------

version 1 |  16.0 GB | 83min
version 2 |   9.9 GB | 77min
path walk |   6.4 GB | 74min

Making the code less brittle to changes is good, but also I'm interested in ways to improve our test infrastructure or adding defensive features. I mentioned a couple of ideas in an earlier message.

Thanks,
-Stolee

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?

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...

Patch looks good.

From: Derrick Stolee <[email protected]>

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.

---
 path-walk.c | 57 +++++++++++++++++++++++------------------------------
 1 file changed, 25 insertions(+), 32 deletions(-)

index 1215ed398f4f..f1ceed99e94c 100644
@@ -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:

gitgitgadget

Looks like a straightforward collapsing of common code into a new function.

Now that the --path-walk feature for git repack is out in the wild and getting more visibility than it did in the Git for Windows fork, the following issue was brought to my attention:

Some folks would report missing objects after git repack -adf --path-walk!

It turns out that this snuck through the cracks because it was pretty difficult to create a reproducing test case (patch 1) but it boils down to:

1. A path has exactly one version across all of the history being repacked.
2. That path is in the index.
3. The object at that path is not a loose object.
4. pack.useSparse=true in the config (this is the default)

It is also something where users don't necessarily notice the missing objects until they fetch and a missing object is used as a delta base. Doing normal checkouts doesn't cause changes to these files, so they are never opened by Git. Users hitting this issue can usually recover using git fetch --refetch to repopulate the missing objects from a remote (unless they never had a remote at all).

Patch 1 introduces the fix for this issue, which is related to forgetting to initialize a struct indicator when walking the pending objects.

When reflecting on the ways that I missed this when building the feature, I think the core issue was an overreliance on using bare repos in testing. I also think that the way that the UNINTERESTING object exploration was implemented was particularly fragile to missing updates to the initialization of the struct, so patch 2 adds a new initializer to reduce duplicate code and to help avoid this mistake in the future.

Updates in v2
=============

Thank you for the quick and careful review of these patches.

* The test and bug fix are now in the same patch.
* Several commit message typos/grammar edits.

Thanks, -Stolee

P.S. CC'ing all original reviewers of the series.

Derrick Stolee (2):
path-walk: fix setup of pending objects
path-walk: create initializer for path lists

 path-walk.c       | 55 +++++++++++++++++++----------------------
 t/t7700-repack.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 88 insertions(+), 30 deletions(-)

base-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1956%2Fderrickstolee%2Fpath-walk-missing-objects-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1956/derrickstolee/path-walk-missing-objects-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1956

Range-diff vs v1:

1: 5b19173c03d ! 1: 2feedf5a1ee t7700: add failing --path-walk test
@@ Metadata
Author: Derrick Stolee <[email protected]>

## Commit message ##
- t7700: add failing --path-walk test
+ path-walk: fix setup of pending objects

Users reported an issue where objects were missing from their local
- enlistments after a full repack using 'git repack -adf --path-walk'.
- This was alarming, but took a while to create a reproducer.
+ 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
@@ Commit message
issue for reproducibility:

1. File a/b/c has only one committed version.
- 2. Files a/i and x/y only exists as staged changes.
+ 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
@@ Commit message
Note that since the staged root tree is missing, the fsck output cannot even report that the staged x/ tree is missing as well.

- This bug will be fixed in the next change.
+ 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.

+ ## path-walk.c ##
+@@ path-walk.c: 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 {
+@@ path-walk.c: 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. */
+
## t/t7700-repack.sh ##
@@ t/t7700-repack.sh: 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' '
++ 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 &&
@@ t/t7700-repack.sh: test_expect_success '-n overrides repack.updateServerInfo=tru
+ 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 &&
@@ t/t7700-repack.sh: test_expect_success '-n overrides repack.updateServerInfo=tru
+ # test that the cache-tree is walked, too.
+ git sparse-checkout set --sparse-index a x &&
+
-+ # Just _stage_ the changes.
++ # 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 a x &&
++ 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 &&
++ 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
++ git fsck >out &&
++ test_cmp expect out
+ )
+'
+

 2:  0dc4a6323e6 < -:  ----------- path-walk: fix setup of pending objects
 3:  dd458e04354 = 2:  fc2c171f52d path-walk: create initializer for path lists

gitgitgadget

From: Derrick Stolee <[email protected]>

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.

---
 path-walk.c       |  2 ++
 t/t7700-repack.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

index 2d4ddbadd50f..1215ed398f4f 100644
@@ -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. */

gitgitgadget

From: Derrick Stolee <[email protected]>

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.

---
 path-walk.c | 57 +++++++++++++++++++++++------------------------------
 1 file changed, 25 insertions(+), 32 deletions(-)

index 1215ed398f4f..f1ceed99e94c 100644
@@ -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:

gitgitgadget

Thanks, this round addresses all of my feedback and looks good to me.

Let's mark it for 'next', then.

Thanks, both.

Agreed, this version looks good to me and addresses my feedback. But the series has been merged to 'next' already anyway :)

Patrick