Re: [PATCH v4 2/2] mv: reject a destination whose leading path is missing or a symlink
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 27, 2026, 22:24 UTC
- Message-ID
- <xmqqbjbsgjfu.fsf@gitster.g>
- In-Reply-To
- <6b72efb4130d96947c7f90026042fa09a440d091.1785097071.git.gitgitgadget@gmail.com>
"Lucas Zamboni Orioli via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 5 quoted lines
> 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.
I cannot quite parse this. Do you mean to say something like this?
When moving a file, if any leading directory in the destination
path is missing or is not a real directory, the problem is detected
only later when rename() is called. Furthermore, if a leading
directory component is a symbolic link, the issue is not detected
at all.Show 5 quoted lines
> 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.
OK. With [PATCH 1/2] in place, this is an easy case for the user to deal with. Either the directory name was misspelled, or the user forgot to create intermediate levels of the destination directory.
> - 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.True. 'x' cannot become 'a/b' as long as 'a' is a file sitting there.
Show 7 quoted lines
> - 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.
Yeah, that is horrible.
Show 6 quoted lines
> 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.
Nice touch.
> 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.
Nice bugfix.
Show 20 quoted lines
> 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;
> + }With a proper helper, this part of the fix is surprisingly simple.
Show 6 quoted lines
> + /*
> + * 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))) {This small piece of logic is a duplicate of the next block that actually performs the move. I wonder if we can have a small helper function that takes mode and dst_mode as parameters and returns this value? Then this part would become:
if (that_function(modes[i], dst_mode)) {and the "real thing" would become
- if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
- !(dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) &&
+ if (that_function(mode, dst_mode) &&
rename(src, dst) < 0) {
if (ignore_errors)
continue;
die_errno(_("renaming '%s' failed"), src);
}and we will never risk them drifting apart. Naming is the tough part, though. I will leave it up to you and the list to come up with a good name that fits the semantics of what that function computes.
> + char *dst_dir = xstrdup(dst); > + char *slash = strrchr(dst_dir, '/');
Are the elements of the destinations.v[] array normalized so that they are all full final pathnames? I mean, 'mv A B' when B is an existing directory would succeed, remove A, and leave 'B/A' in the resulting working tree. If we can depend on the preprocessing code and the element in destinations.v[] corresponding to the move is 'B/A' (and presumably the corresponding element in the sources.v[] array would be 'A') in such a case, then stripping the final name component and checking whether the remainder (that is, the dirname) is a directory, as the code below does, sounds like the right approach.
Show 16 quoted lines
> + 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 you did this instead
const char *slash_ = strrchr(dst, '/');
if (stash_) {
char *dst_dir = xstrdup(dst);
char *slash = &dst_dir[slash_ - dst];then you need to allocate only if you need a copy. I do not know if it matters, though. What do we do to elements in destinations.v[] that lacks a slash?
> + if (bad) > + goto act_on_entry; > + }
Thanks.