Shaoxuan Yuan wrote:
> Originally, moving a sparse file into cone can result in unwarned
> overwrite of existing entry. The expected behavior is that if the
> <destination> exists in the entry, user should be prompted to supply
> a [-f|--force] to carry out the operation, or the operation should
> fail.
>
> Add a check mechanism to do that.
>
> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@xxxxxxxxx>
> ---
> builtin/mv.c | 23 +++++++++++------------
> t/t7002-mv-sparse-checkout.sh | 2 +-
> 2 files changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 32ad4d5682..62284e3f86 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -185,16 +185,6 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>
> length = strlen(src);
> if (lstat(src, &st) < 0) {
> - /*
> - * TODO: for now, when you try to overwrite a <destination>
> - * with your <source> as a sparse file, if you supply a "--sparse"
> - * flag, then the action will be done without providing "--force"
> - * and no warning.
> - *
> - * This is mainly because the sparse <source>
> - * is not on-disk, and this if-else chain will be cut off early in
> - * this check, thus the "--force" check is ignored. Need fix.
> - */
>
Given that this removes the "TODO" comment you just added in the previous
patch, I agree with Stolee's suggestion [1] that you mention this context in
the patch 2 commit message rather than a code comment. The commit message of
*this* patch already explains the behavior you're correcting, so I don't
think any other changes would be needed here.
[1] https://lore.kernel.org/git/0884b97b-0745-5cad-3034-a679be5d6c3a@xxxxxxxxxx/
> int pos = cache_name_pos(src, length);
> if (pos >= 0) {
> @@ -203,8 +193,17 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
> if (ce_skip_worktree(ce)) {
> if (!ignore_sparse)
> string_list_append(&only_match_skip_worktree, src);
> - else
> - modes[i] = SPARSE;
> + else {
> + /* Check if dst exists in index */
> + if (cache_name_pos(dst, strlen(dst)) >= 0) {
> + if (force)
> + modes[i] = SPARSE;
> + else
> + bad = _("destination exists");
> + }
> + else
> + modes[i] = SPARSE;
> + }
> }
> else
> bad = _("bad source");
> diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
> index 581ef4c0f6..2c9008573a 100755
> --- a/t/t7002-mv-sparse-checkout.sh
> +++ b/t/t7002-mv-sparse-checkout.sh
> @@ -272,7 +272,7 @@ test_expect_success 'can move out-of-cone file with --sparse' '
> test_path_is_file sub/file1
> '
>
> -test_expect_failure 'refuse to move sparse file to existing destination' '
> +test_expect_success 'refuse to move sparse file to existing destination' '
> git sparse-checkout disable &&
> git reset --hard &&
> mkdir folder1 &&
The rest of this looks good to me!
