Re: [WIP v2 3/5] mv: check if <destination> exists in index to handle overwriting

[Victoria Dye <vdye@github.com>]

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@gmail.com>
> ---
>  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@github.com/

>  			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!