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

Commit Message

Philippe Blain via GitGitGadget Feb. 16, 2022, 4:21 a.m. UTC

== 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/[e-mail]/),
   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:


== 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 <[e-mail]>
          Signed-off-by: Elijah Newren <[e-mail]>
      
       ## 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 <[e-mail]>
          Signed-off-by: Elijah Newren <[e-mail]>
      
       ## 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=[e-mail]/
      
          Helped-by: Junio Hamano <[e-mail]>
     +    Reviewed-by: Derrick Stolee <[e-mail]>
          Signed-off-by: Elijah Newren <[e-mail]>
      
       ## 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 <[e-mail]>
          Signed-off-by: Elijah Newren <[e-mail]>
      
       ## 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 <[e-mail]>
          Signed-off-by: Elijah Newren <[e-mail]>
      
       ## 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' '

Comments

Junio C Hamano Feb. 16, 2022, 7:19 a.m. UTC | #1

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

Patch

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