Sparse checkout: fix mixed-mode pattern issues

From: Derrick Stolee <[email protected]>

Add a test to t1091-sparse-checkout-builtin.sh that would result in an infinite loop and out-of-memory error before this change. The issue relies on having non-cone-mode patterns while trying to modify the patterns in cone-mode.

The fix is simple, allowing us to break from the loop when the input path does not contain a slash, as the "dir" pattern we added does not.

This is only a fix to the critical out-of-memory error. A better response to such a strange state will follow in a later change.

Reported-by Calbabreaker <[email protected]>

Helped-by Taylor Blau <[email protected]>

Signed-off-by Derrick Stolee <[email protected]>

---
 builtin/sparse-checkout.c          | 2 +-
 t/t1091-sparse-checkout-builtin.sh | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

builtin/sparse-checkout.c +0 -0

index 8ba9f13787b..b45fd97a98b 100644

From: Derrick Stolee <[email protected]>

Previously, when a user runs 'git sparse-checkout init --cone', the existing patterns would remain, even if the patterns were not in cone mode. This causes confusion as to how the patterns should work when later 'git sparse-checkout add' commands append to the pattern list.

In fact, the way these patterns were modified was not even a strict appending of patterns, but mutating and reordering patterns because of how the paths were interpreted and rewritten.

As a first step, we shall start overwriting the pattern set completely when switching to cone mode, unless the existing patterns already match cone mode patterns. The 'use_cone_patterns' member is set to false if the patterns fail to parse using cone mode restrictions.

Signed-off-by Derrick Stolee <[email protected]>

---
 builtin/sparse-checkout.c          | 13 +++++++++++--
 t/t1091-sparse-checkout-builtin.sh |  9 ++++++++-
 2 files changed, 19 insertions(+), 3 deletions(-)

builtin/sparse-checkout.c +0 -0

index b45fd97a98b..fe76c3eedda 100644

