Changes in v5:
* extracted the shared "will this move rename on disk?" condition into a
needs_worktree_rename() helper used by both the new leading-directory
check and the actual rename(), so the two cannot drift, per Junio C
Hamano
* allocate the dirname copy only when the destination has a slash
* reworded the opening of the commit message for clarity, per Junio C
Hamano
* added tests: moving into an existing directory (destination is normalized
to a full path), and moving to a bare filename in the cwd (no leading
directory to check)
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 | 47 +++++++++++++++++++++--
t/t7001-mv.sh | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 146 insertions(+), 3 deletions(-)
base-commit: 9a0c4701dcd5725c4184599322b52933ff5005ca
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2356%2FZamboniL%2Fmv-detect-non-existing-target-folder-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2356/ZamboniL/mv-detect-non-existing-target-folder-v5
Pull-Request: https://github.com/git/git/pull/2356
Range-diff vs v4:
1: 0d67da588b = 1: 0d67da588b mv: name both source and destination when rename fails
2: 6b72efb413 ! 2: 6c2909e609 mv: reject a destination whose leading path is missing or a symlink
@@ Metadata
## Commit message ##
mv: reject a destination whose leading path is missing or a symlink
- 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.
+ When moving a file, if any leading directory in the destination path
+ is missing or is not a real directory, the problem is detected only
+ later when rename() is called. Furthermore, if a leading directory
+ component is a symbolic link, the issue is not detected at all.
Three cases reach rename(2) unchecked today:
@@ builtin/mv.c
#include "setup.h"
#include "strvec.h"
+@@ builtin/mv.c: enum update_mode {
+ MOVE_VIA_PARENT_DIR = (1 << 5),
+ };
+
++static int needs_worktree_rename(enum update_mode mode, enum update_mode dst_mode)
++{
++ return !(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
++ !(dst_mode & (SKIP_WORKTREE_DIR | SPARSE));
++}
++
+ #define DUP_BASENAME 1
+ #define KEEP_TRAILING_SLASH 2
+
@@ builtin/mv.c: dir_check:
bad = _("destination directory does not exist");
goto act_on_entry;
@@ builtin/mv.c: dir_check:
+ * 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, '/');
++ if (needs_worktree_rename(modes[i], dst_mode)) {
++ const char *slash_ = strrchr(dst, '/');
+
-+ if (slash) {
++ if (slash_) {
+ struct stat dir_st;
++ char *dst_dir = xstrdup(dst);
++ char *slash = &dst_dir[slash_ - dst];
+
+ *slash = '\0';
+ if (lstat(dst_dir, &dir_st) < 0) {
@@ builtin/mv.c: dir_check:
+ } else if (!S_ISDIR(dir_st.st_mode)) {
+ bad = _("destination is not a directory");
+ }
++
++ free(dst_dir);
+ }
-+ free(dst_dir);
+
+ if (bad)
+ goto act_on_entry;
@@ builtin/mv.c: dir_check:
if (ignore_sparse &&
(dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) &&
+@@ builtin/mv.c: remove_entry:
+ printf(_("Renaming %s to %s\n"), src, dst);
+ if (show_only)
+ continue;
+- if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
+- !(dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) &&
++ if (needs_worktree_rename(mode, dst_mode) &&
+ rename(src, dst) < 0) {
+ if (ignore_errors)
+ continue;
## t/t7001-mv.sh ##
@@ t/t7001-mv.sh: test_expect_success 'clean up' '
git reset --hard
'
++test_expect_success 'moving file to directory without trailing slash' '
++ git reset --hard HEAD &&
++ rm -rf file.txt target && mkdir target &&
++ echo content > file.txt &&
++ git add file.txt &&
++ git mv file.txt target &&
++ test_path_is_file target/file.txt
++'
++
++test_expect_success 'moving file to a bare filename in the cwd' '
++ git reset --hard &&
++ rm -rf from dest.txt &&
++ mkdir from &&
++ echo content >from/file &&
++ git add from/file &&
++ git mv from/file dest.txt &&
++ test_path_is_file dest.txt
++'
++
+test_expect_success 'moving to a non-existent directory' '
+ git reset --hard &&
+ rm -rf from && mkdir from &&Lucas Zamboni Orioli via GitGitGadget <gitgitgadget@gmail.com>