| Message ID | d31c641180645ee4059aab9230841ad90f9244fe.1629842085.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Sparse-checkout: modify 'git add', 'git rm', and 'git add' behavior | expand |
On Tue, Aug 24, 2021 at 6:54 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote: > > diff --git a/builtin/mv.c b/builtin/mv.c > index c2f96c8e895..b58fd4ce5ba 100644 > --- a/builtin/mv.c > +++ b/builtin/mv.c > @@ -176,10 +177,22 @@ int cmd_mv(int argc, const char **argv, const char *prefix) > const char *src = source[i], *dst = destination[i]; > int length, src_is_dir; > const char *bad = NULL; > + int skip_sparse = 0; > > if (show_only) > printf(_("Checking rename of '%s' to '%s'\n"), src, dst); > > + if (!path_in_sparse_checkout(src, &the_index)) { `git mv` can only move/rename tracked paths, but since we check whether `src` is sparse before checking if it is in the index, the user will get the sparse error message instead. This is OK, but the advice might be misleading, as it says they can use `--sparse` if they really want to move the file, but repeating the command with `--sparse` will now fail for another reason. I wonder if we should check whether `src` is tracked before checking if it is sparse, or if that is not really an issue we should bother with. > + string_list_append(&only_match_skip_worktree, src); > + skip_sparse = 1; > + } > + if (!path_in_sparse_checkout(dst, &the_index)) { > + string_list_append(&only_match_skip_worktree, dst); > + skip_sparse = 1; > + } > + if (skip_sparse) > + continue; > + > length = strlen(src); > if (lstat(src, &st) < 0) > bad = _("bad source"); > > diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh > new file mode 100755 > index 00000000000..5397c6d07bd > --- /dev/null > +++ b/t/t7002-mv-sparse-checkout.sh > @@ -0,0 +1,99 @@ > +#!/bin/sh > + > +test_description='git mv in sparse working trees' > + > +. ./test-lib.sh > + > +test_expect_success 'setup' " > + mkdir -p sub/dir sub/dir2 && > + touch a b c sub/d sub/dir/e sub/dir2/e && > + git add -A && > + git commit -m files && > + > + cat >sparse_error_header <<-EOF && > + The following pathspecs didn't match any eligible path, but they do match index > + entries outside the current sparse checkout: > + EOF > + > + cat >sparse_hint <<-EOF > + hint: Disable or modify the sparsity rules if you intend to update such entries. > + hint: Disable this message with \"git config advice.updateSparsePath false\" > + EOF > +" > + > +test_expect_success 'mv refuses to move sparse-to-sparse' ' > + rm -f e && At first glance, it confused me a bit that we are removing `e` when the setup didn't create it. But then I realized the test itself might create `e` if `git mv` succeeds in moving the `b` file. Could perhaps this and the others `rm -f e` be a `test_when_finished`, to make it clearer that it is a cleanup? > + git reset --hard && > + git sparse-checkout set a && > + touch b && > + test_must_fail git mv b e 2>stderr && Here we try to move a "tracked sparse path" to an "untracked sparse path". Do we also want to test with a tracked to tracked operation? (Although the code path will be the same, of course.) > + cat sparse_error_header >expect && > + echo b >>expect && > + echo e >>expect && > + cat sparse_hint >>expect && > + test_cmp expect stderr > +' > + > +test_expect_success 'mv refuses to move sparse-to-sparse, ignores failure' ' > + rm -f e && > + git reset --hard && > + git sparse-checkout set a && > + touch b && > + git mv -k b e 2>stderr && Maybe also check that `b` is still there, and `e` is missing? > + cat sparse_error_header >expect && > + echo b >>expect && > + echo e >>expect && > + cat sparse_hint >>expect && > + test_cmp expect stderr > +' > + > +test_expect_success 'mv refuses to move non-sparse-to-sparse' ' > + rm -f e && > + git reset --hard && > + git sparse-checkout set a && > + test_must_fail git mv a e 2>stderr && > + cat sparse_error_header >expect && > + echo e >>expect && > + cat sparse_hint >>expect && > + test_cmp expect stderr > +' OK. > +test_expect_success 'mv refuses to move sparse-to-non-sparse' ' > + rm -f e && > + git reset --hard && > + git sparse-checkout set a e && > + touch b && > + test_must_fail git mv b e 2>stderr && > + cat sparse_error_header >expect && > + echo b >>expect && > + cat sparse_hint >>expect && > + test_cmp expect stderr > +' OK. > +test_expect_success 'recursive mv refuses to move (possible) sparse' ' > + rm -f e && > + git reset --hard && > + # Without cone mode, "sub" and "sub2" do not match > + git sparse-checkout set sub/dir sub2/dir && > + test_must_fail git mv sub sub2 2>stderr && > + cat sparse_error_header >expect && > + echo sub >>expect && > + echo sub2 >>expect && > + cat sparse_hint >>expect && > + test_cmp expect stderr > +' > + > +test_expect_success 'recursive mv refuses to move sparse' ' > + git reset --hard && > + # Use cone mode so "sub/" matches the sparse-checkout patterns > + git sparse-checkout init --cone && > + git sparse-checkout set sub/dir sub2/dir && > + test_must_fail git mv sub sub2 2>stderr && > + cat sparse_error_header >expect && > + echo sub/dir2/e >>expect && > + echo sub2/dir2/e >>expect && > + cat sparse_hint >>expect && > + test_cmp expect stderr > +' > + Ah, these last two are very interesting cases!
On Fri, Aug 27, 2021 at 6:20 PM Matheus Tavares Bernardino <matheus.bernardino@usp.br> wrote: > > On Tue, Aug 24, 2021 at 6:54 PM Derrick Stolee via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > > > diff --git a/builtin/mv.c b/builtin/mv.c > > index c2f96c8e895..b58fd4ce5ba 100644 > > --- a/builtin/mv.c > > +++ b/builtin/mv.c > > @@ -176,10 +177,22 @@ int cmd_mv(int argc, const char **argv, const char *prefix) > > const char *src = source[i], *dst = destination[i]; > > int length, src_is_dir; > > const char *bad = NULL; > > + int skip_sparse = 0; > > > > if (show_only) > > printf(_("Checking rename of '%s' to '%s'\n"), src, dst); > > > > + if (!path_in_sparse_checkout(src, &the_index)) { > > `git mv` can only move/rename tracked paths, but since we check > whether `src` is sparse before checking if it is in the index, the > user will get the sparse error message instead. This is OK, but the > advice might be misleading, as it says they can use `--sparse` if they > really want to move the file, but repeating the command with > `--sparse` will now fail for another reason. I wonder if we should > check whether `src` is tracked before checking if it is sparse, or if > that is not really an issue we should bother with. Another problem is that the displayed message will say that the pathspecs "match index entries outside sparse checkout" even when the path given to mv doesn't really exist: git sparse-checkout set some/dir/ git mv nonexistent-file foo The following pathspecs didn't match any eligible path, but they do match index entries outside the current sparse checkout: nonexistent-file hint: Disable or modify the sparsity rules if you intend to update such entries. hint: Disable this message with "git config advice.updateSparsePath false"
On 8/27/2021 5:20 PM, Matheus Tavares Bernardino wrote: > On Tue, Aug 24, 2021 at 6:54 PM Derrick Stolee via GitGitGadget > <gitgitgadget@gmail.com> wrote: >> >> diff --git a/builtin/mv.c b/builtin/mv.c >> index c2f96c8e895..b58fd4ce5ba 100644 >> --- a/builtin/mv.c >> +++ b/builtin/mv.c >> @@ -176,10 +177,22 @@ int cmd_mv(int argc, const char **argv, const char *prefix) >> const char *src = source[i], *dst = destination[i]; >> int length, src_is_dir; >> const char *bad = NULL; >> + int skip_sparse = 0; >> >> if (show_only) >> printf(_("Checking rename of '%s' to '%s'\n"), src, dst); >> >> + if (!path_in_sparse_checkout(src, &the_index)) { > > `git mv` can only move/rename tracked paths, but since we check > whether `src` is sparse before checking if it is in the index, the > user will get the sparse error message instead. This is OK, but the > advice might be misleading, as it says they can use `--sparse` if they > really want to move the file, but repeating the command with > `--sparse` will now fail for another reason. I wonder if we should > check whether `src` is tracked before checking if it is sparse, or if > that is not really an issue we should bother with. I will move the logic to the last possible place before "accepting" the move, then add a comment detailing why it should be there. >> + string_list_append(&only_match_skip_worktree, src); >> + skip_sparse = 1; >> + } >> + if (!path_in_sparse_checkout(dst, &the_index)) { >> + string_list_append(&only_match_skip_worktree, dst); >> + skip_sparse = 1; >> + } >> + if (skip_sparse) >> + continue; >> + ... >> +test_expect_success 'mv refuses to move sparse-to-sparse' ' >> + rm -f e && > > At first glance, it confused me a bit that we are removing `e` when > the setup didn't create it. But then I realized the test itself might > create `e` if `git mv` succeeds in moving the `b` file. Could perhaps > this and the others `rm -f e` be a `test_when_finished`, to make it > clearer that it is a cleanup? test_when_finished is cleaner. > >> + git reset --hard && >> + git sparse-checkout set a && >> + touch b && >> + test_must_fail git mv b e 2>stderr && > > Here we try to move a "tracked sparse path" to an "untracked sparse > path". Do we also want to test with a tracked to tracked operation? > (Although the code path will be the same, of course.) I can expand these tests to include tracked and untracked targets. >> + cat sparse_error_header >expect && >> + echo b >>expect && >> + echo e >>expect && >> + cat sparse_hint >>expect && >> + test_cmp expect stderr >> +' >> + >> +test_expect_success 'mv refuses to move sparse-to-sparse, ignores failure' ' >> + rm -f e && >> + git reset --hard && >> + git sparse-checkout set a && >> + touch b && >> + git mv -k b e 2>stderr && > > Maybe also check that `b` is still there, and `e` is missing? Good idea. In fact, there is a problem that the '-k' gets around the protections because it doesn't return 1 early. I'll fix this by jumping to the end of the loop which removes the entries from the arrays. Thanks, -Stolee
diff --git a/builtin/mv.c b/builtin/mv.c index c2f96c8e895..b58fd4ce5ba 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -133,6 +133,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix) 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; git_config(git_default_config, NULL); @@ -176,10 +177,22 @@ int cmd_mv(int argc, const char **argv, const char *prefix) const char *src = source[i], *dst = destination[i]; int length, src_is_dir; const char *bad = NULL; + int skip_sparse = 0; if (show_only) printf(_("Checking rename of '%s' to '%s'\n"), src, dst); + if (!path_in_sparse_checkout(src, &the_index)) { + string_list_append(&only_match_skip_worktree, src); + skip_sparse = 1; + } + if (!path_in_sparse_checkout(dst, &the_index)) { + string_list_append(&only_match_skip_worktree, dst); + skip_sparse = 1; + } + if (skip_sparse) + continue; + length = strlen(src); if (lstat(src, &st) < 0) bad = _("bad source"); @@ -266,6 +279,12 @@ int cmd_mv(int argc, const char **argv, const char *prefix) } } + if (only_match_skip_worktree.nr) { + advise_on_updating_sparse_paths(&only_match_skip_worktree); + if (!ignore_errors) + return 1; + } + for (i = 0; i < argc; i++) { const char *src = source[i], *dst = destination[i]; enum update_mode mode = modes[i]; diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh new file mode 100755 index 00000000000..5397c6d07bd --- /dev/null +++ b/t/t7002-mv-sparse-checkout.sh @@ -0,0 +1,99 @@ +#!/bin/sh + +test_description='git mv in sparse working trees' + +. ./test-lib.sh + +test_expect_success 'setup' " + mkdir -p sub/dir sub/dir2 && + touch a b c sub/d sub/dir/e sub/dir2/e && + git add -A && + git commit -m files && + + cat >sparse_error_header <<-EOF && + The following pathspecs didn't match any eligible path, but they do match index + entries outside the current sparse checkout: + EOF + + cat >sparse_hint <<-EOF + hint: Disable or modify the sparsity rules if you intend to update such entries. + hint: Disable this message with \"git config advice.updateSparsePath false\" + EOF +" + +test_expect_success 'mv refuses to move sparse-to-sparse' ' + rm -f e && + git reset --hard && + git sparse-checkout set a && + touch b && + test_must_fail git mv b e 2>stderr && + cat sparse_error_header >expect && + echo b >>expect && + echo e >>expect && + cat sparse_hint >>expect && + test_cmp expect stderr +' + +test_expect_success 'mv refuses to move sparse-to-sparse, ignores failure' ' + rm -f e && + git reset --hard && + git sparse-checkout set a && + touch b && + git mv -k b e 2>stderr && + cat sparse_error_header >expect && + echo b >>expect && + echo e >>expect && + cat sparse_hint >>expect && + test_cmp expect stderr +' + +test_expect_success 'mv refuses to move non-sparse-to-sparse' ' + rm -f e && + git reset --hard && + git sparse-checkout set a && + test_must_fail git mv a e 2>stderr && + cat sparse_error_header >expect && + echo e >>expect && + cat sparse_hint >>expect && + test_cmp expect stderr +' + +test_expect_success 'mv refuses to move sparse-to-non-sparse' ' + rm -f e && + git reset --hard && + git sparse-checkout set a e && + touch b && + test_must_fail git mv b e 2>stderr && + cat sparse_error_header >expect && + echo b >>expect && + cat sparse_hint >>expect && + test_cmp expect stderr +' + +test_expect_success 'recursive mv refuses to move (possible) sparse' ' + rm -f e && + git reset --hard && + # Without cone mode, "sub" and "sub2" do not match + git sparse-checkout set sub/dir sub2/dir && + test_must_fail git mv sub sub2 2>stderr && + cat sparse_error_header >expect && + echo sub >>expect && + echo sub2 >>expect && + cat sparse_hint >>expect && + test_cmp expect stderr +' + +test_expect_success 'recursive mv refuses to move sparse' ' + git reset --hard && + # Use cone mode so "sub/" matches the sparse-checkout patterns + git sparse-checkout init --cone && + git sparse-checkout set sub/dir sub2/dir && + test_must_fail git mv sub sub2 2>stderr && + cat sparse_error_header >expect && + echo sub/dir2/e >>expect && + echo sub2/dir2/e >>expect && + cat sparse_hint >>expect && + test_cmp expect stderr +' + +test_done