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

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

[Elijah Newren via GitGitGadget]

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.

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.

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

Elijah Newren (7):
  sparse-checkout: correct reapply's handling of options
  sparse-checkout: correctly set non-cone mode when expected
  sparse-checkout: pay attention to prefix for {set, add}
  sparse-checkout: error or warn when given individual files
  sparse-checkout: reject non-cone-mode patterns starting with a '#'
  sparse-checkout: reject arguments in cone-mode that look like patterns
  sparse-checkout: make --cone the default and deprecate --no-cone

 Documentation/git-sparse-checkout.txt | 228 +++++++++++++++++++-------
 builtin/sparse-checkout.c             |  86 +++++++++-
 t/t1091-sparse-checkout-builtin.sh    |  97 ++++++++++-
 t/t3602-rm-sparse-checkout.sh         |   6 +-
 t/t3705-add-sparse-checkout.sh        |   4 +-
 t/t6428-merge-conflicts-sparse.sh     |   4 +-
 t/t7002-mv-sparse-checkout.sh         |   2 +-
 t/t7012-skip-worktree-writing.sh      |   2 +-
 8 files changed, 345 insertions(+), 84 deletions(-)


base-commit: a813c7516ab581570509aa7a9c03ca05c769caf7
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1118%2Fnewren%2Fsparse-checkout-default-and-arg-validity-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1118/newren/sparse-checkout-default-and-arg-validity-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1118
-- 
gitgitgadget

[PATCH 1/7] sparse-checkout: correct reapply's handling of options

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

Commit 4e256731d6 ("sparse-checkout: enable reapply to take
--[no-]{cone,sparse-index}", 2021-12-14) made it so that reapply could
take additional options but added no tests.  Tests would have shown that
the feature doesn't work because the initial values are set AFTER
parsing the command line options instead of before.  Add a test and set
the initial value at the appropriate time.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c          |  6 +++---
 t/t1091-sparse-checkout-builtin.sh | 31 ++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index b9f79e74969..510cb89b544 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -785,15 +785,15 @@ static int sparse_checkout_reapply(int argc, const char **argv)
 	if (!core_apply_sparse_checkout)
 		die(_("must be in a sparse-checkout to reapply sparsity patterns"));
 
+	reapply_opts.cone_mode = -1;
+	reapply_opts.sparse_index = -1;
+
 	argc = parse_options(argc, argv, NULL,
 			     builtin_sparse_checkout_reapply_options,
 			     builtin_sparse_checkout_reapply_usage, 0);
 
 	repo_read_index(the_repository);
 
-	reapply_opts.cone_mode = -1;
-	reapply_opts.sparse_index = -1;
-
 	if (update_modes(&reapply_opts.cone_mode, &reapply_opts.sparse_index))
 		return 1;
 
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 502d42d183a..4a7394f7a58 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -510,6 +510,37 @@ test_expect_failure 'sparse-checkout reapply' '
 	git -C tweak sparse-checkout disable
 '
 
+test_expect_success 'reapply can handle config options' '
+	git -C repo sparse-checkout init --cone --no-sparse-index &&
+	git -C repo config --worktree --list >actual &&
+	cat >expect <<-\EOF &&
+	core.sparsecheckout=true
+	core.sparsecheckoutcone=true
+	index.sparse=false
+	EOF
+	test_cmp expect actual &&
+
+	git -C repo sparse-checkout reapply --no-cone --no-sparse-index &&
+	git -C repo config --worktree --list >actual &&
+	cat >expect <<-\EOF &&
+	core.sparsecheckout=true
+	core.sparsecheckoutcone=false
+	index.sparse=false
+	EOF
+	test_cmp expect actual &&
+
+	git -C repo sparse-checkout reapply --cone --sparse-index &&
+	git -C repo config --worktree --list >actual &&
+	cat >expect <<-\EOF &&
+	core.sparsecheckout=true
+	core.sparsecheckoutcone=true
+	index.sparse=true
+	EOF
+	test_cmp expect actual &&
+
+	git -C repo sparse-checkout disable
+'
+
 test_expect_success 'cone mode: set with core.ignoreCase=true' '
 	rm repo/.git/info/sparse-checkout &&
 	git -C repo sparse-checkout init --cone &&
-- 
gitgitgadget

[PATCH 2/7] sparse-checkout: correctly set non-cone mode when expected

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

commit f2e3a218e8 ("sparse-checkout: enable `set` to initialize
sparse-checkout mode", 2021-12-14) made the `set` command able to
intialize sparse-checkout mode, but it also had to function when
sparse-checkout mode was already setup and the user just wanted to
change the sparsity paths.  So, if the user passed --cone or --no-cone,
then we should override the current setting, but if they didn't pass
either, we should use whatever the current cone mode setting is.

Unfortunately, there was a small error in the logic in that it would not
set the in-memory cone mode value (core_sparse_checkout_one) when
--no-cone was specified, but since it did set the config setting on
disk, any subsequent git invocation would correctly get non-cone mode.
As such, the error did not previously matter.  However, a sbusequent
commit will add some logic that depends on core_sparse_checkout_cone
being set to the correct mode, so make sure it is set consistently with
the config values we will be writing to disk.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 510cb89b544..8d595189ea3 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -399,6 +399,8 @@ static int update_modes(int *cone_mode, int *sparse_index)
 		core_sparse_checkout_cone = 1;
 	} else {
 		mode = MODE_ALL_PATTERNS;
+		if (record_mode)
+			core_sparse_checkout_cone = 0;
 	}
 	if (record_mode && set_config(mode))
 		return 1;
-- 
gitgitgadget

[PATCH 3/7] sparse-checkout: pay attention to prefix for {set, add}

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

In cone mode, non-option arguments to set & add are clearly paths, and
as such, we should pay attention to prefix.

In non-cone mode, it is not clear that folks intend to provide paths
since the inputs are gitignore-style patterns.  Paying attention to
prefix would prevent folks from doing things like
   git sparse-checkout add /.gitattributes
   git sparse-checkout add '/toplevel-dir/*'
In fact, the former will result in
   fatal: '/.gitattributes' is outside repository...
while the later will result in
   fatal: Invalid path '/toplevel-dir': No such file or directory
despite the fact that both are valid gitignore-style patterns that would
select real files if added to the sparse-checkout file.  However, these
commands can be run successfully from the toplevel directory, and many
gitignore-style patterns ARE paths, and bash completion seems to be
suggesting directories and files, so perhaps for consistency we pay
attention to the prefix?  It's not clear what is okay here, but maybe
that's yet another reason to deprecate non-cone mode as we will do later
in this series.

For now, incorporate prefix into the positional arguments for either
cone or non-cone mode.  For additional discussion of this issue, see
https://lore.kernel.org/git/29f0410e-6dfa-2e86-394d-b1fb735e7608@gmail.com/

Helped-by: Junio Hamano <gitster@pobox.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c          | 22 ++++++++++++++++++++++
 t/t1091-sparse-checkout-builtin.sh | 27 +++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 8d595189ea3..bec423d5af9 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -681,6 +681,24 @@ static int modify_pattern_list(int argc, const char **argv, int use_stdin,
 	return result;
 }
 
+static void sanitize_paths(int argc, const char **argv, const char *prefix)
+{
+	if (!argc)
+		return;
+
+	if (prefix && *prefix) {
+		/*
+		 * The args are not pathspecs, so unfortunately we
+		 * cannot imitate how cmd_add() uses parse_pathspec().
+		 */
+		int i;
+		int prefix_len = strlen(prefix);
+
+		for (i = 0; i < argc; i++)
+			argv[i] = prefix_path(prefix, prefix_len, argv[i]);
+	}
+}
+
 static char const * const builtin_sparse_checkout_add_usage[] = {
 	N_("git sparse-checkout add (--stdin | <patterns>)"),
 	NULL
@@ -708,6 +726,8 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
 			     builtin_sparse_checkout_add_usage,
 			     PARSE_OPT_KEEP_UNKNOWN);
 
+	sanitize_paths(argc, argv, prefix);
+
 	return modify_pattern_list(argc, argv, add_opts.use_stdin, ADD);
 }
 
@@ -759,6 +779,8 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
 	if (!core_sparse_checkout_cone && argc == 0) {
 		argv = default_patterns;
 		argc = default_patterns_nr;
+	} else {
+		sanitize_paths(argc, argv, prefix);
 	}
 
 	return modify_pattern_list(argc, argv, set_opts.use_stdin, REPLACE);
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 4a7394f7a58..b9e6987f5a6 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -816,4 +816,31 @@ test_expect_success 'malformed cone-mode patterns' '
 	grep "warning: disabling cone pattern matching" err
 '
 
+test_expect_success 'set from subdir pays attention to prefix' '
+	git -C repo sparse-checkout disable &&
+	git -C repo/deep sparse-checkout set --cone deeper2 ../folder1 &&
+
+	git -C repo sparse-checkout list >actual &&
+
+	cat >expect <<-\EOF &&
+	deep/deeper2
+	folder1
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'add from subdir pays attention to prefix' '
+	git -C repo sparse-checkout set --cone deep/deeper2 &&
+	git -C repo/deep sparse-checkout add deeper1/deepest ../folder1 &&
+
+	git -C repo sparse-checkout list >actual &&
+
+	cat >expect <<-\EOF &&
+	deep/deeper1/deepest
+	deep/deeper2
+	folder1
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
gitgitgadget

[PATCH 4/7] sparse-checkout: error or warn when given individual files

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

The set and add subcommands accept multiple positional arguments.
The meaning of these arguments differs slightly in the two modes:

Cone mode only accepts directories.  If given a file, it would
previously treat it as a directory, causing not just the file itself to
be included but all sibling files as well -- likely against users'
expectations.  Throw an error if the specified path is a file in the
index.  Provide a --skip-checks argument to allow users to override
(e.g. for the case when the given path IS a directory on another
branch).

Non-cone mode accepts general gitignore patterns.  However, it has an
O(N*M) performance baked into its design, where all N index files must
be matched against all M sparse-checkout patterns with EVERY call to
unpack_trees() that updates the working tree.  As such, it is important
to keep the number of patterns small, and thus we should warn users to
prefer passing directories and more generic glob patterns to get the
paths they want instead of listing each individual file.  (The
--skip-checks argument can also be used to bypass this warning.)  Also,
even when users do want to specify individual files, they will often
want to do so by providing a leading '/' (to avoid selecting the same
filename in all subdirectories), in which case this error message would
never trigger anyway.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c          | 43 +++++++++++++++++++++++++-----
 t/t1091-sparse-checkout-builtin.sh | 16 ++++++++++-
 2 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index bec423d5af9..8f8d2bd6ba5 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -1,4 +1,5 @@
 #include "builtin.h"
+#include "cache.h"
 #include "config.h"
 #include "dir.h"
 #include "parse-options.h"
@@ -681,8 +682,11 @@ static int modify_pattern_list(int argc, const char **argv, int use_stdin,
 	return result;
 }
 
-static void sanitize_paths(int argc, const char **argv, const char *prefix)
+static void sanitize_paths(int argc, const char **argv,
+			   const char *prefix, int skip_checks)
 {
+	int i;
+
 	if (!argc)
 		return;
 
@@ -691,26 +695,49 @@ static void sanitize_paths(int argc, const char **argv, const char *prefix)
 		 * The args are not pathspecs, so unfortunately we
 		 * cannot imitate how cmd_add() uses parse_pathspec().
 		 */
-		int i;
 		int prefix_len = strlen(prefix);
 
 		for (i = 0; i < argc; i++)
 			argv[i] = prefix_path(prefix, prefix_len, argv[i]);
 	}
+
+	if (skip_checks)
+		return;
+
+	for (i = 0; i < argc; i++) {
+		struct cache_entry *ce;
+		struct index_state *index = the_repository->index;
+		int pos = index_name_pos(index, argv[i], strlen(argv[i]));
+
+		if (pos < 0)
+			continue;
+		ce = index->cache[pos];
+		if (S_ISSPARSEDIR(ce->ce_mode))
+			continue;
+
+		if (core_sparse_checkout_cone)
+			die(_("\"%s\" is not a directory; to treat it as a directory anyway, rerun with --skip-checks"), argv[i]);
+		else
+			warning(_("path \"%s\" is an individual file; passing directories or less specific patterns is recommended (see NON-CONE PROBLEMS in the git-sparse-checkout manual)."), argv[i]);
+	}
 }
 
 static char const * const builtin_sparse_checkout_add_usage[] = {
-	N_("git sparse-checkout add (--stdin | <patterns>)"),
+	N_("git sparse-checkout add [--skip-checks] (--stdin | <patterns>)"),
 	NULL
 };
 
 static struct sparse_checkout_add_opts {
+	int skip_checks;
 	int use_stdin;
 } add_opts;
 
 static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
 {
 	static struct option builtin_sparse_checkout_add_options[] = {
+		OPT_BOOL_F(0, "skip-checks", &add_opts.skip_checks,
+			   N_("skip some sanity checks on the given paths that might give false positives"),
+			   PARSE_OPT_NONEG),
 		OPT_BOOL(0, "stdin", &add_opts.use_stdin,
 			 N_("read patterns from standard in")),
 		OPT_END(),
@@ -726,19 +753,20 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
 			     builtin_sparse_checkout_add_usage,
 			     PARSE_OPT_KEEP_UNKNOWN);
 
-	sanitize_paths(argc, argv, prefix);
+	sanitize_paths(argc, argv, prefix, add_opts.skip_checks);
 
 	return modify_pattern_list(argc, argv, add_opts.use_stdin, ADD);
 }
 
 static char const * const builtin_sparse_checkout_set_usage[] = {
-	N_("git sparse-checkout set [--[no-]cone] [--[no-]sparse-index] (--stdin | <patterns>)"),
+	N_("git sparse-checkout set [--[no-]cone] [--[no-]sparse-index] [--skip-checks] (--stdin | <patterns>)"),
 	NULL
 };
 
 static struct sparse_checkout_set_opts {
 	int cone_mode;
 	int sparse_index;
+	int skip_checks;
 	int use_stdin;
 } set_opts;
 
@@ -752,6 +780,9 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
 			 N_("initialize the sparse-checkout in cone mode")),
 		OPT_BOOL(0, "sparse-index", &set_opts.sparse_index,
 			 N_("toggle the use of a sparse index")),
+		OPT_BOOL_F(0, "skip-checks", &set_opts.skip_checks,
+			   N_("skip some sanity checks on the given paths that might give false positives"),
+			   PARSE_OPT_NONEG),
 		OPT_BOOL_F(0, "stdin", &set_opts.use_stdin,
 			   N_("read patterns from standard in"),
 			   PARSE_OPT_NONEG),
@@ -780,7 +811,7 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
 		argv = default_patterns;
 		argc = default_patterns_nr;
 	} else {
-		sanitize_paths(argc, argv, prefix);
+		sanitize_paths(argc, argv, prefix, set_opts.skip_checks);
 	}
 
 	return modify_pattern_list(argc, argv, set_opts.use_stdin, REPLACE);
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index b9e6987f5a6..1d95fa47258 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -580,7 +580,7 @@ test_expect_success 'different sparse-checkouts with worktrees' '
 '
 
 test_expect_success 'set using filename keeps file on-disk' '
-	git -C repo sparse-checkout set a deep &&
+	git -C repo sparse-checkout set --skip-checks a deep &&
 	cat >expect <<-\EOF &&
 	/*
 	!/*/
@@ -843,4 +843,18 @@ test_expect_success 'add from subdir pays attention to prefix' '
 	test_cmp expect actual
 '
 
+test_expect_success 'by default, cone mode will error out when passed files' '
+	git -C repo sparse-checkout reapply --cone &&
+	test_must_fail git -C repo sparse-checkout add .gitignore 2>error &&
+
+	grep ".gitignore.*is not a directory" error
+'
+
+test_expect_success 'by default, non-cone mode will warn on individual files' '
+	git -C repo sparse-checkout reapply --no-cone &&
+	git -C repo sparse-checkout add .gitignore 2>warning &&
+
+	grep "passing directories or less specific patterns is recommended" warning
+'
+
 test_done
-- 
gitgitgadget

[PATCH 5/7] sparse-checkout: reject non-cone-mode patterns starting with a '#'

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

In sparse-checkout add/set, in cone mode any specified directories will
be transformed into appropriate patterns.  In non-cone mode, the
non-option arguments are treated as patterns.

