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
+'Lucas Zamboni Orioli via GitGitGadget <gitgitgadget@gmail.com>