[1/7] sparse-index: prevent repo root from becoming sparse

Commit Message

Victoria Dye Feb. 23, 2022, 6:25 p.m. UTC

From: Victoria Dye <[e-mail]>

Prevent the repository root from being collapsed into a sparse directory by
treating an empty path as "inside the sparse-checkout". When collapsing a
sparse index (e.g. in 'git sparse-checkout reapply'), the root directory
typically could not become a sparse directory due to the presence of in-cone
root-level files and directories. However, if no such in-cone files or
directories were present, there was no explicit check signaling that the
"repository root path" (an empty string, in the case of
'convert_to_sparse(...)') was in-cone, and a sparse directory index entry
would be created from the repository root directory.

The documentation in Documentation/git-sparse-checkout.txt explicitly states
that the files in the root directory are expected to be in-cone for a
cone-mode sparse-checkout. Collapsing the root into a sparse directory entry
violates that assumption, as sparse directory entries are expected to be
outside the sparse cone and have SKIP_WORKTREE enabled. This invalid state
in turn causes issues with commands that interact with the index, e.g.
'git status'.

Treating an empty (root) path as in-cone prevents the creation of a root
sparse directory in 'convert_to_sparse(...)'. Because the repository root is
otherwise never compared with sparse patterns (in both cone-mode and
non-cone sparse-checkouts), the new check does not cause additional changes
to how sparse patterns are applied.

Signed-off-by: Victoria Dye <[e-mail]>
---
 dir.c                                    |  7 ++++---
 t/t1092-sparse-checkout-compatibility.sh | 18 ++++++++++++++++++
 2 files changed, 22 insertions(+), 3 deletions(-)

Comments

Derrick Stolee Feb. 24, 2022, 4:48 p.m. UTC | #1

On 2/23/2022 1:25 PM, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <[e-mail]>

Aside: It looks like you have the same "mailto:" email addresses in the
CC similar to a mistake I made. The root cause of my issue was that I
copy-pasted from the rendered description of another PR instead of
copying the plaintext available by clicking the "Edit" button. Perhaps
this is worth extending GGG to prevent this issue in the future.

> Prevent the repository root from being collapsed into a sparse directory by
> treating an empty path as "inside the sparse-checkout". When collapsing a
> sparse index (e.g. in 'git sparse-checkout reapply'), the root directory
> typically could not become a sparse directory due to the presence of in-cone
> root-level files and directories. However, if no such in-cone files or
> directories were present, there was no explicit check signaling that the
> "repository root path" (an empty string, in the case of
> 'convert_to_sparse(...)') was in-cone, and a sparse directory index entry
> would be created from the repository root directory.
> 
> The documentation in Documentation/git-sparse-checkout.txt explicitly states
> that the files in the root directory are expected to be in-cone for a
> cone-mode sparse-checkout. Collapsing the root into a sparse directory entry
> violates that assumption, as sparse directory entries are expected to be
> outside the sparse cone and have SKIP_WORKTREE enabled. This invalid state
> in turn causes issues with commands that interact with the index, e.g.
> 'git status'.
> 
> Treating an empty (root) path as in-cone prevents the creation of a root
> sparse directory in 'convert_to_sparse(...)'. Because the repository root is
> otherwise never compared with sparse patterns (in both cone-mode and
> non-cone sparse-checkouts), the new check does not cause additional changes
> to how sparse patterns are applied.

Good catch! I agree with everything said here.

> +	if (!strlen(path) ||

Instead of a full strlen(), could we just check "if (!*path ||" to
look for the nul byte?

Thanks,
-Stolee

Victoria Dye Feb. 24, 2022, 9:42 p.m. UTC | #2

Derrick Stolee wrote:
> On 2/23/2022 1:25 PM, Victoria Dye via GitGitGadget wrote:
>> From: Victoria Dye <[e-mail]>
> 
> Aside: It looks like you have the same "mailto:" email addresses in the
> CC similar to a mistake I made. The root cause of my issue was that I
> copy-pasted from the rendered description of another PR instead of
> copying the plaintext available by clicking the "Edit" button. Perhaps
> this is worth extending GGG to prevent this issue in the future.
> 
>> Prevent the repository root from being collapsed into a sparse directory by
>> treating an empty path as "inside the sparse-checkout". When collapsing a
>> sparse index (e.g. in 'git sparse-checkout reapply'), the root directory
>> typically could not become a sparse directory due to the presence of in-cone
>> root-level files and directories. However, if no such in-cone files or
>> directories were present, there was no explicit check signaling that the
>> "repository root path" (an empty string, in the case of
>> 'convert_to_sparse(...)') was in-cone, and a sparse directory index entry
>> would be created from the repository root directory.
>>
>> The documentation in Documentation/git-sparse-checkout.txt explicitly states
>> that the files in the root directory are expected to be in-cone for a
>> cone-mode sparse-checkout. Collapsing the root into a sparse directory entry
>> violates that assumption, as sparse directory entries are expected to be
>> outside the sparse cone and have SKIP_WORKTREE enabled. This invalid state
>> in turn causes issues with commands that interact with the index, e.g.
>> 'git status'.
>>
>> Treating an empty (root) path as in-cone prevents the creation of a root
>> sparse directory in 'convert_to_sparse(...)'. Because the repository root is
>> otherwise never compared with sparse patterns (in both cone-mode and
>> non-cone sparse-checkouts), the new check does not cause additional changes
>> to how sparse patterns are applied.
> 
> Good catch! I agree with everything said here.
> 
>> +	if (!strlen(path) ||
> 
> Instead of a full strlen(), could we just check "if (!*path ||" to
> look for the nul byte?
> 
> Thanks,
> -Stolee

I'll reroll with corrected CC's & make the `strlen` -> `*path` change.
Thanks!

Patch

diff --git a/dir.c b/dir.c
index d91295f2bcd..f1cbef23a8a 100644
--- a/dir.c
+++ b/dir.c
@@ -1463,10 +1463,11 @@  static int path_in_sparse_checkout_1(const char *path,
 	const char *end, *slash;
 
 	/*
-	 * We default to accepting a path if there are no patterns or
-	 * they are of the wrong type.
+	 * We default to accepting a path if the path is empty, there are no
+	 * patterns, or the patterns are of the wrong type.
 	 */
-	if (init_sparse_checkout_patterns(istate) ||
+	if (!strlen(path) ||
+	    init_sparse_checkout_patterns(istate) ||
 	    (require_cone_mode &&
 	     !istate->sparse_checkout_patterns->use_cone_patterns))
 		return 1;
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index f3a059e5af5..9ef7cd80885 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -244,6 +244,24 @@  test_expect_success 'expanded in-memory index matches full index' '
 	test_sparse_match git ls-files --stage
 '
 
+test_expect_success 'root directory cannot be sparse' '
+	init_repos &&
+
+	# Remove all in-cone files and directories from the index, collapse index
+	# with `git sparse-checkout reapply`
+	git -C sparse-index rm -r . &&
+	git -C sparse-index sparse-checkout reapply &&
+
+	# Verify sparse directories still present, root directory is not sparse
+	cat >expect <<-EOF &&
+	folder1/
+	folder2/
+	x/
+	EOF
+	git -C sparse-index ls-files --sparse >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'status with options' '
 	init_repos &&
 	test_sparse_match ls &&