Re: Memory leak with sparse-checkout

[Derrick Stolee <stolee@gmail.com>]

On 9/21/2021 12:32 PM, Taylor Blau wrote:
> On Tue, Sep 21, 2021 at 08:55:01AM -0400, Derrick Stolee wrote:
>> On 9/20/2021 5:20 PM, Taylor Blau wrote:
>>> On Mon, Sep 20, 2021 at 04:56:47PM -0400, Derrick Stolee wrote:
>>>>>> I double-checked this to see how to fix this, and the 'list' subcommand
>>>>>> already notices that the patterns are not in cone mode and reverts its
>>>>>> behavior to writing all of the sparse-checkout file to stdout. It also
>>>>>> writes warnings over stderr before that.
>>>>>>
>>>>>> There might not be anything pressing to do here.
>>>>>
>>>>> Hmm. I think we'd probably want the same behavior for init and any other
>>>>> commands which could potentially overwrite the contents of the
>>>>> sparse-checkout file.
>>>>
>>>> Could you elaborate on what you mean by "the same behavior"?
>>>>
>>>> Do you mean that "git sparse-checkout add X" should act as if cone mode
>>>> is not enabled if the existing patterns are not cone-mode patterns?
>>>>
>>>> What exactly do you mean about "init" changing behavior here?
>>>
>>> No, I was referring to your suggestion from [1] to add a warning from
>>> "git sparse-checkout init --cone" when there are existing patterns which
>>> are not in cone-mode.
>>
>> This warning is part of the sparse-checkout pattern parsing logic, so
>> it happens whenever the patterns are loaded, including the "list"
>> subcommand (among other commands, not just the sparse-checkout builtin).
> 
> I might be misunderstanding what you're saying. But what I'm wondering
> is: if we detect when existing patterns aren't in cone-mode, why didn't
> we catch that case originally when the memory leak was discovered?

Easy answer: there was a bug. The pattern in the original report evaded
the checks that were implemented to identify non-cone-mode patterns.

My patch 3 [1] fixes that problem, with this hunk:

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

and a test case change is required to avoid having the warning message
appear in the wrong places:

@@ -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 &&

[1] https://lore.kernel.org/git/d513b28b75189d066f9c66de44a1a578cbc38139.1632160658.git.gitgitgadget@gmail.com/

> I thought that it might have been related to your third patch to change
> how bad patterns are detected. But I ran the following script after
> applying each of your three patches individually:
> 
>     rm -fr repo
>     git init repo
>     cd repo
> 
>     git sparse-checkout init
>     git sparse-checkout add foo
>     git sparse-checkout init --cone
>     git sparse-checkout add foo
> 
> and the only difference is that we started silently dropping the bad
> "foo" pattern after re-adding foo in cone-mode starting with the second
> patch.

In patch 2, we "detect" that the old patterns were not in cone mode
because the core.sparseCheckoutCone config is false when parsing the
patterns, so use_cone_patterns is 0.

> I guess my question is: it seems like there may be a friendlier way to
> tell the user that we're about to drop their sparse-checkout definition
> instead of just doing it silently. And it seems like you're saying that
> we already have something that detects that and is used everywhere. But
> I wonder why it wasn't kicking in in the original report.

You are correct that we should make better documentation around the
re-initialization of patterns. And this _is_ a change of behavior and
I will cave to concerns around that. I think this is more a case of
matching what users expect from the interface, or at minimum helping
them fall into the "pit of success".

Let's move the discussion to that thread so we can interleave the
patches themselves.

Thanks,
-Stolee