All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mv: report missing destination leading directory
@ 2026-07-15 14:32 Lucas Zamboni Orioli via GitGitGadget
  2026-07-15 16:46 ` Ben Knoble
  2026-07-23 13:13 ` [PATCH v2 0/2] " Lucas Zamboni Orioli via GitGitGadget
  0 siblings, 2 replies; 21+ messages in thread
From: Lucas Zamboni Orioli via GitGitGadget @ 2026-07-15 14:32 UTC (permalink / raw)
  To: git; +Cc: Lucas Zamboni Orioli, Lucas Zamboni Orioli

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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH] mv: report missing destination leading directory
  2026-07-15 14:32 [PATCH] mv: report missing destination leading directory Lucas Zamboni Orioli via GitGitGadget
@ 2026-07-15 16:46 ` Ben Knoble
  2026-07-22 21:32   ` Lucas Zamboni Orioli
  2026-07-23 13:13 ` [PATCH v2 0/2] " Lucas Zamboni Orioli via GitGitGadget
  1 sibling, 1 reply; 21+ messages in thread
From: Ben Knoble @ 2026-07-15 16:46 UTC (permalink / raw)
  To: Lucas Zamboni Orioli via GitGitGadget; +Cc: git, Lucas Zamboni Orioli


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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] mv: report missing destination leading directory
  2026-07-15 16:46 ` Ben Knoble
@ 2026-07-22 21:32   ` Lucas Zamboni Orioli
  0 siblings, 0 replies; 21+ messages in thread
From: Lucas Zamboni Orioli @ 2026-07-22 21:32 UTC (permalink / raw)
  To: Ben Knoble; +Cc: Lucas Zamboni Orioli via GitGitGadget, git

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 0/2] mv: report missing destination leading directory
  2026-07-15 14:32 [PATCH] mv: report missing destination leading directory Lucas Zamboni Orioli via GitGitGadget
  2026-07-15 16:46 ` Ben Knoble
@ 2026-07-23 13:13 ` Lucas Zamboni Orioli via GitGitGadget
  2026-07-23 13:13   ` [PATCH v2 1/2] mv: name both source and destination when rename fails Lucas Zamboni Orioli via GitGitGadget
                     ` (2 more replies)
  1 sibling, 3 replies; 21+ messages in thread
From: Lucas Zamboni Orioli via GitGitGadget @ 2026-07-23 13:13 UTC (permalink / raw)
  To: git; +Cc: Ben Knoble, Lucas Zamboni Orioli

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 1/2] mv: name both source and destination when rename fails
  2026-07-23 13:13 ` [PATCH v2 0/2] " Lucas Zamboni Orioli via GitGitGadget
@ 2026-07-23 13:13   ` Lucas Zamboni Orioli via GitGitGadget
  2026-07-23 17:36     ` Junio C Hamano
  2026-07-23 13:13   ` [PATCH v2 2/2] mv: check for missing destination directory before renaming Lucas Zamboni Orioli via GitGitGadget
  2026-07-23 21:40   ` [PATCH v3 0/2] mv: report missing destination leading directory Lucas Zamboni Orioli via GitGitGadget
  2 siblings, 1 reply; 21+ messages in thread
From: Lucas Zamboni Orioli via GitGitGadget @ 2026-07-23 13:13 UTC (permalink / raw)
  To: git; +Cc: Ben Knoble, Lucas Zamboni Orioli, Lucas Zamboni Orioli

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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v2 2/2] mv: check for missing destination directory before renaming
  2026-07-23 13:13 ` [PATCH v2 0/2] " Lucas Zamboni Orioli via GitGitGadget
  2026-07-23 13:13   ` [PATCH v2 1/2] mv: name both source and destination when rename fails Lucas Zamboni Orioli via GitGitGadget
@ 2026-07-23 13:13   ` Lucas Zamboni Orioli via GitGitGadget
  2026-07-23 17:42     ` Junio C Hamano
  2026-07-23 18:30     ` Junio C Hamano
  2026-07-23 21:40   ` [PATCH v3 0/2] mv: report missing destination leading directory Lucas Zamboni Orioli via GitGitGadget
  2 siblings, 2 replies; 21+ messages in thread
