[PATCH 0/3] Sparse checkout: fix mixed-mode pattern issues

This fixes a memory leak reported in [1], but also fixes some behavior
concerns around the sparse-checkout command when the sparse-checkout file
does not use cone mode patterns, but cone mode is enabled.

[1]
https://lore.kernel.org/git/CAKRwm5a9PyqffEC5N__urSpNcZ-d5vz9GBM2Ei16eGS25B=-FQ@mail.gmail.com/

 1. The first patch fixes the OOM as recommended by Taylor.
 2. The second patch changes the behavior of 'git sparse-checkout init
    --cone' to overwrite the sparse-checkout file if the patterns are not in
    cone mode.
 3. The third patch causes 'git sparse-checkout add' to fail if cone mode is
    enabled but the existing patterns are not in cone mode. This also
    requires strengthening our pattern filtering to require the first
    character be a slash ('/'), which should have been there from the start.

Thanks, -Stolee

Derrick Stolee (3):
  sparse-checkout: fix OOM error with mixed patterns
  sparse-checkout: clear patterns when switching modes
  sparse-checkout: refuse to add to bad patterns

 builtin/sparse-checkout.c          | 18 +++++++++++++++---
 dir.c                              |  2 +-
 t/t1091-sparse-checkout-builtin.sh | 22 ++++++++++++++++++++--
 3 files changed, 36 insertions(+), 6 deletions(-)


base-commit: 4c719308ce59dc70e606f910f40801f2c6051b24
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1043%2Fderrickstolee%2Fsparse-checkout-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1043/derrickstolee/sparse-checkout-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1043
-- 
gitgitgadget

