From 83ad8ca596dd879d2907a240fb24a1603bb1decc Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Thu, 29 Jul 2021 14:52:03 +0000 Subject: [PATCH 1/5] t1092: test merge conflicts outside cone Conflicts can occur outside of the sparse-checkout definition, and in that case users might try to resolve the conflicts in several ways. Document a few of these ways in a test. Make it clear that this behavior is not necessarily the optimal flow, since users can become confused when Git deletes these files from the worktree in later commands. Reviewed-by: Elijah Newren Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- t/t1092-sparse-checkout-compatibility.sh | 43 ++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index 91e30d6ec2..4c3bcb3499 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -114,6 +114,16 @@ test_expect_success 'setup' ' git add . && git commit -m "file to dir" && + for side in left right + do + git checkout -b merge-$side base && + echo $side >>deep/deeper2/a && + echo $side >>folder1/a && + echo $side >>folder2/a && + git add . && + git commit -m "$side" || return 1 + done && + git checkout -b deepest base && echo "updated deepest" >deep/deeper1/deepest/a && git commit -a -m "update deepest" && @@ -482,6 +492,39 @@ test_expect_success 'merge' ' test_all_match git rev-parse HEAD^{tree} ' +# NEEDSWORK: This test is documenting current behavior, but that +# behavior can be confusing to users so there is desire to change it. +# Right now, users might be using this flow to work through conflicts, +# so any solution should present advice to users who try this sequence +# of commands to follow whatever new method we create. +test_expect_success 'merge with conflict outside cone' ' + init_repos && + + test_all_match git checkout -b merge-tip merge-left && + test_all_match git status --porcelain=v2 && + test_all_match test_must_fail git merge -m merge merge-right && + test_all_match git status --porcelain=v2 && + + # Resolve the conflict in different ways: + # 1. Revert to the base + test_all_match git checkout base -- deep/deeper2/a && + test_all_match git status --porcelain=v2 && + + # 2. Add the file with conflict markers + test_all_match git add folder1/a && + test_all_match git status --porcelain=v2 && + + # 3. Rename the file to another sparse filename and + # accept conflict markers as resolved content. + run_on_all mv folder2/a folder2/z && + test_all_match git add folder2 && + test_all_match git status --porcelain=v2 && + + test_all_match git merge --continue && + test_all_match git status --porcelain=v2 && + test_all_match git rev-parse HEAD^{tree} +' + test_expect_success 'merge with outside renames' ' init_repos && From 5e7cbab19680745d4f3e0c50903025d9f02e7468 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Thu, 29 Jul 2021 14:52:04 +0000 Subject: [PATCH 2/5] add: allow operating on a sparse-only index Disable command_requires_full_index for 'git add'. This does not require any additional removals of ensure_full_index(). The main reason is that 'git add' discovers changes based on the pathspec and the worktree itself. These are then inserted into the index directly, and calls to index_name_pos() or index_file_exists() already call expand_to_path() at the appropriate time to support a sparse-index. Add a test to check that 'git add -A' and 'git add ' does not expand the index at all, as long as is not within a sparse directory. This does not help the global 'git add .' case. We can measure the improvement using p2000-sparse-operations.sh with these results: Test HEAD~1 HEAD ------------------------------------------------------------------------------ 2000.6: git add -A (full-index-v3) 0.35(0.30+0.05) 0.37(0.29+0.06) +5.7% 2000.7: git add -A (full-index-v4) 0.31(0.26+0.06) 0.33(0.27+0.06) +6.5% 2000.8: git add -A (sparse-index-v3) 0.57(0.53+0.07) 0.05(0.04+0.08) -91.2% 2000.9: git add -A (sparse-index-v4) 0.58(0.55+0.06) 0.05(0.05+0.06) -91.4% While the 91% improvement seems impressive, it's important to recognize that previously we had significant overhead for expanding the sparse-index. Comparing to the full index case, 'git add -A' goes from 0.37s to 0.05s, which is "only" an 86% improvement. This modification to 'git add' creates some behavior change depending on the use of a sparse index. We modify a test in t1092 to demonstrate these changes which will be remedied in future changes. Reviewed-by: Elijah Newren Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- builtin/add.c | 3 +++ t/t1092-sparse-checkout-compatibility.sh | 25 +++++++++++++++++------- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/builtin/add.c b/builtin/add.c index b773b5a499..c76e6ddd35 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -528,6 +528,9 @@ int cmd_add(int argc, const char **argv, const char *prefix) add_new_files = !take_worktree_changes && !refresh_only && !add_renormalize; require_pathspec = !(take_worktree_changes || (0 < addremove_explicit)); + prepare_repo_settings(the_repository); + the_repository->settings.command_requires_full_index = 0; + hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR); /* diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index 4c3bcb3499..77343cb6d9 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -340,21 +340,27 @@ test_expect_success 'status/add: outside sparse cone' ' test_sparse_match git status --porcelain=v2 && - # This "git add folder1/a" fails with a warning - # in the sparse repos, differing from the full - # repo. This is intentional. + # Adding the path outside of the sparse-checkout cone should fail. test_sparse_match test_must_fail git add folder1/a && - test_sparse_match test_must_fail git add --refresh folder1/a && - test_all_match git status --porcelain=v2 && + + test_must_fail git -C sparse-checkout add --refresh folder1/a 2>sparse-checkout-err && + test_must_fail git -C sparse-index add --refresh folder1/a 2>sparse-index-err && + # NEEDSWORK: A sparse index changes the error message. + ! test_cmp sparse-checkout-err sparse-index-err && + + # NEEDSWORK: Adding a newly-tracked file outside the cone succeeds + test_sparse_match git add folder1/new && test_all_match git add . && test_all_match git status --porcelain=v2 && test_all_match git commit -m folder1/new && + test_all_match git rev-parse HEAD^{tree} && run_on_all ../edit-contents folder1/newer && test_all_match git add folder1/ && test_all_match git status --porcelain=v2 && - test_all_match git commit -m folder1/newer + test_all_match git commit -m folder1/newer && + test_all_match git rev-parse HEAD^{tree} ' test_expect_success 'checkout and reset --hard' ' @@ -641,7 +647,12 @@ test_expect_success 'sparse-index is not expanded' ' git -C sparse-index reset --hard && ensure_not_expanded checkout rename-out-to-out -- deep/deeper1 && git -C sparse-index reset --hard && - ensure_not_expanded restore -s rename-out-to-out -- deep/deeper1 + ensure_not_expanded restore -s rename-out-to-out -- deep/deeper1 && + + echo >>sparse-index/README.md && + ensure_not_expanded add -A && + echo >>sparse-index/extra.txt && + ensure_not_expanded add extra.txt ' # NEEDSWORK: a sparse-checkout behaves differently from a full checkout From 4eaffd81a5fbffc692f1044374a3a16689fc37a5 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Thu, 29 Jul 2021 14:52:05 +0000 Subject: [PATCH 3/5] pathspec: stop calling ensure_full_index The add_pathspec_matches_against_index() focuses on matching a pathspec to file entries in the index. This already works correctly for its only use: checking if untracked files exist in the index. The compatibility checks in t1092 already test that 'git add ' works for a directory outside of the sparse cone. That provides coverage for removing this guard. This finalizes our ability to run 'git add .' without expanding a sparse index to a full one. This is evidenced by an update to t1092 and by these performance numbers for p2000-sparse-operations.sh: Test HEAD~1 HEAD -------------------------------------------------------------------------------- 2000.10: git add . (full-index-v3) 0.37(0.28+0.07) 0.36(0.27+0.06) -2.7% 2000.11: git add . (full-index-v4) 0.33(0.26+0.06) 0.32(0.28+0.05) -3.0% 2000.12: git add . (sparse-index-v3) 0.57(0.53+0.07) 0.06(0.06+0.07) -89.5% 2000.13: git add . (sparse-index-v4) 0.57(0.53+0.07) 0.05(0.03+0.09) -91.2% While the ~90% improvement is shown by the test results, it is worth noting that expanding the sparse index was adding overhead in previous commits. Comparing to the full index case, we see the performance go from 0.33s to 0.05s, an 85% improvement. Reviewed-by: Elijah Newren Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- pathspec.c | 2 -- t/t1092-sparse-checkout-compatibility.sh | 7 +++---- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/pathspec.c b/pathspec.c index 08f8d3eedc..44306fdaca 100644 --- a/pathspec.c +++ b/pathspec.c @@ -37,8 +37,6 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec, num_unmatched++; if (!num_unmatched) return; - /* TODO: audit for interaction with sparse-index. */ - ensure_full_index(istate); for (i = 0; i < istate->cache_nr; i++) { const struct cache_entry *ce = istate->cache[i]; if (sw_action == PS_IGNORE_SKIP_WORKTREE && ce_skip_worktree(ce)) diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index 77343cb6d9..dee20db5bb 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -322,9 +322,6 @@ test_expect_success 'commit including unstaged changes' ' test_expect_success 'status/add: outside sparse cone' ' init_repos && - # adding a "missing" file outside the cone should fail - test_sparse_match test_must_fail git add folder1/a && - # folder1 is at HEAD, but outside the sparse cone run_on_sparse mkdir folder1 && cp initial-repo/folder1/a sparse-checkout/folder1/a && @@ -652,7 +649,9 @@ test_expect_success 'sparse-index is not expanded' ' echo >>sparse-index/README.md && ensure_not_expanded add -A && echo >>sparse-index/extra.txt && - ensure_not_expanded add extra.txt + ensure_not_expanded add extra.txt && + echo >>sparse-index/untracked.txt && + ensure_not_expanded add . ' # NEEDSWORK: a sparse-checkout behaves differently from a full checkout From 939fa07582a2c9455e71f599263e2dcbe1d655b5 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Thu, 29 Jul 2021 14:52:06 +0000 Subject: [PATCH 4/5] add: ignore outside the sparse-checkout in refresh() Since b243012 (refresh_index(): add flag to ignore SKIP_WORKTREE entries, 2021-04-08), 'git add --refresh ' will output a warning message when the path is outside the sparse-checkout definition. The implementation of this warning happened in parallel with the sparse-index work to add ensure_full_index() calls throughout the codebase. Update this loop to have the proper logic that checks to see if the pathspec is outside the sparse-checkout definition. This avoids the need to expand the sparse directory entry and determine if the path is tracked, untracked, or ignored. We simply avoid updating the stat() information because there isn't even an entry that matches the path! Reviewed-by: Elijah Newren Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- builtin/add.c | 10 +++++++++- t/t1092-sparse-checkout-compatibility.sh | 6 +----- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/builtin/add.c b/builtin/add.c index c76e6ddd35..d512ece655 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -192,13 +192,21 @@ static int refresh(int verbose, const struct pathspec *pathspec) struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP; int flags = REFRESH_IGNORE_SKIP_WORKTREE | (verbose ? REFRESH_IN_PORCELAIN : REFRESH_QUIET); + struct pattern_list pl = { 0 }; + int sparse_checkout_enabled = !get_sparse_checkout_patterns(&pl); seen = xcalloc(pathspec->nr, 1); refresh_index(&the_index, flags, pathspec, seen, _("Unstaged changes after refreshing the index:")); for (i = 0; i < pathspec->nr; i++) { if (!seen[i]) { - if (matches_skip_worktree(pathspec, i, &skip_worktree_seen)) { + const char *path = pathspec->items[i].original; + int dtype = DT_REG; + + if (matches_skip_worktree(pathspec, i, &skip_worktree_seen) || + (sparse_checkout_enabled && + !path_matches_pattern_list(path, strlen(path), NULL, + &dtype, &pl, &the_index))) { string_list_append(&only_match_skip_worktree, pathspec->items[i].original); } else { diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index dee20db5bb..ddc86bb415 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -339,11 +339,7 @@ test_expect_success 'status/add: outside sparse cone' ' # Adding the path outside of the sparse-checkout cone should fail. test_sparse_match test_must_fail git add folder1/a && - - test_must_fail git -C sparse-checkout add --refresh folder1/a 2>sparse-checkout-err && - test_must_fail git -C sparse-index add --refresh folder1/a 2>sparse-index-err && - # NEEDSWORK: A sparse index changes the error message. - ! test_cmp sparse-checkout-err sparse-index-err && + test_sparse_match test_must_fail git add --refresh folder1/a && # NEEDSWORK: Adding a newly-tracked file outside the cone succeeds test_sparse_match git add folder1/new && From 42f8ed6ca24854141c34aa82472ca126fdaeaf65 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Thu, 29 Jul 2021 14:52:07 +0000 Subject: [PATCH 5/5] add: remove ensure_full_index() with --renormalize The --renormalize option updates the EOL conversions for the tracked files. However, the loop already ignores files marked with the SKIP_WORKTREE bit, so it will continue to do so with a sparse index because the sparse directory entries also have this bit set. Reviewed-by: Elijah Newren Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- builtin/add.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/builtin/add.c b/builtin/add.c index d512ece655..c49e179abc 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -144,8 +144,6 @@ static int renormalize_tracked_files(const struct pathspec *pathspec, int flags) { int i, retval = 0; - /* TODO: audit for interaction with sparse-index. */ - ensure_full_index(&the_index); for (i = 0; i < active_nr; i++) { struct cache_entry *ce = active_cache[i];