Since .git/info/sparse-checkout will ignore any patterns starting with a
'#' (they are just gitignore patterns), if the user passes an argument
starting with a '#' to sparse-checkout add/set in non-cone mode, it
would just be treated as a comment and ignored.  Error out in such
cases, informing the user that they need to backslash escape it.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c          | 5 +++++
 t/t1091-sparse-checkout-builtin.sh | 6 ++++++
 2 files changed, 11 insertions(+)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 8f8d2bd6ba5..0f9e737ed97 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -704,6 +704,11 @@ static void sanitize_paths(int argc, const char **argv,
 	if (skip_checks)
 		return;
 
+	if (!core_sparse_checkout_cone)
+		for (i = 0; i < argc; i++)
+			if (argv[i][0] == '#')
+				die(_("paths beginning with a '#' must be preceeded by a backslash"));
+
 	for (i = 0; i < argc; i++) {
 		struct cache_entry *ce;
 		struct index_state *index = the_repository->index;
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 1d95fa47258..32b77415679 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -857,4 +857,10 @@ test_expect_success 'by default, non-cone mode will warn on individual files' '
 	grep "passing directories or less specific patterns is recommended" warning
 '
 
+test_expect_success 'paths starting with hash must be escaped in non-cone mode' '
+	test_must_fail git -C repo sparse-checkout set --no-cone "#funny-path" 2>error &&
+
+	grep "paths beginning.*#.*must be preceeded by a backslash" error
+'
+
 test_done
-- 
gitgitgadget

[PATCH 6/7] sparse-checkout: reject arguments in cone-mode that look like patterns

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

In sparse-checkout add/set under cone mode, the arguments passed are
supposed to be directories rather than gitignore-style patterns.
However, given the amount of effort spent in the manual discussing
patterns, it is easy for users to assume they need to pass patterns such
as
   /foo/*
or
   !/bar/*/
or perhaps they really do ignore the directory rule and specify a
random gitignore-style pattern like
   *.c

To help catch such mistakes, throw an error if any of the positional
arguments:
  * starts with any of '/!'
  * contains any of '*\?[]'

Inform users they can pass --skip-checks if they have a directory that
really does have such special characters in its name.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c          | 16 ++++++++++++++--
 t/t1091-sparse-checkout-builtin.sh |  2 +-
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 0f9e737ed97..6e0af166f80 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -704,10 +704,22 @@ static void sanitize_paths(int argc, const char **argv,
 	if (skip_checks)
 		return;
 
-	if (!core_sparse_checkout_cone)
-		for (i = 0; i < argc; i++)
+	for (i = 0; i < argc; i++) {
+		if (core_sparse_checkout_cone) {
+			if (argv[i][0] == '/')
+				die(_("specify directories rather than patterns (no leading slash)"));
+			if (argv[i][0] == '!')
+				die(_("specify directories rather than patterns.  If your directory starts with a '!', pass --skip-checks"));
+			if (strchr(argv[i], '*') ||
+			    strchr(argv[i], '?') ||
+			    strchr(argv[i], '[') ||
+			    strchr(argv[i], ']'))
+				die(_("specify directories rather than patterns.  If your directory really has any of '*?[]' in it, pass --skip-checks"));
+		} else {
 			if (argv[i][0] == '#')
 				die(_("paths beginning with a '#' must be preceeded by a backslash"));
+		}
+	}
 
 	for (i = 0; i < argc; i++) {
 		struct cache_entry *ce;
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 32b77415679..343ce16a534 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -691,7 +691,7 @@ test_expect_success BSLASHPSPEC 'pattern-checks: escaped characters' '
 	git -C escaped reset --hard $COMMIT &&
 	check_files escaped "a deep folder1 folder2 zbad\\dir zdoes*exist" zglob[!a]? &&
 	git -C escaped sparse-checkout init --cone &&
-	git -C escaped sparse-checkout set zbad\\dir/bogus "zdoes*not*exist" "zdoes*exist" "zglob[!a]?" &&
+	git -C escaped sparse-checkout set --skip-checks zbad\\dir/bogus "zdoes*not*exist" "zdoes*exist" "zglob[!a]?" &&
 	cat >expect <<-\EOF &&
 	/*
 	!/*/
-- 
gitgitgadget

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

[Elijah Newren via GitGitGadget]

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.

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-sparse-checkout.txt | 228 +++++++++++++++++++-------
 builtin/sparse-checkout.c             |   8 +-
 t/t1091-sparse-checkout-builtin.sh    |  15 +-
 t/t3602-rm-sparse-checkout.sh         |   6 +-
 t/t3705-add-sparse-checkout.sh        |   4 +-
 t/t6428-merge-conflicts-sparse.sh     |   4 +-
 t/t7002-mv-sparse-checkout.sh         |   2 +-
 t/t7012-skip-worktree-writing.sh      |   2 +-
 8 files changed, 190 insertions(+), 79 deletions(-)

diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
index 88e55f432f3..abe8c796313 100644
--- a/Documentation/git-sparse-checkout.txt
+++ b/Documentation/git-sparse-checkout.txt
@@ -42,11 +42,11 @@ COMMANDS
 
 'set'::
 	Enable the necessary sparse-checkout config settings
-	(`core.sparseCheckout`, `core.sparseCheckoutCone`, and
-	`index.sparse`) if they are not already set to the desired values,
-	and write a set of patterns to the sparse-checkout file from the
-	list of arguments following the 'set' subcommand. Update the
-	working directory to match the new patterns.
+	(`extensions.worktreeConfig`, `core.sparseCheckout`,
+	`core.sparseCheckoutCone`, and `index.sparse`) if they are not
+	already set to the desired values, populate the sparse-checkout
+	file from the list of arguments following the 'set' subcommand,
+	and update the working directory to match.
 +
 To ensure that adjusting the sparse-checkout settings within a worktree
 does not alter the sparse-checkout settings in other worktrees, the 'set'
@@ -60,22 +60,24 @@ 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
-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
---name-only`.  This includes interpreting pathnames that begin with a
-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.
+By default, the input list is considered a list of directories, matching
+the output of `git ls-tree -d --name-only`.  This includes interpreting
+pathnames that begin with a double quote (") as C-style quoted strings.
+Note that all files under the specified directories (at any depth) will
+be included in the sparse checkout, as well as files that are siblings
+of either the given directory or any of its ancestors (see 'CONE PATTERN
+SET' below for more details).  In the past, this was not the default,
+and `--cone` needed to be specified or `core.sparseCheckoutCone` needed
+to be enabled.
 +
-When `--no-cone` is passed or `core.sparseCheckoutCone` is not enabled,
-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
-Set" sections below for more details.
+When `--no-cone` is passed, the input list is considered a list of
+patterns.  This mode is harder to use, and unless you can keep the
+number of patterns very small, its design also scales quite poorly.  It
+used to be the default mode, but we do not recommend using it.  It does
+not work with the `--sparse-index` option, and will likely be
+incompatible with other new features as they are added.  See the
+"Non-cone Problems" section below and the "Sparse Checkout" section of
+linkgit:git-read-tree[1] for more details.
 +
 Use the `--[no-]sparse-index` option to use a sparse index (the
 default is to not use it).  A sparse index reduces the size of the
@@ -137,8 +139,44 @@ paths to pass to a subsequent 'set' or 'add' command.  However,
 the disable command, so the easy restore of calling a plain `init`
 decreased in utility.
 
-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
+	directory).  If already in a sparse checkout, change which files
+	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
+----------------------------
 
 "Sparse checkout" allows populating the working directory sparsely.  It
 uses the skip-worktree bit (see linkgit:git-update-index[1]) to tell Git
@@ -155,22 +193,93 @@ directory, it updates the skip-worktree bits in the index based
 on this file. The files matching the patterns in the file will
 appear in the working directory, and the rest will not.
 
-To enable the sparse-checkout feature, run `git sparse-checkout set` to
-set the patterns you want to use.
-
-To repopulate the working directory with all files, use the
-`git sparse-checkout disable` command.
-
-
-FULL PATTERN SET
-----------------
-
-By default, the sparse-checkout file uses the same syntax as `.gitignore`
-files.
-
-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 remove the file `unwanted`:
+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:
+
+  * Fundamentally, it makes various worktree-updating processes (pull,
+    merge, rebase, switch, reset, checkout, etc.) require O(N*M) pattern
+    matches, where N is the number of patterns and M is the number of
+    paths in the index.  This scales poorly.
+
+  * Avoiding the scaling issue means selecting paths via leading
+    directory name or glob.  Passing globs on the command line is
+    error-prone as users may forget to quote the glob, causing the
+    shell to expand it into all matching files and pass them all
+    individually along to sparse-checkout set/add.  This both
+    exacerbates the scaling problem, and hardcodes the list of
+    selected files to those which were present at the time the initial
+    set/add subcommand was run (and thus ignoring other files matching
+    the same glob which come into the working tree after switching
+    branches or pulling down updates).
+
+  * It uses "ignore"/"exclude" syntax for selecting what to "include",
+    which periodically causes confusion.
+
+  * It introduces inconsistencies in the Git command line, since other
+    commands use pathspecs, but sparse-checkout (in non-cone mode) uses
+    gitignore patterns.
+
+  * It has edge cases where the "right" behavior is unclear.  Two examples:
+
+    First, two users are in a subdirectory, and the first runs
+       git sparse-checkout set '/toplevel-dir/*.c'
+    while the second runs
+       git sparse-checkout set relative-dir
+    Should those arguments be transliterated into
+       current/subdirectory/toplevel-dir/*.c
+    and
+       current/subdirectory/relative-dir
+    before inserting into the sparse-checkout file?  The user who typed
+    the first command is probably aware that arguments to set/add are
+    supposed to be patterns in non-cone mode, and probably would not be
+    happy with such a transliteration.  However, many gitignore-style
+    patterns are just paths, which might be what the user who typed the
+    second command was thinking, and they'd be upset if their argument
+    wasn't transliterated.
+
+    Second, what should bash-completion complete on for set/add commands
+    for non-cone users?  If it suggests paths, is it exacerbating the
+    problem above?  Also, if it suggests paths, what if the user has a
+    file or directory that begins with either a '!' or '#' or has a '*',
+    '\', '?', '[', or ']' in its name?  And if it suggests paths, will
+    it complete "/pro" to "/proc" (in the root filesytem) rather than to
+    "/progress.txt" in the current directory?  (Note that users are
+    likely to want to start paths with a leading '/' in non-cone mode,
+    for the same reason that .gitignore files often have one.)
+    Completing on files or directories might give nasty surprises in
+    all these cases.
+
+  * The excessive flexibility made other extensions essentially
+    impractical.  `--sparse-index` may not have been feasible in
+    non-cone mode, but even if it was, it would have been far more work
+    to implement and may have been too slow in practice.  Some ideas for
+    adding coupling between partial clones and sparse-checkouts are only
+    practical with a more restricted set of paths.
+
+For all these reasons, non-cone mode is deprecated.  Please switch to
+using cone mode.
+
+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`:
 
 ----------------
 /*
@@ -178,14 +287,14 @@ using negative patterns. For example, to remove the file `unwanted`:
 ----------------
 
 
-CONE PATTERN SET
-----------------
+INTERNALS -- CONE PATTERN SET
+-----------------------------
 
-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
+of patterns and M is the number of paths in the index. To combat this
+performance issue, a more restricted pattern set is used by default.
 
 The accepted patterns in the cone pattern set are:
 
@@ -197,9 +306,9 @@ In addition to the above two patterns, we also expect that all files in the
 root directory are included. If a recursive pattern is added, then all
 leading directories are added as parent patterns.
 
-By default, when running `git sparse-checkout init`, the root directory is
-added as a parent pattern. At this point, the sparse-checkout file contains
-the following patterns:
+By default, when running `git sparse-checkout set` with no directories
+specified, the root directory is added as a parent pattern. At this
+point, the sparse-checkout file contains the following patterns:
 
 ----------------
 /*
@@ -208,11 +317,11 @@ the following patterns:
 
 This says "include everything in root, but nothing two levels below root."
 
-When in cone mode, the `git sparse-checkout set` subcommand takes a list of
-directories instead of a list of sparse-checkout patterns. In this mode,
-the command `git sparse-checkout set A/B/C` sets the directory `A/B/C` as
-a recursive pattern, the directories `A` and `A/B` are added as parent
-patterns. The resulting sparse-checkout file is now
+When in cone mode, the `git sparse-checkout set` subcommand takes a list
+of directories. In this mode, the command `git sparse-checkout set
+A/B/C` sets the directory `A/B/C` as a recursive pattern, the
+directories `A` and `A/B` are added as parent patterns. The resulting
+sparse-checkout file is now
 
 ----------------
 /*
@@ -227,10 +336,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
@@ -257,8 +367,8 @@ use `git add` and `git commit` to store them, then remove any remaining files
 manually to ensure Git can behave optimally.
 
 
-SUBMODULES
-----------
+INTERNALS -- SUBMODULES
+-----------------------
 
 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;
 	}
 	if (record_mode && set_config(mode))
 		return 1;
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 343ce16a534..5e1a6d91411 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -72,7 +72,7 @@ test_expect_success 'git sparse-checkout list (populated)' '
 '
 
 test_expect_success 'git sparse-checkout init' '
-	git -C repo sparse-checkout init &&
+	git -C repo sparse-checkout init --no-cone &&
 	cat >expect <<-\EOF &&
 	/*
 	!/*/
@@ -111,6 +111,7 @@ test_expect_success 'init with existing sparse-checkout' '
 
 test_expect_success 'clone --sparse' '
 	git clone --sparse "file://$(pwd)/repo" clone &&
+	git -C clone sparse-checkout reapply --no-cone &&
 	git -C clone sparse-checkout list >actual &&
 	cat >expect <<-\EOF &&
 	/*
@@ -124,7 +125,7 @@ test_expect_success 'switching to cone mode with non-cone mode patterns' '
 	git init bad-patterns &&
 	(
 		cd bad-patterns &&
-		git sparse-checkout init &&
+		git sparse-checkout init --no-cone &&
 		git sparse-checkout add dir &&
 		git config --worktree core.sparseCheckoutCone true &&
 		test_must_fail git sparse-checkout add dir 2>err &&
@@ -402,7 +403,7 @@ test_expect_success 'revert to old sparse-checkout on empty update' '
 		git sparse-checkout set nothing 2>err &&
 		test_i18ngrep ! "Sparse checkout leaves no entry on working directory" err &&
 		test_i18ngrep ! ".git/index.lock" err &&
-		git sparse-checkout set file
+		git sparse-checkout set --no-cone file
 	)
 '
 
@@ -424,7 +425,7 @@ test_expect_success 'sparse-checkout (init|set|disable) warns with dirty status'
 	git clone repo dirty &&
 	echo dirty >dirty/folder1/a &&
 
-	git -C dirty sparse-checkout init 2>err &&
+	git -C dirty sparse-checkout init --no-cone 2>err &&
 	test_i18ngrep "warning.*The following paths are not up to date" err &&
 
 	git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* 2>err &&
@@ -435,7 +436,7 @@ test_expect_success 'sparse-checkout (init|set|disable) warns with dirty status'
 	test_must_be_empty err &&
 
 	git -C dirty reset --hard &&
-	git -C dirty sparse-checkout init &&
+	git -C dirty sparse-checkout init --no-cone &&
 	git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
 	test_path_is_missing dirty/folder1/a &&
 	git -C dirty sparse-checkout disable &&
@@ -451,7 +452,7 @@ test_expect_success 'sparse-checkout (init|set|disable) warns with unmerged stat
 	EOF
 	git -C unmerged update-index --index-info <input &&
 
-	git -C unmerged sparse-checkout init 2>err &&
+	git -C unmerged sparse-checkout init --no-cone 2>err &&
 	test_i18ngrep "warning.*The following paths are unmerged" err &&
 
 	git -C unmerged sparse-checkout set /folder2/* /deep/deeper1/* 2>err &&
@@ -462,7 +463,7 @@ test_expect_success 'sparse-checkout (init|set|disable) warns with unmerged stat
 	test_i18ngrep "warning.*The following paths are unmerged" err &&
 
 	git -C unmerged reset --hard &&
-	git -C unmerged sparse-checkout init &&
+	git -C unmerged sparse-checkout init --no-cone &&
 	git -C unmerged sparse-checkout set /folder2/* /deep/deeper1/* &&
 	git -C unmerged sparse-checkout disable
 '
diff --git a/t/t3602-rm-sparse-checkout.sh b/t/t3602-rm-sparse-checkout.sh
index 034ec010910..08580fd3dcc 100755
--- a/t/t3602-rm-sparse-checkout.sh
+++ b/t/t3602-rm-sparse-checkout.sh
@@ -30,7 +30,7 @@ test_expect_success 'setup' "
 for opt in "" -f --dry-run
 do
 	test_expect_success "rm${opt:+ $opt} does not remove sparse entries" '
-		git sparse-checkout set a &&
+		git sparse-checkout set --no-cone a &&
 		test_must_fail git rm $opt b 2>stderr &&
 		test_cmp b_error_and_hint stderr &&
 		git ls-files --error-unmatch b
@@ -118,7 +118,7 @@ test_expect_success 'can remove files from non-sparse dir' '
 	test_commit w/f &&
 	test_commit x/y/f &&
 
-	git sparse-checkout set w !/x y/ &&
+	git sparse-checkout set --no-cone w !/x y/ &&
 	git rm w/f.t x/y/f.t 2>stderr &&
 	test_must_be_empty stderr
 '
@@ -128,7 +128,7 @@ test_expect_success 'refuse to remove non-skip-worktree file from sparse dir' '
 	git sparse-checkout disable &&
 	mkdir -p x/y/z &&
 	test_commit x/y/z/f &&
-	git sparse-checkout set !/x y/ !x/y/z &&
+	git sparse-checkout set --no-cone !/x y/ !x/y/z &&
 
 	git update-index --no-skip-worktree x/y/z/f.t &&
 	test_must_fail git rm x/y/z/f.t 2>stderr &&
diff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh
index 95609046c61..2bade9e804f 100755
--- a/t/t3705-add-sparse-checkout.sh
+++ b/t/t3705-add-sparse-checkout.sh
@@ -166,7 +166,7 @@ test_expect_success 'do not warn when pathspec matches dense entries' '
 test_expect_success 'git add fails outside of sparse-checkout definition' '
 	test_when_finished git sparse-checkout disable &&
 	test_commit a &&
-	git sparse-checkout init &&
+	git sparse-checkout init --no-cone &&
 	git sparse-checkout set a &&
 	echo >>sparse_entry &&
 
@@ -208,7 +208,7 @@ test_expect_success 'add obeys advice.updateSparsePath' '
 '
 
 test_expect_success 'add allows sparse entries with --sparse' '
-	git sparse-checkout set a &&
+	git sparse-checkout set --no-cone a &&
 	echo modified >sparse_entry &&
 	test_must_fail git add sparse_entry &&
 	test_sparse_entry_unchanged &&
diff --git a/t/t6428-merge-conflicts-sparse.sh b/t/t6428-merge-conflicts-sparse.sh
index 142c9aaabc5..064be1b629e 100755
--- a/t/t6428-merge-conflicts-sparse.sh
+++ b/t/t6428-merge-conflicts-sparse.sh
@@ -87,7 +87,7 @@ test_expect_success 'conflicting entries written to worktree even if sparse' '
 		test_path_is_file numerals &&
 
 		git sparse-checkout init &&
-		git sparse-checkout set README &&
+		git sparse-checkout set --no-cone README &&
 
 		test_path_is_file README &&
 		test_path_is_missing numerals &&
@@ -123,7 +123,7 @@ test_expect_success 'present-despite-SKIP_WORKTREE handled reasonably' '
 		test_path_is_file numerals &&
 
 		git sparse-checkout init &&
-		git sparse-checkout set README &&
+		git sparse-checkout set --no-cone README &&
 
 		test_path_is_file README &&
 		test_path_is_missing numerals &&
diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
index 1d3d2aca21c..f0f7cbfcdb7 100755
--- a/t/t7002-mv-sparse-checkout.sh
+++ b/t/t7002-mv-sparse-checkout.sh
@@ -27,7 +27,7 @@ test_expect_success 'setup' "
 test_expect_success 'mv refuses to move sparse-to-sparse' '
 	test_when_finished rm -f e &&
 	git reset --hard &&
-	git sparse-checkout set a &&
+	git sparse-checkout set --no-cone a &&
 	touch b &&
 	test_must_fail git mv b e 2>stderr &&
 	cat sparse_error_header >expect &&
diff --git a/t/t7012-skip-worktree-writing.sh b/t/t7012-skip-worktree-writing.sh
index cb9f1a6981e..cd5c20fe51b 100755
--- a/t/t7012-skip-worktree-writing.sh
+++ b/t/t7012-skip-worktree-writing.sh
@@ -151,7 +151,7 @@ test_expect_success 'stash restore in sparse checkout' '
 
 		git stash push &&
 
-		git sparse-checkout set subdir &&
+		git sparse-checkout set --no-cone subdir &&
 
 		# Ensure after sparse-checkout we only have expected files
 		cat >expect <<-EOF &&
-- 
gitgitgadget

Re: [PATCH 2/7] sparse-checkout: correctly set non-cone mode when expected

[Derrick Stolee]

On 2/12/2022 7:39 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> commit f2e3a218e8 ("sparse-checkout: enable `set` to initialize
> sparse-checkout mode", 2021-12-14) made the `set` command able to
> intialize sparse-checkout mode, but it also had to function when

s/intialize/initialize/

> sparse-checkout mode was already setup and the user just wanted to
> change the sparsity paths.  So, if the user passed --cone or --no-cone,
> then we should override the current setting, but if they didn't pass
> either, we should use whatever the current cone mode setting is.
> 
> Unfortunately, there was a small error in the logic in that it would not
> set the in-memory cone mode value (core_sparse_checkout_one) when
> --no-cone was specified, but since it did set the config setting on
> disk, any subsequent git invocation would correctly get non-cone mode.
> As such, the error did not previously matter.  However, a sbusequent

s/sbusequent/subsequent/

> commit will add some logic that depends on core_sparse_checkout_cone
> being set to the correct mode, so make sure it is set consistently with
> the config values we will be writing to disk.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  builtin/sparse-checkout.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index 510cb89b544..8d595189ea3 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -399,6 +399,8 @@ static int update_modes(int *cone_mode, int *sparse_index)
>  		core_sparse_checkout_cone = 1;
>  	} else {
>  		mode = MODE_ALL_PATTERNS;
> +		if (record_mode)
> +			core_sparse_checkout_cone = 0;

Is there a special reason why this is guarded by "if (record_mode)"?

Thanks,
-Stolee

Re: [PATCH 3/7] sparse-checkout: pay attention to prefix for {set, add}

[Derrick Stolee]

On 2/12/2022 7:39 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> In cone mode, non-option arguments to set & add are clearly paths, and
> as such, we should pay attention to prefix.
> 
> In non-cone mode, it is not clear that folks intend to provide paths
> since the inputs are gitignore-style patterns.  Paying attention to
> prefix would prevent folks from doing things like
>    git sparse-checkout add /.gitattributes
>    git sparse-checkout add '/toplevel-dir/*'
> In fact, the former will result in
>    fatal: '/.gitattributes' is outside repository...
> while the later will result in
>    fatal: Invalid path '/toplevel-dir': No such file or directory
> despite the fact that both are valid gitignore-style patterns that would
> select real files if added to the sparse-checkout file.  However, these
> commands can be run successfully from the toplevel directory, and many
> gitignore-style patterns ARE paths, and bash completion seems to be
> suggesting directories and files, so perhaps for consistency we pay
> attention to the prefix?  It's not clear what is okay here, but maybe
> that's yet another reason to deprecate non-cone mode as we will do later
> in this series.
> 
> For now, incorporate prefix into the positional arguments for either
> cone or non-cone mode.  For additional discussion of this issue, see
> https://lore.kernel.org/git/29f0410e-6dfa-2e86-394d-b1fb735e7608@gmail.com/

Perhaps this was covered in the issue, but for non-cone mode, it
matters if there is a leading slash or not in the pattern. Will
this change make it impossible for a user to input that distinction?

Will there still be a difference between:

	git sparse-checkout set --no-cone /.vs/

and

	git sparse-checkout set --no-cone .vs/

?

> Helped-by: Junio Hamano <gitster@pobox.com>
> Signed-off-by: Elijah Newren <newren@gmail.com>

This could probably use a

  Reported-by: Lessley Dennington <lessleydennington@gmail.com>

> +static void sanitize_paths(int argc, const char **argv, const char *prefix)
> +{
> +	if (!argc)
> +		return;
> +
> +	if (prefix && *prefix) {
> +		/*
> +		 * The args are not pathspecs, so unfortunately we
> +		 * cannot imitate how cmd_add() uses parse_pathspec().
> +		 */
> +		int i;
> +		int prefix_len = strlen(prefix);
> +
> +		for (i = 0; i < argc; i++)
> +			argv[i] = prefix_path(prefix, prefix_len, argv[i]);
> +	}
> +}
> +
>  static char const * const builtin_sparse_checkout_add_usage[] = {
>  	N_("git sparse-checkout add (--stdin | <patterns>)"),
>  	NULL
> @@ -708,6 +726,8 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
>  			     builtin_sparse_checkout_add_usage,
>  			     PARSE_OPT_KEEP_UNKNOWN);
>  
> +	sanitize_paths(argc, argv, prefix);
> +
>  	return modify_pattern_list(argc, argv, add_opts.use_stdin, ADD);
>  }
>  
> @@ -759,6 +779,8 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
>  	if (!core_sparse_checkout_cone && argc == 0) {
>  		argv = default_patterns;
>  		argc = default_patterns_nr;
> +	} else {
> +		sanitize_paths(argc, argv, prefix);
>  	}

Code changes appear to do as described: apply the prefix everywhere
it matters, no matter the mode.

> +test_expect_success 'set from subdir pays attention to prefix' '
> +	git -C repo sparse-checkout disable &&
> +	git -C repo/deep sparse-checkout set --cone deeper2 ../folder1 &&
> +
> +	git -C repo sparse-checkout list >actual &&
> +
> +	cat >expect <<-\EOF &&
> +	deep/deeper2
> +	folder1
> +	EOF
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'add from subdir pays attention to prefix' '
> +	git -C repo sparse-checkout set --cone deep/deeper2 &&
> +	git -C repo/deep sparse-checkout add deeper1/deepest ../folder1 &&
> +
> +	git -C repo sparse-checkout list >actual &&
> +
> +	cat >expect <<-\EOF &&
> +	deep/deeper1/deepest
> +	deep/deeper2
> +	folder1
> +	EOF
> +	test_cmp expect actual
> +'
> +
>  test_done

These tests could use a non-cone-mode version to demonstrate the behavior
in that mode.

Thanks,
-Stolee

Re: [PATCH 4/7] sparse-checkout: error or warn when given individual files

[Derrick Stolee]

On 2/12/2022 7:39 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> The set and add subcommands accept multiple positional arguments.
> The meaning of these arguments differs slightly in the two modes:
> 
> Cone mode only accepts directories.  If given a file, it would
> previously treat it as a directory, causing not just the file itself to
> be included but all sibling files as well -- likely against users'
> expectations.  Throw an error if the specified path is a file in the
> index.  Provide a --skip-checks argument to allow users to override
> (e.g. for the case when the given path IS a directory on another
> branch).

I agree that this is likely to be an improvement for users. The
sparse-checkout builtin isn't integrated with the sparse index
yet. At least not integrated upstream: we have commits in microsoft/git
that we plan to send when other things in flight are merged. This
change likely introduces a new opportunity for the index to expand,
so I will keep that in mind when upstreaming.
 
> Non-cone mode accepts general gitignore patterns.  However, it has an
> O(N*M) performance baked into its design, where all N index files must
> be matched against all M sparse-checkout patterns with EVERY call to
> unpack_trees() that updates the working tree.  As such, it is important
> to keep the number of patterns small, and thus we should warn users to
> prefer passing directories and more generic glob patterns to get the
> paths they want instead of listing each individual file.  (The
> --skip-checks argument can also be used to bypass this warning.)  Also,
> even when users do want to specify individual files, they will often
> want to do so by providing a leading '/' (to avoid selecting the same
> filename in all subdirectories), in which case this error message would
> never trigger anyway.

I think the case of "I want only one file from this directory" and "I
want files with the given name pattern" are the main reason to still
use non-cone-mode. Users with this need usually have a directory full
of large files, and they choose which of those large files they need
using sparse-checkout. The repository reorganization required to use
cone mode for this use is perhaps too great (or they haven't thought
about doing it). For this reason, I would prefer that we do not do
these checks when not in cone mode.

> +test_expect_success 'by default, cone mode will error out when passed files' '
> +	git -C repo sparse-checkout reapply --cone &&
> +	test_must_fail git -C repo sparse-checkout add .gitignore 2>error &&
> +
> +	grep ".gitignore.*is not a directory" error
> +'
> +
> +test_expect_success 'by default, non-cone mode will warn on individual files' '
> +	git -C repo sparse-checkout reapply --no-cone &&
> +	git -C repo sparse-checkout add .gitignore 2>warning &&
> +
> +	grep "passing directories or less specific patterns is recommended" warning
> +'

So I would expect this second test to have

	test_must_be_empty warning

to show that no warning occurs when specifying a file in non-cone-mode.

Thanks,
-Stolee

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

[Derrick Stolee]

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"?

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

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.

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

Thanks,
-Stolee

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

[Derrick Stolee]

On 2/12/2022 7:39 PM, Elijah Newren via GitGitGadget wrote:
> 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.)

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

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

Re: [PATCH 5/7] sparse-checkout: reject non-cone-mode patterns starting with a '#'

[Junio C Hamano]

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

> +	if (!core_sparse_checkout_cone)
> +		for (i = 0; i < argc; i++)
> +			if (argv[i][0] == '#')
> +				die(_("paths beginning with a '#' must be preceeded by a backslash"));
> +

Whenever I see an error message like this, where it is clear that
the command knows the only viable solution is to the issue, and yet
still refuses to do-what-the-user-clearly-meant-to-do (is there a
valid reason to copy and paste "# comment" line, which clearly is
not about choosing which paths to use/ignore, from an existing file
and feed it to the command?), I question if it should be solved the
opposite way.

That is, to pretend as if "\" + argv[i] was given and then give the
user either a warning saying what we did, or an unsquelcheable advice
message (no need for advice.* config---the user can avoid triggering
it by learning what the advice message would say, which is to use \#
when they mean to give a pattern that begins with a pound).


