Re: Memory leak with sparse-checkout

[Taylor Blau <me@xxxxxxxxxxxx>]

On Mon, Sep 20, 2021 at 09:45:14PM +0930, Calbabreaker wrote:
> What did you do before the bug happened? (Steps to reproduce your issue)
>
> This was ran:
>
> git clone https://github.com/Calbabreaker/piano --sparse
> cd piano
> git sparse-checkout add any_text
> git checkout deploy-frontend
> git sparse-checkout init --cone
> git sparse-checkout add any_text

Thanks for the reproduction. An even simpler one may be (inside of any
repository):

    git sparse-checkout init
    git sparse-checkout add dir
    git sparse-checkout init --cone
    git sparse-checkout add dir

The problem occurs because we keep existing entries when adding to the
sparse-checkout list, and cone-mode patterns do not mix with
non cone-mode patterns.

So after the first init and "add dir", your sparse-checkout file looks
like:

  /*
  !/*/
  dir

but then when we convert to cone-mode and try and add "dir" (which in
cone-mode we'll convert to "/dir/"), we run into trouble when adding the
existing "dir" entry. That's because add_patterns_cone_mode() calls
insert_recursive_pattern() on every entry in the existing list,
including "dir".

So when we call insert_recursive_pattern() with any pattern list and
path containing "dir", we first insert "dir" into the list, and then:

  char *slash = strrchr(e->pattern, '/');
  char *oldpattern = e->pattern;

  if (slash == e->pattern)
    break;
  // trim off a slash, repeat

except slash is NULL because "dir" doesn't contain a slash. And that
explains the problem you're seeing, because (a) we'll stay in that while
loop forever, and (b) because each iteration allocates memory to
accommodate the new pattern, so we'll eventually run out of memory.

The wrong thing to do would be to handle this case by changing the
conditional to "if (!slash || slash == e->pattern)", because we can't
blindly carry forward some patterns which look like cone-mode patterns,
since together the list of sparse-checkout entries may not represent a
cone.

(An example here is if we added /foo/bar/baz/* without the corresponding
/foo/, !/foo/*, and so on).

So I think the problem really is that we need to drop existing patterns
when re-initializing the sparse-checkout in cone mode. We could try to
recognize that existing patterns may already constitute a cone (and/or
create a cone that covers the existing patterns).

But I think the easiest thing (if a little unfriendly) would be to just
drop them and start afresh when re-initializing the sparse-checkout in
cone mode.

I'm adding Stolee to the CC to see if he thinks that would be sensible
behavior or not.

Thanks,
Taylor