From: Lucas Zamboni Orioli via GitGitGadget @ 2026-07-23 13:13 UTC (permalink / raw)
  To: git; +Cc: Ben Knoble, Lucas Zamboni Orioli, Lucas Zamboni Orioli

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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/2] mv: name both source and destination when rename fails
  2026-07-23 13:13   ` [PATCH v2 1/2] mv: name both source and destination when rename fails Lucas Zamboni Orioli via GitGitGadget
@ 2026-07-23 17:36     ` Junio C Hamano
  0 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2026-07-23 17:36 UTC (permalink / raw)
  To: Lucas Zamboni Orioli via GitGitGadget
  Cc: git, Ben Knoble, Lucas Zamboni Orioli

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/2] mv: check for missing destination directory before renaming
  2026-07-23 13:13   ` [PATCH v2 2/2] mv: check for missing destination directory before renaming Lucas Zamboni Orioli via GitGitGadget
@ 2026-07-23 17:42     ` Junio C Hamano
  2026-07-23 18:30     ` Junio C Hamano
  1 sibling, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2026-07-23 17:42 UTC (permalink / raw)
  To: Lucas Zamboni Orioli via GitGitGadget
  Cc: git, Ben Knoble, Lucas Zamboni Orioli

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/2] mv: check for missing destination directory before renaming
  2026-07-23 13:13   ` [PATCH v2 2/2] mv: check for missing destination directory before renaming Lucas Zamboni Orioli via GitGitGadget
  2026-07-23 17:42     ` Junio C Hamano
@ 2026-07-23 18:30     ` Junio C Hamano
  2026-07-23 21:38       ` Lucas Zamboni Orioli
  1 sibling, 1 reply; 21+ messages in thread
From: Junio C Hamano @ 2026-07-23 18:30 UTC (permalink / raw)
  To: Lucas Zamboni Orioli via GitGitGadget
  Cc: git, Ben Knoble, Lucas Zamboni Orioli

"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?

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/2] mv: check for missing destination directory before renaming
  2026-07-23 18:30     ` Junio C Hamano
@ 2026-07-23 21:38       ` Lucas Zamboni Orioli
  2026-07-23 22:40         ` Junio C Hamano
  2026-07-23 23:28         ` Junio C Hamano
  0 siblings, 2 replies; 21+ messages in thread
From: Lucas Zamboni Orioli @ 2026-07-23 21:38 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Lucas Zamboni Orioli via GitGitGadget, git, Ben Knoble

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v3 0/2] mv: report missing destination leading directory
  2026-07-23 13:13 ` [PATCH v2 0/2] " Lucas Zamboni Orioli via GitGitGadget
  2026-07-23 13:13   ` [PATCH v2 1/2] mv: name both source and destination when rename fails Lucas Zamboni Orioli via GitGitGadget
  2026-07-23 13:13   ` [PATCH v2 2/2] mv: check for missing destination directory before renaming Lucas Zamboni Orioli via GitGitGadget
@ 2026-07-23 21:40   ` Lucas Zamboni Orioli via GitGitGadget
  2026-07-23 21:40     ` [PATCH v3 1/2] mv: name both source and destination when rename fails Lucas Zamboni Orioli via GitGitGadget
                       ` (2 more replies)
  2 siblings, 3 replies; 21+ messages in thread
From: Lucas Zamboni Orioli via GitGitGadget @ 2026-07-23 21:40 UTC (permalink / raw)
  To: git; +Cc: Ben Knoble, Lucas Zamboni Orioli

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v3 1/2] mv: name both source and destination when rename fails
  2026-07-23 21:40   ` [PATCH v3 0/2] mv: report missing destination leading directory Lucas Zamboni Orioli via GitGitGadget
@ 2026-07-23 21:40     ` Lucas Zamboni Orioli via GitGitGadget
  2026-07-23 21:40     ` [PATCH v3 2/2] mv: check for missing destination directory before renaming Lucas Zamboni Orioli via GitGitGadget
  2026-07-26 20:17     ` [PATCH v4 0/2] mv: report missing destination leading directory Lucas Zamboni Orioli via GitGitGadget
  2 siblings, 0 replies; 21+ messages in thread
From: Lucas Zamboni Orioli via GitGitGadget @ 2026-07-23 21:40 UTC (permalink / raw)
  To: git; +Cc: Ben Knoble, Lucas Zamboni Orioli, Lucas Zamboni Orioli

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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v3 2/2] mv: check for missing destination directory before renaming
  2026-07-23 21:40   ` [PATCH v3 0/2] mv: report missing destination leading directory Lucas Zamboni Orioli via GitGitGadget
  2026-07-23 21:40     ` [PATCH v3 1/2] mv: name both source and destination when rename fails Lucas Zamboni Orioli via GitGitGadget
@ 2026-07-23 21:40     ` Lucas Zamboni Orioli via GitGitGadget
  2026-07-26 15:28       ` Pablo Sabater
  2026-07-26 20:17     ` [PATCH v4 0/2] mv: report missing destination leading directory Lucas Zamboni Orioli via GitGitGadget
  2 siblings, 1 reply; 21+ messages in thread
