[PATCH v3 00/18] Sparse checkout improvements -- improved sparsity updating

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

This series provides a replacement for the sparsity updating in
sparse-checkout that is based on the logic from git read-tree -mu HEAD. The
most important bit is patch 9 and its lengthy commit message explaining the
current state and rationale for most the series, though patches 16 and 17
have additional related directions and rationale for the series. Those three
patches are the most important to review.

Changes since v2:

 * addressed Stolee's cleanups and added his Reviewed-by to the series
   (hopefully the next two changes don't invalidate that)
 * added a test for the new 'reapply' subcommand (noticed it was missing
   with Stolee's test coverage report)
 * fix a minor issue with two of the other tests I modified -- when I'm
   grepping through stderr for a command, I need to make sure to actually
   record stderr from that command (otherwise my grep is looking through the
   stderr of a previous command that was recorded). Also, since I messed
   this up, I added one or two more sanity checks while I was at it.

Elijah Newren (18):
  unpack-trees: fix minor typo in comment
  unpack-trees: remove unused error type
  unpack-trees: simplify verify_absent_sparse()
  unpack-trees: simplify pattern_list freeing
  t1091: make some tests a little more defensive against failures
  unpack-trees: allow check_updates() to work on a different index
  unpack-trees: do not mark a dirty path with SKIP_WORKTREE
  unpack-trees: pull sparse-checkout pattern reading into a new function
  unpack-trees: add a new update_sparsity() function
  sparse-checkout: use new update_sparsity() function
  sparse-checkout: use improved unpack_trees porcelain messages
  unpack-trees: move ERROR_WOULD_LOSE_SUBMODULE earlier
  unpack-trees: rename ERROR_* fields meant for warnings to WARNING_*
  unpack-trees: split display_error_msgs() into two
  unpack-trees: make sparse path messages sound like warnings
  unpack-trees: provide warnings on sparse updates for unmerged paths
    too
  unpack-trees: failure to set SKIP_WORKTREE bits always just a warning
  sparse-checkout: provide a new reapply subcommand

 Documentation/git-sparse-checkout.txt |  10 +
 builtin/sparse-checkout.c             |  55 +++---
 t/t1011-read-tree-sparse-checkout.sh  |  11 +-
 t/t1091-sparse-checkout-builtin.sh    | 105 ++++++++++-
 t/t2018-checkout-branch.sh            |  22 +++
 unpack-trees.c                        | 255 ++++++++++++++++++++------
 unpack-trees.h                        |  28 ++-
 7 files changed, 375 insertions(+), 111 deletions(-)


base-commit: be8661a3286c67a5d4088f4226cbd7f8b76544b0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-726%2Fnewren%2Fsparse-checkout-improvements-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-726/newren/sparse-checkout-improvements-v3
Pull-Request: https://github.com/git/git/pull/726

Range-diff vs v2:

  1:  3c183727c98 !  1:  bb41a2a52f4 unpack-trees: fix minor typo in comment
     @@ -2,6 +2,7 @@
      
          unpack-trees: fix minor typo in comment
      
     +    Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       diff --git a/unpack-trees.c b/unpack-trees.c
  2:  e6beb1c5810 !  2:  1dc36eb33f7 unpack-trees: remove unused error type
     @@ -19,6 +19,7 @@
          introduced and nothing else has used it in the intervening decade
          either.  Remove it.
      
     +    Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       diff --git a/unpack-trees.c b/unpack-trees.c
  3:  739b52393fe !  3:  97a95e91d8f unpack-trees: simplify verify_absent_sparse()
     @@ -11,6 +11,7 @@
          and no other caller has been created, and no other value has ever been
          passed, so just pass the expected value to begin with.
      
     +    Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       diff --git a/unpack-trees.c b/unpack-trees.c
  4:  65772dd46df !  4:  1ce5dbe9c68 unpack-trees: simplify pattern_list freeing
     @@ -13,6 +13,7 @@
          additional flag in unpack_trees_options.  Put the logic back in the
          relevant function and discard the now unnecessary flag.
      
     +    Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
  5:  5cbab2a7a56 !  5:  da4b74093cd t1091: make some tests a little more defensive against failures
     @@ -2,6 +2,7 @@
      
          t1091: make some tests a little more defensive against failures
      
     +    Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
  6:  5fea19f0136 !  6:  555eedf5380 unpack-trees: allow check_updates() to work on a different index
     @@ -6,6 +6,7 @@
          to use this function in combination with a different index_state, so
          take the intended index_state as a parameter.
      
     +    Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       diff --git a/unpack-trees.c b/unpack-trees.c
  7:  3d2531ca818 !  7:  10ec70fa697 unpack-trees: do not mark a dirty path with SKIP_WORKTREE
     @@ -15,6 +15,7 @@
          paths.  Leaving dirty working files in the working tree is thus a
          natural extension of what we have already been doing.
      
     +    Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       diff --git a/unpack-trees.c b/unpack-trees.c
  8:  27d6304d33f !  8:  8ed2f4e49ed unpack-trees: pull sparse-checkout pattern reading into a new function
     @@ -6,6 +6,7 @@
          path_patterns from $GIT_DIR/info/sparse-checkout so that we can re-use
          it elsewhere.
      
     +    Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       diff --git a/unpack-trees.c b/unpack-trees.c
  9:  a46439c8536 !  9:  3a1f084641e unpack-trees: add a new update_sparsity() function
     @@ -71,6 +71,7 @@
          are marked SKIP_WORKTREE and thus doesn't delete them, and `commit -a`
          is similar), it seems logical to me.
      
     +    Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       diff --git a/unpack-trees.c b/unpack-trees.c
     @@ -130,7 +131,6 @@
      +
      +		if (!ce_skip_worktree(ce))
      +			empty_worktree = 0;
     -+
      +	}
      +
      +	/*
     @@ -166,8 +166,8 @@
       --- a/unpack-trees.h
       +++ b/unpack-trees.h
      @@
     - 	NB_UNPACK_TREES_ERROR_TYPES
     - };
     + int unpack_trees(unsigned n, struct tree_desc *t,
     + 		 struct unpack_trees_options *options);
       
      +enum update_sparsity_result {
      +	UPDATE_SPARSITY_SUCCESS = 0,
     @@ -176,14 +176,7 @@
      +	UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES = -2
      +};
      +
     - /*
     -  * Sets the list of user-friendly error messages to be used by the
     -  * command "cmd" (either merge or checkout), and show_all_errors to 1.
     -@@
     - int unpack_trees(unsigned n, struct tree_desc *t,
     - 		 struct unpack_trees_options *options);
     - 
     -+int update_sparsity(struct unpack_trees_options *options);
     ++enum update_sparsity_result update_sparsity(struct unpack_trees_options *options);
      +
       int verify_uptodate(const struct cache_entry *ce,
       		    struct unpack_trees_options *o);
 10:  27ed4a3f7a2 ! 10:  9040ac97175 sparse-checkout: use new update_sparsity() function
     @@ -6,6 +6,12 @@
          codepaths for setting the SKIP_WORKTREE bits and instead use the new
          update_sparsity() function.
      
     +    Note that when an issue is hit, the error message splits 'error' and
     +    'Cannot update sparse checkout' on separate lines.  For now, we use two
     +    greps to find both pieces of the error message but subsequent commits
     +    will clean up the messages reported to the user.
     +
     +    Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
     @@ -147,14 +153,21 @@
      +	test_i18ngrep "error" err &&
      +	test_i18ngrep "Cannot update sparse checkout" err &&
      +
     -+	git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
     ++	git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* 2>err &&
      +	test_i18ngrep "error" err &&
      +	test_i18ngrep "Cannot update sparse checkout" err &&
     ++	test_path_is_file dirty/folder1/a &&
      +
     -+	git -C dirty sparse-checkout disable &&
     -+	test_i18ngrep "error" err &&
     -+	test_i18ngrep "Cannot update sparse checkout" err &&
     ++	git -C dirty sparse-checkout disable 2>err &&
     ++	test_must_be_empty err &&
      +
       	git -C dirty reset --hard &&
       	git -C dirty sparse-checkout init &&
       	git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
     +-	git -C dirty sparse-checkout disable
     ++	test_path_is_missing dirty/folder1/a &&
     ++	git -C dirty sparse-checkout disable &&
     ++	test_path_is_file dirty/folder1/a
     + '
     + 
     + test_expect_success 'cone mode: set with core.ignoreCase=true' '
 11:  79b9cc1df55 ! 11:  e9c7e8ec46f sparse-checkout: use improved unpack_trees porcelain messages
     @@ -23,6 +23,7 @@
          we even called unpack_trees().  The previous commit changed that,
          though, so let's use the better error messages.
      
     +    Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
     @@ -50,15 +51,10 @@
      -	test_i18ngrep "Cannot update sparse checkout" err &&
      +	test_i18ngrep "error.*Cannot update sparse checkout" err &&
       
     - 	git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
     + 	git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* 2>err &&
      -	test_i18ngrep "error" err &&
      -	test_i18ngrep "Cannot update sparse checkout" err &&
      +	test_i18ngrep "error.*Cannot update sparse checkout" err &&
     + 	test_path_is_file dirty/folder1/a &&
       
     - 	git -C dirty sparse-checkout disable &&
     --	test_i18ngrep "error" err &&
     --	test_i18ngrep "Cannot update sparse checkout" err &&
     -+	test_i18ngrep "error.*Cannot update sparse checkout" err &&
     - 
     - 	git -C dirty reset --hard &&
     - 	git -C dirty sparse-checkout init &&
     + 	git -C dirty sparse-checkout disable 2>err &&
 12:  2ba7bbdaee3 ! 12:  c3741107a9d unpack-trees: move ERROR_WOULD_LOSE_SUBMODULE earlier
     @@ -5,6 +5,7 @@
          A minor change, but we want to convert the sparse messages to warnings
          and this allows us to group warnings and errors.
      
     +    Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       diff --git a/unpack-trees.c b/unpack-trees.c
 13:  203ece0ec2f ! 13:  90c514e220b unpack-trees: rename ERROR_* fields meant for warnings to WARNING_*
     @@ -6,6 +6,7 @@
          rather than an error; rename the enum values to reflect this intent as
          a simple step towards that goal.
      
     +    Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       diff --git a/unpack-trees.c b/unpack-trees.c
     @@ -72,4 +73,4 @@
      +	NB_UNPACK_TREES_ERROR_TYPES,
       };
       
     - enum update_sparsity_result {
     + /*
 14:  57679c8e292 ! 14:  5676e890f65 unpack-trees: split display_error_msgs() into two
     @@ -10,6 +10,7 @@
          A subsequent commit will further modify unpack_trees() and how it calls
          the new display_warning_msgs().
      
     +    Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
     @@ -22,16 +23,12 @@
      -	test_i18ngrep "error.*Cannot update sparse checkout" err &&
      +	test_i18ngrep "warning.*Cannot update sparse checkout" err &&
       
     - 	git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
     + 	git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* 2>err &&
      -	test_i18ngrep "error.*Cannot update sparse checkout" err &&
      +	test_i18ngrep "warning.*Cannot update sparse checkout" err &&
     + 	test_path_is_file dirty/folder1/a &&
       
     - 	git -C dirty sparse-checkout disable &&
     --	test_i18ngrep "error.*Cannot update sparse checkout" err &&
     -+	test_i18ngrep "warning.*Cannot update sparse checkout" err &&
     - 
     - 	git -C dirty reset --hard &&
     - 	git -C dirty sparse-checkout init &&
     + 	git -C dirty sparse-checkout disable 2>err &&
      
       diff --git a/unpack-trees.c b/unpack-trees.c
       --- a/unpack-trees.c
     @@ -100,7 +97,7 @@
      +{
      +	int e;
      +	unsigned warning_displayed = 0;
     -+	for (e = NB_UNPACK_TREES_ERROR_TYPES+1;
     ++	for (e = NB_UNPACK_TREES_ERROR_TYPES + 1;
      +	     e < NB_UNPACK_TREES_WARNING_TYPES; e++) {
      +		struct string_list *rejects = &o->unpack_rejects[e];
      +
     @@ -160,7 +157,7 @@
      +	NB_UNPACK_TREES_WARNING_TYPES,
       };
       
     - enum update_sparsity_result {
     + /*
      @@
       	struct dir_struct *dir;
       	struct pathspec *pathspec;
 15:  02a8e01f35b ! 15:  74e13ccee40 unpack-trees: make sparse path messages sound like warnings
     @@ -7,6 +7,7 @@
          operation abort.  Reword the messages to make sense in their new
          context.
      
     +    Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
     @@ -28,16 +29,12 @@
      -	test_i18ngrep "warning.*Cannot update sparse checkout" err &&
      +	test_i18ngrep "warning.*The following paths are not up to date" err &&
       
     - 	git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
     + 	git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* 2>err &&
      -	test_i18ngrep "warning.*Cannot update sparse checkout" err &&
      +	test_i18ngrep "warning.*The following paths are not up to date" err &&
     + 	test_path_is_file dirty/folder1/a &&
       
     - 	git -C dirty sparse-checkout disable &&
     --	test_i18ngrep "warning.*Cannot update sparse checkout" err &&
     -+	test_i18ngrep "warning.*The following paths are not up to date" err &&
     - 
     - 	git -C dirty reset --hard &&
     - 	git -C dirty sparse-checkout init &&
     + 	git -C dirty sparse-checkout disable 2>err &&
      
       diff --git a/unpack-trees.c b/unpack-trees.c
       --- a/unpack-trees.c
 16:  f664a6dcbb3 ! 16:  5d9e8a9d27e unpack-trees: provide warnings on sparse updates for unmerged paths too
     @@ -7,13 +7,14 @@
          those files have dirty changes.  Add a similar warning for unmerged
          paths as well.
      
     +    Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       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
      @@
     - 	git -C dirty sparse-checkout disable
     + 	test_path_is_file dirty/folder1/a
       '
       
      +test_expect_success 'sparse-checkout (init|set|disable) warns with unmerged status' '
     @@ -28,10 +29,11 @@
      +	git -C unmerged sparse-checkout init 2>err &&
      +	test_i18ngrep "warning.*The following paths are unmerged" err &&
      +
     -+	git -C unmerged sparse-checkout set /folder2/* /deep/deeper1/* &&
     ++	git -C unmerged sparse-checkout set /folder2/* /deep/deeper1/* 2>err &&
      +	test_i18ngrep "warning.*The following paths are unmerged" err &&
     ++	test_path_is_file dirty/folder1/a &&
      +
     -+	git -C unmerged sparse-checkout disable &&
     ++	git -C unmerged sparse-checkout disable 2>err &&
      +	test_i18ngrep "warning.*The following paths are unmerged" err &&
      +
      +	git -C unmerged reset --hard &&
 17:  fa997a1c72f ! 17:  844306c3e86 unpack-trees: failure to set SKIP_WORKTREE bits always just a warning
     @@ -37,6 +37,7 @@
          Downgrade any errors from inability to toggle the SKIP_WORKTREE bit to a
          warning and allow the operations to continue.
      
     +    Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
 18:  5730f7d250a ! 18:  93dbdd78acf sparse-checkout: provide a new reapply subcommand
     @@ -7,6 +7,7 @@
          due to dirty changes, users may want a command to simply 'reapply' the
          sparsity rules.  Provide one.
      
     +    Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
     @@ -64,3 +65,55 @@
       		if (!strcmp(argv[0], "disable"))
       			return sparse_checkout_disable(argc, argv);
       	}
     +
     + 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
     +@@
     + 	git -C unmerged sparse-checkout disable
     + '
     + 
     ++test_expect_success 'sparse-checkout reapply' '
     ++	git clone repo tweak &&
     ++
     ++	echo dirty >tweak/deep/deeper2/a &&
     ++
     ++	cat >input <<-EOF &&
     ++	0 0000000000000000000000000000000000000000	folder1/a
     ++	100644 $(git -C tweak rev-parse HEAD:folder1/a) 1	folder1/a
     ++	EOF
     ++	git -C tweak update-index --index-info <input &&
     ++
     ++	git -C tweak sparse-checkout init --cone 2>err &&
     ++	test_i18ngrep "warning.*The following paths are not up to date" err &&
     ++	test_i18ngrep "warning.*The following paths are unmerged" err &&
     ++
     ++	git -C tweak sparse-checkout set folder2 deep/deeper1 2>err &&
     ++	test_i18ngrep "warning.*The following paths are not up to date" err &&
     ++	test_i18ngrep "warning.*The following paths are unmerged" err &&
     ++
     ++	git -C tweak sparse-checkout reapply 2>err &&
     ++	test_i18ngrep "warning.*The following paths are not up to date" err &&
     ++	test_path_is_file tweak/deep/deeper2/a &&
     ++	test_i18ngrep "warning.*The following paths are unmerged" err &&
     ++	test_path_is_file tweak/folder1/a &&
     ++
     ++	git -C tweak checkout HEAD deep/deeper2/a &&
     ++	git -C tweak sparse-checkout reapply 2>err &&
     ++	test_i18ngrep ! "warning.*The following paths are not up to date" err &&
     ++	test_path_is_missing tweak/deep/deeper2/a &&
     ++	test_i18ngrep "warning.*The following paths are unmerged" err &&
     ++	test_path_is_file tweak/folder1/a &&
     ++
     ++	git -C tweak add folder1/a &&
     ++	git -C tweak sparse-checkout reapply 2>err &&
     ++	test_must_be_empty err &&
     ++	test_path_is_missing tweak/deep/deeper2/a &&
     ++	test_path_is_missing tweak/folder1/a &&
     ++
     ++	git -C tweak 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