[PATCH] mv: report missing destination leading directory

[PATCH] mv: report missing destination leading directory

[Lucas Zamboni Orioli via GitGitGadget]

From: Lucas Zamboni Orioli <lucaszam0@gmail.com>

When moving a file to a destination whose leading directory does not
exist, "git mv" fails at the rename(2) syscall with ENOENT. Because
the error is reported via die_errno() using only the source path:

    fatal: renaming 'src' failed: No such file or directory

the message misleadingly blames the source, even though it is the
destination's parent directory that is missing. A user who runs

    git mv a/file b/does-not-exist/file

is told the problem is with 'a/file', which exists, giving no hint
that 'b/does-not-exist/' needs to be created first.

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 rename(2). As a result "git mv -n" also fails to detect the
problem, since the dry run never reaches the syscall and reports a
move that would not actually succeed.

Detect this during the checking phase instead: 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.

This gives a clear message and lets "git mv -n" report the failure.

Signed-off-by: Lucas Zamboni Orioli <lucaszam0@gmail.com>
---
    mv: report missing destination leading directory

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2356%2FZamboniL%2Fmv-detect-non-existing-target-folder-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2356/ZamboniL/mv-detect-non-existing-target-folder-v1
Pull-Request: https://github.com/git/git/pull/2356

 builtin/mv.c  | 21 +++++++++++++++++++++
 t/t7001-mv.sh | 14 ++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/builtin/mv.c b/builtin/mv.c