From: Lucas Zamboni Orioli via GitGitGadget @ 2026-07-23 21:40 UTC (permalink / raw)
  To: git; +Cc: Ben Knoble, Lucas Zamboni Orioli, Lucas Zamboni Orioli

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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/2] mv: check for missing destination directory before renaming
  2026-07-23 21:38       ` Lucas Zamboni Orioli
@ 2026-07-23 22:40         ` Junio C Hamano
  2026-07-23 23:28         ` Junio C Hamano
  1 sibling, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2026-07-23 22:40 UTC (permalink / raw)
  To: Lucas Zamboni Orioli
  Cc: Lucas Zamboni Orioli via GitGitGadget, git, Ben Knoble

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/2] mv: check for missing destination directory before renaming
  2026-07-23 21:38       ` Lucas Zamboni Orioli
  2026-07-23 22:40         ` Junio C Hamano
@ 2026-07-23 23:28         ` Junio C Hamano
  2026-07-26 14:59           ` Junio C Hamano
  1 sibling, 1 reply; 21+ messages in thread
From: Junio C Hamano @ 2026-07-23 23:28 UTC (permalink / raw)
  To: Lucas Zamboni Orioli
  Cc: Lucas Zamboni Orioli via GitGitGadget, git, Ben Knoble

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.

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/2] mv: check for missing destination directory before renaming
  2026-07-23 23:28         ` Junio C Hamano
@ 2026-07-26 14:59           ` Junio C Hamano
  2026-07-26 17:59             ` Lucas Zamboni Orioli
  0 siblings, 1 reply; 21+ messages in thread
From: Junio C Hamano @ 2026-07-26 14:59 UTC (permalink / raw)
  To: Lucas Zamboni Orioli
  Cc: Lucas Zamboni Orioli via GitGitGadget, git, Ben Knoble

Junio C Hamano <gitster@pobox.com> writes:

> 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.
>
> I doubt that using stat() instead of lstat() is the right approach.
> Doing so essentially amounts to ignoring the presence of symbolic
> links.

I actually think "outside the working tree" is an irrelevant red
herring.  What is relevant is the fact that Git tracks symbolic
links.

If you have x (file) and y (another file), you would want to
complain when the user says:

    $ git mv x y  

because the location y is "taken" and the command line tells us only
about what it wants to do to x, without saying anything about what
you want to do to that existing y.  If y were a symbolic link
instead, you should behave exactly the same way.

It actually takes even more care, and I do not know if the
implementation of git-mv is done carefully enough, but think about
what should happen to:

    $ git mv x a/b/c  

when 'a' is a tracked symbolic link, and it points at, say, '.'.
Should it behave exactly the same as:

    $ git mv x b/c  

or should it simply error out?  I think the latter, "I see a symlink
in the middle, so I refuse to follow," is the right behavior.

Think carefully about cases where 'a' is a directory and 'a/b' is a
symlink, or where 'a' and 'a/b' are directories and 'a/b/c' is a
symlink, and so on.  We do not want to craft an arbitrary rule that
says we allow or refuse to operate depending on the link target.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 2/2] mv: check for missing destination directory before renaming
  2026-07-23 21:40     ` [PATCH v3 2/2] mv: check for missing destination directory before renaming Lucas Zamboni Orioli via GitGitGadget
@ 2026-07-26 15:28       ` Pablo Sabater
  0 siblings, 0 replies; 21+ messages in thread
From: Pablo Sabater @ 2026-07-26 15:28 UTC (permalink / raw)
  To: Lucas Zamboni Orioli via GitGitGadget, git
  Cc: Ben Knoble, Lucas Zamboni Orioli

On Thu Jul 23, 2026 at 11:40 PM CEST, Lucas Zamboni Orioli via GitGitGadget wrote:
> 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))) {

nit: indentation.

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

nit: the if above has braces, this else if should too.

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


The rest looks good.

Regards,
Pablo


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/2] mv: check for missing destination directory before renaming
  2026-07-26 14:59           ` Junio C Hamano
@ 2026-07-26 17:59             ` Lucas Zamboni Orioli
  0 siblings, 0 replies; 21+ messages in thread