builtin/sparse-checkout.c +0 -0

 	}
 
 	if (get_oid("HEAD", &oid)) {

t/t1091-sparse-checkout-builtin.sh +0 -0

index a429d2cc671..af0acd32bd9 100755
@@ -108,7 +108,14 @@ test_expect_success 'switching to cone mode with non-cone mode patterns' '
 	git -C bad-patterns sparse-checkout init &&
 	git -C bad-patterns sparse-checkout add dir &&

See my message at [1] for some more details about a possible suggestion there.

[1]: https://lore.kernel.org/git/YUi55%[email protected]/

From: Derrick Stolee <[email protected]>

When in cone mode sparse-checkout, it is unclear how 'git sparse-checkout add <dir1> ...' should behave if the existing sparse-checkout file does not match the cone mode patterns. Change the behavior to fail with an error message about the existing patterns.

Also, all cone mode patterns start with a '/' character, so add that restriction. This is necessary for our example test 'cone mode: warn on bad pattern', but also requires modifying the example sparse-checkout file we use to test the warnings related to recognizing cone mode patterns.

Signed-off-by Derrick Stolee <[email protected]>

---
 builtin/sparse-checkout.c          |  3 +++
 dir.c                              |  2 +-
 t/t1091-sparse-checkout-builtin.sh | 11 +++++++----
 3 files changed, 11 insertions(+), 5 deletions(-)

builtin/sparse-checkout.c +0 -0

index fe76c3eedda..2492ae828a9 100644

dir.c +0 -0

 	    strstr(given->pattern, "**")) {
 		/* Not a cone pattern. */
 		warning(_("unrecognized pattern: '%s'"), given->pattern);

t/t1091-sparse-checkout-builtin.sh +2 -1

index af0acd32bd9..780c6a1aaae 100755
@@ -108,14 +108,17 @@ test_expect_success 'switching to cone mode with non-cone mode patterns' '
 	git -C bad-patterns sparse-checkout init &&
 	git -C bad-patterns sparse-checkout add dir &&
 	git -C bad-patterns config core.sparseCheckoutCone true &&
-	git -C bad-patterns sparse-checkout add dir &&
+
+	test_must_fail git -C bad-patterns sparse-checkout add dir 2>err &&

t/t1091-sparse-checkout-builtin.sh +0 -0

 '
 
 test_expect_success 'interaction with clone --no-checkout (unborn index)' '

@@ -182,9 +185,9 @@ test_expect_success 'set sparse-checkout using --stdin' '
 test_expect_success 'add to sparse-checkout' '

Quoted 26 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

Add a test to t1091-sparse-checkout-builtin.sh that would result in an infinite loop and out-of-memory error before this change. The issue relies on having non-cone-mode patterns while trying to modify the patterns in cone-mode.

The fix is simple, allowing us to break from the loop when the input path does not contain a slash, as the "dir" pattern we added does not.

This is only a fix to the critical out-of-memory error. A better response to such a strange state will follow in a later change.

Reported-by Calbabreaker <[email protected]>

Helped-by Taylor Blau <[email protected]>

Signed-off-by Derrick Stolee <[email protected]>

---
 builtin/sparse-checkout.c          | 2 +-
 t/t1091-sparse-checkout-builtin.sh | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

builtin/sparse-checkout.c +0 -0

index 8ba9f13787b..b45fd97a98b 100644

builtin/sparse-checkout.c static void insert_recursive_pattern(struct pattern_list *pl, struct strbuf *pat · line 393

Quoted 6 lines

builtin/sparse-checkout.c +1 -1

@@ -389,7 +389,7 @@ static void insert_recursive_pattern(struct pattern_list *pl, struct strbuf *pat
 		char *oldpattern = e->pattern;
 		size_t newlen;
 
-		if (slash == e->pattern)
+		if (!slash || slash == e->pattern)
 			break;

If we are preparing to make it so that we do not blindly copy patterns from a sparse checkout without cone mode enabled, then wouldn't this new case be a BUG()?

Of course, users could still tweak the contents of their sparse-checkout file as they wish, but I'd expect that we'd catch those cases, too (i.e., by validating the contents of the existing sparse-checkout before calling this function).

t/t1091-sparse-checkout-builtin.sh test_expect_success 'clone --sparse' ' · line 110

Quoted 13 lines

t/t1091-sparse-checkout-builtin.sh +5 -0

index 38fc8340f5c..a429d2cc671 100755
@@ -103,6 +103,14 @@ test_expect_success 'clone --sparse' '
 	check_files clone a
 '
 
+test_expect_success 'switching to cone mode with non-cone mode patterns' '
+	git init bad-patterns &&
+	git -C bad-patterns sparse-checkout init &&
+	git -C bad-patterns sparse-checkout add dir &&
+	git -C bad-patterns config core.sparseCheckoutCone true &&

Makes sense that we'd want to update the config rather than call "init --cone" here, since a subsequent patch would change the thing that this is testing (from "doesn't OOM in the above-described situation" to "clears the existing contents of your sparse-checkout").

t/t1091-sparse-checkout-builtin.sh test_expect_success 'clone --sparse' ' · line 113

Quoted 3 lines

t/t1091-sparse-checkout-builtin.sh +3 -0

+	git -C bad-patterns sparse-checkout add dir
+'
+

In other series I've suggested a cosmetic change to move all of these to a sub-shell that begins with "cd bad-patterns &&", but obviously that is a relatively unimportant suggestion.

Quoted 8 lines

If we are preparing to make it so that we do not blindly copy patterns from a sparse checkout without cone mode enabled, then wouldn't this new case be a BUG()?

Of course, users could still tweak the contents of their sparse-checkout file as they wish, but I'd expect that we'd catch those cases, too (i.e., by validating the contents of the existing sparse-checkout before calling this function).

If I was more confident that we were catching absolutely every possible case of non-cone mode patterns in our parsing logic, then I suppose a BUG() could apply here. At minimum, at this point in time (before fixing the gap in parsing in patch 3) the test below would not even work with test_must_fail, since the exit code would be unexpected.

Quoted 7 lines

t/t1091-sparse-checkout-builtin.sh test_expect_success 'clone --sparse' ' · line 113

Quoted 3 lines

t/t1091-sparse-checkout-builtin.sh +3 -0

+	git -C bad-patterns sparse-checkout add dir
+'
+

In other series I've suggested a cosmetic change to move all of these to a sub-shell that begins with "cd bad-patterns &&", but obviously that is a relatively unimportant suggestion.

The only defense I have for not using a subshell and 'cd' is that later I can use an "expect" file in my current directory without it being "in" the repository. It doesn't really matter for this example, but it has in the past, causing me to do this by habit. A tab is smaller than the string " -C bad-patterns", so it's probably worth changing.