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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 14, 2022, 20:34 UTC
Message-ID: <xmqq8rtcqnnn.fsf@gitster.g>
In-Reply-To: <a53179764bc5d411726a095a587ea728aa9a20d3.1647054681.git.gitgitgadget@gmail.com>

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 5 quoted lines

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

Show 7 quoted lines

> @@ -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.

>  	/* 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.  Are there very small
number of choke points where we always pass, after parsing
configuration variables and options, that we ought to know which
mode to use before the control comes here, and commit the choice
with "if (cone_mode < 0) cone_mode = 1"?

We see beyond precontext of this hunk an assignment to *cone_mode to
tell the choice we made to our caller.  Shouldn't we be doing the
same in this if/else that still assumes we can get "undecided"?

>  		mode = MODE_CONE_PATTERNS;
>  		core_sparse_checkout_cone = 1;
>  	} else {