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

Em seg., 27 de jul. de 2026 às 19:24, Junio C Hamano
<[email protected]> escreveu:

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