On Tue, Aug 24, 2021 at 6:54 PM Derrick Stolee via GitGitGadget
<gitgitgadget@xxxxxxxxx> 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!