>  	for (i = 0; i < argc; i++) {
>  		struct cache_entry *ce;
>  		struct index_state *index = the_repository->index;
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index 1d95fa47258..32b77415679 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -857,4 +857,10 @@ test_expect_success 'by default, non-cone mode will warn on individual files' '
>  	grep "passing directories or less specific patterns is recommended" warning
>  '
>  
> +test_expect_success 'paths starting with hash must be escaped in non-cone mode' '
> +	test_must_fail git -C repo sparse-checkout set --no-cone "#funny-path" 2>error &&
> +
> +	grep "paths beginning.*#.*must be preceeded by a backslash" error
> +'
> +
>  test_done

Re: [PATCH 2/7] sparse-checkout: correctly set non-cone mode when expected

[Elijah Newren]

On Mon, Feb 14, 2022 at 7:44 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>
> >
> > commit f2e3a218e8 ("sparse-checkout: enable `set` to initialize
> > sparse-checkout mode", 2021-12-14) made the `set` command able to
> > intialize sparse-checkout mode, but it also had to function when
>
> s/intialize/initialize/

Thanks.

> > sparse-checkout mode was already setup and the user just wanted to
> > change the sparsity paths.  So, if the user passed --cone or --no-cone,
> > then we should override the current setting, but if they didn't pass
> > either, we should use whatever the current cone mode setting is.
> >
> > Unfortunately, there was a small error in the logic in that it would not
> > set the in-memory cone mode value (core_sparse_checkout_one) when
> > --no-cone was specified, but since it did set the config setting on
> > disk, any subsequent git invocation would correctly get non-cone mode.
> > As such, the error did not previously matter.  However, a sbusequent
>
> s/sbusequent/subsequent/

Likewise.

> > commit will add some logic that depends on core_sparse_checkout_cone
> > being set to the correct mode, so make sure it is set consistently with
> > the config values we will be writing to disk.
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >  builtin/sparse-checkout.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> > index 510cb89b544..8d595189ea3 100644
> > --- a/builtin/sparse-checkout.c
> > +++ b/builtin/sparse-checkout.c
> > @@ -399,6 +399,8 @@ static int update_modes(int *cone_mode, int *sparse_index)
> >               core_sparse_checkout_cone = 1;
> >       } else {
> >               mode = MODE_ALL_PATTERNS;
> > +             if (record_mode)
> > +                     core_sparse_checkout_cone = 0;
>
> Is there a special reason why this is guarded by "if (record_mode)"?

It turns out it's not needed.

I was worried about changing core_sparse_checkout_cone when we got to
this function without either --cone or --no-cone mode being specified,
but re-checking the code, in such a case *cone_mode would have been
set to core_sparse_checkout_cone a few lines above, so setting
core_sparse_checkout_cone unconditionally here is safe.  I will remove
the check.

Re: [PATCH 3/7] sparse-checkout: pay attention to prefix for {set, add}

[Elijah Newren]

On Mon, Feb 14, 2022 at 7:50 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>
> >
> > In cone mode, non-option arguments to set & add are clearly paths, and
> > as such, we should pay attention to prefix.
> >
> > In non-cone mode, it is not clear that folks intend to provide paths
> > since the inputs are gitignore-style patterns.  Paying attention to
> > prefix would prevent folks from doing things like
> >    git sparse-checkout add /.gitattributes
> >    git sparse-checkout add '/toplevel-dir/*'
> > In fact, the former will result in
> >    fatal: '/.gitattributes' is outside repository...
> > while the later will result in
> >    fatal: Invalid path '/toplevel-dir': No such file or directory
> > despite the fact that both are valid gitignore-style patterns that would
> > select real files if added to the sparse-checkout file.  However, these
> > commands can be run successfully from the toplevel directory, and many
> > gitignore-style patterns ARE paths, and bash completion seems to be
> > suggesting directories and files, so perhaps for consistency we pay
> > attention to the prefix?  It's not clear what is okay here, but maybe
> > that's yet another reason to deprecate non-cone mode as we will do later
> > in this series.
> >
> > For now, incorporate prefix into the positional arguments for either
> > cone or non-cone mode.  For additional discussion of this issue, see
> > https://lore.kernel.org/git/29f0410e-6dfa-2e86-394d-b1fb735e7608@gmail.com/
>
> Perhaps this was covered in the issue, but for non-cone mode, it
> matters if there is a leading slash or not in the pattern. Will
> this change make it impossible for a user to input that distinction?
>
> Will there still be a difference between:
>
>         git sparse-checkout set --no-cone /.vs/
>
> and
>
>         git sparse-checkout set --no-cone .vs/
>
> ?

If you are in the toplevel directory, you can run either of these and
they have the same meaning they traditionally had.

Before this patch, if you are in a subdirectory, the first of those
would have specified a toplevel ".vs" directory, and the second would
have specified a ".vs/" directory in the toplevel OR any subdirectory.
Those choices might be what the user wanted, or both of those could be
a nasty surprise for the user.

After this patch, if you are in a subdirectory, the first of those
throw an error:
    $ git sparse-checkout set --no-cone /.vs/
    fatal: Invalid path '/.vs': No such file or directory
(which might be an annoyance, but how would you possibly specify a
leading slash on a path that needs to be prefixed anyway?)  The second
will specify a SUBDIR/.vs/ from the toplevel directory (which again,
might be what the user wanted, or might be a nasty surprise if they
were trying to specify a pattern relative to the root).

Does this change make sense?  For some users, sure -- especially those
with the idea that you specify paths for non-cone mode (though
bash-completion may guide folks to presume that).  But for those who
understand that non-cone mode is all about patterns and that we have a
single toplevel file where everything must be recorded, it's possibly
detrimental to them.  To me, I wonder if it seems fraught with nasty
surprises for us to do anything other than throw an error when
--no-cone is specified and we are in a subdirectory.  Perhaps I should
do that instead of this change here.

> > Helped-by: Junio Hamano <gitster@pobox.com>
> > Signed-off-by: Elijah Newren <newren@gmail.com>
>
> This could probably use a
>
>   Reported-by: Lessley Dennington <lessleydennington@gmail.com>

It'd be more of a "Report-Formalized-by:" if we were to include such a
tag.  Check the history here:
https://lore.kernel.org/git/52d638fc-e7e7-1b0a-482b-cff7c9500b92@gmail.com/

In short: I was the original reporter; I noted the issue while
reviewing her completion series.  The bug was not related to her
series, but her series did prompt me to check and discover the issue.
She didn't want the issue to get lost, and decided to make a formal
report.

> > +static void sanitize_paths(int argc, const char **argv, const char *prefix)
> > +{
> > +     if (!argc)
> > +             return;
> > +
> > +     if (prefix && *prefix) {
> > +             /*
> > +              * The args are not pathspecs, so unfortunately we
> > +              * cannot imitate how cmd_add() uses parse_pathspec().
> > +              */
> > +             int i;
> > +             int prefix_len = strlen(prefix);
> > +
> > +             for (i = 0; i < argc; i++)
> > +                     argv[i] = prefix_path(prefix, prefix_len, argv[i]);
> > +     }
> > +}
> > +
> >  static char const * const builtin_sparse_checkout_add_usage[] = {
> >       N_("git sparse-checkout add (--stdin | <patterns>)"),
> >       NULL
> > @@ -708,6 +726,8 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
> >                            builtin_sparse_checkout_add_usage,
> >                            PARSE_OPT_KEEP_UNKNOWN);
> >
> > +     sanitize_paths(argc, argv, prefix);
> > +
> >       return modify_pattern_list(argc, argv, add_opts.use_stdin, ADD);
> >  }
> >
> > @@ -759,6 +779,8 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
> >       if (!core_sparse_checkout_cone && argc == 0) {
> >               argv = default_patterns;
> >               argc = default_patterns_nr;
> > +     } else {
> > +             sanitize_paths(argc, argv, prefix);
> >       }
>
> Code changes appear to do as described: apply the prefix everywhere
> it matters, no matter the mode.
>
> > +test_expect_success 'set from subdir pays attention to prefix' '
> > +     git -C repo sparse-checkout disable &&
> > +     git -C repo/deep sparse-checkout set --cone deeper2 ../folder1 &&
> > +
> > +     git -C repo sparse-checkout list >actual &&
> > +
> > +     cat >expect <<-\EOF &&
> > +     deep/deeper2
> > +     folder1
> > +     EOF
> > +     test_cmp expect actual
> > +'
> > +
> > +test_expect_success 'add from subdir pays attention to prefix' '
> > +     git -C repo sparse-checkout set --cone deep/deeper2 &&
> > +     git -C repo/deep sparse-checkout add deeper1/deepest ../folder1 &&
> > +
> > +     git -C repo sparse-checkout list >actual &&
> > +
> > +     cat >expect <<-\EOF &&
> > +     deep/deeper1/deepest
> > +     deep/deeper2
> > +     folder1
> > +     EOF
> > +     test_cmp expect actual
> > +'
> > +
> >  test_done
>
> These tests could use a non-cone-mode version to demonstrate the behavior
> in that mode.

Fair enough, though I hesitated in part because I wasn't sure we even
wanted to make that change, and I figured getting that answer might be
useful before writing the tests.

Re: [PATCH 4/7] sparse-checkout: error or warn when given individual files

[Elijah Newren]

On Mon, Feb 14, 2022 at 7:56 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>
> >
> > The set and add subcommands accept multiple positional arguments.
> > The meaning of these arguments differs slightly in the two modes:
> >
> > Cone mode only accepts directories.  If given a file, it would
> > previously treat it as a directory, causing not just the file itself to
> > be included but all sibling files as well -- likely against users'
> > expectations.  Throw an error if the specified path is a file in the
> > index.  Provide a --skip-checks argument to allow users to override
> > (e.g. for the case when the given path IS a directory on another
> > branch).
>
> I agree that this is likely to be an improvement for users. The
> sparse-checkout builtin isn't integrated with the sparse index
> yet. At least not integrated upstream: we have commits in microsoft/git
> that we plan to send when other things in flight are merged. This
> change likely introduces a new opportunity for the index to expand,
> so I will keep that in mind when upstreaming.

Actually, I thought about that during development, and my presumption
was that we would not expand the index.  We've survived a few years
without reporting any argument errors to the user and folks seem to
usually get things right, so while I think it adds value to report on
likely errors, I don't think it's important for us to catch and warn
on every potential misuse.  I think the probable errors are the ones
where they specify a <file> that exists in both the working tree and
index.  The remaining ones are less probable, and also possibly quite
expensive to catch.  I'm not sure it's worth the cost to try to report
those.

> > Non-cone mode accepts general gitignore patterns.  However, it has an
> > O(N*M) performance baked into its design, where all N index files must
> > be matched against all M sparse-checkout patterns with EVERY call to
> > unpack_trees() that updates the working tree.  As such, it is important
> > to keep the number of patterns small, and thus we should warn users to
> > prefer passing directories and more generic glob patterns to get the
> > paths they want instead of listing each individual file.  (The
> > --skip-checks argument can also be used to bypass this warning.)  Also,
> > even when users do want to specify individual files, they will often
> > want to do so by providing a leading '/' (to avoid selecting the same
> > filename in all subdirectories), in which case this error message would
> > never trigger anyway.
>
> I think the case of "I want only one file from this directory" and "I
> want files with the given name pattern" are the main reason to still
> use non-cone-mode. Users with this need usually have a directory full
> of large files, and they choose which of those large files they need
> using sparse-checkout. The repository reorganization required to use
> cone mode for this use is perhaps too great (or they haven't thought
> about doing it). For this reason, I would prefer that we do not do
> these checks when not in cone mode.

If they "only want one file from this directory", isn't the correct
way to specify that by mentioning the path with a leading slash?
Otherwise, they'd potentially grab files with similar names from many
directories, right?  So, even in that usecase, we should still error
out if they specify a <filename> rather than /<filename>.  Perhaps my
reasoning should lead with that and I should fix up the warning
message a bit, but I still think we should probably give a warning
even for those who are explicitly wanting the usecase you mention.

Also, note this is a warning and not an error -- and the warning can
be suppressed with --skip-checks.

> > +test_expect_success 'by default, cone mode will error out when passed files' '
> > +     git -C repo sparse-checkout reapply --cone &&
> > +     test_must_fail git -C repo sparse-checkout add .gitignore 2>error &&
> > +
> > +     grep ".gitignore.*is not a directory" error
> > +'
> > +
> > +test_expect_success 'by default, non-cone mode will warn on individual files' '
> > +     git -C repo sparse-checkout reapply --no-cone &&
> > +     git -C repo sparse-checkout add .gitignore 2>warning &&
> > +
> > +     grep "passing directories or less specific patterns is recommended" warning
> > +'
>
> So I would expect this second test to have
>
>         test_must_be_empty warning
>
> to show that no warning occurs when specifying a file in non-cone-mode.

or perhaps

grep "please specify a leading slash to select a single file" warning

?

Re: [PATCH 5/7] sparse-checkout: reject non-cone-mode patterns starting with a '#'

[Elijah Newren]

On Mon, Feb 14, 2022 at 9:59 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +     if (!core_sparse_checkout_cone)
> > +             for (i = 0; i < argc; i++)
> > +                     if (argv[i][0] == '#')
> > +                             die(_("paths beginning with a '#' must be preceeded by a backslash"));
> > +
>
> Whenever I see an error message like this, where it is clear that
> the command knows the only viable solution is to the issue, and yet
> still refuses to do-what-the-user-clearly-meant-to-do (is there a
> valid reason to copy and paste "# comment" line, which clearly is
> not about choosing which paths to use/ignore, from an existing file
> and feed it to the command?), I question if it should be solved the
> opposite way.
>
> That is, to pretend as if "\" + argv[i] was given and then give the
> user either a warning saying what we did, or an unsquelcheable advice
> message (no need for advice.* config---the user can avoid triggering
> it by learning what the advice message would say, which is to use \#
> when they mean to give a pattern that begins with a pound).

If this were the only special character case, I'd totally agree, but I
do worry a bit that escaping this particular case might lead users to
expect us to escape and fix other special characters from '*?[]!\'.
If users have files with those characters and specify an argument with
one of those, are we to automatically escape them as well?  If we
don't escape the other characters but do escape '#', aren't we being
inconsistent?  And if we do escape those other characters too, aren't
we breaking users who are trying to specify patterns (which is what
non-cone mode is all about)?  Personally, I'd rather drop this patch
than introduce such an inconsistency.

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

[Elijah Newren]

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.

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

[Elijah Newren]

On Mon, Feb 14, 2022 at 8:19 AM Derrick Stolee <derrickstolee@github.com> wrote:
>
> On 2/12/2022 7:39 PM, Elijah Newren via GitGitGadget wrote:
> > 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.)

Why would that be needed?  The documentation does not specify anything
about cone vs. non-cone mode, only that the initial working tree will
only have files from the toplevel directory present.  So, the
documentation is correct without any needed changes.

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

Yeah, and en/present-despite-skipped addressed all reviewer comments a
full month ago, so I'd like to believe it's ready to merge down soon
too.

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

In which case shouldn't we still show a warning when users specify a
path rather than a pattern, since the former risks selecting more than
one file?  (Adding a leading slash should be recommended for such a
case, right?)

> It might be worth adding some documentation about how to reorganize a repo
> to fit cone mode patterns, but that's not necessary.
>
> >  * 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.

Will do.

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

We can discuss this more later, but I think it's worthwhile to
consider what happens even if folks didn't read the BIG warning about
behavioral changes in the git-sparse-checkout.txt manual, and didn't
know about the default change, and tried to use it anyway.  If they
specify something other than a directory, then they'd get an error
message due to the first six patches of this series -- at which point
they can look to the manual and decide to add --no-cone to their
command.  In other words, it basically does the same thing that we'd
do if we decided to have an interim period with an error when neither
--cone or --no-cone were specified, other than the error message
perhaps being slightly different.  What if the user does specify
directories and doesn't know about the default mode change?  Well,
that's where the two modes overlap and things work fine (with only
minor differences in behavior, such as better performance, and files
from leading directories being included), so the user would be able to
continue with their work.  So, I'm not sure that an interim period
where we error out when neither --cone or --no-cone are specified is
going to buy us much of anything.  And besides, we do have that super
big scary warning in the manual.  Anyway, I'll bring this all up again
when I resubmit the final patch broken up into a separate series.

[PATCH v2 0/6] sparse checkout: fix a few bugs and check argument validity for set/add

[Elijah Newren via GitGitGadget]

== Maintainer notes ==

Note1: This has been rebased on origin/master. v1 wasn't picked up anyway,
so this shouldn't matter, but just pointing it out.

Note2: There is a small textual and small semantic conflict with
ds/sparse-checkout-requires-per-worktree-config in seen. I included the diff
with the correct resolution near the end of this cover letter.

== Overview ==

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 (i.e. in
   v2.35.0)
 * Patch 3 fixes sparse-checkout-from-subdirectories-ignores-"prefix" (see
   https://lore.kernel.org/git/29f0410e-6dfa-2e86-394d-b1fb735e7608@gmail.com/),
   in cone mode. Since we'll get nasty surprises whether we use or don't use
   "prefix" for non-cone mode, simply throw an error if set/add subcommands
   of sparse-checkout are run from a subdirectory.
 * 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.

== Update history ==

Changes since v1:

 * Dropped the commit changing cone-mode to default (patch 7, which will be
   split into multiple patches and submitted as a separate series)
 * Removed the RFC label
 * Decided to error out when running set/add with paths from a subdirectory
   in non-cone mode, and added tests
 * Changed the warning for non-cone mode with individual files to point out
   that the user is likely trying to select an individual file, but should
   likely add a leading slash to ensure that is what happens
 * Fixed typos, removed unnecessary condition checks

== Conflict resolution ==

Patch to resolve textual and semantic conflict with
ds/sparse-checkout-requires-per-worktree-config:

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
remerge CONFLICT (content): Merge conflict in t/t1091-sparse-checkout-builtin.sh
index 3c6adeb885..3a95d2996d 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -275,24 +275,8 @@ test_expect_success 'sparse-index enabled and disabled' '
     diff -u sparse full | tail -n +3 >actual &&
     test_cmp expect actual &&
 
-<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 286c22e5ec (sparse-checkout: reject arguments in cone-mode that look like patterns)
     git -C repo config --list >config &&
-    ! grep index.sparse config
-|||||||||||||||||||||||||||||||| 89bece5c8c
-        diff -u sparse full | tail -n +3 >actual &&
-        test_cmp expect actual &&
-
-        git -C repo config --list >config &&
-        ! grep index.sparse config
-    )
-================================
-        diff -u sparse full | tail -n +3 >actual &&
-        test_cmp expect actual &&
-
-        git -C repo config --list >config &&
-        test_cmp_config -C repo false index.sparse
-    )
->>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 3ce1138272 (config: make git_configset_get_string_tmp() private)
+    test_cmp_config -C repo false index.sparse
 '
 
 test_expect_success 'cone mode: init and set' '
@@ -532,6 +516,7 @@ test_expect_success 'reapply can handle config options' '
     cat >expect <<-\EOF &&
     core.sparsecheckout=true
     core.sparsecheckoutcone=true
+    index.sparse=false
     EOF
     test_cmp expect actual &&
 
@@ -539,6 +524,8 @@ test_expect_success 'reapply can handle config options' '
     git -C repo config --worktree --list >actual &&
     cat >expect <<-\EOF &&
     core.sparsecheckout=true
+    core.sparsecheckoutcone=false
+    index.sparse=false
     EOF
     test_cmp expect actual &&


== CCs ==

Elijah Newren (6):
  sparse-checkout: correct reapply's handling of options
  sparse-checkout: correctly set non-cone mode when expected
  sparse-checkout: pay attention to prefix for {set, add}
  sparse-checkout: error or warn when given individual files
  sparse-checkout: reject non-cone-mode patterns starting with a '#'
  sparse-checkout: reject arguments in cone-mode that look like patterns

 builtin/sparse-checkout.c          | 84 +++++++++++++++++++++++++--
 t/t1091-sparse-checkout-builtin.sh | 93 +++++++++++++++++++++++++++++-
 2 files changed, 170 insertions(+), 7 deletions(-)


base-commit: b80121027d1247a0754b3cc46897fee75c050b44
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1118%2Fnewren%2Fsparse-checkout-default-and-arg-validity-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1118/newren/sparse-checkout-default-and-arg-validity-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1118

Range-diff vs v1:

 1:  00777e77118 ! 1:  5536fe6498e sparse-checkout: correct reapply's handling of options
     @@ t/t1091-sparse-checkout-builtin.sh: test_expect_failure 'sparse-checkout reapply
      +	cat >expect <<-\EOF &&
      +	core.sparsecheckout=true
      +	core.sparsecheckoutcone=true
     -+	index.sparse=false
      +	EOF
      +	test_cmp expect actual &&
      +
     @@ t/t1091-sparse-checkout-builtin.sh: test_expect_failure 'sparse-checkout reapply
      +	git -C repo config --worktree --list >actual &&
      +	cat >expect <<-\EOF &&
      +	core.sparsecheckout=true
     -+	core.sparsecheckoutcone=false
     -+	index.sparse=false
      +	EOF
      +	test_cmp expect actual &&
      +
 2:  3bab5960404 ! 2:  9edad872e0d sparse-checkout: correctly set non-cone mode when expected
     @@ Commit message
      
          commit f2e3a218e8 ("sparse-checkout: enable `set` to initialize
          sparse-checkout mode", 2021-12-14) made the `set` command able to
     -    intialize sparse-checkout mode, but it also had to function when
     +    initialize sparse-checkout mode, but it also had to function when
          sparse-checkout mode was already setup and the user just wanted to
          change the sparsity paths.  So, if the user passed --cone or --no-cone,
          then we should override the current setting, but if they didn't pass
     @@ Commit message
          set the in-memory cone mode value (core_sparse_checkout_one) when
          --no-cone was specified, but since it did set the config setting on
          disk, any subsequent git invocation would correctly get non-cone mode.
     -    As such, the error did not previously matter.  However, a sbusequent
     +    As such, the error did not previously matter.  However, a subsequent
          commit will add some logic that depends on core_sparse_checkout_cone
          being set to the correct mode, so make sure it is set consistently with
          the config values we will be writing to disk.
     @@ builtin/sparse-checkout.c: static int update_modes(int *cone_mode, int *sparse_i
       		core_sparse_checkout_cone = 1;
       	} else {
       		mode = MODE_ALL_PATTERNS;
     -+		if (record_mode)
     -+			core_sparse_checkout_cone = 0;
     ++		core_sparse_checkout_cone = 0;
       	}
       	if (record_mode && set_config(mode))
       		return 1;
 3:  679f869ff11 ! 3:  f57820e25d6 sparse-checkout: pay attention to prefix for {set, add}
     @@ Commit message
          while the later will result in
             fatal: Invalid path '/toplevel-dir': No such file or directory
          despite the fact that both are valid gitignore-style patterns that would
     -    select real files if added to the sparse-checkout file.  However, these
     -    commands can be run successfully from the toplevel directory, and many
     -    gitignore-style patterns ARE paths, and bash completion seems to be
     -    suggesting directories and files, so perhaps for consistency we pay
     -    attention to the prefix?  It's not clear what is okay here, but maybe
     -    that's yet another reason to deprecate non-cone mode as we will do later
     -    in this series.
     +    select real files if added to the sparse-checkout file.  This might lead
     +    people to just use the path without the leading slash, potentially
     +    resulting in them grabbing files with the same name throughout the
     +    directory hierarchy contrary to their expectations.  See also [1] and
     +    [2].  Adding prefix seems to just be fraught with error; so for now
     +    simply throw an error in non-cone mode when sparse-checkout set/add are
     +    run from a subdirectory.
      
     -    For now, incorporate prefix into the positional arguments for either
     -    cone or non-cone mode.  For additional discussion of this issue, see
     -    https://lore.kernel.org/git/29f0410e-6dfa-2e86-394d-b1fb735e7608@gmail.com/
     +    [1] https://lore.kernel.org/git/e1934710-e228-adc4-d37c-f706883bd27c@gmail.com/
     +    [2] https://lore.kernel.org/git/CABPp-BHXZ-XLxY0a3wCATfdq=6-EjW62RzbxKAoFPeXfJswD2w@mail.gmail.com/
      
          Helped-by: Junio Hamano <gitster@pobox.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
     @@ builtin/sparse-checkout.c: static int modify_pattern_list(int argc, const char *
      +		int i;
      +		int prefix_len = strlen(prefix);
      +
     ++		if (!core_sparse_checkout_cone)
     ++			die("please run from the toplevel directory in non-cone mode");
     ++
      +		for (i = 0; i < argc; i++)
      +			argv[i] = prefix_path(prefix, prefix_len, argv[i]);
      +	}
     @@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'malformed cone-mode pat
      +	EOF
      +	test_cmp expect actual
      +'
     ++
     ++test_expect_success 'set from subdir in non-cone mode throws an error' '
     ++	git -C repo sparse-checkout disable &&
     ++	test_must_fail git -C repo/deep sparse-checkout set --no-cone deeper2 ../folder1 2>error &&
     ++
     ++	grep "run from the toplevel directory in non-cone mode" error
     ++'
     ++
     ++test_expect_success 'set from subdir in non-cone mode throws an error' '
     ++	git -C repo sparse-checkout set --no-cone deep/deeper2 &&
     ++	test_must_fail git -C repo/deep sparse-checkout add deeper1/deepest ../folder1 2>error &&
     ++
     ++	grep "run from the toplevel directory in non-cone mode" error
     ++'
      +
       test_done
 4:  5e27cad17a7 ! 4:  c3bb2a3b2f1 sparse-checkout: error or warn when given individual files
     @@ Commit message
          (e.g. for the case when the given path IS a directory on another
          branch).
      
     -    Non-cone mode accepts general gitignore patterns.  However, it has an
     -    O(N*M) performance baked into its design, where all N index files must
     -    be matched against all M sparse-checkout patterns with EVERY call to
     -    unpack_trees() that updates the working tree.  As such, it is important
     -    to keep the number of patterns small, and thus we should warn users to
     -    prefer passing directories and more generic glob patterns to get the
     -    paths they want instead of listing each individual file.  (The
     -    --skip-checks argument can also be used to bypass this warning.)  Also,
     -    even when users do want to specify individual files, they will often
     -    want to do so by providing a leading '/' (to avoid selecting the same
     -    filename in all subdirectories), in which case this error message would
     -    never trigger anyway.
     +    Non-cone mode accepts general gitignore patterns.  There are many
     +    reasons to avoid this mode, but one possible reason to use it instead of
     +    cone mode: to be able to select individual files within a directory.
     +    However, if a file is passed to set/add in non-cone mode, you won't be
     +    selecting a single file, you'll be selecting a file with the same name
     +    in any directory.  Thus users will likely want to prefix any paths they
     +    specify with a leading '/' character; warn users if the patterns they
     +    specify exactly name a file because it means they are likely missing
     +    such a missing leading slash.
      
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
     @@ builtin/sparse-checkout.c: static void sanitize_paths(int argc, const char **arg
      -		int i;
       		int prefix_len = strlen(prefix);
       
     + 		if (!core_sparse_checkout_cone)
     +@@ builtin/sparse-checkout.c: static void sanitize_paths(int argc, const char **argv, const char *prefix)
       		for (i = 0; i < argc; i++)
       			argv[i] = prefix_path(prefix, prefix_len, argv[i]);
       	}
     @@ builtin/sparse-checkout.c: static void sanitize_paths(int argc, const char **arg
      +		if (core_sparse_checkout_cone)
      +			die(_("\"%s\" is not a directory; to treat it as a directory anyway, rerun with --skip-checks"), argv[i]);
      +		else
     -+			warning(_("path \"%s\" is an individual file; passing directories or less specific patterns is recommended (see NON-CONE PROBLEMS in the git-sparse-checkout manual)."), argv[i]);
     ++			warning(_("pass a leading slash before paths such as \"%s\" if you want a single file (see NON-CONE PROBLEMS in the git-sparse-checkout manual)."), argv[i]);
      +	}
       }
       
     @@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'different sparse-checko
       	cat >expect <<-\EOF &&
       	/*
       	!/*/
     -@@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'add from subdir pays attention to prefix' '
     - 	test_cmp expect actual
     +@@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'set from subdir in non-cone mode throws an error' '
     + 	grep "run from the toplevel directory in non-cone mode" error
       '
       
      +test_expect_success 'by default, cone mode will error out when passed files' '
     @@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'add from subdir pays at
      +	git -C repo sparse-checkout reapply --no-cone &&
      +	git -C repo sparse-checkout add .gitignore 2>warning &&
      +
     -+	grep "passing directories or less specific patterns is recommended" warning
     ++	grep "pass a leading slash before paths.*if you want a single file" warning
      +'
      +
       test_done
 5:  265cbe36b2d ! 5:  c5d4ae2cfd6 sparse-checkout: reject non-cone-mode patterns starting with a '#'
     @@ builtin/sparse-checkout.c: static void sanitize_paths(int argc, const char **arg
      
       ## t/t1091-sparse-checkout-builtin.sh ##
      @@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'by default, non-cone mode will warn on individual files' '
     - 	grep "passing directories or less specific patterns is recommended" warning
     + 	grep "pass a leading slash before paths.*if you want a single file" warning
       '
       
      +test_expect_success 'paths starting with hash must be escaped in non-cone mode' '
 6:  502da48b8f4 = 6:  286c22e5ecd sparse-checkout: reject arguments in cone-mode that look like patterns
 7:  e30119b96df < -:  ----------- sparse-checkout: make --cone the default and deprecate --no-cone

-- 
gitgitgadget

[PATCH v2 1/6] sparse-checkout: correct reapply's handling of options

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

Commit 4e256731d6 ("sparse-checkout: enable reapply to take
--[no-]{cone,sparse-index}", 2021-12-14) made it so that reapply could
take additional options but added no tests.  Tests would have shown that
the feature doesn't work because the initial values are set AFTER
parsing the command line options instead of before.  Add a test and set
the initial value at the appropriate time.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c          |  6 +++---
 t/t1091-sparse-checkout-builtin.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index a311483a7d2..fcd574f5fc1 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -789,15 +789,15 @@ static int sparse_checkout_reapply(int argc, const char **argv)
 	if (!core_apply_sparse_checkout)
 		die(_("must be in a sparse-checkout to reapply sparsity patterns"));
 
+	reapply_opts.cone_mode = -1;
+	reapply_opts.sparse_index = -1;
+
 	argc = parse_options(argc, argv, NULL,
 			     builtin_sparse_checkout_reapply_options,
 			     builtin_sparse_checkout_reapply_usage, 0);
 
 	repo_read_index(the_repository);
 
-	reapply_opts.cone_mode = -1;
-	reapply_opts.sparse_index = -1;
-
 	if (update_modes(&reapply_opts.cone_mode, &reapply_opts.sparse_index))
 		return 1;
 
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 3592d124424..ce5e7c19efa 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -495,6 +495,34 @@ test_expect_failure 'sparse-checkout reapply' '
 	git -C tweak sparse-checkout disable
 '
 
+test_expect_success 'reapply can handle config options' '
+	git -C repo sparse-checkout init --cone --no-sparse-index &&
+	git -C repo config --worktree --list >actual &&
+	cat >expect <<-\EOF &&
+	core.sparsecheckout=true
+	core.sparsecheckoutcone=true
+	EOF
+	test_cmp expect actual &&
+
+	git -C repo sparse-checkout reapply --no-cone --no-sparse-index &&
+	git -C repo config --worktree --list >actual &&
+	cat >expect <<-\EOF &&
+	core.sparsecheckout=true
+	EOF
+	test_cmp expect actual &&
+
+	git -C repo sparse-checkout reapply --cone --sparse-index &&
+	git -C repo config --worktree --list >actual &&
+	cat >expect <<-\EOF &&
+	core.sparsecheckout=true
+	core.sparsecheckoutcone=true
+	index.sparse=true
+	EOF
+	test_cmp expect actual &&
+
+	git -C repo sparse-checkout disable
+'
+
 test_expect_success 'cone mode: set with core.ignoreCase=true' '
 	rm repo/.git/info/sparse-checkout &&
 	git -C repo sparse-checkout init --cone &&
-- 
gitgitgadget

[PATCH v2 2/6] sparse-checkout: correctly set non-cone mode when expected

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

commit f2e3a218e8 ("sparse-checkout: enable `set` to initialize
sparse-checkout mode", 2021-12-14) made the `set` command able to
initialize sparse-checkout mode, but it also had to function when
sparse-checkout mode was already setup and the user just wanted to
change the sparsity paths.  So, if the user passed --cone or --no-cone,
then we should override the current setting, but if they didn't pass
either, we should use whatever the current cone mode setting is.

Unfortunately, there was a small error in the logic in that it would not
set the in-memory cone mode value (core_sparse_checkout_one) when
--no-cone was specified, but since it did set the config setting on
disk, any subsequent git invocation would correctly get non-cone mode.
As such, the error did not previously matter.  However, a subsequent
commit will add some logic that depends on core_sparse_checkout_cone
being set to the correct mode, so make sure it is set consistently with
the config values we will be writing to disk.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index fcd574f5fc1..fb85a1459c3 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -403,6 +403,7 @@ static int update_modes(int *cone_mode, int *sparse_index)
 		core_sparse_checkout_cone = 1;
 	} else {
 		mode = MODE_ALL_PATTERNS;
+		core_sparse_checkout_cone = 0;
 	}
 	if (record_mode && set_config(mode))
 		return 1;
-- 
gitgitgadget

[PATCH v2 3/6] sparse-checkout: pay attention to prefix for {set, add}

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

In cone mode, non-option arguments to set & add are clearly paths, and
as such, we should pay attention to prefix.

In non-cone mode, it is not clear that folks intend to provide paths
since the inputs are gitignore-style patterns.  Paying attention to
prefix would prevent folks from doing things like
   git sparse-checkout add /.gitattributes
   git sparse-checkout add '/toplevel-dir/*'
In fact, the former will result in
   fatal: '/.gitattributes' is outside repository...
while the later will result in
   fatal: Invalid path '/toplevel-dir': No such file or directory
despite the fact that both are valid gitignore-style patterns that would
select real files if added to the sparse-checkout file.  This might lead
people to just use the path without the leading slash, potentially
resulting in them grabbing files with the same name throughout the
directory hierarchy contrary to their expectations.  See also [1] and
[2].  Adding prefix seems to just be fraught with error; so for now
simply throw an error in non-cone mode when sparse-checkout set/add are
run from a subdirectory.

[1] https://lore.kernel.org/git/e1934710-e228-adc4-d37c-f706883bd27c@gmail.com/
[2] https://lore.kernel.org/git/CABPp-BHXZ-XLxY0a3wCATfdq=6-EjW62RzbxKAoFPeXfJswD2w@mail.gmail.com/

Helped-by: Junio Hamano <gitster@pobox.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c          | 25 ++++++++++++++++++
 t/t1091-sparse-checkout-builtin.sh | 41 ++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index fb85a1459c3..a2192a69219 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -684,6 +684,27 @@ static int modify_pattern_list(int argc, const char **argv, int use_stdin,
 	return result;
 }
 
+static void sanitize_paths(int argc, const char **argv, const char *prefix)
+{
+	if (!argc)
+		return;
+
+	if (prefix && *prefix) {
+		/*
+		 * The args are not pathspecs, so unfortunately we
+		 * cannot imitate how cmd_add() uses parse_pathspec().
+		 */
+		int i;
+		int prefix_len = strlen(prefix);
+
+		if (!core_sparse_checkout_cone)
+			die("please run from the toplevel directory in non-cone mode");
+
+		for (i = 0; i < argc; i++)
+			argv[i] = prefix_path(prefix, prefix_len, argv[i]);
+	}
+}
+
 static char const * const builtin_sparse_checkout_add_usage[] = {
 	N_("git sparse-checkout add (--stdin | <patterns>)"),
 	NULL
@@ -711,6 +732,8 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
 			     builtin_sparse_checkout_add_usage,
 			     PARSE_OPT_KEEP_UNKNOWN);
 
+	sanitize_paths(argc, argv, prefix);
+
 	return modify_pattern_list(argc, argv, add_opts.use_stdin, ADD);
 }
 
@@ -762,6 +785,8 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
 	if (!core_sparse_checkout_cone && argc == 0) {
 		argv = default_patterns;
 		argc = default_patterns_nr;
+	} else {
+		sanitize_paths(argc, argv, prefix);
 	}
 
 	return modify_pattern_list(argc, argv, set_opts.use_stdin, REPLACE);
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index ce5e7c19efa..c1f86b0e02e 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -798,4 +798,45 @@ test_expect_success 'malformed cone-mode patterns' '
 	grep "warning: disabling cone pattern matching" err
 '
 
+test_expect_success 'set from subdir pays attention to prefix' '
+	git -C repo sparse-checkout disable &&
+	git -C repo/deep sparse-checkout set --cone deeper2 ../folder1 &&
+
+	git -C repo sparse-checkout list >actual &&
+
+	cat >expect <<-\EOF &&
+	deep/deeper2
+	folder1
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'add from subdir pays attention to prefix' '
+	git -C repo sparse-checkout set --cone deep/deeper2 &&
+	git -C repo/deep sparse-checkout add deeper1/deepest ../folder1 &&
+
+	git -C repo sparse-checkout list >actual &&
+
+	cat >expect <<-\EOF &&
+	deep/deeper1/deepest
+	deep/deeper2
+	folder1
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'set from subdir in non-cone mode throws an error' '
+	git -C repo sparse-checkout disable &&
+	test_must_fail git -C repo/deep sparse-checkout set --no-cone deeper2 ../folder1 2>error &&
+
+	grep "run from the toplevel directory in non-cone mode" error
+'
+
+test_expect_success 'set from subdir in non-cone mode throws an error' '
+	git -C repo sparse-checkout set --no-cone deep/deeper2 &&
+	test_must_fail git -C repo/deep sparse-checkout add deeper1/deepest ../folder1 2>error &&
+
+	grep "run from the toplevel directory in non-cone mode" error
+'
+
 test_done
-- 
gitgitgadget

[PATCH v2 4/6] sparse-checkout: error or warn when given individual files

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

The set and add subcommands accept multiple positional arguments.
The meaning of these arguments differs slightly in the two modes:

Cone mode only accepts directories.  If given a file, it would
previously treat it as a directory, causing not just the file itself to
be included but all sibling files as well -- likely against users'
expectations.  Throw an error if the specified path is a file in the
index.  Provide a --skip-checks argument to allow users to override
(e.g. for the case when the given path IS a directory on another
branch).

Non-cone mode accepts general gitignore patterns.  There are many
reasons to avoid this mode, but one possible reason to use it instead of
cone mode: to be able to select individual files within a directory.
However, if a file is passed to set/add in non-cone mode, you won't be
selecting a single file, you'll be selecting a file with the same name
in any directory.  Thus users will likely want to prefix any paths they
specify with a leading '/' character; warn users if the patterns they
specify exactly name a file because it means they are likely missing
such a missing leading slash.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c          | 43 +++++++++++++++++++++++++-----
 t/t1091-sparse-checkout-builtin.sh | 16 ++++++++++-
 2 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index a2192a69219..64583fa704f 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -1,4 +1,5 @@
 #include "builtin.h"
+#include "cache.h"
 #include "config.h"
 #include "dir.h"
 #include "parse-options.h"
@@ -684,8 +685,11 @@ static int modify_pattern_list(int argc, const char **argv, int use_stdin,
 	return result;
 }
 
-static void sanitize_paths(int argc, const char **argv, const char *prefix)
+static void sanitize_paths(int argc, const char **argv,
+			   const char *prefix, int skip_checks)
 {
+	int i;
+
 	if (!argc)
 		return;
 
@@ -694,7 +698,6 @@ static void sanitize_paths(int argc, const char **argv, const char *prefix)
 		 * The args are not pathspecs, so unfortunately we
 		 * cannot imitate how cmd_add() uses parse_pathspec().
 		 */
-		int i;
 		int prefix_len = strlen(prefix);
 
 		if (!core_sparse_checkout_cone)
@@ -703,20 +706,44 @@ static void sanitize_paths(int argc, const char **argv, const char *prefix)
 		for (i = 0; i < argc; i++)
 			argv[i] = prefix_path(prefix, prefix_len, argv[i]);
 	}
+
+	if (skip_checks)
+		return;
+
+	for (i = 0; i < argc; i++) {
+		struct cache_entry *ce;
+		struct index_state *index = the_repository->index;
+		int pos = index_name_pos(index, argv[i], strlen(argv[i]));
+
+		if (pos < 0)
+			continue;
+		ce = index->cache[pos];
+		if (S_ISSPARSEDIR(ce->ce_mode))
+			continue;
+
+		if (core_sparse_checkout_cone)
+			die(_("\"%s\" is not a directory; to treat it as a directory anyway, rerun with --skip-checks"), argv[i]);
+		else
+			warning(_("pass a leading slash before paths such as \"%s\" if you want a single file (see NON-CONE PROBLEMS in the git-sparse-checkout manual)."), argv[i]);
+	}
 }
 
 static char const * const builtin_sparse_checkout_add_usage[] = {
-	N_("git sparse-checkout add (--stdin | <patterns>)"),
+	N_("git sparse-checkout add [--skip-checks] (--stdin | <patterns>)"),
 	NULL
 };
 
 static struct sparse_checkout_add_opts {
+	int skip_checks;
 	int use_stdin;
 } add_opts;
 
 static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
 {
 	static struct option builtin_sparse_checkout_add_options[] = {
+		OPT_BOOL_F(0, "skip-checks", &add_opts.skip_checks,
+			   N_("skip some sanity checks on the given paths that might give false positives"),
+			   PARSE_OPT_NONEG),
 		OPT_BOOL(0, "stdin", &add_opts.use_stdin,
 			 N_("read patterns from standard in")),
 		OPT_END(),
@@ -732,19 +759,20 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
 			     builtin_sparse_checkout_add_usage,
 			     PARSE_OPT_KEEP_UNKNOWN);
 
-	sanitize_paths(argc, argv, prefix);
+	sanitize_paths(argc, argv, prefix, add_opts.skip_checks);
 
 	return modify_pattern_list(argc, argv, add_opts.use_stdin, ADD);
 }
 
 static char const * const builtin_sparse_checkout_set_usage[] = {
-	N_("git sparse-checkout set [--[no-]cone] [--[no-]sparse-index] (--stdin | <patterns>)"),
+	N_("git sparse-checkout set [--[no-]cone] [--[no-]sparse-index] [--skip-checks] (--stdin | <patterns>)"),
 	NULL
 };
 
 static struct sparse_checkout_set_opts {
 	int cone_mode;
 	int sparse_index;
+	int skip_checks;
 	int use_stdin;
 } set_opts;
 
