Re: [PATCH] fix `git mv existing-dir non-existing-dir`*
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 8, 2023, 17:36 UTC
- Message-ID
- <xmqqy1il77wp.fsf@gitster.g>
- In-Reply-To
- <pull.1561.git.1691506431114.gitgitgadget@gmail.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?
Show 8 quoted lines
> 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.
Show 19 quoted lines
> 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.
Show 12 quoted lines
> 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.