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

[Ben Knoble <ben.knoble@gmail.com>]

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