| Message ID | pull.726.v3.git.git.1585270142.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Sparse checkout improvements -- improved sparsity updating | expand |
On 3/26/2020 8:48 PM, Elijah Newren via GitGitGadget wrote: > 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) The other changes below look good to me. My Reviewed-by stands. > * 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. Thanks! -Stolee
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 &&