Re: [PATCH] unpack-trees: fix sparse directory recursion check

From: Derrick Stolee <derrickstolee@github.com>
Date: Sep 2, 2022, 13:32 UTC
Message-ID: <8dea9ffe-d03b-f838-6cc5-db0de5024d9f@github.com>
In-Reply-To: <pull.1344.git.1662066153644.gitgitgadget@gmail.com>

On 9/1/2022 5:02 PM, Victoria Dye via GitGitGadget wrote:

Show 12 quoted lines

> From: Victoria Dye <vdye@github.com>
> 
> Ensure 'is_sparse_directory_entry()' receives a valid 'name_entry *' if one
> exists in the list of tree(s) being unpacked in 'unpack_callback()'.
> 
> Currently, 'is_sparse_directory_entry()' is called with the first
> 'name_entry' in the 'names' list of entries on 'unpack_callback()'. However,
> this entry may be empty even when other elements of 'names' are not (such as
> when switching from an orphan branch back to a "normal" branch). As a
> result, 'is_sparse_directory_entry()' could incorrectly indicate that a
> sparse directory is *not* actually sparse because the name of the index
> entry does not match the (empty) 'name_entry' path.

Thank you for finding the root cause here. It was very non-obvious!

Show 9 quoted lines

> +test_expect_success 'checkout orphan then non-orphan' '
> +	init_repos &&
> +
> +	test_all_match git checkout --orphan test-orphan &&
> +	test_all_match git status --porcelain=v2 &&
> +	test_all_match git checkout base &&
> +	test_all_match git status --porcelain=v2
> +'
> +

This test demonstrates how specific the case needs to be for this to actually happen.

Could it also happen if we are going from a commit without that sparse directory and then to a commit with that sparse directory? I think that would be a more common case, but I was unable to manipulate the test repo in t1092 to trigger this bug in the existing test cases.

This makes me think that this bug _is_ extremely rare, so we don't need to rush this into a 2.37.4 or anything. It would be good to bring it into 2.38.0-rc0, though.

Thanks! -Stolee