[PATCH v2 00/12] Harden the sparse-checkout builtin

["Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>]

This series is based on ds/sparse-list-in-cone-mode.

This series attempts to clean up some rough edges in the sparse-checkout
feature, especially around the cone mode.

Unfortunately, after the v2.25.0 release, we noticed an issue with the "git
clone --sparse" option when using a URL instead of a local path. This is
fixed and properly tested here.

Also, let's improve Git's response to these more complicated scenarios:

 1. Running "git sparse-checkout init" in a worktree would complain because
    the "info" dir doesn't exist.
 2. Tracked paths that include "*" and "\" in their filenames.
 3. If a user edits the sparse-checkout file to have non-cone pattern, such
    as "**" anywhere or "*" in the wrong place, then we should respond
    appropriately. That is: warn that the patterns are not cone-mode, then
    revert to the old logic.

Updates in V2:

 * Added C-style quoting to the output of "git sparse-checkout list" in cone
   mode.
 * Improved documentation.
 * Responded to most style feedback. Hopefully I didn't miss anything.
 * I was lingering on this a little to see if I could also fix the issue
   raised in [1], but I have not figured that one out, yet.

[1] 
https://lore.kernel.org/git/062301d5d0bc$c3e17760$4ba46620$@Frontier.com/

Thanks, -Stolee

Derrick Stolee (11):
  t1091: use check_files to reduce boilerplate
  t1091: improve here-docs
  sparse-checkout: create leading directories
  clone: fix --sparse option with URLs
  sparse-checkout: cone mode does not recognize "**"
  sparse-checkout: detect short patterns
  sparse-checkout: warn on incorrect '*' in patterns
  sparse-checkout: properly match escaped characters
  sparse-checkout: write escaped patterns in cone mode
  sparse-checkout: use C-style quotes in 'list' subcommand
  sparse-checkout: improve docs around 'set' in cone mode

Jeff King (1):
  sparse-checkout: fix documentation typo for core.sparseCheckoutCone

 Documentation/git-sparse-checkout.txt |  19 +-
 builtin/clone.c                       |   2 +-
 builtin/sparse-checkout.c             |  59 ++++-
 dir.c                                 |  68 +++++-
 dir.h                                 |   1 +
 t/t1091-sparse-checkout-builtin.sh    | 323 +++++++++++++++-----------
 6 files changed, 317 insertions(+), 155 deletions(-)


base-commit: 4fd683b6a35eabd23dd5183da7f654a1e1f00325
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-513%2Fderrickstolee%2Fsparse-harden-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-513/derrickstolee/sparse-harden-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/513

Range-diff vs v1:

  -:  ---------- >  1:  1cc825412f t1091: use check_files to reduce boilerplate
  1:  9f7791ae5e !  2:  b7a6ad145a t1091: use check_files to reduce boilerplate
     @@ -1,14 +1,11 @@
      Author: Derrick Stolee <dstolee@microsoft.com>
      
     -    t1091: use check_files to reduce boilerplate
     +    t1091: improve here-docs
      
     -    When testing the sparse-checkout feature, we need to compare the
     -    contents of the working-directory against some expected output.
     -    Using here-docs was useful in the beginning, but became repetetive
     -    as the test script grew.
     -
     -    Create a check_files helper to make the tests simpler and easier
     -    to extend. It also reduces instances of bad here-doc whitespace.
     +    t1091-sparse-checkout-builtin.sh uses here-docs to populate the
     +    expected contents of the sparse-checkout file. These do not use
     +    shell interpolation, so use "-\EOF" instead of "-EOF". Also use
     +    proper tabbing.
      
          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
      
     @@ -16,20 +13,6 @@
       --- a/t/t1091-sparse-checkout-builtin.sh
       +++ b/t/t1091-sparse-checkout-builtin.sh
      @@
     - 	(cd "$1" && printf '%s\n' *)
     - }
     - 
     -+check_files() {
     -+	DIR=$1
     -+	printf "%s\n" $2 >expect &&
     -+	list_files $DIR >actual &&
     -+	test_cmp expect actual
     -+}
     -+
     - test_expect_success 'setup' '
     - 	git init repo &&
     - 	(
     -@@
       
       test_expect_success 'git sparse-checkout list (populated)' '
       	test_when_finished rm -f repo/.git/info/sparse-checkout &&
     @@ -59,11 +42,7 @@
       	EOF
       	test_cmp expect repo/.git/info/sparse-checkout &&
       	test_cmp_config -C repo true core.sparsecheckout &&
     --	list_files repo >dir  &&
     --	echo a >expect &&
     --	test_cmp expect dir
     -+	check_files repo a
     - '
     +@@
       
       test_expect_success 'git sparse-checkout list after init' '
       	git -C repo sparse-checkout list >actual &&
     @@ -90,16 +69,8 @@
      +	*folder*
       	EOF
       	test_cmp expect repo/.git/info/sparse-checkout &&
     --	list_files repo >dir  &&
     --	cat >expect <<-EOF &&
     --		a
     --		folder1
     --		folder2
     --	EOF
     --	test_cmp expect dir
     -+	check_files repo "a folder1 folder2"
     - '
     - 
     + 	check_files repo a folder1 folder2
     +@@
       test_expect_success 'clone --sparse' '
       	git clone --sparse repo clone &&
       	git -C clone sparse-checkout list >actual &&
     @@ -111,13 +82,7 @@
      +	!/*/
       	EOF
       	test_cmp expect actual &&
     --	list_files clone >dir &&
     --	echo a >expect &&
     --	test_cmp expect dir
     -+	check_files clone a
     - '
     - 
     - test_expect_success 'set enables config' '
     + 	check_files clone a
      @@
       
       test_expect_success 'set sparse-checkout using builtin' '
     @@ -133,15 +98,7 @@
       	EOF
       	git -C repo sparse-checkout list >actual &&
       	test_cmp expect actual &&
     - 	test_cmp expect repo/.git/info/sparse-checkout &&
     --	list_files repo >dir  &&
     --	cat >expect <<-EOF &&
     --		a
     --		folder1
     --		folder2
     --	EOF
     --	test_cmp expect dir
     -+	check_files repo "a folder1 folder2"
     +@@
       '
       
       test_expect_success 'set sparse-checkout using --stdin' '
     @@ -158,72 +115,10 @@
       	EOF
       	git -C repo sparse-checkout set --stdin <expect &&
       	git -C repo sparse-checkout list >actual &&
     - 	test_cmp expect actual &&
     - 	test_cmp expect repo/.git/info/sparse-checkout &&
     --	list_files repo >dir  &&
     --	cat >expect <<-EOF &&
     --		a
     --		folder1
     --		folder2
     --	EOF
     --	test_cmp expect dir
     -+	check_files repo "a folder1 folder2"
     - '
     - 
     - test_expect_success 'cone mode: match patterns' '
     -@@
     - 	git -C repo read-tree -mu HEAD 2>err &&
     - 	test_i18ngrep ! "disabling cone patterns" err &&
     - 	git -C repo reset --hard &&
     --	list_files repo >dir  &&
     --	cat >expect <<-EOF &&
     --		a
     --		folder1
     --		folder2
     --	EOF
     --	test_cmp expect dir
     -+	check_files repo "a folder1 folder2"
     - '
     - 
     - test_expect_success 'cone mode: warn on bad pattern' '
      @@
     - 	test_path_is_file repo/.git/info/sparse-checkout &&
     - 	git -C repo config --list >config &&
     - 	test_must_fail git config core.sparseCheckout &&
     --	list_files repo >dir &&
     --	cat >expect <<-EOF &&
     --		a
     --		deep
     --		folder1
     --		folder2
     --	EOF
     --	test_cmp expect dir
     -+	check_files repo "a deep folder1 folder2"
     - '
     - 
     - test_expect_success 'cone mode: init and set' '
     -@@
     - 	test_cmp expect dir &&
     - 	git -C repo sparse-checkout set deep/deeper1/deepest/ 2>err &&
     - 	test_must_be_empty err &&
     --	list_files repo >dir  &&
     --	cat >expect <<-EOF &&
     --		a
     --		deep
     --	EOF
     --	test_cmp expect dir &&
     --	list_files repo/deep >dir  &&
     --	cat >expect <<-EOF &&
     --		a
     --		deeper1
     --	EOF
     --	test_cmp expect dir &&
     --	list_files repo/deep/deeper1 >dir  &&
     --	cat >expect <<-EOF &&
     --		a
     --		deepest
     --	EOF
     --	test_cmp expect dir &&
     + 	check_files repo a deep &&
     + 	check_files repo/deep a deeper1 &&
     + 	check_files repo/deep/deeper1 a deepest &&
      -	cat >expect <<-EOF &&
      -		/*
      -		!/*/
     @@ -232,9 +127,6 @@
      -		/deep/deeper1/
      -		!/deep/deeper1/*/
      -		/deep/deeper1/deepest/
     -+	check_files repo "a deep" &&
     -+	check_files repo/deep "a deeper1" &&
     -+	check_files repo/deep/deeper1 "a deepest" &&
      +	cat >expect <<-\EOF &&
      +	/*
      +	!/*/
     @@ -253,14 +145,7 @@
      +	folder2
       	EOF
       	test_must_be_empty err &&
     --	cat >expect <<-EOF &&
     --		a
     --		folder1
     --		folder2
     --	EOF
     --	list_files repo >dir &&
     --	test_cmp expect dir
     -+	check_files repo "a folder1 folder2"
     + 	check_files repo a folder1 folder2
       '
       
       test_expect_success 'cone mode: list' '
     @@ -288,21 +173,6 @@
       	EOF
       	test_cmp repo/.git/info/sparse-checkout expect
       '
     -@@
     - 	test_must_fail git -C repo sparse-checkout set deep/deeper1 2>err &&
     - 	test_i18ngrep "cannot set sparse-checkout patterns" err &&
     - 	test_cmp repo/.git/info/sparse-checkout expect &&
     --	list_files repo/deep >dir &&
     --	cat >expect <<-EOF &&
     --		a
     --		deeper1
     --		deeper2
     --	EOF
     --	test_cmp dir expect
     -+	check_files repo/deep "a deeper1 deeper2"
     - '
     - 
     - test_expect_success 'revert to old sparse-checkout on empty update' '
      @@
       test_expect_success 'cone mode: set with core.ignoreCase=true' '
       	git -C repo sparse-checkout init --cone &&
     @@ -317,37 +187,4 @@
      +	/folder1/
       	EOF
       	test_cmp expect repo/.git/info/sparse-checkout &&
     --	list_files repo >dir &&
     --	cat >expect <<-EOF &&
     --		a
     --		folder1
     --	EOF
     --	test_cmp expect dir
     -+	check_files repo "a folder1"
     - '
     - 
     - test_expect_success 'interaction with submodules' '
     -@@
     - 		git sparse-checkout init --cone &&
     - 		git sparse-checkout set folder1
     - 	) &&
     --	list_files super >dir &&
     --	cat >expect <<-\EOF &&
     --		a
     --		folder1
     --		modules
     --	EOF
     --	test_cmp expect dir &&
     --	list_files super/modules/child >dir &&
     --	cat >expect <<-\EOF &&
     --		a
     --		deep
     --		folder1
     --		folder2
     --	EOF
     --	test_cmp expect dir
     -+	check_files super "a folder1 modules" &&
     -+	check_files super/modules/child "a deep folder1 folder2"
     - '
     - 
     - test_done
     + 	check_files repo a folder1
  2:  53a266f9aa !  3:  5497ad8778 sparse-checkout: create leading directories
     @@ -34,7 +34,7 @@
       --- a/t/t1091-sparse-checkout-builtin.sh
       +++ b/t/t1091-sparse-checkout-builtin.sh
      @@
     - 	check_files super/modules/child "a deep folder1 folder2"
     + 	check_files super/modules/child a deep folder1 folder2
       '
       
      +test_expect_success 'different sparse-checkouts with worktrees' '
     @@ -43,8 +43,8 @@
      +	git -C worktree sparse-checkout init --cone &&
      +	git -C repo sparse-checkout set folder1 &&
      +	git -C worktree sparse-checkout set deep/deeper1 &&
     -+	check_files repo "a folder1" &&
     -+	check_files worktree "a deep"
     ++	check_files repo a folder1 &&
     ++	check_files worktree a deep
      +'
      +
       test_done
  3:  3ef8e021a5 !  4:  4991a51f6d clone: fix --sparse option with URLs
     @@ -22,6 +22,7 @@
          Update that target directory to evaluate this correctly. I have also
          manually tested that https:// URLs are handled correctly as well.
      
     +    Acked-by: Taylor Blau <me@ttaylorr.com>
          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
      
       diff --git a/builtin/clone.c b/builtin/clone.c
  -:  ---------- >  5:  ae78c3069b sparse-checkout: fix documentation typo for core.sparseCheckoutCone
  4:  dfa7e20444 !  6:  2ad4d3e467 sparse-checkout: cone mode does not recognize "**"
     @@ -30,7 +30,6 @@
       
      +	if (strstr(given->pattern, "**")) {
      +		/* Not a cone pattern. */
     -+		pl->use_cone_patterns = 0;
      +		warning(_("unrecognized pattern: '%s'"), given->pattern);
      +		goto clear_hashmaps;
      +	}
     @@ -38,12 +37,17 @@
       	if (given->patternlen > 2 &&
       	    !strcmp(given->pattern + given->patternlen - 2, "/*")) {
       		if (!(given->flags & PATTERN_FLAG_NEGATIVE)) {
     + 			/* Not a cone pattern. */
     +-			pl->use_cone_patterns = 0;
     + 			warning(_("unrecognized pattern: '%s'"), given->pattern);
     + 			goto clear_hashmaps;
     + 		}
      
       diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
       --- a/t/t1091-sparse-checkout-builtin.sh
       +++ b/t/t1091-sparse-checkout-builtin.sh
      @@
     - 	check_files worktree "a deep"
     + 	check_files worktree a deep
       '
       
      +check_read_tree_errors () {
     @@ -57,7 +61,7 @@
      +	else
      +		test_i18ngrep "$ERRORS" err
      +	fi &&
     -+	check_files $REPO "$FILES"
     ++	check_files $REPO $FILES
      +}
      +
      +test_expect_success 'pattern-checks: /A/**' '
  5:  9be49908fd !  7:  aace064510 sparse-checkout: detect short patterns
     @@ -21,8 +21,8 @@
      +	if (given->patternlen <= 2 ||
      +	    strstr(given->pattern, "**")) {
       		/* Not a cone pattern. */
     - 		pl->use_cone_patterns = 0;
       		warning(_("unrecognized pattern: '%s'"), given->pattern);
     + 		goto clear_hashmaps;
      
       diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
       --- a/t/t1091-sparse-checkout-builtin.sh
  6:  77a514f50b !  8:  d2a510a3bb sparse-checkout: warn on incorrect '*' in patterns
     @@ -34,8 +34,7 @@
      +	    *given->pattern == '*' ||
       	    strstr(given->pattern, "**")) {
       		/* Not a cone pattern. */
     - 		pl->use_cone_patterns = 0;
     -@@
     + 		warning(_("unrecognized pattern: '%s'"), given->pattern);
       		goto clear_hashmaps;
       	}
       
     @@ -57,7 +56,6 @@
      +			goto increment;
      +
      +		/* Not a cone pattern. */
     -+		pl->use_cone_patterns = 0;
      +		warning(_("unrecognized pattern: '%s'"), given->pattern);
      +		goto clear_hashmaps;
      +
  7:  09dbe1f902 !  9:  65c53d7526 sparse-checkout: properly match escaped characters
     @@ -9,7 +9,7 @@
          Create dup_and_filter_pattern() method to convert a pattern by
          removing escape characters and dropping an optional "/*" at the end.
          This method is available in dir.h as we will use it in
     -    builtin/sparse-chekcout.c in a later change.
     +    builtin/sparse-checkout.c in a later change.
      
          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
      
  8:  79b6e9a565 = 10:  c27a17a2fc sparse-checkout: write escaped patterns in cone mode
  -:  ---------- > 11:  526d5becbc sparse-checkout: use C-style quotes in 'list' subcommand
  -:  ---------- > 12:  1b5858adee sparse-checkout: improve docs around 'set' in cone mode

-- 
gitgitgadget