Re: [PATCH] fix `git mv existing-dir non-existing-dir`*

[Junio C Hamano <gitster@pobox.com>]

"Sebastian Thiel via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Sebastian Thiel <sebastian.thiel@icloud.com>
>
> *in some environments.

Please do not chop a single sentence in the middle and mark that
fact with an asterisk nobody understands what it means.

    Subject: [PATCH] mv: handle lstat() failure correctly

perhaps?

> When moving a directory onto another with `gix mv` various checks
> are performed. One of of these validates that the destination is
> not an existing file.
>
> When calling `lstat` on the destination path and it fails as the
> path doesn't exist, some environments seem to overwrite the passed
> in `stat` memory nonetheless.  (I observed this issue on debian 12
> of x86_64, running on OrbStack on ARM, emulated with Rosetta)

Very cleanly written, except "gix" -> "git".

POSIX does not seem to specify what should happen to buf when the
call fails, which I take to mean that its contents can become any
garbage at that point.

> diff --git a/builtin/mv.c b/builtin/mv.c
> index fa84fcb20d8..05e7156034e 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -184,7 +184,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  	int src_dir_nr = 0, src_dir_alloc = 0;
>  	struct strbuf a_src_dir = STRBUF_INIT;
>  	enum update_mode *modes, dst_mode = 0;
> -	struct stat st;
> +	struct stat st, dest_st;
>  	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
>  	struct lock_file lock_file = LOCK_INIT;
>  	struct cache_entry *ce;
> @@ -304,7 +304,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  			goto act_on_entry;
>  		}
>  		if (S_ISDIR(st.st_mode)
> -		    && lstat(dst, &st) == 0) {
> +		    && lstat(dst, &dest_st) == 0) {

This is good.  After this "if (S_ISDIR)" thing, there is another "if
(S_ISDIR)" on the same st.st_mode, so clobbering st like the
original was a stupid thing to do.

>  			bad = _("cannot move directory over file");

What is curious is that dest_st.st_mode, after lstat on dst
succeeds, is never checked, even though the error message claims
that it detected an attempt to move directory over file.  What
should happen when the user did this then?

    $ git mv existing-dir another-existing-dir

Shouldn't it do something similar to

    $ mv D1 D2

which is to move the entire hierarchy of D1 and make it appear at
D2/D1?

Even if the answer to the above question is "yes", that is a
separate bugfix, so let's not get distracted and see how our
test looks like.

>  			goto act_on_entry;
>  		}
> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
> index 898a9205328..9894bc45ee6 100755
> --- a/t/t7001-mv.sh
> +++ b/t/t7001-mv.sh
> @@ -174,6 +174,12 @@ test_expect_success 'do not move directory over existing directory' '
>  	test_must_fail git mv path2 path0
>  '
>  
> +test_expect_success 'rename directory to non-existing directory' '
> +	mkdir dir-a && touch dir-a/f &&

One command per line, and reserve the use of "touch" to cases where
you care about the timestamps, not existence.  I.e.

	mkdir dir-a &&
	>dir-a/f &&

> +	git add dir-a &&
> +	git mv dir-a non-existing-dir
> +'

OK, there is no guarantee that this would fail on a system whose
lstat() may clobber buf when it notices that the path does not
exist, but it is a good test to have.

Thanks.