Re: Memory leak with sparse-checkout

On Tue, Sep 21, 2021 at 08:55:01AM -0400, Derrick Stolee wrote:

On 9/20/2021 5:20 PM, Taylor Blau wrote:

quoted

On Mon, Sep 20, 2021 at 04:56:47PM -0400, Derrick Stolee wrote:

quoted

quoted

quoted

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?

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.

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.

Thanks,
Taylor