@@ -758,6 +786,9 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
 			 N_("initialize the sparse-checkout in cone mode")),
 		OPT_BOOL(0, "sparse-index", &set_opts.sparse_index,
 			 N_("toggle the use of a sparse index")),
+		OPT_BOOL_F(0, "skip-checks", &set_opts.skip_checks,
+			   N_("skip some sanity checks on the given paths that might give false positives"),
+			   PARSE_OPT_NONEG),
 		OPT_BOOL_F(0, "stdin", &set_opts.use_stdin,
 			   N_("read patterns from standard in"),
 			   PARSE_OPT_NONEG),
@@ -786,7 +817,7 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
 		argv = default_patterns;
 		argc = default_patterns_nr;
 	} else {
-		sanitize_paths(argc, argv, prefix);
+		sanitize_paths(argc, argv, prefix, set_opts.skip_checks);
 	}
 
 	return modify_pattern_list(argc, argv, set_opts.use_stdin, REPLACE);
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index c1f86b0e02e..3b39329266b 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -562,7 +562,7 @@ test_expect_success 'different sparse-checkouts with worktrees' '
 '
 
 test_expect_success 'set using filename keeps file on-disk' '
-	git -C repo sparse-checkout set a deep &&
+	git -C repo sparse-checkout set --skip-checks a deep &&
 	cat >expect <<-\EOF &&
 	/*
 	!/*/
@@ -839,4 +839,18 @@ test_expect_success 'set from subdir in non-cone mode throws an error' '
 	grep "run from the toplevel directory in non-cone mode" error
 '
 
+test_expect_success 'by default, cone mode will error out when passed files' '
+	git -C repo sparse-checkout reapply --cone &&
+	test_must_fail git -C repo sparse-checkout add .gitignore 2>error &&
+
+	grep ".gitignore.*is not a directory" error
+'
+
+test_expect_success 'by default, non-cone mode will warn on individual files' '
+	git -C repo sparse-checkout reapply --no-cone &&
+	git -C repo sparse-checkout add .gitignore 2>warning &&
+
+	grep "pass a leading slash before paths.*if you want a single file" warning
+'
+
 test_done
-- 
gitgitgadget

[PATCH v2 5/6] sparse-checkout: reject non-cone-mode patterns starting with a '#'

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

In sparse-checkout add/set, in cone mode any specified directories will
be transformed into appropriate patterns.  In non-cone mode, the
non-option arguments are treated as patterns.

