Re: [PATCH v4 2/2] mv: reject a destination whose leading path is missing or a symlink

From: Lucas Zamboni Orioli <lucaszam0@gmail.com>
Date: Jul 30, 2026, 11:23 UTC
Message-ID: <CAH01Q--Jeip3VvrYCOfM69ktvcR1gdeA6gVsQynd_xQ+cjsN8w@mail.gmail.com>
In-Reply-To: <xmqqbjbsgjfu.fsf@gitster.g>

Em seg., 27 de jul. de 2026 às 19:24, Junio C Hamano <gitster@pobox.com> escreveu:

Show 8 quoted lines

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

Yes, that's what I mean. Your wording is clearer, so I'll use it for the opening of the commit message.

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

Done. I added a helper:

+ static int needs_worktree_rename(enum update_mode mode,
+                                  enum update_mode dst_mode)
+ {
+         return !(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
+                !(dst_mode & (SKIP_WORKTREE_DIR | SPARSE));
+ }

I'm not attached to the name; happy to take a better one if the list has a preference.

> Are the elements of the destinations.v[] array normalized so that
> they are all full final pathnames?

Yes. When the destination is an existing directory, the setup phase builds the destinations with DUP_BASENAME against dst_w_slash, which appends the source's basename, so "git mv file dir" yields "dir/file" in destinations.v[] by the time this check runs. I added a test for that case which succeeds.

> What do we do to elements in destinations.v[] that lacks a slash?

A slash-less destination is a bare filename in the current directory ("git mv file_a file_b"), which has no leading directory to check, it lands in the cwd, which always exists, so skipping the check when there is no slash is correct. I added a test for that too, moving into a bare filename in the cwd, which succeeds.

> then you need to allocate only if you need a copy.  I do not know if
> it matters, though.

Applied, the xstrdup() now happens inside the "if (slash_)" arm, so a slash-less destination does not allocate.

Thanks for the review.