From: Derrick Stolee <redacted>

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 <redacted>
Helped-by: Taylor Blau [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
---
 builtin/sparse-checkout.c          | 2 +-
 t/t1091-sparse-checkout-builtin.sh | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 8ba9f13787b..b45fd97a98b 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c

@@ -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;
 
 		newlen = slash - e->pattern;

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 38fc8340f5c..a429d2cc671 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh

@@ -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 &&
+	git -C bad-patterns sparse-checkout add dir
+'
+
 test_expect_success 'interaction with clone --no-checkout (unborn index)' '
 	git clone --no-checkout "file://$(pwd)/repo" clone_no_checkout &&
 	git -C clone_no_checkout sparse-checkout init --cone &&

-- 
gitgitgadget

From: Derrick Stolee <redacted>

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 <redacted>
---
 builtin/sparse-checkout.c          | 13 +++++++++++--
 t/t1091-sparse-checkout-builtin.sh |  9 ++++++++-
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index b45fd97a98b..fe76c3eedda 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c

@@ -348,8 +348,17 @@ static int sparse_checkout_init(int argc, const char **argv)
 
 	/* If we already have a sparse-checkout file, use it. */
 	if (res >= 0) {
-		free(sparse_filename);
-		return update_working_directory(NULL);
+		if (pl.use_cone_patterns || !init_opts.cone_mode) {
+			free(sparse_filename);
+			return update_working_directory(NULL);
+		}
+
+		/*
+		 * At this point, note that if res >= 0 but pl.use_cone_patterns
+		 * is false, then we want to override the patterns with the
+		 * initial set of cone mode patterns.
+		 */
+		clear_pattern_list(&pl);
 	}
 
 	if (get_oid("HEAD", &oid)) {

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index a429d2cc671..af0acd32bd9 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh

@@ -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 &&
 	git -C bad-patterns config core.sparseCheckoutCone true &&
-	git -C bad-patterns sparse-checkout add dir
+	git -C bad-patterns sparse-checkout add dir &&
+
+	git -C bad-patterns sparse-checkout init --cone &&
+	cat >expect <<-\EOF &&
+	/*
+	!/*/
+	EOF
+	test_cmp expect bad-patterns/.git/info/sparse-checkout
 '
 
 test_expect_success 'interaction with clone --no-checkout (unborn index)' '

-- 
gitgitgadget

From: Derrick Stolee <redacted>

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 <redacted>
---
 builtin/sparse-checkout.c          |  3 +++
 dir.c                              |  2 +-
 t/t1091-sparse-checkout-builtin.sh | 11 +++++++----
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index fe76c3eedda..2492ae828a9 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c

@@ -513,6 +513,9 @@ static void add_patterns_cone_mode(int argc, const char **argv,
 		die(_("unable to load existing sparse-checkout patterns"));
 	free(sparse_filename);
 
+	if (!existing.use_cone_patterns)
+		die(_("existing sparse-checkout patterns do not use cone mode"));
+
 	hashmap_for_each_entry(&existing.recursive_hashmap, &iter, pe, ent) {
 		if (!hashmap_contains_parent(&pl->recursive_hashmap,
 					pe->pattern, &buffer) ||

diff --git a/dir.c b/dir.c
index 03c4d212672..93136442103 100644
--- a/dir.c
+++ b/dir.c

@@ -727,7 +727,7 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern
 	}
 
 	if (given->patternlen < 2 ||
-	    *given->pattern == '*' ||
+	    *given->pattern != '/' ||
 	    strstr(given->pattern, "**")) {
 		/* Not a cone pattern. */
 		warning(_("unrecognized pattern: '%s'"), given->pattern);

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index af0acd32bd9..780c6a1aaae 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh

@@ -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 &&
+	grep "existing sparse-checkout patterns do not use cone mode" err &&
 
 	git -C bad-patterns sparse-checkout init --cone &&
 	cat >expect <<-\EOF &&
 	/*
 	!/*/
 	EOF
-	test_cmp expect bad-patterns/.git/info/sparse-checkout
+	test_cmp expect bad-patterns/.git/info/sparse-checkout &&
+	git -C bad-patterns sparse-checkout add dir
 '
 
 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' '
 	cat repo/.git/info/sparse-checkout >expect &&
 	cat >add <<-\EOF &&
-	pattern1
+	/pattern1
 	/folder1/
-	pattern2
+	/pattern2
 	EOF
 	cat add >>expect &&
 	git -C repo sparse-checkout add --stdin <add &&

-- 
gitgitgadget

On Mon, Sep 20, 2021 at 05:57:36PM +0000, Derrick Stolee via GitGitGadget wrote:

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

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

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

Thanks,
Taylor

On Mon, Sep 20, 2021 at 05:57:37PM +0000, Derrick Stolee via GitGitGadget wrote:

I traced through this code beginning with sparse_checkout_init() and
right before the check on pl.use_cone_patterns, and I couldn't find
anywhere that this variable is set to a non-zero value. Could you let me
know if I'm missing something, or is the left-hand side of this or
redundant?

I guess what this check wants to be saying is "if the existing
sparse-checkout was in cone mode or we are transitioning to cone mode,
then quit now".

+			free(sparse_filename);
+			return update_working_directory(NULL);
+		}
+
+		/*
+		 * At this point, note that if res >= 0 but pl.use_cone_patterns
+		 * is false, then we want to override the patterns with the
+		 * initial set of cone mode patterns.
+		 */
+		clear_pattern_list(&pl);

...or otherwise, we are transitioning into cone mode from a non-cone
mode state?

If so, this may be somewhat surprising to users who have their patterns
cleared after re-initializing. I guess the command is called "init", but
it may be friendlier to have a `--reinitialize` option or similar which
indicates the user's preference to obliterate existing patterns if
necessary.

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

Makes sense that we have to look at the contents of
$GIT_DIR/info/sparse-checkout directly here to see the explicit '/*' and
'!/*/' patterns.

Thanks,
Taylor

[1]: https://lore.kernel.org/git/YUi55%2F3L9nizTVyA@nand.local/

On Mon, Sep 20, 2021 at 02:52:20PM -0400, Taylor Blau wrote:

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

OK, I see that in the third patch we do start complaining about adding
patterns in cone-mode when the existing sparse checkout configuration
does not form a cone.

So that may indicate that we expect users who wish to blow away their
configuration to just run "init --cone". I'd be OK with that (without an
additional warning step, although I think that doing that would be
better). But either way, we should document this behavior clearly.

[1]: https://lore.kernel.org/git/YUi55%2F3L9nizTVyA@nand.local/

Thanks,
Taylor

On Mon, Sep 20, 2021 at 05:57:38PM +0000, Derrick Stolee via GitGitGadget wrote:

Makes sense.

Makes sense that we require cone-mode patterns to start with '/', which
necessarily means we want to drop the other arm `*given->pattern == '*'`.

Good thinking to make sure that we can add `dir` after clearing. But
let's check the contents of the sparse-checkout file to make sure that
it was added in cone mode.

Hmm. Does this new restriction apply to patterns given over the
command-line? (It looks like we handle the two slightly differently, so
I wonder if we are expecting users to write "git sparse-checkout add
/foo" instead of "... add foo" as a consequence).

Thanks,
Taylor

On 9/20/2021 2:24 PM, Taylor Blau wrote:

On Mon, Sep 20, 2021 at 05:57:36PM +0000, Derrick Stolee via GitGitGadget wrote:

quoted

From: Derrick Stolee <redacted>

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 <redacted>
Helped-by: Taylor Blau [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
---
 builtin/sparse-checkout.c          | 2 +-
 t/t1091-sparse-checkout-builtin.sh | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 8ba9f13787b..b45fd97a98b 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c

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

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

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 38fc8340f5c..a429d2cc671 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh

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

quoted

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

Thanks,
-Stolee

On Tue, Sep 21, 2021 at 09:06:59AM -0400, Derrick Stolee wrote:

quoted

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.

Right, but it could be a test_must_fail after the second patch, no?

Not calling BUG() is fine with me if you think there may be other cases
we haven't discovered. But we should have some way to discover them
instead if a user can generate them organically. Maybe a warning()?

quoted

quoted

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

Yeah. I admit to hardly caring about this (subshell vs. '-C
bad-patterns') at all. Either is completely fine with me.

Thanks,
Taylor