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

Commit Message

Elijah Newren March 12, 2022, 3:11 a.m. UTC

From: Elijah Newren <[e-mail]>

Make cone mode the default, and update the documentation accordingly.

Signed-off-by: Elijah Newren <[e-mail]>
---
 Documentation/git-sparse-checkout.txt | 20 ++++++++++----------
 builtin/sparse-checkout.c             |  2 +-
 2 files changed, 11 insertions(+), 11 deletions(-)

Comments

Junio C Hamano March 14, 2022, 8:34 p.m. UTC | #1

"Elijah Newren via GitGitGadget" <[e-mail]> 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.

>  	/* 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 {

Elijah Newren April 22, 2022, 2:29 a.m. UTC | #2

Sorry for the long delay...

On Mon, Mar 14, 2022 at 1:34 PM Junio C Hamano <[e-mail]> wrote:
>
> "Elijah Newren via GitGitGadget" <[e-mail]> 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.

Patch

diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
index 88e55f432f3..4ef03120797 100644
--- a/Documentation/git-sparse-checkout.txt
+++ b/Documentation/git-sparse-checkout.txt
@@ -22,8 +22,8 @@  present, or undo and go back to having all tracked files present in the
 working copy.
 
 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.
 
 When in a sparse-checkout, other Git commands behave a bit differently.
 For example, switching branches will not update paths outside the
@@ -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
 better performance with a limited set of patterns (see 'CONE PATTERN
 SET' below).  The input format matches the output of `git ls-tree
@@ -68,10 +68,9 @@  SET' below).  The input format matches the output of `git ls-tree
 double quote (") as C-style quoted strings.  Note that the set command
 will write patterns to the sparse-checkout file to include all files
 contained in those directories (recursively) as well as files that are
-siblings of ancestor directories. This may become the default in the
-future; --no-cone can be passed to request non-cone mode.
+siblings of ancestor directories.
 +
-When `--no-cone` is passed or `core.sparseCheckoutCone` is not enabled,
+When `--no-cone` is passed or `core.sparseCheckoutCone` is false,
 the input list is considered a list of patterns.  This mode is harder
 to use and less performant, and is thus not recommended.  See the
 "Sparse Checkout" section of linkgit:git-read-tree[1] and the "Pattern
@@ -227,10 +226,11 @@  patterns. The resulting sparse-checkout file is now
 Here, order matters, so the negative patterns are overridden by the positive
 patterns that appear lower in the file.
 
-If `core.sparseCheckoutCone=true`, then Git will parse the sparse-checkout file
-expecting patterns of these types. Git will warn if the patterns do not match.
-If the patterns do match the expected format, then Git will use faster hash-
-based algorithms to compute inclusion in the sparse-checkout.
+If `core.sparseCheckoutCone=true` (set by default or with an explicit
+`--cone`), then Git will parse the sparse-checkout file expecting
+patterns of these types. Git will warn if the patterns do not match.  If
+the patterns do match the expected format, then Git will use faster
+hash-based algorithms to compute inclusion in the sparse-checkout.
 
 In the cone mode case, the `git sparse-checkout list` subcommand will list the
 directories that define the recursive patterns. For the example sparse-checkout
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 5518ed47f6c..6603a0cc029 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -397,7 +397,7 @@  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) { /* also handles "not specified" (value of -1) */
 		mode = MODE_CONE_PATTERNS;
 		core_sparse_checkout_cone = 1;
 	} else {