index e03823370c..a95531f0b2 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -444,6 +444,27 @@ dir_check:
 			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))) {
+				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 (ignore_sparse &&
 		    (dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) &&
 		    index_entry_exists(the_repository->index, dst, strlen(dst))) {
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 920479e925..8a45997b33 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -114,6 +114,20 @@ test_expect_success 'clean up' '
 	git reset --hard
 '
 
+test_expect_success 'moving to non-existent destination parent directory' '
+	git reset --hard &&
+	mkdir -p 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 'mv --dry-run detects non-existent destination parent directory' '
+	test_must_fail git mv -n from/file no-such-dir/file 2>actual &&
+	test_grep "destination directory does not exist" actual
+'
+
 test_expect_success 'moving to existing untracked target with trailing slash' '
 	mkdir path1 &&
 	git mv path0/ path1/ &&

base-commit: 55526a18268bbc1ddaf8a6b7850c33d984eac9e9
-- 
gitgitgadget

Re: [PATCH] mv: report missing destination leading directory

[Ben Knoble]

> Le 15 juil. 2026 à 10:51, Lucas Zamboni Orioli via GitGitGadget <gitgitgadget@gmail.com> a écrit :
> 
> From: Lucas Zamboni Orioli <lucaszam0@gmail.com>
> 
> When moving a file to a destination whose leading directory does not
> exist, "git mv" fails at the rename(2) syscall with ENOENT. Because
> the error is reported via die_errno() using only the source path:
> 
>    fatal: renaming 'src' failed: No such file or directory
> 
> the message misleadingly blames the source, even though it is the
> destination's parent directory that is missing. A user who runs
> 
>    git mv a/file b/does-not-exist/file
> 
> is told the problem is with 'a/file', which exists, giving no hint
> that 'b/does-not-exist/' needs to be created first.
> 
> 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 rename(2). As a result "git mv -n" also fails to detect the
> problem, since the dry run never reaches the syscall and reports a
> move that would not actually succeed.
> 
> Detect this during the checking phase instead: 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.

I suppose this still allows a TOCTOU issue where the check succeeds and (with lucky timing) the destination then disappears?

In that case, I think a worthwhile additional change would also be for the error message to diagnose which file is missing (or at least include both source and destination).

Now, without checking I somehow doubt whether rename(2) tells us which entry is missing. Worse, if we check afterwards, we could have a « TOUTOC » :p where the entry reappears to confuse the error diagnosis.

So perhaps

    fatal: renaming A -> B failed: no such file or directory

taking some inspiration from the -i modes of cp, mv?

> This gives a clear message and lets "git mv -n" report the failure.
> 
> Signed-off-by: Lucas Zamboni Orioli <lucaszam0@gmail.com>
> ---
>    mv: report missing destination leading directory
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2356%2FZamboniL%2Fmv-detect-non-existing-target-folder-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2356/ZamboniL/mv-detect-non-existing-target-folder-v1
> Pull-Request: https://github.com/git/git/pull/2356
> 
> builtin/mv.c  | 21 +++++++++++++++++++++
> t/t7001-mv.sh | 14 ++++++++++++++
> 2 files changed, 35 insertions(+)
> 
> diff --git a/builtin/mv.c b/builtin/mv.c
> index e03823370c..a95531f0b2 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -444,6 +444,27 @@ dir_check:
>            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))) {
> +                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 (ignore_sparse &&
>            (dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) &&
>            index_entry_exists(the_repository->index, dst, strlen(dst))) {
> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
> index 920479e925..8a45997b33 100755
> --- a/t/t7001-mv.sh
> +++ b/t/t7001-mv.sh
> @@ -114,6 +114,20 @@ test_expect_success 'clean up' '
>    git reset --hard
> '
> 
> +test_expect_success 'moving to non-existent destination parent directory' '
> +    git reset --hard &&
> +    mkdir -p 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 'mv --dry-run detects non-existent destination parent directory' '
> +    test_must_fail git mv -n from/file no-such-dir/file 2>actual &&
> +    test_grep "destination directory does not exist" actual
> +'
> +
> test_expect_success 'moving to existing untracked target with trailing slash' '
>    mkdir path1 &&
>    git mv path0/ path1/ &&
> 
> base-commit: 55526a18268bbc1ddaf8a6b7850c33d984eac9e9
> --
> gitgitgadget
> 

Re: [PATCH] mv: report missing destination leading directory

[Lucas Zamboni Orioli]

Em qua., 15 de jul. de 2026 às 13:50, Ben Knoble
<ben.knoble@gmail.com> escreveu:
> I suppose this still allows a TOCTOU issue where the check succeeds and (with lucky timing) the destination then disappears?

Thank you for the feedback, also great catch, this does end up with a
TOCTOU issue.

>
> In that case, I think a worthwhile additional change would also be for the error message to diagnose which file is missing (or at least include both source and destination).
>
> Now, without checking I somehow doubt whether rename(2) tells us which entry is missing. Worse, if we check afterwards, we could have a « TOUTOC » :p where the entry reappears to confuse the error diagnosis.

I think your suggestion of including both source and destination in the
error message is a good solution, I verified rename(2) just in case and
it does not provide the information about which file is missing.

So what I'm thinking of doing is change the error message to

        fatal: renaming 'source/file' to 'destination/file' failed: No
        such file or directory

'%s' to '%s' seems to be more in the pattern of other git messages
instead of the cp arrow style.

So for v2 I'll split this into two commits:

        1. mv: name both source and destination when rename fails
                (the die_errno change is race-free and always applicable)
        2. mv: check for missing destination directory before renaming
                (the checking-phase/dry-run detection)

The first stands on its own even if the second is dropped, so I'll
order it first.

Since this introduces a new message I'll leave the po/ files to the
l10n team, the new message adds one string and the early check reuses
the existing
'destination directory does not exist' one.

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

[Lucas Zamboni Orioli via GitGitGadget]

Changes since v1:

 * 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  | 23 ++++++++++++++++++++++-
 t/t7001-mv.sh | 14 ++++++++++++++
 2 files changed, 36 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-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2356/ZamboniL/mv-detect-non-existing-target-folder-v2
Pull-Request: https://github.com/git/git/pull/2356

Range-diff vs v1:

 -:  ---------- > 1:  0d67da588b mv: name both source and destination when rename fails
 1:  692f44456f ! 2:  1a790e0016 mv: report missing destination leading directory
     @@ Metadata
      Author: Lucas Zamboni Orioli <lucaszam0@gmail.com>
      
       ## Commit message ##
     -    mv: report missing destination leading directory
     +    mv: check for missing destination directory before renaming
      
     -    When moving a file to a destination whose leading directory does not
     -    exist, "git mv" fails at the rename(2) syscall with ENOENT. Because
     -    the error is reported via die_errno() using only the source path:
     +    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. 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.
      
     -        fatal: renaming 'src' failed: No such file or directory
     +    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.
      
     -    the message misleadingly blames the source, even though it is the
     -    destination's parent directory that is missing. A user who runs
     +    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.
      
     -        git mv a/file b/does-not-exist/file
     -
     -    is told the problem is with 'a/file', which exists, giving no hint
     -    that 'b/does-not-exist/' needs to be created first.
     -
     -    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 rename(2). As a result "git mv -n" also fails to detect the
     -    problem, since the dry run never reaches the syscall and reports a
     -    move that would not actually succeed.
     -
     -    Detect this during the checking phase instead: 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.
     -
     -    This gives a clear message and lets "git mv -n" report the failure.
     +    Add tests covering both the error path and the dry-run detection.
      
          Signed-off-by: Lucas Zamboni Orioli <lucaszam0@gmail.com>
      

-- 
gitgitgadget

[PATCH v2 1/2] mv: name both source and destination when rename fails

[Lucas Zamboni Orioli via GitGitGadget]

From: Lucas Zamboni Orioli <lucaszam0@gmail.com>

When "git mv" fails at the rename(2) syscall, the error is reported
with die_errno() using only the source path:

    fatal: renaming 'src' failed: No such file or directory

rename(2) returns ENOENT both when the source does not exist and when
a directory component of the destination does not exist, and errno
does not distinguish the two. Reporting only the source therefore
misleads the user in the latter case: for

    git mv a/file b/no-such-dir/file

the message blames 'a/file', which exists, and gives no hint that
'b/no-such-dir/' is the missing part.

Inspecting the paths again after the failure to determine which one is
at fault would be racy, since either could appear or disappear between
the rename(2) and the follow-up check. Instead, simply name both the
source and the destination in the message and let the reader see which
one is wrong:

    fatal: renaming 'a/file' to 'b/no-such-dir/file' failed:
    No such file or directory

Signed-off-by: Lucas Zamboni Orioli <lucaszam0@gmail.com>
---
 builtin/mv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index a82fc97a19..35e504484a 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -549,7 +549,7 @@ remove_entry:
 		    rename(src, dst) < 0) {
 			if (ignore_errors)
 				continue;
-			die_errno(_("renaming '%s' failed"), src);
+			die_errno(_("renaming '%s' to '%s' failed"), src, dst);
 		}
 		if (submodule_gitfiles[i]) {
 			if (!update_path_in_gitmodules(src, dst))
-- 
gitgitgadget

[PATCH v2 2/2] mv: check for missing destination directory before renaming

[Lucas Zamboni Orioli via GitGitGadget]

From: Lucas Zamboni Orioli <lucaszam0@gmail.com>

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

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.

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.

Signed-off-by: Lucas Zamboni Orioli <lucaszam0@gmail.com>
---
 builtin/mv.c  | 21 +++++++++++++++++++++
 t/t7001-mv.sh | 14 ++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/builtin/mv.c b/builtin/mv.c
index 35e504484a..eb59fe0f31 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -444,6 +444,27 @@ dir_check:
 			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))) {
+				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 (ignore_sparse &&
 		    (dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) &&
 		    index_entry_exists(the_repository->index, dst, strlen(dst))) {
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 7cf4aa5ba1..2d8a98d8b0 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -114,6 +114,20 @@ test_expect_success 'clean up' '
 	git reset --hard
 '
 
+test_expect_success 'moving to non-existent destination parent directory' '
+	git reset --hard &&
+	mkdir -p 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 'mv --dry-run detects non-existent destination parent directory' '
+	test_must_fail git mv -n from/file no-such-dir/file 2>actual &&
+	test_grep "destination directory does not exist" actual
+'
+
 test_expect_success 'moving to existing untracked target with trailing slash' '
 	mkdir path1 &&
 	git mv path0/ path1/ &&
-- 
gitgitgadget

Re: [PATCH v2 1/2] mv: name both source and destination when rename fails

[Junio C Hamano]

"Lucas Zamboni Orioli via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Lucas Zamboni Orioli <lucaszam0@gmail.com>
>
> When "git mv" fails at the rename(2) syscall, the error is reported
> with die_errno() using only the source path:
>
>     fatal: renaming 'src' failed: No such file or directory
>
> rename(2) returns ENOENT both when the source does not exist and when
> a directory component of the destination does not exist, and errno
> does not distinguish the two. Reporting only the source therefore
> misleads the user in the latter case: for
>
>     git mv a/file b/no-such-dir/file
>
> the message blames 'a/file', which exists, and gives no hint that
> 'b/no-such-dir/' is the missing part.
>
> Inspecting the paths again after the failure to determine which one is
> at fault would be racy, since either could appear or disappear between
> the rename(2) and the follow-up check. Instead, simply name both the
> source and the destination in the message and let the reader see which
> one is wrong:
>
>     fatal: renaming 'a/file' to 'b/no-such-dir/file' failed:
>     No such file or directory
>
> Signed-off-by: Lucas Zamboni Orioli <lucaszam0@gmail.com>
> ---
>  builtin/mv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index a82fc97a19..35e504484a 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -549,7 +549,7 @@ remove_entry:
>  		    rename(src, dst) < 0) {
>  			if (ignore_errors)
>  				continue;
> -			die_errno(_("renaming '%s' failed"), src);
> +			die_errno(_("renaming '%s' to '%s' failed"), src, dst);
>  		}
>  		if (submodule_gitfiles[i]) {
>  			if (!update_path_in_gitmodules(src, dst))

Makes sense.

Re: [PATCH v2 2/2] mv: check for missing destination directory before renaming

[Junio C Hamano]

"Lucas Zamboni Orioli via GitGitGadget" <gitgitgadget@gmail.com>
writes:

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

If "can still disappear" is because we are not taking into account a
move that we are scheduled to make, then that is not very nice, but
as long as it is *not* our making (in other words, somebody else may
actively interferring with the mv we are trying to perform), I think
this is OK.  It is the best we can do.

> Add tests covering both the error path and the dry-run detection.
>
> Signed-off-by: Lucas Zamboni Orioli <lucaszam0@gmail.com>
> ---
>  builtin/mv.c  | 21 +++++++++++++++++++++
>  t/t7001-mv.sh | 14 ++++++++++++++
>  2 files changed, 35 insertions(+)
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 35e504484a..eb59fe0f31 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -444,6 +444,27 @@ dir_check:
>  			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))) {
> +				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);
> +		}

