From 5506683deaab4c1475800e9b4cd2c06b8de4de97 Mon Sep 17 00:00:00 2001 From: Shaoxuan Yuan Date: Tue, 9 Aug 2022 20:09:02 +0800 Subject: [PATCH 01/10] t7002: add tests for moving from in-cone to out-of-cone Add corresponding tests to test that user can move an in-cone to out-of-cone when --sparse is supplied. Such can be either clean or dirty, and moving it results in different behaviors: A clean move should move to in the index (do *not* create in the worktree), then delete from the worktree. A dirty move should move the to the , both in the working tree and the index, but should *not* remove the resulted path from the working tree and should *not* turn on its CE_SKIP_WORKTREE bit. Also make sure that if exists in the index (existing check for if is in the worktree is not enough in in-to-out moves), warn user against the overwrite. And Git should force the overwrite when supplied with -f or --force. Helped-by: Derrick Stolee Helped-by: Victoria Dye Signed-off-by: Shaoxuan Yuan Signed-off-by: Junio C Hamano --- t/t7002-mv-sparse-checkout.sh | 198 ++++++++++++++++++++++++++++++++++ 1 file changed, 198 insertions(+) diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh index 71fe29690f..1ac78edde6 100755 --- a/t/t7002-mv-sparse-checkout.sh +++ b/t/t7002-mv-sparse-checkout.sh @@ -290,4 +290,202 @@ test_expect_success 'move sparse file to existing destination with --force and - test_cmp expect sub/file1 ' +test_expect_failure 'move clean path from in-cone to out-of-cone' ' + test_when_finished "cleanup_sparse_checkout" && + setup_sparse_checkout && + + test_must_fail git mv sub/d folder1 2>stderr && + cat sparse_error_header >expect && + echo "folder1/d" >>expect && + cat sparse_hint >>expect && + test_cmp expect stderr && + + git mv --sparse sub/d folder1 2>stderr && + test_must_be_empty stderr && + + test_path_is_missing sub/d && + test_path_is_missing folder1/d && + git ls-files -t >actual && + ! grep "^H sub/d\$" actual && + grep "S folder1/d" actual +' + +test_expect_failure 'move clean path from in-cone to out-of-cone overwrite' ' + test_when_finished "cleanup_sparse_checkout" && + setup_sparse_checkout && + echo "sub/file1 overwrite" >sub/file1 && + git add sub/file1 && + + test_must_fail git mv sub/file1 folder1 2>stderr && + cat sparse_error_header >expect && + echo "folder1/file1" >>expect && + cat sparse_hint >>expect && + test_cmp expect stderr && + + test_must_fail git mv --sparse sub/file1 folder1 2>stderr && + echo "fatal: destination exists in the index, source=sub/file1, destination=folder1/file1" \ + >expect && + test_cmp expect stderr && + + git mv --sparse -f sub/file1 folder1 2>stderr && + test_must_be_empty stderr && + + test_path_is_missing sub/file1 && + test_path_is_missing folder1/file1 && + git ls-files -t >actual && + ! grep "H sub/file1" actual && + grep "S folder1/file1" actual && + + # compare file content before move and after move + echo "sub/file1 overwrite" >expect && + git ls-files -s -- folder1/file1 | awk "{print \$2}" >oid && + git cat-file blob $(cat oid) >actual && + test_cmp expect actual +' + +# This test is testing the same behavior as the +# "move clean path from in-cone to out-of-cone overwrite" above. +# The only difference is the changes from "folder1" to "folder1/file1" +test_expect_failure 'move clean path from in-cone to out-of-cone file overwrite' ' + test_when_finished "cleanup_sparse_checkout" && + setup_sparse_checkout && + echo "sub/file1 overwrite" >sub/file1 && + git add sub/file1 && + + test_must_fail git mv sub/file1 folder1/file1 2>stderr && + cat sparse_error_header >expect && + echo "folder1/file1" >>expect && + cat sparse_hint >>expect && + test_cmp expect stderr && + + test_must_fail git mv --sparse sub/file1 folder1/file1 2>stderr && + echo "fatal: destination exists in the index, source=sub/file1, destination=folder1/file1" \ + >expect && + test_cmp expect stderr && + + git mv --sparse -f sub/file1 folder1/file1 2>stderr && + test_must_be_empty stderr && + + test_path_is_missing sub/file1 && + test_path_is_missing folder1/file1 && + git ls-files -t >actual && + ! grep "H sub/file1" actual && + grep "S folder1/file1" actual && + + # compare file content before move and after move + echo "sub/file1 overwrite" >expect && + git ls-files -s -- folder1/file1 | awk "{print \$2}" >oid && + git cat-file blob $(cat oid) >actual && + test_cmp expect actual +' + +test_expect_failure 'move directory with one of the files overwrite' ' + test_when_finished "cleanup_sparse_checkout" && + mkdir -p folder1/dir && + touch folder1/dir/file1 && + git add folder1 && + git sparse-checkout set --cone sub && + + echo test >sub/dir/file1 && + git add sub/dir/file1 && + + test_must_fail git mv sub/dir folder1 2>stderr && + cat sparse_error_header >expect && + echo "folder1/dir/e" >>expect && + echo "folder1/dir/file1" >>expect && + cat sparse_hint >>expect && + test_cmp expect stderr && + + test_must_fail git mv --sparse sub/dir folder1 2>stderr && + echo "fatal: destination exists in the index, source=sub/dir/file1, destination=folder1/dir/file1" \ + >expect && + test_cmp expect stderr && + + git mv --sparse -f sub/dir folder1 2>stderr && + test_must_be_empty stderr && + + test_path_is_missing sub/dir/file1 && + test_path_is_missing sub/dir/e && + test_path_is_missing folder1/file1 && + git ls-files -t >actual && + ! grep "H sub/dir/file1" actual && + ! grep "H sub/dir/e" actual && + grep "S folder1/dir/file1" actual && + + # compare file content before move and after move + echo test >expect && + git ls-files -s -- folder1/dir/file1 | awk "{print \$2}" >oid && + git cat-file blob $(cat oid) >actual && + test_cmp expect actual +' + +test_expect_failure 'move dirty path from in-cone to out-of-cone' ' + test_when_finished "cleanup_sparse_checkout" && + setup_sparse_checkout && + echo "modified" >>sub/d && + + test_must_fail git mv sub/d folder1 2>stderr && + cat sparse_error_header >expect && + echo "folder1/d" >>expect && + cat sparse_hint >>expect && + test_cmp expect stderr && + + git mv --sparse sub/d folder1 2>stderr && + + test_path_is_missing sub/d && + test_path_is_file folder1/d && + git ls-files -t >actual && + ! grep "^H sub/d\$" actual && + grep "H folder1/d" actual +' + +test_expect_failure 'move dir from in-cone to out-of-cone' ' + test_when_finished "cleanup_sparse_checkout" && + setup_sparse_checkout && + + test_must_fail git mv sub/dir folder1 2>stderr && + cat sparse_error_header >expect && + echo "folder1/dir/e" >>expect && + cat sparse_hint >>expect && + test_cmp expect stderr && + + git mv --sparse sub/dir folder1 2>stderr && + test_must_be_empty stderr && + + test_path_is_missing folder1 && + git ls-files -t >actual && + ! grep "H sub/dir/e" actual && + grep "S folder1/dir/e" actual +' + +test_expect_failure 'move partially-dirty dir from in-cone to out-of-cone' ' + test_when_finished "cleanup_sparse_checkout" && + setup_sparse_checkout && + touch sub/dir/e2 sub/dir/e3 && + git add sub/dir/e2 sub/dir/e3 && + echo "modified" >>sub/dir/e2 && + echo "modified" >>sub/dir/e3 && + + test_must_fail git mv sub/dir folder1 2>stderr && + cat sparse_error_header >expect && + echo "folder1/dir/e" >>expect && + echo "folder1/dir/e2" >>expect && + echo "folder1/dir/e3" >>expect && + cat sparse_hint >>expect && + test_cmp expect stderr && + + git mv --sparse sub/dir folder1 2>stderr && + + test_path_is_missing folder1/dir/e && + test_path_is_file folder1/dir/e2 && + test_path_is_file folder1/dir/e3 && + git ls-files -t >actual && + ! grep "H sub/dir/e" actual && + ! grep "H sub/dir/e2" actual && + ! grep "H sub/dir/e3" actual && + grep "S folder1/dir/e" actual && + grep "H folder1/dir/e2" actual && + grep "H folder1/dir/e3" actual +' + test_done From 72e59ba19e776abbff7dccbf093c73e43527a339 Mon Sep 17 00:00:00 2001 From: Shaoxuan Yuan Date: Tue, 9 Aug 2022 20:09:03 +0800 Subject: [PATCH 02/10] mv: rename check_dir_in_index() to empty_dir_has_sparse_contents() Method check_dir_in_index() introduced in b91a2b6594 (mv: add check_dir_in_index() and solve general dir check issue, 2022-06-30) does not describe its intent and behavior well. Change its name to empty_dir_has_sparse_contents(), which more precisely describes its purpose. Reverse the return values, check_dir_in_index() return 0 for success and 1 for failure; reverse the values so empty_dir_has_sparse_contents() return 1 for success and 0 for failure. These values are more intuitive because 1 usually means "has" and 0 means "not found". Also modify the documentation to better align with the method's intent and behavior. Helped-by: Derrick Stolee Helped-by: Victoria Dye Signed-off-by: Shaoxuan Yuan Signed-off-by: Junio C Hamano --- builtin/mv.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/builtin/mv.c b/builtin/mv.c index 2a38e2af46..969f464f7d 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -125,15 +125,13 @@ static int index_range_of_same_dir(const char *src, int length, } /* - * Check if an out-of-cone directory should be in the index. Imagine this case - * that all the files under a directory are marked with 'CE_SKIP_WORKTREE' bit - * and thus the directory is sparsified. - * - * Return 0 if such directory exist (i.e. with any of its contained files not - * marked with CE_SKIP_WORKTREE, the directory would be present in working tree). - * Return 1 otherwise. + * Given the path of a directory that does not exist on-disk, check whether the + * directory contains any entries in the index with the SKIP_WORKTREE flag + * enabled. + * Return 1 if such index entries exist. + * Return 0 otherwise. */ -static int check_dir_in_index(const char *name) +static int empty_dir_has_sparse_contents(const char *name) { const char *with_slash = add_slash(name); int length = strlen(with_slash); @@ -144,14 +142,14 @@ static int check_dir_in_index(const char *name) if (pos < 0) { pos = -pos - 1; if (pos >= the_index.cache_nr) - return 1; + return 0; ce = active_cache[pos]; if (strncmp(with_slash, ce->name, length)) - return 1; - if (ce_skip_worktree(ce)) return 0; + if (ce_skip_worktree(ce)) + return 1; } - return 1; + return 0; } int cmd_mv(int argc, const char **argv, const char *prefix) @@ -231,7 +229,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix) if (pos < 0) { const char *src_w_slash = add_slash(src); if (!path_in_sparse_checkout(src_w_slash, &the_index) && - !check_dir_in_index(src)) { + empty_dir_has_sparse_contents(src)) { modes[i] |= SKIP_WORKTREE_DIR; goto dir_check; } From d57690a9c82c8888be6bb8ae17be231a2b2802e6 Mon Sep 17 00:00:00 2001 From: Shaoxuan Yuan Date: Tue, 9 Aug 2022 20:09:04 +0800 Subject: [PATCH 03/10] mv: free the with_slash in check_dir_in_index() with_slash may be a malloc'd pointer, and when it is, free it. Helped-by: Derrick Stolee Helped-by: Victoria Dye Signed-off-by: Shaoxuan Yuan Signed-off-by: Junio C Hamano --- builtin/mv.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/builtin/mv.c b/builtin/mv.c index 969f464f7d..c17df2a12f 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -133,6 +133,7 @@ static int index_range_of_same_dir(const char *src, int length, */ static int empty_dir_has_sparse_contents(const char *name) { + int ret = 0; const char *with_slash = add_slash(name); int length = strlen(with_slash); @@ -142,14 +143,18 @@ static int empty_dir_has_sparse_contents(const char *name) if (pos < 0) { pos = -pos - 1; if (pos >= the_index.cache_nr) - return 0; + goto free_return; ce = active_cache[pos]; if (strncmp(with_slash, ce->name, length)) - return 0; + goto free_return; if (ce_skip_worktree(ce)) - return 1; + ret = 1; } - return 0; + +free_return: + if (with_slash != name) + free((char *)with_slash); + return ret; } int cmd_mv(int argc, const char **argv, const char *prefix) From c08830de41f15a8ee85cf7926266e1db732ec773 Mon Sep 17 00:00:00 2001 From: Shaoxuan Yuan Date: Tue, 9 Aug 2022 20:09:05 +0800 Subject: [PATCH 04/10] mv: check if is a SKIP_WORKTREE_DIR Originally, is assumed to be in the working tree. If it is not found as a directory, then it is determined to be either a regular file path, or error out if used under the second form (move into a directory) of 'git-mv'. Such behavior is not ideal, mainly because Git does not look into the index for , which could potentially be a SKIP_WORKTREE_DIR, which we need to determine for the later "moving from in-cone to out-of-cone" patch. Change the logic so that Git first check if is a directory with all its contents sparsified (a SKIP_WORKTREE_DIR). If is such a sparse directory, then we should modify the index the same way as we would if this were a non-sparse directory. We must be careful to ensure that the is marked with SKIP_WORKTREE_DIR. Also add a `dst_w_slash` to reuse the result from `add_slash()`, which was everywhere and can be simplified. Helped-by: Derrick Stolee Helped-by: Victoria Dye Signed-off-by: Shaoxuan Yuan Signed-off-by: Junio C Hamano --- builtin/mv.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/builtin/mv.c b/builtin/mv.c index c17df2a12f..11aea7b4db 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -171,6 +171,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix) OPT_END(), }; const char **source, **destination, **dest_path, **submodule_gitfile; + const char *dst_w_slash; enum update_mode *modes; struct stat st; struct string_list src_for_dst = STRING_LIST_INIT_NODUP; @@ -200,6 +201,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix) if (argc == 1 && is_directory(argv[0]) && !is_directory(argv[1])) flags = 0; dest_path = internal_prefix_pathspec(prefix, argv + argc, 1, flags); + dst_w_slash = add_slash(dest_path[0]); submodule_gitfile = xcalloc(argc, sizeof(char *)); if (dest_path[0][0] == '\0') @@ -207,12 +209,20 @@ int cmd_mv(int argc, const char **argv, const char *prefix) destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME); else if (!lstat(dest_path[0], &st) && S_ISDIR(st.st_mode)) { - dest_path[0] = add_slash(dest_path[0]); - destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME); + destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME); } else { - if (argc != 1) + if (!path_in_sparse_checkout(dst_w_slash, &the_index) && + empty_dir_has_sparse_contents(dst_w_slash)) { + destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME); + } else if (argc != 1) { die(_("destination '%s' is not a directory"), dest_path[0]); - destination = dest_path; + } else { + destination = dest_path; + } + } + if (dst_w_slash != dest_path[0]) { + free((char *)dst_w_slash); + dst_w_slash = NULL; } /* Checking */ From 9284c3ce266fcf9abb0afbc59645c62dd58026dd Mon Sep 17 00:00:00 2001 From: Shaoxuan Yuan Date: Tue, 9 Aug 2022 20:09:06 +0800 Subject: [PATCH 05/10] mv: remove BOTH from enum update_mode Since BOTH is not used anywhere in the code and its meaning is unclear, remove it. Helped-by: Derrick Stolee Helped-by: Victoria Dye Signed-off-by: Shaoxuan Yuan Signed-off-by: Junio C Hamano --- builtin/mv.c | 1 - 1 file changed, 1 deletion(-) diff --git a/builtin/mv.c b/builtin/mv.c index 11aea7b4db..7ac653be23 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -21,7 +21,6 @@ static const char * const builtin_mv_usage[] = { }; enum update_mode { - BOTH = 0, WORKING_DIRECTORY = (1 << 1), INDEX = (1 << 2), SPARSE = (1 << 3), From 5784db1b22feceafc72454dccd9637d19fdd422c Mon Sep 17 00:00:00 2001 From: Shaoxuan Yuan Date: Tue, 9 Aug 2022 20:09:07 +0800 Subject: [PATCH 06/10] mv: from in-cone to out-of-cone Originally, moving an in-cone to an out-of-cone was not possible, mainly because such is a directory that is not present in the working tree. Change the behavior so that we can move an in-cone to out-of-cone when --sparse is supplied. Notice that can also be an out-of-cone file path, rather than a directory. Such can be either clean or dirty, and moving it results in different behaviors: A clean move should move to in the index (do *not* create in the worktree), then delete from the worktree. A dirty move should move the to the , both in the working tree and the index, but should *not* remove the resulted path from the working tree and should *not* turn on its CE_SKIP_WORKTREE bit. Optional reading ================ We are strict about cone mode when is a file path. The reason is that some of the previous tests that use no-cone mode in t7002 are keep breaking, mainly because the `dst_mode = SPARSE;` line added in this patch. Most features developed in both "from-out-to-in" and "from-in-to-out" only care about cone mode situation, as no-cone mode is becoming irrelevant. And because assigning `SPARSE` to `dst_mode` when the repo is in no-cone mode causes miscellaneous bugs, we should just leave this new functionality to be exclusive cone mode and save some time. Helped-by: Derrick Stolee Helped-by: Victoria Dye Signed-off-by: Shaoxuan Yuan Signed-off-by: Junio C Hamano --- builtin/mv.c | 69 +++++++++++++++++++++++++++++++---- t/t7002-mv-sparse-checkout.sh | 8 ++-- 2 files changed, 65 insertions(+), 12 deletions(-) diff --git a/builtin/mv.c b/builtin/mv.c index 7ac653be23..b64c28bd5b 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -171,12 +171,13 @@ int cmd_mv(int argc, const char **argv, const char *prefix) }; const char **source, **destination, **dest_path, **submodule_gitfile; const char *dst_w_slash; - enum update_mode *modes; + enum update_mode *modes, dst_mode = 0; struct stat st; struct string_list src_for_dst = STRING_LIST_INIT_NODUP; struct lock_file lock_file = LOCK_INIT; struct cache_entry *ce; struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP; + struct string_list dirty_paths = STRING_LIST_INIT_NODUP; git_config(git_default_config, NULL); @@ -213,10 +214,21 @@ int cmd_mv(int argc, const char **argv, const char *prefix) if (!path_in_sparse_checkout(dst_w_slash, &the_index) && empty_dir_has_sparse_contents(dst_w_slash)) { destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME); + dst_mode = SKIP_WORKTREE_DIR; } else if (argc != 1) { die(_("destination '%s' is not a directory"), dest_path[0]); } else { destination = dest_path; + /* + * is a file outside of sparse-checkout + * cone. Insist on cone mode here for backward + * compatibility. We don't want dst_mode to be assigned + * for a file when the repo is using no-cone mode (which + * is deprecated at this point) sparse-checkout. As + * SPARSE here is only considering cone-mode situation. + */ + if (!path_in_cone_mode_sparse_checkout(destination[0], &the_index)) + dst_mode = SPARSE; } } if (dst_w_slash != dest_path[0]) { @@ -410,6 +422,7 @@ remove_entry: const char *src = source[i], *dst = destination[i]; enum update_mode mode = modes[i]; int pos; + int sparse_and_dirty = 0; struct checkout state = CHECKOUT_INIT; state.istate = &the_index; @@ -420,6 +433,7 @@ remove_entry: if (show_only) continue; if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) && + !(dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) && rename(src, dst) < 0) { if (ignore_errors) continue; @@ -439,17 +453,55 @@ remove_entry: pos = cache_name_pos(src, strlen(src)); assert(pos >= 0); + if (!(mode & SPARSE) && !lstat(src, &st)) + sparse_and_dirty = ce_modified(active_cache[pos], &st, 0); rename_cache_entry_at(pos, dst); - if ((mode & SPARSE) && - (path_in_sparse_checkout(dst, &the_index))) { - int dst_pos; + if (ignore_sparse && + core_apply_sparse_checkout && + core_sparse_checkout_cone) { + /* + * NEEDSWORK: we are *not* paying attention to + * "out-to-out" move ( is out-of-cone and + * is out-of-cone) at this point. It + * should be added in a future patch. + */ + if ((mode & SPARSE) && + path_in_sparse_checkout(dst, &the_index)) { + /* from out-of-cone to in-cone */ + int dst_pos = cache_name_pos(dst, strlen(dst)); + struct cache_entry *dst_ce = active_cache[dst_pos]; - dst_pos = cache_name_pos(dst, strlen(dst)); - active_cache[dst_pos]->ce_flags &= ~CE_SKIP_WORKTREE; + dst_ce->ce_flags &= ~CE_SKIP_WORKTREE; - if (checkout_entry(active_cache[dst_pos], &state, NULL, NULL)) - die(_("cannot checkout %s"), active_cache[dst_pos]->name); + if (checkout_entry(dst_ce, &state, NULL, NULL)) + die(_("cannot checkout %s"), dst_ce->name); + } else if ((dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) && + !(mode & SPARSE) && + !path_in_sparse_checkout(dst, &the_index)) { + /* from in-cone to out-of-cone */ + int dst_pos = cache_name_pos(dst, strlen(dst)); + struct cache_entry *dst_ce = active_cache[dst_pos]; + + /* + * if src is clean, it will suffice to remove it + */ + if (!sparse_and_dirty) { + dst_ce->ce_flags |= CE_SKIP_WORKTREE; + unlink_or_warn(src); + } else { + /* + * if src is dirty, move it to the + * destination and create leading + * dirs if necessary + */ + char *dst_dup = xstrdup(dst); + string_list_append(&dirty_paths, dst); + safe_create_leading_directories(dst_dup); + FREE_AND_NULL(dst_dup); + rename(src, dst); + } + } } } @@ -461,6 +513,7 @@ remove_entry: die(_("Unable to write new index file")); string_list_clear(&src_for_dst, 0); + string_list_clear(&dirty_paths, 0); UNLEAK(source); UNLEAK(dest_path); free(submodule_gitfile); diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh index 1ac78edde6..d875f492dd 100755 --- a/t/t7002-mv-sparse-checkout.sh +++ b/t/t7002-mv-sparse-checkout.sh @@ -290,7 +290,7 @@ test_expect_success 'move sparse file to existing destination with --force and - test_cmp expect sub/file1 ' -test_expect_failure 'move clean path from in-cone to out-of-cone' ' +test_expect_success 'move clean path from in-cone to out-of-cone' ' test_when_finished "cleanup_sparse_checkout" && setup_sparse_checkout && @@ -419,7 +419,7 @@ test_expect_failure 'move directory with one of the files overwrite' ' test_cmp expect actual ' -test_expect_failure 'move dirty path from in-cone to out-of-cone' ' +test_expect_success 'move dirty path from in-cone to out-of-cone' ' test_when_finished "cleanup_sparse_checkout" && setup_sparse_checkout && echo "modified" >>sub/d && @@ -439,7 +439,7 @@ test_expect_failure 'move dirty path from in-cone to out-of-cone' ' grep "H folder1/d" actual ' -test_expect_failure 'move dir from in-cone to out-of-cone' ' +test_expect_success 'move dir from in-cone to out-of-cone' ' test_when_finished "cleanup_sparse_checkout" && setup_sparse_checkout && @@ -458,7 +458,7 @@ test_expect_failure 'move dir from in-cone to out-of-cone' ' grep "S folder1/dir/e" actual ' -test_expect_failure 'move partially-dirty dir from in-cone to out-of-cone' ' +test_expect_success 'move partially-dirty dir from in-cone to out-of-cone' ' test_when_finished "cleanup_sparse_checkout" && setup_sparse_checkout && touch sub/dir/e2 sub/dir/e3 && From b6f51e3db978ae2a72c290a10bd205f9e1d6818e Mon Sep 17 00:00:00 2001 From: Shaoxuan Yuan Date: Tue, 9 Aug 2022 20:09:08 +0800 Subject: [PATCH 07/10] mv: cleanup empty WORKING_DIRECTORY Originally, moving from-in-to-out may leave an empty directory on-disk (this kind of directory is marked as WORKING_DIRECTORY). Cleanup such directories if they are empty (don't have any entries under them). Modify two tests that take as WORKING_DIRECTORY to test this behavior. Suggested-by: Derrick Stolee Signed-off-by: Shaoxuan Yuan Signed-off-by: Junio C Hamano --- builtin/mv.c | 27 +++++++++++++++++++++++++++ t/t7002-mv-sparse-checkout.sh | 4 ++++ 2 files changed, 31 insertions(+) diff --git a/builtin/mv.c b/builtin/mv.c index b64c28bd5b..f4961c0ffd 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -171,6 +171,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix) }; const char **source, **destination, **dest_path, **submodule_gitfile; const char *dst_w_slash; + const char **src_dir = NULL; + int src_dir_nr = 0, src_dir_alloc = 0; + struct strbuf a_src_dir = STRBUF_INIT; enum update_mode *modes, dst_mode = 0; struct stat st; struct string_list src_for_dst = STRING_LIST_INIT_NODUP; @@ -313,6 +316,10 @@ dir_check: /* last - first >= 1 */ modes[i] |= WORKING_DIRECTORY; + + ALLOC_GROW(src_dir, src_dir_nr + 1, src_dir_alloc); + src_dir[src_dir_nr++] = src; + n = argc + last - first; REALLOC_ARRAY(source, n); REALLOC_ARRAY(destination, n); @@ -505,6 +512,26 @@ remove_entry: } } + /* + * cleanup the empty src_dirs + */ + for (i = 0; i < src_dir_nr; i++) { + int dummy; + strbuf_addstr(&a_src_dir, src_dir[i]); + /* + * if entries under a_src_dir are all moved away, + * recursively remove a_src_dir to cleanup + */ + if (index_range_of_same_dir(a_src_dir.buf, a_src_dir.len, + &dummy, &dummy) < 1) { + remove_dir_recursively(&a_src_dir, 0); + } + strbuf_reset(&a_src_dir); + } + + strbuf_release(&a_src_dir); + free(src_dir); + if (gitmodules_modified) stage_updated_gitmodules(&the_index); diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh index d875f492dd..df8c0fa572 100755 --- a/t/t7002-mv-sparse-checkout.sh +++ b/t/t7002-mv-sparse-checkout.sh @@ -442,6 +442,7 @@ test_expect_success 'move dirty path from in-cone to out-of-cone' ' test_expect_success 'move dir from in-cone to out-of-cone' ' test_when_finished "cleanup_sparse_checkout" && setup_sparse_checkout && + mkdir sub/dir/deep && test_must_fail git mv sub/dir folder1 2>stderr && cat sparse_error_header >expect && @@ -452,6 +453,7 @@ test_expect_success 'move dir from in-cone to out-of-cone' ' git mv --sparse sub/dir folder1 2>stderr && test_must_be_empty stderr && + test_path_is_missing sub/dir && test_path_is_missing folder1 && git ls-files -t >actual && ! grep "H sub/dir/e" actual && @@ -461,6 +463,7 @@ test_expect_success 'move dir from in-cone to out-of-cone' ' test_expect_success 'move partially-dirty dir from in-cone to out-of-cone' ' test_when_finished "cleanup_sparse_checkout" && setup_sparse_checkout && + mkdir sub/dir/deep && touch sub/dir/e2 sub/dir/e3 && git add sub/dir/e2 sub/dir/e3 && echo "modified" >>sub/dir/e2 && @@ -476,6 +479,7 @@ test_expect_success 'move partially-dirty dir from in-cone to out-of-cone' ' git mv --sparse sub/dir folder1 2>stderr && + test_path_is_missing sub/dir && test_path_is_missing folder1/dir/e && test_path_is_file folder1/dir/e2 && test_path_is_file folder1/dir/e3 && From 5efd533ed8896592740afe22ac07271497d6db36 Mon Sep 17 00:00:00 2001 From: Shaoxuan Yuan Date: Tue, 9 Aug 2022 20:09:09 +0800 Subject: [PATCH 08/10] advice.h: add advise_on_moving_dirty_path() Add an advice. When the user use `git mv --sparse `, Git will warn the user to use `git add --sparse ` then use `git sparse-checkout reapply` to apply the sparsity rules. Add a few lines to previous "move dirty path" tests so we can test this new advice is working. Suggested-by: Derrick Stolee Signed-off-by: Shaoxuan Yuan Signed-off-by: Junio C Hamano --- advice.c | 19 +++++++++++++++++++ advice.h | 1 + builtin/mv.c | 3 +++ t/t7002-mv-sparse-checkout.sh | 24 +++++++++++++++++++++++- 4 files changed, 46 insertions(+), 1 deletion(-) diff --git a/advice.c b/advice.c index 6fda9edbc2..fd18968943 100644 --- a/advice.c +++ b/advice.c @@ -261,3 +261,22 @@ void detach_advice(const char *new_name) fprintf(stderr, fmt, new_name); } + +void advise_on_moving_dirty_path(struct string_list *pathspec_list) +{ + struct string_list_item *item; + + if (!pathspec_list->nr) + return; + + fprintf(stderr, _("The following paths have been moved outside the\n" + "sparse-checkout definition but are not sparse due to local\n" + "modifications.\n")); + for_each_string_list_item(item, pathspec_list) + fprintf(stderr, "%s\n", item->string); + + advise_if_enabled(ADVICE_UPDATE_SPARSE_PATH, + _("To correct the sparsity of these paths, do the following:\n" + "* Use \"git add --sparse \" to update the index\n" + "* Use \"git sparse-checkout reapply\" to apply the sparsity rules")); +} diff --git a/advice.h b/advice.h index 7ddc6cbc1a..07e0f76833 100644 --- a/advice.h +++ b/advice.h @@ -74,5 +74,6 @@ void NORETURN die_conclude_merge(void); void NORETURN die_ff_impossible(void); void advise_on_updating_sparse_paths(struct string_list *pathspec_list); void detach_advice(const char *new_name); +void advise_on_moving_dirty_path(struct string_list *pathspec_list); #endif /* ADVICE_H */ diff --git a/builtin/mv.c b/builtin/mv.c index f4961c0ffd..d80adf8de5 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -532,6 +532,9 @@ remove_entry: strbuf_release(&a_src_dir); free(src_dir); + if (dirty_paths.nr) + advise_on_moving_dirty_path(&dirty_paths); + if (gitmodules_modified) stage_updated_gitmodules(&the_index); diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh index df8c0fa572..5e5eb70e7a 100755 --- a/t/t7002-mv-sparse-checkout.sh +++ b/t/t7002-mv-sparse-checkout.sh @@ -28,12 +28,25 @@ test_expect_success 'setup' " updated in the index: EOF - cat >sparse_hint <<-EOF + cat >sparse_hint <<-EOF && hint: If you intend to update such entries, try one of the following: hint: * Use the --sparse option. hint: * Disable or modify the sparsity rules. hint: Disable this message with \"git config advice.updateSparsePath false\" EOF + + cat >dirty_error_header <<-EOF && + The following paths have been moved outside the + sparse-checkout definition but are not sparse due to local + modifications. + EOF + + cat >dirty_hint <<-EOF + hint: To correct the sparsity of these paths, do the following: + hint: * Use \"git add --sparse \" to update the index + hint: * Use \"git sparse-checkout reapply\" to apply the sparsity rules + hint: Disable this message with \"git config advice.updateSparsePath false\" + EOF " test_expect_success 'mv refuses to move sparse-to-sparse' ' @@ -431,6 +444,10 @@ test_expect_success 'move dirty path from in-cone to out-of-cone' ' test_cmp expect stderr && git mv --sparse sub/d folder1 2>stderr && + cat dirty_error_header >expect && + echo "folder1/d" >>expect && + cat dirty_hint >>expect && + test_cmp expect stderr && test_path_is_missing sub/d && test_path_is_file folder1/d && @@ -478,6 +495,11 @@ test_expect_success 'move partially-dirty dir from in-cone to out-of-cone' ' test_cmp expect stderr && git mv --sparse sub/dir folder1 2>stderr && + cat dirty_error_header >expect && + echo "folder1/dir/e2" >>expect && + echo "folder1/dir/e3" >>expect && + cat dirty_hint >>expect && + test_cmp expect stderr && test_path_is_missing sub/dir && test_path_is_missing folder1/dir/e && From da6fe05b3d624ad5b40472eebfe0499c15ecc93d Mon Sep 17 00:00:00 2001 From: Shaoxuan Yuan Date: Tue, 9 Aug 2022 20:09:10 +0800 Subject: [PATCH 09/10] mv: check overwrite for in-to-out move Add checking logic for overwriting when moving from in-cone to out-of-cone. It is the index version of the original overwrite logic. Helped-by: Derrick Stolee Signed-off-by: Shaoxuan Yuan Signed-off-by: Junio C Hamano --- builtin/mv.c | 12 ++++++++++++ t/t7002-mv-sparse-checkout.sh | 6 +++--- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/builtin/mv.c b/builtin/mv.c index d80adf8de5..4b67bd096a 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -376,6 +376,18 @@ dir_check: goto act_on_entry; } + if (ignore_sparse && + (dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) && + index_entry_exists(&the_index, dst, strlen(dst))) { + bad = _("destination exists in the index"); + if (force) { + if (verbose) + warning(_("overwriting '%s'"), dst); + bad = NULL; + } else { + goto act_on_entry; + } + } /* * We check if the paths are in the sparse-checkout * definition as a very final check, since that diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh index 5e5eb70e7a..26582ae4e5 100755 --- a/t/t7002-mv-sparse-checkout.sh +++ b/t/t7002-mv-sparse-checkout.sh @@ -323,7 +323,7 @@ test_expect_success 'move clean path from in-cone to out-of-cone' ' grep "S folder1/d" actual ' -test_expect_failure 'move clean path from in-cone to out-of-cone overwrite' ' +test_expect_success 'move clean path from in-cone to out-of-cone overwrite' ' test_when_finished "cleanup_sparse_checkout" && setup_sparse_checkout && echo "sub/file1 overwrite" >sub/file1 && @@ -359,7 +359,7 @@ test_expect_failure 'move clean path from in-cone to out-of-cone overwrite' ' # This test is testing the same behavior as the # "move clean path from in-cone to out-of-cone overwrite" above. # The only difference is the changes from "folder1" to "folder1/file1" -test_expect_failure 'move clean path from in-cone to out-of-cone file overwrite' ' +test_expect_success 'move clean path from in-cone to out-of-cone file overwrite' ' test_when_finished "cleanup_sparse_checkout" && setup_sparse_checkout && echo "sub/file1 overwrite" >sub/file1 && @@ -392,7 +392,7 @@ test_expect_failure 'move clean path from in-cone to out-of-cone file overwrite' test_cmp expect actual ' -test_expect_failure 'move directory with one of the files overwrite' ' +test_expect_success 'move directory with one of the files overwrite' ' test_when_finished "cleanup_sparse_checkout" && mkdir -p folder1/dir && touch folder1/dir/file1 && From 7ead46810b507828c1481eaea6d64b9ed635b8b7 Mon Sep 17 00:00:00 2001 From: Shaoxuan Yuan Date: Fri, 9 Sep 2022 15:27:36 -0700 Subject: [PATCH 10/10] builtin/mv.c: fix possible segfault in add_slash() A possible segfault was introduced in c08830de41 (mv: check if is a SKIP_WORKTREE_DIR, 2022-08-09). When running t7001 with SANITIZE=address, problem appears when running: git mv path1/path2/ . or git mv directory ../ or any that makes dest_path[0] an empty string. The add_slash() call could segfault when path argument to it is an empty string, because it makes an out-of-bounds read to decide if an extra slash '/' needs to be appended to it. As add_slash() is used to make sure that a valid pathname to a file in the given directory can be made by appending a filename after the value returned from it, if path is an empty string, we want to return it as-is. The path to a file "F" in the top-level of the working tree (i.e. path=="") is formed by appending "F" after "" (i.e. path) without any slash in between. So, just like the case where a non-empty path already ends with a slash, return an empty path as-is. Reported-by: Jeff King Helped-by: Jeff King Helped-by: Junio C Hamano Helped-by: Derrick Stolee Signed-off-by: Shaoxuan Yuan Signed-off-by: Junio C Hamano --- builtin/mv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/mv.c b/builtin/mv.c index 4b67bd096a..35bf1e9c71 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -71,7 +71,7 @@ static const char **internal_prefix_pathspec(const char *prefix, static const char *add_slash(const char *path) { size_t len = strlen(path); - if (path[len - 1] != '/') { + if (len && path[len - 1] != '/') { char *with_slash = xmalloc(st_add(len, 2)); memcpy(with_slash, path, len); with_slash[len++] = '/';