sparse-checkout: avoid crash when switching between cone and non-cone

[vdye]

Background

The goal of these changes was to fix the infinite loop (and subsequent crash) that results from running the following commands in sequence in a repository:

$ git sparse-checkout init
$ git sparse-checkout add folder1/
$ git sparse-checkout init --cone
$ git sparse-checkout add folder2/

Changes

• Disable cone mode when git sparse-checkout init --cone is called with existing non-cone patterns
• Exit with an error when git sparse-checkout add is called in cone mode with existing non-cone patterns
• Require starting '/' in cone mode patterns (otherwise interpreting them as invalid)

These patches, taken together, fix the failure case mentioned earlier (along with other, similar cases covered in the tests added to t1091).

Other Notes

I originally also had a commit that disallowed running add, set, list, and reapply when sparse-checkout is not enabled. It directly reversed the earlier decision to enable sparse-checkout when git sparse-checkout set is called with sparse-checkout disabled. As far as I can tell, it was added in V5 of the patch series introducing the command, but not much context is given as to why.

Most of the motivation to remove it came from the fact that 1) when sparse-checkout is disabled, the cone mode setting isn't preserved and 2) git sparse-checkout set doesn't include an option to set cone mode (or sparse index). Point 1) came up in the internal bug bash and led to some unexpected behavior; point 2) could partially resolve it, but I still find the (uncomfortably silent) auto-init to be unintuitive.

I ultimately didn't include the change here because lots of tests were relying on set to initialize the sparse-checkout (which, to me, indicates that users might as well?). I may introduce the change again (in a later pull request), but with some updates:

• add, reapply, and list are still disallowed outside an active sparse-checkout
• git sparse-checkout disable preserves the core.sparsecheckoutcone setting. It would be overwritten back to the default by git sparse-checkout init, but could be used when init-ing with git sparse-checkout set
• set called in a disabled sparse checkout causes a warning message to be displayed that says 1) it's initializing the sparse checkout and 2) it's doing so with cone mode enabled/disabled, and to use git sparse-checkout init to change that setting

Let me know what you think - I definitely think there are improvements that could be made with the UX here (brought to light by the bug bash), but because I'm not really the target audience of git sparse-checkout, I'm not sure what the "right" UX is.

[derrickstolee]

Changes

• Disable cone mode when git sparse-checkout init --cone is called with existing non-cone patterns

I think this is fine, and hopefully there is a new warning.

Another approach would be to clear the patterns if they are not cone-mode, since we are doing an init. That's a bigger change to the expectations, though.

• Exit with an error when git sparse-checkout add is called in cone mode with existing non-cone patterns

Good idea.

• Require starting '/' in cone mode patterns (otherwise interpreting them as invalid)

I'm surprised this wasn't already there. Good find.

I originally also had a commit that disallowed running add, set, list, and reapply when sparse-checkout is not enabled. It directly reversed the earlier decision to enable sparse-checkout when git sparse-checkout set is called with sparse-checkout disabled. As far as I can tell, it was added in V5 of the patch series introducing the command, but not much context is given as to why.

I think this is something we can discuss upstream and could reverse that decision.

Most of the motivation to remove it came from the fact that 1) when sparse-checkout is disabled, the cone mode setting isn't preserved and 2) git sparse-checkout set doesn't include an option to set cone mode (or sparse index). Point 1) came up in the internal bug bash and led to some unexpected behavior; point 2) could partially resolve it, but I still find the (uncomfortably silent) auto-init to be unintuitive.

I ultimately didn't include the change here because lots of tests were relying on set to initialize the sparse-checkout (which, to me, indicates that users might as well?).

I doubt users are relying on that, but we would find out quickly. We also have some clear "experimental" flags on the builtin to clarify things like this. As far as I know, all documentation says to init first.

I may introduce the change again (in a later pull request), but with some updates:

• add, reapply, and list are still disallowed outside an active sparse-checkout
• git sparse-checkout disable preserves the core.sparsecheckoutcone setting. It would be overwritten back to the default by git sparse-checkout init, but could be used when init-ing with git sparse-checkout set
• set called in a disabled sparse checkout causes a warning message to be displayed that says 1) it's initializing the sparse checkout and 2) it's doing so with cone mode enabled/disabled, and to use git sparse-checkout init to change that setting

Let me know what you think - I definitely think there are improvements that could be made with the UX here (brought to light by the bug bash), but because I'm not really the target audience of git sparse-checkout, I'm not sure what the "right" UX is.

These big questions are good ones to pursue outside of the release window. I'll dig in now.

[derrickstolee]

All of these changes make sense and look safe. Thanks for hardening the release!

[derrickstolee]

This is an interesting idea, but I agree that the complexity might not be worth it.

I think "deep/" also means "include anything that's in a deep/ directory, including subdirectories such as /bin/deep/", so we can't really convert in this way without changing the definition. Even scanning the index for these deep/ directories is only enough for HEAD and not enough as HEAD moves throughout history.

[derrickstolee]

@vdye Sorry we didn't get these in before the -rc2 rebase. Can you rebase --onto the new tentative/vfs-2.34.0 branch (or git-for-windows/git:main if conflicts don't happen) then merge this?