| Message ID | 20220623114120.12768-8-shaoxuan.yuan02@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mv: fix out-of-cone file/directory move logic | expand |
On 6/23/2022 7:41 AM, Shaoxuan Yuan wrote: > Originally, moving a <source> directory which is not on-disk due > to its existence outside of sparse-checkout cone, "giv mv" command > errors out with "bad source". > > Add a helper check_dir_in_index() function to see if a directory > name exists in the index. Also add a SKIP_WORKTREE_DIR bit to mark > such directories. > > Change the checking logic, so that such <source> directory makes > "giv mv" command warns with "advise_on_updating_sparse_paths()" > instead of "bad source"; also user now can supply a "--sparse" flag so > this operation can be carried out successfully. > > Helped-by: Victoria Dye <vdye@github.com> > Helped-by: Derrick Stolee <derrickstolee@github.com> > Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> > --- > builtin/mv.c | 50 ++++++++++++++++++++++++++++++----- > t/t7002-mv-sparse-checkout.sh | 4 +-- > 2 files changed, 46 insertions(+), 8 deletions(-) > > diff --git a/builtin/mv.c b/builtin/mv.c > index aa29da4337..b5d0d8ef4f 100644 > --- a/builtin/mv.c > +++ b/builtin/mv.c > @@ -25,6 +25,7 @@ enum update_mode { > WORKING_DIRECTORY = (1 << 1), > INDEX = (1 << 2), > SPARSE = (1 << 3), > + SKIP_WORKTREE_DIR = (1 << 4), > }; > > #define DUP_BASENAME 1 > @@ -123,6 +124,36 @@ static int index_range_of_same_dir(const char *src, int length, > return last - first; > } > > +/* > + * 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. > + */ This description and the implementation seems like it will work even if the path exists as a sparse directory in a sparse index. It would be good to consider testing this kind of move for a directory on the sparse boundary (where it would be a sparse directory in a sparse index) _and_ if it is deeper than the boundary (so the sparse index would expand in the cache_name_pos() method). These tests can be written now for correctness, but later the first case can be updated to use the 'ensure_not_expanded' helper in t1092. > +static int check_dir_in_index(const char *name) Thanks, -Stolee
On 6/23/2022 11:14 PM, Derrick Stolee wrote: > On 6/23/2022 7:41 AM, Shaoxuan Yuan wrote: >> Originally, moving a <source> directory which is not on-disk due >> to its existence outside of sparse-checkout cone, "giv mv" command >> errors out with "bad source". >> >> Add a helper check_dir_in_index() function to see if a directory >> name exists in the index. Also add a SKIP_WORKTREE_DIR bit to mark >> such directories. >> >> Change the checking logic, so that such <source> directory makes >> "giv mv" command warns with "advise_on_updating_sparse_paths()" >> instead of "bad source"; also user now can supply a "--sparse" flag so >> this operation can be carried out successfully. >> >> Helped-by: Victoria Dye <vdye@github.com> >> Helped-by: Derrick Stolee <derrickstolee@github.com> >> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> >> --- >> builtin/mv.c | 50 ++++++++++++++++++++++++++++++----- >> t/t7002-mv-sparse-checkout.sh | 4 +-- >> 2 files changed, 46 insertions(+), 8 deletions(-) >> >> diff --git a/builtin/mv.c b/builtin/mv.c >> index aa29da4337..b5d0d8ef4f 100644 >> --- a/builtin/mv.c >> +++ b/builtin/mv.c >> @@ -25,6 +25,7 @@ enum update_mode { >> WORKING_DIRECTORY = (1 << 1), >> INDEX = (1 << 2), >> SPARSE = (1 << 3), >> + SKIP_WORKTREE_DIR = (1 << 4), >> }; >> >> #define DUP_BASENAME 1 >> @@ -123,6 +124,36 @@ static int index_range_of_same_dir(const char *src, int length, >> return last - first; >> } >> >> +/* >> + * 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. >> + */ > > This description and the implementation seems like it will work > even if the path exists as a sparse directory in a sparse index. > > It would be good to consider testing this kind of move for a > directory on the sparse boundary (where it would be a sparse > directory in a sparse index) _and_ if it is deeper than the > boundary (so the sparse index would expand in the cache_name_pos() > method). These tests can be written now for correctness, but later > the first case can be updated to use the 'ensure_not_expanded' > helper in t1092. I'm a bit confused here. Shouldn't we turn on the sparse-index feature for 'mv' before adding sparse-index related tests? Since this series does not go into sparse-index, I'm not sure how the tests can pass. Perhaps we can test about this in the future sparse-index integration series, no? Thanks & Regards, Shaoxuan
On 6/24/2022 3:57 AM, Shaoxuan Yuan wrote: > On 6/23/2022 11:14 PM, Derrick Stolee wrote: >> On 6/23/2022 7:41 AM, Shaoxuan Yuan wrote: >>> +/* >>> + * 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. >>> + */ >> >> This description and the implementation seems like it will work >> even if the path exists as a sparse directory in a sparse index. >> >> It would be good to consider testing this kind of move for a >> directory on the sparse boundary (where it would be a sparse >> directory in a sparse index) _and_ if it is deeper than the >> boundary (so the sparse index would expand in the cache_name_pos() >> method). These tests can be written now for correctness, but later >> the first case can be updated to use the 'ensure_not_expanded' >> helper in t1092. > > I'm a bit confused here. Shouldn't we turn on the sparse-index > feature for 'mv' before adding sparse-index related tests? Since this > series does not go into sparse-index, I'm not sure how the tests can > pass. Perhaps we can test about this in the future sparse-index > integration series, no? I mean that you are making a change right now that might lead to different behavior _when you enable the sparse index later_. Since we are looking at this behavior now, it might be interesting to add the extra test coverage now while we are thinking about this data shape. We can add tests to t1092-sparse-checkout-compatibility.sh even though the sparse index is expanded on every 'git mv' instance, but it can help when you eventually update 'git mv' to not expand on a sparse index. The bit about ensure_not_expanded can't be added until you integrate with the sparse index, but it is easier to add that when you already have tests that check these boundary cases. Thanks, -Stolee
diff --git a/builtin/mv.c b/builtin/mv.c index aa29da4337..b5d0d8ef4f 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -25,6 +25,7 @@ enum update_mode { WORKING_DIRECTORY = (1 << 1), INDEX = (1 << 2), SPARSE = (1 << 3), + SKIP_WORKTREE_DIR = (1 << 4), }; #define DUP_BASENAME 1 @@ -123,6 +124,36 @@ static int index_range_of_same_dir(const char *src, int length, return last - first; } +/* + * 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. + */ +static int check_dir_in_index(const char *name) +{ + const char *with_slash = add_slash(name); + int length = strlen(with_slash); + + int pos = cache_name_pos(with_slash, length); + const struct cache_entry *ce; + + if (pos < 0) { + pos = -pos - 1; + if (pos >= the_index.cache_nr) + return 1; + ce = active_cache[pos]; + if (strncmp(with_slash, ce->name, length)) + return 1; + if (ce_skip_worktree(ce)) + return 0; + } + return 1; +} + int cmd_mv(int argc, const char **argv, const char *prefix) { int i, flags, gitmodules_modified = 0; @@ -184,7 +215,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix) /* Checking */ for (i = 0; i < argc; i++) { const char *src = source[i], *dst = destination[i]; - int length, src_is_dir; + int length; const char *bad = NULL; int skip_sparse = 0; @@ -198,12 +229,17 @@ int cmd_mv(int argc, const char **argv, const char *prefix) pos = cache_name_pos(src, length); 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)) { + modes[i] |= SKIP_WORKTREE_DIR; + goto dir_check; + } /* only error if existence is expected. */ if (!(modes[i] & SPARSE)) bad = _("bad source"); goto act_on_entry; } - ce = active_cache[pos]; if (!ce_skip_worktree(ce)) { bad = _("bad source"); @@ -230,12 +266,14 @@ int cmd_mv(int argc, const char **argv, const char *prefix) bad = _("can not move directory into itself"); goto act_on_entry; } - if ((src_is_dir = S_ISDIR(st.st_mode)) + if (S_ISDIR(st.st_mode) && lstat(dst, &st) == 0) { bad = _("cannot move directory over file"); goto act_on_entry; } - if (src_is_dir) { + +dir_check: + if (S_ISDIR(st.st_mode)) { int j, dst_len, n; int first = cache_name_pos(src, length), last; @@ -369,7 +407,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix) printf(_("Renaming %s to %s\n"), src, dst); if (show_only) continue; - if (!(mode & (INDEX | SPARSE)) && + if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) && rename(src, dst) < 0) { if (ignore_errors) continue; @@ -384,7 +422,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix) 1); } - if (mode & (WORKING_DIRECTORY)) + if (mode & (WORKING_DIRECTORY | SKIP_WORKTREE_DIR)) continue; pos = cache_name_pos(src, strlen(src)); diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh index 6d2fb4f8d2..71fe29690f 100755 --- a/t/t7002-mv-sparse-checkout.sh +++ b/t/t7002-mv-sparse-checkout.sh @@ -219,7 +219,7 @@ test_expect_success 'refuse to move file to non-skip-worktree sparse path' ' test_cmp expect stderr ' -test_expect_failure 'refuse to move out-of-cone directory without --sparse' ' +test_expect_success 'refuse to move out-of-cone directory without --sparse' ' test_when_finished "cleanup_sparse_checkout" && setup_sparse_checkout && @@ -230,7 +230,7 @@ test_expect_failure 'refuse to move out-of-cone directory without --sparse' ' test_cmp expect stderr ' -test_expect_failure 'can move out-of-cone directory with --sparse' ' +test_expect_success 'can move out-of-cone directory with --sparse' ' test_when_finished "cleanup_sparse_checkout" && setup_sparse_checkout &&
Originally, moving a <source> directory which is not on-disk due to its existence outside of sparse-checkout cone, "giv mv" command errors out with "bad source". Add a helper check_dir_in_index() function to see if a directory name exists in the index. Also add a SKIP_WORKTREE_DIR bit to mark such directories. Change the checking logic, so that such <source> directory makes "giv mv" command warns with "advise_on_updating_sparse_paths()" instead of "bad source"; also user now can supply a "--sparse" flag so this operation can be carried out successfully. Helped-by: Victoria Dye <vdye@github.com> Helped-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> --- builtin/mv.c | 50 ++++++++++++++++++++++++++++++----- t/t7002-mv-sparse-checkout.sh | 4 +-- 2 files changed, 46 insertions(+), 8 deletions(-)