Re: [PATCH 7/7] sparse-checkout: make --cone the default and deprecate --no-cone

[Elijah Newren <newren@gmail.com>]

On Mon, Feb 14, 2022 at 8:14 AM Derrick Stolee <derrickstolee@github.com> wrote:
>
> On 2/12/2022 7:39 PM, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > Make --cone mode the default, and deprecate --no-cone mode.  While we
> > have no current plans to actually remove --no-cone mode, we think users
> > would be better off not using it.  Update the documentation accordingly,
> > including explaining why we think non-cone mode is problematic for
> > users.
>
> "deprecate" is a good word here. The functionality remains, but we make
> clear recommendations to not use it.
>
> > While at it, since the new default just uses directories and removes the
> > need to understand patterns, we can mark multiple sections in the manual
> > as "INTERNALS" to reflect the fact that users do not need to wade
> > through those sections to understand how to use the command any more.
> > We can instead add a simple EXAMPLES section to the manual which
> > distills the necessary bits from the more complex INTERNALS sections.
>
> > -SPARSE CHECKOUT
> > ----------------
> > +EXAMPLES
> > +--------
> > +`git sparse-checkout set MY/DIR1 SUB/DIR2`::
> > +
> > +     Change to a sparse-checkout with all files (at any depth) under
> > +     MY/DIR1/ and SUB/DIR2/ present in the working copy (plus all
> > +     files immediately under MY/ and SUB/ and the toplevel
>
> Do we like "toplevel" or "top-level"?

For consistency with the rest of the project, we should probably use
"toplevel" twice as frequently as "top-level":

$ git grep toplevel | wc -l
258
$ git grep top-level | wc -l
127

;-)

>
> > +     directory).  If already in a sparse checkout, change which files
>
> There is some inconsistency between "a sparse checkout" and
> "a sparse-checkout" here. I'm happy with either as long as we
> stay consistent.
>
> > +     are present in the working copy to this new selection.  Note
> > +     that this command will also delete all ignored files in any
> > +     directory that no longer has either tracked or
> > +     non-ignored-untracked files present.
> > +
> > +`git sparse-checkout disable`::
> > +
> > +     Repopulate the working directory with all files, disabling sparse
> > +     checkouts.
> > +
> > +`git sparse-checkout add SOME/DIR/ECTORY`::
> > +
> > +     Add all files under SOME/DIR/ECTORY/ (at any depth) to the
> > +     sparse checkout, as well as all files immediately under
> > +     SOME/DIR/ and immediately under SOME/.  Must already be in a
> > +     sparse checkout before using this command.
> > +
> > +`git sparse-checkout reapply`::
> > +
> > +     It is possible for commands to update the working tree in a way
> > +     that does not respect the selected sparsity directories, either
> > +     because of special cases (such as hitting conflicts when
> > +     merging/rebasing), or because some commands didn't fully support
> > +     sparse checkouts (e.g. the old `recursive` merge backend had
> > +     only limited support).  This command reapplies the existing
> > +     sparse directory specifications to make the working directory
> > +     match.
> > +
> > +INTERNALS -- SPARSE CHECKOUT
> > +----------------------------
>
> I like this switch to talk about "internals".
>
> > +INTERNALS -- NON-CONE PROBLEMS
> > +------------------------------
> > +
> > +The `$GIT_DIR/info/sparse-checkout` file populated by the `set` and
> > +`add` subcommands is defined to be a bunch of patterns (one per line)
> > +using the same syntax as `.gitignore` files.  In cone mode, these
> > +patterns are restricted to matching directories (and users only ever
> > +need supply or see directory names), while in non-cone mode any
> > +gitignore-style pattern is permitted.  Using the full gitignore-style
> > +patterns in non-cone mode has a number of shortcomings:
> ...
> > +For all these reasons, non-cone mode is deprecated.  Please switch to
> > +using cone mode.
>
> I appreciate your very clear description here, as it helps make the
> case for us.
>
> > +INTERNALS -- FULL PATTERN SET
> > +-----------------------------
> > +
> > +As noted above, the sparse-checkout file uses the same syntax as
> > +`.gitignore` files; see linkgit:gitignore[5] for details.  Here, though,
> > +the patterns are being used to select which files to include rather than
> > +which files to exclude.
> > +
> > +To complicate things a bit further, while
> > +`$GIT_DIR/info/sparse-checkout` is usually used to specify what files
> > +are included, you can also specify what files are _not_ included, using
> > +negative patterns. For example, to select everything, and then to remove
> > +the file `unwanted`:
>
> ...
>
> > -----------------
> > +INTERNALS -- CONE PATTERN SET
> > +-----------------------------
>
> I wonder if this should be moved further up, above of the non-cone
> internals.
>
> > -The full pattern set allows for arbitrary pattern matches and complicated
> > -inclusion/exclusion rules. These can result in O(N*M) pattern matches when
> > -updating the index, where N is the number of patterns and M is the number
> > -of paths in the index. To combat this performance issue, a more restricted
> > -pattern set is allowed when `core.sparseCheckoutCone` is enabled.
> > +The full pattern set allows for arbitrary pattern matches and
> > +complicated inclusion/exclusion rules. As noted above, this can result
> > +in O(N*M) pattern matches when updating the index, where N is the number
>
> I see you are including information about O(N*M) "as noted above". I think
> the cone pattern set should shift to assume it's the first mode people read,
> and then the comparisons can happen in the non-cone mode internals.

Okay, I'll try reordering.

> >  If your repository contains one or more submodules, then submodules
> >  are populated based on interactions with the `git submodule` command.
> > diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> > index 6e0af166f80..aa2c66f15e3 100644
> > --- a/builtin/sparse-checkout.c
> > +++ b/builtin/sparse-checkout.c
> > @@ -395,13 +395,13 @@ static int update_modes(int *cone_mode, int *sparse_index)
> >
> >       /* Set cone/non-cone mode appropriately */
> >       core_apply_sparse_checkout = 1;
> > -     if (*cone_mode == 1) {
> > +     if (*cone_mode == 1 || *cone_mode == -1) {
> >               mode = MODE_CONE_PATTERNS;
> > -             core_sparse_checkout_cone = 1;
> > +             if (record_mode)
> > +                     core_sparse_checkout_cone = 1;
> >       } else {
> >               mode = MODE_ALL_PATTERNS;
> > -             if (record_mode)
> > -                     core_sparse_checkout_cone = 0;
> > +             core_sparse_checkout_cone = 0;
> >       }
>
> Ok, this "record_mode" is showing up again here, so I assume it is
> important and based on whatever is the default.

Actually, no, it was either from an earlier version of the series or
just a misunderstanding on my part.  It actually has no net effect;
I'll remove it.

> You are right that this commit is big. I think there are a few ways
> to split it up to be easier to review:
>
> 1. Make test changes to insert "--no-cone" wherever needed.
> 2. Make default switch in code and change docs only to say which is
>    the default.
> 3. Rename the sections of the document and move their current
>    contents.
> 4. Update the non-cone docs with the reasons for its deprecation.
>
> I also think that you've got two full _series_ on your hands. The
> patches 1-6 are likely easier to review quickly and get merged while
> we leave this deprecation series (this patch, split up as you see
> fit) up for discussion.

Yeah, that may make sense.  The first six patches won't have the same
dependencies on other series and might be able to be merged
independently (though there's some minor conflicts with
ds/sparse-checkout-requires-per-worktree-config).  The the last patch
will depend on 4 series -- this one, plus the other three I already
mentioned.

> I am in support of this idea, and I will make that case on your
> cover letter.

Thanks.