Re: [PATCH v2 2/2] mv: check for missing destination directory before renaming

[Junio C Hamano <gitster@pobox.com>]

"Lucas Zamboni Orioli via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> This is a best-effort diagnostic rather than a guarantee: the
> destination directory can still disappear between the check and the
> rename(2). It fixes the common case and, unlike the syscall path,
> lets "git mv -n" report the failure.

If "can still disappear" is because we are not taking into account a
move that we are scheduled to make, then that is not very nice, but
as long as it is *not* our making (in other words, somebody else may
actively interferring with the mv we are trying to perform), I think
this is OK.  It is the best we can do.

> Add tests covering both the error path and the dry-run detection.
>
> Signed-off-by: Lucas Zamboni Orioli <lucaszam0@gmail.com>
> ---
>  builtin/mv.c  | 21 +++++++++++++++++++++
>  t/t7001-mv.sh | 14 ++++++++++++++
>  2 files changed, 35 insertions(+)
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 35e504484a..eb59fe0f31 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -444,6 +444,27 @@ dir_check:
>  			goto act_on_entry;
>  		}
>  
> +		/*
> +		* If we are going to move SRC to DST on disk, DST's leading
> +		* directories must already exist.
> +		*/
> +		if (!(modes[i] & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
> +				!(dst_mode & (SKIP_WORKTREE_DIR | SPARSE))) {
> +				char *dst_dir = xstrdup(dst);
> +				char *slash = strrchr(dst_dir, '/');
> +
> +				if (slash) {
> +						struct stat dir_st;
> +						*slash = '\0';
> +						if (lstat(dst_dir, &dir_st) < 0 && errno == ENOENT) {
> +								free(dst_dir);
> +								bad = _("destination directory does not exist");
> +								goto act_on_entry;
> +						}
> +				}
> +				free(dst_dir);
> +		}

Horrible.  Please fix this overly deep indentation.

> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
> index 7cf4aa5ba1..2d8a98d8b0 100755
> --- a/t/t7001-mv.sh
> +++ b/t/t7001-mv.sh
> @@ -114,6 +114,20 @@ test_expect_success 'clean up' '
>  	git reset --hard
>  '
>  
> +test_expect_success 'moving to non-existent destination parent directory' '
> +	git reset --hard &&
> +	mkdir -p from &&
> +	echo content >from/file &&
> +	git add from/file &&
> +	test_must_fail git mv from/file no-such-dir/file 2>actual &&
> +	test_grep "destination directory does not exist" actual
> +'
> +
> +test_expect_success 'mv --dry-run detects non-existent destination parent directory' '
> +	test_must_fail git mv -n from/file no-such-dir/file 2>actual &&
> +	test_grep "destination directory does not exist" actual
> +'
> +
>  test_expect_success 'moving to existing untracked target with trailing slash' '
>  	mkdir path1 &&
>  	git mv path0/ path1/ &&