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" actualLucas Zamboni Orioli via GitGitGadget <gitgitgadget@gmail.com>