[2.51.0 Bug] Missing singleton objects in 'git repack -adf --path-walk' (#796)

This is a port of gitgitgadget#1956, and I
deem it one of the blockers against making a full release of Microsoft
Git v2.51. Here is what Stolee wrote in the cover letter:

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.

Author: dscho

path-walk.c

@@ -105,6 +105,24 @@ static void push_to_stack(struct path_walk_context *ctx,
 	prio_queue_put(&ctx->path_stack, xstrdup(path));
 }
 
+static void add_path_to_list(struct path_walk_context *ctx,
+			     const char *path,
+			     enum object_type type,
+			     struct object_id *oid,
+			     int interesting)
+{
+	struct type_and_oid_list *list = strmap_get(&ctx->paths_to_lists, path);
+
+	if (!list) {
+		CALLOC_ARRAY(list, 1);
+		list->type = type;
+		strmap_put(&ctx->paths_to_lists, path, list);
+	}
+
+	list->maybe_interesting |= interesting;
+	oid_array_append(&list->oids, oid);
+}
+
 static int add_tree_entries(struct path_walk_context *ctx,
 			    const char *base_path,
 			    struct object_id *oid)
@@ -129,7 +147,6 @@ static int add_tree_entries(struct path_walk_context *ctx,
 
 	init_tree_desc(&desc, &tree->object.oid, tree->buffer, tree->size);
 	while (tree_entry(&desc, &entry)) {
-		struct type_and_oid_list *list;
 		struct object *o;
 		/* Not actually true, but we will ignore submodules later. */
 		enum object_type type = S_ISDIR(entry.mode) ? OBJ_TREE : OBJ_BLOB;
@@ -190,17 +207,10 @@ static int add_tree_entries(struct path_walk_context *ctx,
 				continue;
 		}
 
-		if (!(list = strmap_get(&ctx->paths_to_lists, path.buf))) {
-			CALLOC_ARRAY(list, 1);
-			list->type = type;
-			strmap_put(&ctx->paths_to_lists, path.buf, list);
-		}
-		push_to_stack(ctx, path.buf);
-
-		if (!(o->flags & UNINTERESTING))
-			list->maybe_interesting = 1;
+		add_path_to_list(ctx, path.buf, type, &entry.oid,
+				 !(o->flags & UNINTERESTING));
 
-		oid_array_append(&list->oids, &entry.oid);
+		push_to_stack(ctx, path.buf);
 	}
 
 	free_tree_buffer(tree);
@@ -377,15 +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);
-				}
-				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. */
@@ -396,19 +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);
-				}
-				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:

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