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

On Thu Jul 23, 2026 at 11:40 PM CEST, Lucas Zamboni Orioli via GitGitGadget wrote:
> From: Lucas Zamboni Orioli <[email protected]>
>
> Moving a file into a directory that does not exist fails at rename(2)
> with ENOENT. The checking phase already rejects a missing destination
> directory when the destination ends in a slash, but a destination that
> names a file inside a non-existent directory is not caught and only
> fails later at the syscall. The same is true when a leading path
> component exists but is not a directory: rename(2) fails with ENOTDIR,
> again only at the syscall. As a consequence "git mv -n" does not detect
> either problem: the dry run never reaches rename(2) and reports a move
> that would not actually succeed.
>
> Detect this during the checking phase. For entries that will be renamed
> on disk, stat the destination's leading directory and fail with a
> suitable message if it is missing or is not a directory. stat() is used
> rather than lstat() so that the check follows symlinks the same way
> rename(2) does: a symlink to a directory is accepted, while a symlink to
> a file is rejected. A missing directory or a non-directory path
> component (ENOENT or ENOTDIR) reuses the existing "destination directory
> does not exist" message; a leading component that resolves to a
> non-directory reports "destination is not a directory". Other stat()
> errors fall through to rename(2), which reports them as before.
>
> Add tests covering the missing directory, a path component that is a
> file, a symlink to a file, a symlink to a directory (which must still
> succeed), and dry-run detection.
>
> Signed-off-by: Lucas Zamboni Orioli <[email protected]>
> ---
>  builtin/mv.c  | 24 ++++++++++++++++++++++++
>  t/t7001-mv.sh | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 73 insertions(+)
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 35e504484a..08e27484f2 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -444,6 +444,30 @@ 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))) {

nit: indentation.

> +			char *dst_dir = xstrdup(dst);
> +			char *slash = strrchr(dst_dir, '/');
> +
> +			if (slash) {
> +				struct stat dir_st;
> +				*slash = '\0';
> +				if (stat(dst_dir, &dir_st) < 0) {
> +					/* other errors fall through to rename(), which reports them */
> +					if (errno == ENOENT || errno == ENOTDIR)
> +						bad = _("destination directory does not exist");
> +				} else if (!S_ISDIR(dir_st.st_mode))

nit: the if above has braces, this else if should too.

> +					bad = _("destination is not a directory");
> +			}
> +			free(dst_dir);
> +			if (bad)
> +				goto act_on_entry;
> +		}
> +
>  		if (ignore_sparse &&
>  		    (dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) &&
>  		    index_entry_exists(the_repository->index, dst, strlen(dst))) {
> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
> index 7cf4aa5ba1..c878fb92a8 100755
> --- a/t/t7001-mv.sh
> +++ b/t/t7001-mv.sh
> @@ -114,6 +114,55 @@ test_expect_success 'clean up' '
>  	git reset --hard
>  '
>
> +test_expect_success 'moving to a non-existent path component in the destination' '
> +	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 'moving to a destination with a file as a path component' '
> +	git reset --hard &&
> +	mkdir -p from &&
> +	echo contents >from/file &&
> +	echo blocker >not-dir &&
> +	git add from/file &&
> +	test_must_fail git mv from/file not-dir/file 2>actual &&
> +	test_grep "destination is not a directory" actual
> +'
> +
> +test_expect_success SYMLINKS 'moving to a destination with a symlink to a file as a path component' '
> +	git reset --hard &&
> +	mkdir -p from &&
> +	echo contents >from/file &&
> +	echo target >regular &&
> +	ln -s regular link-to-file &&
> +	git add from/file &&
> +	test_must_fail git mv from/file link-to-file/file 2>actual &&
> +	test_grep "not a directory" actual
> +'
> +
> +test_expect_success SYMLINKS 'moving to a destination with a symlink to a directory' '
> +	git reset --hard &&
> +	mkdir -p from realdir &&
> +	echo contents >from/file &&
> +	ln -s realdir link-to-dir &&
> +	git add from/file &&
> +	git mv from/file link-to-dir/file &&
> +	test_path_is_file realdir/file
> +'
> +
> +test_expect_success 'mv --dry-run detects non-existent destination parent directory' '
> +	git reset --hard &&
> +	mkdir -p from &&
> +	echo content >from/file &&
> +	git add from/file &&
> +	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/ &&


The rest looks good.

Regards,
Pablo