Re: [PATCH 0/7] RFC: sparse checkout: make --cone mode the default, and check add/set argument validity

From: Derrick Stolee <derrickstolee@github.com>
Date: Feb 14, 2022, 16:19 UTC
Message-ID: <dfda7d2e-940b-bb54-6d2b-cda3a00abc0a@github.com>
In-Reply-To: <pull.1118.git.1644712797.gitgitgadget@gmail.com>

On 2/12/2022 7:39 PM, Elijah Newren via GitGitGadget wrote:

Show 12 quoted lines

> Note (reason for RFC): this is RFC primarily because of dependencies (you
> may not want to pick this up yet, Junio), though there is also a question of
> whether to split patch 7 into two steps -- one for now and one we take in
> some future release. In particular, the first step could be to have
> sparse-checkout error out if neither --no-cone nor --cone are specified and
> then change the default to be --cone in some future release. I don't think
> splitting it into two steps is needed given (a) users who are unaware of the
> change will still get useful error messages telling them that directories
> are expected due to patches 4-6 of this series, and (b) the huge
> "EXPERIMENTAL" warning and explicit note about likely behavioral changes in
> git-sparse-checkout.txt serves as warning about the changes. However, the
> two step approach is an alternative.

I support this change. This will also require an update to the 'git clone' documentation around the '--sparse' option, as I imagine we are going to be changing behavior there. (If not, then we should do that as part of the deprecation.)

Show 5 quoted lines

> Note 2 (dependencies): this depends on en/present-despite-skipped (which
> depends on vd/sparse-clean-etc) and on
> ds/sparse-checkout-requires-per-worktree-config, because of otherwise heavy
> text conflicts in patch 7 to git-sparse-checkout.txt. Given that neither of
> those have merged to next yet, it may be premature to pick up this series.

Yes, hopefully things will start to settle down a little, especially since
vd/sparse-clean-etc is due to merge any day now.

Show 14 quoted lines

> This series continues attempts to make sparse-checkouts more user friendly.
> A quick overview:
> 
>  * Patches 1-2 fix existing bugs from en/sparse-checkout-set
>  * Patch 3 fixes sparse-checkout-from-subdirectories-ignores-"prefix" (see
>    https://lore.kernel.org/git/29f0410e-6dfa-2e86-394d-b1fb735e7608@gmail.com/),
>    at least in cone mode. In non-cone mode it is not clear if patch 3 is a
>    "fix" or a "break" (see the "NON-CONE PROBLEMS" section of the manual
>    added in patch 7, and
>    https://lore.kernel.org/git/e1934710-e228-adc4-d37c-f706883bd27c@gmail.com/
>    where Stolee suggested it might be incorrect).
>  * Patches 4-6 check positional arguments to set/add and provide
>    errors/warnings for very likely mistakes. It also adds a --skip-checks
>    flag for overridding in case you have a very unusual situation.

I took a close look at these patches and mostly have minor typo fixes. There was one behavior issue: I don't think you should warn for file paths in non- cone-mode. Being able to select a single file in a directory full of large files is one of the main reasons to use non-cone-mode, in my experience.

It might be worth adding some documentation about how to reorganize a repo to fit cone mode patterns, but that's not necessary.

Show 5 quoted lines

>  * Patch 7 makes cone mode the default, and makes large updates to the
>    documentation both to explain why we changed the default, and to simplify
>    the documentation since users can just use directories and ignore the
>    intricacies of gitignore-style patterns and how they relate to sparse
>    checkouts.

I'm a fan of the end-result of this patch. I responded with some specific comments and a suggestion for splitting it into a series of four patches.

Your first 6 patches are likely to be noncontroversial and could merge more quickly than the deprecation. I think it would be good to get the full deprecation under full review as soon as possible so we can give the community a long window to comment on it.

We can also consider if we need a release or two where this behavior change is announced, but not actually done. I'm not sure if that is necessary. Making '--no-cone' required might stir up some noise that indicate how much of an impact the change would make.

Thanks, -Stolee