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

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

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 &&

-- 
gitgitgadget