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