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

["Lucas Zamboni Orioli via GitGitGadget" <gitgitgadget@gmail.com>]

From: Lucas Zamboni Orioli <lucaszam0@gmail.com>

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 <lucaszam0@gmail.com>
---
 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))) {
+			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))
+					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/ &&
-- 
gitgitgadget