[PATCH v3 0/2] mv: report missing destination leading directory

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

Changes in v3:

 * changed check from lstat to stat so it follows symlinks as suggested by
   Junio C Hamano
 * added ENOTDIR verification as suggested by Junio C Hamano
 * added S_ISDIR check to catch files as path components as suggested by
   Junio C Hamano
 * fixed indentation

Changes in v2:

 * altered the error message to include both source and destination as
   suggested by Ben Knoble

Lucas Zamboni Orioli (2):
  mv: name both source and destination when rename fails
  mv: check for missing destination directory before renaming

 builtin/mv.c  | 26 +++++++++++++++++++++++++-
 t/t7001-mv.sh | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 1 deletion(-)


base-commit: 9a0c4701dcd5725c4184599322b52933ff5005ca
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2356%2FZamboniL%2Fmv-detect-non-existing-target-folder-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2356/ZamboniL/mv-detect-non-existing-target-folder-v3
Pull-Request: https://github.com/git/git/pull/2356

Range-diff vs v2:

 1:  0d67da588b = 1:  0d67da588b mv: name both source and destination when rename fails
 2:  1a790e0016 ! 2:  5ac1587362 mv: check for missing destination directory before renaming
     @@ Commit message
          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. As a consequence "git mv -n" does not
     -    detect the problem either: the dry run never reaches rename(2) and
     -    reports a move that would not actually succeed.
     +    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, if it is
     -    missing, fail with the existing "destination directory does not exist"
     -    message. Guard the check with the same condition under which rename(2)
     -    is invoked, so that directory moves, whose child entries are expanded
     -    to paths under a not-yet-created directory, and sparse or out-of-cone
     -    destinations, which are not written to the worktree, are not flagged
     -    incorrectly.
     +    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.
      
     -    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.
     -
     -    Add tests covering both the error path and the dry-run detection.
     +    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: dir_check:
       		}
       
      +		/*
     -+		* If we are going to move SRC to DST on disk, DST's leading
     -+		* directories must already exist.
     -+		*/
     ++		 * 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, '/');
     ++			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);
     ++			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 &&
     @@ t/t7001-mv.sh: test_expect_success 'clean up' '
       	git reset --hard
       '
       
     -+test_expect_success 'moving to non-existent destination parent directory' '
     ++test_expect_success 'moving to a non-existent path component in the destination' '
      +	git reset --hard &&
      +	mkdir -p from &&
      +	echo content >from/file &&
     @@ t/t7001-mv.sh: test_expect_success 'clean up' '
      +	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
      +'

-- 
gitgitgadget