[PATCH 0/3] [2.51.0 Bug] Missing singleton objects in 'git repack -adf --path-walk'

[PATCH 0/3] [2.51.0 Bug] Missing singleton objects in 'git repack -adf --path-walk'

[Derrick Stolee via GitGitGadget]

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

[PATCH 1/3] t7700: add failing --path-walk test

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <stolee@gmail.com>

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.

Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
 t/t7700-repack.sh | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 611755cc139b..1998d9bf291c 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -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

Re: [PATCH 1/3] t7700: add failing --path-walk test

[Patrick Steinhardt]

On Wed, Aug 20, 2025 at 06:39:54PM +0000, Derrick Stolee via GitGitGadget wrote:
> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
> index 611755cc139b..1998d9bf291c 100755
> --- a/t/t7700-repack.sh
> +++ b/t/t7700-repack.sh
> @@ -838,4 +838,47 @@ test_expect_success '-n overrides repack.updateServerInfo=true' '

Tiny nit: I would've probably squashed this patch into the second patch,
as we usually don't use the add-failing-test-and-then-fix-it-later
dance. On the other hand though it gives some nice context, so I
ultimately don't mind it all that much. So please feel free to ignore
this nit.

>  	test_server_info_missing
>  '
>  
> +test_expect_failure 'pending objects are repacked appropriately' '
> +	git init pending &&

We probably also want `test_when_finished "rm -rf pending"` before
calling git-init(1).

> +
> +	(
> +		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 &&

Nit: I think I would've moved the explanations you have in the commit
message into these hunks so that the test becomes a bit more
self-explanatory.

> +		# 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
> +	)
> +'

Patrick

Re: [PATCH 1/3] t7700: add failing --path-walk test

[Derrick Stolee]

On 8/21/2025 4:00 AM, Patrick Steinhardt wrote:
> On Wed, Aug 20, 2025 at 06:39:54PM +0000, Derrick Stolee via GitGitGadget wrote:
>> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
>> index 611755cc139b..1998d9bf291c 100755
>> --- a/t/t7700-repack.sh
>> +++ b/t/t7700-repack.sh
>> @@ -838,4 +838,47 @@ test_expect_success '-n overrides repack.updateServerInfo=true' '
> 
> Tiny nit: I would've probably squashed this patch into the second patch,
> as we usually don't use the add-failing-test-and-then-fix-it-later
> dance. On the other hand though it gives some nice context, so I
> ultimately don't mind it all that much. So please feel free to ignore
> this nit.

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. 
>>  	test_server_info_missing
>>  '
>>  
>> +test_expect_failure 'pending objects are repacked appropriately' '
>> +	git init pending &&
> 
> We probably also want `test_when_finished "rm -rf pending"` before
> calling git-init(1).

Good idea.

