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@xxxxxxxxx/ > (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@xxxxxxxxx> > > ## 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@xxxxxxxxxx> > Helped-by: Victoria Dye <vdye@xxxxxxxxxx> > 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@xxxxxxxxxx> > Helped-by: Victoria Dye <vdye@xxxxxxxxxx> > Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@xxxxxxxxx> > @@ 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