Since .git/info/sparse-checkout will ignore any patterns starting with a
'#' (they are just gitignore patterns), if the user passes an argument
starting with a '#' to sparse-checkout add/set in non-cone mode, it
would just be treated as a comment and ignored.  Error out in such
cases, informing the user that they need to backslash escape it.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c          | 5 +++++
 t/t1091-sparse-checkout-builtin.sh | 6 ++++++
 2 files changed, 11 insertions(+)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 64583fa704f..74d64ada9f9 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -710,6 +710,11 @@ static void sanitize_paths(int argc, const char **argv,
 	if (skip_checks)
 		return;
 
+	if (!core_sparse_checkout_cone)
+		for (i = 0; i < argc; i++)
+			if (argv[i][0] == '#')
+				die(_("paths beginning with a '#' must be preceeded by a backslash"));
+
 	for (i = 0; i < argc; i++) {
 		struct cache_entry *ce;
 		struct index_state *index = the_repository->index;
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 3b39329266b..3c811724d5d 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -853,4 +853,10 @@ test_expect_success 'by default, non-cone mode will warn on individual files' '
 	grep "pass a leading slash before paths.*if you want a single file" warning
 '
 
+test_expect_success 'paths starting with hash must be escaped in non-cone mode' '
+	test_must_fail git -C repo sparse-checkout set --no-cone "#funny-path" 2>error &&
+
+	grep "paths beginning.*#.*must be preceeded by a backslash" error
+'
+
 test_done
-- 
gitgitgadget

[PATCH v2 6/6] sparse-checkout: reject arguments in cone-mode that look like patterns

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

In sparse-checkout add/set under cone mode, the arguments passed are
supposed to be directories rather than gitignore-style patterns.
However, given the amount of effort spent in the manual discussing
patterns, it is easy for users to assume they need to pass patterns such
as
   /foo/*
or
   !/bar/*/
or perhaps they really do ignore the directory rule and specify a
random gitignore-style pattern like
   *.c

To help catch such mistakes, throw an error if any of the positional
arguments:
  * starts with any of '/!'
  * contains any of '*\?[]'

Inform users they can pass --skip-checks if they have a directory that
really does have such special characters in its name.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c          | 16 ++++++++++++++--
 t/t1091-sparse-checkout-builtin.sh |  2 +-
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 74d64ada9f9..a30c77e7a1a 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -710,10 +710,22 @@ static void sanitize_paths(int argc, const char **argv,
 	if (skip_checks)
 		return;
 
-	if (!core_sparse_checkout_cone)
-		for (i = 0; i < argc; i++)
+	for (i = 0; i < argc; i++) {
+		if (core_sparse_checkout_cone) {
+			if (argv[i][0] == '/')
+				die(_("specify directories rather than patterns (no leading slash)"));
+			if (argv[i][0] == '!')
+				die(_("specify directories rather than patterns.  If your directory starts with a '!', pass --skip-checks"));
+			if (strchr(argv[i], '*') ||
+			    strchr(argv[i], '?') ||
+			    strchr(argv[i], '[') ||
+			    strchr(argv[i], ']'))
+				die(_("specify directories rather than patterns.  If your directory really has any of '*?[]' in it, pass --skip-checks"));
+		} else {
 			if (argv[i][0] == '#')
 				die(_("paths beginning with a '#' must be preceeded by a backslash"));
+		}
+	}
 
 	for (i = 0; i < argc; i++) {
 		struct cache_entry *ce;
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 3c811724d5d..72157fc6f1f 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -673,7 +673,7 @@ test_expect_success BSLASHPSPEC 'pattern-checks: escaped characters' '
 	git -C escaped reset --hard $COMMIT &&
 	check_files escaped "a deep folder1 folder2 zbad\\dir zdoes*exist" zglob[!a]? &&
 	git -C escaped sparse-checkout init --cone &&
-	git -C escaped sparse-checkout set zbad\\dir/bogus "zdoes*not*exist" "zdoes*exist" "zglob[!a]?" &&
+	git -C escaped sparse-checkout set --skip-checks zbad\\dir/bogus "zdoes*not*exist" "zdoes*exist" "zglob[!a]?" &&
 	cat >expect <<-\EOF &&
 	/*
 	!/*/
-- 
gitgitgadget

Re: [PATCH 3/7] sparse-checkout: pay attention to prefix for {set, add}

[Derrick Stolee]

On 2/14/2022 10:52 PM, Elijah Newren wrote:
> On Mon, Feb 14, 2022 at 7:50 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>
>>>
>>> In cone mode, non-option arguments to set & add are clearly paths, and
>>> as such, we should pay attention to prefix.
>>>
>>> In non-cone mode, it is not clear that folks intend to provide paths
>>> since the inputs are gitignore-style patterns.  Paying attention to
>>> prefix would prevent folks from doing things like
>>>    git sparse-checkout add /.gitattributes
>>>    git sparse-checkout add '/toplevel-dir/*'
>>> In fact, the former will result in
>>>    fatal: '/.gitattributes' is outside repository...
>>> while the later will result in
>>>    fatal: Invalid path '/toplevel-dir': No such file or directory
>>> despite the fact that both are valid gitignore-style patterns that would
>>> select real files if added to the sparse-checkout file.  However, these
>>> commands can be run successfully from the toplevel directory, and many
>>> gitignore-style patterns ARE paths, and bash completion seems to be
>>> suggesting directories and files, so perhaps for consistency we pay
>>> attention to the prefix?  It's not clear what is okay here, but maybe
>>> that's yet another reason to deprecate non-cone mode as we will do later
>>> in this series.
>>>
>>> For now, incorporate prefix into the positional arguments for either
>>> cone or non-cone mode.  For additional discussion of this issue, see
>>> https://lore.kernel.org/git/29f0410e-6dfa-2e86-394d-b1fb735e7608@gmail.com/
>>
>> Perhaps this was covered in the issue, but for non-cone mode, it
>> matters if there is a leading slash or not in the pattern. Will
>> this change make it impossible for a user to input that distinction?
>>
>> Will there still be a difference between:
>>
>>         git sparse-checkout set --no-cone /.vs/
>>
>> and
>>
>>         git sparse-checkout set --no-cone .vs/
>>
>> ?
> 
> If you are in the toplevel directory, you can run either of these and
> they have the same meaning they traditionally had.
> 
> Before this patch, if you are in a subdirectory, the first of those
> would have specified a toplevel ".vs" directory, and the second would
> have specified a ".vs/" directory in the toplevel OR any subdirectory.
> Those choices might be what the user wanted, or both of those could be
> a nasty surprise for the user.
> 
> After this patch, if you are in a subdirectory, the first of those
> throw an error:
>     $ git sparse-checkout set --no-cone /.vs/
>     fatal: Invalid path '/.vs': No such file or directory
> (which might be an annoyance, but how would you possibly specify a
> leading slash on a path that needs to be prefixed anyway?)  The second
> will specify a SUBDIR/.vs/ from the toplevel directory (which again,
> might be what the user wanted, or might be a nasty surprise if they
> were trying to specify a pattern relative to the root).
> 
> Does this change make sense?  For some users, sure -- especially those
> with the idea that you specify paths for non-cone mode (though
> bash-completion may guide folks to presume that).  But for those who
> understand that non-cone mode is all about patterns and that we have a
> single toplevel file where everything must be recorded, it's possibly
> detrimental to them.  To me, I wonder if it seems fraught with nasty
> surprises for us to do anything other than throw an error when
> --no-cone is specified and we are in a subdirectory.  Perhaps I should
> do that instead of this change here.

I'd be in favor of this second approach of requiring the base directory.

>>> Helped-by: Junio Hamano <gitster@pobox.com>
>>> Signed-off-by: Elijah Newren <newren@gmail.com>
>>
>> This could probably use a
>>
>>   Reported-by: Lessley Dennington <lessleydennington@gmail.com>
> 
> It'd be more of a "Report-Formalized-by:" if we were to include such a
> tag.  Check the history here:
> https://lore.kernel.org/git/52d638fc-e7e7-1b0a-482b-cff7c9500b92@gmail.com/
> 
> In short: I was the original reporter; I noted the issue while
> reviewing her completion series.  The bug was not related to her
> series, but her series did prompt me to check and discover the issue.
> She didn't want the issue to get lost, and decided to make a formal
> report.

That makes sense. I wasn't caught up with that conversation.

>> These tests could use a non-cone-mode version to demonstrate the behavior
>> in that mode.
> 
> Fair enough, though I hesitated in part because I wasn't sure we even
> wanted to make that change, and I figured getting that answer might be
> useful before writing the tests.

Understandable.

Thanks,
-Stolee

Re: [PATCH 4/7] sparse-checkout: error or warn when given individual files

[Derrick Stolee]

On 2/14/2022 11:17 PM, Elijah Newren wrote:
> On Mon, Feb 14, 2022 at 7:56 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>
>>>
>>> The set and add subcommands accept multiple positional arguments.
>>> The meaning of these arguments differs slightly in the two modes:
>>>
>>> Cone mode only accepts directories.  If given a file, it would
>>> previously treat it as a directory, causing not just the file itself to
>>> be included but all sibling files as well -- likely against users'
>>> expectations.  Throw an error if the specified path is a file in the
>>> index.  Provide a --skip-checks argument to allow users to override
>>> (e.g. for the case when the given path IS a directory on another
>>> branch).
>>
>> I agree that this is likely to be an improvement for users. The
>> sparse-checkout builtin isn't integrated with the sparse index
>> yet. At least not integrated upstream: we have commits in microsoft/git
>> that we plan to send when other things in flight are merged. This
>> change likely introduces a new opportunity for the index to expand,
>> so I will keep that in mind when upstreaming.
> 
> Actually, I thought about that during development, and my presumption
> was that we would not expand the index.  We've survived a few years
> without reporting any argument errors to the user and folks seem to
> usually get things right, so while I think it adds value to report on
> likely errors, I don't think it's important for us to catch and warn
> on every potential misuse.  I think the probable errors are the ones
> where they specify a <file> that exists in both the working tree and
> index.  The remaining ones are less probable, and also possibly quite
> expensive to catch.  I'm not sure it's worth the cost to try to report
> those.

Since we start with only the files at root, if a user specifies a
directory at least two levels deep, then we don't have enough information
in the index to find that directory without parsing trees. Doing a simple
index_name_pos() call would expand the sparse index to a full one.

The checks of this kind typically follow this pattern:

	int pos = index_name_pos(index, name, strlen(name));
	if (pos >= 0)
		/* we have 'name' in the index! */
	else
		/* we don't have 'name' in the index! */

(Of course, there is some special logic for directories to see if they
exist, since 'pos' is a position within the list of files, unless it
points to a sparse directory entry.)

A different method that checks only for existence could find a sparse
directory and then search trees for the contained path. Returning a
boolean answer provides a mechanism for not expanding the index.

But we are getting sidetracked. I'll worry about this later. The
important thing is that we can solve this problem, so it shouldn't block
your feature.

>>> Non-cone mode accepts general gitignore patterns.  However, it has an
>>> O(N*M) performance baked into its design, where all N index files must
>>> be matched against all M sparse-checkout patterns with EVERY call to
>>> unpack_trees() that updates the working tree.  As such, it is important
>>> to keep the number of patterns small, and thus we should warn users to
>>> prefer passing directories and more generic glob patterns to get the
>>> paths they want instead of listing each individual file.  (The
>>> --skip-checks argument can also be used to bypass this warning.)  Also,
>>> even when users do want to specify individual files, they will often
>>> want to do so by providing a leading '/' (to avoid selecting the same
>>> filename in all subdirectories), in which case this error message would
>>> never trigger anyway.
>>
>> I think the case of "I want only one file from this directory" and "I
>> want files with the given name pattern" are the main reason to still
>> use non-cone-mode. Users with this need usually have a directory full
>> of large files, and they choose which of those large files they need
>> using sparse-checkout. The repository reorganization required to use
>> cone mode for this use is perhaps too great (or they haven't thought
>> about doing it). For this reason, I would prefer that we do not do
>> these checks when not in cone mode.
> 
> If they "only want one file from this directory", isn't the correct
> way to specify that by mentioning the path with a leading slash?
> Otherwise, they'd potentially grab files with similar names from many
> directories, right?  So, even in that usecase, we should still error
> out if they specify a <filename> rather than /<filename>.  Perhaps my
> reasoning should lead with that and I should fix up the warning
> message a bit, but I still think we should probably give a warning
> even for those who are explicitly wanting the usecase you mention.

Checking for the leading slash can be a big help to make sure users
are getting the matches they expect.

> Also, note this is a warning and not an error -- and the warning can
> be suppressed with --skip-checks.

I want to avoid surprising users with new warnings for things they
have been doing for years, especially if these tasks run as part of
a script that checks stderr output as a failure condition.

>>> +test_expect_success 'by default, cone mode will error out when passed files' '
>>> +     git -C repo sparse-checkout reapply --cone &&
>>> +     test_must_fail git -C repo sparse-checkout add .gitignore 2>error &&
>>> +
>>> +     grep ".gitignore.*is not a directory" error
>>> +'
>>> +
>>> +test_expect_success 'by default, non-cone mode will warn on individual files' '
>>> +     git -C repo sparse-checkout reapply --no-cone &&
>>> +     git -C repo sparse-checkout add .gitignore 2>warning &&
>>> +
>>> +     grep "passing directories or less specific patterns is recommended" warning
>>> +'
>>
>> So I would expect this second test to have
>>
>>         test_must_be_empty warning
>>
>> to show that no warning occurs when specifying a file in non-cone-mode.
> 
> or perhaps
> 
> grep "please specify a leading slash to select a single file" warning
> 
> ?

Right. Whatever matches the behavior we land on.

Thanks,
-Stolee

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

[Derrick Stolee]

On 2/15/2022 12:12 AM, Elijah Newren wrote:
> On Mon, Feb 14, 2022 at 8:19 AM Derrick Stolee <derrickstolee@github.com> wrote:
>>
>> On 2/12/2022 7:39 PM, Elijah Newren via GitGitGadget wrote:
>>> 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.)
> 
> Why would that be needed?  The documentation does not specify anything
> about cone vs. non-cone mode, only that the initial working tree will
> only have files from the toplevel directory present.  So, the
> documentation is correct without any needed changes.

OK. That makes sense.

>> 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.
> 
> In which case shouldn't we still show a warning when users specify a
> path rather than a pattern, since the former risks selecting more than
> one file?  (Adding a leading slash should be recommended for such a
> case, right?)

Yes, that is enough of a tweak for me. Thanks.

>> 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.
> 
> We can discuss this more later, but I think it's worthwhile to
> consider what happens even if folks didn't read the BIG warning about
> behavioral changes in the git-sparse-checkout.txt manual, and didn't
> know about the default change, and tried to use it anyway.  If they
> specify something other than a directory, then they'd get an error
> message due to the first six patches of this series -- at which point
> they can look to the manual and decide to add --no-cone to their
> command.  In other words, it basically does the same thing that we'd
> do if we decided to have an interim period with an error when neither
> --cone or --no-cone were specified, other than the error message
> perhaps being slightly different.  What if the user does specify
> directories and doesn't know about the default mode change?  Well,
> that's where the two modes overlap and things work fine (with only
> minor differences in behavior, such as better performance, and files
> from leading directories being included), so the user would be able to
> continue with their work.  So, I'm not sure that an interim period
> where we error out when neither --cone or --no-cone are specified is
> going to buy us much of anything.  And besides, we do have that super
> big scary warning in the manual.  Anyway, I'll bring this all up again
> when I resubmit the final patch broken up into a separate series.

I suppose the warning/error messages are a way to help users
self-correct to this behavior change. I'm willing to see how that
plays out.

Thanks,
-Stolee

Re: [PATCH v2 0/6] sparse checkout: fix a few bugs and check argument validity for set/add

[Derrick Stolee]

On 2/15/2022 3:32 AM, Elijah Newren via GitGitGadget wrote:
> == Maintainer notes ==
> 
> Note1: This has been rebased on origin/master. v1 wasn't picked up anyway,
> so this shouldn't matter, but just pointing it out.
> 
> Note2: There is a small textual and small semantic conflict with
> ds/sparse-checkout-requires-per-worktree-config in seen. I included the diff
> with the correct resolution near the end of this cover letter.
> 
> == Overview ==
> 
> 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 (i.e. in
>    v2.35.0)
>  * Patch 3 fixes sparse-checkout-from-subdirectories-ignores-"prefix" (see
>    https://lore.kernel.org/git/29f0410e-6dfa-2e86-394d-b1fb735e7608@gmail.com/),
>    in cone mode. Since we'll get nasty surprises whether we use or don't use
>    "prefix" for non-cone mode, simply throw an error if set/add subcommands
>    of sparse-checkout are run from a subdirectory.
>  * 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.
> 
> == Update history ==
> 
> Changes since v1:
> 
>  * Dropped the commit changing cone-mode to default (patch 7, which will be
>    split into multiple patches and submitted as a separate series)
>  * Removed the RFC label
>  * Decided to error out when running set/add with paths from a subdirectory
>    in non-cone mode, and added tests
>  * Changed the warning for non-cone mode with individual files to point out
>    that the user is likely trying to select an individual file, but should
>    likely add a leading slash to ensure that is what happens
>  * Fixed typos, removed unnecessary condition checks

Thanks for these updates.

We already discussed the changes that are different from my recommendations,
and I agree with your new approach in those.

I read the range-diff carefully and found this version to resolve all of my
concerns with v1.

Reviewed-by: Derrick Stolee <derrickstolee@github.com>

Thanks,
-Stolee


 

Re: [PATCH 5/7] sparse-checkout: reject non-cone-mode patterns starting with a '#'

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> If this were the only special character case, I'd totally agree, but I
> do worry a bit that escaping this particular case might lead users to
> expect us to escape and fix other special characters from '*?[]!\'.

Sorry, but I do not quite get why it is a problem.  I understand
that the idea behind this "rejection" is that "#" is special only
when it appears in files (as comment introducer) and must be
prefixed with "\", right?

Do any of the wildcard characters mean different things depending on
where they appear?  Isn't '*' a wildcard to match 0-or-more-bytes
whether it appears in files or on the command line, and need to
prefixed with "\" to make it non-special regardless of where it is
found?

> If users have files with those characters and specify an argument with
> one of those, are we to automatically escape them as well?  If we
> don't escape the other characters but do escape '#', aren't we being
> inconsistent?

I do not quite get where you are seeing an inconsistency.  Do you
mean that it is inconsistent that "# comment" is only allowed in
files but not on the command line?  If so, a way to make it
consistent may be to allow "# comment" even from the command line
;-)

Re: [PATCH 5/7] sparse-checkout: reject non-cone-mode patterns starting with a '#'

[Elijah Newren]

On Tue, Feb 15, 2022 at 5:07 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > If this were the only special character case, I'd totally agree, but I
> > do worry a bit that escaping this particular case might lead users to
> > expect us to escape and fix other special characters from '*?[]!\'.
>
> Sorry, but I do not quite get why it is a problem.  I understand
> that the idea behind this "rejection" is that "#" is special only
> when it appears in files (as comment introducer) and must be
> prefixed with "\", right?
>
> Do any of the wildcard characters mean different things depending on
> where they appear?  Isn't '*' a wildcard to match 0-or-more-bytes
> whether it appears in files or on the command line, and need to
> prefixed with "\" to make it non-special regardless of where it is
> found?
>
> > If users have files with those characters and specify an argument with
> > one of those, are we to automatically escape them as well?  If we
> > don't escape the other characters but do escape '#', aren't we being
> > inconsistent?
>
> I do not quite get where you are seeing an inconsistency.  Do you
> mean that it is inconsistent that "# comment" is only allowed in
> files but not on the command line?

I don't understand what distinction you are trying to make between the
file or the command line; for non-cone mode, all positional arguments
to sparse-checkout {add,set} are taken as-is and inserted into the
$GIT_DIR/info/sparse-checkout file directly.

I don't like just assuming that users are specifying paths rather than
patterns, when non-cone mode is all about specifying patterns rather
than paths; it just feels broken to me.  However, since comments can
never match anything and part of the point of the sparse-checkout
command is so that users don't have to edit or look at the
$GIT_DIR/info/sparse-checkout file, it seemed worth flagging.

With all the special characters in non-cone mode ('*?[]!#\') and the
years of training we've given to users to edit
$GITDIR/info/sparse-checkout directly, there's really not much we can
check for; this was the only thing I could think of that seemed
reasonable to flag in non-cone mode.  However, it's really not all
that important to me, so I'll just drop this patch.

>  If so, a way to make it
> consistent may be to allow "# comment" even from the command line
> ;-)

Yes, dropping this patch would keep things consistent, and continue
allowing "# comment" from the command line (even if users are unlikely
to ever look at said comment).  I'll do that.

Re: [PATCH 5/7] sparse-checkout: reject non-cone-mode patterns starting with a '#'

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

>> I do not quite get where you are seeing an inconsistency.  Do you
>> mean that it is inconsistent that "# comment" is only allowed in
>> files but not on the command line?
>
> I don't understand what distinction you are trying to make between the
> file or the command line; for non-cone mode, all positional arguments
> to sparse-checkout {add,set} are taken as-is and inserted into the
> $GIT_DIR/info/sparse-checkout file directly.

If so, then '# comment" from the command line would be a valid way
to spell a comment, no?  It sounds like the right thing to do here
is just passing it through to $GIT_DIR/info/sparse-checkout and let
it become comment, instead of warning, \-quoting, or rejecting.

> I don't like just assuming that users are specifying paths rather than
> patterns, when non-cone mode is all about specifying patterns rather
> than paths; it just feels broken to me.

Oh, I don't like such an assumption, either.  If the user gives a
pathspec, we do assume that is a collection of patterns.  If we are
taking patterns from the command line, treating them as patterns is
the right thing to do.  I do not see how that interacts with what a
path or pattern that begins with a pound should be handled, though.

[PATCH v3 0/5] sparse checkout: fix a few bugs and check argument validity for set/add

[Elijah Newren via GitGitGadget]

== Maintainer notes ==

Note: There is a small textual and small semantic conflict with
ds/sparse-checkout-requires-per-worktree-config in seen. I included the diff
with the correct resolution near the end of this cover letter. If you'd
prefer I rebased on top of ds/sparse-chckout-requires-per-worktree-config,
let me know.

== Overview ==

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 (i.e. in
   v2.35.0)
 * Patch 3 fixes sparse-checkout-from-subdirectories-ignores-"prefix" (see
   https://lore.kernel.org/git/29f0410e-6dfa-2e86-394d-b1fb735e7608@gmail.com/),
   in cone mode. Since we'll get nasty surprises whether we use or don't use
   "prefix" for non-cone mode, simply throw an error if set/add subcommands
   of sparse-checkout are run from a subdirectory.
 * Patches 4-5 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.

== Update history ==

Changes since v2:

 * Dropped patch 5
 * Added Stolee's Reviewed-by

Changes since v1:

 * Dropped the commit changing cone-mode to default (patch 7, which will be
   split into multiple patches and submitted as a separate series)
 * Removed the RFC label
 * Decided to error out when running set/add with paths from a subdirectory
   in non-cone mode, and added tests
 * Changed the warning for non-cone mode with individual files to point out
   that the user is likely trying to select an individual file, but should
   likely add a leading slash to ensure that is what happens
 * Fixed typos, removed unnecessary condition checks

== Conflict resolution ==

Patch to resolve textual and semantic conflict with
ds/sparse-checkout-requires-per-worktree-config:

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
remerge CONFLICT (content): Merge conflict in t/t1091-sparse-checkout-builtin.sh
index 3c6adeb885..3a95d2996d 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -275,24 +275,8 @@ test_expect_success 'sparse-index enabled and disabled' '
     diff -u sparse full | tail -n +3 >actual &&
     test_cmp expect actual &&
 
-<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 286c22e5ec (sparse-checkout: reject arguments in cone-mode that look like patterns)
     git -C repo config --list >config &&
-    ! grep index.sparse config
-|||||||||||||||||||||||||||||||| 89bece5c8c
-        diff -u sparse full | tail -n +3 >actual &&
-        test_cmp expect actual &&
-
-        git -C repo config --list >config &&
-        ! grep index.sparse config
-    )
-================================
-        diff -u sparse full | tail -n +3 >actual &&
-        test_cmp expect actual &&
-
-        git -C repo config --list >config &&
-        test_cmp_config -C repo false index.sparse
-    )
->>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 3ce1138272 (config: make git_configset_get_string_tmp() private)
+    test_cmp_config -C repo false index.sparse
 '
 
 test_expect_success 'cone mode: init and set' '
@@ -532,6 +516,7 @@ test_expect_success 'reapply can handle config options' '
     cat >expect <<-\EOF &&
     core.sparsecheckout=true
     core.sparsecheckoutcone=true
+    index.sparse=false
     EOF
     test_cmp expect actual &&
 
@@ -539,6 +524,8 @@ test_expect_success 'reapply can handle config options' '
     git -C repo config --worktree --list >actual &&
     cat >expect <<-\EOF &&
     core.sparsecheckout=true
+    core.sparsecheckoutcone=false
+    index.sparse=false
     EOF
     test_cmp expect actual &&


== CCs ==

Elijah Newren (5):
  sparse-checkout: correct reapply's handling of options
  sparse-checkout: correctly set non-cone mode when expected
  sparse-checkout: pay attention to prefix for {set, add}
  sparse-checkout: error or warn when given individual files
  sparse-checkout: reject arguments in cone-mode that look like patterns

 builtin/sparse-checkout.c          | 81 ++++++++++++++++++++++++++--
 t/t1091-sparse-checkout-builtin.sh | 87 +++++++++++++++++++++++++++++-
 2 files changed, 161 insertions(+), 7 deletions(-)


base-commit: b80121027d1247a0754b3cc46897fee75c050b44
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1118%2Fnewren%2Fsparse-checkout-default-and-arg-validity-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1118/newren/sparse-checkout-default-and-arg-validity-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1118

Range-diff vs v2:

 1:  5536fe6498e ! 1:  5215b7f7179 sparse-checkout: correct reapply's handling of options
     @@ Commit message
          parsing the command line options instead of before.  Add a test and set
          the initial value at the appropriate time.
      
     +    Reviewed-by: Derrick Stolee <derrickstolee@github.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       ## builtin/sparse-checkout.c ##
 2:  9edad872e0d ! 2:  0c2ab523e74 sparse-checkout: correctly set non-cone mode when expected
     @@ Commit message
          being set to the correct mode, so make sure it is set consistently with
          the config values we will be writing to disk.
      
     +    Reviewed-by: Derrick Stolee <derrickstolee@github.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       ## builtin/sparse-checkout.c ##
 3:  f57820e25d6 ! 3:  e68b0a37ff3 sparse-checkout: pay attention to prefix for {set, add}
     @@ Commit message
          [2] https://lore.kernel.org/git/CABPp-BHXZ-XLxY0a3wCATfdq=6-EjW62RzbxKAoFPeXfJswD2w@mail.gmail.com/
      
          Helped-by: Junio Hamano <gitster@pobox.com>
     +    Reviewed-by: Derrick Stolee <derrickstolee@github.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       ## builtin/sparse-checkout.c ##
 4:  c3bb2a3b2f1 ! 4:  1fdebc1953f sparse-checkout: error or warn when given individual files
     @@ Commit message
          specify exactly name a file because it means they are likely missing
          such a missing leading slash.
      
     +    Reviewed-by: Derrick Stolee <derrickstolee@github.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       ## builtin/sparse-checkout.c ##
 5:  c5d4ae2cfd6 < -:  ----------- sparse-checkout: reject non-cone-mode patterns starting with a '#'
 6:  286c22e5ecd ! 5:  2008542d0c7 sparse-checkout: reject arguments in cone-mode that look like patterns
     @@ Commit message
          Inform users they can pass --skip-checks if they have a directory that
          really does have such special characters in its name.
      
     +    Reviewed-by: Derrick Stolee <derrickstolee@github.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       ## builtin/sparse-checkout.c ##
     @@ builtin/sparse-checkout.c: static void sanitize_paths(int argc, const char **arg
       	if (skip_checks)
       		return;
       
     --	if (!core_sparse_checkout_cone)
     --		for (i = 0; i < argc; i++)
     -+	for (i = 0; i < argc; i++) {
     -+		if (core_sparse_checkout_cone) {
     ++	if (core_sparse_checkout_cone) {
     ++		for (i = 0; i < argc; i++) {
      +			if (argv[i][0] == '/')
      +				die(_("specify directories rather than patterns (no leading slash)"));
      +			if (argv[i][0] == '!')
     @@ builtin/sparse-checkout.c: static void sanitize_paths(int argc, const char **arg
      +			    strchr(argv[i], '[') ||
      +			    strchr(argv[i], ']'))
      +				die(_("specify directories rather than patterns.  If your directory really has any of '*?[]' in it, pass --skip-checks"));
     -+		} else {
     - 			if (argv[i][0] == '#')
     - 				die(_("paths beginning with a '#' must be preceeded by a backslash"));
      +		}
      +	}
     - 
     ++
       	for (i = 0; i < argc; i++) {
       		struct cache_entry *ce;
     + 		struct index_state *index = the_repository->index;
      
       ## t/t1091-sparse-checkout-builtin.sh ##
      @@ t/t1091-sparse-checkout-builtin.sh: test_expect_success BSLASHPSPEC 'pattern-checks: escaped characters' '

-- 
gitgitgadget

[PATCH v3 1/5] sparse-checkout: correct reapply's handling of options

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

Commit 4e256731d6 ("sparse-checkout: enable reapply to take
--[no-]{cone,sparse-index}", 2021-12-14) made it so that reapply could
take additional options but added no tests.  Tests would have shown that
the feature doesn't work because the initial values are set AFTER
parsing the command line options instead of before.  Add a test and set
the initial value at the appropriate time.

Reviewed-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c          |  6 +++---
 t/t1091-sparse-checkout-builtin.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index a311483a7d2..fcd574f5fc1 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -789,15 +789,15 @@ static int sparse_checkout_reapply(int argc, const char **argv)
 	if (!core_apply_sparse_checkout)
 		die(_("must be in a sparse-checkout to reapply sparsity patterns"));
 
+	reapply_opts.cone_mode = -1;
+	reapply_opts.sparse_index = -1;
+
 	argc = parse_options(argc, argv, NULL,
 			     builtin_sparse_checkout_reapply_options,
 			     builtin_sparse_checkout_reapply_usage, 0);
 
 	repo_read_index(the_repository);
 
-	reapply_opts.cone_mode = -1;
-	reapply_opts.sparse_index = -1;
-
 	if (update_modes(&reapply_opts.cone_mode, &reapply_opts.sparse_index))
 		return 1;
 
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 3592d124424..ce5e7c19efa 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -495,6 +495,34 @@ test_expect_failure 'sparse-checkout reapply' '
 	git -C tweak sparse-checkout disable
 '
 
+test_expect_success 'reapply can handle config options' '
+	git -C repo sparse-checkout init --cone --no-sparse-index &&
+	git -C repo config --worktree --list >actual &&
+	cat >expect <<-\EOF &&
+	core.sparsecheckout=true
+	core.sparsecheckoutcone=true
+	EOF
+	test_cmp expect actual &&
+
+	git -C repo sparse-checkout reapply --no-cone --no-sparse-index &&
+	git -C repo config --worktree --list >actual &&
+	cat >expect <<-\EOF &&
+	core.sparsecheckout=true
+	EOF
+	test_cmp expect actual &&
+
+	git -C repo sparse-checkout reapply --cone --sparse-index &&
+	git -C repo config --worktree --list >actual &&
+	cat >expect <<-\EOF &&
+	core.sparsecheckout=true
+	core.sparsecheckoutcone=true
+	index.sparse=true
+	EOF
+	test_cmp expect actual &&
+
+	git -C repo sparse-checkout disable
+'
+
 test_expect_success 'cone mode: set with core.ignoreCase=true' '
 	rm repo/.git/info/sparse-checkout &&
 	git -C repo sparse-checkout init --cone &&
-- 
gitgitgadget

[PATCH v3 2/5] sparse-checkout: correctly set non-cone mode when expected

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

commit f2e3a218e8 ("sparse-checkout: enable `set` to initialize
sparse-checkout mode", 2021-12-14) made the `set` command able to
initialize sparse-checkout mode, but it also had to function when
sparse-checkout mode was already setup and the user just wanted to
change the sparsity paths.  So, if the user passed --cone or --no-cone,
then we should override the current setting, but if they didn't pass
either, we should use whatever the current cone mode setting is.

Unfortunately, there was a small error in the logic in that it would not
set the in-memory cone mode value (core_sparse_checkout_one) when
--no-cone was specified, but since it did set the config setting on
disk, any subsequent git invocation would correctly get non-cone mode.
As such, the error did not previously matter.  However, a subsequent
commit will add some logic that depends on core_sparse_checkout_cone
being set to the correct mode, so make sure it is set consistently with
the config values we will be writing to disk.

Reviewed-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index fcd574f5fc1..fb85a1459c3 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -403,6 +403,7 @@ static int update_modes(int *cone_mode, int *sparse_index)
 		core_sparse_checkout_cone = 1;
 	} else {
 		mode = MODE_ALL_PATTERNS;
+		core_sparse_checkout_cone = 0;
 	}
 	if (record_mode && set_config(mode))
 		return 1;
-- 
gitgitgadget

[PATCH v3 3/5] sparse-checkout: pay attention to prefix for {set, add}

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

In cone mode, non-option arguments to set & add are clearly paths, and
as such, we should pay attention to prefix.

In non-cone mode, it is not clear that folks intend to provide paths
since the inputs are gitignore-style patterns.  Paying attention to
prefix would prevent folks from doing things like
   git sparse-checkout add /.gitattributes
   git sparse-checkout add '/toplevel-dir/*'
In fact, the former will result in
   fatal: '/.gitattributes' is outside repository...
while the later will result in
   fatal: Invalid path '/toplevel-dir': No such file or directory
despite the fact that both are valid gitignore-style patterns that would
select real files if added to the sparse-checkout file.  This might lead
people to just use the path without the leading slash, potentially
resulting in them grabbing files with the same name throughout the
directory hierarchy contrary to their expectations.  See also [1] and
[2].  Adding prefix seems to just be fraught with error; so for now
simply throw an error in non-cone mode when sparse-checkout set/add are
run from a subdirectory.

[1] https://lore.kernel.org/git/e1934710-e228-adc4-d37c-f706883bd27c@gmail.com/
[2] https://lore.kernel.org/git/CABPp-BHXZ-XLxY0a3wCATfdq=6-EjW62RzbxKAoFPeXfJswD2w@mail.gmail.com/

Helped-by: Junio Hamano <gitster@pobox.com>
Reviewed-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c          | 25 ++++++++++++++++++
 t/t1091-sparse-checkout-builtin.sh | 41 ++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index fb85a1459c3..a2192a69219 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -684,6 +684,27 @@ static int modify_pattern_list(int argc, const char **argv, int use_stdin,
 	return result;
 }
 
+static void sanitize_paths(int argc, const char **argv, const char *prefix)
+{
+	if (!argc)
+		return;
+
+	if (prefix && *prefix) {
+		/*
+		 * The args are not pathspecs, so unfortunately we
+		 * cannot imitate how cmd_add() uses parse_pathspec().
+		 */
+		int i;
+		int prefix_len = strlen(prefix);
+
+		if (!core_sparse_checkout_cone)
+			die("please run from the toplevel directory in non-cone mode");
+
+		for (i = 0; i < argc; i++)
+			argv[i] = prefix_path(prefix, prefix_len, argv[i]);
+	}
+}
+
 static char const * const builtin_sparse_checkout_add_usage[] = {
 	N_("git sparse-checkout add (--stdin | <patterns>)"),
 	NULL
@@ -711,6 +732,8 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
 			     builtin_sparse_checkout_add_usage,
 			     PARSE_OPT_KEEP_UNKNOWN);
 
+	sanitize_paths(argc, argv, prefix);
+
 	return modify_pattern_list(argc, argv, add_opts.use_stdin, ADD);
 }
 
@@ -762,6 +785,8 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
 	if (!core_sparse_checkout_cone && argc == 0) {
 		argv = default_patterns;
 		argc = default_patterns_nr;
+	} else {
+		sanitize_paths(argc, argv, prefix);
 	}
 
 	return modify_pattern_list(argc, argv, set_opts.use_stdin, REPLACE);
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index ce5e7c19efa..c1f86b0e02e 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -798,4 +798,45 @@ test_expect_success 'malformed cone-mode patterns' '
 	grep "warning: disabling cone pattern matching" err
 '
 
+test_expect_success 'set from subdir pays attention to prefix' '
+	git -C repo sparse-checkout disable &&
+	git -C repo/deep sparse-checkout set --cone deeper2 ../folder1 &&
+
+	git -C repo sparse-checkout list >actual &&
+
+	cat >expect <<-\EOF &&
+	deep/deeper2
+	folder1
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'add from subdir pays attention to prefix' '
+	git -C repo sparse-checkout set --cone deep/deeper2 &&
+	git -C repo/deep sparse-checkout add deeper1/deepest ../folder1 &&
+
+	git -C repo sparse-checkout list >actual &&
+
+	cat >expect <<-\EOF &&
+	deep/deeper1/deepest
+	deep/deeper2
+	folder1
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'set from subdir in non-cone mode throws an error' '
+	git -C repo sparse-checkout disable &&
+	test_must_fail git -C repo/deep sparse-checkout set --no-cone deeper2 ../folder1 2>error &&
+
+	grep "run from the toplevel directory in non-cone mode" error
+'
+
+test_expect_success 'set from subdir in non-cone mode throws an error' '
+	git -C repo sparse-checkout set --no-cone deep/deeper2 &&
+	test_must_fail git -C repo/deep sparse-checkout add deeper1/deepest ../folder1 2>error &&
+
+	grep "run from the toplevel directory in non-cone mode" error
+'
+
 test_done
-- 
gitgitgadget

[PATCH v3 4/5] sparse-checkout: error or warn when given individual files

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

The set and add subcommands accept multiple positional arguments.
The meaning of these arguments differs slightly in the two modes:

Cone mode only accepts directories.  If given a file, it would
previously treat it as a directory, causing not just the file itself to
be included but all sibling files as well -- likely against users'
expectations.  Throw an error if the specified path is a file in the
index.  Provide a --skip-checks argument to allow users to override
(e.g. for the case when the given path IS a directory on another
branch).

Non-cone mode accepts general gitignore patterns.  There are many
reasons to avoid this mode, but one possible reason to use it instead of
cone mode: to be able to select individual files within a directory.
However, if a file is passed to set/add in non-cone mode, you won't be
selecting a single file, you'll be selecting a file with the same name
in any directory.  Thus users will likely want to prefix any paths they
specify with a leading '/' character; warn users if the patterns they
specify exactly name a file because it means they are likely missing
such a missing leading slash.

Reviewed-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c          | 43 +++++++++++++++++++++++++-----
 t/t1091-sparse-checkout-builtin.sh | 16 ++++++++++-
 2 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index a2192a69219..64583fa704f 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -1,4 +1,5 @@
 #include "builtin.h"
+#include "cache.h"
 #include "config.h"
 #include "dir.h"
 #include "parse-options.h"
@@ -684,8 +685,11 @@ static int modify_pattern_list(int argc, const char **argv, int use_stdin,
 	return result;
 }
 
-static void sanitize_paths(int argc, const char **argv, const char *prefix)
+static void sanitize_paths(int argc, const char **argv,
+			   const char *prefix, int skip_checks)
 {
+	int i;
+
 	if (!argc)
 		return;
 
@@ -694,7 +698,6 @@ static void sanitize_paths(int argc, const char **argv, const char *prefix)
 		 * The args are not pathspecs, so unfortunately we
 		 * cannot imitate how cmd_add() uses parse_pathspec().
 		 */
-		int i;
 		int prefix_len = strlen(prefix);
 
 		if (!core_sparse_checkout_cone)
@@ -703,20 +706,44 @@ static void sanitize_paths(int argc, const char **argv, const char *prefix)
 		for (i = 0; i < argc; i++)
 			argv[i] = prefix_path(prefix, prefix_len, argv[i]);
 	}
+
+	if (skip_checks)
+		return;
+
+	for (i = 0; i < argc; i++) {
+		struct cache_entry *ce;
+		struct index_state *index = the_repository->index;
+		int pos = index_name_pos(index, argv[i], strlen(argv[i]));
+
+		if (pos < 0)
+			continue;
+		ce = index->cache[pos];
+		if (S_ISSPARSEDIR(ce->ce_mode))
+			continue;
+
+		if (core_sparse_checkout_cone)
+			die(_("\"%s\" is not a directory; to treat it as a directory anyway, rerun with --skip-checks"), argv[i]);
+		else
+			warning(_("pass a leading slash before paths such as \"%s\" if you want a single file (see NON-CONE PROBLEMS in the git-sparse-checkout manual)."), argv[i]);
+	}
 }
 
 static char const * const builtin_sparse_checkout_add_usage[] = {
-	N_("git sparse-checkout add (--stdin | <patterns>)"),
+	N_("git sparse-checkout add [--skip-checks] (--stdin | <patterns>)"),
 	NULL
 };
 
 static struct sparse_checkout_add_opts {
+	int skip_checks;
 	int use_stdin;
 } add_opts;
 
 static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
 {
 	static struct option builtin_sparse_checkout_add_options[] = {
+		OPT_BOOL_F(0, "skip-checks", &add_opts.skip_checks,
+			   N_("skip some sanity checks on the given paths that might give false positives"),
+			   PARSE_OPT_NONEG),
 		OPT_BOOL(0, "stdin", &add_opts.use_stdin,
 			 N_("read patterns from standard in")),
 		OPT_END(),
@@ -732,19 +759,20 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
 			     builtin_sparse_checkout_add_usage,
 			     PARSE_OPT_KEEP_UNKNOWN);
 
-	sanitize_paths(argc, argv, prefix);
+	sanitize_paths(argc, argv, prefix, add_opts.skip_checks);
 
 	return modify_pattern_list(argc, argv, add_opts.use_stdin, ADD);
 }
 
 static char const * const builtin_sparse_checkout_set_usage[] = {
-	N_("git sparse-checkout set [--[no-]cone] [--[no-]sparse-index] (--stdin | <patterns>)"),
+	N_("git sparse-checkout set [--[no-]cone] [--[no-]sparse-index] [--skip-checks] (--stdin | <patterns>)"),
 	NULL
 };
 
 static struct sparse_checkout_set_opts {
 	int cone_mode;
 	int sparse_index;
+	int skip_checks;
 	int use_stdin;
 } set_opts;
 
@@ -758,6 +786,9 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
 			 N_("initialize the sparse-checkout in cone mode")),
 		OPT_BOOL(0, "sparse-index", &set_opts.sparse_index,
 			 N_("toggle the use of a sparse index")),
+		OPT_BOOL_F(0, "skip-checks", &set_opts.skip_checks,
+			   N_("skip some sanity checks on the given paths that might give false positives"),
+			   PARSE_OPT_NONEG),
 		OPT_BOOL_F(0, "stdin", &set_opts.use_stdin,
 			   N_("read patterns from standard in"),
 			   PARSE_OPT_NONEG),
@@ -786,7 +817,7 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
 		argv = default_patterns;
 		argc = default_patterns_nr;
 	} else {
-		sanitize_paths(argc, argv, prefix);
+		sanitize_paths(argc, argv, prefix, set_opts.skip_checks);
 	}
 
 	return modify_pattern_list(argc, argv, set_opts.use_stdin, REPLACE);
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index c1f86b0e02e..3b39329266b 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -562,7 +562,7 @@ test_expect_success 'different sparse-checkouts with worktrees' '
 '
 
 test_expect_success 'set using filename keeps file on-disk' '
-	git -C repo sparse-checkout set a deep &&
+	git -C repo sparse-checkout set --skip-checks a deep &&
 	cat >expect <<-\EOF &&
 	/*
 	!/*/
@@ -839,4 +839,18 @@ test_expect_success 'set from subdir in non-cone mode throws an error' '
 	grep "run from the toplevel directory in non-cone mode" error
 '
 
+test_expect_success 'by default, cone mode will error out when passed files' '
+	git -C repo sparse-checkout reapply --cone &&
+	test_must_fail git -C repo sparse-checkout add .gitignore 2>error &&
+
+	grep ".gitignore.*is not a directory" error
+'
+
+test_expect_success 'by default, non-cone mode will warn on individual files' '
+	git -C repo sparse-checkout reapply --no-cone &&
+	git -C repo sparse-checkout add .gitignore 2>warning &&
+
+	grep "pass a leading slash before paths.*if you want a single file" warning
+'
+
 test_done
-- 
gitgitgadget

[PATCH v3 5/5] sparse-checkout: reject arguments in cone-mode that look like patterns

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

In sparse-checkout add/set under cone mode, the arguments passed are
supposed to be directories rather than gitignore-style patterns.
However, given the amount of effort spent in the manual discussing
patterns, it is easy for users to assume they need to pass patterns such
as
   /foo/*
or
   !/bar/*/
or perhaps they really do ignore the directory rule and specify a
random gitignore-style pattern like
   *.c

To help catch such mistakes, throw an error if any of the positional
arguments:
  * starts with any of '/!'
  * contains any of '*\?[]'

Inform users they can pass --skip-checks if they have a directory that
really does have such special characters in its name.

Reviewed-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c          | 14 ++++++++++++++
 t/t1091-sparse-checkout-builtin.sh |  2 +-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 64583fa704f..1a74173f7a1 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -710,6 +710,20 @@ static void sanitize_paths(int argc, const char **argv,
 	if (skip_checks)
 		return;
 
+	if (core_sparse_checkout_cone) {
+		for (i = 0; i < argc; i++) {
+			if (argv[i][0] == '/')
+				die(_("specify directories rather than patterns (no leading slash)"));
+			if (argv[i][0] == '!')
+				die(_("specify directories rather than patterns.  If your directory starts with a '!', pass --skip-checks"));
+			if (strchr(argv[i], '*') ||
+			    strchr(argv[i], '?') ||
+			    strchr(argv[i], '[') ||
+			    strchr(argv[i], ']'))
+				die(_("specify directories rather than patterns.  If your directory really has any of '*?[]' in it, pass --skip-checks"));
+		}
+	}
+
 	for (i = 0; i < argc; i++) {
 		struct cache_entry *ce;
 		struct index_state *index = the_repository->index;
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 3b39329266b..421127d99c2 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -673,7 +673,7 @@ test_expect_success BSLASHPSPEC 'pattern-checks: escaped characters' '
 	git -C escaped reset --hard $COMMIT &&
 	check_files escaped "a deep folder1 folder2 zbad\\dir zdoes*exist" zglob[!a]? &&
 	git -C escaped sparse-checkout init --cone &&
-	git -C escaped sparse-checkout set zbad\\dir/bogus "zdoes*not*exist" "zdoes*exist" "zglob[!a]?" &&
+	git -C escaped sparse-checkout set --skip-checks zbad\\dir/bogus "zdoes*not*exist" "zdoes*exist" "zglob[!a]?" &&
 	cat >expect <<-\EOF &&
 	/*
 	!/*/
-- 
gitgitgadget

Re: [PATCH v3 0/5] sparse checkout: fix a few bugs and check argument validity for set/add

[Junio C Hamano]

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

> Patch to resolve textual and semantic conflict with
> ds/sparse-checkout-requires-per-worktree-config:
>
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> remerge CONFLICT (content): Merge conflict in t/t1091-sparse-checkout-builtin.sh
> index 3c6adeb885..3a95d2996d 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -275,24 +275,8 @@ test_expect_success 'sparse-index enabled and disabled' '
>      diff -u sparse full | tail -n +3 >actual &&
>      test_cmp expect actual &&
>  
> -<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 286c22e5ec (sparse-checkout: reject arguments in cone-mode that look like patterns)
>      git -C repo config --list >config &&
> -    ! grep index.sparse config
> -|||||||||||||||||||||||||||||||| 89bece5c8c
> -        diff -u sparse full | tail -n +3 >actual &&
> -        test_cmp expect actual &&
> -
> -        git -C repo config --list >config &&
> -        ! grep index.sparse config
> -    )
> -================================
> -        diff -u sparse full | tail -n +3 >actual &&
> -        test_cmp expect actual &&
> -
> -        git -C repo config --list >config &&
> -        test_cmp_config -C repo false index.sparse
> -    )
> ->>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 3ce1138272 (config: make git_configset_get_string_tmp() private)
> +    test_cmp_config -C repo false index.sparse
>  '

The above is quite straight-forward.

>  test_expect_success 'cone mode: init and set' '
> @@ -532,6 +516,7 @@ test_expect_success 'reapply can handle config options' '
>      cat >expect <<-\EOF &&
>      core.sparsecheckout=true
>      core.sparsecheckoutcone=true
> +    index.sparse=false
>      EOF
>      test_cmp expect actual &&
>  
> @@ -539,6 +524,8 @@ test_expect_success 'reapply can handle config options' '
>      git -C repo config --worktree --list >actual &&
>      cat >expect <<-\EOF &&
>      core.sparsecheckout=true
> +    core.sparsecheckoutcone=false
> +    index.sparse=false
>      EOF
>      test_cmp expect actual &&

These differences are a bit nasty, but of course understandable once
it is shown X-<.

Thanks.

Re: [PATCH v3 5/5] sparse-checkout: reject arguments in cone-mode that look like patterns

[Ævar Arnfjörð Bjarmason]

On Wed, Feb 16 2022, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <newren@gmail.com>
>
> In sparse-checkout add/set under cone mode, the arguments passed are
> supposed to be directories rather than gitignore-style patterns.
> However, given the amount of effort spent in the manual discussing
> patterns, it is easy for users to assume they need to pass patterns such
> as
>    /foo/*
> or
>    !/bar/*/
> or perhaps they really do ignore the directory rule and specify a
> random gitignore-style pattern like
>    *.c
>
> To help catch such mistakes, throw an error if any of the positional
> arguments:
>   * starts with any of '/!'
>   * contains any of '*\?[]'

But not "\" itself, we're just escaping the "?" here?...

> +	if (core_sparse_checkout_cone) {
> +		for (i = 0; i < argc; i++) {
> +			if (argv[i][0] == '/')
> +				die(_("specify directories rather than patterns (no leading slash)"));
> +			if (argv[i][0] == '!')
> +				die(_("specify directories rather than patterns.  If your directory starts with a '!', pass --skip-checks"));
> +			if (strchr(argv[i], '*') ||
> +			    strchr(argv[i], '?') ||
> +			    strchr(argv[i], '[') ||
> +			    strchr(argv[i], ']'))
> +				die(_("specify directories rather than patterns.  If your directory really has any of '*?[]' in it, pass --skip-checks"));

Isn't this nested || a reinvention of a simpler strtok() or strtok_r()
call? I.e. (untested):

	const char *p;
	const char *wildcards = "*?[]";
	if (strtok_r(argv[i], wildcards, &p))
		die(_("specify... has ony of '%s' in it...", wildcards));

That would also allow parameterizing the set of characters for
translators.

Re: [PATCH v3 5/5] sparse-checkout: reject arguments in cone-mode that look like patterns

[Elijah Newren]

On Wed, Feb 16, 2022 at 1:57 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Wed, Feb 16 2022, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > In sparse-checkout add/set under cone mode, the arguments passed are
> > supposed to be directories rather than gitignore-style patterns.
> > However, given the amount of effort spent in the manual discussing
> > patterns, it is easy for users to assume they need to pass patterns such
> > as
> >    /foo/*
> > or
> >    !/bar/*/
> > or perhaps they really do ignore the directory rule and specify a
> > random gitignore-style pattern like
> >    *.c
> >
> > To help catch such mistakes, throw an error if any of the positional
> > arguments:
> >   * starts with any of '/!'
> >   * contains any of '*\?[]'
>
> But not "\" itself, we're just escaping the "?" here?...

Nah, I just missed that one.  I should add it below.

> > +     if (core_sparse_checkout_cone) {
> > +             for (i = 0; i < argc; i++) {
> > +                     if (argv[i][0] == '/')
> > +                             die(_("specify directories rather than patterns (no leading slash)"));
> > +                     if (argv[i][0] == '!')
> > +                             die(_("specify directories rather than patterns.  If your directory starts with a '!', pass --skip-checks"));
> > +                     if (strchr(argv[i], '*') ||
> > +                         strchr(argv[i], '?') ||
> > +                         strchr(argv[i], '[') ||
> > +                         strchr(argv[i], ']'))
> > +                             die(_("specify directories rather than patterns.  If your directory really has any of '*?[]' in it, pass --skip-checks"));
>
> Isn't this nested || a reinvention of a simpler strtok() or strtok_r()
> call? I.e. (untested):
>
>         const char *p;
>         const char *wildcards = "*?[]";
>         if (strtok_r(argv[i], wildcards, &p))
>                 die(_("specify... has ony of '%s' in it...", wildcards));
>
> That would also allow parameterizing the set of characters for
> translators.

I considered strtok, but strtok & strtok_r are documented as modifying
their argument.  Perhaps they don't modify the argument if they don't
find any of the listed tokens, but I didn't want to rely on that since
I found no guarantees in the documentation.

Re: [PATCH v3 5/5] sparse-checkout: reject arguments in cone-mode that look like patterns

[Victoria Dye]

Elijah Newren wrote:
> On Wed, Feb 16, 2022 at 1:57 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> On Wed, Feb 16 2022, Elijah Newren via GitGitGadget wrote:
>>
>>> From: Elijah Newren <newren@gmail.com>
>>>
>>> In sparse-checkout add/set under cone mode, the arguments passed are
>>> supposed to be directories rather than gitignore-style patterns.
>>> However, given the amount of effort spent in the manual discussing
>>> patterns, it is easy for users to assume they need to pass patterns such
>>> as
>>>    /foo/*
>>> or
>>>    !/bar/*/
>>> or perhaps they really do ignore the directory rule and specify a
>>> random gitignore-style pattern like
>>>    *.c
>>>
>>> To help catch such mistakes, throw an error if any of the positional
>>> arguments:
>>>   * starts with any of '/!'
>>>   * contains any of '*\?[]'
>>
>> But not "\" itself, we're just escaping the "?" here?...
> 
> Nah, I just missed that one.  I should add it below.
> 
>>> +     if (core_sparse_checkout_cone) {
>>> +             for (i = 0; i < argc; i++) {
>>> +                     if (argv[i][0] == '/')
>>> +                             die(_("specify directories rather than patterns (no leading slash)"));
>>> +                     if (argv[i][0] == '!')
>>> +                             die(_("specify directories rather than patterns.  If your directory starts with a '!', pass --skip-checks"));
>>> +                     if (strchr(argv[i], '*') ||
>>> +                         strchr(argv[i], '?') ||
>>> +                         strchr(argv[i], '[') ||
>>> +                         strchr(argv[i], ']'))
>>> +                             die(_("specify directories rather than patterns.  If your directory really has any of '*?[]' in it, pass --skip-checks"));
>>
>> Isn't this nested || a reinvention of a simpler strtok() or strtok_r()
>> call? I.e. (untested):
>>
>>         const char *p;
>>         const char *wildcards = "*?[]";
>>         if (strtok_r(argv[i], wildcards, &p))
>>                 die(_("specify... has ony of '%s' in it...", wildcards));
>>
>> That would also allow parameterizing the set of characters for
>> translators.
> 
> I considered strtok, but strtok & strtok_r are documented as modifying
> their argument.  Perhaps they don't modify the argument if they don't
> find any of the listed tokens, but I didn't want to rely on that since
> I found no guarantees in the documentation.

Maybe `strpbrk` would work? Unless I'm misunderstanding, it should
consolidate the condition to one line without potentially modifying the
arguments. E.g.: 

	if (!strpbrk(argv[i], "*?[]"))
		die(_("specify directories rather than patterns.  If your directory really has any of '*?[]' in it, pass --skip-checks"));

Re: [PATCH v3 5/5] sparse-checkout: reject arguments in cone-mode that look like patterns

[Junio C Hamano]

Victoria Dye <vdye@github.com> writes:

>> I considered strtok, but strtok & strtok_r are documented as modifying
>> their argument.  Perhaps they don't modify the argument if they don't
>> find any of the listed tokens, but I didn't want to rely on that since
>> I found no guarantees in the documentation.
>
> Maybe `strpbrk` would work? Unless I'm misunderstanding, it should
> consolidate the condition to one line without potentially modifying the
> arguments. E.g.: 
>
> 	if (!strpbrk(argv[i], "*?[]"))
> 		die(_("specify directories rather than patterns.  If your directory really has any of '*?[]' in it, pass --skip-checks"));

Yes, either that or strspn() are good match for the problem (I
suspect Ævar actually meant strspn() not strtok() in the first
place, though ;-).

Re: [PATCH v3 5/5] sparse-checkout: reject arguments in cone-mode that look like patterns

[Elijah Newren]

On Wed, Feb 16, 2022 at 9:20 AM Victoria Dye <vdye@github.com> wrote:
>
> Elijah Newren wrote:
> > On Wed, Feb 16, 2022 at 1:57 AM Ævar Arnfjörð Bjarmason
> > <avarab@gmail.com> wrote:
> >>
> >> On Wed, Feb 16 2022, Elijah Newren via GitGitGadget wrote:
> >>
> >>> From: Elijah Newren <newren@gmail.com>
> >>>
> >>> In sparse-checkout add/set under cone mode, the arguments passed are
> >>> supposed to be directories rather than gitignore-style patterns.
> >>> However, given the amount of effort spent in the manual discussing
> >>> patterns, it is easy for users to assume they need to pass patterns such
> >>> as
> >>>    /foo/*
> >>> or
> >>>    !/bar/*/
> >>> or perhaps they really do ignore the directory rule and specify a
> >>> random gitignore-style pattern like
> >>>    *.c
> >>>
> >>> To help catch such mistakes, throw an error if any of the positional
> >>> arguments:
> >>>   * starts with any of '/!'
> >>>   * contains any of '*\?[]'
> >>
> >> But not "\" itself, we're just escaping the "?" here?...
> >
> > Nah, I just missed that one.  I should add it below.
> >
> >>> +     if (core_sparse_checkout_cone) {
> >>> +             for (i = 0; i < argc; i++) {
> >>> +                     if (argv[i][0] == '/')
> >>> +                             die(_("specify directories rather than patterns (no leading slash)"));
> >>> +                     if (argv[i][0] == '!')
> >>> +                             die(_("specify directories rather than patterns.  If your directory starts with a '!', pass --skip-checks"));
> >>> +                     if (strchr(argv[i], '*') ||
> >>> +                         strchr(argv[i], '?') ||
> >>> +                         strchr(argv[i], '[') ||
> >>> +                         strchr(argv[i], ']'))
> >>> +                             die(_("specify directories rather than patterns.  If your directory really has any of '*?[]' in it, pass --skip-checks"));
> >>
> >> Isn't this nested || a reinvention of a simpler strtok() or strtok_r()
> >> call? I.e. (untested):
> >>
> >>         const char *p;
> >>         const char *wildcards = "*?[]";
> >>         if (strtok_r(argv[i], wildcards, &p))
> >>                 die(_("specify... has ony of '%s' in it...", wildcards));
> >>
> >> That would also allow parameterizing the set of characters for
> >> translators.
> >
> > I considered strtok, but strtok & strtok_r are documented as modifying
> > their argument.  Perhaps they don't modify the argument if they don't
> > find any of the listed tokens, but I didn't want to rely on that since
> > I found no guarantees in the documentation.
>
> Maybe `strpbrk` would work? Unless I'm misunderstanding, it should
> consolidate the condition to one line without potentially modifying the
> arguments. E.g.:
>
>         if (!strpbrk(argv[i], "*?[]"))
>                 die(_("specify directories rather than patterns.  If your directory really has any of '*?[]' in it, pass --skip-checks"));

Ah, perfect -- thanks for the pointer!  (Though I think the logic
needs to be reversed; error if we find any of those characters rather
than error if we don't.)

Re: [PATCH v3 5/5] sparse-checkout: reject arguments in cone-mode that look like patterns

[Elijah Newren]

On Wed, Feb 16, 2022 at 10:49 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Victoria Dye <vdye@github.com> writes:
>
> >> I considered strtok, but strtok & strtok_r are documented as modifying
> >> their argument.  Perhaps they don't modify the argument if they don't
> >> find any of the listed tokens, but I didn't want to rely on that since
> >> I found no guarantees in the documentation.
> >
> > Maybe `strpbrk` would work? Unless I'm misunderstanding, it should
> > consolidate the condition to one line without potentially modifying the
> > arguments. E.g.:
> >
> >       if (!strpbrk(argv[i], "*?[]"))
> >               die(_("specify directories rather than patterns.  If your directory really has any of '*?[]' in it, pass --skip-checks"));
>
> Yes, either that or strspn() are good match for the problem (I
> suspect Ævar actually meant strspn() not strtok() in the first
> place, though ;-).

After reading up on it, I think you mean strcspn() rather than
strspn(), yes?  Either way, strpbrk() seems more direct to me, so I'll
use it.

Re: [PATCH v3 5/5] sparse-checkout: reject arguments in cone-mode that look like patterns

[Elijah Newren]

I misspoke...

On Wed, Feb 16, 2022 at 8:54 AM Elijah Newren <newren@gmail.com> wrote:
>
> On Wed, Feb 16, 2022 at 1:57 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
> >
> > On Wed, Feb 16 2022, Elijah Newren via GitGitGadget wrote:
> >
> > > From: Elijah Newren <newren@gmail.com>
> > >
> > > In sparse-checkout add/set under cone mode, the arguments passed are
> > > supposed to be directories rather than gitignore-style patterns.
> > > However, given the amount of effort spent in the manual discussing
> > > patterns, it is easy for users to assume they need to pass patterns such
> > > as
> > >    /foo/*
> > > or
> > >    !/bar/*/
> > > or perhaps they really do ignore the directory rule and specify a
> > > random gitignore-style pattern like
> > >    *.c
> > >
> > > To help catch such mistakes, throw an error if any of the positional
> > > arguments:
> > >   * starts with any of '/!'
> > >   * contains any of '*\?[]'
> >
> > But not "\" itself, we're just escaping the "?" here?...
>
> Nah, I just missed that one.  I should add it below.

I should not have included '\' in the commit message; the code was
correct.  The code treats backslashes specially on Windows (see
t1091.46 -- 'cone mode replaces backslashes with slashes'), and I'd
rather avoid having platform-specific error messages/checks, so I left
backslashes out.  But I do need to fix up the commit message to match
the code.

[PATCH v4 0/5] sparse checkout: fix a few bugs and check argument validity for set/add

[Elijah Newren via GitGitGadget]

== Maintainer notes ==

Note: There is a small textual and small semantic conflict with
ds/sparse-checkout-requires-per-worktree-config in seen. I included the diff
with the correct resolution near the end of this cover letter. If you'd
prefer I rebased on top of ds/sparse-chckout-requires-per-worktree-config,
let me know.

== Overview ==

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 (i.e. in
   v2.35.0)
 * Patch 3 fixes sparse-checkout-from-subdirectories-ignores-"prefix" (see
   https://lore.kernel.org/git/29f0410e-6dfa-2e86-394d-b1fb735e7608@gmail.com/),
   in cone mode. Since we'll get nasty surprises whether we use or don't use
   "prefix" for non-cone mode, simply throw an error if set/add subcommands
   of sparse-checkout are run from a subdirectory.
 * Patches 4-5 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.

== Update history ==

Changes since v3:

 * Use strpbrk() instead of multiple strchr(), fix commit message relative
   to backslashes.

Changes since v2:

 * Dropped patch 5
 * Added Stolee's Reviewed-by

Changes since v1:

 * Dropped the commit changing cone-mode to default (patch 7, which will be
   split into multiple patches and submitted as a separate series)
 * Removed the RFC label
 * Decided to error out when running set/add with paths from a subdirectory
   in non-cone mode, and added tests
 * Changed the warning for non-cone mode with individual files to point out
   that the user is likely trying to select an individual file, but should
   likely add a leading slash to ensure that is what happens
 * Fixed typos, removed unnecessary condition checks

== Conflict resolution ==

Patch to resolve textual and semantic conflict with
ds/sparse-checkout-requires-per-worktree-config:

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
remerge CONFLICT (content): Merge conflict in t/t1091-sparse-checkout-builtin.sh
index 3c6adeb885..3a95d2996d 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -275,24 +275,8 @@ test_expect_success 'sparse-index enabled and disabled' '
     diff -u sparse full | tail -n +3 >actual &&
     test_cmp expect actual &&
 
-<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 286c22e5ec (sparse-checkout: reject arguments in cone-mode that look like patterns)
     git -C repo config --list >config &&
-    ! grep index.sparse config
-|||||||||||||||||||||||||||||||| 89bece5c8c
-        diff -u sparse full | tail -n +3 >actual &&
-        test_cmp expect actual &&
-
-        git -C repo config --list >config &&
-        ! grep index.sparse config
-    )
-================================
-        diff -u sparse full | tail -n +3 >actual &&
-        test_cmp expect actual &&
-
-        git -C repo config --list >config &&
-        test_cmp_config -C repo false index.sparse
-    )
->>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 3ce1138272 (config: make git_configset_get_string_tmp() private)
+    test_cmp_config -C repo false index.sparse
 '
 
 test_expect_success 'cone mode: init and set' '
@@ -532,6 +516,7 @@ test_expect_success 'reapply can handle config options' '
     cat >expect <<-\EOF &&
     core.sparsecheckout=true
     core.sparsecheckoutcone=true
+    index.sparse=false
     EOF
     test_cmp expect actual &&
 
@@ -539,6 +524,8 @@ test_expect_success 'reapply can handle config options' '
     git -C repo config --worktree --list >actual &&
     cat >expect <<-\EOF &&
     core.sparsecheckout=true
+    core.sparsecheckoutcone=false
+    index.sparse=false
     EOF
     test_cmp expect actual &&


== CCs ==

Elijah Newren (5):
  sparse-checkout: correct reapply's handling of options
  sparse-checkout: correctly set non-cone mode when expected
  sparse-checkout: pay attention to prefix for {set, add}
  sparse-checkout: error or warn when given individual files
  sparse-checkout: reject arguments in cone-mode that look like patterns

 builtin/sparse-checkout.c          | 78 +++++++++++++++++++++++++--
 t/t1091-sparse-checkout-builtin.sh | 87 +++++++++++++++++++++++++++++-
 2 files changed, 158 insertions(+), 7 deletions(-)


base-commit: b80121027d1247a0754b3cc46897fee75c050b44
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1118%2Fnewren%2Fsparse-checkout-default-and-arg-validity-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1118/newren/sparse-checkout-default-and-arg-validity-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1118

Range-diff vs v3:

 1:  5215b7f7179 = 1:  5215b7f7179 sparse-checkout: correct reapply's handling of options
 2:  0c2ab523e74 = 2:  0c2ab523e74 sparse-checkout: correctly set non-cone mode when expected
 3:  e68b0a37ff3 = 3:  e68b0a37ff3 sparse-checkout: pay attention to prefix for {set, add}
 4:  1fdebc1953f = 4:  1fdebc1953f sparse-checkout: error or warn when given individual files
 5:  2008542d0c7 ! 5:  d68682c25c4 sparse-checkout: reject arguments in cone-mode that look like patterns
     @@ Commit message
          To help catch such mistakes, throw an error if any of the positional
          arguments:
            * starts with any of '/!'
     -      * contains any of '*\?[]'
     +      * contains any of '*?[]'
      
          Inform users they can pass --skip-checks if they have a directory that
     -    really does have such special characters in its name.
     +    really does have such special characters in its name.  (We exclude '\'
     +    because of sparse-checkout's special handling of backslashes; see
     +    the MINGW test in t1091.46.)
      
          Reviewed-by: Derrick Stolee <derrickstolee@github.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
     @@ builtin/sparse-checkout.c: static void sanitize_paths(int argc, const char **arg
      +				die(_("specify directories rather than patterns (no leading slash)"));
      +			if (argv[i][0] == '!')
      +				die(_("specify directories rather than patterns.  If your directory starts with a '!', pass --skip-checks"));
     -+			if (strchr(argv[i], '*') ||
     -+			    strchr(argv[i], '?') ||
     -+			    strchr(argv[i], '[') ||
     -+			    strchr(argv[i], ']'))
     -+				die(_("specify directories rather than patterns.  If your directory really has any of '*?[]' in it, pass --skip-checks"));
     ++			if (strpbrk(argv[i], "*?[]"))
     ++				die(_("specify directories rather than patterns.  If your directory really has any of '*?[]\\' in it, pass --skip-checks"));
      +		}
      +	}
      +

-- 
gitgitgadget

[PATCH v4 1/5] sparse-checkout: correct reapply's handling of options

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

Commit 4e256731d6 ("sparse-checkout: enable reapply to take
--[no-]{cone,sparse-index}", 2021-12-14) made it so that reapply could
take additional options but added no tests.  Tests would have shown that
the feature doesn't work because the initial values are set AFTER
parsing the command line options instead of before.  Add a test and set
the initial value at the appropriate time.

Reviewed-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c          |  6 +++---
 t/t1091-sparse-checkout-builtin.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index a311483a7d2..fcd574f5fc1 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -789,15 +789,15 @@ static int sparse_checkout_reapply(int argc, const char **argv)
 	if (!core_apply_sparse_checkout)
 		die(_("must be in a sparse-checkout to reapply sparsity patterns"));
 
+	reapply_opts.cone_mode = -1;
+	reapply_opts.sparse_index = -1;
+
 	argc = parse_options(argc, argv, NULL,
 			     builtin_sparse_checkout_reapply_options,
 			     builtin_sparse_checkout_reapply_usage, 0);
 
 	repo_read_index(the_repository);
 
-	reapply_opts.cone_mode = -1;
-	reapply_opts.sparse_index = -1;
-
 	if (update_modes(&reapply_opts.cone_mode, &reapply_opts.sparse_index))
 		return 1;
 
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 3592d124424..ce5e7c19efa 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -495,6 +495,34 @@ test_expect_failure 'sparse-checkout reapply' '
 	git -C tweak sparse-checkout disable
 '
 
+test_expect_success 'reapply can handle config options' '
+	git -C repo sparse-checkout init --cone --no-sparse-index &&
+	git -C repo config --worktree --list >actual &&
+	cat >expect <<-\EOF &&
+	core.sparsecheckout=true
+	core.sparsecheckoutcone=true
+	EOF
+	test_cmp expect actual &&
+
+	git -C repo sparse-checkout reapply --no-cone --no-sparse-index &&
+	git -C repo config --worktree --list >actual &&
+	cat >expect <<-\EOF &&
+	core.sparsecheckout=true
+	EOF
+	test_cmp expect actual &&
+
+	git -C repo sparse-checkout reapply --cone --sparse-index &&
+	git -C repo config --worktree --list >actual &&
+	cat >expect <<-\EOF &&
+	core.sparsecheckout=true
+	core.sparsecheckoutcone=true
+	index.sparse=true
+	EOF
+	test_cmp expect actual &&
+
+	git -C repo sparse-checkout disable
+'
+
 test_expect_success 'cone mode: set with core.ignoreCase=true' '
 	rm repo/.git/info/sparse-checkout &&
 	git -C repo sparse-checkout init --cone &&
-- 
gitgitgadget

[PATCH v4 2/5] sparse-checkout: correctly set non-cone mode when expected

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

commit f2e3a218e8 ("sparse-checkout: enable `set` to initialize
sparse-checkout mode", 2021-12-14) made the `set` command able to
initialize sparse-checkout mode, but it also had to function when
sparse-checkout mode was already setup and the user just wanted to
change the sparsity paths.  So, if the user passed --cone or --no-cone,
then we should override the current setting, but if they didn't pass
either, we should use whatever the current cone mode setting is.

Unfortunately, there was a small error in the logic in that it would not
set the in-memory cone mode value (core_sparse_checkout_one) when
--no-cone was specified, but since it did set the config setting on
disk, any subsequent git invocation would correctly get non-cone mode.
As such, the error did not previously matter.  However, a subsequent
commit will add some logic that depends on core_sparse_checkout_cone
being set to the correct mode, so make sure it is set consistently with
the config values we will be writing to disk.

Reviewed-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index fcd574f5fc1..fb85a1459c3 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -403,6 +403,7 @@ static int update_modes(int *cone_mode, int *sparse_index)
 		core_sparse_checkout_cone = 1;
 	} else {
 		mode = MODE_ALL_PATTERNS;
+		core_sparse_checkout_cone = 0;
 	}
 	if (record_mode && set_config(mode))
 		return 1;
-- 
gitgitgadget

[PATCH v4 3/5] sparse-checkout: pay attention to prefix for {set, add}

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

In cone mode, non-option arguments to set & add are clearly paths, and
as such, we should pay attention to prefix.

In non-cone mode, it is not clear that folks intend to provide paths
since the inputs are gitignore-style patterns.  Paying attention to
prefix would prevent folks from doing things like
   git sparse-checkout add /.gitattributes
   git sparse-checkout add '/toplevel-dir/*'
In fact, the former will result in
   fatal: '/.gitattributes' is outside repository...
while the later will result in
   fatal: Invalid path '/toplevel-dir': No such file or directory
despite the fact that both are valid gitignore-style patterns that would
select real files if added to the sparse-checkout file.  This might lead
people to just use the path without the leading slash, potentially
resulting in them grabbing files with the same name throughout the
directory hierarchy contrary to their expectations.  See also [1] and
[2].  Adding prefix seems to just be fraught with error; so for now
simply throw an error in non-cone mode when sparse-checkout set/add are
run from a subdirectory.

[1] https://lore.kernel.org/git/e1934710-e228-adc4-d37c-f706883bd27c@gmail.com/
[2] https://lore.kernel.org/git/CABPp-BHXZ-XLxY0a3wCATfdq=6-EjW62RzbxKAoFPeXfJswD2w@mail.gmail.com/

Helped-by: Junio Hamano <gitster@pobox.com>
Reviewed-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c          | 25 ++++++++++++++++++
 t/t1091-sparse-checkout-builtin.sh | 41 ++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index fb85a1459c3..a2192a69219 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -684,6 +684,27 @@ static int modify_pattern_list(int argc, const char **argv, int use_stdin,
 	return result;
 }
 
+static void sanitize_paths(int argc, const char **argv, const char *prefix)
+{
+	if (!argc)
+		return;
+
+	if (prefix && *prefix) {
+		/*
+		 * The args are not pathspecs, so unfortunately we
+		 * cannot imitate how cmd_add() uses parse_pathspec().
+		 */
+		int i;
+		int prefix_len = strlen(prefix);
+
+		if (!core_sparse_checkout_cone)
+			die("please run from the toplevel directory in non-cone mode");
+
+		for (i = 0; i < argc; i++)
+			argv[i] = prefix_path(prefix, prefix_len, argv[i]);
+	}
+}
+
 static char const * const builtin_sparse_checkout_add_usage[] = {
 	N_("git sparse-checkout add (--stdin | <patterns>)"),
 	NULL
@@ -711,6 +732,8 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
 			     builtin_sparse_checkout_add_usage,
 			     PARSE_OPT_KEEP_UNKNOWN);
 
+	sanitize_paths(argc, argv, prefix);
+
 	return modify_pattern_list(argc, argv, add_opts.use_stdin, ADD);
 }
 
@@ -762,6 +785,8 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
 	if (!core_sparse_checkout_cone && argc == 0) {
 		argv = default_patterns;
 		argc = default_patterns_nr;
+	} else {
+		sanitize_paths(argc, argv, prefix);
 	}
 
 	return modify_pattern_list(argc, argv, set_opts.use_stdin, REPLACE);
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index ce5e7c19efa..c1f86b0e02e 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -798,4 +798,45 @@ test_expect_success 'malformed cone-mode patterns' '
 	grep "warning: disabling cone pattern matching" err
 '
 
+test_expect_success 'set from subdir pays attention to prefix' '
+	git -C repo sparse-checkout disable &&
+	git -C repo/deep sparse-checkout set --cone deeper2 ../folder1 &&
+
+	git -C repo sparse-checkout list >actual &&
+
+	cat >expect <<-\EOF &&
+	deep/deeper2
+	folder1
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'add from subdir pays attention to prefix' '
+	git -C repo sparse-checkout set --cone deep/deeper2 &&
+	git -C repo/deep sparse-checkout add deeper1/deepest ../folder1 &&
+
+	git -C repo sparse-checkout list >actual &&
+
+	cat >expect <<-\EOF &&
+	deep/deeper1/deepest
+	deep/deeper2
+	folder1
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'set from subdir in non-cone mode throws an error' '
+	git -C repo sparse-checkout disable &&
+	test_must_fail git -C repo/deep sparse-checkout set --no-cone deeper2 ../folder1 2>error &&
+
+	grep "run from the toplevel directory in non-cone mode" error
+'
+
+test_expect_success 'set from subdir in non-cone mode throws an error' '
+	git -C repo sparse-checkout set --no-cone deep/deeper2 &&
+	test_must_fail git -C repo/deep sparse-checkout add deeper1/deepest ../folder1 2>error &&
+
+	grep "run from the toplevel directory in non-cone mode" error
+'
+
 test_done
-- 
gitgitgadget

[PATCH v4 4/5] sparse-checkout: error or warn when given individual files

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

The set and add subcommands accept multiple positional arguments.
The meaning of these arguments differs slightly in the two modes:

Cone mode only accepts directories.  If given a file, it would
previously treat it as a directory, causing not just the file itself to
be included but all sibling files as well -- likely against users'
expectations.  Throw an error if the specified path is a file in the
index.  Provide a --skip-checks argument to allow users to override
(e.g. for the case when the given path IS a directory on another
branch).

Non-cone mode accepts general gitignore patterns.  There are many
reasons to avoid this mode, but one possible reason to use it instead of
cone mode: to be able to select individual files within a directory.
However, if a file is passed to set/add in non-cone mode, you won't be
selecting a single file, you'll be selecting a file with the same name
in any directory.  Thus users will likely want to prefix any paths they
specify with a leading '/' character; warn users if the patterns they
specify exactly name a file because it means they are likely missing
such a missing leading slash.

Reviewed-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c          | 43 +++++++++++++++++++++++++-----
 t/t1091-sparse-checkout-builtin.sh | 16 ++++++++++-
 2 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index a2192a69219..64583fa704f 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -1,4 +1,5 @@
 #include "builtin.h"
+#include "cache.h"
 #include "config.h"
 #include "dir.h"
 #include "parse-options.h"
@@ -684,8 +685,11 @@ static int modify_pattern_list(int argc, const char **argv, int use_stdin,
 	return result;
 }
 
-static void sanitize_paths(int argc, const char **argv, const char *prefix)
+static void sanitize_paths(int argc, const char **argv,
+			   const char *prefix, int skip_checks)
 {
+	int i;
+
 	if (!argc)
 		return;
 
@@ -694,7 +698,6 @@ static void sanitize_paths(int argc, const char **argv, const char *prefix)
 		 * The args are not pathspecs, so unfortunately we
 		 * cannot imitate how cmd_add() uses parse_pathspec().
 		 */
-		int i;
 		int prefix_len = strlen(prefix);
 
 		if (!core_sparse_checkout_cone)
@@ -703,20 +706,44 @@ static void sanitize_paths(int argc, const char **argv, const char *prefix)
 		for (i = 0; i < argc; i++)
 			argv[i] = prefix_path(prefix, prefix_len, argv[i]);
 	}
+
+	if (skip_checks)
+		return;
+
+	for (i = 0; i < argc; i++) {
+		struct cache_entry *ce;
+		struct index_state *index = the_repository->index;
+		int pos = index_name_pos(index, argv[i], strlen(argv[i]));
+
+		if (pos < 0)
+			continue;
+		ce = index->cache[pos];
+		if (S_ISSPARSEDIR(ce->ce_mode))
+			continue;
+
+		if (core_sparse_checkout_cone)
+			die(_("\"%s\" is not a directory; to treat it as a directory anyway, rerun with --skip-checks"), argv[i]);
+		else
+			warning(_("pass a leading slash before paths such as \"%s\" if you want a single file (see NON-CONE PROBLEMS in the git-sparse-checkout manual)."), argv[i]);
+	}
 }
 
 static char const * const builtin_sparse_checkout_add_usage[] = {
-	N_("git sparse-checkout add (--stdin | <patterns>)"),
+	N_("git sparse-checkout add [--skip-checks] (--stdin | <patterns>)"),
 	NULL
 };
 
 static struct sparse_checkout_add_opts {
+	int skip_checks;
 	int use_stdin;
 } add_opts;
 
 static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
 {
 	static struct option builtin_sparse_checkout_add_options[] = {
+		OPT_BOOL_F(0, "skip-checks", &add_opts.skip_checks,
+			   N_("skip some sanity checks on the given paths that might give false positives"),
+			   PARSE_OPT_NONEG),
 		OPT_BOOL(0, "stdin", &add_opts.use_stdin,
 			 N_("read patterns from standard in")),
 		OPT_END(),
@@ -732,19 +759,20 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
 			     builtin_sparse_checkout_add_usage,
 			     PARSE_OPT_KEEP_UNKNOWN);
 
-	sanitize_paths(argc, argv, prefix);
+	sanitize_paths(argc, argv, prefix, add_opts.skip_checks);
 
 	return modify_pattern_list(argc, argv, add_opts.use_stdin, ADD);
 }
 
 static char const * const builtin_sparse_checkout_set_usage[] = {
-	N_("git sparse-checkout set [--[no-]cone] [--[no-]sparse-index] (--stdin | <patterns>)"),
+	N_("git sparse-checkout set [--[no-]cone] [--[no-]sparse-index] [--skip-checks] (--stdin | <patterns>)"),
 	NULL
 };
 
 static struct sparse_checkout_set_opts {
 	int cone_mode;
 	int sparse_index;
+	int skip_checks;
 	int use_stdin;
 } set_opts;
 
@@ -758,6 +786,9 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
 			 N_("initialize the sparse-checkout in cone mode")),
 		OPT_BOOL(0, "sparse-index", &set_opts.sparse_index,
 			 N_("toggle the use of a sparse index")),
+		OPT_BOOL_F(0, "skip-checks", &set_opts.skip_checks,
+			   N_("skip some sanity checks on the given paths that might give false positives"),
+			   PARSE_OPT_NONEG),
 		OPT_BOOL_F(0, "stdin", &set_opts.use_stdin,
 			   N_("read patterns from standard in"),
 			   PARSE_OPT_NONEG),
@@ -786,7 +817,7 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
 		argv = default_patterns;
 		argc = default_patterns_nr;
 	} else {
-		sanitize_paths(argc, argv, prefix);
+		sanitize_paths(argc, argv, prefix, set_opts.skip_checks);
 	}
 
 	return modify_pattern_list(argc, argv, set_opts.use_stdin, REPLACE);
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index c1f86b0e02e..3b39329266b 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -562,7 +562,7 @@ test_expect_success 'different sparse-checkouts with worktrees' '
 '
 
 test_expect_success 'set using filename keeps file on-disk' '
-	git -C repo sparse-checkout set a deep &&
+	git -C repo sparse-checkout set --skip-checks a deep &&
 	cat >expect <<-\EOF &&
 	/*
 	!/*/
@@ -839,4 +839,18 @@ test_expect_success 'set from subdir in non-cone mode throws an error' '
 	grep "run from the toplevel directory in non-cone mode" error
 '
 
+test_expect_success 'by default, cone mode will error out when passed files' '
+	git -C repo sparse-checkout reapply --cone &&
+	test_must_fail git -C repo sparse-checkout add .gitignore 2>error &&
+
+	grep ".gitignore.*is not a directory" error
+'
+
+test_expect_success 'by default, non-cone mode will warn on individual files' '
+	git -C repo sparse-checkout reapply --no-cone &&
+	git -C repo sparse-checkout add .gitignore 2>warning &&
+
+	grep "pass a leading slash before paths.*if you want a single file" warning
+'
+
 test_done
-- 
gitgitgadget

[PATCH v4 5/5] sparse-checkout: reject arguments in cone-mode that look like patterns

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

In sparse-checkout add/set under cone mode, the arguments passed are
supposed to be directories rather than gitignore-style patterns.
However, given the amount of effort spent in the manual discussing
patterns, it is easy for users to assume they need to pass patterns such
as
   /foo/*
or
   !/bar/*/
or perhaps they really do ignore the directory rule and specify a
random gitignore-style pattern like
   *.c

To help catch such mistakes, throw an error if any of the positional
arguments:
  * starts with any of '/!'
  * contains any of '*?[]'

Inform users they can pass --skip-checks if they have a directory that
really does have such special characters in its name.  (We exclude '\'
because of sparse-checkout's special handling of backslashes; see
the MINGW test in t1091.46.)

Reviewed-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c          | 11 +++++++++++
 t/t1091-sparse-checkout-builtin.sh |  2 +-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 64583fa704f..14601b8bd67 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -710,6 +710,17 @@ static void sanitize_paths(int argc, const char **argv,
 	if (skip_checks)
 		return;
 
+	if (core_sparse_checkout_cone) {
+		for (i = 0; i < argc; i++) {
+			if (argv[i][0] == '/')
+				die(_("specify directories rather than patterns (no leading slash)"));
+			if (argv[i][0] == '!')
+				die(_("specify directories rather than patterns.  If your directory starts with a '!', pass --skip-checks"));
+			if (strpbrk(argv[i], "*?[]"))
+				die(_("specify directories rather than patterns.  If your directory really has any of '*?[]\\' in it, pass --skip-checks"));
+		}
+	}
+
 	for (i = 0; i < argc; i++) {
 		struct cache_entry *ce;
 		struct index_state *index = the_repository->index;
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 3b39329266b..421127d99c2 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -673,7 +673,7 @@ test_expect_success BSLASHPSPEC 'pattern-checks: escaped characters' '
 	git -C escaped reset --hard $COMMIT &&
 	check_files escaped "a deep folder1 folder2 zbad\\dir zdoes*exist" zglob[!a]? &&
 	git -C escaped sparse-checkout init --cone &&
-	git -C escaped sparse-checkout set zbad\\dir/bogus "zdoes*not*exist" "zdoes*exist" "zglob[!a]?" &&
+	git -C escaped sparse-checkout set --skip-checks zbad\\dir/bogus "zdoes*not*exist" "zdoes*exist" "zglob[!a]?" &&
 	cat >expect <<-\EOF &&
 	/*
 	!/*/
-- 
gitgitgadget

Re: [PATCH v2 3/6] sparse-checkout: pay attention to prefix for {set, add}

[Ævar Arnfjörð Bjarmason]

On Tue, Feb 15 2022, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <newren@gmail.com>

> +			die("please run from the toplevel directory in non-cone mode");

Isn't this missing _() for i18n?

Re: [PATCH v2 4/6] sparse-checkout: error or warn when given individual files

[Ævar Arnfjörð Bjarmason]

On Tue, Feb 15 2022, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <newren@gmail.com>

> +		if (core_sparse_checkout_cone)
> +			die(_("\"%s\" is not a directory; to treat it as a directory anyway, rerun with --skip-checks"), argv[i]);

Almost all of our messaging quotes paths etc. with single quotes, which
also makes this nicer in the C code:

	die(_("'%s' is not a ...

> +		else
> +			warning(_("pass a leading slash before paths such as \"%s\" if you want a single file (see NON-CONE PROBLEMS in the git-sparse-checkout manual)."), argv[i]);

ditto.

>  	static struct option builtin_sparse_checkout_add_options[] = {
> +		OPT_BOOL_F(0, "skip-checks", &add_opts.skip_checks,
> +			   N_("skip some sanity checks on the given paths that might give false positives"),
> +			   PARSE_OPT_NONEG),

This is bikeshedding, but I dislike arguments whose long name requires
us to have a PARSE_OPT_NONEG. Or well, not requires, but
--no-skip-checks sounds odd, so it's disallowed here.

Why not simply have a --check-path, --path-checks or --sanity-check, all
of which combine more naturally with a --no than not.

I.e. set the variable to a default of 1, and let the user negate it.

I think that also makes the synopsis easier to grok at first sight,
since a:

    git x [--no-XYZ]

Makes it immediately clear what the default is. Well, I suppose
"--skip-*" does too, but we use --no-* more.

Another reason to do that is that if you do:

    git cmd --no-<TAB>

You can see all the knobs that are off by default, our completion
supports that specifically.

Re: [PATCH v4 0/5] sparse checkout: fix a few bugs and check argument validity for set/add

[Ævar Arnfjörð Bjarmason]

On Thu, Feb 17 2022, Elijah Newren via GitGitGadget wrote:

> Changes since v3:
>
>  * Use strpbrk() instead of multiple strchr(), fix commit message relative
>    to backslashes.

I read this v4, and then left a couple of replies on the v2 by mistake,
but they apply to the code in v4. Sorry about the mix-up.

Re: [PATCH v3 5/5] sparse-checkout: reject arguments in cone-mode that look like patterns

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> After reading up on it, I think you mean strcspn() rather than

Yeah, strcspn() have many existing uses in our codebase.  Thanks.

Re: [PATCH v4 3/5] sparse-checkout: pay attention to prefix for {set, add}

[Junio C Hamano]

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

> +		for (i = 0; i < argc; i++)
> +			argv[i] = prefix_path(prefix, prefix_len, argv[i]);

Looks sensible.

Re: [PATCH v4 4/5] sparse-checkout: error or warn when given individual files

[Junio C Hamano]

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

>  		int prefix_len = strlen(prefix);
>  
>  		if (!core_sparse_checkout_cone)
> @@ -703,20 +706,44 @@ static void sanitize_paths(int argc, const char **argv, const char *prefix)
>  		for (i = 0; i < argc; i++)
>  			argv[i] = prefix_path(prefix, prefix_len, argv[i]);
>  	}
> +
> +	if (skip_checks)
> +		return;

The placement of skip-checks here makes the earier die() between the
hunks in this block unskippable.

> +	for (i = 0; i < argc; i++) {
> +		struct cache_entry *ce;
> +		struct index_state *index = the_repository->index;
> +		int pos = index_name_pos(index, argv[i], strlen(argv[i]));
> +
> +		if (pos < 0)
> +			continue;

I _think_ the intent for this is to catch possbily common mistakes
rather than a hard rule, so while an unmerged path will evade this
check, I think it is an acceptable trade-off between code simplicity
and thoroughness.

> +		ce = index->cache[pos];
> +		if (S_ISSPARSEDIR(ce->ce_mode))
> +			continue;
> +
> +		if (core_sparse_checkout_cone)
> +			die(_("\"%s\" is not a directory; to treat it as a directory anyway, rerun with --skip-checks"), argv[i]);
> +		else
> +			warning(_("pass a leading slash before paths such as \"%s\" if you want a single file (see NON-CONE PROBLEMS in the git-sparse-checkout manual)."), argv[i]);
> +	}

Hmph. The skip_checks flag was introduced exactly so that what the
code catches as a possible problem is a false alarm, no?  I would
understand if both were die() to be conservative, of both were
warning() to be lenient, but one being die() and the other being
warning() makes this look somewhat uneven.

Besides, being leninent here somewhat defeats the point of having
skip_checks, no?  As long as we believe skip_checks option is useful,
shouldn't these be die() to be equally conservative?

Re: [PATCH v2 3/6] sparse-checkout: pay attention to prefix for {set, add}

[Elijah Newren]

On Thu, Feb 17, 2022 at 1:05 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Tue, Feb 15 2022, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@gmail.com>
>
> > +                     die("please run from the toplevel directory in non-cone mode");
>
> Isn't this missing _() for i18n?

Yes; thanks.

Re: [PATCH v4 4/5] sparse-checkout: error or warn when given individual files

[Elijah Newren]

On Thu, Feb 17, 2022 at 10:07 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> >               int prefix_len = strlen(prefix);
> >
> >               if (!core_sparse_checkout_cone)
> > @@ -703,20 +706,44 @@ static void sanitize_paths(int argc, const char **argv, const char *prefix)
> >               for (i = 0; i < argc; i++)
> >                       argv[i] = prefix_path(prefix, prefix_len, argv[i]);
> >       }
> > +
> > +     if (skip_checks)
> > +             return;
>
> The placement of skip-checks here makes the earier die() between the
> hunks in this block unskippable.

Fair point; I'll restructure a bit.

> > +     for (i = 0; i < argc; i++) {
> > +             struct cache_entry *ce;
> > +             struct index_state *index = the_repository->index;
> > +             int pos = index_name_pos(index, argv[i], strlen(argv[i]));
> > +
> > +             if (pos < 0)
> > +                     continue;
>
> I _think_ the intent for this is to catch possbily common mistakes
> rather than a hard rule, so while an unmerged path will evade this
> check, I think it is an acceptable trade-off between code simplicity
> and thoroughness.
>
> > +             ce = index->cache[pos];
> > +             if (S_ISSPARSEDIR(ce->ce_mode))
> > +                     continue;
> > +
> > +             if (core_sparse_checkout_cone)
> > +                     die(_("\"%s\" is not a directory; to treat it as a directory anyway, rerun with --skip-checks"), argv[i]);
> > +             else
> > +                     warning(_("pass a leading slash before paths such as \"%s\" if you want a single file (see NON-CONE PROBLEMS in the git-sparse-checkout manual)."), argv[i]);
> > +     }
>
> Hmph. The skip_checks flag was introduced exactly so that what the
> code catches as a possible problem is a false alarm, no?  I would
> understand if both were die() to be conservative, of both were
> warning() to be lenient, but one being die() and the other being
> warning() makes this look somewhat uneven.
>
> Besides, being leninent here somewhat defeats the point of having
> skip_checks, no?  As long as we believe skip_checks option is useful,
> shouldn't these be die() to be equally conservative?

One thing that gives me pause about making the leading slash
suggestion a die() instead of a warning() is that on Windows boxes,
users also have to quote arguments with a leading slash to prevent
them from being transformed into a "C:\Random\Garbage\Path\" prefix
instead.  But I don't want to tell linux/macos/etc. users to quote
their patterns unnecessarily, and being rather unfamiliar with Windows
I don't know if it's all Windows systems affected or only some like
cygwin.

Thoughts?

[PATCH v5 0/5] sparse checkout: fix a few bugs and check argument validity for set/add

[Elijah Newren via GitGitGadget]

== Maintainer notes ==

Note: There is a small textual and small semantic conflict with
ds/sparse-checkout-requires-per-worktree-config in seen. I included the diff
with the correct resolution near the end of this cover letter. If you'd
prefer I rebased on top of ds/sparse-chckout-requires-per-worktree-config,
let me know.

== Overview ==

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 (i.e. in
   v2.35.0)
 * Patch 3 fixes sparse-checkout-from-subdirectories-ignores-"prefix" (see
   https://lore.kernel.org/git/29f0410e-6dfa-2e86-394d-b1fb735e7608@gmail.com/),
   in cone mode. Since we'll get nasty surprises whether we use or don't use
   "prefix" for non-cone mode, simply throw an error if set/add subcommands
   of sparse-checkout are run from a subdirectory.
 * Patches 4-5 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.

== Update history ==

Changes since v4:

 * have --skip-checks enable running from a subdirectory in non-cone mode
 * make sure new die() messages are marked for translation (and using single
   quotes instead of double)

Changes since v3:

 * Use strpbrk() instead of multiple strchr(), fix commit message relative
   to backslashes.

Changes since v2:

 * Dropped patch 5
 * Added Stolee's Reviewed-by

Changes since v1:

 * Dropped the commit changing cone-mode to default (patch 7, which will be
   split into multiple patches and submitted as a separate series)
 * Removed the RFC label
 * Decided to error out when running set/add with paths from a subdirectory
   in non-cone mode, and added tests
 * Changed the warning for non-cone mode with individual files to point out
   that the user is likely trying to select an individual file, but should
   likely add a leading slash to ensure that is what happens
 * Fixed typos, removed unnecessary condition checks

== Conflict resolution ==

Patch to resolve textual and semantic conflict with
ds/sparse-checkout-requires-per-worktree-config:

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
remerge CONFLICT (content): Merge conflict in t/t1091-sparse-checkout-builtin.sh
index 3c6adeb885..3a95d2996d 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -275,24 +275,8 @@ test_expect_success 'sparse-index enabled and disabled' '
     diff -u sparse full | tail -n +3 >actual &&
     test_cmp expect actual &&
 
-<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 286c22e5ec (sparse-checkout: reject arguments in cone-mode that look like patterns)
     git -C repo config --list >config &&
-    ! grep index.sparse config
-|||||||||||||||||||||||||||||||| 89bece5c8c
-        diff -u sparse full | tail -n +3 >actual &&
-        test_cmp expect actual &&
-
-        git -C repo config --list >config &&
-        ! grep index.sparse config
-    )
-================================
-        diff -u sparse full | tail -n +3 >actual &&
-        test_cmp expect actual &&
-
-        git -C repo config --list >config &&
-        test_cmp_config -C repo false index.sparse
-    )
->>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 3ce1138272 (config: make git_configset_get_string_tmp() private)
+    test_cmp_config -C repo false index.sparse
 '
 
 test_expect_success 'cone mode: init and set' '
@@ -532,6 +516,7 @@ test_expect_success 'reapply can handle config options' '
     cat >expect <<-\EOF &&
     core.sparsecheckout=true
     core.sparsecheckoutcone=true
+    index.sparse=false
     EOF
     test_cmp expect actual &&
 
@@ -539,6 +524,8 @@ test_expect_success 'reapply can handle config options' '
     git -C repo config --worktree --list >actual &&
     cat >expect <<-\EOF &&
     core.sparsecheckout=true
+    core.sparsecheckoutcone=false
+    index.sparse=false
     EOF
     test_cmp expect actual &&


== CCs ==

Elijah Newren (5):
  sparse-checkout: correct reapply's handling of options
  sparse-checkout: correctly set non-cone mode when expected
  sparse-checkout: pay attention to prefix for {set, add}
  sparse-checkout: error or warn when given individual files
  sparse-checkout: reject arguments in cone-mode that look like patterns

 builtin/sparse-checkout.c          | 78 +++++++++++++++++++++++++--
 t/t1091-sparse-checkout-builtin.sh | 87 +++++++++++++++++++++++++++++-
 2 files changed, 158 insertions(+), 7 deletions(-)


base-commit: b80121027d1247a0754b3cc46897fee75c050b44
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1118%2Fnewren%2Fsparse-checkout-default-and-arg-validity-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1118/newren/sparse-checkout-default-and-arg-validity-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1118

Range-diff vs v4:

 1:  5215b7f7179 = 1:  5215b7f7179 sparse-checkout: correct reapply's handling of options
 2:  0c2ab523e74 = 2:  0c2ab523e74 sparse-checkout: correctly set non-cone mode when expected
 3:  e68b0a37ff3 ! 3:  06cf71bfca4 sparse-checkout: pay attention to prefix for {set, add}
     @@ builtin/sparse-checkout.c: static int modify_pattern_list(int argc, const char *
      +	if (!argc)
      +		return;
      +
     -+	if (prefix && *prefix) {
     ++	if (prefix && *prefix && core_sparse_checkout_cone) {
      +		/*
      +		 * The args are not pathspecs, so unfortunately we
      +		 * cannot imitate how cmd_add() uses parse_pathspec().
     @@ builtin/sparse-checkout.c: static int modify_pattern_list(int argc, const char *
      +		int i;
      +		int prefix_len = strlen(prefix);
      +
     -+		if (!core_sparse_checkout_cone)
     -+			die("please run from the toplevel directory in non-cone mode");
     -+
      +		for (i = 0; i < argc; i++)
      +			argv[i] = prefix_path(prefix, prefix_len, argv[i]);
      +	}
     ++
     ++	if (prefix && *prefix && !core_sparse_checkout_cone)
     ++		die(_("please run from the toplevel directory in non-cone mode"));
     ++
      +}
      +
       static char const * const builtin_sparse_checkout_add_usage[] = {
 4:  1fdebc1953f ! 4:  78bf6016687 sparse-checkout: error or warn when given individual files
     @@ Commit message
          in any directory.  Thus users will likely want to prefix any paths they
          specify with a leading '/' character; warn users if the patterns they
          specify exactly name a file because it means they are likely missing
     -    such a missing leading slash.
     +    such a leading slash.
      
          Reviewed-by: Derrick Stolee <derrickstolee@github.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
     @@ builtin/sparse-checkout.c: static void sanitize_paths(int argc, const char **arg
      -		int i;
       		int prefix_len = strlen(prefix);
       
     - 		if (!core_sparse_checkout_cone)
     -@@ builtin/sparse-checkout.c: static void sanitize_paths(int argc, const char **argv, const char *prefix)
       		for (i = 0; i < argc; i++)
       			argv[i] = prefix_path(prefix, prefix_len, argv[i]);
       	}
     -+
     + 
      +	if (skip_checks)
      +		return;
      +
     + 	if (prefix && *prefix && !core_sparse_checkout_cone)
     + 		die(_("please run from the toplevel directory in non-cone mode"));
     + 
      +	for (i = 0; i < argc; i++) {
      +		struct cache_entry *ce;
      +		struct index_state *index = the_repository->index;
     @@ builtin/sparse-checkout.c: static void sanitize_paths(int argc, const char **arg
      +			continue;
      +
      +		if (core_sparse_checkout_cone)
     -+			die(_("\"%s\" is not a directory; to treat it as a directory anyway, rerun with --skip-checks"), argv[i]);
     ++			die(_("'%s' is not a directory; to treat it as a directory anyway, rerun with --skip-checks"), argv[i]);
      +		else
     -+			warning(_("pass a leading slash before paths such as \"%s\" if you want a single file (see NON-CONE PROBLEMS in the git-sparse-checkout manual)."), argv[i]);
     ++			warning(_("pass a leading slash before paths such as '%s' if you want a single file (see NON-CONE PROBLEMS in the git-sparse-checkout manual)."), argv[i]);
      +	}
       }
       
 5:  d68682c25c4 ! 5:  53183d07502 sparse-checkout: reject arguments in cone-mode that look like patterns
     @@ Commit message
      
       ## builtin/sparse-checkout.c ##
      @@ builtin/sparse-checkout.c: static void sanitize_paths(int argc, const char **argv,
     - 	if (skip_checks)
     - 		return;
     + 	if (prefix && *prefix && !core_sparse_checkout_cone)
     + 		die(_("please run from the toplevel directory in non-cone mode"));
       
      +	if (core_sparse_checkout_cone) {
      +		for (i = 0; i < argc; i++) {

-- 
gitgitgadget

[PATCH v5 1/5] sparse-checkout: correct reapply's handling of options

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

Commit 4e256731d6 ("sparse-checkout: enable reapply to take
--[no-]{cone,sparse-index}", 2021-12-14) made it so that reapply could
take additional options but added no tests.  Tests would have shown that
the feature doesn't work because the initial values are set AFTER
parsing the command line options instead of before.  Add a test and set
the initial value at the appropriate time.

Reviewed-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c          |  6 +++---
 t/t1091-sparse-checkout-builtin.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index a311483a7d2..fcd574f5fc1 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -789,15 +789,15 @@ static int sparse_checkout_reapply(int argc, const char **argv)
 	if (!core_apply_sparse_checkout)
 		die(_("must be in a sparse-checkout to reapply sparsity patterns"));
 
+	reapply_opts.cone_mode = -1;
+	reapply_opts.sparse_index = -1;
+
 	argc = parse_options(argc, argv, NULL,
 			     builtin_sparse_checkout_reapply_options,
 			     builtin_sparse_checkout_reapply_usage, 0);
 
 	repo_read_index(the_repository);
 
-	reapply_opts.cone_mode = -1;
-	reapply_opts.sparse_index = -1;
-
 	if (update_modes(&reapply_opts.cone_mode, &reapply_opts.sparse_index))
 		return 1;
 
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 3592d124424..ce5e7c19efa 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -495,6 +495,34 @@ test_expect_failure 'sparse-checkout reapply' '
 	git -C tweak sparse-checkout disable
 '
 
+test_expect_success 'reapply can handle config options' '
+	git -C repo sparse-checkout init --cone --no-sparse-index &&
+	git -C repo config --worktree --list >actual &&
+	cat >expect <<-\EOF &&
+	core.sparsecheckout=true
+	core.sparsecheckoutcone=true
+	EOF
+	test_cmp expect actual &&
+
+	git -C repo sparse-checkout reapply --no-cone --no-sparse-index &&
+	git -C repo config --worktree --list >actual &&
+	cat >expect <<-\EOF &&
+	core.sparsecheckout=true
+	EOF
+	test_cmp expect actual &&
+
+	git -C repo sparse-checkout reapply --cone --sparse-index &&
+	git -C repo config --worktree --list >actual &&
+	cat >expect <<-\EOF &&
+	core.sparsecheckout=true
+	core.sparsecheckoutcone=true
+	index.sparse=true
+	EOF
+	test_cmp expect actual &&
+
+	git -C repo sparse-checkout disable
+'
+
 test_expect_success 'cone mode: set with core.ignoreCase=true' '
 	rm repo/.git/info/sparse-checkout &&
 	git -C repo sparse-checkout init --cone &&
-- 
gitgitgadget

[PATCH v5 2/5] sparse-checkout: correctly set non-cone mode when expected

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

commit f2e3a218e8 ("sparse-checkout: enable `set` to initialize
sparse-checkout mode", 2021-12-14) made the `set` command able to
initialize sparse-checkout mode, but it also had to function when
sparse-checkout mode was already setup and the user just wanted to
change the sparsity paths.  So, if the user passed --cone or --no-cone,
then we should override the current setting, but if they didn't pass
either, we should use whatever the current cone mode setting is.

Unfortunately, there was a small error in the logic in that it would not
set the in-memory cone mode value (core_sparse_checkout_one) when
--no-cone was specified, but since it did set the config setting on
disk, any subsequent git invocation would correctly get non-cone mode.
As such, the error did not previously matter.  However, a subsequent
commit will add some logic that depends on core_sparse_checkout_cone
being set to the correct mode, so make sure it is set consistently with
the config values we will be writing to disk.

Reviewed-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index fcd574f5fc1..fb85a1459c3 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -403,6 +403,7 @@ static int update_modes(int *cone_mode, int *sparse_index)
 		core_sparse_checkout_cone = 1;
 	} else {
 		mode = MODE_ALL_PATTERNS;
+		core_sparse_checkout_cone = 0;
 	}
 	if (record_mode && set_config(mode))
 		return 1;
-- 
gitgitgadget

[PATCH v5 3/5] sparse-checkout: pay attention to prefix for {set, add}

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

In cone mode, non-option arguments to set & add are clearly paths, and
as such, we should pay attention to prefix.

In non-cone mode, it is not clear that folks intend to provide paths
since the inputs are gitignore-style patterns.  Paying attention to
prefix would prevent folks from doing things like
   git sparse-checkout add /.gitattributes
   git sparse-checkout add '/toplevel-dir/*'
In fact, the former will result in
   fatal: '/.gitattributes' is outside repository...
while the later will result in
   fatal: Invalid path '/toplevel-dir': No such file or directory
despite the fact that both are valid gitignore-style patterns that would
select real files if added to the sparse-checkout file.  This might lead
people to just use the path without the leading slash, potentially
resulting in them grabbing files with the same name throughout the
directory hierarchy contrary to their expectations.  See also [1] and
[2].  Adding prefix seems to just be fraught with error; so for now
simply throw an error in non-cone mode when sparse-checkout set/add are
run from a subdirectory.

[1] https://lore.kernel.org/git/e1934710-e228-adc4-d37c-f706883bd27c@gmail.com/
[2] https://lore.kernel.org/git/CABPp-BHXZ-XLxY0a3wCATfdq=6-EjW62RzbxKAoFPeXfJswD2w@mail.gmail.com/

Helped-by: Junio Hamano <gitster@pobox.com>
Reviewed-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c          | 26 +++++++++++++++++++
 t/t1091-sparse-checkout-builtin.sh | 41 ++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index fb85a1459c3..3f828feb1f0 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -684,6 +684,28 @@ static int modify_pattern_list(int argc, const char **argv, int use_stdin,
 	return result;
 }
 
+static void sanitize_paths(int argc, const char **argv, const char *prefix)
+{
+	if (!argc)
+		return;
+
+	if (prefix && *prefix && core_sparse_checkout_cone) {
+		/*
+		 * The args are not pathspecs, so unfortunately we
+		 * cannot imitate how cmd_add() uses parse_pathspec().
+		 */
+		int i;
+		int prefix_len = strlen(prefix);
+
+		for (i = 0; i < argc; i++)
+			argv[i] = prefix_path(prefix, prefix_len, argv[i]);
+	}
+
+	if (prefix && *prefix && !core_sparse_checkout_cone)
+		die(_("please run from the toplevel directory in non-cone mode"));
+
+}
+
 static char const * const builtin_sparse_checkout_add_usage[] = {
 	N_("git sparse-checkout add (--stdin | <patterns>)"),
 	NULL
@@ -711,6 +733,8 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
 			     builtin_sparse_checkout_add_usage,
 			     PARSE_OPT_KEEP_UNKNOWN);
 
+	sanitize_paths(argc, argv, prefix);
+
 	return modify_pattern_list(argc, argv, add_opts.use_stdin, ADD);
 }
 
@@ -762,6 +786,8 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
 	if (!core_sparse_checkout_cone && argc == 0) {
 		argv = default_patterns;
 		argc = default_patterns_nr;
+	} else {
+		sanitize_paths(argc, argv, prefix);
 	}
 
 	return modify_pattern_list(argc, argv, set_opts.use_stdin, REPLACE);
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index ce5e7c19efa..c1f86b0e02e 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -798,4 +798,45 @@ test_expect_success 'malformed cone-mode patterns' '
 	grep "warning: disabling cone pattern matching" err
 '
 
+test_expect_success 'set from subdir pays attention to prefix' '
+	git -C repo sparse-checkout disable &&
+	git -C repo/deep sparse-checkout set --cone deeper2 ../folder1 &&
+
+	git -C repo sparse-checkout list >actual &&
+
+	cat >expect <<-\EOF &&
+	deep/deeper2
+	folder1
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'add from subdir pays attention to prefix' '
+	git -C repo sparse-checkout set --cone deep/deeper2 &&
+	git -C repo/deep sparse-checkout add deeper1/deepest ../folder1 &&
+
+	git -C repo sparse-checkout list >actual &&
+
+	cat >expect <<-\EOF &&
+	deep/deeper1/deepest
+	deep/deeper2
+	folder1
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'set from subdir in non-cone mode throws an error' '
+	git -C repo sparse-checkout disable &&
+	test_must_fail git -C repo/deep sparse-checkout set --no-cone deeper2 ../folder1 2>error &&
+
+	grep "run from the toplevel directory in non-cone mode" error
+'
+
+test_expect_success 'set from subdir in non-cone mode throws an error' '
+	git -C repo sparse-checkout set --no-cone deep/deeper2 &&
+	test_must_fail git -C repo/deep sparse-checkout add deeper1/deepest ../folder1 2>error &&
+
+	grep "run from the toplevel directory in non-cone mode" error
+'
+
 test_done
-- 
gitgitgadget

[PATCH v5 4/5] sparse-checkout: error or warn when given individual files

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

The set and add subcommands accept multiple positional arguments.
The meaning of these arguments differs slightly in the two modes:

Cone mode only accepts directories.  If given a file, it would
previously treat it as a directory, causing not just the file itself to
be included but all sibling files as well -- likely against users'
expectations.  Throw an error if the specified path is a file in the
index.  Provide a --skip-checks argument to allow users to override
(e.g. for the case when the given path IS a directory on another
branch).

Non-cone mode accepts general gitignore patterns.  There are many
reasons to avoid this mode, but one possible reason to use it instead of
cone mode: to be able to select individual files within a directory.
However, if a file is passed to set/add in non-cone mode, you won't be
selecting a single file, you'll be selecting a file with the same name
in any directory.  Thus users will likely want to prefix any paths they
specify with a leading '/' character; warn users if the patterns they
specify exactly name a file because it means they are likely missing
such a leading slash.

Reviewed-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c          | 42 +++++++++++++++++++++++++-----
 t/t1091-sparse-checkout-builtin.sh | 16 +++++++++++-
 2 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 3f828feb1f0..38ac37d9c61 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -1,4 +1,5 @@
 #include "builtin.h"
+#include "cache.h"
 #include "config.h"
 #include "dir.h"
 #include "parse-options.h"
@@ -684,8 +685,11 @@ static int modify_pattern_list(int argc, const char **argv, int use_stdin,
 	return result;
 }
 
-static void sanitize_paths(int argc, const char **argv, const char *prefix)
+static void sanitize_paths(int argc, const char **argv,
+			   const char *prefix, int skip_checks)
 {
+	int i;
+
 	if (!argc)
 		return;
 
@@ -694,30 +698,52 @@ static void sanitize_paths(int argc, const char **argv, const char *prefix)
 		 * The args are not pathspecs, so unfortunately we
 		 * cannot imitate how cmd_add() uses parse_pathspec().
 		 */
-		int i;
 		int prefix_len = strlen(prefix);
 
 		for (i = 0; i < argc; i++)
 			argv[i] = prefix_path(prefix, prefix_len, argv[i]);
 	}
 
+	if (skip_checks)
+		return;
+
 	if (prefix && *prefix && !core_sparse_checkout_cone)
 		die(_("please run from the toplevel directory in non-cone mode"));
 
+	for (i = 0; i < argc; i++) {
+		struct cache_entry *ce;
+		struct index_state *index = the_repository->index;
+		int pos = index_name_pos(index, argv[i], strlen(argv[i]));
+
+		if (pos < 0)
+			continue;
+		ce = index->cache[pos];
+		if (S_ISSPARSEDIR(ce->ce_mode))
+			continue;
+
+		if (core_sparse_checkout_cone)
+			die(_("'%s' is not a directory; to treat it as a directory anyway, rerun with --skip-checks"), argv[i]);
+		else
+			warning(_("pass a leading slash before paths such as '%s' if you want a single file (see NON-CONE PROBLEMS in the git-sparse-checkout manual)."), argv[i]);
+	}
 }
 
 static char const * const builtin_sparse_checkout_add_usage[] = {
-	N_("git sparse-checkout add (--stdin | <patterns>)"),
+	N_("git sparse-checkout add [--skip-checks] (--stdin | <patterns>)"),
 	NULL
 };
 
 static struct sparse_checkout_add_opts {
+	int skip_checks;
 	int use_stdin;
 } add_opts;
 
 static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
 {
 	static struct option builtin_sparse_checkout_add_options[] = {
+		OPT_BOOL_F(0, "skip-checks", &add_opts.skip_checks,
+			   N_("skip some sanity checks on the given paths that might give false positives"),
+			   PARSE_OPT_NONEG),
 		OPT_BOOL(0, "stdin", &add_opts.use_stdin,
 			 N_("read patterns from standard in")),
 		OPT_END(),
@@ -733,19 +759,20 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
 			     builtin_sparse_checkout_add_usage,
 			     PARSE_OPT_KEEP_UNKNOWN);
 
-	sanitize_paths(argc, argv, prefix);
+	sanitize_paths(argc, argv, prefix, add_opts.skip_checks);
 
 	return modify_pattern_list(argc, argv, add_opts.use_stdin, ADD);
 }
 
 static char const * const builtin_sparse_checkout_set_usage[] = {
-	N_("git sparse-checkout set [--[no-]cone] [--[no-]sparse-index] (--stdin | <patterns>)"),
+	N_("git sparse-checkout set [--[no-]cone] [--[no-]sparse-index] [--skip-checks] (--stdin | <patterns>)"),
 	NULL
 };
 
 static struct sparse_checkout_set_opts {
 	int cone_mode;
 	int sparse_index;
+	int skip_checks;
 	int use_stdin;
 } set_opts;
 
@@ -759,6 +786,9 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
 			 N_("initialize the sparse-checkout in cone mode")),
 		OPT_BOOL(0, "sparse-index", &set_opts.sparse_index,
 			 N_("toggle the use of a sparse index")),
+		OPT_BOOL_F(0, "skip-checks", &set_opts.skip_checks,
+			   N_("skip some sanity checks on the given paths that might give false positives"),
+			   PARSE_OPT_NONEG),
 		OPT_BOOL_F(0, "stdin", &set_opts.use_stdin,
 			   N_("read patterns from standard in"),
 			   PARSE_OPT_NONEG),
@@ -787,7 +817,7 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
 		argv = default_patterns;
 		argc = default_patterns_nr;
 	} else {
-		sanitize_paths(argc, argv, prefix);
+		sanitize_paths(argc, argv, prefix, set_opts.skip_checks);
 	}
 
 	return modify_pattern_list(argc, argv, set_opts.use_stdin, REPLACE);
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index c1f86b0e02e..3b39329266b 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -562,7 +562,7 @@ test_expect_success 'different sparse-checkouts with worktrees' '
 '
 
 test_expect_success 'set using filename keeps file on-disk' '
-	git -C repo sparse-checkout set a deep &&
+	git -C repo sparse-checkout set --skip-checks a deep &&
 	cat >expect <<-\EOF &&
 	/*
 	!/*/
@@ -839,4 +839,18 @@ test_expect_success 'set from subdir in non-cone mode throws an error' '
 	grep "run from the toplevel directory in non-cone mode" error
 '
 
+test_expect_success 'by default, cone mode will error out when passed files' '
+	git -C repo sparse-checkout reapply --cone &&
+	test_must_fail git -C repo sparse-checkout add .gitignore 2>error &&
+
+	grep ".gitignore.*is not a directory" error
+'
+
+test_expect_success 'by default, non-cone mode will warn on individual files' '
+	git -C repo sparse-checkout reapply --no-cone &&
+	git -C repo sparse-checkout add .gitignore 2>warning &&
+
+	grep "pass a leading slash before paths.*if you want a single file" warning
+'
+
 test_done
-- 
gitgitgadget

[PATCH v5 5/5] sparse-checkout: reject arguments in cone-mode that look like patterns

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

In sparse-checkout add/set under cone mode, the arguments passed are
supposed to be directories rather than gitignore-style patterns.
However, given the amount of effort spent in the manual discussing
patterns, it is easy for users to assume they need to pass patterns such
as
   /foo/*
or
   !/bar/*/
or perhaps they really do ignore the directory rule and specify a
random gitignore-style pattern like
   *.c

To help catch such mistakes, throw an error if any of the positional
arguments:
  * starts with any of '/!'
  * contains any of '*?[]'

Inform users they can pass --skip-checks if they have a directory that
really does have such special characters in its name.  (We exclude '\'
because of sparse-checkout's special handling of backslashes; see
the MINGW test in t1091.46.)

Reviewed-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c          | 11 +++++++++++
 t/t1091-sparse-checkout-builtin.sh |  2 +-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 38ac37d9c61..a0c8a1499af 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -710,6 +710,17 @@ static void sanitize_paths(int argc, const char **argv,
 	if (prefix && *prefix && !core_sparse_checkout_cone)
 		die(_("please run from the toplevel directory in non-cone mode"));
 
+	if (core_sparse_checkout_cone) {
+		for (i = 0; i < argc; i++) {
+			if (argv[i][0] == '/')
+				die(_("specify directories rather than patterns (no leading slash)"));
+			if (argv[i][0] == '!')
+				die(_("specify directories rather than patterns.  If your directory starts with a '!', pass --skip-checks"));
+			if (strpbrk(argv[i], "*?[]"))
+				die(_("specify directories rather than patterns.  If your directory really has any of '*?[]\\' in it, pass --skip-checks"));
+		}
+	}
+
 	for (i = 0; i < argc; i++) {
 		struct cache_entry *ce;
 		struct index_state *index = the_repository->index;
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 3b39329266b..421127d99c2 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -673,7 +673,7 @@ test_expect_success BSLASHPSPEC 'pattern-checks: escaped characters' '
 	git -C escaped reset --hard $COMMIT &&
 	check_files escaped "a deep folder1 folder2 zbad\\dir zdoes*exist" zglob[!a]? &&
 	git -C escaped sparse-checkout init --cone &&
-	git -C escaped sparse-checkout set zbad\\dir/bogus "zdoes*not*exist" "zdoes*exist" "zglob[!a]?" &&
+	git -C escaped sparse-checkout set --skip-checks zbad\\dir/bogus "zdoes*not*exist" "zdoes*exist" "zglob[!a]?" &&
 	cat >expect <<-\EOF &&
 	/*
 	!/*/
-- 
gitgitgadget

Re: [PATCH v5 0/5] sparse checkout: fix a few bugs and check argument validity for set/add

[Derrick Stolee]

On 2/19/2022 11:44 AM, Elijah Newren via GitGitGadget wrote:

> == Update history ==
> 
> Changes since v4:
> 
>  * have --skip-checks enable running from a subdirectory in non-cone mode
>  * make sure new die() messages are marked for translation (and using single
>    quotes instead of double)
> 
> Changes since v3:
> 
>  * Use strpbrk() instead of multiple strchr(), fix commit message relative
>    to backslashes.
> 
> Changes since v2:
> 
>  * Dropped patch 5
>  * Added Stolee's Reviewed-by

I looked at the range-diffs for these iterations and found the changes
to be acceptable. This version (still) looks good to go.

Thanks,
-Stolee

Re: [PATCH v5 0/5] sparse checkout: fix a few bugs and check argument validity for set/add

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

> On 2/19/2022 11:44 AM, Elijah Newren via GitGitGadget wrote:
>
>> == Update history ==
>> 
>> Changes since v4:
>> 
>>  * have --skip-checks enable running from a subdirectory in non-cone mode
>>  * make sure new die() messages are marked for translation (and using single
>>    quotes instead of double)
>> 
>> Changes since v3:
>> 
>>  * Use strpbrk() instead of multiple strchr(), fix commit message relative
>>    to backslashes.
>> 
>> Changes since v2:
>> 
>>  * Dropped patch 5
>>  * Added Stolee's Reviewed-by
>
> I looked at the range-diffs for these iterations and found the changes
> to be acceptable. This version (still) looks good to go.

Yup, I think this one is OK enough.  Let's start merging it down.

Thanks.