Horrible.  Please fix this overly deep indentation.

> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
> index 7cf4aa5ba1..2d8a98d8b0 100755
> --- a/t/t7001-mv.sh
> +++ b/t/t7001-mv.sh
> @@ -114,6 +114,20 @@ test_expect_success 'clean up' '
>  	git reset --hard
>  '
>  
> +test_expect_success 'moving to non-existent destination parent directory' '
> +	git reset --hard &&
> +	mkdir -p 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 'mv --dry-run detects non-existent destination parent directory' '
> +	test_must_fail git mv -n from/file no-such-dir/file 2>actual &&
> +	test_grep "destination directory does not exist" actual
> +'
> +
>  test_expect_success 'moving to existing untracked target with trailing slash' '
>  	mkdir path1 &&
>  	git mv path0/ path1/ &&

Re: [PATCH v2 2/2] mv: check for missing destination directory before renaming

[Junio C Hamano]

"Lucas Zamboni Orioli via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> +		/*
> +		* If we are going to move SRC to DST on disk, DST's leading
> +		* directories must already exist.
> +		*/

	/*
	 * Our multi-line comment is formatted like this.  The
	 * asterisks align vertically.
	 */

> +		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 (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);
> +		}

lstat() can succeed and 'dir_st' may indicate something other than a
directory (for example, a symbolic link or a regular file).
Alternatively, it can fail with ENOTDIR when, for example, 'dst_dir'
is 'a/b/c' and 'a/b' is a file rather than a directory.

