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

[Taylor Blau <me@ttaylorr.com>]

On Tue, Sep 21, 2021 at 09:06:59AM -0400, Derrick Stolee 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).
>
> 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()?

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