Re: [PATCH 11/13] mv: refuse to move sparse paths
- From
Matheus Tavares Bernardino <matheus.bernardino@usp.br>
- Date
- Aug 27, 2021, 21:20 UTC
- Message-ID
- <CAHd-oW5-f2Kb5DR-UTfu1qB1fm63oHf62WYsbGd5ajZueOWHtA@mail.gmail.com>
- In-Reply-To
- <d31c641180645ee4059aab9230841ad90f9244fe.1629842085.git.gitgitgadget@gmail.com>
On Tue, Aug 24, 2021 at 6:54 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:
Show 15 quoted lines
>
> 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.
Show 45 quoted lines
> + 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.)
Show 13 quoted lines
> + 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?
Show 17 quoted lines
> + 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.
Show 11 quoted lines
> +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.
Show 26 quoted lines
> +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!