Both cases will cause 'git mv' into a path assumed to be a directory
to fail.  Shouldn't we handle these conditions as well?

Re: [PATCH v2 2/2] mv: check for missing destination directory before renaming

[Lucas Zamboni Orioli]

> lstat() can succeed and 'dir_st' may indicate something other than a
> directory (for example, a symbolic link or a regular file).
> Alternatively, it can fail with ENOTDIR when, for example, 'dst_dir'
> is 'a/b/c' and 'a/b' is a file rather than a directory.
>
> Both cases will cause 'git mv' into a path assumed to be a directory
> to fail.  Shouldn't we handle these conditions as well?

Yes, agreed, both should be handled. For v3 I switched from lstat()
to stat() so that the check follows symlinks the same way rename()
does, and I handle the non-directory cases:

stat() failing with ENOENT or ENOTDIR (missing directory, or a
leading path component that is a file) reports "destination
directory does not exist".

stat() succeeding on something that is not a directory reports
"destination is not a directory".

Other stat() errors fall through to rename(), which reports them as before.

For the messages I used the existing "destination directory does not
exist" string for the missing case and added one new string,
"destination is not a directory", for the non-directory case. I'm
happy to collapse these into a single message instead if you'd prefer
to avoid the extra translatable string, let me know.

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

[Lucas Zamboni Orioli via GitGitGadget]

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
      +'

