From: Lucas Zamboni Orioli via GitGitGadget Date: Sun, 26 Jul 2026 20:17:49 GMT Subject: [PATCH v4 0/2] mv: report missing destination leading directory Message-ID: In-Reply-To: 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 ## 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 ## 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