Re: [PATCH v2 2/9] sparse-checkout: make --cone the default

[Elijah Newren <newren@gmail.com>]

Sorry for the long delay...

On Mon, Mar 14, 2022 at 1:34 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> >  The subset of files is chosen by providing a list of directories in
> > -cone mode (which is recommended), or by providing a list of patterns
> > -in non-cone mode.
> > +cone mode (the default), or by providing a list of patterns in
> > +non-cone mode.
>
> OK.
>
> > @@ -60,7 +60,7 @@ When the `--stdin` option is provided, the directories or patterns are
> >  read from standard in as a newline-delimited list instead of from the
> >  arguments.
> >  +
> > -When `--cone` is passed or `core.sparseCheckoutCone` is enabled, the
> > +When `--cone` is passed or `core.sparseCheckoutCone` is not false, the
> >  input list is considered a list of directories.  This allows for
>
>
> I expected a change to Documentation/config/core.txt in this step,
> because the default value for core.sparseCheckoutCone becomes true
> if unspecified with this step, and the normal expectation by the
> readers is what is not explicitly set to true is either 'false' or
> 'unspecified' (when 'unspecified' has its own meaning, like in
> gitattributes).
>
> Something like this?
>
> diff --git i/Documentation/config/core.txt w/Documentation/config/core.txt
> index c04f62a54a..03cf075821 100644
> --- i/Documentation/config/core.txt
> +++ w/Documentation/config/core.txt
> @@ -615,8 +615,10 @@ core.sparseCheckout::
>
>  core.sparseCheckoutCone::
>         Enables the "cone mode" of the sparse checkout feature. When the
> -       sparse-checkout file contains a limited set of patterns, then this
> -       mode provides significant performance advantages. See
> +       sparse-checkout file contains a limited set of patterns, this
> +       mode provides significant performance advantages. The "non
> +       cone mode" can be requested to allow specifying a more flexible
> +       patterns by setting this variable to 'false'. See
>         linkgit:git-sparse-checkout[1] for more information.
>
>  core.abbrev::
>
> > -When `--no-cone` is passed or `core.sparseCheckoutCone` is not enabled,
> > +When `--no-cone` is passed or `core.sparseCheckoutCone` is false,
>
> "is set to 'false'" would make it clearer.
>
> > +If `core.sparseCheckoutCone=true` (set by default or with an explicit
> > +`--cone`), then Git will parse the sparse-checkout file expecting
>
> "Unless `core.sparseCheckoutCone` is explicitly set to `false`"
> might be clearer, but I am not sure.  It is just that I found the
> phrase "set by default" confusing.

Thanks; I applied all three suggestions.

> >       /* Set cone/non-cone mode appropriately */
> >       core_apply_sparse_checkout = 1;
> > -     if (*cone_mode == 1) {
> > +     if (*cone_mode) { /* also handles "not specified" (value of -1) */
>
> The comment is correct, but if we can make the code not to require
> such a comment, that would be even better.

I had that in v1; I'll revert back to it.