-- 
gitgitgadget

[PATCH v3 1/2] mv: name both source and destination when rename fails

[Lucas Zamboni Orioli via GitGitGadget]

From: Lucas Zamboni Orioli <lucaszam0@gmail.com>

When "git mv" fails at the rename(2) syscall, the error is reported
with die_errno() using only the source path:

    fatal: renaming 'src' failed: No such file or directory

rename(2) returns ENOENT both when the source does not exist and when
a directory component of the destination does not exist, and errno
does not distinguish the two. Reporting only the source therefore
misleads the user in the latter case: for

    git mv a/file b/no-such-dir/file

the message blames 'a/file', which exists, and gives no hint that
'b/no-such-dir/' is the missing part.

Inspecting the paths again after the failure to determine which one is
at fault would be racy, since either could appear or disappear between
the rename(2) and the follow-up check. Instead, simply name both the
source and the destination in the message and let the reader see which
one is wrong:

    fatal: renaming 'a/file' to 'b/no-such-dir/file' failed:
    No such file or directory

Signed-off-by: Lucas Zamboni Orioli <lucaszam0@gmail.com>
---
 builtin/mv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index a82fc97a19..35e504484a 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -549,7 +549,7 @@ remove_entry:
 		    rename(src, dst) < 0) {
 			if (ignore_errors)
 				continue;
-			die_errno(_("renaming '%s' failed"), src);
+			die_errno(_("renaming '%s' to '%s' failed"), src, dst);
 		}
 		if (submodule_gitfiles[i]) {
 			if (!update_path_in_gitmodules(src, dst))
-- 
gitgitgadget

[PATCH v3 2/2] mv: check for missing destination directory before renaming

[Lucas Zamboni Orioli via GitGitGadget]

From: Lucas Zamboni Orioli <lucaszam0@gmail.com>

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.

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.

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  | 24 ++++++++++++++++++++++++
 t/t7001-mv.sh | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+)

diff --git a/builtin/mv.c b/builtin/mv.c
index 35e504484a..08e27484f2 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -444,6 +444,30 @@ dir_check:
 			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))) {
+			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 (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 &&
 		    (dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) &&
 		    index_entry_exists(the_repository->index, dst, strlen(dst))) {
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 7cf4aa5ba1..c878fb92a8 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -114,6 +114,55 @@ test_expect_success 'clean up' '
 	git reset --hard
 '
 
+test_expect_success 'moving to a non-existent path component in the destination' '
+	git reset --hard &&
+	mkdir -p 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' '
+	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
+'
+
 test_expect_success 'moving to existing untracked target with trailing slash' '
 	mkdir path1 &&
 	git mv path0/ path1/ &&
-- 
gitgitgadget

Re: [PATCH v2 2/2] mv: check for missing destination directory before renaming

[Junio C Hamano]

Lucas Zamboni Orioli <lucaszam0@gmail.com> writes:

>> lstat() can succeed and 'dir_st' may indicate something other than a
>> directory (for example, a symbolic link or a regular file).
>> Alternatively, it can fail with ENOTDIR when, for example, 'dst_dir'
>> is 'a/b/c' and 'a/b' is a file rather than a directory.
>>
>> Both cases will cause 'git mv' into a path assumed to be a directory
>> to fail.  Shouldn't we handle these conditions as well?
>
> Yes, agreed, both should be handled. For v3 I switched from lstat()
> to stat() so that the check follows symlinks the same way rename()
> does, and I handle the non-directory cases:

Generally, a symbolic link in a Git-managed working tree should
not be followed.  Following a symbolic link would mean that
'git mv x y' could move 'x' outside the working tree if 'y' is
a tracked symbolic link pointing to a directory outside the
working tree.  "git apply" for example avoids being fooled by a
symbolic link for the same reason, for example.

I doubt that using stat() instead of lstat() is the right
approach.  Doing so essentially amounts to ignoring the
presence of symbolic links.