>> +
>> +	(
>> +		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 &&
> 
> Nit: I think I would've moved the explanations you have in the commit
> message into these hunks so that the test becomes a bit more
> self-explanatory.

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

Re: [PATCH 1/3] t7700: add failing --path-walk test

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

> On 8/21/2025 4:00 AM, Patrick Steinhardt wrote:
>> On Wed, Aug 20, 2025 at 06:39:54PM +0000, Derrick Stolee via GitGitGadget wrote:
>>> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
>>> index 611755cc139b..1998d9bf291c 100755
>>> --- a/t/t7700-repack.sh
>>> +++ b/t/t7700-repack.sh
>>> @@ -838,4 +838,47 @@ test_expect_success '-n overrides repack.updateServerInfo=true' '
>> 
>> Tiny nit: I would've probably squashed this patch into the second patch,
>> as we usually don't use the add-failing-test-and-then-fix-it-later
>> dance. On the other hand though it gives some nice context, so I
>> ultimately don't mind it all that much. So please feel free to ignore
>> this nit.
>
> 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. 

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.

Re: [PATCH 1/3] t7700: add failing --path-walk test

[Elijah Newren]

On Wed, Aug 20, 2025 at 11:39 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <stolee@gmail.com>
>
> Users reported an issue where objects were missing from their local
> enlistments after a full repack using 'git repack -adf --path-walk'.

What is an enlistment?

> This was alarming, but took a while to create a reproducer.

but => and ?

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

exists => exist


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

[PATCH 2/3] path-walk: fix setup of pending objects

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <stolee@gmail.com>

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.

Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
 path-walk.c       | 2 ++
 t/t7700-repack.sh | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/path-walk.c b/path-walk.c
index 2d4ddbadd50f..1215ed398f4f 100644
--- a/path-walk.c
+++ b/path-walk.c
@@ -385,6 +385,7 @@ static int setup_pending_objects(struct path_walk_info *info,
 					list->type = OBJ_TREE;
 					strmap_put(&ctx->paths_to_lists, path, list);
 				}
+				list->maybe_interesting = 1;
 				oid_array_append(&list->oids, &obj->oid);
 				free(path);
 			} else {
@@ -404,6 +405,7 @@ static int setup_pending_objects(struct path_walk_info *info,
 					list->type = OBJ_BLOB;
 					strmap_put(&ctx->paths_to_lists, path, list);
 				}
+				list->maybe_interesting = 1;
 				oid_array_append(&list->oids, &obj->oid);
 			} else {
 				/* assume a root tree, such as a lightweight tag. */
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 1998d9bf291c..030e9e5b2dc7 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -838,7 +838,7 @@ test_expect_success '-n overrides repack.updateServerInfo=true' '
 	test_server_info_missing
 '
 
-test_expect_failure 'pending objects are repacked appropriately' '
+test_expect_success 'pending objects are repacked appropriately' '
 	git init pending &&
 
 	(
-- 
gitgitgadget

Re: [PATCH 2/3] path-walk: fix setup of pending objects

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

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?

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

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

>
> Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---
>  path-walk.c       | 2 ++
>  t/t7700-repack.sh | 2 +-
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/path-walk.c b/path-walk.c
> index 2d4ddbadd50f..1215ed398f4f 100644
> --- a/path-walk.c
> +++ b/path-walk.c
> @@ -385,6 +385,7 @@ static int setup_pending_objects(struct path_walk_info *info,
>  					list->type = OBJ_TREE;
>  					strmap_put(&ctx->paths_to_lists, path, list);
>  				}
> +				list->maybe_interesting = 1;
>  				oid_array_append(&list->oids, &obj->oid);
>  				free(path);
>  			} else {
> @@ -404,6 +405,7 @@ static int setup_pending_objects(struct path_walk_info *info,
>  					list->type = OBJ_BLOB;
>  					strmap_put(&ctx->paths_to_lists, path, list);
>  				}
> +				list->maybe_interesting = 1;
>  				oid_array_append(&list->oids, &obj->oid);
>  			} else {
>  				/* assume a root tree, such as a lightweight tag. */
> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
> index 1998d9bf291c..030e9e5b2dc7 100755
> --- a/t/t7700-repack.sh
> +++ b/t/t7700-repack.sh
> @@ -838,7 +838,7 @@ test_expect_success '-n overrides repack.updateServerInfo=true' '
>  	test_server_info_missing
>  '
>  
> -test_expect_failure 'pending objects are repacked appropriately' '
> +test_expect_success 'pending objects are repacked appropriately' '
>  	git init pending &&
>  
>  	(

Re: [PATCH 2/3] path-walk: fix setup of pending objects

[Derrick Stolee]

On 8/20/2025 3:02 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> 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).
> 
> 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?

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

Re: [PATCH 2/3] path-walk: fix setup of pending objects

[Patrick Steinhardt]

On Wed, Aug 20, 2025 at 03:42:11PM -0400, Derrick Stolee wrote:
> On 8/20/2025 3:02 PM, Junio C Hamano wrote:
> > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> > 
> >> 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).
> > 
> > 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?
> 
> 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,

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

Re: [PATCH 2/3] path-walk: fix setup of pending objects

[Derrick Stolee]

On 8/21/2025 4:01 AM, Patrick Steinhardt wrote:
> On Wed, Aug 20, 2025 at 03:42:11PM -0400, Derrick Stolee wrote:
>> On 8/20/2025 3:02 PM, Junio C Hamano wrote:
>>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

>> 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,
> 
> 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 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. 
> I guess the answer is "yes", which does make me feel a bit uneasy as
> it is very easy to now corrupt the repository.

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

Re: [PATCH 2/3] path-walk: fix setup of pending objects

[Patrick Steinhardt]

On Wed, Aug 20, 2025 at 06:39:55PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
> 
> 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,

s/UNINITERSTING/UNINTERESTING/

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

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

Re: [PATCH 2/3] path-walk: fix setup of pending objects

[Derrick Stolee]

On 8/21/2025 4:01 AM, Patrick Steinhardt wrote:
> On Wed, Aug 20, 2025 at 06:39:55PM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <stolee@gmail.com>
>>
>> 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,
> 
> s/UNINITERSTING/UNINTERESTING/

Thanks!

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

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

> But if the answer is "yes" then adding APIs around it feels like a good
> alternative.
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

Re: [PATCH 2/3] path-walk: fix setup of pending objects

[Elijah Newren]

On Wed, Aug 20, 2025 at 11:40 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <stolee@gmail.com>
>
> The previous change established a buggy instance of 'git repack -adf
> --path-walk' when there exist paths that are tracked in the index and
> that is the only instance of those paths in the history of the
> repository. This change fixes that bug.
>
> The core problem here is that the "maybe_interesting" member of 'struct
> type_and_oid_list' is not initialized to '1'. This member was added in
> 6333e7ae0b (path-walk: mark trees and blobs as UNINTERESTING,
> 2024-12-20) in a way to help when creating packfiles for a small commit
> range using the sparse path algorithm (enabled by pack.useSparse=true).
>
> The idea here is that the list is marked as "maybe_interesting" if an
> object is added that does not have the UNINITERSTING flag on it. Later,
> this is checked again in case all objects in the list were marked
> UNINTERESTING after that point in time. In this case, the algorithm
> skips the list as there is no reason to visit it.
>
> This leads to the problem where the "maybe_interesting" member was not
> appropriately initialized when the list is created from pending objects.
> This is the fix for now.

What is the fix for now?  I think I can deduce it from the above
paragraphs, but then either this sentence is unnecessary or it's just
confusing.  Perhaps change the last sentence to "Initialize it to fix
this problem." or something along those lines?

> To help avoid this from happening in the future, a follow-up change will

I appreciate the future-proofing, which I think you are alluding to,
but to me, "To help avoid this..." suggests your change might not
(fully?) fix the bug you are discussing.  Perhaps you mean "To help
avoid similar problems..." or "A follow-up change will add some
future-proofing to prevent similar problems by..."?  Or maybe I'm just
reading your sentence weird...

> make initializing lists use a shared method instead of allowing for an
> update to this initialization process to miss some existing copies.
>
> Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---
>  path-walk.c       | 2 ++
>  t/t7700-repack.sh | 2 +-
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/path-walk.c b/path-walk.c
> index 2d4ddbadd50f..1215ed398f4f 100644
> --- a/path-walk.c
> +++ b/path-walk.c
> @@ -385,6 +385,7 @@ static int setup_pending_objects(struct path_walk_info *info,
>                                         list->type = OBJ_TREE;
>                                         strmap_put(&ctx->paths_to_lists, path, list);
>                                 }
> +                               list->maybe_interesting = 1;
>                                 oid_array_append(&list->oids, &obj->oid);
>                                 free(path);
>                         } else {
> @@ -404,6 +405,7 @@ static int setup_pending_objects(struct path_walk_info *info,
>                                         list->type = OBJ_BLOB;
>                                         strmap_put(&ctx->paths_to_lists, path, list);
>                                 }
> +                               list->maybe_interesting = 1;
>                                 oid_array_append(&list->oids, &obj->oid);
>                         } else {
>                                 /* assume a root tree, such as a lightweight tag. */
> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
> index 1998d9bf291c..030e9e5b2dc7 100755
> --- a/t/t7700-repack.sh
> +++ b/t/t7700-repack.sh
> @@ -838,7 +838,7 @@ test_expect_success '-n overrides repack.updateServerInfo=true' '
>         test_server_info_missing
>  '
>
> -test_expect_failure 'pending objects are repacked appropriately' '
> +test_expect_success 'pending objects are repacked appropriately' '
>         git init pending &&
>
>         (
> --
> gitgitgadget

Patch looks good.

[PATCH 3/3] path-walk: create initializer for path lists

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <stolee@gmail.com>

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.

Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
 path-walk.c | 57 +++++++++++++++++++++++------------------------------
 1 file changed, 25 insertions(+), 32 deletions(-)

diff --git a/path-walk.c b/path-walk.c
index 1215ed398f4f..f1ceed99e94c 100644
--- a/path-walk.c
+++ b/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,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

Re: [PATCH 3/3] path-walk: create initializer for path lists

[Elijah Newren]

On Wed, Aug 20, 2025 at 11:40 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <stolee@gmail.com>
>
> 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.
>
> Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---
>  path-walk.c | 57 +++++++++++++++++++++++------------------------------
>  1 file changed, 25 insertions(+), 32 deletions(-)
>
> diff --git a/path-walk.c b/path-walk.c
> index 1215ed398f4f..f1ceed99e94c 100644
> --- a/path-walk.c
> +++ b/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,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.

[PATCH v2 0/2] [2.51.0 Bug] Missing singleton objects in 'git repack -adf --path-walk'

[Derrick Stolee via GitGitGadget]

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 <dstolee@microsoft.com>
      
       ## 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.
      
          Signed-off-by: Derrick Stolee <stolee@gmail.com>
      
     + ## 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

[PATCH v2 1/2] path-walk: fix setup of pending objects

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <stolee@gmail.com>

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@gmail.com>
---
 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

[PATCH v2 2/2] path-walk: create initializer for path lists

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <stolee@gmail.com>

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.

Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
 path-walk.c | 57 +++++++++++++++++++++++------------------------------
 1 file changed, 25 insertions(+), 32 deletions(-)

diff --git a/path-walk.c b/path-walk.c
index 1215ed398f4f..f1ceed99e94c 100644
--- a/path-walk.c
+++ b/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,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

Re: [PATCH v2 0/2] [2.51.0 Bug] Missing singleton objects in 'git repack -adf --path-walk'

[Elijah Newren]

On Mon, Aug 25, 2025 at 5:50 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> 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, this round addresses all of my feedback and looks good to me.

Re: [PATCH v2 0/2] [2.51.0 Bug] Missing singleton objects in 'git repack -adf --path-walk'

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> On Mon, Aug 25, 2025 at 5:50 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> 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, this round addresses all of my feedback and looks good to me.

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

Thanks, both.

Re: [PATCH v2 0/2] [2.51.0 Bug] Missing singleton objects in 'git repack -adf --path-walk'

[Patrick Steinhardt]

On Tue, Aug 26, 2025 at 08:58:12AM -0700, Junio C Hamano wrote:
> Elijah Newren <newren@gmail.com> writes:
> 
> > On Mon, Aug 25, 2025 at 5:50 AM Derrick Stolee via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >>
> >> 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, this round addresses all of my feedback and looks good to me.
> 
> Let's mark it for 'next', then.

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

Patrick