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

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

Changes in v4:

 * reverted to lstat and added has_symlink_leading_path() to refuse a
   destination that goes through a symbolic link, independent of the link
   target, per Junio C Hamano's point that Git tracks symlinks and must not
   follow them here
 * added new "destination is beyond a symbolic link" message
 * added tests: symlink as immediate parent and as intermediate component,
   symlink at the destination, -f does not bypass the symlink refusal, and a
   regression test that a move through a symlink no longer corrupts the
   index (see the reproduction reported on the list)

Changes in v3:

 * added ENOTDIR handling and an S_ISDIR check so a non-directory leading
   path component is caught, as suggested by Junio C Hamano
 * (v3 used stat() to resolve symlinks; this was reverted in v4 after Junio
   pointed out symlinks must not be followed)
 * 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: reject a destination whose leading path is missing or a symlink

 builtin/mv.c  | 37 ++++++++++++++++++++++-
 t/t7001-mv.sh | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 119 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-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2356/ZamboniL/mv-detect-non-existing-target-folder-v4
Pull-Request: https://github.com/git/git/pull/2356

Range-diff vs v3:

 1:  0d67da588b = 1:  0d67da588b mv: name both source and destination when rename fails
 2:  5ac1587362 ! 2:  6b72efb413 mv: check for missing destination directory before renaming
     @@ Metadata
      Author: Lucas Zamboni Orioli <lucaszam0@gmail.com>
      
       ## Commit message ##
     -    mv: check for missing destination directory before renaming
     +    mv: reject a destination whose leading path is missing or a symlink
      
     -    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.
     +    Moving a file into a destination whose leading directories are not all
     +    present, real directories is only diagnosed later at rename(2), and for
     +    a symlinked component is not diagnosed at all.
      
     -    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.
     +    Three cases reach rename(2) unchecked today:
      
     -    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.
     +      - A leading directory is missing: rename(2) fails with ENOENT,
     +        reported against the source (misleading), and "git mv -n" does not
     +        detect it since the dry run never reaches the syscall.
     +
     +      - A leading component is a non-directory ("git mv x a/b" with 'a' a
     +        file): rename(2) fails with ENOTDIR, again only at the syscall.
     +
     +      - A leading component is a symbolic link: "git mv" follows it. Since
     +        Git tracks symlinks, the destination is really occupied by a
     +        tracked object, and following it is wrong regardless of the link
     +        target. The move is done on disk at the resolved location while the
     +        index records the literal path, leaving the index describing a
     +        worktree that does not exist. A later "git add" can reconcile it,
     +        but "git mv" alone has already corrupted the state.
     +
     +    Detect all three in the checking phase. Reject a destination that goes
     +    through a symlink with has_symlink_leading_path(), which uses lstat()
     +    and never follows the link, so the refusal is independent of the
     +    target. Then lstat() the leading directory: report "destination
     +    directory does not exist" for ENOENT/ENOTDIR and "destination is not a
     +    directory" for a non-directory. Other errors fall through to rename().
     +    Guard the directory check with the same condition under which rename(2)
     +    runs, so directory moves and sparse/out-of-cone destinations are not
     +    flagged incorrectly.
     +
     +    This changes behavior: a move through a tracked symlink that previously
     +    "succeeded" while corrupting the index is now refused. The other two
     +    cases only change when the failure is diagnosed.
      
          Signed-off-by: Lucas Zamboni Orioli <lucaszam0@gmail.com>
      
       ## builtin/mv.c ##
     +@@
     + #include "string-list.h"
     + #include "parse-options.h"
     + #include "read-cache-ll.h"
     ++#include "symlinks.h"
     + 
     + #include "setup.h"
     + #include "strvec.h"
      @@ builtin/mv.c: dir_check:
     + 			bad = _("destination directory does not exist");
       			goto act_on_entry;
       		}
     - 
     ++		if (has_symlink_leading_path(dst, strlen(dst))) {
     ++			bad = _("destination is beyond a symbolic link");
     ++			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))) {
     ++		    !(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 (lstat(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))
     ++				} 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))) {
      
       ## t/t7001-mv.sh ##
      @@ t/t7001-mv.sh: test_expect_success 'clean up' '
       	git reset --hard
       '
       
     -+test_expect_success 'moving to a non-existent path component in the destination' '
     ++test_expect_success 'moving to a non-existent directory' '
      +	git reset --hard &&
     -+	mkdir -p from &&
     ++	rm -rf from && mkdir 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' '
     ++test_expect_success 'moving to a destination with a file as a leading path component' '
      +	git reset --hard &&
     -+	mkdir -p from &&
     ++	rm -rf from && mkdir from &&
      +	echo contents >from/file &&
      +	echo blocker >not-dir &&
      +	git add from/file &&
     @@ t/t7001-mv.sh: test_expect_success 'clean up' '
      +	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' '
     ++test_expect_success SYMLINKS 'moving to a destination beyond a symlink' '
      +	git reset --hard &&
     -+	mkdir -p from &&
     ++	rm -rf from regular-dir link-to-dir &&
     ++	mkdir from regular-dir &&
      +	echo contents >from/file &&
     -+	echo target >regular &&
     -+	ln -s regular link-to-file &&
     ++	ln -s regular-dir link-to-dir &&
      +	git add from/file &&
     -+	test_must_fail git mv from/file link-to-file/file 2>actual &&
     -+	test_grep "not a directory" actual
     ++	test_must_fail git mv from/file link-to-dir/file 2>actual &&
     ++	test_grep "destination is beyond a symbolic link" actual
      +'
      +
     -+test_expect_success SYMLINKS 'moving to a destination with a symlink to a directory' '
     ++test_expect_success SYMLINKS 'moving to a destination with a symlink as an intermediate component' '
      +	git reset --hard &&
     -+	mkdir -p from realdir &&
     ++	rm -rf from && mkdir -p from/real/inner &&
      +	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
     ++	ln -s real from/link &&
     ++	git add from/file from/link &&
     ++	test_must_fail git mv from/file from/link/inner/dst 2>actual &&
     ++	test_grep "destination is beyond a symbolic link" actual
     ++'
     ++
     ++test_expect_success SYMLINKS 'refuses to overwrite a symlink at the destination' '
     ++	git reset --hard &&
     ++	rm -rf from && mkdir from &&
     ++	echo contents >from/file &&
     ++	ln -s target from/link &&
     ++	git add from/file from/link &&
     ++	test_must_fail git mv from/file from/link 2>actual &&
     ++	test_grep "destination exists" actual
     ++'
     ++
     ++test_expect_success SYMLINKS 'mv through a symlinked leading path does not touch the index' '
     ++	git reset --hard &&
     ++	rm -rf from && mkdir from &&
     ++	echo contents >from/src &&
     ++	ln -s . from/link &&
     ++	git add from/src from/link &&
     ++	git commit -m "setup symlink case" &&
     ++	git ls-files --stage >expect.index &&
     ++	test_must_fail git mv from/src from/link/real/dst 2>actual &&
     ++	test_grep "destination is beyond a symbolic link" actual &&
     ++	git ls-files --stage >actual.index &&
     ++	test_cmp expect.index actual.index
     ++'
     ++
     ++test_expect_success SYMLINKS 'mv -f does not follow a symlinked leading path' '
     ++	git reset --hard &&
     ++	rm -rf from && mkdir from &&
     ++	echo contents >from/src &&
     ++	ln -s file from/link &&
     ++	git add from/src from/link &&
     ++	test_must_fail git mv -f from/src from/link/dst 2>actual &&
     ++	test_grep "destination is beyond a symbolic link" actual
      +'
      +
      +test_expect_success 'mv --dry-run detects non-existent destination parent directory' '
      +	git reset --hard &&
     -+	mkdir -p from &&
     -+	echo content >from/file &&
     ++	rm -rf from && mkdir from &&
     ++	echo contents >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