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

["Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com>]

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