From: Lucas Zamboni Orioli @ 2026-07-26 17:59 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Lucas Zamboni Orioli via GitGitGadget, git, Ben Knoble

Em dom., 26 de jul. de 2026 às 11:59, Junio C Hamano
<gitster@pobox.com> escreveu:
> Think carefully about cases where 'a' is a directory and 'a/b' is a
> symlink, or where 'a' and 'a/b' are directories and 'a/b/c' is a
> symlink, and so on.  We do not want to craft an arbitrary rule that
> says we allow or refuse to operate depending on the link target.

Thanks for pushing on this, chasing the symlink case down turned up
more than a bad message. With a tracked symlink in the leading path,
"git mv" leaves the index inconsistent with the worktree:

    mkdir repo && cd repo
    git init
    echo content >a
    mkdir real-dir
    echo content >real-dir/b
    ln -s . c
    git add .
    git commit -m "initial"
    git mv a c/real-dir/a
    git status

'c' is a tracked symlink to '.'. The move follows it, so on disk the
file lands at the resolved path 'real-dir/a', but the index records
the literal 'c/real-dir/a'. "git status" then reports a staged rename
to 'c/real-dir/a', an unstaged deletion of that same path (nothing is
there on disk), and the real file untracked at 'real-dir/a', with the
symlink 'c' also shown untracked. A later "git add" did reconcile it
by finding the file at its real location, but "git mv" on its own has
already produced an index that describes a worktree that doesn't
exist, it got there precisely by traversing a tracked symlink.

So this is the "not careful enough" case you suspected, and the fix is
the behavior you described: refuse to operate when any component of the
destination's leading path is a symlink, independent of where it
points. I'm thinking of using has_symlink_leading_path() (symlinks.c) for
that check, which is what "git apply" already uses to avoid following in-tree
symlinks, so the behavior stays consistent with the rest of the tree.

For v3 I'll fold this into the series: the leading-directory check will
reject a missing directory or a non-directory/symlink component up
front, which covers both the original misleading-error case and this
symlink traversal. Tests will cover a symlink as the final component
and as an intermediate one ('a/b/c' with 'a' a symlink), plus the
existing missing-directory and dry-run cases.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v4 0/2] mv: report missing destination leading directory
  2026-07-23 21:40   ` [PATCH v3 0/2] mv: report missing destination leading directory Lucas Zamboni Orioli via GitGitGadget
  2026-07-23 21:40     ` [PATCH v3 1/2] mv: name both source and destination when rename fails Lucas Zamboni Orioli via GitGitGadget
  2026-07-23 21:40     ` [PATCH v3 2/2] mv: check for missing destination directory before renaming Lucas Zamboni Orioli via GitGitGadget
@ 2026-07-26 20:17     ` Lucas Zamboni Orioli via GitGitGadget
  2026-07-26 20:17       ` [PATCH v4 1/2] mv: name both source and destination when rename fails Lucas Zamboni Orioli via GitGitGadget
  2026-07-26 20:17       ` [PATCH v4 2/2] mv: reject a destination whose leading path is missing or a symlink Lucas Zamboni Orioli via GitGitGadget
  2 siblings, 2 replies; 21+ messages in thread
From: Lucas Zamboni Orioli via GitGitGadget @ 2026-07-26 20:17 UTC (permalink / raw)
  To: git; +Cc: Ben Knoble, Pablo Sabater, Junio C Hamano, Lucas Zamboni Orioli

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" actual

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v4 1/2] mv: name both source and destination when rename fails
  2026-07-26 20:17     ` [PATCH v4 0/2] mv: report missing destination leading directory Lucas Zamboni Orioli via GitGitGadget
@ 2026-07-26 20:17       ` Lucas Zamboni Orioli via GitGitGadget
  2026-07-26 20:17       ` [PATCH v4 2/2] mv: reject a destination whose leading path is missing or a symlink Lucas Zamboni Orioli via GitGitGadget
  1 sibling, 0 replies; 21+ messages in thread
From: Lucas Zamboni Orioli via GitGitGadget @ 2026-07-26 20:17 UTC (permalink / raw)
  To: git
  Cc: Ben Knoble, Pablo Sabater, Junio C Hamano, Lucas Zamboni Orioli,
	Lucas Zamboni Orioli

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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v4 2/2] mv: reject a destination whose leading path is missing or a symlink
  2026-07-26 20:17     ` [PATCH v4 0/2] mv: report missing destination leading directory Lucas Zamboni Orioli via GitGitGadget
  2026-07-26 20:17       ` [PATCH v4 1/2] mv: name both source and destination when rename fails Lucas Zamboni Orioli via GitGitGadget
@ 2026-07-26 20:17       ` Lucas Zamboni Orioli via GitGitGadget
  1 sibling, 0 replies; 21+ messages in thread
