Re: [PATCH 1/3] sparse-checkout: fix OOM error with mixed patterns

[Taylor Blau <me@ttaylorr.com>]

On Mon, Sep 20, 2021 at 05:57:36PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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 <calbabreaker@gmail.com>
> Helped-by: Taylor Blau <me@ttaylorr.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  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).

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

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