| Message ID | 20220809120910.2021413-1-shaoxuan.yuan02@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | mv: from in-cone to out-of-cone | expand |
Shaoxuan Yuan wrote: > ## Changes since PATCH v2 ## > > 1. Rephrase the descriptions for clean moves to match the actual > implementation. > > 2. Add test for moving to a file path <destination> with overwrite. > > 3. Add test for moving a directory as <source>, with one of its file > overwrites file entry in <destination>. > > 4. *with_slash -> with_slash, remove the dereference sign. > > 5. Take care of in-to-out where <destination> is a file path, rather > than a directory, this is what point 2 is testing. > > 6. Add a NEEDSWORK doc to address "out-to-out" moves. > > ## Changes since PATCH v1 ## > > 1. Change "grep -x" to ^ and $ in t7002 test, and remove some > useless "-x" (lines that are *not* ambiguous). > > 2. Rename check_dir_in_index() to empty_dir_has_sparse_contents() > and add more precise documentation. > > 3. Reverse return values from empty_dir_has_sparse_contents() to > comply with the method's name. > > 4. Make some commit messages more natural/fluent/without typos. > > 5. Split commit "mv: from in-cone to out-of-cone" to two commits, > one does in-to-out functionality, the other one does advice. > > 6. Take care a few memory leaks (`xstrdup`s). > > 7. Add a new way to recursively cleanup empty WORKING_DIRECTORY. > > ## leftover bits ## > > I decided *not* to solve these bits in this series but in a future > one, so things can be handled one at a time without exceeding my > capability. > > 1. When trying to move from-in-to-out, the mechanism assumes that > the <destination> is out-of-cone, and by cone-mode definition, > <destination> is basically an invisible directory > (SKIP_WORKTREE_DIR). However, the dirty move mechanism materializes > the moved file to make sure the information is not lost, and this > mechanism brings the invisible directory back into the worktree. > Hence, if we want to perform a second move from-in-to-out, the > assumption that <destination> is not on-disk is broken, and > everything follows breaks too. > > 2. A logic loophole introduced in the previous out-to-in patch, > especially in b91a2b6594 (mv: add check_dir_in_index() and solve > general dir check issue). Please detach your head to b91a2b6594 > for context. Copy and paste: > > git switch b91a2b6594 > > When moving from out-to-in, <source> is an invisible > SKIP_WORKTREE_DIR. Line 226 uses `lstat()` to make > sure the <source> is not on-disk; then line 233-237 checks if > <source> is a SKIP_WORKTREE_DIR. If the check passes, line 236 > jump to line 276 and try to verify <source> is a directory using > `S_ISDIR`. However, because the prerequisite is that the line 226 > `lstat()` fails so we can go through the steps said above; and when > it fails, the `st` stat, especially the `st_mode` member, is either > an uninitialized chunk of garbage, or the result from previous > `lstat()`. In this case, `st_mode` *is* from a previous `lstat()`, > on line 205. This `lstat()` is testing the <destination>, which, > under the out-to-in situation, is always an on-disk directory. > Thus, by a series of coincidence, the `S_ISDIR()` on line 276 > succeed and everything *looks* OK test-wise. But clearly the `st` > at this point is an impostor: it does not reflect the actual stat > situation of <source> and it luckily slips through. > > This needs to be fixed. > > 3. A NEEDSWORK added to address "out-to-out" move. > > ## PATCH v1 info ## > > This is a sister series to the previous "from out-of-cone to in-cone" [1] > series. This series is trying to make the opposite operation possible for > 'mv', namely move <source>, which is in-cone, to <destination>, which is > out-of-cone. > > Other than the main task, there are also some minor fixes done. > > [1] https://lore.kernel.org/git/20220331091755.385961-1-shaoxuan.yuan02@gmail.com/ > (Sorry for the slow response) The range-diff below looks good to me (w.r.t feedback on the last version). I do still find patch 4 [1] a bit cluttered, but it's a minor point and otherwise everything looks functional; I think this is probably ready for 'next'. Thanks! > Shaoxuan Yuan (9): > t7002: add tests for moving from in-cone to out-of-cone > mv: rename check_dir_in_index() to empty_dir_has_sparse_contents() > mv: free the with_slash in check_dir_in_index() > mv: check if <destination> is a SKIP_WORKTREE_DIR > mv: remove BOTH from enum update_mode > mv: from in-cone to out-of-cone > mv: cleanup empty WORKING_DIRECTORY > advice.h: add advise_on_moving_dirty_path() > mv: check overwrite for in-to-out move > > advice.c | 19 +++ > advice.h | 1 + > builtin/mv.c | 159 ++++++++++++++++++++---- > t/t7002-mv-sparse-checkout.sh | 226 +++++++++++++++++++++++++++++++++- > 4 files changed, 378 insertions(+), 27 deletions(-) > > Range-diff against v2: > 1: a8c360c083 ! 1: 8134fa5990 t7002: add tests for moving from in-cone to out-of-cone > @@ Commit message > Such <source> can be either clean or dirty, and moving it results in > different behaviors: > > - A clean move should move the <source> to the <destination>, both in the > - working tree and the index, then remove the resulted path from the > - working tree, and turn on its CE_SKIP_WORKTREE bit. > + A clean move should move <source> to <destination> in the index (do > + *not* create <destination> in the worktree), then delete <source> from > + the worktree. > > A dirty move should move the <source> to the <destination>, both in the > working tree and the index, but should *not* remove the resulted path > @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'move sparse file to existing > + 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 <destination> 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 && > 2: 725a83c575 = 2: cc5f2770de mv: rename check_dir_in_index() to empty_dir_has_sparse_contents() > 3: 1d4a0c16a6 ! 3: 1780f36825 mv: free the *with_slash in check_dir_in_index() > @@ Metadata > Author: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> > > ## Commit message ## > - mv: free the *with_slash in check_dir_in_index() > + mv: free the with_slash in check_dir_in_index() > > - *with_slash may be a malloc'd pointer, and when it is, free it. > + with_slash may be a malloc'd pointer, and when it is, free it. > > Helped-by: Derrick Stolee <derrickstolee@github.com> > Helped-by: Victoria Dye <vdye@github.com> > 4: 1f35f0eb34 = 4: d935bd8d7a mv: check if <destination> is a SKIP_WORKTREE_DIR > 5: 17a871a06b = 5: 26f29df8c8 mv: remove BOTH from enum update_mode > 6: 32b9f85aa1 ! 6: 2169b873f7 mv: from in-cone to out-of-cone > @@ Commit message > Change the behavior so that we can move an in-cone <source> to > out-of-cone <destination> when --sparse is supplied. > > + Notice that <destination> can also be an out-of-cone file path, rather > + than a directory. > + > Such <source> can be either clean or dirty, and moving it results in > different behaviors: > > - A clean move should move the <source> to the <destination>, both in the > - working tree and the index, then remove the resulted path from the > - working tree, and turn on its CE_SKIP_WORKTREE bit. > + A clean move should move <source> to <destination> in the index (do > + *not* create <destination> in the worktree), then delete <source> from > + the worktree. > > A dirty move should move the <source> to the <destination>, 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 <destination> 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 <derrickstolee@github.com> > Helped-by: Victoria Dye <vdye@github.com> > Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> > @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix) > } else if (argc != 1) { > die(_("destination '%s' is not a directory"), dest_path[0]); > } else { > + destination = dest_path; > ++ /* > ++ * <destination> 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]) { > @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix) > const char *src = source[i], *dst = destination[i]; > enum update_mode mode = modes[i]; > @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix) > if (show_only) > continue; > if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) && > -+ !(dst_mode & SKIP_WORKTREE_DIR) && > ++ !(dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) && > rename(src, dst) < 0) { > if (ignore_errors) > continue; > @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix) > + if (ignore_sparse && > + core_apply_sparse_checkout && > + core_sparse_checkout_cone) { > ++ /* > ++ * NEEDSWORK: we are *not* paying attention to > ++ * "out-to-out" move (<source> is out-of-cone and > ++ * <destination> 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 */ > @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix) > + > + if (checkout_entry(dst_ce, &state, NULL, NULL)) > + die(_("cannot checkout %s"), dst_ce->name); > -+ } else if ((dst_mode & SKIP_WORKTREE_DIR) && > ++ } else if ((dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) && > + !(mode & SPARSE) && > + !path_in_sparse_checkout(dst, &the_index)) { > + /* from in-cone to out-of-cone */ > @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'move sparse file to existing > test_when_finished "cleanup_sparse_checkout" && > setup_sparse_checkout && > > -@@ t/t7002-mv-sparse-checkout.sh: test_expect_failure 'move clean path from in-cone to out-of-cone overwrite' ' > +@@ t/t7002-mv-sparse-checkout.sh: test_expect_failure 'move directory with one of the files overwrite' ' > test_cmp expect actual > ' > > 7: 449dba3853 = 7: 78a55e2a46 mv: cleanup empty WORKING_DIRECTORY > 8: 875756480e = 8: 43ce1c07ec advice.h: add advise_on_moving_dirty_path() > 9: cd5d30505b ! 9: 2e7c6a33c2 mv: check overwrite for in-to-out move > @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix) > } > > + if (ignore_sparse && > -+ (dst_mode & SKIP_WORKTREE_DIR) && > ++ (dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) && > + index_entry_exists(&the_index, dst, strlen(dst))) { > + bad = _("destination exists in the index"); > + if (force) { > @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'move clean path from in-cone > test_when_finished "cleanup_sparse_checkout" && > setup_sparse_checkout && > echo "sub/file1 overwrite" >sub/file1 && > +@@ t/t7002-mv-sparse-checkout.sh: 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 <destination> 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 && > +@@ t/t7002-mv-sparse-checkout.sh: 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 && > > base-commit: c50926e1f48891e2671e1830dbcd2912a4563450
## Changes since PATCH v2 ## 1. Rephrase the descriptions for clean moves to match the actual implementation. 2. Add test for moving to a file path <destination> with overwrite. 3. Add test for moving a directory as <source>, with one of its file overwrites file entry in <destination>. 4. *with_slash -> with_slash, remove the dereference sign. 5. Take care of in-to-out where <destination> is a file path, rather than a directory, this is what point 2 is testing. 6. Add a NEEDSWORK doc to address "out-to-out" moves. ## Changes since PATCH v1 ## 1. Change "grep -x" to ^ and $ in t7002 test, and remove some useless "-x" (lines that are *not* ambiguous). 2. Rename check_dir_in_index() to empty_dir_has_sparse_contents() and add more precise documentation. 3. Reverse return values from empty_dir_has_sparse_contents() to comply with the method's name. 4. Make some commit messages more natural/fluent/without typos. 5. Split commit "mv: from in-cone to out-of-cone" to two commits, one does in-to-out functionality, the other one does advice. 6. Take care a few memory leaks (`xstrdup`s). 7. Add a new way to recursively cleanup empty WORKING_DIRECTORY. ## leftover bits ## I decided *not* to solve these bits in this series but in a future one, so things can be handled one at a time without exceeding my capability. 1. When trying to move from-in-to-out, the mechanism assumes that the <destination> is out-of-cone, and by cone-mode definition, <destination> is basically an invisible directory (SKIP_WORKTREE_DIR). However, the dirty move mechanism materializes the moved file to make sure the information is not lost, and this mechanism brings the invisible directory back into the worktree. Hence, if we want to perform a second move from-in-to-out, the assumption that <destination> is not on-disk is broken, and everything follows breaks too. 2. A logic loophole introduced in the previous out-to-in patch, especially in b91a2b6594 (mv: add check_dir_in_index() and solve general dir check issue). Please detach your head to b91a2b6594 for context. Copy and paste: git switch b91a2b6594 When moving from out-to-in, <source> is an invisible SKIP_WORKTREE_DIR. Line 226 uses `lstat()` to make sure the <source> is not on-disk; then line 233-237 checks if <source> is a SKIP_WORKTREE_DIR. If the check passes, line 236 jump to line 276 and try to verify <source> is a directory using `S_ISDIR`. However, because the prerequisite is that the line 226 `lstat()` fails so we can go through the steps said above; and when it fails, the `st` stat, especially the `st_mode` member, is either an uninitialized chunk of garbage, or the result from previous `lstat()`. In this case, `st_mode` *is* from a previous `lstat()`, on line 205. This `lstat()` is testing the <destination>, which, under the out-to-in situation, is always an on-disk directory. Thus, by a series of coincidence, the `S_ISDIR()` on line 276 succeed and everything *looks* OK test-wise. But clearly the `st` at this point is an impostor: it does not reflect the actual stat situation of <source> and it luckily slips through. This needs to be fixed. 3. A NEEDSWORK added to address "out-to-out" move. ## PATCH v1 info ## This is a sister series to the previous "from out-of-cone to in-cone" [1] series. This series is trying to make the opposite operation possible for 'mv', namely move <source>, which is in-cone, to <destination>, which is out-of-cone. Other than the main task, there are also some minor fixes done. [1] https://lore.kernel.org/git/20220331091755.385961-1-shaoxuan.yuan02@gmail.com/ Shaoxuan Yuan (9): t7002: add tests for moving from in-cone to out-of-cone mv: rename check_dir_in_index() to empty_dir_has_sparse_contents() mv: free the with_slash in check_dir_in_index() mv: check if <destination> is a SKIP_WORKTREE_DIR mv: remove BOTH from enum update_mode mv: from in-cone to out-of-cone mv: cleanup empty WORKING_DIRECTORY advice.h: add advise_on_moving_dirty_path() mv: check overwrite for in-to-out move advice.c | 19 +++ advice.h | 1 + builtin/mv.c | 159 ++++++++++++++++++++---- t/t7002-mv-sparse-checkout.sh | 226 +++++++++++++++++++++++++++++++++- 4 files changed, 378 insertions(+), 27 deletions(-) Range-diff against v2: 1: a8c360c083 ! 1: 8134fa5990 t7002: add tests for moving from in-cone to out-of-cone @@ Commit message Such <source> can be either clean or dirty, and moving it results in different behaviors: - A clean move should move the <source> to the <destination>, both in the - working tree and the index, then remove the resulted path from the - working tree, and turn on its CE_SKIP_WORKTREE bit. + A clean move should move <source> to <destination> in the index (do + *not* create <destination> in the worktree), then delete <source> from + the worktree. A dirty move should move the <source> to the <destination>, both in the working tree and the index, but should *not* remove the resulted path @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'move sparse file to existing + 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 <destination> 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 && 2: 725a83c575 = 2: cc5f2770de mv: rename check_dir_in_index() to empty_dir_has_sparse_contents() 3: 1d4a0c16a6 ! 3: 1780f36825 mv: free the *with_slash in check_dir_in_index() @@ Metadata Author: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> ## Commit message ## - mv: free the *with_slash in check_dir_in_index() + mv: free the with_slash in check_dir_in_index() - *with_slash may be a malloc'd pointer, and when it is, free it. + with_slash may be a malloc'd pointer, and when it is, free it. Helped-by: Derrick Stolee <derrickstolee@github.com> Helped-by: Victoria Dye <vdye@github.com> 4: 1f35f0eb34 = 4: d935bd8d7a mv: check if <destination> is a SKIP_WORKTREE_DIR 5: 17a871a06b = 5: 26f29df8c8 mv: remove BOTH from enum update_mode 6: 32b9f85aa1 ! 6: 2169b873f7 mv: from in-cone to out-of-cone @@ Commit message Change the behavior so that we can move an in-cone <source> to out-of-cone <destination> when --sparse is supplied. + Notice that <destination> can also be an out-of-cone file path, rather + than a directory. + Such <source> can be either clean or dirty, and moving it results in different behaviors: - A clean move should move the <source> to the <destination>, both in the - working tree and the index, then remove the resulted path from the - working tree, and turn on its CE_SKIP_WORKTREE bit. + A clean move should move <source> to <destination> in the index (do + *not* create <destination> in the worktree), then delete <source> from + the worktree. A dirty move should move the <source> to the <destination>, 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 <destination> 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 <derrickstolee@github.com> Helped-by: Victoria Dye <vdye@github.com> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix) } else if (argc != 1) { die(_("destination '%s' is not a directory"), dest_path[0]); } else { + destination = dest_path; ++ /* ++ * <destination> 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]) { @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix) const char *src = source[i], *dst = destination[i]; enum update_mode mode = modes[i]; @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix) if (show_only) continue; if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) && -+ !(dst_mode & SKIP_WORKTREE_DIR) && ++ !(dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) && rename(src, dst) < 0) { if (ignore_errors) continue; @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix) + if (ignore_sparse && + core_apply_sparse_checkout && + core_sparse_checkout_cone) { ++ /* ++ * NEEDSWORK: we are *not* paying attention to ++ * "out-to-out" move (<source> is out-of-cone and ++ * <destination> 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 */ @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix) + + if (checkout_entry(dst_ce, &state, NULL, NULL)) + die(_("cannot checkout %s"), dst_ce->name); -+ } else if ((dst_mode & SKIP_WORKTREE_DIR) && ++ } else if ((dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) && + !(mode & SPARSE) && + !path_in_sparse_checkout(dst, &the_index)) { + /* from in-cone to out-of-cone */ @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'move sparse file to existing test_when_finished "cleanup_sparse_checkout" && setup_sparse_checkout && -@@ t/t7002-mv-sparse-checkout.sh: test_expect_failure 'move clean path from in-cone to out-of-cone overwrite' ' +@@ t/t7002-mv-sparse-checkout.sh: test_expect_failure 'move directory with one of the files overwrite' ' test_cmp expect actual ' 7: 449dba3853 = 7: 78a55e2a46 mv: cleanup empty WORKING_DIRECTORY 8: 875756480e = 8: 43ce1c07ec advice.h: add advise_on_moving_dirty_path() 9: cd5d30505b ! 9: 2e7c6a33c2 mv: check overwrite for in-to-out move @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix) } + if (ignore_sparse && -+ (dst_mode & SKIP_WORKTREE_DIR) && ++ (dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) && + index_entry_exists(&the_index, dst, strlen(dst))) { + bad = _("destination exists in the index"); + if (force) { @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'move clean path from in-cone test_when_finished "cleanup_sparse_checkout" && setup_sparse_checkout && echo "sub/file1 overwrite" >sub/file1 && +@@ t/t7002-mv-sparse-checkout.sh: 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 <destination> 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 && +@@ t/t7002-mv-sparse-checkout.sh: 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 && base-commit: c50926e1f48891e2671e1830dbcd2912a4563450