From: Lucas Zamboni Orioli via GitGitGadget @ 2026-07-26 20:17 UTC (permalink / raw)
  To: git
  Cc: Ben Knoble, Pablo Sabater, Junio C Hamano, Lucas Zamboni Orioli,
	Lucas Zamboni Orioli

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

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.

Three cases reach rename(2) unchecked today:

  - 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  | 35 ++++++++++++++++++++++
 t/t7001-mv.sh | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 118 insertions(+)

diff --git a/builtin/mv.c b/builtin/mv.c
index 35e504484a..535599e6be 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -22,6 +22,7 @@
 #include "string-list.h"
 #include "parse-options.h"
 #include "read-cache-ll.h"
+#include "symlinks.h"
 
 #include "setup.h"
 #include "strvec.h"
@@ -443,6 +444,40 @@ 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))) {
+			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) {
+					/*
+					 * 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)) &&
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 7cf4aa5ba1..7905d629d8 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -114,6 +114,89 @@ test_expect_success 'clean up' '
 	git reset --hard
 '
 
+test_expect_success 'moving to a non-existent directory' '
+	git reset --hard &&
+	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 leading path component' '
+	git reset --hard &&
+	rm -rf from && mkdir 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 beyond a symlink' '
+	git reset --hard &&
+	rm -rf from regular-dir link-to-dir &&
+	mkdir from regular-dir &&
+	echo contents >from/file &&
+	ln -s regular-dir link-to-dir &&
+	git add from/file &&
+	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 as an intermediate component' '
+	git reset --hard &&
+	rm -rf from && mkdir -p from/real/inner &&
+	echo contents >from/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 &&
+	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" actual
+'
+
 test_expect_success 'moving to existing untracked target with trailing slash' '
 	mkdir path1 &&
 	git mv path0/ path1/ &&
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2026-07-26 20:17 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-15 14:32 [PATCH] mv: report missing destination leading directory Lucas Zamboni Orioli via GitGitGadget
2026-07-15 16:46 ` Ben Knoble
2026-07-22 21:32   ` Lucas Zamboni Orioli
2026-07-23 13:13 ` [PATCH v2 0/2] " Lucas Zamboni Orioli via GitGitGadget
2026-07-23 13:13   ` [PATCH v2 1/2] mv: name both source and destination when rename fails Lucas Zamboni Orioli via GitGitGadget
2026-07-23 17:36     ` Junio C Hamano
2026-07-23 13:13   ` [PATCH v2 2/2] mv: check for missing destination directory before renaming Lucas Zamboni Orioli via GitGitGadget
2026-07-23 17:42     ` Junio C Hamano
2026-07-23 18:30     ` Junio C Hamano
2026-07-23 21:38       ` Lucas Zamboni Orioli
2026-07-23 22:40         ` Junio C Hamano
2026-07-23 23:28         ` Junio C Hamano
2026-07-26 14:59           ` Junio C Hamano
2026-07-26 17:59             ` Lucas Zamboni Orioli
2026-07-23 21:40   ` [PATCH v3 0/2] mv: report missing destination leading directory Lucas Zamboni Orioli via GitGitGadget
2026-07-23 21:40     ` [PATCH v3 1/2] mv: name both source and destination when rename fails Lucas Zamboni Orioli via GitGitGadget
2026-07-23 21:40     ` [PATCH v3 2/2] mv: check for missing destination directory before renaming Lucas Zamboni Orioli via GitGitGadget
2026-07-26 15:28       ` Pablo Sabater
2026-07-26 20:17     ` [PATCH v4 0/2] mv: report missing destination leading directory Lucas Zamboni Orioli via GitGitGadget
2026-07-26 20:17       ` [PATCH v4 1/2] mv: name both source and destination when rename fails Lucas Zamboni Orioli via GitGitGadget
2026-07-26 20:17       ` [PATCH v4 2/2] mv: reject a destination whose leading path is missing or a symlink Lucas Zamboni Orioli via GitGitGadget

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.