From: Derrick Stolee <stolee@xxxxxxxxx>
Users reported an issue where objects were missing from their local
repositories after a full repack using 'git repack -adf --path-walk'.
This was alarming and took a while to create a reproducer. Here, we fix
the bug and include a test case that would fail without this fix.
The root cause is that certain objects existed in the index and had no
second versions. These objects are usually blobs, though trees can be
included if a cache-tree exists. The issue is that the revision walk
adds these objects to the "pending" list and the path-walk API forgets
to mark the lists it creates at this point as "maybe_interesting". If
these paths only ever have a single version in the history of the repo
(including the current staged version) then the parent directory never
tries to add a new object to the list and mark the list as
"maybe_interesting". Thus, when walking the list later, the group is
skipped as it is expected that no objects are interesting. This happens
even when there are actually no UNINTERESTING objects at all! This is
based on the optimization enabled by the pack.useSparse=true config
option, which is the default.
Thus, we create a test case that demonstrates the many cases of this
issue for reproducibility:
1. File a/b/c has only one committed version.
2. Files a/i and x/y only exist as staged changes.
3. Tree x/ only exists in the cache-tree.
After performing a non-path-walk repack to force all loose objects into
packfiles, run a --path-walk repack followed by 'git fsck'. This fsck is
what fails with the following errors:
error: invalid object 100644 f2e41136... for 'a/b/c'
This is the dropped instance of the single-versioned a/b/c file.
broken link from tree cfda31d8...
to tree 3f725fcd...
This is the missing tree for the single-versioned a/b/ directory.
missing blob 0ddf2bae... (a/i)
missing blob 975fbec8... (x/y)
missing blob a60d869d... (file)
missing blob f2e41136... (a/b/c)
missing tree 3f725fcd... (a/b/)
dangling tree 5896d7e... (staged root tree)
Note that since the staged root tree is missing, the fsck output cannot
even report that the staged x/ tree is missing as well.
The core problem here is that the "maybe_interesting" member of 'struct
type_and_oid_list' is not initialized to '1'. This member was added in
6333e7ae0b (path-walk: mark trees and blobs as UNINTERESTING,
2024-12-20) in a way to help when creating packfiles for a small commit
range using the sparse path algorithm (enabled by pack.useSparse=true).
The idea here is that the list is marked as "maybe_interesting" if an
object is added that does not have the UNINTERESTING flag on it. Later,
this is checked again in case all objects in the list were marked
UNINTERESTING after that point in time. In this case, the algorithm
skips the list as there is no reason to visit it.
This leads to the problem where the "maybe_interesting" member was not
appropriately initialized when the list is created from pending objects.
Initializing this in the correct places fixes the bug.
To reduce risk of similar bugs around initializing this structure, a
follow-up change will make initializing lists use a shared method.
Signed-off-by: Derrick Stolee <stolee@xxxxxxxxx>
---
path-walk.c | 2 ++
t/t7700-repack.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 65 insertions(+)
diff --git a/path-walk.c b/path-walk.c
index 2d4ddbadd50f..1215ed398f4f 100644
--- a/path-walk.c
+++ b/path-walk.c
@@ -385,6 +385,7 @@ static int setup_pending_objects(struct path_walk_info *info,
list->type = OBJ_TREE;
strmap_put(&ctx->paths_to_lists, path, list);
}
+ list->maybe_interesting = 1;
oid_array_append(&list->oids, &obj->oid);
free(path);
} else {
@@ -404,6 +405,7 @@ static int setup_pending_objects(struct path_walk_info *info,
list->type = OBJ_BLOB;
strmap_put(&ctx->paths_to_lists, path, list);
}
+ list->maybe_interesting = 1;
oid_array_append(&list->oids, &obj->oid);
} else {
/* assume a root tree, such as a lightweight tag. */
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 611755cc139b..73b78bdd887d 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -838,4 +838,67 @@ test_expect_success '-n overrides repack.updateServerInfo=true' '
test_server_info_missing
'
+test_expect_success 'pending objects are repacked appropriately' '
+ test_when_finished rm -rf pending &&
+ git init pending &&
+
+ (
+ cd pending &&
+
+ # Commit file, a/b/c and never change them.
+ mkdir -p a/b &&
+ echo singleton >file &&
+ echo stuff >a/b/c &&
+ echo more >a/d &&
+ git add file a &&
+ git commit -m "single blobs" &&
+
+ # Files a/d and a/e will not be singletons.
+ echo d >a/d &&
+ echo e >a/e &&
+ git add a &&
+ git commit -m "more blobs" &&
+
+ # This use of a sparse index helps to force
+ # test that the cache-tree is walked, too.
+ git sparse-checkout set --sparse-index a x &&
+
+ # Create staged changes:
+ # * a/e now has multiple versions.
+ # * a/i now has only one version.
+ echo f >a/d &&
+ echo h >a/e &&
+ echo i >a/i &&
+ git add a &&
+
+ # Stage and unstage a change to make use of
+ # resolve-undo cache and how that impacts fsck.
+ mkdir x &&
+ echo y >x/y &&
+ git add x &&
+ xy=$(git rev-parse :x/y) &&
+ git rm --cached x/y &&
+
+ # The blob for x/y must persist through repacks,
+ # but fsck currently ignores the REUC extension
+ # for finding links to the blob.
+ cat >expect <<-EOF &&
+ dangling blob $xy
+ EOF
+
+ # Bring the loose objects into a packfile to avoid
+ # leftovers in next test. Without this, the loose
+ # objects persist and the test succeeds for other
+ # reasons.
+ git repack -adf &&
+ git fsck >out &&
+ test_cmp expect out &&
+
+ # Test path walk version with pack.useSparse.
+ git -c pack.useSparse=true repack -adf --path-walk &&
+ git fsck >out &&
+ test_cmp expect out
+ )
+'
+
test_done
--
gitgitgadget
